Skip to content

fix: support freeform stopReason in sampling #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LucaButBoring
Copy link
Contributor

@LucaButBoring LucaButBoring commented Jun 19, 2025

Supports freeform stop reasons in sampling responses by mapping them to a new UNKNOWN value.

Motivation and Context

Previously, sampling response parsing would fail if a stopReason was provided besides endTurn/stopSequence/maxTokens. This fixes that to map the arbitrary values to UNKNOWN instead.

Spec: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/f5ccad944fdf2b7d9cc70cf817f66ca5a8aa03a4/schema/2024-11-05/schema.ts#L807

How Has This Been Tested?

Unit tests

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Closes #328

Previously, sampling response parsing would fail if a stopReason
was provided besides endTurn/stopSequence/maxTokens. This fixes
that to map the arbitrary values to UNKNOWN instead.

Spec: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/f5ccad944fdf2b7d9cc70cf817f66ca5a8aa03a4/schema/2024-11-05/schema.ts#L807
Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LucaButBoring, thanks for addressing this.
The UNKNOWN approach masks the original stop reason. I'm not sure how important the exact stop-reason is for downstream applications?
A better approach would be relaxing the stop reason type to String, but this would be a breaking change. What do you think?

@LucaButBoring
Copy link
Contributor Author

The UNKNOWN approach masks the original stop reason. I'm not sure how important the exact stop-reason is for downstream applications?
A better approach would be relaxing the stop reason type to String, but this would be a breaking change. What do you think?

Yes, I was thinking about this - the reason I added UNKNOWN was to avoid needing to do this. I think there are better ways to model StopReason in general, but didn't want to make a breaking change without a good reason to do so. Also, I did want to keep the well-modeled known stop reasons, though it's not strictly necessary to do so. Given that we can't really have a union of enums and a string like TS/Python, I'd probably want to lean towards a Kotlin-esque approach and do this if we accepted a breaking change:

public sealed class StopReason permits EndTurn, StopSequence, MaxTokens, Unknown {
    private String value;

    public StopReason(String value) {
        this.value = value;
    }

    public class EndTurn : StopReason {
        public EndTurn() {
            super("endTurn");
        }
    }

    public class StopSequence : StopReason {
        public EndTurn() {
            super("stopSequence");
        }
    }

    public class MaxTokens : StopReason {
        public MaxTokens() {
            super("maxTokens");
        }
    }

    public class Unknown : StopReason {
        public Unknown(String value) {
            super(value);
        }

        public String getValue() {
            return value;
        }
    }
}

That way consumers could handle it like so:

if (stopReason instanceof EndTurn) {
    // ...
} /* other cases */ else if (stopReason instanceof Unknown unknownStopReason) {
    log.info("Sampling stopped due to unhandled stop reason: {}", unknownStopReason)
}

I'll defer to you, though - if we're willing to make a breaking change for this and prefer that it just become a String, we can do that, instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Freeform stopReasons in sampling responses are unsupported
2 participants