Skip to content

fix: AI node skill invocation error #7849

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.

Sign up for GitHub

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
shaohuzhang1 merged 1 commit into from
Nov 12, 2026
Merged

fix: AI node skill invocation error #7849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/application/tests/test_ai_chat_tool_output.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ def on_complete(error):

with (
TemporaryDirectory(prefix="maxkb-summary-test-") as directory,
patch.object(agent_module, "init_skills", new=AsyncMock()),
patch.object(agent_module, "init_skills", new=MagicMock()),
patch.object(
agent_module,
"MultiServerMCPClient",
Expand Down Expand Up @@ -413,7 +413,7 @@ async def stream(*args, **kwargs):

with (
TemporaryDirectory(prefix="maxkb-backend-test-") as directory,
patch.object(agent_module, "init_skills", new=AsyncMock()),
patch.object(agent_module, "init_skills", new=MagicMock()),
patch.object(
agent_module, "MultiServerMCPClient", return_value=SimpleNamespace(get_tools=AsyncMock(return_value=[]))
),

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this diff, here are the observations:

Issue: Replacing AsyncMock with MagicMock for an async function

The diff changes init_skills from being patched with AsyncMock() to MagicMock() in two test locations. If init_skills is a coroutine/async function that gets awaited in production code, this change will cause problems.

Problems introduced by this change

  1. Awaitability breaks on call. If the production code does await init_skills(...), a MagicMock() returns a plain (non-awaitable) object when called, raising:

    TypeError: object MagicMock can't be used in 'await' expression

    Unless every awaited return value is explicitly configured with return_value=..., tests may pass by accident/spuriously or fail inconsistently.

  2. Loss of async semantics verification. AsyncMock asserts the call was awaited properly and correctly models async behavior (e.g., it's recognized by asyncio.iscoroutinefunction). Switching to MagicMock silently removes that safety net — a test could pass even if the code stops awaiting init_skills.

  3. Inconsistency risk across test suite. Tests still patching other async functions with AsyncMock (e.g., get_tools remains AsyncMock) but this one becomes MagicMock → inconsistent mocking model, harder to reason about.

  4. Silent false positives. If init_skills isn't actually awaited (a latent bug), MagicMock hides it; AsyncMock would surface it via a "coroutine was never awaited" warning.

Recommendation

  • If init_skills is async: revert to AsyncMock(). This is almost certainly what you want.
  • If it's sync: then AsyncMock was arguably wrong originally, and this change is correct — but double-check why get_tools (clearly async) uses AsyncMock while init_skills doesn't; verify the actual signature of agent_module.init_skills.
  • Confirm you're importing both mocks (from unittest.mock import AsyncMock, MagicMock) — the diff shows usage but not imports, so make sure MagicMock is imported in each test file to avoid NameError.

Minor / optimization notes

  • Both with blocks use nearly identical patch setups (init_skills, MultiServerMCPClient). Consider extracting a shared pytest fixture or helper context manager to reduce duplication and keep mock types consistent.
  • If init_skills returns something consumed downstream, set an explicit return_value (e.g., MagicMock(return_value=...) or AsyncMock(return_value=...)) rather than relying on auto-generated mocks.

Bottom line: This change is suspicious. Verify whether init_skills is async — if so, this is a regression and should be reverted to AsyncMock().

Expand Down
4 changes: 2 additions & 2 deletions apps/application/tests/test_langchain_compatibility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ def lookup(city: str) -> str:

with (
TemporaryDirectory(prefix="maxkb-agent-compat-") as directory,
patch.object(agent_module, "init_skills", new=AsyncMock()),
patch.object(agent_module, "init_skills", new=MagicMock()),
patch.object(
agent_module,
"MultiServerMCPClient",
Expand Down Expand Up @@ -442,7 +442,7 @@ def lookup(city: str) -> str:

with (
TemporaryDirectory(prefix="maxkb-agent-limit-") as directory,
patch.object(agent_module, "init_skills", new=AsyncMock()),
patch.object(agent_module, "init_skills", new=MagicMock()),
patch.object(agent_module.CONFIG, "get", return_value="1"),
patch.object(
agent_module,

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from AsyncMock to MagicMock is risky and likely incorrect unless init_skills has been explicitly refactored to be a synchronous function.

Key Issues & Suggestions:

  1. Asynchronous vs. Synchronous Mocks:

    • If init_skills is originally an async def function, replacing its patch with MagicMock() will break the code if any part of the test or implementation tries to await the mock result. MagicMock() does not support await.
    • Recommendation: Keep AsyncMock() if the underlying function remains asynchronous. Only use MagicMock() if you are certain init_skills was changed to a regular (non-async) function in the source code.

  2. Best Practice for Async Patches:

    • Using new=AsyncMock() is correct for mocking async functions.
    • If you want to control the return value, ensure it's an awaitable if the caller awaits it. For example: patch("module.async_func", new_callable=AsyncMock, return_value="result").

  3. Optimization/Readability:

    • The with statement grouping multiple contexts using parentheses (as seen here) is valid Python 3.9+ syntax and improves readability. No issue here.

Conclusion:

Revert to AsyncMock() unless init_skills is no longer an async function. Otherwise, you risk runtime errors (TypeError: object MagicMock can’t be used in 'await' expression).

Expand Down
3 changes: 2 additions & 1 deletion apps/application/workflow/nodes/ai_chat_node/agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,6 @@ def create_agent(

async def _run():
checkpointer = MemorySaver()
await init_skills(skill_tool_ids, temp_dir)
client = MultiServerMCPClient(json.loads(mcp_servers))
tools = await client.get_tools()
for tool in tools:
Expand Down Expand Up @@ -236,6 +235,8 @@ def _classify_error(e):

error = None
try:
# 技能初始化含同步 ORM / 文件 IO,在当前工作线程执行,不进入事件循环
init_skills(skill_tool_ids, temp_dir)
async_runner.run_async(_run)
except Exception as e:
error = _classify_error(e)
Expand Down
13 changes: 8 additions & 5 deletions apps/application/workflow/nodes/ai_chat_node/tools/skill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,22 @@
import os
import zipfile

from asgiref.sync import sync_to_async
from django.db.models import QuerySet

from common.utils.rsa_util import rsa_long_decrypt
from knowledge.models import File
from tools.models import Tool


async def init_skills(skill_tool_ids, temp_dir):
def init_skills(skill_tool_ids, temp_dir):
"""
同步函数：包含 ORM 查询、RSA 密钥读取(缓存未命中时查库)、文件读取与解压，
必须在工作线程中调用，不能在事件循环线程内执行(否则触发 SynchronousOnlyOperation 并阻塞循环)。
"""
if not skill_tool_ids:
return
skills_dir = os.path.join(temp_dir, "skills")
tools = await sync_to_async(lambda: list(QuerySet(Tool).filter(id__in=skill_tool_ids, is_active=True)))()
tools = list(QuerySet(Tool).filter(id__in=skill_tool_ids, is_active=True))
if not tools:
return

Expand All @@ -35,10 +38,10 @@ async def init_skills(skill_tool_ids, temp_dir):
else:
params = init_params_default_value

file = await sync_to_async(lambda t=tool: QuerySet(File).filter(id=t.code).first())()
file = QuerySet(File).filter(id=tool.code).first()
if not file:
continue
file_bytes = await sync_to_async(file.get_bytes)()
file_bytes = file.get_bytes()

with zipfile.ZipFile(io.BytesIO(file_bytes), "r") as zip_ref:
members = [m for m in zip_ref.namelist() if not m.startswith("__MACOSX/") and "__MACOSX" not in m]

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the issues, potential problems and optimization suggestions for this diff:

Correctness / Potential Issues

  1. Caller contract change is a breaking API change. The function was previously async; it is now sync. Every call site must be updated (removed await, wrapped in an executor/threadpool, e.g. sync_to_async(init_skills)(...) or asyncio.to_thread(...)). If any caller still does await init_skills(...), it will break at runtime. Verify all callers.

  2. The docstring says "cache miss DB query" for RSA key reading, but this init_skills code does not read RSA keys. The comment about rsa_long_decrypt / RSA cache seems to describe surrounding context rather than the shown code — it is misleading here. Also rsa_long_decrypt remains imported but appears unused after the diff (dead import).

  3. list(QuerySet(Tool).filter(...)) — wrapping a QuerySet instance in QuerySet(Tool) is unusual/legacy and unnecessary; use Tool.objects.filter(...) (or QuerySet(model=Tool).filter(...)) instead. The same applies to File.

  4. N+1 pattern kept as-is: one query per tool for File. Since all tools are fetched up front, you can batch fetch files:

    codes = [t.code for t in tools]
files = {f.id: f for f in File.objects.filter(id__in=codes)}

    This reduces N queries to 1 and matters if skill_tool_ids grows.

  5. file.get_bytes() may do I/O or return large blobs; with the function now synchronous, this I/O runs on whatever thread calls it. Ensure the caller offloads it (as the docstring states) — otherwise it will block the event loop if called from async code without a thread wrapper.

  6. Zip handling: zip_ref.namelist() filtering keeps the existing logic, but note that file.get_bytes() returns bytes that are stored fully in memory. For large archives, prefer streaming (zipfile.ZipFile(file_obj) on a file handle) if feasible. Also consider Zip Slip protection when extracting to skills_dir (reject member paths containing .. or absolute paths).

  7. r = ... variable naming: the renamed lambda default-argument workaround was removed, which is correct — but make sure no closure-capture bug existed for other variables within the loop (e.g. params assigned per iteration should be fine).

Style / Minor

  1. Long Chinese docstring in an English-based codebase — inconsistent with the rest of the module. Translate or shorten; also the multi-line format is fine but consider keeping it concise.
  2. Unused imports: rsa_long_decrypt (and sync_to_async is removed intentionally — good) — remove rsa_long_decrypt if truly unused.
  3. Type hints missing: since this function is now sync and will be called from threads, adding def init_skills(skill_tool_ids: list[int] | None, temp_dir: str) -> None: improves clarity.
  4. No error handling per-file: if one zip is corrupt, the whole loop aborts. Consider try/except per tool with logging so one bad tool doesn't break skill initialization.

Summary

The core change (making the function synchronous) is reasonable only if every caller is updated to run it in a worker thread. Fix the dead import, clarify the docstring, switch to Tool.objects/File.objects, and batch the File lookup to avoid N+1 queries.

Expand Down
Loading