From 5cb46eb727ea7270e89914c41134889f6ad2288f Mon Sep 17 00:00:00 2001 From: nankingjing <1079826437@qq.com> Date: Sat, 11 Jul 2026 19:52:22 +0800 Subject: [PATCH 1/4] fix(hooks): route OpenVikingPostCallHook through sync path so return values survive --- .../hooks/builtins/openviking_hooks.py | 9 ++-- .../tests/unit/test_hooks_sync_routing.py | 51 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 bot/vikingbot/tests/unit/test_hooks_sync_routing.py diff --git a/bot/vikingbot/hooks/builtins/openviking_hooks.py b/bot/vikingbot/hooks/builtins/openviking_hooks.py index 261eb64f42..a0158fad19 100644 --- a/bot/vikingbot/hooks/builtins/openviking_hooks.py +++ b/bot/vikingbot/hooks/builtins/openviking_hooks.py @@ -256,10 +256,11 @@ async def execute(self, context: HookContext, **kwargs) -> Any: class OpenVikingPostCallHook(Hook): name = "openviking_post_call" - # Hook execute() is genuinely async (it awaits ov_client search/read). Mark it - # async so the hook manager routes it through asyncio.gather with other async - # hooks instead of the sequential sync_hooks path. - is_sync = False + # Hook execute() is genuinely async (it awaits ov_client search/read), but it + # MUST return its mutated kwargs dict to the caller so result transformations + # (like experience injection) reach tool.post_call. Route it through the sync + # path so the hook manager threads the return value back into kwargs. + is_sync = True async def _get_client(self, workspace_id: str, config: Any = None) -> VikingClient: return await get_global_client(workspace_id, config=config) diff --git a/bot/vikingbot/tests/unit/test_hooks_sync_routing.py b/bot/vikingbot/tests/unit/test_hooks_sync_routing.py new file mode 100644 index 0000000000..93a84e6b42 --- /dev/null +++ b/bot/vikingbot/tests/unit/test_hooks_sync_routing.py @@ -0,0 +1,51 @@ +"""Tests for hook is_sync routing and sync-path return threading. + +OpenVikingPostCallHook.execute() is async but MUST return its mutated kwargs so +result transformations reach tool.post_call. That only happens when the hook is +routed through the sync path (is_sync=True); the async path discards returns. +""" + +from vikingbot.hooks.base import Hook, HookContext +from vikingbot.hooks.builtins.openviking_hooks import OpenVikingPostCallHook +from vikingbot.hooks.manager import HookManager + + +def test_openviking_post_call_hook_is_sync(): + assert OpenVikingPostCallHook.is_sync is True + + +class _SyncEchoHook(Hook): + name = "sync_echo" + is_sync = True + + async def execute(self, context: HookContext, **kwargs): + return {**kwargs, "injected_by": "sync"} + + +class _AsyncEchoHook(Hook): + name = "async_echo" + is_sync = False + + async def execute(self, context: HookContext, **kwargs): + return {**kwargs, "injected_by": "async"} + + +async def test_sync_hook_return_is_threaded_back(): + manager = HookManager() + manager._hooks["tool.post_call"].append(_SyncEchoHook()) + + result = await manager.execute_hooks(HookContext(event_type="tool.post_call"), value=1) + + assert result["injected_by"] == "sync" + assert result["value"] == 1 + + +async def test_async_hook_return_is_discarded(): + manager = HookManager() + manager._hooks["tool.post_call"].append(_AsyncEchoHook()) + + result = await manager.execute_hooks(HookContext(event_type="tool.post_call"), value=1) + + # Async path routes through asyncio.gather and does not thread returns back. + assert "injected_by" not in result + assert result == {"value": 1} From c29afcbed5d67358f5cf3b43a152d9171c4e3b48 Mon Sep 17 00:00:00 2001 From: nankingjing <1079826437@qq.com> Date: Sun, 12 Jul 2026 09:04:50 +0800 Subject: [PATCH 2/4] ci: retrigger API & CLI Integration Tests The previous integration test run on PR #3164 was CANCELLED before completion (likely a runner timeout / superseded by newer push). Other checks (plugin-tests, check-deps) are green. Pushing an empty commit to re-trigger the integration suite so the PR has a definitive result before maintainer review. From d188e441dc395fc592ef37054f9212b6dd7da456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E4=BA=91=E9=BE=99?= <76432572+nankingjing@users.noreply.github.com> Date: Wed, 15 Jul 2026 22:24:08 +0800 Subject: [PATCH 3/4] fix(hooks): add str guard on result before regex, clarify is_sync routing comment --- bot/vikingbot/hooks/builtins/openviking_hooks.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/bot/vikingbot/hooks/builtins/openviking_hooks.py b/bot/vikingbot/hooks/builtins/openviking_hooks.py index a0158fad19..1d049819be 100644 --- a/bot/vikingbot/hooks/builtins/openviking_hooks.py +++ b/bot/vikingbot/hooks/builtins/openviking_hooks.py @@ -256,10 +256,12 @@ async def execute(self, context: HookContext, **kwargs) -> Any: class OpenVikingPostCallHook(Hook): name = "openviking_post_call" - # Hook execute() is genuinely async (it awaits ov_client search/read), but it - # MUST return its mutated kwargs dict to the caller so result transformations - # (like experience injection) reach tool.post_call. Route it through the sync - # path so the hook manager threads the return value back into kwargs. + # is_sync=True routes through the HookManager sync path, where each hook's + # return value is threaded back into kwargs: + # `kwargs = await hook.execute(context, **kwargs)` + # The default is_sync=False routes through asyncio.gather, which discards + # return values — the enriched {tool_name, params, result} dict would be + # silently dropped. is_sync = True async def _get_client(self, workspace_id: str, config: Any = None) -> VikingClient: @@ -383,7 +385,8 @@ async def _search_skill_experiences( async def execute(self, context: HookContext, tool_name, params, result) -> Any: if tool_name == "read_file": - if result and not isinstance(result, Exception): + # Guard: result must be a non-empty string before running regex. + if isinstance(result, str) and result and not isinstance(result, Exception): match = re.search(r"^---\s*\nname:\s*(.+?)\s*\n", result, re.MULTILINE) if match: skill_name = match.group(1).strip() From a2b53629fa80c57868be55fcf52511b45d0b59da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E4=BA=91=E9=BE=99?= <76432572+nankingjing@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:16:32 +0800 Subject: [PATCH 4/4] refactor(hooks): drop dead Exception check in post-call str guard, add execute() tests --- .../hooks/builtins/openviking_hooks.py | 7 +- .../tests/unit/test_hooks_sync_routing.py | 80 +++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/bot/vikingbot/hooks/builtins/openviking_hooks.py b/bot/vikingbot/hooks/builtins/openviking_hooks.py index 1d049819be..d3059acce2 100644 --- a/bot/vikingbot/hooks/builtins/openviking_hooks.py +++ b/bot/vikingbot/hooks/builtins/openviking_hooks.py @@ -385,8 +385,11 @@ async def _search_skill_experiences( async def execute(self, context: HookContext, tool_name, params, result) -> Any: if tool_name == "read_file": - # Guard: result must be a non-empty string before running regex. - if isinstance(result, str) and result and not isinstance(result, Exception): + # Only inspect non-empty string results. Tool failures reach this hook + # as Exception instances (the registry stores the raised exception as + # the result), and re.search would raise TypeError on non-str values — + # which, on the sync hook path, would escalate into a tool-call failure. + if isinstance(result, str) and result: match = re.search(r"^---\s*\nname:\s*(.+?)\s*\n", result, re.MULTILINE) if match: skill_name = match.group(1).strip() diff --git a/bot/vikingbot/tests/unit/test_hooks_sync_routing.py b/bot/vikingbot/tests/unit/test_hooks_sync_routing.py index 93a84e6b42..cf684a8bf3 100644 --- a/bot/vikingbot/tests/unit/test_hooks_sync_routing.py +++ b/bot/vikingbot/tests/unit/test_hooks_sync_routing.py @@ -49,3 +49,83 @@ async def test_async_hook_return_is_discarded(): # Async path routes through asyncio.gather and does not thread returns back. assert "injected_by" not in result assert result == {"value": 1} + + +class _StubbedSearchPostCallHook(OpenVikingPostCallHook): + """OpenVikingPostCallHook with the network-backed experience search stubbed.""" + + def __init__(self): + self.search_queries = [] + + async def _search_skill_experiences( + self, workspace_id, query, config=None, openviking_connection=None + ): + self.search_queries.append(query) + return "remembered experience" + + +def _post_call_context() -> HookContext: + return HookContext(event_type="tool.post_call", workspace_id="ws-test") + + +async def test_post_call_passes_exception_result_through(): + """Tool failures arrive as Exception results; the hook must not touch them. + + On the sync path a TypeError from re.search would escalate into a + tool-call failure, so the str guard is load-bearing here. + """ + hook = _StubbedSearchPostCallHook() + error = RuntimeError("tool blew up") + + out = await hook.execute(_post_call_context(), tool_name="read_file", params={}, result=error) + + assert out == {"tool_name": "read_file", "params": {}, "result": error} + assert hook.search_queries == [] + + +async def test_post_call_passes_non_string_result_through(): + hook = _StubbedSearchPostCallHook() + payload = {"content": "not a string"} + + out = await hook.execute(_post_call_context(), tool_name="read_file", params={}, result=payload) + + assert out["result"] is payload + assert hook.search_queries == [] + + +async def test_post_call_ignores_other_tools(): + hook = _StubbedSearchPostCallHook() + skill_md = "---\nname: web_search\n---\n" + + out = await hook.execute( + _post_call_context(), tool_name="exec_shell", params={}, result=skill_md + ) + + assert out["result"] == skill_md + assert hook.search_queries == [] + + +async def test_post_call_appends_experiences_for_skill_markdown(): + hook = _StubbedSearchPostCallHook() + skill_md = "---\nname: web_search\ndescription: Search the web for facts\n---\nUsage notes." + + out = await hook.execute( + _post_call_context(), tool_name="read_file", params={}, result=skill_md + ) + + assert hook.search_queries == ["Search the web for facts"] + assert out["result"].startswith(skill_md) + assert "## Related Experiences" in out["result"] + assert "remembered experience" in out["result"] + + +async def test_post_call_skips_experience_loader_skill(): + hook = _StubbedSearchPostCallHook() + skill_md = "---\nname: experience_loader\ndescription: loads experiences\n---\n" + + out = await hook.execute( + _post_call_context(), tool_name="read_file", params={}, result=skill_md + ) + + assert out["result"] == skill_md + assert hook.search_queries == []