From a8b4eaecc9dab6bd92ba839845068c52918cac2e Mon Sep 17 00:00:00 2001 From: maxtanh Date: Wed, 13 May 2026 21:05:05 -0700 Subject: [PATCH 1/3] 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 c67320185ca6002b9112efd168e7172e4f7e0ecd Mon Sep 17 00:00:00 2001 From: maxtanh Date: Sun, 17 May 2026 19:19:06 -0700 Subject: [PATCH 2/3] 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 3/3] 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")