feat(marketplace): expose load_plugin on base conversation

[csmith49]

Summary

Follow-up chunk stacked on #3829.

• Add BaseConversation.load_plugin(...) to the shared conversation API surface.
• Keep the base implementation non-abstract and unsupported by default so existing custom BaseConversation subclasses are not forced to implement a new abstract method.
• Add a focused test for the default unsupported behavior.

Why

Adding load_plugin(...) to the shared conversation API gives local and remote conversations a consistent extension point without breaking existing custom conversation subclasses.

How to Test

uv run pre-commit run --files openhands-sdk/openhands/sdk/conversation/base.py tests/sdk/conversation/test_base_span_management.py
uv run pytest tests/sdk/conversation/test_base_span_management.py::test_base_conversation_load_plugin_default_not_supported -q

This PR was created by an AI agent (OpenHands) on behalf of the user.

@csmith49 can click here to continue refining the PR

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-sdk/openhands/sdk/conversation
base.py	101	2	98%	220, 274
TOTAL	32845	9107	72%

[all-hands-bot]

✅ QA Report: PASS

The SDK behavior was exercised before/after and the PR exposes BaseConversation.load_plugin(...) without forcing custom subclasses to implement it.

Does this PR achieve its stated goal?

Yes. On the base branch, a user-created BaseConversation subclass has no load_plugin API at all; on the PR branch, the same user-style subclass instantiates without implementing load_plugin, reports load_plugin as non-abstract, and calling it raises the documented unsupported NotImplementedError. This verifies both stated goals: shared API exposure and backward-compatible default behavior for existing subclasses.

Phase	Result
Environment Setup	✅ make build completed and installed the SDK workspace packages
CI Status	⚠️ PR Description Check failing; sdk-tests/QA still in progress when checked; other visible checks mostly green
Functional Verification	✅ Before/after SDK script confirmed the new base API and default unsupported behavior

Functional Verification

Test 1: Base conversation API exposure and default unsupported behavior

Step 1 — Establish baseline without the fix:
Checked out origin/feat/marketplace-registered-example and ran a short SDK script that imports BaseConversation, dynamically creates a minimal custom subclass implementing only the existing abstract methods, instantiates it, and attempts to use load_plugin if present:

branch: origin/feat/marketplace-registered-example
has_load_plugin: False
load_plugin_is_abstract: False
custom_subclass_abstract_methods: []
instantiated_custom_subclass: QAConversation
load_plugin_result: Attribute missing from BaseConversation API

This shows the baseline problem for the PR goal: BaseConversation did not provide a shared load_plugin API surface.

Step 2 — Apply the PR's changes:
Checked out feat/marketplace-base-load-plugin-api at 60b951eee8b1e6ef0054696e62d6efa881502518.

Step 3 — Re-run with the fix in place:
Ran the same SDK script against the PR branch:

branch: feat/marketplace-base-load-plugin-api
has_load_plugin: True
load_plugin_is_abstract: False
custom_subclass_abstract_methods: []
instantiated_custom_subclass: QAConversation
load_plugin_result: NotImplementedError This conversation does not support loading plugins

This confirms the new method is present on BaseConversation, is not abstract, does not prevent an existing-style custom subclass from instantiating, and fails with the intended default unsupported behavior when called.

Issues Found

None functionally.

This QA review was created by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🟢 Good taste — Elegant, minimal, backward-compatible API extension.

This PR does exactly what it says: adds load_plugin to the BaseConversation API surface using a non-abstract default that raises NotImplementedError, so existing subclasses are unaffected. The design is correct, the docstring is informative for implementors, and the test exercises the real code path with a proper assertion on the raised exception.

Optional improvement (not a blocker):

• [base.py, line 392] The error message "This conversation does not support loading plugins" would be marginally more debuggable as f"{type(self).__name__} does not support loading plugins" — callers would immediately see which concrete class was missing the implementation without needing a stack trace. The existing test uses a substring match so it would still pass.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Additive, non-breaking change. The method is not abstract, so no existing subclass is forced to implement it. The only risk surface is the new API name becoming a permanent commitment — but load_plugin(plugin_ref: str) -> None is a straightforward, well-named addition.

VERDICT:
✅ Worth merging — Clean, purposeful, and properly tested.

KEY INSIGHT:
Choosing a non-abstract default NotImplementedError over @abstractmethod is the right call here: it grows the API surface without fracturing backward compatibility.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

⚠️ QA Report: PASS WITH ISSUES

The PR's functional goal was verified: BaseConversation.load_plugin(...) is now present on the shared SDK base API, remains non-abstract, and defaults to a clear unsupported error for subclasses that do not implement plugin loading.

Does this PR achieve its stated goal?

Yes. I exercised the SDK as a library user by defining a custom BaseConversation subclass that does not implement load_plugin(): on the base branch the method is absent and calling it fails with AttributeError; on the PR branch the same subclass still instantiates, exposes load_plugin, and calling it raises NotImplementedError: This conversation does not support loading plugins. That confirms both stated goals: the API is exposed on the base conversation surface and existing subclasses are not forced to implement a new abstract method.

Phase	Result
Environment Setup	✅ make build completed successfully and installed the editable SDK environment.
CI Status	⚠️ Existing GitHub checks show PR Description Check failing and qa-changes in progress; queried functional/quality checks otherwise reported success.
Functional Verification	✅ Before/after SDK script confirmed the new base API behavior and default unsupported behavior.

Functional Verification

Test 1: BaseConversation.load_plugin is exposed without becoming abstract

Step 1 — Establish baseline without the PR:

Ran:

git checkout --detach origin/feat/marketplace-registered-example
OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY'
# Defined a MarketplaceSubclass implementing the existing BaseConversation abstract API,
# then instantiated it, checked hasattr(conversation, "load_plugin"), and called it.
PY

Observed:

cac26566 docs(examples): show registered marketplace plugins
abstract_methods= ['ask_agent', 'close', 'condense', 'conversation_stats', 'execute_tool', 'fork', 'generate_title', 'id', 'pause', 'reject_pending_actions', 'run', 'send_message', 'set_confirmation_policy', 'set_security_analyzer', 'state', 'update_secrets']
instantiated= MarketplaceSubclass
has_load_plugin= False
load_plugin_result= AttributeError 'MarketplaceSubclass' object has no attribute 'load_plugin'

This establishes the pre-PR behavior: a BaseConversation subclass can instantiate, but the shared base API does not expose load_plugin.

Step 2 — Apply the PR's changes:

Checked out the PR branch:

git checkout feat/marketplace-base-load-plugin-api

Step 3 — Re-run with the PR branch:

Ran the same inline SDK usage script:

OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY'
# Same MarketplaceSubclass: no load_plugin override, instantiate, check hasattr, call load_plugin.
PY

Observed:

60b951ee feat(marketplace): expose load_plugin on base conversation
abstract_methods= ['ask_agent', 'close', 'condense', 'conversation_stats', 'execute_tool', 'fork', 'generate_title', 'id', 'pause', 'reject_pending_actions', 'run', 'send_message', 'set_confirmation_policy', 'set_security_analyzer', 'state', 'update_secrets']
instantiated= MarketplaceSubclass
has_load_plugin= True
load_plugin_result= NotImplementedError This conversation does not support loading plugins

This confirms the changed behavior: load_plugin is available from the base conversation API, it is not abstract because the existing-style subclass still instantiates, and the default implementation fails with the documented unsupported behavior.

Issues Found

• 🟡 Process: Existing CI shows PR Description Check failing. Functional QA found no behavior issue, but this CI failure likely needs human follow-up if it remains required.

This review was created by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🟢 Good taste — Minimal, well-scoped change that correctly widens the BaseConversation API surface without breaking existing subclasses.

Summary

This PR adds load_plugin(plugin_ref: str) -> None to BaseConversation as a non-abstract method whose default implementation raises NotImplementedError. This is the right design call:

• Making it non-abstract preserves backward compatibility — existing custom BaseConversation subclasses (including third-party ones) are not forced to add a new method.
• The pattern is consistent with the rest of the codebase: both LocalConversation and RemoteConversation already have concrete load_plugin implementations; this PR simply formalises the contract on the shared base class.
• The docstring accurately describes both calling conventions (plugin-name and plugin-name@marketplace-name).
• The test is focused and exercises the right thing: the default fallback behaviour, using the existing MockConversation fixture from this file.

One very minor observation: the new test lives in test_base_span_management.py, which is named after a different concern. A future housekeeping pass could move it to a dedicated test_base_conversation.py, but this is purely organisational and does not block merging.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Adds a non-abstract default method to an abstract base class. Cannot break existing subclasses; only affects callers that explicitly invoke load_plugin on a conversation type that does not support it.

VERDICT:
✅ Worth merging — Clean, focused addition with appropriate test coverage.

KEY INSIGHT:
Non-abstract NotImplementedError is the correct backward-compatible pattern for extending an ABC API without forcing all existing subclasses to implement the new method.

---

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. 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.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ QA Report: PASS

Verified the SDK conversation API behavior directly: the PR exposes load_plugin(...) on BaseConversation without forcing existing custom subclasses to implement it.

Does this PR achieve its stated goal?

Yes. On the base branch, an existing BaseConversation subclass had no load_plugin API and calling it failed with AttributeError; on the PR branch, the same subclass still instantiated without implementing load_plugin, exposed the method, and raised the documented default NotImplementedError. This confirms the PR delivers its stated goal of adding a shared, non-abstract, unsupported-by-default base API.

Phase	Result
Environment Setup	✅ make build completed successfully with uv sync --dev and editable package builds.
CI Status	🟡 Observed via gh pr checks: 13 successful, 1 skipped, 1 pending, 0 failing at query time.
Functional Verification	✅ Real SDK import/subclass/call path verified before and after the PR.

Functional Verification

Test 1: BaseConversation exposes a non-abstract default load_plugin API

Step 1 — Reproduce / establish baseline without the fix:
Checked out the PR base branch and ran a small SDK user script that defines an existing custom BaseConversation subclass without implementing load_plugin, instantiates it, and calls conversation.load_plugin("demo-plugin@demo-marketplace").

Ran git checkout --detach origin/feat/marketplace-registered-example && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_base_conversation_load_plugin.py:

HEAD is now at 95591598 docs(examples): show registered marketplace plugins
instantiated ExistingCustomConversation
has_load_plugin False
call_exception AttributeError 'ExistingCustomConversation' object has no attribute 'load_plugin'

This establishes the pre-PR behavior: existing custom conversation subclasses could instantiate, but the shared base conversation API did not expose load_plugin.

Step 2 — Apply the PR's changes:
Checked out the PR head branch at cdc3123c8fcb0c0d7e310fa7558b53c9232d59a1.

Step 3 — Re-run with the fix in place:
Ran git checkout feat/marketplace-base-load-plugin-api && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_base_conversation_load_plugin.py:

Switched to branch 'feat/marketplace-base-load-plugin-api'
cdc3123c feat(marketplace): expose load_plugin on base conversation
instantiated ExistingCustomConversation
has_load_plugin True
call_exception NotImplementedError This conversation does not support loading plugins

This confirms both key claims: load_plugin is now present on the base API, and it is not abstract because the existing custom subclass still instantiates without implementing it. The default behavior is also unsupported with the documented NotImplementedError.

Issues Found

None.

This QA review was created by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🟢 Good taste — Minimal, backward-compatible API surface extension.

This is a small, well-scoped change: BaseConversation.load_plugin(...) is exposed as a non-abstract method with an unsupported default, so existing custom subclasses remain instantiable while LocalConversation and RemoteConversation can provide concrete behavior. The focused regression test exercises that default path.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Additive SDK API change with no dependency, persistence-shape, event-schema, or agent-loop behavior changes. I also ran uv run pytest tests/sdk/conversation/test_base_span_management.py::test_base_conversation_load_plugin_default_not_supported -q successfully.

VERDICT:
✅ Worth merging: Core logic is sound and the change preserves backward compatibility.

KEY INSIGHT:
Keeping the new base method non-abstract is the right API shape: support can be added where available without forcing every existing conversation subclass to change.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[all-hands-bot]

✅ QA Report: PASS

Verified the new SDK conversation API surface behaves as described: BaseConversation.load_plugin(...) now exists, is non-abstract, and defaults to an unsupported error without breaking custom subclasses.

Does this PR achieve its stated goal?

Yes. The goal was to expose load_plugin(...) on BaseConversation while keeping the default implementation non-abstract and unsupported for conversations that do not implement marketplace plugin loading. I exercised the SDK as a library user with a concrete BaseConversation subclass that does not implement load_plugin: before the PR the method was absent and calling it raised AttributeError; at commit a8f478c, the same subclass still instantiates and calling load_plugin("plugin@marketplace") raises the intended NotImplementedError.

Phase	Result
Environment Setup	✅ make build completed successfully and installed the editable SDK packages.
CI Status	🟡 All listed non-QA checks were green at review time; QA Changes by OpenHands / qa-changes was still in progress.
Functional Verification	✅ Before/after SDK script confirmed the new public method and default unsupported behavior.

Functional Verification

Test 1: BaseConversation.load_plugin public API and default behavior

Step 1 — Reproduce / establish baseline without the fix:
Checked out the base branch and ran a real SDK import/call script:

git switch --detach origin/feat/marketplace-registered-example
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_base_conversation_load_plugin.py | grep -E '^(abstractmethods|load_plugin|instantiated)'

Relevant output:

abstractmethods= ['ask_agent', 'close', 'condense', 'conversation_stats', 'execute_tool', 'fork', 'generate_title', 'id', 'pause', 'reject_pending_actions', 'run', 'send_message', 'set_confirmation_policy', 'set_security_analyzer', 'state', 'update_secrets']
load_plugin_on_base= False
load_plugin_is_abstract= False
instantiated_without_load_plugin= True
load_plugin_on_instance= False
load_plugin_exception_type= AttributeError
load_plugin_exception_message= 'MockConversation' object has no attribute 'load_plugin'

This establishes the prior behavior: a user working through the base conversation API could instantiate an existing custom subclass, but load_plugin was not available on the base API surface.

Step 2 — Apply the PR's changes:
Checked out the PR commit:

git switch --detach a8f478ccfabfe047d45bf4286d3a792d9cf69655

Step 3 — Re-run with the fix in place:
Ran the same SDK script against the PR commit:

OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_base_conversation_load_plugin.py | grep -E '^(abstractmethods|load_plugin|instantiated)'

Relevant output:

abstractmethods= ['ask_agent', 'close', 'condense', 'conversation_stats', 'execute_tool', 'fork', 'generate_title', 'id', 'pause', 'reject_pending_actions', 'run', 'send_message', 'set_confirmation_policy', 'set_security_analyzer', 'state', 'update_secrets']
load_plugin_on_base= True
load_plugin_is_abstract= False
instantiated_without_load_plugin= True
load_plugin_on_instance= True
load_plugin_exception_type= NotImplementedError
load_plugin_exception_message= This conversation does not support loading plugins

This confirms the PR delivers the intended behavior: load_plugin is now present on BaseConversation, it is not abstract, existing subclasses are not forced to implement it, and unsupported conversations fail with a clear default NotImplementedError instead of missing the method.

Issues Found

None.

This QA report was created by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

✅ QA Report: PASS

Functional QA verified the new SDK API surface: BaseConversation.load_plugin(...) is now callable, remains non-abstract for existing custom subclasses, and defaults to an unsupported error.

Does this PR achieve its stated goal?

Yes. The PR set out to add BaseConversation.load_plugin(...) while keeping the base implementation non-abstract and unsupported by default. I exercised that exact SDK path with a custom BaseConversation subclass: on the base branch the method was absent and calling it raised AttributeError; on the PR branch the same subclass instantiated without defining load_plugin, the method was present, and calling it raised the intended NotImplementedError.

Phase	Result
Environment Setup	✅ make build completed successfully; dependencies installed and editable packages built.
CI Status	⏳ As of the QA run, pre-commit and most checks were green; sdk-tests and qa-changes were still in progress.
Functional Verification	✅ Base-vs-head SDK script confirmed the API delta and default unsupported behavior.

Functional Verification

Test 1: BaseConversation.load_plugin API exposure and default behavior

Step 1 — Reproduce / establish baseline without the fix:
Checked out feat/marketplace-registered-example and ran a short SDK script that defines a custom BaseConversation subclass implementing all required abstract methods except load_plugin, then attempts to call conversation.load_plugin("demo@marketplace").

Command:

git checkout feat/marketplace-registered-example
uv run python - <<'PY_SCRIPT'
from openhands.sdk.conversation.base import BaseConversation


def make_stub(name: str):
    def stub(self, *args, **kwargs):
        raise NotImplementedError(f"stub {name}")

    return stub


namespace = {
    name: make_stub(name)
    for name in BaseConversation.__abstractmethods__
    if name != "load_plugin"
}
UserConversation = type("UserConversation", (BaseConversation,), namespace)

print(f"BaseConversation has load_plugin: {hasattr(BaseConversation, 'load_plugin')}")
print(
    "BaseConversation requires subclass load_plugin: "
    f"{'load_plugin' in BaseConversation.__abstractmethods__}"
)
conversation = UserConversation()
print("Custom subclass instantiated without defining load_plugin: yes")
try:
    conversation.load_plugin("demo@marketplace")
except Exception as exc:
    print(f"Calling load_plugin raised: {type(exc).__name__}: {exc}")
PY_SCRIPT

Relevant output:

BaseConversation has load_plugin: False
BaseConversation requires subclass load_plugin: False
Custom subclass instantiated without defining load_plugin: yes
Calling load_plugin raised: AttributeError: 'UserConversation' object has no attribute 'load_plugin'

This establishes the prior state: users typing or handling conversations through BaseConversation could not call load_plugin at all.

Step 2 — Apply the PR's changes:
Checked out feat/marketplace-base-load-plugin-api at 7df1f80500ce4f76972b5827965409e706531fce.

Step 3 — Re-run with the fix in place:
Ran the same SDK script on the PR branch.

Relevant output:

7df1f80500ce4f76972b5827965409e706531fce
BaseConversation has load_plugin: True
BaseConversation requires subclass load_plugin: False
Custom subclass instantiated without defining load_plugin: yes
Calling load_plugin raised: NotImplementedError: This conversation does not support loading plugins

This confirms the PR's intended behavior: load_plugin is exposed on the shared base conversation API, does not become a new abstract-method requirement for existing subclasses, and has the documented unsupported default behavior.

Issues Found

None.

This review was created by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

🟢 Good taste — Minimal, focused, backward-compatible.

This PR adds a non-abstract load_plugin default implementation to BaseConversation so code typed against the base class can call the method without casting. Both concrete implementations (LocalConversation, RemoteConversation) already override it. Making it non-abstract is the correct call — an abstract method would have been a silent breaking change for every existing custom BaseConversation subclass.

The docstring clearly documents the expected format (plugin-name or plugin-name@marketplace-name), and the test is meaningful — it instantiates a real MockConversation (not a mock of the unit under test), exercises the default code path, and will catch any regression.

---

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Additive-only change. No behavior change for any existing code. Both concrete implementations already had load_plugin; this formalises the interface on the base class. Existing custom subclasses that don't override it will raise NotImplementedError if called — correct behavior that is clearly documented.

VERDICT:
✅ Worth merging: Clean API surface extension with correct backward-compatibility design.

KEY INSIGHT:
Non-abstract default raising NotImplementedError is the right pattern here — it extends the BaseConversation interface without forcing implementation from all existing subclasses.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation