Nova model support in FunctionCalling

[rithinpullela]

Description

Adds support for Nova Models in function calling

Related Issues

Resolves #4680

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• 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 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: In RestBedrockConverseNovaFunctionCallingIT.java, AWS credentials (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN) are read from environment variables and then embedded as plain text into a JSON request body string (lines 92-95). This could result in credentials being logged in test output, HTTP request logs, or server-side logs, potentially exposing sensitive AWS credentials.

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Nova Model Provider registration in common module Relevant files: common/src/main/java/org/opensearch/ml/common/agent/BedrockConverseNovaModelProvider.java common/src/main/java/org/opensearch/ml/common/input/execute/agent/ModelProviderType.java common/src/main/java/org/opensearch/ml/common/model/ModelProviderFactory.java common/src/test/java/org/opensearch/ml/common/agent/BedrockConverseNovaModelProviderTest.java common/src/test/java/org/opensearch/ml/common/model/ModelProviderFactoryTest.java Sub-PR theme: Nova function calling support in ml-algorithms module Relevant files: ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLPlanExecuteAndReflectAgentRunner.java ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/streaming/StreamingHandlerFactory.java ml-algorithms/src/main/java/org/opensearch/ml/engine/function_calling/BedrockConverseNovaFunctionCalling.java ml-algorithms/src/main/java/org/opensearch/ml/engine/function_calling/FunctionCallingFactory.java ml-algorithms/src/test/java/org/opensearch/ml/engine/function_calling/BedrockConverseNovaFunctionCallingTests.java Sub-PR theme: Nova function calling integration test Relevant files: plugin/src/test/java/org/opensearch/ml/rest/RestBedrockConverseNovaFunctionCallingIT.java

⚡ Recommended focus areas for review Credential Exposure AWS credentials (access_key, secret_key, session_token) are embedded directly in the agent request body string. Even in tests, this pattern risks accidental logging of credentials in test output or server logs. Consider using a helper method that masks or avoids embedding raw credentials in request bodies. String agentBody = String .format( Locale.ROOT, "{\n" + " \"name\": \"Nova Tool Test Agent\",\n" + " \"type\": \"conversational\",\n" + " \"description\": \"Test agent for Nova function calling with tools\",\n" + " \"model\": {\n" + " \"model_id\": \"%s\",\n" + " \"model_provider\": \"bedrock/converse/nova\",\n" + " \"credential\": {\n" + " \"access_key\": \"%s\",\n" + " \"secret_key\": \"%s\",\n" + " \"session_token\": \"%s\"\n" + " },\n" + " \"model_parameters\": {\n" + " \"max_iteration\": 5,\n" + " \"stop_when_no_tool_found\": true,\n" + " \"system_prompt\": \"You are a helpful assistant. Use tools when needed.\"\n" + " }\n" + " },\n" + " \"tools\": [\n" + " {\n" + " \"type\": \"ListIndexTool\"\n" + " }\n" + " ],\n" + " \"memory\": {\n" + " \"type\": \"conversation_index\"\n" + " }\n" + "}", NOVA_MODEL_ID, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN ); Incomplete Validation The validateLLMInterface method only allows LLM_INTERFACE_BEDROCK_CONVERSE_CLAUDE and LLM_INTERFACE_BEDROCK_CONVERSE_NOVA, but other valid interfaces like LLM_INTERFACE_BEDROCK_CONVERSE and LLM_INTERFACE_BEDROCK_CONVERSE_DEEPSEEK_R1 may also be used with the AWS connector. Verify whether these should also be permitted to avoid unexpected runtime failures. private void validateLLMInterface(String llmInterface) { switch (llmInterface) { case LLM_INTERFACE_BEDROCK_CONVERSE_CLAUDE: case LLM_INTERFACE_BEDROCK_CONVERSE_NOVA: break; default: throw new IllegalArgumentException(String.format("Unsupported llm interface: %s", llmInterface)); } Missing Case The switch statement for llmResponseFilter now groups LLM_INTERFACE_BEDROCK_CONVERSE_CLAUDE, LLM_INTERFACE_BEDROCK_CONVERSE_DEEPSEEK_R1, and LLM_INTERFACE_BEDROCK_CONVERSE_NOVA together, but LLM_INTERFACE_BEDROCK_CONVERSE (the base bedrock/converse interface) is not handled and would fall into the default exception. Verify if this is intentional or if it should also be included. String llmResponseFilter = switch (llmInterface.trim().toLowerCase(Locale.ROOT)) { case LLM_INTERFACE_BEDROCK_CONVERSE_CLAUDE, LLM_INTERFACE_BEDROCK_CONVERSE_DEEPSEEK_R1, LLM_INTERFACE_BEDROCK_CONVERSE_NOVA -> "$.output.message.content[0].text"; case LLM_INTERFACE_OPENAI_V1_CHAT_COMPLETIONS -> "$.choices[0].message.content"; default -> throw new MLException(String.format("Unsupported llm interface: %s", llmInterface)); };

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Inconsistent case handling in interface validation The switch statement compares llmInterface directly without trimming or lowercasing, while other places in the codebase (e.g., FunctionCallingFactory, MLPlanExecuteAndReflectAgentRunner) use llmInterface.trim().toLowerCase(Locale.ROOT) before switching. This inconsistency means that inputs with different casing or surrounding whitespace will incorrectly throw an IllegalArgumentException in this method. ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java [195-202] private void validateLLMInterface(String llmInterface) { - switch (llmInterface) { + switch (llmInterface.trim().toLowerCase(Locale.ROOT)) { case LLM_INTERFACE_BEDROCK_CONVERSE_CLAUDE: case LLM_INTERFACE_BEDROCK_CONVERSE_NOVA: break; default: throw new IllegalArgumentException(String.format("Unsupported llm interface: %s", llmInterface)); } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inconsistency where validateLLMInterface uses a raw switch without .trim().toLowerCase(Locale.ROOT), unlike other places in the codebase. This could cause valid interfaces with different casing to fail validation unexpectedly.	Low
General	Test assumes unverified case-insensitive behavior This test assumes that ModelProviderFactory.getProvider handles case-insensitive input, but looking at the factory implementation, ModelProviderType.from(providerType) is called without any case normalization. If ModelProviderType.from does not lowercase the input, this test will fail with an exception rather than returning a BedrockConverseNovaModelProvider. Verify that ModelProviderType.from performs case-insensitive matching, or this test will produce a false expectation. common/src/test/java/org/opensearch/ml/common/model/ModelProviderFactoryTest.java [122-132] @Test public void testGetProvider_BedrockConverseNova_CaseInsensitive() { // Arrange String providerType = "BEDROCK/CONVERSE/NOVA"; - // Act - ModelProvider provider = ModelProviderFactory.getProvider(providerType); - - // Assert - assertNotNull(provider); - assertTrue(provider instanceof BedrockConverseNovaModelProvider); + // Act & Assert: expect exception if case-insensitive matching is not supported, + // or verify ModelProviderType.from lowercases the input before matching + try { + ModelProvider provider = ModelProviderFactory.getProvider(providerType); + assertNotNull(provider); + assertTrue(provider instanceof BedrockConverseNovaModelProvider); + } catch (IllegalArgumentException e) { + // Document that case-insensitive matching is not supported + assertTrue(e.getMessage().contains("Unsupported") || e.getMessage().contains("BEDROCK/CONVERSE/NOVA")); + } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about whether ModelProviderType.from supports case-insensitive matching, but the improved_code replaces a clean assertion with a try-catch that accepts both success and failure, which weakens the test. The suggestion would be better addressed by verifying the from method's implementation rather than changing the test structure.	Low
Security	Credentials exposed in plaintext in test logs The AWS credentials are stored as static final fields, meaning they are read once at class load time. If the environment variables are set after class initialization, the test will fail. More critically, these credentials are embedded directly into the agent request body as plaintext strings, which could be logged or exposed in test output. Consider masking them in logs or using a secrets manager approach. plugin/src/test/java/org/opensearch/ml/rest/RestBedrockConverseNovaFunctionCallingIT.java [27-29] +private static final String AWS_ACCESS_KEY_ID = System.getenv("AWS_ACCESS_KEY_ID"); +private static final String AWS_SECRET_ACCESS_KEY = System.getenv("AWS_SECRET_ACCESS_KEY"); +private static final String AWS_SESSION_TOKEN = System.getenv("AWS_SESSION_TOKEN"); - Suggestion importance[1-10]: 3 __ Why: The existing_code and improved_code are identical, so no actual change is proposed. While the concern about credentials being logged is valid for integration tests, this pattern is common in AWS integration tests and the credentialsNotSet() guard already handles the case where credentials are absent.	Low

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.05%. Comparing base (44b65a6) to head (f06767c).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...thms/agent/MLPlanExecuteAndReflectAgentRunner.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #4682   +/-   ##
=========================================
  Coverage     77.04%   77.05%           
- Complexity    11354    11370   +16     
=========================================
  Files           944      946    +2     
  Lines         51145    51151    +6     
  Branches       6196     6196           
=========================================
+ Hits          39407    39414    +7     
+ Misses         9125     9123    -2     
- Partials       2613     2614    +1     

Flag	Coverage Δ
ml-commons	77.05% <85.71%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.