[ExceptionHandling] Throw proper 400 errors instead of 500 for agent execute and MCP #3988

pyek-bot · 2025-07-17T01:03:50Z

Description

Throws proper 400 errors for bad requests

Request - Illegal value in agent execution

POST /_plugins/_ml/agents/<ID>/_execute?async=true
{
  "parameters": {
    "question": "Respond with Hi",
    "is_async": random
  }
}

Response:

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"org.opensearch.ml.common.input.MLInput.setAlgorithm(org.opensearch.ml.common.FunctionName)\" because \"mlInput\" is null"
            }
        ],
        "type": "null_pointer_exception",
        "reason": "Cannot invoke \"org.opensearch.ml.common.input.MLInput.setAlgorithm(org.opensearch.ml.common.FunctionName)\" because \"mlInput\" is null"
    },
    "status": 500
}

Post this change:

{
    "error": {
        "root_cause": [
            {
                "type": "json_parse_exception",
                "reason": "Unrecognized token 'asjkdhsadkjfh': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 4, column: 36]"
            }
        ],
        "type": "json_parse_exception",
        "reason": "Unrecognized token 'asjkdhsadkjfh': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 4, column: 36]"
    },
    "status": 400
}

Request: - register same name tool in MCP

POST /_plugins/_ml/mcp/tools/_register
{
  "tools": [
    {
      "type": "SearchIndexTool",
      "name": "this is random",
      "attributes": {
        "type": "fool"
      }
    }
  ]
}

Response:

{
    "error": {
        "root_cause": [
            {
                "type": "exception",
                "reason": "Unable to register tools: [this is random] as they already exist"
            }
        ],
        "type": "exception",
        "reason": "Unable to register tools: [this is random] as they already exist"
    },
    "status": 500
}

Post this change:

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "Unable to register tools: [this is random] as they already exist"
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "Unable to register tools: [this is random] as they already exist"
    },
    "status": 400
}

Related Issues

Resolves #3987

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.

Signed-off-by: Pavan Yekbote <pybot@amazon.com>

dhrubo-os · 2025-07-17T01:12:24Z

common/src/main/java/org/opensearch/ml/common/MLCommonsClassLoader.java

    }

-    public static <S> S initConnector(String name, Object[] initArgs, Class<?>... constructorParameterTypes) {
+    public static <S> S initConnector(String name, Object[] initArgs, Class<?>... constructorParameterTypes) throws JsonParseException {


why do we need to add throws JsonParseException?

the internal init method is throwing this exception, so this needs to be as well.

Now we are throwing new type of exception rather than simply error logging and returning null

the internal init method is throwing this exception

Why?

For example:

POST /_plugins/_ml/agents/<agent_id>/_execute { "parameters": { "question": "Give me statistics about my cluster", "is_async": random } }

Here, we want to surface the actual error. In this case, is_async has invalid value, so that triggers the JsonParseException which we want to show to the user.

I'm trying to understand :

if (cause instanceof MLException) { throw (MLException) cause; } else if (cause instanceof IllegalArgumentException) { throw (IllegalArgumentException) cause;

why we don't need to add MLException or IllegalArgumentException in throws?

ah, got it. MLException and IllegalArgumentException are runtime exceptions so they don't need explicit throws.

Unrecognized token 'asjkdhsadkjfh': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n at [Source: REDACTED (StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION disabled); line: 4, column: 36]

I understand it's better to send a json parse exception as a final defense. But I think we should send validation error before even coming here. I assume this value is optional. But if the value is provided then we should check if this is a valid value. If not send a validation error.

If I'm a customer, from this error nothing makes sense to me except I just know a json parse error happened somewhere. And I don't have any direction how can I fix my issue.

Agreed, we should have some validation before even reaching this point.

This PR, i'm just ensuring we don't throw 500 errors to the user as this was raised as a security concern. It can be used to exploit codebase and expose unhandled paths.

As a last defense, I think this is good enough to understand the issue: it is pointing out which is the invalid token

Unrecognized token 'asjkdhsadkjfh': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n

As you rightly mentioned, the correct solution would be to add validation to the input before executing any kind of task.

I created an issue to add proper validation for different kinds of executes depending on the Function being executed: #4013

we can track that there, I would like to avoid this here and immediately prevent potential exploits as a quick fix.

I'm fine if you want to work on a separate PR. But let's make sure we are working on that issue too. I still think this is a broken CX experience.

codecov · 2025-07-17T02:11:58Z

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.63%. Comparing base (7040e8a) to head (bc364c4).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...org/opensearch/ml/common/MLCommonsClassLoader.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3988      +/-   ##
============================================
+ Coverage     80.62%   80.63%   +0.01%     
- Complexity     8003     8007       +4     
============================================
  Files           695      695              
  Lines         35023    35025       +2     
  Branches       3935     3936       +1     
============================================
+ Hits          28236    28243       +7     
+ Misses         5051     5046       -5     
  Partials       1736     1736

Flag	Coverage Δ
ml-commons	`80.63% <33.33%> (+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.

akolarkunnu · 2025-07-29T17:11:49Z

plugin/src/main/java/org/opensearch/ml/action/mcpserver/TransportMcpToolsRegisterAction.java

                            .format(Locale.ROOT, "Unable to register tools: %s as they already exist", existingTools);
                        log.warn(exceptionMessage);
-                        restoreListener.onFailure(new OpenSearchException(exceptionMessage));
+                        restoreListener.onFailure(new IllegalArgumentException(exceptionMessage));


Why is this change?

In this case, since the tool already exists, we can confidently say that it is an IllegalArgumentException as the name provided is already used.

akolarkunnu · 2025-07-29T17:14:53Z

common/src/main/java/org/opensearch/ml/common/MLCommonsClassLoader.java

                throw (MLException) cause;
            } else if (cause instanceof IllegalArgumentException) {
                throw (IllegalArgumentException) cause;
+            } else if (cause instanceof JsonParseException) {


Can you please explain in PR description, how's this fix handles the actual issue.

Added more details to the description regarding the responses before and after this change

akolarkunnu · 2025-07-30T11:43:18Z

common/src/test/java/org/opensearch/ml/common/MLCommonsClassLoaderTests.java


    @Test(expected = IllegalArgumentException.class)
-    public void testConnectorInitializationException() {
+    public void testConnectorInitializationException() throws JsonParseException {


Please add the test case which actually throws JsonParseException, the actual scenario which you are fixing this PR.

@pyek-bot you should be able to write a test like this:

@Test public void testJsonParseExceptionHandling() throws Exception { // Add a field to access connectorClassMap for testing Field connectorClassMapField = MLCommonsClassLoader.class.getDeclaredField("connectorClassMap"); connectorClassMapField.setAccessible(true); @SuppressWarnings("unchecked") Map<String, Class<?>> connectorClassMap = (Map<String, Class<?>>) connectorClassMapField.get(null); // Add our mock class to the map connectorClassMap.put("TestConnector", MockClassThrowingJsonParseException.class); // We expect the method to throw JsonParseException exceptionRule.expect(JsonParseException.class); exceptionRule.expectMessage("Test JsonParseException"); // Call the method that should throw JsonParseException MLCommonsClassLoader.initConnector("TestConnector", new Object[]{}, new Class<?>[]{}); } // Mock class that throws JsonParseException when instantiated private static class MockClassThrowingJsonParseException { public MockClassThrowingJsonParseException() throws JsonParseException { throw new JsonParseException(null, "Test JsonParseException"); } }

thanks for sharing @dhrubo-os, i added a simpler test case in the latest commit, can you take a look at that?

Looks good to me.

Signed-off-by: Pavan Yekbote <pybot@amazon.com>

akolarkunnu

Looks good to me.

dhrubo-os · 2025-07-31T16:38:34Z

Approved the PR. Let's work on the issue you created for validation. Thanks.

pyek-bot · 2025-08-03T21:51:52Z

Thanks for the review and merge!

Can we please add 3.1 backport label as well please?

cc: @dhrubo-os @ylwu-amzn

…execute and MCP (#3988) * improve exception handling Signed-off-by: Pavan Yekbote <pybot@amazon.com> * feat: add test case for jsonparseexception Signed-off-by: Pavan Yekbote <pybot@amazon.com> --------- Signed-off-by: Pavan Yekbote <pybot@amazon.com> Co-authored-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 943ef8c)

…execute and MCP (#3988) (#4051) * improve exception handling * feat: add test case for jsonparseexception --------- (cherry picked from commit 943ef8c) Signed-off-by: Pavan Yekbote <pybot@amazon.com> Co-authored-by: Pavan Yekbote <pybot@amazon.com> Co-authored-by: Dhrubo Saha <dhrubo@amazon.com>

improve exception handling

af3884e

Signed-off-by: Pavan Yekbote <pybot@amazon.com>

pyek-bot requested review from HenryL27, Zhangxunmt, austintlee, b4sjoo, dhrubo-os, jngz-es, mingshl, model-collapse, rbhavna, xinyual, ylwu-amzn and zane-neo as code owners July 17, 2025 01:03

pyek-bot temporarily deployed to ml-commons-cicd-env-require-approval July 17, 2025 01:05 — with GitHub Actions Inactive

pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval July 17, 2025 01:05 — with GitHub Actions Failure

pyek-bot temporarily deployed to ml-commons-cicd-env-require-approval July 17, 2025 01:05 — with GitHub Actions Inactive

brianf-aws approved these changes Jul 17, 2025

View reviewed changes

dhrubo-os reviewed Jul 17, 2025

View reviewed changes

jiapingzeng approved these changes Jul 28, 2025

View reviewed changes

pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval July 28, 2025 22:20 — with GitHub Actions Failure

akolarkunnu reviewed Jul 29, 2025

View reviewed changes

Merge branch 'main' into exception_handling

d3b59b9

dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval July 29, 2025 18:10 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval July 29, 2025 18:10 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval July 29, 2025 18:10 — with GitHub Actions Failure

Merge branch 'main' into exception_handling

132a70c

dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval July 29, 2025 22:08 — with GitHub Actions Failure

akolarkunnu reviewed Jul 30, 2025

View reviewed changes

feat: add test case for jsonparseexception

bc364c4

Signed-off-by: Pavan Yekbote <pybot@amazon.com>

pyek-bot temporarily deployed to ml-commons-cicd-env-require-approval July 30, 2025 19:30 — with GitHub Actions Inactive

pyek-bot temporarily deployed to ml-commons-cicd-env-require-approval July 30, 2025 20:58 — with GitHub Actions Inactive

akolarkunnu approved these changes Jul 31, 2025

View reviewed changes

dhrubo-os approved these changes Jul 31, 2025

View reviewed changes

ylwu-amzn approved these changes Jul 31, 2025

View reviewed changes

dhrubo-os merged commit 943ef8c into opensearch-project:main Aug 2, 2025
12 of 13 checks passed

ylwu-amzn added the backport 3.1 label Aug 3, 2025

opensearch-trigger-bot bot mentioned this pull request Aug 3, 2025

[Backport 3.1] [ExceptionHandling] Throw proper 400 errors instead of 500 for agent execute and MCP #4051

Merged

[ExceptionHandling] Throw proper 400 errors instead of 500 for agent execute and MCP #3988

[ExceptionHandling] Throw proper 400 errors instead of 500 for agent execute and MCP #3988

Uh oh!

Conversation

pyek-bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolarkunnu left a comment

Choose a reason for hiding this comment

Uh oh!

dhrubo-os commented Jul 31, 2025

Uh oh!

Uh oh!

pyek-bot commented Aug 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pyek-bot commented Jul 17, 2025 •

edited

Loading

codecov bot commented Jul 17, 2025 •

edited

Loading