[Mikola Lysenko (mikolalysenko)]

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

Fixes #535
Fixes #550

Summary

When it vendors a Maven reactor, socket-patch reads user properties from
.mvn/maven.config so that an override there beats a pom <properties>
value, as it does in Maven. The reader misread the file in two ways,
and each one can silently downgrade the build to the patched base, after
which vendor --check and vex both confirm the downgrade:

• Vendored Maven reactor reads -D properties from commented-out .mvn/maven.config lines, so a 1.11.0 build is silently downgraded to 1.10.0-socket.* (or a valid patch is refused) #550: it used properties from commented-out # lines, which Maven
  3.9+ ignores.
• Vendored Maven reactor ignores user properties set in .mvn/maven.config as --define=k=v or -D k=v, so a 1.11.0 build is silently downgraded to 1.10.0-socket.* with no warning #535: it missed --define=k=v, -D k=v, and -D with k=v on
  the next line, all of which Maven applies.

Root cause

cli_properties (crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs)
split the whole file on whitespace and kept only tokens shaped -Dk=v.
It never dropped comment lines and knew no other spelling.

Fix

cli_properties now reads the file the way Maven 3.9+ does:

• one argument per line, with blank and # lines skipped;
• -Dk=v, -D k=v, --define=k=v and --define k=v all define a
  property, and so does a bare -D / --define followed by a k=v
  line (a bare -D never takes the next option as its value);
• the value is the rest of the argument, so -Dct.version=1.10.0 # TODO
  is not 1.10.0;
• a fully quoted argument is unquoted, -Dk sets k=true, a later
  definition wins, and trimming matches Java's String.trim.

Every caller (vendor, vendor --check, vex) goes through the same
planner, so all of them pick this up. Older Maven (before 3.9) rejects
# lines itself, and recognising an override that it would not apply
only makes the planner refuse to pin (conflicting_literal_version), so
the planner never claims a patch the build does not link.

Test evidence

Issue	Test	Without fix	With fix
#535	maven_reactor::tests::maven_config_long_and_split_define_spellings_win (--define=, -D k=v, -D/--define on their own line, CRLF, quoted, trailing text)	FAIL (no warning, ${ct.version} rewritten to the patched base)	ok
#550	maven_reactor::tests::maven_config_comment_lines_define_nothing (#, # … old pin, indented #, # just a note)	FAIL (the comment's 1.11.0 used)	ok
both	maven_reactor::tests::maven_config_user_properties_parse_like_maven (parser table)	FAIL	ok

Commands run locally (Linux, rustc 1.93.1):

• cargo fmt --all -- --check: ok
• cargo clippy --workspace --all-features -- -D warnings: ok
• cargo test --workspace --all-features --no-fail-fast: 15077 passed,
  12 failed. The 12 failures come from this container, not this diff, and
  are the same set Fix Go version gate ignoring go.work members (#1751) #1752 recorded: 9 write-failure tests that rely on
  chmod blocking writes (the container runs as root), and 3 in
  e2e_bun_lockb (local bun 1.4.4, CI pins 1.4.2). Every Maven/JVM test
  passes.
• I added no real-Maven e2e. The bug is entirely in how the planner parses
  the file, and the unit tests drive the same plan that vendor,
  vendor --check and vex run.
• No wrapper changes (npm/, pypi/, gem/ only dispatch to the binary).

/code-review high findings not fixed

• merge_maven_config (the writer) matches only the -Dk= spelling of
  its own two properties. If a user sets maven.repo.local.tail or
  aether.offline.protocols with --define=, vendor appends its own
  line rather than extending theirs, and Maven then uses the later one.
  That problem was there before this PR, is in a different function, and
  fixing it needs new undo records, so it is left for a follow-up.
• sbt::reactor_wired looks for the tail directory in the raw config
  text, so a commented-out tail line counts as wiring. It was there
  before this PR and is a separate shape-detection check, so it is left
  for a follow-up.

https://claude.ai/code/session_011f9aeT7DJDAhBe2sDUWU8A

---

Note

Medium Risk
Changes how user properties from maven.config feed version/conflict resolution across vendor, check, and vex; wrong parsing could still mis-pin or over-refuse builds, but behavior is now aligned with Maven and heavily unit-tested.

Overview
Replaces naive whitespace tokenization of .mvn/maven.config with parsing that matches Maven 3.9+: one argument per line, # comments ignored, and support for --define, spaced -D k=v, and -D / --define with k=v on the next line. Values stay on the rest of the line (including trailing # text), quoted args are unquoted, -Dk means k=true, and later definitions win; a legacy pass still picks up pre-3.9-only -Dk=v tokens when the line parser did not set that key.

That fixes the reactor planner treating commented pins as overrides (#550) and missing valid define spellings (#535), which could silently vendor to the patch base instead of refusing when a real override conflicts. New unit tests cover vendor behavior and a full cli_properties fixture table.

^{Reviewed by Cursor Bugbot for commit 0548a33. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: labeled Ready for review at head d3f7410.

• CI: 82/82 checks on d3f7410 passed or skipped (32 success, 50 skipped by path filters). Mergeable with main; no main-wide failures.
• Bugbot: reviewed d3f7410, no issues found. No open review threads.
• Reviewer focus: maven_reactor.rs now tokenises .mvn/maven.config properly — skips # comment lines and handles --define= and split -D key=value forms.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final reviewer: pushed 0548a33. Finding: the per-line reader dropped -Dk=v tokens on multi-argument lines (-B -Dct.version=1.11.0, -Dfoo=1 -Dct.version=1.11.0). Maven before 3.9 splits maven.config on whitespace and applies those, and the nightly tier still runs 3.6.3/3.8.9, so a 3.8 build could be rewritten to the patched base under a newer override. The fix adds any such token that the per-line reading didn't define, which keeps the planner fail-closed (at worst it refuses to pin). Two cases were added to maven_config_long_and_split_define_spellings_win, plus a line in the parser table. maven_reactor tests pass: 70/70.

Removed Ready for review until CI is green on the new head.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

bugbot run

---

Generated by Claude Code

[cursor]

✅ 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 0548a33. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: labeled Ready for review at head 0548a33.

• CI: 50/50 checks on 0548a33 passed or skipped (31 success, 19 skipped). No main-wide failures.
• Bugbot: reviewed 0548a33, no new issues.
• Reviewer focus: cli_properties in crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs now parses .mvn/maven.config line-by-line like Maven 3.9+ (skips # lines, accepts --define, -D k=v, and split -D/k=v lines).
• Slack announcement could not be sent from this run (the Slack send tool is not available); the next run will retry.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does. When socket-patch vendors a Maven reactor, it reads user properties from .mvn/maven.config so an override there beats a pom <properties> value. Before this PR, the reader used properties from commented-out # lines (#550). It also missed --define=k=v, -D k=v and a bare -D followed by k=v on the next line (#535). Either mistake could make vendor, vendor --check and vex treat the patched base as the version the build uses, which silently downgrades it. cli_properties now reads the file one argument per line, as Maven 3.9+ does. It still also picks up -Dk=v tokens the pre-3.9 whitespace split would find.

Risk: low. The change is one pure function plus tests. The extra pre-3.9 reading can only add overrides, and an extra override makes the planner refuse to pin (conflicting_literal_version). So a parsing mistake fails toward refusing, not toward claiming a patch the build doesn't link.

Look here

• cli_properties:`` the per-line parser, then the legacy whitespace pass that only adds keys no line defined.
• the cli_props lookup:``` the only consumer, where an override beats the pom value.
• the spelling test,```` the comment-line test and the parser table.``````

Verified

• Read the full diff (1 file, +150/−7). CHANGELOG.md is untouched.
• Checked these edge cases: --defined=x is not a define, and a bare -D never takes the next - option as its value. CRLF works via lines(). Java-style trim. -Dv=1.0 # note keeps the whole rest of the line as the value, as Maven 3.9 does.
• cargo test -p socket-patch-core --lib maven_reactor::tests::maven_config: 7/7 passed. CI is 50/50 on this head (31 passed, 19 skipped by path filters). Bugbot: success, no issues. No open review threads.

Changes I made: none in this pass. The earlier final-reviewer pass pushed 0548a33, which keeps -Dk=v tokens from multi-argument lines.

Open questions: none.

Auto-merge (squash) is armed, so approving sends this PR straight to the merge queue.

---

Generated by Claude Code