diff --git a/defaults/devclaw/prompts/developer.md b/defaults/devclaw/prompts/developer.md index 60b704d0..ff2ba020 100644 --- a/defaults/devclaw/prompts/developer.md +++ b/defaults/devclaw/prompts/developer.md @@ -7,14 +7,17 @@ When you start work, you're given: - **Issue:** number, title, body, URL, labels, state - **Comments:** full discussion thread on the issue - **Project:** repo path, base branch, project name, projectSlug +- **Checkout contract:** canonical issue branch / ref / commit / expected worktree path when DevClaw can determine them Read the comments carefully — they often contain clarifications, decisions, or scope changes that aren't in the original issue body. +Before you edit code, verify the assigned checkout contract. If the task names an expected branch, ref, commit, or worktree path and you cannot match it in a clean isolated checkout, stop and report `work_finish({ role: "developer", result: "blocked", ... })` instead of continuing in a shared ambient checkout. + ## Workflow ### 1. Create a worktree -**NEVER work in the main checkout.** Create a dedicated git worktree as a sibling to the repo: +**NEVER work in the main checkout.** Create or reuse the dedicated canonical issue worktree named in the task message. If no explicit worktree is given, create one as a sibling to the repo: ```bash # Example: repo is at ~/git/myproject diff --git a/defaults/devclaw/prompts/reviewer.md b/defaults/devclaw/prompts/reviewer.md index 35697758..04859e1c 100644 --- a/defaults/devclaw/prompts/reviewer.md +++ b/defaults/devclaw/prompts/reviewer.md @@ -20,6 +20,9 @@ You are a code reviewer. Your job is to review the PR diff for quality, correctn ## Your Job - Read the PR diff carefully +- Review against the exact target ref/commit named in the task message, not an ambient shared checkout +- If you do any local verification, use the expected isolated worktree from the task message or another equivalent clean checkout pinned to the same target +- Do not give a definitive approve/reject verdict from a dirty tree, mismatched HEAD, or missing expected worktree - Check the code against the review checklist - Call `task_comment` with your review findings - Then call `work_finish` @@ -29,8 +32,9 @@ You are a code reviewer. Your job is to review the PR diff for quality, correctn - **Do NOT use closing keywords in PR/MR descriptions** (no "Closes #X", "Fixes #X", "Resolves #X"). Use "As described in issue #X" or "Addresses issue #X". DevClaw manages issue state — auto-closing bypasses the review lifecycle. - You do NOT run code or tests — you only review the diff - Be specific about issues: file, line, what's wrong, how to fix -- If you approve, briefly note what you checked +- If you approve, briefly note what you checked and include checkout provenance if you inspected code locally - If you reject, list actionable items the developer must fix +- If you inspected code locally, record repo path, worktree path, ref, `git rev-parse HEAD`, and dirty/clean status in your `task_comment` ## Filing Follow-Up Issues diff --git a/defaults/devclaw/prompts/tester.md b/defaults/devclaw/prompts/tester.md index 6fdcba53..206c5cbc 100644 --- a/defaults/devclaw/prompts/tester.md +++ b/defaults/devclaw/prompts/tester.md @@ -4,11 +4,13 @@ You test the deployed version and inspect code on the base branch. ## Your Job -- Pull latest from the base branch +- Validate the exact target ref/commit named in the task message, not whatever happens to be in a shared workspace +- Use the expected isolated worktree from the task message, or an equivalent clean checkout pinned to the same target, for decisive verification +- Refuse a definitive pass/fail run if the expected worktree is missing, `git status --short` is not empty, or `git rev-parse HEAD` does not match the target commit - Run tests and linting - Verify the changes address the issue requirements - Check for regressions in related functionality -- **Always** call `task_comment` with your review findings — even if everything looks good, leave a brief summary of what you checked +- **Always** call `task_comment` with your review findings and provenance — even if everything looks good, leave a brief summary of what you checked ## Conventions @@ -22,6 +24,18 @@ If you discover unrelated bugs or needed improvements during your work, call `ta `task_create({ projectSlug: "", title: "Bug: ...", description: "..." })` +## Required Provenance In task_comment + +Before `work_finish`, include this provenance in your `task_comment`: +- repo path +- worktree path +- branch/ref +- `git rev-parse HEAD` +- dirty/clean status from `git status --short` +- whether HEAD matched the requested target commit + +If the tree is dirty or the commit does not match, do not give a definitive PASS/FAIL. Use `work_finish({ role: "tester", result: "blocked", ... })` or explain the mismatch. + ## Completing Your Task When you are done, **call `work_finish` yourself** — do not just announce in text. diff --git a/lib/dispatch/index.ts b/lib/dispatch/index.ts index 59427e4e..f2ee8501 100644 --- a/lib/dispatch/index.ts +++ b/lib/dispatch/index.ts @@ -5,6 +5,7 @@ * state update (activateWorker), and audit logging. */ import type { PluginRuntime } from "openclaw/plugin-sdk"; +import path from "node:path"; import type { RunCommand } from "../context.js"; import { log as auditLog } from "../audit.js"; import { recordLoopDiagnostic } from "../services/loop-diagnostics.js"; @@ -67,6 +68,75 @@ export type DispatchResult = { announcement: string; }; +async function resolveCheckoutContract( + repoPath: string, + baseBranch: string, + role: string, + issueId: number, + issueTitle: string, + prFeedback: PrFeedback | undefined, + prContext: PrContext | undefined, + provider: import("../providers/provider.js").IssueProvider, + rc: RunCommand, +): Promise<{ targetRef?: string; targetSha?: string; targetBranch?: string; expectedWorktreePath?: string; requiredCleanTree: true; requireIsolatedWorktree: true; decisiveVerdictRequiresMatch: true } | undefined> { + if (!["developer", "tester", "reviewer"].includes(role)) return undefined; + + const slug = issueTitle + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 48) || `issue-${issueId}`; + const fallbackBranch = `issue/${issueId}-${slug}`; + + let targetRef: string | undefined; + let targetBranch: string | undefined; + if (role === "reviewer" && prContext?.url) { + const prStatus = await provider.getPrStatus(issueId).catch(() => undefined); + if (prStatus?.sourceBranch) { + targetBranch = prStatus.sourceBranch; + targetRef = `origin/${prStatus.sourceBranch}`; + } + } else if (prFeedback?.branchName) { + targetBranch = prFeedback.branchName; + targetRef = `origin/${prFeedback.branchName}`; + } else { + targetBranch = role === "developer" ? fallbackBranch : baseBranch; + targetRef = `origin/${baseBranch}`; + } + + const expectedWorktreePath = targetBranch + ? path.join(`${repoPath}.worktrees`, targetBranch) + : undefined; + + let targetSha: string | undefined; + if (targetRef) { + try { + const result = await rc(["git", "ls-remote", "--exit-code", "origin", targetRef.replace(/^origin\//, "")], { + cwd: repoPath, + timeoutMs: 10_000, + }); + targetSha = result.stdout.trim().split(/\s+/)[0] || undefined; + } catch { + try { + const result = await rc(["git", "rev-parse", targetRef], { cwd: repoPath, timeoutMs: 5_000 }); + targetSha = result.stdout.trim() || undefined; + } catch { + // best-effort only + } + } + } + + return { + targetRef, + targetSha, + targetBranch, + expectedWorktreePath, + requiredCleanTree: true, + requireIsolatedWorktree: true, + decisiveVerdictRequiresMatch: true, + }; +} + /** * Dispatch a task to a worker session. * @@ -169,6 +239,17 @@ export async function dispatchTask( } catch { /* best-effort */ } const primaryChannelId = project.channels[0]?.channelId ?? project.slug; + const checkoutContract = await resolveCheckoutContract( + project.repo, + project.baseBranch, + role, + issueId, + issueTitle, + prFeedback, + prContext, + provider, + rc, + ); const isConflictFix = prFeedback?.reason === "merge_conflict"; const taskMessage = isConflictFix && prFeedback ? buildConflictFixMessage({ @@ -181,7 +262,7 @@ export async function dispatchTask( projectName: project.name, channelId: primaryChannelId, role, issueId, issueTitle, issueDescription, issueUrl, repo: project.repo, baseBranch: project.baseBranch, - comments, resolvedRole, prContext, prFeedback, attachmentContext, + comments, resolvedRole, prContext, prFeedback, checkoutContract, attachmentContext, }); // Load role-specific instructions to inject into the worker's system prompt diff --git a/lib/dispatch/message-builder.test.ts b/lib/dispatch/message-builder.test.ts new file mode 100644 index 00000000..98129c13 --- /dev/null +++ b/lib/dispatch/message-builder.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; +import { buildTaskMessage } from "./message-builder.js"; + +describe("buildTaskMessage", () => { + it("includes checkout contract with target ref and sha", () => { + const msg = buildTaskMessage({ + projectName: "devclaw", + channelId: "-100", + role: "tester", + issueId: 171, + issueTitle: "Pin validation checkout", + issueDescription: "Make validation deterministic", + issueUrl: "https://example.com/issues/171", + repo: "/tmp/devclaw", + baseBranch: "main", + checkoutContract: { + targetBranch: "issue/171-checkout-contract-validation", + expectedWorktreePath: "/tmp/devclaw.worktrees/issue/171-checkout-contract-validation", + targetRef: "origin/main", + targetSha: "24164242926923d9accc44cfa65892c1506e77d8", + requiredCleanTree: true, + requireIsolatedWorktree: true, + decisiveVerdictRequiresMatch: true, + }, + }); + + expect(msg).toContain("## Required Checkout Contract"); + expect(msg).toContain("Target branch: `issue/171-checkout-contract-validation`"); + expect(msg).toContain("Expected worktree path: `/tmp/devclaw.worktrees/issue/171-checkout-contract-validation`"); + expect(msg).toContain("Target ref: `origin/main`"); + expect(msg).toContain("Target commit: `24164242926923d9accc44cfa65892c1506e77d8`"); + expect(msg).toContain("git rev-parse HEAD"); + expect(msg).toContain("git status --short"); + expect(msg).toContain("Use an isolated worktree or other clean checkout"); + }); +}); diff --git a/lib/dispatch/message-builder.ts b/lib/dispatch/message-builder.ts index 1524f6d2..1579a952 100644 --- a/lib/dispatch/message-builder.ts +++ b/lib/dispatch/message-builder.ts @@ -26,6 +26,15 @@ export function buildTaskMessage(opts: { resolvedRole?: ResolvedRoleConfig; prContext?: PrContext; prFeedback?: PrFeedback; + checkoutContract?: { + targetRef?: string; + targetSha?: string; + targetBranch?: string; + expectedWorktreePath?: string; + requiredCleanTree?: boolean; + requireIsolatedWorktree?: boolean; + decisiveVerdictRequiresMatch?: boolean; + }; /** Pre-formatted attachment context string (from formatAttachmentsForTask) */ attachmentContext?: string; }): string { @@ -70,7 +79,7 @@ export function buildTaskMessage(opts: { if (opts.prContext) parts.push(...formatPrContext(opts.prContext)); if (opts.prFeedback) { parts.push(...formatPrFeedback(opts.prFeedback, baseBranch)); - + // Defensive warning if branch name is missing (shouldn't happen in practice) if (!opts.prFeedback.branchName && opts.prFeedback.reason === "merge_conflict") { parts.push( @@ -83,6 +92,34 @@ export function buildTaskMessage(opts: { ); } } + + if (opts.checkoutContract) { + const c = opts.checkoutContract; + parts.push(...[ + ``, + `## Required Checkout Contract`, + `Use an isolated worktree or other clean checkout for decisive validation. Do not trust a shared mutable workspace.`, + c.targetBranch ? `- Target branch: \`${c.targetBranch}\`` : undefined, + c.expectedWorktreePath ? `- Expected worktree path: \`${c.expectedWorktreePath}\`` : undefined, + c.targetRef ? `- Target ref: \`${c.targetRef}\`` : `- Target ref: resolve from the linked PR/base branch before validating`, + c.targetSha ? `- Target commit: \`${c.targetSha}\`` : `- Target commit: record the exact commit you validated`, + `- Required clean tree: ${c.requiredCleanTree === false ? "recommended" : "yes"}`, + `- Required isolated worktree: ${c.requireIsolatedWorktree === false ? "recommended" : "yes"}`, + `- Definitive pass/fail or approve/reject only after HEAD matches the target commit and the tree is clean`, + ``, + `Before giving a decisive verdict, record this provenance in your task_comment or summary:`, + `- repo path`, + `- worktree path`, + `- branch/ref`, + `- \`git rev-parse HEAD\``, + `- dirty/clean status from \`git status --short\``, + c.targetSha ? `- confirmation that HEAD == \`${c.targetSha}\`` : `- the target commit you intended to validate`, + ``, + `If the expected branch or isolated worktree is missing, stop and report it as a checkout-contract failure instead of continuing in an ambient shared checkout.`, + `Refuse a definitive verdict if the checkout is dirty or does not match the target commit. Use work_finish("blocked") if needed.`, + ].filter(Boolean) as string[]); + } + if (opts.attachmentContext) parts.push(opts.attachmentContext); parts.push(