From 429502d1d8e375b9ca249b043f4041fb1fcd4f69 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Wed, 12 Aug 2026 16:08:12 -0300 Subject: [PATCH 1/5] fix(review): support unborn repository reviews --- lib/git-commit-transaction.ts | 44 ++++--- lib/review-candidate-view.ts | 66 +++++++++- runtime/git-commit-transaction.mjs | 40 +++--- tests/git-commit-transaction.test.ts | 59 +++++++++ tests/review-candidate-view.test.ts | 120 ++++++++++++++++++ .../review-controller-native-routing.test.ts | 41 ++++++ 6 files changed, 331 insertions(+), 39 deletions(-) diff --git a/lib/git-commit-transaction.ts b/lib/git-commit-transaction.ts index 0f4509dd9..a581d021a 100644 --- a/lib/git-commit-transaction.ts +++ b/lib/git-commit-transaction.ts @@ -77,8 +77,8 @@ interface CommitTransactionRecordBody { command: string; command_hash: string; arguments: readonly string[]; - original_head: string; - original_head_tree: string; + original_head?: string; + original_head_tree?: string; original_index_tree: string; original_index_hash: string; authorized_pre_hook_tree: string; @@ -180,14 +180,17 @@ function absoluteGitPath(cwd: string, name: string): string { return isAbsolute(value) ? value : resolve(cwd, value); } +function resolveHead(cwd: string): string | undefined { + try { return git(cwd, ["rev-parse", "--verify", "HEAD"]); } catch { return undefined; } +} + function repositoryBinding(cwd: string): RepositoryBinding { const root = realpathSync(git(cwd, ["rev-parse", "--show-toplevel"])); const commonDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-common-dir"]); const gitDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-dir"]); const commonDir = realpathSync(commonDirValue); const gitDir = realpathSync(gitDirValue); - const roots = git(root, ["rev-list", "--max-parents=0", "HEAD"]).split(/\r?\n/).filter(Boolean).sort(); - const repositoryId = sha256(canonicalJson({ common_directory: commonDir, roots })); + const repositoryId = sha256(canonicalJson({ common_directory: commonDir })); const worktreeKey = sha256(gitDir).slice("sha256:".length, "sha256:".length + 24); const stateDir = join(commonDir, "gentle-pi", "commit-transactions", worktreeKey); return { @@ -241,11 +244,11 @@ function decodeRecord(value: unknown): CommitTransactionRecord { if (record.schema !== TRANSACTION_SCHEMA) throw new Error("commit transaction record schema is incompatible"); for (const field of [ "transaction_id", "repository_id", "repository_root", "common_directory", "git_directory", - "command", "command_hash", "original_head", "original_head_tree", "original_index_tree", + "command", "command_hash", "original_index_tree", "original_index_hash", "authorized_pre_hook_tree", "state", "created_at", "updated_at", "record_hash", ]) if (typeof record[field] !== "string" || (record[field] as string).length === 0) throw new Error(`commit transaction record ${field} is invalid`); if (!isStringArray(record.arguments) || !isStringArray(record.invocation_ids) || !isStringArray(record.lineage_history)) throw new Error("commit transaction record arrays are invalid"); - for (const field of ["post_hook_tree", "post_hook_index_hash", "authorized_tree", "authority_revision", "gate_context_hash", "committed_head", "committed_tree", "git_created_head", "git_created_tree", "error"] as const) { + for (const field of ["original_head", "original_head_tree", "post_hook_tree", "post_hook_index_hash", "authorized_tree", "authority_revision", "gate_context_hash", "committed_head", "committed_tree", "git_created_head", "git_created_tree", "error"] as const) { if (record[field] !== undefined && typeof record[field] !== "string") throw new Error(`commit transaction record ${field} is invalid`); } if (!Number.isSafeInteger(record.hook_runs) || (record.hook_runs as number) < 0) throw new Error("commit transaction hook count is invalid"); @@ -437,7 +440,7 @@ export function prepareCommitTransactionInvocation(input: { }): CommitTransactionInvocation { assertSafeCommitArguments(input.arguments); const binding = repositoryBinding(input.cwd); - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); const currentTree = git(binding.root, ["write-tree"]); if (currentTree !== input.authorization.intendedTree) throw new Error("commit transaction pre-hook index no longer matches its controller authorization"); let transactionId = randomUUID(); @@ -540,7 +543,7 @@ function validateNativeTree(result: NativeValidateResult, lineageId: string, tre } function createRecord(binding: RepositoryBinding, invocation: CommitTransactionInvocation, now: () => Date): CommitTransactionRecord { - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); const tree = git(binding.root, ["write-tree"]); if (tree !== invocation.authorization.intendedTree) throw new Error("commit transaction index changed after controller authorization"); const timestamp = now().toISOString(); @@ -555,7 +558,7 @@ function createRecord(binding: RepositoryBinding, invocation: CommitTransactionI command_hash: invocation.commandHash, arguments: [...invocation.arguments], original_head: head, - original_head_tree: git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]), + original_head_tree: head === undefined ? undefined : git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]), original_index_tree: tree, original_index_hash: indexFingerprint(binding.root), authorized_pre_hook_tree: invocation.authorization.intendedTree, @@ -571,13 +574,17 @@ function createRecord(binding: RepositoryBinding, invocation: CommitTransactionI function assertInvocationMatches(binding: RepositoryBinding, record: CommitTransactionRecord, invocation: CommitTransactionInvocation): void { if (record.repository_id !== binding.repositoryId || record.repository_root !== binding.root || record.common_directory !== binding.commonDir || record.git_directory !== binding.gitDir) throw new Error("commit transaction repository identity changed"); if (record.transaction_id !== invocation.transactionId || record.command_hash !== invocation.commandHash || record.command !== invocation.command || canonicalJson(record.arguments) !== canonicalJson(invocation.arguments)) throw new Error("commit transaction exact retry does not match the durable command intent"); - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) !== record.original_head) throw new Error("commit transaction HEAD changed before reconciliation"); + if (resolveHead(binding.root) !== record.original_head) throw new Error("commit transaction HEAD changed before reconciliation"); } function recoverCompletedCommit(binding: RepositoryBinding, record: CommitTransactionRecord, now: () => Date): CommitTransactionResult | undefined { if (record.state !== COMMIT_TRANSACTION_STATE.COMMIT_RUNNING && record.state !== COMMIT_TRANSACTION_STATE.COMMITTED) return undefined; - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); if (head === record.original_head) return undefined; + if (head === undefined) { + transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { error: "HEAD disappeared during commit transaction" }, now); + throw new Error("commit transaction incident: HEAD disappeared during commit; publication remains blocked"); + } const tree = git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); if (record.git_created_head === undefined || record.git_created_tree === undefined || head !== record.git_created_head || tree !== record.git_created_tree || tree !== record.authorized_tree) { transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { committed_head: head, committed_tree: tree, error: "HEAD identity differs from the exact Git-created authorized commit" }, now); @@ -684,14 +691,15 @@ export async function runGitCommitTransaction( const commit = await runProcess("git", ["-c", `core.hooksPath=${proxy}`, "commit", ...invocation.arguments], binding.root); if (dependencies.failpoint === "after-commit-before-proof") throw new Error("commit transaction test interruption after Git returned"); record = readRecord(binding.activePath) ?? record; - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); - const headTree = git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); - if (head !== record.original_head && head === record.git_created_head && headTree === record.git_created_tree && headTree === record.authorized_tree) { + const head = resolveHead(binding.root); + const headTree = head === undefined ? undefined : git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); + const headChanged = head !== record.original_head; + if (headChanged && head === record.git_created_head && headTree === record.git_created_tree && headTree === record.authorized_tree) { const committed = transition(binding, record, COMMIT_TRANSACTION_STATE.COMMITTED, { committed_head: head, committed_tree: headTree, ...(commit.code === 0 && commit.signal === null ? {} : { error: `Git returned ${commit.signal ?? `exit ${commit.code}`} after creating the authorized commit` }) }, now); archive(binding, committed); - return { transactionId: record.transaction_id, status: "committed", head, tree: headTree }; + return { transactionId: record.transaction_id, status: "committed", head: head!, tree: headTree! }; } - if (head !== record.original_head) { + if (headChanged) { transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { committed_head: head, committed_tree: headTree, error: "HEAD identity differs from the exact Git-created authorized commit" }, now); throw new Error("commit transaction incident: HEAD identity changed after Git created the authorized commit; publication remains blocked"); } @@ -701,7 +709,7 @@ export async function runGitCommitTransaction( if (record !== undefined && dependencies.signal?.aborted === true && existsSync(binding.activePath)) { try { const active = readRecord(binding.activePath) ?? record; - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) === active.original_head) transition(binding, active, COMMIT_TRANSACTION_STATE.INTERRUPTED, { error: "commit transaction was cancelled" }, now); + if (resolveHead(binding.root) === active.original_head) transition(binding, active, COMMIT_TRANSACTION_STATE.INTERRUPTED, { error: "commit transaction was cancelled" }, now); } catch { /* retain the earlier durable state */ } } throw error; @@ -782,7 +790,7 @@ export function abandonCommitTransaction(cwd: string): CommitTransactionRecord { try { const record = readRecord(binding.activePath); if (record === undefined) throw new Error("active commit transaction disappeared during recovery"); - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) !== record.original_head) throw new Error("cannot abandon a commit transaction after HEAD changed; reconcile the committed tree instead"); + if (resolveHead(binding.root) !== record.original_head) throw new Error("cannot abandon a commit transaction after HEAD changed; reconcile the committed tree instead"); const abandoned = transition(binding, record, COMMIT_TRANSACTION_STATE.ABANDONED, { error: "explicitly abandoned without changing HEAD or index" }, now); return archive(binding, abandoned); } finally { releaseLock(); } diff --git a/lib/review-candidate-view.ts b/lib/review-candidate-view.ts index 7b89de8c4..a172bf7ac 100644 --- a/lib/review-candidate-view.ts +++ b/lib/review-candidate-view.ts @@ -504,11 +504,11 @@ function assertManifestMatchesGit(descriptor: NativeCandidateProjectionDescripto } } -function deriveChangedScope(cwd: string, baseCommit: string, candidateTree: string, entries: readonly CandidateTreeEntry[], executor: CandidateGitExecutor): CandidateViewScope { +function deriveChangedScope(cwd: string, baseTree: string, candidateTree: string, entries: readonly CandidateTreeEntry[], executor: CandidateGitExecutor): CandidateViewScope { const present = new Map(entries.map((entry) => [entry.path, entry])); const paths = new Set(); const deleted = new Set(); - const tokens = gitPathTokens(cwd, ["diff", "--name-status", "-z", "--no-ext-diff", "--find-renames=100%", baseCommit, candidateTree], executor); + const tokens = gitPathTokens(cwd, ["diff", "--name-status", "-z", "--no-ext-diff", "--find-renames=100%", baseTree, candidateTree], executor); for (let index = 0; index < tokens.length;) { const status = tokens[index++]?.toString("ascii"); if (status === undefined || !/^(?:[AMDT]|R[0-9]{3})$/.test(status)) throw new CandidateViewError("candidate scope Git output contains an unsafe status"); @@ -624,8 +624,56 @@ function explicitBaseRefCandidates(cwd: string, selector: string, env: NodeJS.Pr return [...new Set(candidates)].filter((candidate) => refs.has(candidate)); } +// Runs a probe command that may exit nonzero as an expected signal (absent +// ref, detached HEAD). Returns the exit status and trimmed stdout. Timeout, +// output-limit, and unexpected Git failures propagate as sanitized +// CandidateViewError diagnostics, same as candidateGit. +function probeCandidateGit(cwd: string, arguments_: readonly string[], env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): { status: number; stdout: string } { + const timeoutMs = resolveCandidateGitTimeoutMs(env); + try { + const stdout = executor("git", arguments_, { + cwd, encoding: "utf8", env, stdio: ["ignore", "pipe", "pipe"], + timeout: timeoutMs, maxBuffer: CANDIDATE_GIT_MAX_BUFFER_BYTES, windowsHide: true, + }) as string; + return { status: 0, stdout: stdout.trim() }; + } catch (error) { + const detail = error as NodeJS.ErrnoException & { killed?: boolean; status?: number; stdout?: string | Buffer }; + if (detail.code === "ENOBUFS" || detail.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER") throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.OUTPUT_LIMIT, arguments_, timeoutMs); + if (detail.code === "ETIMEDOUT" || detail.killed === true) throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.TIMEOUT, arguments_, timeoutMs); + if (typeof detail.status === "number") return { status: detail.status, stdout: typeof detail.stdout === "string" ? detail.stdout.trim() : "" }; + throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.GIT_FAILURE, arguments_, timeoutMs); + } +} + +// An unborn repository's HEAD is a symbolic ref to a branch with no commits. +// `symbolic-ref --quiet HEAD` exits nonzero for a detached HEAD (not unborn). +// `rev-parse --verify --quiet ` distinguishes a valid unborn (nonzero, ref +// absent) from a broken symbolic ref (exit 0, ref OID text exists even when the +// object is missing). Any unexpected outcome fails closed. +function isUnbornSymbolicHead(cwd: string, env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): boolean { + const symbolic = probeCandidateGit(cwd, ["symbolic-ref", "--quiet", "HEAD"], env, executor); + if (symbolic.status !== 0) return false; + const refProbe = probeCandidateGit(cwd, ["rev-parse", "--verify", "--quiet", symbolic.stdout], env, executor); + if (refProbe.status !== 0) return true; + return false; +} + +// Derives Git's repository-native empty tree without hardcoding the SHA-1 id, +// so a sha256 repository derives its own empty-tree object id. `mktree` with +// ignored stdin reads empty input and writes the empty tree object. +function resolveEmptyTree(cwd: string, env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): string { + return git(cwd, ["mktree"], env, executor); +} + function resolveCandidateBase(cwd: string, baseRef: string | undefined, env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): ResolvedCandidateBase { const selector = baseRef ?? "HEAD"; + // An unborn repository has a symbolic HEAD pointing at a branch with no + // commits yet. Its review base is Git's repository-native empty tree, not a + // missing or malformed commit. Only the default/HEAD selector is entitled to + // the empty-tree base; a detached HEAD over a missing commit stays fail-closed. + if (selector === "HEAD" && isUnbornSymbolicHead(cwd, env, executor)) { + return { commit: "HEAD", tree: resolveEmptyTree(cwd, env, executor) }; + } try { if (baseRef !== undefined) { const candidates = explicitBaseRefCandidates(cwd, selector, env, executor); @@ -692,17 +740,25 @@ function materializeCandidateView(request: CreateCandidateViewRequest, executor: const environment = { ...process.env, GIT_INDEX_FILE: indexPath }; try { const baseCommit = base.commit; - git(contributorRoot, ["read-tree", candidateCommit.commit], environment, executor); + const unborn = baseCommit === "HEAD"; + // For an unborn repository the base tree is Git's empty tree, so seed the + // private candidate index from `--empty` instead of a non-existent commit. + if (unborn) git(contributorRoot, ["read-tree", "--empty"], environment, executor); + else git(contributorRoot, ["read-tree", candidateCommit.commit], environment, executor); if (!committedOnly) git(contributorRoot, ["add", "-A"], environment, executor); const candidateTree = git(contributorRoot, ["write-tree"], environment, executor); const root = join(parent, randomUUID()); - git(contributorRoot, ["worktree", "add", "--detach", "--no-checkout", root, candidateCommit.commit], process.env, executor); + // An unborn repository has no commit to detach a worktree at, so use an + // orphan worktree (unborn branch, no commit, no ref written until a + // commit) to host the materialized candidate tree without a phantom commit. + if (unborn) git(contributorRoot, ["worktree", "add", "--orphan", "-b", `gentle-ai-candidate-${randomUUID()}`, root], process.env, executor); + else git(contributorRoot, ["worktree", "add", "--detach", "--no-checkout", root, candidateCommit.commit], process.env, executor); try { git(root, ["read-tree", candidateTree], process.env, executor); const tree = parseTree(root, candidateTree, executor); checkoutMaterializedEntries(root, tree.entries, executor); const entries = tree.entries.map((entry) => ({ ...entry, contentHash: entryContentHash(root, entry) })); - const scope = deriveChangedScope(contributorRoot, baseCommit, candidateTree, [...tree.entries, ...tree.gitlinks], executor); + const scope = deriveChangedScope(contributorRoot, base.tree, candidateTree, [...tree.entries, ...tree.gitlinks], executor); for (const gitlink of tree.gitlinks) if (lstatSync(join(root, gitlink.path), { throwIfNoEntry: false })) throw new CandidateViewError("candidate view materialized a metadata-only gitlink"); makeReadonly(root, entries); return { token: basename(root), root: realpathSync(root), parent, contributorRoot, commonDir: canonicalCommonDir, baseCommit, baseTree: base.tree, candidateTree, committedOnly, entries, gitlinks: tree.gitlinks, scope, gitExecutor: executor }; diff --git a/runtime/git-commit-transaction.mjs b/runtime/git-commit-transaction.mjs index d8cf9d38f..e3db105de 100644 --- a/runtime/git-commit-transaction.mjs +++ b/runtime/git-commit-transaction.mjs @@ -181,14 +181,17 @@ function absoluteGitPath(cwd , name ) { return isAbsolute(value) ? value : resolve(cwd, value); } +function resolveHead(cwd ) { + try { return git(cwd, ["rev-parse", "--verify", "HEAD"]); } catch { return undefined; } +} + function repositoryBinding(cwd ) { const root = realpathSync(git(cwd, ["rev-parse", "--show-toplevel"])); const commonDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-common-dir"]); const gitDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-dir"]); const commonDir = realpathSync(commonDirValue); const gitDir = realpathSync(gitDirValue); - const roots = git(root, ["rev-list", "--max-parents=0", "HEAD"]).split(/\r?\n/).filter(Boolean).sort(); - const repositoryId = sha256(canonicalJson({ common_directory: commonDir, roots })); + const repositoryId = sha256(canonicalJson({ common_directory: commonDir })); const worktreeKey = sha256(gitDir).slice("sha256:".length, "sha256:".length + 24); const stateDir = join(commonDir, "gentle-pi", "commit-transactions", worktreeKey); return { @@ -242,11 +245,11 @@ function decodeRecord(value ) { if (record.schema !== TRANSACTION_SCHEMA) throw new Error("commit transaction record schema is incompatible"); for (const field of [ "transaction_id", "repository_id", "repository_root", "common_directory", "git_directory", - "command", "command_hash", "original_head", "original_head_tree", "original_index_tree", + "command", "command_hash", "original_index_tree", "original_index_hash", "authorized_pre_hook_tree", "state", "created_at", "updated_at", "record_hash", ]) if (typeof record[field] !== "string" || (record[field] ).length === 0) throw new Error(`commit transaction record ${field} is invalid`); if (!isStringArray(record.arguments) || !isStringArray(record.invocation_ids) || !isStringArray(record.lineage_history)) throw new Error("commit transaction record arrays are invalid"); - for (const field of ["post_hook_tree", "post_hook_index_hash", "authorized_tree", "authority_revision", "gate_context_hash", "committed_head", "committed_tree", "git_created_head", "git_created_tree", "error"] ) { + for (const field of ["original_head", "original_head_tree", "post_hook_tree", "post_hook_index_hash", "authorized_tree", "authority_revision", "gate_context_hash", "committed_head", "committed_tree", "git_created_head", "git_created_tree", "error"] ) { if (record[field] !== undefined && typeof record[field] !== "string") throw new Error(`commit transaction record ${field} is invalid`); } if (!Number.isSafeInteger(record.hook_runs) || (record.hook_runs ) < 0) throw new Error("commit transaction hook count is invalid"); @@ -438,7 +441,7 @@ export function prepareCommitTransactionInvocation(input ) { assertSafeCommitArguments(input.arguments); const binding = repositoryBinding(input.cwd); - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); const currentTree = git(binding.root, ["write-tree"]); if (currentTree !== input.authorization.intendedTree) throw new Error("commit transaction pre-hook index no longer matches its controller authorization"); let transactionId = randomUUID(); @@ -541,7 +544,7 @@ function validateNativeTree(result , lineageId , tre } function createRecord(binding , invocation , now ) { - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); const tree = git(binding.root, ["write-tree"]); if (tree !== invocation.authorization.intendedTree) throw new Error("commit transaction index changed after controller authorization"); const timestamp = now().toISOString(); @@ -556,7 +559,7 @@ function createRecord(binding , invocation command_hash: invocation.commandHash, arguments: [...invocation.arguments], original_head: head, - original_head_tree: git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]), + original_head_tree: head === undefined ? undefined : git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]), original_index_tree: tree, original_index_hash: indexFingerprint(binding.root), authorized_pre_hook_tree: invocation.authorization.intendedTree, @@ -572,13 +575,17 @@ function createRecord(binding , invocation function assertInvocationMatches(binding , record , invocation ) { if (record.repository_id !== binding.repositoryId || record.repository_root !== binding.root || record.common_directory !== binding.commonDir || record.git_directory !== binding.gitDir) throw new Error("commit transaction repository identity changed"); if (record.transaction_id !== invocation.transactionId || record.command_hash !== invocation.commandHash || record.command !== invocation.command || canonicalJson(record.arguments) !== canonicalJson(invocation.arguments)) throw new Error("commit transaction exact retry does not match the durable command intent"); - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) !== record.original_head) throw new Error("commit transaction HEAD changed before reconciliation"); + if (resolveHead(binding.root) !== record.original_head) throw new Error("commit transaction HEAD changed before reconciliation"); } function recoverCompletedCommit(binding , record , now ) { if (record.state !== COMMIT_TRANSACTION_STATE.COMMIT_RUNNING && record.state !== COMMIT_TRANSACTION_STATE.COMMITTED) return undefined; - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); + const head = resolveHead(binding.root); if (head === record.original_head) return undefined; + if (head === undefined) { + transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { error: "HEAD disappeared during commit transaction" }, now); + throw new Error("commit transaction incident: HEAD disappeared during commit; publication remains blocked"); + } const tree = git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); if (record.git_created_head === undefined || record.git_created_tree === undefined || head !== record.git_created_head || tree !== record.git_created_tree || tree !== record.authorized_tree) { transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { committed_head: head, committed_tree: tree, error: "HEAD identity differs from the exact Git-created authorized commit" }, now); @@ -685,14 +692,15 @@ export async function runGitCommitTransaction( const commit = await runProcess("git", ["-c", `core.hooksPath=${proxy}`, "commit", ...invocation.arguments], binding.root); if (dependencies.failpoint === "after-commit-before-proof") throw new Error("commit transaction test interruption after Git returned"); record = readRecord(binding.activePath) ?? record; - const head = git(binding.root, ["rev-parse", "--verify", "HEAD"]); - const headTree = git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); - if (head !== record.original_head && head === record.git_created_head && headTree === record.git_created_tree && headTree === record.authorized_tree) { + const head = resolveHead(binding.root); + const headTree = head === undefined ? undefined : git(binding.root, ["rev-parse", "--verify", "HEAD^{tree}"]); + const headChanged = head !== record.original_head; + if (headChanged && head === record.git_created_head && headTree === record.git_created_tree && headTree === record.authorized_tree) { const committed = transition(binding, record, COMMIT_TRANSACTION_STATE.COMMITTED, { committed_head: head, committed_tree: headTree, ...(commit.code === 0 && commit.signal === null ? {} : { error: `Git returned ${commit.signal ?? `exit ${commit.code}`} after creating the authorized commit` }) }, now); archive(binding, committed); - return { transactionId: record.transaction_id, status: "committed", head, tree: headTree }; + return { transactionId: record.transaction_id, status: "committed", head: head , tree: headTree }; } - if (head !== record.original_head) { + if (headChanged) { transition(binding, record, COMMIT_TRANSACTION_STATE.INCIDENT, { committed_head: head, committed_tree: headTree, error: "HEAD identity differs from the exact Git-created authorized commit" }, now); throw new Error("commit transaction incident: HEAD identity changed after Git created the authorized commit; publication remains blocked"); } @@ -702,7 +710,7 @@ export async function runGitCommitTransaction( if (record !== undefined && dependencies.signal?.aborted === true && existsSync(binding.activePath)) { try { const active = readRecord(binding.activePath) ?? record; - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) === active.original_head) transition(binding, active, COMMIT_TRANSACTION_STATE.INTERRUPTED, { error: "commit transaction was cancelled" }, now); + if (resolveHead(binding.root) === active.original_head) transition(binding, active, COMMIT_TRANSACTION_STATE.INTERRUPTED, { error: "commit transaction was cancelled" }, now); } catch { /* retain the earlier durable state */ } } throw error; @@ -783,7 +791,7 @@ export function abandonCommitTransaction(cwd ) { try { const record = readRecord(binding.activePath); if (record === undefined) throw new Error("active commit transaction disappeared during recovery"); - if (git(binding.root, ["rev-parse", "--verify", "HEAD"]) !== record.original_head) throw new Error("cannot abandon a commit transaction after HEAD changed; reconcile the committed tree instead"); + if (resolveHead(binding.root) !== record.original_head) throw new Error("cannot abandon a commit transaction after HEAD changed; reconcile the committed tree instead"); const abandoned = transition(binding, record, COMMIT_TRANSACTION_STATE.ABANDONED, { error: "explicitly abandoned without changing HEAD or index" }, now); return archive(binding, abandoned); } finally { releaseLock(); } diff --git a/tests/git-commit-transaction.test.ts b/tests/git-commit-transaction.test.ts index fa8b0e3c6..1f5c6ffc3 100644 --- a/tests/git-commit-transaction.test.ts +++ b/tests/git-commit-transaction.test.ts @@ -300,3 +300,62 @@ test("cancellation cannot strand a commit after HEAD advances", async (t) => { assert.equal(result.status, "committed"); assert.deepEqual(inspectCommitTransaction(cwd), { status: "clean" }); }); + +function unbornRepository(t: test.TestContext): string { + const cwd = mkdtempSync(join(tmpdir(), "gentle-pi-commit-transaction-unborn-")); + t.after(() => rmSync(cwd, { recursive: true, force: true })); + git(cwd, "init", "-b", "main"); + git(cwd, "config", "user.name", "Commit Transaction Test"); + git(cwd, "config", "user.email", "commit-transaction@example.invalid"); + writeFileSync(join(cwd, "initial.txt"), "first commit\n"); + git(cwd, "add", "initial.txt"); + return cwd; +} + +test("an unborn repository creates its first reviewed commit without a prior HEAD", async (t) => { + const cwd = unbornRepository(t); + const intendedTree = git(cwd, "write-tree"); + const command = `git commit ${JSON.stringify("-m")} ${JSON.stringify("initial")}`; + const invocation = prepareCommitTransactionInvocation({ + command, + cwd, + arguments: ["-m", "initial"], + authorization: { + lineageId: "unborn-first", + storeRevision: "sha256:" + "a".repeat(64), + fingerprint: "sha256:" + "b".repeat(64), + intendedTree, + }, + }); + const result = await runGitCommitTransaction(invocation, { nativeReviewCli: native(cwd, "unborn-first") }); + assert.equal(result.status, "committed"); + assert.equal(result.tree, intendedTree); + assert.equal(result.head, git(cwd, "rev-parse", "HEAD")); + assert.equal(git(cwd, "rev-parse", "HEAD^{tree}"), intendedTree); + assert.deepEqual(inspectCommitTransaction(cwd), { status: "clean" }); + const verified = verifyCommitTransactionResult(cwd, result.transactionId); + assert.equal(verified.tree, intendedTree); +}); + +test("an unborn repository that fails to commit leaves no HEAD and allows exact retry", async (t) => { + const cwd = unbornRepository(t); + const intendedTree = git(cwd, "write-tree"); + const command = `git commit ${JSON.stringify("-m")} ${JSON.stringify("initial")}`; + const invocation = prepareCommitTransactionInvocation({ + command, + cwd, + arguments: ["-m", "initial"], + authorization: { + lineageId: "unborn-retry", + storeRevision: "sha256:" + "a".repeat(64), + fingerprint: "sha256:" + "b".repeat(64), + intendedTree, + }, + }); + await assert.rejects( + runGitCommitTransaction(invocation, { nativeReviewCli: native(cwd, "unborn-retry", "invalidated") }), + /native pre-commit validation denied/, + ); + assert.throws(() => git(cwd, "rev-parse", "--verify", "HEAD"), /fatal|Needed/i); + assert.equal(inspectCommitTransaction(cwd).record?.state, COMMIT_TRANSACTION_STATE.VALIDATION_FAILED); +}); diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index bc55d214b..46144acc2 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -313,6 +313,27 @@ function commitFileAfterBase(cwd: string): void { git(cwd, "-c", "user.name=Candidate Test", "-c", "user.email=candidate@example.invalid", "commit", "-m", "committed after base"); } +// An unborn repository has a symbolic HEAD pointing at a branch with no +// commits yet; its review base is Git's repository-native empty tree, not a +// missing or malformed commit. `mktree` with empty input derives that empty +// tree object-format-aware (sha1 or sha256) without hardcoding the SHA-1 id. +function emptyTreeOf(cwd: string): string { + return execFileSync("git", ["-C", cwd, "mktree"], { encoding: "utf8", input: "" }).trim(); +} + +function unbornRepository(t: test.TestContext, stage = true): string { + const cwd = mkdtempSync(join(tmpdir(), "gentle-pi-candidate-view-unborn-")); + t.after(() => rmSync(cwd, { recursive: true, force: true })); + git(cwd, "init", "-b", "main"); + git(cwd, "config", "user.name", "Candidate Test"); + git(cwd, "config", "user.email", "candidate@example.invalid"); + if (stage) { + writeFileSync(join(cwd, "staged.txt"), "first staged\n"); + git(cwd, "add", "staged.txt"); + } + return cwd; +} + test("candidate view materializes exact tracked and initially-untracked content while contributor diverges", (t) => { const contributorRoot = repository(t); writeFileSync(join(contributorRoot, "tracked.txt"), "frozen tracked\n"); @@ -1288,3 +1309,102 @@ test("a projection labeled workspace but derived as a genuinely committed range (error: unknown) => error instanceof CandidateViewError && error.reason === "projection-kind-drift", ); }); + +test("candidate view treats an unborn staged repository base as Git's empty tree without needing HEAD^{commit}", (t) => { + const contributorRoot = unbornRepository(t); + const view = createCandidateView({ contributorRoot }); + try { + const emptyTree = emptyTreeOf(contributorRoot); + assert.equal(view.baseTree, emptyTree); + assert.equal(view.baseCommit, "HEAD"); + assert.equal(view.committedOnly, false); + assert.notEqual(view.candidateTree, emptyTree); + assert.deepEqual(view.paths, ["staged.txt"]); + assert.deepEqual(view.deletedPaths, []); + } finally { + view.cleanup(); + } +}); + +test("candidate view of an unborn repository with no content yields an empty candidate scope instead of a misleading base-ref failure", (t) => { + const contributorRoot = unbornRepository(t, false); + const view = createCandidateView({ contributorRoot }); + try { + const emptyTree = emptyTreeOf(contributorRoot); + assert.equal(view.baseTree, emptyTree); + assert.equal(view.candidateTree, emptyTree); + assert.equal(view.baseCommit, "HEAD"); + assert.deepEqual(view.paths, []); + assert.deepEqual(view.deletedPaths, []); + } finally { + view.cleanup(); + } +}); + +test("candidate view materializes an unborn staged repository without mutating the contributor index or creating a phantom commit", (t) => { + const contributorRoot = unbornRepository(t); + const indexBefore = readFileSync(join(contributorRoot, ".git", "index")); + const view = createCandidateView({ contributorRoot }); + try { + // No phantom commit: HEAD stays unborn and no refs exist. + assert.throws(() => git(contributorRoot, "rev-parse", "--verify", "HEAD"), /fatal/i); + assert.equal(git(contributorRoot, "rev-list", "--all").length, 0); + // Contributor's real index is untouched; materialization uses a private index file. + assert.deepEqual(readFileSync(join(contributorRoot, ".git", "index")), indexBefore); + // The materialized candidate carries the staged content, isolated from the contributor tree. + assert.equal(readFileSync(join(view.root, "staged.txt"), "utf8"), "first staged\n"); + assert.deepEqual(view.paths, ["staged.txt"]); + } finally { + view.cleanup(); + } + // Cleanup removed the orphan worktree; no worktree lingers and no commit appeared. + assert.equal(git(contributorRoot, "worktree", "list").split("\n").filter((line) => line.includes("gentle-ai-candidate")).length, 0); + assert.equal(git(contributorRoot, "rev-list", "--all").length, 0); +}); + +test("candidate view fails closed for a detached HEAD pointing at a missing commit, not an unborn empty tree", (t) => { + const contributorRoot = unbornRepository(t); + // Detach HEAD to a non-existent object id. This is a corrupt/missing-object + // existing HEAD state, not a valid unborn symbolic HEAD, so it must fail + // closed rather than producing an empty-tree unborn candidate. + writeFileSync(join(contributorRoot, ".git", "HEAD"), "0".repeat(40)); + let failure: unknown; + try { + createCandidateView({ contributorRoot }); + } catch (error) { + failure = error; + } + assert.ok(failure instanceof CandidateViewError, "a detached missing HEAD must fail closed"); + assert.notEqual((failure as CandidateViewError).reason, undefined); + assert.ok((failure as CandidateViewError).reason !== "base-ref-moved"); +}); + +test("candidate view fails closed when symbolic HEAD target ref exists but points to a missing object, not unborn", (t) => { + const contributorRoot = repository(t); + // Break the main ref: it exists (show-ref --exists returns 0) but points to + // a missing object. This is a broken symbolic HEAD, not a valid unborn repo. + writeFileSync(join(contributorRoot, ".git", "refs", "heads", "main"), `${"0".repeat(40)}\n`); + let failure: unknown; + try { + createCandidateView({ contributorRoot }); + } catch (error) { + failure = error; + } + assert.ok(failure instanceof CandidateViewError, "a broken symbolic HEAD must fail closed, not unborn"); +}); + +test("candidate view probe does not convert a ref-existence probe timeout into an unborn empty-tree base", (t) => { + const contributorRoot = unbornRepository(t); + const executor: CandidateGitExecutor = (file, args, options) => { + if (args[0] === "rev-parse" && args.includes("--quiet")) throw Object.assign(new Error("timed out"), { code: "ETIMEDOUT", killed: true }); + return execFileSync(file, args, options); + }; + let failure: unknown; + try { + new CandidateViewRegistry(executor).create({ contributorRoot }); + } catch (error) { + failure = error; + } + assert.ok(failure instanceof CandidateViewError, "ref-existence probe timeout must fail closed"); + assert.equal((failure as CandidateViewError).reason, "candidate-view-timeout"); +}); diff --git a/tests/review-controller-native-routing.test.ts b/tests/review-controller-native-routing.test.ts index 20bd61e47..910fd9eb0 100644 --- a/tests/review-controller-native-routing.test.ts +++ b/tests/review-controller-native-routing.test.ts @@ -1843,6 +1843,47 @@ test("native START binds an acknowledged committed range and native identity to } }); +test("native START binds a default dirty-inclusive candidate on an unborn repository against Git's empty tree", async (t) => { + // Unborn repository: symbolic HEAD, no commits, staged + untracked content. + // The default targetStatus helper builds a REAL candidate view from this + // repo, so this exercises the real resolveCandidateBase/materializeCandidate + // path on an unborn repository, not a synthetic adapter stub. + const cwd = mkdtempSync(join(tmpdir(), "gentle-pi-native-controller-unborn-")); + t.after(() => rmSync(cwd, { recursive: true, force: true })); + execFileSync("git", ["init", "-b", "main"], { cwd }); + writeFileSync(join(cwd, "staged.txt"), "first staged\n"); + execFileSync("git", ["add", "staged.txt"], { cwd }); + writeFileSync(join(cwd, "untracked.ts"), "export const untracked = true;\n"); + const emptyTree = execFileSync("git", ["-C", cwd, "mktree"], { encoding: "utf8", input: "" }).trim(); + const candidateViews = new CandidateViewRegistry(); + const requests: Parameters[0][] = []; + const { controller } = runtime(fakeNative({ + start: async (request) => { + requests.push(request); + return { lineageId: "unborn-lineage", state: "reviewing", riskLevel: "medium", selectedLenses: ["review-reliability"], changedFiles: 2, changedLines: 2, correctionBudget: 1, action: "created", lensesRequired: true }; + }, + }), undefined, undefined, undefined, candidateViews); + const started = await controller.execute("unborn-start", { operation: "start", input: JSON.stringify({ mode: "ordinary" }) }, undefined, undefined, context(cwd)); + const view = candidateViews.resolveForLens("unborn-lineage", "review-reliability"); + try { + // The unborn candidate base is Git's empty tree, not a phantom commit. + assert.equal(view.baseTree, emptyTree); + assert.equal(view.baseCommit, "HEAD"); + assert.equal(view.committedOnly, false); + // Staged and untracked workspace content is preserved in the candidate. + assert.deepEqual(view.paths, ["staged.txt", "untracked.ts"]); + // No baseRef is sent to native START: the unborn default uses the + // workspace projection only, exactly as the provider expects. + assert.deepEqual(requests, [{ cwd, targetIdentity: `sha256:${"a".repeat(64)}`, projection: "workspace" }]); + const actorBinding = (started.details as { actor_binding: { workspace_root: string; candidate_root: string; candidate_tree: string; candidate_paths: readonly string[] } }).actor_binding; + assert.equal(actorBinding.workspace_root, cwd); + assert.equal(actorBinding.candidate_tree, view.candidateTree); + assert.deepEqual(actorBinding.candidate_paths, view.paths); + } finally { + view.cleanup(); + } +}); + test("native START fails closed before mutation when the workspace target and immutable candidate view differ", async (t) => { const cwd = repository(t); writeFileSync(join(cwd, "app.ts"), "export const value = 2;\n"); From 17d0edd9b9b3390662d2c359cdfd9127b6f5836a Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Wed, 12 Aug 2026 18:15:50 -0300 Subject: [PATCH 2/5] fix(review): harden unborn repository review handling --- lib/git-commit-transaction.ts | 41 ++++++++++++++++++++- lib/review-candidate-view.ts | 40 ++++++++++++++++++-- runtime/git-commit-transaction.mjs | 41 ++++++++++++++++++++- tests/git-commit-transaction.test.ts | 55 +++++++++++++++++++++++++++- tests/review-candidate-view.test.ts | 37 +++++++++++++++++++ 5 files changed, 207 insertions(+), 7 deletions(-) diff --git a/lib/git-commit-transaction.ts b/lib/git-commit-transaction.ts index a581d021a..6f1a6cec4 100644 --- a/lib/git-commit-transaction.ts +++ b/lib/git-commit-transaction.ts @@ -180,8 +180,47 @@ function absoluteGitPath(cwd: string, name: string): string { return isAbsolute(value) ? value : resolve(cwd, value); } +// Runs a probe that may exit nonzero as an expected signal (absent ref, unborn +// HEAD). Returns the exit status and trimmed stdout. Timeout and I/O failures +// propagate instead of being masked as a status, so callers fail closed. +function probeGit(cwd: string, args: readonly string[]): { status: number; stdout: string } { + try { + const stdout = execFileSync("git", args, { + cwd, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + timeout: GIT_TIMEOUT_MS, + windowsHide: true, + }); + return { status: 0, stdout: stdout.trim() }; + } catch (error) { + const detail = error as NodeJS.ErrnoException & { killed?: boolean; status?: number; stdout?: string | Buffer }; + if (detail.code === "ETIMEDOUT" || detail.killed === true) throw error; + if (typeof detail.status === "number") return { status: detail.status, stdout: typeof detail.stdout === "string" ? detail.stdout.trim() : "" }; + throw error; + } +} + +// Resolves HEAD to a commit SHA, or undefined only for a valid unborn symbolic +// HEAD (symbolic HEAD pointing at a branch with no commits). Timeout, I/O, +// corruption, and all other failures propagate (fail closed on uncertain HEAD +// state). Classification uses status-based probes, not localized stderr text. function resolveHead(cwd: string): string | undefined { - try { return git(cwd, ["rev-parse", "--verify", "HEAD"]); } catch { return undefined; } + try { + return git(cwd, ["rev-parse", "--verify", "HEAD"]); + } catch (error) { + const detail = error as NodeJS.ErrnoException & { killed?: boolean }; + if (detail.code === "ETIMEDOUT" || detail.killed === true) throw error; + if (typeof detail.status !== "number") throw error; + const symbolic = probeGit(cwd, ["symbolic-ref", "--quiet", "HEAD"]); + if (symbolic.status !== 0) throw error; + // show-ref --verify --quiet distinguishes: status 1 = ref absent (valid + // unborn), status 0 = ref exists and valid (rethrow original HEAD error), + // any other status (128, etc.) = corruption/missing object (fail closed). + const refProbe = probeGit(cwd, ["show-ref", "--verify", "--quiet", symbolic.stdout]); + if (refProbe.status === 1) return undefined; + throw error; + } } function repositoryBinding(cwd: string): RepositoryBinding { diff --git a/lib/review-candidate-view.ts b/lib/review-candidate-view.ts index a172bf7ac..be0908366 100644 --- a/lib/review-candidate-view.ts +++ b/lib/review-candidate-view.ts @@ -723,6 +723,37 @@ function checkoutMaterializedEntries(root: string, entries: readonly CandidateTr flush(); } +// Creates an unborn worktree (symbolic HEAD pointing at a branch with no +// commits, no ref written, no phantom commit). Git 2.42+ supports --orphan +// directly; older Git lacks the flag and reports an unsupported-option usage +// error (exit status 129). Only that exact status triggers the fallback: a +// temporary empty-tree commit seeds a detached --no-checkout worktree, then +// a symbolic-ref rewrite makes HEAD unborn. The temporary commit is never +// referenced by any ref and is GC-able, so it is not a phantom commit. The +// fallback uses a deterministic author/committer identity and timestamp in a +// copied environment so it does not depend on user.name/user.email or the +// ambient date. Contributor HEAD, branch, refs, and index are never touched. +function addUnbornWorktree(cwd: string, root: string, branch: string, env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): void { + const primary = probeCandidateGit(cwd, ["worktree", "add", "--orphan", "-b", branch, root], env, executor); + if (primary.status === 0) return; + // Only the unsupported-option usage status (129, pre-2.42 Git lacking --orphan) + // triggers the fallback. Every other status propagates as a git-failure. + if (primary.status !== 129 || existsSync(root)) throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.GIT_FAILURE, ["worktree", "add", "--orphan", "-b", branch, root], resolveCandidateGitTimeoutMs(env)); + const fallbackEnv = { + ...env, + GIT_AUTHOR_NAME: "gentle-ai-candidate", + GIT_AUTHOR_EMAIL: "gentle-ai-candidate@example.invalid", + GIT_AUTHOR_DATE: "2000-01-01T00:00:00Z", + GIT_COMMITTER_NAME: "gentle-ai-candidate", + GIT_COMMITTER_EMAIL: "gentle-ai-candidate@example.invalid", + GIT_COMMITTER_DATE: "2000-01-01T00:00:00Z", + }; + const emptyTree = git(cwd, ["mktree"], fallbackEnv, executor); + const tempCommit = git(cwd, ["commit-tree", "-m", "gentle-ai-candidate", emptyTree], fallbackEnv, executor); + git(cwd, ["worktree", "add", "--no-checkout", "--detach", root, tempCommit], env, executor); + git(root, ["symbolic-ref", "HEAD", `refs/heads/${branch}`], env, executor); +} + function materializeCandidateView(request: CreateCandidateViewRequest, executor: CandidateGitExecutor): CandidateViewRecord { const contributorRoot = realpathSync(request.contributorRoot); if (!lstatSync(contributorRoot).isDirectory()) throw new CandidateViewError("contributor root is not a directory"); @@ -748,10 +779,11 @@ function materializeCandidateView(request: CreateCandidateViewRequest, executor: if (!committedOnly) git(contributorRoot, ["add", "-A"], environment, executor); const candidateTree = git(contributorRoot, ["write-tree"], environment, executor); const root = join(parent, randomUUID()); - // An unborn repository has no commit to detach a worktree at, so use an - // orphan worktree (unborn branch, no commit, no ref written until a - // commit) to host the materialized candidate tree without a phantom commit. - if (unborn) git(contributorRoot, ["worktree", "add", "--orphan", "-b", `gentle-ai-candidate-${randomUUID()}`, root], process.env, executor); + // An unborn repository has no commit to detach a worktree at. addUnbornWorktree + // creates an orphan worktree (unborn branch, no commit, no ref) to host the + // materialized candidate tree without a phantom commit, with a fallback for + // Git versions older than 2.42 that do not support --orphan. + if (unborn) addUnbornWorktree(contributorRoot, root, `gentle-ai-candidate-${randomUUID()}`, process.env, executor); else git(contributorRoot, ["worktree", "add", "--detach", "--no-checkout", root, candidateCommit.commit], process.env, executor); try { git(root, ["read-tree", candidateTree], process.env, executor); diff --git a/runtime/git-commit-transaction.mjs b/runtime/git-commit-transaction.mjs index e3db105de..4c557aa8f 100644 --- a/runtime/git-commit-transaction.mjs +++ b/runtime/git-commit-transaction.mjs @@ -181,8 +181,47 @@ function absoluteGitPath(cwd , name ) { return isAbsolute(value) ? value : resolve(cwd, value); } +// Runs a probe that may exit nonzero as an expected signal (absent ref, unborn +// HEAD). Returns the exit status and trimmed stdout. Timeout and I/O failures +// propagate instead of being masked as a status, so callers fail closed. +function probeGit(cwd , args ) { + try { + const stdout = execFileSync("git", args, { + cwd, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + timeout: GIT_TIMEOUT_MS, + windowsHide: true, + }); + return { status: 0, stdout: stdout.trim() }; + } catch (error) { + const detail = error ; + if (detail.code === "ETIMEDOUT" || detail.killed === true) throw error; + if (typeof detail.status === "number") return { status: detail.status, stdout: typeof detail.stdout === "string" ? detail.stdout.trim() : "" }; + throw error; + } +} + +// Resolves HEAD to a commit SHA, or undefined only for a valid unborn symbolic +// HEAD (symbolic HEAD pointing at a branch with no commits). Timeout, I/O, +// corruption, and all other failures propagate (fail closed on uncertain HEAD +// state). Classification uses status-based probes, not localized stderr text. function resolveHead(cwd ) { - try { return git(cwd, ["rev-parse", "--verify", "HEAD"]); } catch { return undefined; } + try { + return git(cwd, ["rev-parse", "--verify", "HEAD"]); + } catch (error) { + const detail = error ; + if (detail.code === "ETIMEDOUT" || detail.killed === true) throw error; + if (typeof detail.status !== "number") throw error; + const symbolic = probeGit(cwd, ["symbolic-ref", "--quiet", "HEAD"]); + if (symbolic.status !== 0) throw error; + // show-ref --verify --quiet distinguishes: status 1 = ref absent (valid + // unborn), status 0 = ref exists and valid (rethrow original HEAD error), + // any other status (128, etc.) = corruption/missing object (fail closed). + const refProbe = probeGit(cwd, ["show-ref", "--verify", "--quiet", symbolic.stdout]); + if (refProbe.status === 1) return undefined; + throw error; + } } function repositoryBinding(cwd ) { diff --git a/tests/git-commit-transaction.test.ts b/tests/git-commit-transaction.test.ts index 1f5c6ffc3..101143ebf 100644 --- a/tests/git-commit-transaction.test.ts +++ b/tests/git-commit-transaction.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; -import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; @@ -14,6 +14,7 @@ import { runGitCommitTransaction, verifyCommitTransactionResult, } from "../lib/git-commit-transaction.ts"; +import type { CommitTransactionInvocation } from "../lib/git-commit-transaction.ts"; import type { NativeReviewCli, NativeValidateResult } from "../lib/native-review-cli.ts"; function git(cwd: string, ...arguments_: string[]): string { @@ -359,3 +360,55 @@ test("an unborn repository that fails to commit leaves no HEAD and allows exact assert.throws(() => git(cwd, "rev-parse", "--verify", "HEAD"), /fatal|Needed/i); assert.equal(inspectCommitTransaction(cwd).record?.state, COMMIT_TRANSACTION_STATE.VALIDATION_FAILED); }); + +function unbornInvocation(cwd: string, lineageId: string): CommitTransactionInvocation { + return prepareCommitTransactionInvocation({ + command: "git commit -m initial", + cwd, + arguments: ["-m", "initial"], + authorization: { + lineageId, + storeRevision: "sha256:" + "a".repeat(64), + fingerprint: "sha256:" + "b".repeat(64), + intendedTree: git(cwd, "write-tree"), + }, + }); +} + +test("resolveHead fails closed for a corrupt HEAD ref instead of masking it as unborn", (t) => { + const cwd = unbornRepository(t); + // Garbage ref (not a valid SHA): symbolic-ref --quiet HEAD fails (128), + // so resolveHead rethrows instead of returning undefined. + mkdirSync(join(cwd, ".git", "refs", "heads"), { recursive: true }); + writeFileSync(join(cwd, ".git", "refs", "heads", "main"), "z".repeat(40)); + assert.throws(() => unbornInvocation(cwd, "corrupt-head")); + assert.equal(inspectCommitTransaction(cwd).status, "clean"); +}); + +test("resolveHead does not mask a symbolic HEAD pointing to a missing object as unborn", (t) => { + const cwd = unbornRepository(t); + // Valid-format SHA with no object: rev-parse --verify HEAD succeeds (returns + // the SHA), so resolveHead never enters the unborn classification. + mkdirSync(join(cwd, ".git", "refs", "heads"), { recursive: true }); + writeFileSync(join(cwd, ".git", "refs", "heads", "main"), "0123456789abcdef0123456789abcdef01234567\n"); + const invocation = unbornInvocation(cwd, "missing-object-head"); + assert.equal(invocation.cwd, realpathSync(cwd)); + assert.equal(inspectCommitTransaction(cwd).status, "clean"); +}); + +test("resolveHead propagates a probe timeout instead of masking it as an unborn HEAD", (t) => { + const cwd = unbornRepository(t); + const wrapperDir = mkdtempSync(join(tmpdir(), "gentle-pi-commit-transaction-timeout-")); + t.after(() => rmSync(wrapperDir, { recursive: true, force: true })); + const realGit = execFileSync("which", ["git"], { encoding: "utf8" }).trim(); + writeFileSync(join(wrapperDir, "git"), `#!/bin/sh\nif [ "$1" = "rev-parse" ] && [ "$2" = "--verify" ] && [ "$3" = "HEAD" ] && [ $# -eq 3 ]; then\nsleep 30\nelse\nexec ${realGit} "$@"\nfi\n`); + chmodSync(join(wrapperDir, "git"), 0o755); + const originalPath = process.env.PATH; + process.env.PATH = `${wrapperDir}:${originalPath}`; + try { + assert.throws(() => unbornInvocation(cwd, "timeout-head")); + } finally { + process.env.PATH = originalPath; + } + assert.equal(inspectCommitTransaction(cwd).status, "clean"); +}); diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index 46144acc2..bf844392b 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -1408,3 +1408,40 @@ test("candidate view probe does not convert a ref-existence probe timeout into a assert.ok(failure instanceof CandidateViewError, "ref-existence probe timeout must fail closed"); assert.equal((failure as CandidateViewError).reason, "candidate-view-timeout"); }); + +test("candidate view unborn worktree falls back when --orphan is unsupported, preserving isolation, no phantom commit, and contributor immutability", (t) => { + const contributorRoot = unbornRepository(t); + const indexBefore = readFileSync(join(contributorRoot, ".git", "index")); + // Intercept --orphan with status 129 (Git < 2.42 unknown option). + const executor: CandidateGitExecutor = (file, args, options) => { + if (args[0] === "worktree" && args[1] === "add" && args.includes("--orphan")) throw Object.assign(new Error("unknown option"), { status: 129 }); + return execFileSync(file, args, options); + }; + const view = new CandidateViewRegistry(executor).create({ contributorRoot }); + try { + assert.equal(readFileSync(join(view.root, "staged.txt"), "utf8"), "first staged\n"); + assert.deepEqual(view.paths, ["staged.txt"]); + assert.equal(view.baseTree, emptyTreeOf(contributorRoot)); + // No phantom commit; contributor index untouched. + assert.throws(() => git(contributorRoot, "rev-parse", "--verify", "HEAD"), /fatal/i); + assert.equal(git(contributorRoot, "rev-list", "--all").length, 0); + assert.deepEqual(readFileSync(join(contributorRoot, ".git", "index")), indexBefore); + } finally { + view.cleanup(); + } + assert.equal(git(contributorRoot, "worktree", "list").split("\n").filter((line) => line.includes("gentle-ai-candidate")).length, 0); + assert.equal(git(contributorRoot, "rev-list", "--all").length, 0); +}); + +test("candidate view unborn worktree propagates a non-usage --orphan failure instead of falling back", (t) => { + const contributorRoot = unbornRepository(t); + // A non-129 status must not trigger the fallback. + const executor: CandidateGitExecutor = (file, args, options) => { + if (args[0] === "worktree" && args[1] === "add" && args.includes("--orphan")) throw Object.assign(new Error("genuine failure"), { status: 128 }); + return execFileSync(file, args, options); + }; + let failure: unknown; + try { new CandidateViewRegistry(executor).create({ contributorRoot }); } catch (error) { failure = error; } + assert.ok(failure instanceof CandidateViewError, "a non-usage --orphan failure must propagate"); + assert.equal((failure as CandidateViewError).reason, "candidate-view-git-failure"); +}); From db9eea875f85af238d2d22a9507f677c1bbbd547 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Wed, 12 Aug 2026 21:55:42 -0300 Subject: [PATCH 3/5] test(review): strengthen unborn failure assertions --- tests/git-commit-transaction.test.ts | 11 ++++++++++- tests/review-candidate-view.test.ts | 4 ++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/git-commit-transaction.test.ts b/tests/git-commit-transaction.test.ts index 101143ebf..e3b52cfdc 100644 --- a/tests/git-commit-transaction.test.ts +++ b/tests/git-commit-transaction.test.ts @@ -406,7 +406,16 @@ test("resolveHead propagates a probe timeout instead of masking it as an unborn const originalPath = process.env.PATH; process.env.PATH = `${wrapperDir}:${originalPath}`; try { - assert.throws(() => unbornInvocation(cwd, "timeout-head")); + // The probe wrapper sleeps past GIT_TIMEOUT_MS (10s), so execFileSync + // kills the process and resolveHead rethrows the raw timeout error + // rather than classifying it as an unborn HEAD. Accept only a failure + // that genuinely represents a timeout, not any arbitrary error. + assert.throws( + () => unbornInvocation(cwd, "timeout-head"), + (error: unknown) => error !== null && typeof error === "object" + && ((error as { code?: unknown }).code === "ETIMEDOUT" || (error as { killed?: unknown }).killed === true), + "resolveHead must propagate a probe timeout instead of masking it as an unborn HEAD", + ); } finally { process.env.PATH = originalPath; } diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index bf844392b..a6868efc8 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -1436,7 +1436,9 @@ test("candidate view unborn worktree falls back when --orphan is unsupported, pr test("candidate view unborn worktree propagates a non-usage --orphan failure instead of falling back", (t) => { const contributorRoot = unbornRepository(t); // A non-129 status must not trigger the fallback. + const calls: string[][] = []; const executor: CandidateGitExecutor = (file, args, options) => { + calls.push([...args]); if (args[0] === "worktree" && args[1] === "add" && args.includes("--orphan")) throw Object.assign(new Error("genuine failure"), { status: 128 }); return execFileSync(file, args, options); }; @@ -1444,4 +1446,6 @@ test("candidate view unborn worktree propagates a non-usage --orphan failure ins try { new CandidateViewRegistry(executor).create({ contributorRoot }); } catch (error) { failure = error; } assert.ok(failure instanceof CandidateViewError, "a non-usage --orphan failure must propagate"); assert.equal((failure as CandidateViewError).reason, "candidate-view-git-failure"); + assert.ok(!calls.some((args) => args[0] === "commit-tree"), "the fallback must not create a temporary commit"); + assert.ok(!calls.some((args) => args[0] === "worktree" && args.includes("--detach")), "the fallback must not add a detached worktree"); }); From 82f2a39630b3f84d3d506afb4ba0e0d112ec9544 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Thu, 13 Aug 2026 15:28:29 -0300 Subject: [PATCH 4/5] fix(review): resolve unborn review follow-ups --- lib/git-commit-transaction.ts | 15 +++- lib/review-candidate-view.ts | 18 ++-- runtime/git-commit-transaction.mjs | 15 +++- tests/git-commit-transaction.test.ts | 127 ++++++++++++++++++++++++--- tests/review-candidate-view.test.ts | 25 +++++- 5 files changed, 181 insertions(+), 19 deletions(-) diff --git a/lib/git-commit-transaction.ts b/lib/git-commit-transaction.ts index 6f1a6cec4..1c2bc4d44 100644 --- a/lib/git-commit-transaction.ts +++ b/lib/git-commit-transaction.ts @@ -229,7 +229,20 @@ function repositoryBinding(cwd: string): RepositoryBinding { const gitDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-dir"]); const commonDir = realpathSync(commonDirValue); const gitDir = realpathSync(gitDirValue); - const repositoryId = sha256(canonicalJson({ common_directory: commonDir })); + // Preserve the durable repository identity across the unborn-handling + // upgrade: a born repository keeps the byte-for-byte previous formula + // `sha256(canonicalJson({ common_directory: commonDir, roots }))` with + // `roots` the sorted root commits reachable from HEAD. An unborn + // repository has no HEAD, so `rev-list HEAD` cannot run; resolveHead + // already classifies HEAD state and propagates timeout/corruption/I/O + // failures rather than masking them, so the unborn branch gets a + // deterministic safe roots representation (the empty set) without + // hiding real errors. + const head = resolveHead(root); + const roots = head === undefined + ? [] + : git(root, ["rev-list", "--max-parents=0", "HEAD"]).split(/\r?\n/).filter(Boolean).sort(); + const repositoryId = sha256(canonicalJson({ common_directory: commonDir, roots })); const worktreeKey = sha256(gitDir).slice("sha256:".length, "sha256:".length + 24); const stateDir = join(commonDir, "gentle-pi", "commit-transactions", worktreeKey); return { diff --git a/lib/review-candidate-view.ts b/lib/review-candidate-view.ts index be0908366..07f82826c 100644 --- a/lib/review-candidate-view.ts +++ b/lib/review-candidate-view.ts @@ -779,13 +779,19 @@ function materializeCandidateView(request: CreateCandidateViewRequest, executor: if (!committedOnly) git(contributorRoot, ["add", "-A"], environment, executor); const candidateTree = git(contributorRoot, ["write-tree"], environment, executor); const root = join(parent, randomUUID()); - // An unborn repository has no commit to detach a worktree at. addUnbornWorktree - // creates an orphan worktree (unborn branch, no commit, no ref) to host the - // materialized candidate tree without a phantom commit, with a fallback for - // Git versions older than 2.42 that do not support --orphan. - if (unborn) addUnbornWorktree(contributorRoot, root, `gentle-ai-candidate-${randomUUID()}`, process.env, executor); - else git(contributorRoot, ["worktree", "add", "--detach", "--no-checkout", root, candidateCommit.commit], process.env, executor); + // The worktree is created under the same try/catch cleanup boundary as + // the read-tree materialization that follows. addUnbornWorktree's + // fallback path can register a worktree with `worktree add` and then + // fail on a later step (for example `symbolic-ref`); moving creation + // here ensures any such partial registration is removed by the catch + // below instead of leaking a registered/admin worktree and directory. try { + // An unborn repository has no commit to detach a worktree at. addUnbornWorktree + // creates an orphan worktree (unborn branch, no commit, no ref) to host the + // materialized candidate tree without a phantom commit, with a fallback for + // Git versions older than 2.42 that do not support --orphan. + if (unborn) addUnbornWorktree(contributorRoot, root, `gentle-ai-candidate-${randomUUID()}`, process.env, executor); + else git(contributorRoot, ["worktree", "add", "--detach", "--no-checkout", root, candidateCommit.commit], process.env, executor); git(root, ["read-tree", candidateTree], process.env, executor); const tree = parseTree(root, candidateTree, executor); checkoutMaterializedEntries(root, tree.entries, executor); diff --git a/runtime/git-commit-transaction.mjs b/runtime/git-commit-transaction.mjs index 4c557aa8f..de9e40648 100644 --- a/runtime/git-commit-transaction.mjs +++ b/runtime/git-commit-transaction.mjs @@ -230,7 +230,20 @@ function repositoryBinding(cwd ) { const gitDirValue = git(root, ["rev-parse", "--path-format=absolute", "--git-dir"]); const commonDir = realpathSync(commonDirValue); const gitDir = realpathSync(gitDirValue); - const repositoryId = sha256(canonicalJson({ common_directory: commonDir })); + // Preserve the durable repository identity across the unborn-handling + // upgrade: a born repository keeps the byte-for-byte previous formula + // `sha256(canonicalJson({ common_directory: commonDir, roots }))` with + // `roots` the sorted root commits reachable from HEAD. An unborn + // repository has no HEAD, so `rev-list HEAD` cannot run; resolveHead + // already classifies HEAD state and propagates timeout/corruption/I/O + // failures rather than masking them, so the unborn branch gets a + // deterministic safe roots representation (the empty set) without + // hiding real errors. + const head = resolveHead(root); + const roots = head === undefined + ? [] + : git(root, ["rev-list", "--max-parents=0", "HEAD"]).split(/\r?\n/).filter(Boolean).sort(); + const repositoryId = sha256(canonicalJson({ common_directory: commonDir, roots })); const worktreeKey = sha256(gitDir).slice("sha256:".length, "sha256:".length + 24); const stateDir = join(commonDir, "gentle-pi", "commit-transactions", worktreeKey); return { diff --git a/tests/git-commit-transaction.test.ts b/tests/git-commit-transaction.test.ts index e3b52cfdc..ce0179211 100644 --- a/tests/git-commit-transaction.test.ts +++ b/tests/git-commit-transaction.test.ts @@ -1,5 +1,6 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -8,6 +9,7 @@ import { setTimeout as delay } from "node:timers/promises"; import { COMMIT_TRANSACTION_STATE, assertNoUnresolvedCommitTransaction, + abandonCommitTransaction, inspectCommitTransaction, prepareCommitTransactionInvocation, reconcileCommitTransaction, @@ -359,6 +361,31 @@ test("an unborn repository that fails to commit leaves no HEAD and allows exact ); assert.throws(() => git(cwd, "rev-parse", "--verify", "HEAD"), /fatal|Needed/i); assert.equal(inspectCommitTransaction(cwd).record?.state, COMMIT_TRANSACTION_STATE.VALIDATION_FAILED); + // A denied attempt leaves the index intact and HEAD unborn, so the exact + // commit command can be retried. The failed transaction requires explicit + // recovery: abandon archives it (HEAD never moved, so abandon is allowed), + // then a fresh invocation authorizes the same tree and commits successfully. + abandonCommitTransaction(cwd); + assert.equal(inspectCommitTransaction(cwd).status, "clean"); + const retry = prepareCommitTransactionInvocation({ + command, + cwd, + arguments: ["-m", "initial"], + authorization: { + lineageId: "unborn-retry", + storeRevision: "sha256:" + "a".repeat(64), + fingerprint: "sha256:" + "b".repeat(64), + intendedTree, + }, + }); + const result = await runGitCommitTransaction(retry, { nativeReviewCli: native(cwd, "unborn-retry") }); + assert.equal(result.status, "committed"); + assert.equal(result.tree, intendedTree); + assert.equal(result.head, git(cwd, "rev-parse", "HEAD")); + assert.equal(git(cwd, "rev-parse", "HEAD^{tree}"), intendedTree); + assert.deepEqual(inspectCommitTransaction(cwd), { status: "clean" }); + const verified = verifyCommitTransactionResult(cwd, result.transactionId); + assert.equal(verified.tree, intendedTree); }); function unbornInvocation(cwd: string, lineageId: string): CommitTransactionInvocation { @@ -375,25 +402,105 @@ function unbornInvocation(cwd: string, lineageId: string): CommitTransactionInvo }); } -test("resolveHead fails closed for a corrupt HEAD ref instead of masking it as unborn", (t) => { +// Mirrors lib/git-commit-transaction.ts canonicalJson + sha256 so tests can +// prove the durable repository identity is byte-for-byte compatible with the +// previous formula `sha256(canonicalJson({ common_directory: commonDir, roots }))`. +function canonicalJson(value: unknown): string { + if (value === null || typeof value !== "object") return JSON.stringify(value); + if (Array.isArray(value)) return `[${value.map(canonicalJson).join(",")}]`; + const object = value as Record; + return `{${Object.keys(object).filter((key) => object[key] !== undefined).sort().map((key) => `${JSON.stringify(key)}:${canonicalJson(object[key])}`).join(",")}}`; +} +function identitySha256(value: string): string { + return `sha256:${createHash("sha256").update(value).digest("hex")}`; +} + +test("a born repository keeps the durable transaction identity formula with sorted root commits across the unborn-handling upgrade", async (t) => { + const cwd = repository(t); + stage(cwd); + installHook(cwd, "pre-commit", "exit 23"); + const commonDir = realpathSync(git(cwd, "rev-parse", "--path-format=absolute", "--git-common-dir")); + const roots = git(cwd, "rev-list", "--max-parents=0", "HEAD").split(/\r?\n/).filter(Boolean).sort(); + const expected = identitySha256(canonicalJson({ common_directory: commonDir, roots })); + await assert.rejects( + runGitCommitTransaction(invocation(cwd, "born-identity"), { nativeReviewCli: native(cwd, "born-identity") }), + /pre-commit hook failed/, + ); + const record = inspectCommitTransaction(cwd).record; + assert.ok(record, "a failing hook leaves an active transaction record carrying the repository identity"); + assert.equal(record!.repository_id, expected); + assert.equal(record!.common_directory, commonDir); + abandonCommitTransaction(cwd); + assert.equal(inspectCommitTransaction(cwd).status, "clean"); +}); + +test("an unborn repository derives a deterministic repository identity without rev-list HEAD and without masking errors", async (t) => { + const cwd = unbornRepository(t); + const commonDir = realpathSync(git(cwd, "rev-parse", "--path-format=absolute", "--git-common-dir")); + // Unborn has no root commits reachable from HEAD; the identity uses the + // empty roots set, which is deterministic and avoids `rev-list HEAD`. + const expected = identitySha256(canonicalJson({ common_directory: commonDir, roots: [] })); + const intendedTree = git(cwd, "write-tree"); + const command = `git commit ${JSON.stringify("-m")} ${JSON.stringify("initial")}`; + const invocation = prepareCommitTransactionInvocation({ + command, + cwd, + arguments: ["-m", "initial"], + authorization: { + lineageId: "unborn-identity", + storeRevision: "sha256:" + "a".repeat(64), + fingerprint: "sha256:" + "b".repeat(64), + intendedTree, + }, + }); + await assert.rejects( + runGitCommitTransaction(invocation, { nativeReviewCli: native(cwd, "unborn-identity", "invalidated") }), + /native pre-commit validation denied/, + ); + const record = inspectCommitTransaction(cwd).record; + assert.ok(record, "a denied unborn attempt leaves an active transaction record carrying the repository identity"); + assert.equal(record!.repository_id, expected); + assert.equal(record!.common_directory, commonDir); + abandonCommitTransaction(cwd); + assert.equal(inspectCommitTransaction(cwd).status, "clean"); +}); + +// Resolves the canonical absolute git common directory for asserting the +// absence of the transaction state root without assuming a literal `.git`. +function transactionStateRoot(cwd: string): string { + const commonDir = realpathSync(git(cwd, "rev-parse", "--path-format=absolute", "--git-common-dir")); + return join(commonDir, "gentle-pi", "commit-transactions"); +} + +test("preparation fails closed for a corrupt HEAD ref without writing transaction state and reports corruption", (t) => { const cwd = unbornRepository(t); - // Garbage ref (not a valid SHA): symbolic-ref --quiet HEAD fails (128), - // so resolveHead rethrows instead of returning undefined. + // A garbage ref (not a valid SHA) makes HEAD unresolvable. Resolving root + // commits during repository binding rethrows, so preparation must throw, + // no transaction state may be written, and inspection reports corruption + // (repository identity itself cannot be resolved while HEAD is corrupt). mkdirSync(join(cwd, ".git", "refs", "heads"), { recursive: true }); writeFileSync(join(cwd, ".git", "refs", "heads", "main"), "z".repeat(40)); assert.throws(() => unbornInvocation(cwd, "corrupt-head")); - assert.equal(inspectCommitTransaction(cwd).status, "clean"); + assert.equal(existsSync(transactionStateRoot(cwd)), false, "no transaction state root is written for a corrupt HEAD"); + const inspection = inspectCommitTransaction(cwd); + assert.equal(inspection.status, "corrupted", "repository corruption makes inspection corrupted even with no transaction state written"); + assert.ok(typeof inspection.reason === "string" && inspection.reason.length > 0, "inspection reason is present and actionable"); }); -test("resolveHead does not mask a symbolic HEAD pointing to a missing object as unborn", (t) => { +test("preparation fails closed for a symbolic HEAD pointing at a missing object instead of classifying it as unborn", (t) => { const cwd = unbornRepository(t); - // Valid-format SHA with no object: rev-parse --verify HEAD succeeds (returns - // the SHA), so resolveHead never enters the unborn classification. + // A valid-format SHA with no object: rev-parse --verify HEAD succeeds, so + // resolveHead returns a commit id rather than classifying HEAD as unborn. + // Resolving root commits then fails on the missing object, so preparation + // must throw, no transaction state may be written, and inspection reports + // corruption. This must never be classified as an unborn empty-roots repo. mkdirSync(join(cwd, ".git", "refs", "heads"), { recursive: true }); writeFileSync(join(cwd, ".git", "refs", "heads", "main"), "0123456789abcdef0123456789abcdef01234567\n"); - const invocation = unbornInvocation(cwd, "missing-object-head"); - assert.equal(invocation.cwd, realpathSync(cwd)); - assert.equal(inspectCommitTransaction(cwd).status, "clean"); + assert.throws(() => unbornInvocation(cwd, "missing-object-head")); + assert.equal(existsSync(transactionStateRoot(cwd)), false, "no transaction state root is written for a missing-object HEAD"); + const inspection = inspectCommitTransaction(cwd); + assert.equal(inspection.status, "corrupted", "a missing-object HEAD is repository corruption, not a clean or unborn repo"); + assert.ok(typeof inspection.reason === "string" && inspection.reason.length > 0, "inspection reason is present and actionable"); }); test("resolveHead propagates a probe timeout instead of masking it as an unborn HEAD", (t) => { diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index a6868efc8..4bee9b630 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; import { createHash } from "node:crypto"; -import { chmodSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, renameSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, realpathSync, renameSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; @@ -1449,3 +1449,26 @@ test("candidate view unborn worktree propagates a non-usage --orphan failure ins assert.ok(!calls.some((args) => args[0] === "commit-tree"), "the fallback must not create a temporary commit"); assert.ok(!calls.some((args) => args[0] === "worktree" && args.includes("--detach")), "the fallback must not add a detached worktree"); }); + +test("candidate view cleans up a partially registered unborn fallback worktree when a later step fails", (t) => { + const contributorRoot = unbornRepository(t); + // Force the --orphan path to take the pre-2.42 fallback, then fail the + // symbolic-ref rewrite that follows `worktree add --no-checkout --detach`. + // The worktree is already registered with Git at that point; the cleanup + // boundary must remove both the registered worktree and its directory. + const executor: CandidateGitExecutor = (file, args, options) => { + if (args[0] === "worktree" && args[1] === "add" && args.includes("--orphan")) throw Object.assign(new Error("unknown option"), { status: 129 }); + if (args[0] === "symbolic-ref") throw Object.assign(new Error("symbolic-ref failed"), { status: 1 }); + return execFileSync(file, args, options); + }; + let failure: unknown; + try { new CandidateViewRegistry(executor).create({ contributorRoot }); } catch (error) { failure = error; } + assert.ok(failure instanceof CandidateViewError, "the symbolic-ref failure must propagate"); + assert.equal((failure as CandidateViewError).reason, "candidate-view-git-failure"); + // No registered/admin worktree remains. + assert.equal(git(contributorRoot, "worktree", "list").split("\n").filter((line) => line.includes("gentle-ai-candidate")).length, 0); + // No candidate directory remains under the candidate-view parent. + const parent = join(realpathSync(git(contributorRoot, "rev-parse", "--path-format=absolute", "--git-common-dir")), "gentle-ai", "candidate-views"); + const leftover = existsSync(parent) ? readdirSync(parent).filter((entry) => lstatSync(join(parent, entry)).isDirectory()) : []; + assert.deepEqual(leftover, [], "no candidate directory remains after a partial unborn fallback failure"); +}); From efe339a1aa5c6fbc13e339cc0e63b65cf1c4ba03 Mon Sep 17 00:00:00 2001 From: Alan-TheGentleman Date: Fri, 14 Aug 2026 19:42:31 +0200 Subject: [PATCH 5/5] fix(review): fail closed on unexpected unborn probe status and cover sha256 unborn repositories --- lib/review-candidate-view.ts | 16 ++++++---- tests/review-candidate-view.test.ts | 47 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/lib/review-candidate-view.ts b/lib/review-candidate-view.ts index 07f82826c..19c2944a5 100644 --- a/lib/review-candidate-view.ts +++ b/lib/review-candidate-view.ts @@ -647,15 +647,19 @@ function probeCandidateGit(cwd: string, arguments_: readonly string[], env: Node // An unborn repository's HEAD is a symbolic ref to a branch with no commits. // `symbolic-ref --quiet HEAD` exits nonzero for a detached HEAD (not unborn). -// `rev-parse --verify --quiet ` distinguishes a valid unborn (nonzero, ref -// absent) from a broken symbolic ref (exit 0, ref OID text exists even when the -// object is missing). Any unexpected outcome fails closed. +// `rev-parse --verify --quiet ` distinguishes a valid unborn (status 1, +// ref absent) from a broken symbolic ref (exit 0, ref OID text exists even +// when the object is missing). Any other status (128, etc.) signals +// corruption or an I/O failure, so it fails closed instead of masquerading as +// an unborn repository. function isUnbornSymbolicHead(cwd: string, env: NodeJS.ProcessEnv, executor: CandidateGitExecutor): boolean { const symbolic = probeCandidateGit(cwd, ["symbolic-ref", "--quiet", "HEAD"], env, executor); if (symbolic.status !== 0) return false; - const refProbe = probeCandidateGit(cwd, ["rev-parse", "--verify", "--quiet", symbolic.stdout], env, executor); - if (refProbe.status !== 0) return true; - return false; + const refProbeArguments = ["rev-parse", "--verify", "--quiet", symbolic.stdout]; + const refProbe = probeCandidateGit(cwd, refProbeArguments, env, executor); + if (refProbe.status === 1) return true; + if (refProbe.status === 0) return false; + throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.GIT_FAILURE, refProbeArguments, resolveCandidateGitTimeoutMs(env)); } // Derives Git's repository-native empty tree without hardcoding the SHA-1 id, diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index 4bee9b630..5c843d611 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -1409,6 +1409,53 @@ test("candidate view probe does not convert a ref-existence probe timeout into a assert.equal((failure as CandidateViewError).reason, "candidate-view-timeout"); }); +test("candidate view fails closed when the unborn ref probe exits with an unexpected status instead of treating it as unborn", (t) => { + const contributorRoot = unbornRepository(t); + // Only status 1 means "ref absent" (valid unborn). Any other nonzero status + // (128, etc.) signals corruption or an I/O failure and must fail closed, + // never masquerade as an unborn empty-tree base. + const executor: CandidateGitExecutor = (file, args, options) => { + if (args[0] === "rev-parse" && args.includes("--quiet")) throw Object.assign(new Error("fatal: probe failed"), { status: 128 }); + return execFileSync(file, args, options); + }; + let failure: unknown; + try { + new CandidateViewRegistry(executor).create({ contributorRoot }); + } catch (error) { + failure = error; + } + assert.ok(failure instanceof CandidateViewError, "an unexpected unborn probe status must fail closed, not unborn"); + assert.equal((failure as CandidateViewError).reason, "candidate-view-git-failure"); +}); + +test("candidate view treats an unborn sha256 repository base as the repository-native sha256 empty tree", (t) => { + const contributorRoot = mkdtempSync(join(tmpdir(), "gentle-pi-candidate-view-unborn-sha256-")); + t.after(() => rmSync(contributorRoot, { recursive: true, force: true })); + try { + git(contributorRoot, "init", "--object-format=sha256", "-b", "main"); + } catch { + t.skip("installed git does not support `git init --object-format=sha256`"); + return; + } + git(contributorRoot, "config", "user.name", "Candidate Test"); + git(contributorRoot, "config", "user.email", "candidate@example.invalid"); + writeFileSync(join(contributorRoot, "staged.txt"), "first staged\n"); + git(contributorRoot, "add", "staged.txt"); + const view = createCandidateView({ contributorRoot }); + try { + const emptyTree = emptyTreeOf(contributorRoot); + assert.equal(emptyTree.length, 64, "a sha256 repository must derive a 64-hex empty tree, not the hardcoded SHA-1 id"); + assert.equal(view.baseTree, emptyTree); + assert.equal(view.baseCommit, "HEAD"); + assert.equal(view.committedOnly, false); + assert.notEqual(view.candidateTree, emptyTree); + assert.deepEqual(view.paths, ["staged.txt"]); + assert.deepEqual(view.deletedPaths, []); + } finally { + view.cleanup(); + } +}); + test("candidate view unborn worktree falls back when --orphan is unsupported, preserving isolation, no phantom commit, and contributor immutability", (t) => { const contributorRoot = unbornRepository(t); const indexBefore = readFileSync(join(contributorRoot, ".git", "index"));