Skip to content

Fix agent kind#14263

Merged
tofarr merged 4 commits into
mainfrom
fix-agent-kind
May 1, 2026
Merged

Fix agent kind#14263
tofarr merged 4 commits into
mainfrom
fix-agent-kind

Conversation

@tofarr

@tofarr tofarr commented May 1, 2026

Copy link
Copy Markdown
Collaborator
  • A human has tested these changes.

Why

Some stored data has llm in here instead of openhands. We fix this legacy data. I think this is because of changes in the schema between sdk versions.

 File "storage/org_store.py", line 53, in get_agent_settings_from_org
  File "pydantic/main.py", line 716, in model_validate
agent_kind
  
Input should be 'openhands' [type=literal_error, input_value='llm', input_type=str]
    
For further information visit https://errors.pydantic.dev/2.12/v/literal_error

How to Test

I saw this when testing the slack integration - I think it will haven whenever you start a conversation in the cloud.

Video/Screenshots

image

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change
  • Docs / chore

Notes

openhands-agent and others added 4 commits May 1, 2026 16:57
- Remove broken architecture links in openhands/README.md and enterprise/doc/architecture/README.md
- Update frontend/README.md test file references to existing files
- Update skills/README.md directory structure and repo.md example to actual files
- Update glossary.md to reference skills/ instead of microagents/
- Rewrite app_server/README.md overview to reflect current SDK purpose
- Remove empty Troubleshooting section from frontend/README.md
- Update LLM docs link in Development.md

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  enterprise/storage
  org_store.py 53-57
Project Total  

This report was generated by python-coverage-comment-action

@tofarr tofarr marked this pull request as ready for review May 1, 2026 19:54

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Acceptable - Prevents crashes from legacy data, but approach could be improved.


[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

This bug fix prevents Pydantic validation errors when loading org settings with legacy agent_kind='llm' values. However, it masks the underlying data problem rather than fixing it through a migration. The risk is medium because: (1) database still contains invalid data, (2) no tests added to prevent regression, (3) the workaround could be bypassed if settings are saved elsewhere without this fix.

VERDICT:
Worth merging: Fixes a real production crash, but should be followed up with data cleanup and tests.

KEY INSIGHT:
The fix prevents immediate crashes but leaves technical debt—consider a follow-up migration to clean the stored data and add regression tests for this legacy data path.

kwargs = dict(org.agent_settings)

# Some saved entries have 'llm' in here which is invalid.
kwargs['agent_kind'] = 'openhands'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Important: This masks the underlying data problem rather than fixing it.

Issues:

  • Database still contains invalid agent_kind='llm' values
  • Forces value at read-time instead of cleaning up stored data
  • If org settings are re-saved elsewhere without this fix, the error could return

Suggested improvements:

  1. Add a one-time data migration to update all agent_kind='llm''openhands' in the database
  2. Add a warning log when legacy data is detected:
    if kwargs.get('agent_kind') == 'llm':
        logger.warning(f"Org {org.id} has legacy agent_kind='llm', auto-correcting to 'openhands'")
  3. Document why this workaround exists in a code comment

Why this matters: Data inconsistency between stored and read values can lead to subtle bugs and makes debugging harder.

Comment on lines 52 to +57
def get_agent_settings_from_org(org: Org) -> OpenHandsAgentSettings:
return OpenHandsAgentSettings.model_validate(dict(org.agent_settings))
kwargs = dict(org.agent_settings)

# Some saved entries have 'llm' in here which is invalid.
kwargs['agent_kind'] = 'openhands'
return OpenHandsAgentSettings.model_validate(kwargs)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Add a test for legacy data handling.

No test verifies that orgs with agent_kind='llm' are handled correctly. Add a test like:

@pytest.mark.asyncio
async def test_get_agent_settings_from_org_handles_legacy_llm_agent_kind():
    """Legacy orgs with agent_kind='llm' should be auto-corrected to 'openhands'"""
    # Create org with legacy agent_kind='llm'
    legacy_org = Org(
        name='legacy-org',
        agent_settings={'agent_kind': 'llm', 'agent': 'CodeActAgent'}
    )
    
    # Should not raise ValidationError
    settings = OrgStore.get_agent_settings_from_org(legacy_org)
    
    # Should auto-correct to 'openhands'
    assert settings.agent_kind == 'openhands'

This prevents regression if the workaround is accidentally removed.

@all-hands-bot

Copy link
Copy Markdown
Collaborator

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

@chuckbutkus chuckbutkus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@tofarr tofarr merged commit d9c428b into main May 1, 2026
27 checks passed
@tofarr tofarr deleted the fix-agent-kind branch May 1, 2026 19:59
ChiragArora31 pushed a commit to ChiragArora31/OpenHands that referenced this pull request May 2, 2026
Co-authored-by: openhands <openhands@all-hands.dev>
@mamoodi mamoodi added the release:cloud-1.26.0 PRs included in cloud release 1.26.0 label May 4, 2026 — with OpenHands AI
neubig pushed a commit to OpenHands/software-agent-sdk that referenced this pull request May 5, 2026
The v1.19.0 LLMAgentSettings -> OpenHandsAgentSettings rename changed
the canonical agent_kind discriminator from 'llm' to 'openhands' but
did not bump the persisted-settings schema_version or register a
migration. Persisted rows that still carry the legacy
agent_kind='llm' now break callers that validate against the canonical
OpenHandsAgentSettings class (or that route through the discriminated
union expecting routing to match the deployed code path).

This is an in-band incident triage:
- enterprise/server/routes/org_models.py OrgResponse.from_org and
  OrgDefaultsSettingsResponse.from_org go through AgentSettings
  from_persisted, which already runs schema migrations on load. They
  inherit the fix automatically once this migration ships.
- The point hot-fix in OpenHands/OpenHands#14263 (force
  agent_kind='openhands' in OrgStore.get_agent_settings_from_org)
  becomes redundant on top of this migration.

Changes:
- Bump AGENT_SETTINGS_SCHEMA_VERSION 1 -> 2.
- Register _migrate_agent_settings_v1_to_v2 that rewrites legacy
  agent_kind='llm' to 'openhands'. The two classes are field-
  compatible (LLMAgentSettings only narrows the discriminator
  literal), and LLMAgentSettings is scheduled for removal in v1.22.0,
  so canonicalizing on read is safe and consistent with the
  deprecation direction.
- Update existing migration tests for the new current_version (the
  v1 -> v2 step bumps schema_version on every from_persisted call,
  including the v0 entry-point and the ACP no-op path).
- Add test for the new canonicalization migration.

Co-authored-by: openhands <openhands@all-hands.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:cloud-1.26.0 PRs included in cloud release 1.26.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants