From 3722f246bc3377693ba362e529e2178a682ea52f Mon Sep 17 00:00:00 2001 From: dbrosio3 Date: Wed, 1 Jul 2026 21:58:44 -0300 Subject: [PATCH 1/4] Add explicit review target selection --- CONTEXT.md | 16 +- README.md | 14 +- bin/pushgate.mjs | 640 +++++++++++++++- docs/README.md | 1 + ...004-centralized-changed-file-resolution.md | 16 +- .../0007-explicit-review-target-selection.md | 47 ++ docs/adr/README.md | 1 + docs/architecture/modules.md | 3 + docs/architecture/runtime-flow.md | 12 +- docs/domain/model.md | 15 +- docs/reference/changed-file-policy.md | 45 +- docs/reference/configuration.md | 8 +- src/git/config.ts | 37 + src/transcript/events.ts | 12 + src/transcript/index.ts | 3 + src/transcript/pushgate-transcript.ts | 66 ++ src/workflows/local-push-gate-run.ts | 59 +- src/workflows/pre-push-hook-context.ts | 74 +- src/workflows/pre-push.ts | 14 +- src/workflows/review-target-selection.ts | 696 ++++++++++++++++++ src/workflows/terminal.ts | 85 +++ templates/base.yml | 2 +- test/pre-push-stdin.test.ts | 54 ++ test/warning-confirmation.test.ts | 34 + test/workflow-run-plan.test.ts | 438 ++++++++++- 25 files changed, 2312 insertions(+), 80 deletions(-) create mode 100644 docs/adr/0007-explicit-review-target-selection.md create mode 100644 src/workflows/review-target-selection.ts diff --git a/CONTEXT.md b/CONTEXT.md index 94a05ba..669889e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -33,11 +33,19 @@ The repository-owned configuration that defines how Pushgate should evaluate pus _Avoid_: settings file, config blob **Target Branch**: -The branch or ref a push is reviewed against when Pushgate determines changed files. +The configured default branch or ref Pushgate starts from when selecting a review target. _Avoid_: base branch, main branch +**Review Target**: +The branch, ref, or commit selected for this push before Pushgate determines changed files. +_Avoid_: base branch, comparison branch + +**Review Target Selection**: +The explicit local choice Pushgate asks for when several review targets could be correct. +_Avoid_: automatic base switching, branch guessing + **Changed File**: -A repository file whose path or content differs from the target branch for the push being evaluated. +A repository file whose path or content differs from the selected review target for the push being evaluated. _Avoid_: staged file, touched file **Changed-File Resolution**: @@ -100,6 +108,10 @@ _Avoid_: validation rule, safety check A one-push instruction that bypasses all Pushgate work or only local AI review. _Avoid_: bypass flag, disable switch +**Review Target Override**: +A one-push instruction that selects the review target without terminal interaction. +_Avoid_: persisted target, saved branch choice + **Warning Confirmation**: The explicit developer acknowledgement required before Pushgate allows a push to continue after warning results. _Avoid_: prompt, approval, confirmation dialog diff --git a/README.md b/README.md index 051e1fe..7f1d21c 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ git push │ ▼ ┌─────────────────────────────────────┐ -│ Changed files vs target branch │ +│ Changed files vs review target │ │ (ignore_paths filtering applied) │ └──────────────┬──────────────────────┘ │ @@ -137,7 +137,7 @@ ai: # model: auto review: - target_branch: main # local ref used to resolve changed files + target_branch: main # configured default review target context_lines: 10 # surrounding context lines included in the diff max_lines_for_full_file: 300 # below this threshold, full file contents are sent # instead of just the diff for richer context @@ -228,6 +228,16 @@ git -c pushgate.skip-ai-check=true push git -c pushgate.skip-all-checks=true push ``` +When several review targets could be correct, Pushgate asks before resolving +changed files. This can happen when local `main` is behind `origin/main`, the +destination branch already exists and an incremental review is possible, or a +likely stacked remote branch is found. To select a target for one push without +using the terminal prompt: + +```bash +git -c pushgate.review-target=origin/part-1-of-feature-A push +``` + ## Runner overrides The installed hook resolves the Pushgate runner in this order: diff --git a/bin/pushgate.mjs b/bin/pushgate.mjs index d73ff47..aa1ec62 100755 --- a/bin/pushgate.mjs +++ b/bin/pushgate.mjs @@ -10013,6 +10013,30 @@ async function readGitBooleanConfig(repoRoot, key, env = process.env, options = `Could not read Git config ${key}. git config exited with ${String(result.code)}.${trimmedStderr ? ` ${trimmedStderr}` : ""}` ); } +async function readGitStringConfig(repoRoot, key, env = process.env, options = {}) { + let result; + try { + result = await runGit(repoRoot, ["config", "--get", key], { + env, + preserveGitConfigOverlay: options.preserveGitConfigOverlay + }); + } catch (error51) { + throw new GitConfigError( + `Failed to read Git config ${key}: ${errorMessage(error51)}` + ); + } + const trimmedStdout = result.stdout.trim(); + const trimmedStderr = result.stderr.trim(); + if (result.code === 0) { + return trimmedStdout; + } + if (result.code === 1 && trimmedStderr === "") { + return void 0; + } + throw new GitConfigError( + `Could not read Git config ${key}. git config exited with ${String(result.code)}.${trimmedStderr ? ` ${trimmedStderr}` : ""}` + ); +} function errorMessage(error51) { return error51 instanceof Error ? error51.message : String(error51); } @@ -10250,6 +10274,7 @@ function createPushgateTranscript(stdout) { deterministic: createDeterministicTranscript(stdout), localAi: createLocalAiTranscript(stdout), push: createPushTranscript(stdout), + reviewTarget: createReviewTargetTranscript(stdout), warningConfirmation: createWarningConfirmationTranscript(stdout) }; } @@ -10418,6 +10443,51 @@ function createLocalAiTranscript(stdout) { } }; } +function createReviewTargetTranscript(stdout) { + let sectionWritten = false; + return { + writeDiagnostics(diagnostics) { + if (diagnostics.length === 0) { + return; + } + ensureSection(); + for (const diagnostic of diagnostics) { + writeResultRow( + stdout, + diagnostic.level === "warning" ? "warning" : "info", + "Review target", + diagnostic.message + ); + if (diagnostic.tip) { + writeDetail(stdout, diagnostic.tip); + } + } + }, + writeSelected(selection) { + ensureSection(); + writeDetail(stdout, `Review target: ${selection.label}`); + writeDetail(stdout, `Review range: ${selection.reviewRange}`); + writeDetail(stdout, `Scan range: ${selection.scanRange}`); + writeLine(stdout); + }, + writeUnavailable(options) { + ensureSection(); + writeLine(stdout, options.message); + writeLine( + stdout, + "Push blocked because Review Target Selection could not be collected." + ); + writeLine(stdout); + } + }; + function ensureSection() { + if (sectionWritten) { + return; + } + writeSection(stdout, "Review target"); + sectionWritten = true; + } +} function createWarningConfirmationTranscript(stdout) { return { writeConfirmed(options) { @@ -28120,11 +28190,52 @@ var InteractiveTerminalError = class extends Error { }; function createInteractiveTerminal() { return { + choose(question, choices) { + return chooseWithInteractiveTerminal(question, choices); + }, confirm(question) { return confirmWithInteractiveTerminal(question); + }, + prompt(question) { + return promptWithInteractiveTerminal(question); } }; } +function chooseWithInteractiveTerminal(question, choices) { + if (choices.length === 0) { + throw new InteractiveTerminalError("No terminal choices were available."); + } + let terminal; + try { + terminal = openInteractiveTerminal(); + for (; ; ) { + writeSync(terminal.outputFd, `${question} +`); + for (const [index, choice] of choices.entries()) { + const detail = choice.detail ? ` - ${choice.detail}` : ""; + writeSync( + terminal.outputFd, + ` ${String(index + 1)}. ${choice.label}${detail} +` + ); + } + writeSync(terminal.outputFd, `Select 1-${String(choices.length)}: `); + const answer = readLineSync(terminal.inputFd).trim(); + const selected = Number.parseInt(answer, 10); + if (Number.isInteger(selected) && String(selected) === answer && selected >= 1 && selected <= choices.length) { + return selected - 1; + } + writeSync(terminal.outputFd, "Please enter one of the listed numbers.\n"); + } + } catch (error51) { + if (error51 instanceof InteractiveTerminalError) { + throw error51; + } + throw new InteractiveTerminalError("No interactive terminal is available."); + } finally { + terminal?.close(); + } +} function confirmWithInteractiveTerminal(question) { let terminal; try { @@ -28152,6 +28263,21 @@ function confirmWithInteractiveTerminal(question) { terminal?.close(); } } +function promptWithInteractiveTerminal(question) { + let terminal; + try { + terminal = openInteractiveTerminal(); + writeSync(terminal.outputFd, `${question} `); + return readLineSync(terminal.inputFd).trim(); + } catch (error51) { + if (error51 instanceof InteractiveTerminalError) { + throw error51; + } + throw new InteractiveTerminalError("No interactive terminal is available."); + } finally { + terminal?.close(); + } +} function formatYesNoPrompt(question) { return `${question} [y/N] `; } @@ -28266,6 +28392,430 @@ function closeFd(fd) { } } +// src/workflows/review-target-selection.ts +var REVIEW_TARGET_CONFIG_KEY = "pushgate.review-target"; +var MAX_STACKED_CANDIDATES = 3; +var ZERO_OBJECT = /^0+$/; +var ReviewTargetSelectionError = class extends Error { + constructor(message) { + super(message); + this.name = new.target.name; + } +}; +async function selectReviewTarget(options) { + const overrideRef = await readGitStringConfig( + options.repoRoot, + REVIEW_TARGET_CONFIG_KEY, + options.env, + { preserveGitConfigOverlay: true } + ); + const discovery = await discoverReviewTargets(options); + options.onDiagnostics?.(discovery.diagnostics); + if (overrideRef) { + return { + diagnostics: discovery.diagnostics, + label: overrideRef, + prompted: false, + ref: overrideRef, + source: "override" + }; + } + if (options.hookContext.branchUpdates.length > 1) { + throw new ReviewTargetSelectionError( + "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time." + ); + } + if (!discovery.promptRequired) { + const configured = discovery.candidates.find( + (candidate) => candidate.source === "configured" + ); + if (!configured) { + throw new ReviewTargetSelectionError( + "Pushgate could not prepare the configured review target." + ); + } + return { + diagnostics: discovery.diagnostics, + label: configured.label, + prompted: false, + ref: configured.ref, + source: configured.source + }; + } + const selector = options.selector ?? createTerminalReviewTargetSelector(); + const selected = await selector({ + candidates: discovery.candidates, + diagnostics: discovery.diagnostics + }); + return { + diagnostics: discovery.diagnostics, + label: selected.label, + prompted: true, + ref: selected.ref, + source: selected.source + }; +} +function createTerminalReviewTargetSelector(options = {}) { + const terminal = options.terminal ?? createInteractiveTerminal(); + return async (request) => { + if (!terminal.choose || !terminal.prompt) { + throw new ReviewTargetSelectionError(noInteractiveTerminalMessage()); + } + try { + const choices = request.candidates.map( + (candidate) => ({ + detail: candidate.detail, + label: candidate.recommended ? `${candidate.label} (recommended)` : candidate.label + }) + ); + choices.push({ + detail: "advanced", + label: "Enter another ref" + }); + const selectedIndex = terminal.choose("Choose review target", choices); + if (selectedIndex < request.candidates.length) { + const selected = request.candidates[selectedIndex]; + if (!selected) { + throw new ReviewTargetSelectionError( + "Pushgate could not read the selected review target." + ); + } + return selected; + } + const customRef = terminal.prompt("Review target ref:").trim(); + if (!customRef) { + throw new ReviewTargetSelectionError( + "Pushgate needs a non-empty review target ref." + ); + } + return { + label: customRef, + ref: customRef, + source: "custom" + }; + } catch (error51) { + if (error51 instanceof ReviewTargetSelectionError) { + throw error51; + } + if (error51 instanceof InteractiveTerminalError) { + throw new ReviewTargetSelectionError(noInteractiveTerminalMessage()); + } + throw error51; + } + }; +} +async function discoverReviewTargets(options) { + const diagnostics = []; + const configuredTarget = await candidateForRef({ + repoRoot: options.repoRoot, + detail: "configured review.target_branch", + label: options.configuredTargetRef, + ref: options.configuredTargetRef, + source: "configured" + }); + const targetRemoteRef = await resolveTargetRemoteRef({ + configuredTargetRef: options.configuredTargetRef, + pushRemote: options.hookContext.remote, + repoRoot: options.repoRoot + }); + const targetRemote = targetRemoteRef && targetRemoteRef !== options.configuredTargetRef ? await candidateForRef({ + repoRoot: options.repoRoot, + detail: "latest fetched target remote", + label: targetRemoteRef, + ref: targetRemoteRef, + source: "target-remote" + }) : null; + const resolvedTargetRemote = targetRemote?.commit ? targetRemote : null; + const freshness = configuredTarget.commit && resolvedTargetRemote?.commit ? await compareCommits( + options.repoRoot, + configuredTarget.commit, + resolvedTargetRemote.commit + ) : "missing"; + const branchUpdate = options.hookContext.branchUpdates.length === 1 ? options.hookContext.branchUpdates[0] : void 0; + const currentBranch = branchUpdate ? branchUpdate.localBranch : await resolveCurrentBranch(options.repoRoot); + const fallbackCurrentRemoteRef = !branchUpdate && currentBranch && options.hookContext.remote ? `${options.hookContext.remote}/${currentBranch}` : void 0; + const incremental = branchUpdate ? await incrementalCandidateFromPrePush(options.repoRoot, branchUpdate) ?? await incrementalCandidateFromRemoteTrackingRef({ + currentBranch, + currentRemoteRef: remoteTrackingRefForUpdate( + options.hookContext.remote, + branchUpdate + ), + repoRoot: options.repoRoot + }) : await incrementalCandidateFromRemoteTrackingRef({ + currentBranch, + currentRemoteRef: fallbackCurrentRemoteRef, + repoRoot: options.repoRoot + }); + const stacked = await findStackedCandidates({ + currentRemoteRef: branchUpdate ? remoteTrackingRefForUpdate(options.hookContext.remote, branchUpdate) : fallbackCurrentRemoteRef, + repoRoot: options.repoRoot, + targetRemoteRef: resolvedTargetRemote?.ref + }); + const promptRequired = freshness === "behind" || freshness === "diverged" || incremental !== null || stacked.length > 0; + appendFreshnessDiagnostic({ + configuredTargetRef: options.configuredTargetRef, + diagnostics, + freshness, + promptRequired, + pushRemote: options.hookContext.remote, + targetRemoteRef: resolvedTargetRemote?.ref + }); + const candidates = dedupeCandidatesByCommit([ + configuredTarget, + resolvedTargetRemote, + incremental, + ...stacked + ]).map((candidate) => ({ + ...candidate, + recommended: candidate === recommendedCandidate({ + candidates: [ + configuredTarget, + resolvedTargetRemote, + incremental, + ...stacked + ], + freshness + }) + })); + return { + candidates, + diagnostics, + promptRequired + }; +} +async function candidateForRef(options) { + return { + detail: options.detail, + label: options.label, + recommended: options.recommended, + ref: options.ref, + source: options.source, + commit: await resolveCommit(options.repoRoot, options.ref) + }; +} +async function resolveCommit(repoRoot, ref) { + const result = await runGit(repoRoot, [ + "rev-parse", + "--verify", + "--quiet", + `${ref}^{commit}` + ]); + return result.code === 0 ? result.stdout.trim() : void 0; +} +async function resolveTargetRemoteRef(options) { + const upstreamResult = await runGit(options.repoRoot, [ + "rev-parse", + "--abbrev-ref", + "--symbolic-full-name", + `${options.configuredTargetRef}@{upstream}` + ]); + if (upstreamResult.code === 0) { + const upstream = upstreamResult.stdout.trim(); + if (upstream) { + return upstream; + } + } + if (!options.pushRemote || isRemoteRefForPushRemote(options.configuredTargetRef, options.pushRemote) || !isSimpleBranchName(options.configuredTargetRef)) { + return null; + } + return `${options.pushRemote}/${options.configuredTargetRef}`; +} +async function compareCommits(repoRoot, localCommit, remoteCommit) { + if (localCommit === remoteCommit) { + return "same"; + } + const localIsAncestor = await isAncestor(repoRoot, localCommit, remoteCommit); + const remoteIsAncestor = await isAncestor(repoRoot, remoteCommit, localCommit); + if (localIsAncestor) { + return "behind"; + } + if (remoteIsAncestor) { + return "ahead"; + } + return "diverged"; +} +function appendFreshnessDiagnostic(options) { + if (!options.targetRemoteRef) { + return; + } + if (options.freshness === "behind") { + options.diagnostics.push({ + level: "warning", + message: `${options.configuredTargetRef} is behind ${options.targetRemoteRef}. Pushgate may review against stale code.`, + tip: fetchTip(options.pushRemote, options.configuredTargetRef) + }); + return; + } + if (options.freshness === "diverged") { + options.diagnostics.push({ + level: "warning", + message: `${options.configuredTargetRef} has diverged from ${options.targetRemoteRef}. Pushgate may review against stale code.`, + tip: fetchTip(options.pushRemote, options.configuredTargetRef) + }); + return; + } + if (options.freshness === "ahead" && options.promptRequired) { + options.diagnostics.push({ + level: "info", + message: `${options.configuredTargetRef} is ahead of ${options.targetRemoteRef}. Choose ${options.configuredTargetRef} only if those local commits belong in the review target.` + }); + } +} +async function incrementalCandidateFromPrePush(repoRoot, branchUpdate) { + if (isZeroObjectName(branchUpdate.remoteSha) || !isLikelyObjectName(branchUpdate.remoteSha)) { + return null; + } + const commit = await resolveCommit(repoRoot, branchUpdate.remoteSha); + if (!commit) { + return null; + } + return { + commit, + detail: `review only commits not already on ${branchUpdate.remoteRef}`, + label: `destination ${branchUpdate.remoteBranch ?? branchUpdate.localBranch} tip`, + ref: commit, + source: "incremental" + }; +} +async function incrementalCandidateFromRemoteTrackingRef(options) { + if (!options.currentBranch || !options.currentRemoteRef) { + return null; + } + const commit = await resolveCommit(options.repoRoot, options.currentRemoteRef); + if (!commit) { + return null; + } + return { + commit, + detail: `review only commits not already on ${options.currentRemoteRef}`, + label: `destination ${options.currentBranch} tip`, + ref: commit, + source: "incremental" + }; +} +async function findStackedCandidates(options) { + const result = await runGit(options.repoRoot, [ + "for-each-ref", + "--format=%(refname:short)%00%(objectname)", + "refs/remotes" + ]); + if (result.code !== 0) { + return []; + } + const candidates = []; + for (const line of result.stdout.split("\n")) { + if (!line.trim()) { + continue; + } + const [ref, commit] = line.split("\0", 2); + if (!ref || !commit || ref.endsWith("/HEAD") || ref === options.currentRemoteRef || ref === options.targetRemoteRef || isZeroObjectName(commit)) { + continue; + } + if (!await isAncestor(options.repoRoot, commit, "HEAD")) { + continue; + } + const distance = await commitDistance(options.repoRoot, commit, "HEAD"); + if (distance === null || distance === 0) { + continue; + } + candidates.push({ + commit, + detail: `${String(distance)} commit(s) behind HEAD`, + distance, + label: ref, + ref, + source: "stacked" + }); + } + return candidates.sort((left, right) => left.distance - right.distance).slice(0, MAX_STACKED_CANDIDATES).map(({ distance: _distance, ...candidate }) => candidate); +} +function recommendedCandidate(options) { + const candidates = options.candidates.filter( + (candidate) => candidate !== null + ); + return candidates.find((candidate) => candidate.source === "incremental") ?? candidates.find((candidate) => candidate.source === "stacked") ?? (options.freshness === "behind" || options.freshness === "diverged" ? candidates.find((candidate) => candidate.source === "target-remote") : void 0) ?? candidates.find((candidate) => candidate.source === "configured") ?? null; +} +function dedupeCandidatesByCommit(candidates) { + const deduped = []; + const seenCommits = /* @__PURE__ */ new Set(); + const seenRefs = /* @__PURE__ */ new Set(); + for (const candidate of candidates) { + if (!candidate) { + continue; + } + const commitKey = candidate.commit; + const refKey = candidate.ref; + if (commitKey && seenCommits.has(commitKey)) { + continue; + } + if (seenRefs.has(refKey)) { + continue; + } + if (commitKey) { + seenCommits.add(commitKey); + } + seenRefs.add(refKey); + deduped.push(candidate); + } + return deduped; +} +function remoteTrackingRefForUpdate(remote, update) { + if (!remote) { + return void 0; + } + const remoteBranch = update.remoteBranch ?? update.localBranch; + return `${remote}/${remoteBranch}`; +} +async function isAncestor(repoRoot, ancestor, descendant) { + const result = await runGit(repoRoot, [ + "merge-base", + "--is-ancestor", + ancestor, + descendant + ]); + return result.code === 0; +} +async function commitDistance(repoRoot, ancestor, descendant) { + const result = await runGit(repoRoot, [ + "rev-list", + "--count", + `${ancestor}..${descendant}` + ]); + if (result.code !== 0) { + return null; + } + const distance = Number.parseInt(result.stdout.trim(), 10); + return Number.isFinite(distance) ? distance : null; +} +async function resolveCurrentBranch(repoRoot) { + const result = await runGit(repoRoot, [ + "symbolic-ref", + "--quiet", + "--short", + "HEAD" + ]); + return result.code === 0 ? result.stdout.trim() : void 0; +} +function isSimpleBranchName(ref) { + return !ref.startsWith("refs/") && !ref.includes("..") && !ref.includes("@{") && !ref.includes(":") && !/^[0-9a-f]{40}$/i.test(ref); +} +function isRemoteRefForPushRemote(ref, pushRemote) { + return ref === pushRemote || ref.startsWith(`${pushRemote}/`) || ref.startsWith("refs/remotes/"); +} +function isZeroObjectName(value) { + return ZERO_OBJECT.test(value); +} +function isLikelyObjectName(value) { + return /^[0-9a-f]{40,64}$/i.test(value); +} +function fetchTip(remote, targetRef) { + const fetchCommand = remote ? `git fetch ${remote}` : "git fetch"; + return `Run \`${fetchCommand}\` and update \`${targetRef}\` before retrying, or choose a review target now.`; +} +function noInteractiveTerminalMessage() { + return `Pushgate needs a review target selection, but no interactive terminal is available. Re-run from a terminal, set review.target_branch explicitly, or use \`git -c ${REVIEW_TARGET_CONFIG_KEY}= push\`.`; +} + // src/workflows/warning-confirmation.ts var WarningConfirmationError = class extends Error { constructor(message) { @@ -28293,11 +28843,24 @@ function createTerminalWarningConfirmer(options = {}) { async function runLocalPushGate(options) { const transcript = createPushgateTranscript(options.stdout); const localAi = getLocalAiPhaseDecision(options.config, options.skipControls); - const changedFileResolution = await resolveChangedFilesIfRequired({ - config: options.config, - localAi, - repoRoot: options.repoRoot - }); + let changedFileResolution; + try { + changedFileResolution = await resolveChangedFilesIfRequired({ + config: options.config, + env: options.env, + hookContext: options.hookContext, + localAi, + repoRoot: options.repoRoot, + reviewTargetSelector: options.reviewTargetSelector, + transcript + }); + } catch (error51) { + if (error51 instanceof ReviewTargetSelectionError) { + transcript.reviewTarget.writeUnavailable({ message: error51.message }); + return 1; + } + throw error51; + } const deterministicSummary = await runDeterministicChecks({ changedFileResolution, config: options.config, @@ -28345,11 +28908,27 @@ async function resolveChangedFilesIfRequired(options) { if (!deterministicPlan.needsChangedFileResolution && options.localAi.kind !== "run") { return null; } - return await resolveChangedFiles({ + const selectedReviewTarget = await selectReviewTarget({ + configuredTargetRef: options.config.review.target_branch, + env: options.env, + hookContext: options.hookContext, + onDiagnostics(diagnostics) { + options.transcript.reviewTarget.writeDiagnostics(diagnostics); + }, repoRoot: options.repoRoot, - targetBranch: options.config.review.target_branch, + selector: options.reviewTargetSelector + }); + const resolution = await resolveChangedFiles({ + repoRoot: options.repoRoot, + targetBranch: selectedReviewTarget.ref, ignorePaths: options.config.ignore_paths }); + options.transcript.reviewTarget.writeSelected({ + label: selectedReviewTarget.label, + reviewRange: resolution.reviewRange, + scanRange: resolution.scanRange + }); + return resolution; } async function runLocalAiPhase(options) { if (options.decision.kind === "skip") { @@ -28427,44 +29006,53 @@ function requireChangedFileResolution2(changedFileResolution, phaseName) { // src/workflows/pre-push-hook-context.ts function buildPrePushContext(options) { + const branchUpdates = options.input?.branchUpdates ?? []; return { - branch: options.branch, + branch: options.branch ?? branchUpdates[0]?.localBranch, + branchUpdates, remote: options.args[0] }; } var MAX_PRE_PUSH_STDIN_LINE_CHARS = 8 * 1024; -function parseBranchFromPrePushLine(line) { +function parsePrePushLine(line) { const trimmed = line.trim(); if (!trimmed) { - return void 0; + return null; } - const [localRef] = trimmed.split(/\s+/, 1); - if (localRef?.startsWith("refs/heads/")) { - return localRef.slice("refs/heads/".length); + const [localRef, localSha, remoteRef, remoteSha] = trimmed.split(/\s+/, 4); + if (!localRef?.startsWith("refs/heads/") || !localSha || !remoteRef || !remoteSha) { + return null; } - return void 0; + return { + localBranch: localRef.slice("refs/heads/".length), + localRef, + localSha, + remoteBranch: remoteRef.startsWith("refs/heads/") ? remoteRef.slice("refs/heads/".length) : void 0, + remoteRef, + remoteSha + }; } -function readPrePushBranchFromStdin(stdin) { +function readPrePushInputFromStdin(stdin) { return new Promise((resolve, reject) => { if (stdin.isTTY) { - resolve(void 0); + resolve({ branchUpdates: [] }); return; } - let branch; + const branchUpdates = []; let line = ""; let lineOverflowed = false; const parseLine = () => { - if (branch !== void 0 || lineOverflowed) { + if (lineOverflowed) { return; } - branch = parseBranchFromPrePushLine(line); + const update = parsePrePushLine(line); + if (update) { + branchUpdates.push(update); + } }; stdin.setEncoding("utf8"); stdin.on("error", reject); stdin.on("data", (chunk) => { - if (branch !== void 0) { - return; - } for (const character of chunk) { if (character === "\n") { if (line.endsWith("\r")) { @@ -28488,7 +29076,7 @@ function readPrePushBranchFromStdin(stdin) { }); stdin.on("end", () => { parseLine(); - resolve(branch); + resolve({ branchUpdates }); }); stdin.resume(); }); @@ -28496,9 +29084,11 @@ function readPrePushBranchFromStdin(stdin) { // src/workflows/pre-push.ts async function runPrePushWorkflow(io) { + const prePushInput = await readPrePushInputFromStdin(io.stdin); const hookContext = buildPrePushContext({ args: io.hookArgs ?? [], - branch: await readPrePushBranchFromStdin(io.stdin) + branch: void 0, + input: prePushInput }); const repoRoot = await resolveGitRepositoryRoot(io.env); writePrePushHeader(io.stdout, repoRoot, hookContext); @@ -28515,7 +29105,9 @@ async function runPrePushWorkflow(io) { return await runLocalPushGate({ config: loaded.config, env: io.env, + hookContext, repoRoot, + ...io.reviewTargetSelector ? { reviewTargetSelector: io.reviewTargetSelector } : {}, stdout: io.stdout, skipControls, ...io.warningConfirmer ? { warningConfirmer: io.warningConfirmer } : {} diff --git a/docs/README.md b/docs/README.md index 1ed4ec4..48dd8bc 100644 --- a/docs/README.md +++ b/docs/README.md @@ -32,6 +32,7 @@ Architecture decisions live in [ADR](./adr/): - [0004 - Centralized Changed-File Resolution](./adr/0004-centralized-changed-file-resolution.md) - [0005 - Provider-Neutral Local AI Review Contract](./adr/0005-provider-neutral-local-ai-review-contract.md) - [0006 - Checked-In Generated Runner](./adr/0006-checked-in-generated-runner.md) +- [0007 - Explicit Review Target Selection](./adr/0007-explicit-review-target-selection.md) ## Reference diff --git a/docs/adr/0004-centralized-changed-file-resolution.md b/docs/adr/0004-centralized-changed-file-resolution.md index fe58394..457db13 100644 --- a/docs/adr/0004-centralized-changed-file-resolution.md +++ b/docs/adr/0004-centralized-changed-file-resolution.md @@ -1,9 +1,10 @@ # Centralized Changed-File Resolution -Pushgate resolves changed files once from the configured local target ref and -shares that normalized result with deterministic checks and local AI. The path -policy module owns target-ref resolution, merge-base selection, diff parsing, -ignore filtering, and named review and scan ranges. +Pushgate resolves changed files once from the selected review target and shares +that normalized result with deterministic checks and local AI. The path policy +module owns target-ref resolution, merge-base selection, diff parsing, ignore +filtering, and named review and scan ranges. ADR 0007 describes how the +workflow chooses that review target before this resolver runs. ## Considered Options @@ -12,6 +13,7 @@ ignore filtering, and named review and scan ranges. ## Consequences -Pushgate fails explicitly when the target ref or merge base is unavailable. It -does not fetch, guess a remote, or silently switch ranges. Consumers use -`reviewRange` and `scanRange` instead of rebuilding Git syntax themselves. +Pushgate fails explicitly when the selected review target ref or merge base is +unavailable. It does not fetch, guess a remote, or silently switch ranges. +Consumers use `reviewRange` and `scanRange` instead of rebuilding Git syntax +themselves. diff --git a/docs/adr/0007-explicit-review-target-selection.md b/docs/adr/0007-explicit-review-target-selection.md new file mode 100644 index 0000000..b077ac8 --- /dev/null +++ b/docs/adr/0007-explicit-review-target-selection.md @@ -0,0 +1,47 @@ +# Explicit Review Target Selection + +Pushgate selects one review target before changed-file resolution when local Git +state makes more than one target plausible. + +## Context + +The original resolver compared `HEAD` with the configured local +`review.target_branch`, usually `main`. That is simple, but it can review the +wrong range when local `main` is stale, when the destination branch already +exists and the developer wants only the incremental push reviewed, or when a +branch is stacked on top of another feature branch. + +Silently switching from the configured target to a remote-tracking ref or +incremental base would make the transcript harder to trust. + +## Decision + +Pushgate keeps `review.target_branch` as the configured default and introduces +per-push review target selection. + +When changed-file resolution is needed, Pushgate diagnoses locally available Git +state without fetching. It prompts only when there is ambiguity: + +- the configured target is behind or diverged from its fetched remote target +- the destination branch already has a remote object id +- likely stacked remote ancestor branches exist + +The selected review target is used for all changed-file consumers: deterministic +checks, plugins, built-in policies, and local AI review. The transcript prints +the chosen review target plus the resulting review and scan ranges. + +`pushgate.review-target=` is a one-push override that skips terminal +selection but does not persist. + +## Consequences + +Pushgate does not fetch or mutate refs during a pre-push hook run. Stale-target +diagnostics tell the developer to run `git fetch` and update the local target +before retrying. + +Non-interactive ambiguous pushes fail closed with override guidance instead of +guessing a review target. + +Stacked PR support is heuristic: Pushgate shows the closest remote branches +whose tips are ancestors of `HEAD`, excluding the current branch remote and the +target remote. diff --git a/docs/adr/README.md b/docs/adr/README.md index 3c0e711..58fe79b 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -9,3 +9,4 @@ and the result of a real trade-off. - [0004 - Centralized Changed-File Resolution](./0004-centralized-changed-file-resolution.md) - [0005 - Provider-Neutral Local AI Review Contract](./0005-provider-neutral-local-ai-review-contract.md) - [0006 - Checked-In Generated Runner](./0006-checked-in-generated-runner.md) +- [0007 - Explicit Review Target Selection](./0007-explicit-review-target-selection.md) diff --git a/docs/architecture/modules.md b/docs/architecture/modules.md index c5cbc2d..b195c85 100644 --- a/docs/architecture/modules.md +++ b/docs/architecture/modules.md @@ -12,6 +12,7 @@ know. | Installer | `install.sh [--template name]` | `install.sh`, `templates/*.yml` | Owns runner placement, hook backup, template install, and validation. | | CLI | `main(argv, io)` and `pushgate` subcommands | `src/cli.ts` | Public command surface for hook use. | | Pre-push workflow | `runPrePushWorkflow(io)` | `src/workflows/pre-push.ts`, `src/workflows/local-push-gate-run.ts`, `src/workflows/pre-push-hook-context.ts` | Owns pre-push context, phase order, and warning confirmation. | +| Review target selection | `selectReviewTarget` | `src/workflows/review-target-selection.ts`, `src/workflows/terminal.ts` | Chooses one per-push review target from local Git state before changed-file resolution. | | Config | `loadConfig`, `parseConfigYaml`, `PushgateConfig` | `src/config/*`, `schemas/pushgate-config-v2.schema.json` | Converts user YAML into one normalized internal shape. | | Path policy | `resolveChangedFiles`, changed-file projections | `src/path-policy/*`, `src/git/*` | Owns Git range, diff parsing, ignore, live-path semantics, and changed-line metrics. | | Process execution | `runCommand`, `runTimedCommand`, `runProcessOutcome`, `runInheritedCommand` | `src/process/*` | Shared child-process mechanics and outcome formatting. | @@ -27,6 +28,7 @@ know. |---|---|---|---| | `PushgateConfig` | Config module | Workflow, runner, AI, path policy | Defaults are normalized; active AI modes require a selected provider block. | | `ChangedFileResolution` | Path policy | Deterministic runner, local AI | Contains target ref, target commit, merge base, filtered files, review range, and scan range. | +| `SelectedReviewTarget` | Review target selection | Workflow, path policy | Contains the per-push review target ref, display label, source, and diagnostics. | | `ChangedFile` and projections | Path policy | Policies, tools, AI payload builder | Includes status, optional previous path, binary marker, additions, deletions, live-path selection, and changed text-line counts. | | `ToolResult` | Deterministic runner | Transcript and summary | Status is `passed`, `skipped`, `warning`, or `blocked`. | | `LocalAiReviewPayload` | AI review context | Provider adapters | Contains changed files, rendered diff, optional full-file context, and final prompt. | @@ -62,3 +64,4 @@ helper. | Process outcome behavior | `test/process.test.ts` | | Local AI prompt context, guardrails, provider adapters, output repair, verdicts | `test/ai.test.ts` | | Transcript rendering for Local Push Gate output | `test/transcript.test.ts` | +| Review target selection, stale target warnings, incremental and stacked targets | `test/workflow-run-plan.test.ts`, `test/pre-push-stdin.test.ts` | diff --git a/docs/architecture/runtime-flow.md b/docs/architecture/runtime-flow.md index 42a5f1f..a5d214c 100644 --- a/docs/architecture/runtime-flow.md +++ b/docs/architecture/runtime-flow.md @@ -60,7 +60,8 @@ flowchart TD ConfigDecision -->|yes| Done0["exit 0"] ConfigDecision -->|no| Config["loadConfig"] Config --> Changed{"changed files required?"} - Changed -->|yes| Path["resolveChangedFiles"] + Changed -->|yes| Target["selectReviewTarget"] + Target --> Path["resolveChangedFiles"] Changed -->|no| NoPath["changedFileResolution = null"] Path --> Det["runDeterministicChecks"] NoPath --> Det @@ -75,9 +76,12 @@ flowchart TD Confirm2 --> DoneAI["return final exit code"] ``` -Changed files are resolved once and shared between deterministic checks and -local AI. Deleted files remain in the normalized changed-file result for diff -and AI context, but configured tools receive only live current paths. +When changed files are required, Pushgate first selects one review target. It +prompts only for ambiguous local Git state such as stale configured targets, +existing destination branches, or likely stacked bases. Changed files are then +resolved once and shared between deterministic checks and local AI. Deleted +files remain in the normalized changed-file result for diff and AI context, but +configured tools receive only live current paths. ## Deterministic Phase diff --git a/docs/domain/model.md b/docs/domain/model.md index 6b5dd5a..fbb01d5 100644 --- a/docs/domain/model.md +++ b/docs/domain/model.md @@ -14,10 +14,12 @@ the product treats that as a Git reality rather than pretending otherwise. `--no-verify`. 3. The hook delegates to the Pushgate runner. 4. Pushgate loads repository config and evaluates the push locally. -5. Deterministic checks run before local AI review. -6. Blocking results stop the push. Warning results require explicit terminal +5. If changed-file resolution is needed, Pushgate selects one review target for + this push. +6. Deterministic checks run before local AI review. +7. Blocking results stop the push. Warning results require explicit terminal confirmation before the push continues. -7. The transcript explains what ran, what was skipped, and why the push passed +8. The transcript explains what ran, what was skipped, and why the push passed or failed. ## Core Relationships @@ -28,6 +30,7 @@ the product treats that as a Git reality rather than pretending otherwise. | Pre-Push Hook | The Git entry point that delegates to the runner. | | Pushgate Runner | The executable that owns config loading, phase order, and local verdicts. | | Pushgate Config | Repository-owned policy for deterministic checks, local AI, path filtering, and provider settings. | +| Review Target Selection | The per-push choice of target ref or commit when the configured target, remote target, incremental push base, or stacked base could all be valid. | | Changed-File Resolution | One normalized view of changed files and Git ranges shared by deterministic checks and local AI. | | Deterministic Check | A local check whose result does not depend on a language model. | | Local AI Review | A provider-backed review phase that runs only after deterministic checks pass. | @@ -53,7 +56,9 @@ docs, issues, and code comments. runtime control. - `pushgate.skip-all-checks` skips all local Pushgate work for one push. - `pushgate.skip-ai-check` skips only local AI review for one push. +- `pushgate.review-target=` selects the review target for one push without + persisting that choice. - `.pushgate.yml` is the public config vocabulary. `.push-review.yml` is legacy migration input, not an alternate runtime format. -- Pushgate resolves changed files from locally available Git state. It does not - fetch, guess a remote, or silently choose a fallback range. +- Pushgate resolves review targets and changed files from locally available Git + state. It does not fetch or silently switch ranges. diff --git a/docs/reference/changed-file-policy.md b/docs/reference/changed-file-policy.md index 8bda8ac..2a92556 100644 --- a/docs/reference/changed-file-policy.md +++ b/docs/reference/changed-file-policy.md @@ -5,22 +5,51 @@ into the file and range facts Pushgate phases consume. ## Resolution Contract -`resolveChangedFiles` receives a repository root, `review.target_branch`, and -`ignore_paths`. It returns one `ChangedFileResolution`: +Before `resolveChangedFiles` runs, the workflow selects one review target for +the push. In the simple case that target is `review.target_branch`. When local +state is ambiguous, Pushgate asks the developer to choose from the configured +target, the fetched remote target, the destination branch tip for incremental +push review, likely stacked remote ancestors, or an advanced custom ref. + +`resolveChangedFiles` receives a repository root, the selected review target, +and `ignore_paths`. It returns one `ChangedFileResolution`: | Field | Meaning | |---|---| -| `targetRef` | Configured target branch or ref. | -| `targetCommit` | Commit selected by the configured target ref at resolution time. | +| `targetRef` | Selected review target branch, ref, or commit. | +| `targetCommit` | Commit selected by the review target at resolution time. | | `diffBase` | Merge base selected by the `...HEAD` diff contract. | | `files` | Globally filtered changed files for deterministic and AI consumers. | | `reviewRange` | Git range used to prepare human-readable local AI review context. | | `scanRange` | Git range used by deterministic scanners that inspect pushed commits. | -The target ref must already exist locally. Pushgate fails with an explicit -diagnostic if the ref is missing or there is no usable merge base with `HEAD`. -It does not fetch, guess a remote variant, or switch to a different history -range. +The review target must already exist in local Git state. Pushgate fails with an +explicit diagnostic if the ref is missing or there is no usable merge base with +`HEAD`. It does not fetch or silently switch to a different history range. + +## Review Target Selection + +Pushgate compares the configured target branch with its remote counterpart when +both refs exist locally. It maps `main` to `main@{upstream}` first, then falls +back to `/main`. It warns when the configured target is behind or +diverged from the fetched remote-tracking target and suggests running +`git fetch`. + +Pushgate prompts for a review target only when changed-file resolution is needed +and one of these ambiguities exists: + +- the configured target is behind or diverged from its fetched remote target +- the destination branch already exists, so incremental review is possible +- likely stacked bases exist among remote branches whose tips are ancestors of + `HEAD` + +For incremental review, pre-push stdin's remote object id is preferred over a +remote-tracking ref. If stdin is unavailable or incomplete, Pushgate falls back +to `/` when that ref exists locally. + +`git -c pushgate.review-target= push` selects a review target for one push +and skips the terminal selection prompt. Diagnostics still print when Pushgate +can determine that the configured target is stale. ## Range Semantics diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index de615ca..87e5f41 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -108,10 +108,16 @@ extension point for provider-specific nested settings. | Field | Meaning | |---|---| -| `target_branch` | Local or remote-tracking branch/ref used as the changed-file target. | +| `target_branch` | Configured default branch/ref used when selecting the per-push review target. | | `context_lines` | Surrounding diff lines included when local AI review context is prepared. | | `max_lines_for_full_file` | Diff-size cutoff below which local AI may include full file context. | +When Pushgate detects that the configured target is stale, the destination +branch already exists, or a likely stacked remote ancestor exists, it prompts +for a review target before changed-file resolution. Use +`git -c pushgate.review-target= push` to select a target for one push +without persisting that choice. + ## Tool Commands Tool commands are argv arrays, not shell strings. `{changed_files}` may be one diff --git a/src/git/config.ts b/src/git/config.ts index 0fcb0d2..d4152ac 100644 --- a/src/git/config.ts +++ b/src/git/config.ts @@ -54,6 +54,43 @@ export async function readGitBooleanConfig( ); } +export async function readGitStringConfig( + repoRoot: string, + key: string, + env: NodeJS.ProcessEnv = process.env, + options: { + preserveGitConfigOverlay?: boolean; + } = {}, +): Promise { + let result: Awaited>; + + try { + result = await runGit(repoRoot, ["config", "--get", key], { + env, + preserveGitConfigOverlay: options.preserveGitConfigOverlay, + }); + } catch (error) { + throw new GitConfigError( + `Failed to read Git config ${key}: ${errorMessage(error)}`, + ); + } + + const trimmedStdout = result.stdout.trim(); + const trimmedStderr = result.stderr.trim(); + + if (result.code === 0) { + return trimmedStdout; + } + + if (result.code === 1 && trimmedStderr === "") { + return undefined; + } + + throw new GitConfigError( + `Could not read Git config ${key}. git config exited with ${String(result.code)}.${trimmedStderr ? ` ${trimmedStderr}` : ""}`, + ); +} + function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } diff --git a/src/transcript/events.ts b/src/transcript/events.ts index cd24e1f..0dbfef5 100644 --- a/src/transcript/events.ts +++ b/src/transcript/events.ts @@ -111,6 +111,18 @@ export type LocalAiTranscriptEvent = kind: "review-blocked"; }; +export interface ReviewTargetTranscriptDiagnostic { + level: "info" | "warning"; + message: string; + tip?: string; +} + +export interface ReviewTargetTranscriptSelection { + label: string; + reviewRange: string; + scanRange: string; +} + export type WarningConfirmationPhase = | "deterministic checks" | "local AI review"; diff --git a/src/transcript/index.ts b/src/transcript/index.ts index 0fcbb23..fad4e37 100644 --- a/src/transcript/index.ts +++ b/src/transcript/index.ts @@ -6,6 +6,7 @@ export { type LocalAiTranscript, type PushgateTranscript, type PushTranscript, + type ReviewTargetTranscript, type WarningConfirmationTranscript, } from "./pushgate-transcript.js"; @@ -16,5 +17,7 @@ export type { DeterministicTranscriptSummary, LocalAiSkipReason, LocalAiTranscriptEvent, + ReviewTargetTranscriptDiagnostic, + ReviewTargetTranscriptSelection, WarningConfirmationPhase, } from "./events.js"; diff --git a/src/transcript/pushgate-transcript.ts b/src/transcript/pushgate-transcript.ts index 5e5dd16..e1dd15e 100644 --- a/src/transcript/pushgate-transcript.ts +++ b/src/transcript/pushgate-transcript.ts @@ -17,6 +17,8 @@ import type { DeterministicTranscriptSummary, LocalAiSkipReason, LocalAiTranscriptEvent, + ReviewTargetTranscriptDiagnostic, + ReviewTargetTranscriptSelection, WarningConfirmationPhase, } from "./events.js"; @@ -34,6 +36,12 @@ export interface LocalAiTranscript { writeSkipped(options: { reason: LocalAiSkipReason }): void; } +export interface ReviewTargetTranscript { + writeDiagnostics(diagnostics: readonly ReviewTargetTranscriptDiagnostic[]): void; + writeSelected(selection: ReviewTargetTranscriptSelection): void; + writeUnavailable(options: { message: string }): void; +} + export interface WarningConfirmationTranscript { writeConfirmed(options: { phase: WarningConfirmationPhase; @@ -54,6 +62,7 @@ export interface PushgateTranscript { deterministic: DeterministicTranscript; localAi: LocalAiTranscript; push: PushTranscript; + reviewTarget: ReviewTargetTranscript; warningConfirmation: WarningConfirmationTranscript; } @@ -64,6 +73,7 @@ export function createPushgateTranscript( deterministic: createDeterministicTranscript(stdout), localAi: createLocalAiTranscript(stdout), push: createPushTranscript(stdout), + reviewTarget: createReviewTargetTranscript(stdout), warningConfirmation: createWarningConfirmationTranscript(stdout), }; } @@ -299,6 +309,62 @@ interface LocalAiStreamingTranscriptState { waitSpinnerTimer?: NodeJS.Timeout; } +function createReviewTargetTranscript( + stdout: NodeJS.WritableStream, +): ReviewTargetTranscript { + let sectionWritten = false; + + return { + writeDiagnostics(diagnostics) { + if (diagnostics.length === 0) { + return; + } + + ensureSection(); + + for (const diagnostic of diagnostics) { + writeResultRow( + stdout, + diagnostic.level === "warning" ? "warning" : "info", + "Review target", + diagnostic.message, + ); + + if (diagnostic.tip) { + writeDetail(stdout, diagnostic.tip); + } + } + }, + + writeSelected(selection) { + ensureSection(); + writeDetail(stdout, `Review target: ${selection.label}`); + writeDetail(stdout, `Review range: ${selection.reviewRange}`); + writeDetail(stdout, `Scan range: ${selection.scanRange}`); + writeLine(stdout); + }, + + writeUnavailable(options) { + ensureSection(); + writeLine(stdout, options.message); + writeLine( + stdout, + "Push blocked because Review Target Selection could not be collected.", + ); + writeLine(stdout); + }, + }; + + function ensureSection(): void { + if (sectionWritten) { + return; + } + + writeSection(stdout, "Review target"); + sectionWritten = true; + } +} + function createWarningConfirmationTranscript( stdout: NodeJS.WritableStream, ): WarningConfirmationTranscript { diff --git a/src/workflows/local-push-gate-run.ts b/src/workflows/local-push-gate-run.ts index 6fc9e70..9e3d82b 100644 --- a/src/workflows/local-push-gate-run.ts +++ b/src/workflows/local-push-gate-run.ts @@ -15,6 +15,12 @@ import { type WarningConfirmationTranscript, } from "../transcript/index.js"; import type { SkipControlState } from "../skip-controls.js"; +import type { PrePushHookContext } from "./pre-push-hook-context.js"; +import { + ReviewTargetSelectionError, + selectReviewTarget, + type ReviewTargetSelector, +} from "./review-target-selection.js"; import { createTerminalWarningConfirmer, WarningConfirmationError, @@ -24,7 +30,9 @@ import { export interface LocalPushGateRunOptions { config: PushgateConfig; env: NodeJS.ProcessEnv; + hookContext: PrePushHookContext; repoRoot: string; + reviewTargetSelector?: ReviewTargetSelector; skipControls: Pick; stdout: NodeJS.WritableStream; warningConfirmer?: WarningConfirmer; @@ -50,11 +58,26 @@ export async function runLocalPushGate( ): Promise { const transcript = createPushgateTranscript(options.stdout); const localAi = getLocalAiPhaseDecision(options.config, options.skipControls); - const changedFileResolution = await resolveChangedFilesIfRequired({ - config: options.config, - localAi, - repoRoot: options.repoRoot, - }); + let changedFileResolution: ChangedFileResolution | null; + + try { + changedFileResolution = await resolveChangedFilesIfRequired({ + config: options.config, + env: options.env, + hookContext: options.hookContext, + localAi, + repoRoot: options.repoRoot, + reviewTargetSelector: options.reviewTargetSelector, + transcript, + }); + } catch (error) { + if (error instanceof ReviewTargetSelectionError) { + transcript.reviewTarget.writeUnavailable({ message: error.message }); + return 1; + } + + throw error; + } const deterministicSummary = await runDeterministicChecks({ changedFileResolution, @@ -111,8 +134,12 @@ export async function runLocalPushGate( async function resolveChangedFilesIfRequired(options: { config: PushgateConfig; + env: NodeJS.ProcessEnv; + hookContext: PrePushHookContext; localAi: LocalAiPhaseDecision; repoRoot: string; + reviewTargetSelector: ReviewTargetSelector | undefined; + transcript: ReturnType; }): Promise { const deterministicPlan = buildDeterministicCheckPlan(options.config); @@ -123,11 +150,29 @@ async function resolveChangedFilesIfRequired(options: { return null; } - return await resolveChangedFiles({ + const selectedReviewTarget = await selectReviewTarget({ + configuredTargetRef: options.config.review.target_branch, + env: options.env, + hookContext: options.hookContext, + onDiagnostics(diagnostics) { + options.transcript.reviewTarget.writeDiagnostics(diagnostics); + }, repoRoot: options.repoRoot, - targetBranch: options.config.review.target_branch, + selector: options.reviewTargetSelector, + }); + const resolution = await resolveChangedFiles({ + repoRoot: options.repoRoot, + targetBranch: selectedReviewTarget.ref, ignorePaths: options.config.ignore_paths, }); + + options.transcript.reviewTarget.writeSelected({ + label: selectedReviewTarget.label, + reviewRange: resolution.reviewRange, + scanRange: resolution.scanRange, + }); + + return resolution; } async function runLocalAiPhase(options: { diff --git a/src/workflows/pre-push-hook-context.ts b/src/workflows/pre-push-hook-context.ts index 0aa08bb..55a180d 100644 --- a/src/workflows/pre-push-hook-context.ts +++ b/src/workflows/pre-push-hook-context.ts @@ -1,14 +1,32 @@ export interface PrePushHookContext { branch?: string; + branchUpdates: PrePushBranchUpdate[]; remote?: string; } +export interface PrePushBranchUpdate { + localBranch: string; + localRef: string; + localSha: string; + remoteBranch?: string; + remoteRef: string; + remoteSha: string; +} + +export interface PrePushInput { + branchUpdates: PrePushBranchUpdate[]; +} + export function buildPrePushContext(options: { args: readonly string[]; branch: string | undefined; + input?: PrePushInput; }): PrePushHookContext { + const branchUpdates = options.input?.branchUpdates ?? []; + return { - branch: options.branch, + branch: options.branch ?? branchUpdates[0]?.localBranch, + branchUpdates, remote: options.args[0], }; } @@ -18,19 +36,37 @@ const MAX_PRE_PUSH_STDIN_LINE_CHARS = 8 * 1024; export function parseBranchFromPrePushLine( line: string, ): string | undefined { + return parsePrePushLine(line)?.localBranch; +} + +export function parsePrePushLine(line: string): PrePushBranchUpdate | null { const trimmed = line.trim(); if (!trimmed) { - return undefined; + return null; } - const [localRef] = trimmed.split(/\s+/, 1); + const [localRef, localSha, remoteRef, remoteSha] = trimmed.split(/\s+/, 4); - if (localRef?.startsWith("refs/heads/")) { - return localRef.slice("refs/heads/".length); + if ( + !localRef?.startsWith("refs/heads/") || + !localSha || + !remoteRef || + !remoteSha + ) { + return null; } - return undefined; + return { + localBranch: localRef.slice("refs/heads/".length), + localRef, + localSha, + remoteBranch: remoteRef.startsWith("refs/heads/") + ? remoteRef.slice("refs/heads/".length) + : undefined, + remoteRef, + remoteSha, + }; } /** @@ -42,31 +78,39 @@ export function parseBranchFromPrePushLine( export function readPrePushBranchFromStdin( stdin: NodeJS.ReadableStream, ): Promise { + return readPrePushInputFromStdin(stdin).then( + (input) => input.branchUpdates[0]?.localBranch, + ); +} + +export function readPrePushInputFromStdin( + stdin: NodeJS.ReadableStream, +): Promise { return new Promise((resolve, reject) => { if ((stdin as { isTTY?: boolean }).isTTY) { - resolve(undefined); + resolve({ branchUpdates: [] }); return; } - let branch: string | undefined; + const branchUpdates: PrePushBranchUpdate[] = []; let line = ""; let lineOverflowed = false; const parseLine = () => { - if (branch !== undefined || lineOverflowed) { + if (lineOverflowed) { return; } - branch = parseBranchFromPrePushLine(line); + const update = parsePrePushLine(line); + + if (update) { + branchUpdates.push(update); + } }; stdin.setEncoding("utf8"); stdin.on("error", reject); stdin.on("data", (chunk: string) => { - if (branch !== undefined) { - return; - } - for (const character of chunk) { if (character === "\n") { if (line.endsWith("\r")) { @@ -94,7 +138,7 @@ export function readPrePushBranchFromStdin( }); stdin.on("end", () => { parseLine(); - resolve(branch); + resolve({ branchUpdates }); }); stdin.resume(); }); diff --git a/src/workflows/pre-push.ts b/src/workflows/pre-push.ts index 275f3c6..10a8d97 100644 --- a/src/workflows/pre-push.ts +++ b/src/workflows/pre-push.ts @@ -14,19 +14,23 @@ import { PUSHGATE_VERSION } from "../version.js"; import { runLocalPushGate } from "./local-push-gate-run.js"; import { buildPrePushContext, - readPrePushBranchFromStdin, + readPrePushInputFromStdin, type PrePushHookContext, } from "./pre-push-hook-context.js"; +import type { ReviewTargetSelector } from "./review-target-selection.js"; import type { WarningConfirmer } from "./warning-confirmation.js"; export { + parsePrePushLine, parseBranchFromPrePushLine, + readPrePushInputFromStdin, readPrePushBranchFromStdin, } from "./pre-push-hook-context.js"; export interface PrePushWorkflowIO { env: NodeJS.ProcessEnv; hookArgs?: readonly string[]; + reviewTargetSelector?: ReviewTargetSelector; stderr: NodeJS.WritableStream; stdin: NodeJS.ReadableStream; stdout: NodeJS.WritableStream; @@ -36,9 +40,11 @@ export interface PrePushWorkflowIO { export async function runPrePushWorkflow( io: PrePushWorkflowIO, ): Promise { + const prePushInput = await readPrePushInputFromStdin(io.stdin); const hookContext = buildPrePushContext({ args: io.hookArgs ?? [], - branch: await readPrePushBranchFromStdin(io.stdin), + branch: undefined, + input: prePushInput, }); const repoRoot = await resolveGitRepositoryRoot(io.env); @@ -60,7 +66,11 @@ export async function runPrePushWorkflow( return await runLocalPushGate({ config: loaded.config, env: io.env, + hookContext, repoRoot, + ...(io.reviewTargetSelector + ? { reviewTargetSelector: io.reviewTargetSelector } + : {}), stdout: io.stdout, skipControls, ...(io.warningConfirmer diff --git a/src/workflows/review-target-selection.ts b/src/workflows/review-target-selection.ts new file mode 100644 index 0000000..23752d4 --- /dev/null +++ b/src/workflows/review-target-selection.ts @@ -0,0 +1,696 @@ +import { readGitStringConfig } from "../git/config.js"; +import { runGit } from "../git/command.js"; +import type { + PrePushBranchUpdate, + PrePushHookContext, +} from "./pre-push-hook-context.js"; +import { + createInteractiveTerminal, + InteractiveTerminalError, + type InteractiveTerminal, + type InteractiveTerminalChoice, +} from "./terminal.js"; + +export const REVIEW_TARGET_CONFIG_KEY = "pushgate.review-target" as const; + +export type ReviewTargetCandidateSource = + | "configured" + | "custom" + | "incremental" + | "stacked" + | "target-remote"; + +export interface ReviewTargetCandidate { + detail?: string; + label: string; + ref: string; + recommended?: boolean; + source: ReviewTargetCandidateSource; +} + +export interface ReviewTargetDiagnostic { + level: "info" | "warning"; + message: string; + tip?: string; +} + +export interface ReviewTargetSelectionPrompt { + candidates: readonly ReviewTargetCandidate[]; + diagnostics: readonly ReviewTargetDiagnostic[]; +} + +export type ReviewTargetSelector = ( + request: ReviewTargetSelectionPrompt, +) => Promise; + +export interface SelectedReviewTarget { + diagnostics: readonly ReviewTargetDiagnostic[]; + label: string; + prompted: boolean; + ref: string; + source: ReviewTargetCandidateSource | "override"; +} + +export interface SelectReviewTargetOptions { + configuredTargetRef: string; + env: NodeJS.ProcessEnv; + hookContext: PrePushHookContext; + onDiagnostics?: (diagnostics: readonly ReviewTargetDiagnostic[]) => void; + repoRoot: string; + selector?: ReviewTargetSelector; +} + +interface CandidateWithCommit extends ReviewTargetCandidate { + commit?: string; +} + +type TargetFreshness = "ahead" | "behind" | "diverged" | "missing" | "same"; + +const MAX_STACKED_CANDIDATES = 3; +const ZERO_OBJECT = /^0+$/; + +export class ReviewTargetSelectionError extends Error { + constructor(message: string) { + super(message); + this.name = new.target.name; + } +} + +export async function selectReviewTarget( + options: SelectReviewTargetOptions, +): Promise { + const overrideRef = await readGitStringConfig( + options.repoRoot, + REVIEW_TARGET_CONFIG_KEY, + options.env, + { preserveGitConfigOverlay: true }, + ); + const discovery = await discoverReviewTargets(options); + + options.onDiagnostics?.(discovery.diagnostics); + + if (overrideRef) { + return { + diagnostics: discovery.diagnostics, + label: overrideRef, + prompted: false, + ref: overrideRef, + source: "override", + }; + } + + if (options.hookContext.branchUpdates.length > 1) { + throw new ReviewTargetSelectionError( + "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time.", + ); + } + + if (!discovery.promptRequired) { + const configured = discovery.candidates.find( + (candidate) => candidate.source === "configured", + ); + + if (!configured) { + throw new ReviewTargetSelectionError( + "Pushgate could not prepare the configured review target.", + ); + } + + return { + diagnostics: discovery.diagnostics, + label: configured.label, + prompted: false, + ref: configured.ref, + source: configured.source, + }; + } + + const selector = options.selector ?? createTerminalReviewTargetSelector(); + const selected = await selector({ + candidates: discovery.candidates, + diagnostics: discovery.diagnostics, + }); + + return { + diagnostics: discovery.diagnostics, + label: selected.label, + prompted: true, + ref: selected.ref, + source: selected.source, + }; +} + +export function createTerminalReviewTargetSelector( + options: { terminal?: InteractiveTerminal } = {}, +): ReviewTargetSelector { + const terminal = options.terminal ?? createInteractiveTerminal(); + + return async (request) => { + if (!terminal.choose || !terminal.prompt) { + throw new ReviewTargetSelectionError(noInteractiveTerminalMessage()); + } + + try { + const choices: InteractiveTerminalChoice[] = request.candidates.map( + (candidate) => ({ + detail: candidate.detail, + label: candidate.recommended + ? `${candidate.label} (recommended)` + : candidate.label, + }), + ); + choices.push({ + detail: "advanced", + label: "Enter another ref", + }); + + const selectedIndex = terminal.choose("Choose review target", choices); + + if (selectedIndex < request.candidates.length) { + const selected = request.candidates[selectedIndex]; + + if (!selected) { + throw new ReviewTargetSelectionError( + "Pushgate could not read the selected review target.", + ); + } + + return selected; + } + + const customRef = terminal.prompt("Review target ref:").trim(); + + if (!customRef) { + throw new ReviewTargetSelectionError( + "Pushgate needs a non-empty review target ref.", + ); + } + + return { + label: customRef, + ref: customRef, + source: "custom", + }; + } catch (error) { + if (error instanceof ReviewTargetSelectionError) { + throw error; + } + + if (error instanceof InteractiveTerminalError) { + throw new ReviewTargetSelectionError(noInteractiveTerminalMessage()); + } + + throw error; + } + }; +} + +async function discoverReviewTargets(options: SelectReviewTargetOptions): Promise<{ + candidates: ReviewTargetCandidate[]; + diagnostics: ReviewTargetDiagnostic[]; + promptRequired: boolean; +}> { + const diagnostics: ReviewTargetDiagnostic[] = []; + const configuredTarget = await candidateForRef({ + repoRoot: options.repoRoot, + detail: "configured review.target_branch", + label: options.configuredTargetRef, + ref: options.configuredTargetRef, + source: "configured", + }); + const targetRemoteRef = await resolveTargetRemoteRef({ + configuredTargetRef: options.configuredTargetRef, + pushRemote: options.hookContext.remote, + repoRoot: options.repoRoot, + }); + const targetRemote = + targetRemoteRef && targetRemoteRef !== options.configuredTargetRef + ? await candidateForRef({ + repoRoot: options.repoRoot, + detail: "latest fetched target remote", + label: targetRemoteRef, + ref: targetRemoteRef, + source: "target-remote", + }) + : null; + const resolvedTargetRemote = targetRemote?.commit ? targetRemote : null; + const freshness = + configuredTarget.commit && resolvedTargetRemote?.commit + ? await compareCommits( + options.repoRoot, + configuredTarget.commit, + resolvedTargetRemote.commit, + ) + : "missing"; + const branchUpdate = + options.hookContext.branchUpdates.length === 1 + ? options.hookContext.branchUpdates[0] + : undefined; + const currentBranch = branchUpdate + ? branchUpdate.localBranch + : await resolveCurrentBranch(options.repoRoot); + const fallbackCurrentRemoteRef = + !branchUpdate && currentBranch && options.hookContext.remote + ? `${options.hookContext.remote}/${currentBranch}` + : undefined; + const incremental = branchUpdate + ? (await incrementalCandidateFromPrePush(options.repoRoot, branchUpdate)) ?? + (await incrementalCandidateFromRemoteTrackingRef({ + currentBranch, + currentRemoteRef: remoteTrackingRefForUpdate( + options.hookContext.remote, + branchUpdate, + ), + repoRoot: options.repoRoot, + })) + : await incrementalCandidateFromRemoteTrackingRef({ + currentBranch, + currentRemoteRef: fallbackCurrentRemoteRef, + repoRoot: options.repoRoot, + }); + const stacked = await findStackedCandidates({ + currentRemoteRef: branchUpdate + ? remoteTrackingRefForUpdate(options.hookContext.remote, branchUpdate) + : fallbackCurrentRemoteRef, + repoRoot: options.repoRoot, + targetRemoteRef: resolvedTargetRemote?.ref, + }); + const promptRequired = + freshness === "behind" || + freshness === "diverged" || + incremental !== null || + stacked.length > 0; + + appendFreshnessDiagnostic({ + configuredTargetRef: options.configuredTargetRef, + diagnostics, + freshness, + promptRequired, + pushRemote: options.hookContext.remote, + targetRemoteRef: resolvedTargetRemote?.ref, + }); + + const candidates = dedupeCandidatesByCommit([ + configuredTarget, + resolvedTargetRemote, + incremental, + ...stacked, + ]).map((candidate) => ({ + ...candidate, + recommended: candidate === recommendedCandidate({ + candidates: [ + configuredTarget, + resolvedTargetRemote, + incremental, + ...stacked, + ], + freshness, + }), + })); + + return { + candidates, + diagnostics, + promptRequired, + }; +} + +async function candidateForRef( + options: ReviewTargetCandidate & { repoRoot: string }, +): Promise { + return { + detail: options.detail, + label: options.label, + recommended: options.recommended, + ref: options.ref, + source: options.source, + commit: await resolveCommit(options.repoRoot, options.ref), + }; +} + +async function resolveCommit( + repoRoot: string, + ref: string, +): Promise { + const result = await runGit(repoRoot, [ + "rev-parse", + "--verify", + "--quiet", + `${ref}^{commit}`, + ]); + + return result.code === 0 ? result.stdout.trim() : undefined; +} + +async function resolveTargetRemoteRef(options: { + configuredTargetRef: string; + pushRemote: string | undefined; + repoRoot: string; +}): Promise { + const upstreamResult = await runGit(options.repoRoot, [ + "rev-parse", + "--abbrev-ref", + "--symbolic-full-name", + `${options.configuredTargetRef}@{upstream}`, + ]); + + if (upstreamResult.code === 0) { + const upstream = upstreamResult.stdout.trim(); + + if (upstream) { + return upstream; + } + } + + if ( + !options.pushRemote || + isRemoteRefForPushRemote(options.configuredTargetRef, options.pushRemote) || + !isSimpleBranchName(options.configuredTargetRef) + ) { + return null; + } + + return `${options.pushRemote}/${options.configuredTargetRef}`; +} + +async function compareCommits( + repoRoot: string, + localCommit: string, + remoteCommit: string, +): Promise { + if (localCommit === remoteCommit) { + return "same"; + } + + const localIsAncestor = await isAncestor(repoRoot, localCommit, remoteCommit); + const remoteIsAncestor = await isAncestor(repoRoot, remoteCommit, localCommit); + + if (localIsAncestor) { + return "behind"; + } + + if (remoteIsAncestor) { + return "ahead"; + } + + return "diverged"; +} + +function appendFreshnessDiagnostic(options: { + configuredTargetRef: string; + diagnostics: ReviewTargetDiagnostic[]; + freshness: TargetFreshness; + promptRequired: boolean; + pushRemote: string | undefined; + targetRemoteRef: string | undefined; +}): void { + if (!options.targetRemoteRef) { + return; + } + + if (options.freshness === "behind") { + options.diagnostics.push({ + level: "warning", + message: `${options.configuredTargetRef} is behind ${options.targetRemoteRef}. Pushgate may review against stale code.`, + tip: fetchTip(options.pushRemote, options.configuredTargetRef), + }); + return; + } + + if (options.freshness === "diverged") { + options.diagnostics.push({ + level: "warning", + message: `${options.configuredTargetRef} has diverged from ${options.targetRemoteRef}. Pushgate may review against stale code.`, + tip: fetchTip(options.pushRemote, options.configuredTargetRef), + }); + return; + } + + if (options.freshness === "ahead" && options.promptRequired) { + options.diagnostics.push({ + level: "info", + message: `${options.configuredTargetRef} is ahead of ${options.targetRemoteRef}. Choose ${options.configuredTargetRef} only if those local commits belong in the review target.`, + }); + } +} + +async function incrementalCandidateFromPrePush( + repoRoot: string, + branchUpdate: PrePushBranchUpdate, +): Promise { + if ( + isZeroObjectName(branchUpdate.remoteSha) || + !isLikelyObjectName(branchUpdate.remoteSha) + ) { + return null; + } + + const commit = await resolveCommit(repoRoot, branchUpdate.remoteSha); + + if (!commit) { + return null; + } + + return { + commit, + detail: `review only commits not already on ${branchUpdate.remoteRef}`, + label: `destination ${branchUpdate.remoteBranch ?? branchUpdate.localBranch} tip`, + ref: commit, + source: "incremental", + }; +} + +async function incrementalCandidateFromRemoteTrackingRef(options: { + currentBranch: string | undefined; + currentRemoteRef: string | undefined; + repoRoot: string; +}): Promise { + if (!options.currentBranch || !options.currentRemoteRef) { + return null; + } + + const commit = await resolveCommit(options.repoRoot, options.currentRemoteRef); + + if (!commit) { + return null; + } + + return { + commit, + detail: `review only commits not already on ${options.currentRemoteRef}`, + label: `destination ${options.currentBranch} tip`, + ref: commit, + source: "incremental", + }; +} + +async function findStackedCandidates(options: { + currentRemoteRef: string | undefined; + repoRoot: string; + targetRemoteRef: string | undefined; +}): Promise { + const result = await runGit(options.repoRoot, [ + "for-each-ref", + "--format=%(refname:short)%00%(objectname)", + "refs/remotes", + ]); + + if (result.code !== 0) { + return []; + } + + const candidates: Array = []; + + for (const line of result.stdout.split("\n")) { + if (!line.trim()) { + continue; + } + + const [ref, commit] = line.split("\0", 2); + + if ( + !ref || + !commit || + ref.endsWith("/HEAD") || + ref === options.currentRemoteRef || + ref === options.targetRemoteRef || + isZeroObjectName(commit) + ) { + continue; + } + + if (!(await isAncestor(options.repoRoot, commit, "HEAD"))) { + continue; + } + + const distance = await commitDistance(options.repoRoot, commit, "HEAD"); + + if (distance === null || distance === 0) { + continue; + } + + candidates.push({ + commit, + detail: `${String(distance)} commit(s) behind HEAD`, + distance, + label: ref, + ref, + source: "stacked", + }); + } + + return candidates + .sort((left, right) => left.distance - right.distance) + .slice(0, MAX_STACKED_CANDIDATES) + .map(({ distance: _distance, ...candidate }) => candidate); +} + +function recommendedCandidate(options: { + candidates: readonly (CandidateWithCommit | null)[]; + freshness: TargetFreshness; +}): CandidateWithCommit | null { + const candidates = options.candidates.filter( + (candidate): candidate is CandidateWithCommit => candidate !== null, + ); + + return ( + candidates.find((candidate) => candidate.source === "incremental") ?? + candidates.find((candidate) => candidate.source === "stacked") ?? + (options.freshness === "behind" || options.freshness === "diverged" + ? candidates.find((candidate) => candidate.source === "target-remote") + : undefined) ?? + candidates.find((candidate) => candidate.source === "configured") ?? + null + ); +} + +function dedupeCandidatesByCommit( + candidates: readonly (CandidateWithCommit | null)[], +): CandidateWithCommit[] { + const deduped: CandidateWithCommit[] = []; + const seenCommits = new Set(); + const seenRefs = new Set(); + + for (const candidate of candidates) { + if (!candidate) { + continue; + } + + const commitKey = candidate.commit; + const refKey = candidate.ref; + + if (commitKey && seenCommits.has(commitKey)) { + continue; + } + + if (seenRefs.has(refKey)) { + continue; + } + + if (commitKey) { + seenCommits.add(commitKey); + } + + seenRefs.add(refKey); + deduped.push(candidate); + } + + return deduped; +} + +function remoteTrackingRefForUpdate( + remote: string | undefined, + update: PrePushBranchUpdate, +): string | undefined { + if (!remote) { + return undefined; + } + + const remoteBranch = update.remoteBranch ?? update.localBranch; + + return `${remote}/${remoteBranch}`; +} + +async function isAncestor( + repoRoot: string, + ancestor: string, + descendant: string, +): Promise { + const result = await runGit(repoRoot, [ + "merge-base", + "--is-ancestor", + ancestor, + descendant, + ]); + + return result.code === 0; +} + +async function commitDistance( + repoRoot: string, + ancestor: string, + descendant: string, +): Promise { + const result = await runGit(repoRoot, [ + "rev-list", + "--count", + `${ancestor}..${descendant}`, + ]); + + if (result.code !== 0) { + return null; + } + + const distance = Number.parseInt(result.stdout.trim(), 10); + + return Number.isFinite(distance) ? distance : null; +} + +async function resolveCurrentBranch( + repoRoot: string, +): Promise { + const result = await runGit(repoRoot, [ + "symbolic-ref", + "--quiet", + "--short", + "HEAD", + ]); + + return result.code === 0 ? result.stdout.trim() : undefined; +} + +function isSimpleBranchName(ref: string): boolean { + return ( + !ref.startsWith("refs/") && + !ref.includes("..") && + !ref.includes("@{") && + !ref.includes(":") && + !/^[0-9a-f]{40}$/i.test(ref) + ); +} + +function isRemoteRefForPushRemote(ref: string, pushRemote: string): boolean { + return ( + ref === pushRemote || + ref.startsWith(`${pushRemote}/`) || + ref.startsWith("refs/remotes/") + ); +} + +function isZeroObjectName(value: string): boolean { + return ZERO_OBJECT.test(value); +} + +function isLikelyObjectName(value: string): boolean { + return /^[0-9a-f]{40,64}$/i.test(value); +} + +function fetchTip(remote: string | undefined, targetRef: string): string { + const fetchCommand = remote ? `git fetch ${remote}` : "git fetch"; + + return `Run \`${fetchCommand}\` and update \`${targetRef}\` before retrying, or choose a review target now.`; +} + +function noInteractiveTerminalMessage(): string { + return `Pushgate needs a review target selection, but no interactive terminal is available. Re-run from a terminal, set review.target_branch explicitly, or use \`git -c ${REVIEW_TARGET_CONFIG_KEY}= push\`.`; +} diff --git a/src/workflows/terminal.ts b/src/workflows/terminal.ts index e20437f..76662e3 100644 --- a/src/workflows/terminal.ts +++ b/src/workflows/terminal.ts @@ -1,7 +1,17 @@ import { closeSync, openSync, readSync, writeSync } from "node:fs"; export interface InteractiveTerminal { + choose?( + question: string, + choices: readonly InteractiveTerminalChoice[], + ): number; confirm(question: string): boolean; + prompt?(question: string): string; +} + +export interface InteractiveTerminalChoice { + detail?: string; + label: string; } interface TerminalFileDescriptors { @@ -26,12 +36,69 @@ export class InteractiveTerminalError extends Error { export function createInteractiveTerminal(): InteractiveTerminal { return { + choose(question, choices) { + return chooseWithInteractiveTerminal(question, choices); + }, confirm(question) { return confirmWithInteractiveTerminal(question); }, + prompt(question) { + return promptWithInteractiveTerminal(question); + }, }; } +function chooseWithInteractiveTerminal( + question: string, + choices: readonly InteractiveTerminalChoice[], +): number { + if (choices.length === 0) { + throw new InteractiveTerminalError("No terminal choices were available."); + } + + let terminal: TerminalFileDescriptors | undefined; + + try { + terminal = openInteractiveTerminal(); + + for (;;) { + writeSync(terminal.outputFd, `${question}\n`); + + for (const [index, choice] of choices.entries()) { + const detail = choice.detail ? ` - ${choice.detail}` : ""; + writeSync( + terminal.outputFd, + ` ${String(index + 1)}. ${choice.label}${detail}\n`, + ); + } + + writeSync(terminal.outputFd, `Select 1-${String(choices.length)}: `); + + const answer = readLineSync(terminal.inputFd).trim(); + const selected = Number.parseInt(answer, 10); + + if ( + Number.isInteger(selected) && + String(selected) === answer && + selected >= 1 && + selected <= choices.length + ) { + return selected - 1; + } + + writeSync(terminal.outputFd, "Please enter one of the listed numbers.\n"); + } + } catch (error) { + if (error instanceof InteractiveTerminalError) { + throw error; + } + + throw new InteractiveTerminalError("No interactive terminal is available."); + } finally { + terminal?.close(); + } +} + function confirmWithInteractiveTerminal(question: string): boolean { let terminal: TerminalFileDescriptors | undefined; @@ -67,6 +134,24 @@ function confirmWithInteractiveTerminal(question: string): boolean { } } +function promptWithInteractiveTerminal(question: string): string { + let terminal: TerminalFileDescriptors | undefined; + + try { + terminal = openInteractiveTerminal(); + writeSync(terminal.outputFd, `${question} `); + return readLineSync(terminal.inputFd).trim(); + } catch (error) { + if (error instanceof InteractiveTerminalError) { + throw error; + } + + throw new InteractiveTerminalError("No interactive terminal is available."); + } finally { + terminal?.close(); + } +} + function formatYesNoPrompt(question: string): string { return `${question} [y/N] `; } diff --git a/templates/base.yml b/templates/base.yml index 08776f9..f87b02c 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -41,7 +41,7 @@ ai: # model: auto review: - # Branch to diff against when collecting changes. + # Configured default review target. target_branch: main # Lines of surrounding context included in the diff sent to the provider. diff --git a/test/pre-push-stdin.test.ts b/test/pre-push-stdin.test.ts index d6c1bc1..57f2d30 100644 --- a/test/pre-push-stdin.test.ts +++ b/test/pre-push-stdin.test.ts @@ -3,7 +3,9 @@ import { Readable } from "node:stream"; import test from "node:test"; import { + parsePrePushLine, parseBranchFromPrePushLine, + readPrePushInputFromStdin, readPrePushBranchFromStdin, } from "../src/workflows/pre-push.js"; @@ -23,6 +25,58 @@ test("parseBranchFromPrePushLine returns the local branch ref", () => { assert.equal(parseBranchFromPrePushLine(""), undefined); }); +test("parsePrePushLine returns branch update metadata from Git pre-push input", () => { + assert.deepEqual( + parsePrePushLine( + "refs/heads/feature local-sha refs/heads/feature remote-sha", + ), + { + localBranch: "feature", + localRef: "refs/heads/feature", + localSha: "local-sha", + remoteBranch: "feature", + remoteRef: "refs/heads/feature", + remoteSha: "remote-sha", + }, + ); + assert.equal( + parsePrePushLine( + "refs/tags/v1.0.0 local-sha refs/tags/v1.0.0 remote-sha", + ), + null, + ); + assert.equal(parsePrePushLine(""), null); +}); + +test("readPrePushInputFromStdin keeps every bounded branch update", async () => { + const input = await readPrePushInputFromStdin( + Readable.from([ + "refs/tags/v1.0.0 local-sha refs/tags/v1.0.0 remote-sha\n", + "refs/heads/feature local-sha refs/heads/feature remote-sha\n", + "refs/heads/other other-local-sha refs/heads/other other-remote-sha\n", + ]), + ); + + assert.deepEqual(input.branchUpdates, [ + { + localBranch: "feature", + localRef: "refs/heads/feature", + localSha: "local-sha", + remoteBranch: "feature", + remoteRef: "refs/heads/feature", + remoteSha: "remote-sha", + }, + { + localBranch: "other", + localRef: "refs/heads/other", + localSha: "other-local-sha", + remoteBranch: "other", + remoteRef: "refs/heads/other", + remoteSha: "other-remote-sha", + }, + ]); +}); + test("readPrePushBranchFromStdin parses incrementally and discards trailing input", async () => { const branch = await readPrePushBranchFromStdin( Readable.from([ diff --git a/test/warning-confirmation.test.ts b/test/warning-confirmation.test.ts index ebbdf86..5ad007a 100644 --- a/test/warning-confirmation.test.ts +++ b/test/warning-confirmation.test.ts @@ -7,6 +7,11 @@ import { createTerminalWarningConfirmer, WarningConfirmationError, } from "../src/workflows/warning-confirmation.js"; +import { + createTerminalReviewTargetSelector, + REVIEW_TARGET_CONFIG_KEY, + ReviewTargetSelectionError, +} from "../src/workflows/review-target-selection.js"; test("terminal warning confirmer asks the default-no push question through the terminal", async () => { const questions: string[] = []; @@ -51,3 +56,32 @@ test("terminal warning confirmer maps terminal unavailability to a warning confi }, ); }); + +test("terminal review target selector fails closed without interactive choice support", async () => { + const terminal: InteractiveTerminal = { + confirm() { + return false; + }, + }; + const selector = createTerminalReviewTargetSelector({ terminal }); + + await assert.rejects( + () => + selector({ + candidates: [ + { + label: "main", + ref: "main", + source: "configured", + }, + ], + diagnostics: [], + }), + (error) => { + assert.ok(error instanceof ReviewTargetSelectionError); + assert.match(error.message, /no interactive terminal is available/); + assert.match(error.message, new RegExp(REVIEW_TARGET_CONFIG_KEY)); + return true; + }, + ); +}); diff --git a/test/workflow-run-plan.test.ts b/test/workflow-run-plan.test.ts index ed9eddb..873971d 100644 --- a/test/workflow-run-plan.test.ts +++ b/test/workflow-run-plan.test.ts @@ -7,8 +7,14 @@ import { Readable, Writable } from "node:stream"; import test from "node:test"; import { sanitizeGitLocalEnv } from "../src/git/environment.js"; +import type { + ReviewTargetCandidate, + ReviewTargetSelector, +} from "../src/workflows/review-target-selection.js"; import { runPrePushWorkflow } from "../src/workflows/pre-push.js"; +const ZERO_OBJECT = "0".repeat(40); + test("skip-all-checks bypasses config loading", async () => { await withGitRepo(async (repoRoot) => { await writeRepoFile(repoRoot, ".pushgate.yml", "version: nope\n"); @@ -103,13 +109,219 @@ test("skip-ai-check keeps deterministic changed-file work", async () => { }); }); +test("existing destination branch can review only the incremental push diff", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]); + + const seenCandidates: ReviewTargetCandidate[] = []; + const selector: ReviewTargetSelector = async (request) => { + seenCandidates.push(...request.candidates); + const incremental = request.candidates.find( + (candidate) => candidate.source === "incremental", + ); + + assert.ok(incremental, "expected an incremental review target candidate"); + return incremental; + }; + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: selector, + stdin: Readable.from( + `refs/heads/feature ${commits.head} refs/heads/feature ${commits.remoteFeature}\n`, + ), + }); + + assert.equal(result.code, 0, formatResult(result)); + assert.deepEqual( + seenCandidates.map((candidate) => candidate.source), + ["configured", "incremental"], + ); + assert.match( + result.stdout, + /Review target:\s+destination feature tip/, + ); + assert.match(result.stdout, /Review range:/); + assert.match(result.stdout, /Checks passed/); + assert.equal(result.stderr, ""); + }); +}); + +test("missing pre-push stdin falls back to the current branch remote-tracking ref", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]); + await checkedRun( + "git", + ["update-ref", "refs/remotes/origin/feature", commits.remoteFeature], + { cwd: repoRoot }, + ); + + const selector: ReviewTargetSelector = async (request) => { + const incremental = request.candidates.find( + (candidate) => candidate.source === "incremental", + ); + + assert.ok(incremental, "expected an incremental review target candidate"); + return incremental; + }; + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: selector, + }); + + assert.equal(result.code, 0, formatResult(result)); + assert.match( + result.stdout, + /Review target:\s+destination feature tip/, + ); + assert.match(result.stdout, /Checks passed/); + assert.equal(result.stderr, ""); + }); +}); + +test("stale local target prompts with the fetched remote target candidate", async () => { + await withStaleTargetRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/feature.ts"]); + + const seenDiagnostics: string[] = []; + const seenCandidates: ReviewTargetCandidate[] = []; + const selector: ReviewTargetSelector = async (request) => { + seenDiagnostics.push( + ...request.diagnostics.map((diagnostic) => diagnostic.message), + ); + seenCandidates.push(...request.candidates); + const targetRemote = request.candidates.find( + (candidate) => candidate.source === "target-remote", + ); + + assert.ok(targetRemote, "expected a target remote candidate"); + return targetRemote; + }; + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: selector, + stdin: Readable.from( + `refs/heads/feature ${commits.head} refs/heads/feature ${ZERO_OBJECT}\n`, + ), + }); + + assert.equal(result.code, 0, formatResult(result)); + assert.deepEqual( + seenCandidates.map((candidate) => candidate.source), + ["configured", "target-remote"], + ); + assert.match(seenDiagnostics.join("\n"), /main is behind origin\/main/); + assert.match(result.stdout, /main is behind origin\/main/); + assert.match(result.stdout, /Run `git fetch origin`/); + assert.match(result.stdout, /Review target:\s+origin\/main/); + assert.equal(result.stderr, ""); + }); +}); + +test("stacked remote ancestor can review only the stacked branch diff", async () => { + await withStackedFeatureRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/part2.ts"]); + + const seenCandidates: ReviewTargetCandidate[] = []; + const selector: ReviewTargetSelector = async (request) => { + seenCandidates.push(...request.candidates); + const stacked = request.candidates.find( + (candidate) => candidate.source === "stacked", + ); + + assert.ok(stacked, "expected a stacked review target candidate"); + return stacked; + }; + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: selector, + stdin: Readable.from( + `refs/heads/part-2-of-feature-A ${commits.head} refs/heads/part-2-of-feature-A ${ZERO_OBJECT}\n`, + ), + }); + + assert.equal(result.code, 0, formatResult(result)); + assert.deepEqual( + seenCandidates.map((candidate) => candidate.source), + ["configured", "stacked"], + ); + assert.match( + result.stdout, + /Review target:\s+origin\/part-1-of-feature-A/, + ); + assert.match(result.stdout, /Checks passed/); + assert.equal(result.stderr, ""); + }); +}); + +test("one-push review target override skips interactive selection", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, [ + "src/one.ts", + "src/two.ts", + ]); + await checkedRun("git", ["config", "pushgate.review-target", "main"], { + cwd: repoRoot, + }); + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: async () => { + throw new Error("override should skip review target selection"); + }, + stdin: Readable.from( + `refs/heads/feature ${commits.head} refs/heads/feature ${commits.remoteFeature}\n`, + ), + }); + + assert.equal(result.code, 0, formatResult(result)); + assert.match(result.stdout, /Review target:\s+main/); + assert.match(result.stdout, /Checks passed/); + assert.equal(result.stderr, ""); + }); +}); + +test("multi-branch pushes fail before choosing one review target", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]); + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + stdin: Readable.from( + [ + `refs/heads/feature ${commits.head} refs/heads/feature ${commits.remoteFeature}`, + `refs/heads/other ${commits.head} refs/heads/other ${ZERO_OBJECT}`, + "", + ].join("\n"), + ), + }); + + assert.equal(result.code, 1, formatResult(result)); + assert.match(result.stdout, /updates multiple branches/); + assert.match(result.stdout, /Push one branch at a time/); + assert.equal(result.stderr, ""); + }); +}); + interface WorkflowResult { code: number; stderr: string; stdout: string; } -async function runWorkflowInRepo(repoRoot: string): Promise { +interface RunWorkflowOptions { + hookArgs?: readonly string[]; + reviewTargetSelector?: ReviewTargetSelector; + stdin?: Readable; +} + +async function runWorkflowInRepo( + repoRoot: string, + options: RunWorkflowOptions = {}, +): Promise { const previousCwd = process.cwd(); const stdout = captureOutput(); const stderr = captureOutput(); @@ -119,8 +331,10 @@ async function runWorkflowInRepo(repoRoot: string): Promise { try { const code = await runPrePushWorkflow({ env: sanitizeGitLocalEnv(process.env), + hookArgs: options.hookArgs, + reviewTargetSelector: options.reviewTargetSelector, stderr: stderr.stream, - stdin: Readable.from(""), + stdin: options.stdin ?? Readable.from(""), stdout: stdout.stream, }); @@ -196,6 +410,179 @@ async function withChangedFileRepo( }); } +async function withIncrementalPushRepo( + callback: ( + repoRoot: string, + commits: { head: string; remoteFeature: string }, + ) => Promise, +): Promise { + await withGitRepo(async (repoRoot) => { + await checkedRun("git", ["config", "user.email", "workflow@example.test"], { + cwd: repoRoot, + }); + await checkedRun("git", ["config", "user.name", "Pushgate Workflow"], { + cwd: repoRoot, + }); + await writeRepoFile(repoRoot, "README.md", "baseline\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "baseline"], { + cwd: repoRoot, + }); + await checkedRun("git", ["switch", "--quiet", "-c", "feature"], { + cwd: repoRoot, + }); + await writeRepoFile(repoRoot, "src/one.ts", "export const one = 1;\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "feature one"], { + cwd: repoRoot, + }); + const remoteFeature = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + + await writeRepoFile(repoRoot, "src/two.ts", "export const two = 2;\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "feature two"], { + cwd: repoRoot, + }); + const head = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + + await callback(repoRoot, { head, remoteFeature }); + }); +} + +async function withStaleTargetRepo( + callback: ( + repoRoot: string, + commits: { head: string; originMain: string }, + ) => Promise, +): Promise { + await withGitRepo(async (repoRoot) => { + await checkedRun("git", ["config", "user.email", "workflow@example.test"], { + cwd: repoRoot, + }); + await checkedRun("git", ["config", "user.name", "Pushgate Workflow"], { + cwd: repoRoot, + }); + await writeRepoFile(repoRoot, "README.md", "baseline\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "baseline"], { + cwd: repoRoot, + }); + + await checkedRun("git", ["switch", "--quiet", "-c", "remote-main"], { + cwd: repoRoot, + }); + await writeRepoFile(repoRoot, "src/remote.ts", "export const remote = 1;\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "remote main"], { + cwd: repoRoot, + }); + const originMain = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + await checkedRun( + "git", + ["update-ref", "refs/remotes/origin/main", originMain], + { cwd: repoRoot }, + ); + + await checkedRun("git", ["switch", "--quiet", "main"], { cwd: repoRoot }); + await checkedRun("git", ["switch", "--quiet", "-c", "feature"], { + cwd: repoRoot, + }); + await writeRepoFile( + repoRoot, + "src/feature.ts", + "export const feature = 1;\n", + ); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "feature"], { + cwd: repoRoot, + }); + const head = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + + await callback(repoRoot, { head, originMain }); + }); +} + +async function withStackedFeatureRepo( + callback: ( + repoRoot: string, + commits: { head: string; part1: string }, + ) => Promise, +): Promise { + await withGitRepo(async (repoRoot) => { + await checkedRun("git", ["config", "user.email", "workflow@example.test"], { + cwd: repoRoot, + }); + await checkedRun("git", ["config", "user.name", "Pushgate Workflow"], { + cwd: repoRoot, + }); + await writeRepoFile(repoRoot, "README.md", "baseline\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "baseline"], { + cwd: repoRoot, + }); + + await checkedRun( + "git", + ["switch", "--quiet", "-c", "part-1-of-feature-A"], + { cwd: repoRoot }, + ); + await writeRepoFile(repoRoot, "src/part1.ts", "export const part1 = 1;\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "part one"], { + cwd: repoRoot, + }); + const part1 = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + await checkedRun( + "git", + ["update-ref", "refs/remotes/origin/part-1-of-feature-A", part1], + { cwd: repoRoot }, + ); + + await checkedRun( + "git", + ["switch", "--quiet", "-c", "part-2-of-feature-A"], + { cwd: repoRoot }, + ); + await writeRepoFile(repoRoot, "src/part2.ts", "export const part2 = 2;\n"); + await checkedRun("git", ["add", "--all"], { cwd: repoRoot }); + await checkedRun("git", ["commit", "--quiet", "-m", "part two"], { + cwd: repoRoot, + }); + const head = await gitStdout(repoRoot, ["rev-parse", "HEAD"]); + + await callback(repoRoot, { head, part1 }); + }); +} + +async function writeChangedFilesAssertionConfig( + repoRoot: string, + expectedPaths: readonly string[], +): Promise { + const assertion = [ + "const assert = require('node:assert/strict');", + `const expected = ${JSON.stringify([...expectedPaths].sort())};`, + "const actual = process.argv.slice(1).sort();", + "assert.deepEqual(actual, expected);", + ].join(" "); + + await writeRepoFile( + repoRoot, + ".pushgate.yml", + [ + "version: 2", + "review:", + " target_branch: main", + "ai:", + " mode: off", + "tools:", + " - name: changed-files-tool", + ` command: ${JSON.stringify([process.execPath, "-e", assertion, "{changed_files}"])}`, + " run: changed_files", + "", + ].join("\n"), + ); +} + async function writeRepoFile( repoRoot: string, relativePath: string, @@ -248,6 +635,53 @@ async function checkedRun( } } +async function gitStdout(repoRoot: string, args: string[]): Promise { + const result = await runCommand("git", args, { cwd: repoRoot }); + + if (result.code !== 0) { + throw new Error(formatResult(result)); + } + + return result.stdout.trim(); +} + +async function runCommand( + command: string, + args: string[], + options: CommandOptions, +): Promise<{ + code: number | null; + stderr: string; + stdout: string; +}> { + return await new Promise<{ + code: number | null; + stderr: string; + stdout: string; + }>((resolve, reject) => { + const child = spawn(command, args, { + cwd: options.cwd, + env: sanitizeGitLocalEnv(process.env), + stdio: ["ignore", "pipe", "pipe"], + }); + let stderr = ""; + let stdout = ""; + + child.stdout?.setEncoding("utf8"); + child.stderr?.setEncoding("utf8"); + child.stdout?.on("data", (data: string) => { + stdout += data; + }); + child.stderr?.on("data", (data: string) => { + stderr += data; + }); + child.on("error", reject); + child.on("close", (code) => { + resolve({ code, stderr, stdout }); + }); + }); +} + function formatResult(result: { code: number | null; stderr: string; From c5d12f3e179cbeb5a3b409e1bba27e9c1ee20fd9 Mon Sep 17 00:00:00 2001 From: dbrosio3 Date: Thu, 2 Jul 2026 09:54:30 -0300 Subject: [PATCH 2/4] Support arrow-key review target selection --- bin/pushgate.mjs | 208 ++++++++++++++++++++++++--- src/workflows/terminal.ts | 287 ++++++++++++++++++++++++++++++++++---- test/terminal.test.ts | 28 ++++ 3 files changed, 479 insertions(+), 44 deletions(-) diff --git a/bin/pushgate.mjs b/bin/pushgate.mjs index aa1ec62..5b31152 100755 --- a/bin/pushgate.mjs +++ b/bin/pushgate.mjs @@ -28180,6 +28180,7 @@ function requireChangedFileResolution(changedFileResolution) { } // src/workflows/terminal.ts +import { spawnSync } from "node:child_process"; import { closeSync, openSync, readSync, writeSync } from "node:fs"; var pendingInputByFd = /* @__PURE__ */ new Map(); var InteractiveTerminalError = class extends Error { @@ -28208,24 +28209,11 @@ function chooseWithInteractiveTerminal(question, choices) { let terminal; try { terminal = openInteractiveTerminal(); - for (; ; ) { - writeSync(terminal.outputFd, `${question} -`); - for (const [index, choice] of choices.entries()) { - const detail = choice.detail ? ` - ${choice.detail}` : ""; - writeSync( - terminal.outputFd, - ` ${String(index + 1)}. ${choice.label}${detail} -` - ); - } - writeSync(terminal.outputFd, `Select 1-${String(choices.length)}: `); - const answer = readLineSync(terminal.inputFd).trim(); - const selected = Number.parseInt(answer, 10); - if (Number.isInteger(selected) && String(selected) === answer && selected >= 1 && selected <= choices.length) { - return selected - 1; - } - writeSync(terminal.outputFd, "Please enter one of the listed numbers.\n"); + const rawMode = enableRawMode(terminal.inputFd); + try { + return chooseWithKeyNavigation(terminal, question, choices); + } finally { + rawMode.restore(); } } catch (error51) { if (error51 instanceof InteractiveTerminalError) { @@ -28236,6 +28224,190 @@ function chooseWithInteractiveTerminal(question, choices) { terminal?.close(); } } +function chooseWithKeyNavigation(terminal, question, choices) { + let selectedIndex = 0; + let renderedLineCount = 0; + let numericInput = ""; + let message; + const render = () => { + renderedLineCount = renderChoicePrompt({ + choices, + message, + numericInput, + outputFd: terminal.outputFd, + previousLineCount: renderedLineCount, + question, + selectedIndex + }); + }; + render(); + for (; ; ) { + const key = readChoiceKey(terminal.inputFd); + switch (key.kind) { + case "up": + selectedIndex = (selectedIndex - 1 + choices.length) % choices.length; + numericInput = ""; + message = void 0; + render(); + break; + case "down": + selectedIndex = (selectedIndex + 1) % choices.length; + numericInput = ""; + message = void 0; + render(); + break; + case "digit": + numericInput += key.value; + message = void 0; + render(); + break; + case "backspace": + numericInput = numericInput.slice(0, -1); + message = void 0; + render(); + break; + case "enter": { + if (!numericInput) { + clearChoicePrompt(terminal.outputFd, renderedLineCount); + return selectedIndex; + } + const selected = Number.parseInt(numericInput, 10); + if (selected >= 1 && selected <= choices.length) { + clearChoicePrompt(terminal.outputFd, renderedLineCount); + return selected - 1; + } + message = `Please enter a number from 1 to ${String(choices.length)}.`; + numericInput = ""; + render(); + break; + } + case "interrupt": + throw new InteractiveTerminalError("Terminal input was interrupted."); + case "ignored": + break; + } + } +} +function renderChoicePrompt(options) { + const lines = [ + options.question, + ...options.choices.map((choice, index) => { + const detail = choice.detail ? ` - ${choice.detail}` : ""; + const marker = index === options.selectedIndex ? ">" : " "; + return `${marker} ${String(index + 1)}. ${choice.label}${detail}`; + }), + "Use Up/Down arrows, Enter to select, or type a number." + ]; + if (options.numericInput) { + lines.push(`Selection: ${options.numericInput}`); + } + if (options.message) { + lines.push(options.message); + } + rewriteTerminalBlock(options.outputFd, options.previousLineCount, lines); + return lines.length; +} +function rewriteTerminalBlock(outputFd, previousLineCount, lines) { + if (previousLineCount > 0) { + writeSync(outputFd, `\x1B[${String(previousLineCount)}A`); + } + for (const line of lines) { + writeSync(outputFd, `\r\x1B[2K${line} +`); + } + for (let index = lines.length; index < previousLineCount; index += 1) { + writeSync(outputFd, "\r\x1B[2K\n"); + } + if (previousLineCount > lines.length) { + writeSync(outputFd, `\x1B[${String(previousLineCount - lines.length)}A`); + } +} +function clearChoicePrompt(outputFd, lineCount) { + if (lineCount === 0) { + return; + } + writeSync(outputFd, `\x1B[${String(lineCount)}A`); + for (let index = 0; index < lineCount; index += 1) { + writeSync(outputFd, "\r\x1B[2K\n"); + } + writeSync(outputFd, `\x1B[${String(lineCount)}A`); +} +function readChoiceKey(fd) { + const char = readCharSync(fd); + if (char === null) { + throw new InteractiveTerminalError("No terminal input was available."); + } + if (char === "") { + return { kind: "interrupt" }; + } + if (char === "\n") { + return { kind: "enter" }; + } + if (char === "\r") { + consumeOptionalLfAfterCarriageReturn(fd); + return { kind: "enter" }; + } + if (char === "\x7F" || char === "\b") { + return { kind: "backspace" }; + } + if (/^\d$/.test(char)) { + return { kind: "digit", value: char }; + } + if (char === "\x1B") { + const second = readCharSync(fd); + if (second === "[") { + const third = readCharSync(fd); + if (third === "A") { + return { kind: "up" }; + } + if (third === "B") { + return { kind: "down" }; + } + } + if (second === "O") { + const third = readCharSync(fd); + if (third === "A") { + return { kind: "up" }; + } + if (third === "B") { + return { kind: "down" }; + } + } + } + return { kind: "ignored" }; +} +function enableRawMode(fd) { + if (process.platform === "win32") { + return noopRawMode(); + } + const state = spawnSync("stty", ["-g"], { + encoding: "utf8", + stdio: [fd, "pipe", "ignore"] + }); + if (state.status !== 0 || state.error || !state.stdout.trim()) { + return noopRawMode(); + } + const savedState = state.stdout.trim(); + const raw = spawnSync("stty", ["raw", "-echo"], { + stdio: [fd, "ignore", "ignore"] + }); + if (raw.status !== 0 || raw.error) { + return noopRawMode(); + } + return { + restore() { + spawnSync("stty", [savedState], { + stdio: [fd, "ignore", "ignore"] + }); + } + }; +} +function noopRawMode() { + return { + restore() { + } + }; +} function confirmWithInteractiveTerminal(question) { let terminal; try { diff --git a/src/workflows/terminal.ts b/src/workflows/terminal.ts index 76662e3..4652939 100644 --- a/src/workflows/terminal.ts +++ b/src/workflows/terminal.ts @@ -1,3 +1,4 @@ +import { spawnSync } from "node:child_process"; import { closeSync, openSync, readSync, writeSync } from "node:fs"; export interface InteractiveTerminal { @@ -20,6 +21,10 @@ interface TerminalFileDescriptors { outputFd: number; } +interface TerminalRawMode { + restore(): void; +} + interface TerminalDevicePath { input: string; output: string; @@ -60,33 +65,12 @@ function chooseWithInteractiveTerminal( try { terminal = openInteractiveTerminal(); + const rawMode = enableRawMode(terminal.inputFd); - for (;;) { - writeSync(terminal.outputFd, `${question}\n`); - - for (const [index, choice] of choices.entries()) { - const detail = choice.detail ? ` - ${choice.detail}` : ""; - writeSync( - terminal.outputFd, - ` ${String(index + 1)}. ${choice.label}${detail}\n`, - ); - } - - writeSync(terminal.outputFd, `Select 1-${String(choices.length)}: `); - - const answer = readLineSync(terminal.inputFd).trim(); - const selected = Number.parseInt(answer, 10); - - if ( - Number.isInteger(selected) && - String(selected) === answer && - selected >= 1 && - selected <= choices.length - ) { - return selected - 1; - } - - writeSync(terminal.outputFd, "Please enter one of the listed numbers.\n"); + try { + return chooseWithKeyNavigation(terminal, question, choices); + } finally { + rawMode.restore(); } } catch (error) { if (error instanceof InteractiveTerminalError) { @@ -99,6 +83,257 @@ function chooseWithInteractiveTerminal( } } +function chooseWithKeyNavigation( + terminal: TerminalFileDescriptors, + question: string, + choices: readonly InteractiveTerminalChoice[], +): number { + let selectedIndex = 0; + let renderedLineCount = 0; + let numericInput = ""; + let message: string | undefined; + + const render = () => { + renderedLineCount = renderChoicePrompt({ + choices, + message, + numericInput, + outputFd: terminal.outputFd, + previousLineCount: renderedLineCount, + question, + selectedIndex, + }); + }; + + render(); + + for (;;) { + const key = readChoiceKey(terminal.inputFd); + + switch (key.kind) { + case "up": + selectedIndex = (selectedIndex - 1 + choices.length) % choices.length; + numericInput = ""; + message = undefined; + render(); + break; + case "down": + selectedIndex = (selectedIndex + 1) % choices.length; + numericInput = ""; + message = undefined; + render(); + break; + case "digit": + numericInput += key.value; + message = undefined; + render(); + break; + case "backspace": + numericInput = numericInput.slice(0, -1); + message = undefined; + render(); + break; + case "enter": { + if (!numericInput) { + clearChoicePrompt(terminal.outputFd, renderedLineCount); + return selectedIndex; + } + + const selected = Number.parseInt(numericInput, 10); + + if (selected >= 1 && selected <= choices.length) { + clearChoicePrompt(terminal.outputFd, renderedLineCount); + return selected - 1; + } + + message = `Please enter a number from 1 to ${String(choices.length)}.`; + numericInput = ""; + render(); + break; + } + case "interrupt": + throw new InteractiveTerminalError("Terminal input was interrupted."); + case "ignored": + break; + } + } +} + +function renderChoicePrompt(options: { + choices: readonly InteractiveTerminalChoice[]; + message: string | undefined; + numericInput: string; + outputFd: number; + previousLineCount: number; + question: string; + selectedIndex: number; +}): number { + const lines = [ + options.question, + ...options.choices.map((choice, index) => { + const detail = choice.detail ? ` - ${choice.detail}` : ""; + const marker = index === options.selectedIndex ? ">" : " "; + return `${marker} ${String(index + 1)}. ${choice.label}${detail}`; + }), + "Use Up/Down arrows, Enter to select, or type a number.", + ]; + + if (options.numericInput) { + lines.push(`Selection: ${options.numericInput}`); + } + + if (options.message) { + lines.push(options.message); + } + + rewriteTerminalBlock(options.outputFd, options.previousLineCount, lines); + return lines.length; +} + +function rewriteTerminalBlock( + outputFd: number, + previousLineCount: number, + lines: readonly string[], +): void { + if (previousLineCount > 0) { + writeSync(outputFd, `\u001B[${String(previousLineCount)}A`); + } + + for (const line of lines) { + writeSync(outputFd, `\r\u001B[2K${line}\n`); + } + + for (let index = lines.length; index < previousLineCount; index += 1) { + writeSync(outputFd, "\r\u001B[2K\n"); + } + + if (previousLineCount > lines.length) { + writeSync(outputFd, `\u001B[${String(previousLineCount - lines.length)}A`); + } +} + +function clearChoicePrompt(outputFd: number, lineCount: number): void { + if (lineCount === 0) { + return; + } + + writeSync(outputFd, `\u001B[${String(lineCount)}A`); + + for (let index = 0; index < lineCount; index += 1) { + writeSync(outputFd, "\r\u001B[2K\n"); + } + + writeSync(outputFd, `\u001B[${String(lineCount)}A`); +} + +type ChoiceKey = + | { kind: "backspace" } + | { kind: "digit"; value: string } + | { kind: "down" } + | { kind: "enter" } + | { kind: "ignored" } + | { kind: "interrupt" } + | { kind: "up" }; + +function readChoiceKey(fd: number): ChoiceKey { + const char = readCharSync(fd); + + if (char === null) { + throw new InteractiveTerminalError("No terminal input was available."); + } + + if (char === "\u0003") { + return { kind: "interrupt" }; + } + + if (char === "\n") { + return { kind: "enter" }; + } + + if (char === "\r") { + consumeOptionalLfAfterCarriageReturn(fd); + return { kind: "enter" }; + } + + if (char === "\u007F" || char === "\b") { + return { kind: "backspace" }; + } + + if (/^\d$/.test(char)) { + return { kind: "digit", value: char }; + } + + if (char === "\u001B") { + const second = readCharSync(fd); + + if (second === "[") { + const third = readCharSync(fd); + + if (third === "A") { + return { kind: "up" }; + } + + if (third === "B") { + return { kind: "down" }; + } + } + + if (second === "O") { + const third = readCharSync(fd); + + if (third === "A") { + return { kind: "up" }; + } + + if (third === "B") { + return { kind: "down" }; + } + } + } + + return { kind: "ignored" }; +} + +function enableRawMode(fd: number): TerminalRawMode { + if (process.platform === "win32") { + return noopRawMode(); + } + + const state = spawnSync("stty", ["-g"], { + encoding: "utf8", + stdio: [fd, "pipe", "ignore"], + }); + + if (state.status !== 0 || state.error || !state.stdout.trim()) { + return noopRawMode(); + } + + const savedState = state.stdout.trim(); + const raw = spawnSync("stty", ["raw", "-echo"], { + stdio: [fd, "ignore", "ignore"], + }); + + if (raw.status !== 0 || raw.error) { + return noopRawMode(); + } + + return { + restore() { + spawnSync("stty", [savedState], { + stdio: [fd, "ignore", "ignore"], + }); + }, + }; +} + +function noopRawMode(): TerminalRawMode { + return { + restore() { + // Nothing to restore. + }, + }; +} + function confirmWithInteractiveTerminal(question: string): boolean { let terminal: TerminalFileDescriptors | undefined; diff --git a/test/terminal.test.ts b/test/terminal.test.ts index ed7ebb6..737b36c 100644 --- a/test/terminal.test.ts +++ b/test/terminal.test.ts @@ -51,6 +51,34 @@ test("interactive terminal consumes CRLF as one line ending", async () => { assert.equal(output, "Continue with push? [y/N] ".repeat(2)); }); +test("interactive terminal choice prompt supports arrow-key navigation", async () => { + let selected = -1; + const output = await withTerminalInput("\x1B[B\x1B[B\x1B[A\r", (terminal) => { + selected = terminal.choose?.("Choose review target", [ + { label: "main", detail: "configured review.target_branch" }, + { label: "origin/main", detail: "latest fetched target remote" }, + { label: "Enter another ref", detail: "advanced" }, + ]) ?? -1; + }); + + assert.equal(selected, 1); + assert.match(output, /> 2\. origin\/main - latest fetched target remote/); +}); + +test("interactive terminal choice prompt still accepts numeric selection", async () => { + let selected = -1; + + await withTerminalInput("3\n", (terminal) => { + selected = terminal.choose?.("Choose review target", [ + { label: "main" }, + { label: "origin/main" }, + { label: "Enter another ref" }, + ]) ?? -1; + }); + + assert.equal(selected, 2); +}); + async function withTempDir( callback: (tempDir: string) => Promise, ): Promise { From d82f56c467cdd094d455d100b9a5ce17113ea1db Mon Sep 17 00:00:00 2001 From: dbrosio3 Date: Thu, 2 Jul 2026 10:16:06 -0300 Subject: [PATCH 3/4] Fix review target override safety --- bin/pushgate.mjs | 50 ++++++++++++------ src/workflows/review-target-selection.ts | 67 ++++++++++++++++-------- test/workflow-run-plan.test.ts | 26 +++++++++ 3 files changed, 105 insertions(+), 38 deletions(-) diff --git a/bin/pushgate.mjs b/bin/pushgate.mjs index 5b31152..16cc9da 100755 --- a/bin/pushgate.mjs +++ b/bin/pushgate.mjs @@ -28567,6 +28567,7 @@ function closeFd(fd) { // src/workflows/review-target-selection.ts var REVIEW_TARGET_CONFIG_KEY = "pushgate.review-target"; var MAX_STACKED_CANDIDATES = 3; +var MAX_STACKED_DISTANCE_CANDIDATES = 25; var ZERO_OBJECT = /^0+$/; var ReviewTargetSelectionError = class extends Error { constructor(message) { @@ -28575,6 +28576,11 @@ var ReviewTargetSelectionError = class extends Error { } }; async function selectReviewTarget(options) { + if (options.hookContext.branchUpdates.length > 1) { + throw new ReviewTargetSelectionError( + "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time." + ); + } const overrideRef = await readGitStringConfig( options.repoRoot, REVIEW_TARGET_CONFIG_KEY, @@ -28592,11 +28598,6 @@ async function selectReviewTarget(options) { source: "override" }; } - if (options.hookContext.branchUpdates.length > 1) { - throw new ReviewTargetSelectionError( - "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time." - ); - } if (!discovery.promptRequired) { const configured = discovery.candidates.find( (candidate) => candidate.source === "configured" @@ -28868,13 +28869,15 @@ async function incrementalCandidateFromRemoteTrackingRef(options) { async function findStackedCandidates(options) { const result = await runGit(options.repoRoot, [ "for-each-ref", + "--merged=HEAD", + "--sort=-committerdate", "--format=%(refname:short)%00%(objectname)", "refs/remotes" ]); if (result.code !== 0) { return []; } - const candidates = []; + const ancestorCandidates = []; for (const line of result.stdout.split("\n")) { if (!line.trim()) { continue; @@ -28883,23 +28886,36 @@ async function findStackedCandidates(options) { if (!ref || !commit || ref.endsWith("/HEAD") || ref === options.currentRemoteRef || ref === options.targetRemoteRef || isZeroObjectName(commit)) { continue; } - if (!await isAncestor(options.repoRoot, commit, "HEAD")) { - continue; - } - const distance = await commitDistance(options.repoRoot, commit, "HEAD"); - if (distance === null || distance === 0) { - continue; - } - candidates.push({ + ancestorCandidates.push({ commit, - detail: `${String(distance)} commit(s) behind HEAD`, - distance, label: ref, ref, source: "stacked" }); + if (ancestorCandidates.length >= MAX_STACKED_DISTANCE_CANDIDATES) { + break; + } } - return candidates.sort((left, right) => left.distance - right.distance).slice(0, MAX_STACKED_CANDIDATES).map(({ distance: _distance, ...candidate }) => candidate); + const candidatesWithDistance = await Promise.all( + ancestorCandidates.map(async (candidate) => { + const distance = await commitDistance( + options.repoRoot, + candidate.commit ?? candidate.ref, + "HEAD" + ); + if (distance === null || distance === 0) { + return null; + } + return { + ...candidate, + detail: `${String(distance)} commit(s) behind HEAD`, + distance + }; + }) + ); + return candidatesWithDistance.filter( + (candidate) => candidate !== null + ).sort((left, right) => left.distance - right.distance).slice(0, MAX_STACKED_CANDIDATES).map(({ distance: _distance, ...candidate }) => candidate); } function recommendedCandidate(options) { const candidates = options.candidates.filter( diff --git a/src/workflows/review-target-selection.ts b/src/workflows/review-target-selection.ts index 23752d4..a3345e5 100644 --- a/src/workflows/review-target-selection.ts +++ b/src/workflows/review-target-selection.ts @@ -64,9 +64,15 @@ interface CandidateWithCommit extends ReviewTargetCandidate { commit?: string; } +interface StackedCandidateWithDistance extends CandidateWithCommit { + detail: string; + distance: number; +} + type TargetFreshness = "ahead" | "behind" | "diverged" | "missing" | "same"; const MAX_STACKED_CANDIDATES = 3; +const MAX_STACKED_DISTANCE_CANDIDATES = 25; const ZERO_OBJECT = /^0+$/; export class ReviewTargetSelectionError extends Error { @@ -79,6 +85,12 @@ export class ReviewTargetSelectionError extends Error { export async function selectReviewTarget( options: SelectReviewTargetOptions, ): Promise { + if (options.hookContext.branchUpdates.length > 1) { + throw new ReviewTargetSelectionError( + "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time.", + ); + } + const overrideRef = await readGitStringConfig( options.repoRoot, REVIEW_TARGET_CONFIG_KEY, @@ -99,12 +111,6 @@ export async function selectReviewTarget( }; } - if (options.hookContext.branchUpdates.length > 1) { - throw new ReviewTargetSelectionError( - "Pushgate cannot choose one review target for a push that updates multiple branches. Push one branch at a time.", - ); - } - if (!discovery.promptRequired) { const configured = discovery.candidates.find( (candidate) => candidate.source === "configured", @@ -491,6 +497,8 @@ async function findStackedCandidates(options: { }): Promise { const result = await runGit(options.repoRoot, [ "for-each-ref", + "--merged=HEAD", + "--sort=-committerdate", "--format=%(refname:short)%00%(objectname)", "refs/remotes", ]); @@ -499,7 +507,7 @@ async function findStackedCandidates(options: { return []; } - const candidates: Array = []; + const ancestorCandidates: CandidateWithCommit[] = []; for (const line of result.stdout.split("\n")) { if (!line.trim()) { @@ -519,27 +527,44 @@ async function findStackedCandidates(options: { continue; } - if (!(await isAncestor(options.repoRoot, commit, "HEAD"))) { - continue; - } - - const distance = await commitDistance(options.repoRoot, commit, "HEAD"); - - if (distance === null || distance === 0) { - continue; - } - - candidates.push({ + ancestorCandidates.push({ commit, - detail: `${String(distance)} commit(s) behind HEAD`, - distance, label: ref, ref, source: "stacked", }); + + if (ancestorCandidates.length >= MAX_STACKED_DISTANCE_CANDIDATES) { + break; + } } - return candidates + const candidatesWithDistance: Array = + await Promise.all( + ancestorCandidates.map(async (candidate) => { + const distance = await commitDistance( + options.repoRoot, + candidate.commit ?? candidate.ref, + "HEAD", + ); + + if (distance === null || distance === 0) { + return null; + } + + return { + ...candidate, + detail: `${String(distance)} commit(s) behind HEAD`, + distance, + }; + }), + ); + + return candidatesWithDistance + .filter( + (candidate): candidate is StackedCandidateWithDistance => + candidate !== null, + ) .sort((left, right) => left.distance - right.distance) .slice(0, MAX_STACKED_CANDIDATES) .map(({ distance: _distance, ...candidate }) => candidate); diff --git a/test/workflow-run-plan.test.ts b/test/workflow-run-plan.test.ts index 873971d..7b3c158 100644 --- a/test/workflow-run-plan.test.ts +++ b/test/workflow-run-plan.test.ts @@ -306,6 +306,32 @@ test("multi-branch pushes fail before choosing one review target", async () => { }); }); +test("multi-branch pushes fail even when review target override is set", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]); + await checkedRun("git", ["config", "pushgate.review-target", "main"], { + cwd: repoRoot, + }); + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + stdin: Readable.from( + [ + `refs/heads/feature ${commits.head} refs/heads/feature ${commits.remoteFeature}`, + `refs/heads/other ${commits.head} refs/heads/other ${ZERO_OBJECT}`, + "", + ].join("\n"), + ), + }); + + assert.equal(result.code, 1, formatResult(result)); + assert.match(result.stdout, /updates multiple branches/); + assert.match(result.stdout, /Push one branch at a time/); + assert.doesNotMatch(result.stdout, /Review target:\s+main/); + assert.equal(result.stderr, ""); + }); +}); + interface WorkflowResult { code: number; stderr: string; From 84c84627bf2dfc1c46fac266e7e9948c783d7ae4 Mon Sep 17 00:00:00 2001 From: dbrosio3 Date: Thu, 13 Aug 2026 12:57:46 -0300 Subject: [PATCH 4/4] Address review target selection edge cases --- bin/pushgate.mjs | 38 ++++++++++++++---------- src/workflows/review-target-selection.ts | 36 ++++++++++++---------- src/workflows/terminal.ts | 4 +++ test/terminal.test.ts | 13 ++++++++ test/workflow-run-plan.test.ts | 27 +++++++++++++++++ 5 files changed, 88 insertions(+), 30 deletions(-) diff --git a/bin/pushgate.mjs b/bin/pushgate.mjs index 16cc9da..1291a03 100755 --- a/bin/pushgate.mjs +++ b/bin/pushgate.mjs @@ -28373,6 +28373,9 @@ function readChoiceKey(fd) { return { kind: "down" }; } } + if (second !== null && second !== "[" && second !== "O") { + pendingInputByFd.set(fd, second); + } } return { kind: "ignored" }; } @@ -28568,6 +28571,7 @@ function closeFd(fd) { var REVIEW_TARGET_CONFIG_KEY = "pushgate.review-target"; var MAX_STACKED_CANDIDATES = 3; var MAX_STACKED_DISTANCE_CANDIDATES = 25; +var FULL_OBJECT_NAME = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i; var ZERO_OBJECT = /^0+$/; var ReviewTargetSelectionError = class extends Error { constructor(message) { @@ -28590,6 +28594,11 @@ async function selectReviewTarget(options) { const discovery = await discoverReviewTargets(options); options.onDiagnostics?.(discovery.diagnostics); if (overrideRef) { + if (!await resolveCommit(options.repoRoot, overrideRef)) { + throw new ReviewTargetSelectionError( + `One-push override ${REVIEW_TARGET_CONFIG_KEY}="${overrideRef}" cannot be resolved locally. Check the ref and retry.` + ); + } return { diagnostics: discovery.diagnostics, label: overrideRef, @@ -28733,22 +28742,19 @@ async function discoverReviewTargets(options) { pushRemote: options.hookContext.remote, targetRemoteRef: resolvedTargetRemote?.ref }); - const candidates = dedupeCandidatesByCommit([ + const candidatePool = [ configuredTarget, resolvedTargetRemote, incremental, ...stacked - ]).map((candidate) => ({ + ]; + const recommended = recommendedCandidate({ + candidates: candidatePool, + freshness + }); + const candidates = dedupeCandidatesByCommit(candidatePool).map((candidate) => ({ ...candidate, - recommended: candidate === recommendedCandidate({ - candidates: [ - configuredTarget, - resolvedTargetRemote, - incremental, - ...stacked - ], - freshness - }) + recommended: candidate === recommended })); return { candidates, @@ -28797,8 +28803,10 @@ async function compareCommits(repoRoot, localCommit, remoteCommit) { if (localCommit === remoteCommit) { return "same"; } - const localIsAncestor = await isAncestor(repoRoot, localCommit, remoteCommit); - const remoteIsAncestor = await isAncestor(repoRoot, remoteCommit, localCommit); + const [localIsAncestor, remoteIsAncestor] = await Promise.all([ + isAncestor(repoRoot, localCommit, remoteCommit), + isAncestor(repoRoot, remoteCommit, localCommit) + ]); if (localIsAncestor) { return "behind"; } @@ -28985,7 +28993,7 @@ async function resolveCurrentBranch(repoRoot) { return result.code === 0 ? result.stdout.trim() : void 0; } function isSimpleBranchName(ref) { - return !ref.startsWith("refs/") && !ref.includes("..") && !ref.includes("@{") && !ref.includes(":") && !/^[0-9a-f]{40}$/i.test(ref); + return !ref.startsWith("refs/") && !ref.includes("..") && !ref.includes("@{") && !ref.includes(":") && !FULL_OBJECT_NAME.test(ref); } function isRemoteRefForPushRemote(ref, pushRemote) { return ref === pushRemote || ref.startsWith(`${pushRemote}/`) || ref.startsWith("refs/remotes/"); @@ -28994,7 +29002,7 @@ function isZeroObjectName(value) { return ZERO_OBJECT.test(value); } function isLikelyObjectName(value) { - return /^[0-9a-f]{40,64}$/i.test(value); + return FULL_OBJECT_NAME.test(value); } function fetchTip(remote, targetRef) { const fetchCommand = remote ? `git fetch ${remote}` : "git fetch"; diff --git a/src/workflows/review-target-selection.ts b/src/workflows/review-target-selection.ts index a3345e5..4e4aecd 100644 --- a/src/workflows/review-target-selection.ts +++ b/src/workflows/review-target-selection.ts @@ -73,6 +73,7 @@ type TargetFreshness = "ahead" | "behind" | "diverged" | "missing" | "same"; const MAX_STACKED_CANDIDATES = 3; const MAX_STACKED_DISTANCE_CANDIDATES = 25; +const FULL_OBJECT_NAME = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i; const ZERO_OBJECT = /^0+$/; export class ReviewTargetSelectionError extends Error { @@ -102,6 +103,12 @@ export async function selectReviewTarget( options.onDiagnostics?.(discovery.diagnostics); if (overrideRef) { + if (!(await resolveCommit(options.repoRoot, overrideRef))) { + throw new ReviewTargetSelectionError( + `One-push override ${REVIEW_TARGET_CONFIG_KEY}="${overrideRef}" cannot be resolved locally. Check the ref and retry.`, + ); + } + return { diagnostics: discovery.diagnostics, label: overrideRef, @@ -296,22 +303,19 @@ async function discoverReviewTargets(options: SelectReviewTargetOptions): Promis targetRemoteRef: resolvedTargetRemote?.ref, }); - const candidates = dedupeCandidatesByCommit([ + const candidatePool = [ configuredTarget, resolvedTargetRemote, incremental, ...stacked, - ]).map((candidate) => ({ + ]; + const recommended = recommendedCandidate({ + candidates: candidatePool, + freshness, + }); + const candidates = dedupeCandidatesByCommit(candidatePool).map((candidate) => ({ ...candidate, - recommended: candidate === recommendedCandidate({ - candidates: [ - configuredTarget, - resolvedTargetRemote, - incremental, - ...stacked, - ], - freshness, - }), + recommended: candidate === recommended, })); return { @@ -388,8 +392,10 @@ async function compareCommits( return "same"; } - const localIsAncestor = await isAncestor(repoRoot, localCommit, remoteCommit); - const remoteIsAncestor = await isAncestor(repoRoot, remoteCommit, localCommit); + const [localIsAncestor, remoteIsAncestor] = await Promise.all([ + isAncestor(repoRoot, localCommit, remoteCommit), + isAncestor(repoRoot, remoteCommit, localCommit), + ]); if (localIsAncestor) { return "behind"; @@ -690,7 +696,7 @@ function isSimpleBranchName(ref: string): boolean { !ref.includes("..") && !ref.includes("@{") && !ref.includes(":") && - !/^[0-9a-f]{40}$/i.test(ref) + !FULL_OBJECT_NAME.test(ref) ); } @@ -707,7 +713,7 @@ function isZeroObjectName(value: string): boolean { } function isLikelyObjectName(value: string): boolean { - return /^[0-9a-f]{40,64}$/i.test(value); + return FULL_OBJECT_NAME.test(value); } function fetchTip(remote: string | undefined, targetRef: string): string { diff --git a/src/workflows/terminal.ts b/src/workflows/terminal.ts index 4652939..b633cae 100644 --- a/src/workflows/terminal.ts +++ b/src/workflows/terminal.ts @@ -289,6 +289,10 @@ function readChoiceKey(fd: number): ChoiceKey { return { kind: "down" }; } } + + if (second !== null && second !== "[" && second !== "O") { + pendingInputByFd.set(fd, second); + } } return { kind: "ignored" }; diff --git a/test/terminal.test.ts b/test/terminal.test.ts index 737b36c..7f0b0ce 100644 --- a/test/terminal.test.ts +++ b/test/terminal.test.ts @@ -79,6 +79,19 @@ test("interactive terminal choice prompt still accepts numeric selection", async assert.equal(selected, 2); }); +test("interactive terminal preserves input typed immediately after escape", async () => { + let selected = -1; + + await withTerminalInput("\x1B2\n", (terminal) => { + selected = terminal.choose?.("Choose review target", [ + { label: "main" }, + { label: "origin/main" }, + ]) ?? -1; + }); + + assert.equal(selected, 1); +}); + async function withTempDir( callback: (tempDir: string) => Promise, ): Promise { diff --git a/test/workflow-run-plan.test.ts b/test/workflow-run-plan.test.ts index 7b3c158..fe899c2 100644 --- a/test/workflow-run-plan.test.ts +++ b/test/workflow-run-plan.test.ts @@ -284,6 +284,33 @@ test("one-push review target override skips interactive selection", async () => }); }); +test("one-push review target override reports an invalid ref explicitly", async () => { + await withIncrementalPushRepo(async (repoRoot, commits) => { + await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]); + await checkedRun( + "git", + ["config", "pushgate.review-target", "does-not-exist"], + { cwd: repoRoot }, + ); + + const result = await runWorkflowInRepo(repoRoot, { + hookArgs: ["origin", "git@example.test:repo.git"], + reviewTargetSelector: async () => { + throw new Error("override should skip review target selection"); + }, + stdin: Readable.from( + `refs/heads/feature ${commits.head} refs/heads/feature ${commits.remoteFeature}\n`, + ), + }); + + assert.equal(result.code, 1, formatResult(result)); + assert.match(result.stdout, /pushgate\.review-target="does-not-exist"/); + assert.match(result.stdout, /cannot be resolved locally/); + assert.doesNotMatch(result.stdout, /Configured review\.target_branch/); + assert.equal(result.stderr, ""); + }); +}); + test("multi-branch pushes fail before choosing one review target", async () => { await withIncrementalPushRepo(async (repoRoot, commits) => { await writeChangedFilesAssertionConfig(repoRoot, ["src/two.ts"]);