Skip to content

fix(hooks): route OpenVikingPostCallHook through the sync path so enriched results aren't discarded - #3164

Open
nankingjing wants to merge 4 commits into
volcengine:mainfrom
nankingjing:fix/async-hooks-discard-returns
Open

fix(hooks): route OpenVikingPostCallHook through the sync path so enriched results aren't discarded#3164
nankingjing wants to merge 4 commits into
volcengine:mainfrom
nankingjing:fix/async-hooks-discard-returns

Conversation

@nankingjing

@nankingjing nankingjing commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

OpenVikingPostCallHook.execute() enriches read_file results for skill files with related experience memories, returning {tool_name, params, result}. Since #2503 flipped the hook to is_sync = False, that return value has been silently discarded: the enrichment never reaches the model.

Problem

HookManager.execute_hooks has two paths:

  • async path (is_sync = False): asyncio.gather(...) — results are only inspected for error logging and then dropped;
  • sync path (is_sync = True): kwargs = await hook.execute(context, **kwargs) — the return value is threaded back and ultimately consumed by the tool registry via hook_result.get("result").

#2503 moved this hook to the async path on the reasoning that "execute() is genuinely async". But both paths await coroutines (the sync path also awaits), and execute_hooks awaits the gather before returning anyway — so the async path buys no latency here; it only loses the return value. Result: the appended "Related Experiences" block silently vanished from every read_file on a skill file.

Fix

  • Restore is_sync = True with a comment documenting the routing contract (sync path = return values threaded back; async path = returns discarded).
  • Add an isinstance(result, str) guard before running the regex. Tool failures reach this hook as Exception instances (the registry stores the raised exception as the result), and re.search raises TypeError on non-str input. On the sync path such an exception would now propagate and turn a completed tool call into a failure, so the guard is load-bearing. The previous not isinstance(result, Exception) check is dropped: a str can never also be an Exception (CPython rejects that MI with "multiple bases have instance lay-out conflict"), so it was dead code.

Tests

bot/vikingbot/tests/unit/test_hooks_sync_routing.py (8 tests):

  • routing: post-call hook is marked sync; sync-path returns are threaded back; async-path returns are discarded (documents the manager contract this fix relies on);
  • execute() behavior with the network search stubbed: Exception/dict results pass through untouched, non-read_file tools are ignored, skill markdown gets the experiences appended, and the experience_loader skill is skipped.

The previous integration test run on PR volcengine#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.
@nankingjing

Copy link
Copy Markdown
Contributor Author
Self-reviewed: diff is correct, minimal, and well-tested. No issues found.

@nankingjing nankingjing changed the title fix(hooks): route OpenVikingPostCallHook async so enriched result isn't discarded fix(hooks): route OpenVikingPostCallHook through the sync path so enriched results aren't discarded Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant