fix(commit): pair staged lock files with the split-plan commit that owns them
git_overview hides EXCLUDED_LOCK_FILES from the model so lock files never drive split decisions, but runSplitCommit then re-fetched the raw staged set and rejected any plan that failed to enumerate them, aborting `omp commit` with "Split commit plan missing staged files: <lockfile>". Skipping the validator would have masked a real drop — the executor resets the index and only re-stages files listed in each commit group. Introduce packages/coding-agent/src/commit/agentic/lock-files.ts with a LOCK_FILE_MANIFESTS map and an assignLockFilesToPlan helper that attaches each orphaned lock file to (1) the commit group touching a sibling manifest in the same directory, (2) any commit group touching a matching manifest, or (3) the last commit group. git-overview.ts imports EXCLUDED_LOCK_FILES from the shared module so the filter and the pairing table stay in sync. Fixes #4632
This commit is contained in:
roboomp
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed `omp commit` split plans aborting with `Split commit plan missing staged files: <lockfile>` when a lock file was staged alongside its manifest: the model deliberately never sees lock files, so it cannot include them, but the split validator was comparing against the raw staged set. Lock files are now deterministically paired with the commit group touching their sibling manifest (falling back to the last commit), so the validator and the model view agree ([#4632](https://github.com/can1357/oh-my-pi/issues/4632)).
|
||||
|
||||
## [16.3.8] - 2026-07-05
|
||||
|
||||
### Fixed
|
||||
|
||||
@@ -13,6 +13,7 @@ import { discoverAuthStorage, discoverContextFiles } from "../../sdk";
|
||||
import * as git from "../../utils/git";
|
||||
import { type ExistingChangelogEntries, runCommitAgentSession } from "./agent";
|
||||
import { generateFallbackProposal } from "./fallback";
|
||||
import { assignLockFilesToPlan } from "./lock-files";
|
||||
import splitConfirmPrompt from "./prompts/split-confirm.md" with { type: "text" };
|
||||
import type { CommitAgentState, CommitProposal, HunkSelector, SplitCommitPlan } from "./state";
|
||||
import { computeDependencyOrder } from "./topo-sort";
|
||||
@@ -230,6 +231,7 @@ async function runSplitCommit(
|
||||
appendFilesToLastCommit(plan, ctx.additionalFiles);
|
||||
}
|
||||
const stagedFiles = await git.diff.changedFiles(ctx.cwd, { cached: true });
|
||||
assignLockFilesToPlan(plan, stagedFiles);
|
||||
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) {
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
/**
|
||||
* Lock-file handling for the split-commit workflow.
|
||||
*
|
||||
* The commit agent hides these machine-generated files from analysis so the
|
||||
* model does not waste tokens on them and does not treat them as evidence for
|
||||
* commit boundaries. That leaves them staged but unseen: without deterministic
|
||||
* post-plan placement the split validator rejects the plan with
|
||||
* `Split commit plan missing staged files: <lockfile>`, and the executor
|
||||
* (`git stage.reset` -> per-group `stage.hunks`) would silently drop the file
|
||||
* if the validator were skipped. See issue #4632.
|
||||
*/
|
||||
|
||||
import type { SplitCommitPlan } from "./state";
|
||||
|
||||
/**
|
||||
* Lock file basename -> ordered sibling manifests. Order matters: the first
|
||||
* manifest present in a commit group's changes wins.
|
||||
*/
|
||||
export const LOCK_FILE_MANIFESTS: Readonly<Record<string, readonly string[]>> = {
|
||||
"Cargo.lock": ["Cargo.toml"],
|
||||
"package-lock.json": ["package.json"],
|
||||
"yarn.lock": ["package.json"],
|
||||
"pnpm-lock.yaml": ["package.json"],
|
||||
"bun.lock": ["package.json"],
|
||||
"bun.lockb": ["package.json"],
|
||||
"go.sum": ["go.mod"],
|
||||
"poetry.lock": ["pyproject.toml"],
|
||||
"Pipfile.lock": ["Pipfile"],
|
||||
"uv.lock": ["pyproject.toml"],
|
||||
"composer.lock": ["composer.json"],
|
||||
"Gemfile.lock": ["Gemfile"],
|
||||
"flake.lock": ["flake.nix"],
|
||||
"pubspec.lock": ["pubspec.yaml"],
|
||||
"Podfile.lock": ["Podfile"],
|
||||
"mix.lock": ["mix.exs"],
|
||||
"gradle.lockfile": ["build.gradle", "build.gradle.kts", "settings.gradle", "settings.gradle.kts"],
|
||||
};
|
||||
|
||||
/**
|
||||
* Lock-file basenames the commit agent excludes from `git_overview` output and
|
||||
* from split-commit validation. Derived from {@link LOCK_FILE_MANIFESTS} so a
|
||||
* single edit keeps both the analysis filter and the post-plan pairing in sync.
|
||||
*/
|
||||
export const EXCLUDED_LOCK_FILES: ReadonlySet<string> = new Set(Object.keys(LOCK_FILE_MANIFESTS));
|
||||
|
||||
/**
|
||||
* Attach staged lock files the model never saw to the split plan.
|
||||
*
|
||||
* Placement precedence per lock file:
|
||||
* 1. commit group that touches a sibling manifest (same directory)
|
||||
* 2. commit group that touches a manifest in any directory
|
||||
* 3. last commit group (fallback)
|
||||
*
|
||||
* Mutates {@link plan} in place. No-ops on an empty plan, on lock files
|
||||
* already present in some commit group, and on staged files that are not
|
||||
* recognized lock files.
|
||||
*/
|
||||
export function assignLockFilesToPlan(plan: SplitCommitPlan, stagedFiles: readonly string[]): void {
|
||||
if (plan.commits.length === 0) return;
|
||||
|
||||
const planned = new Set(plan.commits.flatMap(commit => commit.changes.map(change => change.path)));
|
||||
const orphanedLockFiles: string[] = [];
|
||||
for (const file of stagedFiles) {
|
||||
if (planned.has(file)) continue;
|
||||
const parts = file.split("/");
|
||||
const basename = parts[parts.length - 1];
|
||||
if (EXCLUDED_LOCK_FILES.has(basename)) orphanedLockFiles.push(file);
|
||||
}
|
||||
if (orphanedLockFiles.length === 0) return;
|
||||
|
||||
for (const lockFile of orphanedLockFiles) {
|
||||
const parts = lockFile.split("/");
|
||||
const basename = parts[parts.length - 1];
|
||||
const dir = parts.slice(0, -1).join("/");
|
||||
const manifests = LOCK_FILE_MANIFESTS[basename] ?? [];
|
||||
const targetIndex = findManifestCommitIndex(plan, dir, manifests);
|
||||
plan.commits[targetIndex].changes.push({ path: lockFile, hunks: { type: "all" } });
|
||||
planned.add(lockFile);
|
||||
}
|
||||
}
|
||||
|
||||
function findManifestCommitIndex(plan: SplitCommitPlan, lockDir: string, manifests: readonly string[]): number {
|
||||
// Prefer a manifest in the same directory as the lock file — the strongest
|
||||
// semantic signal (e.g. workspace-crate `Cargo.toml` next to `Cargo.lock`).
|
||||
for (const manifestName of manifests) {
|
||||
for (let i = 0; i < plan.commits.length; i++) {
|
||||
for (const change of plan.commits[i].changes) {
|
||||
const parts = change.path.split("/");
|
||||
const basename = parts[parts.length - 1];
|
||||
const dir = parts.slice(0, -1).join("/");
|
||||
if (basename === manifestName && dir === lockDir) return i;
|
||||
}
|
||||
}
|
||||
}
|
||||
// Fall back to any matching manifest — a monorepo may lock at repo root
|
||||
// while the manifest sits under a subpath.
|
||||
for (const manifestName of manifests) {
|
||||
for (let i = 0; i < plan.commits.length; i++) {
|
||||
for (const change of plan.commits[i].changes) {
|
||||
const parts = change.path.split("/");
|
||||
if (parts[parts.length - 1] === manifestName) return i;
|
||||
}
|
||||
}
|
||||
}
|
||||
// Nothing matched: attach to the last commit so the file still ships.
|
||||
return plan.commits.length - 1;
|
||||
}
|
||||
@@ -3,26 +3,7 @@ import type { CommitAgentState, GitOverviewSnapshot } from "../../../commit/agen
|
||||
import { extractScopeCandidates } from "../../../commit/analysis/scope";
|
||||
import type { CustomTool } from "../../../extensibility/custom-tools/types";
|
||||
import * as git from "../../../utils/git";
|
||||
|
||||
const EXCLUDED_LOCK_FILES = new Set([
|
||||
"Cargo.lock",
|
||||
"package-lock.json",
|
||||
"yarn.lock",
|
||||
"pnpm-lock.yaml",
|
||||
"bun.lock",
|
||||
"bun.lockb",
|
||||
"go.sum",
|
||||
"poetry.lock",
|
||||
"Pipfile.lock",
|
||||
"uv.lock",
|
||||
"composer.lock",
|
||||
"Gemfile.lock",
|
||||
"flake.lock",
|
||||
"pubspec.lock",
|
||||
"Podfile.lock",
|
||||
"mix.lock",
|
||||
"gradle.lockfile",
|
||||
]);
|
||||
import { EXCLUDED_LOCK_FILES } from "../lock-files";
|
||||
|
||||
function isExcludedFile(path: string): boolean {
|
||||
const basename = path.split("/").pop() ?? path;
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import { assignLockFilesToPlan, EXCLUDED_LOCK_FILES, LOCK_FILE_MANIFESTS } from "../src/commit/agentic/lock-files";
|
||||
import type { SplitCommitPlan } from "../src/commit/agentic/state";
|
||||
|
||||
/**
|
||||
* Contract: staged lock files that `git_overview` hid from the model must be
|
||||
* paired with the split-plan commit touching their sibling manifest, so the
|
||||
* validator in `runSplitCommit` (packages/coding-agent/src/commit/agentic/index.ts)
|
||||
* no longer rejects an otherwise valid plan with
|
||||
* `Split commit plan missing staged files: <lockfile>`. See issue #4632.
|
||||
*/
|
||||
|
||||
function plan(...commits: Array<{ paths: string[] }>): SplitCommitPlan {
|
||||
return {
|
||||
warnings: [],
|
||||
commits: commits.map(commit => ({
|
||||
changes: commit.paths.map(path => ({ path, hunks: { type: "all" } })),
|
||||
type: "chore",
|
||||
scope: null,
|
||||
summary: "test",
|
||||
details: [],
|
||||
issueRefs: [],
|
||||
dependencies: [],
|
||||
})),
|
||||
};
|
||||
}
|
||||
|
||||
function pathsOf(target: SplitCommitPlan, index: number): string[] {
|
||||
return target.commits[index].changes.map(change => change.path);
|
||||
}
|
||||
|
||||
describe("assignLockFilesToPlan", () => {
|
||||
it("attaches a lock file to the commit group that touches its sibling manifest", () => {
|
||||
const target = plan({ paths: ["pyproject.toml"] }, { paths: ["src/foo.py"] });
|
||||
assignLockFilesToPlan(target, ["pyproject.toml", "src/foo.py", "uv.lock"]);
|
||||
expect(pathsOf(target, 0)).toEqual(["pyproject.toml", "uv.lock"]);
|
||||
expect(pathsOf(target, 1)).toEqual(["src/foo.py"]);
|
||||
});
|
||||
|
||||
it("prefers a sibling manifest in the same directory over a matching manifest elsewhere", () => {
|
||||
const target = plan({ paths: ["packages/root/package.json"] }, { paths: ["packages/child/package.json"] });
|
||||
assignLockFilesToPlan(target, [
|
||||
"packages/root/package.json",
|
||||
"packages/child/package.json",
|
||||
"packages/child/package-lock.json",
|
||||
]);
|
||||
expect(pathsOf(target, 0)).toEqual(["packages/root/package.json"]);
|
||||
expect(pathsOf(target, 1)).toEqual(["packages/child/package.json", "packages/child/package-lock.json"]);
|
||||
});
|
||||
|
||||
it("falls back to any matching manifest when no sibling in the lock file's directory is planned", () => {
|
||||
const target = plan({ paths: ["docs/README.md"] }, { paths: ["crates/thing/Cargo.toml"] });
|
||||
assignLockFilesToPlan(target, ["docs/README.md", "crates/thing/Cargo.toml", "Cargo.lock"]);
|
||||
expect(pathsOf(target, 0)).toEqual(["docs/README.md"]);
|
||||
expect(pathsOf(target, 1)).toEqual(["crates/thing/Cargo.toml", "Cargo.lock"]);
|
||||
});
|
||||
|
||||
it("falls back to the last commit when no manifest sibling is planned", () => {
|
||||
const target = plan({ paths: ["src/a.ts"] }, { paths: ["src/b.ts"] });
|
||||
assignLockFilesToPlan(target, ["src/a.ts", "src/b.ts", "package-lock.json"]);
|
||||
expect(pathsOf(target, 0)).toEqual(["src/a.ts"]);
|
||||
expect(pathsOf(target, 1)).toEqual(["src/b.ts", "package-lock.json"]);
|
||||
});
|
||||
|
||||
it("leaves the plan unchanged when the lock file is already accounted for", () => {
|
||||
const target = plan({ paths: ["pyproject.toml", "uv.lock"] }, { paths: ["src/foo.py"] });
|
||||
assignLockFilesToPlan(target, ["pyproject.toml", "uv.lock", "src/foo.py"]);
|
||||
expect(pathsOf(target, 0)).toEqual(["pyproject.toml", "uv.lock"]);
|
||||
expect(pathsOf(target, 1)).toEqual(["src/foo.py"]);
|
||||
});
|
||||
|
||||
it("ignores staged files that are not recognized lock files", () => {
|
||||
const target = plan({ paths: ["src/a.ts"] });
|
||||
assignLockFilesToPlan(target, ["src/a.ts", "src/mystery.bin", "README.md"]);
|
||||
expect(pathsOf(target, 0)).toEqual(["src/a.ts"]);
|
||||
expect(target.commits).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("no-ops on an empty plan (nothing to attach to)", () => {
|
||||
const target: SplitCommitPlan = { commits: [], warnings: [] };
|
||||
assignLockFilesToPlan(target, ["uv.lock"]);
|
||||
expect(target.commits).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps EXCLUDED_LOCK_FILES in sync with LOCK_FILE_MANIFESTS", () => {
|
||||
const manifestKeys = new Set<string>();
|
||||
for (const key in LOCK_FILE_MANIFESTS) manifestKeys.add(key);
|
||||
expect(new Set(EXCLUDED_LOCK_FILES)).toEqual(manifestKeys);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user