diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d3573541f..e3a1f4ff6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -17,6 +17,10 @@ ### Fixed - Fixed `omp commit` exiting 0 when the commit agent failed and the mechanical fallback wrote the commit: the command now exits non-zero when the fallback was used, so callers can distinguish a degraded numstat commit from a legitimate single-commit decision ([#7835](https://github.com/can1357/oh-my-pi/issues/7835)). +### 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 diff --git a/packages/coding-agent/src/commands/commit.ts b/packages/coding-agent/src/commands/commit.ts index b8d1bf8e0..037f90c68 100644 --- a/packages/coding-agent/src/commands/commit.ts +++ b/packages/coding-agent/src/commands/commit.ts @@ -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,11 +41,20 @@ 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). - const { usedFallback } = await runCommitCommand(cmd); - // A commit written by the mechanical fallback is a degraded outcome: - // the agent did not do the work it was asked to do, so the command - // reports non-zero so callers can distinguish it from a real - // single-commit decision (issue #7835). - await postmortem.quit(usedFallback ? 1 : 0); + let exitCode = 0; + try { + const { usedFallback } = await runCommitCommand(cmd); + // A commit written by the mechanical fallback is a degraded outcome: + // the agent did not do the work it was asked to do, so the command + // reports non-zero so callers can distinguish it from a real + // single-commit decision (issue #7835). + if (usedFallback) exitCode = 1; + } 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); } } diff --git a/packages/coding-agent/src/commit/agentic/index.ts b/packages/coding-agent/src/commit/agentic/index.ts index 8e68ec3d4..e27e8ca1d 100644 --- a/packages/coding-agent/src/commit/agentic/index.ts +++ b/packages/coding-agent/src/commit/agentic/index.ts @@ -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<{ usedF ); 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 { usedFallback: false }; } @@ -239,12 +245,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( @@ -297,7 +305,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 = { @@ -307,14 +315,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 { diff --git a/packages/coding-agent/src/commit/execute.ts b/packages/coding-agent/src/commit/execute.ts new file mode 100644 index 000000000..96c4bb09a --- /dev/null +++ b/packages/coding-agent/src/commit/execute.ts @@ -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 { + 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"); +} diff --git a/packages/coding-agent/src/commit/index.ts b/packages/coding-agent/src/commit/index.ts index 1bdf6a5b8..043a6d01e 100644 --- a/packages/coding-agent/src/commit/index.ts +++ b/packages/coding-agent/src/commit/index.ts @@ -2,4 +2,5 @@ * Entry points for the omp commit command. */ -export { runCommitCommand } from "./pipeline"; +export * from "./execute"; +export * from "./pipeline"; diff --git a/packages/coding-agent/src/commit/pipeline.ts b/packages/coding-agent/src/commit/pipeline.ts index 35611539f..38a4312ec 100644 --- a/packages/coding-agent/src/commit/pipeline.ts +++ b/packages/coding-agent/src/commit/pipeline.ts @@ -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"; @@ -70,6 +71,11 @@ async function runLegacyCommitCommand(args: CommitCommandArgs): Promise { 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; } @@ -135,12 +141,14 @@ async function runLegacyCommitCommand(args: CommitCommandArgs): Promise { 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: { diff --git a/packages/coding-agent/test/commit-command-exit.test.ts b/packages/coding-agent/test/commit-command-exit.test.ts index f50f55f60..aad0451c2 100644 --- a/packages/coding-agent/test/commit-command-exit.test.ts +++ b/packages/coding-agent/test/commit-command-exit.test.ts @@ -70,4 +70,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); + }); }); diff --git a/packages/coding-agent/test/commit-execute.test.ts b/packages/coding-agent/test/commit-execute.test.ts new file mode 100644 index 000000000..39f5b1449 --- /dev/null +++ b/packages/coding-agent/test/commit-execute.test.ts @@ -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 { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +async function runGit(cwd: string, args: string[]): Promise { + 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 { + 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 { + 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); + }); +});