Fix: Tools: atomic commit splitting — order unrelated changes by dependency - #5870
Fix: Tools: atomic commit splitting — order unrelated changes by dependency#5870goransh-walia wants to merge 4 commits into
Conversation
|
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 |
Hmbown
left a comment
Hmbown
left a comment
There was a problem hiding this comment.
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.
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>
3ebd8e4 to
f94d4aa
Compare
|
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const DEFINING_KEYWORDS: &[&str] = &[ | ||
| "fn", | ||
| "func", | ||
| "def", | ||
| "function", | ||
| "struct", | ||
| "enum", | ||
| "trait", | ||
| "class", | ||
| "interface", | ||
| "type", | ||
| "const", | ||
| "let", | ||
| "mod", | ||
| ]; |
There was a problem hiding this comment.
🟡 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.
| 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) | ||
| } |
There was a problem hiding this comment.
🟡 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.
| (m_name, l_name) => { | ||
| file_stem(manifest) == file_stem(lock) | ||
| || (m_name.ends_with(".json") && l_name.ends_with(".json")) | ||
| } |
There was a problem hiding this comment.
🟡 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.
| (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.
| let path = rest | ||
| .rfind(" b/") | ||
| .map(|pos| &rest[pos + 3..]) | ||
| .unwrap_or(rest) | ||
| .trim_matches('"') | ||
| .to_string(); |
There was a problem hiding this comment.
🟡 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.
| 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), | ||
| }; |
There was a problem hiding this comment.
🟡 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.
| - `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). |
There was a problem hiding this comment.
🔍 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.
| 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", | ||
| ); |
There was a problem hiding this comment.
🔍 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.
2 participants
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
mainatf9746854c; author commit preserved below themaintainer commit):
Gitactioncommit_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, nogit commit, no index or object-storemutation. Landing commits stays on the ordinary
git add/git commitshell path, where the approval gate already applies.
ApprovalRequirement::Manualvariant is gone; the tooldeclares
ReadOnly+Sandboxableand classifies read-only end to end(envelope
Bounded, approval policy Safe/Benign, hooks gatesafe,history activity
File).tests, including a propose-only proof that the index,
HEAD, and theuntracked list are untouched after planning.
[Unreleased]crediting @goransh-walia.Gates on the final tree:
cargo fmt --allclean;cargo clippy --workspace --all-targets --all-features --locked -D warningsclean; targetedtools::git git_tool commit_plan canonical_action49 passed / 0 failed /0 ignored; full
cargo test -p codewhale-tui --lib --locked11880 passed /0 failed / 13 ignored.