fix(commit): reject empty split hunk selectors
This commit is contained in:
@@ -182,6 +182,8 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed `omp commit` split plans rejecting hunk selectors that resolve to no parsed hunks before resetting the index ([#2098](https://github.com/can1357/oh-my-pi/issues/2098)).
|
||||
|
||||
- Fixed inline `find` and `search` result blocks to align with grouped `read` output and render their success headers with the normal tool-title color instead of accent blue.
|
||||
|
||||
- Fixed the working-status shimmer to opt into the loader's 30fps animated-message repaint path while keeping both the status spinner and pending bash/eval tool spinners on their normal 80 ms glyph cadence.
|
||||
|
||||
@@ -102,7 +102,7 @@ export function createSplitCommitTool(
|
||||
}
|
||||
warnings.push(...summaryValidation.warnings.map(warning => `Commit ${index + 1}: ${warning}`));
|
||||
warnings.push(...typeValidation.warnings.map(warning => `Commit ${index + 1}: ${warning}`));
|
||||
const hunkValidation = validateHunkSelectors(index, changes, files);
|
||||
const hunkValidation = validateHunkSelectors(index, changes, files, diffText);
|
||||
warnings.push(...hunkValidation.warnings);
|
||||
errors.push(...hunkValidation.errors);
|
||||
errors.push(...validateDependencies(index, dependencies, params.commits.length));
|
||||
@@ -186,6 +186,7 @@ function validateHunkSelectors(
|
||||
commitIndex: number,
|
||||
changes: SplitCommitGroup["changes"],
|
||||
files: string[],
|
||||
diffText: string,
|
||||
): { errors: string[]; warnings: string[] } {
|
||||
const errors: string[] = [];
|
||||
const warnings: string[] = [];
|
||||
@@ -215,6 +216,11 @@ function validateHunkSelectors(
|
||||
}
|
||||
}
|
||||
}
|
||||
if (errors.length === 0) {
|
||||
for (const error of git.validateHunkSelections(diffText, changes)) {
|
||||
errors.push(`${prefix}: ${error.message}`);
|
||||
}
|
||||
}
|
||||
return { errors, warnings };
|
||||
}
|
||||
|
||||
|
||||
@@ -45,6 +45,10 @@ export interface StageHunksOptions {
|
||||
readonly rawDiff?: string;
|
||||
readonly signal?: AbortSignal;
|
||||
}
|
||||
export interface HunkSelectionValidationError {
|
||||
readonly path: string;
|
||||
readonly message: string;
|
||||
}
|
||||
|
||||
export interface DiffOptions {
|
||||
readonly allowFailure?: boolean;
|
||||
@@ -678,6 +682,34 @@ function selectHunks(file: FileHunks, selector: HunkSelection["hunks"]): FileHun
|
||||
return file.hunks;
|
||||
}
|
||||
|
||||
export function validateHunkSelections(
|
||||
rawDiff: string,
|
||||
selections: readonly HunkSelection[],
|
||||
): HunkSelectionValidationError[] {
|
||||
const fileDiffs = parseFileDiffs(rawDiff);
|
||||
const fileDiffMap = new Map(fileDiffs.map(entry => [entry.filename, entry]));
|
||||
const errors: HunkSelectionValidationError[] = [];
|
||||
|
||||
for (const selection of selections) {
|
||||
const fileDiff = fileDiffMap.get(selection.path);
|
||||
if (!fileDiff) {
|
||||
errors.push({ path: selection.path, message: `No diff found for ${selection.path}` });
|
||||
continue;
|
||||
}
|
||||
if (selection.hunks.type === "all") continue;
|
||||
if (fileDiff.isBinary) {
|
||||
errors.push({ path: selection.path, message: `Cannot select hunks for binary file ${selection.path}` });
|
||||
continue;
|
||||
}
|
||||
const selected = selectHunks(parseFileHunks(fileDiff), selection.hunks);
|
||||
if (selected.length === 0) {
|
||||
errors.push({ path: selection.path, message: `No hunks selected for ${selection.path}` });
|
||||
}
|
||||
}
|
||||
|
||||
return errors;
|
||||
}
|
||||
|
||||
function parseStatusPorcelain(text: string): GitStatusSummary {
|
||||
let staged = 0;
|
||||
let unstaged = 0;
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
import { afterEach, describe, expect, it, vi } from "bun:test";
|
||||
import type { CommitAgentState } from "../src/commit/agentic/state";
|
||||
import { createSplitCommitTool } from "../src/commit/agentic/tools/split-commit";
|
||||
import * as git from "../src/utils/git";
|
||||
|
||||
const STAGED_DIFF = `diff --git a/src/a.ts b/src/a.ts
|
||||
index 1111111..2222222 100644
|
||||
--- a/src/a.ts
|
||||
+++ b/src/a.ts
|
||||
@@ -1,3 +1,3 @@
|
||||
export function a() {
|
||||
- return 1;
|
||||
+ return 2;
|
||||
}
|
||||
diff --git a/src/b.ts b/src/b.ts
|
||||
index 3333333..4444444 100644
|
||||
--- a/src/b.ts
|
||||
+++ b/src/b.ts
|
||||
@@ -1,3 +1,3 @@
|
||||
export function b() {
|
||||
- return 1;
|
||||
+ return 2;
|
||||
}
|
||||
`;
|
||||
|
||||
describe("split_commit hunk selector validation", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("rejects hunk index selectors that match no parsed hunk", async () => {
|
||||
vi.spyOn(git, "diff").mockResolvedValue(STAGED_DIFF);
|
||||
const state: CommitAgentState = {
|
||||
overview: { files: ["src/a.ts", "src/b.ts"], stat: "", numstat: [], scopeCandidates: "", isWideScope: false },
|
||||
};
|
||||
const tool = createSplitCommitTool("/repo", state, []);
|
||||
|
||||
const result = await tool.execute(
|
||||
"split-commit",
|
||||
{
|
||||
commits: [
|
||||
{
|
||||
changes: [{ path: "src/a.ts", hunks: { type: "indices", indices: [2] } }],
|
||||
type: "fix",
|
||||
scope: null,
|
||||
summary: "Fixed invalid selector handling",
|
||||
},
|
||||
{
|
||||
changes: [{ path: "src/b.ts", hunks: { type: "all" } }],
|
||||
type: "fix",
|
||||
scope: null,
|
||||
summary: "Fixed split commit coverage",
|
||||
},
|
||||
],
|
||||
},
|
||||
undefined,
|
||||
{} as never,
|
||||
);
|
||||
|
||||
expect(result.details.valid).toBe(false);
|
||||
expect(result.details.errors).toContain("Commit 1: No hunks selected for src/a.ts");
|
||||
expect(state.splitProposal).toBeUndefined();
|
||||
});
|
||||
|
||||
it("rejects line selectors that overlap no parsed hunk", async () => {
|
||||
vi.spyOn(git, "diff").mockResolvedValue(STAGED_DIFF);
|
||||
const state: CommitAgentState = {
|
||||
overview: { files: ["src/a.ts", "src/b.ts"], stat: "", numstat: [], scopeCandidates: "", isWideScope: false },
|
||||
};
|
||||
const tool = createSplitCommitTool("/repo", state, []);
|
||||
|
||||
const result = await tool.execute(
|
||||
"split-commit",
|
||||
{
|
||||
commits: [
|
||||
{
|
||||
changes: [{ path: "src/a.ts", hunks: { type: "lines", start: 50, end: 60 } }],
|
||||
type: "fix",
|
||||
scope: null,
|
||||
summary: "Fixed invalid line selectors",
|
||||
},
|
||||
{
|
||||
changes: [{ path: "src/b.ts", hunks: { type: "all" } }],
|
||||
type: "fix",
|
||||
scope: null,
|
||||
summary: "Fixed split commit coverage",
|
||||
},
|
||||
],
|
||||
},
|
||||
undefined,
|
||||
{} as never,
|
||||
);
|
||||
|
||||
expect(result.details.valid).toBe(false);
|
||||
expect(result.details.errors).toContain("Commit 1: No hunks selected for src/a.ts");
|
||||
expect(state.splitProposal).toBeUndefined();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user