In eclipse-jdtls/eclipse.jdt.ls#3901, the fix was a single line of Python:
if os.name == 'posix':
- os.execvp(java_executable, exec_args)
+ os.execvp(java_executable, [java_executable] + exec_args)
else:
subprocess.run([java_executable] + exec_args)One line added, one line removed. But the exchange in the pull request comments holds a lesson on code review, system contracts, and how to evaluate code when there is no test suite to protect you.
the bug
When users ran the Eclipse JDT Language Server on Linux using a runtime version manager like mise or asdf, the server failed to boot.
These version managers use shims. Instead of pointing directly to the real Java binary, your $PATH points to a shim executable. When you run java, the shim inspects its own argv[0] to determine what program was invoked, checks your local configuration, and delegates to the appropriate JDK installation.
In POSIX, execvp(file, argv) requires that argv[0] contains the filename or path of the program being executed. Python's os.execvp(file, args) follows this directly: the first element of args becomes argv[0].
In jdtls.py, exec_args contained only JVM flags:
exec_args = ['-Djdk.xml.maxGeneralEntitySizeLimit=0', ...]Because java_executable was omitted from exec_args, the first JVM argument (-Djdk.xml...) became argv[0].
A direct Java binary often ignores argv[0] and reads flags starting at argv[1], which is why the bug went unnoticed for standard installations. But a shim cannot do that. It read -Djdk.xml... as the binary name, failed to recognize it, and crashed.
symptom checking vs contract thinking
The review from Rob Stryker, one of the project maintainers, was what made the PR interesting. Rather than only checking whether the patch resolved the symptom for mise users, his review evaluated the underlying contract:
"Every exec'd program — java's own launcher, real java binaries, wrapper scripts, ps//proc inspection, crash/error reporting that logs argv[0] — expects argv[0] to be the program name/path. There's no case where omitting it is correct.
Seems this is a genuine bug with a genuine fix. Nice find!"
Notice the shift in framing. The question is not: "Does mise need this workaround?"
The question is: "Is omitting argv[0] ever valid in POSIX?"
The answer is no: POSIX specifies argv[0] as the filename associated with the process invocation, and callers such as wrappers and other tooling can rely on it being present. Looking at the non-POSIX branch immediately below the diff confirms it: subprocess.run([java_executable] + exec_args) was already passing java_executable first.
The original POSIX branch was violating an invariant. The fix was not an ad-hoc adjustment for one tool, but a restoration of standard POSIX semantics.
"how can this blow up in my face?"
The conversation continued with an explanation of how to review changes in areas with minimal automated testing:
"Not to get too detailed, but, I tend to first look at a PR and see if it looks obvious. Then i'll dig in a bit to the code myself and see if it fixes the supposed issue. However, I'm always thinking in the back of my mind, this is a piece of code with no real test suite that I can find. So how can this blow up in my face? Sure, it fixes the thing the user complained about, but, what if it breaks 10 more things and we don't know? So you dig into the API calls, in this case os.execvp, and see what the documentation says.
But at the end, when it all looks mostly ok, you have a few options. A smoke test can usually work at least a little to calm you down and make sure nothing broke. But... at the end of the day, for a bit more confidence, asking an AI helps a lot. They're not always right, but, sometimes they point out things you didn't consider, or they point you to documentation that proves 100% you understood it correctly already."
When a repository lacks automated tests for a specific integration, engineers often fall into two traps:
- Blind trust: It looks simple, so merge it immediately.
- Paralyzed avoidance: There are no tests, so leave the PR sitting untouched.
The maintainer's review shows a structured path between those extremes.
building the review mindset
The review left me with four habits I want to carry into future code reviews:
1. start with the blast radius
Before asking whether a change works, ask: What else relies on this code path? If you change how arguments are passed to a process, every invocation downstream is affected. Identifying the blast radius tells you how rigorously you need to scrutinize the API contract.
2. check the contract, not just the behavior
Code often "works" by accident when consumers are lenient. Standard Java binaries tolerated a missing argv[0], hiding the bug for years. Shims did not. When reviewing, don't stop at "does it work on my machine." Check the specification: what does POSIX say? What does Python's os.execvp documentation require?
3. find consistency in adjacent code
Frequently, the correct pattern is already in the file. Right below the broken os.execvp line was:
subprocess.run([java_executable] + exec_args)The Windows/subprocess branch had the correct arguments. The POSIX branch did not. Comparing adjacent implementations of the same conceptual operation will often reveal whether a difference is intentional or an oversight.
4. verify when you cannot reproduce
You will not always have the exact toolchain, OS, or version manager installed locally to reproduce a bug. When reproduction is impractical:
- Read the official API documentation for the functions involved.
- Trace arguments manually from definition to call site.
- Run a quick smoke test on the code path with a standard environment.
- Use an external sanity check (asking another contributor or querying an LLM to review edge cases and cite documentation).
A one-line diff can carry as much architectural weight as a multi-file refactor. Learning to review from invariants and API contracts rather than superficial symptoms is what makes code review reliable.