Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the plugin management system to enforce explicit activation for plugins, moving away from implicit discovery for certain scenarios. It introduces checks against a list of configured plugins during both main class loader discovery and JAR-based loading, ensuring that only intended plugins are registered. Additionally, it improves robustness by preventing and logging warnings for duplicate plugin registrations.

Highlights

• Explicit Plugin Activation: Plugins discovered on the main class loader are now only registered if their name or full class name is present in the configuredPlugins list, ensuring explicit activation.
• Duplicate Plugin Handling: The system now explicitly checks for and logs a warning when attempting to load a plugin that is already registered, preventing duplicate entries.
• JAR Plugin Loading Refinement: Plugin loading from JAR files has been updated to first create a PluginDescriptor and then check if the plugin is already loaded or configured before registration.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

🧪 CI Insights

Here's what we observed from your CI run for d8c11a7.

🟢 All jobs passed!

But CI Insights is watching 👀

Code Review

Thanks for the fix to enforce explicit plugin activation. The intent is clear, but I've spotted several issues that should be addressed before merging.

1. Indentation Bug in discoverPluginsOnMainClassLoader (High)

The body of the new if block was never re-indented. While Java doesn't care about whitespace, the result is visually misleading — the code looks like lines 88–92 are at the for-loop level, not inside the if body:

for (ServerPlugin pluginInstance : serviceLoader) {
  String name = pluginInstance.getClass().getSimpleName();
  if (configuredPlugins.contains(name) || configuredPlugins.contains(pluginInstance.getClass().getName())) {
    // Register the plugin
  final PluginDescriptor descriptor = ...  // ← wrong indent
  descriptor.setPluginInstance(pluginInstance);
  plugins.put(name, descriptor);
  LogManager.instance().log(...);
}   // looks like it closes the for, but closes the if
}   // closes the for
}   // closes the method

The descriptor creation and registration lines must be indented two extra spaces to sit inside the if block.

2. final Keyword Removed — Violates Project Style (Medium)

CLAUDE.md explicitly requires: "use the final keyword when possible on variables and parameters."

The PR removes final from:

• String name in discoverPluginsOnMainClassLoader (reassigned? No — should remain final)
• The for (ServerPlugin pluginInstance : …) iterator variable in loadPlugin (should be for (final ServerPlugin pluginInstance : …))
• String name in loadPlugin (never reassigned — should be final)

3. Likely Test Failures — Existing Tests Not Updated (High)

The new behavior filters main-classpath plugins against configuredPlugins. However, several existing tests assert that AutoBackupSchedulerPlugin is always discovered from the main classpath, but that plugin is not present in the test's SERVER_PLUGINS configuration:

// PluginManagerTest.java – setup() only adds TestPlugin1, TestPlugin2, etc.
// AutoBackupSchedulerPlugin is NOT configured

// These tests will now fail:
assertThat(pluginManager.getPluginNames()).contains("AutoBackupSchedulerPlugin");  // line 102
assertThat(pluginManager.getPluginCount()).isGreaterThanOrEqualTo(1);  // lines 101, 133
assertThat(pluginManager.getPluginCount()).isEqualTo(2);  // lines 147, 180, 212, 259 — were expecting +1 for AutoBackup

Either add AutoBackupSchedulerPlugin to the test configuration, or update the count assertions to remove the +1 offset that assumed it was always auto-discovered. A dedicated test for "unconfigured plugin on main classpath is NOT registered" should also be added to cover the new behaviour.

4. PluginDescriptor Created Before Duplicate Check (Low)

In loadPlugin, the descriptor is created unconditionally, even when the plugin turns out to be a duplicate:

// Created here...
final PluginDescriptor descriptor = new PluginDescriptor(pluginInstance.getName(), classLoader);
descriptor.setPluginInstance(pluginInstance);

String name = pluginInstance.getName();
// ...only to be discarded here
if (plugins.containsKey(name)) {
  break;
}

Move the descriptor creation to after the duplicate check. Also, pluginInstance.getName() is called twice — assign it to name first and reuse it.

5. Behavioural Change: continue → break on Duplicate (Low)

In the original code, finding a duplicate triggered continue, allowing the loop to inspect subsequent plugins from the same JAR. The new code uses break, which abandons the whole JAR. For single-plugin JARs (the common case) this is fine, but the change in semantics is worth calling out explicitly in the commit message or a comment.

6. Inconsistent Key Used for Plugin Identity (Medium)

discoverPluginsOnMainClassLoader stores plugins under pluginInstance.getClass().getSimpleName(), while loadPlugin uses pluginInstance.getName() (the value returned by the plugin's own getName() method). If getName() ≠ getSimpleName(), the duplicate detection between the two discovery paths will silently fail and the same plugin can be loaded twice.

Both paths should use the same key — preferably pluginInstance.getName() (as the plugin author explicitly controls it), or getSimpleName() consistently if preferred.

Summary

The checklist items in the PR description (build + unit tests) are both unchecked — please confirm the build passes and the tests are green with these changes.

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

Codecov Report

❌ Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.00%. Comparing base (24c5260) to head (d8c11a7).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3578      +/-   ##
==========================================
- Coverage   65.70%   65.00%   -0.71%     
==========================================
  Files        1514     1514              
  Lines      103646   103653       +7     
  Branches    21449    21452       +3     
==========================================
- Hits        68101    67377     -724     
- Misses      26302    27102     +800     
+ Partials     9243     9174      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Thanks for the fix to enforce explicit plugin activation. The intent is clear, but I have spotted several issues that should be addressed before merging.

1. Dead Code in ArcadeDBServer.init() (Low)

Line 740 reads the configuration value but discards the result. It is a pure no-op and should be removed:

configuration.setValue(GlobalConfiguration.SERVER_PLUGINS, configuredPlugins);
configuration.getValueAsString(GlobalConfiguration.SERVER_PLUGINS);  // return value discarded
pluginManager = new PluginManager(this, configuration);

2. Duplicate Detection Does Not Actually Skip Registration (High)

In loadPlugin, when a duplicate is detected the code logs a warning but then falls through. The plugin is still registered, silently overwriting the existing entry:

if (plugins.containsKey(name)) {
    LogManager.instance().log(this, Level.WARNING, "Plugin with name already loaded, skipping duplicate...", ...);
    // break was commented out, no continue added
}

// execution falls through here regardless:
if (configuredPlugins.contains(name) || ...) {
    plugins.put(name, descriptor);  // overwrites the already-registered plugin!
    ...
}

The warning says "skipping" but the code does not skip anything. A continue statement is needed after the warning to prevent the fallthrough, or the original break should be reinstated if the intent is to bail out of the entire JAR loop.

3. Trailing Comma When SERVER_PLUGINS Is Empty (Low)

configuredPlugins = "AutoBackupSchedulerPlugin," + configuredPlugins;

If SERVER_PLUGINS is empty (the default), this produces "AutoBackupSchedulerPlugin," with a trailing comma. getConfiguredPlugins() splits on , and adds an empty string to the configuredPlugins set. While harmless today, it is a code smell. A guard is warranted:

configuredPlugins = configuredPlugins.isEmpty()
    ? "AutoBackupSchedulerPlugin"
    : "AutoBackupSchedulerPlugin," + configuredPlugins;

4. Tests Rely on Implicit Side-Effect of ArcadeDBServer Constructor (Medium)

The PluginManagerTest setup does:

server = new ArcadeDBServer(configuration);        // calls init(), which mutates `configuration`
pluginManager = new PluginManager(server, configuration);  // uses the now-mutated configuration

ArcadeDBServer(ContextConfiguration) calls init(), which prepends "AutoBackupSchedulerPlugin," to SERVER_PLUGINS in the shared configuration object. The test's separately-constructed PluginManager then picks that up, which is why AutoBackupSchedulerPlugin appears in assertions such as assertThat(pluginManager.getPluginCount()).isGreaterThanOrEqualTo(1).

This is fragile:

• The test silently depends on a constructor side-effect that mutates shared state.
• Any test that constructs PluginManager without first constructing ArcadeDBServer will behave differently.
• If init() is refactored, these tests can silently break.

The explicit plugin list in the test setup should simply include AutoBackupSchedulerPlugin, making the dependency explicit rather than accidental.

5. Commented-Out Code Left in Production (Low)

Two break statements were commented out in loadPlugin without a replacement or explanation of why they were removed:

//          break; // Exit loop - classloader will be closed in finally block
...
//        break; // Only load the first plugin from each JAR

These should either be reinstated (with the decision documented) or removed entirely.

Summary

Issue #2 is a correctness bug: the duplicate-detection path logs "skipping" but does not skip.

Code Review

The overall intent of this PR is good: enforcing explicit plugin activation rather than auto-discovering everything on the classpath is a safer approach. However, I found several issues worth addressing.

1. Dead code in ArcadeDBServer.init() (Bug)

Line 740 reads the configuration value but immediately discards it. This is dead code and should be removed:

configuration.setValue(GlobalConfiguration.SERVER_PLUGINS, configuredPlugins);
configuration.getValueAsString(GlobalConfiguration.SERVER_PLUGINS);  // result discarded, does nothing

2. Hardcoded string literal instead of class reference (Maintainability)

configuredPlugins = "AutoBackupSchedulerPlugin," + configuredPlugins;

Using a raw string literal for the class simple name is fragile — it silently breaks if AutoBackupSchedulerPlugin is ever renamed. Prefer:

configuredPlugins = AutoBackupSchedulerPlugin.class.getSimpleName() + "," + configuredPlugins;

3. init() silently mutates the shared configuration object (Design concern)

ArcadeDBServer.init() modifies the passed-in configuration object as a side effect:

configuration.setValue(GlobalConfiguration.SERVER_PLUGINS, configuredPlugins);

Callers do not expect the constructor to mutate their configuration. It also means if init() were called more than once (e.g. in a restart scenario), AutoBackupSchedulerPlugin would be prepended again, leading to "AutoBackupSchedulerPlugin,AutoBackupSchedulerPlugin,...". The tests work around this implicitly (they rely on new ArcadeDBServer(configuration) having already mutated the config before new PluginManager(server, configuration) reads it), which is non-obvious.

A cleaner alternative: always include AutoBackupSchedulerPlugin unconditionally in discoverPluginsOnMainClassLoader(), or introduce a separate concept of "built-in" vs "user-configured" plugins.

4. Stale comment vs removed break (Inconsistency)

In loadPlugin(), the comment still says "Load the first plugin implementation (typically only one per JAR)" but the break that enforced this was removed. The new behaviour loads all plugins in a JAR. Update the comment to match the intent.

5. PluginDescriptor created before authorization check (Minor)

In loadPlugin(), the PluginDescriptor is allocated before knowing whether the plugin will be registered, and pluginInstance.getName() is called twice. Assign to a local variable first and move descriptor creation inside the authorization block to avoid unnecessary allocations.

6. JAR filename used as authorization bypass vector (Security, pre-existing but worth noting)

if (configuredPlugins.contains(name) ||
    configuredPlugins.contains(pluginName) ||   // pluginName = JAR filename without extension
    configuredPlugins.contains(pluginInstance.getClass().getName())) {

pluginName is derived from the JAR filename. A JAR named AutoBackupSchedulerPlugin.jar containing a different plugin implementation would pass this check even if that plugin's own name/class is not configured. This means placing a file in the plugins directory is sufficient to bypass explicit activation for any configured plugin name. Consider removing the configuredPlugins.contains(pluginName) check, or at least documenting it as an intentional design choice.

What's good

• The explicit activation model is a solid improvement over implicit auto-discovery.
• The duplicate-plugin warning log in loadPlugin() is a useful addition.
• Replacing Assertions.assertThat(...) with the already-statically-imported assertThat(...) in the test is a nice consistency fix.
• Tests cover key scenarios well, including priority ordering, lifecycle, and class loader isolation.