test(integ-test): gate optional integration environments

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6

[mengweieric]

Summary

Plain external-cluster integration runs do not always include the Security, geospatial, telemetry, or remote-cluster environments required by their dedicated test suites. Those suites currently fail during setup instead of reporting that the dependency is unavailable.

This change detects optional plugins through _cat/plugins and skips only when the required component is absent. Cross-cluster tests similarly skip when no remote cluster is configured. Dedicated environments continue to run the tests unchanged.

No production behavior is modified.

Validation

• On a plain five-shard cluster, all optional-environment suites skipped cleanly and the task passed
• Plugin-present paths remain enabled by runtime detection
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0179dc6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The isPluginInstalled method opens an InputStream from response.getEntity().getContent() but does not close it if readAllBytes() throws an exception. While the try-with-resources closes the stream on normal completion, an exception during readAllBytes() (e.g., network interruption, malformed encoding) leaves the stream open, leaking the HTTP connection resource. try (InputStreamcontent = response.getEntity().getContent()) { body = newString(content.readAllBytes(), StandardCharsets.UTF_8); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 0179dc6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle cross-platform line endings The current implementation may fail to match plugin names on Windows systems where line endings are \r\n instead of \n. After splitting by \n, lines may still contain trailing \r characters, causing the exact equality check to fail even when the plugin is installed. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [122-126] -for (String line : body.split("\n")) {+for (String line : body.split("\\r?\\n")) { if (line.trim().equals(pluginComponentName)) { return true; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential cross-platform compatibility issue where Windows line endings (\r\n) could cause the plugin detection to fail. The improved_code properly handles both Unix and Windows line endings by using the regex pattern \\r?\\n.	Medium
General	Add exception handling in static setup The @BeforeClass method requireRemoteCluster() is static but calls requireOrAssume which may throw exceptions. If a subclass overrides instance initialization methods, the static check runs before any instance setup, which could lead to confusing error messages or test framework issues. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [34-48] -ClusterPlugins.requireOrAssume(- HAS_REMOTE_CLUSTER,- Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster); skipping",- "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"- + " 'remote*' cluster) but none was found, and this task marks it required via -D"- + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+@BeforeClass+public static void requireRemoteCluster() {+ try {+ ClusterPlugins.requireOrAssume(+ HAS_REMOTE_CLUSTER,+ Boolean.getBoolean(ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY),+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster); skipping",+ "Cross-cluster search requires a configured remote cluster (-Dcluster.names must include a"+ + " 'remote*' cluster) but none was found, and this task marks it required via -D"+ + ClusterPlugins.REQUIRE_REMOTE_CLUSTER_PROPERTY);+ } catch (Exception e) {+ throw new RuntimeException("Failed to verify remote cluster configuration", e);+ }+} Suggestion importance[1-10]: 2 __ Why: The suggestion is incorrect. The requireOrAssume method is designed to throw AssumptionViolatedException or AssertionError by design, which are the standard JUnit mechanisms for skipping tests or failing them. Wrapping these in a RuntimeException would break the intended test framework behavior and make the error messages less clear.	Low

---

Previous suggestions

Suggestions up to commit 4192322

Category	Suggestion	Impact
General	Trim whitespace from cluster names The code doesn't trim whitespace from cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Add trim() to each cluster name token. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [25-28] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ String trimmed = cluster.trim();+ if (trimmed.startsWith("remote")) {+ remote = trimmed; break; } } } Suggestion importance[1-10]: 7 __ Why: This is a valid bug fix. The code splits on commas but doesn't trim whitespace from individual tokens, which could cause startsWith("remote") to fail if the property value contains spaces like "cluster1, remote2". The suggested fix correctly trims each token and updates the remote variable assignment accordingly.	Medium
Security	Add response size limit check The readAllBytes() method can throw OutOfMemoryError if the response is unexpectedly large. Consider adding a size limit check or using a bounded read to prevent potential memory exhaustion from malicious or malformed responses. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [82-84] try (InputStream content = response.getEntity().getContent()) { - body = new String(content.readAllBytes(), StandardCharsets.UTF_8);+ byte[] bytes = content.readNBytes(1024 * 1024); // 1MB limit+ if (content.read() != -1) {+ throw new IOException("Response exceeds maximum allowed size");+ }+ body = new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential memory exhaustion issue when reading unbounded response content. While _cat/plugins responses are typically small, adding a size limit is a reasonable defensive measure. However, the suggested implementation has issues: it uses readNBytes() which may read less than requested, and the follow-up read() check is unreliable. A better approach would use Content-Length header validation or a proper bounded stream wrapper.	Low

Suggestions up to commit 5b565cf

Category	Suggestion	Impact
Possible issue	Use exact line matching for plugin detection The contains() check may produce false positives if pluginComponentName is a substring of another plugin name. Use line-by-line matching to ensure exact component name matches. integ-test/src/test/java/org/opensearch/sql/util/ClusterPlugins.java [45-47] Response response = client.performRequest(new Request("GET", "/_cat/plugins?h=component")); String body = new String(response.getEntity().getContent().readAllBytes()); -return body.contains(pluginComponentName);+return body.lines().anyMatch(line -> line.trim().equals(pluginComponentName)); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential false positive issue where contains() could match partial plugin names. Using exact line matching with lines().anyMatch() ensures accurate plugin detection and prevents bugs.	Medium
General	Trim cluster names after splitting The code doesn't trim cluster names after splitting, which could cause startsWith("remote") to fail if there are spaces around commas in the property value. Apply trim() to each cluster name. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterTestBase.java [22-30] String clusterNamesProp = System.getProperty("cluster.names"); String remote = null; if (clusterNamesProp != null && !clusterNamesProp.isBlank()) { for (var cluster : clusterNamesProp.split(",")) { - if (cluster.startsWith("remote")) {- remote = cluster;+ if (cluster.trim().startsWith("remote")) {+ remote = cluster.trim(); break; } } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion that addresses a potential issue where whitespace in the cluster.names property could cause the startsWith("remote") check to fail. Trimming ensures robust parsing of cluster names.	Medium

[github-actions]

Persistent review updated to latest commit 4192322

[github-actions]

Persistent review updated to latest commit 0179dc6