-
Notifications
You must be signed in to change notification settings - Fork 38
fix(brain): restore agent_types/skill_types compat shims dropped in #542 #621
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # Copyright (c) 2026 Innate Inc | ||
| """Deprecated: moved to brain_client.agents.types. | ||
|
|
||
| Backward-compat shim for custom agents that still import from the old path. | ||
| Remove in a future release once external agent files have migrated. | ||
| """ | ||
|
|
||
| from brain_client.agents.types import * # noqa: F401, F403 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # Copyright (c) 2026 Innate Inc | ||
| """Deprecated: moved to brain_client.skills.types. | ||
|
|
||
| Backward-compat shim for custom skills that still import from the old path. | ||
| Remove in a future release once external skill files have migrated. | ||
| """ | ||
|
|
||
| from brain_client.skills.types import * # noqa: F401, F403 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
105 changes: 105 additions & 0 deletions
105
ros2_ws/src/brain/brain_client/test/test_compat_shims.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # Copyright (c) 2026 Innate Inc | ||
| """The pre-#542 import paths are load-bearing: custom skills and agents on | ||
| robots in the field import ``brain_client.skill_types`` and | ||
| ``brain_client.agent_types``, so the shims must keep resolving and re-export | ||
| the *same* class objects — registration via ``__init_subclass__`` and every | ||
| isinstance check depend on identity, not just equal names. | ||
|
|
||
| The agent-side end-to-end regression (legacy path → roster) lives in | ||
| test_agent_loading.py; the skill-side one is here, through the same | ||
| discovery functions the catalog calls (``_load_code_skills``).""" | ||
|
|
||
| import importlib | ||
| import logging | ||
| import sys | ||
| import textwrap | ||
|
|
||
| import pytest | ||
|
|
||
| from brain_client import agent_types, skill_types | ||
| from brain_client.agents import types as agents_types | ||
| from brain_client.skills import types as skills_types | ||
| from brain_client.skills.workspace_import import import_workspace_packages, registered_workspace_skills | ||
|
|
||
| LOGGER = logging.getLogger("test_compat_shims") | ||
|
|
||
|
|
||
| def test_agent_types_shim_reexports_same_objects(): | ||
| for name in ("Agent", "SkillRef", "InputRef"): | ||
| assert getattr(agent_types, name) is getattr(agents_types, name) | ||
|
|
||
|
|
||
| def test_skill_types_shim_reexports_same_objects(): | ||
| for name in ( | ||
| "Skill", | ||
| "SkillResult", | ||
| "SkillOutput", | ||
| "SkillCancelled", | ||
| "RobotState", | ||
| "RobotStateType", | ||
| "Interface", | ||
| "InterfaceType", | ||
| ): | ||
| assert getattr(skill_types, name) is getattr(skills_types, name) | ||
|
|
||
|
|
||
| # A skill exactly as robots in the field author them pre-#542: old import | ||
| # path, class-attribute Interface/RobotState declarations, tuple return. | ||
| LEGACY_SKILL_SRC = textwrap.dedent( | ||
| ''' | ||
| from brain_client.skill_types import ( | ||
| Interface, | ||
| InterfaceType, | ||
| RobotState, | ||
| RobotStateType, | ||
| Skill, | ||
| SkillResult, | ||
| ) | ||
|
|
||
|
|
||
| class LegacyProbe(Skill): | ||
| """Probe the head, legacy style.""" | ||
|
|
||
| head = Interface(InterfaceType.HEAD) | ||
| odom = RobotState(RobotStateType.LAST_ODOM) | ||
|
|
||
| def execute(self): | ||
| return "ok", SkillResult.SUCCESS | ||
| ''' | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def workspace(tmp_path, monkeypatch): | ||
| """A synthetic $INNATE_OS_ROOT, with the process-global state discovery | ||
| touches (sys.path, sys.modules, Skill._registry) snapshotted and restored | ||
| — same isolation model as test_agent_loading's fixture.""" | ||
| (tmp_path / "workspace" / "custom_skills").mkdir(parents=True) | ||
| monkeypatch.setenv("INNATE_OS_ROOT", str(tmp_path)) | ||
|
|
||
| saved_path = list(sys.path) | ||
| saved_modules = set(sys.modules) | ||
| saved_registry = dict(skills_types.Skill._registry) | ||
| # Cleared, not just snapshotted: other test modules register Skills (some | ||
| # deliberately broken), and discovery both rosters and *prunes* whatever | ||
| # the live registry holds — the test must see only this workspace's. | ||
| skills_types.Skill._registry.clear() | ||
| yield tmp_path / "workspace" | ||
| for name in set(sys.modules) - saved_modules: | ||
| sys.modules.pop(name, None) | ||
| sys.path[:] = saved_path | ||
| skills_types.Skill._registry.clear() | ||
| skills_types.Skill._registry.update(saved_registry) | ||
|
|
||
|
|
||
| def test_legacy_skill_loads_through_workspace_discovery(workspace): | ||
| (workspace / "custom_skills" / "legacy_probe.py").write_text(LEGACY_SKILL_SRC) | ||
| importlib.invalidate_caches() | ||
|
|
||
| errors = import_workspace_packages(LOGGER) | ||
| skills, broken = registered_workspace_skills(LOGGER) | ||
|
|
||
| assert errors == {} | ||
| assert broken == {} | ||
| assert "local/legacy_probe" in skills | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test checks the skill shim only through direct imports, while legacy custom skills use the workspace discovery and registration path. Adding a dynamic-loading regression like the agent-side test would prevent loader-specific compatibility regressions from passing these identity assertions unnoticed.
Knowledge Base Used: brain_client: agent/skill runtime and ROS bridge
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — added in 6f2bb4d: a legacy-authored skill (old
brain_client.skill_typesimport, class-attributeInterface/RobotStatedeclarations, tuple return) now loads end-to-end throughimport_workspace_packages+registered_workspace_skills, the same discovery functions_load_code_skillscalls. The fixture clearsSkill._registryso discovery sees only the synthetic workspace — other test modules register Skills (some deliberately broken), and discovery both rosters and prunes whatever the live registry holds.