From e59700325a894a38131fbb25b167b0f4c983a436 Mon Sep 17 00:00:00 2001 From: artale Date: Sun, 5 Jul 2026 00:45:35 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20review=20fixes=20=E2=80=94=20dead=20code?= =?UTF-8?q?,=20zero-test=20gate,=20safeBranch=20regex,=20worktree=20collis?= =?UTF-8?q?ion=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove dead worktreeExists() and unused WorktreeSpec import (#1, #2) - verify-gate: require counts.total > 0 — silent runner no longer passes (#4) - worktree-isolation: add /g flag to safeBranch regex (#5) - execution-pipeline: try/catch around wm.create with prune-and-retry (#7) - Tests updated to output parseable '1 passed' for mock test commands 69 files, 295 tests. --- src/fable5/execution-pipeline.test.ts | 8 +++---- src/fable5/execution-pipeline.ts | 33 ++++++++++++++------------- src/fable5/verify-gate.ts | 3 ++- src/fable5/worktree-isolation.ts | 2 +- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/fable5/execution-pipeline.test.ts b/src/fable5/execution-pipeline.test.ts index c4dae0e..9b84abb 100644 --- a/src/fable5/execution-pipeline.test.ts +++ b/src/fable5/execution-pipeline.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi, beforeAll, afterAll } from "vitest"; import * as path from "node:path"; import * as os from "node:os"; import * as fs from "node:fs"; @@ -44,7 +44,7 @@ describe("execution pipeline", () => { agentPrompt: "do the thing", repoRoot: testRoot, buildCmd: [process.execPath, "-e", "process.exit(0)"], - testCmd: [process.execPath, "-e", "process.exit(0)"], + testCmd: [process.execPath, "-e", "console.log('1 passed'); process.exit(0)"], runAgent: makeMockAgent("done"), interruptOpts: { defaultValue: "allow" }, }; @@ -83,7 +83,7 @@ describe("execution pipeline", () => { agentPrompt: "do the thing", repoRoot: testRoot, buildCmd: [process.execPath, "-e", "process.exit(0)"], - testCmd: [process.execPath, "-e", "process.exit(0)"], + testCmd: [process.execPath, "-e", "console.log('1 passed'); process.exit(0)"], runAgent: makeMockAgent("done"), interruptOpts: { defaultValue: "deny" }, }; @@ -108,7 +108,7 @@ describe("execution pipeline", () => { agentPrompt: "original prompt", repoRoot: testRoot, buildCmd: [process.execPath, "-e", "process.exit(0)"], - testCmd: [process.execPath, "-e", "process.exit(0)"], + testCmd: [process.execPath, "-e", "console.log('1 passed'); process.exit(0)"], runAgent: capturingAgent, interruptOpts: { defaultValue: "allow" }, }; diff --git a/src/fable5/execution-pipeline.ts b/src/fable5/execution-pipeline.ts index 127a52b..9fa355b 100644 --- a/src/fable5/execution-pipeline.ts +++ b/src/fable5/execution-pipeline.ts @@ -1,4 +1,4 @@ -import { WorktreeSpec, WorktreeManager } from "./worktree-isolation.js"; +import { WorktreeManager } from "./worktree-isolation.js"; import { VerifyGateSpec, VerifyGateResult, runVerifyGate } from "./verify-gate.js"; import { humanInterruptGate, InterruptGateOptions } from "./interrupt-gate.js"; import { FlatLedger, LedgerEntry } from "./flat-ledger.js"; @@ -26,16 +26,6 @@ export interface PipelineResult { durationMs: number; } -function worktreeExists(wm: WorktreeManager, name: string): boolean { - try { - const path = (wm as any).baseDir; - if (!path) return false; - return require("node:fs").existsSync(require("node:path").join(path, name)); - } catch { - return false; - } -} - export async function runPipeline(spec: PipelineSpec): Promise { const start = Date.now(); const session = FlatLedger.sessionNameFromTask(spec.taskName); @@ -45,11 +35,22 @@ export async function runPipeline(spec: PipelineSpec): Promise { // 1. Create worktree wm.initDir(); - const worktreePath = wm.create({ - name: worktreeName, - branch: spec.worktreeBranch ?? `pipeline/${session}`, - targetDir: spec.repoRoot, - }); + let worktreePath: string; + try { + worktreePath = wm.create({ + name: worktreeName, + branch: spec.worktreeBranch ?? `pipeline/${session}`, + targetDir: spec.repoRoot, + }); + } catch (e: any) { + // Worktree or branch already exists from a prior run — prune and retry once + wm.prune(); + worktreePath = wm.create({ + name: worktreeName, + branch: `${spec.worktreeBranch ?? `pipeline/${session}`}-${Date.now()}`, + targetDir: spec.repoRoot, + }); + } // 2. Inject repo map into prompt const repoMap = buildRepoMap(spec.repoRoot, 3); diff --git a/src/fable5/verify-gate.ts b/src/fable5/verify-gate.ts index c53cdbe..e06a916 100644 --- a/src/fable5/verify-gate.ts +++ b/src/fable5/verify-gate.ts @@ -44,6 +44,7 @@ function parseTestOutput(output: string): { passed: number; failed: number; tota export function runVerifyGate(spec: VerifyGateSpec): VerifyGateResult { const start = Date.now(); + // ponytail: worktrees have a .git *file* (pointer), not a dir — fs.existsSync covers both if (!fs.existsSync(path.join(spec.cwd, ".git"))) { return { build: { pass: false, exitCode: -1, output: "" }, @@ -69,7 +70,7 @@ export function runVerifyGate(spec: VerifyGateSpec): VerifyGateResult { } const testResult = run(spec.testCmd, spec.cwd); const counts = parseTestOutput(testResult.output); - const testPass = testResult.exitCode === 0 && counts.failed === 0; + const testPass = testResult.exitCode === 0 && counts.failed === 0 && counts.total > 0; const test = { pass: testPass, exitCode: testResult.exitCode, output: testResult.output, ...counts }; return { build, diff --git a/src/fable5/worktree-isolation.ts b/src/fable5/worktree-isolation.ts index 10a1b51..5140a99 100644 --- a/src/fable5/worktree-isolation.ts +++ b/src/fable5/worktree-isolation.ts @@ -165,6 +165,6 @@ export class WorktreeManager { } private safeBranch(value: string): string { - return this.safeName(value).replace(/\/+/, "-"); + return this.safeName(value).replace(/\/+/g, "-"); } }