Skip to content

Fix #2096: [Bug] hermes adapter broken on main: MemosHttpClient is imported but not defined#2097

Open
Memtensor-AI wants to merge 3 commits into
MemTensor:dev-v2.0.24from
Memtensor-AI:bugfix/autodev-2096-20260710041928689
Open

Fix #2096: [Bug] hermes adapter broken on main: MemosHttpClient is imported but not defined#2097
Memtensor-AI wants to merge 3 commits into
MemTensor:dev-v2.0.24from
Memtensor-AI:bugfix/autodev-2096-20260710041928689

Conversation

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

Description

Fixes #2096. The hermes adapter at apps/memos-local-plugin/adapters/hermes/memos_provider/__init__.py was importing MemosHttpClient from bridge_client, but the class was never committed — every plugin python test failed at collection with ImportError and any real Hermes host loading memos_provider hit the same failure at module load. The offending references arrived via the pr/may27-fixes merge (0398e0e) as a half-committed HTTP-bridge feature.

Chose the "revert usages" option from the issue's two suggested fixes because writing a real MemosHttpClient (subprocess lifecycle, keep-alive, viewer probe, host-handler reverse channel) is a feature commit not a bug hotfix, and a stub raising NotImplementedError would spam warning logs on every reconnect since _connect_http_bridge catches all exceptions and falls back to stdio. Removed the MemosHttpClient import, the type union on self._bridge, the _connect_http_bridge helper, and the HTTP-first branches in initialize and _reconnect_bridge. Also removed the now-unused probe_viewer_status / startup_lock_active imports (their definitions in daemon_manager.py are untouched for a future HTTP PR). Net diff: 31 additions / 54 deletions across the adapter plus the new regression test.

Added test_module_imports_cleanly to tests/python/test_hermes_provider_pipeline.py asserting MemTensorProvider / MemosBridgeClient / BridgeError are on the module surface and MemosHttpClient is not — this way a future half-merge fails with a targeted message instead of a cascade of unrelated collection errors.

Verification: python3 -m unittest discover -s apps/memos-local-plugin/tests/python runs 78 tests, all OK (was: collection ImportError). ruff check + ruff format --check clean on both modified files. opsp artifacts (task.md, proposal.md, spec.md, design.md, verification-report.md) archived to memos-autodev-specs main. Reviewers @whipser030 and @hijzy to be added by scheduler.

Related Issue (Required): Fixes #2096

Type of change

Please delete options that are not relevant.

  • 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 not work as expected)
  • Refactor (does not change functionality, e.g. code style improvements, linting)
  • Documentation update

How Has This Been Tested?

Automated tests are pending.

  • Unit Test
  • Test Script Or Test Steps (please provide)
  • Pipeline Automated API Test (please provide)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have created related documentation issue/PR in MemOS-Docs (if applicable)
  • I have linked the issue to this PR (if applicable)
  • I have mentioned the person who will review this PR

@whipser030, @hijzy please review this PR.

Reviewer Checklist

…emTensor#2096)

The hermes adapter's __init__.py imports MemosHttpClient from
bridge_client, but the class was never committed — every plugin
python test fails at collection and any real Hermes host loading
memos_provider hits ImportError at module load.

Reverting the four usages and the two HTTP-first branches in
initialize/_reconnect_bridge restores the adapter to its
pre-half-merge stdio-only behavior. A dedicated
test_module_imports_cleanly regression test asserts the module
surface (MemTensorProvider / MemosBridgeClient / BridgeError
present, MemosHttpClient absent) so this class of import-level
breakage cannot silently return.

Fixes MemTensor#2096
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2097
Task: bbe9a1b0b80a6c1f
Base: main
Head: bugfix/autodev-2096-20260710041928689
Head SHA: 289eb84501a761958fd8723707493ccd61154e50

🔍 OpenCodeReview found 2 issue(s) in this PR.


1. apps/memos-local-plugin/tests/python/test_hermes_provider_pipeline.py (L83-L84)

These two hasattr assertions are testing implementation-leak details rather than the public contract. MemosBridgeClient and BridgeError appear on the memos_provider module namespace only because __init__.py uses a bare from bridge_client import BridgeError, MemosBridgeClient, which incidentally pollutes the package namespace. They are not part of memos_provider's public API.

This makes the test brittle: any refactor that changes the import style inside __init__.py (e.g., import bridge_client then using bridge_client.MemosBridgeClient) would make these assertions fail even though the module is perfectly correct. It also gives a false guarantee — passing these assertions does not mean the import itself works (that is already proven by import memos_provider at the top of the file succeeding at collection time, as the docstring itself explains).

Consider replacing these two assertions with a direct check on the actual import:

import importlib
bc = importlib.import_module("bridge_client")
self.assertTrue(hasattr(bc, "MemosBridgeClient"))
self.assertTrue(hasattr(bc, "BridgeError"))

Or, if the intent is specifically to guard that memos_provider patches itself (i.e., other Hermes host code reads these names through memos_provider), add an explicit __all__ to __init__.py that exports them, then test against that contract.


2. apps/memos-local-plugin/tests/python/test_hermes_provider_pipeline.py (L71-L73)

The docstring claims the regression is already caught at collection time — "if any dangling reference is reintroduced, import memos_provider raises ImportError and the whole file fails to collect" — which is true and sufficient. However, test_module_imports_cleanly is placed as the very first test in the suite, so a collection-level failure from a bad import would already prevent every other test from running, making this test body redundant for that specific claim. The test's real value is the assertFalse(hasattr(..., "MemosHttpClient")) regression guard, not the assertTrue assertions above. Consider updating the docstring to clarify what uniquely this test body proves (i.e., the negative MemosHttpClient guard), so future readers don't mistake the positive assertions for meaningful coverage.

💡 Suggested Change

Before:

    def test_module_imports_cleanly(self) -> None:
        """Regression for #2096: ``memos_provider`` must import without
        pulling in symbols that don't exist in ``bridge_client``.

After:

    def test_module_imports_cleanly(self) -> None:
        """Regression guard for #2096: asserts that ``MemosHttpClient`` is NOT
        present in ``memos_provider``, since the class was referenced before
        it was ever committed (see issue #2096).

        Note: the import itself is already validated at collection time —
        the ``import memos_provider`` at the top of this file will raise
        ``ImportError`` if a dangling reference is reintroduced, causing the
        entire test file to fail to load. This test body only adds the
        explicit negative guard below.

Generated by cloud-assistant via Open Code Review.

Address two Open Code Review findings on the
test_module_imports_cleanly regression test added in MemTensor#2097:

* The positive hasattr assertions on memos_provider.MemosBridgeClient
  and memos_provider.BridgeError were locking in an implementation
  detail — those names only appear on the memos_provider namespace
  because __init__.py uses a bare 'from bridge_client import ...'. If
  __init__.py ever switched to 'import bridge_client', the test would
  fail even though the module is correct. Assert the real contract on
  bridge_client directly via importlib.

* The docstring claimed a role the test body does not uniquely fill:
  since test_module_imports_cleanly is the first test in the suite, a
  collection-level ImportError would already prevent every test from
  running. Rewrite the docstring to spell out that the only unique
  guard the body provides is the negative assertion that
  MemosHttpClient is absent (plus a positive MemTensorProvider check
  for the class Hermes actually instantiates).

Refs MemTensor#2096.
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2097
Task: bbe9a1b0b80a6c1f
Base: main
Head: bugfix/autodev-2096-20260710041928689
Head SHA: 6272765b80384817162c88d3ff13cca85fee6d01

🔍 OpenCodeReview found 1 issue(s) in this PR.


1. apps/memos-local-plugin/tests/python/test_hermes_provider_pipeline.py (L106)

The assertFalse(hasattr(memos_provider, "MemosHttpClient")) check only detects MemosHttpClient being re-exported through the memos_provider package namespace. If MemosHttpClient is added back to bridge_client but not re-exported (e.g. imported with an alias, or used internally without from bridge_client import MemosHttpClient), this assertion would pass — and the half-merged feature could silently re-enter the codebase.

To make the regression guard complete, also assert absence on the bc (bridge_client) module itself:

self.assertFalse(hasattr(bc, "MemosHttpClient"))

This ensures the class cannot land in bridge_client at all until it is intentionally committed as a complete feature.

💡 Suggested Change

Before:

        self.assertFalse(hasattr(memos_provider, "MemosHttpClient"))

After:

        self.assertFalse(hasattr(memos_provider, "MemosHttpClient"))
        self.assertFalse(hasattr(bc, "MemosHttpClient"))

Generated by cloud-assistant via Open Code Review.

…e_client

Round 2 OCR fix on PR MemTensor#2097. The prior negative guard only checked
memos_provider — but memos_provider only sees MemosHttpClient through
a bare 'from bridge_client import ...' in __init__.py. If the class
were re-added to bridge_client without a matching re-export (e.g. an
aliased import, or used internally), the assertion would still pass
and the half-merged feature could silently re-enter the codebase.

Add the complementary assertFalse(hasattr(bc, 'MemosHttpClient')) so
the class cannot land in bridge_client at all until it is intentionally
committed as a complete feature. Update the docstring to note the
guard now covers both surfaces.

Refs MemTensor#2096.
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2097
Task: bbe9a1b0b80a6c1f
Base: main
Head: bugfix/autodev-2096-20260710041928689
Head SHA: 4e7f07c00960c064da3e2cecad4d5ad7b0b49f2e

OpenCodeReview: No comments generated. Looks good to me.

Generated by cloud-assistant via Open Code Review.

@CarltonXiang CarltonXiang changed the base branch from main to dev-v2.0.24 July 10, 2026 06:10
@fancyboi999

Copy link
Copy Markdown
Contributor

Verified locally as the reporter of #2096:

  • On the merge-base (13fbd437), importing the provider fails with exactly the reported error: ImportError: cannot import name 'MemosHttpClient' from 'bridge_client'.
  • On this branch (4e7f07c0), the full plugin suite passes: 78 passed in apps/memos-local-plugin/tests/python/ (pytest + pyyaml).

The revert removes the phantom import together with every code path that referenced it (_connect_http_bridge, the HTTP branches in initialize / _reconnect_bridge), and the new import-cleanliness tests guard both memos_provider and bridge_client against regressing. Since these plugin tests don't run in CI, posting the local run here for the record.

One sequencing note for maintainers: #2095 touches the same two files (memos_provider/__init__.py, test_hermes_provider_pipeline.py). Whichever lands second needs a rebase — I'm happy to rebase #2095 once this merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] hermes adapter broken on main: MemosHttpClient is imported but not defined anywhere

3 participants