diff --git a/packages/opencode/src/agent/prompt/adversarial-pipeline.txt b/packages/opencode/src/agent/prompt/adversarial-pipeline.txt index 7865521bc4e..72c502acbfd 100644 --- a/packages/opencode/src/agent/prompt/adversarial-pipeline.txt +++ b/packages/opencode/src/agent/prompt/adversarial-pipeline.txt @@ -62,6 +62,11 @@ YOU DO NOT: 4. Ensure error handling covers documented failures ### Scope Creep +Your review scope is STRICTLY LIMITED to the task's acceptance criteria. Do NOT flag: +- Commits from dev that appear in the diff due to branch rebasing +- Changes in files not related to the task description +- If a commit SHA appears in the diff that predates this task, ignore it entirely + - Does the implementation add ANYTHING not in the task description? - Extra tests not covering the implementation → ISSUES_FOUND (MEDIUM) - New functions or helpers not requested → ISSUES_FOUND (HIGH) @@ -90,6 +95,12 @@ Flag ONLY changes that appear in this diff as out-of-scope. ## Verdict Categories +**WORKFLOW_ERROR** — For process failures only (not implementation issues): +- No committed changes found (empty diff) +- Wrong working directory detected +- Wrong branch detected +Use WORKFLOW_ERROR instead of CRITICAL_ISSUES_FOUND for pure process failures. + **CRITICAL_ISSUES_FOUND** — Must fix before proceeding: - Security vulnerabilities - Data corruption risks diff --git a/packages/opencode/src/agent/prompt/developer-pipeline.txt b/packages/opencode/src/agent/prompt/developer-pipeline.txt index c85b7960ee7..daa7ab8fef2 100644 --- a/packages/opencode/src/agent/prompt/developer-pipeline.txt +++ b/packages/opencode/src/agent/prompt/developer-pipeline.txt @@ -29,12 +29,11 @@ You will receive a task description with: ## First Action: Load Skills **BEFORE writing any code:** -1. Identify the domain from the task -2. Load appropriate skill via `mcp_skill` tool -3. Confirm: "Loaded [skill-name] for this task" -4. Use context7 for any related technical documentation - -Skills are loaded dynamically based on task domain (Python, Rails, React, Rust, TypeScript, etc.) +1. This is a TypeScript/Bun project +2. Load the `bun-file-io` skill via `mcp_skill` tool +3. DO NOT load go-idiomatic, python-tdd, rails-conventions, or other non-TypeScript skills +4. Confirm: "Loaded bun-file-io skill" +5. Use context7 for any related technical documentation ## Workflow diff --git a/packages/opencode/src/tasks/pulse-scheduler.ts b/packages/opencode/src/tasks/pulse-scheduler.ts index 568de8741f8..fc2a7046243 100644 --- a/packages/opencode/src/tasks/pulse-scheduler.ts +++ b/packages/opencode/src/tasks/pulse-scheduler.ts @@ -17,6 +17,7 @@ import { Instance as InstanceImport } from "../project/instance" import type { Task, AdversarialVerdict } from "./types" import { MAX_ADVERSARIAL_ATTEMPTS } from "./pulse-verdicts" import { resolveModel } from "./pulse" +import * as PulseUtils from "./pulse-utils" const log = Log.create({ service: "taskctl.pulse.scheduler" }) @@ -230,7 +231,7 @@ async function spawnDeveloper(task: Task, jobId: string, projectId: string, pmSe ) const model = await resolveModel(pmSessionId) - const prompt = buildDeveloperPrompt(task) + const prompt = buildDeveloperPrompt(task, worktreeInfo.directory, worktreeInfo.branch) try { await SessionPrompt.prompt({ sessionID: devSession.id, @@ -273,7 +274,16 @@ async function spawnDeveloper(task: Task, jobId: string, projectId: string, pmSe } } -function buildDeveloperPrompt(task: Task): string { +function buildDeveloperPrompt(task: Task, worktreeDir: string, branch: string): string { + const worktreeWarning = `⚠️ WORKING DIRECTORY: You are working in worktree: ${worktreeDir} +On branch: ${branch} + +BEFORE any file operation: +1. Verify with: git branch --show-current +2. If output is NOT "${branch}", STOP immediately and report the error +3. All file reads/writes must use paths under ${worktreeDir} +` + return `Implement the following task with TDD: **Title:** ${task.title} @@ -307,9 +317,27 @@ async function spawnAdversarial(task: Task, jobId: string, projectId: string, pm log.error("invalid worktree for adversarial spawn", { taskId: task.id, worktree: task.worktree }) return } - const safeWorktree = task.worktree.replace(/[^\w\-./]/g, "") + const safeWorktree = sanitizeWorktree(task.worktree) if (!safeWorktree) { - log.error("worktree sanitization resulted in empty string", { taskId: task.id, worktree: task.worktree }) + log.error("worktree sanitization failed or resulted in null", { taskId: task.id, worktree: task.worktree }) + return + } + + await Store.updateTask(projectId, task.id, { + pipeline: { ...task.pipeline, stage: "adversarial-running", last_activity: new Date().toISOString() }, + }, true) + + // Check if developer committed changes + const hasChanges = await PulseUtils.hasCommittedChanges(safeWorktree, task.base_commit) + if (!hasChanges) { + await Store.addComment(projectId, task.id, { + author: "system", + message: "No committed changes found. Developer wrote code but did not commit. Respawning developer.", + created_at: new Date().toISOString(), + }) + await Store.updateTask(projectId, task.id, { + pipeline: { ...task.pipeline, stage: "developing", last_activity: new Date().toISOString() }, + }, true) return } @@ -326,11 +354,8 @@ async function spawnAdversarial(task: Task, jobId: string, projectId: string, pm return } - await Store.updateTask(projectId, task.id, { - pipeline: { ...task.pipeline, stage: "adversarial-running", last_activity: new Date().toISOString() }, - }, true) - - const baseCommitStr = task.base_commit ? `Base Commit: ${task.base_commit}` : "Base Commit: Not captured" + const validatedBaseCommit = PulseUtils.validateBaseCommit(task.base_commit) ?? "dev" + const baseCommitStr = task.base_commit ? `Base Commit: ${validatedBaseCommit}` : "Base Commit: Not captured" const prompt = `Review the implementation in worktree at: ${safeWorktree} Task ID: ${task.id} @@ -342,7 +367,7 @@ ${baseCommitStr} When reviewing changes, use git diff to see ONLY the developer's changes: \`\`\`bash cd ${safeWorktree} -git diff ${task.base_commit || "dev"}..HEAD +git diff ${validatedBaseCommit}..HEAD \`\`\` This ensures you only review changes made by the developer, not commits that were already in dev. @@ -444,8 +469,18 @@ async function respawnDeveloper( true, ) + const worktreeWarning = `⚠️ WORKING DIRECTORY: You are working in worktree: ${task.worktree} +On branch: ${task.branch || "feature"} + +BEFORE any file operation: +1. Run: git branch --show-current +2. If output is NOT "${task.branch || "feature"}", STOP and report immediately +3. All file paths must be under ${task.worktree} + +` + const issueLines = verdict.issues.map((i) => ` - ${i.location} [${i.severity}]: ${i.fix}`).join("\n") - const prompt = `This is retry attempt ${attempt} of ${MAX_ADVERSARIAL_ATTEMPTS}. The previous implementation had issues that must be fixed. + const prompt = `${worktreeWarning}This is retry attempt ${attempt} of ${MAX_ADVERSARIAL_ATTEMPTS}. The previous implementation had issues that must be fixed. **Task:** ${task.title} **Description:** ${task.description} diff --git a/packages/opencode/src/tasks/pulse-utils.ts b/packages/opencode/src/tasks/pulse-utils.ts new file mode 100644 index 00000000000..f9256455756 --- /dev/null +++ b/packages/opencode/src/tasks/pulse-utils.ts @@ -0,0 +1,42 @@ +import { $ } from "bun" +import { Log } from "../util/log" + +const log = Log.create({ service: "taskctl.pulse.utils" }) + +/** + * Validate a git commit/branch name is safe to use in shell commands and prompts + * Prevents command injection and code execution + */ +export function validateBaseCommit(baseCommit: string | null | undefined): string | null { + if (baseCommit === null || baseCommit === undefined) { + return null + } + + // Allow only: SHA-1 hashes (40 hex chars), SHA-256 hashes (64 hex chars), or simple branch/tag names + // Pattern restricts to: alphanumeric, hyphen, underscore, slash, or dot + // Prevents: command substitution, shell metacharacters, flag injection (no leading hyphen) + if (!/^[a-fA-F0-9]{40}$/.test(baseCommit) && + !/^[a-fA-F0-9]{64}$/.test(baseCommit) && + !/^[a-zA-Z0-9](?:[-a-zA-Z0-9_./]*[a-zA-Z0-9])?$/.test(baseCommit)) { + return null + } + + return baseCommit +} + +/** + * 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 diffCheck = await $`git diff ${validated}..HEAD --stat`.quiet().nothrow().cwd(worktreePath) + 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 diff --git a/packages/opencode/test/fixture/fixture.ts b/packages/opencode/test/fixture/fixture.ts index ed8c5e344a8..ad9a13cbb0e 100644 --- a/packages/opencode/test/fixture/fixture.ts +++ b/packages/opencode/test/fixture/fixture.ts @@ -42,4 +42,4 @@ export async function tmpdir(options?: TmpDirOptions) { extra: extra as T, } return result -} +} \ No newline at end of file diff --git a/packages/opencode/test/tasks/pulse-happy-path.test.ts b/packages/opencode/test/tasks/pulse-happy-path.test.ts index a6b561ad384..2be8b2e7b64 100644 --- a/packages/opencode/test/tasks/pulse-happy-path.test.ts +++ b/packages/opencode/test/tasks/pulse-happy-path.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test, spyOn } from "bun:test" +import { describe, expect, test, spyOn, mock } from "bun:test" import { Instance } from "../../src/project/instance" import { Store } from "../../src/tasks/store" import type { Task, Job } from "../../src/tasks/types" @@ -8,7 +8,12 @@ import { SessionPrompt } from "../../src/session/prompt" import { Worktree } from "../../src/worktree" import { SessionStatus } from "../../src/session/status" -// Import the tick functions - these need to be exported from pulse.ts +// Mock hasCommittedChanges BEFORE any imports that use it +mock.module("../../src/tasks/pulse-utils", () => ({ + hasCommittedChanges: async () => true, +})) + +// Import the tick functions AFTER the mock is set up import { scheduleReadyTasks, heartbeatActiveAgents, @@ -20,10 +25,6 @@ describe("taskctl pulse: full happy path integration test", () => { // Mock SessionPrompt.prompt to return immediately (simulating developer/adversarial completing) const promptSpy = spyOn(SessionPrompt, "prompt").mockImplementation(() => Promise.resolve()) - // Mock Worktree.remove to avoid cleanup noise - const removeSpy = spyOn(Worktree, "remove") - removeSpy.mockImplementation(async () => true) - await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -47,6 +48,7 @@ describe("taskctl pulse: full happy path integration test", () => { pulse_pid: null, max_workers: 3, pm_session_id: pmSession.id, + feature_branch: `feature/issue-257-test`, } // Create a task in open state @@ -148,6 +150,5 @@ describe("taskctl pulse: full happy path integration test", () => { // Clean up mocks promptSpy.mockRestore() - removeSpy.mockRestore() }) }) \ No newline at end of file