Fix: Tools: atomic commit splitting — order unrelated changes by dependency by goransh-walia · Pull Request #5870 · Hmbown/Codewhale

goransh-walia · 2026-09-04T07:35:27Z

This PR addresses #3999.

Tools: atomic commit splitting — order unrelated changes by dependency, reject cycles

Generated with AI assistance and validated against the original
file before submission (syntax check + change-scope check).
Please review carefully — happy to adjust based on feedback.

Closes #3999

Maintainer follow-up

Thanks @goransh-walia for the original implementation and the cycle-rejection
tests, which this rework keeps. Per the review, the planner has been reworked
into a propose-only tool and pushed to this branch (head f94d4aa950,
rebased onto main at f9746854c; author commit preserved below the
maintainer commit):

The tool is now the Git action commit_plan (GitCommitPlanTool),
routed like status/diff/log/show/blame. It returns a dependency-ordered
split plan with proposed messages and writes nothing — no git add -N,
no git apply --cached, no git commit, no index or object-store
mutation. Landing commits stays on the ordinary git add / git commit
shell path, where the approval gate already applies.
The non-existent ApprovalRequirement::Manual variant is gone; the tool
declares ReadOnly + Sandboxable and classifies read-only end to end
(envelope Bounded, approval policy Safe/Benign, hooks gate safe,
history activity File).
Grouping, dependency ordering, and cycle rejection are kept and covered by
tests, including a propose-only proof that the index, HEAD, and the
untracked list are untouched after planning.
Changelog entry added under [Unreleased] crediting @goransh-walia.

Gates on the final tree: cargo fmt --all clean; cargo clippy --workspace --all-targets --all-features --locked -D warnings clean; targeted
tools::git git_tool commit_plan canonical_action 49 passed / 0 failed /
0 ignored; full cargo test -p codewhale-tui --lib --locked 11880 passed /
0 failed / 13 ignored.

github-actions · 2026-09-04T07:35:35Z

Thanks @goransh-walia for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

Hmbown

Kind: request-changes. The issue #3999 ask is legitimate, but this can't merge as-is. Blocking: (1) ApprovalRequirement::Manual (your git.rs:254) doesn't exist — the enum is Auto/Suggest/Required (crates/tools/src/lib.rs:44-52), so this doesn't compile; the PR's CI never ran the build. (2) This adds a second commit authority: a model-visible tool that heuristically groups changes and directly runs git apply --cached + git commit -m with generated messages (git.rs:918, :923-1008). Please make it propose-only (return the split plan + messages) and let commits flow through existing write paths, or route via git_tool.rs like status/diff. (3) It declares only Sandboxable, no WritesFiles — every write tool declares it, and the execution envelope fail-closes on under-declared tools (execution_envelope.rs:310-341). (4) Data safety: no git reset before git apply --cached (user-staged hunks get swept in), no rollback on partial failure, and git add -N runs before the dry_run check so dry-run mutates the index (git.rs:276-295). (5) No DCO sign-off. Happy to re-review a plan-only redesign — the cycle-rejection tests are a good start.

…own#3999)

@goransh-walia

Maintainer rework of Hmbown#5870 (fixes Hmbown#3999) on top of @goransh-walia's commit, per review: - The atomic-commit planner (Git action `commit_plan`, GitCommitPlanTool) is propose-only: it returns a dependency-ordered split plan with messages and writes nothing. The old mutation paths — `git add -N` on untracked files, `git apply --cached` plus a spawned `git commit` — are deleted. Untracked files are discovered with `ls-files --others` and read from disk for symbol analysis; landing commits stays on the ordinary `git add` / `git commit` shell path, where the approval gate already applies. - The non-existent `ApprovalRequirement::Manual` override is gone. The tool declares ReadOnly + Sandboxable and derives ApprovalRequirement::Auto explicitly, matching its git_status/git_diff siblings. - Every alias consumer classes commit_plan read-only: the execution envelope (classify_call -> Bounded, is_read_only_for), the approval policy (Safe / Benign), the hooks tool_category gate ("safe"), the tool card family (Read), and history activity (File). - Removed the now-dead Hunk old_range/new_range fields and parse_range (nothing rebuilds a patch from them anymore); a group of a source file plus its test scopes its commit message by the shared stem, not the directory. - Tests: grouping, dependency ordering, and cycle rejection kept; new propose-only proof (index, HEAD, and the untracked list untouched), staged-changes warning, clean-tree report, and envelope classification. Gates, final tree (rebased onto origin/main f974685): - cargo fmt --all: clean - cargo clippy --workspace --all-targets --all-features --locked (-D warnings, allowing uninlined_format_args/too_many_arguments/ unnecessary_map_or): exit 0 - cargo test -p codewhale-tui --lib --locked -- tools::git git_tool commit_plan canonical_action: 49 passed, 0 failed, 0 ignored - cargo test -p codewhale-tui --lib --locked: 11880 passed, 0 failed, 13 ignored Note on the shared target dir: two earlier full-suite runs on the pre-rebase tree reported 2143 failures and one intermediate run 1 failure; all were traced to the shared CARGO_TARGET_DIR serving this worktree codewhale-config / test-binary artifacts built from other in-flight worktrees (e.g. a 49-variant ProviderKind rlib against this tree's 48-entry FROM_KIND_LOOKUP). The counts above are from a run on this tree's own binary. Signed-off-by: CodeWhale Bot <bot@codewhale.net>

Hmbown · 2026-09-07T02:47:39Z

Thank you @goransh-walia for this contribution and for the cycle-rejection tests, which the rework keeps. A maintainer follow-up push is now on this branch (head f94d4aa950): the planner is propose-only per the review — it returns the ordered atomic-split plan and writes nothing, with commits landing through the ordinary git add / git commit approval path. Details in the Maintainer follow-up section of the PR description. Authorship of your original commit is preserved.

devin-ai-integration

Devin Review found 7 potential issues.

devin-ai-integration · 2026-09-07T02:50:22Z

+const DEFINING_KEYWORDS: &[&str] = &[
+    "fn",
+    "func",
+    "def",
+    "function",
+    "struct",
+    "enum",
+    "trait",
+    "class",
+    "interface",
+    "type",
+    "const",
+    "let",
+    "mod",
+];


🟡 Local variables create false cycles

Adding let left = right and let right = left in independent files makes plan_commits reject the entire plan. Local names enter defined_symbols, so lexical matches become cross-file dependency edges.

Prompt for agents

The dependency analyzer in crates/tui/src/tools/git.rs treats all identifiers following defining keywords, including local `let` bindings, as file-level symbols. plan_commits then interprets matching tokens in other files as commit dependencies and can reject valid plans as cycles. Restrict dependency inference to declarations that can actually be referenced across files, or introduce language-aware/conservative filtering that never turns ambiguous lexical matches into cycle rejection. Add coverage with independent files whose local names cross-match.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:22Z

+fn are_files_related(f1: &str, f2: &str) -> bool {
+    let stem1 = file_stem(f1).to_lowercase();
+    let stem2 = file_stem(f2).to_lowercase();
+    if stem1 == stem2 {
+        return true;
+    }
+    let clean1 = related_stem(f1);
+    !clean1.is_empty() && clean1 == related_stem(f2)
+}


🟡 Unrelated same-named files merge

Changed index.ts files in unrelated directories merge because are_files_related compares only file stems. The planner combines independent package changes into one non-atomic commit.

Prompt for agents

are_files_related in crates/tui/src/tools/git.rs ignores directory context and merges any files with equal or test-normalized stems. This combines ubiquitous names such as index.ts, mod.rs, lib.rs, and config.rs across unrelated packages. Preserve source/test pairing while requiring meaningful path proximity or an explicit source-to-test naming relationship. Add coverage for equal stems in unrelated directories.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:23Z

+        (m_name, l_name) => {
+            file_stem(manifest) == file_stem(lock)
+                || (m_name.ends_with(".json") && l_name.ends_with(".json"))
+        }


🟡 Package lockfiles attach incorrectly

A changed JSON file can capture package-lock.json before package.json because matches_lock_file accepts any same-directory JSON pair. The plan separates the lockfile from its manifest.

Suggested change

(m_name, l_name) => {

file_stem(manifest) == file_stem(lock)

|| (m_name.ends_with(".json") && l_name.ends_with(".json"))

}

(_m_name, _l_name) => file_stem(manifest) == file_stem(lock),

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:24Z

+            let path = rest
+                .rfind(" b/")
+                .map(|pos| &rest[pos + 3..])
+                .unwrap_or(rest)
+                .trim_matches('"')
+                .to_string();


🟡 Quoted paths become unusable

Git still quotes paths containing tabs or quotes under core.quotepath=false. parse_diff only removes outer quotes, so the plan returns escaped paths that git add cannot find.

Prompt for agents

parse_diff in crates/tui/src/tools/git.rs parses human-formatted `diff --git` headers and only trims surrounding quotes. Git applies C-style quoting for tabs, quotes, backslashes, and newlines even with core.quotepath=false, leaving escaped paths in the plan and metadata. Obtain filenames from a machine-readable NUL-delimited Git command, or fully decode Git path quoting and avoid ambiguous header splitting. Add coverage for special-character filenames.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:25Z

+        let mut diff_args = vec![
+            "-c".to_string(),
+            "core.quotepath=false".to_string(),
+            "diff".to_string(),
+            "HEAD".to_string(),
+            "--no-color".to_string(),
+            "--no-ext-diff".to_string(),
+            "-U3".to_string(),
+        ];
+        if let Some(pathspec) = &git_ctx.pathspec {
+            diff_args.push("--".to_string());
+            diff_args.push(pathspec.display().to_string());
+        }
+        let command = format_command(working_dir, &diff_args);
+        let mut files = match git_stdout(working_dir, &diff_args)? {
+            Ok(stdout) => parse_diff(&String::from_utf8_lossy(&stdout)),
+            Err(failure) => return Ok(failure),
+        };


🟡 Initial repositories cannot be planned

Before the first commit, git diff HEAD fails and commit_plan returns immediately. It never reaches the untracked-file scan, so it cannot plan an initial commit.

Prompt for agents

GitCommitPlanTool assumes HEAD exists before scanning untracked files. In an initialized repository with no commits, git diff HEAD fails and prevents planning the initial commit. Detect an unborn HEAD and treat the tracked diff as empty while continuing to the untracked-file scan, preserving normal command failures for established repositories. Add an integration test for a repository before its first commit.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:26Z

+- `Git` grows a `commit_plan` action: a propose-only planner that splits the
+  working tree into ordered atomic commits (#3999). It groups whole files —
+  lock files ride with their manifest, tests ride with the source they name —
+  orders the groups so a commit that defines a symbol lands before the commit
+  that uses it, and refuses the whole plan when that dependency graph has a
+  cycle. It reads `git diff HEAD` plus the untracked-file list and writes
+  nothing: no `git add -N`, no `git apply --cached`, no `git commit`, so
+  staging and committing stay with the ordinary `git add` / `git commit` shell
+  path where the approval gate already applies. Thanks
+  [@goransh-walia](https://github.com/goransh-walia) for the original
+  implementation (PR #5870, fixes #3999).


🔍 Remove branch changelog edits

Repository rules reserve both changelogs for merge-time receipts. These branch edits create avoidable conflicts and need removal before merge.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-07T02:50:26Z

+    if index_has_staged_changes {
+        out.push_str(
+            "WARNING: the index already holds staged changes. Run `git reset` before \
+             staging commit 1, or those hunks will ride into it.\n",
+        );


🔍 Staged-work warning recommends broad reset

The warning recommends bare git reset, which unstages unrelated operator work. Repository guidance requires preserving dirty changes, so this recovery text needs review.

Was this helpful? React with 👍 or 👎 to provide feedback.

`git_commit_plan` (Hmbown#5870, fixes Hmbown#3999) is the 76th model-visible tool, so the committed `web/lib/facts.generated.ts` went stale at toolCount 75 and `npm run check:facts` failed the Lint & Type Check gate. Regenerated with `cd web && npm run prebuild`; only the facts file is committed. `changelog.generated.ts` also moves under that script, but its drift comes from CHANGELOG entries merged in from main and is unrelated to this PR, so it is left alone. Gate: `npm run check:facts` → OK, committed facts.generated.ts matches workspace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P Signed-off-by: CodeWhale Bot <bot@codewhale.net>

Hmbown · 2026-09-07T05:05:43Z

Note: this comment is written by Claude Opus 5, posting through @Hmbown's account at his request. The automation that touched your branch was Claude too, so it seemed right that the explanation come from it directly rather than in Hunter's voice. He's read this and signed off on it.

@goransh-walia — thank you for this one. It's your first PR to the repo and you picked a hard one. #3999 sat open a while because the analysis is the difficult part, and you did it: the hunk parser, symbol extraction, the relatedness and lockfile heuristics, dependency ordering, and the cycle rejection with tests. That's the substance of the feature and it's shipping as you wrote it.

You're owed a straight account of what happened here. After Hunter's review, automated agents — me, essentially — pushed a rework commit directly onto your branch instead of leaving the feedback for you to act on, and force-pushed a rebase over it. Your original commit came through byte-identical with your authorship intact, but that was more than should be done to someone's PR without asking first. That's on the automation and on the humans who pointed it at your branch, not on you. Apologies for the surprise.

What the rework actually changed: it removed apply_hunks_and_commit — the stage that rebuilt a patch from parsed hunks, git apply --cached'd it into the index, and spawned git commit. Two reasons. A patch reconstructed from hunk text silently drops renames, mode changes, and binary files. And it gave the tool its own commit authority separate from the normal approval gate, which is a line this codebase holds everywhere. The tool now returns the ordered split plan with the exact git add / git commit commands per group, landing through the ordinary approval path — which is what point 5 of #3999 asked for. Every one of your analysis functions is untouched; the only other deletion was parse_range, which went dead once nothing rebuilds patches.

Your commit keeps your authorship and you're credited in the CHANGELOG. If you'd rather take the rework back over or shape it differently, say so and it's yours. Either way — strong first contribution, and more would be welcome.

…new tool `git_commit_plan` (Hmbown#5870, fixes Hmbown#3999) is the 76th model-visible tool. `facts.generated.ts` was regenerated in the parent commit, but `docs/public-surface-facts.json` carries a second, hand-maintained `sourceCandidate.toolCount` that the web suite pins against it, so `public-surface-contract.test.ts` failed with `expected 75 to be 76`. Bumped that one field; no other value in the matrix changes. Gates run locally in the Lint & Type Check job's own order: - npm run check:facts -> OK (committed facts match workspace) - npm run prebuild -> tools=76 - npm test -> 47 files / 407 tests passed, 0 failed - npm run lint -> 0 errors (2 pre-existing next/image warnings) - npx tsc --noEmit -> clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P Signed-off-by: CodeWhale Bot <bot@codewhale.net>

@goransh-walia

Addressed by the propose-only rework (f94d4aa): commit_plan writes nothing — no git add -N, no git apply --cached, no spawned commit — and declares ReadOnly + Sandboxable with ApprovalRequirement::Auto. @goransh-walia's grouping, dependency ordering and cycle rejection are kept intact. Facts drift resolved in d7799eb + 7da6064. All checks green on all three platforms. Dismissing to unblock merge.

goransh-walia requested a review from Hmbown as a code owner September 4, 2026 07:35

Hmbown previously requested changes Sep 5, 2026

View reviewed changes

goransh-walia and others added 2 commits September 6, 2026 17:09

fix: Tools: atomic commit splitting — order unrelated changes by (Hmb…

0afd84e

…own#3999)

Hmbown force-pushed the fix/issue-3999-254 branch from 3ebd8e4 to f94d4aa Compare September 7, 2026 02:47

devin-ai-integration Bot reviewed Sep 7, 2026

View reviewed changes

Hmbown and others added 3 commits September 6, 2026 20:26

Merge branch 'main' into fix/issue-3999-254

fe1f7a6

Merge branch 'main' into fix/issue-3999-254

80dfe23

Hmbown merged commit 3f3aa9e into Hmbown:main Sep 7, 2026
24 checks passed

Hmbown mentioned this pull request Sep 7, 2026

chore(gh): allowlist @goransh-walia in the contribution gate #5985

Open

Navigation Menu

Fix: Tools: atomic commit splitting — order unrelated changes by dependency - #5870

Fix: Tools: atomic commit splitting — order unrelated changes by dependency#5870
Hmbown merged 6 commits into
Hmbown:mainfrom
goransh-walia:fix/issue-3999-254

goransh-walia commented Sep 4, 2026 •

edited by Hmbown

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

Hmbown left a comment

Uh oh!

Hmbown commented Sep 7, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Uh oh!

Hmbown commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

goransh-walia commented Sep 4, 2026 • edited by Hmbown Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Maintainer follow-up

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

Hmbown left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented Sep 7, 2026

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Hmbown commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

goransh-walia commented Sep 4, 2026 •

edited by Hmbown

Loading