Skip to content

fix: prevent TypeScript import merge corruption (#24) #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
edouard-andrei wants to merge 2 commits into
base: main
Choose a base branch
from
+234 −50
Open

fix: prevent TypeScript import merge corruption (#24) #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
423ba12
fix: prevent TS import merge corruption (#24)
edouard-andrei Jun 18, 2026
fef57f0
Harden named import parsing against malformed braces
edouard-andrei Jun 18, 2026
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
158 changes: 118 additions & 40 deletions crates/weave-core/src/merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,6 @@ fn merge_interstitials(
let base_content = base_map.get(key).copied().unwrap_or("");
let ours_content = ours_map.get(key).copied().unwrap_or("");
let theirs_content = theirs_map.get(key).copied().unwrap_or("");

// If all same, no merge needed
if ours_content == theirs_content {
merged.insert(key.to_string(), ours_content.to_string());
Expand Down Expand Up @@ -1630,12 +1629,14 @@ fn parse_single_line_specifiers(trimmed: &str) -> Vec<String> {
if trimmed.starts_with("import ") {
if let Some(brace_start) = trimmed.find('{') {
if let Some(brace_end) = trimmed.find('}') {
let inner = &trimmed[brace_start + 1..brace_end];
return inner
.split(',')
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.collect();
if brace_start < brace_end {
let inner = &trimmed[brace_start + 1..brace_end];
return inner
.split(',')
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.collect();
}
}
}
}
Expand Down Expand Up @@ -1765,8 +1766,8 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String {
.collect();

// Build import groups from ours (import lines only)
let mut groups: Vec<Vec<&str>> = Vec::new();
let mut current_group: Vec<&str> = Vec::new();
let mut groups: Vec<Vec<String>> = Vec::new();
let mut current_group: Vec<String> = Vec::new();

for line in ours.lines() {
if line.trim().is_empty() {
Expand All @@ -1778,7 +1779,7 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String {
if theirs_deleted.contains(line) {
continue;
}
current_group.push(line);
current_group.push(line.to_string());
}
}
if !current_group.is_empty() {
Expand All @@ -1792,19 +1793,34 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String {
} else {
groups.len() - 1
};
let mut matching_line = None;
for (i, group) in groups.iter().enumerate() {
if group
if let Some(pos) = group
.iter()
.any(|l| is_import_line(l) && import_source_prefix(l) == prefix)
.position(|l| is_import_line(l) && import_source_prefix(l) == prefix)
{
best_group = i;
matching_line = Some(pos);
break;
}
}
if let Some(pos) = matching_line {
let base_specs: Vec<String> = base_imports
.iter()
.filter(|i| i.source == prefix)
.flat_map(|i| i.specifiers.iter().cloned())
.collect();
if let Some(merged) =
merge_same_source_named_import_line(&groups[best_group][pos], add, &base_specs)
{
groups[best_group][pos] = merged;
continue;
}
}
if best_group < groups.len() {
groups[best_group].push(add);
groups[best_group].push(add.to_string());
} else {
groups.push(vec![add]);
groups.push(vec![add.to_string()]);
}
}

Expand All @@ -1813,12 +1829,12 @@ fn merge_imports_commutatively(base: &str, ours: &str, theirs: &str) -> String {
group.sort_unstable();
}

let mut import_lines: Vec<&str> = Vec::new();
let mut import_lines: Vec<String> = Vec::new();
for (i, group) in groups.iter().enumerate() {
if i > 0 {
import_lines.push("");
import_lines.push(String::new());
}
import_lines.extend(group);
import_lines.extend(group.iter().cloned());
}

let import_block = import_lines.join("\n");
Expand Down Expand Up @@ -2137,35 +2153,16 @@ fn merge_imports_with_multiline(
// Non-import lines: use diffy 3-way merge so adds/deletes/edits on
// either side are handled correctly (fixes #60).
let extract_non_imports = |content: &str| -> String {
content
.lines()
.filter(|l| !l.trim().is_empty() && !is_import_line(l))
.filter(|l| {
let t = l.trim();
// Exclude multi-line import continuation lines:
// - specifier lines ending with comma (but not assignments)
// - bare closing parens/braces
// - closing lines like `} from "./foo"` or `) from "bar"`
if (t.ends_with(',') && !t.contains('=')) || t == ")" || t == "}" {
return false;
}
// Closing line of JS/TS multi-line import: `} from "..."` or `} from '...'`
if t.starts_with('}') && t.contains("from ") {
return false;
}
// Closing line of Python multi-line import: `) ` at end or just `)`
if t.starts_with(')') {
return false;
}
true
})
let (_, non_import_lines) = parse_import_statements(content);
non_import_lines
.into_iter()
.filter(|l| !l.trim().is_empty())
.collect::<Vec<_>>()
.join("\n")
};
let base_ni = extract_non_imports(_base_raw);
let ours_ni = extract_non_imports(ours_raw);
let theirs_ni = extract_non_imports(_theirs_raw);

if !base_ni.is_empty() || !ours_ni.is_empty() || !theirs_ni.is_empty() {
let merged_ni = match diffy::merge(&base_ni, &ours_ni, &theirs_ni) {
Ok(m) => m,
Expand All @@ -2186,6 +2183,79 @@ fn merge_imports_with_multiline(
result
}

fn named_import_parts(line: &str) -> Option<(&str, &str)> {
let trimmed_start = line.len() - line.trim_start().len();
let trimmed = &line[trimmed_start..];

if let Some(open) = trimmed.find('{') {
let close = trimmed.rfind('}')?;
let from = trimmed.find(" from ")?;
if open < close && close < from {
return Some((
&line[..trimmed_start + open + 1],
&line[trimmed_start + close..],
));
}
}

if trimmed.starts_with("from ") {
if let Some(pos) = trimmed.find(" import ") {
let prefix_end = trimmed_start + pos + " import ".len();
return Some((&line[..prefix_end], ""));
}
}

None
}

fn merge_same_source_named_import_line(
ours: &str,
theirs: &str,
base_specs: &[String],
) -> Option<String> {
let ours_specs = parse_single_line_specifiers(ours.trim());
let theirs_specs = parse_single_line_specifiers(theirs.trim());
if ours_specs.is_empty() || theirs_specs.is_empty() {
return None;
}

let (ours_prefix, ours_suffix) = named_import_parts(ours)?;
let (theirs_prefix, theirs_suffix) = named_import_parts(theirs)?;
if ours_prefix.trim() != theirs_prefix.trim() || ours_suffix.trim() != theirs_suffix.trim() {
return None;
}

let base_set: HashSet<&str> = base_specs.iter().map(String::as_str).collect();
let theirs_set: HashSet<&str> = theirs_specs.iter().map(String::as_str).collect();
let theirs_removed: HashSet<&str> = base_set.difference(&theirs_set).copied().collect();

let mut final_specs: Vec<String> = ours_specs
.into_iter()
.filter(|s| !theirs_removed.contains(s.as_str()))
.collect();

for spec in theirs_specs {
if !base_set.contains(spec.as_str()) && !final_specs.contains(&spec) {
final_specs.push(spec);
}
}

if final_specs.is_empty() {
return None;
}

if ours_suffix.is_empty() {
Some(format!("{}{}", ours_prefix, final_specs.join(", ")))
} else {
Some(format!(
"{} {} {}",
ours_prefix,
final_specs.join(", "),
ours_suffix.trim_start()
))
}
}

/// Extract the source/module prefix from an import line for group matching.
/// e.g. "from collections import OrderedDict" -> "collections"
/// "import React from 'react'" -> "react"
Expand Down Expand Up @@ -4079,6 +4149,14 @@ export function agentB() {
assert!(!is_import_line("function foo() {}"));
}

#[test]
fn test_named_import_parsing_rejects_reversed_braces() {
let malformed = "import } foo { from 'bar';";
assert!(parse_single_line_specifiers(malformed).is_empty());
assert!(named_import_parts(malformed).is_none());
assert!(named_import_parts("import { foo } from 'bar';").is_some());
}

#[test]
fn test_commutative_import_merge_both_add_different() {
// The key scenario: both branches add different imports
Expand Down
126 changes: 116 additions & 10 deletions crates/weave-core/tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2860,10 +2860,10 @@ fn json_both_add_keys_in_different_sections() {
}

// ---------------------------------------------------------------------------
// 7. TS: both modify the same import line differently (conflict expected)
// 7. TS: both add different specifiers to the same single-line import (auto-merge)
// ---------------------------------------------------------------------------
#[test]
fn ts_both_modify_same_import_conflict() {
fn ts_both_modify_same_import_merges_named_specifiers() {
let base = r#"import { foo } from './utils';

export function run() {
Expand All @@ -2883,14 +2883,120 @@ export function run() {
}
"#;
let result = entity_merge(base, ours, theirs, "run.ts");
// Both modify the same import line — the commutative import merger should
// handle this (both add to same import source). The key check is that
// neither bar nor baz is silently dropped.
let has_bar = result.content.contains("bar");
let has_baz = result.content.contains("baz");
assert!(
has_bar && has_baz,
"Both bar and baz must be present (merged or conflicted).\nContent:\n{}",
assert!(
result.is_clean(),
"same-source import specifier additions should auto-resolve.\nConflicts: {:?}\nContent:\n{}",
result.conflicts,
result.content
);
assert_eq!(
result.content.matches("from './utils'").count(),
1,
"same-source import must not be duplicated.\nContent:\n{}",
result.content
);
assert!(
result
.content
.contains("import { foo, bar, baz } from './utils';"),
"Both bar and baz must be merged into one import.\nContent:\n{}",
result.content
);
}

#[test]
fn ts_multiline_imports_without_trailing_commas_do_not_leak_specifiers() {
fn has_orphan_import_member(content: &str) -> bool {
let mut depth = 0_i32;
for line in content.lines() {
let t = line.trim();
let indented = line.starts_with(' ') || line.starts_with('\t');
let specifier_like = indented
&& (t.ends_with(',') || t.starts_with("type "))
&& !t.contains('=')
&& !t.contains('(');
if depth == 0 && specifier_like {
return true;
}
depth += line.matches('{').count() as i32;
depth -= line.matches('}').count() as i32;
if depth < 0 {
return true;
}
}
false
}

let base = r#"import {
type A
} from "./file1"
import {
type B
} from "./file2"
import {
type C
} from "./file3"

export function main() { return 1; }
"#;
let ours = r#"import {
type A,
type A2
} from "./file1"
import {
type B
} from "./file2"
import {
type C
} from "./file3"

export function main() { return 1; }
"#;
let theirs = r#"import {
type A
} from "./file1"
import {
type B
} from "./file2"
import {
type C,
type C2
} from "./file3"

export function main() { return 1; }
"#;

let result = entity_merge(base, ours, theirs, "imports.ts");
assert!(
result.is_clean(),
"multi-line import additions should auto-resolve.\nConflicts: {:?}\nContent:\n{}",
result.conflicts,
result.content
);
assert!(
!result.content.contains("<<<<<<<"),
"import merge must not introduce conflict markers.\nContent:\n{}",
result.content
);
assert!(
!has_orphan_import_member(&result.content),
"import specifier/closer leaked outside an import block.\nContent:\n{}",
result.content
);
for source in ["./file1", "./file2", "./file3"] {
assert_eq!(
result
.content
.matches(&format!("from \"{source}\""))
.count(),
1,
"source {source} should appear exactly once.\nContent:\n{}",
result.content
);
}
assert!(
result.content.contains("type A2,") && result.content.contains("type C2,"),
"both sides' added specifiers must be present.\nContent:\n{}",
result.content
);
}
Expand Down