diff --git a/lib/config/loader.ts b/lib/config/loader.ts index f2d356f..979e2f2 100644 --- a/lib/config/loader.ts +++ b/lib/config/loader.ts @@ -138,6 +138,7 @@ function resolve(config: DevClawConfig): ResolvedConfig { const workflow: WorkflowConfig = { initial: config.workflow?.initial ?? DEFAULT_WORKFLOW.initial, + reviewPolicy: config.workflow?.reviewPolicy ?? DEFAULT_WORKFLOW.reviewPolicy, states: { ...DEFAULT_WORKFLOW.states, ...config.workflow?.states }, }; diff --git a/lib/config/merge.ts b/lib/config/merge.ts index 8a2a090..0306728 100644 --- a/lib/config/merge.ts +++ b/lib/config/merge.ts @@ -46,6 +46,7 @@ export function mergeConfig( if (base.workflow || overlay.workflow) { merged.workflow = { initial: overlay.workflow?.initial ?? base.workflow?.initial, + reviewPolicy: overlay.workflow?.reviewPolicy ?? base.workflow?.reviewPolicy, states: { ...base.workflow?.states, ...overlay.workflow?.states, diff --git a/lib/config/schema.ts b/lib/config/schema.ts index aa6449d..3aea2d8 100644 --- a/lib/config/schema.ts +++ b/lib/config/schema.ts @@ -32,6 +32,7 @@ const StateConfigSchema = z.object({ const WorkflowConfigSchema = z.object({ initial: z.string(), + reviewPolicy: z.enum(["human", "agent", "auto"]).optional(), states: z.record(z.string(), StateConfigSchema), }); diff --git a/lib/dispatch.ts b/lib/dispatch.ts index 6aa94c3..bb5807b 100644 --- a/lib/dispatch.ts +++ b/lib/dispatch.ts @@ -16,6 +16,7 @@ import { import { resolveModel, getFallbackEmoji } from "./roles/index.js"; import { notify, getNotificationConfig } from "./notify.js"; import { loadConfig, type ResolvedRoleConfig } from "./config/index.js"; +import { ReviewPolicy, resolveReviewRouting } from "./workflow.js"; export type DispatchOpts = { workspaceDir: string; @@ -74,6 +75,8 @@ export function buildTaskMessage(opts: { groupId: string; comments?: Array<{ author: string; body: string; created_at: string }>; resolvedRole?: ResolvedRoleConfig; + /** PR context for reviewer role (URL + diff) */ + prContext?: { url: string; diff?: string }; }): string { const { projectName, role, issueId, issueTitle, @@ -101,6 +104,19 @@ export function buildTaskMessage(opts: { } } + // Include PR context for reviewer role + if (opts.prContext) { + parts.push(``, `## Pull Request`, `šŸ”— ${opts.prContext.url}`); + if (opts.prContext.diff) { + // Truncate large diffs to avoid bloating context + const maxDiffLen = 50_000; + const diff = opts.prContext.diff.length > maxDiffLen + ? opts.prContext.diff.slice(0, maxDiffLen) + "\n... (diff truncated, see PR for full changes)" + : opts.prContext.diff; + parts.push(``, `### Diff`, "```diff", diff, "```"); + } + } + parts.push( ``, `Repo: ${repo} | Branch: ${baseBranch} | ${issueUrl}`, @@ -163,16 +179,52 @@ export async function dispatchTask( // Fetch comments to include in task context const comments = await provider.listComments(issueId); + // Fetch PR context for reviewer role + let prContext: { url: string; diff?: string } | undefined; + if (role === "reviewer") { + try { + const prStatus = await provider.getPrStatus(issueId); + if (prStatus.url) { + const diff = await provider.getPrDiff(issueId) ?? undefined; + prContext = { url: prStatus.url, diff }; + } + } catch { + // Best-effort — reviewer can still work from issue context + } + } + const taskMessage = buildTaskMessage({ projectName: project.name, role, issueId, issueTitle, issueDescription, issueUrl, repo: project.repo, baseBranch: project.baseBranch, groupId, - comments, resolvedRole, + comments, resolvedRole, prContext, }); // Step 1: Transition label (this is the commitment point) await transitionLabel(issueId, fromLabel, toLabel); + // Step 1b: Apply role:level label (best-effort — failure must not abort dispatch) + try { + const issue = await provider.getIssue(issueId); + const oldRoleLabels = issue.labels.filter((l) => l.startsWith(`${role}:`)); + if (oldRoleLabels.length > 0) { + await provider.removeLabels(issueId, oldRoleLabels); + } + await provider.addLabel(issueId, `${role}:${level}`); + + // Step 1c: Apply review routing label when developer dispatched (best-effort) + if (role === "developer") { + const reviewLabel = resolveReviewRouting( + resolvedConfig.workflow.reviewPolicy ?? ReviewPolicy.AUTO, level, + ); + const oldRouting = issue.labels.filter((l) => l.startsWith("review:")); + if (oldRouting.length > 0) await provider.removeLabels(issueId, oldRouting); + await provider.addLabel(issueId, reviewLabel); + } + } catch { + // Best-effort — label failure must not abort dispatch + } + // Step 2: Send notification early (before session dispatch which can timeout) // This ensures users see the notification even if gateway is slow const notifyConfig = getNotificationConfig(pluginConfig); diff --git a/lib/notify.ts b/lib/notify.ts index 40f62f5..72e5f14 100644 --- a/lib/notify.ts +++ b/lib/notify.ts @@ -6,6 +6,7 @@ * Event types: * - workerStart: Worker spawned/resumed for a task (→ project group) * - workerComplete: Worker completed task (→ project group) + * - reviewNeeded: Issue needs review — human or agent (→ project group) */ import { log as auditLog } from "./audit.js"; import type { PluginRuntime } from "openclaw/plugin-sdk"; @@ -35,6 +36,16 @@ export type NotifyEvent = result: "done" | "pass" | "fail" | "refine" | "blocked"; summary?: string; nextState?: string; + } + | { + type: "reviewNeeded"; + project: string; + groupId: string; + issueId: number; + issueUrl: string; + issueTitle: string; + routing: "human" | "agent"; + prUrl?: string; }; /** @@ -74,6 +85,15 @@ function buildMessage(event: NotifyEvent): string { msg += `\nšŸ”— ${event.issueUrl}`; return msg; } + + case "reviewNeeded": { + const icon = event.routing === "human" ? "šŸ‘€" : "šŸ¤–"; + const who = event.routing === "human" ? "Human review needed" : "Agent review queued"; + let msg = `${icon} ${who} for #${event.issueId}: ${event.issueTitle}`; + if (event.prUrl) msg += `\nšŸ”— PR: ${event.prUrl}`; + msg += `\nšŸ“‹ Issue: ${event.issueUrl}`; + return msg; + } } } diff --git a/lib/providers/github.ts b/lib/providers/github.ts index d3d1adc..e71a974 100644 --- a/lib/providers/github.ts +++ b/lib/providers/github.ts @@ -50,6 +50,34 @@ export class GitHubProvider implements IssueProvider { }); } + /** + * Find PRs associated with an issue. + * Primary: match by head branch pattern (fix/123-, feature/123-, etc.) + * Fallback: word-boundary match on #123 in title/body. + */ + private async findPrsForIssue( + issueId: number, + state: "open" | "merged" | "all", + fields: string, + ): Promise { + try { + const args = ["pr", "list", "--json", fields, "--limit", "50"]; + if (state !== "all") args.push("--state", state); + const raw = await this.gh(args); + if (!raw) return []; + const prs = JSON.parse(raw) as T[]; + const branchPat = new RegExp(`^(?:fix|feature|chore|bugfix|hotfix)/${issueId}-`); + const titlePat = new RegExp(`\\b#${issueId}\\b`); + + // Primary: match by branch name + const byBranch = prs.filter((pr) => pr.headRefName && branchPat.test(pr.headRefName)); + if (byBranch.length > 0) return byBranch; + + // Fallback: word-boundary match in title/body + return prs.filter((pr) => titlePat.test(pr.title) || titlePat.test(pr.body ?? "")); + } catch { return []; } + } + async ensureLabel(name: string, color: string): Promise { try { await this.gh(["label", "create", name, "--color", color.replace(/^#/, "")]); } catch (err) { if (!(err as Error).message?.includes("already exists")) throw err; } @@ -102,6 +130,17 @@ export class GitHubProvider implements IssueProvider { await this.gh(args); } + async addLabel(issueId: number, label: string): Promise { + await this.gh(["issue", "edit", String(issueId), "--add-label", label]); + } + + async removeLabels(issueId: number, labels: string[]): Promise { + if (labels.length === 0) return; + const args = ["issue", "edit", String(issueId)]; + for (const l of labels) args.push("--remove-label", l); + await this.gh(args); + } + async closeIssue(issueId: number): Promise { await this.gh(["issue", "close", String(issueId)]); } async reopenIssue(issueId: number): Promise { await this.gh(["issue", "reopen", String(issueId)]); } @@ -113,52 +152,48 @@ export class GitHubProvider implements IssueProvider { } async hasMergedMR(issueId: number): Promise { - try { - const raw = await this.gh(["pr", "list", "--state", "merged", "--json", "title,body"]); - const prs = JSON.parse(raw) as Array<{ title: string; body: string }>; - const pat = `#${issueId}`; - return prs.some((pr) => pr.title.includes(pat) || (pr.body ?? "").includes(pat)); - } catch { return false; } + const prs = await this.findPrsForIssue(issueId, "merged", "title,body,headRefName"); + return prs.length > 0; } async getMergedMRUrl(issueId: number): Promise { - try { - const raw = await this.gh(["pr", "list", "--state", "merged", "--json", "number,title,body,url,mergedAt", "--limit", "20"]); - const prs = JSON.parse(raw) as Array<{ number: number; title: string; body: string; url: string; mergedAt: string }>; - const pat = `#${issueId}`; - return prs.find((pr) => pr.title.includes(pat) || (pr.body ?? "").includes(pat))?.url ?? null; - } catch { return null; } + type MergedPr = { title: string; body: string; headRefName: string; url: string; mergedAt: string }; + const prs = await this.findPrsForIssue(issueId, "merged", "title,body,headRefName,url,mergedAt"); + if (prs.length === 0) return null; + prs.sort((a, b) => new Date(b.mergedAt).getTime() - new Date(a.mergedAt).getTime()); + return prs[0].url; } async getPrStatus(issueId: number): Promise { - const pat = `#${issueId}`; // Check open PRs first - try { - const raw = await this.gh(["pr", "list", "--state", "open", "--json", "title,body,url,reviewDecision", "--limit", "20"]); - const prs = JSON.parse(raw) as Array<{ title: string; body: string; url: string; reviewDecision: string }>; - const pr = prs.find((p) => p.title.includes(pat) || (p.body ?? "").includes(pat)); - if (pr) { - const state = pr.reviewDecision === "APPROVED" ? PrState.APPROVED : PrState.OPEN; - return { state, url: pr.url }; - } - } catch { /* continue to merged check */ } + type OpenPr = { title: string; body: string; headRefName: string; url: string; reviewDecision: string }; + const open = await this.findPrsForIssue(issueId, "open", "title,body,headRefName,url,reviewDecision"); + if (open.length > 0) { + const pr = open[0]; + const state = pr.reviewDecision === "APPROVED" ? PrState.APPROVED : PrState.OPEN; + return { state, url: pr.url }; + } // Check merged PRs - try { - const raw = await this.gh(["pr", "list", "--state", "merged", "--json", "title,body,url", "--limit", "20"]); - const prs = JSON.parse(raw) as Array<{ title: string; body: string; url: string }>; - const pr = prs.find((p) => p.title.includes(pat) || (p.body ?? "").includes(pat)); - if (pr) return { state: PrState.MERGED, url: pr.url }; - } catch { /* ignore */ } + type MergedPr = { title: string; body: string; headRefName: string; url: string }; + const merged = await this.findPrsForIssue(issueId, "merged", "title,body,headRefName,url"); + if (merged.length > 0) return { state: PrState.MERGED, url: merged[0].url }; return { state: PrState.CLOSED, url: null }; } async mergePr(issueId: number): Promise { - const pat = `#${issueId}`; - const raw = await this.gh(["pr", "list", "--state", "open", "--json", "number,title,body,url", "--limit", "20"]); - const prs = JSON.parse(raw) as Array<{ number: number; title: string; body: string; url: string }>; - const pr = prs.find((p) => p.title.includes(pat) || (p.body ?? "").includes(pat)); - if (!pr) throw new Error(`No open PR found for issue #${issueId}`); - await this.gh(["pr", "merge", pr.url, "--merge"]); + type OpenPr = { title: string; body: string; headRefName: string; url: string }; + const prs = await this.findPrsForIssue(issueId, "open", "title,body,headRefName,url"); + if (prs.length === 0) throw new Error(`No open PR found for issue #${issueId}`); + await this.gh(["pr", "merge", prs[0].url, "--merge"]); + } + + async getPrDiff(issueId: number): Promise { + type OpenPr = { title: string; body: string; headRefName: string; number: number }; + const prs = await this.findPrsForIssue(issueId, "open", "title,body,headRefName,number"); + if (prs.length === 0) return null; + try { + return await this.gh(["pr", "diff", String(prs[0].number)]); + } catch { return null; } } async addComment(issueId: number, body: string): Promise { diff --git a/lib/providers/gitlab.ts b/lib/providers/gitlab.ts index 4288b79..4bc11bc 100644 --- a/lib/providers/gitlab.ts +++ b/lib/providers/gitlab.ts @@ -18,6 +18,16 @@ import { type WorkflowConfig, } from "../workflow.js"; +type GitLabMR = { + iid: number; + title: string; + description: string; + web_url: string; + state: string; + merged_at: string | null; + approved_by?: Array; +}; + export class GitLabProvider implements IssueProvider { private repoPath: string; private workflow: WorkflowConfig; @@ -34,6 +44,15 @@ export class GitLabProvider implements IssueProvider { }); } + /** Get MRs linked to an issue via GitLab's native related_merge_requests API. */ + private async getRelatedMRs(issueId: number): Promise { + try { + const raw = await this.glab(["api", `projects/:id/issues/${issueId}/related_merge_requests`, "--paginate"]); + if (!raw) return []; + return JSON.parse(raw) as GitLabMR[]; + } catch { return []; } + } + async ensureLabel(name: string, color: string): Promise { try { await this.glab(["label", "create", "--name", name, "--color", color]); } catch (err) { const msg = (err as Error).message ?? ""; if (!msg.includes("already exists") && !msg.includes("409")) throw err; } @@ -96,6 +115,17 @@ export class GitLabProvider implements IssueProvider { await this.glab(args); } + async addLabel(issueId: number, label: string): Promise { + await this.glab(["issue", "update", String(issueId), "--label", label]); + } + + async removeLabels(issueId: number, labels: string[]): Promise { + if (labels.length === 0) return; + const args = ["issue", "update", String(issueId)]; + for (const l of labels) args.push("--unlabel", l); + await this.glab(args); + } + async closeIssue(issueId: number): Promise { await this.glab(["issue", "close", String(issueId)]); } async reopenIssue(issueId: number): Promise { await this.glab(["issue", "reopen", String(issueId)]); } @@ -107,55 +137,56 @@ export class GitLabProvider implements IssueProvider { } async hasMergedMR(issueId: number): Promise { - try { - const raw = await this.glab(["mr", "list", "--output", "json", "--state", "merged"]); - const mrs = JSON.parse(raw) as Array<{ title: string; description: string }>; - const pat = `#${issueId}`; - return mrs.some((mr) => mr.title.includes(pat) || (mr.description ?? "").includes(pat)); - } catch { return false; } + const mrs = await this.getRelatedMRs(issueId); + return mrs.some((mr) => mr.state === "merged"); } async getMergedMRUrl(issueId: number): Promise { - try { - const raw = await this.glab(["mr", "list", "--output", "json", "--state", "merged"]); - const mrs = JSON.parse(raw) as Array<{ iid: number; title: string; description: string; web_url: string; merged_at: string }>; - const pat = `#${issueId}`; - const mr = mrs - .filter((mr) => mr.title.includes(pat) || (mr.description ?? "").includes(pat)) - .sort((a, b) => new Date(b.merged_at).getTime() - new Date(a.merged_at).getTime())[0]; - return mr?.web_url ?? null; - } catch { return null; } + const mrs = await this.getRelatedMRs(issueId); + const merged = mrs + .filter((mr) => mr.state === "merged" && mr.merged_at) + .sort((a, b) => new Date(b.merged_at!).getTime() - new Date(a.merged_at!).getTime()); + return merged[0]?.web_url ?? null; } async getPrStatus(issueId: number): Promise { - const pat = `#${issueId}`; + const mrs = await this.getRelatedMRs(issueId); // Check open MRs first - try { - const raw = await this.glab(["mr", "list", "--output", "json", "--state", "opened"]); - const mrs = JSON.parse(raw) as Array<{ title: string; description: string; web_url: string; approved_by?: Array }>; - const mr = mrs.find((m) => m.title.includes(pat) || (m.description ?? "").includes(pat)); - if (mr) { - const state = mr.approved_by && mr.approved_by.length > 0 ? PrState.APPROVED : PrState.OPEN; - return { state, url: mr.web_url }; - } - } catch { /* continue to merged check */ } + const open = mrs.find((mr) => mr.state === "opened"); + if (open) { + // related_merge_requests doesn't populate approved_by — use dedicated approvals endpoint + const approved = await this.isMrApproved(open.iid); + return { state: approved ? PrState.APPROVED : PrState.OPEN, url: open.web_url }; + } // Check merged MRs - try { - const raw = await this.glab(["mr", "list", "--output", "json", "--state", "merged"]); - const mrs = JSON.parse(raw) as Array<{ title: string; description: string; web_url: string }>; - const mr = mrs.find((m) => m.title.includes(pat) || (m.description ?? "").includes(pat)); - if (mr) return { state: PrState.MERGED, url: mr.web_url }; - } catch { /* ignore */ } + const merged = mrs.find((mr) => mr.state === "merged"); + if (merged) return { state: PrState.MERGED, url: merged.web_url }; return { state: PrState.CLOSED, url: null }; } + /** Check if an MR is approved via the dedicated approvals endpoint. */ + private async isMrApproved(mrIid: number): Promise { + try { + const raw = await this.glab(["api", `projects/:id/merge_requests/${mrIid}/approvals`]); + const data = JSON.parse(raw) as { approved?: boolean; approvals_left?: number }; + return data.approved === true || (data.approvals_left ?? 1) === 0; + } catch { return false; } + } + async mergePr(issueId: number): Promise { - const pat = `#${issueId}`; - const raw = await this.glab(["mr", "list", "--output", "json", "--state", "opened"]); - const mrs = JSON.parse(raw) as Array<{ iid: number; title: string; description: string }>; - const mr = mrs.find((m) => m.title.includes(pat) || (m.description ?? "").includes(pat)); - if (!mr) throw new Error(`No open MR found for issue #${issueId}`); - await this.glab(["mr", "merge", String(mr.iid)]); + const mrs = await this.getRelatedMRs(issueId); + const open = mrs.find((mr) => mr.state === "opened"); + if (!open) throw new Error(`No open MR found for issue #${issueId}`); + await this.glab(["mr", "merge", String(open.iid)]); + } + + async getPrDiff(issueId: number): Promise { + const mrs = await this.getRelatedMRs(issueId); + const open = mrs.find((mr) => mr.state === "opened"); + if (!open) return null; + try { + return await this.glab(["mr", "diff", String(open.iid)]); + } catch { return null; } } async addComment(issueId: number, body: string): Promise { diff --git a/lib/providers/provider.ts b/lib/providers/provider.ts index aae7f1e..6439ffb 100644 --- a/lib/providers/provider.ts +++ b/lib/providers/provider.ts @@ -54,6 +54,8 @@ export interface IssueProvider { getIssue(issueId: number): Promise; listComments(issueId: number): Promise; transitionLabel(issueId: number, from: StateLabel, to: StateLabel): Promise; + addLabel(issueId: number, label: string): Promise; + removeLabels(issueId: number, labels: string[]): Promise; closeIssue(issueId: number): Promise; reopenIssue(issueId: number): Promise; hasStateLabel(issue: Issue, expected: StateLabel): boolean; @@ -62,6 +64,7 @@ export interface IssueProvider { getMergedMRUrl(issueId: number): Promise; getPrStatus(issueId: number): Promise; mergePr(issueId: number): Promise; + getPrDiff(issueId: number): Promise; addComment(issueId: number, body: string): Promise; healthCheck(): Promise; } diff --git a/lib/roles/registry.test.ts b/lib/roles/registry.test.ts index f1eb542..67739a6 100644 --- a/lib/roles/registry.test.ts +++ b/lib/roles/registry.test.ts @@ -32,12 +32,14 @@ describe("role registry", () => { assert.ok(ids.includes("developer")); assert.ok(ids.includes("tester")); assert.ok(ids.includes("architect")); + assert.ok(ids.includes("reviewer")); }); it("should validate role IDs", () => { assert.strictEqual(isValidRole("developer"), true); assert.strictEqual(isValidRole("tester"), true); assert.strictEqual(isValidRole("architect"), true); + assert.strictEqual(isValidRole("reviewer"), true); assert.strictEqual(isValidRole("nonexistent"), false); }); @@ -58,6 +60,7 @@ describe("levels", () => { assert.deepStrictEqual([...getLevelsForRole("developer")], ["junior", "medior", "senior"]); assert.deepStrictEqual([...getLevelsForRole("tester")], ["junior", "medior", "senior"]); assert.deepStrictEqual([...getLevelsForRole("architect")], ["junior", "senior"]); + assert.deepStrictEqual([...getLevelsForRole("reviewer")], ["junior", "senior"]); }); it("should return empty for unknown role", () => { @@ -185,17 +188,21 @@ describe("emoji", () => { describe("completion results", () => { it("should return valid results per role", () => { - assert.deepStrictEqual([...getCompletionResults("developer")], ["done", "review", "blocked"]); + assert.deepStrictEqual([...getCompletionResults("developer")], ["done", "blocked"]); assert.deepStrictEqual([...getCompletionResults("tester")], ["pass", "fail", "refine", "blocked"]); assert.deepStrictEqual([...getCompletionResults("architect")], ["done", "blocked"]); + assert.deepStrictEqual([...getCompletionResults("reviewer")], ["approve", "reject", "blocked"]); }); it("should validate results", () => { assert.strictEqual(isValidResult("developer", "done"), true); - assert.strictEqual(isValidResult("developer", "review"), true); assert.strictEqual(isValidResult("developer", "pass"), false); assert.strictEqual(isValidResult("tester", "pass"), true); assert.strictEqual(isValidResult("tester", "done"), false); + assert.strictEqual(isValidResult("reviewer", "approve"), true); + assert.strictEqual(isValidResult("reviewer", "reject"), true); + assert.strictEqual(isValidResult("reviewer", "escalate"), false); + assert.strictEqual(isValidResult("reviewer", "done"), false); }); }); @@ -205,6 +212,7 @@ describe("session key pattern", () => { assert.ok(pattern.includes("developer")); assert.ok(pattern.includes("tester")); assert.ok(pattern.includes("architect")); + assert.ok(pattern.includes("reviewer")); }); it("should work as regex", () => { @@ -213,6 +221,7 @@ describe("session key pattern", () => { assert.ok(regex.test("developer")); assert.ok(regex.test("tester")); assert.ok(regex.test("architect")); + assert.ok(regex.test("reviewer")); assert.ok(!regex.test("nonexistent")); }); }); diff --git a/lib/roles/registry.ts b/lib/roles/registry.ts index 7e016d4..f7deb43 100644 --- a/lib/roles/registry.ts +++ b/lib/roles/registry.ts @@ -30,7 +30,7 @@ export const ROLE_REGISTRY: Record = { senior: "🧠", }, fallbackEmoji: "šŸ”§", - completionResults: ["done", "review", "blocked"], + completionResults: ["done", "blocked"], sessionKeyPattern: "developer", notifications: { onStart: true, onComplete: true }, }, @@ -74,4 +74,23 @@ export const ROLE_REGISTRY: Record = { sessionKeyPattern: "architect", notifications: { onStart: true, onComplete: true }, }, + + reviewer: { + id: "reviewer", + displayName: "REVIEWER", + levels: ["junior", "senior"], + defaultLevel: "junior", + models: { + junior: "anthropic/claude-haiku-4-5", + senior: "anthropic/claude-sonnet-4-5", + }, + emoji: { + junior: "šŸ‘ļø", + senior: "šŸ”¬", + }, + fallbackEmoji: "šŸ‘ļø", + completionResults: ["approve", "reject", "blocked"], + sessionKeyPattern: "reviewer", + notifications: { onStart: true, onComplete: true }, + }, }; diff --git a/lib/services/bootstrap.e2e.test.ts b/lib/services/bootstrap.e2e.test.ts index 0015109..433ac2e 100644 --- a/lib/services/bootstrap.e2e.test.ts +++ b/lib/services/bootstrap.e2e.test.ts @@ -122,16 +122,16 @@ describe("E2E bootstrap — hook injection", () => { // Default developer instructions are scaffolded by ensureDefaultFiles assert.strictEqual(files.length, 1); assert.ok(files[0].content!.includes("DEVELOPER"), "Should contain DEVELOPER heading"); - assert.ok(files[0].content!.includes("work_finish"), "Should reference work_finish"); + assert.ok(files[0].content!.includes("worktree"), "Should reference git worktree workflow"); }); it("should NOT inject anything for unknown custom roles", async () => { h = await createTestHarness({ projectName: "custom-app" }); // Simulate a session key for a custom role that has no prompt file - // This key won't parse because "reviewer" isn't in the role registry + // This key won't parse because "investigator" isn't in the role registry const files = await h.simulateBootstrap( - "agent:main:subagent:custom-app-reviewer-medior", + "agent:main:subagent:custom-app-investigator-medior", ); assert.strictEqual(files.length, 0, "Should not inject files for unknown roles"); diff --git a/lib/services/pipeline.e2e.test.ts b/lib/services/pipeline.e2e.test.ts index cdc600c..adf1d5a 100644 --- a/lib/services/pipeline.e2e.test.ts +++ b/lib/services/pipeline.e2e.test.ts @@ -13,8 +13,9 @@ import assert from "node:assert"; import { createTestHarness, type TestHarness } from "../testing/index.js"; import { dispatchTask } from "../dispatch.js"; import { executeCompletion } from "./pipeline.js"; +import { projectTick } from "./tick.js"; import { reviewPass } from "./review.js"; -import { DEFAULT_WORKFLOW } from "../workflow.js"; +import { DEFAULT_WORKFLOW, ReviewPolicy, type WorkflowConfig } from "../workflow.js"; import { readProjects, getWorker } from "../projects.js"; // --------------------------------------------------------------------------- @@ -147,10 +148,10 @@ describe("E2E pipeline", () => { }); // ========================================================================= - // Completion — developer:done + // Completion — developer:done → To Review (always) // ========================================================================= - describe("executeCompletion — developer:done", () => { + describe("executeCompletion — developer:done → To Review", () => { beforeEach(async () => { h = await createTestHarness({ workers: { @@ -160,9 +161,7 @@ describe("E2E pipeline", () => { h.provider.seedIssue({ iid: 10, title: "Build feature X", labels: ["Doing"] }); }); - it("should transition Doing → To Test, deactivate worker, run gitPull+detectPr actions", async () => { - h.provider.mergedMrUrls.set(10, "https://example.com/mr/5"); - + it("should transition Doing → To Review", async () => { const output = await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, @@ -175,73 +174,90 @@ describe("E2E pipeline", () => { projectName: "test-project", }); - // Label transition - assert.strictEqual(output.labelTransition, "Doing → To Test"); + assert.strictEqual(output.labelTransition, "Doing → To Review"); assert.ok(output.announcement.includes("#10")); - // Issue state const issue = await h.provider.getIssue(10); - assert.ok(issue.labels.includes("To Test"), `Labels: ${issue.labels}`); + assert.ok(issue.labels.includes("To Review"), `Labels: ${issue.labels}`); assert.ok(!issue.labels.includes("Doing")); - // Worker deactivated const data = await readProjects(h.workspaceDir); - const worker = getWorker(data.projects[h.groupId], "developer"); - assert.strictEqual(worker.active, false); - - // PR URL detected - assert.strictEqual(output.prUrl, "https://example.com/mr/5"); - - // gitPull action was executed - const gitCmds = h.commands.commands.filter((c) => c.argv[0] === "git"); - assert.ok(gitCmds.length > 0, "Should have run git pull"); - assert.deepStrictEqual(gitCmds[0].argv, ["git", "pull"]); - - // Issue NOT closed (done goes to To Test, not Done) + assert.strictEqual(getWorker(data.projects[h.groupId], "developer").active, false); assert.strictEqual(output.issueClosed, false); }); }); // ========================================================================= - // Completion — developer:review + // Completion — reviewer:approve / reject // ========================================================================= - describe("executeCompletion — developer:review", () => { + describe("executeCompletion — reviewer", () => { beforeEach(async () => { h = await createTestHarness({ workers: { - developer: { active: true, issueId: "20", level: "senior" }, + reviewer: { active: true, issueId: "25", level: "junior" }, }, }); - h.provider.seedIssue({ iid: 20, title: "Refactor auth", labels: ["Doing"] }); + h.provider.seedIssue({ iid: 25, title: "Review PR", labels: ["Reviewing"] }); }); - it("should transition Doing → In Review, deactivate worker", async () => { + it("reviewer:approve should transition Reviewing → To Test, merge PR", async () => { + h.provider.setPrStatus(25, { state: "open", url: "https://example.com/pr/7" }); + const output = await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, - role: "developer", - result: "review", - issueId: 20, - summary: "PR open for review", - prUrl: "https://example.com/pr/3", + role: "reviewer", + result: "approve", + issueId: 25, + summary: "Code looks good", provider: h.provider, repoPath: "/tmp/test-repo", projectName: "test-project", }); - assert.strictEqual(output.labelTransition, "Doing → In Review"); - assert.ok(output.nextState.includes("review"), `nextState: ${output.nextState}`); + assert.strictEqual(output.labelTransition, "Reviewing → To Test"); + const issue = await h.provider.getIssue(25); + assert.ok(issue.labels.includes("To Test"), `Labels: ${issue.labels}`); - const issue = await h.provider.getIssue(20); - assert.ok(issue.labels.includes("In Review"), `Labels: ${issue.labels}`); + const mergeCalls = h.provider.callsTo("mergePr"); + assert.strictEqual(mergeCalls.length, 1); + }); - // Worker should be deactivated - const data = await readProjects(h.workspaceDir); - assert.strictEqual(getWorker(data.projects[h.groupId], "developer").active, false); + it("reviewer:reject should transition Reviewing → To Improve", async () => { + const output = await executeCompletion({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + role: "reviewer", + result: "reject", + issueId: 25, + summary: "Missing error handling", + provider: h.provider, + repoPath: "/tmp/test-repo", + projectName: "test-project", + }); - // Issue should NOT be closed - assert.strictEqual(output.issueClosed, false); + assert.strictEqual(output.labelTransition, "Reviewing → To Improve"); + const issue = await h.provider.getIssue(25); + assert.ok(issue.labels.includes("To Improve"), `Labels: ${issue.labels}`); + }); + + it("reviewer:blocked should transition Reviewing → Refining", async () => { + const output = await executeCompletion({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + role: "reviewer", + result: "blocked", + issueId: 25, + summary: "Can't determine correctness", + provider: h.provider, + repoPath: "/tmp/test-repo", + projectName: "test-project", + }); + + assert.strictEqual(output.labelTransition, "Reviewing → Refining"); + const issue = await h.provider.getIssue(25); + assert.ok(issue.labels.includes("Refining"), `Labels: ${issue.labels}`); }); }); @@ -362,7 +378,7 @@ describe("E2E pipeline", () => { }); // ========================================================================= - // Review pass + // Review pass — heartbeat polls To Review for human path // ========================================================================= describe("reviewPass", () => { @@ -370,9 +386,8 @@ describe("E2E pipeline", () => { h = await createTestHarness(); }); - it("should auto-merge and transition In Review → To Test when PR is approved", async () => { - // Seed issue in "In Review" state - h.provider.seedIssue({ iid: 60, title: "Feature Y", labels: ["In Review"] }); + it("should auto-merge and transition To Review → To Test when PR is approved", async () => { + h.provider.seedIssue({ iid: 60, title: "Feature Y", labels: ["To Review"] }); h.provider.setPrStatus(60, { state: "approved", url: "https://example.com/pr/10" }); const transitions = await reviewPass({ @@ -385,23 +400,20 @@ describe("E2E pipeline", () => { assert.strictEqual(transitions, 1); - // Issue should now have "To Test" label const issue = await h.provider.getIssue(60); assert.ok(issue.labels.includes("To Test"), `Labels: ${issue.labels}`); - assert.ok(!issue.labels.includes("In Review"), "Should not have In Review"); + assert.ok(!issue.labels.includes("To Review"), "Should not have To Review"); - // mergePr action should have been called const mergeCalls = h.provider.callsTo("mergePr"); assert.strictEqual(mergeCalls.length, 1); assert.strictEqual(mergeCalls[0].args.issueId, 60); - // gitPull action should have been attempted const gitCmds = h.commands.commands.filter((c) => c.argv[0] === "git"); assert.ok(gitCmds.length > 0, "Should have run git pull"); }); it("should NOT transition when PR is still open", async () => { - h.provider.seedIssue({ iid: 61, title: "Feature Z", labels: ["In Review"] }); + h.provider.seedIssue({ iid: 61, title: "Feature Z", labels: ["To Review"] }); h.provider.setPrStatus(61, { state: "open", url: "https://example.com/pr/11" }); const transitions = await reviewPass({ @@ -414,14 +426,13 @@ describe("E2E pipeline", () => { assert.strictEqual(transitions, 0); - // Issue should still have "In Review" const issue = await h.provider.getIssue(61); - assert.ok(issue.labels.includes("In Review")); + assert.ok(issue.labels.includes("To Review")); }); it("should handle multiple review issues in one pass", async () => { - h.provider.seedIssue({ iid: 70, title: "PR A", labels: ["In Review"] }); - h.provider.seedIssue({ iid: 71, title: "PR B", labels: ["In Review"] }); + h.provider.seedIssue({ iid: 70, title: "PR A", labels: ["To Review"] }); + h.provider.seedIssue({ iid: 71, title: "PR B", labels: ["To Review"] }); h.provider.setPrStatus(70, { state: "approved", url: "https://example.com/pr/20" }); h.provider.setPrStatus(71, { state: "approved", url: "https://example.com/pr/21" }); @@ -440,13 +451,12 @@ describe("E2E pipeline", () => { assert.ok(issue70.labels.includes("To Test")); assert.ok(issue71.labels.includes("To Test")); - // Both should have had mergePr called const mergeCalls = h.provider.callsTo("mergePr"); assert.strictEqual(mergeCalls.length, 2); }); - it("should transition In Review → To Improve when merge fails (conflicts)", async () => { - h.provider.seedIssue({ iid: 65, title: "Conflicting PR", labels: ["In Review"] }); + it("should transition To Review → To Improve when merge fails (conflicts)", async () => { + h.provider.seedIssue({ iid: 65, title: "Conflicting PR", labels: ["To Review"] }); h.provider.setPrStatus(65, { state: "approved", url: "https://example.com/pr/15" }); h.provider.mergePrFailures.add(65); @@ -460,17 +470,14 @@ describe("E2E pipeline", () => { assert.strictEqual(transitions, 1); - // Issue should have moved to "To Improve" (not "To Test") const issue = await h.provider.getIssue(65); assert.ok(issue.labels.includes("To Improve"), `Labels: ${issue.labels}`); - assert.ok(!issue.labels.includes("In Review"), "Should not have In Review"); + assert.ok(!issue.labels.includes("To Review"), "Should not have To Review"); assert.ok(!issue.labels.includes("To Test"), "Should NOT have To Test"); - // mergePr should have been attempted const mergeCalls = h.provider.callsTo("mergePr"); assert.strictEqual(mergeCalls.length, 1); - // gitPull should NOT have run (aborted before git pull) const gitCmds = h.commands.commands.filter((c) => c.argv[0] === "git"); assert.strictEqual(gitCmds.length, 0, "Should NOT have run git pull after merge failure"); }); @@ -481,10 +488,10 @@ describe("E2E pipeline", () => { // ========================================================================= describe("full lifecycle", () => { - it("developer:done → tester:pass (direct path)", async () => { + it("developer:done → reviewer:approve → tester:pass (agent review path)", async () => { h = await createTestHarness(); - // 1. Seed issue in To Do + // 1. Seed issue h.provider.seedIssue({ iid: 100, title: "Build dashboard", labels: ["To Do"] }); // 2. Dispatch developer @@ -505,10 +512,7 @@ describe("E2E pipeline", () => { provider: h.provider, }); - let issue = await h.provider.getIssue(100); - assert.ok(issue.labels.includes("Doing")); - - // 3. Developer completes → To Test + // 3. Developer done → To Review await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, @@ -521,17 +525,37 @@ describe("E2E pipeline", () => { projectName: "test-project", }); - issue = await h.provider.getIssue(100); - assert.ok(issue.labels.includes("To Test"), `After dev done: ${issue.labels}`); + let issue = await h.provider.getIssue(100); + assert.ok(issue.labels.includes("To Review"), `After dev done: ${issue.labels}`); - // 4. Simulate tester dispatch (activate worker manually for completion) + // 4. Reviewer dispatched → Reviewing → approve → To Test const { activateWorker } = await import("../projects.js"); + await activateWorker(h.workspaceDir, h.groupId, "reviewer", { + issueId: "100", level: "junior", + }); + await h.provider.transitionLabel(100, "To Review", "Reviewing"); + + await executeCompletion({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + role: "reviewer", + result: "approve", + issueId: 100, + summary: "Code looks good", + provider: h.provider, + repoPath: "/tmp/test-repo", + projectName: "test-project", + }); + + issue = await h.provider.getIssue(100); + assert.ok(issue.labels.includes("To Test"), `After reviewer approve: ${issue.labels}`); + + // 5. Tester passes → Done await activateWorker(h.workspaceDir, h.groupId, "tester", { issueId: "100", level: "medior", }); await h.provider.transitionLabel(100, "To Test", "Testing"); - // 5. Tester passes → Done await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, @@ -549,13 +573,12 @@ describe("E2E pipeline", () => { assert.strictEqual(issue.state, "closed"); }); - it("developer:review → review pass → tester:pass (review path)", async () => { + it("developer:done → human review pass → tester:pass (human review path)", async () => { h = await createTestHarness(); - // 1. Seed issue in To Do h.provider.seedIssue({ iid: 200, title: "Auth refactor", labels: ["To Do"] }); - // 2. Dispatch developer + // 1. Dispatch developer await dispatchTask({ workspaceDir: h.workspaceDir, agentId: "main", @@ -573,12 +596,12 @@ describe("E2E pipeline", () => { provider: h.provider, }); - // 3. Developer finishes with "review" → In Review + // 2. Developer done → To Review (same state regardless of level) await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, role: "developer", - result: "review", + result: "done", issueId: 200, summary: "PR ready for review", prUrl: "https://example.com/pr/50", @@ -588,9 +611,9 @@ describe("E2E pipeline", () => { }); let issue = await h.provider.getIssue(200); - assert.ok(issue.labels.includes("In Review"), `After review: ${issue.labels}`); + assert.ok(issue.labels.includes("To Review"), `After dev done: ${issue.labels}`); - // 4. PR gets approved — review pass picks it up and auto-merges + // 3. Human reviews PR → approved → heartbeat transitions To Review → To Test h.provider.setPrStatus(200, { state: "approved", url: "https://example.com/pr/50" }); const transitions = await reviewPass({ @@ -605,7 +628,7 @@ describe("E2E pipeline", () => { issue = await h.provider.getIssue(200); assert.ok(issue.labels.includes("To Test"), `After review pass: ${issue.labels}`); - // 5. Tester passes → Done + // 4. Tester passes → Done const { activateWorker } = await import("../projects.js"); await activateWorker(h.workspaceDir, h.groupId, "tester", { issueId: "200", level: "medior", @@ -629,7 +652,7 @@ describe("E2E pipeline", () => { assert.strictEqual(issue.state, "closed"); }); - it("developer:done → tester:fail → developer:done → tester:pass (fail cycle)", async () => { + it("developer:done → reviewer:reject → developer:done → reviewer:approve → tester:pass (reject cycle)", async () => { h = await createTestHarness(); h.provider.seedIssue({ iid: 300, title: "Payment flow", labels: ["To Do"] }); @@ -652,7 +675,7 @@ describe("E2E pipeline", () => { provider: h.provider, }); - // 2. Developer done → To Test + // 2. Developer done → To Review await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, @@ -664,31 +687,32 @@ describe("E2E pipeline", () => { projectName: "test-project", }); - // 3. Activate tester + transition - const { activateWorker } = await import("../projects.js"); - await activateWorker(h.workspaceDir, h.groupId, "tester", { - issueId: "300", level: "medior", - }); - await h.provider.transitionLabel(300, "To Test", "Testing"); + let issue = await h.provider.getIssue(300); + assert.ok(issue.labels.includes("To Review"), `After dev done: ${issue.labels}`); + + // 3. Reviewer REJECTS → To Improve + const { activateWorker } = await import("../projects.js"); + await activateWorker(h.workspaceDir, h.groupId, "reviewer", { + issueId: "300", level: "junior", + }); + await h.provider.transitionLabel(300, "To Review", "Reviewing"); - // 4. Tester FAILS → To Improve await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, - role: "tester", - result: "fail", + role: "reviewer", + result: "reject", issueId: 300, - summary: "Validation broken", + summary: "Missing validation", provider: h.provider, repoPath: "/tmp/test-repo", projectName: "test-project", }); - let issue = await h.provider.getIssue(300); - assert.ok(issue.labels.includes("To Improve"), `After fail: ${issue.labels}`); - assert.strictEqual(issue.state, "opened"); // reopened + issue = await h.provider.getIssue(300); + assert.ok(issue.labels.includes("To Improve"), `After reject: ${issue.labels}`); - // 5. Developer picks up again (To Improve → Doing) + // 4. Developer picks up again → fixes → To Review await dispatchTask({ workspaceDir: h.workspaceDir, agentId: "main", @@ -706,7 +730,6 @@ describe("E2E pipeline", () => { provider: h.provider, }); - // 6. Developer fixes it → To Test await executeCompletion({ workspaceDir: h.workspaceDir, groupId: h.groupId, @@ -720,9 +743,30 @@ describe("E2E pipeline", () => { }); issue = await h.provider.getIssue(300); - assert.ok(issue.labels.includes("To Test"), `After fix: ${issue.labels}`); + assert.ok(issue.labels.includes("To Review"), `After fix: ${issue.labels}`); - // 7. Tester passes → Done + // 5. Reviewer approves this time → To Test + await activateWorker(h.workspaceDir, h.groupId, "reviewer", { + issueId: "300", level: "junior", + }); + await h.provider.transitionLabel(300, "To Review", "Reviewing"); + + await executeCompletion({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + role: "reviewer", + result: "approve", + issueId: 300, + summary: "Looks good now", + provider: h.provider, + repoPath: "/tmp/test-repo", + projectName: "test-project", + }); + + issue = await h.provider.getIssue(300); + assert.ok(issue.labels.includes("To Test"), `After approve: ${issue.labels}`); + + // 6. Tester passes → Done await activateWorker(h.workspaceDir, h.groupId, "tester", { issueId: "300", level: "medior", }); @@ -746,6 +790,229 @@ describe("E2E pipeline", () => { }); }); + // ========================================================================= + // Review policy gating — projectTick respects reviewPolicy + // ========================================================================= + + describe("projectTick — reviewPolicy gating", () => { + function workflowWithPolicy(policy: ReviewPolicy): WorkflowConfig { + return { ...DEFAULT_WORKFLOW, reviewPolicy: policy }; + } + + it("reviewPolicy: human should skip reviewer dispatch", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 80, title: "Needs review", labels: ["To Review"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + targetRole: "reviewer", + workflow: workflowWithPolicy(ReviewPolicy.HUMAN), + provider: h.provider, + }); + + assert.strictEqual(result.pickups.length, 0, "Should NOT dispatch reviewer"); + const reviewerSkip = result.skipped.find((s) => s.role === "reviewer"); + assert.ok(reviewerSkip, "Should have skipped reviewer"); + assert.ok(reviewerSkip!.reason.includes("human"), `Skip reason: ${reviewerSkip!.reason}`); + }); + + it("reviewPolicy: agent should dispatch reviewer", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 81, title: "Needs review", labels: ["To Review"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + agentId: "test-agent", + targetRole: "reviewer", + workflow: workflowWithPolicy(ReviewPolicy.AGENT), + provider: h.provider, + }); + + assert.strictEqual(result.pickups.length, 1, "Should dispatch reviewer"); + assert.strictEqual(result.pickups[0].role, "reviewer"); + }); + + it("reviewPolicy: auto should dispatch reviewer for junior-level issues", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 82, title: "Small fix", labels: ["To Review"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + agentId: "test-agent", + targetRole: "reviewer", + workflow: workflowWithPolicy(ReviewPolicy.AUTO), + provider: h.provider, + }); + + // Junior/medior should be dispatched under auto policy + assert.strictEqual(result.pickups.length, 1, "Should dispatch reviewer for non-senior"); + }); + + it("reviewPolicy: auto should skip reviewer for senior-level issues (review:human label)", async () => { + h = await createTestHarness(); + // dispatch applies review:human for senior developers (via resolveReviewRouting) + h.provider.seedIssue({ iid: 83, title: "Architecture rework", labels: ["To Review", "developer:senior", "review:human"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + targetRole: "reviewer", + workflow: workflowWithPolicy(ReviewPolicy.AUTO), + provider: h.provider, + }); + + assert.strictEqual(result.pickups.length, 0, "Should NOT dispatch reviewer for review:human"); + const reviewerSkip = result.skipped.find((s) => s.role === "reviewer"); + assert.ok(reviewerSkip, "Should have skipped reviewer"); + assert.ok(reviewerSkip!.reason.includes("review:human"), `Skip reason: ${reviewerSkip!.reason}`); + }); + + it("reviewPolicy: human should still allow developer and tester dispatch", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 84, title: "Dev task", labels: ["To Do"] }); + h.provider.seedIssue({ iid: 85, title: "Test task", labels: ["To Test"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + agentId: "test-agent", + workflow: workflowWithPolicy(ReviewPolicy.HUMAN), + provider: h.provider, + }); + + const roles = result.pickups.map((p) => p.role); + assert.ok(roles.includes("developer"), `Should dispatch developer, got: ${roles}`); + assert.ok(roles.includes("tester"), `Should dispatch tester, got: ${roles}`); + assert.ok(!roles.includes("reviewer"), "Should NOT dispatch reviewer"); + }); + }); + + // ========================================================================= + // Role:level labels — dispatch applies labels, tick reads them + // ========================================================================= + + describe("role:level labels", () => { + it("dispatch should apply role:level label to issue", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 400, title: "Label test", labels: ["To Do"] }); + + await dispatchTask({ + workspaceDir: h.workspaceDir, + agentId: "test-agent", + groupId: h.groupId, + project: h.project, + issueId: 400, + issueTitle: "Label test", + issueDescription: "", + issueUrl: "https://example.com/issues/400", + role: "developer", + level: "senior", + fromLabel: "To Do", + toLabel: "Doing", + transitionLabel: (id, from, to) => h.provider.transitionLabel(id, from, to), + provider: h.provider, + }); + + const issue = await h.provider.getIssue(400); + assert.ok(issue.labels.includes("developer:senior"), `Should have developer:senior, got: ${issue.labels}`); + assert.ok(issue.labels.includes("Doing"), "Should have Doing label"); + // Senior developer dispatch should also apply review:human routing label + assert.ok(issue.labels.includes("review:human"), `Should have review:human for senior, got: ${issue.labels}`); + }); + + it("dispatch should apply review:agent label for non-senior developer", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 404, title: "Junior task", labels: ["To Do"] }); + + await dispatchTask({ + workspaceDir: h.workspaceDir, + agentId: "test-agent", + groupId: h.groupId, + project: h.project, + issueId: 404, + issueTitle: "Junior task", + issueDescription: "", + issueUrl: "https://example.com/issues/404", + role: "developer", + level: "junior", + fromLabel: "To Do", + toLabel: "Doing", + transitionLabel: (id, from, to) => h.provider.transitionLabel(id, from, to), + provider: h.provider, + }); + + const issue = await h.provider.getIssue(404); + assert.ok(issue.labels.includes("developer:junior"), `Should have developer:junior, got: ${issue.labels}`); + assert.ok(issue.labels.includes("review:agent"), `Should have review:agent for junior, got: ${issue.labels}`); + }); + + it("dispatch should replace old role:level label", async () => { + h = await createTestHarness(); + // Issue already has a developer:junior label from a previous dispatch + h.provider.seedIssue({ iid: 401, title: "Re-dispatch", labels: ["To Improve", "developer:junior"] }); + + await dispatchTask({ + workspaceDir: h.workspaceDir, + agentId: "test-agent", + groupId: h.groupId, + project: h.project, + issueId: 401, + issueTitle: "Re-dispatch", + issueDescription: "", + issueUrl: "https://example.com/issues/401", + role: "developer", + level: "medior", + fromLabel: "To Improve", + toLabel: "Doing", + transitionLabel: (id, from, to) => h.provider.transitionLabel(id, from, to), + provider: h.provider, + }); + + const issue = await h.provider.getIssue(401); + assert.ok(issue.labels.includes("developer:medior"), `Should have developer:medior, got: ${issue.labels}`); + assert.ok(!issue.labels.includes("developer:junior"), "Should NOT have developer:junior"); + }); + + it("projectTick should skip reviewer when review:human label present", async () => { + h = await createTestHarness(); + // review:human applied by dispatch for senior developers + h.provider.seedIssue({ iid: 402, title: "Senior review", labels: ["To Review", "developer:senior", "review:human"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + targetRole: "reviewer", + workflow: { ...DEFAULT_WORKFLOW, reviewPolicy: ReviewPolicy.AUTO }, + provider: h.provider, + }); + + assert.strictEqual(result.pickups.length, 0, "Should NOT dispatch reviewer for review:human"); + const reviewerSkip = result.skipped.find((s) => s.role === "reviewer"); + assert.ok(reviewerSkip, "Should have skipped reviewer"); + assert.ok(reviewerSkip!.reason.includes("review:human"), `Skip reason: ${reviewerSkip!.reason}`); + }); + + it("projectTick should dispatch reviewer when review:agent label present", async () => { + h = await createTestHarness(); + h.provider.seedIssue({ iid: 403, title: "Junior fix", labels: ["To Review", "developer:junior", "review:agent"] }); + + const result = await projectTick({ + workspaceDir: h.workspaceDir, + groupId: h.groupId, + agentId: "test-agent", + targetRole: "reviewer", + workflow: { ...DEFAULT_WORKFLOW, reviewPolicy: ReviewPolicy.AUTO }, + provider: h.provider, + }); + + assert.strictEqual(result.pickups.length, 1, "Should dispatch reviewer for review:agent"); + assert.strictEqual(result.pickups[0].role, "reviewer"); + }); + }); + // ========================================================================= // Provider call tracking // ========================================================================= diff --git a/lib/services/pipeline.ts b/lib/services/pipeline.ts index 01e22de..443bc8e 100644 --- a/lib/services/pipeline.ts +++ b/lib/services/pipeline.ts @@ -10,6 +10,7 @@ import { runCommand } from "../run-command.js"; import { notify, getNotificationConfig } from "../notify.js"; import { log as auditLog } from "../audit.js"; import { loadConfig } from "../config/index.js"; +import { detectStepRouting } from "./queue-scan.js"; import { DEFAULT_WORKFLOW, Action, @@ -146,6 +147,36 @@ export async function executeCompletion(opts: { } } + // Send review routing notification when developer completes + if (role === "developer" && result === "done") { + // Re-fetch issue to get labels after transition + const updated = await provider.getIssue(issueId); + const routing = detectStepRouting(updated.labels, "review") as "human" | "agent" | null; + if (routing === "human" || routing === "agent") { + notify( + { + type: "reviewNeeded", + project: projectName, + groupId, + issueId, + issueUrl: updated.web_url, + issueTitle: updated.title, + routing, + prUrl, + }, + { + workspaceDir, + config: notifyConfig, + groupId, + channel: channel ?? "telegram", + runtime, + }, + ).catch((err) => { + auditLog(workspaceDir, "pipeline_warning", { step: "reviewNotify", issue: issueId, role, error: (err as Error).message ?? String(err) }).catch(() => {}); + }); + } + } + // Build announcement using workflow-derived emoji const emoji = getCompletionEmoji(role, result); const label = key.replace(":", " ").toUpperCase(); diff --git a/lib/services/queue-scan.ts b/lib/services/queue-scan.ts index aef88af..fcc7b56 100644 --- a/lib/services/queue-scan.ts +++ b/lib/services/queue-scan.ts @@ -22,7 +22,16 @@ import { export function detectLevelFromLabels(labels: string[]): string | null { const lower = labels.map((l) => l.toLowerCase()); - // Match role.level labels (e.g., "dev.senior", "qa.mid", "architect.junior") + // Priority 1: Match role:level labels (e.g., "developer:senior", "tester:junior") + for (const l of lower) { + const colon = l.indexOf(":"); + if (colon === -1) continue; + const level = l.slice(colon + 1); + const all = getAllLevels(); + if (all.includes(level)) return level; + } + + // Priority 2: Match legacy role.level labels (e.g., "dev.senior", "qa.mid") for (const l of lower) { const dot = l.indexOf("."); if (dot === -1) continue; @@ -37,6 +46,36 @@ export function detectLevelFromLabels(labels: string[]): string | null { return all.find((l) => lower.includes(l)) ?? null; } +/** + * Detect role and level from colon-format labels (e.g. "developer:senior"). + * Returns the first match found, or null if no role:level label exists. + */ +export function detectRoleLevelFromLabels( + labels: string[], +): { role: string; level: string } | null { + for (const label of labels) { + const colon = label.indexOf(":"); + if (colon === -1) continue; + const role = label.slice(0, colon).toLowerCase(); + const level = label.slice(colon + 1).toLowerCase(); + const roleLevels = getLevelsForRole(role); + if (roleLevels.includes(level)) return { role, level }; + } + return null; +} + +/** + * Detect step routing from labels (e.g. "review:human", "test:skip"). + * Returns the routing value for the given step, or null if no routing label exists. + */ +export function detectStepRouting( + labels: string[], step: string, +): string | null { + const prefix = `${step}:`; + const match = labels.find((l) => l.toLowerCase().startsWith(prefix)); + return match ? match.slice(prefix.length).toLowerCase() : null; +} + /** * Detect role from a label using workflow config. */ diff --git a/lib/services/review.ts b/lib/services/review.ts index c66baa1..76a5fc7 100644 --- a/lib/services/review.ts +++ b/lib/services/review.ts @@ -11,7 +11,6 @@ import { Action, ReviewCheck, WorkflowEvent, - StateType, type WorkflowConfig, type StateConfig, } from "../workflow.js"; @@ -33,9 +32,9 @@ export async function reviewPass(opts: { const { workspaceDir, groupId, workflow, provider, repoPath, gitPullTimeoutMs = 30_000 } = opts; let transitions = 0; - // Find all review-type states + // Find all states with a review check (e.g. toReview with check: prApproved) const reviewStates = Object.entries(workflow.states) - .filter(([, s]) => s.type === StateType.REVIEW) as [string, StateConfig][]; + .filter(([, s]) => s.check != null) as [string, StateConfig][]; for (const [stateKey, state] of reviewStates) { if (!state.on || !state.check) continue; @@ -50,9 +49,9 @@ export async function reviewPass(opts: { if (!conditionMet) continue; - // Find the success transition (first event that isn't BLOCKED or MERGE_FAILED) + // Find the success transition — use the APPROVED event (matches check condition) const successEvent = Object.keys(state.on).find( - (e) => e !== WorkflowEvent.BLOCKED && e !== WorkflowEvent.MERGE_FAILED, + (e) => e === WorkflowEvent.APPROVED, ); if (!successEvent) continue; diff --git a/lib/services/tick.ts b/lib/services/tick.ts index d334ac2..d6f8305 100644 --- a/lib/services/tick.ts +++ b/lib/services/tick.ts @@ -11,15 +11,16 @@ import { createProvider } from "../providers/index.js"; import { selectLevel } from "../model-selector.js"; import { getWorker, getSessionForLevel, readProjects } from "../projects.js"; import { dispatchTask } from "../dispatch.js"; -import { roleForLevel } from "../roles/index.js"; +import { getLevelsForRole } from "../roles/index.js"; import { loadConfig } from "../config/index.js"; import { ExecutionMode, + ReviewPolicy, getActiveLabel, type WorkflowConfig, type Role, } from "../workflow.js"; -import { detectLevelFromLabels, findNextIssueForRole } from "./queue-scan.js"; +import { detectRoleLevelFromLabels, detectStepRouting, findNextIssueForRole } from "./queue-scan.js"; // --------------------------------------------------------------------------- // projectTick @@ -109,12 +110,37 @@ export async function projectTick(opts: { continue; } + // Review policy gate: fallback for issues dispatched before step routing labels existed + if (role === "reviewer") { + const policy = workflow.reviewPolicy ?? ReviewPolicy.AUTO; + if (policy === ReviewPolicy.HUMAN) { + skipped.push({ role, reason: "Review policy: human (heartbeat handles via PR polling)" }); + continue; + } + } + const next = await findNextIssueForRole(provider, role, workflow); if (!next) continue; const { issue, label: currentLabel } = next; const targetLabel = getActiveLabel(workflow, role); + // Step routing: check for review:human / review:skip / test:skip labels + if (role === "reviewer") { + const routing = detectStepRouting(issue.labels, "review"); + if (routing === "human" || routing === "skip") { + skipped.push({ role, reason: `review:${routing} label` }); + continue; + } + } + if (role === "tester") { + const routing = detectStepRouting(issue.labels, "test"); + if (routing === "skip") { + skipped.push({ role, reason: "test:skip label" }); + continue; + } + } + // Level selection: label → heuristic const selectedLevel = resolveLevelForIssue(issue, role); @@ -158,15 +184,25 @@ export async function projectTick(opts: { // --------------------------------------------------------------------------- /** - * Determine the level for an issue based on labels, role overrides, and heuristic fallback. + * Determine the level for an issue based on labels and heuristic fallback. + * + * Priority: + * 1. This role's own label (e.g. tester:medior from a previous dispatch) + * 2. Inherit from another role's label (e.g. developer:medior → tester uses medior) + * 3. Heuristic fallback (first dispatch, no labels yet) */ function resolveLevelForIssue(issue: Issue, role: Role): string { - const labelLevel = detectLevelFromLabels(issue.labels); - if (labelLevel) { - const labelRole = roleForLevel(labelLevel); - // If label level belongs to a different role, use heuristic for correct role - if (labelRole && labelRole !== role) return selectLevel(issue.title, issue.description ?? "", role).level; - return labelLevel; + const roleLevel = detectRoleLevelFromLabels(issue.labels); + + // Own role label + if (roleLevel?.role === role) return roleLevel.level; + + // Inherit from another role's label if level is valid for this role + if (roleLevel) { + const levels = getLevelsForRole(role); + if (levels.includes(roleLevel.level)) return roleLevel.level; } + + // Heuristic fallback return selectLevel(issue.title, issue.description ?? "", role).level; } diff --git a/lib/templates.ts b/lib/templates.ts index dd1ac5c..2ac34c0 100644 --- a/lib/templates.ts +++ b/lib/templates.ts @@ -26,10 +26,7 @@ Read the comments carefully — they often contain clarifications, decisions, or - Run tests before completing - Create an MR/PR to the base branch - **IMPORTANT:** 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" instead. DevClaw manages issue state — auto-closing bypasses QA. -- **Merge or request review:** - - Merge the PR yourself → call work_finish with result "done" - - Leave the PR open for human review → call work_finish with result "review" (the heartbeat will auto-merge when approved and advance to testing) -- Clean up the worktree after merging (if you merged) +- **Do NOT merge the PR yourself** — leave it open for review. The system will auto-merge when approved. - If you discover unrelated bugs, call task_create to file them - Do NOT call work_start, status, health, or project_register `; @@ -120,16 +117,55 @@ Your session is persistent — you may be called back for refinements. Do NOT call work_start, status, health, or project_register. `; +export const DEFAULT_REVIEWER_INSTRUCTIONS = `# REVIEWER Worker Instructions + +You are a code reviewer. Your job is to review the PR diff for quality, correctness, and style. + +## Context You Receive + +- **Issue:** the original task description and discussion +- **PR diff:** the code changes to review +- **PR URL:** link to the pull request + +## Review Checklist + +1. **Correctness** — Does the code do what the issue asks for? +2. **Bugs** — Any logic errors, off-by-one, null handling issues? +3. **Security** — SQL injection, XSS, hardcoded secrets, command injection? +4. **Style** — Consistent with the codebase? Readable? +5. **Tests** — Are changes tested? Any missing edge cases? +6. **Scope** — Does the PR stay within the issue scope? Any unrelated changes? + +## Your Job + +- Read the PR diff carefully +- Check the code against the review checklist +- Call task_comment with your review findings +- Then call work_finish with role "reviewer" and one of: + - result "approve" if the code looks good + - result "reject" with specific issues if problems found + - result "blocked" if you can't complete the review + +## Important + +- 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 reject, list actionable items the developer must fix +- Do NOT call work_start, status, health, or project_register +`; + /** Default role instructions indexed by role ID. Used by project scaffolding. */ export const DEFAULT_ROLE_INSTRUCTIONS: Record = { developer: DEFAULT_DEV_INSTRUCTIONS, tester: DEFAULT_QA_INSTRUCTIONS, architect: DEFAULT_ARCHITECT_INSTRUCTIONS, + reviewer: DEFAULT_REVIEWER_INSTRUCTIONS, }; export const AGENTS_MD_TEMPLATE = `# AGENTS.md - Development Orchestration (DevClaw) -## If You Are a Sub-Agent (DEVELOPER/TESTER Worker) +## If You Are a Sub-Agent (DEVELOPER/TESTER/REVIEWER Worker) Skip the orchestrator section. Follow your task message and role instructions (appended to the task message). @@ -149,11 +185,12 @@ Skip the orchestrator section. Follow your task message and role instructions (a When you are done, **call \`work_finish\` yourself** — do not just announce in text. -- **DEVELOPER done (merged):** \`work_finish({ role: "developer", result: "done", projectGroupId: "", summary: "" })\` -- **DEVELOPER review (PR open):** \`work_finish({ role: "developer", result: "review", projectGroupId: "", summary: "" })\` +- **DEVELOPER done:** \`work_finish({ role: "developer", result: "done", projectGroupId: "", summary: "" })\` - **TESTER pass:** \`work_finish({ role: "tester", result: "pass", projectGroupId: "", summary: "" })\` - **TESTER fail:** \`work_finish({ role: "tester", result: "fail", projectGroupId: "", summary: "" })\` - **TESTER refine:** \`work_finish({ role: "tester", result: "refine", projectGroupId: "", summary: "" })\` +- **REVIEWER approve:** \`work_finish({ role: "reviewer", result: "approve", projectGroupId: "", summary: "" })\` +- **REVIEWER reject:** \`work_finish({ role: "reviewer", result: "reject", projectGroupId: "", summary: "" })\` - **Architect done:** \`work_finish({ role: "architect", result: "done", projectGroupId: "", summary: "" })\` The \`projectGroupId\` is included in your task message. @@ -233,17 +270,21 @@ All orchestration goes through these tools. You do NOT manually manage sessions, ### Pipeline Flow \`\`\` -Planning → To Do → Doing → To Test → Testing → Done - ↓ ↑ - In Review ā”€ā”€ā”€ā”€ā”€ā”˜ (auto-merges when PR approved) - ↓ - To Improve → Doing (merge conflict / fix cycle) - ↓ - Refining (human decision) +Planning → To Do → Doing → To Review ──┬── [agent] → Reviewing → approve → To Test → Testing → Done + │ → reject → To Improve + │ → blocked → Refining + └── [human] → PR approved → To Test (heartbeat auto-transitions) +To Improve → Doing (fix cycle) +Refining (human decision) To Design → Designing → Planning (design complete) \`\`\` +Review policy (configurable per project in workflow.yaml): +- **auto** (default): junior/medior → agent review, senior → human review +- **agent**: always agent review +- **human**: always human review (stays in To Review, heartbeat polls PR) + Issue labels are the single source of truth for task state. ### Developer Assignment @@ -268,12 +309,15 @@ All roles (Developer, Tester, Architect) use the same level scheme. Levels descr Workers call \`work_finish\` themselves — the label transition, state update, and audit log happen atomically. The heartbeat service will pick up the next task on its next cycle: -- Developer "done" → issue moves to "To Test" → scheduler dispatches Tester -- Developer "review" → issue moves to "In Review" → heartbeat polls PR status → auto-merges and advances to "To Test" when approved (merge conflicts → "To Improve" for developer to fix) -- Tester "fail" → issue moves to "To Improve" → scheduler dispatches Developer +- Developer "done" → "To Review" → routes based on review policy: + - Agent/auto-junior: reviewer agent dispatched → "Reviewing" → approve/reject + - Human/auto-senior: heartbeat polls PR status → auto-merges when approved → "To Test" +- Reviewer "approve" → merges PR → "To Test" → scheduler dispatches Tester +- Reviewer "reject" → "To Improve" → scheduler dispatches Developer +- Tester "fail" → "To Improve" → scheduler dispatches Developer - Tester "pass" → Done, no further dispatch - Tester "refine" / blocked → needs human input -- Architect "done" → issue moves to "Planning" → ready for tech lead review +- Architect "done" → "Planning" → ready for tech lead review **Always include issue URLs** in your response — these are in the \`announcement\` fields. @@ -283,7 +327,7 @@ Workers receive role-specific instructions appended to their task message. These ### Heartbeats -**Do nothing.** The heartbeat service runs automatically as an internal interval-based process — zero LLM tokens. It handles health checks (zombie detection, stale workers), review polling (auto-advancing "In Review" issues when PRs are merged), and queue dispatch (filling free worker slots by priority) every 60 seconds by default. Configure via \`plugins.entries.devclaw.config.work_heartbeat\` in openclaw.json. +**Do nothing.** The heartbeat service runs automatically as an internal interval-based process — zero LLM tokens. It handles health checks (zombie detection, stale workers), review polling (auto-advancing "To Review" issues when PRs are approved), and queue dispatch (filling free worker slots by priority) every 60 seconds by default. Configure via \`plugins.entries.devclaw.config.work_heartbeat\` in openclaw.json. ### Safety @@ -325,7 +369,7 @@ You are a **development orchestrator** — you plan, prioritize, and dispatch. Y - You receive requests via chat (Telegram, WhatsApp, or web) - You break work into issues, assign complexity levels, and dispatch workers -- Workers (developer, tester, architect) do the actual work in isolated sessions +- Workers (developer, reviewer, tester, architect) do the actual work in isolated sessions - You track progress, handle failures, and keep the human informed - The heartbeat runs automatically — you don't manage it diff --git a/lib/testing/harness.ts b/lib/testing/harness.ts index 4807cf2..3ac8534 100644 --- a/lib/testing/harness.ts +++ b/lib/testing/harness.ts @@ -191,6 +191,7 @@ export async function createTestHarness(opts?: HarnessOptions): Promise(); /** Issue IDs where mergePr should fail (simulates merge conflicts). */ mergePrFailures = new Set(); + /** PR diffs per issue (for reviewer tests). */ + prDiffs = new Map(); /** All calls, in order. */ calls: ProviderCall[] = []; @@ -118,6 +123,7 @@ export class TestProvider implements IssueProvider { this.prStatuses.clear(); this.mergedMrUrls.clear(); this.mergePrFailures.clear(); + this.prDiffs.clear(); this.calls = []; this.nextIssueId = 1; } @@ -193,6 +199,22 @@ export class TestProvider implements IssueProvider { issue.labels.push(to); } + async addLabel(issueId: number, label: string): Promise { + this.calls.push({ method: "addLabel", args: { issueId, label } }); + const issue = this.issues.get(issueId); + if (issue && !issue.labels.includes(label)) { + issue.labels.push(label); + } + } + + async removeLabels(issueId: number, labels: string[]): Promise { + this.calls.push({ method: "removeLabels", args: { issueId, labels } }); + const issue = this.issues.get(issueId); + if (issue) { + issue.labels = issue.labels.filter((l) => !labels.includes(l)); + } + } + async closeIssue(issueId: number): Promise { this.calls.push({ method: "closeIssue", args: { issueId } }); const issue = this.issues.get(issueId); @@ -241,6 +263,11 @@ export class TestProvider implements IssueProvider { } } + async getPrDiff(issueId: number): Promise { + this.calls.push({ method: "getPrDiff", args: { issueId } }); + return this.prDiffs.get(issueId) ?? null; + } + async addComment(issueId: number, body: string): Promise { this.calls.push({ method: "addComment", args: { issueId, body } }); const existing = this.comments.get(issueId) ?? []; diff --git a/lib/tools/project-register.ts b/lib/tools/project-register.ts index 650627c..ff386b2 100644 --- a/lib/tools/project-register.ts +++ b/lib/tools/project-register.ts @@ -15,7 +15,8 @@ import { resolveRepoPath } from "../projects.js"; import { createProvider } from "../providers/index.js"; import { log as auditLog } from "../audit.js"; import { getAllRoleIds, getLevelsForRole } from "../roles/index.js"; -import { ExecutionMode } from "../workflow.js"; +import { ExecutionMode, getRoleLabels } from "../workflow.js"; +import { loadConfig } from "../config/index.js"; import { DEFAULT_ROLE_INSTRUCTIONS } from "../templates.js"; import { DATA_DIR } from "../setup/migrate-layout.js"; @@ -141,6 +142,13 @@ export function createProjectRegisterTool() { // 4. Create all state labels (idempotent) await provider.ensureAllStateLabels(); + // 4b. Create role:level + step routing labels (e.g. developer:junior, review:human, test:skip) + const resolvedConfig = await loadConfig(workspaceDir, name); + const roleLabels = getRoleLabels(resolvedConfig.roles); + for (const { name: labelName, color } of roleLabels) { + await provider.ensureLabel(labelName, color); + } + // 5. Add project to projects.json // Build workers map from all registered roles const workers: Record = {}; diff --git a/lib/tools/task-update.test.ts b/lib/tools/task-update.test.ts index 7137b8f..4572158 100644 --- a/lib/tools/task-update.test.ts +++ b/lib/tools/task-update.test.ts @@ -1,64 +1,133 @@ /** - * Integration test for task_update tool. + * Tests for task_update tool — state transitions and level overrides. * - * Run manually: node --loader ts-node/esm lib/tools/task-update.test.ts + * Run: npx tsx --test lib/tools/task-update.test.ts */ import { describe, it } from "node:test"; import assert from "node:assert"; +import { DEFAULT_WORKFLOW, getStateLabels, ReviewPolicy, resolveReviewRouting } from "../workflow.js"; +import { detectLevelFromLabels, detectRoleLevelFromLabels, detectStepRouting } from "../services/queue-scan.js"; describe("task_update tool", () => { it("has correct schema", () => { - // Verify the tool signature matches requirements - const requiredParams = ["projectGroupId", "issueId", "state"]; - const optionalParams = ["reason"]; - - // Schema validation would go here in a real test - assert.ok(true, "Schema structure is valid"); + // state is now optional — at least one of state or level required + const requiredParams = ["projectGroupId", "issueId"]; + assert.strictEqual(requiredParams.length, 2); }); it("supports all state labels", () => { - const validStates = [ - "Planning", - "To Do", - "Doing", - "To Test", - "Testing", - "Done", - "To Improve", - "Refining", - "In Review", - ]; - - // In a real test, we'd verify these against the tool's enum - assert.strictEqual(validStates.length, 9); + const labels = getStateLabels(DEFAULT_WORKFLOW); + assert.strictEqual(labels.length, 12); + assert.ok(labels.includes("Planning")); + assert.ok(labels.includes("Done")); + assert.ok(labels.includes("To Review")); }); it("validates required parameters", () => { - // Test cases: - // - Missing projectGroupId → Error - // - Missing issueId → Error - // - Missing state → Error - // - Invalid state → Error - // - Valid params → Success + // At least one of state or level required assert.ok(true, "Parameter validation works"); }); it("handles same-state transitions gracefully", () => { - // When current state === new state, should return success without changes assert.ok(true, "No-op transitions handled correctly"); }); it("logs to audit trail", () => { - // Verify auditLog is called with correct parameters assert.ok(true, "Audit logging works"); }); }); -// Test scenarios for manual verification: -// 1. task_update({ projectGroupId: "-5239235162", issueId: 28, state: "Planning" }) -// → Should transition from "To Do" to "Planning" -// 2. task_update({ projectGroupId: "-5239235162", issueId: 28, state: "Planning", reason: "Needs more discussion" }) -// → Should log reason in audit trail -// 3. task_update({ projectGroupId: "-5239235162", issueId: 28, state: "To Do" }) -// → Should transition back from "Planning" to "To Do" +describe("detectLevelFromLabels — colon format", () => { + it("should detect level from colon-format labels", () => { + assert.strictEqual(detectLevelFromLabels(["developer:senior", "Doing"]), "senior"); + assert.strictEqual(detectLevelFromLabels(["tester:junior", "Testing"]), "junior"); + assert.strictEqual(detectLevelFromLabels(["reviewer:medior", "Reviewing"]), "medior"); + }); + + it("should prioritize colon format over dot format", () => { + // Colon format should win since it's checked first + assert.strictEqual(detectLevelFromLabels(["developer:senior", "dev.junior"]), "senior"); + }); + + it("should fall back to dot format", () => { + assert.strictEqual(detectLevelFromLabels(["developer.senior", "Doing"]), "senior"); + }); + + it("should fall back to plain level name", () => { + assert.strictEqual(detectLevelFromLabels(["senior", "Doing"]), "senior"); + }); + + it("should return null when no level found", () => { + assert.strictEqual(detectLevelFromLabels(["Doing", "bug"]), null); + }); +}); + +describe("detectRoleLevelFromLabels", () => { + it("should detect role and level from colon-format labels", () => { + const result = detectRoleLevelFromLabels(["developer:senior", "Doing"]); + assert.deepStrictEqual(result, { role: "developer", level: "senior" }); + }); + + it("should detect tester role", () => { + const result = detectRoleLevelFromLabels(["tester:medior", "Testing"]); + assert.deepStrictEqual(result, { role: "tester", level: "medior" }); + }); + + it("should return null for step routing labels", () => { + // review:human is a step routing label, not a role:level label + const result = detectRoleLevelFromLabels(["review:human", "Doing"]); + assert.strictEqual(result, null); + }); + + it("should return null when no colon labels present", () => { + assert.strictEqual(detectRoleLevelFromLabels(["Doing", "bug"]), null); + }); +}); + +describe("detectStepRouting", () => { + it("should detect review:human", () => { + assert.strictEqual(detectStepRouting(["review:human", "Doing"], "review"), "human"); + }); + + it("should detect review:agent", () => { + assert.strictEqual(detectStepRouting(["review:agent", "To Review"], "review"), "agent"); + }); + + it("should detect review:skip", () => { + assert.strictEqual(detectStepRouting(["review:skip", "To Review"], "review"), "skip"); + }); + + it("should detect test:skip", () => { + assert.strictEqual(detectStepRouting(["test:skip", "To Test"], "test"), "skip"); + }); + + it("should return null when no matching step label", () => { + assert.strictEqual(detectStepRouting(["developer:senior", "Doing"], "review"), null); + }); + + it("should be case-insensitive", () => { + assert.strictEqual(detectStepRouting(["Review:Human", "Doing"], "review"), "human"); + }); +}); + +describe("resolveReviewRouting", () => { + it("should return review:human for HUMAN policy", () => { + assert.strictEqual(resolveReviewRouting(ReviewPolicy.HUMAN, "junior"), "review:human"); + assert.strictEqual(resolveReviewRouting(ReviewPolicy.HUMAN, "senior"), "review:human"); + }); + + it("should return review:agent for AGENT policy", () => { + assert.strictEqual(resolveReviewRouting(ReviewPolicy.AGENT, "junior"), "review:agent"); + assert.strictEqual(resolveReviewRouting(ReviewPolicy.AGENT, "senior"), "review:agent"); + }); + + it("should return review:human for AUTO + senior", () => { + assert.strictEqual(resolveReviewRouting(ReviewPolicy.AUTO, "senior"), "review:human"); + }); + + it("should return review:agent for AUTO + non-senior", () => { + assert.strictEqual(resolveReviewRouting(ReviewPolicy.AUTO, "junior"), "review:agent"); + assert.strictEqual(resolveReviewRouting(ReviewPolicy.AUTO, "medior"), "review:agent"); + }); +}); diff --git a/lib/tools/task-update.ts b/lib/tools/task-update.ts index c3387d8..a3f1cbb 100644 --- a/lib/tools/task-update.ts +++ b/lib/tools/task-update.ts @@ -11,27 +11,30 @@ import { jsonResult } from "openclaw/plugin-sdk"; import type { ToolContext } from "../types.js"; import { log as auditLog } from "../audit.js"; import type { StateLabel } from "../providers/provider.js"; -import { DEFAULT_WORKFLOW, getStateLabels } from "../workflow.js"; +import { DEFAULT_WORKFLOW, getStateLabels, findStateByLabel } from "../workflow.js"; +import { loadConfig } from "../config/index.js"; import { requireWorkspaceDir, resolveProject, resolveProvider } from "../tool-helpers.js"; export function createTaskUpdateTool(api: OpenClawPluginApi) { return (ctx: ToolContext) => ({ name: "task_update", label: "Task Update", - description: `Change issue state programmatically. Use this when you need to update an issue's status without going through the full pickup/complete flow. + description: `Change issue state and/or role:level assignment. Use this when you need to update an issue's status or override the assigned level. Use cases: - Orchestrator or worker needs to change state manually - Manual status adjustments (e.g., Planning → To Do after approval) +- Override the assigned level (e.g., escalate to senior for human review) +- Force human review via level change - Failed auto-transitions that need correction -- Bulk state changes Examples: -- Simple: { projectGroupId: "-123456789", issueId: 42, state: "To Do" } -- With reason: { projectGroupId: "-123456789", issueId: 42, state: "To Do", reason: "Approved for development" }`, +- State only: { projectGroupId: "-123456789", issueId: 42, state: "To Do" } +- Level only: { projectGroupId: "-123456789", issueId: 42, level: "senior" } +- Both: { projectGroupId: "-123456789", issueId: 42, state: "To Do", level: "senior", reason: "Escalating to senior" }`, parameters: { type: "object", - required: ["projectGroupId", "issueId", "state"], + required: ["projectGroupId", "issueId"], properties: { projectGroupId: { type: "string", @@ -46,9 +49,13 @@ Examples: enum: getStateLabels(DEFAULT_WORKFLOW), description: `New state for the issue. One of: ${getStateLabels(DEFAULT_WORKFLOW).join(", ")}`, }, + level: { + type: "string", + description: "Override the role:level assignment (e.g., 'senior', 'junior'). Detects role from current state label.", + }, reason: { type: "string", - description: "Optional audit log reason for the state change", + description: "Optional audit log reason for the change", }, }, }, @@ -56,41 +63,86 @@ Examples: async execute(_id: string, params: Record) { const groupId = params.projectGroupId as string; const issueId = params.issueId as number; - const newState = params.state as StateLabel; + const newState = (params.state as StateLabel) ?? undefined; + const newLevel = (params.level as string) ?? undefined; const reason = (params.reason as string) ?? undefined; const workspaceDir = requireWorkspaceDir(ctx); + if (!newState && !newLevel) { + throw new Error("At least one of 'state' or 'level' must be provided."); + } + const { project } = await resolveProject(workspaceDir, groupId); const { provider, type: providerType } = await resolveProvider(project); const issue = await provider.getIssue(issueId); const currentState = provider.getCurrentStateLabel(issue); if (!currentState) { - throw new Error(`Issue #${issueId} has no recognized state label. Cannot perform transition.`); + throw new Error(`Issue #${issueId} has no recognized state label. Cannot perform update.`); } - if (currentState === newState) { - return jsonResult({ - success: true, issueId, state: newState, changed: false, - message: `Issue #${issueId} is already in state "${newState}".`, - project: project.name, provider: providerType, - }); + let stateChanged = false; + let levelChanged = false; + let fromLevel: string | undefined; + + // Handle state transition + if (newState && currentState !== newState) { + await provider.transitionLabel(issueId, currentState, newState); + stateChanged = true; } - await provider.transitionLabel(issueId, currentState, newState); + // Handle level override + if (newLevel) { + // Detect role from current (or new) state label + const effectiveState = newState ?? currentState; + const workflow = (await loadConfig(workspaceDir, project.name)).workflow; + const stateConfig = findStateByLabel(workflow, effectiveState); + const role = stateConfig?.role; + if (!role) { + throw new Error(`Cannot determine role from state "${effectiveState}". Level can only be set on role-assigned states.`); + } + // Validate level exists for role + const resolvedConfig = await loadConfig(workspaceDir, project.name); + const roleConfig = resolvedConfig.roles[role]; + if (!roleConfig || !roleConfig.levels.includes(newLevel)) { + throw new Error(`Invalid level "${newLevel}" for role "${role}". Valid levels: ${roleConfig?.levels.join(", ") ?? "none"}`); + } + + // Remove old role:* labels, add new role:level + const oldRoleLabels = issue.labels.filter((l) => l.startsWith(`${role}:`)); + fromLevel = oldRoleLabels[0]?.split(":")[1]; + if (oldRoleLabels.length > 0) { + await provider.removeLabels(issueId, oldRoleLabels); + } + await provider.addLabel(issueId, `${role}:${newLevel}`); + levelChanged = fromLevel !== newLevel; + } + + // Audit await auditLog(workspaceDir, "task_update", { project: project.name, groupId, issueId, - fromState: currentState, toState: newState, + ...(stateChanged ? { fromState: currentState, toState: newState } : {}), + ...(levelChanged ? { fromLevel: fromLevel ?? null, toLevel: newLevel } : {}), reason: reason ?? null, provider: providerType, }); + // Build announcement + const parts: string[] = []; + if (stateChanged) parts.push(`"${currentState}" → "${newState}"`); + if (levelChanged) parts.push(`level: ${fromLevel ?? "none"} → ${newLevel}`); + const changeDesc = parts.join(", "); + return jsonResult({ success: true, issueId, issueTitle: issue.title, - state: newState, changed: true, - labelTransition: `${currentState} → ${newState}`, + ...(newState ? { state: newState } : {}), + ...(newLevel ? { level: newLevel } : {}), + changed: stateChanged || levelChanged, + ...(stateChanged ? { labelTransition: `${currentState} → ${newState}` } : {}), project: project.name, provider: providerType, - announcement: `šŸ”„ Updated #${issueId}: "${currentState}" → "${newState}"${reason ? ` (${reason})` : ""}`, + announcement: stateChanged || levelChanged + ? `šŸ”„ Updated #${issueId}: ${changeDesc}${reason ? ` (${reason})` : ""}` + : `Issue #${issueId} is already in the requested state.`, }); }, }); diff --git a/lib/tools/work-finish.ts b/lib/tools/work-finish.ts index 8ca0daa..9686020 100644 --- a/lib/tools/work-finish.ts +++ b/lib/tools/work-finish.ts @@ -18,13 +18,13 @@ export function createWorkFinishTool(api: OpenClawPluginApi) { return (ctx: ToolContext) => ({ name: "work_finish", label: "Work Finish", - description: `Complete a task: Developer done/blocked, Tester pass/fail/refine/blocked. Handles label transition, state update, issue close/reopen, notifications, and audit logging.`, + description: `Complete a task: Developer done (PR created, goes to review) or blocked. Tester pass/fail/refine/blocked. Reviewer approve/reject/blocked. Handles label transition, state update, issue close/reopen, notifications, and audit logging.`, parameters: { type: "object", required: ["role", "result", "projectGroupId"], properties: { role: { type: "string", enum: getAllRoleIds(), description: "Worker role" }, - result: { type: "string", enum: ["done", "pass", "fail", "refine", "blocked"], description: "Completion result" }, + result: { type: "string", enum: ["done", "pass", "fail", "refine", "blocked", "approve", "reject"], description: "Completion result" }, projectGroupId: { type: "string", description: "Project group ID" }, summary: { type: "string", description: "Brief summary" }, prUrl: { type: "string", description: "PR/MR URL (auto-detected if omitted)" }, diff --git a/lib/tools/work-start.ts b/lib/tools/work-start.ts index 2fa708b..9ae5ca2 100644 --- a/lib/tools/work-start.ts +++ b/lib/tools/work-start.ts @@ -12,8 +12,8 @@ import type { StateLabel } from "../providers/provider.js"; import { selectLevel } from "../model-selector.js"; import { getWorker } from "../projects.js"; import { dispatchTask } from "../dispatch.js"; -import { findNextIssue, detectRoleFromLabel, detectLevelFromLabels } from "../services/queue-scan.js"; -import { getAllRoleIds, isLevelForRole } from "../roles/index.js"; +import { findNextIssue, detectRoleFromLabel, detectRoleLevelFromLabels } from "../services/queue-scan.js"; +import { getAllRoleIds, getLevelsForRole } from "../roles/index.js"; import { requireWorkspaceDir, resolveProject, resolveProvider, getPluginConfig } from "../tool-helpers.js"; import { loadWorkflow, getActiveLabel, ExecutionMode } from "../workflow.js"; @@ -81,20 +81,16 @@ export function createWorkStartTool(api: OpenClawPluginApi) { // Get target label from workflow const targetLabel = getActiveLabel(workflow, role); - // Select level + // Select level: LLM param → own role label → inherit other role label → heuristic let selectedLevel: string, levelReason: string, levelSource: string; if (levelParam) { selectedLevel = levelParam; levelReason = "LLM-selected"; levelSource = "llm"; } else { - const labelLevel = detectLevelFromLabels(issue.labels); - if (labelLevel) { - if (!isLevelForRole(labelLevel, role)) { - // Label level belongs to a different role — use heuristic for this role - const s = selectLevel(issue.title, issue.description ?? "", role); - selectedLevel = s.level; levelReason = `${role} overrides other role's level "${labelLevel}"`; levelSource = "role-override"; - } else { - selectedLevel = labelLevel; levelReason = `Label: "${labelLevel}"`; levelSource = "label"; - } + const roleLevel = detectRoleLevelFromLabels(issue.labels); + if (roleLevel?.role === role) { + selectedLevel = roleLevel.level; levelReason = `Label: "${role}:${roleLevel.level}"`; levelSource = "label"; + } else if (roleLevel && getLevelsForRole(role).includes(roleLevel.level)) { + selectedLevel = roleLevel.level; levelReason = `Inherited from ${roleLevel.role}:${roleLevel.level}`; levelSource = "inherited"; } else { const s = selectLevel(issue.title, issue.description ?? "", role); selectedLevel = s.level; levelReason = s.reason; levelSource = "heuristic"; diff --git a/lib/workflow.ts b/lib/workflow.ts index e0d46a3..cda6e47 100644 --- a/lib/workflow.ts +++ b/lib/workflow.ts @@ -19,7 +19,6 @@ export const StateType = { ACTIVE: "active", HOLD: "hold", TERMINAL: "terminal", - REVIEW: "review", } as const; export type StateType = (typeof StateType)[keyof typeof StateType]; @@ -30,6 +29,14 @@ export const ExecutionMode = { } as const; export type ExecutionMode = (typeof ExecutionMode)[keyof typeof ExecutionMode]; +/** Review policy for PR review after developer completion. */ +export const ReviewPolicy = { + HUMAN: "human", + AGENT: "agent", + AUTO: "auto", +} as const; +export type ReviewPolicy = (typeof ReviewPolicy)[keyof typeof ReviewPolicy]; + /** Role identifier. Built-in: "developer", "tester", "architect". Extensible via config. */ export type Role = string; /** Action identifier. Built-in actions listed in `Action`; custom actions are also valid strings. */ @@ -63,6 +70,7 @@ export const WorkflowEvent = { REFINE: "REFINE", BLOCKED: "BLOCKED", APPROVE: "APPROVE", + REJECT: "REJECT", } as const; export type TransitionTarget = string | { @@ -84,6 +92,7 @@ export type StateConfig = { export type WorkflowConfig = { initial: string; + reviewPolicy?: ReviewPolicy; states: Record; }; @@ -99,6 +108,7 @@ export type CompletionRule = { export const DEFAULT_WORKFLOW: WorkflowConfig = { initial: "planning", + reviewPolicy: ReviewPolicy.AUTO, states: { // ── Main pipeline (happy path) ────────────────────────────── planning: { @@ -121,19 +131,31 @@ export const DEFAULT_WORKFLOW: WorkflowConfig = { label: "Doing", color: "#f0ad4e", on: { - [WorkflowEvent.COMPLETE]: { target: "toTest", actions: [Action.GIT_PULL, Action.DETECT_PR] }, - [WorkflowEvent.REVIEW]: { target: "reviewing", actions: [Action.DETECT_PR] }, + [WorkflowEvent.COMPLETE]: { target: "toReview", actions: [Action.DETECT_PR] }, [WorkflowEvent.BLOCKED]: "refining", }, }, - reviewing: { - type: StateType.REVIEW, - label: "In Review", - color: "#c5def5", + toReview: { + type: StateType.QUEUE, + role: "reviewer", + label: "To Review", + color: "#7057ff", + priority: 2, check: ReviewCheck.PR_APPROVED, on: { + [WorkflowEvent.PICKUP]: "reviewing", [WorkflowEvent.APPROVED]: { target: "toTest", actions: [Action.MERGE_PR, Action.GIT_PULL] }, [WorkflowEvent.MERGE_FAILED]: "toImprove", + }, + }, + reviewing: { + type: StateType.ACTIVE, + role: "reviewer", + label: "Reviewing", + color: "#c5def5", + on: { + [WorkflowEvent.APPROVE]: { target: "toTest", actions: [Action.MERGE_PR, Action.GIT_PULL] }, + [WorkflowEvent.REJECT]: "toImprove", [WorkflowEvent.BLOCKED]: "refining", }, }, @@ -240,6 +262,83 @@ export function getLabelColors(workflow: WorkflowConfig): Record return colors; } +// --------------------------------------------------------------------------- +// Role:level labels — dynamic from config +// --------------------------------------------------------------------------- + +/** Step routing label values — per-issue overrides for workflow steps. */ +export const StepRouting = { + HUMAN: "human", + AGENT: "agent", + SKIP: "skip", +} as const; +export type StepRoutingValue = (typeof StepRouting)[keyof typeof StepRouting]; + +/** Known step routing labels (created on the provider during project registration). */ +export const STEP_ROUTING_LABELS: readonly string[] = [ + "review:human", "review:agent", "review:skip", + "test:skip", +]; + +/** Step routing label color. */ +const STEP_ROUTING_COLOR = "#d93f0b"; + +/** + * Determine review routing label for an issue based on project policy and developer level. + * Called during developer dispatch to persist the routing decision as a label. + */ +export function resolveReviewRouting( + policy: ReviewPolicy, level: string, +): "review:human" | "review:agent" { + if (policy === ReviewPolicy.HUMAN) return "review:human"; + if (policy === ReviewPolicy.AGENT) return "review:agent"; + // AUTO: senior → human, else agent + return level === "senior" ? "review:human" : "review:agent"; +} + +/** Default colors per role for role:level labels. */ +const ROLE_LABEL_COLORS: Record = { + developer: "#0e8a16", + tester: "#5319e7", + architect: "#0075ca", + reviewer: "#d93f0b", +}; + +/** + * Generate all role:level label definitions from resolved config roles. + * Returns array of { name, color } for label creation (e.g. "developer:junior"). + */ +export function getRoleLabels( + roles: Record, +): Array<{ name: string; color: string }> { + const labels: Array<{ name: string; color: string }> = []; + for (const [roleId, role] of Object.entries(roles)) { + if (role.enabled === false) continue; + for (const level of role.levels) { + labels.push({ + name: `${roleId}:${level}`, + color: getRoleLabelColor(roleId), + }); + } + } + // Step routing labels (review:human, review:agent, test:skip, etc.) + for (const routingLabel of STEP_ROUTING_LABELS) { + labels.push({ name: routingLabel, color: STEP_ROUTING_COLOR }); + } + return labels; +} + +/** + * Get the label color for a role. Falls back to gray for unknown roles. + */ +export function getRoleLabelColor(role: string): string { + return ROLE_LABEL_COLORS[role] ?? "#cccccc"; +} + +// --------------------------------------------------------------------------- +// Queue helpers +// --------------------------------------------------------------------------- + /** * Get queue labels for a role, ordered by priority (highest first). */ @@ -348,7 +447,6 @@ export function findStateKeyByLabel(workflow: WorkflowConfig, label: string): st */ function resultToEvent(result: string): string { if (result === "done") return WorkflowEvent.COMPLETE; - if (result === "review") return WorkflowEvent.REVIEW; return result.toUpperCase(); } @@ -405,7 +503,6 @@ export function getNextStateDescription( if (!targetState) return ""; if (targetState.type === StateType.TERMINAL) return "Done!"; - if (targetState.type === StateType.REVIEW) return "awaiting PR review"; if (targetState.type === StateType.HOLD) return "awaiting human decision"; if (targetState.type === StateType.QUEUE && targetState.role) { return `${targetState.role.toUpperCase()} queue`; @@ -420,11 +517,12 @@ export function getNextStateDescription( */ const RESULT_EMOJI: Record = { done: "āœ…", - review: "šŸ‘€", pass: "šŸŽ‰", fail: "āŒ", refine: "šŸ¤”", blocked: "🚫", + approve: "āœ…", + reject: "āŒ", }; export function getCompletionEmoji(_role: Role, result: string): string {