[shaohuzhang1]

fix: AI node skill invocation error

[shaohuzhang1]

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

8. 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.
9. Unused imports: rsa_long_decrypt (and sync_to_async is removed intentionally — good) — remove rsa_long_decrypt if truly unused.
10. 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.
11. 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.

[shaohuzhang1]

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().

[shaohuzhang1]

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).