From 1e3a520a51c7a1b9f81ebccbde65ee92d6eb42de Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:04:38 +0300 Subject: [PATCH 1/7] WIP v2.5: CommandExecutor, presets, agents config --- THIRD_PARTY_NOTICES.md | 5 + bin/factory | 14 +- src/agents/env.ts | 39 ++++ src/agents/executor.ts | 172 +++++++++++++++ src/agents/final-message.ts | 20 ++ src/agents/presets/claude.ts | 104 +++++++++ src/agents/presets/codex.ts | 49 +++++ src/agents/presets/index.ts | 8 + src/agents/prompt.ts | 39 ++++ src/agents/types.ts | 37 ++++ src/agents/usage.ts | 50 +++++ src/config.ts | 49 +++++ src/executor.ts | 213 ++++--------------- src/watch.ts | 4 +- tests/executor.test.ts | 9 +- tests/ported/machinist/final-message.test.ts | 54 +++++ tests/ported/machinist/runner.test.ts | 92 ++++++++ tests/ported/machinist/usage.test.ts | 78 +++++++ 18 files changed, 849 insertions(+), 187 deletions(-) create mode 100644 src/agents/env.ts create mode 100644 src/agents/executor.ts create mode 100644 src/agents/final-message.ts create mode 100644 src/agents/presets/claude.ts create mode 100644 src/agents/presets/codex.ts create mode 100644 src/agents/presets/index.ts create mode 100644 src/agents/prompt.ts create mode 100644 src/agents/types.ts create mode 100644 src/agents/usage.ts create mode 100644 tests/ported/machinist/final-message.test.ts create mode 100644 tests/ported/machinist/runner.test.ts create mode 100644 tests/ported/machinist/usage.test.ts diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index b7e6633..64ea4fe 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -16,6 +16,11 @@ Source: https://github.com/owainlewis/machinist (MIT, Copyright (c) 2026 Owain L - `dashboard/public/lib/task-presentation.js` from `internal/controlplane/web/src/task-presentation.js` - `dashboard/public/lib/routes.js` from `internal/controlplane/web/src/routes.js` - `src/revision.ts` from `internal/runner/revision.go` (shape from `internal/protocol/revision.go`) +- `src/agents/executor.ts` from `internal/runner/runner.go` (process group kill from `process_unix.go`) +- `src/agents/env.ts` from `internal/runner/runner.go` +- `src/agents/final-message.ts` from `internal/runner/codex_usage.go` +- `src/agents/presets/codex.ts` from `internal/runner/codex_usage.go` +- `src/agents/usage.ts` from `internal/runner/codex_usage.go` ## owainlewis/assembler@7cac671 diff --git a/bin/factory b/bin/factory index da0a916..1ec6d49 100755 --- a/bin/factory +++ b/bin/factory @@ -8,8 +8,8 @@ import { resolve } from "node:path"; import { GitHub } from "../src/github"; import { GitCommandRunner, Git } from "../src/git"; import { FactoryState, DEFAULT_DB_PATH } from "../src/state"; -import { ClaudeExecutor } from "../src/executor"; -import { loadConfig } from "../src/config"; +import { CommandExecutor } from "../src/agents/executor"; +import { loadConfig, type FactoryConfig } from "../src/config"; import { EXIT, UsageError, failureJson, successJson } from "../src/cli-output"; import { advanceIssue, pollOnce, recoverInFlight, startWatch, type WatchDeps } from "../src/watch"; import { ShellGateRunner } from "../src/gates"; @@ -70,7 +70,7 @@ function buildWatchDeps(cloneDir: string): WatchDeps { github: new GitHub(), git: new Git(new GitCommandRunner()), state: new FactoryState(flag("db") ?? DEFAULT_DB_PATH), - executor: new ClaudeExecutor(), + executor: new CommandExecutor(config.agents, config.stages), gateRunner: new ShellGateRunner(), cloneDir, workspacesDir: flag("workspaces") ?? defaultWorkspacesDir(), @@ -80,7 +80,7 @@ function buildWatchDeps(cloneDir: string): WatchDeps { async function cmdWatch(): Promise { const cloneDir = await resolveCloneDir(); const config = await loadConfig(cloneDir); - const deps = buildWatchDeps(cloneDir); + const deps = buildWatchDeps(cloneDir, config); console.log(`factory watch: polling ${config.repo} every ${config.pollIntervalSeconds}s`); // Re-drive anything a crashed or previously-killed process left sitting in // a running label before the first poll — otherwise it just sits there, @@ -110,7 +110,7 @@ async function cmdRun(): Promise { const config = await loadConfig(cloneDir); const issueNumber = Number(flag("issue")); if (!issueNumber) throw new UsageError("run: --issue is required"); - const deps = buildWatchDeps(cloneDir); + const deps = buildWatchDeps(cloneDir, config); const issue = await deps.github.getIssue(config.repo, issueNumber); const outcome = await advanceIssue(deps, config, issue); if (has("json")) console.log(successJson({ issue: issueNumber, outcome }, outcome !== "failed")); @@ -124,7 +124,7 @@ async function cmdRun(): Promise { async function cmdTick(): Promise { const cloneDir = await resolveCloneDir(); const config = await loadConfig(cloneDir); - const deps = buildWatchDeps(cloneDir); + const deps = buildWatchDeps(cloneDir, config); const result = await pollOnce(deps, config); console.log(has("json") ? successJson(result, !result.paused) : JSON.stringify(result, null, 2)); if (result.paused) process.exit(EXIT.paused); @@ -318,7 +318,7 @@ async function cmdDashboard(): Promise { async function cmdUp(): Promise { const cloneDir = await resolveCloneDir(); const config = await loadConfig(cloneDir); - const deps = buildWatchDeps(cloneDir); + const deps = buildWatchDeps(cloneDir, config); const recovered = await recoverInFlight(deps, config); if (recovered.length) console.log(`factory up: recovered #${recovered.join(", #")}`); startWatch(deps, config, (r) => { diff --git a/src/agents/env.ts b/src/agents/env.ts new file mode 100644 index 0000000..9b77304 --- /dev/null +++ b/src/agents/env.ts @@ -0,0 +1,39 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/runner.go:669-706 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: also strips GH_TOKEN, GITHUB_TOKEN and FACTORY_* (the runner's own secrets, audit finding #14), which machinist has no equivalent of. + +// The agent inherits everything else (PATH, HOME, its own model key): it needs +// a key to work, so the residual risk is a spend-capped key, documented in the README. +const STRIPPED_ENV_PREFIXES = ["GH_TOKEN", "GITHUB_TOKEN", "FACTORY_", "GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"]; + +// Repository-pointing git variables. A leaked GIT_DIR or GIT_INDEX_FILE makes +// the agent's git commands act on the wrong repository. +const REPOSITORY_GIT_ENV = new Set([ + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_CEILING_DIRECTORIES", + "GIT_COMMON_DIR", + "GIT_CONFIG", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_PARAMETERS", + "GIT_DIR", + "GIT_DISCOVERY_ACROSS_FILESYSTEM", + "GIT_GRAFT_FILE", + "GIT_IMPLICIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_INTERNAL_SUPER_PREFIX", + "GIT_NAMESPACE", + "GIT_NO_REPLACE_OBJECTS", + "GIT_OBJECT_DIRECTORY", + "GIT_PREFIX", + "GIT_REPLACE_REF_BASE", + "GIT_SHALLOW_FILE", + "GIT_WORK_TREE", +]); + +export function sanitizeEnv(env: NodeJS.ProcessEnv): Record { + const out: Record = {}; + for (const [key, value] of Object.entries(env)) { + if (value === undefined) continue; + if (REPOSITORY_GIT_ENV.has(key) || STRIPPED_ENV_PREFIXES.some((p) => key === p || key.startsWith(p))) continue; + out[key] = value; + } + return out; +} diff --git a/src/agents/executor.ts b/src/agents/executor.ts new file mode 100644 index 0000000..fdd6deb --- /dev/null +++ b/src/agents/executor.ts @@ -0,0 +1,172 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/runner.go:190-240 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the process-group kill is process_unix.go, and TypeScript on Bun (`detached` is setsid); the event cap and 1 MiB line cap follow events.go and codex_usage.go; the tool-call cap and stderr tail are the factory's own (audit finding #15). +// The one executor: spawn any agent CLI, feed the prompt, watch its output +// through the preset's line parser, and kill the whole process group on a +// timeout or a runaway tool-call count. + +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { runDir } from "../artifacts"; +import { aggregateStageEvents, type Executor, type StageEvent, type StageRunOptions, type StageRunResult } from "../executor"; +import { sanitizeEnv } from "./env"; +import { renderPrompt } from "./prompt"; +import { PRESETS } from "./presets"; +import type { AgentConfig, AgentPreset, StageAgents } from "./types"; + +const DEFAULT_TIMEOUT_MINUTES = 15; +export const MAX_EVENT_LINE_BYTES = 1 << 20; +export const MAX_RECORDED_OUTPUT_BYTES = 64 << 20; + +export interface ResolvedAgent { + readonly name: string; + readonly config: AgentConfig; + readonly preset?: AgentPreset; +} + +export function resolveAgent(agents: Record, stages: StageAgents, stage: keyof StageAgents & string): ResolvedAgent { + const name = stages[stage] ?? stages.default ?? "claude"; + const config = agents[name]; + if (!config) throw new Error(`stage ${stage} uses agent "${name}", which is not in config.agents`); + const preset = config.preset ? PRESETS[config.preset] : undefined; + if (config.preset && !preset) throw new Error(`agent "${name}": unknown preset "${config.preset}"`); + return { name, config, preset }; +} + +// Placeholders in a config command. Anything else is passed through verbatim. +export function renderCommand(command: readonly string[], values: { prompt: string; promptFile: string; model: string }): { argv: string[]; usesStdin: boolean } { + let usesStdin = true; + const argv = command.map((part) => + part.replace(/\{\{(prompt|promptFile|model)\}\}/g, (_m, key: "prompt" | "promptFile" | "model") => { + if (key !== "model") usesStdin = false; + return values[key]; + }), + ); + return { argv, usesStdin }; +} + +export class CommandExecutor implements Executor { + constructor( + private readonly agents: Record, + private readonly stages: StageAgents, + ) {} + + async runStage(opts: StageRunOptions): Promise { + const agent = resolveAgent(this.agents, this.stages, opts.stage); + const scratch = mkdtempSync(join(tmpdir(), `factory-scratch-${opts.issue}-`)); + try { + return await this.spawnStage(opts, agent, scratch); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + } + + private async spawnStage(opts: StageRunOptions, agent: ResolvedAgent, scratch: string): Promise { + const artifactDir = join(opts.cwd, runDir(opts.issue)); + mkdirSync(artifactDir, { recursive: true }); + + let argv: readonly string[]; + let stdin: string | undefined; + if (agent.preset?.ownsPrompt) { + ({ argv, stdin } = agent.preset.command(opts, agent.config, "")); + } else { + const prompt = await renderPrompt(opts); + if (agent.config.command) { + const promptFile = join(scratch, "prompt.md"); + writeFileSync(promptFile, prompt); + const rendered = renderCommand(agent.config.command, { prompt, promptFile, model: agent.config.model ?? "" }); + argv = rendered.argv; + stdin = rendered.usesStdin ? prompt : undefined; + } else if (agent.preset) { + ({ argv, stdin } = agent.preset.command(opts, agent.config, prompt)); + } else { + throw new Error(`agent "${agent.name}" has neither a preset nor a command`); + } + } + + const proc = Bun.spawn([...argv], { + cwd: opts.cwd, + stdin: stdin === undefined ? "ignore" : new Blob([stdin]), + stdout: "pipe", + stderr: "pipe", + detached: true, // its own process group, so a kill reaches the agent's children too + env: { + ...sanitizeEnv(process.env), + FACTORY_ARTIFACT_DIR: artifactDir, + FACTORY_ISSUE: String(opts.issue), + FACTORY_STAGE: opts.stage, + FACTORY_SCRATCH_DIR: scratch, + }, + }); + const killGroup = () => { + // ESRCH means it already exited; that is the goal. + try { + process.kill(-proc.pid, "SIGKILL"); + } catch { + proc.kill(); + } + }; + const events: StageEvent[] = []; + let toolCalls = 0; + let killedReason: string | undefined; + let usageComplete = agent.preset !== undefined; + let recorded = 0; + // Read alongside stdout, not after: an unread pipe can fill its OS buffer + // and stall the child, and launch-time errors go to stderr only. + const stderrPromise = new Response(proc.stderr).text(); + + const timeoutMinutes = opts.timeoutMinutes ?? DEFAULT_TIMEOUT_MINUTES; + const timer = setTimeout(() => { + killedReason = `stage exceeded stageTimeoutMinutes=${timeoutMinutes}`; + killGroup(); + }, timeoutMinutes * 60_000); + + const handle = (line: string) => { + if (!agent.preset) return; + if (Buffer.byteLength(line) > MAX_EVENT_LINE_BYTES) { + // Dropped, not parsed; if it was the terminal usage event the count is gone. + if (agent.preset.isUsageCandidate(line.slice(0, 4096))) usageComplete = false; + return; + } + for (const e of agent.preset.parseLine(line)) { + recorded += Buffer.byteLength(e.text ?? ""); + if (recorded > MAX_RECORDED_OUTPUT_BYTES) { + if (events.at(-1)?.kind !== "truncated") events.push({ kind: "truncated", text: `recording stopped after ${MAX_RECORDED_OUTPUT_BYTES} output bytes; the agent keeps running` }); + } else events.push(e); + if (e.kind !== "tool_use") continue; + toolCalls += 1; + if (opts.maxToolCalls && toolCalls > opts.maxToolCalls && !killedReason) { + killedReason = `stage exceeded maxToolCalls=${opts.maxToolCalls}`; + killGroup(); + } + } + }; + + try { + const reader = proc.stdout.getReader(); + const decoder = new TextDecoder(); + let buffer = ""; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + const lines = buffer.split("\n"); + buffer = lines.pop() ?? ""; + for (const line of lines) handle(line); + // An unterminated line past the cap is dropped now, not buffered without bound. + if (buffer.length > MAX_EVENT_LINE_BYTES) { + if (agent.preset?.isUsageCandidate(buffer.slice(0, 4096))) usageComplete = false; + buffer = ""; + } + } + if (buffer.trim()) handle(buffer); + } finally { + clearTimeout(timer); + } + + const [exitCode, stderr] = await Promise.all([proc.exited, stderrPromise]); + const stderrTail = stderr.trim().slice(-4000) || undefined; + const base0 = aggregateStageEvents(events, exitCode, stderrTail); + const base = { ...base0, agent: agent.name, model: agent.config.model ?? null, usageComplete: usageComplete && base0.usageComplete !== false }; + return killedReason ? { ...base, exitCode: exitCode || 1, killedReason } : base; + } +} diff --git a/src/agents/final-message.ts b/src/agents/final-message.ts new file mode 100644 index 0000000..c34b864 --- /dev/null +++ b/src/agents/final-message.ts @@ -0,0 +1,20 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage.go:455-473 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the cut is by code point in JS, which is the same rune-boundary rule. + +export const MAX_FINAL_MESSAGE_BYTES = 16 * 1024; +const TRUNCATION_MARK = "\n\n[truncated]"; + +// The agent's last message, shown as the run summary. Cut on a character +// boundary so a multi-byte character is never split. +export function truncateFinalMessage(text: string): string { + const message = text.trim(); + if (Buffer.byteLength(message) <= MAX_FINAL_MESSAGE_BYTES) return message; + let out = ""; + let used = 0; + for (const ch of message) { + const n = Buffer.byteLength(ch); + if (used + n > MAX_FINAL_MESSAGE_BYTES) break; + out += ch; + used += n; + } + return out + TRUNCATION_MARK; +} diff --git a/src/agents/presets/claude.ts b/src/agents/presets/claude.ts new file mode 100644 index 0000000..a2bd70f --- /dev/null +++ b/src/agents/presets/claude.ts @@ -0,0 +1,104 @@ +// Claude Code: `claude -p /factory- N` with stream-json output. The argv +// is pinned byte for byte by tests/agents.test.ts; the repo's own skill is the +// prompt, so this preset ignores the rendered one. +// Flags checked against `claude --help` on 2026-09-23. + +import type { StageEvent, StageRunOptions } from "../../executor"; +import { STAGE_GUIDANCE, stageSettings } from "../../stage-permissions"; +import type { AgentPreset } from "../types"; +import { isUsageResultCandidate, readUsage } from "../usage"; + +// One line of Claude Code's --output-format stream-json. Only the fields this +// runner needs; the real stream carries more. +interface StreamJsonLine { + type?: string; + subtype?: string; + result?: string; + cost_usd?: number; + total_cost_usd?: number; + message?: { + content?: Array<{ type: string; text?: string; name?: string }>; + usage?: { input_tokens?: number; output_tokens?: number }; + }; + usage?: unknown; + permission_denials?: Array<{ tool_name?: string; tool_input?: { file_path?: string; command?: string } }>; +} + +export function parseStreamJsonLine(line: string): StageEvent[] { + const trimmed = line.trim(); + if (!trimmed) return []; + let parsed: StreamJsonLine; + try { + parsed = JSON.parse(trimmed); + } catch { + // A cut-off final result means the totals cannot be trusted. + return isUsageResultCandidate(trimmed, "result") ? [{ kind: "usage", invalid: true }] : []; + } + const events: StageEvent[] = []; + + if (parsed.type === "assistant" && parsed.message?.content) { + for (const block of parsed.message.content) { + if (block.type === "text" && block.text) { + events.push({ kind: "text", text: block.text }); + } else if (block.type === "tool_use" && block.name) { + events.push({ kind: "tool_use", toolName: block.name }); + } + } + if (parsed.message.usage) { + events.push({ + kind: "usage", + tokensIn: parsed.message.usage.input_tokens ?? 0, + tokensOut: parsed.message.usage.output_tokens ?? 0, + }); + } + } else if (parsed.type === "result") { + const denials = (parsed.permission_denials ?? []).map( + (d) => `${d.tool_name ?? "tool"} ${d.tool_input?.file_path ?? d.tool_input?.command ?? ""}`.trim(), + ); + // Cumulative usage incl. cache tokens (machinist codex_usage.go); a result with no usage keeps the per-message sums. + if (parsed.usage !== undefined) { + const usage = readUsage(parsed.usage, true); + events.push(usage ? { kind: "usage", tokensIn: usage.tokensIn, tokensOut: usage.tokensOut, total: true } : { kind: "usage", invalid: true }); + } + events.push({ + kind: "result", + costUsd: parsed.total_cost_usd ?? parsed.cost_usd ?? 0, + text: parsed.subtype, + ...(denials.length ? { denials } : {}), + ...(parsed.result ? { finalText: parsed.result } : {}), + }); + } + return events; +} + +export function claudeArgs(opts: StageRunOptions): string[] { + return [ + "-p", + `/factory-${opts.stage} ${opts.issue}`, + "--output-format", + "stream-json", + "--verbose", + "--permission-mode", + "dontAsk", + "--permission-prompts", + "none", + "--setting-sources", + "project,local", + "--settings", + stageSettings(opts.stage, opts.issue, opts.agentCommands), + "--append-system-prompt", + STAGE_GUIDANCE, + "--no-session-persistence", + "--max-budget-usd", + String(opts.maxBudgetUsd), + ]; +} + +export const claudePreset: AgentPreset = { + name: "claude", + binary: "claude", + ownsPrompt: true, + command: (opts, agent) => ({ argv: ["claude", ...claudeArgs(opts), ...(agent.model ? ["--model", agent.model] : [])] }), + parseLine: parseStreamJsonLine, + isUsageCandidate: (line) => isUsageResultCandidate(line, "result"), +}; diff --git a/src/agents/presets/codex.ts b/src/agents/presets/codex.ts new file mode 100644 index 0000000..4d6b6b5 --- /dev/null +++ b/src/agents/presets/codex.ts @@ -0,0 +1,49 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage.go:508-560 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: input and output tokens are kept apart (machinist sums them); tool calls are counted from item.completed command, file and MCP items, which machinist does not need. +// Codex CLI: `codex exec --json -s workspace-write -` with the prompt on stdin. +// Read-only is not usable here because every stage writes its artifacts; the +// runner's protected-path diff and gates are the backstop. + +import type { StageEvent } from "../../executor"; +import type { AgentPreset } from "../types"; +import { isUsageResultCandidate, readUsage } from "../usage"; + +interface CodexLine { + type?: string; + usage?: unknown; + item?: { type?: string; text?: string; command?: string }; +} + +const TOOL_ITEMS = new Set(["command_execution", "file_change", "mcp_tool_call", "web_search"]); + +export function parseCodexLine(line: string): StageEvent[] { + const trimmed = line.trim(); + if (!trimmed) return []; + let parsed: CodexLine; + try { + parsed = JSON.parse(trimmed); + } catch { + return isUsageResultCandidate(trimmed, "turn.completed") ? [{ kind: "usage", invalid: true }] : []; + } + const item = parsed.item; + if (parsed.type === "item.completed" && item) { + if (item.type === "agent_message" && item.text) return [{ kind: "text", text: item.text, finalText: item.text }]; + if (item.type && TOOL_ITEMS.has(item.type)) return [{ kind: "tool_use", toolName: item.type === "command_execution" ? "Bash" : item.type }]; + return []; + } + if (parsed.type === "turn.completed") { + const usage = readUsage(parsed.usage, false); + return [usage ? { kind: "usage", tokensIn: usage.tokensIn, tokensOut: usage.tokensOut, total: true } : { kind: "usage", invalid: true }]; + } + return []; +} + +export const codexPreset: AgentPreset = { + name: "codex", + binary: "codex", + command: (_opts, agent, prompt) => ({ + argv: ["codex", "exec", "--json", "-s", "workspace-write", ...(agent.model ? ["-m", agent.model] : []), "-"], + stdin: prompt, + }), + parseLine: parseCodexLine, + isUsageCandidate: (line) => isUsageResultCandidate(line, "turn.completed"), +}; diff --git a/src/agents/presets/index.ts b/src/agents/presets/index.ts new file mode 100644 index 0000000..70dbfd1 --- /dev/null +++ b/src/agents/presets/index.ts @@ -0,0 +1,8 @@ +import type { AgentPreset } from "../types"; +import { claudePreset } from "./claude"; +import { codexPreset } from "./codex"; + +export const PRESETS: Record = { + claude: claudePreset, + codex: codexPreset, +}; diff --git a/src/agents/prompt.ts b/src/agents/prompt.ts new file mode 100644 index 0000000..62b8fee --- /dev/null +++ b/src/agents/prompt.ts @@ -0,0 +1,39 @@ +// The prompt a non-Claude agent receives: the stage skill's body, then the +// artifact contract. Any agent that can write a file can do a stage, so the +// contract is the only interface. Claude runs `/factory- N` itself. +// Shape after owainlewis/machinist@3943516 internal/runner/runner.go:220 (the contract suffix). + +import { readFile } from "node:fs/promises"; +import { COMMENT_FILENAMES, JSON_FILENAMES, runDir } from "../artifacts"; +import type { StageRunOptions } from "../executor"; + +export function stripFrontmatter(text: string): string { + return text.startsWith("---\n") ? text.slice(text.indexOf("\n---", 3) + 4).replace(/^\n+/, "") : text; +} + +export function artifactContract(opts: Pick): string { + return [ + "## Artifact contract", + "", + `You run in the issue's worktree. Write your results as files in $FACTORY_ARTIFACT_DIR (${runDir(opts.issue)}/):`, + `- ${JSON_FILENAMES[opts.stage]}: the structured result, exactly as the instructions above describe.`, + `- ${COMMENT_FILENAMES[opts.stage]}: the comment the runner posts on the issue.`, + "- question-comment.md: only when you cannot proceed without an answer from a human.", + "", + "You have no GitHub access and cannot push or merge; the runner does that. Finish by exiting.", + ].join("\n"); +} + +export async function renderPrompt(opts: StageRunOptions): Promise { + const skill = await readFile(`${opts.cwd}/.claude/skills/factory-${opts.stage}/SKILL.md`, "utf8").catch(() => { + throw new Error(`stage skill .claude/skills/factory-${opts.stage}/SKILL.md not found in ${opts.cwd}; run \`factory install --update\``); + }); + return [ + `Stage: ${opts.stage}. Issue number: ${opts.issue}. Wherever the instructions say , use ${opts.issue}.`, + "", + stripFrontmatter(skill).trim(), + "", + artifactContract(opts), + "", + ].join("\n"); +} diff --git a/src/agents/types.ts b/src/agents/types.ts new file mode 100644 index 0000000..0101aba --- /dev/null +++ b/src/agents/types.ts @@ -0,0 +1,37 @@ +// The factory knows no agent by name. An agent is config (a command, or a +// preset that fills one in) plus an optional line parser for the numbers. + +import type { StageEvent, StageName, StageRunOptions } from "../executor"; + +export interface AgentConfig { + // A built-in preset ("claude", "codex"); it supplies the command and parser. + readonly preset?: string; + // Any other CLI: argv with {{prompt}}, {{promptFile}} and {{model}} placeholders. + // With no placeholder the prompt goes to stdin. + readonly command?: readonly string[]; + readonly model?: string; +} + +// stages.default is the agent for every stage; a stage name overrides it. +export type StageAgents = Partial>; + +export interface StageInvocation { + readonly argv: readonly string[]; + readonly stdin?: string; +} + +export interface AgentPreset { + readonly name: string; + // The binary `factory doctor` looks for. + readonly binary: string; + // Builds argv and stdin. `prompt` is the rendered stage prompt. + command(opts: StageRunOptions, agent: AgentConfig, prompt: string): StageInvocation; + parseLine(line: string): StageEvent[]; + // True for a line (or its first bytes) that would have been the terminal usage event. + isUsageCandidate(line: string): boolean; + // Claude runs the repo's `/factory-` skill itself; others get the skill body inlined. + readonly ownsPrompt?: boolean; +} + +// Where the runner tells the agent to put its files. +export const ARTIFACT_ENV = ["FACTORY_ARTIFACT_DIR", "FACTORY_ISSUE", "FACTORY_STAGE", "FACTORY_SCRATCH_DIR"] as const; diff --git a/src/agents/usage.ts b/src/agents/usage.ts new file mode 100644 index 0000000..49e11e4 --- /dev/null +++ b/src/agents/usage.ts @@ -0,0 +1,50 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage.go:508-640 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: input and output tokens are returned apart (machinist sums them into one number); int64 overflow is any value past Number.MAX_SAFE_INTEGER; the candidate scan is a depth-aware string scanner rather than a json.Decoder. + +// A terminal event's token usage, or null when it cannot be trusted. The last +// terminal event wins; a bad one makes usage "not reported" rather than zero. +export interface TokenUsage { + readonly tokensIn: number; + readonly tokensOut: number; +} + +const count = (v: unknown): number | null => (typeof v === "number" && Number.isSafeInteger(v) && v >= 0 ? v : null); + +// Codex: input + output. Claude (`cache`): input + cache creation + cache read + output. +export function readUsage(raw: unknown, cache: boolean): TokenUsage | null { + if (typeof raw !== "object" || raw === null) return null; + const u = raw as Record; + const input = count(u.input_tokens); + const output = count(u.output_tokens); + if (input === null || output === null) return null; + let tokensIn = input; + if (cache) { + const created = count(u.cache_creation_input_tokens); + const read = count(u.cache_read_input_tokens); + if (created === null || read === null) return null; + tokensIn += created + read; + } + return Number.isSafeInteger(tokensIn + output) ? { tokensIn, tokensOut: output } : null; +} + +// True when the line's own top-level "type" is `resultType`, even if the rest +// of the line is cut off or malformed. A nested `"type"` does not count. +export function isUsageResultCandidate(line: string, resultType: string): boolean { + let depth = 0; + for (let i = 0; i < line.length; i++) { + const c = line[i]; + if (c === "{" || c === "[") depth++; + else if (c === "}" || c === "]") depth--; + else if (c === '"') { + let j = i + 1; + while (j < line.length && line[j] !== '"') j += line[j] === "\\" ? 2 : 1; + if (j >= line.length) return false; + const key = line.slice(i + 1, j); + i = j; + if (depth === 1 && key === "type") { + const m = /^\s*:\s*"((?:[^"\\]|\\.)*)"/.exec(line.slice(j + 1)); + return m !== null && m[1] === resultType; + } + } + } + return false; +} diff --git a/src/config.ts b/src/config.ts index 28b4f28..a11fe5c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -3,6 +3,9 @@ // only reads it, and `factory doctor` checks it exists. Missing fields fall // back to DEFAULT_CONFIG so a minimal config.json still works. +import { PRESETS } from "./agents/presets"; +import type { AgentConfig, StageAgents } from "./agents/types"; + export interface RiskPolicy { // Low risk (docs, test-only, a single non-protected module) is eligible for // auto-approve; the toggle in state.ts still has to be on. @@ -48,6 +51,9 @@ export interface FactoryConfig { readonly maxToolCalls: number; // kills a runaway stage before it burns budget readonly gates: readonly GateSpec[]; // read by .factory/gates.sh readonly agentCommands: AgentCommands; + // Named agents, each a preset or a command; `stages` says which one runs a stage. + readonly agents: Readonly>; + readonly stages: StageAgents; } export const DEFAULT_CONFIG: FactoryConfig = { @@ -64,6 +70,8 @@ export const DEFAULT_CONFIG: FactoryConfig = { maxToolCalls: 60, gates: [], agentCommands: { read: [], build: [], verify: [] }, + agents: { claude: { preset: "claude" } }, + stages: { default: "claude" }, }; export function mergeConfig(partial: Partial): FactoryConfig { @@ -73,6 +81,8 @@ export function mergeConfig(partial: Partial): FactoryConfig { riskPolicy: { ...DEFAULT_CONFIG.riskPolicy, ...partial.riskPolicy }, maxBudgetUsd: { ...DEFAULT_CONFIG.maxBudgetUsd, ...partial.maxBudgetUsd }, agentCommands: { ...DEFAULT_CONFIG.agentCommands, ...partial.agentCommands }, + agents: { ...DEFAULT_CONFIG.agents, ...partial.agents }, + stages: { ...DEFAULT_CONFIG.stages, ...partial.stages }, }; } @@ -95,6 +105,8 @@ const TOP_LEVEL: Record = { maxToolCalls: "posInt", gates: "object", agentCommands: "object", + agents: "object", + stages: "object", riskCriteria: "object", }; @@ -138,6 +150,43 @@ export function configProblems(raw: unknown): string[] { else checkKeys(g as Record, { name: "string", cmd: "string", required: "boolean" }, `gates[${i}].`, problems); }); } + problems.push(...agentProblems(cfg.agents, cfg.stages)); + return problems; +} + +const STAGE_KEYS = ["default", "triage", "plan", "build", "verify", "pr"]; + +// An agent is a preset or a command. `{{prompt}}` in the executable slot would +// run the prompt as a program (assembler validateConfig), so it is refused. +function agentProblems(agents: unknown, stages: unknown): string[] { + const problems: string[] = []; + const named = new Set(["claude"]); + if (agents !== undefined && typeof agents === "object" && agents !== null && !Array.isArray(agents)) { + for (const [name, raw] of Object.entries(agents as Record)) { + if (name.startsWith("_")) continue; + named.add(name); + const where = `agents.${name}.`; + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + problems.push(`agents.${name}: expected an object`); + continue; + } + const a = raw as Record; + checkKeys(a, { preset: "string", command: "strings", model: "string" }, where, problems); + if (typeof a.preset === "string" && !PRESETS[a.preset]) problems.push(`${where}preset: unknown "${a.preset}" (built in: ${Object.keys(PRESETS).join(", ")})`); + if (a.preset === undefined && a.command === undefined) problems.push(`${where}needs a "preset" or a "command"`); + if (Array.isArray(a.command)) { + if (a.command.length === 0) problems.push(`${where}command: must not be empty`); + else if (/\{\{/.test(String(a.command[0]))) problems.push(`${where}command: the executable cannot be a placeholder`); + } + } + } else if (agents !== undefined) problems.push("agents: expected an object"); + if (stages !== undefined && typeof stages === "object" && stages !== null && !Array.isArray(stages)) { + for (const [stage, agent] of Object.entries(stages as Record)) { + if (stage.startsWith("_")) continue; + if (!STAGE_KEYS.includes(stage)) problems.push(`stages.${stage}: unknown stage (allowed: ${STAGE_KEYS.join(", ")})`); + else if (typeof agent !== "string" || !named.has(agent)) problems.push(`stages.${stage}: "${String(agent)}" is not an agent in config.agents`); + } + } else if (stages !== undefined) problems.push("stages: expected an object"); return problems; } diff --git a/src/executor.ts b/src/executor.ts index 5470900..079cec1 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -1,27 +1,30 @@ -// Runs one stage of the loop. "claude" spawns the real CLI; "replay" reads a -// recorded stream-json fixture with no model and no network — this is what -// `bun test` uses, per the plan's rule that `make check` needs neither. -// -// Invocation (verified against `claude --help` on this machine, 2026-09-23): -// claude -p "/factory- " --output-format stream-json --verbose -// --permission-mode dontAsk --setting-sources project,local -// --no-session-persistence --max-budget-usd -// All six flags exist as written in the plan; no corrections were needed. +// The stage contract: types, result aggregation and the replay executor that +// `bun test` uses (no model, no network). Real agents run through +// src/agents/executor.ts; the Claude and Codex line parsers live in +// src/agents/presets/. import type { AgentCommands } from "./config"; -import { STAGE_GUIDANCE, stageSettings } from "./stage-permissions"; +import { truncateFinalMessage } from "./agents/final-message"; +import { parseStreamJsonLine } from "./agents/presets/claude"; export type StageName = "triage" | "plan" | "build" | "verify" | "pr"; export interface StageEvent { - readonly kind: "tool_use" | "text" | "usage" | "result"; + readonly kind: "tool_use" | "text" | "usage" | "result" | "truncated"; readonly toolName?: string; readonly text?: string; readonly tokensIn?: number; readonly tokensOut?: number; + // On "usage" events: `total` means these are the run's final totals (replace + // what was summed); `invalid` means the terminal event was unreadable, so + // tokens are "not reported". + readonly total?: boolean; + readonly invalid?: boolean; readonly costUsd?: number; // On "result" events: tool calls the permission system refused, as "Tool path". readonly denials?: string[]; + // The agent's own closing message (Claude `result.result`, Codex `agent_message`). + readonly finalText?: string; } export interface StageRunOptions { @@ -32,6 +35,8 @@ export interface StageRunOptions { readonly timeoutMinutes?: number; readonly maxToolCalls?: number; readonly agentCommands?: AgentCommands; + // The agent name from config; recorded in stage_runs. Unset means "claude". + readonly agent?: string; } export interface StageRunResult { @@ -51,187 +56,53 @@ export interface StageRunResult { readonly stderrTail?: string; // Tool calls `claude` refused (from the result event's permission_denials). readonly permissionDenials: string[]; + // The agent's last message, capped at 16 KiB (machinist final_message.go). + readonly finalMessage?: string; + // False when an over-long event line or a missing parser meant tokens are + // not a full count; the dashboard shows "Not reported" rather than zero. + readonly usageComplete?: boolean; + readonly agent?: string; + readonly model?: string | null; } export interface Executor { runStage(opts: StageRunOptions): Promise; } -// One line of Claude Code's --output-format stream-json. Only the fields this -// runner needs; the real stream carries more. -interface StreamJsonLine { - type?: string; - subtype?: string; - cost_usd?: number; - total_cost_usd?: number; - message?: { - content?: Array<{ type: string; text?: string; name?: string }>; - usage?: { input_tokens?: number; output_tokens?: number }; - }; - usage?: { input_tokens?: number; output_tokens?: number }; - permission_denials?: Array<{ tool_name?: string; tool_input?: { file_path?: string; command?: string } }>; -} - -export function parseStreamJsonLine(line: string): StageEvent[] { - const trimmed = line.trim(); - if (!trimmed) return []; - let parsed: StreamJsonLine; - try { - parsed = JSON.parse(trimmed); - } catch { - return []; - } - const events: StageEvent[] = []; - - if (parsed.type === "assistant" && parsed.message?.content) { - for (const block of parsed.message.content) { - if (block.type === "text" && block.text) { - events.push({ kind: "text", text: block.text }); - } else if (block.type === "tool_use" && block.name) { - events.push({ kind: "tool_use", toolName: block.name }); - } - } - if (parsed.message.usage) { - events.push({ - kind: "usage", - tokensIn: parsed.message.usage.input_tokens ?? 0, - tokensOut: parsed.message.usage.output_tokens ?? 0, - }); - } - } else if (parsed.type === "result") { - const denials = (parsed.permission_denials ?? []).map( - (d) => `${d.tool_name ?? "tool"} ${d.tool_input?.file_path ?? d.tool_input?.command ?? ""}`.trim(), - ); - events.push({ - kind: "result", - costUsd: parsed.total_cost_usd ?? parsed.cost_usd ?? 0, - text: parsed.subtype, - ...(denials.length ? { denials } : {}), - }); - } - return events; -} - export function aggregateStageEvents(events: StageEvent[], exitCode: number, stderrTail?: string): StageRunResult { let toolCalls = 0; let tokensIn = 0; let tokensOut = 0; let costUsd = 0; const permissionDenials: string[] = []; + let finalText: string | undefined; + let final: { in: number; out: number } | "invalid" | undefined; for (const e of events) { + if (e.finalText !== undefined) finalText = e.finalText; if (e.kind === "tool_use") toolCalls += 1; if (e.kind === "usage") { - tokensIn += e.tokensIn ?? 0; - tokensOut += e.tokensOut ?? 0; + if (e.invalid) final = "invalid"; + else if (e.total) final = { in: e.tokensIn ?? 0, out: e.tokensOut ?? 0 }; + else { + tokensIn += e.tokensIn ?? 0; + tokensOut += e.tokensOut ?? 0; + } } if (e.kind === "result") { costUsd = e.costUsd ?? costUsd; permissionDenials.push(...(e.denials ?? [])); } } - const result = { events, toolCalls, tokensIn, tokensOut, costUsd, exitCode, permissionDenials }; - return exitCode !== 0 && stderrTail ? { ...result, stderrTail } : result; -} - -export function claudeArgs(opts: StageRunOptions): string[] { - return [ - "-p", - `/factory-${opts.stage} ${opts.issue}`, - "--output-format", - "stream-json", - "--verbose", - "--permission-mode", - "dontAsk", - "--permission-prompts", - "none", - "--setting-sources", - "project,local", - "--settings", - stageSettings(opts.stage, opts.issue, opts.agentCommands), - "--append-system-prompt", - STAGE_GUIDANCE, - "--no-session-persistence", - "--max-budget-usd", - String(opts.maxBudgetUsd), - ]; -} - -// Strips the runner's own secrets from the environment the agent process -// inherits: the target repo's `GH_TOKEN` would otherwise let a rogue `Bash` -// call push or merge over the guard hook's head, and `FACTORY_*` leaks the -// dashboard token and DB path (audit finding #14). Everything else (PATH, -// HOME, ANTHROPIC_API_KEY, ...) passes through — the agent still needs a -// model key, so the residual risk (a spend-capped key, documented in the -// README) is deliberate, not an oversight. -const STRIPPED_ENV_PREFIXES = ["GH_TOKEN", "GITHUB_TOKEN", "FACTORY_"]; - -export function sanitizeEnv(env: NodeJS.ProcessEnv): Record { - const out: Record = {}; - for (const [key, value] of Object.entries(env)) { - if (value === undefined) continue; - if (STRIPPED_ENV_PREFIXES.some((p) => key === p || key.startsWith(p))) continue; - out[key] = value; - } - return out; -} - -const DEFAULT_TIMEOUT_MINUTES = 15; - -export class ClaudeExecutor implements Executor { - async runStage(opts: StageRunOptions): Promise { - const proc = Bun.spawn(["claude", ...claudeArgs(opts)], { - cwd: opts.cwd, - stdout: "pipe", - stderr: "pipe", - env: sanitizeEnv(process.env), - }); - const events: StageEvent[] = []; - let toolCalls = 0; - let killedReason: string | undefined; - // Read alongside stdout, not after: an unread pipe can otherwise fill its - // OS buffer and stall the child, and any launch-time error (bad flag, - // auth, a permission refusal) `claude` prints only goes to stderr. - const stderrPromise = new Response(proc.stderr).text(); - - const timeoutMinutes = opts.timeoutMinutes ?? DEFAULT_TIMEOUT_MINUTES; - const timer = setTimeout(() => { - killedReason = `stage exceeded stageTimeoutMinutes=${timeoutMinutes}`; - proc.kill(); - }, timeoutMinutes * 60_000); - - try { - const reader = proc.stdout.getReader(); - const decoder = new TextDecoder(); - let buffer = ""; - for (;;) { - const { done, value } = await reader.read(); - if (done) break; - buffer += decoder.decode(value, { stream: true }); - const lines = buffer.split("\n"); - buffer = lines.pop() ?? ""; - for (const line of lines) { - const parsed = parseStreamJsonLine(line); - events.push(...parsed); - for (const e of parsed) { - if (e.kind !== "tool_use") continue; - toolCalls += 1; - if (opts.maxToolCalls && toolCalls > opts.maxToolCalls && !killedReason) { - killedReason = `stage exceeded maxToolCalls=${opts.maxToolCalls}`; - proc.kill(); - } - } - } - } - if (buffer.trim()) events.push(...parseStreamJsonLine(buffer)); - } finally { - clearTimeout(timer); - } - - const [exitCode, stderr] = await Promise.all([proc.exited, stderrPromise]); - const stderrTail = stderr.trim().slice(-4000) || undefined; - const result = aggregateStageEvents(events, exitCode, stderrTail); - return killedReason ? { ...result, exitCode: exitCode || 1, killedReason } : result; + if (final === "invalid") { + tokensIn = 0; + tokensOut = 0; + } else if (final) { + tokensIn = final.in; + tokensOut = final.out; } + const finalMessage = finalText === undefined ? undefined : truncateFinalMessage(finalText); + const result = { events, toolCalls, tokensIn, tokensOut, costUsd, exitCode, permissionDenials, usageComplete: final !== "invalid", ...(finalMessage ? { finalMessage } : {}) }; + return exitCode !== 0 && stderrTail ? { ...result, stderrTail } : result; } // Fixture-driven, deterministic, no network. Keyed by ":" -> an @@ -247,7 +118,7 @@ export class ReplayExecutor implements Executor { const key = `${opts.stage}:${opts.issue}`; const lines = this.fixtures[key]; if (!lines) throw new Error(`replay: no fixture recorded for ${key}`); - const events = lines.flatMap(parseStreamJsonLine); + const events = lines.flatMap((l) => parseStreamJsonLine(l)); return aggregateStageEvents(events, 0); } } diff --git a/src/watch.ts b/src/watch.ts index 7d54164..fba64f5 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -192,8 +192,8 @@ async function runStage( repo: config.repo, issue: issueNumber, stage: stage as Stage, - agent: "claude", // the only executor until v2.5 makes the agent a config choice - model: null, + agent: result.agent ?? "claude", // ReplayExecutor reports none + model: result.model ?? null, started_at: startedAt.toISOString(), finished_at: finishedAt.toISOString(), duration_ms: finishedAt.getTime() - startedAt.getTime(), diff --git a/tests/executor.test.ts b/tests/executor.test.ts index ec08d1e..e95c72f 100644 --- a/tests/executor.test.ts +++ b/tests/executor.test.ts @@ -11,13 +11,8 @@ import { describe, expect, test } from "bun:test"; import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { - aggregateStageEvents, - claudeArgs, - parseStreamJsonLine, - ReplayExecutor, - type StageName, -} from "../src/executor"; +import { aggregateStageEvents, ReplayExecutor, type StageName } from "../src/executor"; +import { claudeArgs, parseStreamJsonLine } from "../src/agents/presets/claude"; import { DEFAULT_CONFIG, mergeConfig } from "../src/config"; import { runDir } from "../src/artifacts"; import { baseIssue, FakeGateRunner, FakeGit, FakeGitHub, fixtureFor, MultiStageExecutor } from "./harness"; diff --git a/tests/ported/machinist/final-message.test.ts b/tests/ported/machinist/final-message.test.ts new file mode 100644 index 0000000..a400284 --- /dev/null +++ b/tests/ported/machinist/final-message.test.ts @@ -0,0 +1,54 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/final_message_test.go:10-52 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the Go collector is replaced by the preset parsers plus aggregateStageEvents; TestExecuteRecordsTheFinalAgentMessage runs a real child through CommandExecutor instead of Go's Execute. + +import { expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { CommandExecutor } from "../../../src/agents/executor"; +import { MAX_FINAL_MESSAGE_BYTES } from "../../../src/agents/final-message"; +import { claudePreset } from "../../../src/agents/presets/claude"; +import { codexPreset } from "../../../src/agents/presets/codex"; +import { aggregateStageEvents } from "../../../src/executor"; +import type { AgentPreset } from "../../../src/agents/types"; + +const finalOf = (preset: AgentPreset, output: string) => + aggregateStageEvents(output.split("\n").flatMap((l) => preset.parseLine(l)), 0).finalMessage; + +test("the collector keeps the last Codex agent message", () => { + const out = + '{"type":"item.completed","item":{"type":"agent_message","text":"Looking at issues."}}\n' + + '{"type":"item.completed","item":{"type":"command_execution","command":"gh issue list"}}\n' + + '{"type":"item.completed","item":{"type":"agent_message","text":" Labelled #496 as bug. "}}'; + expect(finalOf(codexPreset, out)).toBe("Labelled #496 as bug."); +}); + +test("the collector keeps the Claude result", () => { + const out = + '{"type":"assistant","message":{"content":[{"type":"text","text":"Working."}]}}\n' + + '{"type":"result","result":"All done.","usage":{"input_tokens":1,"cache_creation_input_tokens":0,"cache_read_input_tokens":0,"output_tokens":1}}\n'; + expect(finalOf(claudePreset, out)).toBe("All done."); +}); + +test("long messages are truncated on a character boundary", () => { + const text = "é".repeat(MAX_FINAL_MESSAGE_BYTES); + const got = finalOf(codexPreset, JSON.stringify({ type: "item.completed", item: { type: "agent_message", text } }) + "\n")!; + expect(got.endsWith("[truncated]")).toBe(true); + expect(Buffer.byteLength(got)).toBeLessThanOrEqual(MAX_FINAL_MESSAGE_BYTES + "\n\n[truncated]".length); + expect(got.startsWith("éé")).toBe(true); + expect(got).not.toContain("�"); +}); + +test("a real child's final agent message is recorded", async () => { + const cwd = mkdtempSync(join(tmpdir(), "factory-final-")); + try { + mkdirSync(join(cwd, ".claude/skills/factory-triage"), { recursive: true }); + writeFileSync(join(cwd, ".claude/skills/factory-triage/SKILL.md"), "---\nname: factory-triage\n---\nDo it.\n"); + const output = '{"type":"item.completed","item":{"type":"agent_message","text":"Triaged 9 issues."}}\n'; + const executor = new CommandExecutor({ codex: { preset: "codex", command: ["sh", "-c", `cat >/dev/null; printf '%s' '${output}'`] } }, { default: "codex" }); + const result = await executor.runStage({ stage: "triage", issue: 1, cwd, maxBudgetUsd: 1 }); + expect(result.finalMessage).toBe("Triaged 9 issues."); + expect(result.agent).toBe("codex"); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } +}); diff --git a/tests/ported/machinist/runner.test.ts b/tests/ported/machinist/runner.test.ts new file mode 100644 index 0000000..7ebaf27 --- /dev/null +++ b/tests/ported/machinist/runner.test.ts @@ -0,0 +1,92 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/runner_test.go:162-260,579-610 and process_unix_test.go (MIT, Copyright (c) 2026 Owain Lewis). Deviations: cases run a shell script through CommandExecutor; the Go run-store, cancellation and darwin-EPERM cases are not ported (there is no run store, and Bun's kill of a negative pid surfaces ESRCH as an exception that the executor already swallows). + +import { afterAll, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { CommandExecutor } from "../../../src/agents/executor"; +import { sanitizeEnv } from "../../../src/agents/env"; + +const roots: string[] = []; +afterAll(() => roots.forEach((r) => rmSync(r, { recursive: true, force: true }))); + +function repo(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "factory-runner-"))); + roots.push(dir); + Bun.spawnSync(["git", "init", "-q"], { cwd: dir }); + mkdirSync(join(dir, ".claude/skills/factory-plan"), { recursive: true }); + writeFileSync(join(dir, ".claude/skills/factory-plan/SKILL.md"), "---\nname: factory-plan\n---\nPlan it.\n"); + return dir; +} + +const run = (cwd: string, script: string, extra: Record = {}) => + new CommandExecutor({ a: { command: ["sh", "-c", script, "agent"] } }, { default: "a" }).runStage({ stage: "plan", issue: 7, cwd, maxBudgetUsd: 1, ...extra }); + +describe("environment", () => { + test("the agent gets the factory's directories and none of the runner's secrets", async () => { + const cwd = repo(); + const before = { gh: process.env.GH_TOKEN, fac: process.env.FACTORY_DASHBOARD_TOKEN }; + process.env.GH_TOKEN = "secret"; + process.env.FACTORY_DASHBOARD_TOKEN = "secret"; + try { + await run(cwd, 'cat >/dev/null; printf "%s\\n" "$FACTORY_STAGE" "$FACTORY_ISSUE" "$FACTORY_ARTIFACT_DIR" "${GH_TOKEN:-none}" "${FACTORY_DASHBOARD_TOKEN:-none}" "$FACTORY_SCRATCH_DIR" > "$FACTORY_ARTIFACT_DIR/env.txt"'); + } finally { + if (before.gh === undefined) delete process.env.GH_TOKEN; else process.env.GH_TOKEN = before.gh; + if (before.fac === undefined) delete process.env.FACTORY_DASHBOARD_TOKEN; else process.env.FACTORY_DASHBOARD_TOKEN = before.fac; + } + const [stage, issue, dir, gh, dash, scratch] = readFileSync(join(cwd, ".factory/runs/issue-7/env.txt"), "utf8").trim().split("\n"); + expect([stage, issue, dir, gh, dash]).toEqual(["plan", "7", join(cwd, ".factory/runs/issue-7"), "none", "none"]); + expect(scratch).toContain("factory-scratch-7-"); + expect(existsSync(scratch!)).toBe(false); // removed when the stage ends + }); + + test("an inherited GIT_DIR or GIT_WORK_TREE does not redirect the agent's git", async () => { + const requested = repo(); + const other = repo(); + const before = { d: process.env.GIT_DIR, w: process.env.GIT_WORK_TREE }; + process.env.GIT_DIR = join(other, ".git"); + process.env.GIT_WORK_TREE = other; + try { + await run(requested, 'cat >/dev/null; git rev-parse --show-toplevel > "$FACTORY_ARTIFACT_DIR/top.txt"'); + } finally { + if (before.d === undefined) delete process.env.GIT_DIR; else process.env.GIT_DIR = before.d; + if (before.w === undefined) delete process.env.GIT_WORK_TREE; else process.env.GIT_WORK_TREE = before.w; + } + expect(readFileSync(join(requested, ".factory/runs/issue-7/top.txt"), "utf8").trim()).toBe(requested); + }); + + test("sanitizeEnv drops repository git variables but keeps identity and PATH", () => { + const clean = sanitizeEnv({ PATH: "/bin", GIT_DIR: "x", GIT_INDEX_FILE: "y", GIT_CONFIG_KEY_0: "k", GIT_AUTHOR_NAME: "a", ANTHROPIC_API_KEY: "k" }); + expect(Object.keys(clean).sort()).toEqual(["ANTHROPIC_API_KEY", "GIT_AUTHOR_NAME", "PATH"]); + }); +}); + +describe("process group", () => { + test("a timeout kills the agent and its children, and the stage still finishes", async () => { + const cwd = repo(); + const marker = join(cwd, "child.pid"); + const started = Date.now(); + const result = await run(cwd, `cat >/dev/null; sleep 30 & echo $! > ${marker}; wait`, { timeoutMinutes: 0.03 }); + expect(Date.now() - started).toBeLessThan(10_000); + expect(result.killedReason).toContain("stageTimeoutMinutes"); + expect(result.exitCode).not.toBe(0); + const pid = Number(readFileSync(marker, "utf8")); + expect(() => process.kill(pid, 0)).toThrow(); // the grandchild is gone + }); + + test("a descendant that keeps the output pipe open does not hang a finished stage", async () => { + const cwd = repo(); + const started = Date.now(); + const result = await run(cwd, "cat >/dev/null; (sleep 30 >/dev/null 2>&1 &) ; exit 0"); + expect(result.exitCode).toBe(0); + expect(Date.now() - started).toBeLessThan(5_000); + }); +}); + +test("a command that cannot start is a failed stage with the reason, not a crash", async () => { + const cwd = repo(); + const result = await new CommandExecutor({ a: { command: [join(cwd, "not-an-agent")] } }, { default: "a" }) + .runStage({ stage: "plan", issue: 7, cwd, maxBudgetUsd: 1 }) + .catch((e: Error) => e); + expect(result instanceof Error ? result.message : `exit ${result.exitCode}`).toMatch(/not-an-agent|ENOENT|exit [1-9]/); +}); diff --git a/tests/ported/machinist/usage.test.ts b/tests/ported/machinist/usage.test.ts new file mode 100644 index 0000000..094a556 --- /dev/null +++ b/tests/ported/machinist/usage.test.ts @@ -0,0 +1,78 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage_test.go:17-108 and claude_usage_test.go:143-176 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the Go collector is a stream of Write calls; here each line goes through the preset parser and aggregateStageEvents, and the token total is in + out. The command-recognition and flag-injection cases are not ported: our presets build the command, so nothing is guessed from an arbitrary one. + +import { describe, expect, test } from "bun:test"; +import { aggregateStageEvents, type StageEvent } from "../../../src/executor"; +import { claudePreset } from "../../../src/agents/presets/claude"; +import { codexPreset } from "../../../src/agents/presets/codex"; +import type { AgentPreset } from "../../../src/agents/types"; + +// Feeds text in arbitrary chunks the way a pipe would, splitting on newlines. +function collect(preset: AgentPreset, ...chunks: string[]): { total: number | null; message?: string } { + const events: StageEvent[] = []; + const lines = chunks.join("").split("\n"); + for (const line of lines) events.push(...preset.parseLine(line)); + const r = aggregateStageEvents(events, 0); + return { total: r.usageComplete === false ? null : r.tokensIn + r.tokensOut, message: r.finalMessage }; +} + +const codex = (...chunks: string[]) => collect(codexPreset, ...chunks); +const claude = (...chunks: string[]) => collect(claudePreset, ...chunks); + +describe("codex usage", () => { + test("reads the final structured usage across chunk boundaries", () => { + const got = codex( + '{"type":"turn.completed","usage":{"input_tokens":10,"cached_input_tokens":8,"output_tokens":2}}\n{"type":"item.completed",', + '"item":{"type":"agent_message","text":"done"}}\n{"type":"turn.completed","usage":{"input_tokens":40,', + '"cached_input_tokens":35,"output_tokens":3}}', + ); + expect(got.total).toBe(43); + }); + + test.each([ + ["missing event", '{"type":"item.completed","item":{}}', 0], + ["malformed JSON", '{"type":"turn.completed","usage":', null], + ["missing input", '{"type":"turn.completed","usage":{"output_tokens":2}}', null], + ["negative input", '{"type":"turn.completed","usage":{"input_tokens":-1,"output_tokens":2}}', null], + ["overflow", '{"type":"turn.completed","usage":{"input_tokens":9223372036854775807,"output_tokens":1}}', null], + ])("leaves invalid usage unavailable: %s", (_name, line, want) => { + // "missing event" has no terminal event at all: nothing to trust or distrust, so zero tokens, not "unavailable". + expect(codex(line + "\n").total).toBe(want); + }); + + test("uses the last completed turn, and a malformed final one makes usage unavailable", () => { + expect(codex('{"type":"turn.completed","usage":{"input_tokens":4,"output_tokens":5}}\n{"type":"turn.completed","usage":{"input_tokens":"invalid","output_tokens":5}}\n').total).toBeNull(); + }); + + test("a truncated final completed turn invalidates usage", () => { + expect(codex('{"type":"turn.completed","usage":{"input_tokens":4,"output_tokens":5}}\n{"type":"turn.completed","usage":{"input_tokens":8').total).toBeNull(); + }); + + test("unrelated malformed output is ignored", () => { + expect(codex('{"type":"turn.completed","usage":{"input_tokens":4,"output_tokens":5}}\n{"type":"item.completed","item":').total).toBe(9); + }); + + test.each([ + ["result", claudePreset, '{"type":"result","usage":{"input_tokens":4,"cache_creation_input_tokens":0,"cache_read_input_tokens":0,"output_tokens":5}}'], + ["turn.completed", codexPreset, '{"type":"turn.completed","usage":{"input_tokens":4,"output_tokens":5}}'], + ] as const)("a nested malformed %s event is ignored", (type, preset, valid) => { + expect(collect(preset, valid + "\n", `{"item":{"type":"${type}","usage":`).total).toBe(9); + }); +}); + +describe("claude usage", () => { + test("reads all usage fields, cache included", () => { + expect(claude('{"type":"system","subtype":"init"}\n{"type":"result","usage":{"input_tokens":100,"cache_creation_input_tokens":20,"cache_read_input_tokens":30,"output_tokens":4}}').total).toBe(154); + }); + + test.each([ + ["missing field", '{"type":"result","usage":{"input_tokens":1,"cache_creation_input_tokens":2,"cache_read_input_tokens":3}}'], + ["malformed", '{"type":"result","usage":'], + ["negative", '{"type":"result","usage":{"input_tokens":-1,"cache_creation_input_tokens":2,"cache_read_input_tokens":3,"output_tokens":4}}'], + ["fractional", '{"type":"result","usage":{"input_tokens":1.5,"cache_creation_input_tokens":2,"cache_read_input_tokens":3,"output_tokens":4}}'], + ["overflow", '{"type":"result","usage":{"input_tokens":9223372036854775807,"cache_creation_input_tokens":1,"cache_read_input_tokens":0,"output_tokens":0}}'], + ["malformed field before type", '{"usage":invalid,"type":"result"}'], + ])("rejects an invalid final usage: %s", (_name, line) => { + const good = '{"type":"result","usage":{"input_tokens":1,"cache_creation_input_tokens":2,"cache_read_input_tokens":3,"output_tokens":4}}'; + expect(claude(good + "\n" + line + "\n").total).toBeNull(); + }); +}); From f651c02c76da240bbed4c8ad0bf1440010d3adba Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:07:01 +0300 Subject: [PATCH 2/7] v2.5: agents tests, fake agent fixture Co-Authored-By: Claude Sonnet 5 --- tests/agents.test.ts | 173 ++++++++++++++++++++++++++++ tests/fixtures/agents/fake-agent.sh | 24 ++++ 2 files changed, 197 insertions(+) create mode 100644 tests/agents.test.ts create mode 100755 tests/fixtures/agents/fake-agent.sh diff --git a/tests/agents.test.ts b/tests/agents.test.ts new file mode 100644 index 0000000..23b73e9 --- /dev/null +++ b/tests/agents.test.ts @@ -0,0 +1,173 @@ +// The agent layer: the Claude command is pinned byte for byte, the config that +// picks an agent is validated, and an agent with no preset and no JSON output +// completes a whole run through CommandExecutor. Also structural: every stage's +// artifact contract reaches the prompt, and every preset can be diagnosed. + +import { afterAll, describe, expect, test } from "bun:test"; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { CommandExecutor, renderCommand, resolveAgent } from "../src/agents/executor"; +import { artifactContract, renderPrompt, stripFrontmatter } from "../src/agents/prompt"; +import { claudeArgs } from "../src/agents/presets/claude"; +import { PRESETS } from "../src/agents/presets"; +import { COMMENT_FILENAMES, JSON_FILENAMES } from "../src/artifacts"; +import { configProblems, DEFAULT_CONFIG, mergeConfig } from "../src/config"; +import type { StageName } from "../src/executor"; +import { STAGE_GUIDANCE, stageSettings } from "../src/stage-permissions"; +import { FactoryState } from "../src/state"; +import { LABEL } from "../src/labels"; +import { processReadyIssue } from "../src/watch"; +import { baseIssue, FakeGateRunner, FakeGit, FakeGitHub } from "./harness"; + +const STAGES: StageName[] = ["triage", "plan", "build", "verify", "pr"]; +const scratch = mkdtempSync(join(tmpdir(), "factory-agents-")); +afterAll(() => rmSync(scratch, { recursive: true, force: true })); + +describe("claude preset", () => { + test("the argv is what v2.4.0 sent, byte for byte", () => { + const opts = { stage: "build" as const, issue: 12, cwd: "/w", maxBudgetUsd: 5 }; + expect(claudeArgs(opts)).toEqual([ + "-p", "/factory-build 12", + "--output-format", "stream-json", "--verbose", + "--permission-mode", "dontAsk", "--permission-prompts", "none", + "--setting-sources", "project,local", + "--settings", stageSettings("build", 12, undefined), + "--append-system-prompt", STAGE_GUIDANCE, + "--no-session-persistence", + "--max-budget-usd", "5", + ]); + expect(STAGE_GUIDANCE).toBe("Read files with the Read tool, one call per file. Run shell commands one at a time: no &&, ;, pipes or brace expansion."); + expect(PRESETS.claude!.command(opts, { preset: "claude" }, "").argv).toEqual(["claude", ...claudeArgs(opts)]); + }); + + test("a configured model is added and nothing else changes", () => { + const opts = { stage: "plan" as const, issue: 1, cwd: "/w", maxBudgetUsd: 2 }; + const argv = PRESETS.claude!.command(opts, { preset: "claude", model: "opus" }, "").argv; + expect(argv.slice(0, -2)).toEqual(["claude", ...claudeArgs(opts)]); + expect(argv.slice(-2)).toEqual(["--model", "opus"]); + }); +}); + +describe("codex preset", () => { + test("prompt on stdin, JSON events, workspace-write", () => { + const inv = PRESETS.codex!.command({ stage: "plan", issue: 1, cwd: "/w", maxBudgetUsd: 2 }, { preset: "codex", model: "gpt-5.6-terra" }, "the prompt"); + expect(inv.argv).toEqual(["codex", "exec", "--json", "-s", "workspace-write", "-m", "gpt-5.6-terra", "-"]); + expect(inv.stdin).toBe("the prompt"); + }); +}); + +describe("agents config", () => { + test("the default is Claude for every stage", () => { + expect(DEFAULT_CONFIG.stages).toEqual({ default: "claude" }); + expect(mergeConfig({ repo: "a/b", agents: { codex: { preset: "codex" } }, stages: { verify: "codex" } }).stages).toEqual({ default: "claude", verify: "codex" }); + }); + + test.each([ + [{ agents: { x: {} } }, /needs a "preset" or a "command"/], + [{ agents: { x: { preset: "nope" } } }, /unknown "nope"/], + [{ agents: { x: { command: [] } } }, /must not be empty/], + [{ agents: { x: { command: ["{{prompt}}"] } } }, /executable cannot be a placeholder/], + [{ agents: { x: { command: ["a"], sandbox: true } } }, /unknown key/], + [{ stages: { verify: "ghost" } }, /"ghost" is not an agent/], + [{ stages: { deploy: "claude" } }, /unknown stage/], + ])("refuses %j", (cfg, want) => { + expect(configProblems({ repo: "a/b", ...cfg }).join("\n")).toMatch(want); + }); + + test("a valid mix is accepted", () => { + expect(configProblems({ repo: "a/b", agents: { codex: { preset: "codex" }, aider: { command: ["aider", "--message-file", "{{promptFile}}"] } }, stages: { default: "claude", build: "aider" } })).toEqual([]); + }); + + test("resolveAgent picks the stage's agent, else the default, else claude", () => { + const agents = { claude: { preset: "claude" }, codex: { preset: "codex" } }; + expect(resolveAgent(agents, { default: "claude", verify: "codex" }, "verify").name).toBe("codex"); + expect(resolveAgent(agents, { default: "codex" }, "plan").name).toBe("codex"); + expect(resolveAgent(agents, {}, "plan").name).toBe("claude"); + expect(() => resolveAgent(agents, { plan: "ghost" }, "plan")).toThrow(/not in config.agents/); + }); + + test("renderCommand fills placeholders and knows when the prompt is not on stdin", () => { + const v = { prompt: "P", promptFile: "/f", model: "m" }; + expect(renderCommand(["a", "--model", "{{model}}"], v)).toEqual({ argv: ["a", "--model", "m"], usesStdin: true }); + expect(renderCommand(["a", "{{prompt}}"], v)).toEqual({ argv: ["a", "P"], usesStdin: false }); + expect(renderCommand(["a", "--f={{promptFile}}"], v)).toEqual({ argv: ["a", "--f=/f"], usesStdin: false }); + }); +}); + +describe("the prompt", () => { + test.each(STAGES)("%s: every artifact the stage may write is named in the contract", (stage) => { + const c = artifactContract({ stage, issue: 4 }); + expect(c).toContain(JSON_FILENAMES[stage]); + expect(c).toContain(COMMENT_FILENAMES[stage]); + expect(c).toContain("question-comment.md"); + expect(c).toContain(".factory/runs/issue-4/"); + }); + + test.each(STAGES)("%s: the rendered prompt is the skill body plus the contract", async (stage) => { + const cwd = mkdtempSync(join(scratch, "prompt-")); + cpSync(join(import.meta.dir, "../template/.claude/skills"), join(cwd, ".claude/skills"), { recursive: true }); + const prompt = await renderPrompt({ stage, issue: 4, cwd, maxBudgetUsd: 1 }); + const body = stripFrontmatter(readFileSync(join(cwd, `.claude/skills/factory-${stage}/SKILL.md`), "utf8")).trim(); + expect(prompt).toContain(body); + expect(prompt).toContain("Issue number: 4"); + expect(prompt).toContain(artifactContract({ stage, issue: 4 })); + expect(prompt).not.toContain("name: factory-"); + }); + + test("a missing skill says how to fix it", async () => { + await expect(renderPrompt({ stage: "plan", issue: 1, cwd: scratch, maxBudgetUsd: 1 })).rejects.toThrow(/factory install --update/); + }); +}); + +describe("an agent with no preset", () => { + class SkillGit extends FakeGit { + override async ensureWorktree(cloneDir: string, worktreeDir: string, issue: number) { + await super.ensureWorktree(cloneDir, worktreeDir, issue); + cpSync(join(import.meta.dir, "../template/.claude/skills"), join(worktreeDir, ".claude/skills"), { recursive: true }); + } + } + + test("completes triage to PR from a script that only writes files", async () => { + const workspacesDir = mkdtempSync(join(scratch, "ws-")); + const cloneDir = mkdtempSync(join(scratch, "clone-")); + const log = mkdtempSync(join(scratch, "log-")); + process.env.FAKE_AGENT_LOG = log; + const issue = baseIssue(1, [LABEL.ready]); + const github = new FakeGitHub([issue]); + const state = new FactoryState(":memory:"); + state.setToggle("auto_approve_low_risk", true); + const config = mergeConfig({ + repo: "acme/widgets", + agents: { scripted: { command: [join(import.meta.dir, "fixtures/agents/fake-agent.sh")] } }, + stages: { default: "scripted" }, + }); + const deps = { github, git: new SkillGit(), state, executor: new CommandExecutor(config.agents, config.stages), gateRunner: new FakeGateRunner(), cloneDir, workspacesDir }; + try { + expect(await processReadyIssue(issue, deps, config)).toBe("shipped"); + } finally { + delete process.env.FAKE_AGENT_LOG; + } + expect(github.createdPrs[0]!.body).toContain("Closes #1"); + // Every stage was handed its contract on stdin. + for (const stage of STAGES) expect(readFileSync(join(log, `${stage}.prompt`), "utf8")).toContain(JSON_FILENAMES[stage]); + // The run history names the agent and says the numbers are not reported. + const rows = state.listStageRuns("acme/widgets"); + expect(rows.map((r) => r.agent)).toEqual(Array(rows.length).fill("scripted")); + expect(rows.length).toBe(5); + state.close(); + }); +}); + +describe("every preset is diagnosable", () => { + test("has a binary, parses a blank line to nothing, and never lets the prompt into argv when it owns stdin", () => { + for (const [name, preset] of Object.entries(PRESETS)) { + expect(preset.name).toBe(name); + expect(preset.binary.length).toBeGreaterThan(0); + expect(preset.parseLine("")).toEqual([]); + const inv = preset.command({ stage: "plan", issue: 1, cwd: "/w", maxBudgetUsd: 1 }, { preset: name }, "SECRET-PROMPT"); + expect(inv.argv[0]).toBe(preset.binary); + if (!preset.ownsPrompt) expect(inv.argv.join(" ")).not.toContain("SECRET-PROMPT"); + } + }); +}); diff --git a/tests/fixtures/agents/fake-agent.sh b/tests/fixtures/agents/fake-agent.sh new file mode 100755 index 0000000..8d8dc65 --- /dev/null +++ b/tests/fixtures/agents/fake-agent.sh @@ -0,0 +1,24 @@ +#!/bin/sh +# A stand-in for any coding agent with no preset and no JSON output: it reads +# the prompt on stdin and writes the stage's artifacts to $FACTORY_ARTIFACT_DIR. +# FAKE_AGENT_LOG (a directory) keeps each stage's prompt for the tests to read. +prompt=$(cat) +[ -n "$FAKE_AGENT_LOG" ] && printf '%s' "$prompt" > "$FAKE_AGENT_LOG/$FACTORY_STAGE.prompt" +d="$FACTORY_ARTIFACT_DIR" +n="$FACTORY_ISSUE" +case "$FACTORY_STAGE" in + triage) + printf '\nlooks good\n' > "$d/triage-comment.md" + printf '{"disposition":"proceed","type":"bug","risk":"low","done_when":"tests pass","files_expected":["src/a.ts"],"gate_level":"make check","confidence":0.9}\n' > "$d/triage.json" ;; + plan) + printf '\nplan body\n' > "$d/plan-comment.md" + printf '{"risk":"low","revision":1,"files":["src/a.ts"],"autoApproveEligible":true}\n' > "$d/plan.json" ;; + build) + printf '\nbuilding\n' > "$d/status-comment.md" + printf '{"status":"green","gate_line":"make check: 10 pass","rounds":1}\n' > "$d/build.json" ;; + verify) + printf '\npass\n' > "$d/verdict-comment.md" + printf '{"result":"pass","rounds":1,"findings":[]}\n' > "$d/verdict.json" ;; + pr) + printf '## Summary\nDid the thing.\nCloses #%s\n' "$n" > "$d/pr-body.md" ;; +esac From 50924b0dfd19d8c73a37e90b26db3299806eba95 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:08:10 +0300 Subject: [PATCH 3/7] v2.5: fix provenance headers Co-Authored-By: Claude Sonnet 5 --- tests/ported/machinist/runner.test.ts | 2 +- tests/ported/machinist/usage.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ported/machinist/runner.test.ts b/tests/ported/machinist/runner.test.ts index 7ebaf27..88a4e86 100644 --- a/tests/ported/machinist/runner.test.ts +++ b/tests/ported/machinist/runner.test.ts @@ -1,4 +1,4 @@ -// Ported from owainlewis/machinist@3943516 internal/runner/runner_test.go:162-260,579-610 and process_unix_test.go (MIT, Copyright (c) 2026 Owain Lewis). Deviations: cases run a shell script through CommandExecutor; the Go run-store, cancellation and darwin-EPERM cases are not ported (there is no run store, and Bun's kill of a negative pid surfaces ESRCH as an exception that the executor already swallows). +// Ported from owainlewis/machinist@3943516 internal/runner/runner_test.go:162-260,579-610 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the process_unix_test.go group-kill cases are folded in; cases run a shell script through CommandExecutor; the Go run-store, cancellation and darwin-EPERM cases are not ported (there is no run store, and Bun's kill of a negative pid surfaces ESRCH as an exception that the executor already swallows). import { afterAll, describe, expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; diff --git a/tests/ported/machinist/usage.test.ts b/tests/ported/machinist/usage.test.ts index 094a556..dd4ac55 100644 --- a/tests/ported/machinist/usage.test.ts +++ b/tests/ported/machinist/usage.test.ts @@ -1,4 +1,4 @@ -// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage_test.go:17-108 and claude_usage_test.go:143-176 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the Go collector is a stream of Write calls; here each line goes through the preset parser and aggregateStageEvents, and the token total is in + out. The command-recognition and flag-injection cases are not ported: our presets build the command, so nothing is guessed from an arbitrary one. +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage_test.go:17-108 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: claude_usage_test.go:143-176 is folded in; the Go collector is a stream of Write calls; here each line goes through the preset parser and aggregateStageEvents, and the token total is in + out. The command-recognition and flag-injection cases are not ported: our presets build the command, so nothing is guessed from an arbitrary one. import { describe, expect, test } from "bun:test"; import { aggregateStageEvents, type StageEvent } from "../../../src/executor"; From 4a4fc76bfa8f4382707353dc8c361de43573c14e Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:08:33 +0300 Subject: [PATCH 4/7] v2.5: doctor checks each agent in use Co-Authored-By: Claude Sonnet 5 --- bin/factory | 2 +- src/doctor.ts | 33 +++++++++++++++++++++++++++------ tests/doctor.test.ts | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/bin/factory b/bin/factory index 1ec6d49..edfc72d 100755 --- a/bin/factory +++ b/bin/factory @@ -268,7 +268,7 @@ async function cmdDoctor(): Promise { } }, }, - { repo: config.repo, cloneDir, baselineTag: config.baselineTag, factoryMode: process.env.FACTORY_MODE }, + { repo: config.repo, cloneDir, baselineTag: config.baselineTag, factoryMode: process.env.FACTORY_MODE, agents: config.agents, stages: config.stages }, ); const allOk = checks.every((c) => c.ok); if (has("json")) console.log(successJson({ checks }, allOk)); diff --git a/src/doctor.ts b/src/doctor.ts index 26aa65f..0f76462 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -3,6 +3,8 @@ import type { CommandRunner, GitHub } from "./github"; import { LABELS } from "./labels"; +import type { AgentConfig, StageAgents } from "./agents/types"; +import { PRESETS } from "./agents/presets"; export interface DoctorCheck { readonly name: string; @@ -25,6 +27,8 @@ export interface DoctorContext { readonly cloneDir: string; readonly baselineTag: string; readonly factoryMode?: string; // FACTORY_MODE, "actions" when CI drives the loop + readonly agents?: Record; + readonly stages?: StageAgents; } export async function runDoctor(deps: DoctorDeps, ctx: DoctorContext): Promise { @@ -36,12 +40,29 @@ export async function runDoctor(deps: DoctorDeps, ctx: DoctorContext): Promise Boolean(n))); + for (const name of [...used].sort()) { + const agent = agents[name]; + const preset = agent?.preset ? PRESETS[agent.preset] : undefined; + const binary = agent?.command?.[0] ?? preset?.binary; + checks.push({ + name: `${binary ?? name} on PATH`, + ok: binary !== undefined && (await deps.which(binary)), + detail: `agent "${name}" runs each stage it is assigned`, + fixable: false, + }); + if (agent && !preset) { + checks.push({ + name: `agent "${name}" reports usage`, + ok: true, + detail: "no preset, so tokens show as not reported and the tool-call cap is not enforced; the timeout is the backstop (consider a sandbox)", + fixable: false, + }); + } + } checks.push({ name: "python3 on PATH", ok: await deps.which("python3"), diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index ee08cb5..38c1c6f 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -98,3 +98,22 @@ describe("runDoctor", () => { expect(local.some((c) => c.name.includes("FACTORY_MODE"))).toBe(false); }); }); + +describe("agents in doctor", () => { + const agents = { claude: { preset: "claude" }, codex: { preset: "codex" }, aider: { command: ["aider", "--yes"] }, unused: { preset: "codex" } }; + + test("checks the binary of every agent a stage uses, and only those", async () => { + const checks = await runDoctor(deps({ which: new Set(["gh", "claude", "python3", "jq"]) }), { ...ctx, agents, stages: { default: "claude", verify: "codex", build: "aider" } }); + const on = (n: string) => checks.find((c) => c.name === `${n} on PATH`); + expect(on("claude")!.ok).toBe(true); + expect(on("codex")!.ok).toBe(false); + expect(on("aider")!.ok).toBe(false); + expect(checks.filter((c) => c.name.endsWith(" on PATH")).map((c) => c.name)).not.toContain("unused on PATH"); + }); + + test("a preset-less agent gets a usage warning, a preset agent does not", async () => { + const checks = await runDoctor(deps({ which: new Set(["gh", "claude", "aider", "python3", "jq"]) }), { ...ctx, agents, stages: { default: "aider" } }); + expect(checks.map((c) => c.name)).toContain('agent "aider" reports usage'); + expect((await runDoctor(deps(), ctx)).map((c) => c.name)).not.toContain('agent "claude" reports usage'); + }); +}); From cc29be26ca26162894f62ec72a00cf54061621b5 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:11:19 +0300 Subject: [PATCH 5/7] v2.5: structured findings, verdict consistency, step-result limits, gate evidence Co-Authored-By: Claude Sonnet 5 --- src/artifacts.ts | 100 +++++++++++++++++- src/git.ts | 6 ++ src/watch.ts | 12 ++- template/.claude/agents/factory-reviewer.md | 8 +- .../.claude/skills/factory-verify/SKILL.md | 21 +++- tests/agents.test.ts | 81 +++++++++++++- tests/fixtures/agents/fake-agent.sh | 5 +- tests/harness.ts | 4 + 8 files changed, 222 insertions(+), 15 deletions(-) diff --git a/src/artifacts.ts b/src/artifacts.ts index b64ecdd..ef95016 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -5,7 +5,7 @@ // `.factory/runs/issue-/*`, which the guard hook always allows, and the // runner is the only thing that turns those files into GitHub API calls. -import { unlink } from "node:fs/promises"; +import { mkdir, unlink } from "node:fs/promises"; export type Disposition = "proceed" | "needs-info" | "refused" | "duplicate"; export type Risk = "low" | "medium" | "high"; @@ -37,10 +37,82 @@ export interface BuildArtifact { readonly rounds: number; } +// A finding is a plain string (older verdicts) or the blueprint review shape: +// severity, a 0-5 confidence, and what/why/where/fix. Strings never block a pass. +export type Severity = "must" | "should" | "could"; +export interface Finding { + readonly severity: Severity; + readonly confidence: number; + readonly what: string; + readonly where?: string; + readonly why?: string; + readonly fix?: string; +} +export interface Criterion { + readonly id: string; + readonly status: "pass" | "fail" | "unverified"; + readonly gap?: string; +} + export interface VerdictArtifact { readonly result: VerdictResult; readonly rounds: number; - readonly findings: string[]; + readonly findings: (string | Finding)[]; + readonly criteria?: Criterion[]; +} + +// A step result is one JSON object of at most 16 KiB (machinist workflow.go). +export const MAX_STEP_JSON_BYTES = 16 * 1024; +const VERDICT_KEYS = new Set(["result", "rounds", "findings", "criteria"]); +const FINDING_KEYS = new Set(["severity", "confidence", "what", "where", "why", "fix"]); +const CRITERION_KEYS = new Set(["id", "status", "gap"]); +// A finding this sure and this serious contradicts a pass. +export const BLOCKING_CONFIDENCE = 3; + +function unknownKey(obj: object, allowed: Set): string | undefined { + return Object.keys(obj).find((k) => !allowed.has(k)); +} + +// Rejects a verdict that is malformed or contradicts itself. The runner never +// trusts a "pass" that lists a blocking finding or an unverified criterion. +export function validateVerdict(raw: unknown): { ok: true; verdict: VerdictArtifact } | { ok: false; reason: string } { + const bad = (reason: string) => ({ ok: false as const, reason }); + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) return bad("verdict.json is not a JSON object"); + const v = raw as Record; + const extra = unknownKey(v, VERDICT_KEYS); + if (extra) return bad(`verdict.json has unknown field "${extra}"`); + if (v.result !== "pass" && v.result !== "reject" && v.result !== "uncertain") return bad('verdict.json "result" must be pass, reject or uncertain'); + if (!Number.isInteger(v.rounds) || (v.rounds as number) < 0) return bad('verdict.json "rounds" must be a non-negative integer'); + if (!Array.isArray(v.findings)) return bad('verdict.json "findings" must be an array'); + for (const f of v.findings) { + if (typeof f === "string") continue; + if (typeof f !== "object" || f === null) return bad("a finding must be a string or an object"); + const key = unknownKey(f, FINDING_KEYS); + if (key) return bad(`a finding has unknown field "${key}"`); + const o = f as Record; + if (o.severity !== "must" && o.severity !== "should" && o.severity !== "could") return bad("a finding's severity must be must, should or could"); + if (!Number.isInteger(o.confidence) || (o.confidence as number) < 0 || (o.confidence as number) > 5) return bad("a finding's confidence must be an integer from 0 to 5"); + if (typeof o.what !== "string" || !o.what.trim()) return bad("a finding needs a non-empty \"what\""); + } + if (v.criteria !== undefined) { + if (!Array.isArray(v.criteria)) return bad('verdict.json "criteria" must be an array'); + for (const c of v.criteria) { + if (typeof c !== "object" || c === null) return bad("a criterion must be an object"); + const key = unknownKey(c, CRITERION_KEYS); + if (key) return bad(`a criterion has unknown field "${key}"`); + const o = c as Record; + if (typeof o.id !== "string" || !/^AC-\d+$/.test(o.id)) return bad("a criterion id must look like AC-1"); + if (o.status !== "pass" && o.status !== "fail" && o.status !== "unverified") return bad("a criterion status must be pass, fail or unverified"); + } + } + const verdict = v as unknown as VerdictArtifact; + if (verdict.result === "pass") { + const blocking = verdict.findings.find((f) => typeof f !== "string" && f.severity !== "could" && f.confidence >= BLOCKING_CONFIDENCE); + if (blocking) return bad(`verdict is pass but lists a blocking finding: ${(blocking as Finding).what}`); + const open = verdict.criteria?.find((c) => c.status !== "pass"); + if (open) return bad(`verdict is pass but ${open.id} is ${open.status}`); + } + return { ok: true, verdict }; } export type ArtifactStage = "triage" | "plan" | "build" | "verify" | "pr"; @@ -68,10 +140,30 @@ export const JSON_FILENAMES: Record = { pr: "pr.json", }; +// Undefined for a missing file, and also for one that is too big, not JSON, or +// not a single object: the caller reports "no valid .json" either way. async function readJson(path: string): Promise { const file = Bun.file(path); - if (!(await file.exists())) return undefined; - return (await file.json()) as T; + if (!(await file.exists()) || file.size > MAX_STEP_JSON_BYTES) return undefined; + try { + const value: unknown = await file.json(); + return typeof value === "object" && value !== null && !Array.isArray(value) ? (value as T) : undefined; + } catch { + return undefined; + } +} + +// What the runner measured after build, tied to the tree it measured, so a +// read-only verifier can trust it instead of re-running the gates. +export interface GateEvidence { + readonly line: string; + readonly status: string; + readonly tree: string; +} + +export async function writeGateEvidence(cwd: string, issue: number, evidence: GateEvidence): Promise { + await mkdir(`${cwd}/${runDir(issue)}`, { recursive: true }); + await Bun.write(`${cwd}/${runDir(issue)}/gate.json`, `${JSON.stringify(evidence)}\n`); } async function readText(path: string): Promise { diff --git a/src/git.ts b/src/git.ts index 6d2cf01..f657d12 100644 --- a/src/git.ts +++ b/src/git.ts @@ -106,6 +106,12 @@ export class Git { return this.runner.run(["push", "origin", `HEAD:refs/heads/${branch}`], { cwd: worktreeDir }); } + // The tree of HEAD: the same hash for the same content, whatever the commit. + async treeHash(worktreeDir: string): Promise { + const result = await this.runner.run(["rev-parse", "HEAD^{tree}"], { cwd: worktreeDir }); + return result.stdout.trim(); + } + async hasCommits(worktreeDir: string, base: string): Promise { const result = await this.runner.run(["rev-list", `origin/${base}..HEAD`, "--count"], { cwd: worktreeDir }); return Number(result.stdout.trim() || "0") > 0; diff --git a/src/watch.ts b/src/watch.ts index fba64f5..cf7f874 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -15,12 +15,13 @@ import { writeRevision } from "./revision"; import type { Executor, StageName, StageRunResult } from "./executor"; import { clearStageArtifacts, + validateVerdict, + writeGateEvidence, readStageArtifacts, runDir, type BuildArtifact, type PlanArtifact, type TriageArtifact, - type VerdictArtifact, } from "./artifacts"; import { isHumanComment, latestTrustedCommentAfter, parseChatOps } from "./chatops"; import { deriveIssueState } from "./derive"; @@ -358,6 +359,7 @@ async function runFromStage( } deps.state.updateRun(config.repo, issueNumber, { gate_line: gate.raw }); + await writeGateEvidence(worktree, issueNumber, { line: gate.raw, status: gate.status, tree: await deps.git.treeHash(worktree) }); await deps.git.push(worktree, issueNumber); await moveLabel(deps, config, issueNumber, LABEL.building, LABEL.verifying); stage = "verify"; @@ -367,7 +369,13 @@ async function runFromStage( if (stage === "verify") { const result = await runStage(deps, config, issue, "verify", worktree); const art = await readStageArtifacts(worktree, issueNumber, "verify"); - const json = art.json as VerdictArtifact | undefined; + const checked = art.json === undefined ? undefined : validateVerdict(art.json); + const json = checked?.ok ? checked.verdict : undefined; + if (checked && !checked.ok) { + await moveLabel(deps, config, issueNumber, LABEL.verifying, LABEL.needsHuman); + finish(deps, config, issueNumber, "needs-human", checked.reason); + return "needs-human"; + } if (result.exitCode !== 0 || !json || json.result === "uncertain") { await moveLabel(deps, config, issueNumber, LABEL.verifying, LABEL.needsHuman); finish( diff --git a/template/.claude/agents/factory-reviewer.md b/template/.claude/agents/factory-reviewer.md index 761d90d..3f33aa0 100644 --- a/template/.claude/agents/factory-reviewer.md +++ b/template/.claude/agents/factory-reviewer.md @@ -27,9 +27,11 @@ build stage (or a human) fixes them. ## How to report -One finding per line, each naming the file and line, what's wrong, and why -it matters — not a style pass. Severity matters: separate "blocks this -verdict" from "worth a follow-up issue, not blocking." If you find +One finding per item, each with: severity (`must` fix, `should` fix, or +`could` fix), confidence 0-5, what is wrong, where (file and line), why it +matters, and the fix. Not a style pass. Confidence 0-2 means you could not +show it from the diff: leave it out. `must` and `should` at 3 or more block +the verdict; `could` does not. If you find nothing, say "None" plainly; do not manufacture a nitpick to look thorough. diff --git a/template/.claude/skills/factory-verify/SKILL.md b/template/.claude/skills/factory-verify/SKILL.md index f253025..34a6433 100644 --- a/template/.claude/skills/factory-verify/SKILL.md +++ b/template/.claude/skills/factory-verify/SKILL.md @@ -13,6 +13,10 @@ the runner posts the verdict and moves the issue's label. - `.factory/runs/issue-/issue.json`, `plan.json`, `plan-comment.md`, `build.json` — the plan's AC-n and NG-n, and what build reports it did, including build's own `rounds` count. +- `.factory/runs/issue-/gate.json`: what the runner measured after build + (`line`, `status`, `tree`). If its `tree` equals `git rev-parse HEAD^{tree}`, + the gate result is current: use it and do not re-run the gates. If the tree + differs, or the file is missing, the evidence is stale: report `uncertain`. - The worktree at its current state (build's commits, uncommitted or not). - `.factory/runs/issue-/verdict.json`, if present from a prior round, to read its `rounds` so you increment it, not reset it. The runner tracks @@ -30,10 +34,16 @@ Dispatch to `factory-reviewer` (fresh context, read-only): correctness, security (injection, authz, secrets), and whether the diff crosses any NG-n. Collect its findings verbatim; do not soften or drop one. +Then re-check each finding yourself against the diff at the current head. Drop +one the code does not support (confidence 0-2); keep the rest. Never keep a +finding you could not reproduce from the diff. + ## 3. Decide the verdict - **pass** — every AC has evidence, the gate is green, no NG-n crossed, no - blocking reviewer finding. + blocking reviewer finding. A `pass` that lists a `must` or `should` finding + at confidence 3 or more, or a criterion that is not `pass`, is refused by the + runner and goes to a human. - **reject** — any AC unproven, gate red, an NG-n crossed, or a blocking finding. The runner sends the issue back to `factory-build` up to twice; a third reject is routed to a human automatically, so just report @@ -56,10 +66,17 @@ Then write `.factory/runs/issue-/verdict.json`: { "result": "pass", "rounds": 1, - "findings": [] + "findings": [], + "criteria": [{ "id": "AC-1", "status": "pass" }] } ``` +A finding is `{ "severity": "must|should|could", "confidence": 0-5, "what": "...", +"where": "file:line", "why": "...", "fix": "..." }` (`what` is required). A +criterion is `pass`, `fail`, or `unverified` (with a `gap`); AC ids come from the +plan and are never renumbered. No other fields are allowed, and the file must be +one JSON object under 16 KiB. + `result` is `pass`, `reject`, or `uncertain`. `findings` is the reviewer's list verbatim (empty array if none). On `reject`, the runner sends the issue back to `factory-build`, up to twice; a third reject, or an diff --git a/tests/agents.test.ts b/tests/agents.test.ts index 23b73e9..f99910f 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -4,14 +4,14 @@ // artifact contract reaches the prompt, and every preset can be diagnosed. import { afterAll, describe, expect, test } from "bun:test"; -import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { CommandExecutor, renderCommand, resolveAgent } from "../src/agents/executor"; import { artifactContract, renderPrompt, stripFrontmatter } from "../src/agents/prompt"; import { claudeArgs } from "../src/agents/presets/claude"; import { PRESETS } from "../src/agents/presets"; -import { COMMENT_FILENAMES, JSON_FILENAMES } from "../src/artifacts"; +import { COMMENT_FILENAMES, JSON_FILENAMES, MAX_STEP_JSON_BYTES, readStageArtifacts, validateVerdict, writeGateEvidence } from "../src/artifacts"; import { configProblems, DEFAULT_CONFIG, mergeConfig } from "../src/config"; import type { StageName } from "../src/executor"; import { STAGE_GUIDANCE, stageSettings } from "../src/stage-permissions"; @@ -159,6 +159,33 @@ describe("an agent with no preset", () => { }); }); +describe("the runner refuses a self-contradicting verdict", () => { + test("pass with a blocking finding goes to a human, and the verdict is never posted", async () => { + const workspacesDir = mkdtempSync(join(scratch, "ws-")); + process.env.FAKE_VERDICT = JSON.stringify({ result: "pass", rounds: 1, findings: [{ severity: "must", confidence: 5, what: "drops a cent" }] }); + const issue = baseIssue(2, [LABEL.ready]); + const github = new FakeGitHub([issue]); + const state = new FactoryState(":memory:"); + const config = mergeConfig({ repo: "acme/widgets", agents: { scripted: { command: [join(import.meta.dir, "fixtures/agents/fake-agent.sh")] } }, stages: { default: "scripted" } }); + class SkillGit extends FakeGit { + override async ensureWorktree(c: string, w: string, n: number) { + await super.ensureWorktree(c, w, n); + cpSync(join(import.meta.dir, "../template/.claude/skills"), join(w, ".claude/skills"), { recursive: true }); + } + } + try { + const deps = { github, git: new SkillGit(), state, executor: new CommandExecutor(config.agents, config.stages), gateRunner: new FakeGateRunner(), cloneDir: scratch, workspacesDir }; + state.setToggle("auto_approve_low_risk", true); + expect(await processReadyIssue(issue, deps, config)).toBe("needs-human"); + } finally { + delete process.env.FAKE_VERDICT; + } + expect(state.getRun("acme/widgets", 2)!.reason).toMatch(/blocking finding: drops a cent/); + expect(github.createdPrs).toHaveLength(0); + state.close(); + }); +}); + describe("every preset is diagnosable", () => { test("has a binary, parses a blank line to nothing, and never lets the prompt into argv when it owns stdin", () => { for (const [name, preset] of Object.entries(PRESETS)) { @@ -171,3 +198,53 @@ describe("every preset is diagnosable", () => { } }); }); + +describe("verdict rules", () => { + const f = (severity: string, confidence: number) => ({ severity, confidence, what: "x" }); + const check = (v: unknown) => validateVerdict(v); + + test("a consistent verdict is accepted, old string findings included", () => { + expect(check({ result: "pass", rounds: 1, findings: [], criteria: [{ id: "AC-1", status: "pass" }] }).ok).toBe(true); + expect(check({ result: "reject", rounds: 1, findings: ["a", f("must", 5)] }).ok).toBe(true); + expect(check({ result: "pass", rounds: 1, findings: ["note", f("could", 5), f("must", 2)] }).ok).toBe(true); + }); + + test.each([ + [[], /not a JSON object/], + [{ result: "pass", rounds: 1, findings: [], extra: 1 }, /unknown field "extra"/], + [{ result: "ok", rounds: 1, findings: [] }, /"result" must be/], + [{ result: "pass", rounds: 1.5, findings: [] }, /"rounds"/], + [{ result: "pass", rounds: 1, findings: [f("must", 9)] }, /confidence/], + [{ result: "pass", rounds: 1, findings: [{ ...f("must", 1), color: "red" }] }, /unknown field "color"/], + [{ result: "pass", rounds: 1, findings: [f("must", 4)] }, /pass but lists a blocking finding/], + [{ result: "pass", rounds: 1, findings: [f("should", 3)] }, /pass but lists a blocking finding/], + [{ result: "pass", rounds: 1, findings: [], criteria: [{ id: "AC-2", status: "unverified" }] }, /AC-2 is unverified/], + [{ result: "pass", rounds: 1, findings: [], criteria: [{ id: "two", status: "pass" }] }, /look like AC-1/], + ])("refuses %j", (v, want) => { + const r = check(v); + expect(r.ok).toBe(false); + expect(r.ok ? "" : r.reason).toMatch(want); + }); + + test("a step result that is oversize, not JSON or not an object reads as absent", async () => { + const cwd = mkdtempSync(join(scratch, "art-")); + const dir = join(cwd, ".factory/runs/issue-3"); + mkdirSync(dir, { recursive: true }); + for (const [body, name] of [["x".repeat(MAX_STEP_JSON_BYTES + 1), "big"], ["{nope", "junk"], ["[1]", "array"]] as const) { + writeFileSync(join(dir, "verdict.json"), name === "big" ? JSON.stringify({ result: "pass", pad: body }) : body); + expect((await readStageArtifacts(cwd, 3, "verify")).json, name).toBeUndefined(); + } + }); + + test("build hands the verifier gate evidence tied to the tree it measured", async () => { + const cwd = mkdtempSync(join(scratch, "gate-")); + await writeGateEvidence(cwd, 5, { line: "FACTORY_GATES: status=GREEN", status: "GREEN", tree: "abc" }); + expect(JSON.parse(readFileSync(join(cwd, ".factory/runs/issue-5/gate.json"), "utf8"))).toEqual({ line: "FACTORY_GATES: status=GREEN", status: "GREEN", tree: "abc" }); + }); + + test("the verify skill and reviewer teach the same schema the runner enforces", () => { + const skill = readFileSync(join(import.meta.dir, "../template/.claude/skills/factory-verify/SKILL.md"), "utf8"); + for (const word of ["gate.json", "HEAD^{tree}", "unverified", "must|should|could", "16 KiB", "AC-1"]) expect(skill).toContain(word); + expect(readFileSync(join(import.meta.dir, "../template/.claude/agents/factory-reviewer.md"), "utf8")).toContain("confidence 0-5"); + }); +}); diff --git a/tests/fixtures/agents/fake-agent.sh b/tests/fixtures/agents/fake-agent.sh index 8d8dc65..4c48ba6 100755 --- a/tests/fixtures/agents/fake-agent.sh +++ b/tests/fixtures/agents/fake-agent.sh @@ -1,7 +1,7 @@ #!/bin/sh # A stand-in for any coding agent with no preset and no JSON output: it reads # the prompt on stdin and writes the stage's artifacts to $FACTORY_ARTIFACT_DIR. -# FAKE_AGENT_LOG (a directory) keeps each stage's prompt for the tests to read. +# FAKE_VERDICT overrides verdict.json. FAKE_AGENT_LOG (a directory) keeps each stage's prompt for the tests to read. prompt=$(cat) [ -n "$FAKE_AGENT_LOG" ] && printf '%s' "$prompt" > "$FAKE_AGENT_LOG/$FACTORY_STAGE.prompt" d="$FACTORY_ARTIFACT_DIR" @@ -18,7 +18,8 @@ case "$FACTORY_STAGE" in printf '{"status":"green","gate_line":"make check: 10 pass","rounds":1}\n' > "$d/build.json" ;; verify) printf '\npass\n' > "$d/verdict-comment.md" - printf '{"result":"pass","rounds":1,"findings":[]}\n' > "$d/verdict.json" ;; + if [ -n "$FAKE_VERDICT" ]; then printf '%s\n' "$FAKE_VERDICT" > "$d/verdict.json" + else printf '{"result":"pass","rounds":1,"findings":[]}\n' > "$d/verdict.json"; fi ;; pr) printf '## Summary\nDid the thing.\nCloses #%s\n' "$n" > "$d/pr-body.md" ;; esac diff --git a/tests/harness.ts b/tests/harness.ts index caa7edd..4edd92e 100644 --- a/tests/harness.ts +++ b/tests/harness.ts @@ -181,6 +181,10 @@ export class FakeGit extends Git { return { stdout: "", stderr: "", code: 0 }; } + override async treeHash(): Promise { + return "faketree"; + } + override async hasCommits(): Promise { return true; } From abf28d14c67a8147514aed5e8b51097b1aca3f1f Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:12:46 +0300 Subject: [PATCH 6/7] v2.5.0: docs, changelog, version bump Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 24 ++++++++++++++++++++++++ README.md | 26 ++++++++++++++++++++++++-- package.json | 2 +- template-ci/factory.yml.example | 2 +- tests/release.test.ts | 7 +++++++ 5 files changed, 57 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb4e595..e3d40be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,29 @@ # Changelog +## v2.5.0 + +Any coding agent: the factory no longer knows Claude by name. An agent is config. + +- `src/agents/`: one `CommandExecutor` (spawn, prompt on stdin or in the command, timeout, process-group + kill, stderr tail, bounded event log, tool-call cap) and presets for `claude` and `codex`. Claude's + command is unchanged from v2.4.0 (a test pins it), plus `--model` when `agent.model` is set. +- `.factory/config.json` gains `agents` (a `preset`, or your own `command` with `{{prompt}}`, + `{{promptFile}}`, `{{model}}`) and `stages` (`default` plus per-stage overrides, so one agent can build and + another verify). Config is validated at boot, including an unknown preset or stage. +- Any agent that can write files works with no adapter: it gets the stage skill plus an artifact contract, and + `FACTORY_ARTIFACT_DIR`, `FACTORY_ISSUE`, `FACTORY_STAGE`, `FACTORY_SCRATCH_DIR`. Runner credentials and repo + `GIT_*` variables are stripped from its environment. Without a preset, tokens show as not reported. +- Token totals follow machinist: the last terminal event wins, malformed usage is "not reported", never zero. + `stage_runs` records which agent and model ran each stage. +- Verify: findings are structured (`must|should|could`, confidence 0-5, what/why/where/fix) and per-criterion + `pass|fail|unverified`. The runner sends a self-contradicting `pass` to a human. A step result must be one + JSON object under 16 KiB. The runner writes `gate.json` (gate line and git tree hash) so a read-only + verifier trusts current evidence instead of re-running the gates. +- `factory doctor` checks the binary of each agent a stage uses and warns about one with no preset. + +Not in this release: Gemini, OpenCode, Pi, omp, Mastra Code and Amp presets (v2.6.0). The Codex preset is +covered by recorded-shape tests only; a live run is pending. + ## v2.4.0 The cockpit: a redesigned dashboard and one place for everything that waits on a human. diff --git a/README.md b/README.md index 2de2ac1..f5643e0 100644 --- a/README.md +++ b/README.md @@ -170,7 +170,8 @@ Checkpoint branches in the example target repo (`learnwithparam/splitbill`) use - [Bun](https://bun.sh) ≥ 1.3 - [`gh`](https://cli.github.com), authenticated (`gh auth status`) with write access to the target repo -- [`claude`](https://claude.com/claude-code) on `PATH`, logged in, each stage runs as `claude -p` +- an agent CLI on `PATH`, logged in: [`claude`](https://claude.com/claude-code) by default (each stage runs + as `claude -p`), or `codex`, or any CLI you configure under `agents` - [`uv`](https://docs.astral.sh/uv/) (for `uvx`, used to validate skills against the [agentskills.io](https://agentskills.io) spec) - [Docker](https://www.docker.com) only if you're using VM mode @@ -231,6 +232,27 @@ build). Stages get only what the config grants, through `--settings` on `claude Unknown keys and a missing `repo` are errors, other missing fields take the defaults in `src/config.ts`. +### Choosing the agent + +The factory knows no agent by name. Each one is config, and `stages` says which agent runs which stage: + +```json +{ + "agents": { + "claude": { "preset": "claude" }, + "codex": { "preset": "codex", "model": "gpt-5.6-terra" }, + "aider": { "command": ["aider", "--yes-always", "--message-file", "{{promptFile}}"] } + }, + "stages": { "default": "claude", "verify": "codex" } +} +``` + +Presets: `claude`, `codex`. A `command` agent gets the stage skill plus an artifact contract on stdin, or +where `{{prompt}}` / `{{promptFile}}` appears, and writes its results as files under +`$FACTORY_ARTIFACT_DIR`. It runs with no event parser: tokens show as "not reported" and the tool-call cap +cannot be enforced, so the timeout is the backstop and `factory doctor` says so. The guard hook and +`--settings` rules are Claude-only. The runner's diff check, gates and commit apply to every agent. + Then bring the target repo up to speed and start the loop: ```bash @@ -256,7 +278,7 @@ dependency bump), clone it to see the loop run against something real before wir | Command | Does | |---|---| | `factory install [--dry-run] [--update] [--ci]` | install or update the template in a repo | -| `factory doctor --repo-dir [--fix]` | check `gh`/`claude`/`python3`/`jq` on PATH, `gh auth status`, config, charter, gates.sh, baseline tag, labels | +| `factory doctor --repo-dir [--fix]` | check `gh`, each agent's binary, `python3`, `jq` on PATH, `gh auth status`, config, charter, gates.sh, baseline tag, labels | | `factory up [--repo-dir \| --repo ]` | watch + dashboard in one process, the Docker/VM entrypoint | | `factory watch [--repo-dir \| --repo ] [--once]` | poll and drive the loop (local mode) | | `factory run [--repo-dir \| --repo ] --issue ` | advance one issue once, then exit (CI mode) | diff --git a/package.json b/package.json index 984ff26..94c87d2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "software-factory", - "version": "2.4.0", + "version": "2.5.0", "private": true, "type": "module", "description": "GitHub-native SDLC loop for coding agents: triage, plan, build, verify, PR.", diff --git a/template-ci/factory.yml.example b/template-ci/factory.yml.example index 9d71732..ba95d08 100644 --- a/template-ci/factory.yml.example +++ b/template-ci/factory.yml.example @@ -35,7 +35,7 @@ on: required: false env: - FACTORY_RUNNER_REF: v2.4.0 # pinned software-factory release; bump deliberately + FACTORY_RUNNER_REF: v2.5.0 # pinned software-factory release; bump deliberately FACTORY_RUNNER_REPO: learnwithparam/software-factory CLAUDE_CODE_VERSION: "2.1.281" # pinned claude, same version the Dockerfile installs diff --git a/tests/release.test.ts b/tests/release.test.ts index 2705161..e05c812 100644 --- a/tests/release.test.ts +++ b/tests/release.test.ts @@ -26,3 +26,10 @@ test("the README describes only what exists: every label is listed and no monito expect(readme).not.toMatch(/verify, PR, monitor/); expect(readme).not.toMatch(/the monitor closes/); }); + +test("the README names every agent preset and the config keys that choose one", async () => { + const { PRESETS } = await import("../src/agents/presets"); + const readme = read("README.md"); + for (const name of Object.keys(PRESETS)) expect(readme).toContain(`"preset": "${name}"`); + for (const word of ['"agents"', '"stages"', "{{promptFile}}", "FACTORY_ARTIFACT_DIR"]) expect(readme).toContain(word); +}); From 56cecfbd093ae5873e6cdcd7f2b1ff9bb5c139ff Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 11:16:18 +0300 Subject: [PATCH 7/7] v2.5: post-kill pipe deadline, shell placeholder refusal, doctor default fix Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 1 + README.md | 6 +++--- src/agents/executor.ts | 20 +++++++++++++++++++- src/config.ts | 5 +++++ src/doctor.ts | 4 +++- tests/agents.test.ts | 36 ++++++++++++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3d40be..b3d0726 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Any coding agent: the factory no longer knows Claude by name. An agent is config `pass|fail|unverified`. The runner sends a self-contradicting `pass` to a human. A step result must be one JSON object under 16 KiB. The runner writes `gate.json` (gate line and git tree hash) so a read-only verifier trusts current evidence instead of re-running the gates. +- After a timeout the runner stops waiting on pipes held by a descendant that left the process group. - `factory doctor` checks the binary of each agent a stage uses and warns about one with no preset. Not in this release: Gemini, OpenCode, Pi, omp, Mastra Code and Amp presets (v2.6.0). The Codex preset is diff --git a/README.md b/README.md index f5643e0..ef26e79 100644 --- a/README.md +++ b/README.md @@ -247,11 +247,11 @@ The factory knows no agent by name. Each one is config, and `stages` says which } ``` -Presets: `claude`, `codex`. A `command` agent gets the stage skill plus an artifact contract on stdin, or +Presets: `claude`, `codex`. Setting both `preset` and `command` keeps the preset's event parser but runs your command. A `command` agent gets the stage skill plus an artifact contract on stdin, or where `{{prompt}}` / `{{promptFile}}` appears, and writes its results as files under `$FACTORY_ARTIFACT_DIR`. It runs with no event parser: tokens show as "not reported" and the tool-call cap -cannot be enforced, so the timeout is the backstop and `factory doctor` says so. The guard hook and -`--settings` rules are Claude-only. The runner's diff check, gates and commit apply to every agent. +cannot be enforced, so the timeout is the backstop and `factory doctor` says so. A shell as the executable may not take `{{prompt}}` as an argument. The guard hook and +`--settings` rules are Claude-only. A preset-less agent inherits your environment (minus `GH_TOKEN`, `GITHUB_TOKEN`, `FACTORY_*`, repo `GIT_*`), including model API keys and `~/.config/gh`: run it in a sandbox until v3.0. The runner's diff check, gates and commit apply to every agent. Then bring the target repo up to speed and start the loop: diff --git a/src/agents/executor.ts b/src/agents/executor.ts index fdd6deb..c519cdb 100644 --- a/src/agents/executor.ts +++ b/src/agents/executor.ts @@ -14,6 +14,7 @@ import { PRESETS } from "./presets"; import type { AgentConfig, AgentPreset, StageAgents } from "./types"; const DEFAULT_TIMEOUT_MINUTES = 15; +const STDERR_KEEP_BYTES = 64 * 1024; export const MAX_EVENT_LINE_BYTES = 1 << 20; export const MAX_RECORDED_OUTPUT_BYTES = 64 << 20; @@ -97,6 +98,7 @@ export class CommandExecutor implements Executor { FACTORY_SCRATCH_DIR: scratch, }, }); + let cancelRead: (() => void) | undefined; const killGroup = () => { // ESRCH means it already exited; that is the goal. try { @@ -104,6 +106,9 @@ export class CommandExecutor implements Executor { } catch { proc.kill(); } + // A descendant that left the group (setsid) can still hold the pipe open: + // give the reader a moment to drain, then stop waiting for it. + setTimeout(() => cancelRead?.(), 2000).unref(); }; const events: StageEvent[] = []; let toolCalls = 0; @@ -112,7 +117,16 @@ export class CommandExecutor implements Executor { let recorded = 0; // Read alongside stdout, not after: an unread pipe can fill its OS buffer // and stall the child, and launch-time errors go to stderr only. - const stderrPromise = new Response(proc.stderr).text(); + const errReader = proc.stderr.getReader(); + const stderrPromise = (async () => { + const dec = new TextDecoder(); + let text = ""; + for (;;) { + const { done, value } = await errReader.read().catch(() => ({ done: true, value: undefined })); + if (done) return text + dec.decode(); + text = (text + dec.decode(value, { stream: true })).slice(-STDERR_KEEP_BYTES); + } + })(); const timeoutMinutes = opts.timeoutMinutes ?? DEFAULT_TIMEOUT_MINUTES; const timer = setTimeout(() => { @@ -143,6 +157,10 @@ export class CommandExecutor implements Executor { try { const reader = proc.stdout.getReader(); + cancelRead = () => { + void reader.cancel().catch(() => {}); + void errReader.cancel().catch(() => {}); + }; const decoder = new TextDecoder(); let buffer = ""; for (;;) { diff --git a/src/config.ts b/src/config.ts index a11fe5c..d0bdb08 100644 --- a/src/config.ts +++ b/src/config.ts @@ -176,7 +176,12 @@ function agentProblems(agents: unknown, stages: unknown): string[] { if (a.preset === undefined && a.command === undefined) problems.push(`${where}needs a "preset" or a "command"`); if (Array.isArray(a.command)) { if (a.command.length === 0) problems.push(`${where}command: must not be empty`); + else if (!String(a.command[0]).trim()) problems.push(`${where}command: the executable must not be empty`); else if (/\{\{/.test(String(a.command[0]))) problems.push(`${where}command: the executable cannot be a placeholder`); + // `sh -c "{{prompt}}"` would run issue text as shell code. + else if (/^(ba|z|da|k|c)?sh$/.test(String(a.command[0]).split("/").pop()!) && a.command.slice(1).some((x) => /\{\{prompt\}\}/.test(String(x)))) { + problems.push(`${where}command: {{prompt}} must not be an argument of a shell (use {{promptFile}} or stdin)`); + } } } } else if (agents !== undefined) problems.push("agents: expected an object"); diff --git a/src/doctor.ts b/src/doctor.ts index 0f76462..db1db38 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -43,7 +43,9 @@ export async function runDoctor(deps: DoctorDeps, ctx: DoctorContext): Promise Boolean(n))); + // The default only counts if some stage falls back to it. + const STAGES = ["triage", "plan", "build", "verify", "pr"] as const; + const used = new Set(STAGES.map((st) => stages[st] ?? stages.default ?? "claude")); for (const name of [...used].sort()) { const agent = agents[name]; const preset = agent?.preset ? PRESETS[agent.preset] : undefined; diff --git a/tests/agents.test.ts b/tests/agents.test.ts index f99910f..f888e73 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -248,3 +248,39 @@ describe("verdict rules", () => { expect(readFileSync(join(import.meta.dir, "../template/.claude/agents/factory-reviewer.md"), "utf8")).toContain("confidence 0-5"); }); }); + +describe("hardening from the verifier report", () => { + test.each([ + [{ agents: { x: { command: [""] } } }, /must not be empty/], + [{ agents: { x: { command: ["sh", "-c", "run {{prompt}}"] } } }, /must not be an argument of a shell/], + [{ agents: { x: { command: ["/bin/bash", "-c", "{{prompt}}"] } } }, /must not be an argument of a shell/], + ])("refuses %j", (cfg, want) => { + expect(configProblems({ repo: "a/b", ...cfg }).join("\n")).toMatch(want); + }); + + test("a shell that reads the prompt from a file is fine", () => { + expect(configProblems({ repo: "a/b", agents: { x: { command: ["sh", "run.sh", "{{promptFile}}"] } } })).toEqual([]); + }); + + test("doctor does not require claude when every stage names another agent", async () => { + const { runDoctor } = await import("../src/doctor"); + const deps = { github: { authStatus: async () => ({ ok: true, detail: "" }), listLabels: async () => [] } as never, git: { run: async () => ({ stdout: "x", stderr: "", code: 0 }) }, which: async () => true, fileExists: async () => true, readFile: async () => "{}", isExecutable: async () => true }; + const all = Object.fromEntries(STAGES.map((s) => [s, "codex"])); + const checks = await runDoctor(deps, { repo: "a/b", cloneDir: "/x", baselineTag: "b", agents: { claude: { preset: "claude" }, codex: { preset: "codex" } }, stages: { default: "claude", ...all } }); + expect(checks.map((c) => c.name)).toContain("codex on PATH"); + expect(checks.map((c) => c.name)).not.toContain("claude on PATH"); + }); + + test("a grandchild that leaves the process group cannot hang the stage past the timeout", async () => { + const cwd = mkdtempSync(join(scratch, "setsid-")); + cpSync(join(import.meta.dir, "../template/.claude/skills"), join(cwd, ".claude/skills"), { recursive: true }); + const script = join(cwd, "escape.sh"); + writeFileSync(script, `#!/bin/sh\nperl -e 'use POSIX; POSIX::setsid(); exec "sleep","4719"' &\nsleep 30\n`, { mode: 0o755 }); + const ex = new CommandExecutor({ x: { command: [script] } }, { default: "x" }); + const t0 = Date.now(); + const r = await ex.runStage({ stage: "triage", issue: 1, cwd, maxBudgetUsd: 1, timeoutMinutes: 0.02 }); + Bun.spawnSync(["pkill", "-f", "sleep 4719"]); + expect(r.killedReason).toMatch(/stageTimeoutMinutes/); + expect(Date.now() - t0).toBeLessThan(6000); + }); +});