Skip to content

Fix Maven local repository lookup (#423) - #1801

Merged
Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-maven-local-repo-resolution
Oct 29, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-maven-local-repo-resolution

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #423

Root cause

m2_repo_path_with (crates/socket-patch-core/src/crawlers/maven_crawler.rs) picks the Maven local repository that agent mode and -g work on. It doesn't match how Maven picks it:

  1. It uses $M2_HOME/repository. M2_HOME is the Maven install directory, and no Maven version keeps a local repository there.
  2. It never reads <localRepository> from the user's ~/.m2/settings.xml or the install's conf/settings.xml, or -Dmaven.repo.local from MAVEN_OPTS.
  3. On Windows it prefers HOME over USERPROFILE, but Java's user.home (which Maven uses) is the profile directory.

In each case the crawler scans an empty or missing directory. scan -g reports 0 packages, and rollback -g drops the patch record while the real local repository stays patched.

Plan

  • Resolve the local repository the way Maven does: MAVEN_REPO_LOCAL, then -Dmaven.repo.local in MAVEN_OPTS, then <localRepository> in user settings, then in global settings ($MAVEN_HOME/$M2_HOME conf/settings.xml), then <user.home>/.m2/repository. On Windows, user.home is USERPROFILE.
  • Regression tests for each trigger (M2_HOME, user settings, global settings, MAVEN_OPTS, Windows HOME≠USERPROFILE).

Progress

  • e33d5b5: the lookup now follows Maven's order. 20dd097 rewrites the integration test that asserted $M2_HOME/repository. dec7a9b reads <localRepository> as XML (CDATA, entities, not a profile property), takes -Duser.home from MAVEN_OPTS only, and drops the Maven variables from the CLI e2e children's environment.
  • Red→green: 7 of the 8 first tests in crawlers::maven_crawler::local_repo_tests failed on the old resolver. The eighth (unusable settings fall back to the default) passed there too. All pass with the fix: cargo test -p socket-patch-core --lib maven_crawler reports 71 passed. --test crawler_maven_e2e passes (28) and --test crawler_gradle_e2e passes (33).
  • cargo fmt --all -- --check and cargo clippy --workspace --all-features -- -D warnings are clean.
  • 5adf231: settings.xml is read FIFO-safely, which the crawler architecture test requires.
  • cargo test -p socket-patch-core --all-features: 6932 passed. Five failed. One was the architecture guard, fixed in 5adf231. The other four are chmod-based tests (copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_*, pypi_requirements::wire_failure_rolls_back_*) in files this PR doesn't touch. They fail only because this container runs as root, which ignores 0o555.
  • Not run yet: the socket-patch-cli test suites. Compiling them ran out of disk in the container.
  • 448fbe0: fixes both Bugbot findings. A comment inside <localRepository> no longer leaks into the path (new settings_local_repository_skips_comments, red before and green after), and the Maven crawler e2e harness clears M2_HOME. Full CI is green on 448fbe0 and Bugbot found no issues.

Review findings not fixed (/code-review high)

  • On Unix, user.home still prefers $HOME over the passwd entry. That predates this PR (B66 tests pin it), and the e2e harnesses rely on HOME for isolation. Changing it is a separate decision.
  • A Gradle-only build's mavenLocal() shares this lookup, so MAVEN_OPTS -Dmaven.repo.local also steers it. Gradle does read settings.xml the same way.
  • .mvn/maven.config, -s/-gs in MAVEN_ARGS, and shell-literal quoting in MAVEN_OPTS aren't modelled. The lookup has no project context.
  • Each JvmEnv::from_process re-reads up to two small settings files. That cost is negligible next to the crawl.

https://claude.ai/code/session_011f9aeNoGBTiq4ogGfW9bp8


Note

Medium Risk
Changes core JVM cache path resolution used by global scan and patching; behavior is heavily tested but misconfiguration could still affect which repo is crawled.

Overview
Fixes #423 by replacing the Maven local-repo resolver so agent mode and -g scan the same directory Maven uses, instead of wrongly treating $M2_HOME/repository as the cache.

m2_repo_path_with now follows Maven’s precedence: MAVEN_REPO_LOCAL, then -Dmaven.repo.local in MAVEN_ARGS / MAVEN_OPTS, then <localRepository> in user then global settings.xml (under MAVEN_HOME / M2_HOME), then user.home/.m2/repository. On Windows, user.home comes from USERPROFILE, not HOME. New helpers parse settings XML (CDATA, entities, ${…} interpolation, comments, ignoring profile properties named localRepository) and read settings FIFO-safely.

CLI JVM test isolation scrubs MAVEN_OPTS, MAVEN_ARGS, MAVEN_HOME, and M2_HOME so e2e runs don’t hit a developer’s real Maven config. docs/ecosystems.md documents the discovery order; integration tests and local_repo_tests cover the regression triggers.

Reviewed by Cursor Bugbot for commit a39bc39. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Agent mode and -g looked for the Maven local repository in
$M2_HOME/repository, which is the Maven install, and never read
<localRepository> from settings.xml. On Windows they also preferred
HOME over the profile directory Java uses. With any of these, scan -g
found 0 packages, and rollback -g dropped the patch record while the
real local repository stayed patched.

The lookup now follows Maven: MAVEN_REPO_LOCAL, then
-Dmaven.repo.local in MAVEN_ARGS or MAVEN_OPTS, then <localRepository>
in the user settings, then in the install's conf/settings.xml, then
.m2/repository under user.home. user.home honours -Duser.home and is
USERPROFILE on Windows. ${user.home} and ${env.X} expand in settings.

Fixes #423

Assisted-by: Claude Code:claude-opus-5-5
The integration test asserted the old $M2_HOME/repository lookup.
It now checks that $M2_HOME/repository is not scanned and that the
install's conf/settings.xml <localRepository> is.

Assisted-by: Claude Code:claude-opus-5-5
Maven reads a CDATA <localRepository> as its text and decodes
entities, ignores a profile property of the same name, and fixes
user.home before command-line -D flags in MAVEN_ARGS apply. The
lookup now does the same. CLI e2e children also drop MAVEN_OPTS,
MAVEN_ARGS, MAVEN_HOME and M2_HOME so a developer's Maven setup
can't redirect their crawl.

Assisted-by: Claude Code:claude-opus-5-5
A FIFO planted where settings.xml is expected is now refused instead
of blocking the crawl in open(2), as the crawler architecture test
requires for every crawler read.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 29, 2026 03:22
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

A comment inside <localRepository> in settings.xml was kept as part
of the path, so the crawler fell back to ~/.m2/repository while Maven
used the real local repository. The value is now read as Maven's
parser reads it: comments drop out, entities decode, CDATA is kept
verbatim.

The Maven crawler e2e tests also clear M2_HOME now, so a host Maven
install's settings.xml can't steer tests that pin the repo list.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review by the burn-down agent.


Generated by Claude Code

m2_repo_path_treats_empty_m2_home_as_unset now reads the user's
settings.xml, so a developer whose ~/.m2/settings.xml sets
<localRepository> got a red cargo test. Point HOME/USERPROFILE at an
empty temp dir for that test.

Also drop a format! with no placeholders in local_repo_tests, which
clippy --all-targets flags as useless_format.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run

Final reviewer: pushed 72f4c71 (test-only). It makes m2_repo_path_treats_empty_m2_home_as_unset hermetic: the test failed for anyone whose ~/.m2/settings.xml sets <localRepository>. It also drops a placeholder-free format! that clippy --all-targets flags. I removed Ready for review so the burn-down re-labels the PR once CI is green. I found no blocking issues and will post the review brief on the new head.


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review again by the burn-down agent, after the final reviewer's test-only push.

  • Head: 72f4c7115585605dbc59c33fab7a41f123912beb, mergeable, no conflicts
  • CI: 50/50 check runs passed or skipped
  • Bugbot: reviewed 72f4c71 and passed; both earlier threads are resolved
  • Reviewer focus: the Maven local-repo lookup order in maven_crawler.rs (settings.xml <localRepository>, MAVEN_OPTS, and USERPROFILE on Windows)

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: Ready for review again at 72f4c7115585605dbc59c33fab7a41f123912beb (first labeled at 448fbe0; the final reviewer then pushed test-only fixes).

  • CI: 50/50 check runs green (31 success, 19 skipped). Mergeable, no conflicts.
  • Bugbot: reviewed 72f4c71, no findings; no unresolved review threads.
  • Reviewer focus: Maven local repository resolution in maven_crawler.rs (empty M2_HOME treated as unset) and the matching test helpers.

Slack announcement not sent this run (the Slack send tool was not available to the agent); the next run retries.


Generated by Claude Code

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

Final review of 72f4c71 found one blocker and fixed it in a39bc39:

  • Windows test failure (fixed). maven_repo_local_system_property_beats_settings and maven_opts_user_home_moves_the_m2_directory resolved with Os::Unix. Unix-mode split_opts reads \ as an escape, so on Windows the tempdir path C:\Users\…\Temp\.tmpX in MAVEN_OPTS became C:Users…, failed has_root() and the asserts got the settings/HOME path. PR CI skips the Windows test leg, so this would only have shown up on the Windows nightly. These tests, and maven_args_user_home_does_not_move_the_m2_directory, now use Os::current(). local_repo_tests 11/11 pass on Linux.

Non-blocking notes, each a possible follow-up rather than a change for this PR:

  1. If a user settings.xml sets a <localRepository> that is relative or has an unresolved ${…}, the code falls through to the global settings. Maven would stop at the user value instead. Main read no settings at all, so this is not a regression.
  2. ${maven.home} inside <localRepository> isn't interpolated.
  3. The global settings file is found only through MAVEN_HOME/M2_HOME, so distro and Homebrew installs (/usr/share/maven/conf, /etc/maven) are missed. -s/-gs and .mvn/maven.config aren't read.
  4. On Unix, user.home comes from $HOME, not the passwd entry the JVM uses. It differs only when HOME is overridden.
  5. The description leaves out that -Dmaven.repo.local in MAVEN_ARGS is honored ahead of MAVEN_OPTS. The code and docs do it, matching Maven.

Generated by Claude Code

Three local_repo_tests put a tempdir path into MAVEN_OPTS/MAVEN_ARGS but
resolved with Os::Unix. Unix-mode split_opts treats backslashes as
escapes, so on Windows `C:\Users\...\Temp\.tmpX` became the
drive-relative `C:Users...`, failed has_root(), and the tests got the
settings or HOME path instead. Resolve with Os::current() so the
options are split the way the host would split them; with only HOME set
the Windows branch falls back to HOME, so expectations are unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit a39bc39. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: Ready for review again at a39bc3982c37054c932497137b50b53284e10e62 (after the final reviewer's Windows test fix).

  • CI: 50/50 check runs green (31 success, 19 skipped). Mergeable, no conflicts.
  • Bugbot: reviewed a39bc39, passed; both review threads resolved.
  • Reviewer focus: Maven local-repo precedence in maven_crawler.rs; the tests now parse MAVEN_OPTS with Os::current() so Windows paths survive. The final reviewer's 5 non-blocking follow-up notes above are worth a glance.

Slack announcement not sent this run (the Slack send tool is unavailable to the agent); the next run retries.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Agent mode and -g now find the Maven local repository the way Maven does: MAVEN_REPO_LOCAL, then -Dmaven.repo.local in MAVEN_ARGS/MAVEN_OPTS, then <localRepository> in user then global settings.xml, then user.home/.m2/repository. It stops treating $M2_HOME/repository (the Maven install dir) as the cache, and on Windows takes user.home from USERPROFILE. Before this, scans could hit an empty directory and rollback -g could drop a record while the real repo stayed patched (#423).

Risk: medium. It changes which directory global scan/patch/rollback touch for Maven users with a non-default setup. Default setups (~/.m2/repository) resolve as before. The new XML read is FIFO-safe and offset-preserving, and relative or unresolvable paths are skipped rather than resolved against the CWD.

Look here

Verified

  • Read the full diff (8 files). Scope is tight: the only edit outside Maven is making nuget::decode_entities pub(crate) for reuse. No debug code, and CHANGELOG.md is untouched.
  • Checked that xml::blank_non_markup blanks byte-for-byte, so slicing the raw text by element offsets is sound, including for non-ASCII paths.
  • Ran cargo test -p socket-patch-core --lib maven_crawler locally on a39bc39: 72 passed.
  • CI on a39bc39: 50/50 check runs green (31 success, 19 skipped), with ci-ok and clippy passing. Bugbot passed on a39bc39 and both of its threads are resolved.

Changes I made: a39bc39 in an earlier run, where the Windows tests now parse MAVEN_OPTS with the host OS rules. No changes this run.

Open questions (non-blocking, from the PR description):

  • .mvn/maven.config, -s/-gs and shell quoting in MAVEN_OPTS aren't modelled.
  • On Unix, user.home still prefers $HOME over the passwd entry. That predates this PR.

Auto-merge is armed, so an approval sends this straight to the merge queue.


Generated by Claude Code

Merged via the queue into main with commit ca6dc05 Oct 29, 2026
50 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-maven-local-repo-resolution branch October 29, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants