From edd1cdbf439e8c7ed5bdcd3445488d76bda40dd1 Mon Sep 17 00:00:00 2001 From: Janni Turunen Date: Mon, 23 Feb 2026 15:25:58 +0200 Subject: [PATCH 1/2] fix(taskctl): implement mergeTaskBranchesToFeatureBranch and guard createPRForJob (#318) --- packages/opencode/src/tasks/job-commands.ts | 12 ++- packages/opencode/src/tasks/pulse-verdicts.ts | 85 ++++++++++++++++++- packages/opencode/src/tasks/pulse.ts | 79 +++++++++++++---- 3 files changed, 155 insertions(+), 21 deletions(-) diff --git a/packages/opencode/src/tasks/job-commands.ts b/packages/opencode/src/tasks/job-commands.ts index cfc9f87cc18..7390cc0f3cd 100644 --- a/packages/opencode/src/tasks/job-commands.ts +++ b/packages/opencode/src/tasks/job-commands.ts @@ -11,7 +11,9 @@ const log = Log.create({ service: "taskctl.tool.job-commands" }) export async function executeStart(projectId: string, params: any, ctx: any): Promise<{ title: string; output: string; metadata: {} }> { const issueNumber = params.issueNumber - if (!issueNumber) throw new Error("start requires issueNumber") + if (!issueNumber || !Number.isInteger(Number(issueNumber)) || Number(issueNumber) <= 0) { + throw new Error("issueNumber must be a valid positive integer") + } const existingJob = await Store.findJobByIssue(projectId, issueNumber) if (existingJob) { @@ -56,6 +58,14 @@ export async function executeStart(projectId: string, params: any, ctx: any): Pr const result = await $`git checkout -b ${featureBranch} dev`.cwd(ctx.session.directory).quiet().nothrow() if (result.exitCode !== 0) { log.error("failed to create feature branch", { issueNumber, featureBranch }) + } else { + // Push the feature branch to origin + const pushResult = await $`git push -u origin ${featureBranch}`.cwd(ctx.session.directory).quiet().nothrow() + if (pushResult.exitCode !== 0) { + log.warn("failed to push feature branch to origin", { issueNumber, featureBranch }) + } else { + log.info("feature branch created and pushed", { issueNumber, featureBranch }) + } } } catch (e) { log.error("error creating feature branch", { issueNumber, featureBranch, error: String(e) }) diff --git a/packages/opencode/src/tasks/pulse-verdicts.ts b/packages/opencode/src/tasks/pulse-verdicts.ts index 4d870ccbf6a..8013343135a 100644 --- a/packages/opencode/src/tasks/pulse-verdicts.ts +++ b/packages/opencode/src/tasks/pulse-verdicts.ts @@ -322,6 +322,63 @@ async function escalateCommitFailure( }) } +async function mergeTaskBranchesToFeatureBranch(projectRoot: string, featureBranch: string, tasks: Task[]): Promise<{ ok: true } | { ok: false; error: string }> { + const branches: string[] = [] + for (const task of tasks) { + if (task.branch && task.branch !== featureBranch) { + branches.push(task.branch) + } + } + if (!branches.length) { + log.debug("no task branches to merge", { featureBranch }) + return { ok: true } + } + + try { + const { $ } = await import("bun") + + // Verify origin remote exists + const remoteCheck = await $`git ls-remote origin HEAD`.cwd(projectRoot).quiet().nothrow() + if (remoteCheck.exitCode !== 0) { + log.warn("origin remote not configured", { projectRoot }) + return { ok: false, error: "origin remote not configured" } + } + + // Checkout feature branch + const checkoutRes = await $`git checkout ${featureBranch}`.cwd(projectRoot).nothrow() + if (checkoutRes.exitCode !== 0) { + log.error("failed to checkout feature branch", { featureBranch }) + return { ok: false, error: `Failed to checkout feature branch ${featureBranch}` } + } + + // Merge each task branch + for (const branch of branches) { + const mergeRes = await $`git merge --no-ff ${branch} -m "merge task branch ${branch}"`.cwd(projectRoot).nothrow() + if (mergeRes.exitCode !== 0) { + log.error("failed to merge task branch, aborting", { branch, featureBranch }) + // Abort merge to leave repository in clean state + await $`git merge --abort`.cwd(projectRoot).nothrow() + return { ok: false, error: `Merge conflict with branch ${branch}, aborting` } + } + log.info("merged task branch", { branch, featureBranch }) + } + + // Push feature branch + const pushResult = await $`git push origin ${featureBranch}`.cwd(projectRoot).nothrow() + if (pushResult.exitCode !== 0) { + const stderr = pushResult.stderr ? new TextDecoder().decode(pushResult.stderr) : "Unknown error" + log.error("failed to push feature branch", { featureBranch, error: stderr }) + return { ok: false, error: `Failed to push feature branch: ${stderr}` } + } + + log.info("pushed feature branch after merging task branches", { featureBranch }) + return { ok: true } + } catch (e) { + log.error("error merging task branches", { featureBranch, error: String(e) }) + return { ok: false, error: String(e) } + } +} + async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: string, issueNumber: number): Promise<{ ok: true; prUrl: string } | { ok: false; error: string }> { if (tasks.length === 0) { return { ok: false, error: "No tasks found in job" } @@ -332,7 +389,7 @@ async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: str // Fallback to first task branch if job or feature_branch is missing if (!featureBranch) { - const firstTaskWithBranch = tasks.find((t) => t.branch) + const firstTaskWithBranch = tasks.find((t) => t.branch && t.branch.trim().length > 0) if (firstTaskWithBranch) { featureBranch = firstTaskWithBranch.branch } else { @@ -340,6 +397,11 @@ async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: str } } + // After fallback, validate featureBranch is not empty + if (!featureBranch || featureBranch.trim().length === 0) { + return { ok: false, error: "Invalid feature branch name for job" } + } + try { const parentSession = await Session.get(pmSessionId).catch(() => null) if (!parentSession?.directory) { @@ -347,13 +409,22 @@ async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: str } const { $ } = await import("bun") + + // Check if feature branch has commits ahead of dev + const ahead = await $`git rev-list --count dev..${featureBranch}`.cwd(parentSession.directory).quiet().nothrow() + const count = parseInt(new TextDecoder().decode(ahead.stdout).trim() || "0") + if (count === 0) { + log.warn("feature branch has no commits ahead of dev, skipping PR creation", { featureBranch }) + return { ok: false, error: `Feature branch ${featureBranch} has no commits ahead of dev` } + } + const repo = "randomm/opencode" const prTitle = `Issue #${issueNumber}: Automated PR from taskctl` const prBody = `Closes #${issueNumber} This PR was automatically created by the taskctl pipeline after all tasks completed.` - // Use proper shell escaping to prevent command injection + // Bun Shell auto-escapes interpolated values; no manual escaping needed const result = await $`gh pr create --repo ${repo} --base dev --head ${featureBranch} --title ${prTitle} --body ${prBody}` .cwd(parentSession.directory) .quiet() @@ -413,4 +484,12 @@ async function processAdversarialVerdicts(jobId: string, projectId: string, pmSe } } -export { processAdversarialVerdicts, commitTask, escalateToPM, escalateCommitFailure, notifyPM, createPRForJob } \ No newline at end of file +export { + processAdversarialVerdicts, + commitTask, + escalateToPM, + escalateCommitFailure, + notifyPM, + createPRForJob, + mergeTaskBranchesToFeatureBranch, +} \ No newline at end of file diff --git a/packages/opencode/src/tasks/pulse.ts b/packages/opencode/src/tasks/pulse.ts index ee8f28562bd..421512d4f68 100644 --- a/packages/opencode/src/tasks/pulse.ts +++ b/packages/opencode/src/tasks/pulse.ts @@ -5,6 +5,7 @@ import { BackgroundTaskEvent } from "../session/async-tasks" import { Instance, context as instanceContext } from "../project/instance" import { Store } from "./store" import { MessageV2 } from "../session/message-v2" +import { Session } from "../session" import { Provider } from "../provider/provider" import { writeLockFile, @@ -15,7 +16,7 @@ import { scheduleReadyTasks, sanitizeWorktree, } from "./pulse-scheduler" -import { processAdversarialVerdicts, notifyPM, escalateToPM, createPRForJob } from "./pulse-verdicts" +import { processAdversarialVerdicts, notifyPM, escalateToPM, createPRForJob, mergeTaskBranchesToFeatureBranch } from "./pulse-verdicts" import { heartbeatActiveAgents, checkTimeouts, checkSteering, gracefulStop } from "./pulse-monitoring" // Re-exports for backward compatibility with tests @@ -28,7 +29,13 @@ export { scheduleReadyTasks, sanitizeWorktree, } from "./pulse-scheduler" -export { processAdversarialVerdicts, notifyPM, escalateToPM, createPRForJob } from "./pulse-verdicts" +export { + processAdversarialVerdicts, + notifyPM, + escalateToPM, + createPRForJob, + mergeTaskBranchesToFeatureBranch, +} from "./pulse-verdicts" export { heartbeatActiveAgents, checkTimeouts, checkSteering, gracefulStop } from "./pulse-monitoring" const log = Log.create({ service: "taskctl.pulse" }) @@ -215,27 +222,65 @@ export async function checkCompletion( await Store.updateJob(projectId, jobId, { status: "complete" }) Bus.publish(BackgroundTaskEvent.Completed, { taskID: jobId, sessionID: pmSessionId, parentSessionID: pmSessionId }) - // Create PR for the job - const issueNumber = jobTasks[0]?.parent_issue ?? 0 - const prResult = await createPRForJob(projectId, jobTasks, pmSessionId, issueNumber) - if (prResult.ok) { - log.info("PR created successfully", { jobId, prUrl: prResult.prUrl }) + // Guard: Cannot create PR if no tasks exist (all skipped/overridden) + if (jobTasks.length === 0) { + log.warn("job completed with no tasks, skipping PR creation", { jobId }) const notifyResult = await notifyPM( pmSessionId, - `šŸŽ‰ Job complete: all tasks done for issue #${jobTasks[0]?.parent_issue ?? "unknown"}\n\nPR created: ${prResult.prUrl}`, - ) - if (!notifyResult.ok) { - log.warn("failed to notify PM of job completion with PR", { jobId, error: notifyResult.error }) - } - } else { - log.warn("failed to create PR for completed job", { jobId, error: prResult.error }) - const notifyResult = await notifyPM( - pmSessionId, - `šŸŽ‰ Job complete: all tasks done for issue #${jobTasks[0]?.parent_issue ?? "unknown"}\n\nāš ļø PR creation failed: ${prResult.error}`, + `šŸŽ‰ Job complete: all tasks were skipped/overridden. No PR created.`, ) if (!notifyResult.ok) { log.warn("failed to notify PM of job completion", { jobId, error: notifyResult.error }) } + return + } + + // Merge task branches into feature branch before creating PR + const job = await Store.getJob(projectId, jobId) + const featureBranch = job?.feature_branch + let mergeSuccess = true + let mergeError = "" + + if (featureBranch) { + const pmSession = await Session.get(pmSessionId).catch(() => null) + if (pmSession?.directory) { + const mergeResult = await mergeTaskBranchesToFeatureBranch(pmSession.directory, featureBranch, jobTasks) + if (!mergeResult.ok) { + mergeSuccess = false + mergeError = mergeResult.error + log.warn("failed to merge task branches", { jobId, error: mergeError }) + } + } else { + log.warn("PM session not found for merging task branches", { pmSessionId }) + } + } else { + log.warn("No feature branch found for job, skipping task branch merge", { jobId }) + } + + // Create PR for the job (only if merge succeeded) + const issueNumber = jobTasks[0]?.parent_issue ?? 0 + let prResult: { ok: true; prUrl: string } | { ok: false; error: string } + + if (!mergeSuccess) { + prResult = { ok: false, error: `Merge failed: ${mergeError}` } + } else { + prResult = await createPRForJob(projectId, jobTasks, pmSessionId, issueNumber) + } + + // Build appropriate completion message based on result + let completionMessage: string + if (prResult.ok) { + completionMessage = `šŸŽ‰ Job complete: all tasks done for issue #${jobTasks[0]?.parent_issue ?? "unknown"}\n\nPR created: ${prResult.prUrl}` + log.info("PR created successfully", { jobId, prUrl: prResult.prUrl }) + } else if (!mergeSuccess) { + completionMessage = `šŸŽ‰ Job complete: all tasks done for issue #${jobTasks[0]?.parent_issue ?? "unknown"}\n\nāš ļø Merge failed: ${prResult.error}` + } else { + completionMessage = `šŸŽ‰ Job complete: all tasks merged for issue #${jobTasks[0]?.parent_issue ?? "unknown"}\n\nāš ļø PR creation failed: ${prResult.error}` + } + + const notifyResult = await notifyPM(pmSessionId, completionMessage) + if (!notifyResult.ok) { + log.warn("failed to notify PM of job completion", { jobId, error: notifyResult.error }) } } catch (e) { activeTicks.get(projectId)?.delete(jobId) From 57159474c044775b4d3891f85238da3a6922c25c Mon Sep 17 00:00:00 2001 From: Janni Turunen Date: Mon, 23 Feb 2026 15:32:01 +0200 Subject: [PATCH 2/2] fix(taskctl): add branch name validation and fix push failure handling (#318) --- packages/opencode/src/tasks/job-commands.ts | 34 +- packages/opencode/src/tasks/pulse-verdicts.ts | 58 ++- .../opencode/test/tasks/pr-creation.test.ts | 406 +++++++++++++++++- 3 files changed, 465 insertions(+), 33 deletions(-) diff --git a/packages/opencode/src/tasks/job-commands.ts b/packages/opencode/src/tasks/job-commands.ts index 7390cc0f3cd..818ea8414d9 100644 --- a/packages/opencode/src/tasks/job-commands.ts +++ b/packages/opencode/src/tasks/job-commands.ts @@ -9,6 +9,13 @@ import { Worktree } from "../worktree" const log = Log.create({ service: "taskctl.tool.job-commands" }) +// Branch name validation for security +const BRANCH_REGEX = /^[a-zA-Z0-9_\-\/\+.]+$/ + +function safeBranch(name: string): string | null { + return BRANCH_REGEX.test(name) ? name : null +} + export async function executeStart(projectId: string, params: any, ctx: any): Promise<{ title: string; output: string; metadata: {} }> { const issueNumber = params.issueNumber if (!issueNumber || !Number.isInteger(Number(issueNumber)) || Number(issueNumber) <= 0) { @@ -52,23 +59,32 @@ export async function executeStart(projectId: string, params: any, ctx: any): Pr const jobId = `job-${Date.now()}` const featureBranch = `feature/issue-${issueNumber}` + // Validate feature branch name before using in git operations + const safeFeatureBranch = safeBranch(featureBranch) + if (!safeFeatureBranch) { + log.error("feature branch name failed validation", { issueNumber, featureBranch }) + throw new Error(`Invalid feature branch name: ${featureBranch}`) + } + // Create the feature branch in the main repository try { const { $ } = await import("bun") - const result = await $`git checkout -b ${featureBranch} dev`.cwd(ctx.session.directory).quiet().nothrow() + const result = await $`git checkout -b ${safeFeatureBranch} dev`.cwd(ctx.session.directory).quiet().nothrow() if (result.exitCode !== 0) { - log.error("failed to create feature branch", { issueNumber, featureBranch }) + log.error("failed to create feature branch", { issueNumber, featureBranch: safeFeatureBranch }) } else { - // Push the feature branch to origin - const pushResult = await $`git push -u origin ${featureBranch}`.cwd(ctx.session.directory).quiet().nothrow() + // Push the feature branch to origin - MUST succeed before creating job + const pushResult = await $`git push -u origin ${safeFeatureBranch}`.cwd(ctx.session.directory).quiet().nothrow() if (pushResult.exitCode !== 0) { - log.warn("failed to push feature branch to origin", { issueNumber, featureBranch }) - } else { - log.info("feature branch created and pushed", { issueNumber, featureBranch }) + const stderr = pushResult.stderr ? new TextDecoder().decode(pushResult.stderr) : "Unknown error" + log.error("failed to push feature branch to origin", { issueNumber, featureBranch: safeFeatureBranch, error: stderr }) + throw new Error(`Failed to push feature branch ${safeFeatureBranch} to origin: ${stderr}`) } + log.info("feature branch created and pushed", { issueNumber, featureBranch: safeFeatureBranch }) } } catch (e) { - log.error("error creating feature branch", { issueNumber, featureBranch, error: String(e) }) + log.error("error creating feature branch", { issueNumber, featureBranch: safeFeatureBranch, error: String(e) }) + throw e } await Store.createJob(projectId, { @@ -80,7 +96,7 @@ export async function executeStart(projectId: string, params: any, ctx: any): Pr pulse_pid: null, max_workers: 3, pm_session_id: ctx.sessionID, - feature_branch: featureBranch, + feature_branch: safeFeatureBranch, }) enableAutoWakeup(ctx.sessionID) diff --git a/packages/opencode/src/tasks/pulse-verdicts.ts b/packages/opencode/src/tasks/pulse-verdicts.ts index 8013343135a..8858f83de77 100644 --- a/packages/opencode/src/tasks/pulse-verdicts.ts +++ b/packages/opencode/src/tasks/pulse-verdicts.ts @@ -19,8 +19,15 @@ import { isSessionActivelyRunning, lockFilePath } from "./pulse-scheduler" // Allow 6 attempts to resolve minor test flakiness before escalating to PM const MAX_ADVERSARIAL_ATTEMPTS = 6 +// Branch name validation: only alphanumeric, hyphen, underscore, slash, dot, plus (anchors ensure full string match) +const BRANCH_REGEX = /^[a-zA-Z0-9_\-\/\+.]+$/ + const log = Log.create({ service: "taskctl.pulse.verdicts" }) +function safeBranch(name: string): string | null { + return BRANCH_REGEX.test(name) ? name : null +} + export { MAX_ADVERSARIAL_ATTEMPTS } async function notifyPM(pmSessionId: string, text: string): Promise<{ ok: true } | { ok: false; error: string }> { @@ -323,14 +330,26 @@ async function escalateCommitFailure( } async function mergeTaskBranchesToFeatureBranch(projectRoot: string, featureBranch: string, tasks: Task[]): Promise<{ ok: true } | { ok: false; error: string }> { + // Validate feature branch name + const safeFeatureBranch = safeBranch(featureBranch) + if (!safeFeatureBranch) { + log.error("feature branch name contains invalid characters", { featureBranch }) + return { ok: false, error: `Invalid feature branch name: ${featureBranch}` } + } + const branches: string[] = [] for (const task of tasks) { - if (task.branch && task.branch !== featureBranch) { - branches.push(task.branch) + if (task.branch && task.branch !== safeFeatureBranch) { + const safeTaskBranch = safeBranch(task.branch) + if (!safeTaskBranch) { + log.warn("task branch name contains invalid characters, skipping", { taskId: task.id, branch: task.branch }) + continue + } + branches.push(safeTaskBranch) } } if (!branches.length) { - log.debug("no task branches to merge", { featureBranch }) + log.debug("no task branches to merge", { featureBranch: safeFeatureBranch }) return { ok: true } } @@ -345,33 +364,33 @@ async function mergeTaskBranchesToFeatureBranch(projectRoot: string, featureBran } // Checkout feature branch - const checkoutRes = await $`git checkout ${featureBranch}`.cwd(projectRoot).nothrow() + const checkoutRes = await $`git checkout ${safeFeatureBranch}`.cwd(projectRoot).nothrow() if (checkoutRes.exitCode !== 0) { - log.error("failed to checkout feature branch", { featureBranch }) - return { ok: false, error: `Failed to checkout feature branch ${featureBranch}` } + log.error("failed to checkout feature branch", { featureBranch: safeFeatureBranch }) + return { ok: false, error: `Failed to checkout feature branch ${safeFeatureBranch}` } } // Merge each task branch for (const branch of branches) { const mergeRes = await $`git merge --no-ff ${branch} -m "merge task branch ${branch}"`.cwd(projectRoot).nothrow() if (mergeRes.exitCode !== 0) { - log.error("failed to merge task branch, aborting", { branch, featureBranch }) + log.error("failed to merge task branch, aborting", { branch, featureBranch: safeFeatureBranch }) // Abort merge to leave repository in clean state await $`git merge --abort`.cwd(projectRoot).nothrow() return { ok: false, error: `Merge conflict with branch ${branch}, aborting` } } - log.info("merged task branch", { branch, featureBranch }) + log.info("merged task branch", { branch, featureBranch: safeFeatureBranch }) } // Push feature branch - const pushResult = await $`git push origin ${featureBranch}`.cwd(projectRoot).nothrow() + const pushResult = await $`git push origin ${safeFeatureBranch}`.cwd(projectRoot).nothrow() if (pushResult.exitCode !== 0) { const stderr = pushResult.stderr ? new TextDecoder().decode(pushResult.stderr) : "Unknown error" - log.error("failed to push feature branch", { featureBranch, error: stderr }) + log.error("failed to push feature branch", { featureBranch: safeFeatureBranch, error: stderr }) return { ok: false, error: `Failed to push feature branch: ${stderr}` } } - log.info("pushed feature branch after merging task branches", { featureBranch }) + log.info("pushed feature branch after merging task branches", { featureBranch: safeFeatureBranch }) return { ok: true } } catch (e) { log.error("error merging task branches", { featureBranch, error: String(e) }) @@ -402,20 +421,27 @@ async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: str return { ok: false, error: "Invalid feature branch name for job" } } + // Validate branch name contains only safe characters + const safeFeatureBranch = safeBranch(featureBranch) + if (!safeFeatureBranch) { + log.error("feature branch name contains invalid characters", { featureBranch }) + return { ok: false, error: `Invalid feature branch name: ${featureBranch}` } + } + try { const parentSession = await Session.get(pmSessionId).catch(() => null) if (!parentSession?.directory) { - return { ok: false, error: `PM session not found for PR creation (session: ${pmSessionId}, branch: ${featureBranch})` } + return { ok: false, error: `PM session not found for PR creation (session: ${pmSessionId}, branch: ${safeFeatureBranch})` } } const { $ } = await import("bun") // Check if feature branch has commits ahead of dev - const ahead = await $`git rev-list --count dev..${featureBranch}`.cwd(parentSession.directory).quiet().nothrow() + const ahead = await $`git rev-list --count dev..${safeFeatureBranch}`.cwd(parentSession.directory).quiet().nothrow() const count = parseInt(new TextDecoder().decode(ahead.stdout).trim() || "0") if (count === 0) { - log.warn("feature branch has no commits ahead of dev, skipping PR creation", { featureBranch }) - return { ok: false, error: `Feature branch ${featureBranch} has no commits ahead of dev` } + log.warn("feature branch has no commits ahead of dev, skipping PR creation", { featureBranch: safeFeatureBranch }) + return { ok: false, error: `Feature branch ${safeFeatureBranch} has no commits ahead of dev` } } const repo = "randomm/opencode" @@ -425,7 +451,7 @@ async function createPRForJob(projectId: string, tasks: Task[], pmSessionId: str This PR was automatically created by the taskctl pipeline after all tasks completed.` // Bun Shell auto-escapes interpolated values; no manual escaping needed - const result = await $`gh pr create --repo ${repo} --base dev --head ${featureBranch} --title ${prTitle} --body ${prBody}` + const result = await $`gh pr create --repo ${repo} --base dev --head ${safeFeatureBranch} --title ${prTitle} --body ${prBody}` .cwd(parentSession.directory) .quiet() .nothrow() diff --git a/packages/opencode/test/tasks/pr-creation.test.ts b/packages/opencode/test/tasks/pr-creation.test.ts index 8889cf8d22e..e5da9402fb1 100644 --- a/packages/opencode/test/tasks/pr-creation.test.ts +++ b/packages/opencode/test/tasks/pr-creation.test.ts @@ -2,8 +2,9 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test" import fs from "fs/promises" import path from "path" import { Instance } from "../../src/project/instance" +import { Session } from "../../src/session" import { Store } from "../../src/tasks/store" -import { createPRForJob } from "../../src/tasks/pulse-verdicts" +import { createPRForJob, mergeTaskBranchesToFeatureBranch } from "../../src/tasks/pulse-verdicts" import type { Task, Job } from "../../src/tasks/types" const TEST_PROJECT_ID = "test-pr-creation" @@ -91,7 +92,7 @@ describe("PR Creation on Job Completion", () => { const result = await createPRForJob(projectId, testTasks, "ses_" + "test".repeat(8), 123) expect(result.ok).toBe(false) - if (!result.ok) { + if (!result.ok!) { expect(result.error).toContain("No feature branch found") } }, @@ -154,11 +155,8 @@ describe("PR Creation on Job Completion", () => { } const result = await createPRForJob(projectId, testTasks, "ses_" + "test".repeat(8), 123) - // We expect this to fail because we're not in a real git repo - // but the important thing is it tried to use the feature_branch expect(result.ok).toBe(false) - // The error should contain the PR command with the correct branch - if (!result.ok) { + if (!result.ok!) { expect(result.error).toContain("feature/issue-123-test") } }, @@ -221,13 +219,405 @@ describe("PR Creation on Job Completion", () => { } const result = await createPRForJob(projectId, testTasks, "ses_" + "test".repeat(8), 123) - // Should try to use the fallback branch expect(result.ok).toBe(false) - if (!result.ok) { + if (!result.ok!) { expect(result.error).toContain("fallback-branch-123") } }, }) }) + + test("returns early when 0 commits ahead of dev", async () => { + await Instance.provide({ + directory: testDataDir, + fn: async () => { + const projectId = Instance.project.id + const pmSessionId = "ses_0000001234567890abctest" + + // Initialize a real git repo + const { $ } = await import("bun") + await $`git init`.cwd(testDataDir).quiet() + await $`git config user.email "test@example.com"`.cwd(testDataDir).quiet() + await $`git config user.name "Test User"`.cwd(testDataDir).quiet() + await $`git checkout -b dev`.cwd(testDataDir).quiet() + await $`git checkout -b feature/issue-123`.cwd(testDataDir).quiet() + + // Create the PM session + await Session.createNext({ + id: pmSessionId, + directory: testDataDir, + title: "Test PM Session", + }) + + const testJob: Job = { + id: "job-test", + parent_issue: 123, + status: "running", + created_at: new Date().toISOString(), + stopping: false, + pulse_pid: null, + max_workers: 3, + pm_session_id: pmSessionId, + feature_branch: "feature/issue-123", + } + + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: testJob.id, + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: null, + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + await Store.createJob(projectId, testJob) + for (const task of testTasks) { + await Store.createTask(projectId, task) + } + + const result = await createPRForJob(projectId, testTasks, pmSessionId, 123) + expect(result.ok).toBe(false) + if (!result.ok!) { + expect(result.error).toContain("no commits ahead of dev") + } + }, + }) + }) + +test("rejects branch names with invalid characters", async () => { + await Instance.provide({ + directory: testDataDir, + fn: async () => { + const projectId = Instance.project.id + const testJob: Job = { + id: "job-invalid-branch", + parent_issue: 123, + status: "running", + created_at: new Date().toISOString(), + stopping: false, + pulse_pid: null, + max_workers: 3, + pm_session_id: "ses_0000001234567890abctest", + feature_branch: "feature/issue-123;rm -rf /", // Contains invalid semicolon and space + } + + const pmSessionId = "ses_" + "test".repeat(8) + + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: testJob.id, + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: null, + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + await Store.createJob(projectId, testJob) + for (const task of testTasks) { + await Store.createTask(projectId, task) + } + + const result = await createPRForJob(projectId, testTasks, pmSessionId, 123) + expect(result.ok).toBe(false) + if (!result.ok!) { + expect(result.error).toContain("Invalid feature branch name") + } + }, + }) + }) + + test("rejects branch names with embedded newlines", async () => { + // This tests that the regex anchors (^ and $) prevent embedded newline injection + // Without end anchor, test('feature/issue-123\necho hacked') would return true + await Instance.provide({ + directory: testDataDir, + fn: async () => { + const projectId = Instance.project.id + const testJob: Job = { + id: "job-newline-branch", + parent_issue: 123, + status: "running", + created_at: new Date().toISOString(), + stopping: false, + pulse_pid: null, + max_workers: 3, + pm_session_id: "ses_0000001234567890abctest", + feature_branch: "feature/issue-123\necho hacked", // Contains embedded newline + } + + const pmSessionId = "ses_" + "test".repeat(8) + + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: testJob.id, + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: null, + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + await Store.createJob(projectId, testJob) + for (const task of testTasks) { + await Store.createTask(projectId, task) + } + + const result = await createPRForJob(projectId, testTasks, pmSessionId, 123) + expect(result.ok).toBe(false) + if (!result.ok!) { + expect(result.error).toContain("Invalid feature branch name") + } + }, + }) + }) + }) + + describe("mergeTaskBranchesToFeatureBranch", () => { + test("skips tasks with null branches", async () => { + await Instance.provide({ + directory: testDataDir, + fn: async () => { + // Initialize a real git repo + const { $ } = await import("bun") + await $`git init`.cwd(testDataDir).quiet() + await $`git config user.email "test@example.com"`.cwd(testDataDir).quiet() + await $`git config user.name "Test User"`.cwd(testDataDir).quiet() + await $`git checkout -b feature/issue-123`.cwd(testDataDir).quiet() + + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task with null branch", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: "job-1", + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: null, + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + { + id: "task-2", + title: "Test Task with empty string branch", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: "job-1", + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: "", + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + const result = await mergeTaskBranchesToFeatureBranch(testDataDir, "feature/issue-123", testTasks) + expect(result.ok).toBe(true) + }, + }) + }) + + test("skips tasks with invalid branch names", async () => { + await Instance.provide({ + directory: testDataDir, + fn: async () => { + // Initialize a real git repo + const { $ } = await import("bun") + await $`git init`.cwd(testDataDir).quiet() + await $`git config user.email "test@example.com"`.cwd(testDataDir).quiet() + await $`git config user.name "Test User"`.cwd(testDataDir).quiet() + await $`git checkout -b feature/issue-123`.cwd(testDataDir).quiet() + + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task with invalid branch", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: "job-1", + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: "branch;rm -rf /", // Invalid: contains semicolon and space + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + const result = await mergeTaskBranchesToFeatureBranch(testDataDir, "feature/issue-123", testTasks) + expect(result.ok).toBe(true) + }, + }) + }) + + test("rejects invalid feature branch name", async () => { + await Instance.provide({ + directory: testDataDir, + fn: async () => { + const testTasks: Task[] = [ + { + id: "task-1", + title: "Test Task", + description: "Description", + acceptance_criteria: "Criteria", + parent_issue: 123, + job_id: "job-1", + status: "closed", + priority: 0, + task_type: "implementation", + labels: [], + depends_on: [], + assignee: null, + assignee_pid: null, + worktree: null, + branch: null, + base_commit: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + close_reason: "approved and committed", + comments: [], + pipeline: { + stage: "done", + attempt: 0, + last_activity: null, + last_steering: null, + history: [], + adversarial_verdict: null, + }, + }, + ] + + const result = await mergeTaskBranchesToFeatureBranch(testDataDir, "feature/issue-123;rm", testTasks) + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error).toContain("Invalid feature branch name") + } + }, + }) + }) }) }) \ No newline at end of file