-
-
Notifications
You must be signed in to change notification settings - Fork 87
feat(miner): wire real git worktree preparation into the attempt pipeline (#5132) #5237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import type { WorktreeExecFn } from "@jsonbored/gittensory-engine"; | ||
| import type { RunGitFn } from "./repo-clone.js"; | ||
|
|
||
| export function createRealWorktreeExec(timeoutMs?: number): WorktreeExecFn; | ||
|
|
||
| export type PrepareAttemptWorktreeOptions = { | ||
| baseBranch?: string; | ||
| cloneBaseDir?: string; | ||
| env?: Record<string, string | undefined>; | ||
| exec?: WorktreeExecFn; | ||
| timeoutMs?: number; | ||
| remoteUrl?: string; | ||
| runGit?: RunGitFn; | ||
| }; | ||
|
|
||
| export type PrepareAttemptWorktreeResult = | ||
| | { ok: true; worktreePath: string; branchName: string; repoPath: string } | ||
| | { ok: false; repoPath?: string; error: string }; | ||
|
|
||
| export function prepareAttemptWorktree( | ||
| repoFullName: string, | ||
| attemptId: string, | ||
| options?: PrepareAttemptWorktreeOptions, | ||
| ): Promise<PrepareAttemptWorktreeResult>; | ||
|
|
||
| export function cleanupAttemptWorktree( | ||
| repoPath: string, | ||
| worktreePath: string, | ||
| attemptOk: boolean, | ||
| options?: { exec?: WorktreeExecFn; timeoutMs?: number }, | ||
| ): Promise<{ ok: boolean; removed: boolean; error?: string }>; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { spawn } from "node:child_process"; | ||
| import { addWorktree, removeWorktree, shouldRetainWorktree } from "@jsonbored/gittensory-engine"; | ||
| import { ensureRepoCloned } from "./repo-clone.js"; | ||
|
|
||
| // Real attempt-worktree preparation (#5132, Wave 3.5 follow-up). Composes ensureRepoCloned (repo-clone.js, | ||
| // the missing base-clone-management step) with @jsonbored/gittensory-engine's already-built, already-tested | ||
| // addWorktree/removeWorktree primitives -- which existed but were never called from this package, so | ||
| // `workingDirectory` handed to runIterateLoop was always just an empty directory with no real git repo in | ||
| // it. This is the caller that finally exercises them for real. | ||
|
|
||
| const DEFAULT_TIMEOUT_MS = 120_000; | ||
|
|
||
| /** | ||
| * Real child_process-backed implementation of the engine's WorktreeExecFn contract. Resolves (never | ||
| * rejects) on error/timeout, mirroring coding-agent-construction.js's createRealCliSubprocessSpawn -- a | ||
| * failed `git worktree add`'s stderr is the diagnosable signal, not something to lose to an unhandled | ||
| * rejection. | ||
| * | ||
| * @returns {import("@jsonbored/gittensory-engine").WorktreeExecFn} | ||
| */ | ||
| export function createRealWorktreeExec(timeoutMs = DEFAULT_TIMEOUT_MS) { | ||
| return (cmd, args, opts) => | ||
| new Promise((resolve) => { | ||
| const child = spawn(cmd, args, { cwd: opts.cwd, stdio: ["ignore", "pipe", "pipe"] }); | ||
| let stdout = ""; | ||
| let stderr = ""; | ||
| const timer = setTimeout(() => { | ||
| child.kill("SIGKILL"); | ||
| resolve({ code: null, stdout, stderr: `${stderr}\ntimed_out_after_${timeoutMs}ms`.trim() }); | ||
| }, timeoutMs); | ||
| child.stdout?.on("data", (chunk) => { | ||
| stdout += chunk.toString("utf8"); | ||
| }); | ||
| child.stderr?.on("data", (chunk) => { | ||
| stderr += chunk.toString("utf8"); | ||
| }); | ||
| child.on("error", (err) => { | ||
| clearTimeout(timer); | ||
| resolve({ code: null, stdout, stderr: err.message }); | ||
| }); | ||
| child.on("close", (code) => { | ||
| clearTimeout(timer); | ||
| resolve({ code, stdout, stderr }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Prepare a real, isolated git worktree for one attempt: ensure the target repo's base clone exists and is | ||
| * current, then create a fresh `git worktree` off it on a deterministically-named branch. Fails closed | ||
| * (`ok: false`) on any step's failure rather than handing back a half-prepared directory. | ||
| * | ||
| * @param {string} repoFullName | ||
| * @param {string} attemptId | ||
| * @param {{ | ||
| * baseBranch?: string, cloneBaseDir?: string, env?: Record<string, string | undefined>, | ||
| * exec?: import("@jsonbored/gittensory-engine").WorktreeExecFn, timeoutMs?: number, | ||
| * remoteUrl?: string, runGit?: import("./repo-clone.js").RunGitFn, | ||
| * }} [options] | ||
| * @returns {Promise<{ ok: boolean, worktreePath?: string, branchName?: string, repoPath?: string, error?: string }>} | ||
| */ | ||
| export async function prepareAttemptWorktree(repoFullName, attemptId, options = {}) { | ||
| const cloneResult = await ensureRepoCloned(repoFullName, { | ||
| baseBranch: options.baseBranch, | ||
| cloneBaseDir: options.cloneBaseDir, | ||
| env: options.env, | ||
| timeoutMs: options.timeoutMs, | ||
| remoteUrl: options.remoteUrl, | ||
| runGit: options.runGit, | ||
| }); | ||
| if (!cloneResult.ok) return { ok: false, error: cloneResult.error ?? "ensure_repo_cloned_failed" }; | ||
|
|
||
| const exec = options.exec ?? createRealWorktreeExec(options.timeoutMs); | ||
| const baseBranch = typeof options.baseBranch === "string" && options.baseBranch.trim() ? options.baseBranch.trim() : "main"; | ||
| const added = await addWorktree({ exec, repoPath: cloneResult.repoPath, baseBranch, attemptId }); | ||
| if (!added.ok) return { ok: false, repoPath: cloneResult.repoPath, error: added.error ?? "git_worktree_add_failed" }; | ||
|
|
||
| return { ok: true, worktreePath: added.plan.worktreePath, branchName: added.plan.branchName, repoPath: cloneResult.repoPath }; | ||
| } | ||
|
|
||
| /** | ||
| * Tear down an attempt's worktree once the attempt concludes, per the engine's own retention policy: a | ||
| * failed attempt's worktree is RETAINED for post-mortem inspection, a succeeded one is removed. | ||
| * | ||
| * @param {string} repoPath | ||
| * @param {string} worktreePath | ||
| * @param {boolean} attemptOk | ||
| * @param {{ exec?: import("@jsonbored/gittensory-engine").WorktreeExecFn, timeoutMs?: number }} [options] | ||
| * @returns {Promise<{ ok: boolean, removed: boolean, error?: string }>} | ||
| */ | ||
| export async function cleanupAttemptWorktree(repoPath, worktreePath, attemptOk, options = {}) { | ||
| const exec = options.exec ?? createRealWorktreeExec(options.timeoutMs); | ||
| return removeWorktree({ exec, repoPath, worktreePath, retain: shouldRetainWorktree(attemptOk) }); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| export function resolveRepoCloneBaseDir(env?: Record<string, string | undefined>): string; | ||
|
|
||
| export function resolveRepoCloneDir(repoFullName: string, env?: Record<string, string | undefined>): string; | ||
|
|
||
| export type EnsureRepoClonedResult = { ok: boolean; repoPath: string; error?: string }; | ||
|
|
||
| export type RunGitFn = (args: string[], cwd: string, timeoutMs: number) => Promise<{ ok: boolean; stdout: string; stderr: string }>; | ||
|
|
||
| export function ensureRepoCloned( | ||
| repoFullName: string, | ||
| options?: { | ||
| baseBranch?: string; | ||
| cloneBaseDir?: string; | ||
| env?: Record<string, string | undefined>; | ||
| timeoutMs?: number; | ||
| remoteUrl?: string; | ||
| runGit?: RunGitFn; | ||
| }, | ||
| ): Promise<EnsureRepoClonedResult>; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import { execFile } from "node:child_process"; | ||
| import { existsSync, mkdirSync } from "node:fs"; | ||
| import { homedir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { promisify } from "node:util"; | ||
|
|
||
| // Per-repo base-clone cache (#5132, Wave 3.5 follow-up). packages/gittensory-engine/src/miner/ | ||
| // worktree-allocator.ts's real `addWorktree` primitive (git worktree add -b <branch> <path> <baseBranch>) | ||
| // requires an EXISTING git clone to branch off -- it has never been wired into this package because that | ||
| // clone-management step didn't exist yet. This module is that step: clone a target repo once, then keep it | ||
| // current (fetch + hard-reset to the base branch) on every subsequent attempt, so `addWorktree` always | ||
| // branches off real, fresh content. Relies entirely on whatever git/gh credentials are already configured | ||
| // on this machine -- same assumption execute-local-write.js's `gh pr create` already makes; this module | ||
| // never embeds a token in a clone URL. | ||
|
|
||
| const execFileAsync = promisify(execFile); | ||
| const DEFAULT_CLONE_DIR_NAME = "repos"; | ||
| const DEFAULT_BASE_BRANCH = "main"; | ||
|
|
||
| export function resolveRepoCloneBaseDir(env = process.env) { | ||
| const explicitPath = typeof env.GITTENSORY_MINER_REPO_CLONE_DIR === "string" ? env.GITTENSORY_MINER_REPO_CLONE_DIR.trim() : ""; | ||
| if (explicitPath) return explicitPath; | ||
|
|
||
| const explicitConfigDir = typeof env.GITTENSORY_MINER_CONFIG_DIR === "string" ? env.GITTENSORY_MINER_CONFIG_DIR.trim() : ""; | ||
| if (explicitConfigDir) return join(explicitConfigDir, DEFAULT_CLONE_DIR_NAME); | ||
|
|
||
| const configHome = typeof env.XDG_CONFIG_HOME === "string" && env.XDG_CONFIG_HOME.trim() ? env.XDG_CONFIG_HOME.trim() : join(homedir(), ".config"); | ||
| return join(configHome, "gittensory-miner", DEFAULT_CLONE_DIR_NAME); | ||
| } | ||
|
|
||
| // GitHub owner/repo names are restricted to alphanumerics, hyphens, underscores, and periods, and are never | ||
| // exactly "." or ".." -- both are rejected here so a value like "../foo" can't make resolveRepoCloneDir's | ||
| // join(cloneBaseDir, owner, repo) escape the intended clone directory (a real path-traversal finding). | ||
| const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; | ||
|
|
||
| function isPathTraversalSegment(segment) { | ||
| return segment === "." || segment === ".."; | ||
| } | ||
|
|
||
| function normalizeRepoFullName(repoFullName) { | ||
| if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name"); | ||
| const [owner, repo, extra] = repoFullName.trim().split("/"); | ||
| if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); | ||
| if (!REPO_SEGMENT_PATTERN.test(owner) || !REPO_SEGMENT_PATTERN.test(repo)) throw new Error("invalid_repo_full_name"); | ||
| if (isPathTraversalSegment(owner) || isPathTraversalSegment(repo)) throw new Error("invalid_repo_full_name"); | ||
| return { owner, repo, repoFullName: `${owner}/${repo}` }; | ||
| } | ||
|
|
||
| export function resolveRepoCloneDir(repoFullName, env = process.env) { | ||
| const target = normalizeRepoFullName(repoFullName); | ||
| return join(resolveRepoCloneBaseDir(env), target.owner, target.repo); | ||
| } | ||
|
|
||
| async function defaultRunGit(args, cwd, timeoutMs) { | ||
| try { | ||
| const { stdout, stderr } = await execFileAsync("git", args, { cwd, timeout: timeoutMs }); | ||
| return { ok: true, stdout, stderr }; | ||
| } catch (error) { | ||
| const stderr = typeof error?.stderr === "string" ? error.stderr : ""; | ||
| return { ok: false, stdout: "", stderr: stderr || (error instanceof Error ? error.message : String(error)) }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensure a real, current local clone of `repoFullName` exists at the deterministic per-repo cache path. | ||
| * First use: `git clone`. Subsequent use: `git fetch origin` + hard-reset the base branch to | ||
| * `origin/<baseBranch>`, so every attempt branches off fresh content, not a stale prior checkout. | ||
| * | ||
| * @param {string} repoFullName | ||
| * @param {{ | ||
| * baseBranch?: string, cloneBaseDir?: string, env?: Record<string, string | undefined>, timeoutMs?: number, | ||
| * remoteUrl?: string, runGit?: (args: string[], cwd: string, timeoutMs: number) => Promise<{ ok: boolean, stdout: string, stderr: string }>, | ||
| * }} [options] | ||
| * @returns {Promise<{ ok: boolean, repoPath: string, error?: string }>} | ||
| */ | ||
| export async function ensureRepoCloned(repoFullName, options = {}) { | ||
| const target = normalizeRepoFullName(repoFullName); | ||
| const baseBranch = typeof options.baseBranch === "string" && options.baseBranch.trim() ? options.baseBranch.trim() : DEFAULT_BASE_BRANCH; | ||
| const cloneBaseDir = typeof options.cloneBaseDir === "string" && options.cloneBaseDir.trim() ? options.cloneBaseDir.trim() : resolveRepoCloneBaseDir(options.env); | ||
| const repoPath = join(cloneBaseDir, target.owner, target.repo); | ||
| const timeoutMs = Number.isFinite(options.timeoutMs) ? options.timeoutMs : 120_000; | ||
| const runGit = options.runGit ?? defaultRunGit; | ||
|
|
||
| if (!existsSync(repoPath)) { | ||
| mkdirSync(join(cloneBaseDir, target.owner), { recursive: true, mode: 0o700 }); | ||
| const cloneUrl = typeof options.remoteUrl === "string" && options.remoteUrl.trim() ? options.remoteUrl.trim() : `https://github.com/${target.owner}/${target.repo}.git`; | ||
| const cloned = await runGit(["clone", cloneUrl, repoPath], cloneBaseDir, timeoutMs); | ||
| if (!cloned.ok) return { ok: false, repoPath, error: cloned.stderr || "git_clone_failed" }; | ||
| return { ok: true, repoPath }; | ||
| } | ||
|
|
||
| const fetched = await runGit(["fetch", "origin"], repoPath, timeoutMs); | ||
| if (!fetched.ok) return { ok: false, repoPath, error: fetched.stderr || "git_fetch_failed" }; | ||
|
|
||
| const checkedOut = await runGit(["checkout", baseBranch], repoPath, timeoutMs); | ||
| if (!checkedOut.ok) return { ok: false, repoPath, error: checkedOut.stderr || "git_checkout_failed" }; | ||
|
|
||
| const reset = await runGit(["reset", "--hard", `origin/${baseBranch}`], repoPath, timeoutMs); | ||
| if (!reset.ok) return { ok: false, repoPath, error: reset.stderr || "git_reset_failed" }; | ||
|
|
||
| return { ok: true, repoPath }; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2:
repoFullNamepath traversal allows cloning repositories outside intended clone directorynormalizeRepoFullNamedoes not reject..segments inownerorrepo, allowingrepoPathto escapecloneBaseDir.Validate
ownerandrepowith a regex or explicit..rejection to prevent path traversal.AI prompt