Skip to content

Expose apply_patch preflight metadata#1971

Open
kunpeng-ai-lab wants to merge 2 commits into
Hmbown:mainfrom
kunpeng-ai-lab:cdx/apply-patch-preflight-v0841
Open

Expose apply_patch preflight metadata#1971
kunpeng-ai-lab wants to merge 2 commits into
Hmbown:mainfrom
kunpeng-ai-lab:cdx/apply-patch-preflight-v0841

Conversation

@kunpeng-ai-lab
Copy link
Copy Markdown

Summary

Reopens the stale pre-v0.8.41 apply_patch preflight work from #1634 on current main.

This adds a no-mutation preflight surface for apply_patch inputs so harness layers can inspect intended file mutations before or around execution without reparsing ad hoc diffs.

Changes

  • Add ApplyPatchPreflight and preflight_apply_patch(&serde_json::Value).
  • Report touched files, file count, hunk count, creates, deletes, path override, and header mismatch.
  • Carry parsed apply_patch plans into execution to avoid double parsing.
  • Attach apply_patch.preflight metadata to successful apply_patch tool results.
  • Reuse the same preflight parser for LSP post-edit path discovery.
  • Preserve optional metadata fields by serializing from the serde output.
  • Add regression coverage for changes-list inputs, multi-file diffs, duplicate paths, header mismatch metadata, and invalid diffs.

Validation

  • cargo fmt --check
  • cargo test -p codewhale-tui apply_patch (25 passed)
  • cargo test -p codewhale-tui edited_paths_for_apply_patch (3 passed)
  • cargo check --workspace

Supersedes #1634, which became stale after the v0.8.41 rebrand.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a preflight mechanism for the apply_patch tool, allowing it to generate a summary of intended file changes—including touched, created, and deleted files—before any mutations occur. This summary is integrated into the tool's execution metadata and used by LSP hooks to identify edited paths. Review feedback highlights opportunities to improve the resilience of the LSP path extraction by adding fallbacks for parsing errors and filtering out deleted files. Further suggestions include optimizing performance by reducing redundant patch iterations and refining the 'creates' metadata to only include files explicitly marked as new in the diff headers.

Comment on lines +27 to +35
"apply_patch" => preflight_apply_patch(input)
.map(|preflight| {
preflight
.touched_files
.into_iter()
.map(PathBuf::from)
.collect()
})
.unwrap_or_default(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new implementation of edited_paths_for_tool for apply_patch is less resilient than the previous manual extraction. Previously, if the patch text was invalid or missing, the hook could still return paths from the path or files overrides. Now, because preflight_apply_patch returns a Result, any error in parsing the patch (e.g., missing headers or hunks) will cause the entire hook to return an empty list via unwrap_or_default().

Additionally, preflight.touched_files now includes files that are being deleted (where +++ /dev/null is used), whereas the previous implementation explicitly skipped them. Requesting LSP diagnostics for deleted files is unnecessary and may lead to noise or minor inefficiencies in the LSP manager.

        "apply_patch" => preflight_apply_patch(input)
            .map(|preflight| {
                preflight
                    .touched_files
                    .into_iter()
                    // Filter out deleted files to avoid unnecessary LSP diagnostic requests
                    .filter(|p| !preflight.deletes.contains(p))
                    .map(PathBuf::from)
                    .collect()
            })
            .unwrap_or_else(|_| {
                // Fallback for resilience: if preflight fails, try to extract at least the 'path' override
                input.get("path")
                    .and_then(|v| v.as_str())
                    .map(|p| vec![PathBuf::from(p)])
                    .unwrap_or_default()
            }),

Comment on lines +342 to +343
let patch_shape = inspect_patch_shape(patch_text);
validate_patch_shape(&patch_shape, path_override)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is redundant iteration over the patch content here. inspect_patch_shape scans the entire patch to validate its structure, but preflight_apply_patch_plan immediately follows this by calling parse_unified_diff or parse_unified_diff_files, which also scan the entire patch. For large multi-file patches, this double-parsing can be avoided by performing the validation on the results of the full parser instead.

Comment on lines +395 to +397
if file_patch.create_if_missing && !file_patch.delete_after {
push_unique(&mut creates, file_patch.path.clone());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The creates list in the preflight summary is overly broad when the global create_if_missing flag is set to true. In this case, every file modified by the patch (that isn't being deleted) will be included in creates, even if the file already exists. This makes the preflight metadata noisy and potentially misleading. The preflight should ideally only report files as 'created' if the diff headers explicitly indicate a new file (e.g., --- /dev/null).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant