Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions crates/aft/src/commands/edit_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,12 @@ pub fn handle_edit_match(req: &RawRequest, ctx: &AppContext) -> Response {
// sequences before the string reaches us. Adding unescape_str on top caused
// double-interpretation that corrupted source code with literal escapes.

// Detect glob pattern
if is_glob_pattern(file) {
// Detect glob pattern. Prefer the literal interpretation when the path
// already exists on disk, even if its name contains glob metacharacters
// such as `[`, `]`, `*`, `?`, or `{`. This prevents files in directories
// with brackets (e.g. `src/[another]/file.rs`) from being misclassified as
// globs.
if should_treat_as_glob(file, ctx) {
return handle_glob_edit_match(req, ctx, file, match_str, replacement, &op_id);
}

Expand Down Expand Up @@ -368,6 +372,36 @@ fn is_glob_pattern(path: &str) -> bool {
path.contains('*') || path.contains('?') || path.contains('{') || path.contains('[')
}

/// Returns true when `file` should be treated as a glob pattern rather than a
/// literal path. A path that exists on disk is always treated literally, even
/// if its name contains glob metacharacters such as `[`, `]`, `*`, `?`, or `{`.
/// This defends against directories or files with brackets in their
/// names being misclassified as glob patterns (see issue #132).
///
/// Resolution mirrors `ctx.validate_path` so the literal-vs-glob decision stays
/// consistent with the single-file path handler's interpretation.
fn should_treat_as_glob(file: &str, ctx: &AppContext) -> bool {
if !is_glob_pattern(file) {
return false;
}
match ctx.validate_path("literal-check", Path::new(file)) {
Ok(candidate) => !candidate.exists(),
Err(resp)
if resp.data.get("code").and_then(|c| c.as_str()) == Some("path_outside_root") =>
{
false
}
Err(resp) => {
log::debug!(
"edit_match: validate_path failed for '{}', deferring to single-file handler: {:?}",
file,
resp.data
);
false
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}
}

/// Handle a glob-based multi-file edit_match.
fn handle_glob_edit_match(
req: &RawRequest,
Expand Down
144 changes: 144 additions & 0 deletions crates/aft/tests/integration/edit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,150 @@ fn edit_match_no_match() {
assert!(status.success());
}

// Regression for #132: edit_match must treat literal paths with brackets as
// single-file edits, not glob patterns. Brackets (`[]`) are glob metacharacters,
// so the old character-detection heuristic misclassified real file paths.
#[test]
fn edit_match_handles_brackets_in_literal_path() {
let mut aft = AftProcess::spawn();
let dir = tempfile::tempdir().unwrap();
let subdir = dir.path().join("[another]");
fs::create_dir_all(&subdir).unwrap();
let target = subdir.join("file.ts");
fs::write(&target, "const value = OLD;\n").unwrap();

let req = serde_json::json!({
"id": "em-brackets",
"command": "edit_match",
"file": target.display().to_string(),
"match": "OLD",
"replacement": "NEW"
});
let resp = aft.send(&serde_json::to_string(&req).unwrap());

assert_eq!(
resp["success"], true,
"edit_match should succeed for path with brackets: {:?}",
resp
);
assert_eq!(resp["replacements"], 1);
let content = fs::read_to_string(&target).unwrap();
assert!(
content.contains("NEW"),
"replacement should apply: {}",
content
);
assert!(
!content.contains("OLD"),
"original text should be gone: {}",
content
);

let status = aft.shutdown();
assert!(status.success());
}

// Sanity check: parentheses are also called out in the issue, but they are
// not glob metacharacters, so they were never misclassified. This test only
// ensures literal paths containing them remain single-file edits.
#[test]
fn edit_match_handles_parentheses_in_literal_path() {
let mut aft = AftProcess::spawn();
let dir = tempfile::tempdir().unwrap();
let subdir = dir.path().join("(something)");
fs::create_dir_all(&subdir).unwrap();
let target = subdir.join("file.ts");
fs::write(&target, "const value = OLD;\n").unwrap();

let req = serde_json::json!({
"id": "em-parens",
"command": "edit_match",
"file": target.display().to_string(),
"match": "OLD",
"replacement": "NEW"
});
let resp = aft.send(&serde_json::to_string(&req).unwrap());

assert_eq!(
resp["success"], true,
"edit_match should succeed for path with parentheses: {:?}",
resp
);
assert_eq!(resp["replacements"], 1);
let content = fs::read_to_string(&target).unwrap();
assert!(
content.contains("NEW"),
"replacement should apply: {}",
content
);
assert!(
!content.contains("OLD"),
"original text should be gone: {}",
content
);

let status = aft.shutdown();
assert!(status.success());
}

// Regression for #132: relative paths with brackets must also be treated as
// literal single-file edits when resolved against the configured project root.
#[test]
fn edit_match_handles_brackets_in_relative_literal_path() {
let mut aft = AftProcess::spawn();
let dir = tempfile::tempdir().unwrap();
let root = dir.path();
let subdir = root.join("[another]");
fs::create_dir_all(&subdir).unwrap();
let target = subdir.join("file.ts");
fs::write(&target, "const value = OLD;\n").unwrap();

let configure = aft.send(
&serde_json::json!({
"id": "cfg-restrict",
"command": "configure",
"harness": "opencode",
"project_root": root.to_string_lossy(),
"config": user_config(serde_json::json!({ "restrict_to_project_root": true }))
})
.to_string(),
);
assert_eq!(
configure["success"], true,
"configure should succeed: {configure:?}"
);

let req = serde_json::json!({
"id": "em-brackets-relative",
"command": "edit_match",
"file": "[another]/file.ts",
"match": "OLD",
"replacement": "NEW"
});
let resp = aft.send(&serde_json::to_string(&req).unwrap());

assert_eq!(
resp["success"], true,
"edit_match should succeed for relative path with brackets: {:?}",
resp
);
assert_eq!(resp["replacements"], 1);
let content = fs::read_to_string(&target).unwrap();
assert!(
content.contains("NEW"),
"replacement should apply: {}",
content
);
assert!(
!content.contains("OLD"),
"original text should be gone: {}",
content
);

let status = aft.shutdown();
assert!(status.success());
}

// ============================================================================
// batch command tests
// ============================================================================
Expand Down