diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3a03a1977..063d52e2e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,10 @@ - Memoized non-message token totals (system prompt, tool schemas, skills) so the per-turn compaction and context-threshold paths recompute them at most once per input change instead of on every call. `getContextBreakdown` and `#estimateStoredContextTokens` previously re-tokenized the system prompt and every tool's wire schema (per-tool `JSON.stringify`) several times per turn over inputs that change at most once per turn. +### Fixed + +- Fixed `omp commit` agent sessions so a valid proposal is committed before session teardown can dispose mnemopi/autolearn resources, missing required host outputs now fail non-zero instead of returning cleanly, and git subprocesses no longer force `GPG_TTY=not a tty` on signing-enabled repositories ([#4794](https://github.com/can1357/oh-my-pi/issues/4794)). + ## [16.3.11] - 2026-07-06 ### Changed diff --git a/packages/coding-agent/src/commit/agentic/agent.ts b/packages/coding-agent/src/commit/agentic/agent.ts index 68b529e97..98b06540d 100644 --- a/packages/coding-agent/src/commit/agentic/agent.ts +++ b/packages/coding-agent/src/commit/agentic/agent.ts @@ -29,6 +29,7 @@ export interface CommitAgentInput { requireChangelog: boolean; diffText?: string; existingChangelogEntries?: ExistingChangelogEntries[]; + onComplete?: (state: CommitAgentState) => Promise | void; } export interface ExistingChangelogEntries { @@ -175,6 +176,9 @@ export async function runCommitAgentSession(input: CommitAgentInput): Promise { } process.stdout.write("● Starting commit agent...\n"); - let commitState: CommitAgentState; - let usedFallback = false; + let agentSessionCompleted = false; try { - commitState = await runCommitAgentSession({ + await runCommitAgentSession({ cwd, model: agentModel, thinkingLevel: agentThinkingLevel, @@ -139,42 +138,67 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise { requireChangelog: !args.noChangelog && changelogTargets.length > 0, diffText: diff, existingChangelogEntries, + onComplete: async commitState => { + agentSessionCompleted = true; + await completeAgentCommitState(commitState, { + cwd, + dryRun: args.dryRun, + push: args.push, + noChangelog: args.noChangelog, + changelogTargets, + numstat, + }); + }, }); + return; } catch (error) { + if (agentSessionCompleted) { + throw error; + } const errorMessage = error instanceof Error ? error.message : String(error); process.stderr.write(`Agent error: ${errorMessage}\n`); if (error instanceof Error && error.stack && $env.DEBUG) { process.stderr.write(`${error.stack}\n`); } process.stdout.write("● Using fallback commit generation...\n"); - commitState = { proposal: generateFallbackProposal(numstat) }; - usedFallback = true; + const fallbackProposal = generateFallbackProposal(numstat); + await runSingleCommit(fallbackProposal, { cwd, dryRun: args.dryRun, push: args.push }); + return; } +} - if (!usedFallback && !commitState.proposal && !commitState.splitProposal) { +async function completeAgentCommitState( + commitState: CommitAgentState, + ctx: CommitExecutionContext & { + noChangelog: boolean; + changelogTargets: string[]; + numstat: NumstatEntry[]; + }, +): Promise { + let usedFallback = false; + if (!commitState.proposal && !commitState.splitProposal) { if ($env.PI_COMMIT_NO_FALLBACK?.toLowerCase() !== "true") { process.stdout.write("● Agent did not provide proposal, using fallback...\n"); - commitState.proposal = generateFallbackProposal(numstat); + commitState.proposal = generateFallbackProposal(ctx.numstat); usedFallback = true; } } let updatedChangelogFiles: string[] = []; - if (!args.noChangelog && changelogTargets.length > 0 && !usedFallback) { + if (!ctx.noChangelog && ctx.changelogTargets.length > 0 && !usedFallback) { if (!commitState.changelogProposal) { - process.stderr.write("Commit agent did not provide changelog entries.\n"); - return; + throw new Error("Commit agent did not provide changelog entries."); } process.stdout.write("● Applying changelog entries...\n"); const updated = await applyChangelogProposals({ - cwd, + cwd: ctx.cwd, proposals: commitState.changelogProposal.entries, - dryRun: args.dryRun, + dryRun: ctx.dryRun, onProgress: message => { process.stdout.write(` ├─ ${message}\n`); }, }); - updatedChangelogFiles = updated.map(filePath => path.relative(cwd, filePath)); + updatedChangelogFiles = updated.map(filePath => path.relative(ctx.cwd, filePath)); if (updated.length > 0) { for (const filePath of updated) { process.stdout.write(` └─ ${filePath}\n`); @@ -185,21 +209,21 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise { } if (commitState.proposal) { - await runSingleCommit(commitState.proposal, { cwd, dryRun: args.dryRun, push: args.push }); + await runSingleCommit(commitState.proposal, ctx); return; } if (commitState.splitProposal) { await runSplitCommit(commitState.splitProposal, { - cwd, - dryRun: args.dryRun, - push: args.push, + cwd: ctx.cwd, + dryRun: ctx.dryRun, + push: ctx.push, additionalFiles: updatedChangelogFiles, }); return; } - process.stderr.write("Commit agent did not provide a proposal.\n"); + throw new Error("Commit agent did not provide a proposal."); } async function runSingleCommit(proposal: CommitProposal, ctx: CommitExecutionContext): Promise { @@ -212,6 +236,7 @@ async function runSingleCommit(proposal: CommitProposal, ctx: CommitExecutionCon process.stdout.write(`${commitMessage}\n`); return; } + process.stdout.write("● Creating commit...\n"); await git.commit(ctx.cwd, commitMessage); process.stdout.write("Commit created.\n"); if (ctx.push) { @@ -235,8 +260,7 @@ async function runSplitCommit( const plannedFiles = new Set(plan.commits.flatMap(commit => commit.changes.map(change => change.path))); const missingFiles = stagedFiles.filter(file => !plannedFiles.has(file)); if (missingFiles.length > 0) { - process.stderr.write(`Split commit plan missing staged files: ${missingFiles.join(", ")}\n`); - return; + throw new Error(`Split commit plan missing staged files: ${missingFiles.join(", ")}`); } if (ctx.dryRun) { @@ -268,6 +292,7 @@ async function runSplitCommit( throw new Error(order.error); } + 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) { diff --git a/packages/coding-agent/src/exec/non-interactive-env.ts b/packages/coding-agent/src/exec/non-interactive-env.ts index c1b58ba91..1d25df10e 100644 --- a/packages/coding-agent/src/exec/non-interactive-env.ts +++ b/packages/coding-agent/src/exec/non-interactive-env.ts @@ -15,7 +15,6 @@ export const NON_INTERACTIVE_ENV: Readonly> = { LESS: "FRX", // Disable terminal features that can block the process. TERM: "dumb", - GPG_TTY: "not a tty", NO_COLOR: "1", PYTHONUNBUFFERED: "1", // Disable editor and terminal credential prompts. diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index d0a9eff5b..2974b0c68 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -197,7 +197,6 @@ const GIT_NON_INTERACTIVE_ENV = { GIT_ASKPASS: "true", GIT_EDITOR: "true", GIT_TERMINAL_PROMPT: "0", - GPG_TTY: "not a tty", SSH_ASKPASS: "/usr/bin/false", } satisfies Record; const GH_NON_INTERACTIVE_ENV = { diff --git a/packages/coding-agent/test/commit-agentic-attribution.test.ts b/packages/coding-agent/test/commit-agentic-attribution.test.ts index 54bf677fb..f3ee64dc0 100644 --- a/packages/coding-agent/test/commit-agentic-attribution.test.ts +++ b/packages/coding-agent/test/commit-agentic-attribution.test.ts @@ -46,4 +46,50 @@ describe("commit agent prompt attribution", () => { expect(prompt.options?.expandPromptTemplates).toBe(false); } }); + + it("runs completion before session disposal", async () => { + const events: string[] = []; + const session = { + prompt: async () => {}, + subscribe: () => () => {}, + dispose: async () => { + events.push("dispose"); + }, + }; + + vi.spyOn(sdkModule, "createAgentSession").mockResolvedValue({ session } as unknown as CreateAgentSessionResult); + vi.spyOn(toolsModule, "createCommitTools").mockImplementation(options => { + options.state.proposal = { + analysis: { + type: "fix", + scope: "commit", + details: [], + issueRefs: [], + }, + summary: "create commit before teardown", + warnings: [], + }; + return []; + }); + + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) { + throw new Error("Expected claude-sonnet-4-5 model to exist"); + } + + await runCommitAgentSession({ + cwd: "/tmp", + model, + settings: Settings.isolated(), + modelRegistry: {} as never, + authStorage: {} as never, + changelogTargets: [], + requireChangelog: false, + onComplete: state => { + events.push(state.proposal?.summary ?? "missing proposal"); + }, + }); + + expect(events).toEqual(["create commit before teardown", "dispose"]); + }); }); diff --git a/packages/coding-agent/test/commit-command-exit.test.ts b/packages/coding-agent/test/commit-command-exit.test.ts index f75247049..f5cd985c6 100644 --- a/packages/coding-agent/test/commit-command-exit.test.ts +++ b/packages/coding-agent/test/commit-command-exit.test.ts @@ -32,4 +32,24 @@ describe("omp commit command lifecycle (issue #1041)", () => { expect(runCommitSpy.mock.invocationCallOrder[0]).toBeLessThan(quitSpy.mock.invocationCallOrder[0]); expect(quitSpy).toHaveBeenCalledWith(0); }); + + it("does not convert commit pipeline failures into exit 0", async () => { + const initThemeSpy = vi.spyOn(themeModule, "initTheme").mockResolvedValue(undefined); + const runCommitSpy = vi + .spyOn(commitModule, "runCommitCommand") + .mockRejectedValue(new Error("commit was not created")); + const quitSpy = vi.spyOn(postmortem, "quit").mockResolvedValue(undefined); + + const command = new CommitCommand([], { + bin: "omp", + version: "0.0.0-test", + commands: new Map(), + }); + + await expect(command.run()).rejects.toThrow("commit was not created"); + + expect(initThemeSpy).toHaveBeenCalledTimes(1); + expect(runCommitSpy).toHaveBeenCalledTimes(1); + expect(quitSpy).not.toHaveBeenCalled(); + }); }); diff --git a/packages/coding-agent/test/git-process-config.test.ts b/packages/coding-agent/test/git-process-config.test.ts index 65e5ee039..68f508f15 100644 --- a/packages/coding-agent/test/git-process-config.test.ts +++ b/packages/coding-agent/test/git-process-config.test.ts @@ -110,4 +110,42 @@ describe("git subprocess config", () => { "HEAD:refs/heads/feature", ]); }); + + it("preserves the caller's GPG_TTY for signing-capable commands", async () => { + const originalGpgTty = process.env.GPG_TTY; + const spawnCalls: SpawnCall[] = []; + vi.spyOn(Bun, "spawn").mockImplementation(createSpawnMock(spawnCalls)); + + process.env.GPG_TTY = "/dev/pts/42"; + try { + await git.commit("/work/pi", "fix: preserve signing tty"); + } finally { + if (originalGpgTty === undefined) { + delete process.env.GPG_TTY; + } else { + process.env.GPG_TTY = originalGpgTty; + } + } + + expect(spawnCalls).toHaveLength(1); + expect(spawnCalls[0]?.options.env?.GPG_TTY).toBe("/dev/pts/42"); + }); + + it("does not invent a bogus GPG_TTY when the caller has none", async () => { + const originalGpgTty = process.env.GPG_TTY; + const spawnCalls: SpawnCall[] = []; + vi.spyOn(Bun, "spawn").mockImplementation(createSpawnMock(spawnCalls)); + + delete process.env.GPG_TTY; + try { + await git.commit("/work/pi", "fix: allow gui pinentry"); + } finally { + if (originalGpgTty !== undefined) { + process.env.GPG_TTY = originalGpgTty; + } + } + + expect(spawnCalls).toHaveLength(1); + expect(spawnCalls[0]?.options.env).not.toHaveProperty("GPG_TTY"); + }); }); diff --git a/packages/coding-agent/test/non-interactive-env.test.ts b/packages/coding-agent/test/non-interactive-env.test.ts index 57c4ca878..0eec8035b 100644 --- a/packages/coding-agent/test/non-interactive-env.test.ts +++ b/packages/coding-agent/test/non-interactive-env.test.ts @@ -44,4 +44,16 @@ describe("buildNonInteractiveEnv", () => { expect(env.LANG).toBeUndefined(); expect(env.LC_ALL).toBeUndefined(); }); + + it("does not invent a bogus GPG_TTY", () => { + const env = buildNonInteractiveEnv(undefined, {}, "linux"); + + expect(env).not.toHaveProperty("GPG_TTY"); + }); + + it("preserves per-command GPG_TTY overrides", () => { + const env = buildNonInteractiveEnv({ GPG_TTY: "/dev/pts/7" }, {}, "linux"); + + expect(env.GPG_TTY).toBe("/dev/pts/7"); + }); });