Expose apply_patch preflight metadata#1971
Conversation
There was a problem hiding this comment.
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.
| "apply_patch" => preflight_apply_patch(input) | ||
| .map(|preflight| { | ||
| preflight | ||
| .touched_files | ||
| .into_iter() | ||
| .map(PathBuf::from) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
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()
}),| let patch_shape = inspect_patch_shape(patch_text); | ||
| validate_patch_shape(&patch_shape, path_override)?; |
There was a problem hiding this comment.
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.
| if file_patch.create_if_missing && !file_patch.delete_after { | ||
| push_unique(&mut creates, file_patch.path.clone()); | ||
| } |
There was a problem hiding this comment.
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).
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_patchinputs so harness layers can inspect intended file mutations before or around execution without reparsing ad hoc diffs.Changes
ApplyPatchPreflightandpreflight_apply_patch(&serde_json::Value).apply_patch.preflightmetadata to successfulapply_patchtool results.Validation
cargo fmt --checkcargo test -p codewhale-tui apply_patch(25 passed)cargo test -p codewhale-tui edited_paths_for_apply_patch(3 passed)cargo check --workspaceSupersedes #1634, which became stale after the v0.8.41 rebrand.