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/job.go b/internal/orchestrator/job.go
index 7815ca0..c4e3440 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))
@@ -594,7 +597,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
@@ -970,8 +980,14 @@ 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 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)
b.WriteString("\n\n")
}
b.WriteString("## Validation\n\n")
@@ -1211,4 +1227,135 @@ 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
+// (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
+
+ 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))
+
+ // 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")
+ 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). 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
+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"]}
+`+"```\n"+`
+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",
+ 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. 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" {
+ 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)
+}
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/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go
index 553bb8a..62b4c2d 100644
--- a/internal/orchestrator/orchestrator_test.go
+++ b/internal/orchestrator/orchestrator_test.go
@@ -555,6 +555,47 @@ 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)
+ }
+
+ // 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) {
h := newTestHarness(t)
h.cfg.Approval.Mode = "handoff"
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