From 8da8248e934a98bcd1ae5559cc71ce3850bd782a Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:19:29 +0300 Subject: [PATCH 1/9] Ported: step-outcome protocol (P30), config header (P38), per-row provenance markers Co-Authored-By: Claude Sonnet 5 --- THIRD_PARTY_NOTICES.md | 32 ++++++++-------- src/artifacts.ts | 49 ++++++++++++++++++++++++- src/config.ts | 1 + src/watch.ts | 37 ++++++++++++++++--- tests/ported/assembler/config.test.ts | 24 ++++++++++++ tests/ported/machinist/workflow.test.ts | 47 ++++++++++++++++++++++++ tests/provenance.test.ts | 35 ++++++++++++++++-- tests/scenarios.test.ts | 36 ++++++++++++++++++ 8 files changed, 235 insertions(+), 26 deletions(-) create mode 100644 tests/ported/assembler/config.test.ts create mode 100644 tests/ported/machinist/workflow.test.ts diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index 64ea4fe..2deb3b4 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -8,26 +8,28 @@ in step. Only MIT-licensed code is copied. Source: https://github.com/owainlewis/machinist (MIT, Copyright (c) 2026 Owain Lewis) -- `dashboard/public/styles.css` from `internal/controlplane/web/src/styles.css` -- `dashboard/public/lib/run-metrics.js` from `internal/controlplane/web/src/run-metrics.js` -- `dashboard/public/lib/runs-board.js` from `internal/controlplane/web/src/runs-board.js` -- `dashboard/public/lib/status-loader.js` from `internal/controlplane/web/src/status-loader.js` -- `dashboard/public/lib/analytics-state.js` from `internal/controlplane/web/src/analytics-state.js` -- `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` +- `dashboard/public/styles.css` from `internal/controlplane/web/src/styles.css` [no upstream test] +- `dashboard/public/lib/run-metrics.js` from `internal/controlplane/web/src/run-metrics.js` [tested by `tests/ported/machinist/run-metrics.test.ts`] +- `dashboard/public/lib/runs-board.js` from `internal/controlplane/web/src/runs-board.js` [tested by `tests/ported/machinist/runs-board.test.ts`] +- `dashboard/public/lib/status-loader.js` from `internal/controlplane/web/src/status-loader.js` [tested by `tests/ported/machinist/status-ui.test.ts`] +- `dashboard/public/lib/analytics-state.js` from `internal/controlplane/web/src/analytics-state.js` [no upstream test] +- `dashboard/public/lib/task-presentation.js` from `internal/controlplane/web/src/task-presentation.js` [tested by `tests/ported/machinist/task-presentation.test.ts`] +- `dashboard/public/lib/routes.js` from `internal/controlplane/web/src/routes.js` [tested by `tests/ported/machinist/routes.test.ts`] +- `src/revision.ts` from `internal/runner/revision.go` (shape from `internal/protocol/revision.go`) [tested by `tests/ported/machinist/revision.test.ts`] +- `src/agents/executor.ts` from `internal/runner/runner.go` (process group kill from `process_unix.go`) [tested by `tests/ported/machinist/runner.test.ts`] +- `src/agents/env.ts` from `internal/runner/runner.go` [tested by `tests/ported/machinist/runner.test.ts`] +- `src/agents/final-message.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/final-message.test.ts`] +- `src/agents/presets/codex.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] +- `src/agents/usage.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] +- `src/artifacts.ts` from `internal/protocol/workflow.go` [tested by `tests/ported/machinist/workflow.test.ts`] ## owainlewis/assembler@7cac671 Source: https://github.com/owainlewis/assembler (MIT, Copyright (c) 2026 Owain Lewis) -- `src/display.ts` from `src/display.ts` -- `src/logs.ts` from `src/runs.ts` +- `src/display.ts` from `src/display.ts` [tested by `tests/ported/assembler/display.test.ts`] +- `src/logs.ts` from `src/runs.ts` [tested by `tests/ported/assembler/logs.test.ts`] +- `src/config.ts` from `src/index.ts` [tested by `tests/ported/assembler/config.test.ts`] ## License text (both projects) diff --git a/src/artifacts.ts b/src/artifacts.ts index ef95016..ce34096 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -1,3 +1,4 @@ +// Ported from owainlewis/machinist@3943516 internal/protocol/workflow.go:11-37 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the step envelope (outcome, summary) is optional on our stage artifacts and an absent outcome means complete; every stage rejects unknown fields against its own key list; the runner reads the file, so the 16 KiB and one-object rules live in readJson. // The contract between a stage skill (running inside `claude`, sandboxed by // guard-paths.sh + settings.json) and the runner. Section 9 of the plan says // "the agent cannot push, merge or call gh; only the runner talks to GitHub" — @@ -12,7 +13,13 @@ export type Risk = "low" | "medium" | "high"; export type VerdictResult = "pass" | "reject" | "uncertain"; export type BuildStatus = "green" | "red" | "needs-info"; +// How a step ended, from machinist's StepResult. Absent means complete. +export type StepOutcome = "complete" | "blocked" | "failed"; +const STEP_OUTCOMES: readonly string[] = ["complete", "blocked", "failed"]; + export interface TriageArtifact { + readonly outcome?: StepOutcome; + readonly summary?: string; readonly disposition: Disposition; readonly type: "bug" | "feature" | "docs" | "security" | "dependency"; readonly risk: Risk; @@ -23,6 +30,8 @@ export interface TriageArtifact { } export interface PlanArtifact { + readonly outcome?: StepOutcome; + readonly summary?: string; readonly status?: "needs-info"; readonly risk: Risk; readonly revision: number; @@ -32,6 +41,8 @@ export interface PlanArtifact { } export interface BuildArtifact { + readonly outcome?: StepOutcome; + readonly summary?: string; readonly status: BuildStatus; readonly gate_line: string; readonly rounds: number; @@ -55,6 +66,8 @@ export interface Criterion { } export interface VerdictArtifact { + readonly outcome?: StepOutcome; + readonly summary?: string; readonly result: VerdictResult; readonly rounds: number; readonly findings: (string | Finding)[]; @@ -63,7 +76,7 @@ export interface VerdictArtifact { // 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 VERDICT_KEYS = new Set(["result", "rounds", "findings", "criteria", "outcome", "summary"]); 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. @@ -73,6 +86,38 @@ function unknownKey(obj: object, allowed: Set): string | undefined { return Object.keys(obj).find((k) => !allowed.has(k)); } +function stepEnvelopeProblem(o: Record): string | undefined { + if (o.outcome !== undefined && !STEP_OUTCOMES.includes(o.outcome as string)) return "step outcome must be complete, blocked, or failed"; + if (o.summary !== undefined && (typeof o.summary !== "string" || !o.summary.trim())) return "step summary must be a non-empty string"; + return undefined; +} + +// Every stage artifact rejects a field it does not define, so a typo cannot +// pass as a silent no-op. The verdict has its own, deeper validator above. +const STEP_KEYS: Record, readonly string[]> = { + triage: ["disposition", "type", "risk", "done_when", "files_expected", "gate_level", "confidence", "outcome", "summary"], + plan: ["status", "risk", "revision", "files", "autoApproveEligible", "commentId", "outcome", "summary"], + build: ["status", "gate_line", "rounds", "outcome", "summary"], + pr: ["outcome", "summary"], +}; + +export function validateStepJson(stage: Exclude, raw: unknown): { ok: true } | { ok: false; reason: string } { + const name = JSON_FILENAMES[stage]; + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) return { ok: false, reason: `${name} is not a JSON object` }; + const o = raw as Record; + const extra = unknownKey(o, new Set(STEP_KEYS[stage])); + if (extra) return { ok: false, reason: `${name} has unknown field "${extra}"` }; + const problem = stepEnvelopeProblem(o); + return problem ? { ok: false, reason: `${name}: ${problem}` } : { ok: true }; +} + +// Where a step that reported "blocked" or "failed" stops. Undefined means carry on. +export function stepStop(json: { outcome?: StepOutcome; summary?: string } | undefined): { status: "needs-human" | "failed"; reason: string } | undefined { + if (json?.outcome === "blocked") return { status: "needs-human", reason: json.summary ?? "the agent reported it was blocked" }; + if (json?.outcome === "failed") return { status: "failed", reason: json.summary ?? "the agent reported failure" }; + return undefined; +} + // 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 } { @@ -81,6 +126,8 @@ export function validateVerdict(raw: unknown): { ok: true; verdict: VerdictArtif const v = raw as Record; const extra = unknownKey(v, VERDICT_KEYS); if (extra) return bad(`verdict.json has unknown field "${extra}"`); + const envelope = stepEnvelopeProblem(v); + if (envelope) return bad(envelope); 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'); diff --git a/src/config.ts b/src/config.ts index d0bdb08..77063c3 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,3 +1,4 @@ +// Ported from owainlewis/assembler@7cac671 src/index.ts:64-90 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: validateConfig's harness rules (executable not a {prompt} placeholder, a command must be non-empty) live in agentProblems; the codex/claude providers are presets; errors are collected, not thrown one at a time; the key table, boot refusal and defaults are the factory's own. // Shape of the target repo's `.factory/config.json`. Built by the repo-specific // side (splitbill) or by whoever installs this template elsewhere; the runner // only reads it, and `factory doctor` checks it exists. Missing fields fall diff --git a/src/watch.ts b/src/watch.ts index cf7f874..43de629 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -15,6 +15,8 @@ import { writeRevision } from "./revision"; import type { Executor, StageName, StageRunResult } from "./executor"; import { clearStageArtifacts, + stepStop, + validateStepJson, validateVerdict, writeGateEvidence, readStageArtifacts, @@ -214,6 +216,21 @@ async function runStage( return result; } +// A stage's JSON, or why it cannot be used: an unknown field is refused, and +// an `outcome` of blocked or failed stops the run where the agent said it did. +type StepStage = "triage" | "plan" | "build"; +function stageJson(stage: StepStage, raw: unknown): { json?: T; problem?: string } { + if (raw === undefined) return {}; + const checked = validateStepJson(stage, raw); + return checked.ok ? { json: raw as T } : { problem: checked.reason }; +} + +async function stopStep(deps: WatchDeps, config: FactoryConfig, issueNumber: number, from: string, stop: { status: "needs-human" | "failed"; reason: string }): Promise { + await moveLabel(deps, config, issueNumber, from, stop.status === "failed" ? LABEL.failed : LABEL.needsHuman); + finish(deps, config, issueNumber, stop.status, stop.reason); + return stop.status; +} + interface RunCtx { rejectRound: number; questionRound: number; @@ -239,12 +256,14 @@ async function runFromStage( if (stage === "triage") { const result = await runStage(deps, config, issue, "triage", worktree); const art = await readStageArtifacts(worktree, issueNumber, "triage"); - const json = art.json as TriageArtifact | undefined; + const { json, problem } = stageJson("triage", art.json); if (result.exitCode !== 0 || !json) { await moveLabel(deps, config, issueNumber, LABEL.triaging, LABEL.failed); - finish(deps, config, issueNumber, "failed", stageFailure(result, "triage produced no valid triage.json")); + finish(deps, config, issueNumber, "failed", problem ?? stageFailure(result, "triage produced no valid triage.json")); return "failed"; } + const triageStop = stepStop(json); + if (triageStop) return stopStep(deps, config, issueNumber, LABEL.triaging, triageStop); if (art.comment) await postComment(deps, config, issueNumber, art.comment, { stage: "triage", json }); if (json.disposition === "refused" || json.disposition === "duplicate") { await moveLabel(deps, config, issueNumber, LABEL.triaging, LABEL.needsHuman); @@ -271,15 +290,17 @@ async function runFromStage( if (stage === "plan") { const result = await runStage(deps, config, issue, "plan", worktree); const art = await readStageArtifacts(worktree, issueNumber, "plan"); - const json = art.json as PlanArtifact | undefined; + const { json, problem } = stageJson("plan", art.json); // A plan stage that crashes (or writes nothing) used to fall through // to "not eligible" and park as awaiting-approval with no plan comment // to approve against — stuck forever (audit finding #10). if (result.exitCode !== 0 || !json) { await moveLabel(deps, config, issueNumber, LABEL.planning, LABEL.failed); - finish(deps, config, issueNumber, "failed", stageFailure(result, "plan produced no valid plan.json")); + finish(deps, config, issueNumber, "failed", problem ?? stageFailure(result, "plan produced no valid plan.json")); return "failed"; } + const planStop = stepStop(json); + if (planStop) return stopStep(deps, config, issueNumber, LABEL.planning, planStop); if (json.status === "needs-info") { if (ctx.questionRound >= MAX_QUESTION_ROUNDS) { await moveLabel(deps, config, issueNumber, LABEL.planning, LABEL.needsHuman); @@ -308,7 +329,9 @@ async function runFromStage( if (stage === "build") { const result = await runStage(deps, config, issue, "build", worktree); const art = await readStageArtifacts(worktree, issueNumber, "build"); - const json = art.json as BuildArtifact | undefined; + const { json, problem } = stageJson("build", art.json); + const buildStop = stepStop(json); + if (buildStop) return stopStep(deps, config, issueNumber, LABEL.building, buildStop); // The agent's own "needs-info" is a legitimate escape hatch (not a // crash), matched to the runner's exact spelling (audit finding #5: @@ -336,7 +359,7 @@ async function runFromStage( await moveLabel(deps, config, issueNumber, LABEL.building, LABEL.failed); deps.state.updateRun(config.repo, issueNumber, { gate_line: gate.raw }); const reason = - stageFailure(result) ?? + problem ?? stageFailure(result) ?? `gates ${gate.status.toLowerCase()}${gate.failedGates.length ? `: ${gate.failedGates.join(", ")}` : ""}`; finish(deps, config, issueNumber, "failed", reason); return "failed"; @@ -376,6 +399,8 @@ async function runFromStage( finish(deps, config, issueNumber, "needs-human", checked.reason); return "needs-human"; } + const verifyStop = stepStop(json); + if (verifyStop) return stopStep(deps, config, issueNumber, LABEL.verifying, verifyStop); if (result.exitCode !== 0 || !json || json.result === "uncertain") { await moveLabel(deps, config, issueNumber, LABEL.verifying, LABEL.needsHuman); finish( diff --git a/tests/ported/assembler/config.test.ts b/tests/ported/assembler/config.test.ts new file mode 100644 index 0000000..726a5b9 --- /dev/null +++ b/tests/ported/assembler/config.test.ts @@ -0,0 +1,24 @@ +// Ported from owainlewis/assembler@7cac671 test/runtime.test.ts:9-17,54-57 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: validateConfig throws one error and here configProblems lists all of them; "argument input requires {prompt}" has no equivalent (a command with no placeholder gets the prompt on stdin); the prompt is checked through renderCommand and Bun.spawn instead of execute(). + +import { expect, test } from "bun:test"; +import { configProblems } from "../../../src/config"; +import { renderCommand } from "../../../src/agents/executor"; + +const cfg = (agents: object, stages: object = {}) => configProblems({ repo: "a/b", agents, stages }); + +test("prompts remain literal arguments, including shell syntax", async () => { + const prompt = '`touch bad` $(echo bad) "quotes"\nline'; + const { argv, usesStdin } = renderCommand([process.execPath, "-e", "console.log(process.argv[1])", "{{prompt}}"], { prompt, promptFile: "", model: "" }); + expect(usesStdin).toBe(false); + const proc = Bun.spawn(argv, { stdout: "pipe" }); + expect((await new Response(proc.stdout).text()).trim()).toBe(prompt); +}); + +test("invalid agent configuration fails before execution", () => { + expect(cfg({ x: { command: ["x"] } }, { default: "missing" }).join("\n")).toMatch(/not an agent in config\.agents/); + expect(cfg({ x: { command: ["{{prompt}}"] } }).join("\n")).toMatch(/executable cannot be a placeholder/); + expect(cfg({ x: { command: [] } }).join("\n")).toMatch(/must not be empty/); + expect(cfg({ x: { command: [" "] } }).join("\n")).toMatch(/executable must not be empty/); + expect(cfg({ x: {} }).join("\n")).toMatch(/needs a "preset" or a "command"/); + expect(cfg({ x: { command: ["sh", "-c", "{{prompt}}"] } }).join("\n")).toMatch(/must not be an argument of a shell/); +}); diff --git a/tests/ported/machinist/workflow.test.ts b/tests/ported/machinist/workflow.test.ts new file mode 100644 index 0000000..1d19c8f --- /dev/null +++ b/tests/ported/machinist/workflow.test.ts @@ -0,0 +1,47 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/workflow_test.go:12-40 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the shell-script agent is replaced by the artifact file it would have written; "nonzero overrides result" is a runner rule and is asserted in tests/scenarios.test.ts (14c); the 16 KiB and one-object rules from protocol/workflow.go are added here. + +import { afterAll, expect, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { MAX_STEP_JSON_BYTES, readStageArtifacts, runDir, stepStop, validateStepJson } from "../../../src/artifacts"; + +const dir = mkdtempSync(join(tmpdir(), "factory-step-")); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +async function readBuild(body: string | undefined, issue: number) { + mkdirSync(join(dir, runDir(issue)), { recursive: true }); + if (body !== undefined) writeFileSync(join(dir, runDir(issue), "build.json"), body); + return (await readStageArtifacts(dir, issue, "build")).json; +} + +test("complete: a valid step result is read and does not stop the run", async () => { + const json = await readBuild('{"outcome":"complete","summary":"PR created","status":"green","gate_line":"ok","rounds":1}', 1); + expect(validateStepJson("build", json)).toEqual({ ok: true }); + expect(stepStop(json as never)).toBeUndefined(); +}); + +test("blocked: the summary becomes the reason the run parks", async () => { + const json = await readBuild('{"outcome":"blocked","summary":"Need requirements"}', 2); + expect(validateStepJson("build", json)).toEqual({ ok: true }); + expect(stepStop(json as never)).toEqual({ status: "needs-human", reason: "Need requirements" }); +}); + +test("missing: no file reads as no result", async () => { + expect(await readBuild(undefined, 3)).toBeUndefined(); +}); + +test("invalid: an outcome outside complete|blocked|failed is refused", async () => { + const json = await readBuild('{"outcome":"approved","summary":"ok"}', 4); + expect(validateStepJson("build", json)).toEqual({ ok: false, reason: "build.json: step outcome must be complete, blocked, or failed" }); +}); + +test("a summary must not be blank, and unknown fields are refused", () => { + expect(validateStepJson("build", { outcome: "complete", summary: " " }).ok).toBe(false); + expect(validateStepJson("build", { outcome: "complete", extra: 1 })).toEqual({ ok: false, reason: 'build.json has unknown field "extra"' }); +}); + +test("more than one JSON object, or a body over 16 KiB, reads as no result", async () => { + expect(await readBuild('{"outcome":"complete"}{"outcome":"complete"}', 5)).toBeUndefined(); + expect(await readBuild(JSON.stringify({ outcome: "complete", summary: "x".repeat(MAX_STEP_JSON_BYTES) }), 6)).toBeUndefined(); +}); diff --git a/tests/provenance.test.ts b/tests/provenance.test.ts index 51605ca..0dde5df 100644 --- a/tests/provenance.test.ts +++ b/tests/provenance.test.ts @@ -7,7 +7,8 @@ import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; import { join, relative } from "node:path"; const root = join(import.meta.dir, ".."); -const HEADER = /Ported from owainlewis\/([a-z.-]+)@([0-9a-f]{7}) (\S+) \(MIT, Copyright \(c\) 2026 Owain Lewis\)\. Deviations: \S/; +const HEADER = + /Ported from (?:owainlewis|mastra-ai)\/([a-z.-]+)@([0-9a-f]{7}) (\S+) \((?:MIT, Copyright \(c\) 2026 Owain Lewis|Apache-2\.0, [^)]+)\)\. Deviations: \S/; function walk(dir: string, out: string[] = []): string[] { for (const name of readdirSync(dir)) { @@ -19,15 +20,28 @@ function walk(dir: string, out: string[] = []): string[] { return out; } -const headers = walk(root).flatMap((file) => { - const first = readFileSync(file, "utf8").split("\n").slice(0, 2).join("\n"); +// Skills are ported too; their header is an HTML comment in the first lines. +function walkTemplate(dir: string, out: string[] = []): string[] { + if (!existsSync(dir)) return out; + for (const name of readdirSync(dir)) { + const path = join(dir, name); + if (statSync(path).isDirectory()) walkTemplate(path, out); + else if (name.endsWith(".md")) out.push(path); + } + return out; +} + +const sources = [...walk(root), ...walkTemplate(join(root, "template"))]; +const headers = sources.flatMap((file) => { + const first = readFileSync(file, "utf8").split("\n").slice(0, file.endsWith(".md") ? 8 : 2).join("\n"); const m = HEADER.exec(first); return first.includes("Ported from ") ? [{ file: relative(root, file), m }] : []; }); const notices = readFileSync(join(root, "THIRD_PARTY_NOTICES.md"), "utf8"); const entries = [...notices.matchAll(/^## owainlewis\/([a-z.-]+)@([0-9a-f]{7})$/gm)].map((m) => `${m[1]}@${m[2]}`); -const listed = [...notices.matchAll(/^- `([^`]+)` from /gm)].map((m) => m[1]!); +const rows = [...notices.matchAll(/^- `([^`]+)` from .*$/gm)].map((m) => ({ file: m[1]!, line: m[0] })); +const listed = rows.map((r) => r.file); test("every Ported-from header is well formed", () => { expect(headers.length).toBeGreaterThan(0); @@ -50,6 +64,19 @@ test("every file listed in the notices exists and carries a header", () => { } }); +test("every listed port names its upstream test, or says it has none", () => { + for (const { file, line } of rows) { + const t = /\[tested by `([^`]+)`\]/.exec(line); + if (!t) { + expect(line, `${file}: needs [tested by \`tests/ported/...\`] or [no upstream test]`).toContain("[no upstream test]"); + continue; + } + const path = join(root, t[1]!); + expect(existsSync(path), `${file}: ${t[1]} does not exist`).toBe(true); + expect(readFileSync(path, "utf8").split("\n").slice(0, 2).join("\n"), `${t[1]}: no Ported-from header`).toContain("Ported from "); + } +}); + test("every ported repo has ported upstream tests, and the licence text is included", () => { for (const repo of new Set(headers.map((h) => h.m![1]!))) { const dir = join(root, "tests", "ported", repo); diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index b31ee8a..200867b 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -335,6 +335,42 @@ describe("failure paths", () => { expect(c.state.getRun("acme/widgets", 1)!.reason).toContain("Write .factory/runs/issue-1/triage.json"); done(c); }); + + test("14b. a step that reports outcome blocked parks as needs-human with its summary", async () => { + const c = setup([LABEL.ready]); + c.push("triage", triage({ outcome: "blocked", summary: "Need the payment provider's sandbox key" })); + expect(await c.step()).toBe("needs-human"); + expect(labels(c.github, 1)).toEqual([LABEL.needsHuman]); + expect(c.state.getRun("acme/widgets", 1)!.reason).toBe("Need the payment provider's sandbox key"); + done(c); + }); + + test("14c. outcome failed fails the run; an outcome complete does not hide a non-zero exit", async () => { + const c = setup([LABEL.ready]); + c.push("triage", triage({ outcome: "failed", summary: "cannot reproduce" })); + expect(await c.step()).toBe("failed"); + expect(c.state.getRun("acme/widgets", 1)!.reason).toBe("cannot reproduce"); + const d = setup([LABEL.ready]); + d.push("triage", triage({ outcome: "complete", summary: "ok" }), { exitCode: 1 }); + expect(await d.step()).toBe("failed"); + done(c); + done(d); + }); + + test("14d. an unknown field in any stage JSON fails the stage and names the field", async () => { + const c = setup([LABEL.ready]); + c.push("triage", triage({ dispositon: "proceed" })); + expect(await c.step()).toBe("failed"); + expect(c.state.getRun("acme/widgets", 1)!.reason).toBe('triage.json has unknown field "dispositon"'); + const d = setup([LABEL.ready]); + d.state.setToggle("auto_approve_low_risk", true); + d.push("triage", triage()); + d.push("plan", plan("low", { file: ["src/b.ts"] })); + expect(await d.step()).toBe("failed"); + expect(d.state.getRun("acme/widgets", 1)!.reason).toBe('plan.json has unknown field "file"'); + done(c); + done(d); + }); }); describe("in-review and claim paths", () => { From 2ec06e75643f834653705e1c4573759da8d1c3dd Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:24:18 +0300 Subject: [PATCH 2/9] Cost meter (P16): pricing table, cached tokens, usage_complete, Not reported Co-Authored-By: Claude Sonnet 5 --- dashboard/public/app.js | 2 +- src/agents/presets/claude.ts | 4 +- src/agents/presets/codex.ts | 2 +- src/agents/usage.ts | 12 +++++- src/executor.ts | 20 +++++++-- src/pricing.ts | 45 ++++++++++++++++++++ src/state.ts | 25 +++++++++-- src/watch.ts | 16 ++++++- tests/ported/machinist/usage-metrics.test.ts | 31 ++++++++++++++ tests/pricing.test.ts | 31 ++++++++++++++ tests/scenarios.test.ts | 20 +++++++++ tests/state.test.ts | 13 ++++++ 12 files changed, 206 insertions(+), 15 deletions(-) create mode 100644 src/pricing.ts create mode 100644 tests/ported/machinist/usage-metrics.test.ts create mode 100644 tests/pricing.test.ts diff --git a/dashboard/public/app.js b/dashboard/public/app.js index 0fb3a60..34abc4a 100644 --- a/dashboard/public/app.js +++ b/dashboard/public/app.js @@ -215,7 +215,7 @@ function renderSheet() { h("h2", null, "Stages"), !sheet.stages ? h("p", { class: "muted" }, "Loading") : sheet.stages.length ? h("div", { class: "table-wrap" }, h("table", null, h("thead", null, h("tr", null, ["Stage", "Agent", "Took", "Tokens", "Cost"].map((c, i) => h("th", { class: i >= 2 ? "num" : "" }, c)))), h("tbody", null, sheet.stages.map((s) => h("tr", null, h("td", null, h("span", { class: "status", "data-tone": s.exit_code === 0 && !s.killed_reason ? "ok" : "bad" }, s.stage)), h("td", null, s.agent), h("td", { class: "num" }, formatDurationMillis(s.duration_ms)), - h("td", { class: "num" }, s.tokens_in + s.tokens_out ? compact(s.tokens_in + s.tokens_out) : "Not reported"), h("td", { class: "num" }, money(s.cost_usd))))))) : h("p", { class: "muted" }, "No stage has finished yet."), + h("td", { class: "num" }, s.usage_complete !== 0 && s.tokens_in + s.tokens_out ? compact(s.tokens_in + s.tokens_out) : "Not reported"), h("td", { class: "num" }, s.usage_complete === 0 ? "Not reported" : money(s.cost_usd))))))) : h("p", { class: "muted" }, "No stage has finished yet."), h("h2", { style: "margin-top:1.5rem" }, "Files from the run"), !sheet.artifacts ? h("p", { class: "muted" }, "Loading") : sheet.artifacts.length ? h("div", { class: "actions" }, sheet.artifacts.map((f) => h("button", { class: "btn", type: "button", onclick: () => preview(f.name) }, `${f.name} (${compact(f.size)}B)`))) : h("p", { class: "muted" }, "This run left no files on this machine."), sheet.preview && [h("p", { class: "muted" }, `${sheet.preview.name}${sheet.preview.truncated ? " (first 1 MiB)" : ""} `, h("a", { href: `/api/issues/${sheet.issue}/artifacts?file=${encodeURIComponent(sheet.preview.name)}&download=1` }, "Download")), h("pre", { class: "log" }, sheet.preview.text)], diff --git a/src/agents/presets/claude.ts b/src/agents/presets/claude.ts index a2bd70f..42be95d 100644 --- a/src/agents/presets/claude.ts +++ b/src/agents/presets/claude.ts @@ -58,11 +58,11 @@ export function parseStreamJsonLine(line: string): StageEvent[] { // 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(usage ? { kind: "usage", tokensIn: usage.tokensIn, tokensOut: usage.tokensOut, tokensCached: usage.tokensCached, total: true } : { kind: "usage", invalid: true }); } events.push({ kind: "result", - costUsd: parsed.total_cost_usd ?? parsed.cost_usd ?? 0, + ...(parsed.total_cost_usd ?? parsed.cost_usd) === undefined ? {} : { costUsd: parsed.total_cost_usd ?? parsed.cost_usd }, text: parsed.subtype, ...(denials.length ? { denials } : {}), ...(parsed.result ? { finalText: parsed.result } : {}), diff --git a/src/agents/presets/codex.ts b/src/agents/presets/codex.ts index 4d6b6b5..c88c14d 100644 --- a/src/agents/presets/codex.ts +++ b/src/agents/presets/codex.ts @@ -32,7 +32,7 @@ export function parseCodexLine(line: string): StageEvent[] { } 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 [usage ? { kind: "usage", tokensIn: usage.tokensIn, tokensOut: usage.tokensOut, tokensCached: usage.tokensCached, total: true } : { kind: "usage", invalid: true }]; } return []; } diff --git a/src/agents/usage.ts b/src/agents/usage.ts index 49e11e4..3726da4 100644 --- a/src/agents/usage.ts +++ b/src/agents/usage.ts @@ -1,10 +1,12 @@ -// 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. +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage.go:508-640 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: cache reads are reported apart as tokensCached, from evals/agent.py record_usage (Codex cached_input_tokens is optional and already inside input); 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; + // Cache reads, a subset of tokensIn (machinist agent.py record_usage). + readonly tokensCached: number; } const count = (v: unknown): number | null => (typeof v === "number" && Number.isSafeInteger(v) && v >= 0 ? v : null); @@ -17,13 +19,19 @@ export function readUsage(raw: unknown, cache: boolean): TokenUsage | null { const output = count(u.output_tokens); if (input === null || output === null) return null; let tokensIn = input; + let cached = 0; 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; + cached = read; + } else if (u.cached_input_tokens !== undefined) { + const c = count(u.cached_input_tokens); + if (c === null || c > input) return null; + cached = c; } - return Number.isSafeInteger(tokensIn + output) ? { tokensIn, tokensOut: output } : null; + return Number.isSafeInteger(tokensIn + output) ? { tokensIn, tokensOut: output, tokensCached: cached } : null; } // True when the line's own top-level "type" is `resultType`, even if the rest diff --git a/src/executor.ts b/src/executor.ts index 079cec1..9035571 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -15,6 +15,7 @@ export interface StageEvent { readonly text?: string; readonly tokensIn?: number; readonly tokensOut?: number; + readonly tokensCached?: 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". @@ -44,7 +45,10 @@ export interface StageRunResult { readonly toolCalls: number; readonly tokensIn: number; readonly tokensOut: number; + readonly tokensCached?: number; readonly costUsd: number; + // True when the agent itself reported a cost; false means costFor decides. + readonly costReported?: boolean; readonly exitCode: number; // Set when the runner killed the process itself (timeout or tool-call cap) // rather than letting it exit on its own — audit finding #15. @@ -74,34 +78,42 @@ export function aggregateStageEvents(events: StageEvent[], exitCode: number, std let tokensIn = 0; let tokensOut = 0; let costUsd = 0; + let costReported = false; + let tokensCached = 0; const permissionDenials: string[] = []; let finalText: string | undefined; - let final: { in: number; out: number } | "invalid" | undefined; + let final: { in: number; out: number; cached: 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") { if (e.invalid) final = "invalid"; - else if (e.total) final = { in: e.tokensIn ?? 0, out: e.tokensOut ?? 0 }; + else if (e.total) final = { in: e.tokensIn ?? 0, out: e.tokensOut ?? 0, cached: e.tokensCached ?? 0 }; else { tokensIn += e.tokensIn ?? 0; tokensOut += e.tokensOut ?? 0; + tokensCached += e.tokensCached ?? 0; } } if (e.kind === "result") { - costUsd = e.costUsd ?? costUsd; + if (e.costUsd !== undefined) { + costUsd = e.costUsd; + costReported = true; + } permissionDenials.push(...(e.denials ?? [])); } } if (final === "invalid") { tokensIn = 0; tokensOut = 0; + tokensCached = 0; } else if (final) { tokensIn = final.in; tokensOut = final.out; + tokensCached = final.cached; } const finalMessage = finalText === undefined ? undefined : truncateFinalMessage(finalText); - const result = { events, toolCalls, tokensIn, tokensOut, costUsd, exitCode, permissionDenials, usageComplete: final !== "invalid", ...(finalMessage ? { finalMessage } : {}) }; + const result = { events, toolCalls, tokensIn, tokensOut, tokensCached, costUsd, costReported, exitCode, permissionDenials, usageComplete: final !== "invalid", ...(finalMessage ? { finalMessage } : {}) }; return exitCode !== 0 && stderrTail ? { ...result, stderrTail } : result; } diff --git a/src/pricing.ts b/src/pricing.ts new file mode 100644 index 0000000..1189fbb --- /dev/null +++ b/src/pricing.ts @@ -0,0 +1,45 @@ +// Per-model list prices in USD per million tokens. An unknown model is +// undefined, never 0: an invented price is worse than "Not reported". +export interface Price { + readonly input: number; + readonly output: number; + readonly cacheRead: number; + readonly cacheWrite: number; + readonly source: string; + readonly asOf: string; +} + +const ANTHROPIC = "https://www.anthropic.com/pricing"; + +export const PRICES: Readonly> = { + "claude-haiku-4-5": { input: 1, output: 5, cacheRead: 0.1, cacheWrite: 1.25, source: ANTHROPIC, asOf: "2026-09-24" }, + "claude-sonnet-4-5": { input: 3, output: 15, cacheRead: 0.3, cacheWrite: 3.75, source: ANTHROPIC, asOf: "2026-09-24" }, + "claude-opus-4-5": { input: 5, output: 25, cacheRead: 0.5, cacheWrite: 6.25, source: ANTHROPIC, asOf: "2026-09-24" }, +}; + +// Models the docs and example config name whose price has not been sourced yet. +// Each one shows "Not reported" until a row moves into PRICES. +export const UNPRICED: Readonly> = { + "gpt-5.6-terra": "OpenAI list price not yet captured (needs a source URL)", +}; + +export interface PricedUsage { + readonly tokensIn: number; + readonly tokensOut: number; + // Cache reads, already counted inside tokensIn (Claude adds them there). + readonly tokensCached?: number; +} + +// Dated snapshots ("claude-haiku-4-5-20251001") price as their family. +function lookup(model: string): Price | undefined { + const key = model.replace(/-\d{8}$/, ""); + return PRICES[key]; +} + +export function costFor(model: string | null | undefined, usage: PricedUsage): number | undefined { + if (!model) return undefined; + const p = lookup(model); + if (!p) return undefined; + const cached = Math.min(usage.tokensCached ?? 0, usage.tokensIn); + return ((usage.tokensIn - cached) * p.input + cached * p.cacheRead + usage.tokensOut * p.output) / 1e6; +} diff --git a/src/state.ts b/src/state.ts index 21188e2..3ea090c 100644 --- a/src/state.ts +++ b/src/state.ts @@ -62,12 +62,16 @@ export interface StageRun { tool_calls: number; tokens_in: number; tokens_out: number; + tokens_cached: number; cost_usd: number; + // 0 when tokens or cost are not a full count; the dashboard shows "Not reported". + usage_complete: number; exit_code: number; killed_reason: string | null; } -export type StageRunInput = Omit; +// The two v2.5.1 columns default to 0 cached tokens and a complete count. +export type StageRunInput = Omit & Partial>; // Absolute, rooted at FACTORY_HOME (~/.factory by default, /data in Docker) — // see paths.ts. A relative path here broke on any machine where the process @@ -148,6 +152,19 @@ export class FactoryState { value TEXT NOT NULL ); `); + // Forward-only and idempotent: safe to run on boot from several replicas. + const have = new Set((this.db.query("PRAGMA table_info(stage_runs)").all() as { name: string }[]).map((c) => c.name)); + for (const [col, ddl] of [ + ["tokens_cached", "INTEGER NOT NULL DEFAULT 0"], + ["usage_complete", "INTEGER NOT NULL DEFAULT 1"], + ] as const) { + if (have.has(col)) continue; + try { + this.db.exec(`ALTER TABLE stage_runs ADD COLUMN ${col} ${ddl}`); + } catch (e) { + if (!/duplicate column/i.test(String(e))) throw e; + } + } } upsertRun(input: { @@ -242,8 +259,8 @@ export class FactoryState { recordStageRun(input: StageRunInput): void { this.db .query( - `INSERT INTO stage_runs (repo, issue, stage, agent, model, started_at, finished_at, duration_ms, tool_calls, tokens_in, tokens_out, cost_usd, exit_code, killed_reason) - VALUES ($repo, $issue, $stage, $agent, $model, $started_at, $finished_at, $duration_ms, $tool_calls, $tokens_in, $tokens_out, $cost_usd, $exit_code, $killed_reason)`, + `INSERT INTO stage_runs (repo, issue, stage, agent, model, started_at, finished_at, duration_ms, tool_calls, tokens_in, tokens_out, tokens_cached, cost_usd, usage_complete, exit_code, killed_reason) + VALUES ($repo, $issue, $stage, $agent, $model, $started_at, $finished_at, $duration_ms, $tool_calls, $tokens_in, $tokens_out, $tokens_cached, $cost_usd, $usage_complete, $exit_code, $killed_reason)`, ) .run({ $repo: input.repo, @@ -257,7 +274,9 @@ export class FactoryState { $tool_calls: input.tool_calls, $tokens_in: input.tokens_in, $tokens_out: input.tokens_out, + $tokens_cached: input.tokens_cached ?? 0, $cost_usd: input.cost_usd, + $usage_complete: input.usage_complete ?? 1, $exit_code: input.exit_code, $killed_reason: input.killed_reason, }); diff --git a/src/watch.ts b/src/watch.ts index 43de629..7d04d47 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -12,6 +12,7 @@ import type { FactoryConfig } from "./config"; import { writeRevision } from "./revision"; +import { costFor } from "./pricing"; import type { Executor, StageName, StageRunResult } from "./executor"; import { clearStageArtifacts, @@ -191,6 +192,15 @@ async function runStage( }); for (const e of result.events) deps.state.appendEvent(run.id, stage as Stage, e.kind, e.text ?? e.toolName ?? ""); const finishedAt = new Date(); + // The agent's own cost wins; otherwise price the tokens. No price means the + // cost is unknown, which is stored as 0 with usage_complete = 0, never as real. + const cached = result.tokensCached ?? 0; + const priced = + result.costReported === false + ? costFor(result.model, { tokensIn: result.tokensIn, tokensOut: result.tokensOut, tokensCached: cached }) + : result.costUsd; + const usageComplete = result.usageComplete !== false && priced !== undefined; + const costUsd = usageComplete ? (priced ?? 0) : 0; deps.state.recordStageRun({ repo: config.repo, issue: issueNumber, @@ -203,7 +213,9 @@ async function runStage( tool_calls: result.toolCalls, tokens_in: result.tokensIn, tokens_out: result.tokensOut, - cost_usd: result.costUsd, + tokens_cached: cached, + cost_usd: costUsd, + usage_complete: usageComplete ? 1 : 0, exit_code: result.exitCode, killed_reason: result.killedReason ?? null, }); @@ -211,7 +223,7 @@ async function runStage( tool_calls: run.tool_calls + result.toolCalls, tokens_in: run.tokens_in + result.tokensIn, tokens_out: run.tokens_out + result.tokensOut, - cost_usd: run.cost_usd + result.costUsd, + cost_usd: run.cost_usd + costUsd, }); return result; } diff --git a/tests/ported/machinist/usage-metrics.test.ts b/tests/ported/machinist/usage-metrics.test.ts new file mode 100644 index 0000000..4295632 --- /dev/null +++ b/tests/ported/machinist/usage-metrics.test.ts @@ -0,0 +1,31 @@ +// Ported from owainlewis/machinist@3943516 evals/test_agent.py:1113-1146 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: record_usage accumulates into a global METRICS dict; readUsage returns one event's usage and the test sums them; there is no cache_write field, so only reads are asserted. + +import { expect, test } from "bun:test"; +import { readUsage } from "../../../src/agents/usage"; +import { costFor } from "../../../src/pricing"; + +test("unknown usage is not zero", () => { + expect(readUsage(null, false)).toBeNull(); + expect(readUsage({}, false)).toBeNull(); + expect(readUsage(undefined, true)).toBeNull(); +}); + +test("provider cache accounting", () => { + const codex = readUsage({ input_tokens: 100, output_tokens: 20, cached_input_tokens: 80 }, false)!; + const claude = readUsage({ input_tokens: 10, output_tokens: 5, cache_read_input_tokens: 40, cache_creation_input_tokens: 15 }, true)!; + // Claude adds cache reads and writes to input; Codex's cached tokens are already inside input. + expect(codex.tokensIn + codex.tokensOut + claude.tokensIn + claude.tokensOut).toBe(190); + expect(codex.tokensIn + claude.tokensIn).toBe(165); + expect(codex.tokensCached + claude.tokensCached).toBe(120); +}); + +test("a cached count larger than the input is not trusted", () => { + expect(readUsage({ input_tokens: 5, output_tokens: 1, cached_input_tokens: 9 }, false)).toBeNull(); +}); + +test("an unpriced model has no cost, a priced one bills cache reads at the cache rate", () => { + expect(costFor("gpt-not-priced", { tokensIn: 1, tokensOut: 1 })).toBeUndefined(); + expect(costFor(null, { tokensIn: 1, tokensOut: 1 })).toBeUndefined(); + // haiku 4.5: 20 fresh in at $1, 80 cached at $0.10, 10 out at $5, per million. + expect(costFor("claude-haiku-4-5-20251001", { tokensIn: 100, tokensOut: 10, tokensCached: 80 })).toBeCloseTo((20 * 1 + 80 * 0.1 + 10 * 5) / 1e6, 12); +}); diff --git a/tests/pricing.test.ts b/tests/pricing.test.ts new file mode 100644 index 0000000..152c659 --- /dev/null +++ b/tests/pricing.test.ts @@ -0,0 +1,31 @@ +// Every model the docs or the example config name must either have a price +// row or be listed as unpriced with a reason, so "Not reported" is a choice. + +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { PRICES, UNPRICED } from "../src/pricing"; + +const root = join(import.meta.dir, ".."); +const MODEL = /"model":\s*"([^"]+)"|--model[ =]([A-Za-z0-9._-]+)/g; + +function modelsIn(file: string): string[] { + return [...readFileSync(join(root, file), "utf8").matchAll(MODEL)].map((m) => (m[1] ?? m[2])!); +} + +test("every model in the example config and README has a price row or an unpriced reason", () => { + const models = new Set([...modelsIn("README.md"), ...modelsIn("template/.factory/config.example.json")]); + expect(models.size).toBeGreaterThan(0); + for (const m of models) { + const family = m.replace(/-\d{8}$/, ""); + expect(family in PRICES || m in UNPRICED, `${m}: add a row to PRICES or an entry to UNPRICED`).toBe(true); + } +}); + +test("a model is never both priced and unpriced, and every row is dated and sourced", () => { + for (const m of Object.keys(UNPRICED)) expect(PRICES[m], m).toBeUndefined(); + for (const [m, p] of Object.entries(PRICES)) { + expect(p.source, m).toMatch(/^https:\/\//); + expect(p.asOf, m).toMatch(/^\d{4}-\d{2}-\d{2}$/); + } +}); diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index 200867b..3dfef24 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -177,6 +177,26 @@ describe("approval paths", () => { done(c); }); + test("4c. an unpriced or partial usage is stored as not reported, a priced one gets a cost", async () => { + const c = setup([LABEL.ready]); + c.state.setToggle("auto_approve_low_risk", true); + const usage = { tokensIn: 1000, tokensOut: 100, tokensCached: 400, costUsd: 0, costReported: false }; + c.push("triage", triage(), { ...usage, model: "gpt-not-priced" }); + c.push("plan", plan("low"), { ...usage, model: "claude-haiku-4-5" }); + c.push("build", build(), { ...usage, model: "claude-haiku-4-5", usageComplete: false }); + c.push("verify", verdict("pass"), { ...usage, costUsd: 0.25, costReported: true, model: "gpt-not-priced" }); + c.push("pr", pr()); + expect(await c.step()).toBe("shipped"); + const rows = Object.fromEntries(c.state.listStageRuns("acme/widgets", { issue: 1 }).map((r) => [r.stage, r])); + expect([rows.triage!.usage_complete, rows.triage!.cost_usd]).toEqual([0, 0]); + expect(rows.plan!.usage_complete).toBe(1); + expect(rows.plan!.tokens_cached).toBe(400); + expect(rows.plan!.cost_usd).toBeCloseTo((600 * 1 + 400 * 0.1 + 100 * 5) / 1e6, 12); + expect(rows.build!.usage_complete).toBe(0); + expect([rows.verify!.usage_complete, rows.verify!.cost_usd]).toEqual([1, 0.25]); + done(c); + }); + test("18. an untrusted /factory approve is ignored", async () => { const c = setup([LABEL.ready]); c.push("triage", triage({ risk: "medium" })); diff --git a/tests/state.test.ts b/tests/state.test.ts index 0945692..ea65a79 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -51,6 +51,19 @@ test("migrating twice, and on a database from v2.2 without stage_runs, keeps the state.close(); }); +test("a v2.5.0 stage_runs table gains tokens_cached and usage_complete once, keeping its rows", () => { + const path = join(dir, "v250.db"); + const old = new Database(path); + old.exec("CREATE TABLE stage_runs (id INTEGER PRIMARY KEY AUTOINCREMENT, repo TEXT NOT NULL, issue INTEGER NOT NULL, stage TEXT NOT NULL, agent TEXT NOT NULL, model TEXT, started_at TEXT NOT NULL, finished_at TEXT NOT NULL, duration_ms INTEGER NOT NULL, tool_calls INTEGER NOT NULL DEFAULT 0, tokens_in INTEGER NOT NULL DEFAULT 0, tokens_out INTEGER NOT NULL DEFAULT 0, cost_usd REAL NOT NULL DEFAULT 0, exit_code INTEGER NOT NULL, killed_reason TEXT)"); + old.exec("INSERT INTO stage_runs (repo, issue, stage, agent, started_at, finished_at, duration_ms, exit_code) VALUES ('acme/widgets', 7, 'plan', 'claude', 'a', 'b', 1, 0)"); + old.close(); + for (let i = 0; i < 2; i++) new FactoryState(path).close(); + const state = new FactoryState(path); + const [kept] = state.listStageRuns("acme/widgets"); + expect([kept!.tokens_cached, kept!.usage_complete]).toEqual([0, 1]); + state.close(); +}); + test("two processes opening a fresh database at once both get the full schema", async () => { const dir = mkdtempSync(join(tmpdir(), "factory-race-")); const path = join(dir, "factory.db"); From ec32d086a2fe4040501bfd5988a45896fcb36c02 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:42:18 +0300 Subject: [PATCH 3/9] Stage policy: Codex read-only for triage/plan/verify, artifact returned as final message (P5, P37) Co-Authored-By: Claude Sonnet 5 --- THIRD_PARTY_NOTICES.md | 1 + src/agents/executor.ts | 12 +++++- src/agents/presets/codex.ts | 22 +++++++--- src/agents/prompt.ts | 16 ++++++-- src/agents/reply.ts | 41 ++++++++++++++++++ src/agents/types.ts | 21 +++++++++- tests/agents.test.ts | 57 ++++++++++++++++++++++++-- tests/fixtures/agents/fake-codex.ts | 30 ++++++++++++++ tests/ported/assembler/outputs.test.ts | 26 ++++++++++++ 9 files changed, 211 insertions(+), 15 deletions(-) create mode 100644 src/agents/reply.ts create mode 100755 tests/fixtures/agents/fake-codex.ts create mode 100644 tests/ported/assembler/outputs.test.ts diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index 2deb3b4..0d7a80b 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -30,6 +30,7 @@ Source: https://github.com/owainlewis/assembler (MIT, Copyright (c) 2026 Owain L - `src/display.ts` from `src/display.ts` [tested by `tests/ported/assembler/display.test.ts`] - `src/logs.ts` from `src/runs.ts` [tested by `tests/ported/assembler/logs.test.ts`] - `src/config.ts` from `src/index.ts` [tested by `tests/ported/assembler/config.test.ts`] +- `src/agents/reply.ts` from `src/index.ts` [tested by `tests/ported/assembler/outputs.test.ts`] ## License text (both projects) diff --git a/src/agents/executor.ts b/src/agents/executor.ts index c519cdb..d9d3f99 100644 --- a/src/agents/executor.ts +++ b/src/agents/executor.ts @@ -11,7 +11,8 @@ import { aggregateStageEvents, type Executor, type StageEvent, type StageRunOpti import { sanitizeEnv } from "./env"; import { renderPrompt } from "./prompt"; import { PRESETS } from "./presets"; -import type { AgentConfig, AgentPreset, StageAgents } from "./types"; +import { writeReply } from "./reply"; +import { type AgentConfig, type AgentPreset, type StageAgents, stagePolicy } from "./types"; const DEFAULT_TIMEOUT_MINUTES = 15; const STDERR_KEEP_BYTES = 64 * 1024; @@ -65,12 +66,14 @@ export class CommandExecutor implements Executor { const artifactDir = join(opts.cwd, runDir(opts.issue)); mkdirSync(artifactDir, { recursive: true }); + // A read-only stage on a preset that cannot write files returns them instead. + const readOnly = !stagePolicy(opts.stage).write && agent.preset?.returnsArtifact === true && !agent.config.command; 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); + const prompt = await renderPrompt(opts, readOnly); if (agent.config.command) { const promptFile = join(scratch, "prompt.md"); writeFileSync(promptFile, prompt); @@ -185,6 +188,11 @@ export class CommandExecutor implements Executor { 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 }; + if (readOnly && exitCode === 0 && !killedReason) { + const problem = await writeReply(opts.cwd, opts.issue, opts.stage, base.finalMessage); + // No file is left behind, so the runner reports "no valid .json". + if (problem) base.events.push({ kind: "text", text: `read-only reply rejected: ${problem}` }); + } return killedReason ? { ...base, exitCode: exitCode || 1, killedReason } : base; } } diff --git a/src/agents/presets/codex.ts b/src/agents/presets/codex.ts index c88c14d..5e8bbe3 100644 --- a/src/agents/presets/codex.ts +++ b/src/agents/presets/codex.ts @@ -1,10 +1,10 @@ // 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. +// Codex CLI: `codex exec --json -s -` with the prompt on stdin. +// Build and pr run workspace-write; triage, plan and verify run read-only and +// return their artifacts as the final message, which the runner writes. import type { StageEvent } from "../../executor"; -import type { AgentPreset } from "../types"; +import { type AgentPreset, stagePolicy } from "../types"; import { isUsageResultCandidate, readUsage } from "../usage"; interface CodexLine { @@ -40,8 +40,18 @@ export function parseCodexLine(line: string): StageEvent[] { export const codexPreset: AgentPreset = { name: "codex", binary: "codex", - command: (_opts, agent, prompt) => ({ - argv: ["codex", "exec", "--json", "-s", "workspace-write", ...(agent.model ? ["-m", agent.model] : []), "-"], + returnsArtifact: true, + command: (opts, agent, prompt, ctx) => ({ + argv: [ + "codex", + "exec", + "--json", + "-s", + stagePolicy(opts.stage).write ? "workspace-write" : "read-only", + ...(ctx?.schemaFile ? ["--output-schema", ctx.schemaFile] : []), + ...(agent.model ? ["-m", agent.model] : []), + "-", + ], stdin: prompt, }), parseLine: parseCodexLine, diff --git a/src/agents/prompt.ts b/src/agents/prompt.ts index 62b8fee..6146514 100644 --- a/src/agents/prompt.ts +++ b/src/agents/prompt.ts @@ -11,7 +11,17 @@ 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 { +export function artifactContract(opts: Pick, readOnly = false): string { + if (readOnly) { + return [ + "## Artifact contract", + "", + "This stage is read-only: you cannot write files. Do not try. Your final message must be ONE JSON object and nothing else, with no code fence:", + `{"artifact": , "comment": "", "question": ""}`, + "", + "You have no GitHub access and cannot push or merge; the runner does that.", + ].join("\n"); + } return [ "## Artifact contract", "", @@ -24,7 +34,7 @@ export function artifactContract(opts: Pick) ].join("\n"); } -export async function renderPrompt(opts: StageRunOptions): Promise { +export async function renderPrompt(opts: StageRunOptions, readOnly = false): 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\``); }); @@ -33,7 +43,7 @@ export async function renderPrompt(opts: StageRunOptions): Promise { "", stripFrontmatter(skill).trim(), "", - artifactContract(opts), + artifactContract(opts, readOnly), "", ].join("\n"); } diff --git a/src/agents/reply.ts b/src/agents/reply.ts new file mode 100644 index 0000000..cf84113 --- /dev/null +++ b/src/agents/reply.ts @@ -0,0 +1,41 @@ +// Ported from owainlewis/assembler@7cac671 src/index.ts:228-255 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the reply is an envelope {artifact, comment, question} that the runner writes as the stage's files; validation is the stage validators in artifacts.ts, not zod; a code fence around the JSON is tolerated. +// A read-only agent cannot write its artifacts, so it returns them as its final +// message. This turns that message into the files the runner already reads. + +import { mkdir, writeFile } from "node:fs/promises"; +import { type ArtifactStage, COMMENT_FILENAMES, JSON_FILENAMES, MAX_STEP_JSON_BYTES, runDir } from "../artifacts"; + +export type Reply = { ok: true; artifact: Record; comment?: string; question?: string } | { ok: false; reason: string }; + +export function parseReply(text: string | undefined): Reply { + if (!text?.trim()) return { ok: false, reason: "the read-only agent returned no final message" }; + const fenced = /^```(?:json)?\s*\n([\s\S]*?)\n```\s*$/.exec(text.trim()); + let value: unknown; + try { + value = JSON.parse(fenced ? fenced[1]! : text.trim()); + } catch (e) { + return { ok: false, reason: `the final message is not JSON: ${e instanceof Error ? e.message : String(e)}` }; + } + if (typeof value !== "object" || value === null || Array.isArray(value)) return { ok: false, reason: "the final message is not a JSON object" }; + const o = value as Record; + const extra = Object.keys(o).find((k) => !["artifact", "comment", "question"].includes(k)); + if (extra) return { ok: false, reason: `the final message has unknown field "${extra}"` }; + if (typeof o.artifact !== "object" || o.artifact === null || Array.isArray(o.artifact)) return { ok: false, reason: 'the final message has no "artifact" object' }; + for (const k of ["comment", "question"]) if (o[k] !== undefined && typeof o[k] !== "string") return { ok: false, reason: `"${k}" must be a string` }; + const artifact = o.artifact as Record; + if (Buffer.byteLength(JSON.stringify(artifact)) > MAX_STEP_JSON_BYTES) return { ok: false, reason: `the artifact is over ${MAX_STEP_JSON_BYTES} bytes` }; + return { ok: true, artifact, ...(o.comment ? { comment: o.comment as string } : {}), ...(o.question ? { question: o.question as string } : {}) }; +} + +// Writes the envelope's parts where the runner reads them. Returns the reason +// when the reply is unusable, so the caller can fail the stage with it. +export async function writeReply(cwd: string, issue: number, stage: ArtifactStage, text: string | undefined): Promise { + const reply = parseReply(text); + if (!reply.ok) return reply.reason; + const dir = `${cwd}/${runDir(issue)}`; + await mkdir(dir, { recursive: true }); + await writeFile(`${dir}/${JSON_FILENAMES[stage]}`, `${JSON.stringify(reply.artifact)}\n`); + if (reply.comment) await writeFile(`${dir}/${COMMENT_FILENAMES[stage]}`, reply.comment); + if (reply.question) await writeFile(`${dir}/question-comment.md`, reply.question); + return undefined; +} diff --git a/src/agents/types.ts b/src/agents/types.ts index 0101aba..83753cd 100644 --- a/src/agents/types.ts +++ b/src/agents/types.ts @@ -15,6 +15,22 @@ export interface AgentConfig { // stages.default is the agent for every stage; a stage name overrides it. export type StageAgents = Partial>; +// What a stage may do to the worktree. Triage, plan and verify only read; +// build writes code, and pr writes the PR body and pr.json. +export interface StagePolicy { + readonly write: boolean; +} + +export function stagePolicy(stage: StageName): StagePolicy { + return { write: stage === "build" || stage === "pr" }; +} + +// Extra things the executor hands a preset for one stage. +export interface StageContext { + // A JSON Schema file for the agent's final message (only when opted in). + readonly schemaFile?: string; +} + export interface StageInvocation { readonly argv: readonly string[]; readonly stdin?: string; @@ -25,7 +41,10 @@ export interface AgentPreset { // 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; + command(opts: StageRunOptions, agent: AgentConfig, prompt: string, ctx?: StageContext): StageInvocation; + // True when a read-only stage cannot write its files: the agent returns them + // as its final message and the runner writes them (see reply.ts). + readonly returnsArtifact?: boolean; parseLine(line: string): StageEvent[]; // True for a line (or its first bytes) that would have been the terminal usage event. isUsageCandidate(line: string): boolean; diff --git a/tests/agents.test.ts b/tests/agents.test.ts index f888e73..a7d01b8 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -4,7 +4,7 @@ // 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, writeFileSync } from "node:fs"; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { CommandExecutor, renderCommand, resolveAgent } from "../src/agents/executor"; @@ -50,10 +50,12 @@ describe("claude preset", () => { }); 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"); + test("prompt on stdin, JSON events, sandbox by stage policy", () => { + const inv = PRESETS.codex!.command({ stage: "build", 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"); + for (const stage of ["triage", "plan", "verify"] as const) expect(PRESETS.codex!.command({ stage, issue: 1, cwd: "/w", maxBudgetUsd: 2 }, { preset: "codex" }, "").argv).toContain("read-only"); + expect(PRESETS.codex!.command({ stage: "pr", issue: 1, cwd: "/w", maxBudgetUsd: 2 }, { preset: "codex" }, "").argv).toContain("workspace-write"); }); }); @@ -159,6 +161,55 @@ describe("an agent with no preset", () => { }); }); +describe("codex with read-only stages", () => { + 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 }); + } + } + const bin = mkdtempSync(join(scratch, "bin-")); + symlinkSync(join(import.meta.dir, "fixtures/agents/fake-codex.ts"), join(bin, "codex")); + + async function run(n: number, bad?: string) { + const log = mkdtempSync(join(scratch, "log-")); + const saved = process.env.PATH; + process.env.PATH = `${bin}:${saved}`; + process.env.FAKE_AGENT_LOG = log; + if (bad !== undefined) process.env.FAKE_BAD_REPLY = bad; + const issue = baseIssue(n, [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: { codex: { preset: "codex", model: "gpt-5.6-terra" } }, stages: { default: "codex" } }); + const deps = { github, git: new SkillGit(), state, executor: new CommandExecutor(config.agents, config.stages), gateRunner: new FakeGateRunner(), cloneDir: mkdtempSync(join(scratch, "clone-")), workspacesDir: mkdtempSync(join(scratch, "ws-")) }; + try { + return { result: await processReadyIssue(issue, deps, config), github, state, log }; + } finally { + process.env.PATH = saved; + delete process.env.FAKE_AGENT_LOG; + delete process.env.FAKE_BAD_REPLY; + } + } + + test("triage, plan and verify run read-only and the runner writes their files; build writes its own", async () => { + const { result, github, state, log } = await run(5); + expect(result).toBe("shipped"); + expect(readFileSync(join(log, "sandboxes"), "utf8").trim().split("\n").map((l) => l.trim())).toEqual(["triage=read-only", "plan=read-only", "build=workspace-write", "verify=read-only", "pr=workspace-write"]); + expect(github.createdPrs[0]!.body).toContain("Closes #5"); + // Cached tokens are stored, and gpt-5.6-terra has no price, so the cost is "not reported". + const [row] = state.listStageRuns("acme/widgets"); + expect([row!.tokens_cached, row!.usage_complete]).toEqual([40, 0]); + state.close(); + }); + + test("a read-only reply that is not the envelope fails the stage instead of shipping", async () => { + const { result, state } = await run(6, "I looked at the code and it seems fine."); + expect(result).toBe("failed"); + state.close(); + }); +}); + 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-")); diff --git a/tests/fixtures/agents/fake-codex.ts b/tests/fixtures/agents/fake-codex.ts new file mode 100755 index 0000000..72f62b7 --- /dev/null +++ b/tests/fixtures/agents/fake-codex.ts @@ -0,0 +1,30 @@ +#!/usr/bin/env bun +// A stand-in for `codex exec --json`: it obeys its own -s flag. Read-only, it +// answers with the reply envelope; workspace-write, it writes the files. +import { appendFileSync, writeFileSync } from "node:fs"; + +const args = process.argv.slice(2); +const sandbox = args[args.indexOf("-s") + 1]; +await Bun.stdin.text(); +const stage = process.env.FACTORY_STAGE!; +const dir = process.env.FACTORY_ARTIFACT_DIR!; +if (process.env.FAKE_AGENT_LOG) appendFileSync(`${process.env.FAKE_AGENT_LOG}/sandboxes`, `${stage}=${sandbox} ${args.includes("--output-schema") ? "schema" : ""}\n`); + +const artifacts: Record = { + triage: [{ disposition: "proceed", type: "bug", risk: "low", done_when: "tests pass", files_expected: ["src/a.ts"], gate_level: "make check", confidence: 0.9 }, "\nlooks good\n"], + plan: [{ risk: "low", revision: 1, files: ["src/a.ts"], autoApproveEligible: true }, "\nplan body\n"], + build: [{ status: "green", gate_line: "make check: 10 pass", rounds: 1 }, "\nbuilding\n"], + verify: [{ result: "pass", rounds: 1, findings: [] }, "\npass\n"], +}; +const emit = (o: object) => console.log(JSON.stringify(o)); +const [json, comment] = artifacts[stage] ?? [{}, ""]; +if (sandbox === "read-only") { + const reply = process.env.FAKE_BAD_REPLY ?? JSON.stringify({ artifact: json, comment }); + emit({ type: "item.completed", item: { type: "agent_message", text: reply } }); +} else if (stage === "pr") { + writeFileSync(`${dir}/pr-body.md`, `## Summary\nDid the thing.\nCloses #${process.env.FACTORY_ISSUE}\n`); +} else { + writeFileSync(`${dir}/${stage === "build" ? "build" : stage}.json`, JSON.stringify(json)); + writeFileSync(`${dir}/${stage === "build" ? "status-comment.md" : `${stage}-comment.md`}`, comment); +} +emit({ type: "turn.completed", usage: { input_tokens: 100, cached_input_tokens: 40, output_tokens: 10 } }); diff --git a/tests/ported/assembler/outputs.test.ts b/tests/ported/assembler/outputs.test.ts new file mode 100644 index 0000000..58608b2 --- /dev/null +++ b/tests/ported/assembler/outputs.test.ts @@ -0,0 +1,26 @@ +// Ported from owainlewis/assembler@7cac671 test/outputs.test.ts:72-101 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the cases are the structured-reply ones (wrong shape rejected, typed data returned, chatter around the JSON is not accepted); the Claude envelope and --output-last-message cases have no equivalent because the runner reads the final agent message. + +import { expect, test } from "bun:test"; +import { parseReply } from "../../../src/agents/reply"; + +test("structured reply of the wrong shape is rejected", () => { + const bad = parseReply(JSON.stringify({ approved: "yes" })); + expect(bad.ok).toBe(false); + expect(parseReply(JSON.stringify({ artifact: "no" })).ok).toBe(false); + expect(parseReply(JSON.stringify({ artifact: {}, extra: 1 })).ok).toBe(false); +}); + +test("a structured reply validates and returns typed data", () => { + const r = parseReply(JSON.stringify({ artifact: { approved: true }, comment: "hi" })); + expect(r).toEqual({ ok: true, artifact: { approved: true }, comment: "hi" }); +}); + +test("chatter around the JSON is not accepted, a fence is", () => { + expect(parseReply('Here you go: {"artifact":{}}').ok).toBe(false); + expect(parseReply("").ok).toBe(false); + expect(parseReply('```json\n{"artifact":{"a":1}}\n```').ok).toBe(true); +}); + +test("an artifact over 16 KiB is rejected", () => { + expect(parseReply(JSON.stringify({ artifact: { x: "a".repeat(17000) } })).ok).toBe(false); +}); From 42d734a92af64722e3a74edd5f8ea61d5427eda3 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:44:33 +0300 Subject: [PATCH 4/9] Event byte budget (P29), artifact schemas and outputSchema (P37) Co-Authored-By: Claude Sonnet 5 --- THIRD_PARTY_NOTICES.md | 1 + src/agents/executor.ts | 8 +++- src/agents/prompt.ts | 5 ++- src/agents/types.ts | 2 + src/artifacts.ts | 5 ++- src/config.ts | 2 +- src/event-budget.ts | 30 ++++++++++++++ src/schemas.ts | 59 +++++++++++++++++++++++++++ src/state.ts | 17 ++++++++ src/watch.ts | 3 +- tests/agents.test.ts | 11 ++++- tests/ported/machinist/events.test.ts | 37 +++++++++++++++++ tests/schemas.test.ts | 38 +++++++++++++++++ 13 files changed, 210 insertions(+), 8 deletions(-) create mode 100644 src/event-budget.ts create mode 100644 src/schemas.ts create mode 100644 tests/ported/machinist/events.test.ts create mode 100644 tests/schemas.test.ts diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index 0d7a80b..bd5a3cf 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -18,6 +18,7 @@ Source: https://github.com/owainlewis/machinist (MIT, Copyright (c) 2026 Owain L - `src/revision.ts` from `internal/runner/revision.go` (shape from `internal/protocol/revision.go`) [tested by `tests/ported/machinist/revision.test.ts`] - `src/agents/executor.ts` from `internal/runner/runner.go` (process group kill from `process_unix.go`) [tested by `tests/ported/machinist/runner.test.ts`] - `src/agents/env.ts` from `internal/runner/runner.go` [tested by `tests/ported/machinist/runner.test.ts`] +- `src/event-budget.ts` from `internal/runner/events.go` [tested by `tests/ported/machinist/events.test.ts`] - `src/agents/final-message.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/final-message.test.ts`] - `src/agents/presets/codex.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] - `src/agents/usage.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] diff --git a/src/agents/executor.ts b/src/agents/executor.ts index d9d3f99..f298332 100644 --- a/src/agents/executor.ts +++ b/src/agents/executor.ts @@ -11,6 +11,7 @@ import { aggregateStageEvents, type Executor, type StageEvent, type StageRunOpti import { sanitizeEnv } from "./env"; import { renderPrompt } from "./prompt"; import { PRESETS } from "./presets"; +import { replySchema } from "../schemas"; import { writeReply } from "./reply"; import { type AgentConfig, type AgentPreset, type StageAgents, stagePolicy } from "./types"; @@ -81,7 +82,12 @@ export class CommandExecutor implements Executor { argv = rendered.argv; stdin = rendered.usesStdin ? prompt : undefined; } else if (agent.preset) { - ({ argv, stdin } = agent.preset.command(opts, agent.config, prompt)); + let schemaFile: string | undefined; + if (readOnly && agent.config.outputSchema) { + schemaFile = join(scratch, "reply.schema.json"); + writeFileSync(schemaFile, JSON.stringify(replySchema(opts.stage))); + } + ({ argv, stdin } = agent.preset.command(opts, agent.config, prompt, { schemaFile })); } else { throw new Error(`agent "${agent.name}" has neither a preset nor a command`); } diff --git a/src/agents/prompt.ts b/src/agents/prompt.ts index 6146514..754830e 100644 --- a/src/agents/prompt.ts +++ b/src/agents/prompt.ts @@ -5,6 +5,7 @@ import { readFile } from "node:fs/promises"; import { COMMENT_FILENAMES, JSON_FILENAMES, runDir } from "../artifacts"; +import { stageSchema } from "../schemas"; import type { StageRunOptions } from "../executor"; export function stripFrontmatter(text: string): string { @@ -19,6 +20,8 @@ export function artifactContract(opts: Pick, "This stage is read-only: you cannot write files. Do not try. Your final message must be ONE JSON object and nothing else, with no code fence:", `{"artifact": , "comment": "", "question": ""}`, "", + `The artifact must match this JSON Schema: ${JSON.stringify(stageSchema(opts.stage))}`, + "", "You have no GitHub access and cannot push or merge; the runner does that.", ].join("\n"); } @@ -26,7 +29,7 @@ export function artifactContract(opts: Pick, "## 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.`, + `- ${JSON_FILENAMES[opts.stage]}: the structured result, exactly as the instructions above describe. Schema: ${JSON.stringify(stageSchema(opts.stage))}`, `- ${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.", "", diff --git a/src/agents/types.ts b/src/agents/types.ts index 83753cd..3a44e57 100644 --- a/src/agents/types.ts +++ b/src/agents/types.ts @@ -10,6 +10,8 @@ export interface AgentConfig { // With no placeholder the prompt goes to stdin. readonly command?: readonly string[]; readonly model?: string; + // Pass the reply schema to the CLI (Codex --output-schema). Off until a live run confirms the CLI accepts it. + readonly outputSchema?: boolean; } // stages.default is the agent for every stage; a stage name overrides it. diff --git a/src/artifacts.ts b/src/artifacts.ts index ce34096..3f6b068 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -76,7 +76,8 @@ export interface VerdictArtifact { // 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", "outcome", "summary"]); +export const VERDICT_KEY_LIST = ["result", "rounds", "findings", "criteria", "outcome", "summary"] as const; +const VERDICT_KEYS = new Set(VERDICT_KEY_LIST); 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. @@ -94,7 +95,7 @@ function stepEnvelopeProblem(o: Record): string | undefined { // Every stage artifact rejects a field it does not define, so a typo cannot // pass as a silent no-op. The verdict has its own, deeper validator above. -const STEP_KEYS: Record, readonly string[]> = { +export const STEP_KEYS: Record, readonly string[]> = { triage: ["disposition", "type", "risk", "done_when", "files_expected", "gate_level", "confidence", "outcome", "summary"], plan: ["status", "risk", "revision", "files", "autoApproveEligible", "commentId", "outcome", "summary"], build: ["status", "gate_line", "rounds", "outcome", "summary"], diff --git a/src/config.ts b/src/config.ts index 77063c3..1244662 100644 --- a/src/config.ts +++ b/src/config.ts @@ -172,7 +172,7 @@ function agentProblems(agents: unknown, stages: unknown): string[] { continue; } const a = raw as Record; - checkKeys(a, { preset: "string", command: "strings", model: "string" }, where, problems); + checkKeys(a, { preset: "string", command: "strings", model: "string", outputSchema: "boolean" }, 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)) { diff --git a/src/event-budget.ts b/src/event-budget.ts new file mode 100644 index 0000000..a97d81e --- /dev/null +++ b/src/event-budget.ts @@ -0,0 +1,30 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/events.go:14-90 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the bound applies to a run's rows in the events table rather than to a JSONL file; the sequence and base64 encoding are not needed; the seed is the bytes already stored, so a restart keeps the bound. + +export const MAX_EVENT_LOG_BYTES = 32 << 20; +// Room kept for the one truncation event, so it always fits. +export const TRUNCATION_EVENT_RESERVE = 1 << 10; +export const TRUNCATION_KIND = "process.output_truncated"; + +export class EventBudget { + private truncated = false; + + constructor( + private used = 0, + readonly limit = MAX_EVENT_LOG_BYTES, + ) {} + + // "write" the event, "truncate" to write the marker instead (once), or "drop". + admit(bytes: number): "write" | "truncate" | "drop" { + if (this.truncated) return "drop"; + if (this.used + bytes + TRUNCATION_EVENT_RESERVE > this.limit) { + this.truncated = true; + return "truncate"; + } + this.used += bytes; + return "write"; + } + + message(): string { + return `recording stopped after ${this.limit} event bytes; live output continues`; + } +} diff --git a/src/schemas.ts b/src/schemas.ts new file mode 100644 index 0000000..ad67c9a --- /dev/null +++ b/src/schemas.ts @@ -0,0 +1,59 @@ +// JSON Schema (draft-07) for each stage artifact. The property lists come from +// STEP_KEYS and VERDICT_KEYS in artifacts.ts, so the validators stay the one +// definition of which fields exist; only the field types are written here. + +import { type ArtifactStage, STEP_KEYS, VERDICT_KEY_LIST } from "./artifacts"; + +type Prop = Record; +const str: Prop = { type: "string" }; +const int: Prop = { type: "integer" }; +const strings: Prop = { type: "array", items: str }; +const outcome: Prop = { enum: ["complete", "blocked", "failed"] }; +const risk: Prop = { enum: ["low", "medium", "high"] }; + +const TYPES: Record> = { + triage: { + disposition: { enum: ["proceed", "needs-info", "refused", "duplicate"] }, + type: { enum: ["bug", "feature", "docs", "security", "dependency"] }, + risk, + done_when: str, + files_expected: strings, + gate_level: str, + confidence: { type: "number", minimum: 0, maximum: 1 }, + }, + plan: { status: { enum: ["needs-info"] }, risk, revision: int, files: strings, autoApproveEligible: { type: "boolean" }, commentId: int }, + build: { status: { enum: ["green", "red", "needs-info"] }, gate_line: str, rounds: int }, + verify: { + result: { enum: ["pass", "reject", "uncertain"] }, + rounds: int, + findings: { type: "array", items: { anyOf: [str, { type: "object" }] } }, + criteria: { type: "array", items: { type: "object" } }, + }, + pr: {}, +}; + +const REQUIRED: Record = { + triage: ["disposition", "type", "risk", "done_when", "files_expected", "gate_level", "confidence"], + plan: ["risk", "revision", "files", "autoApproveEligible"], + build: ["status", "gate_line", "rounds"], + verify: ["result", "rounds", "findings"], + pr: [], +}; + +export function stageSchema(stage: ArtifactStage): Record { + const keys = stage === "verify" ? VERDICT_KEY_LIST : STEP_KEYS[stage]; + const properties: Record = {}; + for (const k of keys) properties[k] = k === "outcome" ? outcome : k === "summary" ? str : (TYPES[stage][k] ?? {}); + return { $schema: "http://json-schema.org/draft-07/schema#", type: "object", properties, required: REQUIRED[stage], additionalProperties: false }; +} + +// The read-only reply envelope around a stage's schema. +export function replySchema(stage: ArtifactStage): Record { + return { + $schema: "http://json-schema.org/draft-07/schema#", + type: "object", + properties: { artifact: stageSchema(stage), comment: str, question: str }, + required: ["artifact"], + additionalProperties: false, + }; +} diff --git a/src/state.ts b/src/state.ts index 3ea090c..89d410b 100644 --- a/src/state.ts +++ b/src/state.ts @@ -4,6 +4,7 @@ import { Database, type SQLQueryBindings } from "bun:sqlite"; import { mkdirSync } from "node:fs"; +import { EventBudget, MAX_EVENT_LOG_BYTES, TRUNCATION_KIND } from "./event-budget"; import { dirname } from "node:path"; import { defaultStatePath } from "./paths"; @@ -241,7 +242,23 @@ export class FactoryState { return this.db.query("SELECT * FROM runs ORDER BY updated_at DESC").all() as Run[]; } + private readonly budgets = new Map(); + // Per-run cap on stored event bytes (machinist events.go); tests lower it. + eventByteLimit = MAX_EVENT_LOG_BYTES; + appendEvent(runId: number, stage: Stage, kind: string, text: string): void { + let budget = this.budgets.get(runId); + if (!budget) { + const row = this.db.query("SELECT COALESCE(SUM(LENGTH(CAST(text AS BLOB)) + LENGTH(kind)), 0) AS n FROM events WHERE run_id = $id").get({ $id: runId }) as { n: number }; + budget = new EventBudget(row.n, this.eventByteLimit); + this.budgets.set(runId, budget); + } + const verdict = budget.admit(Buffer.byteLength(text) + kind.length); + if (verdict === "drop") return; + if (verdict === "truncate") { + kind = TRUNCATION_KIND; + text = budget.message(); + } this.db .query("INSERT INTO events (run_id, ts, stage, kind, text) VALUES ($run_id, $ts, $stage, $kind, $text)") .run({ $run_id: runId, $ts: new Date().toISOString(), $stage: stage, $kind: kind, $text: text }); diff --git a/src/watch.ts b/src/watch.ts index 7d04d47..1494ff5 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -12,6 +12,7 @@ import type { FactoryConfig } from "./config"; import { writeRevision } from "./revision"; +import { TRUNCATION_KIND } from "./event-budget"; import { costFor } from "./pricing"; import type { Executor, StageName, StageRunResult } from "./executor"; import { @@ -190,7 +191,7 @@ async function runStage( maxToolCalls: config.maxToolCalls, agentCommands: config.agentCommands, }); - for (const e of result.events) deps.state.appendEvent(run.id, stage as Stage, e.kind, e.text ?? e.toolName ?? ""); + for (const e of result.events) deps.state.appendEvent(run.id, stage as Stage, e.kind === "truncated" ? TRUNCATION_KIND : e.kind, e.text ?? e.toolName ?? ""); const finishedAt = new Date(); // The agent's own cost wins; otherwise price the tokens. No price means the // cost is unknown, which is stored as 0 with usage_complete = 0, never as real. diff --git a/tests/agents.test.ts b/tests/agents.test.ts index a7d01b8..f244244 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -171,7 +171,7 @@ describe("codex with read-only stages", () => { const bin = mkdtempSync(join(scratch, "bin-")); symlinkSync(join(import.meta.dir, "fixtures/agents/fake-codex.ts"), join(bin, "codex")); - async function run(n: number, bad?: string) { + async function run(n: number, bad?: string, outputSchema?: boolean) { const log = mkdtempSync(join(scratch, "log-")); const saved = process.env.PATH; process.env.PATH = `${bin}:${saved}`; @@ -181,7 +181,7 @@ describe("codex with read-only stages", () => { const github = new FakeGitHub([issue]); const state = new FactoryState(":memory:"); state.setToggle("auto_approve_low_risk", true); - const config = mergeConfig({ repo: "acme/widgets", agents: { codex: { preset: "codex", model: "gpt-5.6-terra" } }, stages: { default: "codex" } }); + const config = mergeConfig({ repo: "acme/widgets", agents: { codex: { preset: "codex", model: "gpt-5.6-terra", ...(outputSchema ? { outputSchema } : {}) } }, stages: { default: "codex" } }); const deps = { github, git: new SkillGit(), state, executor: new CommandExecutor(config.agents, config.stages), gateRunner: new FakeGateRunner(), cloneDir: mkdtempSync(join(scratch, "clone-")), workspacesDir: mkdtempSync(join(scratch, "ws-")) }; try { return { result: await processReadyIssue(issue, deps, config), github, state, log }; @@ -203,6 +203,13 @@ describe("codex with read-only stages", () => { state.close(); }); + test("outputSchema hands the read-only stages a schema file, and only those", async () => { + const { result, log, state } = await run(7, undefined, true); + expect(result).toBe("shipped"); + expect(readFileSync(join(log, "sandboxes"), "utf8").trim().split("\n").map((l) => l.trim())).toEqual(["triage=read-only schema", "plan=read-only schema", "build=workspace-write", "verify=read-only schema", "pr=workspace-write"]); + state.close(); + }); + test("a read-only reply that is not the envelope fails the stage instead of shipping", async () => { const { result, state } = await run(6, "I looked at the code and it seems fine."); expect(result).toBe("failed"); diff --git a/tests/ported/machinist/events.test.ts b/tests/ported/machinist/events.test.ts new file mode 100644 index 0000000..8db788b --- /dev/null +++ b/tests/ported/machinist/events.test.ts @@ -0,0 +1,37 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/runner_test.go:340-395 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the log is the events table, and the bound counts stored text and kind bytes; TestEventLogTruncatesRecordingWithoutFailing's output limit is the executor's recorded-output cap and is asserted in agents.test.ts. + +import { expect, test } from "bun:test"; +import { TRUNCATION_KIND } from "../../../src/event-budget"; +import { FactoryState } from "../../../src/state"; + +function fill(limit: number, chunks: number) { + const state = new FactoryState(":memory:"); + state.eventByteLimit = limit; + const run = state.upsertRun({ issue: 1, repo: "a/b", title: "t", stage: "build", status: "running" }); + for (let i = 0; i < chunks; i++) state.appendEvent(run.id, "build", "text", "x"); + return { state, events: state.listEvents(run.id, { limit: 5000 }) }; +} + +test("the event log is bounded across tiny chunks and ends with one truncation marker", () => { + const { state, events } = fill(4 << 10, 1000); + const stored = events.reduce((n, e) => n + e.text.length + e.kind.length, 0); + expect(stored).toBeLessThanOrEqual(4 << 10); + expect(events.filter((e) => e.kind === TRUNCATION_KIND)).toHaveLength(1); + expect(events.at(-1)!.kind).toBe(TRUNCATION_KIND); + state.close(); +}); + +test("under the limit nothing is truncated", () => { + const { state, events } = fill(1 << 20, 10); + expect(events).toHaveLength(10); + state.close(); +}); + +test("a full run stays full", () => { + const { state, events } = fill(2 << 10, 500); + const before = events.length; + const run = state.getRun("a/b", 1)!; + state.appendEvent(run.id, "build", "text", "more"); + expect(state.listEvents(run.id, { limit: 5000 })).toHaveLength(before); + state.close(); +}); diff --git a/tests/schemas.test.ts b/tests/schemas.test.ts new file mode 100644 index 0000000..903dd74 --- /dev/null +++ b/tests/schemas.test.ts @@ -0,0 +1,38 @@ +// The schemas and the validators must agree: same fields, and the fixture +// artifacts the validators accept satisfy the schema's required list. + +import { expect, test } from "bun:test"; +import { STEP_KEYS, validateStepJson, validateVerdict, type ArtifactStage } from "../src/artifacts"; +import { replySchema, stageSchema } from "../src/schemas"; + +const STAGES: ArtifactStage[] = ["triage", "plan", "build", "verify", "pr"]; +const GOOD: Record = { + triage: { disposition: "proceed", type: "bug", risk: "low", done_when: "x", files_expected: [], gate_level: "g", confidence: 0.9 }, + plan: { risk: "low", revision: 1, files: [], autoApproveEligible: true }, + build: { status: "green", gate_line: "l", rounds: 1 }, + verify: { result: "pass", rounds: 1, findings: [] }, + pr: {}, +}; + +test("every stage has a schema whose properties are exactly the validator's fields", () => { + for (const stage of STAGES) { + const s = stageSchema(stage) as { properties: object; required: string[]; additionalProperties: boolean }; + const keys = Object.keys(s.properties); + if (stage !== "verify") expect(keys.sort(), stage).toEqual([...STEP_KEYS[stage as Exclude]].sort()); + expect(s.additionalProperties).toBe(false); + for (const r of s.required) expect(keys, `${stage}: required ${r} is not a property`).toContain(r); + } +}); + +test("a good artifact for each stage satisfies the schema's required list and the validator", () => { + for (const stage of STAGES) { + const good = GOOD[stage] as Record; + for (const r of (stageSchema(stage) as { required: string[] }).required) expect(good, `${stage}.${r}`).toHaveProperty(r); + expect(stage === "verify" ? validateVerdict(good).ok : validateStepJson(stage, good).ok, stage).toBe(true); + } +}); + +test("the reply schema wraps the stage schema", () => { + const r = replySchema("plan") as { properties: { artifact: unknown } }; + expect(r.properties.artifact).toEqual(stageSchema("plan")); +}); From 3660075824822fa035f2f5061fd15856e132b88c Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:47:28 +0300 Subject: [PATCH 5/9] Detect codex and claude behind env, mise and direnv wrappers and inject the JSON flag Co-Authored-By: Claude Sonnet 5 --- THIRD_PARTY_NOTICES.md | 1 + src/agents/executor.ts | 4 + src/agents/structured.ts | 233 ++++++++++++++++++++++ tests/agents.test.ts | 12 ++ tests/ported/machinist/structured.test.ts | 93 +++++++++ 5 files changed, 343 insertions(+) create mode 100644 src/agents/structured.ts create mode 100644 tests/ported/machinist/structured.test.ts diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index bd5a3cf..b0106c9 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -19,6 +19,7 @@ Source: https://github.com/owainlewis/machinist (MIT, Copyright (c) 2026 Owain L - `src/agents/executor.ts` from `internal/runner/runner.go` (process group kill from `process_unix.go`) [tested by `tests/ported/machinist/runner.test.ts`] - `src/agents/env.ts` from `internal/runner/runner.go` [tested by `tests/ported/machinist/runner.test.ts`] - `src/event-budget.ts` from `internal/runner/events.go` [tested by `tests/ported/machinist/events.test.ts`] +- `src/agents/structured.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/structured.test.ts`] - `src/agents/final-message.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/final-message.test.ts`] - `src/agents/presets/codex.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] - `src/agents/usage.ts` from `internal/runner/codex_usage.go` [tested by `tests/ported/machinist/usage.test.ts`] diff --git a/src/agents/executor.ts b/src/agents/executor.ts index f298332..d93f854 100644 --- a/src/agents/executor.ts +++ b/src/agents/executor.ts @@ -11,6 +11,7 @@ import { aggregateStageEvents, type Executor, type StageEvent, type StageRunOpti import { sanitizeEnv } from "./env"; import { renderPrompt } from "./prompt"; import { PRESETS } from "./presets"; +import { structuredCommand } from "./structured"; import { replySchema } from "../schemas"; import { writeReply } from "./reply"; import { type AgentConfig, type AgentPreset, type StageAgents, stagePolicy } from "./types"; @@ -32,6 +33,9 @@ export function resolveAgent(agents: Record, stages: StageA 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}"`); + // A bare `codex exec` or `claude -p` command still gets its JSON flag and parser. + const found = !preset && config.command ? structuredCommand(name, config.command) : undefined; + if (found) return { name, config: { ...config, command: found.command }, preset: PRESETS[found.preset] }; return { name, config, preset }; } diff --git a/src/agents/structured.ts b/src/agents/structured.ts new file mode 100644 index 0000000..b890dc1 --- /dev/null +++ b/src/agents/structured.ts @@ -0,0 +1,233 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage.go:41-435 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: returns the preset name with the command instead of a collector; the executor name is the agent's config name; nice is handled for claude only, as upstream does; Windows ".exe" and env's -S handling follow upstream. +// A config `command` that is really `codex exec` or `claude -p`, even behind +// env, mise or direnv, gets the JSON flag its preset parses. Anything not +// recognised is returned unchanged, so an odd command never gets a wrong flag. + +export interface Structured { + readonly preset: "codex" | "claude"; + readonly command: string[]; +} + +const base = (s: string): string => (s.split("/").pop() ?? s).toLowerCase().replace(/\.exe$/, ""); +const named = (executor: string, tool: string): boolean => executor.toLowerCase().split(/[-_.]+/).includes(tool); +const startsWithOpt = (arg: string, opts: readonly string[]): boolean => opts.some((o) => arg === o || arg.startsWith(`${o}=`)); + +// [recognized, takesNextValue] +type Opt = readonly [boolean, boolean]; +const NO: Opt = [false, false]; + +function envShort(arg: string): Opt { + if (arg.length < 2 || arg[0] !== "-" || arg[1] === "-") return NO; + for (let i = 1; i < arg.length; i++) { + const c = arg[i]!; + if ("iv0".includes(c)) continue; + if ("uCPSa".includes(c)) return [true, i + 1 === arg.length]; + return NO; + } + return [true, false]; +} + +function envSplitString(arg: string): boolean { + if (arg.length < 2 || arg[0] !== "-" || arg[1] === "-") return false; + for (const c of arg.slice(1)) { + if ("iv0".includes(c)) continue; + return c === "S"; + } + return false; +} + +function envProgramIndex(cmd: readonly string[]): number { + for (let i = 1; i < cmd.length; i++) { + const a = cmd[i]!; + if (a === "--") return i + 1 < cmd.length ? i + 1 : -1; + if (a.includes("=") && !a.startsWith("-")) continue; + if (a === "-" || ["--ignore-environment", "--null", "--debug", "--block-signal", "--default-signal", "--ignore-signal", "--list-signal-handling"].includes(a)) continue; + if (envSplitString(a)) return -1; + const [ok, next] = envShort(a); + if (ok) { + if (next) i++; + continue; + } + if (a === "--split-string" || a.startsWith("--split-string=")) return -1; + if (["--unset", "--chdir", "--argv0"].includes(a)) { + i++; + continue; + } + if (["--unset=", "--chdir=", "--argv0=", "--block-signal=", "--default-signal=", "--ignore-signal="].some((p) => a.startsWith(p))) continue; + if (a.startsWith("-")) return -1; + return i; + } + return -1; +} + +function miseGlobal(a: string): Opt { + for (const o of ["--cd", "--env", "--jobs", "--output"]) { + if (a === o) return [true, true]; + if (a.startsWith(`${o}=`)) return [true, false]; + } + if (["--quiet", "--verbose", "--yes", "--raw", "--locked", "--silent", "--no-config", "--no-env", "--no-hooks", "--help"].includes(a)) return [true, false]; + if (a.length >= 2 && a[0] === "-" && a[1] !== "-") { + for (let i = 1; i < a.length; i++) { + const c = a[i]!; + if ("qvyh".includes(c)) continue; + if ("CEj".includes(c)) return [true, i + 1 === a.length]; + return NO; + } + return [true, false]; + } + return NO; +} + +function miseProgramIndex(cmd: readonly string[]): number { + for (let i = 1; i < cmd.length; i++) { + const [ok, next] = miseGlobal(cmd[i]!); + if (ok) { + if (next) i++; + continue; + } + if (cmd[i] !== "exec" && cmd[i] !== "x") return -1; + for (let j = i + 1; j < cmd.length; j++) if (cmd[j] === "--" && j + 1 < cmd.length) return j + 1; + return -1; + } + return -1; +} + +function direnvProgramIndex(cmd: readonly string[]): number { + return cmd.length < 4 || cmd[1] !== "exec" || cmd[2] === "" || cmd[2]!.startsWith("-") ? -1 : 3; +} + +function wrappedProgramIndex(cmd: readonly string[]): number { + let at = 0; + while (at < cmd.length) { + let nested: number; + switch (base(cmd[at]!)) { + case "env": nested = envProgramIndex(cmd.slice(at)); break; + case "mise": nested = miseProgramIndex(cmd.slice(at)); break; + case "direnv": nested = direnvProgramIndex(cmd.slice(at)); break; + default: return at; + } + if (nested < 1) return -1; + at += nested; + } + return -1; +} + +function codexRoot(a: string): Opt { + for (const o of ["-c", "--config", "--enable", "--disable", "--remote", "--remote-auth-token-env", "-i", "--image", "-m", "--model", "--local-provider", "-p", "--profile", "-s", "--sandbox", "-C", "--cd", "--add-dir", "-a", "--ask-for-approval"]) { + if (a === o) return [true, true]; + if (a.startsWith(`${o}=`) || (o.length === 2 && a.startsWith(o) && a.length > 2)) return [true, false]; + } + return ["--strict-config", "--oss", "--dangerously-bypass-approvals-and-sandbox", "--dangerously-bypass-hook-trust", "--approve-for-me", "--not-so-yolo", "--search", "--no-alt-screen", "-h", "--help", "-V", "--version"].includes(a) ? [true, false] : NO; +} + +function codexExecAfter(cmd: readonly string[], program: number): number { + for (let i = program + 1; i < cmd.length; i++) { + const a = cmd[i]!; + const [ok, next] = codexRoot(a); + if (ok) { + if (next) i++; + continue; + } + return a.startsWith("-") ? -1 : a === "exec" ? i : -1; + } + return -1; +} + +function codexExecIndex(executor: string, cmd: readonly string[]): number { + if (named(executor, "codex")) { + for (let p = 0; p < cmd.length; p++) if (base(cmd[p]!) === "codex") { + const at = codexExecAfter(cmd, p); + if (at >= 0) return at; + } + } + const program = wrappedProgramIndex(cmd); + if (program < 0 || (base(cmd[program]!) !== "codex" && !named(executor, "codex"))) return -1; + return codexExecAfter(cmd, program); +} + +function claudeProgramIndex(cmd: readonly string[]): number { + let p = wrappedProgramIndex(cmd); + if (p >= 0 && base(cmd[p]!) === "nice") { + p++; + if (p >= cmd.length) return -1; + if (cmd[p] === "-n" || cmd[p] === "--adjustment") p += 2; + else if (cmd[p]!.startsWith("-n") || cmd[p]!.startsWith("--adjustment=")) p++; + if (p >= cmd.length) return -1; + } + return p; +} + +const CLAUDE_VALUE = ["--advisor", "--agent", "--agents", "--append-subagent-system-prompt", "--append-system-prompt", "--append-system-prompt-file", "--betas", "--debug-file", "--disallowedTools", "--dangerously-load-development-channels", "--disallowed-tools", "--effort", "--fallback-model", "--from-pr", "--input-format", "--json-schema", "--max-budget-usd", "--max-turns", "--mcp-config", "--model", "--name", "-n", "--output-format", "--permission-mode", "--permission-prompt-tool", "--plugin-dir", "--plugin-url", "--session-id", "--settings", "--system-prompt", "--system-prompt-file", "--setting-sources", "--teammate-mode", "--tools", "--worktree", "-w"]; +const CLAUDE_BOOL = ["--allow-dangerously-skip-permissions", "--ax-screen-reader", "--bare", "--chrome", "--continue", "-c", "--dangerously-skip-permissions", "--disable-slash-commands", "--enable-auto-mode", "--exclude-dynamic-system-prompt-sections", "--debug", "--fork-session", "--forward-subagent-text", "--ide", "--include-hook-events", "--include-partial-messages", "--init", "--init-only", "--maintenance", "--no-chrome", "--no-session-persistence", "--print", "-p", "--replay-user-messages", "--restricted", "--safe-mode", "--strict-mcp-config", "--teleport", "--verbose"]; +const CLAUDE_VARIADIC = ["--add-dir", "--allowedTools", "--allowed-tools", "--betas", "--disallowedTools", "--disallowed-tools", "--mcp-config", "--tools"]; +const claudeOptional = (a: string): boolean => a === "--resume" || a === "-r"; + +function claudeRoot(a: string): Opt { + if (a === "--debug" || a.startsWith("--debug=")) return [true, false]; + if (claudeOptional(a) || CLAUDE_VARIADIC.includes(a)) return [true, false]; + for (const o of CLAUDE_VALUE) { + if (a === o) return [true, true]; + if (a.startsWith(`${o}=`)) return [true, false]; + } + if (CLAUDE_BOOL.includes(a)) return [true, false]; + if (a === "--prompt-suggestions" || a.startsWith("--prompt-suggestions=")) return [true, false]; + return NO; +} + +interface ClaudeInfo { + printIndex: number; + hasOutputFormat: boolean; + outputFormat: string; + hasVerbose: boolean; +} + +function claudeInfo(executor: string, cmd: readonly string[]): ClaudeInfo | undefined { + const program = claudeProgramIndex(cmd); + if (program < 0 || (base(cmd[program]!) !== "claude" && !named(executor, "claude"))) return undefined; + const info: ClaudeInfo = { printIndex: -1, hasOutputFormat: false, outputFormat: "", hasVerbose: false }; + for (let i = program + 1; i < cmd.length; i++) { + const a = cmd[i]!; + if (a === "--print" || a === "-p") { + if (info.printIndex >= 0) return undefined; + info.printIndex = i; + continue; + } + if (a === "--" || !a.startsWith("-")) return undefined; + const [ok, next] = claudeRoot(a); + if (!ok) return undefined; + if (a === "--output-format") { + if (i + 1 >= cmd.length) return undefined; + info.hasOutputFormat = true; + info.outputFormat = cmd[++i]!; + } else if (a.startsWith("--output-format=")) { + info.hasOutputFormat = true; + info.outputFormat = a.slice("--output-format=".length); + } else if (a === "--verbose") info.hasVerbose = true; + else if (claudeOptional(a)) { + if (i + 1 < cmd.length && !cmd[i + 1]!.startsWith("-")) i++; + } else if (CLAUDE_VARIADIC.includes(a)) { + let v = i + 1; + while (v < cmd.length && !cmd[v]!.startsWith("-")) v++; + if (v === i + 1) return undefined; + i = v - 1; + } else if (next) { + if (i + 1 >= cmd.length) return undefined; + i++; + } + } + return info.printIndex < 0 ? undefined : info; +} + +export function structuredCommand(executor: string, command: readonly string[]): Structured | undefined { + const exec = codexExecIndex(executor, command); + if (exec >= 1) { + const json = command.slice(exec + 1).includes("--json"); + return { preset: "codex", command: json ? [...command] : [...command.slice(0, exec + 1), "--json", ...command.slice(exec + 1)] }; + } + const info = claudeInfo(executor, command); + if (!info) return undefined; + if (info.hasOutputFormat) { + return info.outputFormat === "json" || info.outputFormat === "stream-json" ? { preset: "claude", command: [...command] } : undefined; + } + return { preset: "claude", command: [...command.slice(0, info.printIndex + 1), ...(info.hasVerbose ? [] : ["--verbose"]), "--output-format", "stream-json", ...command.slice(info.printIndex + 1)] }; +} diff --git a/tests/agents.test.ts b/tests/agents.test.ts index f244244..6494eed 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -342,3 +342,15 @@ describe("hardening from the verifier report", () => { expect(Date.now() - t0).toBeLessThan(6000); }); }); + +describe("preset-less command agents", () => { + test("a bare codex exec command gets the codex preset and --json", () => { + const a = resolveAgent({ codex: { command: ["env", "X=1", "codex", "exec", "{{prompt}}"] } }, { default: "codex" }, "triage"); + expect(a.preset).toBe(PRESETS.codex); + expect(a.config.command).toEqual(["env", "X=1", "codex", "exec", "--json", "{{prompt}}"]); + }); + test("an unrecognised command is left alone", () => { + const a = resolveAgent({ aider: { command: ["aider", "--message", "{{prompt}}"] } }, { default: "aider" }, "triage"); + expect(a.preset).toBeUndefined(); + }); +}); diff --git a/tests/ported/machinist/structured.test.ts b/tests/ported/machinist/structured.test.ts new file mode 100644 index 0000000..4a79eec --- /dev/null +++ b/tests/ported/machinist/structured.test.ts @@ -0,0 +1,93 @@ +// Ported from owainlewis/machinist@3943516 internal/runner/codex_usage_test.go:141-190 (MIT, Copyright (c) 2026 Owain Lewis). Deviations: the table is the Go table verbatim, generated from codex_usage_test.go and claude_usage_test.go:16-77,114-140; the collector-enabled assertions have no equivalent because the preset is chosen by structuredCommand. + +import { expect, test } from "bun:test"; +import { structuredCommand } from "../../../src/agents/structured"; + +const CODEX: [string, string, string[], string[]][] = [ + ["direct", "codex", ["codex", "exec", "-"], ["codex", "exec", "--json", "-"]], + ["root option value matches subcommand", "codex", ["codex", "--profile", "exec", "exec", "-"], ["codex", "--profile", "exec", "exec", "--json", "-"]], + ["custom executor name", "codex-local", ["agent", "exec", "-"], ["agent", "exec", "--json", "-"]], + ["wrapped", "custom", ["/usr/bin/env", "codex", "exec", "-"], ["/usr/bin/env", "codex", "exec", "--json", "-"]], + ["wrapped renamed executable", "codex-local", ["/usr/bin/env", "agent", "exec", "-"], ["/usr/bin/env", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after diagnostic option", "codex-local", ["/usr/bin/env", "-v", "agent", "exec", "-"], ["/usr/bin/env", "-v", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after compact options", "codex-local", ["/usr/bin/env", "-iv", "agent", "exec", "-"], ["/usr/bin/env", "-iv", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after compact unset", "codex-local", ["/usr/bin/env", "-iuMISSING", "agent", "exec", "-"], ["/usr/bin/env", "-iuMISSING", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after argv zero", "codex-local", ["/usr/bin/env", "--argv0=codex", "agent", "exec", "-"], ["/usr/bin/env", "--argv0=codex", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after short argv zero", "codex-local", ["/usr/bin/env", "-a", "codex", "agent", "exec", "-"], ["/usr/bin/env", "-a", "codex", "agent", "exec", "--json", "-"]], + ["wrapped renamed executable after empty environment alias", "codex-local", ["/usr/bin/env", "-", "agent", "exec", "-"], ["/usr/bin/env", "-", "agent", "exec", "--json", "-"]], + ["wrapper has its own exec", "custom", ["mise", "exec", "--", "codex", "exec", "-"], ["mise", "exec", "--", "codex", "exec", "--json", "-"]], + ["wrapper has global options", "custom", ["mise", "-q", "exec", "--", "codex", "exec", "-"], ["mise", "-q", "exec", "--", "codex", "exec", "--json", "-"]], + ["wrapper has compact global options", "custom", ["mise", "-qC/tmp", "exec", "--", "codex", "exec", "-"], ["mise", "-qC/tmp", "exec", "--", "codex", "exec", "--json", "-"]], + ["nested wrappers", "codex-local", ["env", "mise", "exec", "--", "codex", "exec", "-"], ["env", "mise", "exec", "--", "codex", "exec", "--json", "-"]], + ["reverse nested wrappers", "codex-local", ["mise", "exec", "--", "env", "agent", "exec", "-"], ["mise", "exec", "--", "env", "agent", "exec", "--json", "-"]], + ["direnv wrapper", "codex-local", ["direnv", "exec", ".", "codex", "exec", "-"], ["direnv", "exec", ".", "codex", "exec", "--json", "-"]], + ["direnv wrapper with renamed executable", "codex-local", ["direnv", "exec", ".", "agent", "exec", "-"], ["direnv", "exec", ".", "agent", "exec", "--json", "-"]], + ["nice wrapper", "codex-local", ["nice", "codex", "exec", "-"], ["nice", "codex", "exec", "--json", "-"]], + ["automatic review root option", "codex", ["codex", "--approve-for-me", "exec", "-"], ["codex", "--approve-for-me", "exec", "--json", "-"]], + ["legacy automatic review root option", "codex", ["codex", "--not-so-yolo", "exec", "-"], ["codex", "--not-so-yolo", "exec", "--json", "-"]], + ["already structured", "codex", ["codex", "exec", "--json", "-"], ["codex", "exec", "--json", "-"]], + ["other executor", "claude", ["claude", "exec", "-"], ["claude", "exec", "-"]], + ["Codex words are data", "custom", ["echo", "codex", "exec"], ["echo", "codex", "exec"]], + ["Codex words are env split string data", "custom", ["env", "-iSecho", "codex", "exec", "-"], ["env", "-iSecho", "codex", "exec", "-"]], + ["Codex words are long env split string data", "custom", ["env", "--split-string=echo", "codex", "exec", "-"], ["env", "--split-string=echo", "codex", "exec", "-"]], + ["Codex words are mise task arguments", "custom", ["mise", "run", "build", "--", "codex", "exec"], ["mise", "run", "build", "--", "codex", "exec"]], + ["other codex command", "codex", ["codex", "serve"], ["codex", "serve"]], + ["exec argument to another Codex command", "codex", ["codex", "review", "exec"], ["codex", "review", "exec"]], + ["unknown Codex root option", "codex", ["codex", "--future-option", "exec", "-"], ["codex", "--future-option", "exec", "-"]], +]; +const CLAUDE: [string, string, string[], string[]][] = [ + ["direct", "claude", ["claude", "--print"], ["claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["short print", "claude", ["claude", "-p", "--dangerously-skip-permissions"], ["claude", "-p", "--verbose", "--output-format", "stream-json", "--dangerously-skip-permissions"]], + ["renamed executable", "claude-local", ["agent", "--print"], ["agent", "--print", "--verbose", "--output-format", "stream-json"]], + ["env wrapper", "custom", ["env", "claude", "--print"], ["env", "claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["mise wrapper", "custom", ["mise", "exec", "--", "claude", "--print"], ["mise", "exec", "--", "claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["nice wrapper", "custom", ["nice", "claude", "--print"], ["nice", "claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["nice long adjustment", "custom", ["nice", "--adjustment=5", "claude", "--print"], ["nice", "--adjustment=5", "claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["nice separate long adjustment", "custom", ["nice", "--adjustment", "5", "claude", "--print"], ["nice", "--adjustment", "5", "claude", "--print", "--verbose", "--output-format", "stream-json"]], + ["max turns", "claude", ["claude", "--print", "--max-turns", "3"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--max-turns", "3"]], + ["bare resume", "claude", ["claude", "--resume", "--output-format=json", "--print"], ["claude", "--resume", "--output-format=json", "--print"]], + ["named resume", "claude", ["claude", "--resume", "session-name", "--print"], ["claude", "--resume", "session-name", "--print", "--verbose", "--output-format", "stream-json"]], + ["variadic allowed tools", "claude", ["claude", "--print", "--allowedTools", "Bash", "Edit"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--allowedTools", "Bash", "Edit"]], + ["variadic add dirs", "claude", ["claude", "--add-dir", "../apps", "../lib", "--print"], ["claude", "--add-dir", "../apps", "../lib", "--print", "--verbose", "--output-format", "stream-json"]], + ["agent", "claude", ["claude", "--print", "--agent", "reviewer"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--agent", "reviewer"]], + ["strict MCP config", "claude", ["claude", "--print", "--strict-mcp-config"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--strict-mcp-config"]], + ["prompt suggestions flag", "claude", ["claude", "--print", "--prompt-suggestions"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--prompt-suggestions"]], + ["prompt suggestions separate value", "claude", ["claude", "--print", "--prompt-suggestions", "false"], ["claude", "--print", "--prompt-suggestions", "false"]], + ["prompt suggestions equals value", "claude", ["claude", "--print", "--prompt-suggestions=false"], ["claude", "--print", "--verbose", "--output-format", "stream-json", "--prompt-suggestions=false"]], + ["existing verbose", "claude", ["claude", "--print", "--verbose"], ["claude", "--print", "--output-format", "stream-json", "--verbose"]], + ["explicit text", "claude", ["claude", "--print", "--output-format", "text"], ["claude", "--print", "--output-format", "text"]], + ["explicit json", "claude", ["claude", "--output-format=json", "--print"], ["claude", "--output-format=json", "--print"]], + ["explicit stream json", "claude", ["claude", "--print", "--output-format", "stream-json"], ["claude", "--print", "--output-format", "stream-json"]], +]; +const REJECT: [string, string, string[]][] = [ + ["missing print", "claude", ["claude", "--verbose"]], + ["prompt argument", "claude", ["claude", "--print", "prompt"]], + ["short version option is not verbose", "claude", ["claude", "--print", "-v"]], + ["unknown option", "claude", ["claude", "--future-option", "--print"]], + ["missing option value", "claude", ["claude", "--print", "--model"]], + ["missing variadic option value", "claude", ["claude", "--print", "--allowedTools"]], + ["prompt suggestions optional value", "claude", ["claude", "--print", "--prompt-suggestions", "false"]], + ["invalid output format", "claude", ["claude", "--print", "--output-format", "yaml"]], + ["misleading data", "custom", ["echo", "claude", "--print"]], + ["misleading data with Claude executor", "claude", ["echo", "claude", "--print"]], + ["malformed env wrapper", "custom", ["env", "--split-string=claude --print"]], +]; +test("Codex commands get --json, wrapped or not, and odd ones are left alone", () => { + for (const [name, executor, command, want] of CODEX) { + const before = [...command]; + const got = structuredCommand(executor, command); + const isCodex = want.includes("--json") && want.includes("exec"); + expect(got ? got.command : command, name).toEqual(want.length ? want : command); + if (got) expect(got.preset, name).toBe(executor === "claude" ? "claude" : "codex"); + else expect(isCodex && !command.includes("--json"), name).toBe(false); + expect(command, `${name}: input mutated`).toEqual(before); + } +}); + +test("Claude print commands get stream-json, and ambiguous ones are left alone", () => { + for (const [name, executor, command, want] of CLAUDE) { + const got = structuredCommand(executor, command); + expect(got ? got.command : command, name).toEqual(want); + } + for (const [name, executor, command] of REJECT) expect(structuredCommand(executor, command), name).toBeUndefined(); +}); From 4e60be9a456e572de7d9cbd630a0261aec3f1f3c Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:48:34 +0300 Subject: [PATCH 6/9] Refuse stale gate evidence before verify (P39) Co-Authored-By: Claude Sonnet 5 --- src/artifacts.ts | 5 +++++ src/watch.ts | 10 ++++++++++ tests/harness.ts | 6 +++++- tests/scenarios.test.ts | 13 +++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/artifacts.ts b/src/artifacts.ts index 3f6b068..fc520c0 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -214,6 +214,11 @@ export async function writeGateEvidence(cwd: string, issue: number, evidence: Ga await Bun.write(`${cwd}/${runDir(issue)}/gate.json`, `${JSON.stringify(evidence)}\n`); } +export async function readGateEvidence(cwd: string, issue: number): Promise { + const v = await readJson>(`${cwd}/${runDir(issue)}/gate.json`); + return typeof v?.tree === "string" && typeof v.line === "string" && typeof v.status === "string" ? { line: v.line, status: v.status, tree: v.tree } : undefined; +} + async function readText(path: string): Promise { const file = Bun.file(path); if (!(await file.exists())) return undefined; diff --git a/src/watch.ts b/src/watch.ts index 1494ff5..d7cd076 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -20,6 +20,7 @@ import { stepStop, validateStepJson, validateVerdict, + readGateEvidence, writeGateEvidence, readStageArtifacts, runDir, @@ -403,6 +404,15 @@ async function runFromStage( } if (stage === "verify") { + // A verdict is only as good as its evidence: gate.json must describe + // the tree being verified, so a resumed or amended run re-measures it. + const tree = await deps.git.treeHash(worktree); + const seen = await readGateEvidence(worktree, issueNumber); + if (seen?.tree !== tree) { + const fresh = await runGates(deps.gateRunner, worktree); + deps.state.updateRun(config.repo, issueNumber, { gate_line: fresh.raw }); + await writeGateEvidence(worktree, issueNumber, { line: fresh.raw, status: fresh.status, tree }); + } const result = await runStage(deps, config, issue, "verify", worktree); const art = await readStageArtifacts(worktree, issueNumber, "verify"); const checked = art.json === undefined ? undefined : validateVerdict(art.json); diff --git a/tests/harness.ts b/tests/harness.ts index 4edd92e..d6a9a52 100644 --- a/tests/harness.ts +++ b/tests/harness.ts @@ -181,8 +181,10 @@ export class FakeGit extends Git { return { stdout: "", stderr: "", code: 0 }; } + trees = ["faketree"]; + // Each call returns the next tree, then repeats the last, so a test can move HEAD between build and verify. override async treeHash(): Promise { - return "faketree"; + return this.trees.length > 1 ? this.trees.shift()! : this.trees[0]!; } override async hasCommits(): Promise { @@ -195,7 +197,9 @@ export class FakeGit extends Git { // test can swap `line` for a red one to exercise the failure path. export class FakeGateRunner implements GateRunner { line = "FACTORY_GATES: status=GREEN passed=10 failed=0 skipped=0 failed_gates=-"; + runs = 0; async run(_worktreeDir: string) { + this.runs++; return { stdout: this.line, stderr: "", code: 0 }; } } diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index 3dfef24..cc965f1 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -365,6 +365,19 @@ describe("failure paths", () => { done(c); }); + test("14d. verify re-runs the gates when gate.json describes a different tree", async () => { + const same = setup(["factory:ready"]); + happy(same); + await same.step(); + expect(same.gateRunner.runs).toBe(1); + + const moved = setup(["factory:ready"]); + moved.git.trees = ["built", "amended"]; + happy(moved); + await moved.step(); + expect(moved.gateRunner.runs).toBe(2); + }); + test("14c. outcome failed fails the run; an outcome complete does not hide a non-zero exit", async () => { const c = setup([LABEL.ready]); c.push("triage", triage({ outcome: "failed", summary: "cannot reproduce" })); From 43ca3982f5b7b9c1a77f24e93d008367080fafb6 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:49:57 +0300 Subject: [PATCH 7/9] Templates teach outcome and AC ids, build stops at 3, triage refuses a taken issue Co-Authored-By: Claude Sonnet 5 --- src/github.ts | 3 ++- src/watch.ts | 10 ++++++++++ template/.claude/skills/factory-build/SKILL.md | 9 ++++++--- .../skills/factory-comment/assets/verdict.md | 2 +- template/.claude/skills/factory-plan/SKILL.md | 5 ++++- template/.claude/skills/factory-triage/SKILL.md | 3 +++ template/.claude/skills/factory-verify/SKILL.md | 2 ++ tests/agents.test.ts | 15 +++++++++++++++ tests/scenarios.test.ts | 8 ++++++++ 9 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/github.ts b/src/github.ts index 30e2d33..b2dfad0 100644 --- a/src/github.ts +++ b/src/github.ts @@ -57,6 +57,7 @@ export interface GhPr { state: string; headRefName: string; isDraft: boolean; + closingIssuesReferences?: { number: number }[]; } class GhError extends Error { @@ -198,7 +199,7 @@ export class GitHub { "--state", opts?.state ?? "open", "--json", - "number,url,state,headRefName,isDraft", + "number,url,state,headRefName,isDraft,closingIssuesReferences", "--limit", "100", ]); diff --git a/src/watch.ts b/src/watch.ts index d7cd076..2aa41db 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -268,6 +268,16 @@ async function runFromStage( for (;;) { if (stage === "triage") { + // Someone else's open PR already closes this issue: don't spend tokens on a second fix. + const own = deps.git.branchName(issueNumber); + const taken = (await deps.github.listPrs(config.repo, { state: "open" })).find( + (p) => p.headRefName !== own && p.closingIssuesReferences?.some((r) => r.number === issueNumber), + ); + if (taken) { + await moveLabel(deps, config, issueNumber, LABEL.triaging, LABEL.needsHuman); + finish(deps, config, issueNumber, "needs-human", `open PR #${taken.number} already closes this issue`); + return "needs-human"; + } const result = await runStage(deps, config, issue, "triage", worktree); const art = await readStageArtifacts(worktree, issueNumber, "triage"); const { json, problem } = stageJson("triage", art.json); diff --git a/template/.claude/skills/factory-build/SKILL.md b/template/.claude/skills/factory-build/SKILL.md index 060c694..fa0896b 100644 --- a/template/.claude/skills/factory-build/SKILL.md +++ b/template/.claude/skills/factory-build/SKILL.md @@ -35,9 +35,9 @@ Quote the exact gate line — the command and its pass/fail output — verbatim in the status comment. Do not paraphrase or summarize a failure as "some tests failed"; show the line that failed. -If the gate fails and you can see why, fix it and re-run. Don't loop more -than a few times guessing; if you can't get it green, say so in the status -comment and stop — `factory-verify` will catch a red gate anyway, but a +If the gate fails and you can see why, fix it and re-run. Stop after 3 +failed gate runs: write `"outcome": "blocked"` and say so in the status +comment. If you can't get it green, say so in the status comment and stop — `factory-verify` will catch a red gate anyway, but a build that knows it's broken shouldn't pretend otherwise. ## 4. Escape hatch: back to needs-info mid-build @@ -67,6 +67,9 @@ Write `.factory/runs/issue-/status-comment.md` using the } ``` +`outcome` is optional: `complete` (the default), `blocked` (you cannot go on and a human must +look; put the reason in `summary`), or `failed`. No other fields are allowed. + `status` is one of `green`, `red` (gate never went green after reasonable effort), or `needs-info` (see step 4). `rounds` is this issue's build attempt count so far, including any verify-reject that sent you back here diff --git a/template/.claude/skills/factory-comment/assets/verdict.md b/template/.claude/skills/factory-comment/assets/verdict.md index e8b56cb..e3d8e4a 100644 --- a/template/.claude/skills/factory-comment/assets/verdict.md +++ b/template/.claude/skills/factory-comment/assets/verdict.md @@ -3,7 +3,7 @@ ### Acceptance criteria evidence {{#each ac}} -- **AC-{{n}}:** {{pass_or_fail}} — {{evidence_command_and_result}} +- **AC-{{n}}:** {{pass|fail|unverified}} — {{evidence_command_and_result}} {{/each}} ### The test that bites diff --git a/template/.claude/skills/factory-plan/SKILL.md b/template/.claude/skills/factory-plan/SKILL.md index 883e796..473ffcc 100644 --- a/template/.claude/skills/factory-plan/SKILL.md +++ b/template/.claude/skills/factory-plan/SKILL.md @@ -33,7 +33,7 @@ own context doing the same search yourself. ## 3. Write the plan -One line goal. Acceptance criteria `AC-1..n`, each checkable by a named +One line goal. Acceptance criteria `AC-1..n` (ids are never renumbered on a revision), each checkable by a named command or test. Non-goals `NG-1..n`: binding — the verifier fails a diff that crosses one, so write ones you actually mean. Files to touch. Tests to write first, named. Repo skills to apply, or "none". Risk: low, medium, or @@ -65,6 +65,9 @@ plan, or the previous revision + 1 when `revise.md` is present. Then write } ``` +`outcome` is optional: `complete` (the default), `blocked` (you cannot go on and a human must +look; put the reason in `summary`), or `failed`. No other fields are allowed. + `autoApproveEligible` is your judgment call, not just a mirror of `risk`: set it false for anything you'd want a second look at even at low risk. diff --git a/template/.claude/skills/factory-triage/SKILL.md b/template/.claude/skills/factory-triage/SKILL.md index a9c4245..909e04e 100644 --- a/template/.claude/skills/factory-triage/SKILL.md +++ b/template/.claude/skills/factory-triage/SKILL.md @@ -67,6 +67,9 @@ Use the `factory-comment` skill's `triage.md` template to write } ``` +`outcome` is optional: `complete` (the default), `blocked` (you cannot go on and a human must +look; put the reason in `summary`), or `failed`. No other fields are allowed. + If `disposition` is `needs-info`, also write `.factory/runs/issue-/question-comment.md` using the `question.md` template: at most 3 numbered questions, 2-3 lettered options each, a diff --git a/template/.claude/skills/factory-verify/SKILL.md b/template/.claude/skills/factory-verify/SKILL.md index 34a6433..46d0c99 100644 --- a/template/.claude/skills/factory-verify/SKILL.md +++ b/template/.claude/skills/factory-verify/SKILL.md @@ -77,6 +77,8 @@ criterion is `pass`, `fail`, or `unverified` (with a `gap`); AC ids come from th plan and are never renumbered. No other fields are allowed, and the file must be one JSON object under 16 KiB. +Set `outcome` to `blocked` (with a `summary`) only if you could not review at all. + `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 6494eed..677f56e 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -307,6 +307,21 @@ describe("verdict rules", () => { }); }); +describe("skill text the runner depends on", () => { + const skill = (n: string) => readFileSync(join(import.meta.dir, `../template/.claude/skills/${n}/SKILL.md`), "utf8"); + test("build stops at 3 gate runs, plan never renumbers, every step skill teaches outcome", () => { + expect(skill("factory-build")).toContain("Stop after 3"); + expect(skill("factory-build")).not.toContain("a few times"); + expect(skill("factory-plan")).toContain("never renumbered"); + for (const n of ["triage", "plan", "build", "verify"]) expect(skill(`factory-${n}`), n).toContain("outcome"); + }); + test("the verdict template renders a per-criterion status", () => { + const t = readFileSync(join(import.meta.dir, "../template/.claude/skills/factory-comment/assets/verdict.md"), "utf8"); + expect(t).toContain("{{pass|fail|unverified}}"); + expect(t).not.toContain("pass_or_fail"); + }); +}); + describe("hardening from the verifier report", () => { test.each([ [{ agents: { x: { command: [""] } } }, /must not be empty/], diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index cc965f1..7509cb4 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -378,6 +378,14 @@ describe("failure paths", () => { expect(moved.gateRunner.runs).toBe(2); }); + test("14e. triage refuses an issue another open PR already closes, and spends no tokens", async () => { + const c = setup(["factory:ready"]); + c.github.prs.push({ number: 9, url: "u", state: "open", headRefName: "someone/fix", isDraft: false, closingIssuesReferences: [{ number: c.n }] }); + happy(c); + expect(await c.step()).toBe("needs-human"); + expect((c.state as unknown as { db: { query(q: string): { all(): unknown[] } } }).db.query("SELECT 1 FROM stage_runs").all()).toHaveLength(0); + }); + test("14c. outcome failed fails the run; an outcome complete does not hide a non-zero exit", async () => { const c = setup([LABEL.ready]); c.push("triage", triage({ outcome: "failed", summary: "cannot reproduce" })); From 570d94909371282f530bfd109dfa4553417b20c1 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:52:04 +0300 Subject: [PATCH 8/9] Opaque session cookie, plain() on thread, artifacts, run titles and CLI errors, InboxChannel Co-Authored-By: Claude Sonnet 5 --- bin/factory | 10 ++------- dashboard/server.ts | 37 +++++++++++++++++++++++++--------- src/cli-output.ts | 3 ++- src/inbox.ts | 18 +++++++++++++++++ tests/cli-output.test.ts | 6 ++++++ tests/dashboard-routes.test.ts | 34 +++++++++++++++++++++++++++++++ tests/inbox.test.ts | 37 +++++++++++++++++++++++++++++++++- 7 files changed, 126 insertions(+), 19 deletions(-) diff --git a/bin/factory b/bin/factory index edfc72d..a13053a 100755 --- a/bin/factory +++ b/bin/factory @@ -15,7 +15,7 @@ import { advanceIssue, pollOnce, recoverInFlight, startWatch, type WatchDeps } f import { ShellGateRunner } from "../src/gates"; import { scan } from "../src/scan"; import { streamLogs } from "../src/logs"; -import { act, buildInbox, type InboxAction } from "../src/inbox"; +import { act, buildInbox, inboxPositionals, type InboxAction } from "../src/inbox"; import { reset, rebaseline } from "../src/reset"; import { runDoctor, fixDoctor } from "../src/doctor"; import { createDashboard } from "../dashboard/server"; @@ -164,13 +164,7 @@ async function cmdInbox(): Promise { if (!repo) throw new UsageError("inbox: --repo (or FACTORY_REPO) is required"); const github = new GitHub(); const items = buildInbox(await github.listOpenIssues(repo)); - const positional: string[] = []; - for (let i = 1; i < args.length; i++) { - if (args[i] === "--json") continue; - if (args[i]!.startsWith("--")) i++; // a flag and its value - else positional.push(args[i]!); - } - const [issueArg, actionArg] = positional; + const [issueArg, actionArg] = inboxPositionals(args); if (issueArg) { const item = items.find((i) => i.issue === Number(issueArg)); if (!item) throw new UsageError(`inbox: #${issueArg} is not waiting for you`); diff --git a/dashboard/server.ts b/dashboard/server.ts index 331c4b4..7541e16 100644 --- a/dashboard/server.ts +++ b/dashboard/server.ts @@ -6,7 +6,7 @@ // works with no local SQLite at all (a CI/VM run with no watcher on this // machine). -import { createHash, timingSafeEqual } from "node:crypto"; +import { createHash, randomBytes, timingSafeEqual } from "node:crypto"; import { readFileSync, readdirSync, realpathSync, statSync } from "node:fs"; import { dirname, join, sep } from "node:path"; import { fileURLToPath } from "node:url"; @@ -44,6 +44,8 @@ export function tokenMatches(presented: string, expected: string): boolean { } const SESSION_COOKIE = "factory_session"; +const SESSION_TTL_MS = 8 * 60 * 60 * 1000; +const MAX_SESSIONS = 1000; function cookie(req: Request, name: string): string { for (const part of (req.headers.get("cookie") ?? "").split(";")) { @@ -61,9 +63,18 @@ const ASSET_TYPES: Record = { css: "text/css; charset=utf-8", js export const ARTIFACT_LIMIT = 1024 * 1024; +function plainRun(run: T): T { + return { ...run, title: plain(run.title) }; +} + +function plainIssue(issue: T): T { + return { ...issue, title: plain(issue.title), body: plain(issue.body), comments: issue.comments.map((c) => ({ ...c, body: plain(c.body) })) }; +} + export function createDashboard(state: FactoryState, github: GitHub, repo: string, autoApproveDefault = false, workspaces = workspacesDir()) { const indexHtml = readFileSync(join(here, "public", "index.html"), "utf8"); + const sessions = new Map(); let boardCache: { at: number; issues: Awaited> } | null = null; let boardInflight: Promise>> | null = null; @@ -152,7 +163,8 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin "x-artifact-truncated": String(truncated), }; if (url.searchParams.get("download")) headers["content-disposition"] = `attachment; filename="${name.replace(/[^\w.-]/g, "_")}"`; - return new Response(body, { headers }); + // A preview is text for a person; a download stays byte for byte. + return new Response(url.searchParams.get("download") ? body : plain(Buffer.from(body).toString("utf8")), { headers }); } type Handler = (req: Request, url: URL, m: RegExpMatchArray) => Response | Promise; @@ -191,10 +203,16 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin if (!DASHBOARD_TOKEN || typeof body.token !== "string" || !tokenMatches(body.token, DASHBOARD_TOKEN)) { return json({ error: "unauthorized" }, { status: 401 }); } - return json({ ok: true }, { headers: { "set-cookie": `${SESSION_COOKIE}=${DASHBOARD_TOKEN}; HttpOnly; SameSite=Strict; Path=/` } }); + // The cookie is a random id, never the token, so a leaked cookie cannot be replayed as a Bearer token. + const id = randomBytes(32).toString("hex"); + const now = Date.now(); + for (const [k, expires] of sessions) if (expires <= now) sessions.delete(k); + if (sessions.size >= MAX_SESSIONS) sessions.delete(sessions.keys().next().value!); + sessions.set(id, now + SESSION_TTL_MS); + return json({ ok: true }, { headers: { "set-cookie": `${SESSION_COOKIE}=${id}; HttpOnly; SameSite=Strict; Path=/; Max-Age=${SESSION_TTL_MS / 1000}` } }); }, }, - { method: "GET", pattern: /^\/api\/runs$/, label: "GET /api/runs", handler: () => json({ repo, runs: state.listRuns(repo || undefined) }) }, + { method: "GET", pattern: /^\/api\/runs$/, label: "GET /api/runs", handler: () => json({ repo, runs: state.listRuns(repo || undefined).map(plainRun) }) }, { method: "GET", pattern: /^\/api\/board$/, @@ -219,7 +237,7 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin method: "GET", pattern: /^\/api\/issues\/(\d+)\/thread$/, label: "GET /api/issues/:n/thread", - handler: async (_req, _url, m) => needRepo() ?? json({ issue: await github.getIssue(repo, Number(m[1])) }), + handler: async (_req, _url, m) => needRepo() ?? json({ issue: plainIssue(await github.getIssue(repo, Number(m[1]))) }), }, { method: "GET", @@ -243,7 +261,7 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin handler: (_req, _url, m) => { const run = state.listRuns(repo || undefined).find((r) => r.id === Number(m[1])); if (!run) return json({ error: "no such run" }, { status: 404 }); - return json({ run, stages: state.listStageRuns(run.repo, { issue: run.issue }) }); + return json({ run: plainRun(run), stages: state.listStageRuns(run.repo, { issue: run.issue }) }); }, }, { @@ -257,7 +275,7 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin return json({ repo, rows: runs.map((run) => ({ - run, + run: plainRun(run), stages: attempts .filter((a) => a.issue === run.issue) .map((a) => ({ stage: a.stage, agent: a.agent, duration_ms: a.duration_ms, cost_usd: a.cost_usd, ok: a.exit_code === 0 && !a.killed_reason })), @@ -375,8 +393,9 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin function authorized(req: Request): boolean { const header = req.headers.get("authorization") ?? ""; - const presented = header.startsWith("Bearer ") ? header.slice(7) : cookie(req, SESSION_COOKIE); - return tokenMatches(presented, DASHBOARD_TOKEN); + if (header.startsWith("Bearer ")) return tokenMatches(header.slice(7), DASHBOARD_TOKEN); + const expires = sessions.get(cookie(req, SESSION_COOKIE)); + return expires !== undefined && expires > Date.now(); } // `remoteAddress` comes from `server.requestIP(req)` at the real Bun.serve diff --git a/src/cli-output.ts b/src/cli-output.ts index 57ee142..64e7896 100644 --- a/src/cli-output.ts +++ b/src/cli-output.ts @@ -5,6 +5,7 @@ // Deviations: the codes are the factory's own, and 1 keeps meaning "failed" so CI steps still fail. import { ConfigError } from "./config"; +import { plain } from "./display"; export const EXIT = { ok: 0, @@ -29,6 +30,6 @@ export function successJson(data: unknown, ok = true): string { } export function failureJson(err: unknown): string { - const message = err instanceof Error ? err.message : String(err); + const message = plain(err instanceof Error ? err.message : String(err)); return JSON.stringify({ ok: false, error: { kind: errorKind(err), message } }, null, 2); } diff --git a/src/inbox.ts b/src/inbox.ts index 71aff94..713ac27 100644 --- a/src/inbox.ts +++ b/src/inbox.ts @@ -92,3 +92,21 @@ export async function act( await github.commentIssue(repo, item.issue, body); return body; } + +// What a chat or push channel implements (v3.1): show the waiting items, and +// send replies back through `act`, so it shares the trust rule with everything else. +export interface InboxChannel { + readonly name: string; + send(items: readonly InboxItem[]): Promise; +} + +// `factory inbox [issue [action]] [--flag value] [--json]`: the positional words after the verb. +export function inboxPositionals(argv: readonly string[]): string[] { + const out: string[] = []; + for (let i = 1; i < argv.length; i++) { + if (argv[i] === "--json") continue; + if (argv[i]!.startsWith("--")) i++; // a flag and its value + else out.push(argv[i]!); + } + return out; +} diff --git a/tests/cli-output.test.ts b/tests/cli-output.test.ts index 40d950f..8dd3c0e 100644 --- a/tests/cli-output.test.ts +++ b/tests/cli-output.test.ts @@ -69,3 +69,9 @@ test("every pause-aware --json command exits EXIT.paused, so tick and watch --on const src = readFileSync(bin, "utf8"); expect(src.match(/if \(result\.paused\) process\.exit\(EXIT\.paused\)/g)?.length).toBe(2); }); + +describe("failureJson", () => { + test("strips terminal escapes from an error message", () => { + expect(JSON.parse(failureJson(new Error("bad\u001b[31m thing"))).error.message).toBe("bad thing"); + }); +}); diff --git a/tests/dashboard-routes.test.ts b/tests/dashboard-routes.test.ts index bc59880..fa70ab9 100644 --- a/tests/dashboard-routes.test.ts +++ b/tests/dashboard-routes.test.ts @@ -19,6 +19,9 @@ class FakeGitHub extends GitHub { override async listOpenIssues(): Promise { return this.issues; } + override async getIssue(_repo: string, number: number): Promise { + return this.issues.find((i) => i.number === number)!; + } override async commentIssue(_repo: string, issue: number, body: string): Promise { this.posted.push({ issue, body }); return 1; @@ -213,3 +216,34 @@ describe("line and assets", () => { expect((await dashboard.handle(new Request("http://localhost:4100/lib/..%2Fserver.js"), "10.0.0.9")).status).toBe(401); }); }); + +describe("session cookie and terminal-safe text", () => { + const ESC = "\u001b[31m"; + + test("the cookie is a random id, not the token, and it is not accepted as a Bearer token", async () => { + const { dashboard } = await make(TOKEN); + const login = async () => + dashboard.handle(new Request("http://localhost:4100/api/session", { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({ token: TOKEN }) }), "203.0.113.7"); + const one = (await login()).headers.get("set-cookie")!; + const two = (await login()).headers.get("set-cookie")!; + expect(one).not.toContain(TOKEN); + expect(one.split(";")[0]).not.toBe(two.split(";")[0]); + const id = one.split(";")[0]!.split("=")[1]!; + const at = (headers: Record) => dashboard.handle(new Request("http://localhost:4100/api/runs", { headers }), "203.0.113.7"); + expect((await at({ authorization: `Bearer ${id}` })).status).toBe(401); + expect((await at({ cookie: `factory_session=${TOKEN}` })).status).toBe(401); + expect((await at({ cookie: `factory_session=${id}` })).status).toBe(200); + }); + + test("thread, run titles and artifact previews carry no terminal escapes; a download stays raw", async () => { + const issue: GhIssue = { number: 1, title: `T${ESC}`, body: `B${ESC}`, labels: [], comments: [{ id: 1, author: "a", authorAssociation: "NONE", body: `C${ESC}`, createdAt: "2026-09-20T10:00:00Z" }] }; + const { dashboard, state } = await make("", [issue]); + state.upsertRun({ issue: 1, repo: "acme/widgets", title: `Run${ESC}`, stage: "build", status: "running" }); + const dir = join(scratch, "issue-1", ".factory", "runs", "issue-1"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "plan.md"), `plan${ESC}`); + const get = async (path: string) => dashboard.handle(new Request(`http://localhost:4100${path}`), "127.0.0.1"); + for (const path of ["/api/runs", "/api/issues/1/thread", "/api/issues/1/artifacts?file=plan.md", "/api/line"]) expect(await (await get(path)).text(), path).not.toContain("\u001b"); + expect(await (await get("/api/issues/1/artifacts?file=plan.md&download=1")).text()).toContain("\u001b"); + }); +}); diff --git a/tests/inbox.test.ts b/tests/inbox.test.ts index 79ac0b7..97b9430 100644 --- a/tests/inbox.test.ts +++ b/tests/inbox.test.ts @@ -5,7 +5,7 @@ import { describe, expect, test } from "bun:test"; import { parseChatOps } from "../src/chatops"; import type { GhIssue } from "../src/github"; -import { InboxError, WAITING, WAITING_LABELS, act, buildInbox, commandText } from "../src/inbox"; +import { InboxError, inboxPositionals, type InboxChannel, type InboxAction, WAITING, WAITING_LABELS, act, buildInbox, commandText } from "../src/inbox"; import { LABELS, LABEL, PARKED_LABELS, STATE_LABELS } from "../src/labels"; const at = (n: number) => `2026-09-2${n}T10:00:00Z`; @@ -90,3 +90,38 @@ describe("act", () => { expect(parseChatOps("USD only").type).toBe("answer"); }); }); + +describe("ChatOps commands, structurally", () => { + const ACTIONS: InboxAction[] = ["approve", "revise", "answer", "retry", "cancel"]; + + test("every action round-trips through the parser, and every command the parser knows is offered somewhere", () => { + for (const a of ACTIONS) expect(parseChatOps(commandText(a, "words")).type, a).toBe(a); + const offered = new Set(Object.values(WAITING).flatMap((w) => w.actions)); + for (const a of ACTIONS) expect(offered.has(a), `${a} is offered by no waiting state`).toBe(true); + }); + + test("a command typed in a comment is only a command when it starts the comment", () => { + for (const a of ["approve", "retry", "cancel"] as const) expect(parseChatOps(`please /factory ${a}`).type).toBe("answer"); + }); +}); + +describe("inbox CLI arguments", () => { + test("positionals skip flags with their values and --json", () => { + expect(inboxPositionals(["inbox"])).toEqual([]); + expect(inboxPositionals(["inbox", "12", "revise", "--text", "smaller"])).toEqual(["12", "revise"]); + expect(inboxPositionals(["inbox", "--repo", "a/b", "--json", "7", "approve"])).toEqual(["7", "approve"]); + }); +}); + +describe("InboxChannel", () => { + test("a channel receives items and replies through act, so the trust rule is shared", async () => { + const seen: number[] = []; + const channel: InboxChannel = { name: "fake", send: async (items) => void seen.push(...items.map((i) => i.issue)) }; + const items = buildInbox([issue(5, LABEL.awaitingApproval)]); + await channel.send(items); + expect(seen).toEqual([5]); + const posted: string[] = []; + expect(await act({ commentIssue: async (_r, _n, body) => (posted.push(body), 1) }, "a/b", items[0]!, "approve")).toBe("/factory approve"); + expect(posted).toEqual(["/factory approve"]); + }); +}); From 674c2401af3f77e31eb84bbc2905b01828f8de9c Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Thu, 24 Sep 2026 12:55:32 +0300 Subject: [PATCH 9/9] v2.5.1: plain() on CLI errors, changelog, version bump Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 36 +++++++++++++++++++++++++++++++++ bin/factory | 3 ++- package.json | 2 +- template-ci/factory.yml.example | 2 +- 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3d0726..81141e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,41 @@ # Changelog +## v2.5.1 + +Finishes v2.5: the pieces it promised and did not ship. + +- Cost meter: `src/pricing.ts` holds a dated per-model price table. An unknown model is "not reported", + never $0. `stage_runs` gains `tokens_cached` and `usage_complete` (guarded, idempotent `ALTER`), and the + dashboard shows "Not reported" for incomplete usage. +- Stage policy: triage, plan and verify run Codex with `-s read-only` and return their artifact as the final + message; the runner validates it (`src/agents/reply.ts`) and writes the file. Build and pr keep + `workspace-write`. Opt in to `--output-schema` with `outputSchema: true` on an agent. +- Stage artifacts get a JSON Schema built from the validators (`src/schemas.ts`), included in the artifact + contract. Every step artifact accepts `outcome: complete|blocked|failed` and rejects unknown fields; + `blocked` goes to needs-human with the reason. +- Event log has a 32 MiB byte budget per run, with a `process.output_truncated` marker. +- A `command` agent that is really `codex exec` or `claude -p`, even behind `env`, `mise` or `direnv`, gets + its JSON flag and preset parser. +- Verify refuses stale evidence: if `gate.json` names a different tree than HEAD, the gates re-run first. + Triage refuses an issue another open PR already closes, before any tokens are spent. +- Skills: `outcome` is taught, verdict comments render `pass|fail|unverified` per criterion, AC ids are never + renumbered, build stops after 3 failed gate runs. +- Dashboard: the session cookie is a random id, not the token. Thread, run titles, artifact previews and CLI + errors pass through `plain()`. `InboxChannel` interface, inbox argument parsing and every ChatOps + command are tested. +- Provenance: the test now checks one ported test per row and Markdown ports. + +Not in this release: + +- No live Codex run (#18). Claude gets no `--json-schema`; `outputSchema` is opt-in. +- `gpt-5.6-terra` is unpriced, so its cost is not reported. Incomplete usage stores `cost_usd = 0` with + `usage_complete = 0`, not NULL. +- A read-only reply's artifact is capped at 16 KiB. +- The tool-free finding re-check call (P40) is still prompt text only. +- Sub-minute durations keep the upstream `42.5s` format. +- Upstream `runs-view.test.js`, `artifacts_test.go` and the auth tests are not ported. +- Real Claude fixtures per stage are not recorded; they spend tokens. + ## v2.5.0 Any coding agent: the factory no longer knows Claude by name. An agent is config. diff --git a/bin/factory b/bin/factory index a13053a..a602562 100755 --- a/bin/factory +++ b/bin/factory @@ -15,6 +15,7 @@ import { advanceIssue, pollOnce, recoverInFlight, startWatch, type WatchDeps } f import { ShellGateRunner } from "../src/gates"; import { scan } from "../src/scan"; import { streamLogs } from "../src/logs"; +import { plain } from "../src/display"; import { act, buildInbox, inboxPositionals, type InboxAction } from "../src/inbox"; import { reset, rebaseline } from "../src/reset"; import { runDoctor, fixDoctor } from "../src/doctor"; @@ -376,6 +377,6 @@ try { await main(); } catch (err) { if (has("json")) console.error(failureJson(err)); - else console.error(`factory: ${err instanceof Error ? err.message : err}`); + else console.error(`factory: ${plain(err instanceof Error ? err.message : String(err))}`); process.exit(EXIT.error); } diff --git a/package.json b/package.json index 94c87d2..32f0a52 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "software-factory", - "version": "2.5.0", + "version": "2.5.1", "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 ba95d08..a6a6625 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.5.0 # pinned software-factory release; bump deliberately + FACTORY_RUNNER_REF: v2.5.1 # 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