Fix DiscriminatedUnionEnvParser to use single parser when only one kind exists #1741

tofarr · 2026-01-16T02:37:55Z

Summary

This PR modifies the DiscriminatedUnionEnvParser class to automatically use the single available parser when there is exactly one kind defined, without requiring the _KIND environment variable to be set.

Previous behavior: Returns MISSING if _KIND is not set, regardless of how many kinds are available.

New behavior:

0 kinds defined: Returns MISSING
1 kind defined: Automatically uses that kind's parser (no _KIND env var required)
2+ kinds defined: Returns MISSING if _KIND is not set (original behavior preserved)
_KIND explicitly set: Uses the specified kind's parser (always works)

This differs slightly from standard pydantic functionality but provides better UX when there's an unambiguous single option.

Checklist

If the PR is changing/adding functionality, are there tests to reflect this?
If there is an example, have you run the example to make sure that it works?
If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name?
Is the github CI passing?

@tofarr can click here to continue refining the PR

Agent Server images for this PR

• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant	Architectures	Base Image	Docs / Tags
java	amd64, arm64	`eclipse-temurin:17-jdk`	Link
python	amd64, arm64	`nikolaik/python-nodejs:python3.12-nodejs22`	Link
golang	amd64, arm64	`golang:1.21-bookworm`	Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:d5aef3a-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-d5aef3a-python \
  ghcr.io/openhands/agent-server:d5aef3a-python

All tags pushed for this build

ghcr.io/openhands/agent-server:d5aef3a-golang-amd64
ghcr.io/openhands/agent-server:d5aef3a-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:d5aef3a-golang-arm64
ghcr.io/openhands/agent-server:d5aef3a-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:d5aef3a-java-amd64
ghcr.io/openhands/agent-server:d5aef3a-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:d5aef3a-java-arm64
ghcr.io/openhands/agent-server:d5aef3a-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:d5aef3a-python-amd64
ghcr.io/openhands/agent-server:d5aef3a-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:d5aef3a-python-arm64
ghcr.io/openhands/agent-server:d5aef3a-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:d5aef3a-golang
ghcr.io/openhands/agent-server:d5aef3a-java
ghcr.io/openhands/agent-server:d5aef3a-python

About Multi-Architecture Support

Each variant tag (e.g., d5aef3a-python) is a multi-arch manifest supporting both amd64 and arm64
Docker automatically pulls the correct architecture for your platform
Individual architecture tags (e.g., d5aef3a-python-amd64) are also available if needed

…nd exists When there is exactly one kind defined in a DiscriminatedUnionEnvParser, it should automatically use that parser without requiring the _KIND environment variable to be set. This differs slightly from pydantic functionality but provides better UX. Changes: - Modified from_env() to check parser count when KIND is not set - Added 4 comprehensive tests for the new behavior Co-authored-by: openhands <openhands@all-hands.dev>

github-actions · 2026-01-16T02:39:29Z

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-agent-server/openhands/agent_server
env_parser.py	319	195	38%	44–46, 51–53, 56, 61–63, 68–70, 75–77, 82–86, 89–90, 98–101, 104, 106–107, 109, 121–122, 138–146, 151–161, 167–170, 172, 174, 183, 186–196, 199–200, 202–203, 206, 208–211, 214, 216, 219–222, 225–228, 231–235, 237, 245–249, 252–253, 255–264, 266–271, 279–280, 282–283, 285, 287, 289–290, 292–294, 297–298, 308–309, 312–313, 317–330, 332–333, 335–336, 338, 350, 367–368, 370, 374–376, 380–383, 399–402, 420–434, 436, 451–453, 462–467
TOTAL	15539	7531	51%

all-hands-bot

The PR logic looks good overall, but there are important error handling issues that need to be addressed. See inline comments for details.

all-hands-bot · 2026-01-16T03:42:38Z

openhands-agent-server/openhands/agent_server/env_parser.py

+            else:
+                return MISSING
        assert isinstance(kind, str)
        parser = self.parsers[kind]


🔴 Critical: Missing error handling for invalid KIND values. If a user sets an invalid _KIND (one that doesn't exist in self.parsers), this will raise a KeyError instead of returning MISSING or a meaningful error.

Consider adding validation:

parser = self.parsers.get(kind) if parser is None: return MISSING

Or provide a more descriptive error message if you want to fail explicitly.

all-hands-bot · 2026-01-16T03:42:38Z

openhands-agent-server/openhands/agent_server/env_parser.py

+                kind = next(iter(self.parsers.keys()))
+            else:
+                return MISSING
        assert isinstance(kind, str)


🟠 Important: Using assert for validation is problematic because assertions can be disabled with Python's -O flag, and the error message isn't helpful to users. Consider replacing with proper validation:

if not isinstance(kind, str): return MISSING

all-hands-bot

Additional Feedback

🔴 Critical: `to_env` method has the same KeyError issue

The to_env method at line 294 also has the same KeyError vulnerability:

def to_env(self, key: str, value: Any, output: IO):
    parser = self.parsers[value.kind]  # Can raise KeyError
    parser.to_env(key, value, output)

This should also be protected with proper error handling.

🟡 Suggestion: Test Coverage

Consider adding these additional test cases:

Test invalid KIND value: Verify behavior when _KIND is set to a non-existent kind
Test single kind with explicit KIND: Ensure backward compatibility when there's one kind but _KIND is explicitly set
Test to_env with invalid kind: Verify that to_env handles invalid kinds gracefully

These would help ensure robust error handling across all code paths.

openhands-ai · 2026-01-16T03:46:38Z

Looks like there are a few issues preventing this PR from being merged!

GitHub Actions are failing:
- Agent Server

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #1741 at branch `fix-discriminated-union-single-kind`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}

aaron-seq

The PR successfully implements the core feature of automatically using a single parser when only one kind exists, which improves UX. The test coverage is solid with tests for single kind, multiple kinds, and explicit KIND scenarios.

However, there are critical error handling issues that must be addressed before merging:

Line 287: Missing validation for invalid KIND values - will raise KeyError instead of returning MISSING
Line 286: Using assert for validation is problematic as it can be disabled with -O flag

These issues were correctly identified by all-hands-bot. Please implement proper error handling using dict.get() or try-except blocks before merging.

aaron-seq · 2026-01-16T10:36:23Z

openhands-agent-server/openhands/agent_server/env_parser.py

        if kind is MISSING:
-            return MISSING
+            # If there is exactly one kind, use it directly
+            if len(self.parsers) == 1:


The logic here correctly addresses the issue. When there's exactly one parser, using it directly without requiring the _KIND env var improves user experience. However, consider adding validation for edge cases mentioned by all-hands-bot below.

aaron-seq · 2026-01-16T10:37:16Z

tests/agent_server/test_env_parser.py

+# ============================================================================
+
+
+def test_discriminated_union_single_kind_uses_parser_directly(clean_env):


Good test coverage. The test correctly validates that a single kind parser works without requiring the _KIND environment variable. This test confirms the PR's objective is met.

tofarr · 2026-01-16T14:36:32Z

Line 287: Missing validation for invalid KIND values - will raise KeyError instead of returning MISSING

Line 286: Using assert for validation is problematic as it can be disabled with -O flag

These are issues that have been raised in previous code reviews.

We should not want to return MISSING if the kind is invalid - we want an error. (Since if you have a typo in your environment variable value, you would want to know about that, rather than have the system act as if you never specified it - which could make such issues much more difficult to debug)
The asserts here are actually sanity checks designed to placate the linter. They are not required - only confirm what should already be true.

tofarr · 2026-01-16T14:38:15Z

@OpenHands Please review aaron-seq's comment and tofarr's response. Are there simple changes we can make for clarity which would mean that agents will stop flagging this?

openhands-ai · 2026-01-16T14:38:25Z

I'm on it! tofarr can track my progress at all-hands.dev

Replace assert statements with cast() for type narrowing and add explicit comments explaining the intentional behavior: - Use cast(str, kind) instead of assert isinstance(kind, str) for type narrowing after the if-block guarantees kind is a string - Use cast(dict, parser_result) instead of assert isinstance for type narrowing of discriminated union parser results - Add comment explaining that KeyError for invalid KIND values is intentional - typos in env vars should be caught early rather than silently returning MISSING These changes make the code's intent clearer to both human reviewers and automated code analysis tools.

openhands-ai · 2026-01-16T14:41:46Z

I've reviewed aaron-seq's comment and tofarr's response, and made simple changes to improve code clarity so that automated agents will stop flagging these patterns.

Changes Made

The automated agent (all-hands-bot) flagged two issues:

Using assert for validation (can be disabled with -O flag)
Missing error handling for invalid KIND values (raises KeyError)

As tofarr explained, both behaviors are intentional:

The asserts were just for type narrowing/linter satisfaction
KeyError for invalid KIND is correct - typos should fail early, not silently return MISSING

Solution

I replaced the assert statements with cast() calls and added explicit comments explaining the intentional behavior:

# Type narrowing: kind is str here (from os.environ.get or dict keys)
kind = cast(str, kind)
# Intentionally raise KeyError for invalid KIND - typos should fail early
parser = self.parsers[kind]
parser_result = parser.from_env(key)
# Type narrowing: discriminated union parsers always return dicts
parser_result = cast(dict, parser_result)

This approach:

✅ Removes the assert statements that agents flag as problematic
✅ Uses cast() for proper type narrowing (already imported in the file)
✅ Adds clear comments explaining that KeyError is intentional behavior
✅ Preserves the exact same runtime behavior
✅ All tests pass

The changes have been pushed to the fix-discriminated-union-single-kind branch and will update PR #1741.

View full conversation

tofarr added 3 commits January 15, 2026 20:28

Lint fixes

4473942

Lint fixes

312d573

Merge branch 'main' into fix-discriminated-union-single-kind

eac7217

tofarr marked this pull request as ready for review January 16, 2026 03:39

all-hands-bot reviewed Jan 16, 2026

View reviewed changes

aaron-seq suggested changes Jan 16, 2026

View reviewed changes

Merge branch 'main' into fix-discriminated-union-single-kind

d9c1691

tofarr requested a review from aaron-seq January 16, 2026 14:46

tofarr added 2 commits January 16, 2026 08:42

Merge branch 'main' into fix-discriminated-union-single-kind

b44b022

Merge branch 'main' into fix-discriminated-union-single-kind

34b9231

		# ============================================================================


		def test_discriminated_union_single_kind_uses_parser_directly(clean_env):

Fix DiscriminatedUnionEnvParser to use single parser when only one kind exists #1741

Are you sure you want to change the base?

Fix DiscriminatedUnionEnvParser to use single parser when only one kind exists #1741

Uh oh!

Conversation

tofarr commented Jan 16, 2026 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

all-hands-bot left a comment

Choose a reason for hiding this comment

Uh oh!

all-hands-bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

all-hands-bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

all-hands-bot left a comment

Choose a reason for hiding this comment

Additional Feedback

🔴 Critical: to_env method has the same KeyError issue

🟡 Suggestion: Test Coverage

Uh oh!

openhands-ai bot commented Jan 16, 2026

Uh oh!

aaron-seq left a comment

Choose a reason for hiding this comment

Uh oh!

aaron-seq Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

aaron-seq Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

tofarr commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofarr commented Jan 16, 2026

Uh oh!

openhands-ai bot commented Jan 16, 2026

Uh oh!

openhands-ai bot commented Jan 16, 2026

Changes Made

Solution

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tofarr commented Jan 16, 2026 •

edited by github-actions bot

Loading

github-actions bot commented Jan 16, 2026 •

edited

Loading

🔴 Critical: `to_env` method has the same KeyError issue

tofarr commented Jan 16, 2026 •

edited

Loading