diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index bd81a6f64..e5326dd39 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/commands/commit.ts b/packages/coding-agent/src/commands/commit.ts index ade85bd95..b8d1bf8e0 100644 --- a/packages/coding-agent/src/commands/commit.ts +++ b/packages/coding-agent/src/commands/commit.ts @@ -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); } } diff --git a/packages/coding-agent/src/commit/agentic/index.ts b/packages/coding-agent/src/commit/agentic/index.ts index cf67c7ee7..8e68ec3d4 100644 --- a/packages/coding-agent/src/commit/agentic/index.ts +++ b/packages/coding-agent/src/commit/agentic/index.ts @@ -25,7 +25,7 @@ interface CommitExecutionContext { push: boolean; } -export async function runAgenticCommit(args: CommitCommandArgs): Promise { +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 { 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 { 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 { 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 { 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 { 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 { }); }, }); - return; + return { usedFallback }; } catch (error) { if (agentSessionCompleted) { throw error; @@ -164,7 +165,7 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise { 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 { +): Promise { 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."); diff --git a/packages/coding-agent/src/commit/pipeline.ts b/packages/coding-agent/src/commit/pipeline.ts index e0a8a8cba..35611539f 100644 --- a/packages/coding-agent/src/commit/pipeline.ts +++ b/packages/coding-agent/src/commit/pipeline.ts @@ -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 { +export async function runCommitCommand(args: CommitCommandArgs): Promise<{ usedFallback: boolean }> { if (args.legacy) { - return runLegacyCommitCommand(args); + await runLegacyCommitCommand(args); + return { usedFallback: false }; } return runAgenticCommit(args); } diff --git a/packages/coding-agent/test/commit-command-exit.test.ts b/packages/coding-agent/test/commit-command-exit.test.ts index f5cd985c6..f50f55f60 100644 --- a/packages/coding-agent/test/commit-command-exit.test.ts +++ b/packages/coding-agent/test/commit-command-exit.test.ts @@ -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 diff --git a/packages/coding-agent/test/commit-extension-providers.test.ts b/packages/coding-agent/test/commit-extension-providers.test.ts index 518057064..b00465ed3 100644 --- a/packages/coding-agent/test/commit-extension-providers.test.ts +++ b/packages/coding-agent/test/commit-extension-providers.test.ts @@ -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 }); }); }); diff --git a/packages/coding-agent/test/commit-fallback-exit.test.ts b/packages/coding-agent/test/commit-fallback-exit.test.ts new file mode 100644 index 000000000..fce3d9824 --- /dev/null +++ b/packages/coding-agent/test/commit-fallback-exit.test.ts @@ -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 }; + 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 }; + 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 }); + }); +});