Skip to content

Sanitize tooling server JVM environment#1356

Open
ReigenArataka-jp wants to merge 2 commits into
appdevforall:stagefrom
ReigenArataka-jp:sanitize-tooling-env
Open

Sanitize tooling server JVM environment#1356
ReigenArataka-jp wants to merge 2 commits into
appdevforall:stagefrom
ReigenArataka-jp:sanitize-tooling-env

Conversation

@ReigenArataka-jp
Copy link
Copy Markdown

Fixes issue #1056

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 15214769-e17b-4882-a8a1-68cb58139366

📥 Commits

Reviewing files that changed from the base of the PR and between 4f206c6 and baf651f.

📒 Files selected for processing (1)
  • app/src/main/java/com/itsaky/androidide/services/builder/ToolingServerRunner.kt

📝 Walkthrough

Release Notes

  • Environment variable sanitization for tooling server JVM: Introduced filtering of Android/ART runtime classpath environment variables (BOOTCLASSPATH, DEX2OATBOOTCLASSPATH, SYSTEMSERVERCLASSPATH, STANDALONE_SYSTEMSERVER_JARS, CLASSPATH) that are no longer inherited by the standalone OpenJDK process used for the Tooling API. This prevents crashes on some OEM devices (e.g., Huawei/Honor) where stale or incompatible framework classpath entries can cause native ART registration to abort the JVM before the tooling server initializes.

  • ProcessBuilder environment isolation: The tooling server process environment is now explicitly cleared and populated only with sanitized variables, ensuring complete isolation from the parent app process environment rather than inheriting and filtering.

Risks & Best Practices

  • ⚠️ High-risk change: Modifies core process execution environment behavior. This could affect tooling server operation on edge-case Android devices or in specialized environments. Comprehensive testing on various OEM devices and Android versions is strongly recommended before release.

  • ⚠️ Hardcoded environment filtering: The list of excluded environment variables is hardcoded in a private constant. Future additions or changes to the exclusion list require code modifications. Consider implementing a configuration mechanism if dynamic filtering becomes necessary.

  • Environment isolation trade-off: Explicitly clearing and selectively repopulating the environment (rather than filtering from inherited parent variables) is the secure and stable approach, but could inadvertently exclude legitimate environment variables if requirements change.

Walkthrough

ToolingServerRunner is updated to sanitize the child process environment when starting the tooling API server. A new ANDROID_RUNTIME_ENV_KEYS set defines which Android/ART runtime variables to exclude. The process startup now filters the input environment and passes only sanitized variables to ProcessBuilder, replacing the prior approach that delegated to executeProcessAsync.

Changes

Process environment sanitization

Layer / File(s) Summary
Environment variable exclusion list
app/src/main/java/com/itsaky/androidide/services/builder/ToolingServerRunner.kt
A new private constant set ANDROID_RUNTIME_ENV_KEYS is introduced, containing the names of Android/ART runtime environment variables to exclude from the tooling server process.
Sanitized process startup with ProcessBuilder
app/src/main/java/com/itsaky/androidide/services/builder/ToolingServerRunner.kt
ToolingServerRunner.startAsync filters the input environment to remove Android/ART keys, then passes only the sanitized environment to ProcessBuilder. Import changes update coroutine cancellation handling for error conditions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A server wakes up, scrubbed clean and bright,
No Android ghosts in the runtime light,
With ProcessBuilder, we're tidy and true,
Environment filtered—a fresh start anew! 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Sanitize tooling server JVM environment' accurately reflects the main change: sanitizing the environment variables passed to the tooling server process by removing Android/ART runtime keys.
Description check ✅ Passed The description references issue #1056 which is related to the changeset, indicating this PR addresses a specific bug or enhancement.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@hal-eisen-adfa
Copy link
Copy Markdown
Collaborator

@ReigenArataka-jp Thanks for this! I'll take a look this week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants