diff --git a/docs/EXAMPLES.md b/docs/EXAMPLES.md index 656288d..183e684 100644 --- a/docs/EXAMPLES.md +++ b/docs/EXAMPLES.md @@ -4,9 +4,9 @@ Concrete examples for both audiences: humans at a terminal and AI agents consuming `--json`. All outputs below are captured from the real binary. Flags (see `jmove --help`): `mv [--dry-run] [--json] -[--no-git]`, `check [--json]`, and the global `--root `. Paths may be -relative to the root or absolute inside it; `.gitignore`d files are never -indexed. Inside a git repo, `mv` of a tracked file uses `git mv` (the +[--no-git]`, `check [--json]`, and the global `--root ` / +`--source-root ` (index one subtree only — the monorepo disambiguator +for duplicate Java packages). `.gitignore`d files are never indexed. Inside a git repo, `mv` of a tracked file uses `git mv` (the rename is staged); `--no-git` forces a plain filesystem rename. Exit codes: `0` ok · `1` operation error · `2` `check` found broken imports. diff --git a/docs/SKILL.md b/docs/SKILL.md index 1b05d81..f3e0cf5 100644 --- a/docs/SKILL.md +++ b/docs/SKILL.md @@ -18,7 +18,7 @@ kept in sync); Python/Go on the roadmap. Single binary, no LSP needed. ### mv — move a file and rewrite its importers ``` -jmove mv [--root DIR] [--dry-run] [--json] [--no-git] +jmove mv [--root DIR] [--source-root DIR] [--dry-run] [--json] [--no-git] ``` Always run `--dry-run` first and confirm the change set looks right. @@ -80,6 +80,12 @@ without `--dry-run`. For Java projects the practical loop is: `mv` → - `check` never reports unresolved Java imports (jdk, third-party, `pkg.*`) as broken — they are external by design, like TS bare specifiers. +- Monorepos with duplicate packages (`guava` vs `android/guava` under one + `--root`): the same FQN exists in parallel trees, so resolution picks the + sorted-first copy and a move rewrites the wrong files. Pass the global + `--source-root DIR` to index (and fix/move) only that subtree; each tree + is then self-contained and correct. Without the flag behavior is + unchanged; `--source-root` on a non-directory exits 1. ## Recommended agent workflow diff --git a/src/cli/fix.rs b/src/cli/fix.rs index 961458b..40daca4 100644 --- a/src/cli/fix.rs +++ b/src/cli/fix.rs @@ -71,12 +71,20 @@ pub struct FixDryRunData { } /// `fix` handler: validate rule, index, plan, then dry-run or apply. -pub fn fix(root: &Path, rule: Option<&str>, dry_run: bool, json: bool) -> Flow { +pub fn fix( + root: &Path, + source_root: Option<&Path>, + rule: Option<&str>, + dry_run: bool, + json: bool, +) -> Flow { let root = flow(json, "fix", root.canonicalize().map_err(JmoveError::from))?; + let source_root = flow(json, "fix", super::normalize_scope(&root, source_root))?; if let Some(rejected) = fix_reject(rule) { return Err(fail(json, "fix", rejected)); } - let index = flow(json, "fix", Index::build(&root))?; + let scope = source_root.as_deref(); + let index = flow(json, "fix", Index::build_scoped(&root, scope))?; let plan = plan_fix(&index, rule); if plan.is_empty() { if json { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 8d3a470..58d6049 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -29,6 +29,10 @@ pub struct Args { /// Project root (defaults to the current directory). #[arg(long, global = true, default_value = ".")] pub root: PathBuf, + /// Index only files under this root-relative directory: the monorepo + /// disambiguator for duplicate Java packages (`guava` vs `android/guava`). + #[arg(long, global = true)] + pub source_root: Option, } /// Available subcommands (MVP: `mv`, `check`). @@ -95,13 +99,27 @@ pub fn run() -> anyhow::Result { dry_run, json, no_git, - } => mv(&args.root, &source, &target, dry_run, json, no_git), - Command::Check { json } => check(&args.root, json), + } => mv( + &args.root, + args.source_root.as_deref(), + &source, + &target, + dry_run, + json, + no_git, + ), + Command::Check { json } => check(&args.root, args.source_root.as_deref(), json), Command::Fix { rule, dry_run, json, - } => fix::fix(&args.root, rule.as_deref(), dry_run, json), + } => fix::fix( + &args.root, + args.source_root.as_deref(), + rule.as_deref(), + dry_run, + json, + ), }; // Handlers report their own failures; both arms carry an exit code. Ok(outcome.unwrap_or_else(identity)) @@ -110,6 +128,7 @@ pub fn run() -> anyhow::Result { /// `mv` handler: normalize paths, validate, index, plan, then dry-run or apply. fn mv( root: &Path, + source_root: Option<&Path>, source: &Path, target: &Path, dry_run: bool, @@ -118,13 +137,15 @@ fn mv( ) -> Flow { let git = apply::GitMode::from_no_git(no_git); let root = flow(json, "mv", root.canonicalize().map_err(JmoveError::from))?; + let source_root = flow(json, "mv", normalize_scope(&root, source_root))?; let source = flow(json, "mv", core::rel_from_root(&root, source))?; let target = flow(json, "mv", core::rel_from_root(&root, target))?; - if let Some(rejected) = mv_reject(&root, &source, &target) { + if let Some(rejected) = output::mv_reject(&root, &source, &target) { return Err(fail(json, "mv", rejected)); } - let index = flow(json, "mv", Index::build(&root))?; + let scope = source_root.as_deref(); + let index = flow(json, "mv", Index::build_scoped(&root, scope))?; let plan = flow(json, "mv", plan::plan_move(&index, &source, &target))?; if dry_run { return mv_dry_run(&root, json, &plan, git); @@ -149,48 +170,6 @@ fn mv( Ok(exit::OK) } -/// Pre-flight `mv` validation. A file that exists on disk but is absent -/// from the import index stays moveable: its plan simply has no rewrites. -fn mv_reject(root: &Path, source: &Path, target: &Path) -> Option { - let bad = |code: &str, message: String, hint: &str| { - Some(ErrorData::new(code, message, Some(hint.into()))) - }; - if source == target { - let msg = "source and target are the same path".into(); - return bad("INVALID_ARGUMENT", msg, "pick a different destination"); - } - if !root.join(source).is_file() { - let msg = format!("source file '{}' does not exist", core::rel_str(source)); - return bad( - "SOURCE_NOT_FOUND", - msg, - "check the path or run `jmove check`", - ); - } - if root.join(target).exists() { - let msg = format!("target path '{}' already exists", core::rel_str(target)); - return bad( - "TARGET_EXISTS", - msg, - "remove or rename the existing target first", - ); - } - // `target` names a file, so `parent()` always yields the directory part. - let parent = root.join(target.parent().unwrap_or(Path::new(""))); - if parent.exists() && !parent.is_dir() { - let msg = format!( - "target parent of '{}' is not a directory", - core::rel_str(target) - ); - return bad( - "INVALID_ARGUMENT", - msg, - "pick a destination inside a directory", - ); - } - None -} - /// Dry-run branch: unified diff for humans, structured preview for agents. fn mv_dry_run(root: &Path, json: bool, plan: &MovePlan, git: apply::GitMode) -> Flow { let diff = flow(json, "mv", apply::render_diff(root, plan))?; @@ -211,9 +190,11 @@ fn mv_dry_run(root: &Path, json: bool, plan: &MovePlan, git: apply::GitMode) -> /// Exit code is `2` when at least one broken import was found, in both the /// human and the `--json` mode (the JSON `status` stays `"ok"` — the /// command itself succeeded; agents read `total` or the exit code). -fn check(root: &Path, json: bool) -> Flow { +fn check(root: &Path, source_root: Option<&Path>, json: bool) -> Flow { let root = flow(json, "check", root.canonicalize().map_err(JmoveError::from))?; - let index = flow(json, "check", Index::build(&root))?; + let source_root = flow(json, "check", normalize_scope(&root, source_root))?; + let scope = source_root.as_deref(); + let index = flow(json, "check", Index::build_scoped(&root, scope))?; let broken = flow(json, "check", output::broken_imports(&root, &index))?; let code = if broken.is_empty() { exit::OK @@ -234,6 +215,21 @@ fn check(root: &Path, json: bool) -> Flow { Ok(code) } +/// Validate the global `--source-root`: project-relative, existing dir. +fn normalize_scope(root: &Path, scope: Option<&Path>) -> JmoveResult> { + let Some(scope) = scope else { + return Ok(None); + }; + let rel = core::rel_from_root(root, scope)?; + if !root.join(&rel).is_dir() { + return Err(JmoveError::InvalidArgument(format!( + "--source-root '{}' is not a directory", + core::rel_str(&rel) + ))); + } + Ok(Some(rel)) +} + /// Unwrap a core result, routing failures through the CLI error channel. fn flow(json: bool, operation: &'static str, result: JmoveResult) -> Flow { result.map_err(|err| fail(json, operation, ErrorData::from_core(&err))) diff --git a/src/cli/output.rs b/src/cli/output.rs index 4fa23d6..10cf3b1 100644 --- a/src/cli/output.rs +++ b/src/cli/output.rs @@ -11,7 +11,7 @@ use crate::core::index::Index; use crate::core::plan::{MovePlan, Rewrite}; use crate::core::{JmoveResult, rel_str}; -use super::json::{BrokenImport, Change, ChangedFile}; +use super::json::{BrokenImport, Change, ChangedFile, ErrorData}; /// `check` stdout line when the project has no broken imports. const CHECK_CLEAN: &str = "check: no broken imports found"; @@ -95,6 +95,47 @@ pub fn changed_files(root: &Path, plan: &MovePlan) -> JmoveResult Option { + let bad = |code: &str, message: String, hint: &str| { + Some(ErrorData::new(code, message, Some(hint.into()))) + }; + if source == target { + let msg = "source and target are the same path".into(); + return bad("INVALID_ARGUMENT", msg, "pick a different destination"); + } + if !root.join(source).is_file() { + let msg = format!("source file '{}' does not exist", rel_str(source)); + return bad( + "SOURCE_NOT_FOUND", + msg, + "check the path or run `jmove check`", + ); + } + if root.join(target).exists() { + let msg = format!("target path '{}' already exists", rel_str(target)); + return bad( + "TARGET_EXISTS", + msg, + "remove or rename the existing target first", + ); + } + // `target` names a file, so `parent()` always yields the directory part. + let parent = root.join(target.parent().unwrap_or(Path::new(""))); + if parent.exists() && !parent.is_dir() { + let msg = format!("target parent of '{}' is not a directory", rel_str(target)); + return bad( + "INVALID_ARGUMENT", + msg, + "pick a destination inside a directory", + ); + } + None +} + /// `moved src -> tgt, updated N imports in M files` success summary, /// noting when the rename went through `git mv`. #[must_use] diff --git a/src/core/index/mod.rs b/src/core/index/mod.rs index 12965f9..83aa37e 100644 --- a/src/core/index/mod.rs +++ b/src/core/index/mod.rs @@ -54,6 +54,15 @@ impl Index { /// Scan `root`, parse every supported source file and build the graph. /// Unreadable or unparseable files are skipped, not fatal. pub fn build(root: &Path) -> JmoveResult { + Self::build_scoped(root, None) + } + + /// Like [`build`](Self::build), but when `source_root` (project-relative, + /// normalized) is `Some`, only files under that subtree are indexed. This + /// is the monorepo escape hatch: `guava` vs `android/guava` declare the + /// same FQNs, and scoping the index to one self-contained copy makes + /// collision-free resolution (and therefore `mv`/`fix` rewrites) exact. + pub fn build_scoped(root: &Path, source_root: Option<&Path>) -> JmoveResult { let root = root.canonicalize()?; let mut index = Self { root, @@ -62,7 +71,7 @@ impl Index { packages: HashMap::new(), java_classes: JavaClassIndex::default(), }; - index.scan()?; + index.scan(source_root)?; // Resolution needs the complete file set (extension/index guessing) // and the full package map, so it runs as a second pass. let java_classes = JavaClassIndex::new(&index.files, &index.packages); @@ -83,11 +92,17 @@ impl Index { } // Walk the project and parse each supported source file, staging the - // raw records with `target: None` for the resolution pass above. - fn scan(&mut self) -> JmoveResult<()> { - // Sorted map: deterministic discovery order. + // raw records with `target: None` for the resolution pass above. When + // `source_root` (project-relative, normalized) is set, only files under + // that subtree are indexed — see [`Index::build_scoped`]. + fn scan(&mut self, source_root: Option<&Path>) -> JmoveResult<()> { + // Sorted map: deterministic discovery order. Walking starts at the + // scoped subtree when set, so the rest of the monorepo is not even + // opened; paths stay root-relative because the prefix removed is + // always `self.root`. let mut found: BTreeMap = BTreeMap::new(); - for entry in WalkBuilder::new(&self.root).require_git(false).build() { + let base = source_root.map_or(self.root.clone(), |scope| self.root.join(scope)); + for entry in WalkBuilder::new(base).require_git(false).build() { // Walker errors (unreadable dirs, etc.) simply skip the entry. let Ok(entry) = entry else { continue }; if entry.path_is_symlink() || !entry.file_type().is_some_and(|t| t.is_file()) { diff --git a/src/core/index/tests.rs b/src/core/index/tests.rs index 57019b6..92e873e 100644 --- a/src/core/index/tests.rs +++ b/src/core/index/tests.rs @@ -119,3 +119,81 @@ fn importers_of_returns_sorted_reverse_edges() -> JmoveResult<()> { assert!(index.importers_of(Path::new("missing.ts")).is_empty()); Ok(()) } + +// The guava monorepo shape: `android/guava` and `guava` both declare +// `com.example.App`, so an unscoped index resolves every importer to the +// same (sorted-first) copy and a move in one tree rewrites the wrong files. +fn monorepo() -> JmoveResult { + let dir = tempfile::TempDir::new()?; + let root = dir.path(); + for tree in ["guava", "android/guava"] { + write_file( + root, + &format!("{tree}/src/com/example/App.java"), + "package com.example;\npublic class App {}\n", + )?; + write_file( + root, + &format!("{tree}/src/com/example/Use.java"), + "package com.example;\nimport com.example.App;\nclass Use { App a; }\n", + )?; + } + Ok(dir) +} + +fn resolved_target(index: &Index, importer: &str) -> Option { + index + .imports + .get(Path::new(importer))? + .iter() + .find_map(|r| r.target.clone()) +} + +#[test] +fn unscoped_index_resolves_the_duplicate_fqn_to_the_sorted_first_tree() -> JmoveResult<()> { + let dir = monorepo()?; + let index = Index::build(dir.path())?; + // Both trees' `App` collide; the class index keeps `android/guava` (sorts first). + assert_eq!( + resolved_target(&index, "guava/src/com/example/Use.java").as_deref(), + Some(Path::new("android/guava/src/com/example/App.java")), + "demonstrates the collision the flag exists to fix" + ); + Ok(()) +} + +#[test] +fn source_root_scopes_indexing_so_the_fqn_resolves_within_the_tree() -> JmoveResult<()> { + let dir = monorepo()?; + let index = Index::build_scoped(dir.path(), Some(Path::new("guava")))?; + // Only the guava tree is indexed at all. + assert!( + index + .files + .contains(Path::new("guava/src/com/example/App.java")) + ); + assert!( + !index + .files + .contains(Path::new("android/guava/src/com/example/App.java")) + ); + assert_eq!( + resolved_target(&index, "guava/src/com/example/Use.java").as_deref(), + Some(Path::new("guava/src/com/example/App.java")), + "the scoped index resolves to the same tree" + ); + Ok(()) +} + +#[test] +fn source_root_does_not_leak_siblings_sharing_a_name_prefix() -> JmoveResult<()> { + // `--source-root guava` must match the directory `guava`, not `guavaX`. + let dir = tempfile::TempDir::new()?; + let root = dir.path(); + write_file(root, "guava/src/A.java", "package p;\npublic class A {}\n")?; + write_file(root, "guavax/src/B.java", "package p;\npublic class B {}\n")?; + let index = Index::build_scoped(root, Some(Path::new("guava")))?; + assert!(index.files.contains(Path::new("guava/src/A.java"))); + assert!(!index.files.contains(Path::new("guavax/src/B.java"))); + Ok(()) +} diff --git a/tests/cli_java.rs b/tests/cli_java.rs index 04534d8..64ae171 100644 --- a/tests/cli_java.rs +++ b/tests/cli_java.rs @@ -149,3 +149,56 @@ fn default_package_file_cannot_change_directory() { .success(); assert!(in_root(tmp.path(), "src/main/java/Run.java").is_file()); } + +fn monorepo() -> tempfile::TempDir { + fixture("monorepo") +} + +#[test] +fn source_root_scopes_the_move_to_one_duplicate_tree() { + let tmp = monorepo(); + jmove( + &tmp, + &[ + "mv", + "--source-root", + "guava", + "guava/src/com/example/Primitives.java", + "guava/src/com/example/util/Primitives.java", + ], + ) + .success() + .stdout(predicate::str::contains("updated 2 imports in 2 files")); + + // The scoped tree moved and its importer followed the new package. + let app = read(&in_root(tmp.path(), "guava/src/com/example/app/App.java")); + assert!(app.contains("import com.example.util.Primitives;"), "{app}"); + let moved = read(&in_root( + tmp.path(), + "guava/src/com/example/util/Primitives.java", + )); + assert!(moved.contains("package com.example.util;"), "{moved}"); + + // The sibling copy is a self-contained tree: not one byte touched. + let android = read(&in_root( + tmp.path(), + "android/guava/src/com/example/app/App.java", + )); + assert!( + android.contains("import com.example.Primitives;"), + "{android}" + ); + assert!(in_root(tmp.path(), "android/guava/src/com/example/Primitives.java").is_file()); + jmove(&tmp, &["check", "--source-root", "guava"]).success(); +} + +#[test] +fn unknown_source_root_is_rejected() { + let tmp = monorepo(); + jmove(&tmp, &["check", "--source-root", "nope"]) + .failure() + .code(1) + .stderr(predicate::str::contains( + "--source-root 'nope' is not a directory", + )); +} diff --git a/tests/java/monorepo/android/guava/src/com/example/Primitives.java b/tests/java/monorepo/android/guava/src/com/example/Primitives.java new file mode 100644 index 0000000..f711bb9 --- /dev/null +++ b/tests/java/monorepo/android/guava/src/com/example/Primitives.java @@ -0,0 +1,3 @@ +package com.example; + +public class Primitives {} diff --git a/tests/java/monorepo/android/guava/src/com/example/app/App.java b/tests/java/monorepo/android/guava/src/com/example/app/App.java new file mode 100644 index 0000000..4e9380c --- /dev/null +++ b/tests/java/monorepo/android/guava/src/com/example/app/App.java @@ -0,0 +1,5 @@ +package com.example.app; + +import com.example.Primitives; + +public class App { Primitives p; } diff --git a/tests/java/monorepo/guava/src/com/example/Primitives.java b/tests/java/monorepo/guava/src/com/example/Primitives.java new file mode 100644 index 0000000..f711bb9 --- /dev/null +++ b/tests/java/monorepo/guava/src/com/example/Primitives.java @@ -0,0 +1,3 @@ +package com.example; + +public class Primitives {} diff --git a/tests/java/monorepo/guava/src/com/example/app/App.java b/tests/java/monorepo/guava/src/com/example/app/App.java new file mode 100644 index 0000000..4e9380c --- /dev/null +++ b/tests/java/monorepo/guava/src/com/example/app/App.java @@ -0,0 +1,5 @@ +package com.example.app; + +import com.example.Primitives; + +public class App { Primitives p; } diff --git a/todo.md b/todo.md index be0bb39..28fdd7b 100644 --- a/todo.md +++ b/todo.md @@ -74,9 +74,10 @@ AI оставляем СНАРУЖИ: при неоднозначности jmov - [ ] (после fix) повторить обе перемещения как `mv` + авто-`fix` и добить compile до SUCCESS (паттерн воспроизведён и закрыт локально: mv файла с bare-ссылкой на соседний пакет → `fix` добавил импорт → javac SUCCESS; на реальном guava ещё не прогонялось) -- [ ] Индексация в monorepo с дублями пакетов (guava vs android/guava в одном --root): - FQN-коллизии → class_index оставляет первый по сортировке, импорт резолвится не туда. - Нужен выбор/фильтр source root (например `--source-root` или авто-определение по mv-цели) +- [x] Индексация в monorepo с дублями пакетов (guava vs android/guava в одном --root): + глобальный `--source-root DIR` — индексирует (mv/check/fix) только поддерево, + FQN-коллизии исчезают, соседнее дерево не трогается; авто-определение по mv-цели + осознанно НЕ делаем (явный флаг предсказуемее, см. KISS) ## Phase 2 - [ ] Кэш индекса на диске (bincode/rkyv) → .jmove/index