fix: prevent TypeScript import merge corruption (#24)

[edouard-andrei]

Summary

Fixes #24. Weave's commutative import merge had two failure modes that silently corrupted TypeScript imports (exit 0, no conflict markers):

1. Same-source single-line edits were duplicated instead of merged. base: import { a } from "mod", ours: import { a, b }, theirs: import { a, c } produced two separate import … from "mod" lines instead of import { a, b, c } from "mod".

2. Multi-line import blocks leaked. extract_non_imports stripped import-continuation lines with a line heuristic that only matched specifier lines ending in a comma. A specifier without a trailing comma (and its closing } from "…") leaked into the non-import 3-way diff. With several multi-line imports from different sources this produced orphaned specifier lists with no import { opener and spurious conflicts — the symptom @kurtextrem reported.

Changes

• Merge same-source named specifiers into a single import line, source- and shape-aware: only when the import prefix/suffix match, otherwise fall back to the existing line-set insertion.
• Replace the heuristic in extract_non_imports with parse_import_statements, so import vs non-import classification is structural and consistent with how the import block is reconstructed.

Verification

Both failure modes were reproduced deterministically against main and confirmed fixed on this branch:

• mode (1) single-line duplicate → fixed
• mode (2) reporter shape (4 sources, specifiers without trailing commas) → orphaned specifier list on main, clean merge here
• a 16-combo multi-source multi-line fuzz → 10/16 corrupted on main, 0 here
• interleaved non-import content (e.g. a top-level const between imports) is preserved — not dropped, duplicated, or orphaned

Regression tests added for both modes. cargo fmt --check, cargo clippy -p weave-core --tests, and cargo test -p weave-core all pass.

cc @rs545837

[inspect-review]

inspect review

Triage: 8 entities analyzed | 0 critical, 0 high, 3 medium, 5 low
Verdict: standard_review

Findings (5)

1. [low] In merge_same_source_named_import_line, the function returns None when final_specs.is_empty(), but this can cause silent data loss. If both sides remove all specifiers from an import, the function returns None and the caller will add theirs as a new line instead of recognizing the import should be removed entirely. This leads to duplicate or incorrect import statements.
2. [low] In merge_imports_commutatively, when merge_same_source_named_import_line returns Some(merged), the code updates groups[best_group][pos] and continues without checking if the merged line is actually different from the original. More critically, if the merge succeeds, it skips adding add to the group, but add (from theirs) might have additional changes beyond just specifiers that are now lost.
3. [low] In named_import_parts, the function checks if open < from && close < from to validate brace positions, but this logic is incorrect. For a valid named import like import { foo } from './bar', close should be LESS than from, but open should also be less than from. However, the condition allows cases where the closing brace comes before 'from' which is correct, but doesn't validate that open comes before close, potentially matching malformed imports.
4. [low] In merge_same_source_named_import_line, the logic for detecting removed specifiers is flawed. It computes theirs_removed as specifiers in base but not in theirs, then filters ours to remove those. However, it doesn't check if ours also independently removed those same specifiers. If both sides remove the same specifier, it's correctly removed. But if ours keeps a specifier that theirs removed, the current code will remove it from the final result, which may not be the intended 3-way merge behavior (should potentially be a conflict).
5. [low] In merge_same_source_named_import_line, the function calls parse_single_line_specifiers on both ours and theirs, but then calls named_import_parts which may fail if the import format doesn't match expectations. If named_import_parts returns None after successfully parsing specifiers, the function returns None and the merge silently fails, potentially causing import duplication.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[inspect-review]

inspect review

Triage: 11 entities analyzed | 0 critical, 0 high, 5 medium, 6 low
Verdict: standard_review

Findings (5)

1. [low] In merge_imports_commutatively, the code attempts to merge same-source imports by calling merge_same_source_named_import_line and updating groups[best_group][pos], but then continues to the next iteration. However, if the merge fails (returns None), the code falls through to add the import again, potentially creating duplicates. The logic should only skip adding when merge succeeds.
2. [low] In named_import_parts, the function checks if open < close && close < from to validate brace positions, but this logic is incorrect for TypeScript imports. The from keyword comes AFTER the closing brace in import { foo } from 'bar', so close < from is correct. However, the function finds from using trimmed.find(" from ") which returns the position in trimmed, but then compares it with open and close which are also positions in trimmed. This is correct, but the condition should ensure all three are in the right order: open < close < from.
3. [low] In merge_same_source_named_import_line, when constructing the final merged import, the code uses ours_suffix.trim_start() in the else branch but doesn't trim ours_prefix consistently. If ours_prefix has trailing whitespace or ours_suffix has leading whitespace, the spacing in the output could be incorrect, potentially creating malformed imports like import { foo } } from 'bar' (double space before closing brace).
4. [low] In parse_single_line_specifiers, the new check if brace_start < brace_end prevents parsing when braces are reversed, but the function uses rfind('}') to find the closing brace. If there are multiple } characters in the line (e.g., in nested structures or comments), rfind will find the last one, which may not be the matching closing brace for the first {. This could cause incorrect parsing or false negatives.
5. [low] In named_import_parts, the function finds close using rfind('}') and from using find(" from "), then checks if open < close && close < from. However, if there are multiple occurrences of } or from in the string, this logic may incorrectly validate malformed imports. For example, import { foo } from 'bar' } from 'baz' would pass this check even though it's invalid.

---

Reviewed by inspect | Entity-level triage found 0 high-risk changes

[edouard-andrei]

Re: inspect review on fef57f0

Thanks — I checked these and I don't think another code change is warranted here.

• 1 describes the intended fallback: we only skip adding theirs when the same-shape merge succeeds. If it returns None, the shapes did not match, so keeping both lines is safer than collapsing them.
• 2 is the check added in fef57f0: open < close && close < from.
• 3 should still format correctly: the prefix ends at {, the suffix starts at }, and the constructor inserts one separator space around the merged specifier list.
• 4/5 are malformed-import edge cases. The helper remains shape-conservative there: if parts don't validate, it falls back instead of merging.

The #24 regressions are covered by tests, and the branch is green with cargo fmt --check, cargo test -p weave-core, and cargo clippy -p weave-core --tests.