Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/gittensory-miner/lib/attempt-cli.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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<number>;
58 changes: 54 additions & 4 deletions packages/gittensory-miner/lib/attempt-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <owner/repo> <issue#> --miner-login <login> [--base <branch>] [--live] [--json]";

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)();
Expand Down Expand Up @@ -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",
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down
103 changes: 102 additions & 1 deletion test/unit/miner-attempt-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ 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
// own `finally` block closes them, so registering the same objects here would double-close (the underlying
// 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<PrepareAttemptWorktreeResult, { ok: true }> {
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);
Expand Down Expand Up @@ -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" },
Expand All @@ -206,6 +216,8 @@ describe("runAttempt (#5132)", () => {
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
resolveRejectionSignaled: async () => false,
prepareAttemptWorktree: prepareAttemptWorktreeSpy,
cleanupAttemptWorktree: cleanupAttemptWorktreeSpy,
});

expect(exitCode).toBe(4);
Expand All @@ -220,14 +232,17 @@ 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.
expect(releaseSpy).toHaveBeenCalledWith("fixed-attempt-id");
// 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 () => {
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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" }));
});
});