feat: implement auto-merge for approved PRs and update workflow documentation

2026-02-16 14:34:08 +08:00
parent f7aa47102f
commit 25ce06e14f
15 changed files with 208 additions and 163 deletions
--- a/lib/providers/github.ts
+++ b/lib/providers/github.ts
@@ -152,6 +152,15 @@ export class GitHubProvider implements IssueProvider {
    return { state: PrState.CLOSED, url: null };
  }

+  async mergePr(issueId: number): Promise<void> {
+    const pat = `#${issueId}`;
+    const raw = await this.gh(["pr", "list", "--state", "open", "--json", "number,title,body,url", "--limit", "20"]);
+    const prs = JSON.parse(raw) as Array<{ number: number; title: string; body: string; url: string }>;
+    const pr = prs.find((p) => p.title.includes(pat) || (p.body ?? "").includes(pat));
+    if (!pr) throw new Error(`No open PR found for issue #${issueId}`);
+    await this.gh(["pr", "merge", pr.url, "--merge"]);
+  }
+
  async addComment(issueId: number, body: string): Promise<void> {
    await this.gh(["issue", "comment", String(issueId), "--body", body]);
  }
--- a/lib/providers/gitlab.ts
+++ b/lib/providers/gitlab.ts
@@ -149,6 +149,15 @@ export class GitLabProvider implements IssueProvider {
    return { state: PrState.CLOSED, url: null };
  }

+  async mergePr(issueId: number): Promise<void> {
+    const pat = `#${issueId}`;
+    const raw = await this.glab(["mr", "list", "--output", "json", "--state", "opened"]);
+    const mrs = JSON.parse(raw) as Array<{ iid: number; title: string; description: string }>;
+    const mr = mrs.find((m) => m.title.includes(pat) || (m.description ?? "").includes(pat));
+    if (!mr) throw new Error(`No open MR found for issue #${issueId}`);
+    await this.glab(["mr", "merge", String(mr.iid)]);
+  }
+
  async addComment(issueId: number, body: string): Promise<void> {
    // Pass message directly as argv — no shell escaping needed with spawn
    await this.glab(["issue", "note", String(issueId), "--message", body]);
--- a/lib/providers/provider.ts
+++ b/lib/providers/provider.ts
@@ -61,6 +61,7 @@ export interface IssueProvider {
  hasMergedMR(issueId: number): Promise<boolean>;
  getMergedMRUrl(issueId: number): Promise<string | null>;
  getPrStatus(issueId: number): Promise<PrStatus>;
+  mergePr(issueId: number): Promise<void>;
  addComment(issueId: number, body: string): Promise<void>;
  healthCheck(): Promise<boolean>;
 }
--- a/lib/services/pipeline.e2e.test.ts
+++ b/lib/services/pipeline.e2e.test.ts
@@ -370,10 +370,10 @@ describe("E2E pipeline", () => {
      h = await createTestHarness();
    });

-    it("should transition In Review → To Test when PR is merged", async () => {
+    it("should auto-merge and transition In Review → To Test when PR is approved", async () => {
      // Seed issue in "In Review" state
      h.provider.seedIssue({ iid: 60, title: "Feature Y", labels: ["In Review"] });
-      h.provider.setPrStatus(60, { state: "merged", url: "https://example.com/pr/10" });
+      h.provider.setPrStatus(60, { state: "approved", url: "https://example.com/pr/10" });

      const transitions = await reviewPass({
        workspaceDir: h.workspaceDir,
@@ -390,6 +390,11 @@ describe("E2E pipeline", () => {
      assert.ok(issue.labels.includes("To Test"), `Labels: ${issue.labels}`);
      assert.ok(!issue.labels.includes("In Review"), "Should not have In Review");

+      // mergePr action should have been called
+      const mergeCalls = h.provider.callsTo("mergePr");
+      assert.strictEqual(mergeCalls.length, 1);
+      assert.strictEqual(mergeCalls[0].args.issueId, 60);
+
      // gitPull action should have been attempted
      const gitCmds = h.commands.commands.filter((c) => c.argv[0] === "git");
      assert.ok(gitCmds.length > 0, "Should have run git pull");
@@ -417,8 +422,8 @@ describe("E2E pipeline", () => {
    it("should handle multiple review issues in one pass", async () => {
      h.provider.seedIssue({ iid: 70, title: "PR A", labels: ["In Review"] });
      h.provider.seedIssue({ iid: 71, title: "PR B", labels: ["In Review"] });
-      h.provider.setPrStatus(70, { state: "merged", url: "https://example.com/pr/20" });
-      h.provider.setPrStatus(71, { state: "merged", url: "https://example.com/pr/21" });
+      h.provider.setPrStatus(70, { state: "approved", url: "https://example.com/pr/20" });
+      h.provider.setPrStatus(71, { state: "approved", url: "https://example.com/pr/21" });

      const transitions = await reviewPass({
        workspaceDir: h.workspaceDir,
@@ -434,6 +439,40 @@ describe("E2E pipeline", () => {
      const issue71 = await h.provider.getIssue(71);
      assert.ok(issue70.labels.includes("To Test"));
      assert.ok(issue71.labels.includes("To Test"));
+
+      // Both should have had mergePr called
+      const mergeCalls = h.provider.callsTo("mergePr");
+      assert.strictEqual(mergeCalls.length, 2);
+    });
+
+    it("should transition In Review → To Improve when merge fails (conflicts)", async () => {
+      h.provider.seedIssue({ iid: 65, title: "Conflicting PR", labels: ["In Review"] });
+      h.provider.setPrStatus(65, { state: "approved", url: "https://example.com/pr/15" });
+      h.provider.mergePrFailures.add(65);
+
+      const transitions = await reviewPass({
+        workspaceDir: h.workspaceDir,
+        groupId: h.groupId,
+        workflow: DEFAULT_WORKFLOW,
+        provider: h.provider,
+        repoPath: "/tmp/test-repo",
+      });
+
+      assert.strictEqual(transitions, 1);
+
+      // Issue should have moved to "To Improve" (not "To Test")
+      const issue = await h.provider.getIssue(65);
+      assert.ok(issue.labels.includes("To Improve"), `Labels: ${issue.labels}`);
+      assert.ok(!issue.labels.includes("In Review"), "Should not have In Review");
+      assert.ok(!issue.labels.includes("To Test"), "Should NOT have To Test");
+
+      // mergePr should have been attempted
+      const mergeCalls = h.provider.callsTo("mergePr");
+      assert.strictEqual(mergeCalls.length, 1);
+
+      // gitPull should NOT have run (aborted before git pull)
+      const gitCmds = h.commands.commands.filter((c) => c.argv[0] === "git");
+      assert.strictEqual(gitCmds.length, 0, "Should NOT have run git pull after merge failure");
    });
  });

@@ -551,8 +590,8 @@ describe("E2E pipeline", () => {
      let issue = await h.provider.getIssue(200);
      assert.ok(issue.labels.includes("In Review"), `After review: ${issue.labels}`);

-      // 4. PR gets merged — review pass picks it up
-      h.provider.setPrStatus(200, { state: "merged", url: "https://example.com/pr/50" });
+      // 4. PR gets approved — review pass picks it up and auto-merges
+      h.provider.setPrStatus(200, { state: "approved", url: "https://example.com/pr/50" });

      const transitions = await reviewPass({
        workspaceDir: h.workspaceDir,
--- a/lib/services/pipeline.ts
+++ b/lib/services/pipeline.ts
@@ -91,6 +91,11 @@ export async function executeCompletion(opts: {
          auditLog(workspaceDir, "pipeline_warning", { step: "detectPr", issue: issueId, role, error: (err as Error).message ?? String(err) }).catch(() => {});
        } }
        break;
+      case Action.MERGE_PR:
+        try { await provider.mergePr(issueId); } catch (err) {
+          auditLog(workspaceDir, "pipeline_warning", { step: "mergePr", issue: issueId, role, error: (err as Error).message ?? String(err) }).catch(() => {});
+        }
+        break;
    }
  }

--- a/lib/services/review.ts
+++ b/lib/services/review.ts
@@ -50,8 +50,10 @@ export async function reviewPass(opts: {

      if (!conditionMet) continue;

-      // Find the success transition (first non-BLOCKED event)
-      const successEvent = Object.keys(state.on).find((e) => e !== WorkflowEvent.BLOCKED);
+      // Find the success transition (first event that isn't BLOCKED or MERGE_FAILED)
+      const successEvent = Object.keys(state.on).find(
+        (e) => e !== WorkflowEvent.BLOCKED && e !== WorkflowEvent.MERGE_FAILED,
+      );
      if (!successEvent) continue;

      const transition = state.on[successEvent];
@@ -60,10 +62,41 @@ export async function reviewPass(opts: {
      const targetState = workflow.states[targetKey];
      if (!targetState) continue;

-      // Execute transition actions
+      // Execute transition actions — mergePr is critical (aborts on failure)
+      let aborted = false;
      if (actions) {
        for (const action of actions) {
          switch (action) {
+            case Action.MERGE_PR:
+              try {
+                await provider.mergePr(issue.iid);
+              } catch (err) {
+                // Merge failed → fire MERGE_FAILED transition (developer fixes conflicts)
+                await auditLog(workspaceDir, "review_merge_failed", {
+                  groupId,
+                  issueId: issue.iid,
+                  from: state.label,
+                  error: (err as Error).message ?? String(err),
+                });
+                const failedTransition = state.on[WorkflowEvent.MERGE_FAILED];
+                if (failedTransition) {
+                  const failedKey = typeof failedTransition === "string" ? failedTransition : failedTransition.target;
+                  const failedState = workflow.states[failedKey];
+                  if (failedState) {
+                    await provider.transitionLabel(issue.iid, state.label, failedState.label);
+                    await auditLog(workspaceDir, "review_transition", {
+                      groupId,
+                      issueId: issue.iid,
+                      from: state.label,
+                      to: failedState.label,
+                      reason: "merge_failed",
+                    });
+                    transitions++;
+                  }
+                }
+                aborted = true;
+              }
+              break;
            case Action.GIT_PULL:
              try { await runCommand(["git", "pull"], { timeoutMs: gitPullTimeoutMs, cwd: repoPath }); } catch { /* best-effort */ }
              break;
@@ -74,9 +107,12 @@ export async function reviewPass(opts: {
              await provider.reopenIssue(issue.iid);
              break;
          }
+          if (aborted) break;
        }
      }

+      if (aborted) continue; // skip normal transition, move to next issue
+
      // Transition label
      await provider.transitionLabel(issue.iid, state.label, targetState.label);

--- a/lib/templates.ts
+++ b/lib/templates.ts
@@ -2,6 +2,9 @@
 * Shared templates for workspace files.
 * Used by setup and project_register.
 */
+import YAML from "yaml";
+import { DEFAULT_WORKFLOW } from "./workflow.js";
+import { ROLE_REGISTRY } from "./roles/registry.js";

 export const DEFAULT_DEV_INSTRUCTIONS = `# DEVELOPER Worker Instructions

@@ -25,7 +28,7 @@ Read the comments carefully — they often contain clarifications, decisions, or
 - **IMPORTANT:** Do NOT use closing keywords in PR/MR descriptions (no "Closes #X", "Fixes #X", "Resolves #X"). Use "As described in issue #X" or "Addresses issue #X" instead. DevClaw manages issue state — auto-closing bypasses QA.
 - **Merge or request review:**
  - Merge the PR yourself → call work_finish with result "done"
-  - Leave the PR open for human review → call work_finish with result "review" (the heartbeat will auto-advance when the PR is merged)
+  - Leave the PR open for human review → call work_finish with result "review" (the heartbeat will auto-merge when approved and advance to testing)
 - Clean up the worktree after merging (if you merged)
 - If you discover unrelated bugs, call task_create to file them
 - Do NOT call work_start, status, health, or project_register
@@ -228,9 +231,9 @@ All orchestration goes through these tools. You do NOT manually manage sessions,
 \`\`\`
 Planning → To Do → Doing → To Test → Testing → Done
                     ↓          ↑
-                  In Review ─────┘ (auto-advances when PR merged)
+                  In Review ─────┘ (auto-merges when PR approved)
                     ↓
-                 To Improve → Doing (fix cycle)
+                 To Improve → Doing (merge conflict / fix cycle)
                     ↓
                 Refining (human decision)

@@ -262,7 +265,7 @@ All roles (Developer, Tester, Architect) use the same level scheme. Levels descr
 Workers call \`work_finish\` themselves — the label transition, state update, and audit log happen atomically. The heartbeat service will pick up the next task on its next cycle:

 - Developer "done" → issue moves to "To Test" → scheduler dispatches Tester
- Developer "review" → issue moves to "In Review" → heartbeat polls PR status → auto-advances to "To Test" when merged
+- Developer "review" → issue moves to "In Review" → heartbeat polls PR status → auto-merges and advances to "To Test" when approved (merge conflicts → "To Improve" for developer to fix)
 - Tester "fail" → issue moves to "To Improve" → scheduler dispatches Developer
 - Tester "pass" → Done, no further dispatch
 - Tester "refine" / blocked → needs human input
@@ -292,121 +295,19 @@ export const HEARTBEAT_MD_TEMPLATE = `# HEARTBEAT.md
 Do nothing. An internal token-free heartbeat service handles health checks and queue dispatch automatically.
 `;

-export const WORKFLOW_YAML_TEMPLATE = `# DevClaw workflow configuration
-# Modify values to customize. Copy to devclaw/projects/<project>/workflow.yaml for project-specific overrides.
+/**
+ * Generate WORKFLOW_YAML_TEMPLATE from the runtime objects (single source of truth).
+ */
+function buildWorkflowYaml(): string {
+  const roles: Record<string, { models: Record<string, string> }> = {};
+  for (const [id, config] of Object.entries(ROLE_REGISTRY)) {
+    roles[id] = { models: { ...config.models } };
+  }

-roles:
-  developer:
-    models:
-      junior: anthropic/claude-haiku-4-5
-      medior: anthropic/claude-sonnet-4-5
-      senior: anthropic/claude-opus-4-6
-  tester:
-    models:
-      junior: anthropic/claude-haiku-4-5
-      medior: anthropic/claude-sonnet-4-5
-      senior: anthropic/claude-opus-4-6
-  architect:
-    models:
-      junior: anthropic/claude-sonnet-4-5
-      senior: anthropic/claude-opus-4-6
-  # Disable a role entirely:
-  # architect: false
+  const header =
+    "# DevClaw workflow configuration\n" +
+    "# Modify values to customize. Copy to devclaw/projects/<project>/workflow.yaml for project-specific overrides.\n\n";
+  return header + YAML.stringify({ roles, workflow: DEFAULT_WORKFLOW });
+}

-workflow:
-  initial: planning
-  states:
-    planning:
-      type: hold
-      label: Planning
-      color: "#95a5a6"
-      on:
-        APPROVE: todo
-    todo:
-      type: queue
-      role: developer
-      label: To Do
-      color: "#428bca"
-      priority: 1
-      on:
-        PICKUP: doing
-    doing:
-      type: active
-      role: developer
-      label: Doing
-      color: "#f0ad4e"
-      on:
-        COMPLETE:
-          target: toTest
-          actions: [gitPull, detectPr]
-        REVIEW:
-          target: reviewing
-          actions: [detectPr]
-        BLOCKED: refining
-    toTest:
-      type: queue
-      role: tester
-      label: To Test
-      color: "#5bc0de"
-      priority: 2
-      on:
-        PICKUP: testing
-    testing:
-      type: active
-      role: tester
-      label: Testing
-      color: "#9b59b6"
-      on:
-        PASS:
-          target: done
-          actions: [closeIssue]
-        FAIL:
-          target: toImprove
-          actions: [reopenIssue]
-        REFINE: refining
-        BLOCKED: refining
-    toImprove:
-      type: queue
-      role: developer
-      label: To Improve
-      color: "#d9534f"
-      priority: 3
-      on:
-        PICKUP: doing
-    refining:
-      type: hold
-      label: Refining
-      color: "#f39c12"
-      on:
-        APPROVE: todo
-    reviewing:
-      type: review
-      label: In Review
-      color: "#c5def5"
-      check: prMerged
-      on:
-        APPROVED:
-          target: toTest
-          actions: [gitPull]
-        BLOCKED: refining
-    done:
-      type: terminal
-      label: Done
-      color: "#5cb85c"
-    toDesign:
-      type: queue
-      role: architect
-      label: To Design
-      color: "#0075ca"
-      priority: 1
-      on:
-        PICKUP: designing
-    designing:
-      type: active
-      role: architect
-      label: Designing
-      color: "#d4c5f9"
-      on:
-        COMPLETE: planning
-        BLOCKED: refining
-`;
+export const WORKFLOW_YAML_TEMPLATE = buildWorkflowYaml();
--- a/lib/testing/test-provider.ts
+++ b/lib/testing/test-provider.ts
@@ -21,16 +21,28 @@ import { DEFAULT_WORKFLOW, type WorkflowConfig } from "../workflow.js";
 export type ProviderCall =
  | { method: "ensureLabel"; args: { name: string; color: string } }
  | { method: "ensureAllStateLabels"; args: {} }
-  | { method: "createIssue"; args: { title: string; description: string; label: StateLabel; assignees?: string[] } }
+  | {
+      method: "createIssue";
+      args: {
+        title: string;
+        description: string;
+        label: StateLabel;
+        assignees?: string[];
+      };
+    }
  | { method: "listIssuesByLabel"; args: { label: StateLabel } }
  | { method: "getIssue"; args: { issueId: number } }
  | { method: "listComments"; args: { issueId: number } }
-  | { method: "transitionLabel"; args: { issueId: number; from: StateLabel; to: StateLabel } }
+  | {
+      method: "transitionLabel";
+      args: { issueId: number; from: StateLabel; to: StateLabel };
+    }
  | { method: "closeIssue"; args: { issueId: number } }
  | { method: "reopenIssue"; args: { issueId: number } }
  | { method: "hasMergedMR"; args: { issueId: number } }
  | { method: "getMergedMRUrl"; args: { issueId: number } }
  | { method: "getPrStatus"; args: { issueId: number } }
+  | { method: "mergePr"; args: { issueId: number } }
  | { method: "addComment"; args: { issueId: number; body: string } }
  | { method: "healthCheck"; args: {} };

@@ -49,6 +61,8 @@ export class TestProvider implements IssueProvider {
  prStatuses = new Map<number, PrStatus>();
  /** Merged MR URLs per issue. */
  mergedMrUrls = new Map<number, string>();
+  /** Issue IDs where mergePr should fail (simulates merge conflicts). */
+  mergePrFailures = new Set<number>();
  /** All calls, in order. */
  calls: ProviderCall[] = [];

@@ -71,7 +85,8 @@ export class TestProvider implements IssueProvider {
      description: overrides.description ?? "",
      labels: overrides.labels ?? [],
      state: overrides.state ?? "opened",
-      web_url: overrides.web_url ?? `https://example.com/issues/${overrides.iid}`,
+      web_url:
+        overrides.web_url ?? `https://example.com/issues/${overrides.iid}`,
    };
    this.issues.set(issue.iid, issue);
    if (issue.iid >= this.nextIssueId) this.nextIssueId = issue.iid + 1;
@@ -102,6 +117,7 @@ export class TestProvider implements IssueProvider {
    this.labels.clear();
    this.prStatuses.clear();
    this.mergedMrUrls.clear();
+    this.mergePrFailures.clear();
    this.calls = [];
    this.nextIssueId = 1;
  }
@@ -129,7 +145,10 @@ export class TestProvider implements IssueProvider {
    label: StateLabel,
    assignees?: string[],
  ): Promise<Issue> {
-    this.calls.push({ method: "createIssue", args: { title, description, label, assignees } });
+    this.calls.push({
+      method: "createIssue",
+      args: { title, description, label, assignees },
+    });
    const iid = this.nextIssueId++;
    const issue: Issue = {
      iid,
@@ -210,10 +229,26 @@ export class TestProvider implements IssueProvider {
    return this.prStatuses.get(issueId) ?? { state: "closed", url: null };
  }

+  async mergePr(issueId: number): Promise<void> {
+    this.calls.push({ method: "mergePr", args: { issueId } });
+    if (this.mergePrFailures.has(issueId)) {
+      throw new Error(`Merge conflict: cannot merge PR for issue #${issueId}`);
+    }
+    // Simulate successful merge — update PR status to merged
+    const existing = this.prStatuses.get(issueId);
+    if (existing) {
+      this.prStatuses.set(issueId, { state: "merged", url: existing.url });
+    }
+  }
+
  async addComment(issueId: number, body: string): Promise<void> {
    this.calls.push({ method: "addComment", args: { issueId, body } });
    const existing = this.comments.get(issueId) ?? [];
-    existing.push({ author: "test", body, created_at: new Date().toISOString() });
+    existing.push({
+      author: "test",
+      body,
+      created_at: new Date().toISOString(),
+    });
    this.comments.set(issueId, existing);
  }

--- a/lib/workflow.ts
+++ b/lib/workflow.ts
@@ -39,6 +39,7 @@ export type TransitionAction = string;
 export const Action = {
  GIT_PULL: "gitPull",
  DETECT_PR: "detectPr",
+  MERGE_PR: "mergePr",
  CLOSE_ISSUE: "closeIssue",
  REOPEN_ISSUE: "reopenIssue",
 } as const;
@@ -56,6 +57,7 @@ export const WorkflowEvent = {
  COMPLETE: "COMPLETE",
  REVIEW: "REVIEW",
  APPROVED: "APPROVED",
+  MERGE_FAILED: "MERGE_FAILED",
  PASS: "PASS",
  FAIL: "FAIL",
  REFINE: "REFINE",
@@ -98,6 +100,7 @@ export type CompletionRule = {
 export const DEFAULT_WORKFLOW: WorkflowConfig = {
  initial: "planning",
  states: {
+    // ── Main pipeline (happy path) ──────────────────────────────
    planning: {
      type: StateType.HOLD,
      label: "Planning",
@@ -123,6 +126,17 @@ export const DEFAULT_WORKFLOW: WorkflowConfig = {
        [WorkflowEvent.BLOCKED]: "refining",
      },
    },
+    reviewing: {
+      type: StateType.REVIEW,
+      label: "In Review",
+      color: "#c5def5",
+      check: ReviewCheck.PR_APPROVED,
+      on: {
+        [WorkflowEvent.APPROVED]: { target: "toTest", actions: [Action.MERGE_PR, Action.GIT_PULL] },
+        [WorkflowEvent.MERGE_FAILED]: "toImprove",
+        [WorkflowEvent.BLOCKED]: "refining",
+      },
+    },
    toTest: {
      type: StateType.QUEUE,
      role: "tester",
@@ -143,6 +157,13 @@ export const DEFAULT_WORKFLOW: WorkflowConfig = {
        [WorkflowEvent.BLOCKED]: "refining",
      },
    },
+    done: {
+      type: StateType.TERMINAL,
+      label: "Done",
+      color: "#5cb85c",
+    },
+
+    // ── Side paths (loops back into main pipeline) ──────────────
    toImprove: {
      type: StateType.QUEUE,
      role: "developer",
@@ -157,21 +178,8 @@ export const DEFAULT_WORKFLOW: WorkflowConfig = {
      color: "#f39c12",
      on: { [WorkflowEvent.APPROVE]: "todo" },
    },
-    reviewing: {
-      type: StateType.REVIEW,
-      label: "In Review",
-      color: "#c5def5",
-      check: ReviewCheck.PR_MERGED,
-      on: {
-        [WorkflowEvent.APPROVED]: { target: "toTest", actions: [Action.GIT_PULL] },
-        [WorkflowEvent.BLOCKED]: "refining",
-      },
-    },
-    done: {
-      type: StateType.TERMINAL,
-      label: "Done",
-      color: "#5cb85c",
-    },
+
+    // ── Architect track ─────────────────────────────────────────
    toDesign: {
      type: StateType.QUEUE,
      role: "architect",