Skip to content

Commit 589cce1

Browse files
authored
fix: improve Windows local skill file reading (AstrBotDevs#6028)
* chore: ignore local worktrees * fix: improve Windows local skill file reading * fix: address Windows path and decoding review feedback * fix: simplify shell decoding follow-up * fix: harden sandbox skill prompt metadata * fix: preserve safe sandbox skill summaries * fix: relax sandbox summary sanitization * fix: tighten path sanitization for skill prompts * fix: harden sandbox skill display metadata * fix: preserve Unicode skill paths in prompts * fix: quote Windows skill prompt paths * fix: simplify local shell output decoding * fix: localize Windows prompt path handling * fix: normalize Windows-style skill paths in prompts * fix: align prompt and shell decoding behavior
1 parent e254caf commit 589cce1

5 files changed

Lines changed: 455 additions & 20 deletions

File tree

.gitignore

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,4 +62,4 @@ GenieData/
6262
.opencode/
6363
.kilocode/
6464
.worktrees/
65-
docs/plans/
65+

astrbot/core/computer/booters/local.py

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from __future__ import annotations
22

33
import asyncio
4+
import locale
45
import os
56
import shutil
67
import subprocess
@@ -52,6 +53,31 @@ def _ensure_safe_path(path: str) -> str:
5253
return abs_path
5354

5455

56+
def _decode_shell_output(output: bytes | None) -> str:
57+
if output is None:
58+
return ""
59+
60+
preferred = locale.getpreferredencoding(False) or "utf-8"
61+
try:
62+
return output.decode("utf-8")
63+
except (LookupError, UnicodeDecodeError):
64+
pass
65+
66+
if os.name == "nt":
67+
for encoding in ("mbcs", "cp936", "gbk", "gb18030"):
68+
try:
69+
return output.decode(encoding)
70+
except (LookupError, UnicodeDecodeError):
71+
continue
72+
73+
try:
74+
return output.decode(preferred)
75+
except (LookupError, UnicodeDecodeError):
76+
pass
77+
78+
return output.decode("utf-8", errors="replace")
79+
80+
5581
@dataclass
5682
class LocalShellComponent(ShellComponent):
5783
async def exec(
@@ -72,28 +98,32 @@ def _run() -> dict[str, Any]:
7298
run_env.update({str(k): str(v) for k, v in env.items()})
7399
working_dir = _ensure_safe_path(cwd) if cwd else get_astrbot_root()
74100
if background:
75-
proc = subprocess.Popen(
101+
# `command` is intentionally executed through the current shell so
102+
# local computer-use behavior matches existing tool semantics.
103+
# Safety relies on `_is_safe_command()` and the allowed-root checks.
104+
proc = subprocess.Popen( # noqa: S602 # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit
76105
command,
77106
shell=shell,
78107
cwd=working_dir,
79108
env=run_env,
80-
stdout=subprocess.PIPE,
81-
stderr=subprocess.PIPE,
82-
text=True,
109+
stdout=subprocess.DEVNULL,
110+
stderr=subprocess.DEVNULL,
83111
)
84112
return {"pid": proc.pid, "stdout": "", "stderr": "", "exit_code": None}
85-
result = subprocess.run(
113+
# `command` is intentionally executed through the current shell so
114+
# local computer-use behavior matches existing tool semantics.
115+
# Safety relies on `_is_safe_command()` and the allowed-root checks.
116+
result = subprocess.run( # noqa: S602 # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit
86117
command,
87118
shell=shell,
88119
cwd=working_dir,
89120
env=run_env,
90121
timeout=timeout,
91122
capture_output=True,
92-
text=True,
93123
)
94124
return {
95-
"stdout": result.stdout,
96-
"stderr": result.stderr,
125+
"stdout": _decode_shell_output(result.stdout),
126+
"stderr": _decode_shell_output(result.stderr),
97127
"exit_code": result.returncode,
98128
}
99129

astrbot/core/skills/skill_manager.py

Lines changed: 82 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import json
44
import os
55
import re
6+
import shlex
67
import shutil
78
import tempfile
89
import zipfile
@@ -79,7 +80,59 @@ def _parse_frontmatter_description(text: str) -> str:
7980

8081
# Regex for sanitizing paths used in prompt examples — only allow
8182
# safe path characters to prevent prompt injection via crafted skill paths.
82-
_SAFE_PATH_RE = re.compile(r"[^A-Za-z0-9_./ -]")
83+
_SAFE_PATH_RE = re.compile(r"[^\w./ ,()'\-]", re.UNICODE)
84+
_WINDOWS_DRIVE_PATH_RE = re.compile(r"^[A-Za-z]:(?:/|\\)")
85+
_WINDOWS_UNC_PATH_RE = re.compile(r"^(//|\\\\)[^/\\]+[/\\][^/\\]+")
86+
_CONTROL_CHARS_RE = re.compile(r"[\x00-\x1F\x7F]")
87+
88+
89+
def _is_windows_prompt_path(path: str) -> bool:
90+
if os.name != "nt":
91+
return False
92+
return bool(_WINDOWS_DRIVE_PATH_RE.match(path) or _WINDOWS_UNC_PATH_RE.match(path))
93+
94+
95+
def _sanitize_prompt_path_for_prompt(path: str) -> str:
96+
if not path:
97+
return ""
98+
99+
if _WINDOWS_DRIVE_PATH_RE.match(path) or _WINDOWS_UNC_PATH_RE.match(path):
100+
path = path.replace("\\", "/")
101+
102+
drive_prefix = ""
103+
if _WINDOWS_DRIVE_PATH_RE.match(path):
104+
drive_prefix = path[:2]
105+
path = path[2:]
106+
107+
path = path.replace("`", "")
108+
path = _CONTROL_CHARS_RE.sub("", path)
109+
sanitized = _SAFE_PATH_RE.sub("", path)
110+
return f"{drive_prefix}{sanitized}"
111+
112+
113+
def _sanitize_prompt_description(description: str) -> str:
114+
description = description.replace("`", "")
115+
description = _CONTROL_CHARS_RE.sub(" ", description)
116+
description = " ".join(description.split())
117+
return description
118+
119+
120+
def _sanitize_skill_display_name(name: str) -> str:
121+
if _SKILL_NAME_RE.fullmatch(name):
122+
return name
123+
return "<invalid_skill_name>"
124+
125+
126+
def _build_skill_read_command_example(path: str) -> str:
127+
if path == "<skills_root>/<skill_name>/SKILL.md":
128+
return f"cat {path}"
129+
if _is_windows_prompt_path(path):
130+
command = "type"
131+
path_arg = f'"{path}"'
132+
else:
133+
command = "cat"
134+
path_arg = shlex.quote(path)
135+
return f"{command} {path_arg}"
83136

84137

85138
def build_skills_prompt(skills: list[SkillInfo]) -> str:
@@ -92,16 +145,37 @@ def build_skills_prompt(skills: list[SkillInfo]) -> str:
92145
skills_lines: list[str] = []
93146
example_path = ""
94147
for skill in skills:
148+
display_name = _sanitize_skill_display_name(skill.name)
149+
95150
description = skill.description or "No description"
151+
if skill.source_type == "sandbox_only":
152+
description = _sanitize_prompt_description(description)
153+
if not description:
154+
description = "Read SKILL.md for details."
155+
156+
if skill.source_type == "sandbox_only":
157+
rendered_path = (
158+
f"{str(SANDBOX_WORKSPACE_ROOT)}/{str(SANDBOX_SKILLS_ROOT)}/"
159+
f"{display_name}/SKILL.md"
160+
)
161+
else:
162+
rendered_path = _sanitize_prompt_path_for_prompt(skill.path)
163+
if not rendered_path:
164+
rendered_path = "<skills_root>/<skill_name>/SKILL.md"
165+
96166
skills_lines.append(
97-
f"- **{skill.name}**: {description}\n File: `{skill.path}`"
167+
f"- **{display_name}**: {description}\n File: `{rendered_path}`"
98168
)
99169
if not example_path:
100-
example_path = skill.path
170+
example_path = rendered_path
101171
skills_block = "\n".join(skills_lines)
102172
# Sanitize example_path — it may originate from sandbox cache (untrusted)
103-
example_path = _SAFE_PATH_RE.sub("", example_path) if example_path else ""
104-
example_path = example_path or "<skills_root>/<skill_name>/SKILL.md"
173+
if example_path == "<skills_root>/<skill_name>/SKILL.md":
174+
example_path = "<skills_root>/<skill_name>/SKILL.md"
175+
else:
176+
example_path = _sanitize_prompt_path_for_prompt(example_path)
177+
example_path = example_path or "<skills_root>/<skill_name>/SKILL.md"
178+
example_command = _build_skill_read_command_example(example_path)
105179

106180
return (
107181
"## Skills\n\n"
@@ -119,8 +193,9 @@ def build_skills_prompt(skills: list[SkillInfo]) -> str:
119193
"*Never silently skip a matching skill* — either use it or briefly "
120194
"explain why you chose not to.\n"
121195
"3. **Mandatory grounding** — Before executing any skill you MUST "
122-
"first read its `SKILL.md` by running a shell command with the "
123-
f"**absolute path** shown above (e.g. `cat {example_path}`). "
196+
"first read its `SKILL.md` by running a shell command compatible "
197+
"with the current runtime shell and using the **absolute path** "
198+
f"shown above (e.g. `{example_command}`). "
124199
"Never rely on memory or assumptions about a skill's content.\n"
125200
"4. **Progressive disclosure** — Load only what is directly "
126201
"referenced from `SKILL.md`:\n"
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
from __future__ import annotations
2+
3+
import asyncio
4+
import subprocess
5+
6+
from astrbot.core.computer.booters import local as local_booter
7+
from astrbot.core.computer.booters.local import LocalShellComponent
8+
9+
10+
class _FakeCompletedProcess:
11+
def __init__(self, stdout: bytes, stderr: bytes = b"", returncode: int = 0):
12+
self.stdout = stdout
13+
self.stderr = stderr
14+
self.returncode = returncode
15+
16+
17+
def test_local_shell_component_decodes_utf8_output(monkeypatch):
18+
def fake_run(*args, **kwargs):
19+
_ = args, kwargs
20+
return _FakeCompletedProcess(stdout="技能内容".encode())
21+
22+
monkeypatch.setattr(subprocess, "run", fake_run)
23+
24+
result = asyncio.run(LocalShellComponent().exec("dummy"))
25+
26+
assert result["stdout"] == "技能内容"
27+
assert result["stderr"] == ""
28+
assert result["exit_code"] == 0
29+
30+
31+
def test_local_shell_component_prefers_utf8_before_windows_locale(
32+
monkeypatch,
33+
):
34+
def fake_run(*args, **kwargs):
35+
_ = args, kwargs
36+
return _FakeCompletedProcess(stdout="技能内容".encode())
37+
38+
monkeypatch.setattr(subprocess, "run", fake_run)
39+
monkeypatch.setattr(local_booter.os, "name", "nt", raising=False)
40+
monkeypatch.setattr(
41+
local_booter.locale,
42+
"getpreferredencoding",
43+
lambda _do_setlocale=False: "cp936",
44+
)
45+
46+
result = asyncio.run(LocalShellComponent().exec("dummy"))
47+
48+
assert result["stdout"] == "技能内容"
49+
assert result["stderr"] == ""
50+
assert result["exit_code"] == 0
51+
52+
53+
def test_local_shell_component_falls_back_to_gbk_on_windows(monkeypatch):
54+
def fake_run(*args, **kwargs):
55+
_ = args, kwargs
56+
return _FakeCompletedProcess(stdout="微博热搜".encode("gbk"))
57+
58+
monkeypatch.setattr(subprocess, "run", fake_run)
59+
monkeypatch.setattr(local_booter.os, "name", "nt", raising=False)
60+
monkeypatch.setattr(
61+
local_booter.locale,
62+
"getpreferredencoding",
63+
lambda _do_setlocale=False: "cp1252",
64+
)
65+
66+
result = asyncio.run(LocalShellComponent().exec("dummy"))
67+
68+
assert result["stdout"] == "微博热搜"
69+
assert result["stderr"] == ""
70+
assert result["exit_code"] == 0
71+
72+
73+
def test_local_shell_component_falls_back_to_utf8_replace(monkeypatch):
74+
def fake_run(*args, **kwargs):
75+
_ = args, kwargs
76+
return _FakeCompletedProcess(stdout=b"\xffabc")
77+
78+
monkeypatch.setattr(subprocess, "run", fake_run)
79+
monkeypatch.setattr(local_booter.os, "name", "posix", raising=False)
80+
monkeypatch.setattr(
81+
local_booter.locale,
82+
"getpreferredencoding",
83+
lambda _do_setlocale=False: "utf-8",
84+
)
85+
86+
result = asyncio.run(LocalShellComponent().exec("dummy"))
87+
88+
assert result["stdout"] == "\ufffdabc"

0 commit comments

Comments
 (0)