ADFA-3588: Enforce plugin-api binary compatibility with BCV#1474
ADFA-3588: Enforce plugin-api binary compatibility with BCV#1474Daniel-ADFA wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
📝 Walkthrough
WalkthroughThis PR adds Kotlin binary-compatibility validation to the ChangesPlugin API Binary Compatibility Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugin-api/build.gradle.kts (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider ignoring the generated
Rclass as well.Only
BuildConfigis excluded from the ABI dump. Android library modules also generate a publicRclass (com.itsaky.androidide.plugins.api.R), which will likely show up inplugin-api.apiand cause spuriousapiCheckfailures whenever resources change, unrelated to actual API changes.♻️ Proposed addition
apiValidation { ignoredClasses.add("com.itsaky.androidide.plugins.api.BuildConfig") + ignoredClasses.add("com.itsaky.androidide.plugins.api.R") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin-api/build.gradle.kts` around lines 31 - 34, The apiValidation block currently ignores only BuildConfig, so the generated public R class can still pollute the ABI dump and trigger false apiCheck failures. Update the apiValidation configuration in build.gradle.kts to also ignore the generated R class for the plugin-api module, using the same ignoredClasses list alongside com.itsaky.androidide.plugins.api.BuildConfig..github/workflows/debug.yml (1)
133-136: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider combining apiCheck into the main Gradle invocation.
Running
:plugin-api:apiCheckas a separateflox activate/Gradle invocation before:app:assembleV8Debugincurs a second full Gradle configuration pass. Combining both tasks into a single invocation (./gradlew :plugin-api:apiCheck :app:assembleV8Debug --no-daemon) would preserve fail-fast semantics for the API check while avoiding duplicate startup/configuration overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/debug.yml around lines 133 - 136, The workflow currently runs :plugin-api:apiCheck in a separate flox activate/Gradle invocation before :app:assembleV8Debug, causing an extra full Gradle configuration pass. Update the debug workflow step that invokes ./gradlew so :plugin-api:apiCheck and :app:assembleV8Debug run in the same Gradle command, preserving fail-fast behavior while removing the duplicate startup overhead. Use the existing workflow job and the Gradle invocation around :plugin-api:apiCheck and :app:assembleV8Debug as the location to adjust.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin-api/api/plugin-api.api`:
- Around line 111-125: `PluginFragmentHelper` is exposing host-owned mutators in
the public plugin ABI, so move the lifecycle/state-changing APIs out of the
plugin-facing surface. Keep `getCurrentActivityContext`,
`getCurrentActivityTheme`, `getPluginContext`, `getPluginInflater`,
`getServiceRegistry`, and the error callback accessors public, but hide or
relocate `registerPluginContext`, `registerServiceRegistry`,
`unregisterPluginContext`, and `clearAllContexts` to a host-internal API so
plugins cannot modify shared state.
---
Nitpick comments:
In @.github/workflows/debug.yml:
- Around line 133-136: The workflow currently runs :plugin-api:apiCheck in a
separate flox activate/Gradle invocation before :app:assembleV8Debug, causing an
extra full Gradle configuration pass. Update the debug workflow step that
invokes ./gradlew so :plugin-api:apiCheck and :app:assembleV8Debug run in the
same Gradle command, preserving fail-fast behavior while removing the duplicate
startup overhead. Use the existing workflow job and the Gradle invocation around
:plugin-api:apiCheck and :app:assembleV8Debug as the location to adjust.
In `@plugin-api/build.gradle.kts`:
- Around line 31-34: The apiValidation block currently ignores only BuildConfig,
so the generated public R class can still pollute the ABI dump and trigger false
apiCheck failures. Update the apiValidation configuration in build.gradle.kts to
also ignore the generated R class for the plugin-api module, using the same
ignoredClasses list alongside com.itsaky.androidide.plugins.api.BuildConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa38a1fa-f60c-4b71-a78c-bbb40328b386
📒 Files selected for processing (4)
.github/workflows/debug.ymlgradle/libs.versions.tomlplugin-api/api/plugin-api.apiplugin-api/build.gradle.kts
| public final class com/itsaky/androidide/plugins/base/PluginFragmentHelper { | ||
| public static final field INSTANCE Lcom/itsaky/androidide/plugins/base/PluginFragmentHelper; | ||
| public static final fun clearAllContexts ()V | ||
| public static final fun getCurrentActivityContext (Ljava/lang/String;)Landroid/content/Context; | ||
| public static final fun getCurrentActivityTheme (Ljava/lang/String;)Landroid/content/res/Resources$Theme; | ||
| public static final fun getOnPluginInflationError ()Lkotlin/jvm/functions/Function2; | ||
| public static final fun getPluginContext (Ljava/lang/String;)Landroid/content/Context; | ||
| public static final fun getPluginInflater (Ljava/lang/String;Landroid/view/LayoutInflater;)Landroid/view/LayoutInflater; | ||
| public static final fun getServiceRegistry (Ljava/lang/String;)Lcom/itsaky/androidide/plugins/ServiceRegistry; | ||
| public static final fun registerPluginContext (Ljava/lang/String;Landroid/content/Context;)V | ||
| public static final fun registerPluginContext (Ljava/lang/String;Landroid/content/Context;Z)V | ||
| public static synthetic fun registerPluginContext$default (Ljava/lang/String;Landroid/content/Context;ZILjava/lang/Object;)V | ||
| public static final fun registerServiceRegistry (Ljava/lang/String;Lcom/itsaky/androidide/plugins/ServiceRegistry;)V | ||
| public static final fun setOnPluginInflationError (Lkotlin/jvm/functions/Function2;)V | ||
| public static final fun unregisterPluginContext (Ljava/lang/String;)V |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify source intent and callers before changing the ABI surface.
rg -n -C4 'clearAllContexts|registerPluginContext|registerServiceRegistry|unregisterPluginContext|onPluginInflationError' plugin-api/src/main/kotlinRepository: appdevforall/CodeOnTheGo
Length of output: 6550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementation and surrounding API surface.
sed -n '1,160p' plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt
printf '\n--- API dump excerpt ---\n'
sed -n '100,140p' plugin-api/api/plugin-api.api
printf '\n--- References to the helper from plugin-api ---\n'
rg -n -C2 'PluginFragmentHelper\.' plugin-api/src/main/kotlin plugin-api/apiRepository: appdevforall/CodeOnTheGo
Length of output: 7888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find who calls the helper mutators and how the package is described elsewhere.
rg -n -C3 'PluginFragmentHelper\.(clearAllContexts|registerPluginContext|registerServiceRegistry|unregisterPluginContext|setOnPluginInflationError|getPluginInflater|getPluginContext|getServiceRegistry)' .
printf '\n--- plugin-api module files ---\n'
git ls-files 'plugin-api/src/main/kotlin/**/*.kt' 'plugin-api/src/main/kotlin/*.kt'Repository: appdevforall/CodeOnTheGo
Length of output: 14918
Keep PluginFragmentHelper mutators out of the plugin-facing API.
registerPluginContext, registerServiceRegistry, unregisterPluginContext, and clearAllContexts are host-owned lifecycle hooks, but they’re exposed as public ABI here. Any plugin can call them and wipe or replace shared context/registry state for other plugins. Split the manager-only mutators into a host-internal surface and keep only the read-only/plugin-side helpers public.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin-api/api/plugin-api.api` around lines 111 - 125, `PluginFragmentHelper`
is exposing host-owned mutators in the public plugin ABI, so move the
lifecycle/state-changing APIs out of the plugin-facing surface. Keep
`getCurrentActivityContext`, `getCurrentActivityTheme`, `getPluginContext`,
`getPluginInflater`, `getServiceRegistry`, and the error callback accessors
public, but hide or relocate `registerPluginContext`, `registerServiceRegistry`,
`unregisterPluginContext`, and `clearAllContexts` to a host-internal API so
plugins cannot modify shared state.
1 participant
No description provided.