fix(commit): report hook refusals cleanly and honor --push on a clean tree

git.commit() was awaited without a try in both agentic commit routes and the legacy pipeline, so a refusing pre-commit/commit-msg hook escaped as an uncaught GitCommandError. In a bundled build Bun renders that as kilobytes of minified source. The split loop also threw out of its order loop, reporting no progress.

The empty-staged-tree early return fired before args.push was read, so 'omp commit --push' on a clean tree exited 0 without pushing.

Route commit/push failures through a shared execute helper: abortOnGitFailure prints the hook's own message (plus split-plan progress) and throws a CommitAbortedError the command maps to exit 1; pushOrAbort pushes existing commits when the tree is clean and reports refused pushes cleanly.

Fixes #7834
This commit is contained in:
roboomp
2026-08-06 13:26:51 +00:00
parent 3a8591a8af
commit 8ca2230675
8 changed files with 252 additions and 20 deletions
+5
View File
@@ -2,6 +2,11 @@
## [Unreleased]
### Fixed
- Fixed `omp commit` printing a wall of bundled source when a `pre-commit`/`commit-msg` hook refuses a commit: hook failures are now reported with the hook's own message, split plans report how far they got, and the command exits non-zero cleanly ([#7834](https://github.com/can1357/oh-my-pi/issues/7834)).
- Fixed `omp commit --push` exiting 0 without pushing when the working tree is already clean; it now pushes the existing commits (or fails non-zero if the push is refused) ([#7834](https://github.com/can1357/oh-my-pi/issues/7834)).
## [17.2.10] - 2026-08-06
### Breaking Changes
+11 -3
View File
@@ -5,7 +5,7 @@
import { postmortem } from "@oh-my-pi/pi-utils";
import { Command, Flags } from "@oh-my-pi/pi-utils/cli";
import { commitHelp as commandHelp } from "../cli/command-help";
import { runCommitCommand } from "../commit";
import { CommitAbortedError, runCommitCommand } from "../commit";
import type { CommitCommandArgs } from "../commit/types";
import { initTheme } from "../modes/theme/theme";
@@ -41,7 +41,15 @@ export default class Commit extends Command {
// is already written. Mirror the `runPrintMode` exit pattern from
// `main.ts` so the CLI returns to the shell instead of stranding the user
// on Ctrl+C (issue #1041).
await runCommitCommand(cmd);
await postmortem.quit(0);
let exitCode = 0;
try {
await runCommitCommand(cmd);
} catch (error) {
if (!(error instanceof CommitAbortedError)) throw error;
// Failure already reported with a readable message; exit non-zero
// without letting the runtime dump a stack/minified-source blob.
exitCode = 1;
}
await postmortem.quit(exitCode);
}
}
@@ -11,6 +11,7 @@ import { ModelRegistry } from "../../config/model-registry";
import { Settings } from "../../config/settings";
import { discoverAuthStorage, discoverContextFiles, loadCliExtensionProviders } from "../../sdk";
import * as git from "../../utils/git";
import { abortOnGitFailure, pushOrAbort } from "../execute";
import { type ExistingChangelogEntries, runCommitAgentSession } from "./agent";
import { generateFallbackProposal } from "./fallback";
import { assignLockFilesToPlan } from "./lock-files";
@@ -56,6 +57,11 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
);
if (stagedFiles.length === 0) {
if (args.push) {
process.stdout.write("No changes to commit; pushing existing commits...\n");
await pushOrAbort(cwd);
return;
}
process.stderr.write("No changes to commit.\n");
return;
}
@@ -238,12 +244,14 @@ async function runSingleCommit(proposal: CommitProposal, ctx: CommitExecutionCon
return;
}
process.stdout.write("● Creating commit...\n");
await git.commit(ctx.cwd, commitMessage);
process.stdout.write("Commit created.\n");
if (ctx.push) {
await git.push(ctx.cwd);
process.stdout.write("Pushed to remote.\n");
try {
await git.commit(ctx.cwd, commitMessage);
} catch (error) {
if (error instanceof git.GitCommandError) abortOnGitFailure("Commit failed", error);
throw error;
}
process.stdout.write("Commit created.\n");
if (ctx.push) await pushOrAbort(ctx.cwd);
}
async function runSplitCommit(
@@ -296,7 +304,7 @@ async function runSplitCommit(
process.stdout.write("● Creating split commits...\n");
const stagedDiff = await git.diff(ctx.cwd, { cached: true, binary: true });
await git.stage.reset(ctx.cwd);
for (const commitIndex of order) {
for (const [position, commitIndex] of order.entries()) {
const commit = plan.commits[commitIndex];
await git.stage.hunks(ctx.cwd, commit.changes, { rawDiff: stagedDiff, diffCached: true });
const analysis: ConventionalAnalysis = {
@@ -306,14 +314,23 @@ async function runSplitCommit(
issueRefs: commit.issueRefs,
};
const message = formatCommitMessage(analysis, commit.summary);
await git.commit(ctx.cwd, message);
try {
await git.commit(ctx.cwd, message);
} catch (error) {
if (error instanceof git.GitCommandError) {
const stagedNow = await git.diff.changedFiles(ctx.cwd, { cached: true });
abortOnGitFailure(
`Commit ${position + 1} of ${order.length} failed`,
error,
`${position} of ${order.length} commits created; ${stagedNow.length} file(s) remain staged. No changes were lost.`,
);
}
throw error;
}
await git.stage.reset(ctx.cwd);
}
process.stdout.write("Split commits created.\n");
if (ctx.push) {
await git.push(ctx.cwd);
process.stdout.write("Pushed to remote.\n");
}
if (ctx.push) await pushOrAbort(ctx.cwd);
}
function appendFilesToLastCommit(plan: SplitCommitPlan, files: string[]): void {
@@ -0,0 +1,56 @@
/**
* Shared commit-execution helpers for the agentic and legacy `omp commit`
* pipelines: readable failure reporting for refusing git hooks and a push
* wrapper that keeps a requested `--push` honest.
*/
import * as git from "../utils/git";
/**
* A commit or push failure that has already been reported to the user with a
* readable message. It is thrown so the CLI exits non-zero without the runtime
* dumping a stack trace — in a bundled build that trace is several kilobytes of
* minified source, which buries the one line the user needs (issue #7834).
*
* `runCommitCommand`'s caller (`commands/commit.ts`) maps this to exit code 1.
*/
export class CommitAbortedError extends Error {
constructor() {
super("commit aborted");
this.name = "CommitAbortedError";
}
}
/**
* Print a git-command failure — most often a `pre-commit`/`commit-msg` hook
* that exited non-zero — as an indented message under `context`, then abort.
*
* @param context Label for the failed step, e.g. `"Commit 1 of 2 failed"`.
* @param error The failure to surface; its captured stderr/stdout is shown.
* @param note Optional trailing status line (split-plan progress, recovery).
*/
export function abortOnGitFailure(context: string, error: git.GitCommandError, note?: string): never {
const detail = error.result.stderr.trim() || error.result.stdout.trim() || error.message;
const body = detail
.split("\n")
.map(line => ` ${line}`)
.join("\n");
process.stderr.write(`✗ ${context}:\n${body}\n`);
if (note) process.stderr.write(` ${note}\n`);
throw new CommitAbortedError();
}
/**
* Push the current branch, reporting a refused push (missing upstream, rejected
* ref) through {@link abortOnGitFailure} instead of letting the raw
* `GitCommandError` escape. Prints the success line on completion.
*/
export async function pushOrAbort(cwd: string): Promise<void> {
try {
await git.push(cwd);
} catch (error) {
if (error instanceof git.GitCommandError) abortOnGitFailure("Push failed", error);
throw error;
}
process.stdout.write("Pushed to remote.\n");
}
+2 -1
View File
@@ -2,4 +2,5 @@
* Entry points for the omp commit command.
*/
export { runCommitCommand } from "./pipeline";
export * from "./execute";
export * from "./pipeline";
+13 -5
View File
@@ -16,6 +16,7 @@ import {
validateSummary,
} from "./analysis";
import { runChangelogFlow } from "./changelog";
import { abortOnGitFailure, pushOrAbort } from "./execute";
import { runMapReduceAnalysis, shouldUseMapReduce } from "./map-reduce";
import { formatCommitMessage } from "./message";
import { resolvePrimaryModel, resolveSmolModel } from "./model-selection";
@@ -65,6 +66,11 @@ async function runLegacyCommitCommand(args: CommitCommandArgs): Promise<void> {
stagedFiles = await git.diff.changedFiles(cwd, { cached: true });
}
if (stagedFiles.length === 0) {
if (args.push) {
process.stdout.write("No changes to commit; pushing existing commits...\n");
await pushOrAbort(cwd);
return;
}
process.stderr.write("No changes to commit.\n");
return;
}
@@ -130,12 +136,14 @@ async function runLegacyCommitCommand(args: CommitCommandArgs): Promise<void> {
return;
}
await git.commit(cwd, commitMessage);
process.stdout.write("Commit created.\n");
if (args.push) {
await git.push(cwd);
process.stdout.write("Pushed to remote.\n");
try {
await git.commit(cwd, commitMessage);
} catch (error) {
if (error instanceof git.GitCommandError) abortOnGitFailure("Commit failed", error);
throw error;
}
process.stdout.write("Commit created.\n");
if (args.push) await pushOrAbort(cwd);
}
async function generateAnalysis(input: {
@@ -52,4 +52,22 @@ describe("omp commit command lifecycle (issue #1041)", () => {
expect(runCommitSpy).toHaveBeenCalledTimes(1);
expect(quitSpy).not.toHaveBeenCalled();
});
it("maps CommitAbortedError to exit code 1 without rethrowing (issue #7834)", async () => {
vi.spyOn(themeModule, "initTheme").mockResolvedValue(undefined);
vi.spyOn(commitModule, "runCommitCommand").mockRejectedValue(new commitModule.CommitAbortedError());
const quitSpy = vi.spyOn(postmortem, "quit").mockResolvedValue(undefined);
const command = new CommitCommand([], {
bin: "omp",
version: "0.0.0-test",
commands: new Map(),
});
// A hook refusal is already reported with a readable message; the command
// must exit non-zero rather than let the runtime dump the error.
await command.run();
expect(quitSpy).toHaveBeenCalledWith(1);
});
});
@@ -0,0 +1,119 @@
import { afterEach, describe, expect, it, vi } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import { removeWithRetries } from "@oh-my-pi/pi-utils";
import { abortOnGitFailure, CommitAbortedError, pushOrAbort } from "../src/commit/execute";
import * as git from "../src/utils/git";
const tempDirs: string[] = [];
async function mkTempDir(prefix: string): Promise<string> {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
tempDirs.push(dir);
return dir;
}
async function runGit(cwd: string, args: string[]): Promise<void> {
const env = { ...process.env, HOME: cwd, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null" };
const proc = Bun.spawn(["git", "-C", cwd, ...args], { env, stdout: "ignore", stderr: "pipe" });
const code = await proc.exited;
if (code !== 0) {
const stderr = await new Response(proc.stderr).text();
throw new Error(`git ${args.join(" ")} failed (${code}): ${stderr}`);
}
}
async function initRepoWithCommit(dir: string): Promise<void> {
await runGit(dir, ["init", "-q", "-b", "main"]);
await runGit(dir, ["config", "user.email", "test@example.com"]);
await runGit(dir, ["config", "user.name", "Test"]);
await fs.writeFile(path.join(dir, "a.txt"), "one\n");
await runGit(dir, ["add", "."]);
await runGit(dir, ["commit", "-q", "-m", "seed"]);
}
async function revParse(cwd: string, rev: string): Promise<string> {
const env = { ...process.env, HOME: cwd, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null" };
const proc = Bun.spawn(["git", "-C", cwd, "rev-parse", rev], { env, stdout: "pipe", stderr: "ignore" });
const [out, code] = await Promise.all([new Response(proc.stdout).text(), proc.exited]);
if (code !== 0) throw new Error(`git rev-parse ${rev} failed`);
return out.trim();
}
afterEach(async () => {
vi.restoreAllMocks();
await Promise.all(tempDirs.splice(0).map(dir => removeWithRetries(dir)));
});
describe("abortOnGitFailure (issue #7834)", () => {
it("surfaces a refusing hook's message and aborts with a sentinel instead of the raw error", async () => {
const dir = await mkTempDir("omp-commit-hook-");
await initRepoWithCommit(dir);
const hook = path.join(dir, ".git", "hooks", "pre-commit");
await fs.writeFile(hook, '#!/bin/sh\necho "policy: this change is not allowed" >&2\nexit 1\n');
await fs.chmod(hook, 0o755);
await fs.appendFile(path.join(dir, "a.txt"), "two\n");
await runGit(dir, ["add", "-A"]);
// A refused commit yields a GitCommandError from the central git wrapper;
// this is the input both commit routes hand to abortOnGitFailure.
let commitError: unknown;
try {
await git.commit(dir, "feat: x");
} catch (error) {
commitError = error;
}
expect(commitError).toBeInstanceOf(git.GitCommandError);
const stderrSpy = vi.spyOn(process.stderr, "write").mockReturnValue(true);
expect(() =>
abortOnGitFailure(
"Commit 1 of 2 failed",
commitError as git.GitCommandError,
"0 of 2 commits created; 1 file(s) remain staged. No changes were lost.",
),
).toThrow(CommitAbortedError);
const printed = stderrSpy.mock.calls.map(call => String(call[0])).join("");
expect(printed).toContain("Commit 1 of 2 failed:");
expect(printed).toContain("policy: this change is not allowed");
expect(printed).toContain("0 of 2 commits created; 1 file(s) remain staged. No changes were lost.");
// The readable message must not carry a stack trace / source dump.
expect(printed).not.toContain("at ");
});
});
describe("pushOrAbort (issue #7834)", () => {
it("pushes existing commits when the working tree is clean", async () => {
const root = await mkTempDir("omp-push-clean-");
const bare = path.join(root, "remote.git");
await runGit(root, ["init", "-q", "--bare", bare]);
const work = path.join(root, "work");
await fs.mkdir(work);
await initRepoWithCommit(work);
await runGit(work, ["remote", "add", "origin", bare]);
await runGit(work, ["push", "-q", "-u", "origin", "main"]);
// A local commit that never made it to the remote — the defect-2 scenario.
await fs.appendFile(path.join(work, "a.txt"), "two\n");
await runGit(work, ["add", "a.txt"]);
await runGit(work, ["commit", "-q", "-m", "local only"]);
const localHead = await revParse(work, "HEAD");
expect(await revParse(bare, "main")).not.toBe(localHead);
vi.spyOn(process.stdout, "write").mockReturnValue(true);
await pushOrAbort(work);
expect(await revParse(bare, "main")).toBe(localHead);
});
it("aborts with a sentinel when the branch has no upstream", async () => {
const dir = await mkTempDir("omp-push-noupstream-");
await initRepoWithCommit(dir);
vi.spyOn(process.stderr, "write").mockReturnValue(true);
await expect(pushOrAbort(dir)).rejects.toBeInstanceOf(CommitAbortedError);
});
});