diff --git a/ms_agent/agent_hub/_commands.py b/ms_agent/agent_hub/_commands.py index e76b8ab42..1712238a2 100644 --- a/ms_agent/agent_hub/_commands.py +++ b/ms_agent/agent_hub/_commands.py @@ -39,7 +39,7 @@ def api_error_message(e: APIError, action: str = 'request') -> str: """Return a user-friendly message based on the HTTP status code.""" status = e.status_code or 0 rid = f' (request_id={e.request_id})' if getattr(e, 'request_id', - None) else '' + None) else '' if status == 401: return 'authentication failed. Please login again.' + rid if status == 403: @@ -389,6 +389,78 @@ def cmd_upload( return 0 +def _remote_paths_for_agent( + framework: str, + requested: str, + remote_paths: list[str], + local_dir=None, +) -> tuple[dict[str, str] | None, str | None]: + """Map remote repo paths to workspace-relative paths for *requested*. + + A repo uploaded with ``-n all`` from a root-per-agent framework stores the + default agent at bare paths and every named agent under its per-agent + prefix (hermes ``profiles//``, openclaw ``workspace-/``, + qwenpaw ``/``). When downloading a single agent from such a repo, + only that agent's files may be taken (with the prefix stripped) -- + otherwise another agent's files would be written into the requested + agent's directory and its own files silently dropped. + + Returns ``(mapping, error)``: *mapping* is ``{remote_path: relative_path}`` + restricted to the requested agent (identity mapping for legacy + single-agent bare repos), *error* is set when the repo is an all-layout + repo that has no files for *requested*. + """ + identity = {p: p for p in remote_paths} + if requested in (ALL_AGENT_NAME, GLOBAL_AGENT_NAME): + # all: every agent's files are wanted as-is; global: shared files are + # selected by the (name-free) pattern filter downstream. + return identity, None + probe = build_spec(framework, requested, local_dir) + if not probe.is_root_per_agent: + return identity, None + + prefix = probe.join_all_path(requested, '') + if not prefix: + # Requested agent lives at bare paths in all mode (hermes default): + # keep bare files, exclude every named-agent prefix. + selected = { + p: p + for p in remote_paths + if probe.split_all_path(p)[0] in (requested, None) + } + return (selected, None) if selected else ( + None, f"repository has no files for agent '{requested}'.") + + selected = { + p: p[len(prefix):] + for p in remote_paths if p.startswith(prefix) and p != prefix + } + if selected: + return selected, None + + # No prefixed files for the requested agent. Either this is a legacy + # single-agent repo (bare paths ARE the requested agent's files) or an + # all-layout repo that simply lacks this agent. Named-agent paths that do + # NOT match the framework's bare workspace patterns are unambiguous + # all-layout evidence (bare-pattern matches such as qwenpaw's + # ``skills/x/SKILL.md`` would split like an agent prefix but are regular + # single-agent content). + bare_patterns = probe.resolved_patterns() + other_agents: set[str] = set() + has_bare = False + for p in remote_paths: + agent, _ = probe.split_all_path(p) + if probe.matches(p, bare_patterns): + has_bare = True + elif agent not in (None, DEFAULT_AGENT_NAME): + other_agents.add(agent) + if other_agents: + found = (['default'] if has_bare else []) + sorted(other_agents) + return None, (f"repository has no agent named '{requested}' " + f"(agents in repo: {', '.join(found)}).") + return identity, None + + def cmd_download( framework: str, repo: str, @@ -444,6 +516,7 @@ def cmd_download( # Conversion needs the full remote payload, so it stays full-download. incremental = (target or framework) == framework remote_all_paths: set[str] | None = None + requested = name or DEFAULT_AGENT_NAME try: info = client.repo_info(group, repo_n) if info is None: @@ -453,7 +526,24 @@ def cmd_download( remote_detail = client.list_repo_files_detail(group, repo_n) if not remote_detail: return _fail(f'repository {group}/{repo_n} has no files.') - remote_all_paths = {f.path for f in remote_detail} + # Restrict an all-layout repo to the requested agent's files + # (prefix stripped); identity mapping for single-agent repos. + remote_map, map_err = _remote_paths_for_agent( + framework, requested, [f.path for f in remote_detail], + local_dir) + if map_err: + return _fail(map_err) + excluded = sorted({f.path + for f in remote_detail} - set(remote_map)) + if excluded: + display.file_list( + 'Excluded', + excluded, + color=display.COLOR_SKIP, + marker='[skip]', + note=f"belongs to other agents (requested '{requested}')") + remote_detail = [f for f in remote_detail if f.path in remote_map] + remote_all_paths = {remote_map[f.path] for f in remote_detail} # Local baseline for sha comparison (raw bytes -> sha256). probe_spec = build_spec(framework, name or DEFAULT_AGENT_NAME, local_dir) @@ -463,8 +553,9 @@ def cmd_download( skipped_same: list[str] = [] to_download = [] for f in remote_detail: - if f.path in local_sha and local_sha[f.path] == f.sha256: - skipped_same.append(f.path) + rel = remote_map[f.path] + if rel in local_sha and local_sha[rel] == f.sha256: + skipped_same.append(rel) else: to_download.append(f) if skipped_same: @@ -478,17 +569,31 @@ def cmd_download( total = len(to_download) for i, f in enumerate(to_download, 1): print(f' [{i}/{total}] downloading {f.path}', flush=True) - resources[f.path] = client.download_repo_file( + resources[remote_map[f.path]] = client.download_repo_file( group, repo_n, f.path) else: paths = client.list_repo_files(group, repo_n) if not paths: return _fail(f'repository {group}/{repo_n} has no files.') + remote_map, map_err = _remote_paths_for_agent( + framework, requested, paths, local_dir) + if map_err: + return _fail(map_err) + excluded = sorted(set(paths) - set(remote_map)) + if excluded: + display.file_list( + 'Excluded', + excluded, + color=display.COLOR_SKIP, + marker='[skip]', + note=f"belongs to other agents (requested '{requested}')") + paths = [p for p in paths if p in remote_map] resources = {} total = len(paths) for i, pth in enumerate(paths, 1): print(f' [{i}/{total}] downloading {pth}', flush=True) - resources[pth] = client.download_repo_file(group, repo_n, pth) + resources[remote_map[pth]] = client.download_repo_file( + group, repo_n, pth) except APIError as e: return _fail(api_error_message(e, 'download')) except Exception as e: @@ -1105,17 +1210,22 @@ def cmd_recover( # workspaces/ parent and, because a single-agent zip stores bare (unprefixed) # paths, wrongly treat every sibling agent's files as "extra" and delete them. restore_name = name or parsed_name or ALL_AGENT_NAME - if not name: - name = parsed_name or parsed_fw spec = build_spec(framework, restore_name, local_dir) root = spec.workspace_root - # Backup current local files + # Backup current local files. The label must follow the shared backup + # naming convention ``{fw}_{name}_{date}_{time}`` (all-scope: ``{fw}_...``) + # used by convert and watch -- ``backups -f `` parses the framework + # from the filename, so an unprefixed name would make this safety backup + # invisible under the framework filter (and unusable to undo a restore). from ._sync import backup_local current_resources = spec.collect() if current_resources: - pre_restore_backup = backup_local(spec, name) + backup_label = ( + framework if restore_name == ALL_AGENT_NAME else + f'{framework}_{restore_name}') + pre_restore_backup = backup_local(spec, backup_label) print(f'Pre-restore backup: {pre_restore_backup.name}') else: print('No existing files to backup.') diff --git a/ms_agent/agent_hub/_workspace.py b/ms_agent/agent_hub/_workspace.py index 44fccaeb6..da028e55b 100644 --- a/ms_agent/agent_hub/_workspace.py +++ b/ms_agent/agent_hub/_workspace.py @@ -146,8 +146,9 @@ def scrub_json_secrets(obj) -> None: Mirrors the YAML/TOML scrubbers' vocabulary so JSON config files (ms-agent ``settings.json``) use one secret policy: - * a key matching :func:`is_secret_key` whose value is a scalar -> value set - to ``''`` (e.g. ``openai_api_key``, ``*_token``); + * a key matching :func:`is_secret_key` -> value blanked to ``''`` whatever + its type (a nested mapping under e.g. ``credentials`` is wiped wholesale + -- its inner field names may look harmless); * every value inside an ``env`` mapping -> blanked regardless of key name (an ``env`` block, e.g. under an MCP server, is a free-form secret bag). @@ -158,8 +159,7 @@ def scrub_json_secrets(obj) -> None: for key, val in obj.items(): if key == 'env' and isinstance(val, dict): obj[key] = {k: '' for k in val} - elif is_secret_key(key) and isinstance(val, - (str, int, float, bool)): + elif is_secret_key(key): obj[key] = '' else: scrub_json_secrets(val) diff --git a/ms_agent/agent_hub/frameworks/qwenpaw.py b/ms_agent/agent_hub/frameworks/qwenpaw.py index a05d3d66b..8eab61151 100644 --- a/ms_agent/agent_hub/frameworks/qwenpaw.py +++ b/ms_agent/agent_hub/frameworks/qwenpaw.py @@ -122,22 +122,49 @@ def list_agents(self) -> list[str]: # ------------------------------------------------------------------ def _strip_agent_json_secrets(self, data: dict) -> None: - """Blank per-channel + MCP-env secrets in an ``agent.json`` dict in place. + """Blank every secret in an ``agent.json`` dict in place. Shared by the inbound (download) and outbound (upload) sanitizers so a key never lands on disk from a remote agent, and a local key is never pushed to the remote repo / its git history. + + The whole JSON tree is walked recursively and any key matching + :func:`is_secret_key` is blanked wherever it lives (top-level + ``model.api_key``, ``channels.*.client_secret``, future additions...). + Two structural rules are applied on top of the vocabulary: + + * channel configs additionally blank ``_CHANNEL_LOCAL_KEYS`` + (machine-local paths such as ``db_path``); + * every ``mcp.clients.*.env`` mapping is cleared wholesale -- env var + names are arbitrary, so values there are treated as secrets even + when the name does not match the vocabulary. """ - # Strip secrets from every channel config. + + def scrub(node: Any) -> None: + if isinstance(node, dict): + for key, value in node.items(): + # Secret-named key: wipe the WHOLE value (even a nested + # mapping) -- credentials blobs must not survive because + # their inner field names happen to look harmless. + if is_secret_key(key): + node[key] = '' + elif isinstance(value, (dict, list)): + scrub(value) + elif isinstance(node, list): + for item in node: + scrub(item) + + scrub(data) + # Channel configs: also blank machine-local (non-secret-named) keys. channels = data.get('channels') if isinstance(channels, dict): for ch in channels.values(): if not isinstance(ch, dict): continue for key in list(ch.keys()): - if is_secret_key(key) or key in self._CHANNEL_LOCAL_KEYS: + if key in self._CHANNEL_LOCAL_KEYS: ch[key] = '' - # Strip MCP env secrets (API keys live under mcp.clients.*.env). + # MCP env blocks: clear every value regardless of the env var name. mcp = data.get('mcp') if isinstance(mcp, dict): clients = mcp.get('clients') diff --git a/tests/agent_hub/test_cli.py b/tests/agent_hub/test_cli.py index b390a9b69..9f2219764 100644 --- a/tests/agent_hub/test_cli.py +++ b/tests/agent_hub/test_cli.py @@ -637,6 +637,67 @@ def test_restore_all_scope_backup_uses_prefixed_paths(self, mock_cache, mock_syn self.assertEqual(self._read("bot-a", "SOUL.md"), "# Soul\nbot-a all-restored.\n") self.assertEqual(self._read("bot-b", "SOUL.md"), "# Soul\nbot-b all-restored.\n") + @mock.patch("pathlib.Path.home") + @mock.patch("ms_agent.agent_hub._sync.cache_dir") + @mock.patch("ms_agent.agent_hub._cache.cache_dir") + def test_pre_restore_backup_has_framework_prefix(self, mock_cache, mock_sync_cache, mock_home): + """The automatic pre-restore backup must follow the shared naming + convention ``{fw}_{name}_{date}_{time}.zip`` so ``backups -f `` + can find it (bug: it was named ``{name}_...`` and got parsed as + framework='paw' for name='paw_qa_01').""" + mock_cache.return_value = self.cache_dir + mock_sync_cache.return_value = self.cache_dir + mock_home.return_value = self.home + # agent whose name contains '_' -- the original mis-parse trigger. + agent_dir = self.ws / "paw_qa_01" + agent_dir.mkdir() + (agent_dir / "SOUL.md").write_text("# Soul\ncurrent state.\n") + self._make_backup( + "qwenpaw_paw_qa_01_20260702_170208", + {"SOUL.md": "# Soul\nfrom backup.\n"}, + ) + rc = cmd_recover(target="last", framework="qwenpaw", name="paw_qa_01") + self.assertEqual(rc, 0) + pre = [ + f for f in self.cache_dir.glob("*.zip") + if f.stem != "qwenpaw_paw_qa_01_20260702_170208" + ] + self.assertEqual(len(pre), 1, "exactly one pre-restore backup expected") + # named like every other backup: framework prefix + agent name. + self.assertTrue( + pre[0].name.startswith("qwenpaw_paw_qa_01_"), + f"pre-restore backup misnamed: {pre[0].name}") + # and the backups -f filter parses the right framework out of it. + from ms_agent.agent_hub._commands import _parse_backup_meta + fw, nm = _parse_backup_meta(pre[0].stem) + self.assertEqual(fw, "qwenpaw") + + @mock.patch("pathlib.Path.home") + @mock.patch("ms_agent.agent_hub._sync.cache_dir") + @mock.patch("ms_agent.agent_hub._cache.cache_dir") + def test_pre_restore_backup_all_scope_uses_framework_only(self, mock_cache, mock_sync_cache, mock_home): + """Restoring an all-scope backup names its pre-restore backup + ``{fw}_{date}_{time}.zip`` (no name segment), matching the all-scope + convention.""" + mock_cache.return_value = self.cache_dir + mock_sync_cache.return_value = self.cache_dir + mock_home.return_value = self.home + self._make_backup( + "qwenpaw_20260702_170208", + {"default/SOUL.md": "# Soul\nall restored.\n"}, + ) + rc = cmd_recover(target="qwenpaw_20260702_170208") + self.assertEqual(rc, 0) + pre = [ + f for f in self.cache_dir.glob("*.zip") + if f.stem != "qwenpaw_20260702_170208" + ] + self.assertEqual(len(pre), 1) + from ms_agent.agent_hub._commands import _parse_backup_meta + fw, nm = _parse_backup_meta(pre[0].stem) + self.assertEqual(fw, "qwenpaw") + self.assertEqual(nm, "") + # --------------------------------------------------------------------------- # Download command tests (stubbed client) @@ -673,6 +734,29 @@ def __init__(self, *args, **kwargs): _QwenpawAllStub.instances.append(self) +class _HermesAllStub(_RepoStub): + """Serves a hermes repo uploaded with ``-n all``: default agent at bare + paths + named agent under ``profiles/coder/`` (bug: download -n coder + used to write default's files into coder and drop coder's own).""" + + FRAMEWORK = "hermes" + instances = [] + STORE = { + "SOUL.md": "# default soul", + "config.yaml": "model: default-model", + "memories/2026-07-20.md": "default mem", + "skills/daily-ai-news/SKILL.md": "default skill", + "hooks/session_start.sh": "echo hook", + "optional-skills/git-helper/SKILL.md": "default opt skill", + "profiles/coder/SOUL.md": "# coder soul", + "profiles/coder/memories/2026-07-26.md": "coder mem", + "profiles/coder/skills/code-review/SKILL.md": "coder skill", + } + + def __init__(self, *args, **kwargs): + _HermesAllStub.instances.append(self) + + class TestDownload(unittest.TestCase): def setUp(self): self.tmp = tempfile.TemporaryDirectory() @@ -812,6 +896,88 @@ def test_download_all_same_framework_keeps_prefixed_paths(self): # non-spec top-level files are skipped. self.assertFalse((self.out / "README.md").exists()) + # ---- named-agent download from an all-layout repo (bug regression) ---- + + @mock.patch("ms_agent.agent_hub._commands.AgentApi", _HermesAllStub) + def test_download_named_agent_from_all_repo_takes_only_its_files(self): + """-n coder must take ONLY profiles/coder/** (prefix stripped) -- + never default's bare files.""" + rc = cmd_download( + framework="hermes", repo="hm", name="coder", + local_dir=str(self.out), + endpoint="http://s", token="tok", username="u", + ) + self.assertEqual(rc, 0) + coder = self.out / "profiles" / "coder" + # coder's own files, prefix stripped: + self.assertEqual((coder / "SOUL.md").read_text(), "# coder soul") + self.assertEqual( + (coder / "memories" / "2026-07-26.md").read_text(), "coder mem") + self.assertEqual( + (coder / "skills" / "code-review" / "SKILL.md").read_text(), + "coder skill") + # default's files must NOT leak into coder's directory: + self.assertFalse((coder / "config.yaml").exists()) + self.assertFalse((coder / "hooks").exists()) + self.assertFalse((coder / "memories" / "2026-07-20.md").exists()) + self.assertFalse((coder / "skills" / "daily-ai-news").exists()) + self.assertFalse((coder / "optional-skills").exists()) + + @mock.patch("ms_agent.agent_hub._commands.AgentApi", _HermesAllStub) + def test_download_default_from_all_repo_excludes_named_agents(self): + """Default download from an all repo: bare files only, no profiles/.""" + rc = cmd_download( + framework="hermes", repo="hm", + local_dir=str(self.out), + endpoint="http://s", token="tok", username="u", + ) + self.assertEqual(rc, 0) + self.assertEqual((self.out / "SOUL.md").read_text(), "# default soul") + self.assertTrue((self.out / "memories" / "2026-07-20.md").is_file()) + self.assertFalse((self.out / "profiles").exists()) + + @mock.patch("ms_agent.agent_hub._commands.AgentApi", _HermesAllStub) + def test_download_missing_agent_from_all_repo_fails(self): + """An all repo without the requested agent must error, not silently + fill the agent with default's content.""" + rc = cmd_download( + framework="hermes", repo="hm", name="writer", + local_dir=str(self.out), + endpoint="http://s", token="tok", username="u", + ) + self.assertEqual(rc, 1) + self.assertFalse((self.out / "profiles" / "writer").exists()) + + @mock.patch("ms_agent.agent_hub._commands.AgentApi", _QwenpawAllStub) + def test_download_named_agent_from_qwenpaw_all_repo(self): + """Same family bug on qwenpaw: -n bot-a takes only bot-a/**.""" + rc = cmd_download( + framework="qwenpaw", repo="qw", name="bot-a", + local_dir=str(self.out), + endpoint="http://s", token="tok", username="u", + ) + self.assertEqual(rc, 0) + ws = self.out / "workspaces" / "bot-a" + self.assertEqual((ws / "SOUL.md").read_text(), "# bot-a soul") + self.assertEqual((ws / "AGENTS.md").read_text(), "# bot-a agents") + # default's files must not leak into bot-a's workspace: + self.assertFalse( + (ws / "workspaces").exists(), + "no nested workspaces dir expected") + self.assertNotEqual((ws / "AGENTS.md").read_text(), "# default agents") + + @mock.patch("ms_agent.agent_hub._commands.AgentApi", _DownloadStub) + def test_download_single_agent_repo_with_name_keeps_legacy_behavior(self): + """A legacy bare (single-agent) repo downloaded with -n still + writes the bare files into that agent's directory.""" + rc = cmd_download( + framework="nanobot", repo="nano", name="myagent", + local_dir=str(self.out), + endpoint="http://s", token="tok", username="u", + ) + self.assertEqual(rc, 0) + self.assertEqual((self.out / "SOUL.md").read_text(), "soul") + # --------------------------------------------------------------------------- # Convert command tests diff --git a/tests/agent_hub/test_workspace.py b/tests/agent_hub/test_workspace.py index 3fa7ea2d9..ae2ee9641 100644 --- a/tests/agent_hub/test_workspace.py +++ b/tests/agent_hub/test_workspace.py @@ -1,5 +1,6 @@ # Copyright (c) Alibaba, Inc. and its affiliates. """Sub-agent-aware workspace spec collection tests.""" +import json import tempfile import unittest from pathlib import Path @@ -257,5 +258,53 @@ def test_defaults_to_qwenpaw_when_neither_exists(self): self.assertEqual(self._root_name([]), ".copaw") +class TestQwenpawAgentJsonSecrets(unittest.TestCase): + """agent.json sanitize must blank secrets ANYWHERE in the JSON tree. + + Regression for the leak where only ``channels.*`` and + ``mcp.clients.*.env`` were walked, so the top-level ``model.api_key`` + (the primary LLM credential) went to the public repo in plaintext. + """ + + SRC = json.dumps({ + "model": {"api_key": "sk-SECRET-1", "model": "qwen-max"}, + "channels": { + "dingtalk": { + "client_secret": "SECRET-2", + "client_id": "keep-me", + "db_path": "/local/db.sqlite", + } + }, + "mcp": {"clients": {"c1": {"env": {"ANY_NAME": "SECRET-3"}}}}, + "plugins": [{"token": "SECRET-4", "name": "p1"}], + }) + + def setUp(self): + self.spec = QwenpawWorkspace(agent_name="paw_qa_01") + + def _assert_scrubbed(self, out: dict): + # secrets blanked at every depth: + self.assertEqual(out["model"]["api_key"], "") + self.assertEqual(out["channels"]["dingtalk"]["client_secret"], "") + self.assertEqual(out["mcp"]["clients"]["c1"]["env"], {"ANY_NAME": ""}) + self.assertEqual(out["plugins"][0]["token"], "") + # machine-local channel key blanked: + self.assertEqual(out["channels"]["dingtalk"]["db_path"], "") + # non-secret fields preserved for post-migration debugging: + self.assertEqual(out["model"]["model"], "qwen-max") + self.assertEqual(out["channels"]["dingtalk"]["client_id"], "keep-me") + self.assertEqual(out["plugins"][0]["name"], "p1") + + def test_outbound_upload_scrubs_whole_tree(self): + out = json.loads(self.spec._strip_outbound_agent_json(self.SRC)) + self._assert_scrubbed(out) + self.assertNotIn("SECRET", json.dumps(out)) + + def test_inbound_download_scrubs_whole_tree(self): + out = json.loads(self.spec._sanitize_agent_json("paw_qa_01", self.SRC)) + self._assert_scrubbed(out) + self.assertNotIn("SECRET", json.dumps(out)) + + if __name__ == "__main__": unittest.main()