diff --git a/packages/gittensory-miner/lib/attempt-cli.d.ts b/packages/gittensory-miner/lib/attempt-cli.d.ts index fdf90d5879..922a913be9 100644 --- a/packages/gittensory-miner/lib/attempt-cli.d.ts +++ b/packages/gittensory-miner/lib/attempt-cli.d.ts @@ -7,6 +7,7 @@ import type { GovernorLedger } from "./governor-ledger.js"; import type { WorktreeAllocator } from "./worktree-allocator.js"; import type { resolveRejectionSignaled } from "./rejection-signal.js"; import type { SelfReviewContextFetch } from "./self-review-context.js"; +import type { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; export type ParsedAttemptArgs = | { error: string } @@ -32,6 +33,8 @@ export type RunAttemptOptions = { buildAttemptDeps?: typeof buildAttemptDeps; resolveRejectionSignaled?: typeof resolveRejectionSignaled; fetchImpl?: SelfReviewContextFetch; + prepareAttemptWorktree?: typeof prepareAttemptWorktree; + cleanupAttemptWorktree?: typeof cleanupAttemptWorktree; }; export function runAttempt(args: string[], options?: RunAttemptOptions): Promise; diff --git a/packages/gittensory-miner/lib/attempt-cli.js b/packages/gittensory-miner/lib/attempt-cli.js index 04e997f49b..1362672995 100644 --- a/packages/gittensory-miner/lib/attempt-cli.js +++ b/packages/gittensory-miner/lib/attempt-cli.js @@ -23,6 +23,7 @@ import { initAttemptLog } from "./attempt-log.js"; import { initGovernorLedger } from "./governor-ledger.js"; import { openWorktreeAllocator } from "./worktree-allocator.js"; import { resolveRejectionSignaled } from "./rejection-signal.js"; +import { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; const ATTEMPT_USAGE = "Usage: gittensory-miner attempt --miner-login [--base ] [--live] [--json]"; @@ -120,9 +121,10 @@ export function buildAttemptDeps(env, ledgers) { /** * Run the `attempt` CLI subcommand. Checks resolveRejectionSignaled first (before consuming a worktree - * slot), then acquires a real worktree slot (worktree-allocator.js's first production caller), assembles - * real AttemptDeps, then -- since no SelfReviewContext fetcher or coding-task-spec builder exists yet -- - * reports the block instead of calling runMinerAttempt with fabricated data. See this file's header for why. + * slot), acquires a concurrency slot (worktree-allocator.js), assembles real AttemptDeps, then prepares a + * REAL git worktree (attempt-worktree.js: clone/fetch + `git worktree add`) -- then, since no SelfReviewContext + * fetcher or coding-task-spec builder exists yet, reports the block instead of calling runMinerAttempt with + * fabricated data, and cleans up the now-unused worktree. See this file's header for why. */ export async function runAttempt(args, options = {}) { const parsed = parseAttemptArgs(args); @@ -151,6 +153,7 @@ export async function runAttempt(args, options = {}) { let attemptLog = null; let governorLedger = null; let allocation = null; + let worktreeResult = null; try { allocator = (options.openWorktreeAllocator ?? openWorktreeAllocator)(); @@ -211,6 +214,46 @@ export async function runAttempt(args, options = {}) { return 3; } + // Real worktree preparation (repo-clone.js + attempt-worktree.js, #5237): the allocator above only + // reserves a concurrency SLOT (worktree-allocator.js's own `slot-N` placeholder dirs never receive real + // git content) -- this is the step that actually clones/fetches the target repo and creates a real + // `git worktree` for this attempt. Its own path, NOT the allocator's slot path, is the real + // workingDirectory a future runMinerAttempt call must use. + const prepareWorktree = options.prepareAttemptWorktree ?? prepareAttemptWorktree; + worktreeResult = await prepareWorktree(parsed.repoFullName, attemptId, { baseBranch: parsed.base, env }); + if (!worktreeResult.ok) { + const reason = worktreeResult.error; + attemptLog.appendAttemptLogEvent({ + eventType: "attempt_aborted", + attemptId, + actionClass: "open_pr", + mode, + reason, + payload: { repoFullName: parsed.repoFullName, issueNumber: parsed.issueNumber }, + }); + eventLedger.appendEvent({ + type: "attempt_blocked", + repoFullName: parsed.repoFullName, + payload: { issueNumber: parsed.issueNumber, reason }, + }); + const worktreeFailureResult = { + outcome: "blocked_worktree_preparation_failed", + reason, + repoFullName: parsed.repoFullName, + issueNumber: parsed.issueNumber, + minerLogin: parsed.minerLogin, + base: parsed.base, + mode, + attemptId, + }; + if (parsed.json) { + console.log(JSON.stringify(worktreeFailureResult, null, 2)); + } else { + console.error(`Attempt for ${parsed.repoFullName}#${parsed.issueNumber} is blocked: real worktree preparation failed: ${reason}`); + } + return 6; + } + const reason = "missing_self_review_context_and_task_spec"; const blockedResult = { outcome: "blocked_missing_prerequisite", @@ -222,7 +265,7 @@ export async function runAttempt(args, options = {}) { base: parsed.base, mode, attemptId, - worktreePath: allocation.worktreePath, + worktreePath: worktreeResult.worktreePath, }; // "attempt_aborted" is the closest fit in ATTEMPT_LOG_EVENT_TYPES's fixed vocabulary @@ -253,6 +296,13 @@ export async function runAttempt(args, options = {}) { console.error(error instanceof Error ? error.message : String(error)); return 2; } finally { + // No real attempt ever ran in this worktree (every path above stops before invoking runMinerAttempt) -- + // there's nothing to postmortem, so it's always cleaned up (`attemptOk: true`), matching + // cleanupAttemptWorktree's own retention policy for a worktree with no failure to inspect. + if (worktreeResult?.ok) { + const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree; + await cleanupWorktree(worktreeResult.repoPath, worktreeResult.worktreePath, true); + } if (allocation && allocator) allocator.release(attemptId); allocator?.close(); claimLedger?.close(); diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index a6436dba5d..0cc5fc2901 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -13,6 +13,7 @@ import { closeDefaultAttemptLog, initAttemptLog } from "../../packages/gittensor import { closeDefaultGovernorLedger, initGovernorLedger } from "../../packages/gittensory-miner/lib/governor-ledger.js"; import { closeDefaultWorktreeAllocator, openWorktreeAllocator } from "../../packages/gittensory-miner/lib/worktree-allocator.js"; import { buildAttemptDeps, parseAttemptArgs, runAttempt } from "../../packages/gittensory-miner/lib/attempt-cli.js"; +import type { PrepareAttemptWorktreeResult } from "../../packages/gittensory-miner/lib/attempt-worktree.js"; const roots: string[] = []; // Only ever holds ledgers a test itself must close -- runAttempt tests inject theirs via DI and runAttempt's @@ -20,6 +21,12 @@ const roots: string[] = []; // SQLite handle throws "database is not open" / "statement has been finalized" on a second close()). const closeables: Array<{ close(): void }> = []; +/** A stubbed successful prepareAttemptWorktree, for tests exercising code paths past worktree preparation + * that don't themselves care about real git plumbing (covered separately by miner-attempt-worktree.test.ts). */ +function fakeWorktreeResult(): Extract { + return { ok: true, worktreePath: "/fake/repo/.gittensory-worktrees/fake", repoPath: "/fake/repo", branchName: "gittensory/attempt/fake" }; +} + function tempLedgers() { const root = mkdtempSync(join(tmpdir(), "gittensory-miner-attempt-cli-")); roots.push(root); @@ -195,6 +202,9 @@ describe("runAttempt (#5132)", () => { const releaseSpy = vi.spyOn(allocator, "release"); const appendAttemptLogEventSpy = vi.spyOn(attemptLog, "appendAttemptLogEvent"); const appendEventSpy = vi.spyOn(eventLedger, "appendEvent"); + const worktreeResult = fakeWorktreeResult(); + const prepareAttemptWorktreeSpy = vi.fn().mockResolvedValue(worktreeResult); + const cleanupAttemptWorktreeSpy = vi.fn().mockResolvedValue({ ok: true, removed: true }); const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { env: { MINER_CODING_AGENT_PROVIDER: "noop" }, @@ -206,6 +216,8 @@ describe("runAttempt (#5132)", () => { initAttemptLog: () => attemptLog, initGovernorLedger: () => governorLedger, resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: prepareAttemptWorktreeSpy, + cleanupAttemptWorktree: cleanupAttemptWorktreeSpy, }); expect(exitCode).toBe(4); @@ -220,7 +232,7 @@ describe("runAttempt (#5132)", () => { base: "main", mode: "dry_run", attemptId: "fixed-attempt-id", - worktreePath: expect.any(String), + worktreePath: worktreeResult.worktreePath, }); // The worktree slot was acquired for real and then released, not left dangling. @@ -228,6 +240,9 @@ describe("runAttempt (#5132)", () => { // A real, persisted record of the block was written to both ledgers -- not just console output. expect(appendAttemptLogEventSpy).toHaveBeenCalledWith(expect.objectContaining({ eventType: "attempt_aborted", attemptId: "fixed-attempt-id" })); expect(appendEventSpy).toHaveBeenCalledWith(expect.objectContaining({ type: "attempt_blocked", repoFullName: "acme/widgets" })); + // A real git worktree was prepared for this attempt -- and cleaned up, since nothing ran in it. + expect(prepareAttemptWorktreeSpy).toHaveBeenCalledWith("acme/widgets", "fixed-attempt-id", { baseBranch: "main", env: { MINER_CODING_AGENT_PROVIDER: "noop" } }); + expect(cleanupAttemptWorktreeSpy).toHaveBeenCalledWith(worktreeResult.repoPath, worktreeResult.worktreePath, true); }); it("resolves live mode only when --live is passed", async () => { @@ -242,6 +257,8 @@ describe("runAttempt (#5132)", () => { initAttemptLog: () => attemptLog, initGovernorLedger: () => governorLedger, resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: async () => fakeWorktreeResult(), + cleanupAttemptWorktree: async () => ({ ok: true, removed: true }), }); expect(exitCode).toBe(4); @@ -260,6 +277,8 @@ describe("runAttempt (#5132)", () => { initAttemptLog: () => attemptLog, initGovernorLedger: () => governorLedger, resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: async () => fakeWorktreeResult(), + cleanupAttemptWorktree: async () => ({ ok: true, removed: true }), }); expect(exitCode).toBe(4); @@ -386,9 +405,91 @@ describe("runAttempt (#5132)", () => { initGovernorLedger: () => governorLedger, resolveRejectionSignaled: resolveRejectionSignaledSpy, fetchImpl, + prepareAttemptWorktree: async () => fakeWorktreeResult(), + cleanupAttemptWorktree: async () => ({ ok: true, removed: true }), }); expect(resolveRejectionSignaledSpy).toHaveBeenCalledWith("acme/widgets", { fetchImpl }); expect(log).toHaveBeenCalled(); }); + + it("REGRESSION: reports a real block and releases the worktree slot when worktree preparation fails", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const releaseSpy = vi.spyOn(allocator, "release"); + const appendAttemptLogEventSpy = vi.spyOn(attemptLog, "appendAttemptLogEvent"); + const cleanupAttemptWorktreeSpy = vi.fn(); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + attemptId: "clone-failed-attempt", + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: async () => ({ ok: false, error: "git_clone_failed" }), + cleanupAttemptWorktree: cleanupAttemptWorktreeSpy, + }); + + expect(exitCode).toBe(6); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ + outcome: "blocked_worktree_preparation_failed", + reason: "git_clone_failed", + repoFullName: "acme/widgets", + issueNumber: 7, + minerLogin: "alice", + base: "main", + mode: "dry_run", + attemptId: "clone-failed-attempt", + }); + // The worktree slot is still released even though preparation failed -- no leaked allocation. + expect(releaseSpy).toHaveBeenCalledWith("clone-failed-attempt"); + expect(appendAttemptLogEventSpy).toHaveBeenCalledWith( + expect.objectContaining({ eventType: "attempt_aborted", attemptId: "clone-failed-attempt", reason: "git_clone_failed" }), + ); + // Nothing to clean up -- preparation never produced a real worktree to remove. + expect(cleanupAttemptWorktreeSpy).not.toHaveBeenCalled(); + }); + + it("reports a real block with a human-readable message when worktree preparation fails", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: async () => ({ ok: false, error: "git_fetch_failed" }), + cleanupAttemptWorktree: vi.fn(), + }); + + expect(exitCode).toBe(6); + expect(error).toHaveBeenCalledWith(expect.stringContaining("real worktree preparation failed: git_fetch_failed")); + }); + + it("passes parsed.base through as prepareAttemptWorktree's baseBranch", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const prepareAttemptWorktreeSpy = vi.fn().mockResolvedValue(fakeWorktreeResult()); + + await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--base", "develop", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + resolveRejectionSignaled: async () => false, + prepareAttemptWorktree: prepareAttemptWorktreeSpy, + cleanupAttemptWorktree: async () => ({ ok: true, removed: true }), + }); + + expect(prepareAttemptWorktreeSpy).toHaveBeenCalledWith("acme/widgets", expect.any(String), expect.objectContaining({ baseBranch: "develop" })); + }); });