fix(android): auto-detect the Android SDK when fresh worktrees lack local.properties#101273
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d46241327e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!existsSync(defaultSdkDir)) { | ||
| return env; | ||
| } | ||
| return { ...env, ANDROID_HOME: defaultSdkDir }; |
There was a problem hiding this comment.
When this fallback is used for pnpm android:run/android:run:third-party with no SDK env vars and no adb on the shell PATH, Gradle can now install successfully but the post command is still spawned as bare adb from package.json:1498-1499. Adding only ANDROID_HOME here does not make adb discoverable by spawn, so the run lane still exits with ENOENT in the fresh-worktree/default-SDK case this change is trying to cover; add <defaultSdkDir>/platform-tools to PATH or resolve the post command from the SDK.
Useful? React with 👍 / 👎.
d462413 to
4fae5be
Compare
|
Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 10:42 PM ET / 02:42 UTC. Summary PR surface: Tests +53, Other +27. Total +80 across 2 files. Reproducibility: yes. By source inspection, Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the Gradle-side SDK fallback, but when the wrapper discovers a default SDK, also expose or resolve Do we have a high-confidence way to reproduce the issue? Yes. By source inspection, Is this the best way to solve the issue? No. The wrapper is the right layer for the SDK fallback, but the best fix must also make the discovered SDK's Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 632efa2d7343. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Tests +53, Other +27. Total +80 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
4fae5be to
d26d771
Compare
What Problem This Solves
Fixes an issue where developers running any SDK-dependent Android lane (
pnpm android:test,android:assemble,android:install,android:run) from a fresh git worktree would hitSDK location not foundeven though an Android SDK is installed.apps/android/local.propertiesis gitignored, so new worktrees never carry it, and nothing else names the SDK unlessANDROID_HOME/ANDROID_SDK_ROOThappens to be exported. The ktlint lanes (android:format/android:lint) mask the gap because they never touch the SDK.Why This Change Was Made
scripts/run-android-gradle.mjsnow resolves the SDK before spawning Gradle: explicitANDROID_HOME/ANDROID_SDK_ROOTand an existinglocal.propertieskeep full precedence, and only when neither names an SDK does the wrapper fall back to the Android Studio default install path (~/Library/Android/sdkon macOS,~/Android/Sdkelsewhere) — and only if that directory actually exists. No files are written; the fallback is passed asANDROID_HOMEto the spawned process only. CI is unaffected since it exportsANDROID_HOMEexplicitly.User Impact
Android Gradle lanes work out of the box in fresh worktrees and clones on machines with a default-located SDK, with no manual
local.propertiessetup. Existing explicit configuration behaves exactly as before.Evidence
Repro and fix on a fresh worktree (no
local.properties, no SDK env vars):pnpm android:test→SDK location not found. Define a valid SDK location with an ANDROID_HOME environment variable or by setting the sdk.dir path in your project's local properties file...(BUILD FAILED in 616ms)env -u ANDROID_HOME -u ANDROID_SDK_ROOT pnpm android:test→BUILD SUCCESSFUL(:app:testPlayDebugUnitTest)Focused tests:
test/scripts/run-android-gradle.test.ts— 8 passed, including 4 newresolveAndroidSdkEnvcases covering env-var precedence,local.propertiesprecedence, per-platform default fallback, and missing-SDK passthrough.