feat: Add opt-in settings to enforce unique Agent and Agentic Memory Container names

[rithinpullela]

Description

Adds two opt-in, dynamic cluster settings, both default false for backward compatibility:

• plugins.ml_commons.agent_name_uniqueness_enabled
• plugins.ml_commons.agentic_memory_name_uniqueness_enabled

When enabled, registering an Agent or creating an Agentic Memory Container with a name that already exists in the same tenant is rejected with HTTP 409 CONFLICT. Default off preserves existing behavior.

Semantics: best-effort, not atomic. This matches the existing pattern in MLModelGroupManager.validateUniqueModelGroupName (term query on name.keyword + tenantId, then unconditional index write) and inherits the same race window 1:1 — two concurrent registrations with the same name can both observe zero hits and both succeed. Closing the race would require a sidecar lock index + OpType.CREATE on a deterministic _id; that hardening should be applied uniformly across model groups, agents, and memory containers and is intentionally left for a follow-up.

What this PR adds beyond the model-group precedent:

• Opt-in via dynamic cluster setting (model group enforces unconditionally).
• HTTP 409 instead of 400 IllegalArgumentException.
• Tenant validation runs before the uniqueness search, so in multi-tenant mode a missing tenant fails fast with 403 rather than leaking cross-tenant existence via a 409.
• For PLAN_EXECUTE_AND_REFLECT agents without a pre-existing executor_agent_id, the auto-created "<name> (ReAct)" executor agent's name is also validated so the derived agent can't collide with an existing one.
• IndexNotFoundException from the search is treated as "no duplicate possible" so first-ever registrations on a fresh cluster work.

Manual verification

Verified end-to-end on a single-node cluster (./gradlew :opensearch-ml-plugin:run):

#	Scenario	Result
1	Both flags surface via _cluster/settings?include_defaults=true as false	✅
2	Agent: flag OFF → register same name twice	✅ both 200 (BWC preserved)
3	Agent: flag ON → register duplicate name	✅ 409 with clear error
4	Agent: flag ON → register unique name	✅ 200
5	Agent: flip flag OFF → duplicate allowed (dynamic)	✅ 200
6	Memory container: flag OFF → create twice	✅ both 200
7	Memory container: flag ON → duplicate	✅ 409
8	Memory container: flag ON → unique	✅ 200
9	Memory container: flip flag OFF → duplicate	✅ 200
10	Cross-independence: flags gate only their own resource	✅

Related Issues

N/A

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created. (No REST API contract change — only new cluster settings.)
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d4fb1b8)

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 Race Condition The uniqueness check is best-effort and not atomic. Two concurrent registrations with the same name can both observe zero hits and both succeed, creating duplicates. This is acknowledged in the PR description but remains a real issue. If strict uniqueness is required, a sidecar lock index with OpType.CREATE on a deterministic _id is needed. validateAgentNameUniqueness(mlAgent, ActionListener.wrap(unused -> { // Check if this agent needs model creation if (mlAgent.usesUnifiedInterface()) { createModelAndRegisterAgent(mlAgent, listener); return; } registerAgent(mlAgent, listener); }, listener::onFailure)); } /** * When {@code plugins.ml_commons.agent_name_uniqueness_enabled} is enabled, reject the * registration if an agent with the same name already exists in the same tenant. When the * setting is disabled (default), this is a no-op so existing clusters remain backward compatible. * * <p>For PLAN_EXECUTE_AND_REFLECT agents without a pre-existing executor-agent-id, an internal * "{@code <name> (ReAct)}" executor agent is auto-created; this method also checks that * derived name so the auto-created agent cannot collide with an existing one. * * <p>Note: this is a best-effort check, not a transactional guard. Two concurrent register * requests with the same name can both pass this check before either write is visible. * See the PR description for a follow-up plan if stricter semantics are required. */ private void validateAgentNameUniqueness(MLAgent mlAgent, ActionListener<Void> listener) { if (!mlFeatureEnabledSetting.isAgentNameUniquenessEnabled()) { listener.onResponse(null); return; } // MLAgent.validate() already rejects null/blank/over-length names upstream, so we rely on // that invariant here and don't re-check. String name = mlAgent.getName(); String tenantId = mlAgent.getTenantId(); checkAgentNameAvailable(name, tenantId, ActionListener.wrap(unused -> { // If this is a PLAN_EXECUTE_AND_REFLECT registration that will auto-create an // executor agent named "<name> (ReAct)", validate that derived name too. if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); } else { listener.onResponse(null); } }, listener::onFailure)); } private void checkAgentNameAvailable(String name, String tenantId, ActionListener<Void> listener) { NameUniquenessHelper.searchByExactName(client, sdkClient, ML_AGENT_INDEX, name, tenantId, ActionListener.wrap(response -> { if (response == null) { // Index not yet created - no duplicate possible. listener.onResponse(null); return; } long totalHits = response.getHits().getTotalHits() == null ? 0 : response.getHits().getTotalHits().value(); if (totalHits > 0) { listener .onFailure( new OpenSearchStatusException( "An agent with name [" + name + "] already exists. Agent names must be unique.", RestStatus.CONFLICT ) ); } else { listener.onResponse(null); } }, listener::onFailure)); } Possible Issue If sdkClient.searchDataObjectAsync completes exceptionally with an exception other than IndexNotFoundException, the catch block at line 96 will log and call listener.onFailure. However, the try-with-resources block at line 75 already restored the ThreadContext at line 77 before the exception handler runs. If the exception occurs during the async completion (line 76-95), the context.restore() at line 77 has already executed, so the second restore attempt in the catch block at line 96 may operate on stale or incorrect context state. try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { sdkClient.searchDataObjectAsync(searchDataObjectRequest).whenComplete((r, throwable) -> { context.restore(); if (throwable != null) { if (ExceptionsHelper.unwrap(throwable, IndexNotFoundException.class) != null) { // Index not yet created - no duplicate possible. listener.onResponse(null); return; } Exception cause = SdkClientUtils.unwrapAndConvertToException(throwable); log.error("Failed to search index [{}] for name uniqueness check", indexName, cause); listener.onFailure(cause); return; } try { listener.onResponse(r.searchResponse()); } catch (Exception e) { log.error("Failed to parse search response for name uniqueness check on index [{}]", indexName, e); listener.onFailure(e); } }); } catch (Exception e) { log.error("Failed to execute name uniqueness check on index [{}]", indexName, e); listener.onFailure(e); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d4fb1b8

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent thread context leak in async callback The context.restore() call is placed inside the async callback, but if an exception occurs before the callback completes or if the callback is never invoked, the context may not be restored. Move context.restore() into a finally block or ensure it's called in all code paths to prevent thread context leaks. plugin/src/main/java/org/opensearch/ml/helper/NameUniquenessHelper.java [75-95] -try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { +ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext(); +try { sdkClient.searchDataObjectAsync(searchDataObjectRequest).whenComplete((r, throwable) -> { - context.restore(); - if (throwable != null) { - if (ExceptionsHelper.unwrap(throwable, IndexNotFoundException.class) != null) { - // Index not yet created - no duplicate possible. - listener.onResponse(null); + try { + if (throwable != null) { + if (ExceptionsHelper.unwrap(throwable, IndexNotFoundException.class) != null) { + listener.onResponse(null); + return; + } + Exception cause = SdkClientUtils.unwrapAndConvertToException(throwable); + log.error("Failed to search index [{}] for name uniqueness check", indexName, cause); + listener.onFailure(cause); return; } - Exception cause = SdkClientUtils.unwrapAndConvertToException(throwable); - log.error("Failed to search index [{}] for name uniqueness check", indexName, cause); - listener.onFailure(cause); - return; + listener.onResponse(r.searchResponse()); + } finally { + context.restore(); } - ... }); +} catch (Exception e) { + context.restore(); + log.error("Failed to execute name uniqueness check on index [{}]", indexName, e); + listener.onFailure(e); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential thread context leak if the async callback fails to execute or throws an exception. However, the improved_code restructures the flow in a way that may not align with the async callback pattern, and the context.restore() placement in the callback is a common pattern for async operations.	Medium
General	Ensure listener invoked on validation exception The continueUpdate method wraps its logic in a try-catch that calls actionListener.onFailure on exception. However, if validateMemoryContainerNameUniquenessForUpdate itself throws an uncaught exception before invoking the listener, the outer actionListener may never be notified. Wrap the validation call in a try-catch to ensure the listener is always invoked. plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java [128-138] -validateMemoryContainerNameUniquenessForUpdate(newName, container, ActionListener.wrap(unused -> { - continueUpdate( - container, - newName, - newDescription, - allowedBackendRoles, - updateConfiguration, - memoryContainerId, - actionListener - ); -}, actionListener::onFailure)); +try { + validateMemoryContainerNameUniquenessForUpdate(newName, container, ActionListener.wrap(unused -> { + continueUpdate( + container, + newName, + newDescription, + allowedBackendRoles, + updateConfiguration, + memoryContainerId, + actionListener + ); + }, actionListener::onFailure)); +} catch (Exception e) { + log.error("Failed to validate memory container name uniqueness for update", e); + actionListener.onFailure(new OpenSearchStatusException("Internal server error", RestStatus.INTERNAL_SERVER_ERROR)); +} Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential issue where an uncaught exception in validateMemoryContainerNameUniquenessForUpdate could leave the listener uninvoked. However, the validation method itself is unlikely to throw synchronously given its implementation, and the continueUpdate method already has error handling.	Low
	Document race condition in uniqueness check The uniqueness check occurs after tenant validation but before the agent is registered. However, if createModelAndRegisterAgent or registerAgent fail, the name remains "reserved" in the user's mental model even though no agent was created. Consider adding a comment explaining that this is a best-effort check and concurrent requests may both pass validation, or document the race condition in the method's Javadoc. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [109-116] +// Best-effort uniqueness check - concurrent requests may both pass this validation validateAgentNameUniqueness(mlAgent, ActionListener.wrap(unused -> { // Check if this agent needs model creation if (mlAgent.usesUnifiedInterface()) { createModelAndRegisterAgent(mlAgent, listener); return; } registerAgent(mlAgent, listener); }, listener::onFailure)); Suggestion importance[1-10]: 4 __ Why: Adding a comment about the best-effort nature of the check is helpful for maintainability, but the PR description already documents this limitation. The suggestion improves code clarity without addressing a functional issue.	Low

---

Previous suggestions

Suggestions up to commit aa9b9ec

Category	Suggestion	Impact
Possible issue	Ensure thread context is always restored The context.restore() is called unconditionally before checking throwable, but if the whenComplete callback itself throws an exception, the context may not be restored. More critically, context.restore() should be called in a finally-like pattern. Consider wrapping the callback body in a try/finally to ensure context.restore() is always called even if an unexpected exception occurs inside the callback. plugin/src/main/java/org/opensearch/ml/helper/NameUniquenessHelper.java [75-77] try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { sdkClient.searchDataObjectAsync(searchDataObjectRequest).whenComplete((r, throwable) -> { - context.restore(); + try { + if (throwable != null) { + if (ExceptionsHelper.unwrap(throwable, IndexNotFoundException.class) != null) { + listener.onResponse(null); + return; + } + Exception cause = SdkClientUtils.unwrapAndConvertToException(throwable); + log.error("Failed to search index [{}] for name uniqueness check", indexName, cause); + listener.onFailure(cause); + return; + } + try { + listener.onResponse(r.searchResponse()); + } catch (Exception e) { + log.error("Failed to parse search response for name uniqueness check on index [{}]", indexName, e); + listener.onFailure(e); + } + } finally { + context.restore(); + } + }); +} Suggestion importance[1-10]: 6 __ Why: The context.restore() is called before the null check on throwable, but if an unexpected exception occurs inside the callback body, the context may not be restored. Wrapping the callback body in a try/finally would be safer, though in practice the current code handles all branches explicitly.	Low
General	Avoid hardcoded derived agent name suffix The derived executor agent name name + " (ReAct)" is hardcoded as a string literal here. If MLPlanExecuteAndReflectAgentRunner changes the suffix used when auto-creating the executor agent, this check will silently become stale and miss real collisions. The suffix should be referenced from a constant in MLPlanExecuteAndReflectAgentRunner to keep both in sync. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [145-148] if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { - checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); + checkAgentNameAvailable(name + MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_NAME_SUFFIX, tenantId, listener); Suggestion importance[1-10]: 5 __ Why: The hardcoded " (ReAct)" suffix could become stale if MLPlanExecuteAndReflectAgentRunner changes the suffix used when auto-creating the executor agent. Referencing a constant from that class would keep both in sync, but this requires the constant to exist in the outer codebase.	Low
	Ensure new tests are actually executed by test runner These test methods are missing the @Test annotation (or public visibility alone is insufficient for JUnit 4 test discovery in this project's test framework). Looking at the existing tests in the same file, they use public void test... without @Test, which suggests this is an OpenSearch test case using a different runner. However, the new tests in TransportUpdateMemoryContainerActionTests also lack @Test. Verify that the test runner used by this project auto-discovers public void test methods; if it uses JUnit 4 directly, add @Test annotations to ensure these tests are actually executed.* plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java [1996-2048] +@Test public void testDoExecute_UniquenessEnforced_DuplicateNameRejected() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessEnforced_UniqueNameAllowed() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessEnforced_IndexNotFound_AllowsCreation() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessDisabled_SkipsSearch() throws InterruptedException { Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the new test methods lack @Test annotations, but the suggestion itself acknowledges that the project may use a test runner that auto-discovers public void test* methods (like OpenSearch's OpenSearchTestCase). Since existing tests in the same file follow the same pattern without @Test, this is likely intentional and the tests will run correctly.	Low
	Document tenant validation assumption in refactored method The TenantAwareHelper.validateTenantId call was moved from proceedWithAgentRegistration to doExecute, but proceedWithAgentRegistration is also called from createModelAndRegisterAgent which is invoked after the uniqueness check. Since proceedWithAgentRegistration no longer validates the tenant, if createModelAndRegisterAgent is refactored in the future to be called from a different path, the tenant validation could be bypassed. Verify that all code paths into proceedWithAgentRegistration are covered by the upfront tenant check. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [105-116] -if (!TenantAwareHelper.validateTenantId(mlFeatureEnabledSetting, mlAgent.getTenantId(), listener)) { - return; -} +// No code change needed here, but add a comment in proceedWithAgentRegistration to document +// that tenant validation is expected to have been performed by the caller (doExecute). +// Consider adding an assertion or re-validating if the method becomes reachable from other paths. -validateAgentNameUniqueness(mlAgent, ActionListener.wrap(unused -> { - Suggestion importance[1-10]: 2 __ Why: The suggestion asks to verify/document an assumption rather than fix a concrete bug. The improved_code doesn't actually change any code, making this a low-impact documentation suggestion.	Low

Suggestions up to commit 4f8f039

Category	Suggestion	Impact
Possible issue	Fix premature thread context closure before async callback The ThreadContext.StoredContext is closed by the try-with-resources block before the async callback runs, but context.restore() is called inside the callback after the context is already closed. The StoredContext should be captured outside the try-with-resources and restored inside the async callback, similar to how other async operations in the codebase handle thread context stashing. plugin/src/main/java/org/opensearch/ml/helper/NameUniquenessHelper.java [75-99] -try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { +ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext(); +try { sdkClient.searchDataObjectAsync(searchDataObjectRequest).whenComplete((r, throwable) -> { context.restore(); if (throwable != null) { if (ExceptionsHelper.unwrap(throwable, IndexNotFoundException.class) != null) { - // Index not yet created - no duplicate possible. listener.onResponse(null); return; } Exception cause = SdkClientUtils.unwrapAndConvertToException(throwable); log.error("Failed to search index [{}] for name uniqueness check", indexName, cause); listener.onFailure(cause); return; } try { listener.onResponse(r.searchResponse()); } catch (Exception e) { log.error("Failed to parse search response for name uniqueness check on index [{}]", indexName, e); listener.onFailure(e); } }); } catch (Exception e) { + context.close(); log.error("Failed to execute name uniqueness check on index [{}]", indexName, e); listener.onFailure(e); } Suggestion importance[1-10]: 8 __ Why: The try-with-resources block closes StoredContext when the try block exits, but the async callback calls context.restore() after the context is already closed. This is a real bug that could cause thread context corruption in async execution paths.	Medium
	Add missing @test annotations to new test methods These test methods are missing the @Test annotation, which means they will not be executed by JUnit and the new uniqueness behavior will have no test coverage. Add @Test to each of these methods. plugin/src/test/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerActionTests.java [1996-2048] +@Test public void testDoExecute_UniquenessEnforced_DuplicateNameRejected() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessEnforced_UniqueNameAllowed() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessEnforced_IndexNotFound_AllowsCreation() throws InterruptedException { ... +@Test public void testDoExecute_UniquenessDisabled_SkipsSearch() throws InterruptedException { Suggestion importance[1-10]: 8 __ Why: The new test methods in TransportCreateMemoryContainerActionTests are missing @Test annotations, which means they won't be executed by JUnit, leaving the new uniqueness feature without actual test coverage. This is a real correctness issue in the test suite.	Medium
	Ensure tenant validation covers all registration code paths The TenantAwareHelper.validateTenantId call was moved from proceedWithAgentRegistration to doExecute, but proceedWithAgentRegistration is also called from createModelAndRegisterAgent after model creation. If createModelAndRegisterAgent calls proceedWithAgentRegistration directly (bypassing doExecute), the tenant validation that was removed from proceedWithAgentRegistration will no longer run for that code path. Verify that the removed validation in proceedWithAgentRegistration does not leave the model-creation path unprotected. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [105-116] +if (!TenantAwareHelper.validateTenantId(mlFeatureEnabledSetting, mlAgent.getTenantId(), listener)) { + return; +} +validateAgentNameUniqueness(mlAgent, ActionListener.wrap(unused -> { + // Check if this agent needs model creation + if (mlAgent.usesUnifiedInterface()) { + createModelAndRegisterAgent(mlAgent, listener); + return; + } + registerAgent(mlAgent, listener); +}, listener::onFailure)); Suggestion importance[1-10]: 5 __ Why: The concern about proceedWithAgentRegistration being called from createModelAndRegisterAgent without tenant validation is valid. However, the improved_code is identical to existing_code, meaning the suggestion only asks to verify rather than providing a concrete fix.	Low
General	Avoid hardcoding derived executor agent name string The derived executor agent name " (ReAct)" is hardcoded here, but if MLPlanExecuteAndReflectAgentRunner uses a different format or constant to construct this name, the uniqueness check will silently miss the collision. This derived name string should be sourced from a shared constant in MLPlanExecuteAndReflectAgentRunner to stay in sync with the actual auto-creation logic. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [145-152] if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { - checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); + checkAgentNameAvailable(MLPlanExecuteAndReflectAgentRunner.buildExecutorAgentName(name), tenantId, listener); } else { listener.onResponse(null); } Suggestion importance[1-10]: 4 __ Why: The suggestion to use a shared constant for the " (ReAct)" suffix is valid for maintainability, but the improved_code references a method MLPlanExecuteAndReflectAgentRunner.buildExecutorAgentName(name) that may not exist in the codebase, making the suggestion speculative rather than directly actionable.	Low

Suggestions up to commit bbbe096

Category	Suggestion	Impact
Possible issue	Scope uniqueness check to the correct tenant The query only filters by name.keyword but does not filter by tenantId. In multi-tenant mode, this means the uniqueness check is performed across all tenants, potentially causing false 409 conflicts when two different tenants register agents with the same name. A tenantId filter should be added to the query when tenantId is not null. plugin/src/main/java/org/opensearch/ml/helper/NameUniquenessHelper.java [65-66] BoolQueryBuilder query = new BoolQueryBuilder().filter(new TermQueryBuilder("name.keyword", name)); +if (tenantId != null) { + query.filter(new TermQueryBuilder("tenant_id", tenantId)); +} SearchSourceBuilder sourceBuilder = new SearchSourceBuilder().query(query).size(1).fetchSource(false); Suggestion importance[1-10]: 8 __ Why: The tenantId parameter is passed to SearchDataObjectRequest via .tenantId(tenantId), which is the SDK-level tenant scoping mechanism. However, if the underlying index stores tenant data in a field and the SDK doesn't filter by tenant at the query level, a missing tenantId field filter in the BoolQueryBuilder could cause cross-tenant false positives. This is a valid concern for multi-tenant correctness, though it depends on how SdkClient handles tenant scoping internally.	Medium
	Use original agent's tenant ID for update uniqueness check The update uniqueness check uses updateInput.getTenantId() for the tenant scope, but the tenant ID should come from the already-retrieved originalAgent to ensure consistency. If the update input omits or spoofs a tenant ID, the uniqueness check could be scoped to the wrong tenant. plugin/src/main/java/org/opensearch/ml/action/agents/UpdateAgentTransportAction.java [220-221] NameUniquenessHelper - .searchByExactName(client, sdkClient, ML_AGENT_INDEX, newName, updateInput.getTenantId(), ActionListener.wrap(response -> { + .searchByExactName(client, sdkClient, ML_AGENT_INDEX, newName, originalAgent.getTenantId(), ActionListener.wrap(response -> { Suggestion importance[1-10]: 7 __ Why: Using updateInput.getTenantId() instead of originalAgent.getTenantId() could allow a malicious caller to scope the uniqueness check to a different tenant by providing a spoofed tenant ID in the update input. Using the already-validated originalAgent.getTenantId() is more secure and consistent.	Medium
General	Avoid hardcoded derived executor agent name The derived executor agent name name + " (ReAct)" is hardcoded as a string concatenation. If the actual auto-creation logic in proceedWithAgentRegistration uses a different format or constant, the pre-check and the actual creation will be out of sync. This derived name should be extracted into a shared constant to avoid drift. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [145-152] if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { - checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); + String derivedExecutorName = MLPlanExecuteAndReflectAgentRunner.buildExecutorAgentName(name); + checkAgentNameAvailable(derivedExecutorName, tenantId, listener); } else { listener.onResponse(null); } Suggestion importance[1-10]: 4 __ Why: The hardcoded " (ReAct)" suffix could drift from the actual auto-creation logic. However, the improved_code references a MLPlanExecuteAndReflectAgentRunner.buildExecutorAgentName(name) method that may not exist in the codebase, making the suggested fix potentially non-compilable as-is.	Low

Suggestions up to commit bbbe096

Category	Suggestion	Impact
Possible issue	Scope uniqueness search to requesting tenant The query only filters by name.keyword but does not filter by tenantId. In multi-tenant mode, this means the uniqueness check is performed across all tenants rather than being scoped to the requesting tenant, which could incorrectly reject names that are unique within the tenant but exist in another tenant. Add a tenantId term filter to the query when tenantId is not null. plugin/src/main/java/org/opensearch/ml/helper/NameUniquenessHelper.java [65-66] BoolQueryBuilder query = new BoolQueryBuilder().filter(new TermQueryBuilder("name.keyword", name)); +if (tenantId != null) { + query.filter(new TermQueryBuilder("tenant_id", tenantId)); +} SearchSourceBuilder sourceBuilder = new SearchSourceBuilder().query(query).size(1).fetchSource(false); Suggestion importance[1-10]: 8 __ Why: This is a significant correctness issue: without filtering by tenantId, the uniqueness check spans all tenants, which would incorrectly reject names that are unique within the requesting tenant but exist in another tenant. However, the SearchDataObjectRequest already includes tenantId which the SDK client may use for scoping, so the actual impact depends on the SDK client implementation.	Medium
	Use retrieved agent's tenant ID for uniqueness check The update uniqueness check uses updateInput.getTenantId() for the tenant scope, but the tenant ID should come from the already-retrieved originalAgent to ensure consistency. If the update input omits the tenant ID, the search would be unscoped and could produce incorrect results. plugin/src/main/java/org/opensearch/ml/action/agents/UpdateAgentTransportAction.java [220-221] NameUniquenessHelper - .searchByExactName(client, sdkClient, ML_AGENT_INDEX, newName, updateInput.getTenantId(), ActionListener.wrap(response -> { + .searchByExactName(client, sdkClient, ML_AGENT_INDEX, newName, originalAgent.getTenantId(), ActionListener.wrap(response -> { Suggestion importance[1-10]: 6 __ Why: Using updateInput.getTenantId() instead of originalAgent.getTenantId() could result in an unscoped search if the update input omits the tenant ID, potentially producing incorrect uniqueness results. Using the already-retrieved agent's tenant ID is more reliable and consistent.	Low
	Guard against null name in uniqueness check If input.getName() is null (e.g., the caller omits the name field), passing null to NameUniquenessHelper.searchByExactName as the name parameter will result in a TermQueryBuilder with a null value, which may throw a NullPointerException or produce unexpected query behavior. Add a null check before invoking the uniqueness validation. plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportCreateMemoryContainerAction.java [102-105] -validateMemoryContainerNameUniqueness(input.getName(), tenantId, ActionListener.wrap(unused -> { - // Validate configuration before creating memory container +if (mlFeatureEnabledSetting.isAgenticMemoryNameUniquenessEnabled() && input.getName() != null) { + validateMemoryContainerNameUniqueness(input.getName(), tenantId, ActionListener.wrap(unused -> { validateConfigurationAndCreate(input, user, tenantId, listener); }, listener::onFailure)); +} else { + validateConfigurationAndCreate(input, user, tenantId, listener); +} Suggestion importance[1-10]: 4 __ Why: While the comment in validateMemoryContainerNameUniqueness states it relies on upstream null-name rejection, the improved_code duplicates the feature flag check which is already inside validateMemoryContainerNameUniqueness, making the suggestion's implementation awkward. The null guard concern is valid but minor given the upstream invariant.	Low
General	Reference constant for derived executor agent name The derived executor agent name " (ReAct)" is hardcoded as a string literal here. If MLPlanExecuteAndReflectAgentRunner uses a different suffix or this naming convention changes, the uniqueness check will silently become incorrect. Consider referencing a constant from MLPlanExecuteAndReflectAgentRunner for the suffix to keep the two in sync. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [145-152] if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { - checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); + checkAgentNameAvailable(MLPlanExecuteAndReflectAgentRunner.buildExecutorAgentName(name), tenantId, listener); } else { listener.onResponse(null); } Suggestion importance[1-10]: 4 __ Why: The hardcoded " (ReAct)" suffix is a maintainability concern — if the naming convention changes in MLPlanExecuteAndReflectAgentRunner, the uniqueness check would silently become incorrect. However, the improved_code references a buildExecutorAgentName method that may not exist in the codebase, making the suggestion speculative.	Low

Suggestions up to commit 211fb54

Category	Suggestion	Impact
General	Replace hardcoded executor name suffix with a shared constant The derived executor agent name " (ReAct)" is hardcoded as a string literal here, but the actual suffix used during auto-creation in MLPlanExecuteAndReflectAgentRunner may differ or change. This creates a fragile coupling. The suffix should be extracted into a shared constant in MLPlanExecuteAndReflectAgentRunner and referenced here to ensure the uniqueness check always matches the actual auto-created name. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [151-158] if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT && mlAgent.getParameters() != null && !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) { - checkAgentNameAvailable(name + " (ReAct)", tenantId, listener); + checkAgentNameAvailable(name + MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_NAME_SUFFIX, tenantId, listener); } else { listener.onResponse(null); } Suggestion importance[1-10]: 6 __ Why: The hardcoded " (ReAct)" suffix creates a fragile coupling with MLPlanExecuteAndReflectAgentRunner. If the suffix changes, the uniqueness check would silently break. Extracting it to a constant would improve maintainability, though the improved_code references a constant (EXECUTOR_AGENT_NAME_SUFFIX) that may not yet exist in the codebase.	Low
	Extract duplicated name-uniqueness check into shared utility The identical checkAgentNameAvailable / validateMemoryContainerNameUniqueness pattern (build query → build SearchDataObjectRequest → stash context → call searchDataObjectAsync → restore → check hits → call listener) is duplicated across four transport action classes with only minor variations. This duplication increases maintenance burden and risk of inconsistency. Consider extracting a shared static helper (e.g., in a NameUniquenessHelper utility class) that accepts the index name, field name, value, tenant ID, sdk client, thread pool, and listener. plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java [172-174] -try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { - sdkClient.searchDataObjectAsync(searchDataObjectRequest).whenComplete((r, throwable) -> { - context.restore(); +// Extract to a shared utility, e.g.: +// NameUniquenessHelper.checkNameAvailable( +// sdkClient, client.threadPool(), ML_AGENT_INDEX, "name.keyword", +// name, tenantId, listener); Suggestion importance[1-10]: 5 __ Why: The duplication across four transport action classes is a real maintainability concern. However, the improved_code is just a comment placeholder and doesn't provide an actionable implementation, making this more of a design suggestion than a concrete fix.	Low
	Use request tenant ID for uniqueness search scope The uniqueness check for update uses originalContainer.getTenantId() as the tenant scope for the search, but the update request's input (newName) may come from a different tenant context. The tenant ID should be sourced from the update request input (or the validated request context) rather than the stored container, to be consistent with how the create path and the agent update path scope their searches. plugin/src/main/java/org/opensearch/ml/action/memorycontainer/TransportUpdateMemoryContainerAction.java [273] -.tenantId(originalContainer.getTenantId()) +.tenantId(updateInput != null && updateInput.getTenantId() != null ? updateInput.getTenantId() : originalContainer.getTenantId()) Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about tenant ID sourcing, but in the update path the originalContainer.getTenantId() is actually the correct authoritative source since the container was already validated for tenant access. The improved_code adds a conditional that doesn't meaningfully improve correctness and could introduce inconsistency.	Low
	Fallback to stored agent tenant ID when update input lacks one The validateAgentNameUniquenessForUpdate method uses updateInput.getTenantId() for the search scope, but MLAgentUpdateInput.getTenantId() may return null if the caller did not supply a tenant ID (e.g., in single-tenant mode). In that case the search would be unscoped, which is correct for single-tenant but could silently skip tenant isolation in multi-tenant mode. The tenant ID should be validated or defaulted from the retrieved agent's tenant ID as a fallback, consistent with how the memory container update path handles this. plugin/src/main/java/org/opensearch/ml/action/agents/UpdateAgentTransportAction.java [139-147] validateAgentNameUniquenessForUpdate( mlAgentUpdateInput, retrievedAgent, ActionListener .wrap( unused -> updateAgent(agentId, mlAgentUpdateInput, retrievedAgent, wrappedListener), wrappedListener::onFailure ) ); +// In validateAgentNameUniquenessForUpdate, use: +// String tenantId = updateInput.getTenantId() != null ? updateInput.getTenantId() : originalAgent.getTenantId(); Suggestion importance[1-10]: 4 __ Why: The concern about updateInput.getTenantId() returning null is valid for multi-tenant correctness, but the improved_code is identical to the existing_code with only a comment added, making it not actionable as written. The actual fix would need to be inside validateAgentNameUniquenessForUpdate.	Low

[github-actions]

Persistent review updated to latest commit 7bfddb8

[github-actions]

Persistent review updated to latest commit bbf285e

[github-actions]

Persistent review updated to latest commit 211fb54

[github-actions]

Persistent review updated to latest commit bbbe096

[github-actions]

Persistent review updated to latest commit bbbe096

[github-actions]

Persistent review updated to latest commit 4f8f039

[github-actions]

Persistent review updated to latest commit aa9b9ec

[github-actions]

Persistent review updated to latest commit d4fb1b8