From e713e30fe7d8eb237eb03c11acbbfd52efabc0be Mon Sep 17 00:00:00 2001 From: maxtanh Date: Wed, 13 May 2026 20:43:48 -0700 Subject: [PATCH 1/7] feat: post issue comment when reviewer approves a plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the reviewer only posted a comment on rejection. This makes the approval path equally visible — operators and issue authors can see why the reviewer approved and that the gate was evaluated, without needing to check orchestrator logs. --- internal/orchestrator/job.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 7815ca0..47b2ade 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -377,6 +377,9 @@ func (o *Orchestrator) routeAuto(ctx context.Context, job *types.Job, issue type log.Info("reviewer_completed", "decision", dec.Decision) if dec.Decision == "approve" { + reasons := strings.Join(dec.Reasons, "; ") + approveBody := fmt.Sprintf("[symphony-go] ✅ reviewer approved the plan: %s", reasons) + _, _ = o.deps.GitHub.PostIssueComment(ctx, issue.Number, approveBody) job.ApprovalPath = types.ApprovalPathReviewer _ = o.saveJob(job) layout := workspace.LayoutFor(o.deps.WorkspaceRoot, issue.Number, workspace.SanitizeSlug(issue.Title)) From a8b4eaecc9dab6bd92ba839845068c52918cac2e Mon Sep 17 00:00:00 2001 From: maxtanh Date: Wed, 13 May 2026 21:05:05 -0700 Subject: [PATCH 2/7] feat: add post-PR code review using the reviewer agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a PR is created, run the configured reviewer agent on the actual code diff (not just the plan). The reviewer evaluates the implementation for bugs, style issues, missing error handling, test coverage gaps, and security concerns, then posts its findings as a comment on the PR. This is a best-effort step — if the reviewer fails or is not configured, the PR is still created and labeled normally. The code review is informational and does not block the PR. The reviewer reuses the existing reviewer infrastructure (same provider, same timeout config) but with a code-review-specific prompt that includes the full diff against the base branch. --- internal/orchestrator/job.go | 90 ++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 7815ca0..434eb68 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -587,6 +587,9 @@ func (o *Orchestrator) runImplementation(ctx context.Context, job *types.Job, is job.PRNumber = pr.Number log.Info("pr_created", "number", pr.Number, "url", pr.URL) + // 17a. Run code review on the diff (best-effort, non-blocking). + o.runPostPRCodeReview(ctx, job, issue, pr.Number) + // 17. PR-link comment + label. _, _ = o.deps.GitHub.PostIssueComment(ctx, issue.Number, fmt.Sprintf("[symphony-go] opened PR #%d: %s", pr.Number, pr.URL)) if err := o.deps.GitHub.ReplaceStateLabel(ctx, issue.Number, @@ -1212,3 +1215,90 @@ func (o *Orchestrator) resolveValidationCommands(job *types.Job) ([]string, erro } var _ = errors.New // silence unused import if ever + +// runPostPRCodeReview runs the reviewer agent on the actual code diff +// (not just the plan) and posts the review as a PR comment. This is a +// best-effort step — failure does not block the PR from being created. +func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, job *types.Job, issue types.Issue, prNumber int) { + log := o.deps.Logger.With("issue", issue.Number, "pr", prNumber) + cfg := o.deps.Config + + if o.deps.Reviewer == nil { + log.Debug("code_review_skipped", "reason", "no reviewer configured") + return + } + + layout := workspace.LayoutFor(o.deps.WorkspaceRoot, issue.Number, workspace.SanitizeSlug(issue.Title)) + + // Get the diff against base branch. + diffCmd := exec.CommandContext(ctx, "git", "-C", layout.RepoPath, "diff", cfg.Repo.BaseBranch+"..HEAD") + diffOut, err := diffCmd.Output() + if err != nil { + log.Warn("code_review_diff_failed", "err", err) + return + } + + if len(diffOut) == 0 { + log.Debug("code_review_skipped", "reason", "empty diff") + return + } + + // Truncate large diffs to stay within model context limits. + diffStr := string(diffOut) + if len(diffStr) > 30000 { + diffStr = diffStr[:30000] + "\n... (truncated)" + } + + // Build a code review prompt (fixed, not user-controlled). + prompt := fmt.Sprintf(`You are a code review agent. Review this diff for a GitHub pull request. + +Issue: #%d %s +Description: %s + +Diff: +%s + +Review the code for: +1. Bugs or logic errors +2. Missing error handling +3. Style inconsistencies with surrounding code +4. Test coverage gaps +5. Security concerns + +If the code looks good, say so briefly. If there are issues, list them clearly. + +End your response with: +## Decision +`+"```json\n"+`{"decision": "approve", "reasons": ["summary of review"]} +`+"```\n"+` +Set decision to "approve" if the code is acceptable, or "reject" if changes are needed. +`, issue.Number, issue.Title, issue.Description, diffStr) + + reviewerHome := filepath.Join(o.deps.WorkspaceRoot, ".symphony-go", "review-homes", + fmt.Sprintf("issue-%d-code-review", issue.Number)) + _ = os.MkdirAll(reviewerHome, 0o755) + + log.Info("code_review_started") + result, rerr := o.deps.Reviewer.Review(ctx, approval.ReviewInput{ + Issue: issue, + PlanText: prompt, + RepoPath: layout.RepoPath, + HomePath: reviewerHome, + }) + + if rerr != nil { + log.Warn("code_review_failed", "err", rerr) + return + } + + // Post review as a comment on the PR. + reasons := strings.Join(result.Reasons, "\n") + var body string + if result.Decision == "approve" { + body = fmt.Sprintf("[symphony-go] 🔍 **Code Review:** ✅ LGTM\n\n%s", reasons) + } else { + body = fmt.Sprintf("[symphony-go] 🔍 **Code Review:** ⚠️ Changes Requested\n\n%s", reasons) + } + _, _ = o.deps.GitHub.PostIssueComment(ctx, prNumber, body) + log.Info("code_review_completed", "decision", result.Decision) +} From 647d3fb1cea8f6095bffe0689e2f33f1f013fcc7 Mon Sep 17 00:00:00 2001 From: maxtanh Date: Thu, 14 May 2026 17:36:49 -0700 Subject: [PATCH 3/7] fix(orchestrator): deduplicate '## Plan' heading in PR body buildPRBody unconditionally prepended '## Plan' before job.PlanText, but the planning agent often includes its own '## Plan' heading in the markdown output. This resulted in duplicate headings in PR descriptions. Now checks if PlanText already starts with '## Plan' before adding one. --- internal/orchestrator/job.go | 9 ++++++-- internal/orchestrator/orchestrator_test.go | 26 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 7815ca0..931e6b5 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -970,8 +970,13 @@ func buildPRBody(job *types.Job, results []valResult, workflowEdited bool, proof fmt.Fprintf(&b, "Resolves #%d.\n\n", job.IssueNumber) fmt.Fprintf(&b, "Approval path: `%s`\n\n", job.ApprovalPath) if job.PlanText != "" { - b.WriteString("## Plan\n\n") - b.WriteString(job.PlanText) + planText := strings.TrimSpace(job.PlanText) + // The agent may already include a "## Plan" heading in its output. + // Only add our own if the text doesn't already start with one. + if !strings.HasPrefix(planText, "## Plan") { + b.WriteString("## Plan\n\n") + } + b.WriteString(planText) b.WriteString("\n\n") } b.WriteString("## Validation\n\n") diff --git a/internal/orchestrator/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go index 553bb8a..d385a98 100644 --- a/internal/orchestrator/orchestrator_test.go +++ b/internal/orchestrator/orchestrator_test.go @@ -555,6 +555,32 @@ func TestBuildPRBodyIncludesProofArtifacts(t *testing.T) { } } +func TestBuildPRBodyNoDuplicatePlanHeading(t *testing.T) { + // When the agent includes "## Plan" in its plan text, buildPRBody should + // not add a second "## Plan" heading. + job := &types.Job{ + IssueNumber: 42, + ApprovalPath: types.ApprovalPathReviewer, + PlanText: "## Plan\n\n### Steps\n- do thing one\n- do thing two", + } + body := buildPRBody(job, nil, false, nil) + count := strings.Count(body, "## Plan") + if count != 1 { + t.Fatalf("expected exactly 1 '## Plan' heading, got %d:\n%s", count, body) + } + + // When the agent does NOT include a heading, buildPRBody should add one. + job2 := &types.Job{ + IssueNumber: 43, + ApprovalPath: types.ApprovalPathReviewer, + PlanText: "### Steps\n- do thing one\n- do thing two", + } + body2 := buildPRBody(job2, nil, false, nil) + if !strings.Contains(body2, "## Plan\n\n### Steps") { + t.Fatalf("expected '## Plan' heading to be added:\n%s", body2) + } +} + func TestEmptyPlanDoesNotPostBlankComment(t *testing.T) { h := newTestHarness(t) h.cfg.Approval.Mode = "handoff" From c67320185ca6002b9112efd168e7172e4f7e0ecd Mon Sep 17 00:00:00 2001 From: maxtanh Date: Sun, 17 May 2026 19:19:06 -0700 Subject: [PATCH 4/7] fix(orchestrator): address code review feedback on post-PR review --- internal/orchestrator/job.go | 39 ++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 434eb68..04489dd 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -587,9 +587,6 @@ func (o *Orchestrator) runImplementation(ctx context.Context, job *types.Job, is job.PRNumber = pr.Number log.Info("pr_created", "number", pr.Number, "url", pr.URL) - // 17a. Run code review on the diff (best-effort, non-blocking). - o.runPostPRCodeReview(ctx, job, issue, pr.Number) - // 17. PR-link comment + label. _, _ = o.deps.GitHub.PostIssueComment(ctx, issue.Number, fmt.Sprintf("[symphony-go] opened PR #%d: %s", pr.Number, pr.URL)) if err := o.deps.GitHub.ReplaceStateLabel(ctx, issue.Number, @@ -597,7 +594,14 @@ func (o *Orchestrator) runImplementation(ctx context.Context, job *types.Job, is return err } job.Status = types.StatusPRReady - return o.saveJob(job) + if err := o.saveJob(job); err != nil { + return err + } + + // 18. Supplementary post-PR code review (best-effort, purely + // informational — never mutates job state, even on "reject"). + o.runPostPRCodeReview(ctx, issue, pr.Number) + return nil } // runImplementationAgent drives the implementation phase. When multi-turn @@ -1217,9 +1221,13 @@ func (o *Orchestrator) resolveValidationCommands(job *types.Job) ([]string, erro var _ = errors.New // silence unused import if ever // runPostPRCodeReview runs the reviewer agent on the actual code diff -// (not just the plan) and posts the review as a PR comment. This is a -// best-effort step — failure does not block the PR from being created. -func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, job *types.Job, issue types.Issue, prNumber int) { +// (not just the plan) and posts the review as a PR comment. Purely +// informational: it never mutates job state and never triggers a +// revision. A "reject" decision is surfaced as a "Changes Requested" +// comment for human follow-up; the orchestration state machine is +// unaffected. The `job` is intentionally not passed in so the function +// physically cannot mutate it. +func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, issue types.Issue, prNumber int) { log := o.deps.Logger.With("issue", issue.Number, "pr", prNumber) cfg := o.deps.Config @@ -1230,8 +1238,9 @@ func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, job *types.Job, layout := workspace.LayoutFor(o.deps.WorkspaceRoot, issue.Number, workspace.SanitizeSlug(issue.Title)) - // Get the diff against base branch. - diffCmd := exec.CommandContext(ctx, "git", "-C", layout.RepoPath, "diff", cfg.Repo.BaseBranch+"..HEAD") + // Get the diff against base branch (three-dot, origin-prefixed — matches + // gitDiffNameOnly and the rest of the codebase). + diffCmd := exec.CommandContext(ctx, "git", "-C", layout.RepoPath, "diff", "origin/"+cfg.Repo.BaseBranch+"...HEAD") diffOut, err := diffCmd.Output() if err != nil { log.Warn("code_review_diff_failed", "err", err) @@ -1249,7 +1258,9 @@ func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, job *types.Job, diffStr = diffStr[:30000] + "\n... (truncated)" } - // Build a code review prompt (fixed, not user-controlled). + // Build a code review prompt (fixed, not user-controlled). The trailing + // `## Decision` + fenced JSON block matches approval.ParseDecision so + // the result can be parsed via the standard reviewer flow. prompt := fmt.Sprintf(`You are a code review agent. Review this diff for a GitHub pull request. Issue: #%d %s @@ -1268,10 +1279,11 @@ Review the code for: If the code looks good, say so briefly. If there are issues, list them clearly. End your response with: + ## Decision -`+"```json\n"+`{"decision": "approve", "reasons": ["summary of review"]} +`+"```json\n"+`{"decision": "approve", "reasons": ["summary"]} `+"```\n"+` -Set decision to "approve" if the code is acceptable, or "reject" if changes are needed. +Set decision to "approve" if code is acceptable, "reject" if changes needed. `, issue.Number, issue.Title, issue.Description, diffStr) reviewerHome := filepath.Join(o.deps.WorkspaceRoot, ".symphony-go", "review-homes", @@ -1291,7 +1303,8 @@ Set decision to "approve" if the code is acceptable, or "reject" if changes are return } - // Post review as a comment on the PR. + // Post review as a comment on the PR. Even a "reject" decision is + // purely informational here — no triggerRevision, no state changes. reasons := strings.Join(result.Reasons, "\n") var body string if result.Decision == "approve" { From 8c8477fa653ff53ced6f397074438a5644e26270 Mon Sep 17 00:00:00 2001 From: maxtanh Date: Sun, 17 May 2026 20:44:55 -0700 Subject: [PATCH 5/7] fix(code-review): fetch origin/ before computing diff Without this fetch, when multiple jobs run sequentially and earlier PRs get merged between runs, origin/ becomes stale. The three-dot diff (origin/main...HEAD) then includes commits from previously-merged PRs, causing the reviewer to flag unrelated changes. Observed on XF-APAC/one-minute-games PR #50: the code review saw README changes from PR #49 (issue #38) because origin/main hadn't been updated since that PR was merged. --- internal/orchestrator/job.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 04489dd..b6238d3 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -1238,6 +1238,16 @@ func (o *Orchestrator) runPostPRCodeReview(ctx context.Context, issue types.Issu layout := workspace.LayoutFor(o.deps.WorkspaceRoot, issue.Number, workspace.SanitizeSlug(issue.Title)) + // Fetch origin/ so the ref is current. Without this, when + // multiple jobs run sequentially and earlier PRs get merged between + // runs, origin/ can be stale, causing the diff to include + // commits from previously-merged PRs. + fetchCmd := exec.CommandContext(ctx, "git", "-C", layout.RepoPath, "fetch", "origin", cfg.Repo.BaseBranch) + if fetchErr := fetchCmd.Run(); fetchErr != nil { + log.Warn("code_review_fetch_failed", "err", fetchErr) + // Continue anyway — the diff may still be correct if origin/ is recent. + } + // Get the diff against base branch (three-dot, origin-prefixed — matches // gitDiffNameOnly and the rest of the codebase). diffCmd := exec.CommandContext(ctx, "git", "-C", layout.RepoPath, "diff", "origin/"+cfg.Repo.BaseBranch+"...HEAD") From 3b9547c61269aaef561ab6784f484d18b3813001 Mon Sep 17 00:00:00 2001 From: maxtanh Date: Sun, 17 May 2026 21:25:27 -0700 Subject: [PATCH 6/7] feat(orchestrator): add single-cycle PR revision on CHANGES_REQUESTED review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a human submits a CHANGES_REQUESTED review on a symphony-go PR, the orchestrator now detects it via polling and runs a single revision cycle on the existing branch/worktree: 1. PollPRRevisions (new tick step) scans pr_ready jobs for fresh CHANGES_REQUESTED reviews submitted after the job's UpdatedAt. 2. runPRRevision drives the implementation agent with a revision prompt containing the original issue, approved plan, and review feedback. 3. The agent's changes are committed and pushed to the same branch, updating the PR in place (no close/reopen cycle). 4. Job.RevisionAttempted is set to true — only one revision cycle runs per PR, subsequent reviews are left for human follow-up. Changes: - internal/github: add PRReview type, ListPRReviews to Client interface - internal/github/fake: add SeedPRReview + ListPRReviews - internal/types: add RevisionAttempted field to Job - internal/orchestrator/loop: call PollPRRevisions in tick - internal/orchestrator/revision.go (new): polling + execution logic - internal/orchestrator/revision_test.go (new): 3 test cases --- internal/github/fake.go | 56 ++++-- internal/github/github.go | 51 +++++ internal/orchestrator/loop.go | 3 + internal/orchestrator/revision.go | 202 ++++++++++++++++++++ internal/orchestrator/revision_test.go | 251 +++++++++++++++++++++++++ internal/types/types.go | 9 +- 6 files changed, 559 insertions(+), 13 deletions(-) create mode 100644 internal/orchestrator/revision.go create mode 100644 internal/orchestrator/revision_test.go diff --git a/internal/github/fake.go b/internal/github/fake.go index 92c6b70..aee7d21 100644 --- a/internal/github/fake.go +++ b/internal/github/fake.go @@ -37,10 +37,12 @@ type InMemoryFake struct { permissions map[string]string prs []fakePR reactions map[int64][]string + prReviews map[int][]PRReview - // next monotonic IDs for created comments / PRs. - nextCommentID int64 - nextPRNumber int + // next monotonic IDs for created comments / PRs / reviews. + nextCommentID int64 + nextPRNumber int + nextPRReviewID int64 } // NewInMemoryFake constructs an empty InMemoryFake bound to the given @@ -52,14 +54,16 @@ func NewInMemoryFake(fullName string) *InMemoryFake { panic(err) } return &InMemoryFake{ - owner: owner, - repo: repo, - issues: make(map[int]*fakeIssue), - comments: make(map[int][]types.IssueComment), - permissions: make(map[string]string), - reactions: make(map[int64][]string), - nextCommentID: 1, - nextPRNumber: 1000, + owner: owner, + repo: repo, + issues: make(map[int]*fakeIssue), + comments: make(map[int][]types.IssueComment), + permissions: make(map[string]string), + reactions: make(map[int64][]string), + prReviews: make(map[int][]PRReview), + nextCommentID: 1, + nextPRNumber: 1000, + nextPRReviewID: 1, } } @@ -284,6 +288,36 @@ func (f *InMemoryFake) AddReaction(_ context.Context, commentID int64, reaction return nil } +// SeedPRReview appends a review to a PR. If r.ID is zero, a new ID is +// assigned. If r.SubmittedAt is zero, time.Now() is used. +func (f *InMemoryFake) SeedPRReview(prNumber int, r PRReview) PRReview { + f.mu.Lock() + defer f.mu.Unlock() + if r.ID == 0 { + r.ID = f.nextPRReviewID + f.nextPRReviewID++ + } else if r.ID >= f.nextPRReviewID { + f.nextPRReviewID = r.ID + 1 + } + if r.SubmittedAt.IsZero() { + r.SubmittedAt = time.Now().UTC() + } + f.prReviews[prNumber] = append(f.prReviews[prNumber], r) + return r +} + +// ListPRReviews implements Client. Returned reviews are sorted by +// SubmittedAt ascending (oldest first). +func (f *InMemoryFake) ListPRReviews(_ context.Context, prNumber int) ([]PRReview, error) { + f.mu.Lock() + defer f.mu.Unlock() + rs := f.prReviews[prNumber] + out := make([]PRReview, len(rs)) + copy(out, rs) + sort.Slice(out, func(i, j int) bool { return out[i].SubmittedAt.Before(out[j].SubmittedAt) }) + return out, nil +} + // containsLabel does a case-insensitive lookup in a label slice. The // slice is expected to already be lowercase. func containsLabel(labels []string, want string) bool { diff --git a/internal/github/github.go b/internal/github/github.go index 9b9cf33..9ff9cf1 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -33,6 +33,17 @@ type PullRequest struct { State string } +// PRReview is a normalized PR review returned by Client.ListPRReviews. +// State is one of "APPROVED", "CHANGES_REQUESTED", "COMMENTED", "DISMISSED", +// or "PENDING" (mirroring GitHub's review states). +type PRReview struct { + ID int64 + User string + State string + Body string + SubmittedAt time.Time +} + // Client is the minimal GitHub surface used by symphony-go. type Client interface { // ListReadyIssues returns open issues carrying readyLabel. Pull @@ -61,6 +72,8 @@ type Client interface { // FindPRsByHead lists open PRs whose head branch matches the given // branch name (in the repo's owner namespace). FindPRsByHead(ctx context.Context, headBranch string) ([]PullRequest, error) + // ListPRReviews returns every review submitted on a PR, oldest first. + ListPRReviews(ctx context.Context, prNumber int) ([]PRReview, error) // AddReaction adds a reaction (e.g. "+1", "-1", "eyes") to an issue // comment. AddReaction(ctx context.Context, commentID int64, reaction string) error @@ -311,6 +324,28 @@ func (r *realClient) FindPRsByHead(ctx context.Context, headBranch string) ([]Pu return out, nil } +func (r *realClient) ListPRReviews(ctx context.Context, prNumber int) ([]PRReview, error) { + opts := &gh.ListOptions{PerPage: 100} + var out []PRReview + for { + reviews, resp, err := r.c.PullRequests.ListReviews(ctx, r.owner, r.repo, prNumber, opts) + if err != nil { + return nil, fmt.Errorf("github: list PR reviews on #%d: %w", prNumber, err) + } + for _, rv := range reviews { + if rv == nil { + continue + } + out = append(out, normalizePRReview(rv)) + } + if resp == nil || resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return out, nil +} + func (r *realClient) AddReaction(ctx context.Context, commentID int64, reaction string) error { if _, _, err := r.c.Reactions.CreateIssueCommentReaction(ctx, r.owner, r.repo, commentID, reaction); err != nil { return fmt.Errorf("github: add reaction %q to comment %d: %w", reaction, commentID, err) @@ -367,3 +402,19 @@ func normalizePR(p *gh.PullRequest) PullRequest { State: p.GetState(), } } + +// normalizePRReview converts a go-github *PullRequestReview into PRReview. +func normalizePRReview(rv *gh.PullRequestReview) PRReview { + out := PRReview{ + ID: rv.GetID(), + Body: rv.GetBody(), + State: rv.GetState(), + } + if rv.User != nil && rv.User.Login != nil { + out.User = *rv.User.Login + } + if rv.SubmittedAt != nil { + out.SubmittedAt = rv.SubmittedAt.Time + } + return out +} diff --git a/internal/orchestrator/loop.go b/internal/orchestrator/loop.go index f49e1e7..fcddfa2 100644 --- a/internal/orchestrator/loop.go +++ b/internal/orchestrator/loop.go @@ -34,6 +34,9 @@ func (o *Orchestrator) Run(ctx context.Context, once bool) error { if err := o.PollApprovals(ctx); err != nil { o.deps.Logger.Error("tick: PollApprovals", "err", err) } + if err := o.PollPRRevisions(ctx); err != nil { + o.deps.Logger.Error("tick: PollPRRevisions", "err", err) + } max := o.deps.Config.Orchestrator.MaxConcurrentJobs if max < 1 { max = 1 diff --git a/internal/orchestrator/revision.go b/internal/orchestrator/revision.go new file mode 100644 index 0000000..ead4e13 --- /dev/null +++ b/internal/orchestrator/revision.go @@ -0,0 +1,202 @@ +package orchestrator + +import ( + "bytes" + "context" + "fmt" + "os" + "os/exec" + "strings" + "time" + + "github.com/logosc/symphony-go/internal/config" + "github.com/logosc/symphony-go/internal/github" + "github.com/logosc/symphony-go/internal/types" +) + +// revisionSuffixTmpl is appended to the rendered WORKFLOW.md prompt when +// the orchestrator drives a single revision cycle in response to a +// CHANGES_REQUESTED PR review. Format args (in order): issue title, +// issue description, approved plan text, review feedback body. +const revisionSuffixTmpl = ` + +--- + +You are in REVISION phase. A reviewer has requested changes on your pull request. + +Original issue: +Title: %s +Description: %s + +Your approved plan: +%s + +Review feedback requesting changes: +%s + +Address the reviewer's feedback by making the necessary code changes. Focus only on what the reviewer asked for. +` + +// PollPRRevisions services pr_ready jobs whose PRs have received a +// CHANGES_REQUESTED review since the job was finalized. Each job is +// revised at most once; Job.RevisionAttempted guards a second pass. The +// job stays in pr_ready regardless of revision outcome. +func (o *Orchestrator) PollPRRevisions(ctx context.Context) error { + jobs, err := o.deps.State.List() + if err != nil { + return err + } + for _, job := range jobs { + if job.Status != types.StatusPRReady { + continue + } + if job.RevisionAttempted { + continue + } + if job.PRNumber == 0 { + continue + } + if err := o.servicePRRevision(ctx, job); err != nil { + o.deps.Logger.Error("revision: service", "issue", job.IssueNumber, "err", err) + } + } + return nil +} + +// servicePRRevision looks for a CHANGES_REQUESTED review on job.PRNumber +// submitted after job.UpdatedAt, and if one is found, runs a single +// revision cycle under a claim. +func (o *Orchestrator) servicePRRevision(ctx context.Context, job *types.Job) error { + reviews, err := o.deps.GitHub.ListPRReviews(ctx, job.PRNumber) + if err != nil { + return err + } + since := job.UpdatedAt + var match *github.PRReview + for i := range reviews { + r := reviews[i] + if !strings.EqualFold(r.State, "CHANGES_REQUESTED") { + continue + } + if !since.IsZero() && !r.SubmittedAt.After(since) { + continue + } + if o.isIgnoredApprovalUser(r.User) { + continue + } + match = &r + break + } + if match == nil { + return nil + } + if !o.tryClaim(job.IssueNumber) { + return nil + } + defer o.release(job.IssueNumber) + return o.runPRRevision(ctx, job, *match) +} + +// runPRRevision executes the revision cycle: render a revision prompt, +// drive the implementation agent on the existing worktree, commit any +// uncommitted changes, push the branch (which updates the existing PR in +// place), and post a status comment on the PR. Always sets +// RevisionAttempted=true and re-saves the job so the cycle never repeats. +// The job's status (pr_ready) and label are intentionally left alone. +func (o *Orchestrator) runPRRevision(ctx context.Context, job *types.Job, review github.PRReview) error { + cfg := o.deps.Config + log := o.deps.Logger.With("issue", job.IssueNumber, "pr", job.PRNumber) + + finish := func(comment string) error { + if comment != "" { + _, _ = o.deps.GitHub.PostIssueComment(ctx, job.PRNumber, comment) + } + job.RevisionAttempted = true + return o.saveJob(job) + } + + issue, err := o.deps.GitHub.GetIssue(ctx, job.IssueNumber) + if err != nil { + return finish(fmt.Sprintf("[symphony-go] revision: get issue failed: %v", err)) + } + + layout := o.layoutForJob(job) + if _, statErr := os.Stat(layout.RepoPath); statErr != nil { + log.Warn("revision_worktree_missing", "path", layout.RepoPath, "err", statErr) + return finish("[symphony-go] revision skipped: worktree no longer on disk.") + } + + rendered, rerr := config.RenderPrompt(o.promptBodyFor(job.AxisKey), issue, job.Attempt) + if rerr != nil { + return finish(fmt.Sprintf("[symphony-go] revision: render prompt failed: %v", rerr)) + } + revPrompt := rendered + fmt.Sprintf(revisionSuffixTmpl, + issue.Title, issue.Description, job.PlanText, review.Body) + + headBefore, _ := gitRevParseHead(ctx, layout.RepoPath) + + log.Info("revision_started", "reviewer", review.User, "review_id", review.ID) + revReq := types.RunRequest{ + Issue: issue, + RepoPath: layout.RepoPath, + HomePath: layout.HomePath, + Prompt: revPrompt, + Phase: types.PhaseImplementation, + Timeout: time.Duration(cfg.Agent.TimeoutSeconds) * time.Second, + AxisKey: job.AxisKey, + } + revResult, turns, runErr := o.runImplementationAgent(ctx, log, job, revReq) + log.Info("revision_completed", "success", revResult.Success, "turns", turns, "err", runErr) + + if runErr != nil { + return finish(fmt.Sprintf("[symphony-go] revision agent error; PR unchanged.\n\n%s", + truncate(summarizeRunFailure(revResult), 1500))) + } + if !revResult.Success { + return finish(fmt.Sprintf("[symphony-go] revision agent reported failure; PR unchanged.\n\n%s", + truncate(summarizeRunFailure(revResult), 1500))) + } + + statusOut, sErr := gitStatusPorcelain(ctx, layout.RepoPath) + if sErr != nil { + return finish(fmt.Sprintf("[symphony-go] revision: git status failed: %v", sErr)) + } + hasUncommitted := strings.TrimSpace(statusOut) != "" + headAfter, _ := gitRevParseHead(ctx, layout.RepoPath) + agentCommitted := headBefore != "" && headAfter != "" && headBefore != headAfter + if !hasUncommitted && !agentCommitted { + return finish("[symphony-go] revision agent produced no diff; PR unchanged.") + } + + if hasUncommitted { + msg := fmt.Sprintf("Address review feedback on PR #%d", job.PRNumber) + if err := gitCommitAll(ctx, layout.RepoPath, msg); err != nil { + return finish(fmt.Sprintf("[symphony-go] revision: commit failed: %v", err)) + } + } + + pushToken, tokErr := o.resolveGitHubToken(ctx) + if tokErr != nil { + return finish(fmt.Sprintf("[symphony-go] revision: resolve token failed: %v", tokErr)) + } + if err := o.deps.PushFunc(ctx, layout.RepoPath, job.Branch, pushToken); err != nil { + return finish(fmt.Sprintf("[symphony-go] revision: push failed: %v", err)) + } + + log.Info("revision_pushed") + return finish("[symphony-go] pushed revision addressing review feedback") +} + +// gitRevParseHead returns the current HEAD SHA at repoPath. Returns the +// empty string and the underlying error on failure (callers can treat an +// empty result as "unknown" for diff-detection purposes). +func gitRevParseHead(ctx context.Context, repoPath string) (string, error) { + cmd := exec.CommandContext(ctx, "git", "-C", repoPath, "rev-parse", "HEAD") + var out, errb bytes.Buffer + cmd.Stdout = &out + cmd.Stderr = &errb + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("%w: %s", err, errb.String()) + } + return strings.TrimSpace(out.String()), nil +} diff --git a/internal/orchestrator/revision_test.go b/internal/orchestrator/revision_test.go new file mode 100644 index 0000000..b8c6c91 --- /dev/null +++ b/internal/orchestrator/revision_test.go @@ -0,0 +1,251 @@ +package orchestrator + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + gh "github.com/logosc/symphony-go/internal/github" + "github.com/logosc/symphony-go/internal/types" +) + +// installPRReadyJob seeds an issue at the pr-ready label and persists a +// matching local job carrying a non-zero PRNumber. The PR number is also +// seeded as a fake "issue" so PostIssueComment(ctx, prNumber, ...) works +// (real GitHub treats PRs as issues for comment purposes; the fake needs +// an explicit seed). +// +// updatedAt sets Job.UpdatedAt; pass time.Time{} to leave it zero (which +// disables the staleness filter on reviews). Returns the seeded job. +func installPRReadyJob(t *testing.T, h *testHarness, issueNum, prNumber int, updatedAt time.Time) *types.Job { + t.Helper() + iss := types.Issue{ + Number: issueNum, Title: "feature", Description: "context", State: "open", + Labels: []string{h.cfg.Labels.PRReady}, + } + h.gh.SeedIssue(iss, false) + // Seed the PR number as an issue too so issue-comments API calls + // targeting the PR number succeed in the fake. + h.gh.SeedIssue(types.Issue{ + Number: prNumber, Title: "[agent] feature", State: "open", + }, true) + job := &types.Job{ + IssueNumber: issueNum, + Repo: h.cfg.Repo.FullName, + Status: types.StatusPRReady, + WorkspaceRoot: t.TempDir(), + RepoPath: h.repo, + Branch: fmt.Sprintf("symphony/issue-%d-feature", issueNum), + PRNumber: prNumber, + PlanText: canonicalPlan([]string{"a.txt"}), + UpdatedAt: updatedAt, + } + if err := h.state.Save(job); err != nil { + t.Fatalf("save: %v", err) + } + return job +} + +// TestPollPRRevisions_HappyPath runs the full pipeline to pr-ready, then +// posts a CHANGES_REQUESTED review and asserts the revision cycle runs: +// the agent is invoked with the revision prompt, the diff is committed +// and pushed, RevisionAttempted is set, and a confirmation comment is +// posted on the PR. +func TestPollPRRevisions_HappyPath(t *testing.T) { + h := newTestHarness(t) + h.cfg.Approval.Mode = "handoff" + o := h.newOrch(t, "x", false) + + h.runner.Responses[types.PhasePlanning] = types.RunResult{ + Success: true, Text: canonicalPlan([]string{"a.txt"}), + } + implWriter(h.runner, []string{"a.txt"}) + + iss := h.seedReadyIssue(501, "fix things") + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + if err := o.ProcessIssue(ctx, iss); err != nil { + t.Fatalf("ProcessIssue: %v", err) + } + job, err := h.state.Load(501) + if err != nil { + t.Fatalf("load: %v", err) + } + if job.Status != types.StatusPRReady { + t.Fatalf("expected pr_ready before revision, got %q", job.Status) + } + prNumber := job.PRNumber + if prNumber == 0 { + t.Fatalf("expected non-zero PRNumber after ProcessIssue") + } + // Seed the PR number as an issue in the fake so PostIssueComment on + // the PR succeeds (the orchestrator targets the PR for revision + // comments, mirroring real GitHub where PRs are issues). + h.gh.SeedIssue(types.Issue{ + Number: prNumber, Title: "[agent] fix things", State: "open", + }, true) + + // Swap OnRun to a revision-aware writer. We assert on req.Prompt to + // confirm the orchestrator wired in the issue title, plan, and review + // body. The writer drops a fresh file so we can detect the agent's + // effect on the worktree. + var capturedPrompt string + h.runner.OnRun = func(ctx context.Context, req types.RunRequest) (types.RunResult, error) { + capturedPrompt = req.Prompt + if err := os.WriteFile(filepath.Join(req.RepoPath, "fix.txt"), + []byte("revision change\n"), 0o644); err != nil { + return types.RunResult{}, err + } + return types.RunResult{Success: true, Text: "done"}, nil + } + // Reset call log so we can assert "exactly one runner call during + // revision" without counting the prior planning + impl invocations. + h.runner.Reset() + + // Give the review a SubmittedAt that's unambiguously after job.UpdatedAt + // on coarse-clock systems. + time.Sleep(5 * time.Millisecond) + h.gh.SeedPRReview(prNumber, gh.PRReview{ + User: "alice", + State: "CHANGES_REQUESTED", + Body: "Please rename the helper to follow our naming convention.", + SubmittedAt: time.Now().UTC(), + }) + + if err := o.PollPRRevisions(ctx); err != nil { + t.Fatalf("PollPRRevisions: %v", err) + } + + // Job: RevisionAttempted=true, status unchanged. + got, err := h.state.Load(501) + if err != nil { + t.Fatalf("load after revision: %v", err) + } + if !got.RevisionAttempted { + t.Errorf("expected RevisionAttempted=true") + } + if got.Status != types.StatusPRReady { + t.Errorf("status = %q, want pr_ready", got.Status) + } + + // Runner: exactly one revision call. + calls := h.runner.Calls() + if len(calls) != 1 { + t.Fatalf("runner calls during revision = %d, want 1: %+v", len(calls), calls) + } + if calls[0].Phase != types.PhaseImplementation { + t.Errorf("revision call phase = %q, want implementation", calls[0].Phase) + } + + // Prompt: contains review body, issue title, and plan text. + if !strings.Contains(capturedPrompt, "Please rename the helper") { + t.Errorf("revision prompt missing review body; got:\n%s", capturedPrompt) + } + if !strings.Contains(capturedPrompt, "fix things") { + t.Errorf("revision prompt missing issue title; got:\n%s", capturedPrompt) + } + if !strings.Contains(capturedPrompt, "files_touched") { + t.Errorf("revision prompt missing plan text; got:\n%s", capturedPrompt) + } + if !strings.Contains(capturedPrompt, "REVISION phase") { + t.Errorf("revision prompt missing REVISION marker; got:\n%s", capturedPrompt) + } + + // Worktree: agent's file was committed (no uncommitted changes). + if _, statErr := os.Stat(filepath.Join(got.RepoPath, "fix.txt")); statErr != nil { + t.Errorf("fix.txt not present in worktree: %v", statErr) + } + statusOut, _ := gitStatusPorcelain(ctx, got.RepoPath) + if strings.TrimSpace(statusOut) != "" { + t.Errorf("expected clean worktree after revision, got:\n%s", statusOut) + } + + // Comment: confirmation posted on the PR. + comments, err := h.gh.ListIssueComments(ctx, prNumber, time.Time{}) + if err != nil { + t.Fatalf("ListIssueComments(pr): %v", err) + } + if !containsBody(comments, "pushed revision addressing review feedback") { + t.Errorf("expected confirmation comment on PR #%d; got %+v", prNumber, comments) + } +} + +// TestPollPRRevisions_RevisionAttemptedSkips verifies that a job with +// RevisionAttempted=true is skipped even when a fresh CHANGES_REQUESTED +// review is present. +func TestPollPRRevisions_RevisionAttemptedSkips(t *testing.T) { + h := newTestHarness(t) + o := h.newOrch(t, "x", false) + + job := installPRReadyJob(t, h, 502, 1502, time.Now().UTC()) + job.RevisionAttempted = true + if err := h.state.Save(job); err != nil { + t.Fatalf("save: %v", err) + } + + time.Sleep(5 * time.Millisecond) + h.gh.SeedPRReview(1502, gh.PRReview{ + User: "alice", State: "CHANGES_REQUESTED", + Body: "more changes", SubmittedAt: time.Now().UTC(), + }) + + if err := o.PollPRRevisions(context.Background()); err != nil { + t.Fatalf("PollPRRevisions: %v", err) + } + + if calls := h.runner.Calls(); len(calls) != 0 { + t.Errorf("expected runner not called (RevisionAttempted skip), got %d calls", len(calls)) + } + got, _ := h.state.Load(502) + if !got.RevisionAttempted { + t.Errorf("RevisionAttempted should remain true") + } + // No revision comment should have been posted on the PR. + comments, _ := h.gh.ListIssueComments(context.Background(), 1502, time.Time{}) + if containsBody(comments, "pushed revision") || containsBody(comments, "revision agent") { + t.Errorf("unexpected revision comment posted: %+v", comments) + } +} + +// TestPollPRRevisions_StaleReviewIgnored verifies that a review whose +// SubmittedAt is before the job's UpdatedAt is treated as stale and the +// revision cycle is not triggered. +func TestPollPRRevisions_StaleReviewIgnored(t *testing.T) { + h := newTestHarness(t) + o := h.newOrch(t, "x", false) + + jobUpdatedAt := time.Now().UTC() + installPRReadyJob(t, h, 503, 1503, jobUpdatedAt) + + // Review submitted one hour BEFORE the job last advanced. + h.gh.SeedPRReview(1503, gh.PRReview{ + User: "alice", State: "CHANGES_REQUESTED", + Body: "old feedback", SubmittedAt: jobUpdatedAt.Add(-1 * time.Hour), + }) + + if err := o.PollPRRevisions(context.Background()); err != nil { + t.Fatalf("PollPRRevisions: %v", err) + } + + if calls := h.runner.Calls(); len(calls) != 0 { + t.Errorf("expected runner not called for stale review, got %d calls", len(calls)) + } + got, _ := h.state.Load(503) + if got.RevisionAttempted { + t.Errorf("RevisionAttempted should remain false for stale-only reviews") + } +} + +// containsBody reports whether any comment's body contains substr. +func containsBody(comments []types.IssueComment, substr string) bool { + for _, c := range comments { + if strings.Contains(c.Body, substr) { + return true + } + } + return false +} diff --git a/internal/types/types.go b/internal/types/types.go index c916928..d04aba6 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -108,8 +108,13 @@ type Job struct { ApprovalToken string `json:"approval_token,omitempty"` ReviewerDecision *ReviewerDecision `json:"reviewer_decision,omitempty"` PRNumber int `json:"pr_number,omitempty"` - Attempt int `json:"attempt"` - UpdatedAt time.Time `json:"updated_at"` + // RevisionAttempted is set to true once a single PR-revision cycle has + // run on this job (triggered by a CHANGES_REQUESTED review). The + // orchestrator only revises a PR once — subsequent CHANGES_REQUESTED + // reviews are ignored. + RevisionAttempted bool `json:"revision_attempted,omitempty"` + Attempt int `json:"attempt"` + UpdatedAt time.Time `json:"updated_at"` // AxisKey is the per-axis label this job was frozen against at claim // time. "default" when no per-axis map is configured or no concrete // label matched. Empty on jobs persisted before the per-axis feature From 43e5270ffac38ce3dc3aab421d578a04c2491ca0 Mon Sep 17 00:00:00 2001 From: maxtanh Date: Sun, 17 May 2026 23:07:50 -0700 Subject: [PATCH 7/7] fix(orchestrator): detect '# Plan' heading to prevent duplicate in PR body The agent sometimes outputs '# Plan' (H1) instead of '## Plan' (H2). The previous fix (PR #3) only checked for '## Plan' prefix, so an H1 heading still caused buildPRBody to prepend its own '## Plan', resulting in duplicate headings like: ## Plan # Plan ## Issue Context... Replace the simple HasPrefix check with a hasPlanHeading() helper that detects plan headings at any markdown level (#, ##, ###, etc.). Fixes: https://github.com/XF-APAC/one-minute-games/pull/54 --- internal/orchestrator/job.go | 32 ++++++++++++++++++++-- internal/orchestrator/orchestrator_test.go | 15 ++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/internal/orchestrator/job.go b/internal/orchestrator/job.go index 1dfb4ab..c4e3440 100644 --- a/internal/orchestrator/job.go +++ b/internal/orchestrator/job.go @@ -981,9 +981,10 @@ func buildPRBody(job *types.Job, results []valResult, workflowEdited bool, proof fmt.Fprintf(&b, "Approval path: `%s`\n\n", job.ApprovalPath) if job.PlanText != "" { planText := strings.TrimSpace(job.PlanText) - // The agent may already include a "## Plan" heading in its output. - // Only add our own if the text doesn't already start with one. - if !strings.HasPrefix(planText, "## Plan") { + // The agent may already include a plan heading at any markdown + // level (# Plan, ## Plan, ### Plan). Only add our own if the + // text doesn't already start with one. + if !hasPlanHeading(planText) { b.WriteString("## Plan\n\n") } b.WriteString(planText) @@ -1226,6 +1227,31 @@ func (o *Orchestrator) resolveValidationCommands(job *types.Job) ([]string, erro return nil, fmt.Errorf("validation.commands_by_label has no entry for %q and no default", axis) } +// hasPlanHeading reports whether text starts with a markdown heading +// containing "Plan" at any level (# Plan, ## Plan, ### Plan, etc.). +// This prevents buildPRBody from prepending a duplicate heading when the +// agent already emits one. +func hasPlanHeading(text string) bool { + firstLine := text + if i := strings.IndexByte(text, '\n'); i >= 0 { + firstLine = text[:i] + } + firstLine = strings.TrimSpace(firstLine) + // Must start with at least one '#'. + if !strings.HasPrefix(firstLine, "#") { + return false + } + // Strip leading '#' characters and require a space after them. + stripped := strings.TrimLeft(firstLine, "#") + if len(stripped) == 0 || stripped[0] != ' ' { + return false + } + // Check that the heading word is "Plan" — either exactly or followed + // by a space (e.g. "## Plan: details"). Rejects "Planning", "Planet". + lower := strings.ToLower(strings.TrimSpace(stripped)) + return lower == "plan" || strings.HasPrefix(lower, "plan ") +} + var _ = errors.New // silence unused import if ever // runPostPRCodeReview runs the reviewer agent on the actual code diff diff --git a/internal/orchestrator/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go index d385a98..62b4c2d 100644 --- a/internal/orchestrator/orchestrator_test.go +++ b/internal/orchestrator/orchestrator_test.go @@ -579,6 +579,21 @@ func TestBuildPRBodyNoDuplicatePlanHeading(t *testing.T) { if !strings.Contains(body2, "## Plan\n\n### Steps") { t.Fatalf("expected '## Plan' heading to be added:\n%s", body2) } + + // When the agent uses "# Plan" (H1), buildPRBody should NOT add a + // duplicate heading. + job3 := &types.Job{ + IssueNumber: 44, + ApprovalPath: types.ApprovalPathReviewer, + PlanText: "# Plan\n\n## Issue Context\n- details", + } + body3 := buildPRBody(job3, nil, false, nil) + if strings.Contains(body3, "## Plan\n\n# Plan") { + t.Fatalf("duplicate heading when agent uses '# Plan':\n%s", body3) + } + if !strings.Contains(body3, "# Plan\n") { + t.Fatalf("expected agent's '# Plan' heading preserved:\n%s", body3) + } } func TestEmptyPlanDoesNotPostBlankComment(t *testing.T) {