[codex] Bundle Node runtime for ACP desktop

[jinglinpeng]

Description

Bundle a Node.js runtime with the Tauri desktop package and add a small ACP Node settings entry so built-in ACP agents that run through npx can work without requiring users to install Node separately.

Related Issue: N/A

Security Considerations: ACP only prepends the selected Node directory to the ACP child process PATH; it does not modify the user/system PATH. Custom Node paths are validated by running node --version and npx --version before saving.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation
• Refactoring

Component(s) Affected

• Core / Backend (app, agents, config, providers, utils, local_models)
• Console (frontend web UI)
• Channels (DingTalk, Lark, QQ, Discord, iMessage, etc.)
• Skills
• CLI
• Documentation (website)
• Tests
• CI/CD
• Scripts / Deploy

Checklist

• I ran pre-commit run --all-files locally and it passes
• If pre-commit auto-fixed files, I committed those changes and reran checks
• I ran tests locally (pytest or as relevant) and they pass
• Documentation updated (if needed)
• Ready for review

For Channel Changes (DingTalk, Lark, QQ, Console, etc.)

• I ran ./scripts/check-channels.sh (or ./scripts/check-channels.sh --changed) and it passes
• Contract test exists in tests/contract/channels/test_<channel>_contract.py (REQUIRED)
• Contract test implements create_instance() with proper channel initialization
• All 19 contract verification points pass (see tests/contract/channels/__init__.py)
• Optional: Unit tests in tests/unit/channels/test_<channel>.py for complex internal logic

Testing

• python -m compileall src\\qwenpaw\\agents\\acp\\node_runtime.py src\\qwenpaw\\agents\\acp\\service.py src\\qwenpaw\\app\\routers\\config.py src\\qwenpaw\\config\\config.py scripts\\pack-tauri\\stage_node_runtime.py
• python -c "import json, pathlib; files=list(pathlib.Path('console/src/locales').glob('*.json')); [json.load(open(f, encoding='utf-8')) for f in files]; print('locales ok', len(files))"
• git diff --check
• python scripts\\pack-tauri\\stage_node_runtime.py --help
• Verified local node / npx detection with the ACP Node runtime helper
• Triggered QwenPaw Desktop Build on the fork branch for Tauri Windows/macOS packaging

Local Verification Evidence

python -m compileall src\\qwenpaw\\agents\\acp\\node_runtime.py src\\qwenpaw\\agents\\acp\\service.py src\\qwenpaw\\app\\routers\\config.py src\\qwenpaw\\config\\config.py scripts\\pack-tauri\\stage_node_runtime.py
# passed

python -c "import json, pathlib; files=list(pathlib.Path('console/src/locales').glob('*.json')); [json.load(open(f, encoding='utf-8')) for f in files]; print('locales ok', len(files))"
# locales ok 7

git diff --check
# passed

pre-commit run --all-files, full pytest, and frontend type/lint were not run locally. console/node_modules is not installed in this checkout; desktop packaging verification is running in GitHub Actions on the fork branch.

Additional Notes

The desktop bundle includes the Node runtime only, not ACP npm packages. Existing ACP commands such as npx -y @zed-industries/codex-acp still resolve packages at runtime through npm/npx.

[github-actions]

Welcome to QwenPaw! 🐾

Hi @jinglinpeng, this is your 43rd Pull Request.

🙌 Join Developer Community

Thanks so much for your contribution! We'd love to invite you to join the official QwenPaw developer group! You can find the Discord and DingTalk group links under the "Developer Community" section on our docs page:
https://qwenpaw.agentscope.io/docs/community

We truly appreciate your enthusiasm—and look forward to your future contributions! 😊

We'll review your PR soon.

[github-actions]

✅ QwenPaw AI Review: Passed AI Review — Awaiting Human Review

Now I have all the information needed. Let me produce the review.

---

1. Overview

Item	Details
PR Number	#5814
Author	@jinglinpeng
Changes	22 files, +912 / −16
Merge Target	main
Related Issue	N/A (stated in PR body)

2. Background

ACP (Agent Communication Protocol) agents like Codex require a Node.js runtime to execute via npx. Currently, desktop users must install Node.js separately. This PR bundles a Node.js runtime with the Tauri desktop package and adds a UI settings entry so users can configure which Node runtime ACP uses — bundled, system, or a custom path.

3. Core Changes

• New node_runtime.py (285 lines): Core module that detects bundled/system/custom Node runtimes, validates them by running node --version and npx --version, and builds the correct PATH for ACP subprocess spawning.
• service.py integration: _open_conversation now calls build_acp_process_env() to inject the Node runtime into the process environment, and _resolve_process_command() resolves npx to its full path.
• REST API (config.py): New GET /config/acp/node-runtime and PUT /config/acp/node-runtime endpoints for querying and updating the Node runtime.
• stage_node_runtime.py (128 lines): Build script that downloads and stages a Node.js distribution into the Tauri bundle directory.
• Rust side (command.rs): Detects the bundled node-runtime directory in release builds and passes it to the Python backend via QWENPAW_DESKTOP_NODE_RUNTIME env var.
• Frontend (ACP/index.tsx): New "Node Settings" modal with a <Select> dropdown listing available Node candidates, plus a file-picker dialog for custom paths (Tauri native dialog on desktop, window.prompt fallback on web).
• i18n: All 7 locale files updated with identical key structure (nodeSettings, nodePath, nodeRuntime, nodeRuntimeReason, etc.).
• error.ts: parseErrorDetail changed from indexOf to lastIndexOf for " - " separator.
• Build scripts: Both .sh and .ps1 pack scripts updated to invoke stage_node_runtime.py.

4. Strengths

1. Clean separation of concerns: The node_runtime.py module is well-structured with clear functions for detection (get_node_runtime_status), validation (resolve_node_runtime), and process environment building (build_acp_process_env). The candidate-based model makes it easy to extend.

2. Security-conscious design: The Node runtime path is only prepended to the ACP child process PATH (not system-wide). Custom paths are validated via node --version and npx --version before being accepted, preventing arbitrary executable injection.

3. Cross-platform handling: Windows extended-length path prefix stripping (\\?\), case-insensitive path comparison (os.path.normcase), platform-aware executable names (node.exe vs bin/node), and CREATE_NO_WINDOW flag for subprocess on Windows — all handled correctly.

4. Frontend UX: The Select dropdown with disabled unavailable options, color-coded reason messages, and native file dialog on Tauri desktop is a thoughtful implementation. The sameNodePath normalization function handles cross-platform path comparison on the client side.

5. Complete i18n coverage: All 7 locale files are updated with consistent key structures — no missing translations.

6. Route ordering: The new /acp/node-runtime endpoints are correctly placed before /acp/{agent_name} in the FastAPI router, preventing path-parameter hijacking.

7. Caching in staging script: The .node-runtime-version marker file prevents redundant downloads on repeated builds.

5. Issues and Suggestions

High

None.

Medium

1. Blocking subprocess.run in async context

Code reference — src/qwenpaw/agents/acp/node_runtime.py, _version():

def _version(executable: str, env: dict[str, str]) -> tuple[str, str]:
    creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0)
    try:
        result = subprocess.run(
            [executable, "--version"],
            capture_output=True,
            text=True,
            timeout=5,
            env=env,
            creationflags=creationflags,
            check=False,
        )

Explanation: _version() is called from resolve_node_runtime(), which is invoked from the put_acp_node_runtime async endpoint handler. subprocess.run blocks the event loop for up to 5 seconds per invocation (timeout). Since resolve_node_runtime calls _version twice (once for node, once for npx), a single PUT request could block the event loop for up to 10 seconds.

Additionally, build_acp_process_env (called from the async _open_conversation in service.py) doesn't call _version directly, but it does call shutil.which and Path.is_file() which are synchronous disk I/O — acceptable for those since they're fast, but worth noting.

Suggested fix: Wrap the subprocess calls with asyncio.to_thread or use asyncio.create_subprocess_exec:

async def _version(executable: str, env: dict[str, str]) -> tuple[str, str]:
    creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0)
    try:
        proc = await asyncio.create_subprocess_exec(
            executable, "--version",
            stdout=asyncio.subprocess.PIPE,
            stderr=asyncio.subprocess.PIPE,
            env=env,
            creationflags=creationflags,
        )
        stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=5)
        ...

Alternatively, if keeping _version synchronous, run resolve_node_runtime via asyncio.to_thread in the endpoint handler:

candidate = await asyncio.to_thread(resolve_node_runtime, node_path)

---

2. parseErrorDetail behavioral change from indexOf to lastIndexOf is undocumented and may affect other callers

Code reference — console/src/utils/error.ts:

-  const idx = msg.indexOf(" - ");
+  const idx = msg.lastIndexOf(" - ");

Explanation: This is a shared utility function. Changing from first-match to last-match semantics alters behavior for all callers, not just the new ACP Node runtime error handling. If any existing error message format contains " - " within the JSON payload itself (e.g., {"detail": "some - thing"}), lastIndexOf would find the separator inside the JSON and produce incorrect parsing. The PR body does not mention this change, and there are no tests covering it.

Suggested fix: Add a comment explaining why lastIndexOf is needed, and consider verifying that no other callers depend on first-match behavior. If only the ACP error format needs this, consider handling it in the ACP-specific error handler rather than modifying the shared utility.

---

3. tarfile.extractall fallback without filter on Python < 3.12

Code reference — scripts/pack-tauri/stage_node_runtime.py:

    else:
        with tarfile.open(archive, "r:xz") as tar:
            try:
                tar.extractall(workdir, filter="data")
            except TypeError:
                tar.extractall(workdir)

Explanation: On Python versions before 3.12, filter="data" is not supported and the TypeError fallback calls extractall without any filter. This means a maliciously crafted tarball could write files outside workdir via path traversal (e.g., ../../etc/crontab). While the source is nodejs.org (trusted), this is a build-time security concern — a compromised CDN or MITM attack could exploit this.

Suggested fix: Add a manual path validation for the fallback case:

            except TypeError:
                for member in tar.getmembers():
                    member_path = (workdir / member.name).resolve()
                    if not str(member_path).startswith(str(workdir.resolve())):
                        raise SystemExit(
                            f"tar member escapes target: {member.name}"
                        )
                tar.extractall(workdir)

Low

1. _normalize_node_path return type annotation is misleading

Code reference — src/qwenpaw/agents/acp/node_runtime.py:

def _normalize_node_path(value: str) -> Path | None:
    path = Path(os.path.expandvars(value)).expanduser()
    if path.is_dir():
        path = path / ("node.exe" if os.name == "nt" else "bin/node")
    return path

Explanation: The function always returns a Path, never None. The Path | None return type is misleading — callers like _bundled_node_path check for None, but that check will never be true from this function. The None return only comes from _bundled_node_path itself when the env var is empty. Consider correcting the type to Path or explicitly returning None for empty input:

def _normalize_node_path(value: str) -> Path | None:
    if not value:
        return None
    path = Path(os.path.expandvars(value)).expanduser()
    if path.is_dir():
        path = path / ("node.exe" if os.name == "nt" else "bin/node")
    return path

---

2. _version uses broad except Exception

Code reference — src/qwenpaw/agents/acp/node_runtime.py:

    except Exception as exc:
        return "", f"{Path(executable).name} --version failed: {exc}"

Explanation: While this is a reasonable defensive pattern for version detection (any failure should return an error string), the project coding standards discourage overly broad exception handling. Consider narrowing to except (OSError, subprocess.SubprocessError) which covers the realistic failure modes.

---

3. Portuguese (Brazilian) locale uses unaccented characters

Code reference — console/src/locales/pt-BR.json:

"nodePathPrompt": "Informe o caminho do executavel node",
"nodeSaveFailed": "Caminho do Node indisponivel",

Explanation: "executavel" should be "executável" and "indisponivel" should be "indisponível". Other strings in the same file also lack accents (e.g., "Configuracoes" instead of "Configurações", "verificacao" instead of "verificação"). This appears to be consistent with the existing style in this locale file (pre-existing strings also lack accents), so it may be intentional, but consider verifying.

6. Summary

This is a well-architected feature that adds Node.js runtime bundling and configuration for ACP agents, with clean separation between detection logic, API endpoints, and UI. The cross-platform handling is thorough and the i18n coverage is complete. The main concern is the use of blocking subprocess.run in async endpoint handlers, which could stall the event loop under adverse conditions.

• 2 items recommended to address before merge: (1) blocking subprocess.run in async context, (2) undocumented behavioral change to shared parseErrorDetail utility.
• 1 item to consider: tar extraction fallback security hardening.
• 3 items for follow-up: type annotation correction, exception narrowing, and locale accent verification.

---

Auto-generated by QwenPaw Review Agent | Triggered by maintainer approval