From 568165aeaca3cc098a644346f33190a3461d6f84 Mon Sep 17 00:00:00 2001 From: Janni Turunen Date: Sun, 5 Apr 2026 23:29:55 +0300 Subject: [PATCH 1/2] fix(session): use session.directory instead of Instance.directory for working directory (#383) --- packages/opencode/src/session/prompt.ts | 2 +- packages/opencode/src/session/system.ts | 7 ++++--- packages/opencode/src/tool/bash.ts | 24 ++++++++++++++++-------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index d0083e890726..a842f24ec69b 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -688,7 +688,7 @@ export namespace SessionPrompt { await Plugin.trigger("experimental.chat.messages.transform", {}, { messages: sessionMessages }) // Build system prompt, adding structured output instruction if needed - const system = [...(await SystemPrompt.environment(model)), ...(await InstructionPrompt.system())] + const system = [...(await SystemPrompt.environment(model, { directory: session.directory })), ...(await InstructionPrompt.system())] const format = lastUser.format ?? { type: "text" } if (format.type === "json_schema") { system.push(STRUCTURED_OUTPUT_SYSTEM_PROMPT) diff --git a/packages/opencode/src/session/system.ts b/packages/opencode/src/session/system.ts index a61dd8cba551..810e7d6d58e4 100644 --- a/packages/opencode/src/session/system.ts +++ b/packages/opencode/src/session/system.ts @@ -26,14 +26,15 @@ export namespace SystemPrompt { return [PROMPT_ANTHROPIC_WITHOUT_TODO] } - export async function environment(model: Provider.Model) { + export async function environment(model: Provider.Model, session?: { directory: string }) { const project = Instance.project + const directory = session?.directory ?? Instance.directory return [ [ `You are powered by the model named ${model.api.id}. The exact model ID is ${model.providerID}/${model.api.id}`, `Here is some useful information about the environment you are running in:`, ``, - ` Working directory: ${Instance.directory}`, + ` Working directory: ${directory}`, ` Is directory a git repo: ${project.vcs === "git" ? "yes" : "no"}`, ` Platform: ${process.platform}`, ` Today's date: ${new Date().toDateString()}`, @@ -42,7 +43,7 @@ export namespace SystemPrompt { ` ${ project.vcs === "git" && false ? await Ripgrep.tree({ - cwd: Instance.directory, + cwd: directory, limit: 50, }) : "" diff --git a/packages/opencode/src/tool/bash.ts b/packages/opencode/src/tool/bash.ts index b62e5d7674d2..5cefa9c2de4f 100644 --- a/packages/opencode/src/tool/bash.ts +++ b/packages/opencode/src/tool/bash.ts @@ -6,6 +6,7 @@ import path from "path" import DESCRIPTION from "./bash.txt" import { Log } from "../util/log" import { Instance } from "../project/instance" +import { Session } from "../session" import { lazy } from "@/util/lazy" import { Language } from "web-tree-sitter" @@ -79,6 +80,13 @@ export const BashTool = Tool.define("bash", async () => { }), async execute(params, ctx) { let cwd = Instance.directory + try { + const session = await Session.get(ctx.sessionID) + cwd = session.directory + } catch { + // Session might not exist or DB not initialized, fallback to Instance.directory + cwd = Instance.directory + } if (params.workdir && params.workdir.trim()) { // Resolve symlinks and normalize path before validation @@ -88,10 +96,10 @@ export const BashTool = Tool.define("bash", async () => { resolvedWorkdir = realpathSync.native(params.workdir) } catch (error) { const err = error as NodeJS.ErrnoException - log.warn("workdir resolution failed, falling back to Instance.directory", { + log.warn("workdir resolution failed, falling back to session/Instance.directory", { workdir: params.workdir, error: { code: err.code, message: err.message }, - fallback: Instance.directory, + fallback: cwd, }) } @@ -104,24 +112,24 @@ export const BashTool = Tool.define("bash", async () => { if (Instance.containsPath(resolvedWorkdir)) { cwd = resolvedWorkdir } else { - log.warn("workdir is outside project boundary, falling back to Instance.directory", { + log.warn("workdir is outside project boundary, falling back to session/Instance.directory", { workdir: params.workdir, resolved: resolvedWorkdir, - fallback: Instance.directory, + fallback: cwd, }) } } else { - log.warn("workdir does not exist or is not a directory, falling back to Instance.directory", { + log.warn("workdir does not exist or is not a directory, falling back to session/Instance.directory", { workdir: params.workdir, resolved: resolvedWorkdir, - fallback: Instance.directory, + fallback: cwd, }) } } } else if (params.workdir) { - log.warn("workdir is empty or whitespace, falling back to Instance.directory", { + log.warn("workdir is empty or whitespace, falling back to session/Instance.directory", { workdir: params.workdir, - fallback: Instance.directory, + fallback: cwd, }) } From 7b3c8c0a23766daca6ef01129a0245ccb8ee68ff Mon Sep 17 00:00:00 2001 From: Janni Turunen Date: Mon, 6 Apr 2026 10:55:34 +0300 Subject: [PATCH 2/2] fix(taskctl): add git diff to respawnDeveloper prompt, fix unreachable cleanup code (#384) --- AGENTS.md | 14 +++ .../opencode/src/tasks/pulse-scheduler.ts | 94 +++++++++++++++--- .../opencode/test/tasks/respawn-diff-test.ts | 97 +++++++++++++++++++ 3 files changed, 190 insertions(+), 15 deletions(-) create mode 100644 packages/opencode/test/tasks/respawn-diff-test.ts diff --git a/AGENTS.md b/AGENTS.md index c98a14b93b2a..cee1f503a5cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -167,6 +167,20 @@ If the type system complains, fix the underlying issue. If work must be deferred, create a GitHub issue. The issue IS the TODO. +### Fix What You Find + +**When you encounter a pre-existing issue while working on code, you MUST fix it.** + +- Don't leave bugs, malformed structure, or technical debt for future discoverers +- If scope is large, file a separate GitHub issue AND fix what you can in the current PR +- Pre-existing issues are NOT an excuse to leave broken code +- "But I didn't write it that way" is not acceptable + +**Examples:** +- Found malformed indentation during review → Fix it +- Spotted unreachable code while implementing feature → Fix it +- Discovered incorrect error handling in adjacent function → Fix it + --- ## Pre-Push Verification diff --git a/packages/opencode/src/tasks/pulse-scheduler.ts b/packages/opencode/src/tasks/pulse-scheduler.ts index cf9ae1162bc2..9daee169789e 100644 --- a/packages/opencode/src/tasks/pulse-scheduler.ts +++ b/packages/opencode/src/tasks/pulse-scheduler.ts @@ -173,28 +173,32 @@ async function spawnDeveloper(task: Task, jobId: string, projectId: string, pmSe let baseCommit: string | null = null if (baseBranchExists) { + // Use worktree's branch for merge-base calculation instead of HEAD + // This ensures we get the correct merge base even if HEAD is in unexpected state + const worktreeBranch = worktreeInfo.branch try { - const res = await $`git merge-base ${base} HEAD`.quiet().nothrow().cwd(worktreeInfo.directory) + const res = await $`git merge-base ${base} ${worktreeBranch}`.quiet().nothrow().cwd(worktreeInfo.directory) if (res.exitCode === 0 && res.stdout) { baseCommit = new TextDecoder().decode(res.stdout).trim() } if (baseCommit) { - const headRes = await $`git rev-parse HEAD`.quiet().nothrow().cwd(worktreeInfo.directory) + const headRes = await $`git rev-parse ${worktreeBranch}`.quiet().nothrow().cwd(worktreeInfo.directory) 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-base), setting base_commit to null", { + const branchCommit = new TextDecoder().decode(headRes.stdout).trim() + if (baseCommit === branchCommit) { + log.warn("worktree branch equals merge-base (likely worktree-on-base), setting base_commit to null", { taskId: task.id, base_commit: baseCommit, base_branch: base, + worktree_branch: worktreeBranch, }) baseCommit = null } } } } catch (e) { - log.warn("failed to capture base_commit", { taskId: task.id, baseBranch: base, error: String(e) }) + log.warn("failed to capture base_commit", { taskId: task.id, baseBranch: base, worktreeBranch: worktreeBranch, error: String(e) }) } } else { log.warn("base branch not found in worktree, skipping merge-base capture", { taskId: task.id, baseBranch: base }) @@ -400,7 +404,7 @@ This ensures you only review changes made by the developer, not commits that wer model, parts: [{ type: "text", text: prompt }], }) - } catch (e) { +} catch (e) { log.error("adversarial session failed to start", { taskId: task.id, error: String(e) }) try { SessionPrompt.cancel(adversarialSession.id) @@ -413,13 +417,11 @@ This ensures you only review changes made by the developer, not commits that wer }) } - if (task.worktree) { - const safeWorktree = sanitizeWorktree(task.worktree) - if (safeWorktree) { - await Worktree.remove({ directory: safeWorktree }).catch((e) => - log.error("failed to remove worktree after adversarial spawn failed", { taskId: task.id, error: String(e) }), - ) - } + // Worktree cleanup on adversarial spawn failure + if (safeWorktree) { + await Worktree.remove({ directory: safeWorktree }).catch((e) => + log.error("failed to remove worktree after adversarial spawn failed", { taskId: task.id, error: String(e) }), + ) } await Store.updateTask( @@ -521,7 +523,69 @@ ABSOLUTE RULES: ? `\nCoverage level assessed: ${verdict.coverage_level.toUpperCase()}` : "" - const prompt = `${directiveHeader}This is retry attempt ${attempt} of ${MAX_ADVERSARIAL_ATTEMPTS}. The previous implementation had issues that must be fixed. + // Generate git diff showing previous implementation + const validatedBaseCommit = PulseUtils.validateBaseCommit(task.base_commit) ?? "dev" + + // NEW: Verify base commit is ancestor of HEAD + let baseIsAncestor = false + try { + const ancestorCheck = await $`git merge-base --is-ancestor ${validatedBaseCommit} HEAD`.quiet().nothrow().cwd(safeWorktree) + baseIsAncestor = ancestorCheck.exitCode === 0 + } catch { + baseIsAncestor = false + } + + const diffBase = baseIsAncestor ? validatedBaseCommit : "dev" + if (!baseIsAncestor && validatedBaseCommit !== "dev") { + log.warn("base_commit not ancestor of HEAD, using dev as fallback", { taskId: task.id, base_commit: task.base_commit }) + } + + // NEW: Check worktree validity + try { + const worktreeCheck = await $`git rev-parse --is-inside-work-tree`.quiet().nothrow().cwd(safeWorktree) + if (worktreeCheck.exitCode !== 0) { + log.warn("worktree not valid git repository", { taskId: task.id, worktree: safeWorktree }) + } + } catch {} + + let diffOutput = "" + try { + const diffResult = await $`git diff ${diffBase}..HEAD`.quiet().nothrow().cwd(safeWorktree) + if (diffResult.exitCode === 0 && diffResult.stdout) { + const rawDiff = new TextDecoder().decode(diffResult.stdout) + + // NEW: Enforce size limit (50KB max) + const MAX_DIFF_SIZE = 50 * 1024 // 50KB + if (rawDiff.length > MAX_DIFF_SIZE) { + // Use stat summary instead for large diffs + const statResult = await $`git diff --stat ${diffBase}..HEAD`.quiet().nothrow().cwd(safeWorktree) + if (statResult.exitCode === 0 && statResult.stdout) { + diffOutput = `[Diff too large, showing summary]\n${new TextDecoder().decode(statResult.stdout)}` + } + log.warn("git diff exceeded size limit, using stat summary", { taskId: task.id, size: rawDiff.length }) + } else { + diffOutput = rawDiff.trim() + } + } + } catch (e) { + log.warn("failed to generate git diff for respawn", { taskId: task.id, error: String(e) }) + } + + const previousImplementation = diffOutput + ? `# PREVIOUS IMPLEMENTATION (for reference) + +The following changes already exist in this worktree: + +\`\`\`diff +${diffOutput} +\`\`\` + +⚠️ CRITICAL: Read the changed files above FIRST. These changes already exist. +Do NOT rewrite from scratch. Fix ONLY the specific issues listed below. +` + : "" + + const prompt = `${directiveHeader}${previousImplementation}This is retry attempt ${attempt} of ${MAX_ADVERSARIAL_ATTEMPTS}. The previous implementation had issues that must be fixed. **Adversarial feedback — fix these before signaling complete:** Summary: ${verdict.summary} diff --git a/packages/opencode/test/tasks/respawn-diff-test.ts b/packages/opencode/test/tasks/respawn-diff-test.ts new file mode 100644 index 000000000000..f227098d2c78 --- /dev/null +++ b/packages/opencode/test/tasks/respawn-diff-test.ts @@ -0,0 +1,97 @@ +/** + * Test: respawnDeveloper includes git diff in prompt + * + * Verifies that when respawnDeveloper generates a prompt, + * it includes the git diff showing changes from base_commit to HEAD. + */ + +import { describe, test, expect } from "bun:test" + +const SCHEDULER_SRC = "src/tasks/pulse-scheduler.ts" + +describe("respawnDeveloper — git diff integration", () => { + test("includes git diff generation logic in respawnDeveloper", async () => { + const src = await Bun.file(SCHEDULER_SRC).text() + + // Find respawnDeveloper function body + const fnStart = src.indexOf("async function respawnDeveloper(") + const fnEnd = src.indexOf("\nexport {", fnStart) + const body = src.slice(fnStart, fnEnd) + + // Must validate base commit + expect(body).toContain("validateBaseCommit(task.base_commit)") + + // Must generate git diff using diffBase (which handles fallback to "dev") + expect(body).toContain('git diff ${diffBase}..HEAD') + expect(body).toContain('cwd(safeWorktree)') + + // Must handle diff output + expect(body).toContain("diffResult.stdout") + expect(body).toContain("TextDecoder().decode(diffResult.stdout)") + + // Must create previousImplementation variable + expect(body).toContain("previousImplementation") + }) + + test("includes previousImplementation in the prompt", async () => { + const src = await Bun.file(SCHEDULER_SRC).text() + + // Find respawnDeveloper function body + const fnStart = src.indexOf("async function respawnDeveloper(") + const fnEnd = src.indexOf("\nexport {", fnStart) + const body = src.slice(fnStart, fnEnd) + + // The prompt must include previousImplementation + expect(body).toContain("previousImplementation}") + + // The prompt must include the warning about reading changed files + expect(body).toContain("CRITICAL: Read the changed files above FIRST") + }) + + test("includes git diff output in previousImplementation block", async () => { + const src = await Bun.file(SCHEDULER_SRC).text() + + const fnStart = src.indexOf("async function respawnDeveloper(") + const fnEnd = src.indexOf("\nexport {", fnStart) + const body = src.slice(fnStart, fnEnd) + + // Check for the diff output template + expect(body).toContain("PREVIOUS IMPLEMENTATION (for reference)") + expect(body).toContain("The following changes already exist in this worktree:") + + // Check for diff code block formatting - template should contain diffOutput variable + expect(body).toContain("\\`\\`\\`diff") + expect(body).toContain("diffOutput") + }) + + test("has proper error handling for git diff generation", async () => { + const src = await Bun.file(SCHEDULER_SRC).text() + + const fnStart = src.indexOf("async function respawnDeveloper(") + const fnEnd = src.indexOf("\nexport {", fnStart) + const body = src.slice(fnStart, fnEnd) + + // Must have try/catch around git diff + expect(body).toContain("try {") + expect(body).toContain('await $`git diff ${diffBase}..HEAD`') + expect(body).toContain("} catch (e) {") + + // Must log warnings on failure + expect(body).toContain('log.warn("failed to generate git diff for respawn"') + expect(body).toContain("taskId: task.id") + }) + + test("skips previousImplementation when diffOutput is empty", async () => { + const src = await Bun.file(SCHEDULER_SRC).text() + + const fnStart = src.indexOf("async function respawnDeveloper(") + const fnEnd = src.indexOf("\nexport {", fnStart) + const body = src.slice(fnStart, fnEnd) + + // Must conditionally include previousImplementation + expect(body).toContain("const previousImplementation = diffOutput") + + // Must provide empty string when no diff + expect(body).toContain(': ""') + }) +})