ADFA-3588: Introduce plugin API binary compatibility tracking and version contract #1265
Open
Daniel-ADFA wants to merge 5 commits into
Open
ADFA-3588: Introduce plugin API binary compatibility tracking and version contract #1265Daniel-ADFA wants to merge 5 commits into
Daniel-ADFA wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughRelease NotesNew Features
Changed / Deprecated
Breaking Changes
Risks & Best Practices
Testing
WalkthroughAdds a public plugin API surface and PluginApiVersion type, reads min_plugin_api_version from manifests, enforces semantic compatibility via PluginApiVersionChecker (with specific exception handling), updates loader logging, and wires binary-compatibility validation into the build. ChangesPlugin API & Compatibility
Sequence Diagram(s)sequenceDiagram
participant PluginManager
participant PluginLoader
participant PluginApiVersionChecker
participant IPlugin
PluginManager->>PluginLoader: getPluginMetadata()
PluginLoader-->>PluginManager: PluginManifest (with minPluginApiVersion)
PluginManager->>PluginApiVersionChecker: requireCompatible(pluginId, requiredVersion, currentVersion)
alt Compatible
PluginApiVersionChecker-->>PluginManager: Validation passes
PluginManager->>IPlugin: initialize/activate
IPlugin-->>PluginManager: Plugin loads successfully
else Incompatible
PluginApiVersionChecker-->>PluginManager: throws PluginApiIncompatibleException
PluginManager->>PluginManager: Release sidebar slots & fail load
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt (1)
243-264:⚠️ Potential issue | 🟠 MajorDon't silently promote missing
min_plugin_api_versionto1.0.0.This makes pre-contract plugins look compatible with the new API gate. Because this PR also changes the public plugin API surface, older plugins that haven't been rebuilt can still pass load-time validation and then fail later with linkage/runtime errors. Safer rollout options are: require the field explicitly, or map “missing” to a distinct legacy bucket that the loader blocks until those plugins are republished.
🛠️ Minimal first step
- val pluginMinPluginApiVersion = metaData.getString("plugin.min_plugin_api_version") ?: "1.0.0" + val pluginMinPluginApiVersion = metaData.getString("plugin.min_plugin_api_version")Then handle
nullexplicitly at the load gate instead of treating it as a valid1.0.0declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt` around lines 243 - 264, The code currently coerces a missing metaData.getString("plugin.min_plugin_api_version") into "1.0.0"; instead, change pluginMinPluginApiVersion to preserve null (e.g., val pluginMinPluginApiVersion = metaData.getString("plugin.min_plugin_api_version")) and update the PluginManifest data class to accept a nullable minPluginApiVersion (or a distinct legacy marker) so the loader can detect "missing" explicitly; then enforce the load-time gate (in PluginLoader.load / validation code) to reject or route null/legacy entries rather than treating them as compatible with "1.0.0".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionChecker.kt`:
- Around line 42-48: The parse(raw: String) function can throw
NumberFormatException when converting oversized numeric segments with toInt();
wrap the Version(...) construction inside a try-catch that catches
NumberFormatException (and any other NumberFormat-related exceptions you
consider relevant) and return null on failure so malformed/overflowing inputs
map to null as expected by requireCompatible(); update/add a regression test to
feed an oversized segment like "999999999999.0.0" and assert parse returns null
(or that requireCompatible yields MALFORMED_VERSION).
---
Outside diff comments:
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.kt`:
- Around line 243-264: The code currently coerces a missing
metaData.getString("plugin.min_plugin_api_version") into "1.0.0"; instead,
change pluginMinPluginApiVersion to preserve null (e.g., val
pluginMinPluginApiVersion = metaData.getString("plugin.min_plugin_api_version"))
and update the PluginManifest data class to accept a nullable
minPluginApiVersion (or a distinct legacy marker) so the loader can detect
"missing" explicitly; then enforce the load-time gate (in PluginLoader.load /
validation code) to reject or route null/legacy entries rather than treating
them as compatible with "1.0.0".
🪄 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: ce3d0688-fe90-4873-93cd-f6c7e5f45251
📒 Files selected for processing (15)
app/src/main/java/com/itsaky/androidide/activities/PluginManagerActivity.ktbuild.gradle.ktsgradle/libs.versions.tomlplugin-api/api/plugin-api.apiplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/InternalPluginApi.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/PluginApiVersion.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiIncompatibleException.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionChecker.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginSecurityManager.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionCheckerTest.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/pluginManifest.kt
Comment on lines
+42
to
+48
| private fun parse(raw: String): Version? { | ||
| val match = SEMVER.matchEntire(raw.trim()) ?: return null | ||
| return Version( | ||
| major = match.groupValues[1].toInt(), | ||
| minor = match.groupValues[2].ifBlank { "0" }.toInt(), | ||
| patch = match.groupValues[3].ifBlank { "0" }.toInt(), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionChecker.ktRepository: appdevforall/CodeOnTheGo
Length of output: 2453
🏁 Script executed:
find . -type f -name "*PluginApiVersionChecker*Test*" -o -name "*Test*PluginApiVersionChecker*"Repository: appdevforall/CodeOnTheGo
Length of output: 179
🏁 Script executed:
rg -l "PluginApiVersionChecker" --type kotlinRepository: appdevforall/CodeOnTheGo
Length of output: 373
🏁 Script executed:
cat -n ./plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionCheckerTest.ktRepository: appdevforall/CodeOnTheGo
Length of output: 5711
Catch NumberFormatException in parse() to properly handle oversized numeric segments.
Input like 999999999999.0.0 matches the SEMVER regex but causes toInt() to throw NumberFormatException on overflow, escaping as an uncaught exception instead of returning null. This breaks the intended contract where requireCompatible() expects null to signal MALFORMED_VERSION. Wrap the Version construction in a try-catch block to return null on parse failure, and add a regression test for numeric overflow.
🔧 Targeted fix
private fun parse(raw: String): Version? {
val match = SEMVER.matchEntire(raw.trim()) ?: return null
- return Version(
- major = match.groupValues[1].toInt(),
- minor = match.groupValues[2].ifBlank { "0" }.toInt(),
- patch = match.groupValues[3].ifBlank { "0" }.toInt(),
- )
+ return try {
+ Version(
+ major = match.groupValues[1].toInt(),
+ minor = match.groupValues[2].ifBlank { "0" }.toInt(),
+ patch = match.groupValues[3].ifBlank { "0" }.toInt(),
+ )
+ } catch (_: NumberFormatException) {
+ null
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionChecker.kt`
around lines 42 - 48, The parse(raw: String) function can throw
NumberFormatException when converting oversized numeric segments with toInt();
wrap the Version(...) construction inside a try-catch that catches
NumberFormatException (and any other NumberFormat-related exceptions you
consider relevant) and return null on failure so malformed/overflowing inputs
map to null as expected by requireCompatible(); update/add a regression test to
feed an oversized segment like "999999999999.0.0" and assert parse returns null
(or that requireCompatible yields MALFORMED_VERSION).
itsaky-adfa
requested changes
Apr 29, 2026
jatezzz
requested changes
May 6, 2026
| private val SEMVER = Regex("^(\\d+)(?:\\.(\\d+))?(?:\\.(\\d+))?$") | ||
|
|
||
| fun isCompatible(required: String, current: String): Boolean { | ||
| val r = parse(required) ?: return false |
Collaborator
There was a problem hiding this comment.
Could you improve the variable names?
| } | ||
|
|
||
| fun requireCompatible(pluginId: String, required: String, current: String) { | ||
| val r = parse(required) ?: throw PluginApiIncompatibleException( |
| availableVersion = current, | ||
| reason = PluginApiIncompatibleException.Reason.MALFORMED_VERSION, | ||
| ) | ||
| val c = parse(current) ?: error("Invalid current plugin API version: '$current'") |
itsaky-adfa
requested changes
May 19, 2026
Comment on lines
+1
to
+11
| package com.itsaky.androidide.plugins | ||
|
|
||
| /** | ||
| * Plugin API contract version (SemVer). Bump major for binary-incompatible changes | ||
| * flagged by `:plugin-api:apiCheck`, minor for additive changes; then run | ||
| * `:plugin-api:apiDump` to refresh `plugin-api/api/plugin-api.api`. | ||
| */ | ||
| object PluginApiVersion { | ||
|
|
||
| const val CURRENT: String = "1.0.0" | ||
| } |
Contributor
There was a problem hiding this comment.
Plugin versions are structured, so using raw Strings is probably not the correct approach, at least at compile time. I suggest using a proper value type instead, which gives us type-safety, correct ordering and keeps the major/minor semantics :
@JvmInline
value class PluginApiVersion private constructor(val raw: String) : Comparable<PluginApiVersion> {
val major: Int get() = parts()[0]
val minor: Int get() = parts()[1]
val patch: Int get() = parts()[2]
private fun parts() = raw.split('.').map { it.toInt() }
override fun compareTo(other: PluginApiVersion): Int =
compareValuesBy(this, other, { it.major }, { it.minor }, { it.patch })
companion object {
/** the current plugin API version */
val CURRENT = parse("1.0.0")
fun parseOrThrow(s: String) = requireNotNull(parse(s)) { "s is invalid" }
fun parse(s: String): PluginApiVersion? {
/* validate major.minor.patch, or return null */
}
}
}A @JvmInline value class ensures that the versions are still strings at runtime, but gives us type-safety at compile-time. After this, we'd use PluginAPiVersion everywhere instead of raw Strings.
Comment on lines
+401
to
+402
| required = manifest.minPluginApiVersion ?: "1.0.0", | ||
| current = PluginApiVersion.CURRENT, |
Contributor
There was a problem hiding this comment.
Replace with PluginApiVersion type.
| val pluginMinIdeVersion = metaData.getString("plugin.min_ide_version") | ||
| val pluginMaxIdeVersion = metaData.getString("plugin.max_ide_version") | ||
| if (pluginMinPluginApiVersion == null && pluginMinIdeVersion != null) { | ||
| Log.w( |
Contributor
There was a problem hiding this comment.
Use SF4J Logger instances instead of android.util.Log.
Comment on lines
+44
to
+46
| message = "IDE version strings are timestamps and cannot be compared. Plugin API " + | ||
| "compatibility is enforced through minPluginApiVersion. Kept so manifests from " + | ||
| "existing plugins keep parsing; will be removed in the next major plugin API release." |
Contributor
There was a problem hiding this comment.
This string is repeated at least 3 times. Can we please store it in a const val property somewhere?
Comment on lines
+114
to
+115
| minIdeVersion = minIdeVersion ?: "1.0.0", | ||
| minPluginApiVersion = minPluginApiVersion ?: PluginApiVersion.CURRENT, |
Contributor
There was a problem hiding this comment.
Replace with PluginApiVersion type.
Contributor
There was a problem hiding this comment.
Replace raw strings with PluginApiVersion type.
The comparison can be moved into PluginApiVersion as well, or not (either is fine).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/PluginApiVersion.kt (1)
41-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
parse()accepts overflowed numeric segments that later crash on comparison.At Line 41-44, regex-only validation allows values like
999999999999.0.0; thenparts()(Line 29-31) throwsNumberFormatExceptionwhenmajor/minor/patchare accessed. That breaks the nullable-parse contract and can escape the intendedMALFORMED_VERSIONpath.🔧 Targeted fix
fun parse(raw: String): PluginApiVersion? { val trimmed = raw.trim() - return if (SEMVER.matches(trimmed)) PluginApiVersion(trimmed) else null + if (!SEMVER.matches(trimmed)) return null + return try { + PluginApiVersion(trimmed).also { it.parts() } // force numeric-range validation + } catch (_: NumberFormatException) { + null + } }Kotlin String.toInt() behavior: does it throw NumberFormatException for values outside Int range (e.g., "999999999999")?🤖 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/src/main/kotlin/com/itsaky/androidide/plugins/PluginApiVersion.kt` around lines 41 - 44, The parse() method currently relies only on SEMVER regex and then constructs PluginApiVersion(trimmed), which later calls parts() and can throw NumberFormatException for overflowed numeric segments (e.g., "999999999999"); change parse() to safely validate numeric segments before constructing PluginApiVersion: after trimming and SEMVER.matches(trimmed), split the string into segments (reuse pieces from parts() logic), convert each segment using safe parsing (e.g., String.toIntOrNull or try-catch around toInt) and reject the version (return null) if any segment is null or out of Int range, only then call PluginApiVersion(trimmed); reference parse(), SEMVER, PluginApiVersion, and parts() in your change.
🤖 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-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionTest.kt`:
- Around line 44-51: Add a regression test case for numeric overflow by calling
PluginApiVersion.parse("999999999999.0.0") and assert it returns null (and
optionally assert PluginApiVersion.parseOrThrow("999999999999.0.0") throws);
place this new assertion alongside the existing malformed inputs in the `parse
rejects malformed input by returning null` test to cover the conversion path
used in PluginApiVersion.parse (and protect lines performing numeric
conversion).
---
Duplicate comments:
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/PluginApiVersion.kt`:
- Around line 41-44: The parse() method currently relies only on SEMVER regex
and then constructs PluginApiVersion(trimmed), which later calls parts() and can
throw NumberFormatException for overflowed numeric segments (e.g.,
"999999999999"); change parse() to safely validate numeric segments before
constructing PluginApiVersion: after trimming and SEMVER.matches(trimmed), split
the string into segments (reuse pieces from parts() logic), convert each segment
using safe parsing (e.g., String.toIntOrNull or try-catch around toInt) and
reject the version (return null) if any segment is null or out of Int range,
only then call PluginApiVersion(trimmed); reference parse(), SEMVER,
PluginApiVersion, and parts() in your change.
🪄 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: a77bac80-7b6b-4ba9-8710-4b7270ce016a
📒 Files selected for processing (11)
plugin-api/api/plugin-api.apiplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/PluginApiVersion.ktplugin-manager/build.gradle.ktsplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginManifest.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionChecker.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionCheckerTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionTest.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/pluginManifest.kt
Comment on lines
+44
to
+51
| fun `parse rejects malformed input by returning null`() { | ||
| assertNull(PluginApiVersion.parse("v1.0.0")) | ||
| assertNull(PluginApiVersion.parse("1.0.0-snapshot")) | ||
| assertNull(PluginApiVersion.parse("1.0.0.0")) | ||
| assertNull(PluginApiVersion.parse("")) | ||
| assertNull(PluginApiVersion.parse("abc")) | ||
| assertNull(PluginApiVersion.parse("1.x.0")) | ||
| } |
Contributor
There was a problem hiding this comment.
Add an overflow regression case to malformed-version tests.
Please add a case like PluginApiVersion.parse("999999999999.0.0") and assert null (and optionally parseOrThrow throws). This protects the Line 29-31 conversion path.
🤖 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-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/security/PluginApiVersionTest.kt`
around lines 44 - 51, Add a regression test case for numeric overflow by calling
PluginApiVersion.parse("999999999999.0.0") and assert it returns null (and
optionally assert PluginApiVersion.parseOrThrow("999999999999.0.0") throws);
place this new assertion alongside the existing malformed inputs in the `parse
rejects malformed input by returning null` test to cover the conversion path
used in PluginApiVersion.parse (and protect lines performing numeric
conversion).
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
3 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.
Replaces the meaningless
min_ide_versionplugin compatibility check with a real plugin API versioning scheme backed bybinary-compatibility-validator. The previous field could never enforce anything:IDE version strings are timestamps (
C-r-MMDD-HHMM), not comparable to the SemVer values plugins were declaring, and the field was only checked for non-blankness anyway.