diff --git a/packages/opencode/src/tasks/job-commands.ts b/packages/opencode/src/tasks/job-commands.ts index b61f677febf..06e97c1cbc2 100644 --- a/packages/opencode/src/tasks/job-commands.ts +++ b/packages/opencode/src/tasks/job-commands.ts @@ -8,6 +8,7 @@ import { runComposer } from "./composer" import { SessionPrompt } from "../session/prompt" import { Worktree } from "../worktree" import { Instance } from "../project/instance" +import * as PulseUtils from "./pulse-utils" const log = Log.create({ service: "taskctl.tool.job-commands" }) @@ -81,7 +82,8 @@ export async function executeStart(projectId: string, params: any, ctx: any): Pr try { const { $ } = await import("bun") - const result = await $`git checkout -b ${safeFeatureBranch} dev`.cwd(cwd).quiet().nothrow() + const base = await PulseUtils.defaultBranch(cwd) + const result = await $`git checkout -b ${safeFeatureBranch} ${base}`.cwd(cwd).quiet().nothrow() if (result.exitCode !== 0) { const stderr = result.stderr ? new TextDecoder().decode(result.stderr) : "Unknown error" log.error("failed to create feature branch", { issueNumber, featureBranch: safeFeatureBranch, error: stderr }) diff --git a/packages/opencode/src/tasks/pulse-scheduler.ts b/packages/opencode/src/tasks/pulse-scheduler.ts index 3f49b098761..1f1dd3566a5 100644 --- a/packages/opencode/src/tasks/pulse-scheduler.ts +++ b/packages/opencode/src/tasks/pulse-scheduler.ts @@ -151,20 +151,29 @@ async function spawnDeveloper(task: Task, jobId: string, projectId: string, pmSe const now = new Date().toISOString() - let devBranchExists = false + // Detect the default branch dynamically + let base: string try { - const res = await $`git rev-parse --verify dev`.quiet().nothrow().cwd(worktreeInfo.directory) + base = await PulseUtils.defaultBranch(worktreeInfo.directory) + } catch (e) { + log.warn("failed to detect default branch, using fallback", { taskId: task.id, error: String(e) }) + base = "dev" + } + + let baseBranchExists = false + try { + const res = await $`git rev-parse --verify ${base}`.quiet().nothrow().cwd(worktreeInfo.directory) if (res.exitCode === 0) { - devBranchExists = true + baseBranchExists = true } } catch (e) { - log.warn("failed to verify dev branch exists", { taskId: task.id, error: String(e) }) + log.warn("failed to verify base branch exists", { taskId: task.id, baseBranch: base, error: String(e) }) } let baseCommit: string | null = null - if (devBranchExists) { + if (baseBranchExists) { try { - const res = await $`git merge-base dev HEAD`.quiet().nothrow().cwd(worktreeInfo.directory) + const res = await $`git merge-base ${base} HEAD`.quiet().nothrow().cwd(worktreeInfo.directory) if (res.exitCode === 0 && res.stdout) { baseCommit = new TextDecoder().decode(res.stdout).trim() } @@ -174,19 +183,20 @@ async function spawnDeveloper(task: Task, jobId: string, projectId: string, pmSe if (headRes.exitCode === 0 && headRes.stdout) { const headCommit = new TextDecoder().decode(headRes.stdout).trim() if (baseCommit === headCommit) { - log.warn("worktree HEAD equals merge-base (likely worktree-on-dev), setting base_commit to null", { + log.warn("worktree HEAD equals merge-base (likely worktree-on-base), setting base_commit to null", { taskId: task.id, base_commit: baseCommit, + base_branch: base, }) baseCommit = null } } } } catch (e) { - log.warn("failed to capture base_commit", { taskId: task.id, error: String(e) }) + log.warn("failed to capture base_commit", { taskId: task.id, baseBranch: base, error: String(e) }) } } else { - log.warn("dev branch not found in worktree, skipping merge-base capture", { taskId: task.id }) + log.warn("base branch not found in worktree, skipping merge-base capture", { taskId: task.id, baseBranch: base }) } const parentSession = await Session.get(pmSessionId).catch(() => null) @@ -377,7 +387,7 @@ cd ${safeWorktree} git diff ${validatedBaseCommit}..HEAD \`\`\` -This ensures you only review changes made by the developer, not commits that were already in dev. +This ensures you only review changes made by the developer, not commits that were already in the base branch. Read the changed files in the worktree, run typecheck, and record your verdict with taskctl verdict.` diff --git a/packages/opencode/src/tasks/pulse-utils.ts b/packages/opencode/src/tasks/pulse-utils.ts index f9256455756..8d1b8905dc2 100644 --- a/packages/opencode/src/tasks/pulse-utils.ts +++ b/packages/opencode/src/tasks/pulse-utils.ts @@ -1,4 +1,5 @@ import { $ } from "bun" +import { existsSync } from "fs" import { Log } from "../util/log" const log = Log.create({ service: "taskctl.pulse.utils" }) @@ -24,19 +25,83 @@ export function validateBaseCommit(baseCommit: string | null | undefined): strin return baseCommit } +/** + * Validate a branch name is safe to use in shell commands + * Only allow alphanumeric chars, hyphens, underscores, slashes, and dots + */ +function safeBranchName(name: string): string | null { + // Trim all Unicode whitespace variations + const trimmed = name.replace(/^[\s\uFEFF]+|[\s\uFEFF]+$/g, "") + // Only allow safe branch name characters: alphanumeric, hyphen, underscore, slash, dot + return /^[a-zA-Z0-9][a-zA-Z0-9._\-/]*$/.test(trimmed) ? trimmed : null +} + /** * Check if there are committed changes in the worktree (for adversarial review validation) * This is extracted into a separate file for easier test mocking */ export async function hasCommittedChanges(worktreePath: string, baseCommit: string | null): Promise { try { - const validated = validateBaseCommit(baseCommit) ?? "dev" + const validated = validateBaseCommit(baseCommit) + if (!validated) { + log.warn("invalid base commit ref", { baseCommit, worktreePath }) + return false + } + + if (!existsSync(worktreePath)) return false const diffCheck = await $`git diff ${validated}..HEAD --stat`.quiet().nothrow().cwd(worktreePath) + if (diffCheck.exitCode !== 0) return false const diffOutput = new TextDecoder().decode(diffCheck.stdout).trim() return diffOutput.length > 0 } catch (e) { log.warn("failed to check for committed changes", { worktreePath, baseCommit, error: String(e) }) return false } -} \ No newline at end of file +} + +/** + * Detect the default branch of a git repository dynamically + * Tries symbolic-ref first (fastest), falls back to ls-remote, then defaults to 'dev' + */ +export async function defaultBranch(cwd: string): Promise { + try { + // Try symbolic-ref first (fastest, works when origin/HEAD is set) + const ref = await $`git symbolic-ref refs/remotes/origin/HEAD --short`.quiet().nothrow().cwd(cwd) + if (ref.exitCode === 0) { + const name = new TextDecoder().decode(ref.stdout).trim().replace(/^origin\//, "") + const validated = safeBranchName(name) + if (validated) { + log.debug("detected default branch via symbolic-ref", { branch: validated }) + return validated + } + } + } catch (e) { + log.debug("symbolic-ref attempt failed", { error: String(e) }) + } + + try { + // Fallback: use git ls-remote which is faster and more reliable + // Shell timeout 5 enforces the 5 second limit (enforces at shell level via timeout command) + const show = await $`timeout 5 git ls-remote --symref origin HEAD`.quiet().nothrow().cwd(cwd) + if (show.exitCode === 0) { + const output = new TextDecoder().decode(show.stdout) + // Parse output like: "ref: refs/heads/main HEAD" + const match = output.match(/ref:\s*refs\/heads\/([^\t\n\r]+)/) + if (match?.[1]) { + const branch = match[1].trim() + const validated = safeBranchName(branch) + if (validated) { + log.debug("detected default branch via ls-remote", { branch: validated }) + return validated + } + } + } + } catch (e) { + log.debug("ls-remote attempt failed", { error: String(e) }) + } + + // Last resort default + log.warn("could not detect default branch, using fallback", { fallback: "dev" }) + return "dev" +} diff --git a/packages/opencode/test/tasks/pulse-utils.test.ts b/packages/opencode/test/tasks/pulse-utils.test.ts new file mode 100644 index 00000000000..2fd7fcb8e4d --- /dev/null +++ b/packages/opencode/test/tasks/pulse-utils.test.ts @@ -0,0 +1,209 @@ +import { describe, it, expect, beforeEach, afterEach, mock } from "bun:test" +import path from "path" +import { $ } from "bun" +import { defaultBranch, validateBaseCommit, hasCommittedChanges } from "../../src/tasks/pulse-utils" + +describe("pulse-utils", () => { + describe("validateBaseCommit", () => { + it("returns null for undefined", () => { + expect(validateBaseCommit(undefined)).toBeNull() + }) + + it("returns null for null", () => { + expect(validateBaseCommit(null)).toBeNull() + }) + + it("validates SHA-1 hashes", () => { + const sha1 = "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b" + expect(validateBaseCommit(sha1)).toBe(sha1) + }) + + it("validates SHA-256 hashes", () => { + const sha256 = "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2a1b2c3d4e5f6a1b2c3d4e5" + expect(validateBaseCommit(sha256)).toBe(sha256) + }) + + it("validates simple branch names", () => { + expect(validateBaseCommit("dev")).toBe("dev") + expect(validateBaseCommit("main")).toBe("main") + expect(validateBaseCommit("feature-name")).toBe("feature-name") + expect(validateBaseCommit("feature/test")).toBe("feature/test") + }) + + it("rejects command injection attempts", () => { + expect(validateBaseCommit("dev && rm -rf /")).toBeNull() + expect(validateBaseCommit("dev; echo hack")).toBeNull() + expect(validateBaseCommit("`whoami`")).toBeNull() + expect(validateBaseCommit("$(whoami)")).toBeNull() + }) + + it("rejects names with leading hyphen (flag injection)", () => { + expect(validateBaseCommit("-rf")).toBeNull() + }) + + it("rejects names with trailing special chars", () => { + expect(validateBaseCommit("dev-")).toBeNull() + }) + }) + + describe("defaultBranch", () => { + let testDir: string + + beforeEach(async () => { + // Create a temporary git repository for testing + testDir = `/tmp/test-repo-${Date.now()}` + await $`mkdir -p ${testDir}`.quiet() + await $`cd ${testDir} && git init`.quiet() + await $`cd ${testDir} && git config user.email "test@example.com"`.quiet() + await $`cd ${testDir} && git config user.name "Test User"`.quiet() + }) + + afterEach(async () => { + // Cleanup test directory + await $`rm -rf ${testDir}`.quiet().nothrow() + }) + + it("returns dev as fallback when no remote is configured", async () => { + const branch = await defaultBranch(testDir) + expect(branch).toBe("dev") + }) + + it("detects default branch from git symbolic-ref when available", async () => { + // Create an origin remote + const remoteDir = `/tmp/test-remote-${Date.now()}` + await $`mkdir -p ${remoteDir}`.quiet() + await $`cd ${remoteDir} && git init --bare`.quiet() + + // Add remote to test repo + await $`cd ${testDir} && git remote add origin ${remoteDir}`.quiet() + + // Create a commit and push + await $`cd ${testDir} && echo "test" > file.txt && git add file.txt && git commit -m "init"`.quiet() + await $`cd ${testDir} && git branch -M main && git push -u origin main 2>/dev/null || true`.quiet().nothrow() + + // Set the symbolic ref (this simulates a real GitHub setup) + await $`cd ${remoteDir} && git symbolic-ref HEAD refs/heads/main`.quiet() + + // Fetch to update remote tracking branches + await $`cd ${testDir} && git fetch origin 2>/dev/null || true`.quiet().nothrow() + + // The defaultBranch function should detect main + const branch = await defaultBranch(testDir) + expect(branch).toBe("main") + + // Cleanup remote + await $`rm -rf ${remoteDir}`.quiet() + }) + + it("handles git errors gracefully", async () => { + // Use a non-existent directory + const branch = await defaultBranch("/non/existent/path") + expect(branch).toBe("dev") + }) + + it("detects branch from ls-remote when symbolic-ref is not available", async () => { + // Create an origin remote + const remoteDir = `/tmp/test-remote-${Date.now()}` + await $`mkdir -p ${remoteDir}`.quiet() + await $`cd ${remoteDir} && git init --bare`.quiet() + + // Add remote to test repo + await $`cd ${testDir} && git remote add origin ${remoteDir}`.quiet() + + // Create a commit and push to dev branch + await $`cd ${testDir} && echo "test" > file.txt && git add file.txt && git commit -m "init"`.quiet() + await $`cd ${testDir} && git branch -M dev && git push -u origin dev 2>/dev/null || true`.quiet().nothrow() + + // Set the symbolic ref on the remote to dev + await $`cd ${remoteDir} && git symbolic-ref HEAD refs/heads/dev`.quiet() + + // Fetch to update remote tracking branches + await $`cd ${testDir} && git fetch origin 2>/dev/null || true`.quiet().nothrow() + + // Even though symbolic-ref is set, the fallback ls-remote should also detect it correctly + const branch = await defaultBranch(testDir) + expect(branch).toBe("dev") + + // Cleanup remote + await $`rm -rf ${remoteDir}`.quiet() + }) + + it("validates and sanitizes detected branch names", async () => { + // Create an origin remote + const remoteDir = `/tmp/test-remote-${Date.now()}` + await $`mkdir -p ${remoteDir}`.quiet() + await $`cd ${remoteDir} && git init --bare`.quiet() + + // Add remote to test repo + await $`cd ${testDir} && git remote add origin ${remoteDir}`.quiet() + + // Create a commit and push + await $`cd ${testDir} && echo "test" > file.txt && git add file.txt && git commit -m "init"`.quiet() + await $`cd ${testDir} && git branch -M main && git push -u origin main 2>/dev/null || true`.quiet().nothrow() + + // Set a valid symbolic ref + await $`cd ${remoteDir} && git symbolic-ref HEAD refs/heads/main`.quiet() + + // Fetch to update remote tracking branches + await $`cd ${testDir} && git fetch origin 2>/dev/null || true`.quiet().nothrow() + + // Should detect main (valid branch name) + const branch = await defaultBranch(testDir) + expect(branch).toBe("main") + + // Cleanup remote + await $`rm -rf ${remoteDir}`.quiet() + }) + + it("falls back to dev when both detection methods fail", async () => { + // Create a local repo with unreachable remote + const localRepo = `/tmp/test-local-${Date.now()}` + await $`mkdir -p ${localRepo}`.quiet() + await $`cd ${localRepo} && git init`.quiet() + await $`cd ${localRepo} && git config user.email "test@example.com"`.quiet() + await $`cd ${localRepo} && git config user.name "Test User"`.quiet() + + // Add a fake remote that won't work + await $`cd ${localRepo} && git remote add origin "invalid://nowhere"`.quiet() + + // Should fallback to dev + const branch = await defaultBranch(localRepo) + expect(branch).toBe("dev") + + // Cleanup + await $`rm -rf ${localRepo}`.quiet() + }) + }) + + describe("hasCommittedChanges", () => { + let testDir: string + + beforeEach(async () => { + testDir = `/tmp/test-worktree-${Date.now()}` + await $`mkdir -p ${testDir}`.quiet() + await $`cd ${testDir} && git init`.quiet() + await $`cd ${testDir} && git config user.email "test@example.com"`.quiet() + await $`cd ${testDir} && git config user.name "Test User"`.quiet() + }) + + afterEach(async () => { + await $`rm -rf ${testDir}`.quiet().nothrow() + }) + + it("returns true when changes are committed", async () => { + // Create initial commit on 'main' or 'master' + await $`cd ${testDir} && echo "initial" > file.txt && git add file.txt && git commit -m "init"`.quiet() + + // Create another branch and make a commit + await $`cd ${testDir} && git checkout -b feature 2>/dev/null || git switch -c feature`.quiet() + await $`cd ${testDir} && echo "changed" > file.txt && git add file.txt && git commit -m "change"`.quiet() + + // Get the base commit (which should show differences) + const baseCommit = await $`cd ${testDir} && git rev-parse HEAD~1`.quiet().text() + const base = baseCommit.trim() + + const hasChanges = await hasCommittedChanges(testDir, base) + expect(hasChanges).toBe(true) + }) + }) +})