From 2ddb7901414e878c47ee768b2b621c839f0dec49 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 25 Mar 2026 00:34:14 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20auto=5Fpr:=20true=20now=20actually=20cre?= =?UTF-8?q?ates=20PRs=20=E2=80=94=20fix=203=20interacting=20bugs=20(#2302)?= =?UTF-8?q?=20(#2433)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs prevented auto_pr from ever creating a PR: 1. auto_pr was gated on `pushed` flag which requires auto_push to also be true. Changed condition to `!nothingToCommit` so auto_pr works independently. 2. phases.ts called createDraftPR AFTER mergeAndExit (when we're back on main and the milestone branch may not exist on remote). Removed duplicate PR creation from phases.ts — it's already handled inside mergeMilestoneToMain. 3. createDraftPR in git-service.ts lacked --head and --base parameters, so gh would create a PR from whatever branch was current. Added optional opts parameter with head/base support. Fixes #2302 Co-authored-by: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/auto-worktree.ts | 8 +- src/resources/extensions/gsd/auto/phases.ts | 54 +----------- src/resources/extensions/gsd/git-service.ts | 8 +- .../extensions/gsd/tests/auto-pr-bugs.test.ts | 88 +++++++++++++++++++ 4 files changed, 101 insertions(+), 57 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/auto-pr-bugs.test.ts diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index cfd4a241e..784d11276 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -1320,9 +1320,9 @@ export function mergeMilestoneToMain( } } - // 9b. Auto-create PR if enabled (requires push_branches + push succeeded) + // 9b. Auto-create PR if enabled (#2302: no longer gated on pushed/auto_push) let prCreated = false; - if (prefs.auto_pr === true && pushed) { + if (prefs.auto_pr === true && !nothingToCommit) { const remote = prefs.remote ?? "origin"; const prTarget = prefs.pr_target_branch ?? mainBranch; try { @@ -1332,9 +1332,9 @@ export function mergeMilestoneToMain( stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8", }); - // Create PR via gh CLI + // Create PR via gh CLI with explicit --head and --base (#2302) execFileSync("gh", [ - "pr", "create", + "pr", "create", "--draft", "--base", prTarget, "--head", milestoneBranch, "--title", `Milestone ${milestoneId} complete`, diff --git a/src/resources/extensions/gsd/auto/phases.ts b/src/resources/extensions/gsd/auto/phases.ts index cac6ad545..945c4e1a0 100644 --- a/src/resources/extensions/gsd/auto/phases.ts +++ b/src/resources/extensions/gsd/auto/phases.ts @@ -235,23 +235,7 @@ export async function runPreDispatch( // Worktree lifecycle on milestone transition — merge current, enter next deps.resolver.mergeAndExit(s.currentMilestoneId!, ctx.ui); - // Opt-in: create draft PR on milestone completion - if (prefs?.git?.auto_pr) { - try { - const { createDraftPR } = await import("../git-service.js"); - const prUrl = createDraftPR( - s.basePath, - s.currentMilestoneId!, - `[GSD] ${s.currentMilestoneId} complete`, - `Milestone ${s.currentMilestoneId} completed by GSD auto-mode.\n\nSee .gsd/${s.currentMilestoneId}/ for details.`, - ); - if (prUrl) { - ctx.ui.notify(`Draft PR created: ${prUrl}`, "info"); - } - } catch { - // Non-fatal — PR creation is best-effort - } - } + // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) deps.invalidateAllCaches(); @@ -324,23 +308,7 @@ export async function runPreDispatch( if (s.currentMilestoneId) { deps.resolver.mergeAndExit(s.currentMilestoneId, ctx.ui); - // Opt-in: create draft PR on milestone completion - if (prefs?.git?.auto_pr) { - try { - const { createDraftPR } = await import("../git-service.js"); - const prUrl = createDraftPR( - s.basePath, - s.currentMilestoneId, - `[GSD] ${s.currentMilestoneId} complete`, - `Milestone ${s.currentMilestoneId} completed by GSD auto-mode.\n\nSee .gsd/${s.currentMilestoneId}/ for details.`, - ); - if (prUrl) { - ctx.ui.notify(`Draft PR created: ${prUrl}`, "info"); - } - } catch { - // Non-fatal — PR creation is best-effort - } - } + // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) } deps.sendDesktopNotification( "GSD", @@ -424,23 +392,7 @@ export async function runPreDispatch( if (s.currentMilestoneId) { deps.resolver.mergeAndExit(s.currentMilestoneId, ctx.ui); - // Opt-in: create draft PR on milestone completion - if (prefs?.git?.auto_pr) { - try { - const { createDraftPR } = await import("../git-service.js"); - const prUrl = createDraftPR( - s.basePath, - s.currentMilestoneId, - `[GSD] ${s.currentMilestoneId} complete`, - `Milestone ${s.currentMilestoneId} completed by GSD auto-mode.\n\nSee .gsd/${s.currentMilestoneId}/ for details.`, - ); - if (prUrl) { - ctx.ui.notify(`Draft PR created: ${prUrl}`, "info"); - } - } catch { - // Non-fatal — PR creation is best-effort - } - } + // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) } deps.sendDesktopNotification( "GSD", diff --git a/src/resources/extensions/gsd/git-service.ts b/src/resources/extensions/gsd/git-service.ts index f63fb10ea..29cddd10f 100644 --- a/src/resources/extensions/gsd/git-service.ts +++ b/src/resources/extensions/gsd/git-service.ts @@ -684,13 +684,17 @@ export function createDraftPR( milestoneId: string, title: string, body: string, + opts?: { head?: string; base?: string }, ): string | null { try { - const result = execFileSync("gh", [ + const args = [ "pr", "create", "--draft", "--title", title, "--body", body, - ], { cwd: basePath, encoding: "utf8", timeout: 30000, env: GIT_NO_PROMPT_ENV }); + ]; + if (opts?.head) args.push("--head", opts.head); + if (opts?.base) args.push("--base", opts.base); + const result = execFileSync("gh", args, { cwd: basePath, encoding: "utf8", timeout: 30000, env: GIT_NO_PROMPT_ENV }); return result.trim(); } catch { return null; diff --git a/src/resources/extensions/gsd/tests/auto-pr-bugs.test.ts b/src/resources/extensions/gsd/tests/auto-pr-bugs.test.ts new file mode 100644 index 000000000..003d8d10d --- /dev/null +++ b/src/resources/extensions/gsd/tests/auto-pr-bugs.test.ts @@ -0,0 +1,88 @@ +/** + * auto-pr-bugs.test.ts — Regression tests for #2302. + * + * Three interacting bugs prevented auto_pr from ever creating a PR: + * 1. auto_pr was gated on `pushed` (which requires auto_push) + * 2. Milestone branch was not pushed to remote before PR creation + * 3. createDraftPR in git-service.ts lacked --head/--base parameters + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +// ─── Bug 1: auto_pr should not depend on auto_push / pushed flag ──────────── + +const autoWorktreeSrcPath = join(import.meta.dirname, "..", "auto-worktree.ts"); +const autoWorktreeSrc = readFileSync(autoWorktreeSrcPath, "utf-8"); + +test("#2302 bug 1: auto_pr condition should not require pushed flag", () => { + // Find the auto_pr block in mergeMilestoneToMain + const autoPrIdx = autoWorktreeSrc.indexOf("auto_pr"); + assert.ok(autoPrIdx !== -1, "auto_pr reference exists in auto-worktree.ts"); + + // Get context around the auto_pr check + const lineStart = autoWorktreeSrc.lastIndexOf("\n", autoPrIdx) + 1; + const lineEnd = autoWorktreeSrc.indexOf("\n", autoPrIdx); + const autoPrLine = autoWorktreeSrc.slice(lineStart, lineEnd); + + // The condition should NOT include `&& pushed` + assert.ok( + !autoPrLine.includes("&& pushed"), + "auto_pr condition should not be gated on pushed flag (auto_push dependency)", + ); +}); + +// ─── Bug 2: phases.ts should not duplicate PR creation ────────────────────── + +const phasesSrcPath = join(import.meta.dirname, "..", "auto", "phases.ts"); +const phasesSrc = readFileSync(phasesSrcPath, "utf-8"); + +test("#2302 bug 2: phases.ts should not call createDraftPR (handled by mergeMilestoneToMain)", () => { + // After fix, phases.ts should not import or call createDraftPR because + // PR creation is handled inside mergeMilestoneToMain in auto-worktree.ts + const createDraftPRCalls = phasesSrc.match(/createDraftPR\(/g) || []; + + assert.equal( + createDraftPRCalls.length, + 0, + "phases.ts should not call createDraftPR — it's handled by mergeMilestoneToMain", + ); +}); + +// ─── Bug 3: createDraftPR should accept head and base branch parameters ───── + +const gitServiceSrcPath = join(import.meta.dirname, "..", "git-service.ts"); +const gitServiceSrc = readFileSync(gitServiceSrcPath, "utf-8"); + +test("#2302 bug 3: createDraftPR should accept head and base branch parameters", () => { + // Find the createDraftPR function signature + const fnIdx = gitServiceSrc.indexOf("function createDraftPR"); + assert.ok(fnIdx !== -1, "createDraftPR function exists"); + + // Get the function signature (up to the closing paren) + const sigEnd = gitServiceSrc.indexOf(")", fnIdx); + const signature = gitServiceSrc.slice(fnIdx, sigEnd); + + // Should have head and base parameters + assert.ok( + signature.includes("head") || signature.includes("branch"), + "createDraftPR should accept a head/branch parameter", + ); +}); + +test("#2302 bug 3: createDraftPR should pass --head and --base to gh pr create", () => { + const fnIdx = gitServiceSrc.indexOf("function createDraftPR"); + const fnEnd = gitServiceSrc.indexOf("\n}", fnIdx); + const fnBody = gitServiceSrc.slice(fnIdx, fnEnd); + + assert.ok( + fnBody.includes("--head"), + "createDraftPR should pass --head to gh pr create", + ); + assert.ok( + fnBody.includes("--base"), + "createDraftPR should pass --base to gh pr create", + ); +});