From 5d468a23788d9d5c1532816d33f66b7c0be6059f Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 8 Jun 2026 13:28:56 -0300 Subject: [PATCH] fix(commit): reject empty split hunk selectors --- packages/coding-agent/CHANGELOG.md | 2 + .../src/commit/agentic/tools/split-commit.ts | 8 +- packages/coding-agent/src/utils/git.ts | 32 ++++++ .../test/commit-split-hunk-validation.test.ts | 98 +++++++++++++++++++ 4 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 packages/coding-agent/test/commit-split-hunk-validation.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index dfed3a648..154a21016 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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. diff --git a/packages/coding-agent/src/commit/agentic/tools/split-commit.ts b/packages/coding-agent/src/commit/agentic/tools/split-commit.ts index ccb23aba7..43985d0c6 100644 --- a/packages/coding-agent/src/commit/agentic/tools/split-commit.ts +++ b/packages/coding-agent/src/commit/agentic/tools/split-commit.ts @@ -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 }; } diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 40cdfd491..4cb1c6e1c 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -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; diff --git a/packages/coding-agent/test/commit-split-hunk-validation.test.ts b/packages/coding-agent/test/commit-split-hunk-validation.test.ts new file mode 100644 index 000000000..732297740 --- /dev/null +++ b/packages/coding-agent/test/commit-split-hunk-validation.test.ts @@ -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(); + }); +});