From 188083ffdbc354066e207fa7be82e82f738ccfb5 Mon Sep 17 00:00:00 2001 From: Param Harrison Date: Sun, 27 Sep 2026 14:23:16 +0300 Subject: [PATCH] v2.6.2: any repo, safely Per-repo state (FACTORY_HOME///, with a doctor warning on a leftover pre-v2.6.2 shared factory.db/workspaces), a resettable opt-in so reset/rebaseline refuse on a live repo, the base branch defaulting to origin/HEAD instead of a hard-coded main, a setup hook that runs once per worktree before any stage, issue forms plus a PR template that install ships (and install.sh now points at `doctor --fix`, which is what actually creates the labels those forms apply), a CI toolchain placeholder step, and a bun.lock guard so `factory scan` skips cleanly on a non-Bun repo. No new features. This is what a second repo (splitbill-demo plus lwp-website) needed before v2.7 adds routing. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 39 +++++++++++ bin/factory | 43 +++++++++--- install.sh | 2 +- package.json | 2 +- src/config.ts | 32 +++++++++ src/doctor.ts | 32 +++++++++ src/paths.ts | 24 +++++-- src/reset.ts | 13 ++++ src/scan.ts | 8 +++ src/setup.ts | 47 +++++++++++++ src/watch.ts | 40 ++++++++--- teach/sessions.json | 6 +- template-ci/factory.yml.example | 18 ++++- .../skills/factory-comment/assets/verdict.md | 2 +- template/.claude/skills/factory-pr/SKILL.md | 22 +++--- .../.claude/skills/factory-verify/SKILL.md | 2 +- template/.github/ISSUE_TEMPLATE/bug.yml | 27 ++++++++ template/.github/ISSUE_TEMPLATE/docs.yml | 20 ++++++ template/.github/ISSUE_TEMPLATE/feature.yml | 27 ++++++++ template/.github/pull_request_template.md | 25 +++++++ tests/app-agnostic.test.ts | 34 ++++++++++ tests/config.test.ts | 40 +++++++++++ tests/doctor.test.ts | 38 +++++++++++ tests/install.test.ts | 31 +++++++++ tests/reset.test.ts | 30 +++++++- tests/scan.test.ts | 34 ++++++++-- tests/setup.test.ts | 68 +++++++++++++++++++ 27 files changed, 659 insertions(+), 47 deletions(-) create mode 100644 src/setup.ts create mode 100644 template/.github/ISSUE_TEMPLATE/bug.yml create mode 100644 template/.github/ISSUE_TEMPLATE/docs.yml create mode 100644 template/.github/ISSUE_TEMPLATE/feature.yml create mode 100644 template/.github/pull_request_template.md create mode 100644 tests/setup.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fa9f833..0f5d92b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,44 @@ # Changelog +## v2.6.2 + +Any repo, safely. No new features: this closes the gaps a second repo (splitbill-demo plus +lwp-website) hit under one shared `FACTORY_HOME`. + +- **Per-repo state.** `factory.db` and `workspaces/issue-N` now live under + `FACTORY_HOME///`, not one shared `FACTORY_HOME`; two repos each carrying an issue #3 + no longer collide on one worktree or one DB row set. `src/paths.ts` is the one resolver. `factory doctor` + warns (never auto-fixes) when the pre-v2.6.2 shared `factory.db` or `workspaces/` is still on disk; + moving that history is a deliberate, manual step, not something a doctor run does for you. +- **`resettable: true` opt-in.** `factory reset` and `rebaseline` now refuse on a repo that hasn't opted + in; a live repo like lwp-website (which deploys on merge to `main`) can no longer have its issues and + branches wiped by a config that was only ever meant for a sandbox. +- **The base branch defaults to `origin/HEAD`,** not a hard-coded `"main"`. A repo cloned with a + non-`main` default (`trunk`, `develop`) gets the branch it actually has, unless a config names `base` + explicitly. `factory-verify` and `factory-comment`'s skill text now say "the base branch" instead of + quoting `main`. +- **A `setup` hook.** `.factory/config.json` can list `setup` commands (e.g. `npm ci`) that run once per + worktree, before any stage; a failure parks the issue with the log attached, instead of every stage + failing separately on a project that was never installed. +- **Issue forms and a PR template.** `install` now ships `.github/ISSUE_TEMPLATE/{bug,feature,docs}.yml` + (each applies only its own type label, never `factory:ready`) and `.github/pull_request_template.md` + (summary, plan, gate evidence, verify verdict, risk and rollback: the same sections `factory-pr`'s + fallback body now uses when a target repo has no template of its own). `install.sh`'s next-step message + now says `factory doctor --fix`, which is what actually creates the `factory:*` and type labels; it used + to say plain `factory doctor`, which only reports them missing. This is what makes the + `src/labels.ts:48` claim ("set by the issue form") true. +- **Carried over from v2.6.1:** the CI template gets a "set up your toolchain" placeholder step in + `run-issue`, `tick` and `manual` (not `scan`, which only runs `bun audit`); `factory scan` skips with a + clear message, never shelling out, on a repo with no `bun.lock`/`bun.lockb`; `teach/sessions.json` is + renumbered to this roadmap's versions. The Agents table's `overflow-x: auto` wrapper already covered the + 375px-scroll item (verified, no change needed). + +Not in v2.6.2: +- Model routing, machine concurrency, spend caps and `proof: check` for non-test work (v2.7). +- The lwp-website pilot itself, and anything that merges (v2.8). +- A live agent run: no stage's behavior changed here (only wording, config surface and file layout), so + the last live Claude run (v2.6.1) still stands as current evidence. + ## v2.6.1 Residue fixes for v2.6.0. No new features. diff --git a/bin/factory b/bin/factory index 1294399..d24c960 100755 --- a/bin/factory +++ b/bin/factory @@ -13,6 +13,7 @@ import { loadConfig, type FactoryConfig } from "../src/config"; import { EXIT, UsageError, failureJson, successJson } from "../src/cli-output"; import { advanceIssue, pollOnce, recoverInFlight, startWatch, type WatchDeps } from "../src/watch"; import { ShellGateRunner } from "../src/gates"; +import { ShellSetupRunner } from "../src/setup"; import { BinaryRunner, ClaudeRechecker } from "../src/recheck"; import { scan } from "../src/scan"; import { streamLogs } from "../src/logs"; @@ -22,7 +23,7 @@ import { reset, rebaseline } from "../src/reset"; import { runDoctor, fixDoctor } from "../src/doctor"; import { createDashboard } from "../dashboard/server"; import { versionOf, which } from "../src/probes"; -import { workspacesDir as defaultWorkspacesDir } from "../src/paths"; +import { workspacesDir as defaultWorkspacesDir, defaultStatePath } from "../src/paths"; import { LABEL } from "../src/labels"; import { ensureRepoClone } from "../src/repo"; import { helpText } from "../src/help"; @@ -75,12 +76,13 @@ function buildWatchDeps(cloneDir: string, config: FactoryConfig): WatchDeps { return { github: new GitHub(), git: new Git(new GitCommandRunner()), - state: new FactoryState(flag("db") ?? DEFAULT_DB_PATH), + state: new FactoryState(flag("db") ?? process.env.FACTORY_DB_PATH ?? defaultStatePath(process.env, config.repo)), executor: new CommandExecutor(config.agents, config.stages), gateRunner: new ShellGateRunner(), rechecker: verifierIsClaude(config) ? new ClaudeRechecker(new BinaryRunner("claude"), cloneDir) : undefined, cloneDir, - workspacesDir: flag("workspaces") ?? defaultWorkspacesDir(), + workspacesDir: flag("workspaces") ?? defaultWorkspacesDir(process.env, config.repo), + setupRunner: new ShellSetupRunner(), }; } @@ -159,11 +161,16 @@ async function cmdPark(): Promise { async function cmdLogs(): Promise { const issue = Number(args[1]); if (!Number.isInteger(issue) || issue < 1) throw new UsageError("logs: an issue number is required, e.g. `factory logs 12 --follow`"); - const state = new FactoryState(flag("db") ?? process.env.FACTORY_DB_PATH ?? DEFAULT_DB_PATH); - const repo = flag("repo") ?? process.env.FACTORY_REPO ?? state.listRuns().find((r) => r.issue === issue)?.repo ?? ""; + const repoDir = flag("repo-dir"); + // With no --repo/--repo-dir/FACTORY_REPO hint, this stays on the legacy + // shared DB (pre-v2.6.2 installs); with one, it opens that repo's own DB + // (per-repo since v2.6.2) instead of guessing at the shared one. + const repo = flag("repo") ?? process.env.FACTORY_REPO ?? (repoDir ? (await loadConfig(resolve(repoDir))).repo : undefined); + const state = new FactoryState(flag("db") ?? process.env.FACTORY_DB_PATH ?? (repo ? defaultStatePath(process.env, repo) : DEFAULT_DB_PATH)); + const resolvedRepo = repo ?? state.listRuns().find((r) => r.issue === issue)?.repo ?? ""; const controller = new AbortController(); process.on("SIGINT", () => controller.abort()); - await streamLogs(state, repo, issue, { follow: has("follow"), stage: flag("stage"), json: has("json"), signal: controller.signal }); + await streamLogs(state, resolvedRepo, issue, { follow: has("follow"), stage: flag("stage"), json: has("json"), signal: controller.signal }); } async function cmdInbox(): Promise { @@ -201,6 +208,10 @@ async function cmdScan(): Promise { }, }; const result = await scan({ github: new GitHub(), runner: bunRunner }, config.repo, cloneDir); + if (result.skippedReason) { + console.log(`factory scan: skipped — ${result.skippedReason}`); + return; + } console.log(`factory scan: filed ${result.filed.length}, skipped ${result.skipped.length} (already open)`); for (const t of result.filed) console.log(` + ${t}`); } @@ -215,8 +226,9 @@ async function cmdReset(): Promise { baselineTag: config.baselineTag, base: config.base, issuesDir: `${cloneDir}/.factory/issues`, - workspacesDir: flag("workspaces") ?? defaultWorkspacesDir(), - statePath: flag("db") ?? DEFAULT_DB_PATH, + workspacesDir: flag("workspaces") ?? defaultWorkspacesDir(process.env, config.repo), + statePath: flag("db") ?? process.env.FACTORY_DB_PATH ?? defaultStatePath(process.env, config.repo), + resettable: config.resettable, allIssues: has("all-issues"), }; const deps = { github: new GitHub(), git: new GitCommandRunner() }; @@ -237,6 +249,7 @@ async function cmdRebaseline(): Promise { issuesDir: "", workspacesDir: "", statePath: "", + resettable: config.resettable, }; const moved = await rebaseline({ github: new GitHub(), git: new GitCommandRunner() }, ctx, dryRun); console.log(`factory rebaseline${dryRun ? " --dry-run" : ""}: ${config.baselineTag} -> origin/${config.base} (${moved.length} commit(s))`); @@ -286,7 +299,17 @@ async function cmdDoctor(): Promise { } }, }, - { repo: config.repo, cloneDir, baselineTag: config.baselineTag, factoryMode: process.env.FACTORY_MODE, agents: config.agents, stages: config.stages, templateSkills: templateSkills() }, + { + repo: config.repo, + cloneDir, + baselineTag: config.baselineTag, + factoryMode: process.env.FACTORY_MODE, + agents: config.agents, + stages: config.stages, + templateSkills: templateSkills(), + legacyStatePath: defaultStatePath(process.env), + legacyWorkspacesDir: defaultWorkspacesDir(process.env), + }, ); const allOk = checks.every((c) => c.ok || c.warn); if (has("json")) console.log(successJson({ checks }, allOk)); @@ -319,10 +342,10 @@ function serveDashboard(state: FactoryState, github: GitHub, repo: string, autoA } async function cmdDashboard(): Promise { - const dbPath = flag("db") ?? process.env.FACTORY_DB_PATH ?? DEFAULT_DB_PATH; const repoDir = flag("repo-dir"); const config = repoDir ? await loadConfig(resolve(repoDir)) : undefined; const repo = flag("repo") ?? process.env.FACTORY_REPO ?? config?.repo ?? ""; + const dbPath = flag("db") ?? process.env.FACTORY_DB_PATH ?? (repo ? defaultStatePath(process.env, repo) : DEFAULT_DB_PATH); const state = new FactoryState(dbPath); const github = new GitHub(); serveDashboard(state, github, repo, false, config); diff --git a/install.sh b/install.sh index 9a5af95..be01a41 100755 --- a/install.sh +++ b/install.sh @@ -219,5 +219,5 @@ action="wrote" echo "" echo "install.sh: $action $wrote item(s), skipped $skipped existing item(s), $unchanged unchanged in $TARGET" if [[ ! -e "$TARGET/.factory/config.json" ]]; then - echo "install.sh: next: cp .factory/config.example.json .factory/config.json, fill in every TODO (config.json, charter.md), then \`factory doctor --repo-dir $TARGET\`." + echo "install.sh: next: cp .factory/config.example.json .factory/config.json, fill in every TODO (config.json, charter.md), then \`factory doctor --fix --repo-dir $TARGET\` (creates the factory:* and type labels this repo needs)." fi diff --git a/package.json b/package.json index 86bc1a7..f6d8d7d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "software-factory", - "version": "2.6.1", + "version": "2.6.2", "private": true, "type": "module", "description": "GitHub-native SDLC loop for coding agents: triage, plan, build, verify, PR.", diff --git a/src/config.ts b/src/config.ts index e8d400b..8d89caf 100644 --- a/src/config.ts +++ b/src/config.ts @@ -52,6 +52,13 @@ export interface FactoryConfig { readonly stageTimeoutMinutes: number; // kills a stuck `claude` process (audit finding #15) readonly maxToolCalls: number; // kills a runaway stage before it burns budget readonly gates: readonly GateSpec[]; // read by .factory/gates.sh + // Opt-in: `reset`/`rebaseline` refuse on any repo where this is false (the + // default), so a live repo (lwp-website) can never be force-pushed back to + // a baseline tag by a stray `factory reset` (plan v2.6.2 item 2). + readonly resettable: boolean; + // Commands run once per fresh worktree, before any stage (e.g. "npm ci"). + // Idempotent via a marker file in the worktree; a failure parks the issue. + readonly setup: readonly string[]; readonly agentCommands: AgentCommands; // Named agents, each a preset or a command; `stages` says which one runs a stage. readonly agents: Readonly>; @@ -71,6 +78,8 @@ export const DEFAULT_CONFIG: FactoryConfig = { stageTimeoutMinutes: 15, maxToolCalls: 60, gates: [], + resettable: false, + setup: [], agentCommands: { read: [], build: [], verify: [] }, agents: { claude: { preset: "claude" } }, stages: { default: "claude" }, @@ -106,6 +115,8 @@ const TOP_LEVEL: Record = { stageTimeoutMinutes: "posInt", maxToolCalls: "posInt", gates: "object", + resettable: "boolean", + setup: "strings", agentCommands: "object", agents: "object", stages: "object", @@ -212,6 +223,14 @@ export async function loadConfig(targetRepoDir: string): Promise // GitHub calls follow config.repo but git pushes follow origin; a copied config must not aim one at the wrong repo. const origin = originRepo(targetRepoDir); if (config.repo === "" && origin) config = { ...config, repo: origin }; + // "main" is only a fallback default (DEFAULT_CONFIG.base), never a guess: + // a config that omits "base" gets whatever origin/HEAD resolves to (a repo + // on "trunk" or "develop" must not be force-pushed at "main" by default). + // A config that names "base" explicitly, even "main", is never overridden. + if ((raw as Record).base === undefined) { + const detected = defaultBranchOf(targetRepoDir); + if (detected) config = { ...config, base: detected }; + } if (!/^[^/\s]+\/[^/\s]+$/.test(config.repo)) { throw new ConfigError(`${path}: "repo" must be "owner/name", got ${JSON.stringify(config.repo)}`); } @@ -233,3 +252,16 @@ function originRepo(dir: string): string | undefined { const r = Bun.spawnSync(["git", "-C", dir, "remote", "get-url", "origin"], { stdout: "pipe", stderr: "ignore" }); return r.exitCode === 0 ? repoFromRemoteUrl(r.stdout.toString()) : undefined; } + +// The branch name behind origin/HEAD, e.g. "trunk" for a repo cloned with a +// non-"main" default. Mirrors ensureRepoClone's origin/HEAD resolution +// (src/repo.ts) for local `--repo-dir` mode, which never runs that clone +// path. Missing on a bare or freshly-inited repo (no remote fetch yet) -- +// callers fall back to DEFAULT_CONFIG.base ("main") in that case. +function defaultBranchOf(dir: string): string | undefined { + if (!existsSync(`${dir}/.git`)) return undefined; + const r = Bun.spawnSync(["git", "-C", dir, "rev-parse", "--abbrev-ref", "origin/HEAD"], { stdout: "pipe", stderr: "ignore" }); + if (r.exitCode !== 0) return undefined; + const ref = r.stdout.toString().trim(); + return ref.startsWith("origin/") ? ref.slice("origin/".length) : undefined; +} diff --git a/src/doctor.ts b/src/doctor.ts index 8ea224b..96c4bc8 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -35,6 +35,13 @@ export interface DoctorContext { readonly stages?: StageAgents; // Shipped skill files (path relative to the repo root -> content), to spot an install that predates this runner. readonly templateSkills?: Record; + // The pre-v2.6.2 shared paths (`defaultStatePath`/`workspacesDir` called + // with no `repo`), passed in so doctor can warn when they still exist. + // Never auto-migrated: that was an explicit design decision (a shared DB + // moved without asking could interleave two repos' history), so this + // check only names the path and leaves the move to the operator. + readonly legacyStatePath?: string; + readonly legacyWorkspacesDir?: string; } // Flags that let a CLI run headless without waiting on an approval prompt. @@ -205,6 +212,31 @@ export async function runDoctor(deps: DoctorDeps, ctx: DoctorContext): Promise//factory.db. This is never migrated automatically: move any run history you want to keep, then remove it.` + : "none found", + fixable: false, + warn: true, + }); + } + if (ctx.legacyWorkspacesDir) { + const exists = await deps.fileExists(ctx.legacyWorkspacesDir); + checks.push({ + name: "no unmigrated pre-v2.6.2 shared workspaces/", + ok: !exists, + detail: exists + ? `${ctx.legacyWorkspacesDir} still exists; each repo now gets its own workspaces/ under FACTORY_HOME///. This is never migrated automatically: move anything you need, then remove it.` + : "none found", + fixable: false, + warn: true, + }); + } + return checks; } diff --git a/src/paths.ts b/src/paths.ts index 45a7f57..5cc4f3a 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -13,12 +13,24 @@ export function factoryHome(env: NodeJS.ProcessEnv = process.env): string { return resolve(env.FACTORY_HOME ?? `${env.HOME ?? "."}/.factory`); } -export function workspacesDir(env: NodeJS.ProcessEnv = process.env): string { - return `${factoryHome(env)}/workspaces`; +// A second repo (splitbill-demo, lwp-website, ...) sharing one FACTORY_HOME +// must never share a workspace or a state DB with the first: two repos each +// carrying an issue #3 would otherwise collide on one worktree and one row +// set. `repo` ("owner/name") namespaces both under FACTORY_HOME; omitting it +// keeps the pre-v2.6.2 shared path, which DEFAULT_DB_PATH and existing +// fixtures still rely on. +function repoHome(repo: string, env: NodeJS.ProcessEnv): string { + const [owner, name] = repo.split("/"); + if (!owner || !name) throw new Error(`repo must be "owner/name", got ${JSON.stringify(repo)}`); + return `${factoryHome(env)}/${owner}/${name}`; } -export function defaultStatePath(env: NodeJS.ProcessEnv = process.env): string { - return `${factoryHome(env)}/factory.db`; +export function workspacesDir(env: NodeJS.ProcessEnv = process.env, repo?: string): string { + return repo ? `${repoHome(repo, env)}/workspaces` : `${factoryHome(env)}/workspaces`; +} + +export function defaultStatePath(env: NodeJS.ProcessEnv = process.env, repo?: string): string { + return repo ? `${repoHome(repo, env)}/factory.db` : `${factoryHome(env)}/factory.db`; } export function reposDir(env: NodeJS.ProcessEnv = process.env): string { @@ -27,6 +39,6 @@ export function reposDir(env: NodeJS.ProcessEnv = process.env): string { // Where a given issue's worktree lives, always absolute regardless of what // cwd the process was started from. -export function worktreePath(issue: number, env: NodeJS.ProcessEnv = process.env): string { - return `${workspacesDir(env)}/issue-${issue}`; +export function worktreePath(issue: number, env: NodeJS.ProcessEnv = process.env, repo?: string): string { + return `${workspacesDir(env, repo)}/issue-${issue}`; } diff --git a/src/reset.ts b/src/reset.ts index 9f3db49..5bb75eb 100644 --- a/src/reset.ts +++ b/src/reset.ts @@ -34,6 +34,17 @@ export interface ResetContext { readonly statePath: string; // Close every open issue, not just the factory's and the seeded ones (a sandbox that holds nothing else). readonly allIssues?: boolean; + // config.resettable, opt-in per repo. A live repo (lwp-website) never sets + // this, so a stray `factory reset`/`rebaseline` cannot force-push it back + // to a baseline tag (plan v2.6.2 item 2). Defaults closed: omitting the + // field refuses, the same as an explicit `false`. + readonly resettable?: boolean; +} + +export class NotResettableError extends Error { + constructor(repo: string) { + super(`${repo} is not resettable: set "resettable": true in .factory/config.json to allow \`factory reset\`/\`rebaseline\` on it`); + } } export interface ResetDeps { @@ -122,6 +133,7 @@ async function checked(deps: ResetDeps, ctx: ResetContext, args: string[]): Prom // Move the baseline tag to origin/: the "keep this merge" command. export async function rebaseline(deps: ResetDeps, ctx: ResetContext, dryRun: boolean): Promise { + if (!ctx.resettable) throw new NotResettableError(ctx.repo); const moved = await commitsAheadOfTag(deps, ctx); if (!dryRun && moved.length) { await checked(deps, ctx, ["tag", "-f", ctx.baselineTag, `origin/${ctx.base}`]); @@ -226,6 +238,7 @@ export interface ResetSummary { } export async function reset(deps: ResetDeps, ctx: ResetContext, dryRun: boolean): Promise { + if (!ctx.resettable) throw new NotResettableError(ctx.repo); const actions = await planReset(deps, ctx); if (!dryRun) { for (const action of actions) { diff --git a/src/scan.ts b/src/scan.ts index 6978bc9..55a800c 100644 --- a/src/scan.ts +++ b/src/scan.ts @@ -20,6 +20,7 @@ // 48 issues — see tests/fixtures/bun-audit-splitbill.json, captured from a // real run, for the shape this parses. +import { existsSync } from "node:fs"; import type { GitHub } from "./github"; import type { CommandRunner } from "./github"; import { LABEL } from "./labels"; @@ -106,9 +107,16 @@ export interface ScanDeps { export interface ScanResult { readonly filed: string[]; // issue titles filed this run readonly skipped: string[]; // findings already open, deduped by marker + // Set instead of running, on a repo `bun audit` has nothing to say about + // (plan v2.6.2 item 6: a Python or npm-only repo must not fail scan, or + // silently parse `bun audit`'s error output as zero findings). + readonly skippedReason?: string; } export async function scan(deps: ScanDeps, repo: string, cloneDir: string): Promise { + if (!existsSync(`${cloneDir}/bun.lock`) && !existsSync(`${cloneDir}/bun.lockb`)) { + return { filed: [], skipped: [], skippedReason: "no bun.lock (or bun.lockb) here; `factory scan` only audits Bun projects" }; + } const audit = await deps.runner.run(["audit", "--json"], { cwd: cloneDir }); const findings = parseAuditFindings(audit.stdout); diff --git a/src/setup.ts b/src/setup.ts new file mode 100644 index 0000000..cc29a10 --- /dev/null +++ b/src/setup.ts @@ -0,0 +1,47 @@ +// A fresh `git worktree add` has none of the target's dependencies installed +// (no node_modules, no vendor dir), so the first agent's shell commands would +// otherwise fail on install alone. `config.setup` (e.g. ["npm ci +// --prefer-offline"]) runs once per worktree, before any stage (plan v2.6.2 +// item 4; the "pre" idea from owainlewis/factory, no code copied). A marker +// under .factory/runs/ — already excluded from commitAll's pathspec, so it +// never lands in a commit — makes this idempotent: a restart on the same +// worktree does not re-run npm ci. + +export interface SetupRunner { + run(cmd: string, cwd: string): Promise<{ stdout: string; stderr: string; code: number }>; +} + +export class ShellSetupRunner implements SetupRunner { + async run(cmd: string, cwd: string): Promise<{ stdout: string; stderr: string; code: number }> { + const proc = Bun.spawn(["bash", "-c", cmd], { cwd, stdout: "pipe", stderr: "pipe" }); + const [stdout, stderr, code] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { stdout, stderr, code }; + } +} + +export interface SetupResult { + readonly ok: boolean; + readonly ran: boolean; // false when the marker already existed or there was nothing to run + readonly log: string; +} + +const MARKER = ".factory/runs/setup.done"; + +export async function ensureSetup(runner: SetupRunner, worktreeDir: string, commands: readonly string[]): Promise { + if (commands.length === 0) return { ok: true, ran: false, log: "" }; + const marker = `${worktreeDir}/${MARKER}`; + if (await Bun.file(marker).exists()) return { ok: true, ran: false, log: "" }; + + const lines: string[] = []; + for (const cmd of commands) { + const result = await runner.run(cmd, worktreeDir); + lines.push(`$ ${cmd}`, result.stdout.trim(), result.stderr.trim()); + if (result.code !== 0) return { ok: false, ran: true, log: lines.filter(Boolean).join("\n") }; + } + await Bun.write(marker, `${new Date().toISOString()}\n`); + return { ok: true, ran: true, log: lines.filter(Boolean).join("\n") }; +} diff --git a/src/watch.ts b/src/watch.ts index 3b06f83..b42d294 100644 --- a/src/watch.ts +++ b/src/watch.ts @@ -38,6 +38,7 @@ import { touchesProtectedPath } from "./boundary"; 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 { LABEL } from "./labels"; @@ -74,6 +75,9 @@ export interface WatchDeps { readonly rechecker?: Rechecker; readonly cloneDir: string; readonly workspacesDir: string; + // Runs config.setup once per worktree; defaults to a real shell so tests + // can inject a fake instead of actually running `npm ci`. + readonly setupRunner?: SetupRunner; } export interface PollResult { @@ -99,6 +103,26 @@ async function moveLabel(deps: WatchDeps, config: FactoryConfig, issueNumber: nu await deps.github.setStateLabel(config.repo, issueNumber, [from], to); } +// The one place every resume path prepares a worktree: creates it if needed, +// then primes it with config.setup (idempotent). A setup failure parks the +// issue with the command output attached instead of handing a stage a +// worktree with no dependencies installed (plan v2.6.2 item 4). +async function ensureWorktreeReady( + deps: WatchDeps, + config: FactoryConfig, + issueNumber: number, + worktree: string, + fromLabel: string, +): Promise { + await deps.git.ensureWorktree(deps.cloneDir, worktree, issueNumber); + const result = await ensureSetup(deps.setupRunner ?? new ShellSetupRunner(), worktree, config.setup); + if (result.ok) return true; + await moveLabel(deps, config, issueNumber, fromLabel, LABEL.needsHuman); + await postComment(deps, config, issueNumber, `Setup failed in the worktree, so this parked instead of starting a stage:\n\n\`\`\`\n${result.log.slice(-4000)}\n\`\`\``); + finish(deps, config, issueNumber, "needs-human", "setup failed"); + return false; +} + function withDataMarker(body: string, stage: string, json: unknown): string { return `${body}\n\n`; } @@ -551,7 +575,7 @@ export async function processReadyIssue(issue: GhIssue, deps: WatchDeps, config: return "lost-claim"; } const worktree = worktreeFor(deps, issue.number); - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.ready))) return "needs-human"; await deps.github.setStateLabel(config.repo, issue.number, [LABEL.ready], LABEL.triaging); return runFromStage(deps, config, issue, "triage", worktree); } @@ -598,7 +622,7 @@ export async function resumeNeedsInfo(issue: GhIssue, deps: WatchDeps, config: F const derived = deriveIssueState(issue); const worktree = worktreeFor(deps, issue.number); - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.needsInfo))) return "needs-human"; await Bun.write(`${worktree}/${runDir(issue.number)}/answer.md`, reply.body); await deps.github.setStateLabel(config.repo, issue.number, [LABEL.needsInfo], STAGE_LABEL[derived.resumeStage]); return runFromStage(deps, config, issue, derived.resumeStage, worktree, ctxFrom(issue)); @@ -616,19 +640,19 @@ export async function resumeAwaitingApproval(issue: GhIssue, deps: WatchDeps, co const worktree = worktreeFor(deps, issue.number); if (command.type === "approve") { - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.awaitingApproval))) return "needs-human"; await deps.github.setStateLabel(config.repo, issue.number, [LABEL.awaitingApproval], LABEL.building); return runFromStage(deps, config, issue, "build", worktree, ctxFrom(issue)); } if (command.type === "revise") { - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.awaitingApproval))) return "needs-human"; await writeRevision(worktree, issue, reply, command.text); await deps.github.setStateLabel(config.repo, issue.number, [LABEL.awaitingApproval], LABEL.planning); return runFromStage(deps, config, issue, "plan", worktree, ctxFrom(issue)); } if (command.type === "cancel") return cancelRun(issue, deps, config); if (command.type === "retry") { - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.awaitingApproval))) return "needs-human"; await deps.github.setStateLabel(config.repo, issue.number, [LABEL.awaitingApproval], LABEL.planning); return runFromStage(deps, config, issue, "plan", worktree, ctxFrom(issue)); } @@ -651,7 +675,7 @@ export async function resumeParked(issue: GhIssue, deps: WatchDeps, config: Fact const derived = deriveIssueState(issue); const worktree = worktreeFor(deps, issue.number); - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, currentLabel))) return "needs-human"; await deps.github.setStateLabel(config.repo, issue.number, [currentLabel], STAGE_LABEL[derived.resumeStage]); return runFromStage(deps, config, issue, derived.resumeStage, worktree, { rejectRound: derived.rejectRounds, @@ -678,7 +702,7 @@ export async function resumeInReview(issue: GhIssue, deps: WatchDeps, config: Fa if (command.type !== "revise") return undefined; const worktree = worktreeFor(deps, issue.number); - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, LABEL.inReview))) return "needs-human"; await writeRevision(worktree, issue, latest, command.text); const head = deps.git.branchName(issue.number); if (await deps.github.findPrByHead(config.repo, head)) await deps.github.markReady(config.repo, head, false); @@ -742,7 +766,7 @@ export async function recoverInFlight(deps: WatchDeps, config: FactoryConfig): P await runPool(issues, config.concurrency, async (issue) => { const derived = deriveIssueState(issue); const worktree = worktreeFor(deps, issue.number); - await deps.git.ensureWorktree(deps.cloneDir, worktree, issue.number); + if (!(await ensureWorktreeReady(deps, config, issue.number, worktree, STAGE_LABEL[derived.resumeStage]))) return "needs-human"; return runFromStage(deps, config, issue, derived.resumeStage, worktree, { rejectRound: derived.rejectRounds, questionRound: derived.questionRounds, diff --git a/teach/sessions.json b/teach/sessions.json index c1190e7..a4b4a1d 100644 --- a/teach/sessions.json +++ b/teach/sessions.json @@ -6,8 +6,8 @@ { "id": "lesson-2", "title": "Execution: any agent, checked and costed", "tag": "v2.5.2", "planned": false, "checkpoint": "02-execution", "issue": "02-split-remainder-bug.md", "demo": ["Live sequence 1", "Live sequence 3"] }, { "id": "interim-b", "title": "One factory, any agent", "tag": "v2.6.1", "planned": false, "checkpoint": null, "issue": "02-split-remainder-bug.md", "demo": ["Live sequence 1"] }, { "id": "lesson-3", "title": "Context routing", "tag": "v2.7.0", "planned": true, "checkpoint": "03-context", "issue": "04-search-sql-injection.md", "demo": ["Live sequence 3"] }, - { "id": "lesson-5", "title": "Self-healing loop", "tag": "v2.8.0", "planned": true, "checkpoint": null, "issue": "01-readme-missing-run-steps.md", "demo": ["Live sequence 1"] }, - { "id": "lesson-6", "title": "Risk-gated merge", "tag": "v2.9.0", "planned": true, "checkpoint": "06-delivery", "issue": "01-readme-missing-run-steps.md", "demo": ["Deliver and reset"] }, - { "id": "interim-c", "title": "Enterprise hardening", "tag": "v3.0.0", "planned": true, "checkpoint": null, "issue": "05-delete-expense-idor.md", "demo": ["Live sequence 5"] } + { "id": "lesson-6", "title": "Risk-gated merge", "tag": "v2.8.0", "planned": true, "checkpoint": "06-delivery", "issue": "01-readme-missing-run-steps.md", "demo": ["Deliver and reset"] }, + { "id": "lesson-5", "title": "Self-healing loop", "tag": "v3.0.0", "planned": true, "checkpoint": null, "issue": "01-readme-missing-run-steps.md", "demo": ["Live sequence 1"] }, + { "id": "interim-c", "title": "Enterprise hardening", "tag": "v3.3.0", "planned": true, "checkpoint": null, "issue": "05-delete-expense-idor.md", "demo": ["Live sequence 5"] } ] } diff --git a/template-ci/factory.yml.example b/template-ci/factory.yml.example index b21a332..0e4735c 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.6.1 # pinned software-factory release; bump deliberately + FACTORY_RUNNER_REF: v2.6.2 # 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 @@ -69,6 +69,15 @@ jobs: - name: Checkout target repo uses: actions/checkout@v4 + # A placeholder, not a no-op you can skip: `bun ci`/`npm ci` alone is not + # enough on a fresh Actions runner for a stack other than Bun/Node (a + # Python repo needs actions/setup-python first, etc). Add whatever this + # repo's stack needs here, before `factory run` builds and gates on it. + # `config.setup` in .factory/config.json (e.g. `npm ci`) still runs + # after this, once per worktree — this step is for the runtime itself. + - name: Set up your toolchain + run: echo "no language runtime needed beyond what ubuntu-latest ships, or add a setup step here" + - name: Checkout software-factory (pinned) uses: actions/checkout@v4 with: @@ -137,6 +146,10 @@ jobs: timeout-minutes: 60 steps: - uses: actions/checkout@v4 + # See run-issue's "Set up your toolchain" step: `factory tick` can also + # build and gate an issue, so it needs this repo's runtime too. + - name: Set up your toolchain + run: echo "no language runtime needed beyond what ubuntu-latest ships, or add a setup step here" - uses: actions/checkout@v4 with: repository: ${{ env.FACTORY_RUNNER_REPO }} @@ -214,6 +227,9 @@ jobs: timeout-minutes: 60 steps: - uses: actions/checkout@v4 + # See run-issue's "Set up your toolchain" step. + - name: Set up your toolchain + run: echo "no language runtime needed beyond what ubuntu-latest ships, or add a setup step here" - uses: actions/checkout@v4 with: repository: ${{ env.FACTORY_RUNNER_REPO }} diff --git a/template/.claude/skills/factory-comment/assets/verdict.md b/template/.claude/skills/factory-comment/assets/verdict.md index e3d8e4a..333e667 100644 --- a/template/.claude/skills/factory-comment/assets/verdict.md +++ b/template/.claude/skills/factory-comment/assets/verdict.md @@ -7,7 +7,7 @@ {{/each}} ### The test that bites -`{{test_name}}` fails on `main` ({{failing_output_snippet}}), passes here +`{{test_name}}` fails on the base branch ({{failing_output_snippet}}), passes here (`{{command}}` → {{passing_output_snippet}}). ### Reviewer findings diff --git a/template/.claude/skills/factory-pr/SKILL.md b/template/.claude/skills/factory-pr/SKILL.md index 9a96eee..011fbfb 100644 --- a/template/.claude/skills/factory-pr/SKILL.md +++ b/template/.claude/skills/factory-pr/SKILL.md @@ -22,20 +22,26 @@ it. Only reachable after `factory-verify` wrote `result: "pass"`. If a PR template exists, fill its sections from the plan and verdict; do not invent sections it doesn't have or drop ones it does. If no template -exists, use this structure: +exists, use this structure (the same shape as the one this template ships +in `.github/pull_request_template.md`): ```markdown ## Summary {{one_line_goal}} -## Acceptance criteria -- AC-1: {{criterion}} — verified by {{evidence_command}} -... +Closes #{{issue_number}} -## Non-goals respected -{{ng_summary}} +## Plan +{{link_to_plan_comment}}: AC-1 {{criterion}}, verified by {{evidence_command}} ... -Closes #{{issue_number}} +## Gate evidence +{{gate_line_verbatim}} + +## Verify verdict +{{link_to_verdict_comment}}: {{test_that_bites_summary}} + +## Risk and rollback +{{risk_level_from_plan}}: {{rollback_note}} ``` Keep it factual and short: what changed, how each AC was checked, what was @@ -44,7 +50,7 @@ comment; link to it instead of copying it. ## 3. Write the outputs -Write `.factory/runs/issue-/pr-body.md` with the filled body above — +Write `.factory/runs/issue-/pr-body.md` with the filled body above; this is the only file this stage reads back. The runner opens the PR as a draft, titled from the issue itself (` (#)`), with this file as the body, then sets the issue's label to `factory:in-review`. diff --git a/template/.claude/skills/factory-verify/SKILL.md b/template/.claude/skills/factory-verify/SKILL.md index 72fc3f8..a1476f0 100644 --- a/template/.claude/skills/factory-verify/SKILL.md +++ b/template/.claude/skills/factory-verify/SKILL.md @@ -54,7 +54,7 @@ finding you could not reproduce from the diff. Use `factory-comment`'s `verdict.md` template for `.factory/runs/issue-/verdict-comment.md`: per-AC pass/fail with the evidence command and result, the test-that-bites (name, failing output on -`main`, passing output here), reviewer findings verbatim, a summary of +the base branch, passing output here), reviewer findings verbatim, a summary of non-goals respected, and — on reject — which round this is. Then write `.factory/runs/issue-/verdict.json`: diff --git a/template/.github/ISSUE_TEMPLATE/bug.yml b/template/.github/ISSUE_TEMPLATE/bug.yml new file mode 100644 index 0000000..2d3acd2 --- /dev/null +++ b/template/.github/ISSUE_TEMPLATE/bug.yml @@ -0,0 +1,27 @@ +name: Bug +description: Something behaves incorrectly. +labels: ["bug"] +body: + - type: textarea + id: what-happened + attributes: + label: What happened + description: What did you expect, and what happened instead? + validations: + required: true + - type: textarea + id: repro + attributes: + label: Steps to reproduce + validations: + required: true + - type: textarea + id: evidence + attributes: + label: Evidence + description: Logs, a stack trace, or a screenshot, if you have one. + - type: markdown + attributes: + value: | + This only sets the `bug` label. Add `factory:ready` once it is + triaged and you want the factory to pick it up. diff --git a/template/.github/ISSUE_TEMPLATE/docs.yml b/template/.github/ISSUE_TEMPLATE/docs.yml new file mode 100644 index 0000000..ec3080a --- /dev/null +++ b/template/.github/ISSUE_TEMPLATE/docs.yml @@ -0,0 +1,20 @@ +name: Docs +description: Documentation only, no code change. +labels: ["docs"] +body: + - type: textarea + id: what + attributes: + label: What is missing, wrong, or unclear + validations: + required: true + - type: textarea + id: where + attributes: + label: Where this should live + description: The file or page, if you know it. + - type: markdown + attributes: + value: | + This only sets the `docs` label. Add `factory:ready` once it is + triaged and you want the factory to pick it up. diff --git a/template/.github/ISSUE_TEMPLATE/feature.yml b/template/.github/ISSUE_TEMPLATE/feature.yml new file mode 100644 index 0000000..00ced91 --- /dev/null +++ b/template/.github/ISSUE_TEMPLATE/feature.yml @@ -0,0 +1,27 @@ +name: Feature +description: A new capability. +labels: ["feature"] +body: + - type: textarea + id: goal + attributes: + label: What should this do, and why + validations: + required: true + - type: textarea + id: acceptance + attributes: + label: Acceptance criteria + description: One checkable statement per line; factory-plan turns these into AC-n. + validations: + required: true + - type: textarea + id: non-goals + attributes: + label: Non-goals + description: What this should deliberately not cover. + - type: markdown + attributes: + value: | + This only sets the `feature` label. Add `factory:ready` once it is + triaged and you want the factory to pick it up. diff --git a/template/.github/pull_request_template.md b/template/.github/pull_request_template.md new file mode 100644 index 0000000..7ca4f6a --- /dev/null +++ b/template/.github/pull_request_template.md @@ -0,0 +1,25 @@ + + +## Summary + + + +Closes # + +## Plan + + + +## Gate evidence + + + +## Verify verdict + + + +## Risk and rollback + + diff --git a/tests/app-agnostic.test.ts b/tests/app-agnostic.test.ts index 26858a9..a9470b2 100644 --- a/tests/app-agnostic.test.ts +++ b/tests/app-agnostic.test.ts @@ -8,6 +8,7 @@ import { tmpdir } from "node:os"; import { join, relative } from "node:path"; import { loadConfig, repoFromRemoteUrl } from "../src/config"; import { parseGateLine } from "../src/gates"; +import { defaultStatePath, workspacesDir, worktreePath } from "../src/paths"; const ROOT = join(import.meta.dir, ".."); @@ -96,3 +97,36 @@ describe("a Python repo on a trunk branch", () => { expect(runGatesSh(dir)).toBe("RED"); }); }); + +// The other half of "any repo, safely" (plan v2.6.2 item 1): a second repo +// sharing one FACTORY_HOME must never share a worktree or a state DB with +// the first, even when both hold the same issue number. +describe("two repos sharing one FACTORY_HOME never collide", () => { + const env = { HOME: "/tmp/does-not-matter", FACTORY_HOME: "/tmp/factory-home-shared" } as NodeJS.ProcessEnv; + + test("workspaces and state DBs are namespaced by owner/name", () => { + const a = { workspaces: workspacesDir(env, "acme/pyapp"), db: defaultStatePath(env, "acme/pyapp") }; + const b = { workspaces: workspacesDir(env, "acme/other"), db: defaultStatePath(env, "acme/other") }; + expect(a.workspaces).not.toBe(b.workspaces); + expect(a.db).not.toBe(b.db); + expect(a.workspaces.startsWith("/tmp/factory-home-shared/acme/pyapp")).toBe(true); + expect(b.workspaces.startsWith("/tmp/factory-home-shared/acme/other")).toBe(true); + }); + + test("the same issue number in two repos never resolves to the same worktree", () => { + const a = worktreePath(3, env, "acme/pyapp"); + const b = worktreePath(3, env, "acme/other"); + expect(a).not.toBe(b); + expect(a.endsWith("/issue-3")).toBe(true); + expect(b.endsWith("/issue-3")).toBe(true); + }); + + test("omitting repo keeps the pre-v2.6.2 shared path, so an unmigrated single-repo install is unaffected", () => { + expect(workspacesDir(env)).toBe(`${env.FACTORY_HOME}/workspaces`); + expect(defaultStatePath(env)).toBe(`${env.FACTORY_HOME}/factory.db`); + }); + + test("a repo slug with no slash refuses instead of silently sharing a path", () => { + expect(() => workspacesDir(env, "not-a-slug")).toThrow(/repo must be "owner\/name"/); + }); +}); diff --git a/tests/config.test.ts b/tests/config.test.ts index 3c97a94..6b16a33 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -34,6 +34,46 @@ describe("loadConfig", () => { }); }); +// A config generated for a repo whose default branch is not "main" must not +// silently point PRs and reset at a branch that does not exist (plan v2.6.2 +// item 3). Local `--repo-dir` mode has no `ensureRepoClone` (VM/CI-only) to +// resolve this, so loadConfig has to do it itself. +describe("base branch defaults to origin/HEAD, not the hard-coded \"main\"", () => { + function git(cwd: string, ...args: string[]): void { + const r = Bun.spawnSync(["git", ...args], { cwd, stderr: "pipe" }); + if (r.exitCode !== 0) throw new Error(`git ${args.join(" ")}: ${r.stderr.toString()}`); + } + + function cloneOfTrunkRepo(): string { + const src = mkdtempSync(join(dir, "src-")); + git(src, "init", "-q", "-b", "trunk"); + writeFileSync(join(src, "f.txt"), "x"); + git(src, "add", "."); + git(src, "-c", "user.email=a@b.c", "-c", "user.name=a", "commit", "-q", "-m", "x"); + const bare = `${mkdtempSync(join(dir, "bare-"))}.git`; + git(dir, "clone", "-q", "--bare", src, bare); + const wd = join(dir, `wd-${Math.random().toString(36).slice(2)}`); + git(dir, "clone", "-q", bare, wd); + return wd; + } + + test("a config that omits base picks up origin/HEAD's branch", async () => { + const wd = cloneOfTrunkRepo(); + mkdirSync(join(wd, ".factory")); + writeFileSync(join(wd, ".factory/config.json"), JSON.stringify({ repo: "acme/x", gates: [] })); + const c = await loadConfig(wd); + expect(c.base).toBe("trunk"); + }); + + test("a config that names base explicitly, even \"main\", is never overridden", async () => { + const wd = cloneOfTrunkRepo(); + mkdirSync(join(wd, ".factory")); + writeFileSync(join(wd, ".factory/config.json"), JSON.stringify({ repo: "acme/x", base: "main", gates: [] })); + const c = await loadConfig(wd); + expect(c.base).toBe("main"); + }); +}); + describe("config validation at boot", () => { test("an unknown top-level key refuses to start and names the key", async () => { await expect(loadConfig(repoWith({ repo: "a/b", maxOpenFactoryPr: 3 }))).rejects.toThrow(/maxOpenFactoryPr: unknown key/); diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index 04685e3..0497a81 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -99,6 +99,44 @@ describe("runDoctor", () => { }); }); +// Plan v2.6.2 item 1: state moved from one shared FACTORY_HOME/factory.db to +// FACTORY_HOME///factory.db, and the previous design decision +// stands — never auto-migrate a shared DB into the new per-repo layout, only +// warn. No `legacyStatePath`/`legacyWorkspacesDir` in `ctx` (as in every test +// above) means the check is skipped entirely, not silently passing. +describe("legacy shared state warning", () => { + test("silent when the caller passes no legacy paths to check", async () => { + const checks = await runDoctor(deps(), ctx); + expect(checks.some((c) => c.name.includes("legacy"))).toBe(false); + }); + + test("warns, but never fails the run, when the legacy factory.db still exists", async () => { + const checks = await runDoctor(deps(), { ...ctx, legacyStatePath: "/home/.factory/factory.db" }); + const check = checks.find((c) => c.name.includes("factory.db"))!; + expect(check.ok).toBe(false); + expect(check.warn).toBe(true); + expect(check.fixable).toBe(false); + expect(check.detail).toContain("/home/.factory/factory.db"); + expect(checks.every((c) => c.ok || c.warn)).toBe(true); // an all-warn run still counts as passing + }); + + test("warns when the legacy workspaces/ dir still exists", async () => { + const checks = await runDoctor(deps(), { ...ctx, legacyWorkspacesDir: "/home/.factory/workspaces" }); + const check = checks.find((c) => c.name.includes("workspaces"))!; + expect(check.ok).toBe(false); + expect(check.warn).toBe(true); + }); + + test("passes clean when the legacy paths are given but nothing is there", async () => { + const checks = await runDoctor( + { ...deps(), fileExists: async () => false }, + { ...ctx, legacyStatePath: "/home/.factory/factory.db", legacyWorkspacesDir: "/home/.factory/workspaces" }, + ); + expect(checks.find((c) => c.name.includes("factory.db"))!.ok).toBe(true); + expect(checks.find((c) => c.name.includes("workspaces"))!.ok).toBe(true); + }); +}); + describe("agents in doctor", () => { const agents = { claude: { preset: "claude" }, codex: { preset: "codex" }, aider: { command: ["aider", "--yes"] }, unused: { preset: "codex" } }; diff --git a/tests/install.test.ts b/tests/install.test.ts index c661fb1..511773a 100644 --- a/tests/install.test.ts +++ b/tests/install.test.ts @@ -33,6 +33,37 @@ describe("install.sh (default)", () => { rmSync(target, { recursive: true, force: true }); }); + // The `src/labels.ts:48` claim ("set by the issue form") is only true once + // install actually ships a form (plan v2.6.2 item 5); this fails if the + // template stops shipping one or a form starts applying `factory:ready`. + test("ships issue forms (type label only) and a PR template", () => { + const target = scratchTarget(); + run([target]); + for (const type of ["bug", "feature", "docs"]) { + const path = join(target, ".github", "ISSUE_TEMPLATE", `${type}.yml`); + expect(existsSync(path)).toBe(true); + const body = readFileSync(path, "utf8"); + const labelsLine = body.split("\n").find((l) => l.startsWith("labels:")); + expect(labelsLine).toBe(`labels: ["${type}"]`); // the form applies only its type label + } + const pr = readFileSync(join(target, ".github", "pull_request_template.md"), "utf8"); + for (const heading of ["## Summary", "## Plan", "## Gate evidence", "## Verify verdict", "## Risk and rollback"]) { + expect(pr).toContain(heading); + } + rmSync(target, { recursive: true, force: true }); + }); + + // `src/labels.ts:48` claims type labels are "set by the issue form" — true + // only once something actually creates those labels on the repo. The form + // itself can't (a GitHub issue form applies a label, it doesn't create + // one), so install's own next-step points at the command that does. + test("tells the user to run `factory doctor --fix`, which is what actually creates the labels", () => { + const target = scratchTarget(); + const { stdout } = run([target]); + expect(stdout).toContain("factory doctor --fix"); + rmSync(target, { recursive: true, force: true }); + }); + test("never overwrites a file that already exists", () => { const target = scratchTarget(); mkdirSync(join(target, ".claude"), { recursive: true }); diff --git a/tests/reset.test.ts b/tests/reset.test.ts index 511083f..a92edd3 100644 --- a/tests/reset.test.ts +++ b/tests/reset.test.ts @@ -8,7 +8,7 @@ import { describe, expect, test } from "bun:test"; import { mkdtempSync, mkdirSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { parseIssueSeed, planReset, rebaseline, reset, type ResetDeps } from "../src/reset"; +import { NotResettableError, parseIssueSeed, planReset, rebaseline, reset, type ResetDeps } from "../src/reset"; import { GitHub, type CommandResult, type CommandRunner, type GhIssue, type GhPr } from "../src/github"; import { LABELS } from "../src/labels"; @@ -120,6 +120,7 @@ describe("factory reset --dry-run", () => { issuesDir, workspacesDir: "/tmp/factory-ws-does-not-exist", statePath: "/tmp/factory-state-does-not-exist", + resettable: true, }; const actions = await planReset(deps, ctx); @@ -169,6 +170,7 @@ describe("factory reset --dry-run", () => { issuesDir, workspacesDir: mkdtempSync(join(tmpdir(), "factory-ws-")), statePath: mkdtempSync(join(tmpdir(), "factory-state-")), + resettable: true, }; mkdirSync(ctx.workspacesDir, { recursive: true }); @@ -191,7 +193,7 @@ describe("reset wipes the WAL files with the state database", () => { const db = join(dir, "factory.db"); for (const f of [db, `${db}-wal`, `${db}-shm`]) writeFileSync(f, "x"); const github = new FakeGitHub([], []); - const ctx = { repo: "acme/widgets", cloneDir: "/tmp/x", baselineTag: "baseline", base: "trunk", issuesDir: mkdtempSync(join(tmpdir(), "factory-i-")), workspacesDir: join(dir, "ws"), statePath: db }; + const ctx = { repo: "acme/widgets", cloneDir: "/tmp/x", baselineTag: "baseline", base: "trunk", issuesDir: mkdtempSync(join(tmpdir(), "factory-i-")), workspacesDir: join(dir, "ws"), statePath: db, resettable: true }; await reset({ github, git: new FakeGitRunner() }, ctx, false); expect(readdirSync(dir)).toEqual([]); rmSync(dir, { recursive: true, force: true }); @@ -207,6 +209,7 @@ describe("reset keeps merged setup safe", () => { issuesDir, workspacesDir: "/tmp/factory-ws-none", statePath: "/tmp/factory-state-none", + resettable: true, }); test("plan lists the commits the force push would drop", async () => { @@ -270,3 +273,26 @@ describe("reset stays inside what the factory owns", () => { await expect(planReset({ github: new FakeGitHub([issue(1, "x", ["factory:ready"])], []), git }, ctx)).rejects.toThrow(/baseline tag baseline not found/); }); }); + +describe("reset refuses on a repo that never opted in", () => { + const ctx = { repo: "acme/widgets", cloneDir: "/tmp/x", baselineTag: "baseline", base: "main", issuesDir: "/tmp/none", workspacesDir: "/tmp/ws-none", statePath: "/tmp/st-none" }; + + test("reset() without resettable throws before touching gh or git", async () => { + const github = new FakeGitHub([issue(1, "Old bug", ["factory:failed"])], [pr(5, "factory/issue-3")]); + const git = new FakeGitRunner(); + await expect(reset({ github, git }, ctx, false)).rejects.toThrow(NotResettableError); + expect(github.closedPrs).toEqual([]); + expect(github.closedIssues).toEqual([]); + expect(git.calls).toEqual([]); + }); + + test("reset() with resettable: false throws the same as omitting it", async () => { + await expect(reset({ github: new FakeGitHub([], []), git: new FakeGitRunner() }, { ...ctx, resettable: false }, true)).rejects.toThrow(NotResettableError); + }); + + test("rebaseline() without resettable throws before moving the tag", async () => { + const git = new FakeGitRunner(); + await expect(rebaseline({ github: new FakeGitHub([], []), git }, ctx, false)).rejects.toThrow(NotResettableError); + expect(git.calls).toEqual([]); + }); +}); diff --git a/tests/scan.test.ts b/tests/scan.test.ts index 8c3e00d..def55e5 100644 --- a/tests/scan.test.ts +++ b/tests/scan.test.ts @@ -5,8 +5,9 @@ // `Record` with neither field, which would have parsed // to zero findings against the live repo (audit finding #6). -import { describe, expect, test } from "bun:test"; -import { readFileSync } from "node:fs"; +import { afterAll, describe, expect, test } from "bun:test"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { parseAuditFindings, scan, type ScanDeps } from "../src/scan"; import { GitHub, type CommandResult, type CommandRunner, type GhIssue } from "../src/github"; @@ -14,6 +15,12 @@ import { LABEL } from "../src/labels"; const FIXTURE = readFileSync(join(import.meta.dir, "fixtures", "bun-audit-splitbill.json"), "utf8"); +// A Bun project, as far as scan() checks: it never shells out to `bun audit` +// without this marker (plan v2.6.2 item 6). +const bunProjectDir = mkdtempSync(join(tmpdir(), "factory-scan-")); +writeFileSync(join(bunProjectDir, "bun.lock"), "{}"); +afterAll(() => rmSync(bunProjectDir, { recursive: true, force: true })); + describe("parseAuditFindings", () => { test("groups the real splitbill capture into one finding per package, not per advisory", () => { const findings = parseAuditFindings(FIXTURE); @@ -87,7 +94,7 @@ describe("scan()", () => { // live scan should only file nanoid. const github = new FakeGitHub([issue(6, "some body\n\nmore text")]); const deps: ScanDeps = { github, runner: new FakeBunRunner(FIXTURE) }; - const result = await scan(deps, "acme/widgets", "/tmp/does-not-matter"); + const result = await scan(deps, "acme/widgets", bunProjectDir); expect(result.skipped).toHaveLength(1); expect(result.filed).toHaveLength(1); @@ -99,7 +106,7 @@ describe("scan()", () => { test("files both findings when nothing is open yet", async () => { const github = new FakeGitHub([]); const deps: ScanDeps = { github, runner: new FakeBunRunner(FIXTURE) }; - const result = await scan(deps, "acme/widgets", "/tmp/does-not-matter"); + const result = await scan(deps, "acme/widgets", bunProjectDir); expect(result.filed).toHaveLength(2); expect(result.skipped).toHaveLength(0); }); @@ -113,7 +120,24 @@ describe("scan()", () => { } } const github = new FakeGitHub([]); - await scan({ github, runner: new RecordingRunner() }, "acme/widgets", "/tmp/x"); + await scan({ github, runner: new RecordingRunner() }, "acme/widgets", bunProjectDir); expect(calls).toEqual([["audit", "--json"]]); }); + + test("skips with a clear message on a repo with no bun.lock, never shelling out", async () => { + const calls: string[][] = []; + class RecordingRunner implements CommandRunner { + async run(args: string[]): Promise { + calls.push(args); + return { stdout: "{}", stderr: "", code: 0 }; + } + } + const noBunDir = mkdtempSync(join(tmpdir(), "factory-scan-py-")); + const github = new FakeGitHub([]); + const result = await scan({ github, runner: new RecordingRunner() }, "acme/pyapp", noBunDir); + expect(result).toEqual({ filed: [], skipped: [], skippedReason: expect.stringContaining("bun.lock") }); + expect(calls).toEqual([]); + expect(github.created).toEqual([]); + rmSync(noBunDir, { recursive: true, force: true }); + }); }); diff --git a/tests/setup.test.ts b/tests/setup.test.ts new file mode 100644 index 0000000..8fbe465 --- /dev/null +++ b/tests/setup.test.ts @@ -0,0 +1,68 @@ +// A fresh worktree has no dependencies installed; ensureSetup runs +// config.setup once and never again, and a failing command stops the list +// and reports its output instead of silently continuing. + +import { describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { ensureSetup, type SetupRunner } from "../src/setup"; + +class FakeSetupRunner implements SetupRunner { + calls: string[] = []; + failOn?: string; + + async run(cmd: string, _cwd: string) { + this.calls.push(cmd); + if (cmd === this.failOn) return { stdout: "", stderr: "npm ERR! no lockfile", code: 1 }; + return { stdout: `ran ${cmd}`, stderr: "", code: 0 }; + } +} + +function tmpWorktree(): string { + return mkdtempSync(join(tmpdir(), "factory-setup-")); +} + +describe("ensureSetup", () => { + test("no commands: a no-op that never calls the runner", async () => { + const runner = new FakeSetupRunner(); + const result = await ensureSetup(runner, tmpWorktree(), []); + expect(result).toEqual({ ok: true, ran: false, log: "" }); + expect(runner.calls).toEqual([]); + }); + + test("runs every command in order and writes the marker", async () => { + const dir = tmpWorktree(); + const runner = new FakeSetupRunner(); + const result = await ensureSetup(runner, dir, ["npm ci", "npm run build"]); + expect(result.ok).toBe(true); + expect(result.ran).toBe(true); + expect(result.log).toContain("ran npm ci"); + expect(runner.calls).toEqual(["npm ci", "npm run build"]); + expect(await Bun.file(join(dir, ".factory/runs/setup.done")).exists()).toBe(true); + rmSync(dir, { recursive: true, force: true }); + }); + + test("a second call on the same worktree does not re-run the commands", async () => { + const dir = tmpWorktree(); + const runner = new FakeSetupRunner(); + await ensureSetup(runner, dir, ["npm ci"]); + const second = await ensureSetup(runner, dir, ["npm ci"]); + expect(second).toEqual({ ok: true, ran: false, log: "" }); + expect(runner.calls).toEqual(["npm ci"]); // only the first call ran it + rmSync(dir, { recursive: true, force: true }); + }); + + test("a failing command stops the list, reports its output, and never writes the marker", async () => { + const dir = tmpWorktree(); + const runner = new FakeSetupRunner(); + runner.failOn = "npm ci"; + const result = await ensureSetup(runner, dir, ["npm ci", "npm run build"]); + expect(result.ok).toBe(false); + expect(result.ran).toBe(true); + expect(result.log).toContain("npm ERR! no lockfile"); + expect(runner.calls).toEqual(["npm ci"]); // the second command never ran + expect(await Bun.file(join(dir, ".factory/runs/setup.done")).exists()).toBe(false); + rmSync(dir, { recursive: true, force: true }); + }); +});