fix(commit): created commits before agent teardown

- Ran commit host completion before commit-agent session disposal so mnemopi/autolearn teardown cannot preempt a valid proposal.

- Converted missing commit-agent host outputs and split-plan gaps into thrown errors so omp commit cannot resolve into exit 0 without creating a commit.

- Preserved caller GPG_TTY state instead of forcing a bogus signing TTY in git and non-interactive subprocess environments.

Fixes #4794
This commit is contained in:
roboomp
2026-07-11 00:58:34 +00:00
parent ac2ea80fa3
commit 159484ca6f
9 changed files with 170 additions and 23 deletions
+4
View File
@@ -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
@@ -29,6 +29,7 @@ export interface CommitAgentInput {
requireChangelog: boolean;
diffText?: string;
existingChangelogEntries?: ExistingChangelogEntries[];
onComplete?: (state: CommitAgentState) => Promise<void> | void;
}
export interface ExistingChangelogEntries {
@@ -175,6 +176,9 @@ export async function runCommitAgentSession(input: CommitAgentInput): Promise<Co
});
}
if (input.onComplete) {
await input.onComplete(state);
}
return state;
} finally {
unsubscribe();
@@ -6,7 +6,7 @@ import { detectChangelogBoundaries } from "../../commit/changelog/detect";
import { parseUnreleasedSection } from "../../commit/changelog/parse";
import { formatCommitMessage } from "../../commit/message";
import { resolvePrimaryModel, resolveSmolModel } from "../../commit/model-selection";
import type { CommitCommandArgs, ConventionalAnalysis } from "../../commit/types";
import type { CommitCommandArgs, ConventionalAnalysis, NumstatEntry } from "../../commit/types";
import { ModelRegistry } from "../../config/model-registry";
import { Settings } from "../../config/settings";
import { discoverAuthStorage, discoverContextFiles } from "../../sdk";
@@ -122,11 +122,10 @@ export async function runAgenticCommit(args: CommitCommandArgs): Promise<void> {
}
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<void> {
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<void> {
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<void> {
}
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<void> {
@@ -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) {
@@ -15,7 +15,6 @@ export const NON_INTERACTIVE_ENV: Readonly<Record<string, string>> = {
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.
-1
View File
@@ -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<string, string>;
const GH_NON_INTERACTIVE_ENV = {
@@ -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"]);
});
});
@@ -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();
});
});
@@ -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");
});
});
@@ -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");
});
});