fix(coding-agent): signalled fallback commits with a non-zero exit code

2026-08-06 23:03:25 +08:00
parent 3a8591a8af
commit ec0253effd
7 changed files with 161 additions and 17 deletions
@@ -44,6 +44,8 @@
 - Fixed parsing of POSIX `$EDITOR` commands that contain quoted arguments or executable paths with spaces.
 - Fixed persisted Agent Hub rows losing the explicit caller model role when a subagent used a model override, preserving role provenance across restarts.
 - Fixed unobserved promise rejections in browser helpers (such as `tab.waitForResponse()`) causing tab workers to hang or crash.
+- Fixed the bundled `ts-no-tiny-functions` TTSR rule never firing on one-line arrow functions in real files: the second alternative's `$` anchor only matched at the absolute end of input, so the trailing newline present in every real file suppressed the match. The condition now opens with the `(?m)` inline flag so the arrow body matches to the line end ([#6890](https://github.com/can1357/oh-my-pi/issues/6890)).
+- 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)).

 ## [17.2.9] - 2026-08-05

@@ -41,7 +41,11 @@ 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);
+		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);
 	}
 }
@@ -25,7 +25,7 @@ interface CommitExecutionContext {
 	push: boolean;
 }

-export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
+export async function runAgenticCommit(args: CommitCommandArgs): Promise<{ usedFallback: boolean }> {
 	const cwd = getProjectDir();
 	const [settings, authStorage] = await Promise.all([Settings.init({ cwd }), discoverAuthStorage()]);

@@ -57,7 +57,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {

 	if (stagedFiles.length === 0) {
 		process.stderr.write("No changes to commit.\n");
-		return;
+		return { usedFallback: false };
 	}

 	if (!args.noChangelog) {
@@ -94,7 +94,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
 		process.stdout.write("● Forcing fallback commit generation...\n");
 		const fallbackProposal = generateFallbackProposal(numstat);
 		await runSingleCommit(fallbackProposal, { cwd, dryRun: args.dryRun, push: args.push });
-		return;
+		return { usedFallback: true };
 	}

 	const trivialChange = detectTrivialChange(diff);
@@ -111,7 +111,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
 			warnings: [],
 		};
 		await runSingleCommit(trivialProposal, { cwd, dryRun: args.dryRun, push: args.push });
-		return;
+		return { usedFallback: false };
 	}

 	let existingChangelogEntries: ExistingChangelogEntries[] | undefined;
@@ -124,6 +124,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {

 	process.stdout.write("● Starting commit agent...\n");
 	let agentSessionCompleted = false;
+	let usedFallback = false;

 	try {
 		await runCommitAgentSession({
@@ -141,7 +142,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
 			existingChangelogEntries,
 			onComplete: async commitState => {
 				agentSessionCompleted = true;
-				await completeAgentCommitState(commitState, {
+				usedFallback = await completeAgentCommitState(commitState, {
 					cwd,
 					dryRun: args.dryRun,
 					push: args.push,
@@ -151,7 +152,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
 				});
 			},
 		});
-		return;
+		return { usedFallback };
 	} catch (error) {
 		if (agentSessionCompleted) {
 			throw error;
@@ -164,7 +165,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
 		process.stdout.write("● Using fallback commit generation...\n");
 		const fallbackProposal = generateFallbackProposal(numstat);
 		await runSingleCommit(fallbackProposal, { cwd, dryRun: args.dryRun, push: args.push });
-		return;
+		return { usedFallback: true };
 	}
 }

@@ -175,7 +176,7 @@ async function completeAgentCommitState(
 		changelogTargets: string[];
 		numstat: NumstatEntry[];
 	},
-): Promise<void> {
+): Promise<boolean> {
 	let usedFallback = false;
 	if (!commitState.proposal && !commitState.splitProposal) {
 		if ($env.PI_COMMIT_NO_FALLBACK?.toLowerCase() !== "true") {
@@ -211,7 +212,7 @@ async function completeAgentCommitState(

 	if (commitState.proposal) {
 		await runSingleCommit(commitState.proposal, ctx);
-		return;
+		return usedFallback;
 	}

 	if (commitState.splitProposal) {
@@ -221,7 +222,7 @@ async function completeAgentCommitState(
 			push: ctx.push,
 			additionalFiles: updatedChangelogFiles,
 		});
-		return;
+		return usedFallback;
 	}

 	throw new Error("Commit agent did not provide a proposal.");
@@ -30,10 +30,15 @@ const TYPES_DESCRIPTION = (): string => (typesDescription ??= prompt.render(type

 /**
 * Execute the omp commit pipeline for staged changes.
+ *
+ * Returns whether the commit was generated by the mechanical fallback rather
+ * than by the commit agent, so callers can signal a degraded outcome (issue
+ * #7835). The legacy pipeline has no fallback: model failures throw.
 */
-export async function runCommitCommand(args: CommitCommandArgs): Promise<void> {
+export async function runCommitCommand(args: CommitCommandArgs): Promise<{ usedFallback: boolean }> {
 	if (args.legacy) {
-		return runLegacyCommitCommand(args);
+		await runLegacyCommitCommand(args);
+		return { usedFallback: false };
 	}
 	return runAgenticCommit(args);
 }
@@ -11,7 +11,7 @@ describe("omp commit command lifecycle (issue #1041)", () => {

 	it("forces process exit after the commit pipeline resolves", async () => {
 		const initThemeSpy = vi.spyOn(themeModule, "initTheme").mockResolvedValue(undefined);
-		const runCommitSpy = vi.spyOn(commitModule, "runCommitCommand").mockResolvedValue(undefined);
+		const runCommitSpy = vi.spyOn(commitModule, "runCommitCommand").mockResolvedValue({ usedFallback: false });
 		// Stub postmortem.quit so it records the exit code without actually
 		// terminating the test runner. Resolves immediately — the production
 		// implementation never returns, but the contract under test is that
@@ -33,6 +33,24 @@ describe("omp commit command lifecycle (issue #1041)", () => {
 		expect(quitSpy).toHaveBeenCalledWith(0);
 	});

+	it("exits non-zero when the commit pipeline used the mechanical fallback", async () => {
+		const initThemeSpy = vi.spyOn(themeModule, "initTheme").mockResolvedValue(undefined);
+		const runCommitSpy = vi.spyOn(commitModule, "runCommitCommand").mockResolvedValue({ usedFallback: true });
+		const quitSpy = vi.spyOn(postmortem, "quit").mockResolvedValue(undefined);
+
+		const command = new CommitCommand([], {
+			bin: "omp",
+			version: "0.0.0-test",
+			commands: new Map(),
+		});
+
+		await command.run();
+
+		expect(initThemeSpy).toHaveBeenCalledTimes(1);
+		expect(runCommitSpy).toHaveBeenCalledTimes(1);
+		expect(quitSpy).toHaveBeenCalledWith(1);
+	});
+
 	it("does not convert commit pipeline failures into exit 0", async () => {
 		const initThemeSpy = vi.spyOn(themeModule, "initTheme").mockResolvedValue(undefined);
 		const runCommitSpy = vi
@@ -73,7 +73,7 @@ describe.serial("commit extension provider resolution", () => {
 				noChangelog: true,
 				model: SELECTOR,
 			}),
-		).resolves.toBeUndefined();
+		).resolves.toEqual({ usedFallback: false });
 	});

 	test("legacy pipeline resolves the project commit role from an extension provider", async () => {
@@ -84,6 +84,6 @@ describe.serial("commit extension provider resolution", () => {
 				noChangelog: true,
 				legacy: true,
 			}),
-		).resolves.toBeUndefined();
+		).resolves.toEqual({ usedFallback: false });
 	});
 });
@@ -0,0 +1,114 @@
+/**
+ * The fallback commit path (agent failed / no proposal) must be signalled to
+ * callers so a degraded numstat commit is distinguishable from a legitimate
+ * single-commit decision (issue #7835).
+ */
+import { afterEach, describe, expect, it, vi } from "bun:test";
+import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
+import { runAgenticCommit } from "@oh-my-pi/pi-coding-agent/commit/agentic";
+import * as agentModule from "@oh-my-pi/pi-coding-agent/commit/agentic/agent";
+import * as modelSelection from "@oh-my-pi/pi-coding-agent/commit/model-selection";
+import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
+import * as sdkModule from "@oh-my-pi/pi-coding-agent/sdk";
+import * as gitModule from "@oh-my-pi/pi-coding-agent/utils/git";
+
+const NUMSTAT = [{ path: "src/a.ts", additions: 1, deletions: 0 }];
+
+// ModelRegistry's constructor performs provider discovery and storage access;
+// the commit pipeline only needs a registry that resolves no models.
+vi.mock("@oh-my-pi/pi-coding-agent/config/model-registry", () => ({
+	ModelRegistry: class MockModelRegistry {
+		constructor(_authStorage: never) {}
+		async refresh() {}
+	},
+}));
+
+function mockModelResolution() {
+	const model = getBundledModel("anthropic", "claude-sonnet-4-5");
+	if (!model) throw new Error("Expected claude-sonnet-4-5 model to exist");
+	vi.spyOn(sdkModule, "loadCliExtensionProviders").mockResolvedValue(undefined);
+	vi.spyOn(modelSelection, "resolvePrimaryModel").mockResolvedValue({
+		model: { name: "test-primary", provider: "test", id: "test" } as never,
+		apiKey: "test-key",
+	});
+	vi.spyOn(modelSelection, "resolveSmolModel").mockResolvedValue({
+		model: { name: "test-smol", provider: "test", id: "test" } as never,
+		apiKey: "test-key",
+		thinkingLevel: undefined,
+	});
+	return model;
+}
+
+async function setupRepoMocks() {
+	vi.spyOn(Settings, "init").mockResolvedValue(Settings.isolated());
+	vi.spyOn(sdkModule, "discoverAuthStorage").mockResolvedValue({
+		setFallbackResolver: () => {},
+	} as never);
+	vi.spyOn(sdkModule, "discoverContextFiles").mockResolvedValue([]);
+	vi.spyOn(gitModule.diff, "changedFiles").mockResolvedValue(["src/a.ts"]);
+	vi.spyOn(gitModule.diff, "numstat").mockResolvedValue(NUMSTAT);
+	vi.spyOn(gitModule, "commit").mockResolvedValue({ stdout: "", stderr: "", exitCode: 0 } as never);
+}
+
+afterEach(() => {
+	vi.restoreAllMocks();
+	delete process.env.PI_COMMIT_TEST_FALLBACK;
+});
+
+describe("runAgenticCommit fallback signalling (issue #7835)", () => {
+	it("reports usedFallback when the fallback commit path is forced", async () => {
+		mockModelResolution();
+		await setupRepoMocks();
+		process.env.PI_COMMIT_TEST_FALLBACK = "true";
+
+		const result = await runAgenticCommit({ noChangelog: true, push: false, dryRun: false });
+
+		expect(gitModule.commit).toHaveBeenCalledTimes(1);
+		expect(result).toEqual({ usedFallback: true });
+	});
+
+	it("reports usedFallback when the commit agent throws before completing", async () => {
+		mockModelResolution();
+		await setupRepoMocks();
+		vi.spyOn(agentModule, "runCommitAgentSession").mockRejectedValue(new Error("model unreachable"));
+
+		const result = await runAgenticCommit({ noChangelog: true, push: false, dryRun: false });
+
+		expect(gitModule.commit).toHaveBeenCalledTimes(1);
+		expect(result).toEqual({ usedFallback: true });
+	});
+
+	it("reports usedFallback when the agent completes without a proposal", async () => {
+		mockModelResolution();
+		await setupRepoMocks();
+		vi.spyOn(agentModule, "runCommitAgentSession").mockImplementation((async (input: never) => {
+			const { onComplete } = input as { onComplete: (state: never) => Promise<void> };
+			await onComplete({} as never);
+		}) as never);
+
+		const result = await runAgenticCommit({ noChangelog: true, push: false, dryRun: false });
+
+		expect(gitModule.commit).toHaveBeenCalledTimes(1);
+		expect(result).toEqual({ usedFallback: true });
+	});
+
+	it("reports a clean run when the agent produces a proposal", async () => {
+		mockModelResolution();
+		await setupRepoMocks();
+		vi.spyOn(agentModule, "runCommitAgentSession").mockImplementation((async (input: never) => {
+			const { onComplete } = input as { onComplete: (state: never) => Promise<void> };
+			await onComplete({
+				proposal: {
+					analysis: { type: "fix", scope: "test", details: [], issueRefs: [] },
+					summary: "fixed the thing",
+					warnings: [],
+				},
+			} as never);
+		}) as never);
+
+		const result = await runAgenticCommit({ noChangelog: true, push: false, dryRun: false });
+
+		expect(gitModule.commit).toHaveBeenCalledTimes(1);
+		expect(result).toEqual({ usedFallback: false });
+	});
+});