Adjust Install workflow of MicrobotPluginManager#1423
Adjust Install workflow of MicrobotPluginManager#1423gmason0 merged 1 commit intochsami:developmentfrom
Conversation
WalkthroughAdds a manifest-backed cache with scheduled refreshes in MicrobotPluginManager, enables single-plugin sideload install/load and sync, rewrites dependency-aware plugin loading, and changes the UI to read manifests from the manager, fetch plugin counts separately, and scale plugin icons. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Panel as MicrobotPluginHubPanel
participant Manager as MicrobotPluginManager
participant Client as MicrobotPluginClient
User->>Panel: Open / Trigger reload
Panel->>Manager: getManifestMap()
Panel->>Client: getPluginCounts()
alt success
Panel->>Panel: reloadPluginList(manifests, counts)
Panel-->>User: Render list
else failure
Panel-->>User: Show retry UI
end
note over Manager: Background scheduled manifest refresh
Manager->>Manager: loadManifest()
Manager-->>Panel: ExternalPluginsChanged
Panel->>Manager: getManifestMap()
Panel->>Client: getPluginCounts()
Panel->>Panel: reloadPluginList(...)
sequenceDiagram
autonumber
actor User
participant Panel
participant Manager
participant Repo as PluginRepo
participant FS as Filesystem
participant CL as MicrobotPluginClassLoader
participant Bus as EventBus
User->>Panel: Install plugin
Panel->>Manager: install(manifest)
Manager->>Repo: Download jar
Manager->>Manager: Verify SHA-256
Manager->>FS: Write jar + update installed list
Manager->>Manager: Update manifestMap
Manager->>Manager: loadSideLoadPlugin(internalName)
Manager->>FS: Validate jar + hash
Manager->>CL: Load classes
Manager->>Manager: loadPlugins(... dependency resolution ...)
Manager->>Bus: Post ExternalPluginsChanged
Bus-->>Panel: ExternalPluginsChanged
sequenceDiagram
autonumber
participant Manager
participant FS as Filesystem
participant Runtime as LoadedPlugins
participant Bus as EventBus
Manager->>Manager: syncPlugins()
Manager->>FS: Read installed list
Manager->>Runtime: Identify loaded but not installed
Manager->>Runtime: Unload extras
Manager->>FS: Enumerate sideload jars
loop jars
alt installed & not loaded
Manager->>Manager: loadSideLoadPlugin(name)
else skip
end
end
Manager->>Bus: Post ExternalPluginsChanged (if changed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Docstrings generation was requested by @g-mason0. * #1423 (comment) The following files were modified: * `runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java` * `runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java`
|
Note Generated docstrings for this pull request at #1424 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java (1)
574-610: Consider handling edge cases in the reloadPluginList workflowThe current implementation has potential issues:
- If
microbotPluginClient.getPluginCounts()throws an exception, the error UI is shown butreloadPluginList(manifestCollection, pluginCounts)is never called, leaving the plugin list empty.- The
refreshingflag is set to visible early but may not be properly reset if exceptions occur.Consider restructuring the error handling to ensure the plugin list is still loaded even if plugin counts fail:
executor.submit(() -> { Collection<MicrobotPluginManifest> manifestCollection = microbotPluginManager.getManifestMap().values(); Map<String, Integer> pluginCounts = Collections.emptyMap(); try { pluginCounts = microbotPluginClient.getPluginCounts(); } catch (IOException e) { log.warn("Unable to download plugin counts", e); - SwingUtilities.invokeLater(() -> - { - refreshing.setVisible(false); - mainPanel.add(new JLabel("Downloading the plugin manifest failed")); - - JButton retry = new JButton("Retry"); - retry.addActionListener(l -> reloadPluginList()); - mainPanel.add(retry); - mainPanel.revalidate(); - }); } reloadPluginList(manifestCollection, pluginCounts); });runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (3)
84-86: Consider thread-safety implications of the manifestMap getterThe
manifestMapfield is exposed via a public getter, allowing external code to modify the underlyingConcurrentHashMap. This could lead to unexpected behavior if external code modifies the map directly.Return an unmodifiable view to prevent external modifications:
@Getter -private final Map<String, MicrobotPluginManifest> manifestMap = new ConcurrentHashMap<>(); +private final Map<String, MicrobotPluginManifest> manifestMap = new ConcurrentHashMap<>(); + +public Map<String, MicrobotPluginManifest> getManifestMap() { + return Collections.unmodifiableMap(manifestMap); +}
87-99: Consider adding retry logic for manifest loadingThe
loadManifest()method silently catches all exceptions and only logs them. This could leave the manifestMap empty if the initial load fails, potentially causing issues for dependent components.Consider adding a retry mechanism or at least maintaining the previous manifest state on failure:
private void loadManifest() { try { List<MicrobotPluginManifest> manifests = microbotPluginClient.downloadManifest(); + Map<String, MicrobotPluginManifest> newManifestMap = new ConcurrentHashMap<>(); - manifestMap.clear(); for (MicrobotPluginManifest manifest : manifests) { - manifestMap.put(manifest.getInternalName(), manifest); + newManifestMap.put(manifest.getInternalName(), manifest); } + manifestMap.clear(); + manifestMap.putAll(newManifestMap); log.info("Loaded {} plugin manifests.", manifestMap.size()); eventBus.post(new ExternalPluginsChanged()); } catch (Exception e) { log.error("Failed to fetch plugin manifests", e); } }
535-633: Duplicate detection logic could be more robustThe current duplicate detection only checks if the exact class is already loaded (Line 541-551). However, it doesn't check for the same plugin loaded from different classloaders or with different versions.
Consider checking by plugin name/descriptor instead of just class reference:
-Set<Class<?>> alreadyLoaded = pluginManager.getPlugins().stream() - .map(Object::getClass) - .collect(Collectors.toSet()); +Set<String> alreadyLoadedNames = pluginManager.getPlugins().stream() + .filter(p -> p.getClass().isAnnotationPresent(PluginDescriptor.class)) + .map(p -> p.getClass().getAnnotation(PluginDescriptor.class).name()) + .collect(Collectors.toSet()); for (Class<?> clazz : plugins) { - if (alreadyLoaded.contains(clazz)) - { - log.debug("Plugin {} is already loaded, skipping duplicate.", clazz.getSimpleName()); - continue; - } PluginDescriptor pluginDescriptor = clazz.getAnnotation(PluginDescriptor.class); if (pluginDescriptor == null) { if (clazz.getSuperclass() == Plugin.class) { log.error("Class {} is a plugin, but has no plugin descriptor", clazz); } continue; } + + if (alreadyLoadedNames.contains(pluginDescriptor.name())) + { + log.debug("Plugin {} is already loaded, skipping duplicate.", pluginDescriptor.name()); + continue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java(7 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
runelite-client/src/main/java/net/runelite/client/RuneLiteProperties.java (1)
RuneLiteProperties(35-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java (1)
612-672: LGTM! Well-structured plugin list reload logicThe new overloaded
reloadPluginListmethod properly:
- Filters out disabled plugins
- Correctly indexes manifests and loaded plugins
- Handles plugin installation state detection
- Properly resets the refreshing state on the EDT
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
466-497: LGTM! Improved plugin loading workflowThe refactored
loadSideLoadPluginsmethod properly:
- Synchronizes plugins before loading
- Filters out non-JAR files and non-installed plugins
- Avoids reloading already loaded plugins
- Delegates to the single-plugin loader
...rc/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
Show resolved
Hide resolved
...rc/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
Show resolved
Hide resolved
...rc/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
Show resolved
Hide resolved
...ite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (2)
394-457: Good: single-plugin sideload path with hash validation and cleanup
- Validates installation, avoids duplicate load, verifies hash, deletes corrupted JAR, updates plugins.json, and posts ExternalPluginsChanged on success. This aligns with prior feedback.
171-238: Harden install() against empty response bodies and invalid hashes; optionally surface failures to UI
- response.body() can be null; bytes() would NPE. Guard and log.
- verifyHash throws IllegalArgumentException on null/empty inputs; either validate manifest.getSha256() up front or catch the exception.
- Optional: write to a temp file and perform an atomic move to avoid partial JARs on crashes.
- Optional: emit a user-visible event on failure (matches a prior review suggestion).
try (Response response = okHttpClient.newCall(request).execute()) { if (!response.isSuccessful()) { log.error("Error downloading plugin: {}, code: {}", manifest.getInternalName(), response.code()); return; } - byte[] jarData = response.body().bytes(); + if (response.body() == null) + { + log.error("Empty response body for plugin: {}", manifest.getInternalName()); + return; + } + byte[] jarData = response.body().bytes(); // Verify the SHA-256 hash - if (!verifyHash(jarData, manifest.getSha256())) + if (manifest.getSha256() == null || manifest.getSha256().isEmpty()) + { + log.error("Missing SHA-256 in manifest for: {}", manifest.getInternalName()); + return; + } + if (!verifyHash(jarData, manifest.getSha256())) { log.error("Plugin hash verification failed for: {}", manifest.getInternalName()); return; } manifestMap.put(manifest.getInternalName(), manifest); // Save the jar file File pluginFile = getPluginJarFile(manifest.getInternalName()); - Files.write(jarData, pluginFile); + File tmp = File.createTempFile(manifest.getInternalName(), ".jar.tmp", PLUGIN_DIR); + Files.write(jarData, tmp); + Files.move(tmp, pluginFile); @@ - catch (IOException e) + catch (IOException | IllegalArgumentException e) { log.error("Error installing plugin: {}", manifest.getInternalName(), e); }Optionally, post a failure event alongside logs so the UI can inform the user (event class omitted here to avoid API assumptions).
🧹 Nitpick comments (6)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (6)
84-86: Expose manifests read-only to callersRight now external code can mutate manifestMap via the Lombok-generated getter. Since you already refresh the map internally and other components should treat it as read-only state, return an unmodifiable view to prevent accidental writes from outside.
Apply this diff here (remove Lombok getter on the field):
- @Getter - private final Map<String, MicrobotPluginManifest> manifestMap = new ConcurrentHashMap<>(); + // Expose via a read-only accessor to prevent external mutation + private final Map<String, MicrobotPluginManifest> manifestMap = new ConcurrentHashMap<>();Then add this accessor elsewhere in the class:
public Map<String, MicrobotPluginManifest> getManifestMap() { return Collections.unmodifiableMap(manifestMap); }
87-99: Post ExternalPluginsChanged only when manifests actually changeloadManifest() currently posts ExternalPluginsChanged on every refresh, which can wake UI unnecessarily. Build a new map, compare (at least by keys + version/hash), and only swap + post on change. This also avoids any transient “empty” window during clear()/put().
- try { - List<MicrobotPluginManifest> manifests = microbotPluginClient.downloadManifest(); - manifestMap.clear(); - for (MicrobotPluginManifest manifest : manifests) { - manifestMap.put(manifest.getInternalName(), manifest); - } - log.info("Loaded {} plugin manifests.", manifestMap.size()); - eventBus.post(new ExternalPluginsChanged()); + try { + List<MicrobotPluginManifest> manifests = microbotPluginClient.downloadManifest(); + Map<String, MicrobotPluginManifest> next = new HashMap<>(manifests.size()); + for (MicrobotPluginManifest m : manifests) { + next.put(m.getInternalName(), m); + } + boolean changed = !next.keySet().equals(manifestMap.keySet()) + || next.entrySet().stream().anyMatch(e -> { + MicrobotPluginManifest cur = manifestMap.get(e.getKey()); + return cur == null || !Objects.equals(cur.getSha256(), e.getValue().getSha256()); + }); + if (changed) { + manifestMap.clear(); + manifestMap.putAll(next); + log.info("Loaded {} plugin manifests.", manifestMap.size()); + eventBus.post(new ExternalPluginsChanged()); + } else { + log.debug("Plugin manifests unchanged ({} entries), skipping event.", manifestMap.size()); + } } catch (Exception e) { log.error("Failed to fetch plugin manifests", e); }
147-147: Auto-heal corrupt plugins.json on parse errorsYou now catch JsonSyntaxException, but subsequent reads will continue to fail until the file is fixed. Consider auto-healing to an empty list when JSON is corrupt, and keep the existing log.
- catch (IOException | JsonSyntaxException e) - { - log.error("Error reading Microbot plugin list", e); - } + catch (IOException | JsonSyntaxException e) + { + log.error("Error reading Microbot plugin list", e); + // Auto-heal corrupt file to reduce repeated failures + try { + Files.asCharSink(PLUGIN_LIST, StandardCharsets.UTF_8).write("[]"); + } catch (IOException ioEx) { + log.warn("Failed to auto-heal plugins.json", ioEx); + } + }
424-434: Notify UI after quarantining a bad JARWhen a hash mismatch is detected you delete the file and update plugins.json, but the UI may not refresh immediately. Consider posting an event to inform the hub panel.
if (!verifyHash(fileBytes, manifest.getSha256())) { log.error("Hash mismatch for plugin {}. Skipping load.", internalName); pluginFile.delete(); List<String> plugins = getInstalledPlugins(); plugins.remove(internalName); saveInstalledPlugins(plugins); + eventBus.post(new ExternalPluginsChanged()); return; }
459-490: Reuse the same internalName mapping as syncPlugins()loadSideLoadPlugins() also infers loadedInternalNames via PluginDescriptor.name(). Use the same helper to avoid drift and accidental duplicates/removals.
- Set<String> loadedInternalNames = pluginManager.getPlugins().stream() + Set<String> loadedInternalNames = pluginManager.getPlugins().stream() .filter(p -> p.getClass().isAnnotationPresent(PluginDescriptor.class)) .filter(p -> p.getClass().getAnnotation(PluginDescriptor.class).isExternal()) - .map(p -> p.getClass().getAnnotation(PluginDescriptor.class).name()) + .map(this::getPluginInternalName) .collect(Collectors.toSet());
528-626: Only increment progress on successful instantiation; prefer parameterized loggingMinor polish:
- loaded++ increments even when instantiation fails, inflating progress.
- Switch to parameterized logging instead of string concatenation.
- int loaded = 0; + int loaded = 0; List<Plugin> newPlugins = new ArrayList<>(); for (Class<? extends Plugin> pluginClazz : sortedPlugins) { Plugin plugin; try { plugin = instantiate(pluginManager.getPlugins(), (Class<Plugin>) pluginClazz); - log.info("Microbot pluginManager loaded " + plugin.getClass().getSimpleName()); + log.info("Microbot pluginManager loaded {}", plugin.getClass().getSimpleName()); newPlugins.add(plugin); pluginManager.addPlugin(plugin); + loaded++; } catch (PluginInstantiationException ex) { log.error("Error instantiating plugin!", ex); } - loaded++; if (onPluginLoaded != null) { onPluginLoaded.accept(loaded, sortedPlugins.size()); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java(7 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:17:37.571Z
Learnt from: g-mason0
PR: chsami/Microbot#1423
File: runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java:366-389
Timestamp: 2025-08-23T16:17:37.571Z
Learning: The ScheduledExecutorService injected into MicrobotPluginManager (and other RuneLite classes) is configured as a single-threaded executor via Executors.newSingleThreadScheduledExecutor() at RuneLiteModule.java line 130, wrapped in ExecutorServiceExceptionLogger. This ensures all async tasks execute sequentially, preventing race conditions.
Applied to files:
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
🧬 Code graph analysis (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
runelite-client/src/main/java/net/runelite/client/RuneLiteProperties.java (1)
RuneLiteProperties(35-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (3)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (3)
34-34: LGTM: imports align with new usageAdding JsonSyntaxException and TimeUnit/@Getter imports matches later code paths. No issues.
Also applies to: 40-41
101-134: Initialization and scheduled refresh look sound
- Ensuring PLUGIN_DIR and PLUGIN_LIST exist is good.
- Immediate loadManifest() followed by a scheduled refresh every 10 minutes is appropriate.
- Given DI provides a single-threaded ScheduledExecutorService, manifest refreshes are serialized.
366-389: UseconfigName()(or a normalized internal name) for the plugin key to match the manifest’sinternalNameThe current
syncPlugins()collects external plugins byPluginDescriptor.name(), but the installed‐plugins list is based on the manifest’sinternalName. Sincename()is intended as a human‐readable display name (and may include spaces, mixed case, etc.) this can lead to false removals. Instead, centralize the “plugin key” logic into a helper that returnsconfigName()(when present) or falls back toname(), normalized to the same form used in the manifest.• Location:
MicrobotPluginManager.syncPlugins()
– Change the collector’s key extractor
• Add helper:getPluginKey(Plugin)Suggested diff:
--- a/runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java +++ b/runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java @@ public void syncPlugins() - Map<String, Plugin> loadedByInternalName = pluginManager.getPlugins().stream() - .filter(p -> p.getClass().isAnnotationPresent(PluginDescriptor.class)) - .filter(p -> p.getClass().getAnnotation(PluginDescriptor.class).isExternal()) - .collect(Collectors.toMap( - p -> p.getClass().getAnnotation(PluginDescriptor.class).name(), - Function.identity(), - (a, b) -> a - )); + Map<String, Plugin> loadedByInternalName = pluginManager.getPlugins().stream() + .filter(p -> p.getClass().isAnnotationPresent(PluginDescriptor.class)) + .filter(p -> p.getClass().getAnnotation(PluginDescriptor.class).isExternal()) + .collect(Collectors.toMap( + this::getPluginKey, + Function.identity(), + (a, b) -> a + ));Add this helper to the class:
private String getPluginKey(Plugin p) { PluginDescriptor d = p.getClass().getAnnotation(PluginDescriptor.class); if (d == null) { return p.getClass().getSimpleName().toLowerCase(Locale.ROOT); } // Use configName() if explicitly set; otherwise fall back to name(). String key = d.configName().isEmpty() ? d.name() : d.configName(); return key.toLowerCase(Locale.ROOT); }•
configNameis documented as the “internal name used in the config” and defaults to""if unset
• Normalizing to lowercase ensures exact matches againstmanifest.getInternalName()(which you may also normalize)
• This avoids any mismatch ifname()contains spaces or case differencesPlease verify that all external plugins’ descriptors set
configNameto the same value as their manifest’sinternalNameso that this lookup aligns correctly.
…instead of trying to re-load all sideloaded plugins
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 (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java (1)
639-646: Loaded-plugin lookup should use PluginDescriptor.name(), not class simple nameIndexing loaded plugins by
getSimpleName()can desync the UI from real installs/updates when class names diverge frominternalName. Use@PluginDescriptor.name()to align withMicrobotPluginManifest.internalName.Apply:
- Map<String, Collection<Plugin>> pluginsByName = loadedPlugins.stream() - .collect(Collectors.groupingBy( - p -> p.getClass().getSimpleName().toLowerCase(Locale.ROOT), - LinkedHashMap::new, - Collectors.toCollection(LinkedHashSet::new) // stable, no dups - )); + Map<String, Collection<Plugin>> pluginsByName = loadedPlugins.stream() + .collect(Collectors.groupingBy( + p -> { + PluginDescriptor d = p.getClass().getAnnotation(PluginDescriptor.class); + String key = (d != null && d.name() != null) ? d.name() : p.getClass().getSimpleName(); + return key.toLowerCase(Locale.ROOT); + }, + LinkedHashMap::new, + Collectors.toCollection(LinkedHashSet::new) // stable, no dups + ));runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
197-201: Path traversal risk when constructing plugin jar path
getPluginJarFileconcatenatesinternalName + ".jar"directly. A malicious or malformedinternalName(e.g.,"../../foo") can escapePLUGIN_DIR.Harden path building:
private File getPluginJarFile(String internalName) { - return new File(PLUGIN_DIR, internalName + ".jar"); + // sanitize the filename and enforce directory containment + String safe = internalName.replaceAll("[^A-Za-z0-9._-]", "_"); + File f = new File(PLUGIN_DIR, safe + ".jar"); + try + { + String base = PLUGIN_DIR.getCanonicalPath() + File.separator; + String target = f.getCanonicalPath(); + if (!target.startsWith(base)) + { + throw new IllegalArgumentException("Refusing to write outside plugin directory for internalName=" + internalName); + } + } + catch (IOException ioe) + { + throw new RuntimeException("Failed to resolve plugin jar path", ioe); + } + return f; }
♻️ Duplicate comments (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java (1)
723-726: Simplified ExternalPluginsChanged handler — LGTMHooking the event to the no-arg
reloadPluginList()removes duplication and keeps all error/logging paths centralized.runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
202-269: Installation flow lacks robust failure signaling and misses some exception types
Any
IllegalArgumentExceptionfromverifyHash(e.g., missing/empty expected hash) escapes theIOExceptioncatch and gets logged only by the executor wrapper, leaving the UI stuck in “Installing”.Early-return failure paths (disabled, version-incompatible, invalid URL, HTTP error, hash mismatch) don’t notify the UI; the panel will not reset unless a separate event occurs.
Catch
RuntimeException(or at leastIllegalArgumentException) alongsideIOException.Post an
ExternalPluginsChanged(or a dedicatedPluginInstallationFailed) on all failure exits so the UI can refresh.Apply:
public void install(MicrobotPluginManifest manifest) { executor.execute(() -> { // Check if plugin is disabled if (manifest.isDisable()) { log.error("Plugin {} is disabled and cannot be installed.", manifest.getInternalName()); + eventBus.post(new ExternalPluginsChanged()); return; } // Check version compatibility before installing if (!isClientVersionCompatible(manifest.getMinClientVersion())) { log.error("Plugin {} requires client version {} or higher, but current version is {}. Installation aborted.", manifest.getInternalName(), manifest.getMinClientVersion(), RuneLiteProperties.getMicrobotVersion()); + eventBus.post(new ExternalPluginsChanged()); return; } try { HttpUrl url = microbotPluginClient.getJarURL(manifest); if (url == null) { log.error("Invalid URL for plugin: {}", manifest.getInternalName()); + eventBus.post(new ExternalPluginsChanged()); return; } Request request = new Request.Builder() .url(url) .build(); try (Response response = okHttpClient.newCall(request).execute()) { if (!response.isSuccessful()) { log.error("Error downloading plugin: {}, code: {}", manifest.getInternalName(), response.code()); + eventBus.post(new ExternalPluginsChanged()); return; } byte[] jarData = response.body().bytes(); // Verify the SHA-256 hash if (!verifyHash(jarData, manifest.getSha256())) { log.error("Plugin hash verification failed for: {}", manifest.getInternalName()); + eventBus.post(new ExternalPluginsChanged()); return; } manifestMap.put(manifest.getInternalName(), manifest); // Save the jar file File pluginFile = getPluginJarFile(manifest.getInternalName()); Files.write(jarData, pluginFile); List<String> plugins = getInstalledPlugins(); if (!plugins.contains(manifest.getInternalName())) { plugins.add(manifest.getInternalName()); saveInstalledPlugins(plugins); } loadSideLoadPlugin(manifest.getInternalName()); } } - catch (IOException e) + catch (IOException | RuntimeException e) { log.error("Error installing plugin: {}", manifest.getInternalName(), e); + eventBus.post(new ExternalPluginsChanged()); } }); }If you’d prefer explicit user feedback, emit a dedicated event (e.g.,
PluginInstallationFailed(manifest, reason)), and have the UI react accordingly. I can draft that event and wire-up handling if desired.
🧹 Nitpick comments (5)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java (2)
589-606: Incorrect error message and double-render; bail early on counts failureThis catch handles plugin-count download, but the label says “manifest failed”. Also, the code proceeds to
reloadPluginList(manifestCollection, pluginCounts)after scheduling the error UI, which immediately replaces the error UI, making “Retry” pointless.Apply:
catch (IOException e) { log.warn("Unable to download plugin counts", e); SwingUtilities.invokeLater(() -> { refreshing.setVisible(false); - mainPanel.add(new JLabel("Downloading the plugin manifest failed")); + mainPanel.add(new JLabel("Unable to download plugin counts")); JButton retry = new JButton("Retry"); retry.addActionListener(l -> reloadPluginList()); mainPanel.add(retry); mainPanel.revalidate(); }); + return; // don't proceed to reload with empty counts; keep the retry UI visible }
612-672: Minor: avoid magic numbers for icon scaling and add null/blank iconUrl guardThe 48x70 constants in
PluginIcon.load()are duplicated knowledge ofPluginItemdimensions, andiconUrlcan be null/blank in some manifests.
- Consider hoisting ICON_WIDTH/HEIGHT to outer class constants (or reference
PluginItem.ICON_WIDTH/HEIGHTif made package-visible) to avoid drift.- Add a null/blank check before enqueueing/downloading.
Example (outside current diff):
// In PluginIcon private boolean hasIcon() { return iconUrl != null && !iconUrl.isBlank(); } @Override public void paint(Graphics g) { super.paint(g); if (!loadingStarted && hasIcon()) { loadingStarted = true; synchronized (iconLoadQueue) { iconLoadQueue.add(this); if (iconLoadQueue.size() == 1) { executor.submit(MicrobotPluginHubPanel.this::pumpIconQueue); } } } } private void load() { try { if (!hasIcon()) { return; } BufferedImage img = microbotPluginClient.downloadIcon(iconUrl); if (img == null) { return; } BufferedImage scaledImg = scaleImageToFit(img, ICON_WIDTH, HEIGHT); SwingUtilities.invokeLater(() -> setIcon(new ImageIcon(scaledImg))); } catch (IOException e) { log.info("Cannot download icon \"{}\"", iconUrl, e); } }runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (3)
332-341:verifyHashthrows on bad inputs; prefer soft-fail to avoid executor-level exceptionsThrowing
IllegalArgumentExceptionhere bubbles up unless every caller catches it. Given multiple call sites, returningfalseon invalid inputs keeps the error handling uniform and avoids noisy executor exceptions.Apply:
- if ((expectedHash == null || expectedHash.isEmpty()) || (jarData == null || jarData.length == 0)) - { - throw new IllegalArgumentException("Hash or jar data is null/empty"); - } + if ((expectedHash == null || expectedHash.isEmpty()) || (jarData == null || jarData.length == 0)) + { + log.error("Hash or jar data is null/empty"); + return false; + } - String computedHash = calculateSHA256Hash(jarData); - return computedHash.equals(expectedHash); + String computedHash = calculateSHA256Hash(jarData); + return computedHash.equalsIgnoreCase(expectedHash.trim());
100-115: Manifest-change detection ignores non-hash metadata updatesThe “changed” condition only considers keyset and SHA-256 changes. UI-visible fields (displayName, description, tags, iconUrl) won’t trigger refresh if hashes remain the same.
Consider including additional manifest fields in the equality check (or compare entire manifest sans transient fields) so metadata updates refresh the panel. Minimal approach:
- Compare
version,displayName,description, andiconUrlin addition tosha256.
251-260: Optional: write jar atomically to reduce partial-write riskIf the process dies between
Files.write()andsaveInstalledPlugins(), a partially written jar may be left behind.Write to a temp file then
Files.move(temp, pluginFile)withATOMIC_MOVEif supported, and only then updateplugins.json.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java(6 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/ui/MicrobotPluginHubPanel.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:17:37.571Z
Learnt from: g-mason0
PR: chsami/Microbot#1423
File: runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java:366-389
Timestamp: 2025-08-23T16:17:37.571Z
Learning: The ScheduledExecutorService injected into MicrobotPluginManager (and other RuneLite classes) is configured as a single-threaded executor via Executors.newSingleThreadScheduledExecutor() at RuneLiteModule.java line 130, wrapped in ExecutorServiceExceptionLogger. This ensures all async tasks execute sequentially, preventing race conditions.
Applied to files:
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
🧬 Code graph analysis (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java (1)
runelite-client/src/main/java/net/runelite/client/RuneLiteProperties.java (1)
RuneLiteProperties(35-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| catch (PluginInstantiationException | IOException e) | ||
| { | ||
| log.trace("Error loading side-loaded plugin!", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Broaden catch to cover hash/argument issues during sideload load
verifyHash can throw IllegalArgumentException (until refactored). Catch it here to keep the executor log clean and ensure we don’t silently stop processing other plugins.
Apply:
- catch (PluginInstantiationException | IOException e)
+ catch (PluginInstantiationException | IOException | IllegalArgumentException e)
{
log.trace("Error loading side-loaded plugin!", e);
}🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java
around lines 485 to 489, the current catch only handles
PluginInstantiationException and IOException but verifyHash may throw
IllegalArgumentException; broaden the catch clause to include
IllegalArgumentException (or add a separate catch for it) so that these errors
are logged with log.trace("Error loading side-loaded plugin!", e) and do not
abort processing of other plugins.
1 participant
This pull request aims to remedy an issue with the MicrobotPluginManager.
Previously this class would attempt to reload all plugins when installing a plugin from the plugin hub. I have changed this by adjusting the work flow of the hub panel to target an individual internalName instead of reloading all classes.
I have also improved how the manifest is stored & referenced in the plugin hub. This maintains a hash map of the internal plugin name -> manifest, this is refreshed based on an interval to ensure its up to date.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor