From c9ae9accc6dcbbd644c5940892a3dfd6e0a84d62 Mon Sep 17 00:00:00 2001 From: fujiwaranosai850 Date: Tue, 5 May 2026 04:06:06 +0000 Subject: [PATCH] feat: enforce canonical issue checkout contracts (#174) --- defaults/devclaw/prompts/developer.md | 28 +-- defaults/devclaw/prompts/reviewer.md | 2 + defaults/devclaw/prompts/tester.md | 5 +- .../developing-devclaw-with-openclaw.md | 16 +- docs/devclaw-self-hosting.md | 14 +- lib/checkout-contract.test.ts | 30 ++++ lib/checkout-contract.ts | 170 ++++++++++++++++++ lib/dispatch/index.ts | 22 ++- lib/dispatch/message-builder.test.ts | 49 +++++ lib/dispatch/message-builder.ts | 46 ++++- lib/dispatch/pr-context.test.ts | 20 +-- lib/dispatch/pr-context.ts | 12 +- lib/projects/migrations.ts | 5 + lib/projects/mutations.ts | 22 ++- lib/projects/schema-migration.ts | 1 + lib/projects/types.ts | 31 ++++ lib/providers/github.ts | 12 +- lib/providers/gitlab.ts | 7 +- lib/providers/provider.ts | 2 + lib/tools/admin/project-status.ts | 15 ++ lib/tools/worker/work-finish.test.ts | 20 +-- lib/tools/worker/work-finish.ts | 75 +++++++- 22 files changed, 531 insertions(+), 73 deletions(-) create mode 100644 lib/checkout-contract.test.ts create mode 100644 lib/checkout-contract.ts create mode 100644 lib/dispatch/message-builder.test.ts diff --git a/defaults/devclaw/prompts/developer.md b/defaults/devclaw/prompts/developer.md index 60b704d0..72817ddb 100644 --- a/defaults/devclaw/prompts/developer.md +++ b/defaults/devclaw/prompts/developer.md @@ -12,21 +12,21 @@ Read the comments carefully — they often contain clarifications, decisions, or ## Workflow -### 1. Create a worktree +### 1. Adopt the canonical checkout contract -**NEVER work in the main checkout.** Create a dedicated git worktree as a sibling to the repo: +**NEVER work in the main checkout.** Your task message includes a **Canonical Checkout Contract** section with the exact required worktree path and branch. -```bash -# Example: repo is at ~/git/myproject -# Worktree goes to ~/git/myproject.worktrees/feature/123-add-auth -REPO_ROOT="$(git rev-parse --show-toplevel)" -BRANCH="feature/-" -WORKTREE="${REPO_ROOT}.worktrees/${BRANCH}" -git worktree add "$WORKTREE" -b "$BRANCH" -cd "$WORKTREE" -``` +For normal issue work: +- branch name must be `issue/-` +- worktree path must be the canonical path from the task message +- for DevClaw specifically, the implementation base is `devclaw-local-dev` +- `devclaw-local-current` is the operator-managed release/local-truth branch, not your normal implementation base + +If the canonical worktree already exists, verify it is on the required branch and clean before you proceed. +If it is missing, create that exact worktree and branch. +If it is dirty or mismatched and you cannot repair it deterministically, stop and call `work_finish({ role: "developer", result: "blocked", ... })`. -The `.worktrees/` directory sits NEXT TO the repo folder (not inside it). This keeps the main checkout clean for the orchestrator and other workers. If a worktree already exists from a previous task on the same branch, verify it's clean before reusing it. +Only use derived validation checkouts after the canonical issue worktree is preserved and up to date. ### 2. Implement the changes @@ -47,7 +47,7 @@ Conventional commits: `feat:`, `fix:`, `chore:`, `refactor:`, `test:`, `docs:` ### 4. Create a Pull Request -Use `gh pr create` to open a PR against the base branch. **Do NOT use closing keywords** in the description (no "Closes #X", "Fixes #X"). Use "Addresses issue #X" instead — DevClaw manages issue lifecycle. +Use `gh pr create` to open a PR against the implementation base branch from the task message. For DevClaw implementation work, that PR must target `devclaw-local-dev`, not `devclaw-local-current`. **Do NOT use closing keywords** in the description (no "Closes #X", "Fixes #X"). Use "Addresses issue #X" instead — DevClaw manages issue lifecycle. ### Handling PR Feedback (changes requested / To Improve) @@ -99,7 +99,7 @@ you MUST work on the branch explicitly mentioned in the instructions. **The instructions will show:** ``` 🔹 PR: https://github.com/.../pull/123 -🔹 Branch: `feature/456-description` +🔹 Branch: `issue/456-description` ``` Use THAT branch. Do not: diff --git a/defaults/devclaw/prompts/reviewer.md b/defaults/devclaw/prompts/reviewer.md index 35697758..41d30d14 100644 --- a/defaults/devclaw/prompts/reviewer.md +++ b/defaults/devclaw/prompts/reviewer.md @@ -7,6 +7,7 @@ You are a code reviewer. Your job is to review the PR diff for quality, correctn - **Issue:** the original task description and discussion - **PR diff:** the code changes to review - **PR URL:** link to the pull request +- **Canonical Checkout Contract:** the expected implementation branch/worktree identity for the issue ## Review Checklist @@ -27,6 +28,7 @@ You are a code reviewer. Your job is to review the PR diff for quality, correctn ## Conventions - **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. +- Preserve the canonical checkout identity from the task message. If you inspect code locally, use the recorded canonical worktree unless the task explicitly allows an exception mode. - 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 diff --git a/defaults/devclaw/prompts/tester.md b/defaults/devclaw/prompts/tester.md index 6fdcba53..ed382c64 100644 --- a/defaults/devclaw/prompts/tester.md +++ b/defaults/devclaw/prompts/tester.md @@ -1,10 +1,11 @@ # TESTER Worker Instructions -You test the deployed version and inspect code on the base branch. +You test the deployed version and inspect code while preserving the canonical checkout identity from the task message. ## Your Job -- Pull latest from the base branch +- Use the canonical worktree and branch from the task message unless the task explicitly declares an exception mode such as `review/*`, `pr/*`, live self-hosting, or release flow +- Pull latest from the required base or implementation branch for that contract - Run tests and linting - Verify the changes address the issue requirements - Check for regressions in related functionality diff --git a/dev/runbooks/developing-devclaw-with-openclaw.md b/dev/runbooks/developing-devclaw-with-openclaw.md index 83915689..3cd566f5 100644 --- a/dev/runbooks/developing-devclaw-with-openclaw.md +++ b/dev/runbooks/developing-devclaw-with-openclaw.md @@ -7,9 +7,10 @@ It lives under `/dev` because these rules are first-class local operating docs a Treat these branch roles as the working contract: -- `devclaw-local-current`: local truth and day-to-day working lane +- `devclaw-local-dev`: normal project branch for day-to-day implementation work +- `devclaw-local-current`: operator-managed local truth and release branch - `devclaw-local-stable`: local fallback lane when `devclaw-local-current` is too noisy or risky -- `issue/*`: local implementation branches for scoped work +- `issue/*`: canonical local implementation branches for scoped work, derived from `devclaw-local-dev` - `review/*`: local review branches opened against `devclaw-local-current` - `pr/*`: export branches prepared for upstream review @@ -18,11 +19,12 @@ Upstream `main` is a reference point and export target. It is not the normal day ## Operating model 1. Keep local docs and operator runbooks on `devclaw-local-current`. -2. Start implementation from `devclaw-local-current` into an `issue/*` branch when you need isolated task work. -3. Land validated work back onto `devclaw-local-current` so local truth stays complete. -4. When work needs to go upstream, export it onto a matching `pr/*` branch. -5. Preserve the `/dev/` documentation changes on `devclaw-local-current` even when the upstream export omits local-only material. -6. Push runbook and workflow changes to the Git remote that tracks `devclaw-local-current` so the policy is not left only in a local checkout or an unknown branch. +2. Start ordinary implementation from `devclaw-local-dev` into a canonical `issue/*` branch/worktree when you need isolated task work. +3. Land developer PRs from `issue/*` back into `devclaw-local-dev`. +4. Let the operator/orchestrator manage promotion from `devclaw-local-dev` into `devclaw-local-current`. +5. When work needs to go upstream, export it onto a matching `pr/*` branch. +6. Preserve the `/dev/` documentation changes on `devclaw-local-current` even when the upstream export omits local-only material. +7. Push runbook and workflow changes to the Git remote that tracks `devclaw-local-current` so the policy is not left only in a local checkout or an unknown branch. ## Mandatory compliance rule diff --git a/docs/devclaw-self-hosting.md b/docs/devclaw-self-hosting.md index 3fda31a3..60e7c5fa 100644 --- a/docs/devclaw-self-hosting.md +++ b/docs/devclaw-self-hosting.md @@ -23,14 +23,16 @@ That means branch switching for DevClaw development is really a **plugin source ## Recommended branch roles -These are roles, not mandatory branch names: +For DevClaw self-hosting, use this local policy: -- **clean integration branch**: the branch that tracks the normal upstream line -- **working branch**: a feature or fix branch carrying in-progress changes -- **fallback branch**: an optional known-good branch you can switch back to quickly -- **local docs branch**: an optional branch for operator runbooks that are not meant for upstream +- **implementation branch**: `devclaw-local-dev` +- **release/local-truth branch**: `devclaw-local-current` +- **ordinary issue branches**: `issue/*`, based on `devclaw-local-dev` +- **local review branches**: `review/*` +- **upstream export branches**: `pr/*` +- **fallback branch**: an optional known-good lane such as `devclaw-local-stable` -Use names that fit your repo. The workflow matters more than the naming. +Normal implementation work belongs on canonical `issue/*` worktrees derived from `devclaw-local-dev`. Live self-hosting checks and release/promotion work are explicit exception modes and do not replace the ordinary issue checkout contract. ## Safe live-switch procedure diff --git a/lib/checkout-contract.test.ts b/lib/checkout-contract.test.ts new file mode 100644 index 00000000..5b469906 --- /dev/null +++ b/lib/checkout-contract.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from "vitest"; +import { resolveExpectedCheckoutContract } from "./checkout-contract.js"; + +describe("resolveExpectedCheckoutContract", () => { + it("routes normal devclaw issue work to devclaw-local-dev and issue/*", () => { + const contract = resolveExpectedCheckoutContract({ + project: { + slug: "devclaw", + name: "devclaw", + repo: "/home/sai/git/devclaw.worktrees/devclaw-local-current", + groupName: "DevClaw", + deployUrl: "", + baseBranch: "devclaw-local-current", + deployBranch: "devclaw-local-current", + channels: [], + workers: {}, + issueCheckouts: {}, + }, + issueId: 174, + issueTitle: "Implement canonical issue checkout contract", + repoPath: "/home/sai/git/devclaw.worktrees/devclaw-local-current", + role: "developer", + }); + + expect(contract.mode).toBe("issue"); + expect(contract.baseBranch).toBe("devclaw-local-dev"); + expect(contract.canonicalBranch).toBe("issue/174-implement-canonical-issue-checkout-contract"); + expect(contract.canonicalWorktreePath).toContain(".worktrees/issue/174-implement-canonical-issue-checkout-contract"); + }); +}); diff --git a/lib/checkout-contract.ts b/lib/checkout-contract.ts new file mode 100644 index 00000000..0c2fa66d --- /dev/null +++ b/lib/checkout-contract.ts @@ -0,0 +1,170 @@ +/** + * checkout-contract.ts — Canonical checkout contract resolution and enforcement. + */ +import path from "node:path"; +import type { RunCommand } from "./context.js"; +import type { Project } from "./projects/types.js"; + +export type CheckoutMode = "issue" | "review" | "pr" | "live" | "release"; +export type CheckoutStatus = "planned" | "created" | "adopted" | "missing" | "dirty" | "mismatched" | "verified"; + +export type CheckoutProvenance = { + verifiedAt: string; + path: string; + branch: string | null; + headSha: string | null; + clean: boolean; + status: CheckoutStatus; + details?: string; +}; + +export type IssueCheckoutContract = { + issueId: number; + issueTitle?: string; + mode: CheckoutMode; + repoPath: string; + canonicalBranch: string; + canonicalWorktreePath: string; + baseBranch: string; + baseWorktreePath: string; + targetRef: string; + targetSha: string | null; + requiredCleanliness: "clean" | "allow-derived-dirty"; + status: CheckoutStatus; + lastVerifiedProvenance?: CheckoutProvenance; +}; + +export function slugifyIssueTitle(title: string): string { + return title.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-+|-+$/g, "").slice(0, 48) || "task"; +} + +export function getImplementationBaseBranch(project: Project): string { + return project.name === "devclaw" ? "devclaw-local-dev" : project.baseBranch; +} + +export function getReleaseBranch(project: Project): string { + return project.name === "devclaw" ? "devclaw-local-current" : project.baseBranch; +} + +export function inferCheckoutMode(role: string, issueTitle: string, prBranchName?: string): CheckoutMode { + const branch = prBranchName ?? ""; + if (branch.startsWith("review/")) return "review"; + if (branch.startsWith("pr/")) return "pr"; + const title = issueTitle.toLowerCase(); + if (title.includes("self-host") || title.includes("self host") || title.includes("live ")) return "live"; + if (title.includes("release") || title.includes("promotion")) return "release"; + return role === "developer" ? "issue" : "issue"; +} + +function deriveWorktreeRoot(repoPath: string): string { + if (repoPath.includes(".worktrees/")) { + return repoPath.slice(0, repoPath.indexOf(".worktrees/")) + ".worktrees"; + } + return `${repoPath}.worktrees`; +} + +function branchPath(branch: string): string { + return branch; +} + +export function resolveExpectedCheckoutContract(opts: { + project: Project; + issueId: number; + issueTitle: string; + repoPath: string; + role: string; + mode?: CheckoutMode; + prBranchName?: string; + targetSha?: string | null; +}): IssueCheckoutContract { + const mode = opts.mode ?? inferCheckoutMode(opts.role, opts.issueTitle, opts.prBranchName); + const worktreeRoot = deriveWorktreeRoot(opts.repoPath); + const issueSlug = slugifyIssueTitle(opts.issueTitle); + const canonicalBranch = mode === "issue" + ? `issue/${opts.issueId}-${issueSlug}` + : opts.prBranchName ?? `${mode}/${opts.issueId}-${issueSlug}`; + const implementationBaseBranch = getImplementationBaseBranch(opts.project); + const releaseBranch = getReleaseBranch(opts.project); + const baseBranch = mode === "issue" ? implementationBaseBranch : (mode === "release" || mode === "live" ? releaseBranch : canonicalBranch); + const baseWorktreePath = mode === "issue" + ? path.join(worktreeRoot, implementationBaseBranch) + : mode === "release" || mode === "live" + ? path.join(worktreeRoot, releaseBranch) + : path.join(worktreeRoot, branchPath(canonicalBranch)); + + return { + issueId: opts.issueId, + issueTitle: opts.issueTitle, + mode, + repoPath: opts.repoPath, + canonicalBranch, + canonicalWorktreePath: path.join(worktreeRoot, branchPath(canonicalBranch)), + baseBranch, + baseWorktreePath, + targetRef: mode === "issue" ? implementationBaseBranch : canonicalBranch, + targetSha: opts.targetSha ?? null, + requiredCleanliness: mode === "issue" ? "clean" : "allow-derived-dirty", + status: "planned", + }; +} + +async function git(runCommand: RunCommand, cwd: string, ...args: string[]): Promise { + const result = await runCommand(["git", ...args], { cwd, timeoutMs: 15_000 }); + return result.stdout.trim(); +} + +export async function inspectCheckoutContract(contract: IssueCheckoutContract, runCommand: RunCommand): Promise { + const verifiedAt = new Date().toISOString(); + try { + const branch = await git(runCommand, contract.canonicalWorktreePath, "branch", "--show-current"); + const headSha = await git(runCommand, contract.canonicalWorktreePath, "rev-parse", "HEAD"); + const statusOut = await git(runCommand, contract.canonicalWorktreePath, "status", "--porcelain"); + const clean = statusOut.length === 0; + const branchOk = branch === contract.canonicalBranch; + const status: CheckoutStatus = !branchOk ? "mismatched" : (!clean && contract.requiredCleanliness === "clean") ? "dirty" : "verified"; + return { + verifiedAt, + path: contract.canonicalWorktreePath, + branch: branch || null, + headSha: headSha || null, + clean, + status, + details: branchOk ? undefined : `expected ${contract.canonicalBranch}, got ${branch || "(detached)"}`, + }; + } catch (error) { + return { + verifiedAt, + path: contract.canonicalWorktreePath, + branch: null, + headSha: null, + clean: false, + status: "missing", + details: error instanceof Error ? error.message : String(error), + }; + } +} + +export async function ensureCheckoutContract(contract: IssueCheckoutContract, runCommand: RunCommand): Promise { + let provenance = await inspectCheckoutContract(contract, runCommand); + if (provenance.status === "missing" && contract.mode === "issue") { + await runCommand(["git", "worktree", "add", contract.canonicalWorktreePath, "-B", contract.canonicalBranch, contract.baseBranch], { + cwd: contract.repoPath, + timeoutMs: 30_000, + }); + provenance = await inspectCheckoutContract(contract, runCommand); + return { ...contract, status: provenance.status === "verified" ? "created" : provenance.status, lastVerifiedProvenance: provenance, targetSha: provenance.headSha }; + } + return { ...contract, status: provenance.status === "verified" ? "adopted" : provenance.status, lastVerifiedProvenance: provenance, targetSha: provenance.headSha }; +} + +export function renderCheckoutRecoveryGuidance(contract: IssueCheckoutContract): string[] { + return [ + "", + "### Checkout Recovery", + `- Required path: \`${contract.canonicalWorktreePath}\``, + `- Required branch: \`${contract.canonicalBranch}\``, + `- Base branch: \`${contract.baseBranch}\``, + "- If the path is missing, recreate it with the exact canonical branch/worktree.", + "- If the worktree is dirty or on the wrong branch, stop and call work_finish with result \"blocked\" unless you can repair it deterministically.", + ]; +} diff --git a/lib/dispatch/index.ts b/lib/dispatch/index.ts index 59427e4e..94e4b4b2 100644 --- a/lib/dispatch/index.ts +++ b/lib/dispatch/index.ts @@ -14,12 +14,15 @@ import { updateSlot, getRoleWorker, emptySlot, + upsertIssueCheckout, + resolveRepoPath, } from "../projects/index.js"; import { resolveModel } from "../roles/index.js"; import { notify, getNotificationConfig } from "./notify.js"; import { loadConfig, type ResolvedRoleConfig } from "../config/index.js"; import { ReviewPolicy, TestPolicy, resolveReviewRouting, resolveTestRouting, resolveNotifyChannel, isFeedbackState, hasReviewCheck, producesReviewableWork, hasTestPhase, detectOwner, getOwnerLabel, OWNER_LABEL_COLOR, getRoleLabelColor, STEP_ROUTING_COLOR, getStateLabels } from "../workflow/index.js"; import { fetchPrFeedback, fetchPrContext, type PrFeedback, type PrContext } from "./pr-context.js"; +import { ensureCheckoutContract, resolveExpectedCheckoutContract } from "../checkout-contract.js"; import { formatAttachmentsForTask } from "./attachments.js"; import { loadRoleInstructions } from "./bootstrap-hook.js"; import { slotName } from "../names.js"; @@ -162,6 +165,21 @@ export async function dispatchTask( const prContext = hasReviewCheck(workflow, role) ? await fetchPrContext(provider, issueId) : undefined; + const repoPath = resolveRepoPath(project.repo); + const checkoutContract = await ensureCheckoutContract( + resolveExpectedCheckoutContract({ + project, + issueId, + issueTitle, + repoPath, + role, + prBranchName: prFeedback?.branchName, + targetSha: prContext?.diff ? undefined : null, + }), + rc, + ); + await upsertIssueCheckout(workspaceDir, project.slug, checkoutContract); + // Fetch attachment context (best-effort — never blocks dispatch) let attachmentContext: string | undefined; try { @@ -175,13 +193,13 @@ export async function dispatchTask( projectName: project.name, channelId: primaryChannelId, role, issueId, issueTitle, issueUrl, repo: project.repo, baseBranch: project.baseBranch, - resolvedRole, prFeedback, + resolvedRole, prFeedback, checkoutContract, }) : buildTaskMessage({ 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..a21c96b0 --- /dev/null +++ b/lib/dispatch/message-builder.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from "vitest"; +import { buildTaskMessage } from "./message-builder.js"; +import type { IssueCheckoutContract } from "../projects/types.js"; + +const contract: IssueCheckoutContract = { + issueId: 174, + mode: "issue", + repoPath: "/repo", + canonicalBranch: "issue/174-canonical-checkout-contract", + canonicalWorktreePath: "/repo.worktrees/issue/174-canonical-checkout-contract", + baseBranch: "devclaw-local-dev", + baseWorktreePath: "/repo.worktrees/devclaw-local-dev", + targetRef: "devclaw-local-dev", + targetSha: "abc123", + requiredCleanliness: "clean", + status: "verified", + lastVerifiedProvenance: { + verifiedAt: "2026-05-05T00:00:00Z", + path: "/repo.worktrees/issue/174-canonical-checkout-contract", + branch: "issue/174-canonical-checkout-contract", + headSha: "abc123", + clean: true, + status: "verified", + }, +}; + +describe("buildTaskMessage", () => { + it("renders the canonical checkout contract and validation lane distinction", () => { + const msg = buildTaskMessage({ + projectName: "devclaw", + channelId: "-100", + role: "developer", + issueId: 174, + issueTitle: "Implement canonical issue checkout contract", + issueDescription: "Do the work.", + issueUrl: "https://example.com/issues/174", + repo: "/repo", + baseBranch: "devclaw-local-current", + checkoutContract: contract, + resolvedRole: { completionResults: ["done", "blocked"] } as any, + }); + + expect(msg).toContain("## Canonical Checkout Contract"); + expect(msg).toContain("Required worktree: `/repo.worktrees/issue/174-canonical-checkout-contract`"); + expect(msg).toContain("Required branch: `issue/174-canonical-checkout-contract`"); + expect(msg).toContain("Base branch: `devclaw-local-dev`"); + expect(msg).toContain("Allowed derived validation lane"); + }); +}); diff --git a/lib/dispatch/message-builder.ts b/lib/dispatch/message-builder.ts index 1524f6d2..efef5223 100644 --- a/lib/dispatch/message-builder.ts +++ b/lib/dispatch/message-builder.ts @@ -2,8 +2,10 @@ * message-builder.ts — Task message construction for worker sessions. */ import type { ResolvedRoleConfig } from "../config/index.js"; +import type { IssueCheckoutContract } from "../projects/types.js"; import { formatPrContext, formatPrFeedback, type PrContext, type PrFeedback } from "./pr-context.js"; import { getFallbackEmoji } from "../roles/index.js"; +import { renderCheckoutRecoveryGuidance } from "../checkout-contract.js"; /** * Build the task message sent to a worker session. @@ -26,6 +28,7 @@ export function buildTaskMessage(opts: { resolvedRole?: ResolvedRoleConfig; prContext?: PrContext; prFeedback?: PrFeedback; + checkoutContract?: IssueCheckoutContract; /** Pre-formatted attachment context string (from formatAttachmentsForTask) */ attachmentContext?: string; }): string { @@ -68,8 +71,36 @@ export function buildTaskMessage(opts: { } if (opts.prContext) parts.push(...formatPrContext(opts.prContext)); + if (opts.checkoutContract) { + const contract = opts.checkoutContract; + parts.push( + ``, + `## Canonical Checkout Contract`, + `- Mode: \`${contract.mode}\``, + `- Required worktree: \`${contract.canonicalWorktreePath}\``, + `- Required branch: \`${contract.canonicalBranch}\``, + `- Base branch: \`${contract.baseBranch}\``, + `- Base worktree: \`${contract.baseWorktreePath}\``, + `- Required cleanliness: \`${contract.requiredCleanliness}\``, + `- Contract status: \`${contract.status}\``, + `- Required implementation lane: canonical checkout above`, + `- Allowed derived validation lane: ad-hoc temp/review validation is fine only after preserving the canonical checkout identity`, + ); + if (contract.lastVerifiedProvenance) { + parts.push( + `- Last verified path: \`${contract.lastVerifiedProvenance.path}\``, + `- Last verified branch: \`${contract.lastVerifiedProvenance.branch ?? "(missing)"}\``, + `- Last verified HEAD: \`${contract.lastVerifiedProvenance.headSha ?? "(missing)"}\``, + `- Last verified clean: \`${String(contract.lastVerifiedProvenance.clean)}\``, + ); + if (contract.lastVerifiedProvenance.details) { + parts.push(`- Verification note: ${contract.lastVerifiedProvenance.details}`); + } + } + parts.push(...renderCheckoutRecoveryGuidance(contract)); + } if (opts.prFeedback) { - parts.push(...formatPrFeedback(opts.prFeedback, baseBranch)); + parts.push(...formatPrFeedback(opts.prFeedback, baseBranch, opts.checkoutContract)); // Defensive warning if branch name is missing (shouldn't happen in practice) if (!opts.prFeedback.branchName && opts.prFeedback.reason === "merge_conflict") { @@ -126,6 +157,7 @@ export function buildConflictFixMessage(opts: { baseBranch: string; resolvedRole?: ResolvedRoleConfig; prFeedback: PrFeedback; + checkoutContract?: IssueCheckoutContract; }): string { const { projectName, channelId, role, issueId, issueTitle, @@ -143,7 +175,17 @@ export function buildConflictFixMessage(opts: { `> Do NOT re-implement the feature or make other changes.`, ]; - parts.push(...formatPrFeedback(prFeedback, baseBranch)); + if (opts.checkoutContract) { + parts.push( + ``, + `## Canonical Checkout Contract`, + `- Required worktree: \`${opts.checkoutContract.canonicalWorktreePath}\``, + `- Required branch: \`${opts.checkoutContract.canonicalBranch}\``, + `- Base branch: \`${opts.checkoutContract.baseBranch}\``, + ); + } + + parts.push(...formatPrFeedback(prFeedback, baseBranch, opts.checkoutContract)); parts.push( ``, diff --git a/lib/dispatch/pr-context.test.ts b/lib/dispatch/pr-context.test.ts index fa9d24a3..023e8629 100644 --- a/lib/dispatch/pr-context.test.ts +++ b/lib/dispatch/pr-context.test.ts @@ -5,7 +5,7 @@ describe("formatPrFeedback", () => { it("returns empty array when no comments", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/123", - branchName: "feature/123-test", + branchName: "issue/123-test", reason: "merge_conflict", comments: [], }; @@ -16,7 +16,7 @@ describe("formatPrFeedback", () => { it("includes branch name in conflict resolution instructions", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/123", - branchName: "feature/456-test", + branchName: "issue/456-test", reason: "merge_conflict", comments: [ { @@ -30,10 +30,10 @@ describe("formatPrFeedback", () => { const result = formatPrFeedback(feedback, "main"); const text = result.join("\n"); - expect(text).toContain("feature/456-test"); - expect(text).toContain("🔹 Branch: `feature/456-test`"); - expect(text).toContain("git checkout feature/456-test"); - expect(text).toContain("git push --force-with-lease origin feature/456-test"); + expect(text).toContain("issue/456-test"); + expect(text).toContain("🔹 Branch: `issue/456-test`"); + expect(text).toContain("git checkout issue/456-test"); + expect(text).toContain("git push --force-with-lease origin issue/456-test"); }); it("uses fallback branch name when not provided", () => { @@ -59,7 +59,7 @@ describe("formatPrFeedback", () => { it("includes step-by-step instructions for conflict resolution", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/123", - branchName: "feature/123-fix", + branchName: "issue/123-fix", reason: "merge_conflict", comments: [ { @@ -89,7 +89,7 @@ describe("formatPrFeedback", () => { it("correctly formats changes_requested feedback", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/456", - branchName: "feature/789-feature", + branchName: "issue/789-feature", reason: "changes_requested", comments: [ { @@ -112,7 +112,7 @@ describe("formatPrFeedback", () => { it("includes comment location information when available", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/123", - branchName: "feature/456-test", + branchName: "issue/456-test", reason: "changes_requested", comments: [ { @@ -134,7 +134,7 @@ describe("formatPrFeedback", () => { it("uses correct base branch in rebase command", () => { const feedback: PrFeedback = { url: "https://github.com/user/repo/pull/123", - branchName: "feature/test", + branchName: "issue/test", reason: "merge_conflict", comments: [ { diff --git a/lib/dispatch/pr-context.ts b/lib/dispatch/pr-context.ts index 9d2a5263..231ec721 100644 --- a/lib/dispatch/pr-context.ts +++ b/lib/dispatch/pr-context.ts @@ -8,6 +8,7 @@ */ import type { IssueProvider } from "../providers/provider.js"; import { PrState } from "../providers/provider.js"; +import type { IssueCheckoutContract } from "../projects/types.js"; // --------------------------------------------------------------------------- // Types @@ -109,7 +110,7 @@ export function formatPrContext(prContext: PrContext): string[] { /** * Format PR review feedback section for task message. */ -export function formatPrFeedback(prFeedback: PrFeedback, baseBranch: string): string[] { +export function formatPrFeedback(prFeedback: PrFeedback, baseBranch: string, checkoutContract?: IssueCheckoutContract): string[] { if (prFeedback.comments.length === 0) return []; const reasonLabel = prFeedback.reason === "merge_conflict" @@ -130,7 +131,8 @@ export function formatPrFeedback(prFeedback: PrFeedback, baseBranch: string): st } if (prFeedback.reason === "merge_conflict") { - const branchName = prFeedback.branchName || "your-branch"; + const branchName = prFeedback.branchName || checkoutContract?.canonicalBranch || "your-branch"; + const worktreePath = checkoutContract?.canonicalWorktreePath; parts.push( ``, `### Conflict Resolution Instructions`, @@ -146,15 +148,15 @@ export function formatPrFeedback(prFeedback: PrFeedback, baseBranch: string): st ` \`\`\`bash`, ` git fetch origin ${branchName}`, ` git checkout ${branchName}`, - ` # Or if you already have a worktree:`, - ` cd "${branchName}"`, + ` # Or if you already have the canonical worktree:`, + ` cd "${worktreePath ?? branchName}"`, ` git fetch origin`, ` git reset --hard origin/${branchName}`, ` \`\`\``, ``, `2. Rebase onto \`${baseBranch}\`:`, ` \`\`\`bash`, - ` git rebase ${baseBranch}`, + ` git rebase ${checkoutContract?.baseBranch ?? baseBranch}`, ` \`\`\``, ``, `3. Resolve any conflicts:`, diff --git a/lib/projects/migrations.ts b/lib/projects/migrations.ts index ced82bba..9baf4709 100644 --- a/lib/projects/migrations.ts +++ b/lib/projects/migrations.ts @@ -272,6 +272,11 @@ export function migrateProject(project: Project): boolean { changed = true; } + if (!project.issueCheckouts || typeof project.issueCheckouts !== "object") { + project.issueCheckouts = {}; + changed = true; + } + // Remove roleExecution from state — now lives in workflow.yaml if ((raw as Record).roleExecution !== undefined) { delete (raw as Record).roleExecution; diff --git a/lib/projects/mutations.ts b/lib/projects/mutations.ts index 9b79d91b..52ebb77b 100644 --- a/lib/projects/mutations.ts +++ b/lib/projects/mutations.ts @@ -1,7 +1,7 @@ /** * projects/mutations.ts — State mutations for project worker slots. */ -import type { SlotState, RoleWorkerState, Project, ProjectsData } from "./types.js"; +import type { SlotState, RoleWorkerState, Project, ProjectsData, IssueCheckoutContract } from "./types.js"; import { acquireLock, releaseLock, readProjects, writeProjects, resolveProjectSlug } from "./io.js"; import { emptySlot, findFreeSlot, findSlotByIssue } from "./slots.js"; @@ -122,6 +122,26 @@ export async function activateWorker( * Finds the slot by issueId (searches across all levels), or by explicit level+slotIndex. * Accepts slug or channelId (dual-mode). */ +export async function upsertIssueCheckout( + workspaceDir: string, + slugOrChannelId: string, + contract: IssueCheckoutContract, +): Promise { + await acquireLock(workspaceDir); + try { + const data = await readProjects(workspaceDir); + const slug = resolveProjectSlug(data, slugOrChannelId); + if (!slug) throw new Error(`Project not found for slug or channelId: ${slugOrChannelId}`); + const project = data.projects[slug]!; + if (!project.issueCheckouts) project.issueCheckouts = {}; + project.issueCheckouts[String(contract.issueId)] = contract; + await writeProjects(workspaceDir, data); + return data; + } finally { + await releaseLock(workspaceDir); + } +} + export async function deactivateWorker( workspaceDir: string, slugOrChannelId: string, diff --git a/lib/projects/schema-migration.ts b/lib/projects/schema-migration.ts index a0c57713..83831b9e 100644 --- a/lib/projects/schema-migration.ts +++ b/lib/projects/schema-migration.ts @@ -117,6 +117,7 @@ export async function migrateLegacySchema(data: any, runCommand?: RunCommand): P channels, provider: firstProj.provider, workers: mergedWorkers, + issueCheckouts: {}, }; } diff --git a/lib/projects/types.ts b/lib/projects/types.ts index 24302edd..c6acda86 100644 --- a/lib/projects/types.ts +++ b/lib/projects/types.ts @@ -40,6 +40,35 @@ export type Channel = { messageThreadId?: number; }; +export type CheckoutMode = "issue" | "review" | "pr" | "live" | "release"; +export type CheckoutStatus = "planned" | "created" | "adopted" | "missing" | "dirty" | "mismatched" | "verified"; + +export type CheckoutProvenance = { + verifiedAt: string; + path: string; + branch: string | null; + headSha: string | null; + clean: boolean; + status: CheckoutStatus; + details?: string; +}; + +export type IssueCheckoutContract = { + issueId: number; + issueTitle?: string; + mode: CheckoutMode; + repoPath: string; + canonicalBranch: string; + canonicalWorktreePath: string; + baseBranch: string; + baseWorktreePath: string; + targetRef: string; + targetSha: string | null; + requiredCleanliness: "clean" | "allow-derived-dirty"; + status: CheckoutStatus; + lastVerifiedProvenance?: CheckoutProvenance; +}; + /** * Project configuration in the new project-first schema. */ @@ -58,6 +87,8 @@ export type Project = { provider?: "github" | "gitlab"; /** Worker state per role (developer, tester, architect, or custom roles). Shared across all channels. */ workers: Record; + /** Persisted per-issue checkout contract state. */ + issueCheckouts?: Record; }; /** diff --git a/lib/providers/github.ts b/lib/providers/github.ts index cc7d4b67..0ed11f1b 100644 --- a/lib/providers/github.ts +++ b/lib/providers/github.ts @@ -343,8 +343,8 @@ export class GitHubProvider implements IssueProvider { async getPrStatus(issueId: number): Promise { // Check open PRs first — include mergeable for conflict detection - type OpenPr = { title: string; body: string; headRefName: string; url: string; number: number; reviewDecision: string; mergeable: string }; - const open = await this.findPrsForIssue(issueId, "open", "title,body,headRefName,url,number,reviewDecision,mergeable"); + type OpenPr = { title: string; body: string; headRefName: string; baseRefName: string; url: string; number: number; reviewDecision: string; mergeable: string }; + const open = await this.findPrsForIssue(issueId, "open", "title,body,headRefName,baseRefName,url,number,reviewDecision,mergeable"); if (open.length > 0) { const pr = open[0]; let state: PrState; @@ -375,15 +375,15 @@ export class GitHubProvider implements IssueProvider { : pr.mergeable === "MERGEABLE" ? true : undefined; // UNKNOWN or missing — don't assume - return { state, url: pr.url, title: pr.title, sourceBranch: pr.headRefName, mergeable }; + return { state, url: pr.url, title: pr.title, sourceBranch: pr.headRefName, targetBranch: pr.baseRefName, mergeable }; } // Check merged PRs — also fetch reviewDecision to detect approved-then-merged vs self-merged. - type MergedPr = { title: string; body: string; headRefName: string; url: string; reviewDecision: string | null }; - const merged = await this.findPrsForIssue(issueId, "merged", "title,body,headRefName,url,reviewDecision"); + type MergedPr = { title: string; body: string; headRefName: string; baseRefName: string; url: string; reviewDecision: string | null }; + const merged = await this.findPrsForIssue(issueId, "merged", "title,body,headRefName,baseRefName,url,reviewDecision"); if (merged.length > 0) { const pr = merged[0]; const state = pr.reviewDecision === "APPROVED" ? PrState.APPROVED : PrState.MERGED; - return { state, url: pr.url, title: pr.title, sourceBranch: pr.headRefName }; + return { state, url: pr.url, title: pr.title, sourceBranch: pr.headRefName, targetBranch: pr.baseRefName }; } // Check for closed-without-merge PRs. url: non-null = PR was explicitly closed; // url: null = no PR has ever been created for this issue. diff --git a/lib/providers/gitlab.ts b/lib/providers/gitlab.ts index 16ff17b6..2da1b2f0 100644 --- a/lib/providers/gitlab.ts +++ b/lib/providers/gitlab.ts @@ -27,6 +27,7 @@ type GitLabMR = { web_url: string; state: string; source_branch?: string; + target_branch?: string; merged_at: string | null; approved_by?: Array; author?: { username: string }; @@ -220,15 +221,15 @@ export class GitLabProvider implements IssueProvider { // Detect merge conflicts const mergeable = await this.isMrMergeable(open.iid); - return { state, url: open.web_url, title: open.title, sourceBranch: open.source_branch, mergeable }; + return { state, url: open.web_url, title: open.title, sourceBranch: open.source_branch, targetBranch: open.target_branch, mergeable }; } // Check merged MRs const merged = mrs.find((mr) => mr.state === "merged"); - if (merged) return { state: PrState.MERGED, url: merged.web_url, title: merged.title, sourceBranch: merged.source_branch }; + if (merged) return { state: PrState.MERGED, url: merged.web_url, title: merged.title, sourceBranch: merged.source_branch, targetBranch: merged.target_branch }; // Check for closed-without-merge MRs. url: non-null = MR was explicitly closed; // url: null = no MR has ever been created for this issue. const closed = mrs.find((mr) => mr.state === "closed"); - if (closed) return { state: PrState.CLOSED, url: closed.web_url, title: closed.title, sourceBranch: closed.source_branch }; + if (closed) return { state: PrState.CLOSED, url: closed.web_url, title: closed.title, sourceBranch: closed.source_branch, targetBranch: closed.target_branch }; return { state: PrState.CLOSED, url: null }; } diff --git a/lib/providers/provider.ts b/lib/providers/provider.ts index bd0593bb..2e8bd927 100644 --- a/lib/providers/provider.ts +++ b/lib/providers/provider.ts @@ -48,6 +48,8 @@ export type PrStatus = { title?: string; /** Source branch name (e.g. "feature/7-blog-cms"). */ sourceBranch?: string; + /** Target/base branch name for the PR/MR. */ + targetBranch?: string; /** false = has merge conflicts. undefined = unknown or not applicable. */ mergeable?: boolean; }; diff --git a/lib/tools/admin/project-status.ts b/lib/tools/admin/project-status.ts index 16631e02..4983d9ae 100644 --- a/lib/tools/admin/project-status.ts +++ b/lib/tools/admin/project-status.ts @@ -89,6 +89,17 @@ export function createProjectStatusTool(ctx: PluginContext) { .join(" → "), }; + const issueCheckouts = Object.values(project.issueCheckouts ?? {}).map((contract) => ({ + issueId: contract.issueId, + mode: contract.mode, + status: contract.status, + canonicalBranch: contract.canonicalBranch, + canonicalWorktreePath: contract.canonicalWorktreePath, + baseBranch: contract.baseBranch, + verifiedAt: contract.lastVerifiedProvenance?.verifiedAt ?? null, + clean: contract.lastVerifiedProvenance?.clean ?? null, + })); + return jsonResult({ success: true, instanceName, @@ -104,6 +115,10 @@ export function createProjectStatusTool(ctx: PluginContext) { }, workflow: workflowSummary, workers, + checkoutContracts: { + count: issueCheckouts.length, + items: issueCheckouts, + }, }); }, }); diff --git a/lib/tools/worker/work-finish.test.ts b/lib/tools/worker/work-finish.test.ts index 318010f3..c70edebb 100644 --- a/lib/tools/worker/work-finish.test.ts +++ b/lib/tools/worker/work-finish.test.ts @@ -11,23 +11,16 @@ */ import { describe, it, before, after } from "node:test"; import assert from "node:assert"; -import { mkdtemp, writeFile, readFile } from "node:fs/promises"; +import { mkdtemp, writeFile, readFile, mkdir } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { rmdir } from "node:fs/promises"; +import { rm } from "node:fs/promises"; // Helper to create a mock audit log with a merge_conflict transition async function createMockAuditLog(workspaceDir: string, issueId: number, hasMergeConflict: boolean): Promise { const logDir = join(workspaceDir, "devclaw", "log"); - - // Ensure directory exists - try { - await writeFile(join(workspaceDir, "devclaw", "placeholder"), ""); - } catch { - // ignore - } - - const auditPath = join(workspaceDir, "devclaw", "log", "audit.log"); + await mkdir(logDir, { recursive: true }); + const auditPath = join(logDir, "audit.log"); const entries = []; // Add some dummy entries @@ -74,7 +67,7 @@ describe("work_finish: PR validation and conflict resolution", () => { after(async () => { // Clean up try { - await rmdir(tempDir, { recursive: true }); + await rm(tempDir, { recursive: true, force: true }); } catch { // ignore } @@ -143,6 +136,7 @@ describe("work_finish: PR validation and conflict resolution", () => { it("should skip malformed JSON lines in audit log", async () => { const auditPath = join(tempDir, "devclaw", "log", "audit.log"); + await mkdir(join(tempDir, "devclaw", "log"), { recursive: true }); const entries = [ JSON.stringify({ event: "valid", issueId: 999 }), "{ invalid json", @@ -243,7 +237,7 @@ describe("work_finish: PR validation and conflict resolution", () => { it("should handle non-Error exceptions gracefully", () => { // Test that non-Error objects don't cause issues - const notAnError = "some string"; + const notAnError: unknown = "some string"; const shouldRethrow = notAnError instanceof Error && diff --git a/lib/tools/worker/work-finish.ts b/lib/tools/worker/work-finish.ts index a3789ae0..363a8959 100644 --- a/lib/tools/worker/work-finish.ts +++ b/lib/tools/worker/work-finish.ts @@ -12,13 +12,15 @@ import { readFile } from "node:fs/promises"; import { join } from "node:path"; import type { ToolContext } from "../../types.js"; import type { PluginContext, RunCommand } from "../../context.js"; -import { getRoleWorker, resolveRepoPath, findSlotByIssue } from "../../projects/index.js"; +import { getRoleWorker, resolveRepoPath, findSlotByIssue, upsertIssueCheckout } from "../../projects/index.js"; import { executeCompletion, getRule } from "../../services/pipeline.js"; import { log as auditLog } from "../../audit.js"; import { DATA_DIR } from "../../setup/migrate-layout.js"; import { requireWorkspaceDir, resolveChannelId, resolveProject, resolveProvider } from "../helpers.js"; import { getAllRoleIds, isValidResult, getCompletionResults } from "../../roles/index.js"; import { loadWorkflow } from "../../workflow/index.js"; +import { inspectCheckoutContract, getImplementationBaseBranch } from "../../checkout-contract.js"; +import type { IssueCheckoutContract } from "../../projects/types.js"; /** * Get the current git branch name. @@ -84,6 +86,8 @@ async function validatePrExistsForDeveloper( runCommand: RunCommand, workspaceDir: string, projectSlug: string, + expectedBaseBranch: string, + expectedSourceBranch?: string, ): Promise { try { const prStatus = await provider.getPrStatus(issueId); @@ -112,6 +116,24 @@ async function validatePrExistsForDeveloper( // getPrStatus locates PRs via the issue tracker's linked-PR API, so any // non-null url already implies the PR references the issue. + if (expectedSourceBranch && prStatus.sourceBranch && prStatus.sourceBranch !== expectedSourceBranch) { + throw new Error( + `Cannot mark work_finish(done) with the wrong PR source branch.\n\n` + + `✗ Expected source branch: ${expectedSourceBranch}\n` + + `✗ PR source branch: ${prStatus.sourceBranch}\n\n` + + `Update the canonical implementation branch, or open the correct PR from ${expectedSourceBranch}.`, + ); + } + + if (prStatus.targetBranch && prStatus.targetBranch !== expectedBaseBranch) { + throw new Error( + `Cannot mark work_finish(done) with the wrong PR base branch.\n\n` + + `✗ Expected base branch: ${expectedBaseBranch}\n` + + `✗ PR base branch: ${prStatus.targetBranch}\n\n` + + `Open or retarget the implementation PR into ${expectedBaseBranch}, then call work_finish again.`, + ); + } + // Mark PR as "seen" (with eyes emoji) if not already marked. // This helps distinguish system-created PRs from human responses. // Best-effort — don't block completion if this fails. @@ -175,6 +197,38 @@ async function validatePrExistsForDeveloper( } } +async function validateCheckoutContractForCompletion( + contract: IssueCheckoutContract | undefined, + runCommand: RunCommand, + workspaceDir: string, + projectSlug: string, + issueId: number, +): Promise { + if (!contract) return undefined; + const provenance = await inspectCheckoutContract(contract, runCommand); + const updated = { ...contract, status: provenance.status, lastVerifiedProvenance: provenance, targetSha: provenance.headSha }; + if (provenance.status !== "verified") { + await auditLog(workspaceDir, "work_finish_rejected", { + project: projectSlug, + issue: issueId, + reason: "checkout_contract_mismatch", + checkoutStatus: provenance.status, + checkoutPath: provenance.path, + checkoutBranch: provenance.branch, + checkoutDetails: provenance.details, + }); + throw new Error( + `Cannot complete work_finish until the canonical checkout contract is healthy.\n\n` + + `✗ Status: ${provenance.status}\n` + + `✗ Path: ${provenance.path}\n` + + `✗ Branch: ${provenance.branch ?? "(missing)"}\n` + + `${provenance.details ? `✗ Details: ${provenance.details}\n` : ""}\n` + + `Repair the canonical issue worktree if it is safely recoverable. Otherwise call work_finish with result "blocked" and explain the checkout drift.` + ); + } + return updated; +} + export function createWorkFinishTool(ctx: PluginContext) { return (toolCtx: ToolContext) => ({ name: "work_finish", @@ -267,10 +321,27 @@ export function createWorkFinishTool(ctx: PluginContext) { const repoPath = resolveRepoPath(project.repo); const pluginConfig = ctx.pluginConfig; + const completionNeedsVerifiedCheckout = ["done", "approve", "reject", "pass", "fail"].includes(result); + const storedContract = project.issueCheckouts?.[String(issueId)]; + const validatedContract = completionNeedsVerifiedCheckout + ? await validateCheckoutContractForCompletion(storedContract, ctx.runCommand, workspaceDir, project.slug, issueId) + : storedContract; + if (validatedContract) { + await upsertIssueCheckout(workspaceDir, project.slug, validatedContract); + } // For developers marking work as done, validate that a PR exists if (role === "developer" && result === "done") { - await validatePrExistsForDeveloper(issueId, repoPath, provider, ctx.runCommand, workspaceDir, project.slug); + await validatePrExistsForDeveloper( + issueId, + repoPath, + provider, + ctx.runCommand, + workspaceDir, + project.slug, + validatedContract?.baseBranch ?? getImplementationBaseBranch(project), + validatedContract?.canonicalBranch, + ); } const completion = await executeCompletion({