diff --git a/CHANGELOG.md b/CHANGELOG.md index b7275ab..6ce912e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,64 @@ # Changelog +## v2.10.0 + +Memory across runs. A run's lessons now survive it: a read-only retro stage proposes at +most one lesson or skill edit after an issue's final outcome, and `factory learn` batches +whatever's pending into a PR you review, never merges automatically. + +- **`research/agents/claude/memory.md`.** Headless Claude's own auto-memory writes to a + path keyed by a hash of the worktree's absolute directory, under the operator's home + directory: operator-machine-local state, gone once a worktree is torn down, invisible in + CI, and never shared across machines. Confirms the roadmap's pre-decided design: the + factory never relies on `~/.claude`. +- **`src/context.ts`: repo-local memory.** `.factory/memory/lessons.md` is capped at 8 KiB + (`MAX_LESSONS_BYTES`), keeping the most recent lines and dropping the oldest when it + would grow past the cap, and is injected into every stage's context pack the same way on + the Mac, a VPS or in CI. +- **`src/watch.ts`: a retro stage after each final outcome** (merged, rejected by you, or + given up on after the third verify rejection past `MAX_VERIFY_REJECTS`), plus a fourth + trigger from the dashboard's operator-merge route, which queues the retro for `pollOnce` + to drain since it has no `Executor` of its own to run it. `runRetro` never goes through + `runStage`'s `upsertRun` (an already-terminal run would be put back to "running"), but + its cost still rolls into spend-cap accounting through the same `recordStageRun` call. + Read-only, on Haiku by default; it proposes at most one lesson or skill edit, or none + (neo's rule of at most one task per review, idea only, no code copied), and writes a + `retro.json` artifact linking the run. +- **`src/learn.ts`: `factory learn`.** A deterministic, non-agent command that batches + every pending retro proposal into `.factory/memory/lessons.md` and per-skill + `.claude/skills//PROPOSED_EDITS.md` files, commits, and diffs the branch against + base before pushing. `outsideAllowedPaths` (`src/boundary.ts`) is the one exemption from + `protectedPaths`, and only for a `factory/learning-YYYYMMDD` branch: it may touch + `.factory/memory/**` and `.claude/skills/**`, nothing else. A row is marked learned only + after its branch pushes and its PR exists, so a refusal or a crash first leaves it + pending for the next run rather than dropping it. Idempotent per day: a second run + reuses the same branch and PR. It never auto-merges, and shows in the dashboard inbox as + its own read-only `learning-pr` kind, since it has no linked issue to chatops against. +- **`src/boundary.ts`: `touchesProtectedPath` now always refuses `.claude/**` and + `.factory/**`, merged inside the function itself rather than at each call site.** A + normal build's push check (`src/watch.ts`) relied entirely on the repo's own + `protectedPaths`, which defaults to `[]`; `guard-paths.sh` already hardcoded both paths + unconditionally for interactive edits, so a Bash-made edit outside that hook could still + reach a push. Baking the merge into `touchesProtectedPath` itself, rather than patching + the build-stage call site alone, also closes the same gap at `src/merge-policy.ts`'s + `autoEligible`, a second call site the first fix missed. + +Structural tests: a learning PR outside its allowed paths is refused before any push +(`tests/learn.test.ts`); the lessons file cap holds (`tests/context-pack.test.ts`); a +normal build still cannot touch `.claude/**` with the default empty `protectedPaths` +(`tests/scenarios.test.ts`), and neither can an auto-merge-eligible PR +(`tests/boundary.test.ts`, covering both `touchesProtectedPath` call sites at once). + +Not in v2.10.0: +- A live run of the retro stage and `factory learn` against splitbill-demo: the + currently-installed production `factory` predates this branch, so a genuine run needs a + reset to a clean baseline first, the same category of blocker as v2.7.0's live run, + pending your reset OK. +- Mutual-exclusion enforcement between a lesson and a skill edit on one retro row: the + retro skill's own instructions say "propose at most one," but nothing in `src/schemas.ts` + or `src/learn.ts` would refuse a row carrying both. Left as a prompt-level rule, since no + run has produced one yet. + ## v2.9.0 The review inbox stops sending you to GitHub. A PR waiting on you now shows its diff, gate diff --git a/bin/factory b/bin/factory index 8b01491..4abe041 100755 --- a/bin/factory +++ b/bin/factory @@ -18,7 +18,7 @@ import { BinaryRunner, ClaudeRechecker } from "../src/recheck"; 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 { act, buildInbox, inboxPositionals, learningPrItems, type InboxAction } from "../src/inbox"; import { reset, rebaseline } from "../src/reset"; import { runDoctor, fixDoctor } from "../src/doctor"; import { loadMachineConfig, MachineLeases, MachineSpend } from "../src/machine"; @@ -30,6 +30,7 @@ import { ensureRepoClone } from "../src/repo"; import { helpText } from "../src/help"; import { FixtureRecorder } from "../src/agents/record"; import { configFor, formatReport, reportFor } from "../src/verify-agent"; +import { runLearn } from "../src/learn"; const args = process.argv.slice(2); const command = args[0]; @@ -182,7 +183,7 @@ async function cmdInbox(): Promise { const repo = flag("repo") ?? process.env.FACTORY_REPO; if (!repo) throw new UsageError("inbox: --repo (or FACTORY_REPO) is required"); const github = new GitHub(); - const items = buildInbox(await github.listOpenIssues(repo)); + const items = [...buildInbox(await github.listOpenIssues(repo)), ...learningPrItems(await github.listPrs(repo, { state: "open" }))]; const [issueArg, actionArg] = inboxPositionals(args); if (issueArg) { const item = items.find((i) => i.issue === Number(issueArg)); @@ -400,6 +401,22 @@ async function cmdVerifyAgent(): Promise { if (!report.pass) process.exit(EXIT.runFailed); } +async function cmdLearn(): Promise { + const cloneDir = await resolveCloneDir(); + const config = await loadConfig(cloneDir); + const deps = buildWatchDeps(cloneDir, config); + const result = await runLearn(deps, config.repo, config.base); + if (has("json")) { + console.log(successJson(result, true)); + return; + } + if (result.batched === 0) { + console.log("factory learn: nothing pending."); + return; + } + console.log(`factory learn: batched ${result.batched} proposal(s) on ${result.branch} -> ${result.prUrl}`); +} + async function cmdInstall(): Promise { const target = args[1]; if (!target) throw new UsageError("install: usage: factory install [--dry-run] [--update] [--ci] [--agents a,b,c]"); @@ -445,6 +462,8 @@ async function main(): Promise { return cmdInstall(); case "verify-agent": return cmdVerifyAgent(); + case "learn": + return cmdLearn(); default: console.log(helpText()); if (command && !["help", "--help", "-h"].includes(command)) process.exit(EXIT.error); diff --git a/dashboard/server.ts b/dashboard/server.ts index 06976ad..8b74b2e 100644 --- a/dashboard/server.ts +++ b/dashboard/server.ts @@ -15,7 +15,7 @@ import { FactoryState, DEFAULT_DB_PATH } from "../src/state"; import { parseChatOps } from "../src/chatops"; import { buildBoard } from "./board"; import { LABEL } from "../src/labels"; -import { InboxError, act, buildInbox, type InboxAction } from "../src/inbox"; +import { InboxError, act, buildInbox, learningPrItems, type InboxAction } from "../src/inbox"; import { plain } from "../src/display"; import { agentCatalog } from "../src/agents/docs"; import { agentChecks } from "../src/doctor"; @@ -88,6 +88,8 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin const sessions = new Map(); const issuesCache = new Map> }>(); const issuesInflight = new Map>>>(); + const prsCache = new Map> }>(); + const prsInflight = new Map>>>(); // The Agents page shows each configured agent's installed version and doctor rows. // Probing spawns `--version`, so it is cached for a minute and shared by concurrent requests. @@ -145,6 +147,30 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin issuesCache.delete(r); } + // Same single-flight + cache pattern as cachedIssuesFor, for the inbox's + // learning-PR items (plan v2.10.0 item 4), which come from `gh pr list`, + // not issues. + async function cachedPrsFor(r: string) { + const now = Date.now(); + const cached = prsCache.get(r); + if (cached && now - cached.at < BOARD_CACHE_MS) return cached.prs; + const inflight = prsInflight.get(r); + if (inflight) return inflight; + const promise = github + .listPrs(r, { state: "open" }) + .then((prs) => { + prsCache.set(r, { at: Date.now(), prs }); + prsInflight.delete(r); + return prs; + }) + .catch((err) => { + prsInflight.delete(r); + throw err; + }); + prsInflight.set(r, promise); + return promise; + } + async function cachedIssues() { return cachedIssuesFor(repo); } @@ -360,7 +386,13 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin const decision = decideOperatorMerge(readiness, ci); const merged = await attemptMerge(github, r, pr.number, decision); await github.commentIssue(r, issueNumber, renderOperatorAuditComment(decision)); - if (merged) invalidateIssuesCache(r); + // The dashboard has no Executor to run retro itself (plan v2.10.0 item + // 3); queueRetro just records the row, and watch.ts's poll loop + // (runQueuedRetros) drains it on its next tick. + if (merged) { + invalidateIssuesCache(r); + state.queueRetro(r, issueNumber, "merged"); + } return json({ ok: merged, decision }); }, }, @@ -425,7 +457,12 @@ export function createDashboard(state: FactoryState, github: GitHub, repo: strin const filter = url.searchParams.get("repo"); const repos = filter ? [filter] : inboxRepos(); if (repos.length === 0) return json({ error: "no repo configured or discovered" }, { status: 500 }); - const perRepo = await Promise.all(repos.map(async (r) => buildInbox(await cachedIssuesFor(r)).map((i) => ({ ...i, repo: r })))); + const perRepo = await Promise.all( + repos.map(async (r) => [ + ...buildInbox(await cachedIssuesFor(r)).map((i) => ({ ...i, repo: r })), + ...learningPrItems(await cachedPrsFor(r)).map((i) => ({ ...i, repo: r })), + ]), + ); const items = perRepo.flat().sort((a, b) => (a.waitingSince ?? "").localeCompare(b.waitingSince ?? "") || a.issue - b.issue); return json({ repos: inboxRepos(), repo, items }); }, diff --git a/install.sh b/install.sh index be01a41..b887356 100755 --- a/install.sh +++ b/install.sh @@ -104,8 +104,8 @@ while IFS= read -r -d '' file; do continue fi - # Repo-owned files: the charter and config example are the repo's to edit. - if [[ "$rel" == ".factory/charter.md" || "$rel" == ".factory/config.example.json" ]] && [[ -e "$dest" || -L "$dest" ]]; then + # Repo-owned files: the charter, config example and accumulated lessons are the repo's to edit. + if [[ "$rel" == ".factory/charter.md" || "$rel" == ".factory/config.example.json" || "$rel" == ".factory/memory/lessons.md" ]] && [[ -e "$dest" || -L "$dest" ]]; then echo "skip (repo-owned): $rel" skipped=$((skipped + 1)) continue diff --git a/package.json b/package.json index 0e41353..d1551bd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "software-factory", - "version": "2.9.0", + "version": "2.10.0", "private": true, "type": "module", "description": "GitHub-native SDLC loop for coding agents: triage, plan, build, verify, PR.", diff --git a/research/agents/claude/memory.md b/research/agents/claude/memory.md new file mode 100644 index 0000000..9a99aad --- /dev/null +++ b/research/agents/claude/memory.md @@ -0,0 +1,59 @@ +# Headless Claude auto-memory, under the factory's own flags + +v2.10.0 item 1. Answers where headless Claude reads and writes auto-memory when invoked +with the factory's exact flags, and whether the factory can rely on it. It cannot; this +is why item 2 puts memory in the repo instead. + +## Test + +Local `claude` binary, v2.1.283. From a fresh, throwaway `git init` repo (no +`.claude/`, no `CLAUDE.md`), ran the same flags `claudeArgs` builds in +`src/agents/presets/claude.ts`: + +``` +claude -p "Reply with exactly one line: MEMORY_INSTRUCTIONS=yes if your system prompt or +context mentions any auto-memory system, a memory directory, RTK, or engineering.md; +otherwise MEMORY_INSTRUCTIONS=no. Do not use any tools." \ + --output-format stream-json --verbose \ + --setting-sources project,local \ + --no-session-persistence \ + --permission-mode dontAsk +``` + +`--setting-sources project,local` deliberately excludes `user`; `--no-session-persistence` +should mean no transcript is kept. + +## Result + +- The model answered `MEMORY_INSTRUCTIONS=yes`: it saw the invoking user's global + `~/.claude/CLAUDE.md` (RTK, engineering.md) despite `user` being excluded from + `--setting-sources`. +- The CLI's own `system/init` event carried a live path: + `"memory_paths":{"auto":"/Users/param/.claude/projects//memory/"}`, + where `` encodes the probe's absolute working directory. +- That directory exists on disk (created by the CLI on startup) but is empty: this run + wrote nothing into it, only reserved the path. + +## What this means + +- **`--setting-sources` gates settings files, not the auto-memory/global-instructions + mechanism.** That mechanism sits outside the user/project/local settings hierarchy and + stayed active even with `user` excluded and session persistence off. +- **The memory path is keyed by a hash of the absolute working directory**, under the + *invoking user's own* `~/.claude/projects/`. Two consequences for the factory: + - A worktree at `factory/.worktrees/issue-N` gets its own distinct memory directory, + unrelated to any other worktree's, the main checkout's, or a prior run's for the same + issue once the worktree path changes. Nothing coalesces. + - The directory lives under the operator's home directory. It is not in the repo, not + portable to a different machine (a VPS runner), and not visible in CI. +- This is operator-machine-local state, not repo state. It cannot be the factory's memory + mechanism: it cannot be read back by a run on a different machine, doesn't survive a + worktree being torn down and recreated under a fresh path, and isn't inspectable in a PR + diff for review. + +## Conclusion + +Confirms the roadmap's pre-decided design: **the factory never relies on `~/.claude`**. +Persistent lessons must live in the repo itself, at `.factory/memory/lessons.md` +(v2.10.0 item 2), injected through `buildContextPack` (`src/context.ts`) the same way for +every machine, worktree and CI run alike. diff --git a/src/agents/docs.ts b/src/agents/docs.ts index 20decf1..c6e83a0 100644 --- a/src/agents/docs.ts +++ b/src/agents/docs.ts @@ -36,7 +36,7 @@ export function renderAgentsDoc(doc: string): string { return `${doc.slice(0, start + TABLE_START.length)}\n${agentsTable()}\n${doc.slice(end)}`; } -const STAGE_NAMES = ["triage", "plan", "build", "verify", "pr"] as const; +const STAGE_NAMES = ["triage", "plan", "build", "verify", "pr", "retro"] as const; export interface AgentRow { readonly name: string; diff --git a/src/artifacts.ts b/src/artifacts.ts index 4db7802..a4c73da 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -80,6 +80,17 @@ export interface VerdictArtifact { readonly criteria?: Criterion[]; } +// v2.10.0 item 3: read-only, runs after a final outcome. At most one proposal +// (neo's rule of at most one task per review); all three fields absent means +// "nothing worth changing". +export interface RetroArtifact { + readonly outcome?: StepOutcome; + readonly summary?: string; + readonly lesson?: string; + readonly skill_name?: string; + readonly skill_edit?: string; +} + // A step result is one JSON object of at most 16 KiB (machinist workflow.go). export const MAX_STEP_JSON_BYTES = 16 * 1024; export const VERDICT_KEY_LIST = ["result", "rounds", "findings", "criteria", "outcome", "summary"] as const; @@ -106,6 +117,7 @@ export const STEP_KEYS: Record, readonly string plan: ["status", "risk", "revision", "files", "autoApproveEligible", "commentId", "proof", "outcome", "summary"], build: ["status", "gate_line", "rounds", "outcome", "summary"], pr: ["outcome", "summary"], + retro: ["lesson", "skill_name", "skill_edit", "outcome", "summary"], }; // `types` bounds triage's `type` enum to the repo's actual type list @@ -173,7 +185,7 @@ export function validateVerdict(raw: unknown): { ok: true; verdict: VerdictArtif return { ok: true, verdict }; } -export type ArtifactStage = "triage" | "plan" | "build" | "verify" | "pr"; +export type ArtifactStage = "triage" | "plan" | "build" | "verify" | "pr" | "retro"; export function runDir(issue: number): string { return `.factory/runs/issue-${issue}`; @@ -188,6 +200,7 @@ export const COMMENT_FILENAMES: Record = { build: "status-comment.md", verify: "verdict-comment.md", pr: "pr-body.md", + retro: "retro-comment.md", }; export const JSON_FILENAMES: Record = { @@ -196,6 +209,7 @@ export const JSON_FILENAMES: Record = { build: "build.json", verify: "verdict.json", pr: "pr.json", + retro: "retro.json", }; // Undefined for a missing file, and also for one that is too big, not JSON, or diff --git a/src/boundary.ts b/src/boundary.ts index abf1120..e869ea0 100644 --- a/src/boundary.ts +++ b/src/boundary.ts @@ -4,10 +4,22 @@ // independently diffs the branch against its base and refuses to ship if a // protected path was touched by any means. +// Hardcoded regardless of repo config, mirroring guard-paths.sh's own +// unconditional additions (template/.claude/hooks/guard-paths.sh): a normal +// build must never touch the factory's own state or skills, even with the +// default empty protectedPaths. Merged inside touchesProtectedPath itself, +// not at each call site, so a new caller can never forget it (one resolver +// per concept: this repo already has two call sites, src/watch.ts and +// src/merge-policy.ts). `factory learn`'s branch never calls this check (it +// uses outsideAllowedPaths instead), so its exemption never weakens this for +// a normal build. +export const ALWAYS_PROTECTED_PATHS = [".claude/**", ".factory/**"] as const; + export function touchesProtectedPath(changedFiles: readonly string[], protectedPaths: readonly string[]): string[] { + const patterns = [...protectedPaths, ...ALWAYS_PROTECTED_PATHS]; const hits: string[] = []; for (const file of changedFiles) { - for (const pattern of protectedPaths) { + for (const pattern of patterns) { if (new Bun.Glob(pattern).match(file)) { hits.push(file); break; @@ -16,3 +28,16 @@ export function touchesProtectedPath(changedFiles: readonly string[], protectedP } return hits; } + +// Inverse check for `factory learn` (plan v2.10.0 item 4): a learning PR is +// exempt from protectedPaths but is only ever allowed to touch its own two +// globs, so this refuses anything outside them rather than checking for a hit. +export function outsideAllowedPaths(changedFiles: readonly string[], allowedPaths: readonly string[]): string[] { + const misses: string[] = []; + for (const file of changedFiles) { + if (!allowedPaths.some((pattern) => new Bun.Glob(pattern).match(file))) { + misses.push(file); + } + } + return misses; +} diff --git a/src/config.ts b/src/config.ts index 218491f..1fa7042 100644 --- a/src/config.ts +++ b/src/config.ts @@ -20,6 +20,7 @@ export interface StageBudgets { readonly build: number; readonly verify: number; readonly pr: number; + readonly retro: number; } // Extra Bash patterns the repo grants its agents, on top of the read-only @@ -116,7 +117,7 @@ export const DEFAULT_CONFIG: FactoryConfig = { maxOpenFactoryPrs: 3, concurrency: 3, pollIntervalSeconds: 15, - maxBudgetUsd: { triage: 1, plan: 2, build: 5, verify: 3, pr: 1 }, + maxBudgetUsd: { triage: 1, plan: 2, build: 5, verify: 3, pr: 1, retro: 0.5 }, baselineTag: "baseline", base: "main", stageTimeoutMinutes: 15, @@ -207,7 +208,7 @@ export function configProblems(raw: unknown): string[] { if (v !== undefined && kindOk(v, "object") && !Array.isArray(v)) checkKeys(v as Record, shape, `${key}.`, problems); }; nested("riskPolicy", { autoApproveLowRisk: "boolean" }); - nested("maxBudgetUsd", { triage: "positive", plan: "positive", build: "positive", verify: "positive", pr: "positive" }); + nested("maxBudgetUsd", { triage: "positive", plan: "positive", build: "positive", verify: "positive", pr: "positive", retro: "positive" }); nested("spend", { perIssueUsd: "positive", dailyUsd: "positive", maxUnreportedRuns: "posInt" }); nested("merge", { policy: "string", autoPaths: "strings", maxFiles: "posInt", maxLines: "posInt" }); if (cfg.merge !== undefined && kindOk(cfg.merge, "object")) { @@ -228,7 +229,7 @@ export function configProblems(raw: unknown): string[] { return problems; } -const STAGE_KEYS = ["default", "triage", "plan", "build", "verify", "pr"]; +const STAGE_KEYS = ["default", "triage", "plan", "build", "verify", "pr", "retro"]; // Triage runs before an issue's type is known, so a route can never override it. const ROUTABLE_STAGE_KEYS = STAGE_KEYS.filter((s) => s !== "triage" && s !== "default"); diff --git a/src/context.ts b/src/context.ts index dee3986..bf36254 100644 --- a/src/context.ts +++ b/src/context.ts @@ -11,6 +11,13 @@ import { stripFrontmatter } from "./agents/prompt"; export const MAX_CONTEXT_PACK_BYTES = 64 * 1024; +// v2.10.0 item 2: repo-local memory across runs. Capped well under the +// context pack's own budget so a growing lessons file can never crowd out +// AGENTS.md or the plan's own files; `factory learn` (v2.10.0 item 4) enforces +// the same cap when it appends, so reading here should rarely need to trim. +export const LESSONS_PATH = ".factory/memory/lessons.md"; +export const MAX_LESSONS_BYTES = 8 * 1024; + interface Section { readonly label: string; readonly body: string; @@ -23,6 +30,24 @@ async function readOptional(path: string): Promise { return text.trim() ? text : undefined; } +// Keeps the most recent lessons (the tail of the file, whole lines only) when +// over the cap, since a newer lesson is more likely to still apply than an +// older one it may have superseded. +export function capLessons(raw: string): string { + if (Buffer.byteLength(raw) <= MAX_LESSONS_BYTES) return raw; + const lines = raw.split("\n"); + const kept: string[] = []; + let used = 0; + for (let i = lines.length - 1; i >= 0; i--) { + const line = lines[i] ?? ""; + const bytes = Buffer.byteLength(`${line}\n`); + if (used + bytes > MAX_LESSONS_BYTES) break; + kept.unshift(line); + used += bytes; + } + return `Older lessons dropped for the ${MAX_LESSONS_BYTES / 1024} KiB lessons-file cap.\n\n${kept.join("\n")}`; +} + async function planFiles(cwd: string, issue: number): Promise { const { json } = await readStageArtifacts(cwd, issue, "plan"); const files = (json as Partial | undefined)?.files; @@ -34,6 +59,9 @@ async function sections(issue: number, cwd: string, skills: readonly string[]): const agentsMd = await readOptional(`${cwd}/AGENTS.md`); if (agentsMd) out.push({ label: "AGENTS.md", body: agentsMd.trim() }); + const lessons = await readOptional(`${cwd}/${LESSONS_PATH}`); + if (lessons) out.push({ label: "lessons learned", body: capLessons(lessons).trim() }); + for (const name of skills) { const body = await readOptional(`${cwd}/.claude/skills/${name}/SKILL.md`); if (body) out.push({ label: `skill: ${name}`, body: stripFrontmatter(body).trim() }); diff --git a/src/doctor.ts b/src/doctor.ts index ef5bb48..e324917 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -128,7 +128,7 @@ export async function runDoctor(deps: DoctorDeps, ctx: DoctorContext): Promise stages[st] ?? stages.default ?? "claude")); for (const name of [...used].sort()) checks.push(...(await agentChecks(name, agents, deps))); checks.push({ diff --git a/src/executor.ts b/src/executor.ts index c45e20d..32ee2da 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -7,7 +7,7 @@ import type { AgentCommands } from "./config"; import { truncateFinalMessage } from "./agents/final-message"; import { parseStreamJsonLine } from "./agents/presets/claude"; -export type StageName = "triage" | "plan" | "build" | "verify" | "pr"; +export type StageName = "triage" | "plan" | "build" | "verify" | "pr" | "retro"; export interface StageEvent { readonly kind: "tool_use" | "text" | "usage" | "result" | "truncated"; diff --git a/src/git.ts b/src/git.ts index 25f82ce..05d0130 100644 --- a/src/git.ts +++ b/src/git.ts @@ -85,6 +85,23 @@ export class Git { return this.ensureWorktree(cloneDir, worktreeDir, issue); } + // Generic (non-issue) counterpart to ensureWorktree, for `factory learn` + // (plan v2.10.0 item 4): reuses `branch` if origin already has one (a + // same-day run resuming after a restart), otherwise branches it fresh off + // origin/base, since there is no issue number to derive the name from. + async ensureBranchWorktree(cloneDir: string, worktreeDir: string, branch: string, base: string): Promise { + await this.runner.run(["fetch", "origin", base], { cwd: cloneDir }); + const list = await this.runner.run(["worktree", "list", "--porcelain"], { cwd: cloneDir }); + if (list.stdout.includes(`worktree ${worktreeDir}\n`)) return; + const remote = await this.runner.run(["ls-remote", "--heads", "origin", branch], { cwd: cloneDir }); + if (remote.stdout.trim()) { + await this.runner.run(["fetch", "origin", branch], { cwd: cloneDir }); + await this.runner.run(["worktree", "add", worktreeDir, branch], { cwd: cloneDir }); + } else { + await this.runner.run(["worktree", "add", "-b", branch, worktreeDir, `origin/${base}`], { cwd: cloneDir }); + } + } + async removeWorktree(cloneDir: string, worktreeDir: string): Promise { await this.runner.run(["worktree", "remove", "--force", worktreeDir], { cwd: cloneDir }); } @@ -105,7 +122,11 @@ export class Git { // Regular push only; the guard hook and settings.json refuse --force and merge. async push(worktreeDir: string, issue: number): Promise { - const branch = this.branchName(issue); + return this.pushBranch(worktreeDir, this.branchName(issue)); + } + + // Generic (non-issue) counterpart to push, for `factory learn`'s branch. + async pushBranch(worktreeDir: string, branch: string): Promise { return this.runner.run(["push", "origin", `HEAD:refs/heads/${branch}`], { cwd: worktreeDir }); } diff --git a/src/help.ts b/src/help.ts index 80bf537..e987b0a 100644 --- a/src/help.ts +++ b/src/help.ts @@ -15,6 +15,7 @@ export const COMMANDS: { name: string; usage: string; does: string }[] = [ { name: "rebaseline", usage: "rebaseline --repo-dir [--dry-run]", does: "move the baseline tag to origin/, keeping merged setup changes across reset" }, { name: "doctor", usage: "doctor --repo-dir [--fix]", does: "check the loop can run; --fix creates missing labels" }, { name: "verify-agent", usage: "verify-agent --repo-dir --issue [--out ]", does: "run one issue on that agent, record a scrubbed fixture, print pass/fail, cost and tokens" }, + { name: "learn", usage: "learn (--repo-dir | --repo )", does: "batch pending retro lessons and skill-edit proposals into one factory/learning- PR" }, { name: "install", usage: "install [--dry-run] [--update] [--ci] [--agents a,b,c]", does: "install or update the template in a repo; --agents links skills into each agent dir" }, ]; diff --git a/src/inbox.ts b/src/inbox.ts index 6edd3d4..02a32a2 100644 --- a/src/inbox.ts +++ b/src/inbox.ts @@ -6,10 +6,10 @@ import { parseChatOps } from "./chatops"; import { parseDataMarkers } from "./derive"; import { plain } from "./display"; -import type { GhComment, GhIssue, GitHub } from "./github"; +import type { GhComment, GhIssue, GhPr, GitHub } from "./github"; import { LABEL, PARKED_LABELS } from "./labels"; -export type InboxKind = "approve-plan" | "answer-question" | "review-pr" | "merge-dry-run" | "parked" | "failed" | "budget"; +export type InboxKind = "approve-plan" | "answer-question" | "review-pr" | "merge-dry-run" | "parked" | "failed" | "budget" | "learning-pr"; export type InboxAction = "approve" | "revise" | "answer" | "retry" | "cancel"; // Actions that carry the human's own words. @@ -91,6 +91,25 @@ export function buildInbox(issues: readonly GhIssue[]): InboxItem[] { return items.sort((a, b) => (a.waitingSince ?? "").localeCompare(b.waitingSince ?? "") || a.issue - b.issue); } +// `factory learn` (plan v2.10.0 item 4) opens a PR with no linked GitHub +// issue, so it can't derive from labels/comments like buildInbox. Visibility +// only: no chatops actions exist for a learning PR, it never auto-merges. +export function learningPrItems(prs: readonly GhPr[]): InboxItem[] { + return prs + .filter((pr) => pr.headRefName.startsWith("factory/learning-")) + .map((pr) => ({ + id: `learning-pr-${pr.number}`, + kind: "learning-pr" as const, + issue: pr.number, + title: `Learning PR #${pr.number}`, + label: "", + waitingSince: undefined, + ask: `Batched memory/skill-edit proposals: ${pr.url}`, + actions: [] as const, + })) + .sort((a, b) => a.issue - b.issue); +} + export function commandText(action: InboxAction, text: string): string { if (action === "answer") return text; return action === "revise" ? `/factory revise ${text}` : `/factory ${action}`; diff --git a/src/learn.ts b/src/learn.ts new file mode 100644 index 0000000..0015b63 --- /dev/null +++ b/src/learn.ts @@ -0,0 +1,123 @@ +// `factory learn` (plan v2.10.0 item 4): batches pending retro proposals into +// one PR on a daily learning branch. Deterministic, no agent: the retro stage +// already made the judgment (what lesson, what skill edit); this just +// durably exposes it for human review. It never auto-merges. + +import { outsideAllowedPaths } from "./boundary"; +import { capLessons, LESSONS_PATH } from "./context"; +import type { Git } from "./git"; +import type { GitHub } from "./github"; +import type { FactoryState, RetroRow } from "./state"; + +// The one exemption from config.protectedPaths, and only for this branch +// kind: a learning PR may touch memory and skill-proposal files, nothing else. +export const LEARNING_ALLOWED_PATHS = [".factory/memory/**", ".claude/skills/**"] as const; + +export interface LearnDeps { + readonly github: GitHub; + readonly git: Git; + readonly state: FactoryState; + readonly cloneDir: string; + readonly workspacesDir: string; +} + +export interface LearnResult { + readonly batched: number; + readonly prUrl?: string; + readonly branch?: string; +} + +export function branchNameFor(when: Date): string { + const y = when.getUTCFullYear(); + const m = String(when.getUTCMonth() + 1).padStart(2, "0"); + const d = String(when.getUTCDate()).padStart(2, "0"); + return `factory/learning-${y}${m}${d}`; +} + +async function readIfExists(path: string): Promise { + const file = Bun.file(path); + return (await file.exists()) ? file.text() : ""; +} + +// Appends new lessons to the existing file and re-applies the same cap +// buildContextPack's reader enforces (src/context.ts), so the file this +// writes is never the thing that forces a trim at read time. +async function appendLessons(worktreeDir: string, rows: readonly RetroRow[]): Promise { + const lines = rows.filter((r) => r.lesson).map((r) => `- ${r.lesson} (issue #${r.issue})`); + if (lines.length === 0) return; + const path = `${worktreeDir}/${LESSONS_PATH}`; + const existing = (await readIfExists(path)).trim(); + const merged = existing ? `${existing}\n${lines.join("\n")}\n` : `${lines.join("\n")}\n`; + await Bun.write(path, capLessons(merged)); +} + +// One proposal file per named skill, never the real SKILL.md: incorporating +// the wording change is left to a human during PR review. +async function appendProposedEdits(worktreeDir: string, rows: readonly RetroRow[]): Promise { + const skillNames = new Set(rows.filter((r): r is RetroRow & { skill_name: string } => Boolean(r.skill_name)).map((r) => r.skill_name)); + for (const name of skillNames) { + const path = `${worktreeDir}/.claude/skills/${name}/PROPOSED_EDITS.md`; + const entries = rows + .filter((r) => r.skill_name === name && r.skill_edit) + .map((r) => `## From issue #${r.issue} (${r.outcome})\n\n${r.skill_edit}\n`); + if (entries.length === 0) continue; + const existing = (await readIfExists(path)).trim(); + const merged = existing ? `${existing}\n\n${entries.join("\n")}` : entries.join("\n"); + await Bun.write(path, `${merged.trim()}\n`); + } +} + +function buildPrBody(rows: readonly RetroRow[]): string { + const lines = rows.map((r) => `- #${r.issue} (${r.outcome}): ${r.lesson ?? `skill edit proposed for \`${r.skill_name}\``}`); + return [ + `Batches ${rows.length} pending retro proposal(s) into repo-local memory and skill-edit proposals.`, + "", + ...lines, + "", + "Nothing here is applied automatically; review the lesson and any `PROPOSED_EDITS.md` files before merging.", + ].join("\n"); +} + +// Idempotent per day: a second run the same day reuses the branch and PR, and +// only ever batches rows still pending (markLearned stops a row being batched +// twice). Throws, without pushing or marking anything learned, if the batch +// would touch anything outside LEARNING_ALLOWED_PATHS: the one case that +// should never happen since only src/state.ts and this file write these +// paths, but is checked anyway as defense in depth (mirrors +// touchesProtectedPath's use in watch.ts). +export async function runLearn(deps: LearnDeps, repo: string, base: string, now: Date = new Date()): Promise { + const pending = deps.state.pendingLessons(repo); + if (pending.length === 0) return { batched: 0 }; + + const branch = branchNameFor(now); + const worktreeDir = `${deps.workspacesDir}/${branch.replace(/\//g, "-")}`; + await deps.git.ensureBranchWorktree(deps.cloneDir, worktreeDir, branch, base); + + await appendLessons(worktreeDir, pending); + await appendProposedEdits(worktreeDir, pending); + + const committed = await deps.git.commitAll(worktreeDir, `factory: batch ${pending.length} retro proposal(s)`); + if (!committed) return { batched: 0 }; + + const changed = await deps.git.changedFiles(worktreeDir, base); + const outside = outsideAllowedPaths(changed, LEARNING_ALLOWED_PATHS); + if (outside.length > 0) { + throw new Error(`factory learn: refusing to push outside its allowed paths: ${outside.join(", ")}`); + } + + await deps.git.pushBranch(worktreeDir, branch); + + const existingPr = await deps.github.findPrByHead(repo, branch); + const prUrl = existingPr + ? existingPr.url + : await deps.github.createPr({ + repo, + base, + head: branch, + title: `factory: learning batch ${branch.replace("factory/learning-", "")}`, + body: buildPrBody(pending), + }); + + deps.state.markLearned(pending.map((r) => r.id), now.toISOString()); + return { batched: pending.length, prUrl, branch }; +} diff --git a/src/schemas.ts b/src/schemas.ts index b2cb467..3171e16 100644 --- a/src/schemas.ts +++ b/src/schemas.ts @@ -45,6 +45,7 @@ function typesOf(stage: ArtifactStage, types: readonly string[]): Record = { build: ["status", "gate_line", "rounds"], verify: ["result", "rounds", "findings"], pr: [], + // Nothing is required: "no proposal" is a valid, and expected, retro. + retro: [], }; // `types` bounds the triage `type` enum: TYPE_LABELS unless the caller knows diff --git a/src/state.ts b/src/state.ts index a4c1589..af75a67 100644 --- a/src/state.ts +++ b/src/state.ts @@ -9,6 +9,31 @@ import { dirname } from "node:path"; import { defaultStatePath } from "./paths"; export type Stage = "triage" | "plan" | "build" | "verify" | "pr"; + +// The final pipeline outcome that triggers a retro (plan v2.10.0 item 3), +// distinct from a stage artifact's own outcome field (complete/blocked/failed). +export type RetroTrigger = "merged" | "rejected" | "gave-up"; +export type RetroStatus = "queued" | "done" | "failed"; + +// One row per final outcome. Queued by all four triggers (checkMergePolicy, +// cancelRun, the verify-reject cap, and the dashboard's operator-merge route); +// only the dashboard trigger, which has no Executor to run retro itself, +// stays "queued" until watch.ts's poll loop drains it. +export interface RetroRow { + id: number; + repo: string; + issue: number; + outcome: RetroTrigger; + status: RetroStatus; + lesson: string | null; + skill_name: string | null; + skill_edit: string | null; + created_at: string; + completed_at: string | null; + // Set once `factory learn` (plan v2.10.0 item 4) has batched this row's + // proposal into a learning PR, so a later run never batches it twice. + learned_at: string | null; +} export type RunStatus = | "running" | "needs-info" @@ -153,6 +178,20 @@ export class FactoryState { key TEXT PRIMARY KEY, value TEXT NOT NULL ); + CREATE TABLE IF NOT EXISTS retros ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo TEXT NOT NULL, + issue INTEGER NOT NULL, + outcome TEXT NOT NULL, + status TEXT NOT NULL, + lesson TEXT, + skill_name TEXT, + skill_edit TEXT, + created_at TEXT NOT NULL, + completed_at TEXT, + learned_at TEXT + ); + CREATE INDEX IF NOT EXISTS retros_repo_status ON retros(repo, status); `); // 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)); @@ -375,6 +414,66 @@ export class FactoryState { return { costUsd: row.cost, unreportedRuns: row.unreported }; } + // Called first by all four retro triggers, even the three that then complete + // the row within the same call: one queue, one status path, whether retro + // runs immediately or (the dashboard's merge route) waits for the poll loop. + queueRetro(repo: string, issue: number, outcome: RetroTrigger): number { + this.db + .query("INSERT INTO retros (repo, issue, outcome, status, created_at) VALUES ($repo, $issue, $outcome, 'queued', $now)") + .run({ $repo: repo, $issue: issue, $outcome: outcome, $now: new Date().toISOString() }); + return (this.db.query("SELECT last_insert_rowid() AS id").get() as { id: number }).id; + } + + completeRetro(id: number, proposal?: { lesson?: string; skill_name?: string; skill_edit?: string }, status: "done" | "failed" = "done"): void { + this.db + .query( + `UPDATE retros SET status = $status, lesson = $lesson, skill_name = $skill_name, skill_edit = $skill_edit, completed_at = $now WHERE id = $id`, + ) + .run({ + $id: id, + $status: status, + $lesson: proposal?.lesson ?? null, + $skill_name: proposal?.skill_name ?? null, + $skill_edit: proposal?.skill_edit ?? null, + $now: new Date().toISOString(), + }); + } + + // Rows the dashboard's operator-merge route queued but nothing has run yet. + listQueuedRetros(repo: string, limit = 50): RetroRow[] { + return this.db + .query("SELECT * FROM retros WHERE repo = $repo AND status = 'queued' ORDER BY id ASC LIMIT $limit") + .all({ $repo: repo, $limit: limit }) as RetroRow[]; + } + + // Every retro for a repo regardless of status, newest first. `factory learn` + // (plan v2.10.0 item 4) will filter this to done rows with a lesson. + listRetros(repo: string, limit = 50): RetroRow[] { + return this.db.query("SELECT * FROM retros WHERE repo = $repo ORDER BY id DESC LIMIT $limit").all({ $repo: repo, $limit: limit }) as RetroRow[]; + } + + // Done rows with a proposal that `factory learn` (plan v2.10.0 item 4) + // hasn't yet batched into a learning PR. Oldest first, so a batch applies + // lessons in the order they were learned. + pendingLessons(repo: string): RetroRow[] { + return this.db + .query( + "SELECT * FROM retros WHERE repo = $repo AND status = 'done' AND learned_at IS NULL AND (lesson IS NOT NULL OR skill_name IS NOT NULL) ORDER BY id ASC", + ) + .all({ $repo: repo }) as RetroRow[]; + } + + // Stamps the rows a learning PR just batched so a later run never re-batches them. + markLearned(ids: readonly number[], when: string): void { + if (ids.length === 0) return; + const placeholders = ids.map((_, i) => `$id${i}`).join(", "); + const params: Record = { $when: when }; + ids.forEach((id, i) => { + params[`$id${i}`] = id; + }); + this.db.query(`UPDATE retros SET learned_at = $when WHERE id IN (${placeholders})`).run(params); + } + getToggle(key: string, fallback: boolean): boolean { const row = this.db.query("SELECT value FROM toggles WHERE key = $key").get({ $key: key }) as | { value: string } diff --git a/src/watch.ts b/src/watch.ts index c21c01f..3451a35 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -26,6 +26,7 @@ import { runDir, type BuildArtifact, type PlanArtifact, + type RetroArtifact, type TriageArtifact, type VerdictArtifact, } from "./artifacts"; @@ -41,7 +42,7 @@ import { runPool } from "./pool"; import type { GhComment, GhIssue, GitHub } from "./github"; import type { Git } from "./git"; import { ensureSetup, ShellSetupRunner, type SetupRunner } from "./setup"; -import { FactoryState, type Stage } from "./state"; +import { FactoryState, type RetroTrigger, type Stage } from "./state"; import { LABEL, issueType, typesFor } from "./labels"; import { effectiveSlots, type MachineConfig, type MachineLeases, type MachineSpend } from "./machine"; @@ -218,6 +219,19 @@ function stageFailure(result: StageRunResult, fallback?: string): string | undef return result.killedReason ?? denied ?? result.stderrTail ?? fallback; } +// The agent's own cost wins; otherwise price the tokens. No price means the +// cost is unknown, which is stored as NULL with usage_complete = 0, never as $0. +function priceResult(result: StageRunResult): { cached: number; costUsd: number | null; usageComplete: boolean } { + 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 ?? null) : null; + return { cached, costUsd, usageComplete }; +} + async function runStage( deps: WatchDeps, config: FactoryConfig, @@ -250,15 +264,7 @@ async function runStage( }); 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 NULL with usage_complete = 0, never as $0. - 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 ?? null) : null; + const { cached, costUsd, usageComplete } = priceResult(result); deps.state.recordStageRun({ repo: config.repo, issue: issueNumber, @@ -287,6 +293,76 @@ async function runStage( return result; } +// Plan v2.10.0 item 3: a read-only stage after a final outcome (merged, +// rejected, or given up on after verify rejections). Never goes through +// runStage: that wrapper's upsertRun would put an already-terminal run back +// to "running", and retro's cost never rolls into runs.cost_usd (spendSummary +// sums stage_runs, not runs, so the spend caps still see it via recordStageRun). +// `existingId` is set only when a row was queued earlier (the dashboard's +// operator-merge route, which has no Executor to run this itself) and is now +// being drained by runQueuedRetros; the three watch.ts triggers queue and run +// in the same call. +async function runRetro(deps: WatchDeps, config: FactoryConfig, issue: GhIssue, outcome: RetroTrigger, existingId?: number): Promise { + const id = existingId ?? deps.state.queueRetro(config.repo, issue.number, outcome); + const issueNumber = issue.number; + const worktree = worktreeFor(deps, issueNumber); + try { + await rehydrate(worktree, issue); + await writeIssueSnapshot(worktree, issue); + await clearStageArtifacts(worktree, issueNumber, "retro"); + const startedAt = new Date(); + const result = await deps.executor.runStage({ + stage: "retro", + issue: issueNumber, + cwd: worktree, + maxBudgetUsd: config.maxBudgetUsd.retro, + timeoutMinutes: config.stageTimeoutMinutes, + maxToolCalls: config.maxToolCalls, + agentCommands: config.agentCommands, + }); + const finishedAt = new Date(); + const { cached, costUsd, usageComplete } = priceResult(result); + deps.state.recordStageRun({ + repo: config.repo, + issue: issueNumber, + stage: "retro" as Stage, + agent: result.agent ?? "claude", + model: result.model ?? null, + started_at: startedAt.toISOString(), + finished_at: finishedAt.toISOString(), + duration_ms: finishedAt.getTime() - startedAt.getTime(), + tool_calls: result.toolCalls, + tokens_in: result.tokensIn, + tokens_out: result.tokensOut, + tokens_cached: cached, + cost_usd: costUsd, + usage_complete: usageComplete ? 1 : 0, + exit_code: result.exitCode, + killed_reason: result.killedReason ?? null, + }); + if (costUsd !== null) deps.machine?.spend.record(config.repo, costUsd); + const art = await readStageArtifacts(worktree, issueNumber, "retro"); + deps.state.completeRetro(id, art.json as RetroArtifact | undefined); + } catch (err) { + console.error(`retro failed for #${issue.number}:`, err); + deps.state.completeRetro(id, undefined, "failed"); + } +} + +// Drains rows the dashboard's operator-merge route queued but could not run +// itself. Called once per pollOnce tick. +async function runQueuedRetros(deps: WatchDeps, config: FactoryConfig): Promise { + for (const row of deps.state.listQueuedRetros(config.repo)) { + try { + const issue = await deps.github.getIssue(config.repo, row.issue); + await runRetro(deps, config, issue, row.outcome, row.id); + } catch (err) { + console.error(`queued retro failed for #${row.issue}:`, err); + deps.state.completeRetro(row.id, undefined, "failed"); + } + } +} + // 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"; @@ -567,6 +643,7 @@ async function runFromStage( ctx.rejectRound += 1; if (ctx.rejectRound > MAX_VERIFY_REJECTS) { await moveLabel(deps, config, issueNumber, LABEL.verifying, LABEL.needsHuman); + await runRetro(deps, config, issue, "gave-up"); finish(deps, config, issueNumber, "needs-human", `rejected ${ctx.rejectRound} times`); return "needs-human"; } @@ -639,6 +716,7 @@ async function cancelRun(issue: GhIssue, deps: WatchDeps, config: FactoryConfig) if (pr) await deps.github.closePr(config.repo, pr.number); await postComment(deps, config, issue.number, `Cancelled by \`/factory cancel\`. The branch \`${head}\` is kept; label the issue \`${LABEL.ready}\` after deleting it to start over.`); await deps.github.closeIssue(config.repo, issue.number); + await runRetro(deps, config, issue, "rejected"); await deps.git.removeWorktree(deps.cloneDir, worktreeFor(deps, issue.number)); finish(deps, config, issue.number, "cancelled"); return "cancelled"; @@ -764,7 +842,8 @@ async function checkMergePolicy(deps: WatchDeps, config: FactoryConfig, issue: G const marker = mergePolicyMarker(decision.headSha); if (!issue.comments.some((c) => c.body.includes(marker))) await postComment(deps, config, issueNumber, renderAuditComment(decision)); - await attemptMerge(deps.github, config.repo, pr.number, decision); + const merged = await attemptMerge(deps.github, config.repo, pr.number, decision); + if (merged) await runRetro(deps, config, issue, "merged"); } catch (err) { console.error(`merge-policy check failed for #${issue.number}:`, err); } @@ -874,7 +953,16 @@ export async function pollOnce(deps: WatchDeps, config: FactoryConfig, inFlight? return true; }); + // candidates was filtered against inFlight above; mark them before any + // await so an overlapping poll's own candidates filter (same check) never + // sees this batch as still unclaimed. for (const issue of candidates) inFlight?.add(issue.number); + + // Drains retros the dashboard's operator-merge route queued (v2.10.0 item + // 3); independent of intake, so it runs even while stopIf/budgetPaused + // holds back new pickups. + await runQueuedRetros(deps, config); + try { // A pool of `concurrency` workers, not one-at-a-time and not // Promise.all-everything: before this, issue #4 never started until #1's diff --git a/template-ci/factory.yml.example b/template-ci/factory.yml.example index 1f966eb..7244693 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.9.0 # pinned software-factory release; bump deliberately + FACTORY_RUNNER_REF: v2.10.0 # pinned software-factory release; bump deliberately FACTORY_RUNNER_REPO: learnwithparam/software-factory CLAUDE_CODE_VERSION: "2.1.281" # pinned claude, same version the Dockerfile installs # Pins for every agent. The "Install agents" step installs the ones named in the repo VARIABLE diff --git a/template/.claude/skills/factory-retro/SKILL.md b/template/.claude/skills/factory-retro/SKILL.md new file mode 100644 index 0000000..0dda925 --- /dev/null +++ b/template/.claude/skills/factory-retro/SKILL.md @@ -0,0 +1,51 @@ +--- +name: factory-retro +description: Reads a finished run (merged, closed by a human, or given up on after too many verify rejections) and proposes at most one lesson or skill edit, or none. Use as the last stage, after the run has already reached its final outcome; it never changes the run's result. +--- + +# factory-retro + +Invoked as `/factory-retro `. Read-only: no code edit, no push, no `gh` +access. Runs after the outcome is already decided, purely to capture +something worth remembering for next time. + +## 1. Read the inputs + +- `.factory/runs/issue-/issue.json` and every stage artifact and comment + that exists for this run (`triage.json`, `plan.json`, `build.json`, + `verdict.json`, their `-comment.md` files), whatever the run actually + produced before it ended. +- `.factory/memory/lessons.md`, if it exists, so you don't repeat a lesson + already recorded. + +## 2. Decide whether there is a lesson + +Look for one concrete, reusable thing this run's outcome teaches: a plan +that missed an acceptance criterion, a gate that caught something verify's +skill should check earlier, a skill instruction that was ambiguous or +missing and caused a rejection or a human close. Most runs teach nothing +new; saying so is a valid, expected result. + +Propose at most one of: +- **a lesson**: a short, general statement for `.factory/memory/lessons.md` + (not a summary of this specific run); +- **a skill edit**: a named skill (e.g. `factory-build`) and the specific + wording change that would have prevented the outcome. + +Never propose both, and never propose more than one of either. If nothing +rises above "specific to this one issue," propose none. + +## 3. Write the output + +Write `.factory/runs/issue-/retro.json`: + +```json +{ "lesson": "{{lesson_text_or_omit}}", "skill_name": "{{skill_or_omit}}", "skill_edit": "{{edit_text_or_omit}}", "outcome": "complete", "summary": "{{one_line}}" } +``` + +Omit `lesson` and `skill_name`/`skill_edit` entirely when you are proposing +nothing; never propose a lesson and a skill edit together. Then write +`.factory/runs/issue-/retro-comment.md`: one or two sentences on what, if +anything, you propose and why. Nothing here is applied automatically; a +later `factory learn` run reviews proposals like this one and, if it +agrees, opens a PR. diff --git a/template/.factory/config.example.json b/template/.factory/config.example.json index e3bcf6a..a13e4a2 100644 --- a/template/.factory/config.example.json +++ b/template/.factory/config.example.json @@ -21,7 +21,7 @@ "sonnet": { "preset": "claude", "model": "claude-sonnet-5" }, "haiku": { "preset": "claude", "model": "claude-haiku-4-5-20251001" } }, - "stages": { "triage": "haiku", "plan": "opus", "build": "sonnet", "verify": "sonnet", "pr": "haiku" }, + "stages": { "triage": "haiku", "plan": "opus", "build": "sonnet", "verify": "sonnet", "pr": "haiku", "retro": "haiku" }, "routes": { "docs": { "stages": { "build": "haiku" } } } diff --git a/template/.factory/memory/lessons.md b/template/.factory/memory/lessons.md new file mode 100644 index 0000000..ba784b5 --- /dev/null +++ b/template/.factory/memory/lessons.md @@ -0,0 +1,5 @@ +# Lessons learned + +Appended by `factory learn` from retro findings on merged, rejected, or given-up runs +(v2.10.0 item 4). Read into every stage's context pack, capped at 8 KiB; the oldest +lines drop first when it grows past that. This file is yours to edit too. diff --git a/tests/agents-catalog.test.ts b/tests/agents-catalog.test.ts index 30dc9c1..e386248 100644 --- a/tests/agents-catalog.test.ts +++ b/tests/agents-catalog.test.ts @@ -10,7 +10,7 @@ import { createDashboard } from "../dashboard/server"; test("a configured agent lists the stages it serves, and every other preset is listed as not configured", () => { const rows = agentCatalog({ claude: { preset: "claude" }, checker: { preset: "codex" }, mine: { command: ["aider", "--yes"] } }, { default: "claude", verify: "checker" }); const by = Object.fromEntries(rows.map((r) => [r.name, r])); - expect(by.claude!.stages).toEqual(["triage", "plan", "build", "pr"]); + expect(by.claude!.stages).toEqual(["triage", "plan", "build", "pr", "retro"]); expect(by.checker!.stages).toEqual(["verify"]); expect(by.mine).toMatchObject({ preset: null, binary: "aider", pin: null, verified: false, stages: [] }); for (const name of Object.keys(PRESETS)) if (name !== "claude" && name !== "codex") expect(by[name]).toMatchObject({ configured: false, stages: [] }); @@ -18,7 +18,7 @@ test("a configured agent lists the stages it serves, and every other preset is l test("a stage with no agent set is served by claude, as the executor runs it", () => { const rows = agentCatalog({ claude: { preset: "claude" } }, {}); - expect(rows.find((r) => r.name === "claude")!.stages).toEqual(["triage", "plan", "build", "verify", "pr"]); + expect(rows.find((r) => r.name === "claude")!.stages).toEqual(["triage", "plan", "build", "verify", "pr", "retro"]); }); test("only a preset flagged verified shows as verified", () => { @@ -30,7 +30,7 @@ test("GET /api/agents returns the catalog from the configured fleet", async () = const res = await dash.handle(new Request("http://localhost:4100/api/agents"), "127.0.0.1"); expect(res.status).toBe(200); const body = (await res.json()) as { agents: { name: string; stages: string[] }[] }; - expect(body.agents.find((a) => a.name === "claude")!.stages).toHaveLength(5); + expect(body.agents.find((a) => a.name === "claude")!.stages).toHaveLength(6); expect(body.agents.length).toBe(Object.keys(PRESETS).length); }); diff --git a/tests/agents.test.ts b/tests/agents.test.ts index 59bbc71..836f5a1 100644 --- a/tests/agents.test.ts +++ b/tests/agents.test.ts @@ -20,7 +20,9 @@ import { LABEL } from "../src/labels"; import { processReadyIssue } from "../src/watch"; import { baseIssue, FakeGateRunner, FakeGit, FakeGitHub } from "./harness"; -const STAGES: StageName[] = ["triage", "plan", "build", "verify", "pr"]; +const STAGES: StageName[] = ["triage", "plan", "build", "verify", "pr", "retro"]; +// The stages a "shipped" run (no merge, so no retro) actually executes. +const RUN_STAGES: StageName[] = ["triage", "plan", "build", "verify", "pr"]; const scratch = mkdtempSync(join(tmpdir(), "factory-agents-")); afterAll(() => rmSync(scratch, { recursive: true, force: true })); @@ -152,7 +154,7 @@ describe("an agent with no preset", () => { } expect(github.createdPrs[0]!.body).toContain("Closes #1"); // Every stage was handed its contract on stdin. - for (const stage of STAGES) expect(readFileSync(join(log, `${stage}.prompt`), "utf8")).toContain(JSON_FILENAMES[stage]); + for (const stage of RUN_STAGES) expect(readFileSync(join(log, `${stage}.prompt`), "utf8")).toContain(JSON_FILENAMES[stage]); // The run history names the agent and says the numbers are not reported. const rows = state.listStageRuns("acme/widgets"); expect(rows.map((r) => r.agent)).toEqual(Array(rows.length).fill("scripted")); diff --git a/tests/boundary.test.ts b/tests/boundary.test.ts index f2f00a3..a34498b 100644 --- a/tests/boundary.test.ts +++ b/tests/boundary.test.ts @@ -5,7 +5,8 @@ // changed. import { describe, expect, test } from "bun:test"; -import { touchesProtectedPath } from "../src/boundary"; +import { ALWAYS_PROTECTED_PATHS, touchesProtectedPath } from "../src/boundary"; +import { autoEligible } from "../src/merge-policy"; describe("touchesProtectedPath", () => { test("flags an exact-path match", () => { @@ -33,4 +34,35 @@ describe("touchesProtectedPath", () => { test("matches a dotfile pattern like a secrets file", () => { expect(touchesProtectedPath([".env.production"], [".env*"])).toEqual([".env.production"]); }); + + // touchesProtectedPath merges ALWAYS_PROTECTED_PATHS itself (one resolver, + // not a spread every caller must remember), so .claude/** and .factory/** + // are refused for every caller even with the default empty protectedPaths + // config, matching what guard-paths.sh already hardcodes. This covers both + // call sites at once: src/watch.ts's build-stage push check and + // src/merge-policy.ts's autoEligible. + test("ALWAYS_PROTECTED_PATHS flags .claude/** and .factory/** with no repo config at all", () => { + const changed = [".claude/skills/factory-build/SKILL.md", ".factory/memory/lessons.md", "src/ui/button.tsx"]; + expect(touchesProtectedPath(changed, [])).toEqual([".claude/skills/factory-build/SKILL.md", ".factory/memory/lessons.md"]); + }); + + // Explicitly passing ALWAYS_PROTECTED_PATHS too (as a caller migrating from + // the old caller-side merge might still do) must not double-count a hit. + test("passing ALWAYS_PROTECTED_PATHS explicitly alongside the built-in merge does not duplicate hits", () => { + expect(touchesProtectedPath([".claude/x.md"], [...ALWAYS_PROTECTED_PATHS])).toEqual([".claude/x.md"]); + }); + + // The second call site (src/merge-policy.ts's autoEligible, used by the + // "auto" merge policy) gets the same hardening for free because it goes + // through touchesProtectedPath, not a separate check. Proven end to end, + // not just against the shared helper in isolation. + test("a PR touching .claude/** is never auto-eligible, even with an empty repo protectedPaths config", () => { + const refusals = autoEligible( + "low", + [{ path: ".claude/skills/factory-build/SKILL.md", additions: 1, deletions: 0 }], + { autoPaths: [".claude/**"], maxFiles: 10, maxLines: 200 }, + [], + ); + expect(refusals.map((r) => r.reason)).toContain("not-auto-eligible"); + }); }); diff --git a/tests/context-pack.test.ts b/tests/context-pack.test.ts index 47ba4d8..74f0abf 100644 --- a/tests/context-pack.test.ts +++ b/tests/context-pack.test.ts @@ -9,7 +9,7 @@ import { join } from "node:path"; import { claudeArgs } from "../src/agents/presets/claude"; import { renderPrompt } from "../src/agents/prompt"; import { runDir } from "../src/artifacts"; -import { buildContextPack, MAX_CONTEXT_PACK_BYTES } from "../src/context"; +import { buildContextPack, MAX_CONTEXT_PACK_BYTES, MAX_LESSONS_BYTES } from "../src/context"; import { STAGE_GUIDANCE } from "../src/stage-permissions"; function worktree(): string { @@ -78,6 +78,33 @@ describe("buildContextPack", () => { expect(pack).toBe(""); // nothing else to hold, and a missing file is not a "drop" rmSync(cwd, { recursive: true, force: true }); }); + + test("holds .factory/memory/lessons.md (v2.10.0 item 2)", async () => { + const cwd = worktree(); + mkdirSync(`${cwd}/.factory/memory`, { recursive: true }); + writeFileSync(`${cwd}/.factory/memory/lessons.md`, "- 2026-09-01: the checkout API needs a 30s timeout, not the default."); + + const pack = await buildContextPack("build", 1, cwd, []); + + expect(pack).toContain("lessons learned"); + expect(pack).toContain("the checkout API needs a 30s timeout"); + rmSync(cwd, { recursive: true, force: true }); + }); + + test("caps the lessons file on its own budget, keeping the most recent lines", async () => { + const cwd = worktree(); + mkdirSync(`${cwd}/.factory/memory`, { recursive: true }); + const oldLine = `- an old lesson that should drop: ${"x".repeat(MAX_LESSONS_BYTES)}`; + const newLine = "- a recent lesson that must survive the cap"; + writeFileSync(`${cwd}/.factory/memory/lessons.md`, `${oldLine}\n${newLine}\n`); + + const pack = await buildContextPack("build", 1, cwd, []); + + expect(pack).toContain("a recent lesson that must survive the cap"); + expect(pack).not.toContain("an old lesson that should drop"); + expect(pack).toContain("lessons-file cap"); + rmSync(cwd, { recursive: true, force: true }); + }); }); describe("the context pack reaches both agent paths", () => { diff --git a/tests/dashboard-routes.test.ts b/tests/dashboard-routes.test.ts index 1435daa..fe7d535 100644 --- a/tests/dashboard-routes.test.ts +++ b/tests/dashboard-routes.test.ts @@ -20,6 +20,11 @@ class FakeGitHub extends GitHub { override async listOpenIssues(): Promise { return this.issues; } + // The inbox route also merges in learning-PR items (v2.10.0 item 4); no + // fixture here opens one, so an empty list is the right default. + override async listPrs(): Promise { + return []; + } override async getIssue(_repo: string, number: number): Promise { return this.issues.find((i) => i.number === number)!; } diff --git a/tests/harness.ts b/tests/harness.ts index 586b7d9..c18661f 100644 --- a/tests/harness.ts +++ b/tests/harness.ts @@ -233,6 +233,18 @@ export class FakeGit extends Git { return { stdout: "", stderr: "", code: 0 }; } + pushedBranches: string[] = []; + override async pushBranch(_worktreeDir: string, branch: string) { + this.pushedBranches.push(branch); + return { stdout: "", stderr: "", code: 0 }; + } + + branchWorktrees: string[] = []; + override async ensureBranchWorktree(_cloneDir: string, worktreeDir: string, branch: string): Promise { + this.branchWorktrees.push(branch); + mkdirSync(worktreeDir, { recursive: true }); + } + 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 { diff --git a/tests/install.test.ts b/tests/install.test.ts index 511773a..59ae7ca 100644 --- a/tests/install.test.ts +++ b/tests/install.test.ts @@ -150,6 +150,16 @@ describe("install.sh scaffold", () => { expect(readFileSync(join(target, ".factory", "gates.sh"), "utf8")).not.toBe("old"); rmSync(target, { recursive: true, force: true }); }); + + test("writes a starter lessons.md, and --update never overwrites accumulated lessons", () => { + const target = scratchTarget(); + run([target]); + expect(existsSync(join(target, ".factory", "memory", "lessons.md"))).toBe(true); + writeFileSync(join(target, ".factory", "memory", "lessons.md"), "my accumulated lessons"); + run([target, "--update"]); + expect(readFileSync(join(target, ".factory", "memory", "lessons.md"), "utf8")).toBe("my accumulated lessons"); + rmSync(target, { recursive: true, force: true }); + }); }); describe("install.sh --agents", () => { diff --git a/tests/learn.test.ts b/tests/learn.test.ts new file mode 100644 index 0000000..e19cc71 --- /dev/null +++ b/tests/learn.test.ts @@ -0,0 +1,113 @@ +// v2.10.0 item 4: `factory learn` batches pending retro proposals (lessons and +// skill edits) into one PR on a daily learning branch, never touching +// anything outside its allowed paths, and never re-batching a row twice. + +import { afterAll, describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { LEARNING_ALLOWED_PATHS, branchNameFor, runLearn } from "../src/learn"; +import { FactoryState } from "../src/state"; +import { FakeGit, FakeGitHub } from "./harness"; + +const dirs: string[] = []; +afterAll(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); +}); + +function setup() { + const workspacesDir = mkdtempSync(join(tmpdir(), "factory-learn-ws-")); + const cloneDir = mkdtempSync(join(tmpdir(), "factory-learn-clone-")); + dirs.push(workspacesDir, cloneDir); + const github = new FakeGitHub([]); + const git = new FakeGit(); + const state = new FactoryState(":memory:"); + const repo = "acme/widgets"; + return { github, git, state, cloneDir, workspacesDir, repo, deps: { github, git, state, cloneDir, workspacesDir } }; +} + +const NOW = new Date("2026-09-27T12:00:00Z"); + +describe("factory learn", () => { + test("does nothing when there is nothing pending", async () => { + const { deps, repo } = setup(); + const result = await runLearn(deps, repo, "main", NOW); + expect(result).toEqual({ batched: 0 }); + expect(deps.git.pushedBranches).toEqual([]); + expect(deps.github.createdPrs).toEqual([]); + }); + + test("batches a lesson into lessons.md, pushes the branch and opens a PR, then marks the row learned", async () => { + const { deps, repo, git, state } = setup(); + const id = state.queueRetro(repo, 7, "merged"); + state.completeRetro(id, { lesson: "Always add a regression test for the exact bug." }); + + git.changed = [".factory/memory/lessons.md"]; + const result = await runLearn(deps, repo, "main", NOW); + + expect(result.batched).toBe(1); + expect(result.branch).toBe(branchNameFor(NOW)); + expect(deps.git.pushedBranches).toEqual([branchNameFor(NOW)]); + expect(deps.github.createdPrs).toHaveLength(1); + expect(deps.github.createdPrs[0]!.base).toBe("main"); + expect(deps.github.createdPrs[0]!.head).toBe(branchNameFor(NOW)); + expect(result.prUrl).toBe(deps.github.prs[0]!.url); + + const lessonsPath = join(deps.workspacesDir, branchNameFor(NOW).replace(/\//g, "-"), ".factory/memory/lessons.md"); + expect(await Bun.file(lessonsPath).text()).toContain("Always add a regression test for the exact bug."); + + // Row is stamped learned, so a second run has nothing pending. + expect(state.pendingLessons(repo)).toEqual([]); + }); + + test("batches a skill edit into that skill's PROPOSED_EDITS.md, not the real SKILL.md", async () => { + const { deps, repo, git, state } = setup(); + const id = state.queueRetro(repo, 9, "gave-up"); + state.completeRetro(id, { skill_name: "factory-build", skill_edit: "Call out the exact acceptance criterion wording." }); + + git.changed = [".claude/skills/factory-build/PROPOSED_EDITS.md"]; + const result = await runLearn(deps, repo, "main", NOW); + + expect(result.batched).toBe(1); + const path = join(deps.workspacesDir, branchNameFor(NOW).replace(/\//g, "-"), ".claude/skills/factory-build/PROPOSED_EDITS.md"); + const body = await Bun.file(path).text(); + expect(body).toContain("Call out the exact acceptance criterion wording."); + expect(body).toContain("#9"); + }); + + test("refuses to push, and leaves the row pending, when a batch would touch a disallowed path", async () => { + const { deps, repo, git, state } = setup(); + const id = state.queueRetro(repo, 3, "rejected"); + state.completeRetro(id, { lesson: "Something worth remembering." }); + + // Simulate a bug that wrote outside .factory/memory or .claude/skills. + git.changed = ["src/watch.ts"]; + + await expect(runLearn(deps, repo, "main", NOW)).rejects.toThrow(/outside its allowed paths/); + expect(deps.git.pushedBranches).toEqual([]); + expect(deps.github.createdPrs).toEqual([]); + // Never marked learned, so it's still pending for a fixed run to pick up. + expect(state.pendingLessons(repo)).toHaveLength(1); + }); + + test("a second same-day run reuses the existing open PR instead of opening a new one", async () => { + const { deps, repo, git, state } = setup(); + const id1 = state.queueRetro(repo, 1, "merged"); + state.completeRetro(id1, { lesson: "First lesson." }); + git.changed = [".factory/memory/lessons.md"]; + await runLearn(deps, repo, "main", NOW); + expect(deps.github.createdPrs).toHaveLength(1); + + const id2 = state.queueRetro(repo, 2, "merged"); + state.completeRetro(id2, { lesson: "Second lesson." }); + git.changed = [".factory/memory/lessons.md"]; + const second = await runLearn(deps, repo, "main", NOW); + + expect(deps.github.createdPrs).toHaveLength(1); // reused, not a second PR + expect(second.prUrl).toBe(deps.github.prs[0]!.url); + }); + + test("LEARNING_ALLOWED_PATHS covers exactly memory and skills, the branch's one exemption from protectedPaths", () => { + expect(LEARNING_ALLOWED_PATHS).toEqual([".factory/memory/**", ".claude/skills/**"]); + }); +}); diff --git a/tests/retro.test.ts b/tests/retro.test.ts new file mode 100644 index 0000000..6ed8cf0 --- /dev/null +++ b/tests/retro.test.ts @@ -0,0 +1,127 @@ +// v2.10.0 item 3: a retro row is queued and run after every one of the three +// final outcomes reachable inside watch.ts (merged, /factory cancel = rejected, +// a third verify reject = gave-up), plus the fourth path: the dashboard's +// operator-merge route, which has no Executor and only queues, drained here +// by pollOnce's runQueuedRetros. state.test.ts already proves queueRetro/ +// completeRetro store the right fields; these tests prove each trigger site +// actually calls runRetro with the right outcome and that it runs to done. + +import { afterAll, describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { mergeConfig } from "../src/config"; +import type { StageName, StageRunResult } from "../src/executor"; +import { FactoryState } from "../src/state"; +import { LABEL } from "../src/labels"; +import { advanceIssue, pollOnce } from "../src/watch"; +import { baseIssue, FakeGateRunner, FakeGit, FakeGitHub, fixtureFor, MultiStageExecutor } from "./harness"; + +const dirs: string[] = []; +afterAll(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); +}); + +function setup(initial: string[], overrides: Parameters[0] = { repo: "acme/widgets" }, n = 1) { + const workspacesDir = mkdtempSync(join(tmpdir(), "factory-retro-ws-")); + const cloneDir = mkdtempSync(join(tmpdir(), "factory-retro-clone-")); + dirs.push(workspacesDir, cloneDir); + const github = new FakeGitHub([baseIssue(n, initial)]); + const git = new FakeGit(); + const state = new FactoryState(":memory:"); + const executor = new MultiStageExecutor(); + const gateRunner = new FakeGateRunner(); + const config = mergeConfig({ repo: "acme/widgets", ...overrides }); + const deps = { github, git, state, executor, gateRunner, cloneDir, workspacesDir }; + const push = (stage: StageName, files: Record, result?: Partial) => executor.push(stage, n, fixtureFor(stage, n), files, result); + return { n, github, state, executor, config, deps, push }; +} + +const triage = (o: object = {}) => ({ + "triage-comment.md": "\ntriage", + "triage.json": JSON.stringify({ disposition: "proceed", type: "bug", risk: "low", done_when: "x", files_expected: ["src/a.ts"], gate_level: "make check", confidence: 0.9, ...o }), +}); +const plan = (risk: "low" | "medium" = "low") => ({ + "plan-comment.md": "\nplan", + "plan.json": JSON.stringify({ risk, revision: 1, files: ["src/a.ts"], autoApproveEligible: risk === "low" }), +}); +const build = () => ({ "status-comment.md": "\nbuilding", "build.json": JSON.stringify({ status: "green", gate_line: "ok", rounds: 1 }) }); +const verdict = (result: "pass" | "reject") => ({ "verdict-comment.md": "\nverdict", "verdict.json": JSON.stringify({ result, rounds: 1, findings: [] }) }); +const pr = () => ({ "pr-body.md": "Did it.\nCloses #1" }); +const retro = () => ({ "retro-comment.md": "\nnothing new", "retro.json": JSON.stringify({ outcome: "complete", summary: "nothing new" }) }); + +describe("a retro row is queued and run after each of the four final-outcome triggers", () => { + test("checkMergePolicy queues and runs a retro with outcome merged, once the PR actually merges", async () => { + const c = setup([LABEL.ready], { repo: "acme/widgets", merge: { policy: "auto", autoPaths: [], maxFiles: 10, maxLines: 200 } }); + c.state.setToggle("auto_approve_low_risk", true); + c.push("triage", triage()); + c.push("plan", plan()); + c.push("build", build()); + c.push("verify", verdict("pass")); + c.push("pr", pr()); + expect(await advanceIssue(c.deps, c.config, c.github.issues.get(c.n)!)).toBe("shipped"); + const prRow = c.github.prs[0]!; + c.github.queuePrStatus(prRow.number, { + state: "open", + headRefOid: "sha1", + closingIssuesReferences: [{ number: c.n }], + statusCheckRollup: [{ name: "build", status: "COMPLETED", conclusion: "SUCCESS" }], + }); + c.github.queueMergeReadiness(prRow.number, { + state: "open", + isDraft: false, + baseRefName: c.config.base, + headRefOid: "sha1", + mergeable: "MERGEABLE", + reviewDecision: "APPROVED", + hasUnresolvedReviewThreads: false, + changesRequestedStale: false, + }); + c.push("retro", retro()); + await advanceIssue(c.deps, c.config, c.github.issues.get(c.n)!); + expect(c.github.merged).toEqual([{ repo: c.config.repo, prNumber: prRow.number, headSha: "sha1" }]); + // "retro" is stored via an explicit `as Stage` cast (Stage itself excludes it by design). + expect(c.state.listStageRuns(c.config.repo).some((r) => (r.stage as string) === "retro")).toBe(true); + expect(c.state.listRetros(c.config.repo)).toEqual([expect.objectContaining({ issue: c.n, outcome: "merged", status: "done" })]); + c.state.close(); + }); + + test("/factory cancel queues and runs a retro with outcome rejected", async () => { + const c = setup([LABEL.awaitingApproval]); + const waiting = c.github.issues.get(c.n)!; + waiting.comments = [{ id: 5, author: "bot", authorAssociation: "OWNER", body: "", createdAt: "2026-01-01T00:00:00Z" }]; + c.github.say(c.n, "/factory cancel"); + c.push("retro", retro()); + await pollOnce(c.deps, c.config); + expect(c.github.issues.get(c.n)!.labels.map((l) => l.name)).toEqual([]); // cancelRun strips every factory:* label and closes the issue + expect(c.state.listStageRuns(c.config.repo).some((r) => (r.stage as string) === "retro")).toBe(true); + expect(c.state.listRetros(c.config.repo)).toEqual([expect.objectContaining({ issue: c.n, outcome: "rejected", status: "done" })]); + c.state.close(); + }); + + test("a third verify reject queues and runs a retro with outcome gave-up", async () => { + const c = setup([LABEL.ready]); + c.state.setToggle("auto_approve_low_risk", true); + c.push("triage", triage()); + c.push("plan", plan()); + for (let i = 0; i < 3; i++) { + c.push("build", build()); + c.push("verify", verdict("reject")); + } + c.push("retro", retro()); + expect(await advanceIssue(c.deps, c.config, c.github.issues.get(c.n)!)).toBe("needs-human"); + expect(c.state.listStageRuns(c.config.repo).some((r) => (r.stage as string) === "retro")).toBe(true); + expect(c.state.listRetros(c.config.repo)).toEqual([expect.objectContaining({ issue: c.n, outcome: "gave-up", status: "done" })]); + c.state.close(); + }); + + test("pollOnce's runQueuedRetros drains a retro the dashboard's operator-merge route only queued", async () => { + const c = setup([LABEL.inReview]); + const id = c.state.queueRetro(c.config.repo, c.n, "merged"); + c.push("retro", retro()); + await pollOnce(c.deps, c.config); + expect(c.state.listQueuedRetros(c.config.repo)).toEqual([]); + expect(c.state.listRetros(c.config.repo)).toEqual([expect.objectContaining({ id, issue: c.n, outcome: "merged", status: "done" })]); + c.state.close(); + }); +}); diff --git a/tests/routes.test.ts b/tests/routes.test.ts index 3038c47..6487818 100644 --- a/tests/routes.test.ts +++ b/tests/routes.test.ts @@ -73,7 +73,7 @@ describe("config validation of routes", () => { test("a route naming triage is refused: triage never routes on type", () => { const problems = configProblems({ ...DEFAULT_CONFIG, repo: "a/b", routes: { docs: { stages: { triage: "claude" } } } }); - expect(problems).toContain("routes.docs.stages.triage: unknown stage (allowed: plan, build, verify, pr)"); + expect(problems).toContain("routes.docs.stages.triage: unknown stage (allowed: plan, build, verify, pr, retro)"); }); test("proof must be \"test\" or \"check\"", () => { diff --git a/tests/scenarios.test.ts b/tests/scenarios.test.ts index 1acbfaa..5f6eac8 100644 --- a/tests/scenarios.test.ts +++ b/tests/scenarios.test.ts @@ -345,6 +345,19 @@ describe("failure paths", () => { done(c); }); + test("12b. a normal build still cannot touch .claude/**, even with the default empty protectedPaths", async () => { + const c = setup([LABEL.ready]); + c.state.setToggle("auto_approve_low_risk", true); + c.git.changed = [".claude/skills/factory-build/SKILL.md"]; + c.push("triage", triage()); + c.push("plan", plan("low")); + c.push("build", build()); + expect(await c.step()).toBe("needs-human"); + expect(c.git.pushed).toEqual([]); + expect(c.state.getRun("acme/widgets", 1)!.reason).toContain(".claude/skills/factory-build/SKILL.md"); + done(c); + }); + test("13. a killed stage fails with the kill reason", async () => { const c = setup([LABEL.ready]); c.push("triage", triage(), { exitCode: 1, killedReason: "timed out after 15 minutes" }); diff --git a/tests/schemas.test.ts b/tests/schemas.test.ts index 61f15cf..b15444b 100644 --- a/tests/schemas.test.ts +++ b/tests/schemas.test.ts @@ -5,13 +5,14 @@ import { expect, test, describe } 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 STAGES: ArtifactStage[] = ["triage", "plan", "build", "verify", "pr", "retro"]; 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: {}, + retro: {}, }; test("every stage has a schema whose properties are exactly the validator's fields", () => { diff --git a/tests/state.test.ts b/tests/state.test.ts index b9b01dd..b5011ce 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -109,3 +109,60 @@ test("a run's event budget is released when the run stops running, and a resume expect(stored).toBeLessThanOrEqual(2 << 10); state.close(); }); + +// v2.10.0 item 3: one row per final outcome, queued first and completed +// (immediately, or later by runQueuedRetros) with at most one proposal. +test("queueRetro starts a row queued; completeRetro fills in the proposal, status and completed_at", () => { + const path = join(dir, "retro-complete.db"); + const state = new FactoryState(path); + const id = state.queueRetro("acme/widgets", 1, "merged"); + expect(state.listQueuedRetros("acme/widgets").map((r) => r.id)).toEqual([id]); + state.completeRetro(id, { lesson: "keep acceptance criteria checkable", skill_name: "factory-plan", skill_edit: "require a done_when per AC" }); + expect(state.listQueuedRetros("acme/widgets")).toEqual([]); + state.close(); + const check = new Database(path); + const stored = check.query("SELECT status, lesson, skill_name, skill_edit, completed_at FROM retros WHERE id = $id").get({ $id: id }) as Record; + expect(stored).toMatchObject({ status: "done", lesson: "keep acceptance criteria checkable", skill_name: "factory-plan", skill_edit: "require a done_when per AC" }); + expect(stored.completed_at).not.toBeNull(); + check.close(); +}); + +test("completeRetro with no proposal defaults to done with null proposal fields; a failed retro can record 'failed'", () => { + const path = join(dir, "retro-defaults.db"); + const state = new FactoryState(path); + const doneId = state.queueRetro("acme/widgets", 2, "gave-up"); + state.completeRetro(doneId); + const failedId = state.queueRetro("acme/widgets", 3, "rejected"); + state.completeRetro(failedId, undefined, "failed"); + state.close(); + const check = new Database(path); + const rows = check.query("SELECT id, status, lesson FROM retros ORDER BY id").all() as { id: number; status: string; lesson: string | null }[]; + expect(rows).toEqual([ + { id: doneId, status: "done", lesson: null }, + { id: failedId, status: "failed", lesson: null }, + ]); + check.close(); +}); + +test("listQueuedRetros is scoped to its own repo and only ever returns queued rows", () => { + const state = new FactoryState(":memory:"); + const mine = state.queueRetro("acme/widgets", 1, "rejected"); + state.queueRetro("other/repo", 1, "merged"); + const done = state.queueRetro("acme/widgets", 2, "merged"); + state.completeRetro(done, undefined, "failed"); + expect(state.listQueuedRetros("acme/widgets").map((r) => r.id)).toEqual([mine]); + state.close(); +}); + +test("listRetros returns every status for a repo, newest first, scoped to that repo", () => { + const state = new FactoryState(":memory:"); + const first = state.queueRetro("acme/widgets", 1, "merged"); + state.completeRetro(first, { lesson: "l" }); + const second = state.queueRetro("acme/widgets", 2, "gave-up"); + state.queueRetro("other/repo", 1, "rejected"); + expect(state.listRetros("acme/widgets").map((r) => [r.id, r.status, r.outcome])).toEqual([ + [second, "queued", "gave-up"], + [first, "done", "merged"], + ]); + state.close(); +});