From 2c642b1a53dd0a044b21654c5d9d22a6359260b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B0=8F=E6=A9=98?= Date: Wed, 6 May 2026 10:06:27 +0000 Subject: [PATCH] refactor: committer as pure agent role with discriminated union meta (#17) - Remove hardcoded git commands (git-exec.ts deleted) - CommitterMeta is now a discriminated union: committed | failed - Agent handles git operations, committer extracts structured result - Moderator can route on meta.status for retry logic Closes #17 --- .../__tests__/committer.test.ts | 158 ++++++++++-------- packages/workflow-role-committer/package.json | 1 - .../workflow-role-committer/src/committer.ts | 136 ++++++--------- .../workflow-role-committer/src/git-exec.ts | 20 --- packages/workflow-role-committer/src/index.ts | 2 - .../__tests__/solve-issue-template.test.ts | 2 +- .../src/roles.ts | 15 +- 7 files changed, 148 insertions(+), 186 deletions(-) delete mode 100644 packages/workflow-role-committer/src/git-exec.ts diff --git a/packages/workflow-role-committer/__tests__/committer.test.ts b/packages/workflow-role-committer/__tests__/committer.test.ts index 45e2dd2..ff81de3 100644 --- a/packages/workflow-role-committer/__tests__/committer.test.ts +++ b/packages/workflow-role-committer/__tests__/committer.test.ts @@ -1,40 +1,10 @@ import { describe, expect, spyOn, test } from "bun:test"; -import { execFile } from "node:child_process"; -import { appendFile, mkdir, mkdtemp, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { promisify } from "node:util"; import type { AgentFn, ThreadContext } from "@uncaged/workflow"; import { START } from "@uncaged/workflow"; import * as utilRole from "@uncaged/workflow-util-role"; import { createCommitterRole } from "../src/committer.js"; -import { gitExec } from "../src/git-exec.js"; - -const execFileAsync = promisify(execFile); - -async function git(repo: string, args: string[]): Promise { - await gitExec(repo, args); -} - -async function setupRepoWithRemote(): Promise<{ repo: string }> { - const base = await mkdtemp(join(tmpdir(), "wf-committer-")); - const bare = join(base, "origin.git"); - const repo = join(base, "work"); - await mkdir(repo, { recursive: true }); - await mkdir(bare, { recursive: true }); - await execFileAsync("git", ["init"], { cwd: repo, encoding: "utf8" }); - await git(repo, ["config", "user.email", "t@t"]); - await git(repo, ["config", "user.name", "t"]); - await writeFile(join(repo, "README.md"), "# hi\n", "utf8"); - await git(repo, ["add", "README.md"]); - await git(repo, ["commit", "-m", "init"]); - await execFileAsync("git", ["init", "--bare"], { cwd: bare, encoding: "utf8" }); - await git(repo, ["remote", "add", "origin", bare]); - await git(repo, ["push", "-u", "origin", "HEAD"]); - return { repo }; -} function makeCtx(): ThreadContext { return { @@ -50,21 +20,13 @@ function makeCtx(): ThreadContext { const provider = { baseUrl: "https://example.com/v1", apiKey: "k", model: "m" }; -describe("createCommitterRole", () => { - test("returns committed false when working tree clean", async () => { - const { repo } = await setupRepoWithRemote(); - const agent: AgentFn = async () => { - throw new Error("agent should not run"); - }; - const role = createCommitterRole( - agent, - { provider, dryRun: null, dryRunMeta: { branch: "dry-run", message: "chore: dry run" } }, - { cwd: repo, remote: "origin", threadId: null }, - ); - const out = await role(makeCtx()); - expect(out.meta.committed).toBe(false); - }); +const dryRunMeta = { + status: "committed" as const, + branch: "dry-run/placeholder", + commitSha: "0000000", +}; +describe("createCommitterRole", () => { test("dry-run skips pipeline", async () => { const agent: AgentFn = async () => { throw new Error("agent should not run"); @@ -72,39 +34,101 @@ describe("createCommitterRole", () => { const role = createCommitterRole(agent, { provider, dryRun: true, - dryRunMeta: { branch: "dry-run", message: "chore: dry run" }, + dryRunMeta, }); const out = await role(makeCtx()); expect(out.content).toBe("[dry-run] committer skipped"); - expect(out.meta).toEqual({ committed: true }); + expect(out.meta).toEqual(dryRunMeta); }); - test("commits and pushes when changes exist", async () => { - const { repo } = await setupRepoWithRemote(); - await appendFile(join(repo, "README.md"), "\nmore\n", "utf8"); + test("returns committed meta when extraction succeeds", async () => { + const committed = { + status: "committed" as const, + branch: "feat/widget", + commitSha: "deadbeef".repeat(5).slice(0, 40), + }; - const spy = spyOn(utilRole, "extractMetaOrThrow").mockResolvedValue({ - branch: "feat/test-commit", - message: "feat: add more", + const spy = spyOn(utilRole, "extractMetaOrThrow").mockResolvedValue(committed); + + const agent: AgentFn = async (_ctx, prompt) => + `Created branch ${committed.branch}, pushed. SHA ${committed.commitSha}.\n${prompt.slice(0, 80)}…`; + + const role = createCommitterRole(agent, { + provider, + dryRun: null, + dryRunMeta, }); - const agent: AgentFn = async () => "plan text"; - const role = createCommitterRole( - agent, - { provider, dryRun: null, dryRunMeta: { branch: "dry-run", message: "chore: dry run" } }, - { cwd: repo, remote: "origin", threadId: null }, - ); - const out = await role(makeCtx()); - expect(out.meta.committed).toBe(true); + expect(out.meta).toEqual(committed); expect(spy).toHaveBeenCalled(); - - const branches = await gitExec(repo, ["branch", "--list", "feat/test-commit"]); - expect(branches).toContain("feat/test-commit"); - - const remoteRefs = await gitExec(repo, ["ls-remote", "--heads", "origin", "feat/test-commit"]); - expect(remoteRefs.trim().length).toBeGreaterThan(0); - spy.mockRestore(); }); + + test("returns failed meta when extraction reports failure", async () => { + const failed = { + status: "failed" as const, + error: "working tree clean; nothing to commit", + logRef: null as string | null, + }; + + const spy = spyOn(utilRole, "extractMetaOrThrow").mockResolvedValue(failed); + + const agent: AgentFn = async () => "git status shows no changes; skipping branch and commit."; + + const role = createCommitterRole(agent, { + provider, + dryRun: null, + dryRunMeta, + }); + + const out = await role(makeCtx()); + expect(out.meta).toEqual(failed); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); + + test("returns failed meta with logRef when extraction includes it", async () => { + const failed = { + status: "failed" as const, + error: "push rejected", + logRef: "LOGREF01", + }; + + spyOn(utilRole, "extractMetaOrThrow").mockResolvedValue(failed); + + const agent: AgentFn = async () => "Remote rejected non-fast-forward."; + + const role = createCommitterRole(agent, { + provider, + dryRun: null, + dryRunMeta, + }); + + const out = await role(makeCtx()); + expect(out.meta).toEqual(failed); + }); + + test("onFail wraps extraction errors", async () => { + spyOn(utilRole, "extractMetaOrThrow").mockRejectedValue( + new Error("structured extraction failed"), + ); + + const agent: AgentFn = async () => "opaque agent output"; + + const role = createCommitterRole(agent, { + provider, + dryRun: null, + dryRunMeta, + }); + + const out = await role(makeCtx()); + expect(out.meta).toEqual({ + status: "failed", + error: "committer role threw before structured result", + logRef: null, + }); + expect(out.content).toContain("committer failed:"); + expect(out.content).toContain("structured extraction failed"); + }); }); diff --git a/packages/workflow-role-committer/package.json b/packages/workflow-role-committer/package.json index 05bcee2..ed1b50f 100644 --- a/packages/workflow-role-committer/package.json +++ b/packages/workflow-role-committer/package.json @@ -10,7 +10,6 @@ }, "dependencies": { "@uncaged/workflow": "workspace:*", - "@uncaged/workflow-util-agent": "workspace:*", "@uncaged/workflow-util-role": "workspace:*", "zod": "^4.0.0" }, diff --git a/packages/workflow-role-committer/src/committer.ts b/packages/workflow-role-committer/src/committer.ts index e2fb3fe..143e050 100644 --- a/packages/workflow-role-committer/src/committer.ts +++ b/packages/workflow-role-committer/src/committer.ts @@ -1,29 +1,28 @@ -import type { AgentFn, Role, RoleResult, ThreadContext } from "@uncaged/workflow"; +import type { AgentFn, Role, ThreadContext } from "@uncaged/workflow"; import { + createRole, decorateRole, - extractMetaOrThrow, type LlmProvider, onFail, withDryRun, } from "@uncaged/workflow-util-role"; import * as z from "zod/v4"; -import { gitExec } from "./git-exec.js"; +export const committerMetaSchema = z.discriminatedUnion("status", [ + z.object({ + status: z.literal("committed"), + branch: z.string(), + commitSha: z.string(), + }), + z.object({ + status: z.literal("failed"), + error: z.string(), + logRef: z.string().nullable(), + }), +]); -export const committerMetaSchema = z.object({ - committed: z - .boolean() - .describe("true if branch created, changes committed, and pushed successfully"), -}); export type CommitterMeta = z.infer; -const committerPlanSchema = z.object({ - branch: z.string().describe("Feature branch name, e.g. feat/slug or fix/slug"), - message: z.string().describe("Single-line conventional commit subject"), -}); - -export type CommitterPlanMeta = z.infer; - export type CommitterGitConfig = { cwd: string; remote: string; @@ -37,6 +36,12 @@ export const DEFAULT_COMMITTER_GIT_CONFIG: CommitterGitConfig = { threadId: null, }; +const DRY_RUN_COMMITTED_META: CommitterMeta = { + status: "committed", + branch: "dry-run/placeholder", + commitSha: "0000000", +}; + function resolveExtractDryRun(extractDryRun: boolean | null): boolean { return extractDryRun === true; } @@ -50,37 +55,18 @@ function summarizeThreadContext(ctx: ThreadContext): string { return lines.join("\n"); } -function sanitizeBranch(branch: string): string { - const t = branch.trim(); - if ( - t === "" || - t.includes("..") || - t.includes(" ") || - t.startsWith("-") || - t.includes("\n") || - t.includes("\t") - ) { - throw new Error(`invalid branch name: ${branch}`); - } - return t; -} - -function sanitizeCommitMessage(message: string): string { - const line = message.trim().split(/\r?\n/)[0] ?? ""; - if (line === "") { - throw new Error("commit message is empty"); - } - return line; -} - -function committerPlanPrompt(ctx: ThreadContext, gitConfig: CommitterGitConfig): string { +function committerSystemPrompt(ctx: ThreadContext, gitConfig: CommitterGitConfig): string { const threadLine = gitConfig.threadId !== null ? `Optional CLI context: run \`uncaged-workflow thread ${gitConfig.threadId}\` if available.\n` : ""; - return `You plan a git branch and a single-line conventional commit message for the following workflow thread. + return `You are the **git committer** for this workflow. Prior roles planned, implemented, and reviewed the change; your job is to perform git operations in the repository and report the outcome. +## Repository context + +- Working directory (run git commands here): \`${gitConfig.cwd}\` +- Remote name for push: \`${gitConfig.remote}\` ${threadLine} ## Thread context @@ -88,66 +74,44 @@ ${summarizeThreadContext(ctx)} ## Your task -Infer a good branch name (\`feat/\` or \`fix/\`) and a conventional commit **subject** (one line, no body). +1. Inspect the working tree (e.g. \`git status\`). If there is nothing to commit, stop and explain why in your reply. +2. Create a new branch using **conventional** naming (\`feat/\`, \`fix/\`, or \`chore/\` as appropriate). +3. Stage all intended changes, commit with a **single-line conventional commit subject**, and push the branch to \`${gitConfig.remote}\` (e.g. \`git push -u ${gitConfig.remote} \`). +4. In your reply, state clearly whether the push succeeded, the **exact branch name** used, and the **full commit SHA** from \`git rev-parse HEAD\` (or explain the failure). -Reply with enough detail that a maintainer understands the change; structured extraction will read \`branch\` and \`message\` from your answer.`; -} - -async function runCommitterPipeline( - ctx: ThreadContext, - agent: AgentFn, - extract: { provider: LlmProvider; dryRun: boolean | null; dryRunMeta: CommitterPlanMeta }, - gitConfig: CommitterGitConfig, -): Promise> { - const cwd = gitConfig.cwd; - const porcelain = await gitExec(cwd, ["status", "--porcelain"]); - if (porcelain.trim() === "") { - return { - content: "Working tree clean; nothing to commit.", - meta: { committed: false }, - }; - } - - const prompt = committerPlanPrompt(ctx, gitConfig); - const raw = await agent(ctx, prompt); - const plan = await extractMetaOrThrow("committer-plan", raw, committerPlanSchema, { - provider: extract.provider, - dryRun: resolveExtractDryRun(extract.dryRun), - dryRunMeta: extract.dryRunMeta, - }); - - const branch = sanitizeBranch(plan.branch); - const message = sanitizeCommitMessage(plan.message); - - await gitExec(cwd, ["checkout", "-b", branch]); - await gitExec(cwd, ["add", "-A"]); - await gitExec(cwd, ["commit", "-m", message]); - await gitExec(cwd, ["push", "-u", gitConfig.remote, branch]); - - return { - content: raw, - meta: { committed: true }, - }; +Structured extraction will read \`status\`, branch, commit SHA, or error details from your answer.`; } /** - * Git committer role: LLM proposes branch + message; this package runs git via `child_process`. - * Decorators match nerve semantics: dry-run skips work with `committed: true`; failures yield `committed: false`. + * Git committer role: the agent runs git (branch, commit, push); structured extraction yields {@link CommitterMeta}. + * Dry-run skips the agent and returns a stable committed placeholder; unexpected throws yield \`status: "failed"\`. */ export function createCommitterRole( adapter: AgentFn, - extract: { provider: LlmProvider; dryRun: boolean | null; dryRunMeta: CommitterPlanMeta }, + extract: { provider: LlmProvider; dryRun: boolean | null; dryRunMeta: CommitterMeta }, gitConfig: CommitterGitConfig = DEFAULT_COMMITTER_GIT_CONFIG, ): Role { - const inner: Role = async (ctx) => - runCommitterPipeline(ctx, adapter, extract, gitConfig); + const inner: Role = createRole({ + name: "committer", + schema: committerMetaSchema, + systemPrompt: async (ctx) => committerSystemPrompt(ctx, gitConfig), + agent: adapter, + extract, + }); return decorateRole(inner, [ withDryRun({ label: "committer", - meta: { committed: true }, + meta: DRY_RUN_COMMITTED_META, dryRun: resolveExtractDryRun(extract.dryRun), }), - onFail({ label: "committer", meta: { committed: false } }), + onFail({ + label: "committer", + meta: { + status: "failed", + error: "committer role threw before structured result", + logRef: null, + }, + }), ]); } diff --git a/packages/workflow-role-committer/src/git-exec.ts b/packages/workflow-role-committer/src/git-exec.ts deleted file mode 100644 index 3aeb525..0000000 --- a/packages/workflow-role-committer/src/git-exec.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { spawnCli } from "@uncaged/workflow-util-agent"; - -/** Runs `git` with args in `cwd`; throws if git exits non-zero. */ -export async function gitExec(cwd: string, args: readonly string[]): Promise { - const result = await spawnCli("git", [...args], { cwd, timeoutMs: null }); - if (result.ok) { - return result.value; - } - const error = result.error; - switch (error.kind) { - case "non_zero_exit": - throw new Error( - `git ${args.join(" ")} failed (exit ${error.exitCode}): ${error.stderr.trim() || error.stdout.trim()}`, - ); - case "timeout": - throw new Error(`git ${args.join(" ")} timed out`); - case "spawn_failed": - throw new Error(`git ${args.join(" ")} spawn failed: ${error.message}`); - } -} diff --git a/packages/workflow-role-committer/src/index.ts b/packages/workflow-role-committer/src/index.ts index e0bb49a..e7b42de 100644 --- a/packages/workflow-role-committer/src/index.ts +++ b/packages/workflow-role-committer/src/index.ts @@ -1,9 +1,7 @@ export { type CommitterGitConfig, type CommitterMeta, - type CommitterPlanMeta, committerMetaSchema, createCommitterRole, DEFAULT_COMMITTER_GIT_CONFIG, } from "./committer.js"; -export { gitExec } from "./git-exec.js"; diff --git a/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts b/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts index c054188..eec7026 100644 --- a/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts +++ b/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts @@ -61,7 +61,7 @@ function committerStep(): RoleStep { return { role: "committer", content: "commit", - meta: { committed: true }, + meta: { status: "committed", branch: "feat/issue-1", commitSha: "abc1234" }, timestamp: 4, }; } diff --git a/packages/workflow-template-solve-issue/src/roles.ts b/packages/workflow-template-solve-issue/src/roles.ts index b255297..68e7cf5 100644 --- a/packages/workflow-template-solve-issue/src/roles.ts +++ b/packages/workflow-template-solve-issue/src/roles.ts @@ -1,9 +1,5 @@ import type { AgentFn, Role } from "@uncaged/workflow"; -import { - type CommitterMeta, - type CommitterPlanMeta, - createCommitterRole, -} from "@uncaged/workflow-role-committer"; +import { type CommitterMeta, createCommitterRole } from "@uncaged/workflow-role-committer"; import { createRole } from "@uncaged/workflow-role-llm"; import { createReviewerRole, type ReviewerMeta } from "@uncaged/workflow-role-reviewer"; import type { LlmProvider } from "@uncaged/workflow-util-role"; @@ -53,9 +49,10 @@ const REVIEWER_DRY_RUN_META: ReviewerMeta = { approved: true, }; -const COMMITTER_PLAN_DRY_RUN_META: CommitterPlanMeta = { - branch: "dry-run", - message: "chore: dry run", +const COMMITTER_DRY_RUN_META: CommitterMeta = { + status: "committed", + branch: "dry-run/placeholder", + commitSha: "0000000", }; export type SolveIssueMeta = { @@ -142,7 +139,7 @@ export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssue { provider: extract.provider, dryRun: extract.dryRun, - dryRunMeta: COMMITTER_PLAN_DRY_RUN_META, + dryRunMeta: COMMITTER_DRY_RUN_META, }, committerGit, );