From 97cce5d634fb6022c09af21c41324d2eac4719d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Wed, 16 Sep 2026 18:14:28 +0800 Subject: [PATCH 01/10] [Feature] Add ms agent install with a gated plugin loader `ms agent install -r owner/name` downloads an agent into its framework's local workspace by delegating to a framework plugin published as a model repository. The hub gains no framework knowledge. It resolves which plugin to use, fetches that package, verifies it, and hands the agent id over; where files land, how the agent is registered and what the framework needs afterwards stay the plugin's decisions. `download`/`upload`/`list` remain raw transfer, so the boundary the `ms agent` docstring draws is kept -- install delegates rather than knows. Importing a plugin executes code this distribution did not ship, so the path is gated three times in increasing order of cost: - Explicit source. `--plugin-repo` or MODELSCOPE_AGENT_PLUGIN_REPO; there is no built-in default owner, because who publishes the plugin is a deployment decision and a silent fallback would let a typo install from somewhere nobody chose. - Owner allow-list, checked before any download so an untrusted source is refused without touching the network. Matching is case-sensitive: the existing `_env_csv_frozenset` upper-cases its items, which would let a look-alike account pass, hence the new `_env_csv_frozenset_exact`. - Trust opt-in, checked after the manifest is verified so the refusal can show exactly what would run -- repository, revision, version, entry module, frameworks, declared operations and manifest digest. Never persisted: "allow this code to run" is not a preference worth remembering for the user. Between the last two gates, `plugin.json`'s `content_sha256` is checked against every file on disk, and a package with no digest is refused -- without it the import would be unconditional code execution. The hub's own listing is not used as the integrity source: it has been observed returning a git blob SHA-1 in a `sha256` field. The entry operation is negotiated, not hard-coded: `install` is preferred, `download` accepted as a fallback, and `capabilities()['operations']` is authoritative when declared, so a plugin shipping a name without implementing it is not selected and the hub needs no re-release when a plugin grows a richer entry point. Arguments are narrowed to the plugin's signature so new keywords do not break older hubs. The plugin's own exit code passes through unchanged -- the install layer gives 3/4/5/6 distinct meanings and collapsing them to 1 would discard the only machine-readable signal a caller has. Two bugs were caught by running against a published plugin rather than by review: the hub injects `.gitattributes` into every repository, which is never in the author's manifest and made every real package fail verification; and a `"/" in repo` check let `/noname` and `owner/` through to a real API call. Both are fixed and pinned by tests, the latter also asserting no network is reached. 53 tests, mock-only per CI's MODELSCOPE_RUN_REMOTE_TESTS=false. Gate logic is tested once at function level and once through the CLI for exit-code mapping, with no assertion duplicated across the two. Verified end to end against a real published plugin and a live agent repository: the trust gate refuses before import and lists what it would run; with the opt-in the plugin is fetched, verified, imported and installs 8 files; an unlisted owner is refused before download; and the allow-list override takes effect. --- README.md | 75 +++- src/modelscope_hub/agent/__init__.py | 31 +- src/modelscope_hub/agent/_plugin.py | 433 +++++++++++++++++++++ src/modelscope_hub/cli/agent.py | 168 +++++++- src/modelscope_hub/constants.py | 69 ++++ tests/cli/test_agent_install.py | 283 ++++++++++++++ tests/test_agent_plugin.py | 552 +++++++++++++++++++++++++++ 7 files changed, 1596 insertions(+), 15 deletions(-) create mode 100644 src/modelscope_hub/agent/_plugin.py create mode 100644 tests/cli/test_agent_install.py create mode 100644 tests/test_agent_plugin.py diff --git a/README.md b/README.md index 2c5f2ea..cd70f88 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,10 @@ The official Python SDK & CLI for [ModelScope Hub](https://modelscope.cn) — do ## News +**Unreleased** +- **Feature**: `ms-hub agent install -r owner/name` resolves a framework plugin, fetches it from a model repository, and delegates the install to it — plus the `modelscope_hub.agent.install_agent` SDK entry and its underlying `resolve_plugin_repo` / `assert_trusted_owner` / `fetch_plugin` / `verify_manifest` / `load_plugin` / `select_operation` steps. The hub gains no framework knowledge: where files land and how an agent is registered stay the plugin's decisions. +- **Quality**: loading a plugin executes code this package did not ship, so it is gated by an explicit plugin source (no built-in default owner), a case-sensitive owner allow-list checked before any download, and a `--trust-remote-code` opt-in that is never persisted. `plugin.json`'s `content_sha256` is verified against every file before import, because the hub's own listing has been observed reporting a git blob SHA-1 in a `sha256` field. + **v0.4.0** (2026-09-01) - **Feature**: complete OpenAPI coverage for Agent-IDP, MCP, and Studios — Agent Ed25519 identities, OIDC discovery/JWKS and signed JWT issuance (`HubApi`, `ms-hub agent-idp`); Studio lists, variables and configuration options; hosted MCP discovery; protected visibility and runtime metadata; read-only tokens can log in and rejected writes name the required tier. Agent private JWKs are only written to an explicitly requested owner-only file. - **Fix**: Studio compat calls no longer leak connection options or API tokens, or drop cover images; errors distinguish permission, quota and conflicts; anonymous Studio info and log pagination work @@ -640,13 +644,19 @@ ms-hub cache clear --repo-id my-org/old-model --repo-type model --yes ### `ms-hub agent` -Low-level raw file transfer for remote agent repositories: `download`, `upload`, `list`. This command transfers files as-is, with **no framework awareness**. +Remote agent repositories: raw file transfer (`download`, `upload`, `list`) and plugin-driven `install`. ```bash ms-hub agent download -r user/my-agent --local-dir ./my-agent # download raw files ms-hub agent upload -r user/my-agent --local-dir ./my-agent # upload raw +ms-hub agent install -r user/my-agent \ + --plugin-repo modelscope/agent-hub-plugin --trust-remote-code # install into the framework ``` +`download` / `upload` / `list` transfer files as-is, with **no framework awareness**. + +`install` is different: it does not know any framework's file layout either. It resolves *which* plugin to use, fetches that plugin from a model repository, verifies it against its own manifest, and hands the agent id over — the plugin decides where files land, how the agent is registered, and what the framework needs afterwards. See [`ms-hub agent install`](#ms-hub-agent-install) for the security model. + > **Framework-aware operations** (cross-framework `convert`, `watch`/bidirectional sync, `status`, `backups`, `restore`, `stop`) live in **[modelscope-agent](https://github.com/modelscope/ms-agent)** — use `ms-agent agent ...` instead. For example, to download and convert in one step: `ms-agent agent download -f qoder -r user/my-agent --target-framework qwenpaw`.
@@ -683,6 +693,66 @@ ms-hub agent upload -r user/my-agent --local-dir ./my-agent --dry-run | `--revision REV` | no | Repository revision (default: `master`) | | `--dry-run` | no | List files that would be uploaded without uploading | +#### `ms-hub agent install` + +Download an agent and hand it to its **framework plugin**, which installs it into the local workspace. + +```bash +ms-hub agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin --trust-remote-code +ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --local-dir ~/ws +``` + +| Option | Required | Description | +|--------|----------|-------------| +| `-r, --repo REPO` | yes | Agent repository to install (`owner/name`) | +| `--plugin-repo OWNER/NAME` | no | Plugin model repository. Falls back to `$MODELSCOPE_AGENT_PLUGIN_REPO`; **there is no built-in default owner** | +| `--plugin-revision REV` | no | Plugin revision (default: `master`; pin a tag for reproducible installs) | +| `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | +| `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | +| `--framework FW` | no | Override the plugin's framework detection | +| `--local-dir DIR` | no | Override the framework's local root | +| `--dry-run` | no | Report what would happen, change nothing | +| `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | + +Exit codes: `0` success, `2` a gate refused or the command line is wrong, and otherwise **the plugin's own code** — the install layer gives `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed) and `6` (framework mismatch) distinct meanings, and collapsing them to `1` would discard the only machine-readable signal a caller has. + +##### Security model + +Importing a plugin executes code this package did not ship, so the path is gated three times, in increasing order of cost: + +1. **Explicit source.** The plugin repository must be named by `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO`. There is no default owner: who publishes the plugin is a deployment decision, and silently falling back to one would let a typo install from somewhere nobody chose. +2. **Owner allow-list.** Checked *before* anything is downloaded, so an untrusted source is refused without touching the network. Matching is **case-sensitive** — a lower-casing comparison would accept a look-alike account, which is exactly what an allow-list must stop. + + ```bash + # Default: mushenL,modelscope. Override (comma-separated, case-sensitive): + export MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS="modelscope,my-org" + ``` + + A refusal names the owners currently trusted and prints this variable, so the fix is discoverable from the error alone. +3. **Trust opt-in.** Checked after the manifest is verified, so the refusal can show exactly what is about to run — repository, revision, version, entry module, frameworks, declared operations and the manifest digest. The opt-in is a flag or an environment variable and is **never persisted**: "allow this code to run" is not a preference worth remembering on the user's behalf. + +Between gates 2 and 3, `plugin.json`'s `content_sha256` is checked against every file on disk. A package whose contents do not match the digest published with them is refused, as is one with no digest at all — without it the import would be unconditional code execution. The hub's own file listing is deliberately *not* used for this: it has been observed reporting a git blob SHA-1 in a `sha256` field, which makes it unreliable as an integrity source. + +Downloading a plugin does not execute anything, so fetching an untrusted package is safe; only the import is gated. + +##### Python API + +```python +from modelscope_hub.agent import install_agent + +outcome = install_agent( + "user/my-agent", + plugin_repo="modelscope/agent-hub-plugin", + plugin_revision="v0.2.0", + trust_remote_code=True, +) +print(outcome.ok, outcome.operation, outcome.exit_code, outcome.error) +``` + +The lower-level steps are exported too (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `select_operation`) for callers that want to inspect a plugin without running it. + +The plugin's entry operation is negotiated rather than hard-coded: `install` is preferred, `download` accepted as a fallback, and `capabilities()['operations']` is authoritative when the plugin declares it — so a plugin that ships a name without implementing it is not selected, and the hub does not need re-releasing when a plugin grows a richer entry point. Arguments are narrowed to what the plugin's signature accepts, so a plugin adding new keywords does not break older hubs. +
### `ms-hub agent-idp` @@ -839,6 +909,9 @@ Token is persisted locally after `ms-hub login` and auto-loaded in subsequent se | `MODELSCOPE_ENDPOINT` | `https://modelscope.cn` | API endpoint URL | | `MODELSCOPE_CACHE` | `~/.cache/modelscope` | Local cache directory | | `MODELSCOPE_HOME` | `~/.modelscope` | SDK config directory | +| `MODELSCOPE_AGENT_PLUGIN_REPO` | — | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; no built-in default | +| `MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS` | `mushenL,modelscope` | Comma-separated owners allowed to provide the agent plugin (case-sensitive) | +| `MODELSCOPE_AGENT_TRUST_REMOTE_CODE` | `false` | Let `ms-hub agent install` execute plugin code without `--trust-remote-code` | | `MODELSCOPE_PREFER_AI_SITE` | `false` | Prefer `modelscope.ai` over `modelscope.cn` | **Network:** diff --git a/src/modelscope_hub/agent/__init__.py b/src/modelscope_hub/agent/__init__.py index 025d267..66902c4 100644 --- a/src/modelscope_hub/agent/__init__.py +++ b/src/modelscope_hub/agent/__init__.py @@ -1,9 +1,11 @@ # Copyright (c) Alibaba, Inc. and its affiliates. """Agent repository transport SDK for ModelScope Hub. -This package provides only the low-level HTTP client for agent repositories. -Framework-aware workspace management (frameworks, conversion, sync, watch, -backups) lives in **modelscope-agent** (``ms_agent.agent_hub``). +This package provides the low-level HTTP client for agent repositories, plus the +plugin loader behind ``ms agent install``. Neither carries framework knowledge: +workspace management (frameworks, conversion, sync, watch, backups) lives in +**modelscope-agent** (``ms_agent.agent_hub``), and the plugin loader only fetches +and invokes a plugin that a publisher ships as a model repository. Public API ---------- @@ -14,9 +16,22 @@ - ``agent_visibility_label`` / ``agent_last_modified`` -- read renamed agent metadata fields from an API item, tolerating both JSON spellings (snake_case and PascalCase) and legacy keys. +- :func:`install_agent` -- install an agent through its framework plugin. """ from ._api import AgentApi, RemoteFileInfo, agent_last_modified, agent_visibility_label, is_lfs_file +from ._plugin import ( + ENTRY_OPERATIONS, + InstallOutcome, + PluginSpec, + assert_trusted_owner, + fetch_plugin, + install_agent, + load_plugin, + resolve_plugin_repo, + select_operation, + verify_manifest, +) __all__ = [ "AgentApi", @@ -24,4 +39,14 @@ "is_lfs_file", "agent_visibility_label", "agent_last_modified", + "install_agent", + "PluginSpec", + "InstallOutcome", + "ENTRY_OPERATIONS", + "resolve_plugin_repo", + "assert_trusted_owner", + "fetch_plugin", + "verify_manifest", + "load_plugin", + "select_operation", ] diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py new file mode 100644 index 0000000..1f81206 --- /dev/null +++ b/src/modelscope_hub/agent/_plugin.py @@ -0,0 +1,433 @@ +# Copyright (c) Alibaba, Inc. and its affiliates. +"""Locate, fetch, verify and run an agent plugin. + +``ms agent install`` resolves which plugin to use, downloads it from a model +repository, verifies it, and hands the agent id to the plugin's entry point. No +framework knowledge lives here: where files land and how an agent is registered +are the plugin's decisions. + +Integrity comes from ``plugin.json``'s ``content_sha256``, not from the hub's own +file listing -- that listing has been observed reporting a git blob SHA-1 in a +``sha256`` field. +""" + +from __future__ import annotations + +import hashlib +import importlib +import inspect +import json +import os +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from .. import constants +from ..errors import InvalidParameter, NotSupportedError + +MANIFEST_NAME = "plugin.json" + +#: Negotiated rather than hard-coded, so a plugin growing a richer entry point +#: does not require re-releasing the hub. +ENTRY_OPERATIONS: tuple[str, ...] = ("install", "download") + + +@dataclass(frozen=True, slots=True) +class PluginSpec: + """A plugin that has been downloaded and verified, but not yet imported.""" + + repo_id: str + owner: str + name: str + revision: str + directory: Path + manifest: dict[str, Any] + entry_module: str + + @property + def version(self) -> str: + return str(self.manifest.get("version", "unknown")) + + def describe(self) -> str: + frameworks = self.manifest.get("frameworks") or [] + operations = self.manifest.get("api") or [] + digest = _manifest_digest(self.manifest) + return ( + f" plugin : {self.repo_id}\n" + f" revision : {self.revision}\n" + f" version : {self.version}\n" + f" entry : {self.entry_module}\n" + f" frameworks : {', '.join(map(str, frameworks)) or '-'}\n" + f" operations : {', '.join(map(str, operations)) or '-'}\n" + f" directory : {self.directory}\n" + f" manifest : {len(self.manifest.get('content_sha256') or {})} file(s), " + f"sha256 {digest}" + ) + + +@dataclass(frozen=True, slots=True) +class InstallOutcome: + ok: bool + error: str | None = None + operation: str | None = None + plugin: PluginSpec | None = None + result: Any = None + exit_code: int = 0 + + +def _manifest_digest(manifest: dict[str, Any]) -> str: + entries = manifest.get("content_sha256") or {} + if not isinstance(entries, dict) or not entries: + return "unavailable" + blob = "\n".join(f"{k}:{entries[k]}" for k in sorted(entries)) + return hashlib.sha256(blob.encode("utf-8")).hexdigest()[:16] + + +def _split_repo_id(repo_id: str, *, label: str) -> tuple[str, str]: + """Split ``owner/name`` or raise, rejecting empty halves. + + ``"/" in repo_id`` is not enough: ``/name`` and ``owner/`` both contain a + slash but name no repository, and letting either through costs a real request + for somebody else's path. + """ + owner, _, name = repo_id.partition("/") + if not owner or not name: + raise InvalidParameter(f"{label} {repo_id!r} must be in 'owner/name' form.") + return owner, name + + +def resolve_plugin_repo(explicit: str | None = None) -> str: + """Return the plugin repository id, or raise if none was configured. + + Resolution is the argument, then + :data:`~modelscope_hub.constants.ENV_AGENT_PLUGIN_REPO`, and stops there. + There is deliberately no built-in default owner: who publishes the plugin is a + deployment decision, and a silent fallback would let a typo install from + somewhere nobody chose. + """ + repo_id = (explicit or "").strip() + if not repo_id: + repo_id = (os.environ.get(constants.ENV_AGENT_PLUGIN_REPO) or "").strip() + if not repo_id: + error = InvalidParameter( + "no agent plugin repository configured. Pass --plugin-repo owner/name, " + f"or set {constants.ENV_AGENT_PLUGIN_REPO}=owner/name." + ) + error.suggestion = ( + "The plugin is published as a ModelScope model repository. Its owner is a " + "deployment choice, so modelscope-hub does not assume one." + ) + raise error + _split_repo_id(repo_id, label="agent plugin repository") + return repo_id + + +def assert_trusted_owner(repo_id: str) -> tuple[str, str]: + """Split *repo_id* and require its owner on the allow-list. + + Comparison is case-sensitive: owners are identifiers, so normalising case + would let ``mushenl`` pass a list that only trusts ``mushenL``. + """ + owner, name = _split_repo_id(repo_id, label="agent plugin repository") + trusted = constants.AGENT_PLUGIN_TRUSTED_OWNERS + if owner not in trusted: + error = InvalidParameter( + f"owner {owner!r} is not allowed to provide the agent plugin. " + f"Trusted owners: {', '.join(sorted(trusted)) or '(none)'}." + ) + error.suggestion = ( + f"Extend the allow-list with {constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS}" + "=owner1,owner2 (comma-separated, case-sensitive), then retry." + ) + raise error + return owner, name + + +def fetch_plugin( + repo_id: str, + *, + revision: str | None = None, + token: str | None = None, + endpoint: str | None = None, + cache_dir: str | None = None, +) -> Path: + """Download the plugin package and return its directory. + + Transfer executes nothing, so fetching an untrusted plugin is safe; only the + import is gated, by :func:`require_trust`. + """ + from ..compat import snapshot_download + + rev = revision or constants.DEFAULT_AGENT_PLUGIN_REVISION + try: + directory = snapshot_download( + repo_id, + repo_type="model", + revision=rev, + token=token, + endpoint=endpoint, + cache_dir=cache_dir, + ) + except Exception as exc: + # ``snapshot_download`` re-raises hub errors as + # ``requests.exceptions.HTTPError``, so the original type is not a + # reliable discriminator; keep the cause chain instead. + raise NotSupportedError(f"failed to download agent plugin {repo_id}@{rev}: {exc}") from exc + path = Path(directory) + if not path.is_dir(): + raise NotSupportedError(f"agent plugin {repo_id}@{rev} did not resolve to a directory: {path}") + return path + + +#: Present in a downloaded plugin directory but not plugin content, so their +#: absence from ``content_sha256`` is expected. ``.gitattributes`` is injected by +#: the hub for LFS tracking and therefore never appears in an author's manifest. +NOT_PLUGIN_CONTENT: frozenset[str] = frozenset({MANIFEST_NAME, ".gitattributes"}) + + +def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: + """Check the downloaded package against its own ``plugin.json``. + + Strict on purpose: this is what makes the subsequent import something other + than unconditional code execution. + """ + manifest_path = directory / MANIFEST_NAME + if not manifest_path.is_file(): + raise NotSupportedError(f"{repo_id} is not an agent plugin: no {MANIFEST_NAME} at the repository root.") + try: + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise NotSupportedError(f"{repo_id}: cannot read {MANIFEST_NAME}: {exc}") from exc + if not isinstance(manifest, dict): + raise NotSupportedError(f"{repo_id}: {MANIFEST_NAME} must be a JSON object.") + + entry_module = manifest.get("entry_module") + if not isinstance(entry_module, str) or not entry_module: + raise NotSupportedError(f"{repo_id}: {MANIFEST_NAME} has no 'entry_module'; cannot know what to import.") + + recorded = manifest.get("content_sha256") + if not isinstance(recorded, dict) or not recorded: + raise NotSupportedError( + f"{repo_id}: {MANIFEST_NAME} has no 'content_sha256', so the plugin's integrity " + "cannot be verified. Refusing to load it." + ) + + missing, mismatched, unexpected = [], [], [] + for rel, expected in sorted(recorded.items()): + target = directory / rel + if not target.is_file(): + missing.append(rel) + continue + actual = hashlib.sha256(target.read_bytes()).hexdigest() + if actual != expected: + mismatched.append(rel) + recorded_set = set(recorded) + for path in sorted(p for p in directory.rglob("*") if p.is_file()): + relative = path.relative_to(directory) + rel = relative.as_posix() + if rel in NOT_PLUGIN_CONTENT or "__pycache__" in relative.parts: + continue + if rel not in recorded_set: + unexpected.append(rel) + + problems = [] + if missing: + problems.append(f"missing {len(missing)} file(s): {', '.join(missing[:5])}") + if mismatched: + problems.append(f"sha256 mismatch for {len(mismatched)} file(s): {', '.join(mismatched[:5])}") + if unexpected: + problems.append(f"not listed in the manifest: {', '.join(unexpected[:5])}") + if problems: + raise NotSupportedError(f"{repo_id}: plugin integrity check failed -- " + "; ".join(problems)) + return manifest + + +def require_trust(spec: PluginSpec, *, trust_remote_code: bool) -> None: + """Refuse to import the plugin unless execution was opted into. + + The refusal lists what *would* run so the decision is informed. The opt-in is + a flag or an environment variable and is never persisted -- "allow this code + to run" is not a preference worth remembering on the user's behalf. + """ + if trust_remote_code or constants.AGENT_TRUST_REMOTE_CODE: + return + raise NotSupportedError( + "refusing to execute plugin code without an explicit opt-in. The plugin " + "resolved to:\n" + spec.describe() + "\n\n" + "Re-run with --trust-remote-code to import and run it, or set " + f"{constants.ENV_AGENT_TRUST_REMOTE_CODE}=1." + ) + + +def load_plugin(spec: PluginSpec) -> Any: + """Import the plugin's entry module from its downloaded directory. + + The directory goes at the *front* of ``sys.path`` so the fetched revision + wins over any same-named installed distribution. + """ + root = str(spec.directory) + if root not in sys.path: + sys.path.insert(0, root) + try: + return importlib.import_module(spec.entry_module) + except ImportError: + if root in sys.path: + sys.path.remove(root) + raise + + +def select_operation(module: Any) -> tuple[str, Any]: + """Pick the entry operation the plugin actually supports. + + ``capabilities()`` is authoritative when present, so a plugin that ships a + name without implementing it is not selected. + """ + declared = None + capabilities = getattr(module, "capabilities", None) + if callable(capabilities): + try: + payload = capabilities() + declared = set((payload or {}).get("operations") or ()) + except Exception: + declared = None + + for name in ENTRY_OPERATIONS: + func = getattr(module, name, None) + if not callable(func): + continue + if declared is not None and name not in declared: + continue + return name, func + + available = ", ".join(sorted(declared)) if declared else "none reported" + raise NotSupportedError( + f"plugin {module.__name__} exposes none of {', '.join(ENTRY_OPERATIONS)}. Its capabilities are: {available}." + ) + + +def _accepted_kwargs(func: Any, candidates: dict[str, Any]) -> dict[str, Any]: + """Narrow *candidates* to what *func* accepts. + + A plugin's entry signature is its own contract and may grow keywords the hub + knows nothing about; filtering keeps older hubs working instead of raising + ``TypeError``. A function taking ``**kwargs`` gets everything. + """ + try: + parameters = inspect.signature(func).parameters + except (TypeError, ValueError): + return dict(candidates) + if any(p.kind is inspect.Parameter.VAR_KEYWORD for p in parameters.values()): + return dict(candidates) + return {key: value for key, value in candidates.items() if key in parameters} + + +def install_agent( + repo: str, + *, + name: str | None = None, + framework: str | None = None, + local_dir: str | None = None, + dry_run: bool = False, + yes: bool = False, + force: bool = False, + quiet: bool = False, + plugin_repo: str | None = None, + plugin_revision: str | None = None, + trust_remote_code: bool = False, + endpoint: str | None = None, + token: str | None = None, + cache_dir: str | None = None, +) -> InstallOutcome: + """Download *repo*'s agent into the local framework workspace via a plugin. + + *repo* is passed through uninterpreted beyond requiring ``owner/name``. + Plugin failures come back as data (``ok`` False); the three gates + (:func:`resolve_plugin_repo`, :func:`assert_trusted_owner`, + :func:`require_trust`) raise instead, so the CLI can map a misconfigured + command line to exit 2 and keep it distinct from a failed install. + """ + if not repo or not repo.strip(): + raise InvalidParameter("--repo is required, in 'owner/name' form.") + repo = repo.strip() + _split_repo_id(repo, label="agent repository") + + plugin_repo_id = resolve_plugin_repo(plugin_repo) + owner, plugin_name = assert_trusted_owner(plugin_repo_id) + + directory = fetch_plugin( + plugin_repo_id, + revision=plugin_revision, + token=token, + endpoint=endpoint, + cache_dir=cache_dir, + ) + manifest = verify_manifest(directory, plugin_repo_id) + spec = PluginSpec( + repo_id=plugin_repo_id, + owner=owner, + name=plugin_name, + revision=plugin_revision or constants.DEFAULT_AGENT_PLUGIN_REVISION, + directory=directory, + manifest=manifest, + entry_module=str(manifest["entry_module"]), + ) + require_trust(spec, trust_remote_code=trust_remote_code) + + try: + module = load_plugin(spec) + operation, func = select_operation(module) + except NotSupportedError: + raise + except Exception as exc: + return InstallOutcome( + ok=False, + error=f"failed to load plugin {plugin_repo_id}: {exc.__class__.__name__}: {exc}", + plugin=spec, + exit_code=1, + ) + + candidates: dict[str, Any] = { + "repo": repo, + "name": name, + "framework": framework, + "source_framework": framework, + "local_dir": local_dir, + "dry_run": dry_run, + "yes": yes, + "force": force, + "quiet": quiet, + "endpoint": endpoint, + "token": token, + } + # Unset optionals are dropped so the plugin applies its own defaults; a False + # boolean is kept because that is a decision the caller made. + provided = {key: value for key, value in candidates.items() if value is not None} + try: + result = func(**_accepted_kwargs(func, provided)) + except Exception as exc: + return InstallOutcome( + ok=False, + error=f"plugin {operation}() failed: {exc.__class__.__name__}: {exc}", + operation=operation, + plugin=spec, + exit_code=1, + ) + + ok = bool(getattr(result, "ok", True)) + if ok: + return InstallOutcome(ok=True, operation=operation, plugin=spec, result=result) + + error = getattr(result, "error", None) or f"plugin {operation}() reported failure" + try: + exit_code = int(getattr(result, "exit_code", 0) or 0) + except (TypeError, ValueError): + exit_code = 0 + return InstallOutcome( + ok=False, + error=error, + operation=operation, + plugin=spec, + result=result, + exit_code=exit_code or 1, + ) diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index 1f0621a..0c27711 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -1,11 +1,15 @@ # Copyright (c) Alibaba, Inc. and its affiliates. -"""``ms agent`` command -- low-level raw file transfer for agent repositories. +"""``ms agent`` command -- agent repository transfer and plugin-driven install. -This is the *slim* Hub CLI. It supports only ``download``/``upload``/``list`` -for raw file transfer to and from remote agent repositories; Agent-IDP identity, -Ed25519-key, and token operations live in ``ms agent-idp``. Framework-aware -operations (convert, watch/sync, status, backups, restore, stop) live in -**modelscope-agent** -- use ``ms-agent agent ...``. +``download`` / ``upload`` / ``list`` are the *slim* Hub CLI: raw file transfer to +and from remote agent repositories, with no framework awareness. Agent-IDP +identity, Ed25519-key and token operations live in ``ms agent-idp``. +Framework-aware operations (convert, watch/sync, status, backups, restore, stop) +live in **modelscope-agent** -- use ``ms-agent agent ...``. + +``install`` is the exception, and it keeps that boundary by delegating rather +than knowing: it fetches a framework plugin and hands the agent id over, so no +framework file layout enters this distribution. """ from __future__ import annotations @@ -15,10 +19,11 @@ from argparse import RawDescriptionHelpFormatter from pathlib import Path -from ..agent import AgentApi, agent_last_modified, agent_visibility_label, is_lfs_file +from ..agent import AgentApi, agent_last_modified, agent_visibility_label, install_agent, is_lfs_file from ..constants import Visibility from ..errors import APIError -from .base import CLICommand, SubParsers +from .base import CLICommand, SubParsers, info, success +from .compat import add_subcmd_token_endpoint _CONVERT_HINT = ( "This command transfers raw files only. For framework-aware conversion, " @@ -250,11 +255,78 @@ def _cmd_upload(repo, local_dir, revision, dry_run, *, endpoint, token, username return 0 +def _cmd_install( + repo, + *, + name, + framework, + local_dir, + dry_run, + yes, + force, + quiet, + plugin_repo, + plugin_revision, + trust_remote_code, + endpoint, + token, +) -> int: + """Install an agent through its framework plugin. + + The gates in :func:`install_agent` raise rather than return a code, and are + deliberately not caught here so ``run_cmd`` maps them to exit 2: a + misconfigured command line is a different failure from a failed install. + + The plugin's exit code passes through unchanged. The install layer gives + 3/4/5/6 distinct meanings (already exists, refused to overwrite, install or + self-check failed, framework mismatch); collapsing them to 1 would discard + the only machine-readable signal a caller has. + """ + outcome = install_agent( + repo, + name=name, + framework=framework, + local_dir=local_dir, + dry_run=dry_run, + yes=yes, + force=force, + quiet=quiet, + plugin_repo=plugin_repo, + plugin_revision=plugin_revision, + trust_remote_code=trust_remote_code, + endpoint=endpoint, + token=token, + ) + + plugin = outcome.plugin + if plugin is not None and not quiet: + info(f"plugin: {plugin.repo_id}@{plugin.revision} (version {plugin.version})") + if outcome.operation: + info(f"entry : {plugin.entry_module}.{outcome.operation}()") + + if not outcome.ok: + _fail(outcome.error or "install failed") + return outcome.exit_code or 1 + if outcome.exit_code: + # Reported success but a non-zero code; trust the code. + return outcome.exit_code + + if not quiet: + result = outcome.result + written = getattr(result, "files_written", None) + root = getattr(result, "root", None) + if written is not None and root is not None: + success(f"Installed {repo}: {len(written)} file(s) under {root}") + else: + success(f"Installed {repo}") + return 0 + + # --------------------------------------------------------------------------- # CLI command # --------------------------------------------------------------------------- class AgentCommand(CLICommand): - """Raw agent-repository file transfer: download, upload, list.""" + """Agent repositories: raw file transfer, plus plugin-driven install.""" @staticmethod def register(subparsers: SubParsers) -> None: @@ -263,19 +335,29 @@ def register(subparsers: SubParsers) -> None: " download -r REPO [--local-dir DIR] [--revision REV]\n" " upload -r REPO [--local-dir DIR] [--revision REV] [--dry-run]\n" " list [--owner OWNER] [--page N] [--page-size N]\n" + " install -r REPO --plugin-repo OWNER/NAME --trust-remote-code\n" + " [-n NAME] [--framework FW] [--local-dir DIR] [--plugin-revision REV]\n" + " [--dry-run] [-y] [--force] [-q]\n" "\n" "note:\n" f" {_CONVERT_HINT}\n" + " `install` delegates to a framework plugin; see `ms agent install --help`.\n" "\n" "examples:\n" " ms agent download -r user/my-agent --local-dir ./my-agent\n" " ms agent upload -r user/my-agent --local-dir ./my-agent\n" " ms agent list --owner user\n" + " ms agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin \\\n" + " --trust-remote-code\n" ) agent_parser = subparsers.add_parser( "agent", - help="Transfer raw agent repository files (download, upload, list).", - description="Low-level raw file transfer for remote agent repositories. " + _CONVERT_HINT, + help="Agent repositories: raw file transfer (download, upload, list) and install.", + description=( + "Work with remote agent repositories. `download`/`upload`/`list` are low-level raw " + "file transfer. " + _CONVERT_HINT + " `install` instead resolves a framework plugin, " + "fetches it from a model repository, and delegates the install to it." + ), epilog=_epilog, formatter_class=RawDescriptionHelpFormatter, ) @@ -342,6 +424,54 @@ def register(subparsers: SubParsers) -> None: "--page-size", dest="page_size", type=int, default=10, help="Number of items per page (default: 10)" ) + # ---- install ---- + p_install = agent_sub.add_parser( + "install", + help="Install an agent into its framework via the agent plugin", + formatter_class=RawDescriptionHelpFormatter, + description=( + "Download an agent repository and hand it to the framework plugin, which installs it " + "into the local workspace.\n\n" + "Loading a plugin imports code this package did not ship, so the plugin source must be " + "named explicitly (--plugin-repo or MODELSCOPE_AGENT_PLUGIN_REPO), its owner must be on " + "the allow-list (MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS), and execution requires " + "--trust-remote-code." + ), + ) + p_install.add_argument( + "-r", + "--repo", + required=True, + help="Agent repository to install, in owner/name format (e.g. user/my-agent)", + ) + p_install.add_argument( + "-n", "--name", default=None, help="Sub-agent name to install (default: the plugin's choice)" + ) + p_install.add_argument("--framework", default=None, help="Override framework detection") + p_install.add_argument("--local-dir", default=None, help="Override the framework's local root") + p_install.add_argument( + "--plugin-repo", + default=None, + help="Plugin model repository, owner/name (default: $MODELSCOPE_AGENT_PLUGIN_REPO; there is " + "no built-in default owner)", + ) + p_install.add_argument( + "--plugin-revision", + default=None, + help="Plugin revision to fetch (default: master; pin a tag for reproducible installs)", + ) + p_install.add_argument( + "--trust-remote-code", + action="store_true", + help="Allow the downloaded plugin to be imported and executed. Without it (or " + "$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1) the command reports what it would run and stops.", + ) + p_install.add_argument("--dry-run", action="store_true", help="Report what would happen, change nothing") + p_install.add_argument("-y", "--yes", action="store_true", help="Answer the plugin's prompts yes") + p_install.add_argument("--force", action="store_true", help="Let the plugin overwrite an existing agent") + p_install.add_argument("-q", "--quiet", action="store_true", help="Suppress the plugin's progress output") + add_subcmd_token_endpoint(p_install) + def execute(self) -> None: args = self.args action = args.agent_command @@ -396,6 +526,22 @@ def execute(self) -> None: endpoint=endpoint, token=token, ) + elif action == "install": + rc = _cmd_install( + args.repo, + name=args.name, + framework=args.framework, + local_dir=args.local_dir, + dry_run=args.dry_run, + yes=args.yes, + force=args.force, + quiet=args.quiet, + plugin_repo=args.plugin_repo, + plugin_revision=args.plugin_revision, + trust_remote_code=args.trust_remote_code, + endpoint=endpoint, + token=token, + ) else: print(f"Unknown agent action: {action}") rc = 1 diff --git a/src/modelscope_hub/constants.py b/src/modelscope_hub/constants.py index 1a9f970..9d2806c 100644 --- a/src/modelscope_hub/constants.py +++ b/src/modelscope_hub/constants.py @@ -953,13 +953,79 @@ def get_upload_ignore_file_pattern() -> str | None: USER_INFO_FILE_NAME: str = "user" +# --------------------------------------------------------------------------- +# Agent plugin loading (``ms agent install``) +# +# These constrain where the plugin that ``ms agent install`` imports may come +# from. The security model they serve is documented in +# :mod:`modelscope_hub.agent._plugin`. +# --------------------------------------------------------------------------- +ENV_AGENT_PLUGIN_REPO: str = "MODELSCOPE_AGENT_PLUGIN_REPO" +ENV_AGENT_PLUGIN_TRUSTED_OWNERS: str = "MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS" +ENV_AGENT_TRUST_REMOTE_CODE: str = "MODELSCOPE_AGENT_TRUST_REMOTE_CODE" + +DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS: str = "mushenL,modelscope" +DEFAULT_AGENT_PLUGIN_REVISION: str = "master" + +_AGENT_TRUSTED_OWNERS_DESCRIPTION = "Comma-separated owners allowed to provide the agent plugin (case-sensitive)" + + +def _env_csv_frozenset_exact( + name: str, + default: str, + description: str, + category: str, + *deprecated_names: str, +) -> frozenset[str]: + """Read a comma-separated set from the environment, **preserving case**. + + Do not "simplify" this into :func:`_env_csv_frozenset`: that one upper-cases + every item, which would let an owner differing only in case pass an + allow-list check -- the look-alike an allow-list exists to stop. + """ + _env_register(name, default, description, category, deprecated_names=deprecated_names) + raw = _env(name, *deprecated_names) or default + return frozenset(item.strip() for item in raw.split(",") if item.strip()) + + +_env_register( + ENV_AGENT_PLUGIN_REPO, + "-", + "Model repository id ('owner/name') of the agent plugin used by 'ms agent install'", + "Core", +) +_env_register( + ENV_AGENT_TRUST_REMOTE_CODE, + "false", + "Let 'ms agent install' execute plugin code without --trust-remote-code", + "Core", +) + +AGENT_PLUGIN_TRUSTED_OWNERS: frozenset[str] = _env_csv_frozenset_exact( + ENV_AGENT_PLUGIN_TRUSTED_OWNERS, + DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS, + _AGENT_TRUSTED_OWNERS_DESCRIPTION, + "Core", +) +AGENT_TRUST_REMOTE_CODE: bool = _env_bool( + ENV_AGENT_TRUST_REMOTE_CODE, + False, + "Let 'ms agent install' execute plugin code without --trust-remote-code", + "Core", +) + + __all__ = [ + "AGENT_PLUGIN_TRUSTED_OWNERS", + "AGENT_TRUST_REMOTE_CODE", "API_CONNECT_TIMEOUT", "API_MAX_RETRIES", "API_TIMEOUT", "CATEGORY_ORDER", "CONFIG_DIR_NAME", "DATASET_LFS_SUFFIX", + "DEFAULT_AGENT_PLUGIN_REVISION", + "DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS", "DEFAULT_CACHE_DIR_NAME", "DEFAULT_CREDENTIALS_PATH", "DEFAULT_DATASET_REVISION", @@ -979,6 +1045,9 @@ def get_upload_ignore_file_pattern() -> str | None: "DOWNLOAD_PART_SIZE", "DOWNLOAD_RETRY_TIMES", "DOWNLOAD_TIMEOUT", + "ENV_AGENT_PLUGIN_REPO", + "ENV_AGENT_PLUGIN_TRUSTED_OWNERS", + "ENV_AGENT_TRUST_REMOTE_CODE", "ENV_FILE_LOCK", "ENV_CACHE", "ENV_INTRA_CLOUD_ACCELERATION", diff --git a/tests/cli/test_agent_install.py b/tests/cli/test_agent_install.py new file mode 100644 index 0000000..fe64211 --- /dev/null +++ b/tests/cli/test_agent_install.py @@ -0,0 +1,283 @@ +# Copyright (c) Alibaba, Inc. and its affiliates. +"""CLI tests for ``ms agent install``. + +These cover what the CLI layer owns: argument wiring, gate failures mapping to +exit 2, the plugin's exit code passing through unchanged, and output routing +(including ``-q``). The gates' own logic is tested in ``tests/test_agent_plugin.py`` +and is not repeated here. + +Mock-only: CI runs with ``MODELSCOPE_RUN_REMOTE_TESTS=false``. The trust gate +fires after the package is on disk, so it stubs ``fetch_plugin`` and points at a +real plugin tree -- manifest verification and the refusal message are genuine. +""" + +from __future__ import annotations + +import hashlib +import json +import sys +import textwrap +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from modelscope_hub import constants +from modelscope_hub.agent import InstallOutcome, PluginSpec, _plugin +from modelscope_hub.cli.agent import AgentCommand + +from .conftest import run_cli + +TRUSTED = "mushenL" +PLUGIN_REPO = f"{TRUSTED}/agent-hub-plugin" +AGENT_REPO = "owner/my-agent" + +ENTRY = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("install",)} + + def install(repo, **kwargs): + return type("R", (), {"ok": True, "error": None, "exit_code": 0, + "files_written": ("SOUL.md",), "root": "/tmp/ws"})() + """ +).lstrip() + +ALL_OPTIONS = [ + "agent", + "install", + "-r", + AGENT_REPO, + "-n", + "sub", + "--framework", + "qwenpaw", + "--local-dir", + "/tmp/ws", + "--plugin-repo", + PLUGIN_REPO, + "--plugin-revision", + "v1.0.0", + "--trust-remote-code", + "--dry-run", + "-y", + "--force", + "-q", +] + +MINIMAL = ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO, "--trust-remote-code"] + + +def build_plugin(root: Path, *, entry_module: str = "cli_fake_plugin") -> Path: + directory = root / "plugin" + directory.mkdir(parents=True, exist_ok=True) + entry = directory / f"{entry_module}.py" + entry.write_text(ENTRY, encoding="utf-8") + manifest = { + "name": "agent-hub-plugin", + "version": "9.9.9", + "entry_module": entry_module, + "frameworks": ["qwenpaw", "ms-agent"], + "api": ["install"], + "content_sha256": {f"{entry_module}.py": hashlib.sha256(entry.read_bytes()).hexdigest()}, + } + (directory / "plugin.json").write_text(json.dumps(manifest, indent=2), encoding="utf-8") + return directory + + +def outcome(**kwargs) -> InstallOutcome: + return InstallOutcome(**kwargs) + + +def spec(revision: str = "v1.0.0") -> PluginSpec: + return PluginSpec( + repo_id=PLUGIN_REPO, + owner=TRUSTED, + name="agent-hub-plugin", + revision=revision, + directory=Path("/tmp/nowhere"), + manifest={"version": "9.9.9"}, + entry_module="mod", + ) + + +@pytest.fixture(autouse=True) +def _clean_env(monkeypatch): + monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({TRUSTED, "modelscope"})) + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) + monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) + monkeypatch.delenv(constants.ENV_AGENT_TRUST_REMOTE_CODE, raising=False) + yield + sys.modules.pop("cli_fake_plugin", None) + + +@pytest.fixture +def stub_sdk(monkeypatch): + """Replace the SDK entry point and capture what the CLI forwarded.""" + seen: dict[str, Any] = {} + + def fake_install(repo, **kwargs): + seen.update(repo=repo, **kwargs) + return InstallOutcome(ok=True, operation="install") + + monkeypatch.setattr("modelscope_hub.cli.agent.install_agent", fake_install) + return seen + + +# --------------------------------------------------------------------------- +# argument wiring +# --------------------------------------------------------------------------- +def test_parser_wires_install(parser): + args = parser.parse_args(ALL_OPTIONS) + assert args._command is AgentCommand + assert args.agent_command == "install" + assert (args.repo, args.name, args.framework, args.local_dir) == ( + AGENT_REPO, + "sub", + "qwenpaw", + "/tmp/ws", + ) + assert (args.plugin_repo, args.plugin_revision) == (PLUGIN_REPO, "v1.0.0") + assert args.trust_remote_code is True + assert args.dry_run and args.yes and args.force and args.quiet + + +def test_parser_install_defaults(parser): + args = parser.parse_args(["agent", "install", "-r", AGENT_REPO]) + assert args.trust_remote_code is False + assert args.plugin_repo is None + assert args.plugin_revision is None + assert args.dry_run is False + assert args.yes is False + assert args.force is False + assert args.quiet is False + assert args.name is None + assert args.framework is None + assert args.local_dir is None + + +def test_forwards_every_option_to_the_sdk(stub_sdk): + code, _, err = run_cli(ALL_OPTIONS) + assert code == 0, err + assert stub_sdk["repo"] == AGENT_REPO + assert stub_sdk["name"] == "sub" + assert stub_sdk["framework"] == "qwenpaw" + assert stub_sdk["local_dir"] == "/tmp/ws" + assert stub_sdk["plugin_repo"] == PLUGIN_REPO + assert stub_sdk["plugin_revision"] == "v1.0.0" + assert stub_sdk["trust_remote_code"] is True + assert stub_sdk["dry_run"] and stub_sdk["yes"] + assert stub_sdk["force"] and stub_sdk["quiet"] + + +def test_credentials_reach_the_sdk(stub_sdk): + code, _, err = run_cli(MINIMAL, token="tok-123", endpoint="https://pre.modelscope.cn") + assert code == 0, err + assert stub_sdk["token"] == "tok-123" + assert stub_sdk["endpoint"] == "https://pre.modelscope.cn" + + +def test_install_does_not_resolve_a_username(monkeypatch, stub_sdk): + """``install`` always receives ``owner/name``, so it must not pay for a whoami + round trip -- nor fail when that endpoint is unavailable.""" + from modelscope_hub import _openapi + + client = MagicMock(side_effect=AssertionError("whoami must not be called")) + monkeypatch.setattr(_openapi, "OpenAPIClient", client) + + code, _, err = run_cli(MINIMAL, token="tok-123") + assert code == 0, err + client.assert_not_called() + + +# --------------------------------------------------------------------------- +# gates: exit code 2, and where the message lands +# --------------------------------------------------------------------------- +def test_missing_plugin_repo_exits_2_with_guidance(): + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO]) + assert code == 2 + assert "--plugin-repo" in err + # run_cmd prints the message to stderr and the suggestion to stdout. + assert constants.ENV_AGENT_PLUGIN_REPO in out + err + + +def test_untrusted_plugin_owner_exits_2(): + code, out, err = run_cli( + ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", "evil/plugin", "--trust-remote-code"] + ) + assert code == 2 + assert "evil" in err + assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in out + err + + +def test_trust_gate_refuses_and_explains(monkeypatch, tmp_path): + """Without the opt-in the command stops before importing, and says what it + would have run.""" + directory = build_plugin(tmp_path) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO]) + assert code == 2 + combined = out + err + for expected in ("--trust-remote-code", PLUGIN_REPO, "9.9.9", "cli_fake_plugin"): + assert expected in combined + + +def test_trust_gate_can_be_satisfied_by_env(monkeypatch, tmp_path): + directory = build_plugin(tmp_path) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", True) + + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO]) + assert code == 0, err + assert "Installed" in out + + +# --------------------------------------------------------------------------- +# reporting and exit-code mapping +# --------------------------------------------------------------------------- +def test_reports_plugin_and_entry(monkeypatch): + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=True, operation="install", plugin=spec()), + ) + code, out, _ = run_cli(MINIMAL) + assert code == 0 + assert f"{PLUGIN_REPO}@v1.0.0" in out + assert "9.9.9" in out + assert "mod.install()" in out + + +def test_quiet_suppresses_all_hub_output(monkeypatch): + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=True, operation="install", plugin=spec()), + ) + code, out, _ = run_cli([*MINIMAL, "-q"]) + assert code == 0 + assert out == "" + + +@pytest.mark.parametrize("plugin_code", [1, 3, 6]) +def test_plugin_exit_code_is_passed_through(monkeypatch, plugin_code): + """The install layer gives 3/4/5/6 distinct meanings (already exists, refused + to overwrite, install or self-check failed, framework mismatch); collapsing + them to 1 would discard the only machine-readable signal a caller has.""" + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=False, error="nope", exit_code=plugin_code), + ) + code, _, err = run_cli(MINIMAL) + assert code == plugin_code + assert "nope" in err + + +def test_failure_without_an_exit_code_becomes_1(monkeypatch): + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=False, error="download failed"), + ) + code, _, err = run_cli(MINIMAL) + assert code == 1 + assert "download failed" in err diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py new file mode 100644 index 0000000..88ceb01 --- /dev/null +++ b/tests/test_agent_plugin.py @@ -0,0 +1,552 @@ +# Copyright (c) Alibaba, Inc. and its affiliates. +"""Unit tests for the agent plugin loader (``modelscope_hub.agent._plugin``). + +Happy paths build a real plugin package on disk -- manifest, entry module and all +-- so verification, import and operation negotiation are exercised rather than +mocked. Only the network hop (``snapshot_download``) is stubbed, per the rule +that CI runs with ``MODELSCOPE_RUN_REMOTE_TESTS=false``. + +The gates themselves are tested here at function level; that they are *wired +into* ``install_agent`` and map to the right exit codes is covered by +``tests/cli/test_agent_install.py``, so it is not repeated at both levels. +""" + +from __future__ import annotations + +import hashlib +import json +import sys +import textwrap +from pathlib import Path +from typing import Any + +import pytest + +from modelscope_hub import constants +from modelscope_hub.agent import _plugin +from modelscope_hub.errors import InvalidParameter, NotSupportedError + +TRUSTED = "mushenL" +PLUGIN_REPO = f"{TRUSTED}/agent-hub-plugin" + + +def _sha256(path: Path) -> str: + return hashlib.sha256(path.read_bytes()).hexdigest() + + +ENTRY_SOURCE = textwrap.dedent( + """ + from dataclasses import dataclass, field + + + @dataclass(frozen=True) + class Result: + ok: bool = True + error: str | None = None + files_written: tuple = field(default_factory=tuple) + root: str = "/tmp/ws" + exit_code: int = 0 + + + CALLS = [] + + + def capabilities(): + return {"operations": ("install", "download"), "version": "9.9.9"} + + + def install(repo, **kwargs): + CALLS.append(("install", repo, kwargs)) + return Result(files_written=("SOUL.md",)) + + + def download(repo, **kwargs): + CALLS.append(("download", repo, kwargs)) + return Result(files_written=("SOUL.md",)) + """ +).lstrip() + + +def make_plugin( + root: Path, + *, + dirname: str = "plugin", + entry_module: str = "fake_plugin", + version: str = "9.9.9", + operations: tuple[str, ...] = ("install", "download"), + entry_source: str | None = None, + extra_files: dict[str, str] | None = None, + omit_hashes: bool = False, + omit_entry: bool = False, +) -> Path: + """Write a plugin package tree and return its directory.""" + directory = root / dirname + directory.mkdir(parents=True, exist_ok=True) + + source = entry_source if entry_source is not None else ENTRY_SOURCE + (directory / f"{entry_module}.py").write_text(source, encoding="utf-8") + for rel, content in (extra_files or {}).items(): + target = directory / rel + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + + manifest: dict[str, Any] = { + "name": "agent-hub-plugin", + "version": version, + "kind": "agent-hub-plugin", + "frameworks": ["qwenpaw", "ms-agent"], + "api": list(operations), + } + if not omit_entry: + manifest["entry_module"] = entry_module + if not omit_hashes: + files = sorted(p for p in directory.rglob("*") if p.is_file()) + manifest["content_sha256"] = {p.relative_to(directory).as_posix(): _sha256(p) for p in files} + (directory / "plugin.json").write_text(json.dumps(manifest, indent=2), encoding="utf-8") + return directory + + +def spec_for(directory: Path, *, manifest: dict | None = None) -> _plugin.PluginSpec: + resolved = manifest + if resolved is None: + resolved = json.loads((directory / "plugin.json").read_text(encoding="utf-8")) + return _plugin.PluginSpec( + repo_id=PLUGIN_REPO, + owner=TRUSTED, + name="agent-hub-plugin", + revision="master", + directory=directory, + manifest=resolved, + entry_module=str(resolved.get("entry_module", "fake_plugin")), + ) + + +def load_entry(directory: Path, module_name: str): + """Import a generated plugin entry module the way ``load_plugin`` does.""" + return _plugin.load_plugin(spec_for(directory, manifest={"entry_module": module_name})) + + +# --------------------------------------------------------------------------- +# resolve_plugin_repo +# --------------------------------------------------------------------------- +def test_resolve_plugin_repo_resolution_order(monkeypatch): + """Argument beats environment, and there is no third fallback.""" + monkeypatch.setenv(constants.ENV_AGENT_PLUGIN_REPO, "env-owner/env-plugin") + assert _plugin.resolve_plugin_repo("arg-owner/arg-plugin") == "arg-owner/arg-plugin" + assert _plugin.resolve_plugin_repo(None) == "env-owner/env-plugin" + assert _plugin.resolve_plugin_repo(" ") == "env-owner/env-plugin" + + monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) + with pytest.raises(InvalidParameter) as excinfo: + _plugin.resolve_plugin_repo(None) + # A missing default must be actionable from the message alone. + assert "--plugin-repo" in str(excinfo.value) + assert constants.ENV_AGENT_PLUGIN_REPO in str(excinfo.value) + + +@pytest.mark.parametrize("value", ["noslash", "/noname", "owner/"]) +def test_resolve_plugin_repo_requires_owner_slash_name(monkeypatch, value): + monkeypatch.setenv(constants.ENV_AGENT_PLUGIN_REPO, value) + with pytest.raises(InvalidParameter): + _plugin.resolve_plugin_repo(None) + + +# --------------------------------------------------------------------------- +# assert_trusted_owner +# --------------------------------------------------------------------------- +@pytest.fixture +def allow_list(monkeypatch): + monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({"mushenL", "modelscope"})) + + +def test_assert_trusted_owner_accepts_allow_listed(allow_list): + assert _plugin.assert_trusted_owner("mushenL/agent-hub-plugin") == ("mushenL", "agent-hub-plugin") + assert _plugin.assert_trusted_owner("modelscope/agent-hub-plugin") == ("modelscope", "agent-hub-plugin") + + +@pytest.mark.parametrize("owner", ["mushenl", "evil", "mushenL-x"]) +def test_assert_trusted_owner_rejects_others(allow_list, owner): + """Case-sensitive on purpose: normalising case would accept a look-alike.""" + with pytest.raises(InvalidParameter) as excinfo: + _plugin.assert_trusted_owner(f"{owner}/agent-hub-plugin") + assert owner in str(excinfo.value) + assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in excinfo.value.suggestion + + +def test_assert_trusted_owner_empty_list_blocks_everything(monkeypatch): + monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset()) + with pytest.raises(InvalidParameter): + _plugin.assert_trusted_owner("mushenL/agent-hub-plugin") + + +def test_env_csv_helper_preserves_case(monkeypatch): + """The only coverage of the environment parsing behind the allow-list; the + gate tests above patch the resolved constant instead.""" + monkeypatch.setenv("MODELSCOPE_TEST_OWNERS", " mushenL , modelscope ,, ") + got = constants._env_csv_frozenset_exact("MODELSCOPE_TEST_OWNERS", "fallback", "test", "Core") + assert got == frozenset({"mushenL", "modelscope"}) + + +# --------------------------------------------------------------------------- +# verify_manifest +# --------------------------------------------------------------------------- +def test_verify_manifest_accepts_a_consistent_package(tmp_path): + directory = make_plugin(tmp_path) + manifest = _plugin.verify_manifest(directory, PLUGIN_REPO) + assert manifest["entry_module"] == "fake_plugin" + assert manifest["version"] == "9.9.9" + + +@pytest.mark.parametrize( + "case,expected", + [ + ("absent", "not an agent plugin"), + ("no-entry", "entry_module"), + # Without a digest the import would be unconditional code execution. + ("no-hashes", "content_sha256"), + ], +) +def test_verify_manifest_rejects_an_incomplete_manifest(tmp_path, case, expected): + if case == "absent": + directory = tmp_path / "empty" + directory.mkdir() + elif case == "no-entry": + directory = make_plugin(tmp_path, omit_entry=True) + else: + directory = make_plugin(tmp_path, omit_hashes=True) + with pytest.raises(NotSupportedError) as excinfo: + _plugin.verify_manifest(directory, PLUGIN_REPO) + assert expected in str(excinfo.value) + + +@pytest.mark.parametrize( + "case,expected", + [ + ("tampered", "sha256 mismatch"), + ("missing", "missing"), + ("unlisted", "not listed in the manifest"), + ], +) +def test_verify_manifest_rejects_inconsistent_content(tmp_path, case, expected): + directory = make_plugin(tmp_path, extra_files={"pkg/mod.py": "x = 1\n"}) + if case == "tampered": + target = directory / "fake_plugin.py" + target.write_text(target.read_text(encoding="utf-8") + "\n# tampered\n", encoding="utf-8") + elif case == "missing": + (directory / "pkg" / "mod.py").unlink() + else: + (directory / "surprise.py").write_text("import os\n", encoding="utf-8") + with pytest.raises(NotSupportedError) as excinfo: + _plugin.verify_manifest(directory, PLUGIN_REPO) + assert expected in str(excinfo.value) + + +def test_verify_manifest_exempts_non_plugin_files(tmp_path): + """``.gitattributes`` is injected by the hub into every repository and so is + never in an author's manifest; refusing it made every real package + unverifiable. ``plugin.json`` cannot hash itself and ``__pycache__`` is + written locally by a previous import. + + The exemption is exact -- a genuinely unlisted file is still refused. + """ + directory = make_plugin(tmp_path) + (directory / ".gitattributes").write_text("*.bin filter=lfs\n", encoding="utf-8") + cache = directory / "__pycache__" + cache.mkdir() + (cache / "fake_plugin.cpython-311.pyc").write_bytes(b"\x00\x01") + + manifest = _plugin.verify_manifest(directory, PLUGIN_REPO) + listed = manifest["content_sha256"] + assert "plugin.json" not in listed + assert ".gitattributes" not in listed + assert not any("__pycache__" in key for key in listed) + + (directory / "surprise.py").write_text("import os\n", encoding="utf-8") + with pytest.raises(NotSupportedError) as excinfo: + _plugin.verify_manifest(directory, PLUGIN_REPO) + assert "surprise.py" in str(excinfo.value) + assert ".gitattributes" not in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# require_trust +# --------------------------------------------------------------------------- +def test_require_trust_blocks_without_opt_in(tmp_path, monkeypatch): + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) + with pytest.raises(NotSupportedError) as excinfo: + _plugin.require_trust(spec_for(make_plugin(tmp_path)), trust_remote_code=False) + # The refusal must say what would have run, not just "no". + message = str(excinfo.value) + for expected in (PLUGIN_REPO, "9.9.9", "fake_plugin", "--trust-remote-code"): + assert expected in message + + +@pytest.mark.parametrize("via", ["flag", "env"]) +def test_require_trust_allows_with_flag_or_env(tmp_path, monkeypatch, via): + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", via == "env") + _plugin.require_trust( + spec_for(make_plugin(tmp_path)), + trust_remote_code=(via == "flag"), + ) + + +# --------------------------------------------------------------------------- +# fetch_plugin +# --------------------------------------------------------------------------- +def test_fetch_plugin_requests_a_model_repo(monkeypatch, tmp_path): + seen: list[dict[str, Any]] = [] + + def fake_snapshot_download(repo_id, **kwargs): + seen.append({"repo_id": repo_id, **kwargs}) + return str(tmp_path) + + import modelscope_hub.compat as compat + + monkeypatch.setattr(compat, "snapshot_download", fake_snapshot_download) + + got = _plugin.fetch_plugin(PLUGIN_REPO, revision="v1.2.3", token="tok", endpoint="https://ep") + assert got == tmp_path + assert seen[0]["repo_id"] == PLUGIN_REPO + assert seen[0]["repo_type"] == "model" + assert seen[0]["revision"] == "v1.2.3" + assert (seen[0]["token"], seen[0]["endpoint"]) == ("tok", "https://ep") + + _plugin.fetch_plugin(PLUGIN_REPO) + assert seen[1]["revision"] == constants.DEFAULT_AGENT_PLUGIN_REVISION + + +def test_fetch_plugin_wraps_download_failure(monkeypatch): + """``snapshot_download`` re-raises hub errors as ``requests.HTTPError``, so + the original type is not a reliable discriminator.""" + + def boom(repo_id, **kwargs): + raise RuntimeError("404 not found") + + import modelscope_hub.compat as compat + + monkeypatch.setattr(compat, "snapshot_download", boom) + with pytest.raises(NotSupportedError) as excinfo: + _plugin.fetch_plugin(PLUGIN_REPO) + assert PLUGIN_REPO in str(excinfo.value) + assert isinstance(excinfo.value.__cause__, RuntimeError) + + +# --------------------------------------------------------------------------- +# load_plugin / select_operation +# --------------------------------------------------------------------------- +def test_load_plugin_imports_the_declared_entry_module(tmp_path): + directory = make_plugin(tmp_path, entry_module="entry_a") + module = _plugin.load_plugin(spec_for(directory)) + assert module.__name__ == "entry_a" + assert str(directory) in sys.path + sys.path.remove(str(directory)) + sys.modules.pop("entry_a", None) + + +def test_load_plugin_restores_sys_path_on_failure(tmp_path): + directory = tmp_path / "plugin" + directory.mkdir() + spec = _plugin.PluginSpec( + repo_id=PLUGIN_REPO, + owner=TRUSTED, + name="p", + revision="master", + directory=directory, + manifest={}, + entry_module="does_not_exist_xyz", + ) + before = list(sys.path) + with pytest.raises(ImportError): + _plugin.load_plugin(spec) + assert sys.path == before + + +def test_select_operation_prefers_install(tmp_path): + module = load_entry(make_plugin(tmp_path, entry_module="sel_install"), "sel_install") + name, func = _plugin.select_operation(module) + assert (name, func) == ("install", module.install) + + +def test_select_operation_falls_back_to_download(tmp_path): + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("download",)} + + def install(repo, **kwargs): + raise AssertionError("must not be chosen") + + def download(repo, **kwargs): + return "downloaded" + """ + ).lstrip() + directory = make_plugin(tmp_path, entry_module="sel_download", entry_source=source) + module = load_entry(directory, "sel_download") + name, func = _plugin.select_operation(module) + assert (name, func) == ("download", module.download) + + +def test_select_operation_rejects_an_undeclared_name(tmp_path): + """A plugin shipping a name without declaring it in ``capabilities()`` is not + trusted to have implemented it.""" + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ()} + + def install(repo, **kwargs): + raise AssertionError("must not be chosen") + """ + ).lstrip() + directory = make_plugin(tmp_path, entry_module="sel_none", entry_source=source) + module = load_entry(directory, "sel_none") + with pytest.raises(NotSupportedError) as excinfo: + _plugin.select_operation(module) + assert "sel_none" in str(excinfo.value) + + +def test_select_operation_without_capabilities_uses_presence(tmp_path): + source = "def download(repo, **kwargs):\n return 'ok'\n" + directory = make_plugin(tmp_path, entry_module="sel_nocaps", entry_source=source) + assert _plugin.select_operation(load_entry(directory, "sel_nocaps"))[0] == "download" + + +def test_accepted_kwargs_narrowing(): + def positional(repo, name=None): + return repo, name + + assert _plugin._accepted_kwargs(positional, {"repo": "a/b", "name": "x", "force": True}) == { + "repo": "a/b", + "name": "x", + } + + def variadic(**kwargs): + return kwargs + + payload = {"repo": "a/b", "anything": 1} + assert _plugin._accepted_kwargs(variadic, payload) == payload + + +# --------------------------------------------------------------------------- +# install_agent +# --------------------------------------------------------------------------- +@pytest.fixture +def wired(monkeypatch, tmp_path): + """Point ``install_agent`` at a real plugin tree with the network stubbed.""" + monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({TRUSTED})) + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) + monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) + directory = make_plugin(tmp_path, entry_module="e2e_plugin") + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + yield directory + sys.modules.pop("e2e_plugin", None) + + +def test_install_agent_happy_path_and_option_forwarding(wired): + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + assert outcome.ok, outcome.error + assert (outcome.operation, outcome.exit_code) == ("install", 0) + assert outcome.plugin.repo_id == PLUGIN_REPO + assert outcome.plugin.version == "9.9.9" + + # Importable only now: load_plugin put the directory on sys.path. + import e2e_plugin + + assert e2e_plugin.CALLS[-1][:2] == ("install", "owner/my-agent") + # Unset optionals are dropped so the plugin applies its own defaults, but a + # False boolean is a decision the caller made and is forwarded. + assert e2e_plugin.CALLS[-1][2] == { + "dry_run": False, + "yes": False, + "force": False, + "quiet": False, + } + + _plugin.install_agent( + "owner/my-agent", + name="sub", + local_dir="/tmp/ws", + dry_run=True, + force=True, + endpoint="https://pre.modelscope.cn", + token="tok", + plugin_repo=PLUGIN_REPO, + trust_remote_code=True, + ) + forwarded = e2e_plugin.CALLS[-1][2] + assert forwarded["name"] == "sub" + assert forwarded["local_dir"] == "/tmp/ws" + assert forwarded["dry_run"] is True + assert forwarded["force"] is True + assert forwarded["endpoint"] == "https://pre.modelscope.cn" + assert forwarded["token"] == "tok" + + +@pytest.mark.parametrize("repo", ["", " ", "no-slash", "/noname", "owner/"]) +def test_install_agent_validates_the_agent_repo(wired, monkeypatch, repo): + """``/noname`` and ``owner/`` contain a slash but name no repository; they + must be rejected before any network call.""" + + def no_network(*args, **kwargs): + raise AssertionError(f"network reached for malformed repo id {repo!r}") + + monkeypatch.setattr(_plugin, "fetch_plugin", no_network) + import modelscope_hub.compat as compat + + monkeypatch.setattr(compat, "snapshot_download", no_network) + + with pytest.raises(InvalidParameter): + _plugin.install_agent(repo, plugin_repo=PLUGIN_REPO, trust_remote_code=True) + + +def test_install_agent_reports_plugin_failure(wired, monkeypatch): + source = textwrap.dedent( + """ + from dataclasses import dataclass + + + @dataclass(frozen=True) + class Result: + ok: bool = False + error: str = "framework not installed" + exit_code: int = 2 + + + def capabilities(): + return {"operations": ("install",)} + + + def install(repo, **kwargs): + return Result() + """ + ).lstrip() + directory = make_plugin(wired.parent, dirname="fail_plugin", entry_module="fail_plugin", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + assert not outcome.ok + assert outcome.error == "framework not installed" + # The install layer's own codes (3/4/5/6) carry meaning and must survive. + assert outcome.exit_code == 2 + sys.modules.pop("fail_plugin", None) + + +def test_install_agent_contains_a_plugin_exception(wired, monkeypatch): + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("install",)} + + + def install(repo, **kwargs): + raise RuntimeError("boom") + """ + ).lstrip() + directory = make_plugin(wired.parent, dirname="boom_plugin", entry_module="boom_plugin", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + assert not outcome.ok + assert "boom" in outcome.error + assert outcome.exit_code == 1 + sys.modules.pop("boom_plugin", None) From dc2f5559f9b9a1a2b1072dabea2ccba5f997abf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Thu, 17 Sep 2026 10:23:05 +0800 Subject: [PATCH 02/10] [Fix] Negotiate fetch_raw so a transport-only plugin can be driven Joint-testing the loader against agent-hub-plugin 0.2.0 showed the command cannot reach a plugin that only transports bytes, for two separate reasons. The plugin merged its per-framework variants into one package and replaced download with fetch_raw, which writes a repository's bytes into a directory the caller names and never into a framework workspace -- a workspace holds the user's own credentials (agent.json channels, settings.json providers, mcp.json env blocks), and installing over it destroyed them silently. ENTRY_OPERATIONS named only install and download, and capabilities() is authoritative, so selection failed outright: [E3023] plugin agent_hub_core exposes none of install, download. Its capabilities are: fetch_raw, list_backups, restore. Adding the name is not enough on its own. fetch_raw's dest is keyword-only and required, and install_agent never supplied one, so the call died on "missing 1 required keyword-only argument: 'dest'" and fetched nothing. The plugin has no default by design: the only sensible-looking default is a workspace. Resolving the destination is therefore the hub's job, and it is the one piece of placement knowledge this module now holds -- --local-dir when given, otherwise MODELSCOPE_CACHE/agent/agent-staging/---, matching the plugin's own staging_dir() so there are not two conventions. dest joins the candidate kwargs, so narrowing still drops it for operations that do not declare it and 0.1.x plugins are unaffected. install keeps precedence, so the install layer's entry point taking over needs no hub release. The success message now follows the negotiated operation: fetch_raw reports "Fetched N file(s) to " rather than claiming an install that did not happen. Help text said the plugin "installs it into the local workspace", which is no longer something the hub can promise. The old path was not merely different, it was lossy: installing a real ms-agent repository through the 0.1.1 qwenpaw plugin dropped 5 of 10 files as "not part of the qwenpaw workspace spec" -- settings.json, mcp.json and skills.json among them, exactly the inputs the install layer deep-merges. fetch_raw delivers all 10, so this also unblocks the layer that runs next. Verified end to end rather than by unit test alone: the real CLI against the real 0.2.0 artifact and the real pre-release repository ms-agent/stock_data_agent selects agent_hub_core.fetch_raw(), exits 0, and writes all 10 files to --local-dir; sha256 snapshots of ~/.qwenpaw and ~/.ms_agent are byte-identical before and after and no backup is created. The plugin download hop is separately proven over the real network with the published 0.1.1 package (29 files, manifest verified, trust gate passed). Only that hop is stubbed for 0.2.0, since snapshot_download verifies against the remote even on a warm cache and the single package is not published yet. 1020 tests pass. The one failure, test_compat_constants_completeness, is pre-existing: it reproduces identically at the base commit in a clean worktree and compares UPLOAD_REACT_ROUND3_FILE_DELAY against the installed modelscope 1.39.1 (30 vs 5), which this change does not touch. --- README.md | 14 +- src/modelscope_hub/agent/__init__.py | 7 +- src/modelscope_hub/agent/_plugin.py | 49 ++++++- src/modelscope_hub/cli/agent.py | 23 +++- tests/cli/test_agent_install.py | 21 +++ tests/test_agent_plugin.py | 193 ++++++++++++++++++++++++++- 6 files changed, 289 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index cd70f88..70301e1 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ The official Python SDK & CLI for [ModelScope Hub](https://modelscope.cn) — do ## News **Unreleased** -- **Feature**: `ms-hub agent install -r owner/name` resolves a framework plugin, fetches it from a model repository, and delegates the install to it — plus the `modelscope_hub.agent.install_agent` SDK entry and its underlying `resolve_plugin_repo` / `assert_trusted_owner` / `fetch_plugin` / `verify_manifest` / `load_plugin` / `select_operation` steps. The hub gains no framework knowledge: where files land and how an agent is registered stay the plugin's decisions. +- **Feature**: `ms-hub agent install -r owner/name` resolves a framework plugin, fetches it from a model repository, and delegates to the entry operation the plugin declares — plus the `modelscope_hub.agent.install_agent` SDK entry and its underlying `resolve_plugin_repo` / `assert_trusted_owner` / `fetch_plugin` / `verify_manifest` / `load_plugin` / `select_operation` / `default_staging_dir` steps. The hub gains no framework knowledge: how an agent is registered and what its workspace looks like stay the plugin's decisions, the one exception being the destination directory, which the hub resolves for a plugin that only transports bytes. - **Quality**: loading a plugin executes code this package did not ship, so it is gated by an explicit plugin source (no built-in default owner), a case-sensitive owner allow-list checked before any download, and a `--trust-remote-code` opt-in that is never persisted. `plugin.json`'s `content_sha256` is verified against every file before import, because the hub's own listing has been observed reporting a git blob SHA-1 in a `sha256` field. **v0.4.0** (2026-09-01) @@ -655,7 +655,7 @@ ms-hub agent install -r user/my-agent \ `download` / `upload` / `list` transfer files as-is, with **no framework awareness**. -`install` is different: it does not know any framework's file layout either. It resolves *which* plugin to use, fetches that plugin from a model repository, verifies it against its own manifest, and hands the agent id over — the plugin decides where files land, how the agent is registered, and what the framework needs afterwards. See [`ms-hub agent install`](#ms-hub-agent-install) for the security model. +`install` is different: it does not know any framework's file layout either. It resolves *which* plugin to use, fetches that plugin from a model repository, verifies it against its own manifest, and hands the agent id over — the plugin decides how the agent is registered and what the framework needs afterwards, and which entry operation runs is negotiated from what the plugin declares. See [`ms-hub agent install`](#ms-hub-agent-install) for the security model. > **Framework-aware operations** (cross-framework `convert`, `watch`/bidirectional sync, `status`, `backups`, `restore`, `stop`) live in **[modelscope-agent](https://github.com/modelscope/ms-agent)** — use `ms-agent agent ...` instead. For example, to download and convert in one step: `ms-agent agent download -f qoder -r user/my-agent --target-framework qwenpaw`. @@ -695,7 +695,7 @@ ms-hub agent upload -r user/my-agent --local-dir ./my-agent --dry-run #### `ms-hub agent install` -Download an agent and hand it to its **framework plugin**, which installs it into the local workspace. +Download an agent and hand it to its **framework plugin**. What the plugin does with it is negotiated: one with an `install` entry point places the agent into the framework's workspace, one that only transports bytes writes the repository's files into a destination directory and leaves placement to whatever runs next. The command reports which happened (`Installed …` vs `Fetched …`). ```bash ms-hub agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin --trust-remote-code @@ -710,7 +710,7 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --lo | `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | | `--framework FW` | no | Override the plugin's framework detection | -| `--local-dir DIR` | no | Override the framework's local root | +| `--local-dir DIR` | no | Where the agent goes: the destination directory for a plugin that only fetches, the framework's local root for one that installs. Omitted, a fetch-only plugin gets `$MODELSCOPE_CACHE/agent/agent-staging/---/` | | `--dry-run` | no | Report what would happen, change nothing | | `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | @@ -749,9 +749,11 @@ outcome = install_agent( print(outcome.ok, outcome.operation, outcome.exit_code, outcome.error) ``` -The lower-level steps are exported too (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `select_operation`) for callers that want to inspect a plugin without running it. +The lower-level steps are exported too (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `select_operation`, `default_staging_dir`) for callers that want to inspect a plugin without running it. -The plugin's entry operation is negotiated rather than hard-coded: `install` is preferred, `download` accepted as a fallback, and `capabilities()['operations']` is authoritative when the plugin declares it — so a plugin that ships a name without implementing it is not selected, and the hub does not need re-releasing when a plugin grows a richer entry point. Arguments are narrowed to what the plugin's signature accepts, so a plugin adding new keywords does not break older hubs. +The plugin's entry operation is negotiated rather than hard-coded, in preference order `install` → `fetch_raw` → `download`, and `capabilities()['operations']` is authoritative when the plugin declares it — so a plugin that ships a name without implementing it is not selected, and the hub does not need re-releasing when a plugin grows a richer entry point. Arguments are narrowed to what the plugin's signature accepts, so a plugin adding new keywords does not break older hubs. + +`fetch_raw` is a transport, not an installer: it writes the repository's bytes into a directory the caller names and stops, with no workspace registration and no inbound rewriting. Such a plugin deliberately has **no default destination**, because the only sensible-looking default is a framework workspace, and a workspace holds the user's own credentials (`agent.json` channels, `settings.json` providers, `mcp.json` env blocks) that an overwrite would silently destroy. The hub therefore resolves `dest` for it — `--local-dir` when given, otherwise `default_staging_dir(repo)` — and operations that do not declare `dest` never see it. diff --git a/src/modelscope_hub/agent/__init__.py b/src/modelscope_hub/agent/__init__.py index 66902c4..11a5f6c 100644 --- a/src/modelscope_hub/agent/__init__.py +++ b/src/modelscope_hub/agent/__init__.py @@ -16,7 +16,10 @@ - ``agent_visibility_label`` / ``agent_last_modified`` -- read renamed agent metadata fields from an API item, tolerating both JSON spellings (snake_case and PascalCase) and legacy keys. -- :func:`install_agent` -- install an agent through its framework plugin. +- :func:`install_agent` -- fetch or install an agent through its framework + plugin, the choice being negotiated with the plugin. +- :func:`default_staging_dir` -- where a fetch-only plugin's files land when the + caller named no directory. """ from ._api import AgentApi, RemoteFileInfo, agent_last_modified, agent_visibility_label, is_lfs_file @@ -25,6 +28,7 @@ InstallOutcome, PluginSpec, assert_trusted_owner, + default_staging_dir, fetch_plugin, install_agent, load_plugin, @@ -49,4 +53,5 @@ "verify_manifest", "load_plugin", "select_operation", + "default_staging_dir", ] diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index 1f81206..a81bea4 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -3,8 +3,11 @@ ``ms agent install`` resolves which plugin to use, downloads it from a model repository, verifies it, and hands the agent id to the plugin's entry point. No -framework knowledge lives here: where files land and how an agent is registered -are the plugin's decisions. +framework knowledge lives here: how an agent is registered and what its workspace +looks like are the plugin's decisions. The one exception is the destination +directory, which this module resolves for a plugin that only transports bytes -- +it cannot know where such a plugin should write, and the plugin deliberately has +no default of its own. Integrity comes from ``plugin.json``'s ``content_sha256``, not from the hub's own file listing -- that listing has been observed reporting a git blob SHA-1 in a @@ -20,17 +23,29 @@ import os import sys from dataclasses import dataclass +from datetime import datetime from pathlib import Path from typing import Any from .. import constants from ..errors import InvalidParameter, NotSupportedError +from ..utils.file_utils import get_cache_dir MANIFEST_NAME = "plugin.json" #: Negotiated rather than hard-coded, so a plugin growing a richer entry point -#: does not require re-releasing the hub. -ENTRY_OPERATIONS: tuple[str, ...] = ("install", "download") +#: does not require re-releasing the hub. Order is preference: a plugin that +#: implements ``install`` owns placement, registration and completion, so it +#: wins. ``fetch_raw`` is a transport that writes the repository's bytes into a +#: directory the caller names and never touches a framework workspace, which is +#: what keeps a user's own credentials intact. ``download`` is the 0.1.x name +#: for an operation that did install into the workspace. +ENTRY_OPERATIONS: tuple[str, ...] = ("install", "fetch_raw", "download") + +#: Staging root for a fetch-only plugin, relative to ``MODELSCOPE_CACHE``. +#: Matches the plugin's own ``staging_dir()`` so one convention covers both +#: sides and there are not two places agent files can land. +AGENT_STAGING_SUBDIR: tuple[str, ...] = ("agent", "agent-staging") @dataclass(frozen=True, slots=True) @@ -322,6 +337,20 @@ def _accepted_kwargs(func: Any, candidates: dict[str, Any]) -> dict[str, Any]: return {key: value for key, value in candidates.items() if key in parameters} +def default_staging_dir(repo: str) -> Path: + """Where a fetch-only plugin's files land when the caller named no directory. + + The staging directory itself is not created -- the plugin makes it when it + writes, so an operation that ignores ``dest`` leaves no empty directory + behind. Resolving the path does create the SDK cache root, as any download + would. The repository id contains a slash and so is flattened; the timestamp + keeps repeated fetches of one repository apart, to one-second resolution. + """ + slug = repo.replace("/", "--") + stamp = datetime.now().strftime("%Y%m%d_%H%M%S") + return get_cache_dir().joinpath(*AGENT_STAGING_SUBDIR, f"{slug}-{stamp}") + + def install_agent( repo: str, *, @@ -339,7 +368,13 @@ def install_agent( token: str | None = None, cache_dir: str | None = None, ) -> InstallOutcome: - """Download *repo*'s agent into the local framework workspace via a plugin. + """Fetch or install *repo*'s agent through its framework plugin. + + Which of the two happens is the plugin's answer, not this function's: the + entry operation is negotiated in :func:`select_operation`, so a plugin that + installs into the workspace installs, and one that only transports bytes + stages them in ``local_dir`` (or :func:`default_staging_dir`) for the install + layer to place. *repo* is passed through uninterpreted beyond requiring ``owner/name``. Plugin failures come back as data (``ok`` False); the three gates @@ -393,6 +428,10 @@ def install_agent( "framework": framework, "source_framework": framework, "local_dir": local_dir, + # A fetch-only plugin writes where it is told and has no default, so the + # destination is always resolved here: the caller's --local-dir, else a + # staging directory. Operations that do not declare ``dest`` never see it. + "dest": local_dir or str(default_staging_dir(repo)), "dry_run": dry_run, "yes": yes, "force": force, diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index 0c27711..3740ba0 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -315,10 +315,13 @@ def _cmd_install( result = outcome.result written = getattr(result, "files_written", None) root = getattr(result, "root", None) + # ``fetch_raw`` stages files for the install layer to place; reporting + # "Installed" would hide that no framework was touched. + verb, where = ("Fetched", "to") if outcome.operation == "fetch_raw" else ("Installed", "under") if written is not None and root is not None: - success(f"Installed {repo}: {len(written)} file(s) under {root}") + success(f"{verb} {repo}: {len(written)} file(s) {where} {root}") else: - success(f"Installed {repo}") + success(f"{verb} {repo}") return 0 @@ -430,8 +433,12 @@ def register(subparsers: SubParsers) -> None: help="Install an agent into its framework via the agent plugin", formatter_class=RawDescriptionHelpFormatter, description=( - "Download an agent repository and hand it to the framework plugin, which installs it " - "into the local workspace.\n\n" + "Download an agent repository and hand it to the framework plugin.\n\n" + "What the plugin does with it is negotiated, not assumed: a plugin with an install entry " + "point places the agent into the framework's workspace and completes the framework's own " + "registration steps, while one that only transports bytes writes the repository's files " + "into a destination directory and leaves placement to whatever runs next. The command " + "reports which of the two happened.\n\n" "Loading a plugin imports code this package did not ship, so the plugin source must be " "named explicitly (--plugin-repo or MODELSCOPE_AGENT_PLUGIN_REPO), its owner must be on " "the allow-list (MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS), and execution requires " @@ -448,7 +455,13 @@ def register(subparsers: SubParsers) -> None: "-n", "--name", default=None, help="Sub-agent name to install (default: the plugin's choice)" ) p_install.add_argument("--framework", default=None, help="Override framework detection") - p_install.add_argument("--local-dir", default=None, help="Override the framework's local root") + p_install.add_argument( + "--local-dir", + default=None, + help="Where the agent goes: the destination directory for a plugin that only fetches, the " + "framework's local root for one that installs (default: a staging directory under " + "$MODELSCOPE_CACHE/agent/agent-staging/)", + ) p_install.add_argument( "--plugin-repo", default=None, diff --git a/tests/cli/test_agent_install.py b/tests/cli/test_agent_install.py index fe64211..35de413 100644 --- a/tests/cli/test_agent_install.py +++ b/tests/cli/test_agent_install.py @@ -249,6 +249,27 @@ def test_reports_plugin_and_entry(monkeypatch): assert "mod.install()" in out +@pytest.mark.parametrize( + ("operation", "verb", "where"), + [ + ("install", "Installed", "under"), + ("download", "Installed", "under"), + ("fetch_raw", "Fetched", "to"), + ], +) +def test_success_wording_follows_the_negotiated_operation(monkeypatch, operation, verb, where): + """A transport-only plugin stages files; reporting "Installed" would hide + that no framework was touched.""" + result = type("R", (), {"files_written": ("SOUL.md", "AGENTS.md"), "root": "/tmp/staged"})() + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=True, operation=operation, plugin=spec(), result=result), + ) + code, out, _ = run_cli(MINIMAL) + assert code == 0 + assert f"{verb} {AGENT_REPO}: 2 file(s) {where} /tmp/staged" in out + + def test_quiet_suppresses_all_hub_output(monkeypatch): monkeypatch.setattr( "modelscope_hub.cli.agent.install_agent", diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index 88ceb01..a7051f8 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -15,6 +15,7 @@ import hashlib import json +import re import sys import textwrap from pathlib import Path @@ -411,6 +412,49 @@ def test_select_operation_without_capabilities_uses_presence(tmp_path): assert _plugin.select_operation(load_entry(directory, "sel_nocaps"))[0] == "download" +def test_select_operation_prefers_install_over_fetch_raw(tmp_path): + """A plugin that owns placement wins over one that only transports bytes, so + the install layer taking over needs no hub release.""" + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("install", "fetch_raw", "download")} + + def install(repo, **kwargs): + return "installed" + + def fetch_raw(repo, *, dest, **kwargs): + raise AssertionError("must not be chosen while install is declared") + """ + ).lstrip() + directory = make_plugin(tmp_path, entry_module="sel_pref", entry_source=source) + module = load_entry(directory, "sel_pref") + assert _plugin.select_operation(module) == ("install", module.install) + + +def test_select_operation_falls_back_to_fetch_raw(tmp_path): + """A transport-only plugin is usable: ``download`` is present but undeclared, + so it must not be chosen over the operation the plugin actually reports.""" + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("fetch_raw", "restore", "list_backups")} + + def install(repo, **kwargs): + raise AssertionError("must not be chosen") + + def download(repo, **kwargs): + raise AssertionError("must not be chosen") + + def fetch_raw(repo, *, dest, **kwargs): + return "fetched" + """ + ).lstrip() + directory = make_plugin(tmp_path, entry_module="sel_fetch", entry_source=source) + module = load_entry(directory, "sel_fetch") + assert _plugin.select_operation(module) == ("fetch_raw", module.fetch_raw) + + def test_accepted_kwargs_narrowing(): def positional(repo, name=None): return repo, name @@ -436,6 +480,9 @@ def wired(monkeypatch, tmp_path): monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({TRUSTED})) monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) + # install_agent resolves a default staging directory under the cache; keep it + # out of the real user home. + monkeypatch.setenv(constants.ENV_CACHE, str(tmp_path / "cache")) directory = make_plugin(tmp_path, entry_module="e2e_plugin") monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) yield directory @@ -453,14 +500,20 @@ def test_install_agent_happy_path_and_option_forwarding(wired): import e2e_plugin assert e2e_plugin.CALLS[-1][:2] == ("install", "owner/my-agent") + forwarded = e2e_plugin.CALLS[-1][2] # Unset optionals are dropped so the plugin applies its own defaults, but a # False boolean is a decision the caller made and is forwarded. - assert e2e_plugin.CALLS[-1][2] == { + assert {key: forwarded[key] for key in ("dry_run", "yes", "force", "quiet")} == { "dry_run": False, "yes": False, "force": False, "quiet": False, } + # A variadic entry also gets the resolved destination. With no --local-dir + # that is a fresh staging directory under the cache, not a workspace. + staged = Path(forwarded["dest"]) + assert staged.parent.name == "agent-staging" + assert staged.name.startswith("owner--my-agent-") _plugin.install_agent( "owner/my-agent", @@ -476,6 +529,7 @@ def test_install_agent_happy_path_and_option_forwarding(wired): forwarded = e2e_plugin.CALLS[-1][2] assert forwarded["name"] == "sub" assert forwarded["local_dir"] == "/tmp/ws" + assert forwarded["dest"] == "/tmp/ws" assert forwarded["dry_run"] is True assert forwarded["force"] is True assert forwarded["endpoint"] == "https://pre.modelscope.cn" @@ -550,3 +604,140 @@ def install(repo, **kwargs): assert "boom" in outcome.error assert outcome.exit_code == 1 sys.modules.pop("boom_plugin", None) + + +# --------------------------------------------------------------------------- +# fetch-only plugins +# --------------------------------------------------------------------------- +def test_default_staging_dir_computes_without_creating(tmp_path, monkeypatch): + monkeypatch.setenv(constants.ENV_CACHE, str(tmp_path / "cache")) + path = _plugin.default_staging_dir("owner/my-agent") + + assert path.parent == tmp_path / "cache" / "agent" / "agent-staging" + # The repository id contains a slash, so it has to be flattened, and the + # stamp keeps repeats of one repository apart. Resolution is one second, so + # this separates runs, not concurrent calls. + assert re.fullmatch(r"owner--my-agent-\d{8}_\d{6}", path.name) + assert not path.exists(), "computing a destination must not create it" + + +#: Mirrors the real 0.2.0 plugin: ``dest`` is keyword-only and required, and +#: there is no ``local_dir`` at all, so a hub that does not resolve a +#: destination cannot call it. +FETCH_ONLY_SOURCE = textwrap.dedent( + """ + from dataclasses import dataclass, field + + + @dataclass(frozen=True) + class Result: + ok: bool = True + error: str | None = None + files_written: tuple = field(default_factory=tuple) + root: str = "" + exit_code: int = 0 + + + CALLS = [] + + + def capabilities(): + return {"operations": ("fetch_raw", "restore", "list_backups")} + + + def install(**kwargs): + raise AssertionError("placeholder must not be chosen") + + + def download(repo, **kwargs): + raise AssertionError("legacy alias must not be chosen") + + + def fetch_raw(repo, *, dest, name=None, framework=None, dry_run=False, + quiet=False, endpoint=None, token=None): + CALLS.append({"repo": repo, "dest": dest, "framework": framework}) + return Result(files_written=("SOUL.md", "AGENTS.md"), root=dest) + """ +).lstrip() + + +@pytest.fixture +def fetch_only(wired, monkeypatch): + directory = make_plugin( + wired.parent, + dirname="fetch_plugin", + entry_module="fetch_plugin", + entry_source=FETCH_ONLY_SOURCE, + ) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + yield directory + sys.modules.pop("fetch_plugin", None) + + +def test_install_agent_drives_a_fetch_only_plugin(fetch_only): + """The regression test for the joint-testing failure: a transport-only plugin + used to be rejected outright, and then called without its required ``dest``.""" + outcome = _plugin.install_agent( + "owner/my-agent", + framework="qwenpaw", + plugin_repo=PLUGIN_REPO, + trust_remote_code=True, + ) + assert outcome.ok, outcome.error + assert outcome.operation == "fetch_raw" + + import fetch_plugin + + call = fetch_plugin.CALLS[-1] + assert call["repo"] == "owner/my-agent" + assert call["framework"] == "qwenpaw" + assert Path(call["dest"]).parent.name == "agent-staging" + # The destination the plugin reports is the one the hub resolved. + assert str(outcome.result.root) == call["dest"] + + +def test_install_agent_maps_local_dir_onto_dest(fetch_only): + outcome = _plugin.install_agent( + "owner/my-agent", + local_dir="/tmp/joint/staging", + plugin_repo=PLUGIN_REPO, + trust_remote_code=True, + ) + assert outcome.ok, outcome.error + + import fetch_plugin + + assert fetch_plugin.CALLS[-1]["dest"] == "/tmp/joint/staging" + + +def test_dest_is_not_forwarded_to_an_operation_that_does_not_accept_it(wired, monkeypatch): + """Narrowing keeps the legacy path working: an operation with no ``dest`` + parameter must not be handed one, or every 0.1.x plugin would break.""" + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ("download",)} + + CALLS = [] + + def download(repo, *, local_dir=None, dry_run=False): + CALLS.append({"repo": repo, "local_dir": local_dir}) + return "ok" + """ + ).lstrip() + directory = make_plugin(wired.parent, dirname="legacy_plugin", entry_module="legacy_plugin", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + outcome = _plugin.install_agent( + "owner/my-agent", + local_dir="/tmp/ws", + plugin_repo=PLUGIN_REPO, + trust_remote_code=True, + ) + assert outcome.ok, outcome.error + assert outcome.operation == "download" + + import legacy_plugin + + assert legacy_plugin.CALLS[-1] == {"repo": "owner/my-agent", "local_dir": "/tmp/ws"} + sys.modules.pop("legacy_plugin", None) From b4b660e199a59356fe17664d57dec387065ef24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Thu, 17 Sep 2026 14:21:04 +0800 Subject: [PATCH 03/10] [Refactor] Reuse the SDK's repo-id parser and streaming hash An audit of this branch against what the package already had found two places where it reimplemented something that exists. _split_repo_id duplicated HubApi._parse_repo_id, which enforces the identical rule -- reject an id with no slash, and reject "/name" and "owner/" because a slash alone does not name a repository. Both were checked against the same six inputs, including the two edge cases the local copy was written for, and agreed on every one. Two definitions of what a valid repository id is means they can drift, and this one is on the path that decides whether to spend a request on somebody else's namespace. The only loss is the per-call-site label in the error text, which trades a little specificity for one message across the SDK. verify_manifest hashed each plugin file with hashlib.sha256(path.read_bytes()), loading it whole into memory; utils.compute_hash already does this in chunks and is public. Same digest, verified byte for byte on a real file. Both were checked before use rather than assumed: importing HubApi from agent/_plugin.py does not cycle in either import order (api.py reaches .agent_idp, not .agent), and compute_hash is a drop-in for the inline call. 62 plugin and CLI tests pass; the full suite is 1020 passed with the one pre-existing test_compat_constants_completeness failure that reproduces at the base commit. Also re-ran the joint path with no stubs at all -- real plugin download from production, real agent fetch from pre-release, fetch_raw selected, ten files landed, workspaces byte-identical afterwards. --- src/modelscope_hub/agent/_plugin.py | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index a81bea4..24d6fd0 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -28,8 +28,9 @@ from typing import Any from .. import constants +from ..api import HubApi from ..errors import InvalidParameter, NotSupportedError -from ..utils.file_utils import get_cache_dir +from ..utils.file_utils import compute_hash, get_cache_dir MANIFEST_NAME = "plugin.json" @@ -99,19 +100,6 @@ def _manifest_digest(manifest: dict[str, Any]) -> str: return hashlib.sha256(blob.encode("utf-8")).hexdigest()[:16] -def _split_repo_id(repo_id: str, *, label: str) -> tuple[str, str]: - """Split ``owner/name`` or raise, rejecting empty halves. - - ``"/" in repo_id`` is not enough: ``/name`` and ``owner/`` both contain a - slash but name no repository, and letting either through costs a real request - for somebody else's path. - """ - owner, _, name = repo_id.partition("/") - if not owner or not name: - raise InvalidParameter(f"{label} {repo_id!r} must be in 'owner/name' form.") - return owner, name - - def resolve_plugin_repo(explicit: str | None = None) -> str: """Return the plugin repository id, or raise if none was configured. @@ -134,7 +122,7 @@ def resolve_plugin_repo(explicit: str | None = None) -> str: "deployment choice, so modelscope-hub does not assume one." ) raise error - _split_repo_id(repo_id, label="agent plugin repository") + HubApi._parse_repo_id(repo_id) return repo_id @@ -144,7 +132,7 @@ def assert_trusted_owner(repo_id: str) -> tuple[str, str]: Comparison is case-sensitive: owners are identifiers, so normalising case would let ``mushenl`` pass a list that only trusts ``mushenL``. """ - owner, name = _split_repo_id(repo_id, label="agent plugin repository") + owner, name = HubApi._parse_repo_id(repo_id) trusted = constants.AGENT_PLUGIN_TRUSTED_OWNERS if owner not in trusted: error = InvalidParameter( @@ -234,7 +222,7 @@ def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: if not target.is_file(): missing.append(rel) continue - actual = hashlib.sha256(target.read_bytes()).hexdigest() + actual = compute_hash(target) if actual != expected: mismatched.append(rel) recorded_set = set(recorded) @@ -385,7 +373,7 @@ def install_agent( if not repo or not repo.strip(): raise InvalidParameter("--repo is required, in 'owner/name' form.") repo = repo.strip() - _split_repo_id(repo, label="agent repository") + HubApi._parse_repo_id(repo) plugin_repo_id = resolve_plugin_repo(plugin_repo) owner, plugin_name = assert_trusted_owner(plugin_repo_id) From a5890cbe6f82d78427a91d695344e37a1fa67f01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Thu, 17 Sep 2026 15:25:51 +0800 Subject: [PATCH 04/10] [Feature] Default the plugin to the ModelScope org and show its scope Product placed the plugin under the ModelScope organisation, with AI-ModelScope as the alternative, so both join the allow-list and modelscope/agent-hub-plugin becomes the built-in default. That reverses the earlier "no default owner" decision: resolution is now --plugin-repo, then $MODELSCOPE_AGENT_PLUGIN_REPO, then the default, and the allow-list applies to whichever id wins, so a default is a convenience rather than a bypass. The default is not published yet, and an unpublishable default must not fail inscrutably: when the id in play really is the built-in one and the fetch fails, the suggestion now says it was the default and names both overrides. Verified against the live 404 rather than only in a test. Checking the organisation's real casing turned up a bug in the gate. The registry resolves repository ids case-insensitively and normalises the owner -- asking for ModelScope/ollama-linux returns id 'modelscope/ollama-linux', and AI-modelscope/... returns 'AI-ModelScope/...' -- so two owners differing only in case cannot both exist. Matching exactly therefore never stopped a look-alike account; it only rejected the casing somebody copied from the website, which is a live trap here because the organisation displays as ModelScope while its identifier is lowercase. Matching is now case-insensitive, owners are echoed back as typed so messages keep the user's spelling, and the comment claiming the opposite is corrected rather than left to mislead the next reader. The supported range is now stated at run time instead of only in prose. Every run prints a 'scope :' line with the frameworks that plugin build covers, the operations it implements, and the ones it declares but has not implemented with when they land; describe() gains the same planned line, so omitting --trust-remote-code inspects a plugin's scope without executing any of it. The list comes from the manifest, so it cannot go stale in this package. mushenL stays in the default allow-list while the plugin is developed there, now marked as temporary with the removal steps beside it. Removing it is that one line: nothing else reads the default and no test pins it -- confirmed by dropping it and re-running, 69 tests still green and the gate then refuses mushenL while allowing modelscope, AI-ModelScope and ModelScope. Also stops a CLI test from reaching the network: the missing-plugin-repo case it covered no longer exists, and as written it fell through to a real download. 69 plugin and CLI tests pass; full suite 1027 passed with the one pre-existing test_compat_constants_completeness failure. Re-ran the joint path end to end: real CLI, real published plugin, real pre-release agent, scope line rendered, exit 0, workspaces untouched. --- README.md | 44 +++++++++++++--- src/modelscope_hub/agent/_plugin.py | 78 +++++++++++++++++++---------- src/modelscope_hub/cli/agent.py | 28 ++++++++--- src/modelscope_hub/constants.py | 29 ++++++++--- tests/cli/test_agent_install.py | 53 +++++++++++++++++--- tests/test_agent_plugin.py | 25 ++++++--- 6 files changed, 194 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 70301e1..920616d 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ The official Python SDK & CLI for [ModelScope Hub](https://modelscope.cn) — do **Unreleased** - **Feature**: `ms-hub agent install -r owner/name` resolves a framework plugin, fetches it from a model repository, and delegates to the entry operation the plugin declares — plus the `modelscope_hub.agent.install_agent` SDK entry and its underlying `resolve_plugin_repo` / `assert_trusted_owner` / `fetch_plugin` / `verify_manifest` / `load_plugin` / `select_operation` / `default_staging_dir` steps. The hub gains no framework knowledge: how an agent is registered and what its workspace looks like stay the plugin's decisions, the one exception being the destination directory, which the hub resolves for a plugin that only transports bytes. -- **Quality**: loading a plugin executes code this package did not ship, so it is gated by an explicit plugin source (no built-in default owner), a case-sensitive owner allow-list checked before any download, and a `--trust-remote-code` opt-in that is never persisted. `plugin.json`'s `content_sha256` is verified against every file before import, because the hub's own listing has been observed reporting a git blob SHA-1 in a `sha256` field. +- **Quality**: loading a plugin executes code this package did not ship, so it is gated by an owner allow-list checked before any download and a `--trust-remote-code` opt-in that is never persisted. The plugin repository defaults to `modelscope/agent-hub-plugin` and is overridable per call or per environment. `plugin.json`'s `content_sha256` is verified against every file before import, because the hub's own listing has been observed reporting a git blob SHA-1 in a `sha256` field. **v0.4.0** (2026-09-01) - **Feature**: complete OpenAPI coverage for Agent-IDP, MCP, and Studios — Agent Ed25519 identities, OIDC discovery/JWKS and signed JWT issuance (`HubApi`, `ms-hub agent-idp`); Studio lists, variables and configuration options; hosted MCP discovery; protected visibility and runtime metadata; read-only tokens can log in and rejected writes name the required tier. Agent private JWKs are only written to an explicitly requested owner-only file. @@ -705,7 +705,7 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --lo | Option | Required | Description | |--------|----------|-------------| | `-r, --repo REPO` | yes | Agent repository to install (`owner/name`) | -| `--plugin-repo OWNER/NAME` | no | Plugin model repository. Falls back to `$MODELSCOPE_AGENT_PLUGIN_REPO`; **there is no built-in default owner** | +| `--plugin-repo OWNER/NAME` | no | Plugin model repository. Resolution: this flag, then `$MODELSCOPE_AGENT_PLUGIN_REPO`, then the built-in default `modelscope/agent-hub-plugin`. The owner must be on the allow-list either way | | `--plugin-revision REV` | no | Plugin revision (default: `master`; pin a tag for reproducible installs) | | `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | @@ -716,19 +716,47 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --lo Exit codes: `0` success, `2` a gate refused or the command line is wrong, and otherwise **the plugin's own code** — the install layer gives `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed) and `6` (framework mismatch) distinct meanings, and collapsing them to `1` would discard the only machine-readable signal a caller has. +##### Supported scope + +This package supports **no frameworks**. Which agents it can handle, and how far it goes with them, is entirely a property of the plugin build it fetches — so the authoritative list is printed at run time rather than maintained here, where it would go stale: + +``` +plugin: modelscope/agent-hub-plugin@v0.2.0 (version 0.2.0) +entry : agent_hub_core.fetch_raw() +scope : frameworks ms-agent, qwenpaw | operations fetch_raw, list_backups, restore | planned convert (P2), install (entry package), upload (P1) +Fetched user/my-agent: 10 file(s) to /home/me/.cache/modelscope/agent/agent-staging/user--my-agent-20260917_114512 +``` + +Reading that, for the plugin published at the time of writing: + +| Field | Meaning | +|---|---| +| `entry` | Which operation was negotiated. `install` means the agent was placed into the framework's workspace and registered; `fetch_raw` means its files were downloaded to a directory and **nothing was installed** | +| `frameworks` | The agent frameworks that plugin build understands | +| `operations` | What it can actually do in this version | +| `planned` | Names it declares but has not implemented, and when they land. Calling one returns `ok=False` naming the release, rather than failing obscurely | + +The last line is the one to check when a result looks incomplete: `Fetched … to ` means the files are on disk and the framework has not been touched, while `Installed … under ` means it has been. + +To inspect a plugin's scope **without executing any of its code**, omit `--trust-remote-code`. Downloading does not run anything, so the command fetches the package, verifies its manifest, prints the full summary — repository, revision, version, entry module, frameworks, operations, planned work and the manifest digest — and stops before the import: + +```bash +ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 +``` + ##### Security model Importing a plugin executes code this package did not ship, so the path is gated three times, in increasing order of cost: -1. **Explicit source.** The plugin repository must be named by `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO`. There is no default owner: who publishes the plugin is a deployment decision, and silently falling back to one would let a typo install from somewhere nobody chose. -2. **Owner allow-list.** Checked *before* anything is downloaded, so an untrusted source is refused without touching the network. Matching is **case-sensitive** — a lower-casing comparison would accept a look-alike account, which is exactly what an allow-list must stop. +1. **Known source.** The plugin repository defaults to `modelscope/agent-hub-plugin`, and `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO` overrides it. The default is a convenience, not a bypass: whichever id wins goes through the same allow-list, so a typo is still refused before anything is downloaded. When the default itself cannot be fetched, the error says it was the default and names both overrides, because a repository the user never chose should not fail inscrutably. +2. **Owner allow-list.** Checked *before* anything is downloaded, so an untrusted source is refused without touching the network. Matching is **case-insensitive**, because that is how the registry treats identity: it resolves `ModelScope/x` and `modelscope/x` to the same repository and normalises the owner, so two owners differing only in case cannot both exist. An exact comparison would therefore not stop a look-alike account — it would only reject the casing somebody copied from the website, which is a real trap here since the organisation *displays* as `ModelScope` while its identifier is `modelscope`. Owners are echoed back as typed, so messages keep the user's spelling. ```bash - # Default: mushenL,modelscope. Override (comma-separated, case-sensitive): + # Default: modelscope,AI-ModelScope,mushenL. Override (comma-separated): export MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS="modelscope,my-org" ``` - A refusal names the owners currently trusted and prints this variable, so the fix is discoverable from the error alone. + Overriding replaces the list rather than extending it, which is also how a development owner is dropped once it is no longer needed. A refusal names the owners currently trusted and prints this variable, so the fix is discoverable from the error alone. 3. **Trust opt-in.** Checked after the manifest is verified, so the refusal can show exactly what is about to run — repository, revision, version, entry module, frameworks, declared operations and the manifest digest. The opt-in is a flag or an environment variable and is **never persisted**: "allow this code to run" is not a preference worth remembering on the user's behalf. Between gates 2 and 3, `plugin.json`'s `content_sha256` is checked against every file on disk. A package whose contents do not match the digest published with them is refused, as is one with no digest at all — without it the import would be unconditional code execution. The hub's own file listing is deliberately *not* used for this: it has been observed reporting a git blob SHA-1 in a `sha256` field, which makes it unreliable as an integrity source. @@ -911,8 +939,8 @@ Token is persisted locally after `ms-hub login` and auto-loaded in subsequent se | `MODELSCOPE_ENDPOINT` | `https://modelscope.cn` | API endpoint URL | | `MODELSCOPE_CACHE` | `~/.cache/modelscope` | Local cache directory | | `MODELSCOPE_HOME` | `~/.modelscope` | SDK config directory | -| `MODELSCOPE_AGENT_PLUGIN_REPO` | — | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; no built-in default | -| `MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS` | `mushenL,modelscope` | Comma-separated owners allowed to provide the agent plugin (case-sensitive) | +| `MODELSCOPE_AGENT_PLUGIN_REPO` | `modelscope/agent-hub-plugin` | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; `--plugin-repo` wins over it | +| `MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS` | `modelscope,AI-ModelScope,mushenL` | Comma-separated owners allowed to provide the agent plugin; replaces the default list, matched case-insensitively | | `MODELSCOPE_AGENT_TRUST_REMOTE_CODE` | `false` | Let `ms-hub agent install` execute plugin code without `--trust-remote-code` | | `MODELSCOPE_PREFER_AI_SITE` | `false` | Prefer `modelscope.ai` over `modelscope.cn` | diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index 24d6fd0..271baec 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -76,11 +76,30 @@ def describe(self) -> str: f" entry : {self.entry_module}\n" f" frameworks : {', '.join(map(str, frameworks)) or '-'}\n" f" operations : {', '.join(map(str, operations)) or '-'}\n" + f" planned : {self._planned()}\n" f" directory : {self.directory}\n" f" manifest : {len(self.manifest.get('content_sha256') or {})} file(s), " f"sha256 {digest}" ) + def scope(self) -> str: + """One line naming what this build covers, for the success path. + + A command that exits 0 otherwise tells a user nothing about which + frameworks it handled or which operations this plugin version actually + implements, and both decide whether the result is what they wanted. + """ + frameworks = ", ".join(map(str, self.manifest.get("frameworks") or [])) or "-" + operations = ", ".join(map(str, self.manifest.get("api") or [])) or "-" + return f"frameworks {frameworks} | operations {operations} | planned {self._planned()}" + + def _planned(self) -> str: + """Operations the manifest declares as not yet implemented, and when.""" + roadmap = self.manifest.get("roadmap") or {} + if not isinstance(roadmap, dict): + return "-" + return ", ".join(f"{name} ({when})" for name, when in sorted(roadmap.items())) or "-" + @dataclass(frozen=True, slots=True) class InstallOutcome: @@ -101,27 +120,19 @@ def _manifest_digest(manifest: dict[str, Any]) -> str: def resolve_plugin_repo(explicit: str | None = None) -> str: - """Return the plugin repository id, or raise if none was configured. + """Return the plugin repository id: argument, then environment, then default. - Resolution is the argument, then - :data:`~modelscope_hub.constants.ENV_AGENT_PLUGIN_REPO`, and stops there. - There is deliberately no built-in default owner: who publishes the plugin is a - deployment decision, and a silent fallback would let a typo install from - somewhere nobody chose. + The default is :data:`~modelscope_hub.constants.DEFAULT_AGENT_PLUGIN_REPO`, + the plugin published under the ModelScope organisation. It is a default and + not a hard-coded call site because who publishes the plugin is a deployment + decision -- an override is one flag or one environment variable away, and the + owner allow-list applies to whichever id wins. """ repo_id = (explicit or "").strip() if not repo_id: repo_id = (os.environ.get(constants.ENV_AGENT_PLUGIN_REPO) or "").strip() if not repo_id: - error = InvalidParameter( - "no agent plugin repository configured. Pass --plugin-repo owner/name, " - f"or set {constants.ENV_AGENT_PLUGIN_REPO}=owner/name." - ) - error.suggestion = ( - "The plugin is published as a ModelScope model repository. Its owner is a " - "deployment choice, so modelscope-hub does not assume one." - ) - raise error + repo_id = constants.DEFAULT_AGENT_PLUGIN_REPO HubApi._parse_repo_id(repo_id) return repo_id @@ -129,19 +140,22 @@ def resolve_plugin_repo(explicit: str | None = None) -> str: def assert_trusted_owner(repo_id: str) -> tuple[str, str]: """Split *repo_id* and require its owner on the allow-list. - Comparison is case-sensitive: owners are identifiers, so normalising case - would let ``mushenl`` pass a list that only trusts ``mushenL``. + Matching is case-insensitive because that is how the registry treats + identity: it resolves ``ModelScope/x`` and ``modelscope/x`` to the same + repository and normalises the owner, so two owners differing only in case + cannot both exist. An exact comparison would therefore not stop a look-alike + account -- it would only reject the casing somebody copied from the website. """ owner, name = HubApi._parse_repo_id(repo_id) trusted = constants.AGENT_PLUGIN_TRUSTED_OWNERS - if owner not in trusted: + if owner.casefold() not in {entry.casefold() for entry in trusted}: error = InvalidParameter( f"owner {owner!r} is not allowed to provide the agent plugin. " f"Trusted owners: {', '.join(sorted(trusted)) or '(none)'}." ) error.suggestion = ( f"Extend the allow-list with {constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS}" - "=owner1,owner2 (comma-separated, case-sensitive), then retry." + "=owner1,owner2 (comma-separated), then retry." ) raise error return owner, name @@ -378,13 +392,25 @@ def install_agent( plugin_repo_id = resolve_plugin_repo(plugin_repo) owner, plugin_name = assert_trusted_owner(plugin_repo_id) - directory = fetch_plugin( - plugin_repo_id, - revision=plugin_revision, - token=token, - endpoint=endpoint, - cache_dir=cache_dir, - ) + try: + directory = fetch_plugin( + plugin_repo_id, + revision=plugin_revision, + token=token, + endpoint=endpoint, + cache_dir=cache_dir, + ) + except NotSupportedError as exc: + if plugin_repo_id == constants.DEFAULT_AGENT_PLUGIN_REPO: + # The user never named this repository, so a bare download error + # leaves them nothing to act on. + exc.suggestion = ( + f"{plugin_repo_id} is the built-in default. If it is not published yet, " + f"or you built your own, pass --plugin-repo owner/name (or set " + f"{constants.ENV_AGENT_PLUGIN_REPO}) -- its owner must also be listed in " + f"{constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS}." + ) + raise manifest = verify_manifest(directory, plugin_repo_id) spec = PluginSpec( repo_id=plugin_repo_id, diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index 3740ba0..d6d2b82 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -20,7 +20,14 @@ from pathlib import Path from ..agent import AgentApi, agent_last_modified, agent_visibility_label, install_agent, is_lfs_file -from ..constants import Visibility +from ..constants import ( + DEFAULT_AGENT_PLUGIN_REPO, + DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS, + ENV_AGENT_PLUGIN_REPO, + ENV_AGENT_PLUGIN_TRUSTED_OWNERS, + ENV_AGENT_TRUST_REMOTE_CODE, + Visibility, +) from ..errors import APIError from .base import CLICommand, SubParsers, info, success from .compat import add_subcmd_token_endpoint @@ -303,6 +310,7 @@ def _cmd_install( info(f"plugin: {plugin.repo_id}@{plugin.revision} (version {plugin.version})") if outcome.operation: info(f"entry : {plugin.entry_module}.{outcome.operation}()") + info(f"scope : {plugin.scope()}") if not outcome.ok: _fail(outcome.error or "install failed") @@ -439,10 +447,16 @@ def register(subparsers: SubParsers) -> None: "registration steps, while one that only transports bytes writes the repository's files " "into a destination directory and leaves placement to whatever runs next. The command " "reports which of the two happened.\n\n" - "Loading a plugin imports code this package did not ship, so the plugin source must be " - "named explicitly (--plugin-repo or MODELSCOPE_AGENT_PLUGIN_REPO), its owner must be on " - "the allow-list (MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS), and execution requires " - "--trust-remote-code." + "Supported scope comes from the plugin, not from this package, so it cannot go stale here: " + "every run prints a 'scope :' line naming the frameworks that plugin build covers, the " + "operations it implements, and the ones it declares as not yet available. Run without " + "--trust-remote-code to see that summary plus the resolved plugin and its manifest digest " + "without executing any downloaded code.\n\n" + f"Loading a plugin imports code this package did not ship, so its owner must be on the " + f"allow-list ({ENV_AGENT_PLUGIN_TRUSTED_OWNERS}, default: " + f"{DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS}) and execution requires --trust-remote-code " + f"(or {ENV_AGENT_TRUST_REMOTE_CODE}=1). The plugin itself defaults to " + f"{DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo or {ENV_AGENT_PLUGIN_REPO} overrides it." ), ) p_install.add_argument( @@ -465,8 +479,8 @@ def register(subparsers: SubParsers) -> None: p_install.add_argument( "--plugin-repo", default=None, - help="Plugin model repository, owner/name (default: $MODELSCOPE_AGENT_PLUGIN_REPO; there is " - "no built-in default owner)", + help=f"Plugin model repository, owner/name (default: ${ENV_AGENT_PLUGIN_REPO}, else " + f"{DEFAULT_AGENT_PLUGIN_REPO}). Its owner must be on the allow-list.", ) p_install.add_argument( "--plugin-revision", diff --git a/src/modelscope_hub/constants.py b/src/modelscope_hub/constants.py index 9d2806c..df1acc6 100644 --- a/src/modelscope_hub/constants.py +++ b/src/modelscope_hub/constants.py @@ -964,10 +964,18 @@ def get_upload_ignore_file_pattern() -> str | None: ENV_AGENT_PLUGIN_TRUSTED_OWNERS: str = "MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS" ENV_AGENT_TRUST_REMOTE_CODE: str = "MODELSCOPE_AGENT_TRUST_REMOTE_CODE" -DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS: str = "mushenL,modelscope" +#: Owners allowed to provide the agent plugin. ``modelscope`` and +#: ``AI-ModelScope`` are the published homes for it; ``mushenL`` is a personal +#: account used while the plugin is still being developed there, and should be +#: dropped before this ships. Removing it is this line alone -- nothing else reads +#: the default, and no test pins it (the gate tests set the resolved constant +#: themselves). Until then an override is enough: +#: ``MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS=modelscope,AI-ModelScope``. +DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS: str = "modelscope,AI-ModelScope,mushenL" +DEFAULT_AGENT_PLUGIN_REPO: str = "modelscope/agent-hub-plugin" DEFAULT_AGENT_PLUGIN_REVISION: str = "master" -_AGENT_TRUSTED_OWNERS_DESCRIPTION = "Comma-separated owners allowed to provide the agent plugin (case-sensitive)" +_AGENT_TRUSTED_OWNERS_DESCRIPTION = "Comma-separated owners allowed to provide the agent plugin" def _env_csv_frozenset_exact( @@ -977,11 +985,15 @@ def _env_csv_frozenset_exact( category: str, *deprecated_names: str, ) -> frozenset[str]: - """Read a comma-separated set from the environment, **preserving case**. - - Do not "simplify" this into :func:`_env_csv_frozenset`: that one upper-cases - every item, which would let an owner differing only in case pass an - allow-list check -- the look-alike an allow-list exists to stop. + """Read a comma-separated set from the environment, preserving case. + + Case is preserved so the list can be shown back to a user exactly as they or + the default wrote it. It is **not** an identity rule: the registry resolves + repository ids case-insensitively and normalises them (``ModelScope/x`` and + ``modelscope/x`` are one repository), so matching happens case-insensitively + at the comparison site in :mod:`modelscope_hub.agent._plugin`. Do not + "simplify" this into :func:`_env_csv_frozenset`, which upper-cases every item + and would turn ``AI-ModelScope`` into ``AI-MODELSCOPE`` in messages. """ _env_register(name, default, description, category, deprecated_names=deprecated_names) raw = _env(name, *deprecated_names) or default @@ -990,7 +1002,7 @@ def _env_csv_frozenset_exact( _env_register( ENV_AGENT_PLUGIN_REPO, - "-", + DEFAULT_AGENT_PLUGIN_REPO, "Model repository id ('owner/name') of the agent plugin used by 'ms agent install'", "Core", ) @@ -1024,6 +1036,7 @@ def _env_csv_frozenset_exact( "CATEGORY_ORDER", "CONFIG_DIR_NAME", "DATASET_LFS_SUFFIX", + "DEFAULT_AGENT_PLUGIN_REPO", "DEFAULT_AGENT_PLUGIN_REVISION", "DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS", "DEFAULT_CACHE_DIR_NAME", diff --git a/tests/cli/test_agent_install.py b/tests/cli/test_agent_install.py index 35de413..1a3d3f5 100644 --- a/tests/cli/test_agent_install.py +++ b/tests/cli/test_agent_install.py @@ -26,6 +26,7 @@ from modelscope_hub import constants from modelscope_hub.agent import InstallOutcome, PluginSpec, _plugin from modelscope_hub.cli.agent import AgentCommand +from modelscope_hub.errors import NotSupportedError from .conftest import run_cli @@ -194,12 +195,21 @@ def test_install_does_not_resolve_a_username(monkeypatch, stub_sdk): # --------------------------------------------------------------------------- # gates: exit code 2, and where the message lands # --------------------------------------------------------------------------- -def test_missing_plugin_repo_exits_2_with_guidance(): - code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO]) - assert code == 2 - assert "--plugin-repo" in err - # run_cmd prints the message to stderr and the suggestion to stdout. - assert constants.ENV_AGENT_PLUGIN_REPO in out + err +def test_unfetchable_default_plugin_repo_points_at_the_override(monkeypatch): + """The default repository is not the user's choice, so a bare download error + would leave them nothing to act on. Stubbed: this must not reach the network.""" + + def refused(repo_id, **kwargs): + assert repo_id == constants.DEFAULT_AGENT_PLUGIN_REPO, "the built-in default should be used" + raise NotSupportedError(f"failed to download agent plugin {repo_id}@master: record not found") + + monkeypatch.setattr(_plugin, "fetch_plugin", refused) + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--trust-remote-code"]) + assert code != 0 + combined = out + err + assert constants.DEFAULT_AGENT_PLUGIN_REPO in combined + assert "--plugin-repo" in combined + assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in combined def test_untrusted_plugin_owner_exits_2(): @@ -270,6 +280,37 @@ def test_success_wording_follows_the_negotiated_operation(monkeypatch, operation assert f"{verb} {AGENT_REPO}: 2 file(s) {where} /tmp/staged" in out +def test_reports_the_supported_scope(monkeypatch): + """A successful run must show what that plugin build covers, not just an exit + code -- which frameworks, which operations work, and which are declared but + not implemented yet.""" + rich = PluginSpec( + repo_id=PLUGIN_REPO, + owner=TRUSTED, + name="agent-hub-plugin", + revision="v0.2.0", + directory=Path("/tmp/nowhere"), + manifest={ + "version": "0.2.0", + "frameworks": ["ms-agent", "qwenpaw"], + "api": ["fetch_raw", "list_backups", "restore"], + "roadmap": {"install": "entry package", "upload": "P1", "convert": "P2"}, + "content_sha256": {"agent_hub_core/__init__.py": "0" * 64}, + }, + entry_module="agent_hub_core", + ) + result = type("R", (), {"files_written": ("SOUL.md",), "root": "/tmp/staged"})() + monkeypatch.setattr( + "modelscope_hub.cli.agent.install_agent", + lambda *a, **k: outcome(ok=True, operation="fetch_raw", plugin=rich, result=result), + ) + code, out, _ = run_cli(MINIMAL) + assert code == 0 + assert "scope : frameworks ms-agent, qwenpaw" in out + assert "operations fetch_raw, list_backups, restore" in out + assert "planned convert (P2), install (entry package), upload (P1)" in out + + def test_quiet_suppresses_all_hub_output(monkeypatch): monkeypatch.setattr( "modelscope_hub.cli.agent.install_agent", diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index a7051f8..a673441 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -131,18 +131,14 @@ def load_entry(directory: Path, module_name: str): # resolve_plugin_repo # --------------------------------------------------------------------------- def test_resolve_plugin_repo_resolution_order(monkeypatch): - """Argument beats environment, and there is no third fallback.""" + """Argument beats environment beats the built-in default.""" monkeypatch.setenv(constants.ENV_AGENT_PLUGIN_REPO, "env-owner/env-plugin") assert _plugin.resolve_plugin_repo("arg-owner/arg-plugin") == "arg-owner/arg-plugin" assert _plugin.resolve_plugin_repo(None) == "env-owner/env-plugin" assert _plugin.resolve_plugin_repo(" ") == "env-owner/env-plugin" monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) - with pytest.raises(InvalidParameter) as excinfo: - _plugin.resolve_plugin_repo(None) - # A missing default must be actionable from the message alone. - assert "--plugin-repo" in str(excinfo.value) - assert constants.ENV_AGENT_PLUGIN_REPO in str(excinfo.value) + assert _plugin.resolve_plugin_repo(None) == constants.DEFAULT_AGENT_PLUGIN_REPO @pytest.mark.parametrize("value", ["noslash", "/noname", "owner/"]) @@ -165,15 +161,28 @@ def test_assert_trusted_owner_accepts_allow_listed(allow_list): assert _plugin.assert_trusted_owner("modelscope/agent-hub-plugin") == ("modelscope", "agent-hub-plugin") -@pytest.mark.parametrize("owner", ["mushenl", "evil", "mushenL-x"]) +@pytest.mark.parametrize("owner", ["evil", "mushenL-x", "modelscope2", "ai_modelscope"]) def test_assert_trusted_owner_rejects_others(allow_list, owner): - """Case-sensitive on purpose: normalising case would accept a look-alike.""" with pytest.raises(InvalidParameter) as excinfo: _plugin.assert_trusted_owner(f"{owner}/agent-hub-plugin") assert owner in str(excinfo.value) assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in excinfo.value.suggestion +@pytest.mark.parametrize("owner", ["mushenl", "MUSHENL", "ModelScope", "MODELSCOPE", "ai-modelscope"]) +def test_assert_trusted_owner_matches_case_insensitively(monkeypatch, owner): + """The registry resolves ids case-insensitively and normalises the owner -- + ``ModelScope/x`` and ``modelscope/x`` are one repository -- so two owners + differing only in case cannot both exist. Matching exactly would not stop a + look-alike; it would only reject the casing somebody copied from the website, + which is how the product writes ``ModelScope``.""" + monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({"mushenL", "modelscope", "AI-ModelScope"})) + got_owner, got_name = _plugin.assert_trusted_owner(f"{owner}/agent-hub-plugin") + assert got_name == "agent-hub-plugin" + # Echoed as typed, so messages and PluginSpec keep the user's spelling. + assert got_owner == owner + + def test_assert_trusted_owner_empty_list_blocks_everything(monkeypatch): monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset()) with pytest.raises(InvalidParameter): From 7351bd6f4e90a7249762a5e42a3138f9c616ed6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Fri, 18 Sep 2026 15:10:06 +0800 Subject: [PATCH 05/10] [Fix] Say what --local-dir actually does now that a plugin can install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The help called it "the framework's local root for one that installs", which was a guess made when no installing plugin existed. Now that one does, the mapping is observable and the guess is wrong: local_dir is forwarded as dest, which is where the repository is downloaded. The agent itself goes into the framework's own home (~/.ms_agent, ~/.qwenpaw) regardless, and the download is left in place because a directory the user named is never treated as scratch -- the entry package only auto-cleans under its staging root. Verified by running the real chain: --local-dir /tmp/…/hub-dest with MS_AGENT_HOME=/tmp/…/hub-home left eleven downloaded files in the first and the installed agent in the second. Leaving the download behind is defensible but is not what a user expects from a flag named --local-dir, so the wording states it plainly instead of implying the agent lands there. --- README.md | 2 +- src/modelscope_hub/cli/agent.py | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 920616d..a376f93 100644 --- a/README.md +++ b/README.md @@ -710,7 +710,7 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --lo | `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | | `--framework FW` | no | Override the plugin's framework detection | -| `--local-dir DIR` | no | Where the agent goes: the destination directory for a plugin that only fetches, the framework's local root for one that installs. Omitted, a fetch-only plugin gets `$MODELSCOPE_CACHE/agent/agent-staging/---/` | +| `--local-dir DIR` | no | Where the agent repository is **downloaded**. A fetch-only plugin stops there; an installing plugin then places the agent in the framework's own home (`~/.ms_agent`, `~/.qwenpaw`) and leaves the download behind, because a directory you named is never treated as scratch. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` and are cleaned up on success | | `--dry-run` | no | Report what would happen, change nothing | | `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index d6d2b82..e7f195e 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -472,9 +472,11 @@ def register(subparsers: SubParsers) -> None: p_install.add_argument( "--local-dir", default=None, - help="Where the agent goes: the destination directory for a plugin that only fetches, the " - "framework's local root for one that installs (default: a staging directory under " - "$MODELSCOPE_CACHE/agent/agent-staging/)", + help="Where the agent repository is downloaded. A plugin that only fetches leaves the files " + "there and stops; one that installs then places the agent in the framework's own home " + "(e.g. ~/.ms_agent, ~/.qwenpaw) and leaves the download behind, since a directory you named " + "is never treated as scratch. Omitted, downloads go to " + "$MODELSCOPE_CACHE/agent/agent-staging/ and are cleaned up on success.", ) p_install.add_argument( "--plugin-repo", From af7a53afbe0b02aa0b80049e802caa564a5fd044 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Sun, 20 Sep 2026 11:08:57 +0800 Subject: [PATCH 06/10] [Fix] Require ok in a plugin result instead of defaulting it to success `ok` is the only signal that decides whether the user is told the agent was installed, and it defaulted to True: ok = bool(getattr(result, "ok", True)) So a plugin returning None, a bare string, an empty dict or any object without the attribute was reported as a successful install. Verified all four shapes before changing it. Every other gate on this path fails closed -- an unlisted owner is refused before any download, a manifest mismatch refuses the import -- so the one signal that produces the success message was the only one failing open. A missing `ok` is now a contract violation: ok=False, exit 1, and an error naming what the entry operation must return, so a plugin author sees the requirement rather than a silent pass. One existing test broke and was wrong to pass: its fake legacy plugin returned the string "ok". That test is about argument narrowing, so its fixture now returns a contract-shaped result and still asserts what it was written for. Three new cases pin the fail-closed behaviour. 1030 tests pass. Re-ran the real chain afterwards -- published plugin v0.3.1 against a real pre-release agent repository, entry agent_hub_plugin.install(), exit 0, seven files installed -- so the plugin's own InstallResult is unaffected. --- src/modelscope_hub/agent/_plugin.py | 21 +++++++++++++++++++-- tests/test_agent_plugin.py | 27 ++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index 271baec..cad3ba6 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -467,8 +467,25 @@ def install_agent( exit_code=1, ) - ok = bool(getattr(result, "ok", True)) - if ok: + # ``ok`` is required, not defaulted. It is the only signal deciding whether + # the user is told the agent was installed, so defaulting it to True let a + # plugin returning None, a bare string or an empty dict report success. Every + # other gate here fails closed; this one has to as well. + if not hasattr(result, "ok"): + return InstallOutcome( + ok=False, + error=( + f"plugin {operation}() returned {type(result).__name__} with no 'ok' attribute. " + "An entry operation must return a result carrying at least 'ok', plus 'error' " + "and 'exit_code' on failure. Refusing to report an unverifiable install as success." + ), + operation=operation, + plugin=spec, + result=result, + exit_code=1, + ) + + if bool(result.ok): return InstallOutcome(ok=True, operation=operation, plugin=spec, result=result) error = getattr(result, "error", None) or f"plugin {operation}() reported failure" diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index a673441..415ce7d 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -615,6 +615,31 @@ def install(repo, **kwargs): sys.modules.pop("boom_plugin", None) +@pytest.mark.parametrize("returned", ["None", "'done'", "{}"]) +def test_install_agent_refuses_a_result_without_ok(wired, monkeypatch, returned): + """``ok`` is the only signal that decides whether the user is told the agent + was installed, so a result without it must fail closed. Defaulting it to True + meant a plugin returning None, a bare string or an empty dict reported + "Installed" for an install nobody could verify.""" + source = textwrap.dedent( + f""" + def capabilities(): + return {{"operations": ("install",)}} + + def install(repo, **kwargs): + return {returned} + """ + ).lstrip() + directory = make_plugin(wired.parent, dirname="no_ok_plugin", entry_module="no_ok_plugin", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + assert not outcome.ok + assert outcome.exit_code == 1 + assert "no 'ok' attribute" in outcome.error + sys.modules.pop("no_ok_plugin", None) + + # --------------------------------------------------------------------------- # fetch-only plugins # --------------------------------------------------------------------------- @@ -731,7 +756,7 @@ def capabilities(): def download(repo, *, local_dir=None, dry_run=False): CALLS.append({"repo": repo, "local_dir": local_dir}) - return "ok" + return type("R", (), {"ok": True})() """ ).lstrip() directory = make_plugin(wired.parent, dirname="legacy_plugin", entry_module="legacy_plugin", entry_source=source) From 53ea6a53ab8da5e51c140eb93353bd369e153b49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Sun, 20 Sep 2026 13:16:14 +0800 Subject: [PATCH 07/10] [Fix] Harden the plugin loader per review Five changes from review, plus the documentation they showed was wrong. The owner allow-list is now a compile-time constant with no environment override. It is the trust anchor for a command that executes downloaded code, and an anchor any parent process can rewrite through the environment is not an anchor -- a script that can set env vars could point the command at a repository it controls. MODELSCOPE_AGENT_PLUGIN_REPO stays overridable and that is safe: it chooses which repository to fetch, but the owner still has to be listed, so it can pick among trusted owners without widening trust. Removing the override also deletes the custom CSV parser that existed only to read it, which is what made this block the most complicated part of constants.py. The personal development account is gone from the default list. load_plugin no longer leaves the plugin directory at sys.path[0] for the rest of the process. A directory parked there lets any file the plugin ships shadow the standard library or a dependency, and shipping one is not even a rule violation -- every file has to be listed in the manifest, so a json.py passes integrity like anything else. The entry module is also registered under a directory-scoped alias instead of its own name: import_module goes through the global cache, so two plugins whose entry modules share a name meant the second silently ran the first. The alias keys on the resolved directory, not on repository and revision, because those do not identify the bytes -- two checkouts of one revision are different code with the same identity, and a test caught exactly that collision. The scope cannot be narrowed to the import alone: plugins import their own sibling packages lazily, at call time. verify_manifest rejects manifest keys that are absolute or climb out of the package. Keys are attacker-controlled and become paths, and `directory / "/etc/ hosts"` discards the directory outright, so a hostile manifest had the loader hashing files anywhere on disk. Nothing was returned to the caller, so it was not a disclosure, but it was a read the manifest has no business requesting. select_operation no longer swallows a failing capabilities(). Catching it and treating it as "declared nothing" downgraded selection to first-callable-wins, which can pick a placeholder the plugin deliberately left undeclared. A capabilities() that exists and raises is a broken plugin, so it is an error; one that is absent still falls back to presence, which is what lets a minimal plugin work. require_trust warns when the opt-in came from the environment rather than the flag, because that variable applies to every install in the process and an environment the user did not build can opt them in. The refusal now also says that opting in hands the plugin the endpoint and the API token, which is part of what is being agreed to and was not disclosed. Documentation, corrected rather than left to imply more than the code does: content_sha256 is transport integrity plus a stable fingerprint for the trust prompt, not authenticity -- the manifest is unsigned and ships beside the code it describes, so whoever controls the repository controls the hashes. --dry-run still imports the plugin, so module-level code runs; the way to inspect without executing is to omit --trust-remote-code. Staging cleanup is the plugin's, not the hub's. And a new "Writing a plugin" section states the contract, which nothing in the repository did: required and optional manifest fields, operation negotiation, the keyword set, the result contract, and the two constraints that follow from how loading works. Review asked for less README, so the rationale prose went with it -- the three existing subsections lost 29 lines and the install section is 2 lines shorter than before despite gaining the spec. 1039 tests pass, up from 1027. The one failure is the pre-existing test_compat_constants_completeness, which reproduces at the base commit. --- README.md | 87 +++++---- src/modelscope_hub/agent/__init__.py | 2 + src/modelscope_hub/agent/_plugin.py | 272 +++++++++++++++++++++------ src/modelscope_hub/cli/agent.py | 21 ++- src/modelscope_hub/constants.py | 55 ++---- tests/cli/test_agent_install.py | 9 +- tests/test_agent_plugin.py | 176 ++++++++++++++--- 7 files changed, 437 insertions(+), 185 deletions(-) diff --git a/README.md b/README.md index a376f93..2e263e2 100644 --- a/README.md +++ b/README.md @@ -34,8 +34,8 @@ The official Python SDK & CLI for [ModelScope Hub](https://modelscope.cn) — do ## News **Unreleased** -- **Feature**: `ms-hub agent install -r owner/name` resolves a framework plugin, fetches it from a model repository, and delegates to the entry operation the plugin declares — plus the `modelscope_hub.agent.install_agent` SDK entry and its underlying `resolve_plugin_repo` / `assert_trusted_owner` / `fetch_plugin` / `verify_manifest` / `load_plugin` / `select_operation` / `default_staging_dir` steps. The hub gains no framework knowledge: how an agent is registered and what its workspace looks like stay the plugin's decisions, the one exception being the destination directory, which the hub resolves for a plugin that only transports bytes. -- **Quality**: loading a plugin executes code this package did not ship, so it is gated by an owner allow-list checked before any download and a `--trust-remote-code` opt-in that is never persisted. The plugin repository defaults to `modelscope/agent-hub-plugin` and is overridable per call or per environment. `plugin.json`'s `content_sha256` is verified against every file before import, because the hub's own listing has been observed reporting a git blob SHA-1 in a `sha256` field. +- **Feature**: `ms-hub agent install -r owner/name` fetches a framework plugin from a model repository and delegates to the entry operation it declares, plus the `modelscope_hub.agent.install_agent` SDK entry. The hub gains no framework knowledge; see [Writing a plugin](#writing-a-plugin). +- **Quality**: loading a plugin executes code this package did not ship, so it is gated by a compile-time owner allow-list checked before any download, a `content_sha256` manifest verified against every file before import, and a `--trust-remote-code` opt-in that is never persisted. **v0.4.0** (2026-09-01) - **Feature**: complete OpenAPI coverage for Agent-IDP, MCP, and Studios — Agent Ed25519 identities, OIDC discovery/JWKS and signed JWT issuance (`HubApi`, `ms-hub agent-idp`); Studio lists, variables and configuration options; hosted MCP discovery; protected visibility and runtime metadata; read-only tokens can log in and rejected writes name the required tier. Agent private JWKs are only written to an explicitly requested owner-only file. @@ -699,7 +699,7 @@ Download an agent and hand it to its **framework plugin**. What the plugin does ```bash ms-hub agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin --trust-remote-code -ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --local-dir ~/ws +ms-hub agent install -r user/my-agent --plugin-revision v0.3.1 -n sub-agent --local-dir ~/ws ``` | Option | Required | Description | @@ -710,78 +710,76 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -n sub-agent --lo | `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | | `--framework FW` | no | Override the plugin's framework detection | -| `--local-dir DIR` | no | Where the agent repository is **downloaded**. A fetch-only plugin stops there; an installing plugin then places the agent in the framework's own home (`~/.ms_agent`, `~/.qwenpaw`) and leaves the download behind, because a directory you named is never treated as scratch. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` and are cleaned up on success | -| `--dry-run` | no | Report what would happen, change nothing | +| `--local-dir DIR` | no | Where the agent repository is **downloaded** — not where it is installed. An installing plugin then places the agent in the framework's own home (`~/.ms_agent`, `~/.qwenpaw`) and leaves the download in place, since a directory you named is never treated as scratch. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` | +| `--dry-run` | no | Ask the plugin to report instead of change anything. The plugin is still **imported**, so its module-level code runs; to inspect one without executing it, omit `--trust-remote-code` | | `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | Exit codes: `0` success, `2` a gate refused or the command line is wrong, and otherwise **the plugin's own code** — the install layer gives `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed) and `6` (framework mismatch) distinct meanings, and collapsing them to `1` would discard the only machine-readable signal a caller has. ##### Supported scope -This package supports **no frameworks**. Which agents it can handle, and how far it goes with them, is entirely a property of the plugin build it fetches — so the authoritative list is printed at run time rather than maintained here, where it would go stale: +This package supports **no frameworks**; which agents it can handle is a property of the plugin build it fetches. So the authoritative list is printed every run rather than maintained here, where it would go stale: ``` -plugin: modelscope/agent-hub-plugin@v0.2.0 (version 0.2.0) -entry : agent_hub_core.fetch_raw() -scope : frameworks ms-agent, qwenpaw | operations fetch_raw, list_backups, restore | planned convert (P2), install (entry package), upload (P1) -Fetched user/my-agent: 10 file(s) to /home/me/.cache/modelscope/agent/agent-staging/user--my-agent-20260917_114512 +plugin: modelscope/agent-hub-plugin@v0.3.1 (version 0.3.1) +entry : agent_hub_plugin.install() # negotiated: install > fetch_raw > download +scope : frameworks ms-agent, qwenpaw | operations fetch_raw, install, list_backups, restore | planned convert (P2), upload (P1) +Installed user/my-agent ``` -Reading that, for the plugin published at the time of writing: +`entry` is the line to read when a result looks incomplete: `Installed` means the framework was touched, `Fetched … to ` means only that files are on disk. `planned` names operations the plugin declares but has not implemented; calling one returns `ok=False` naming the release rather than failing obscurely. -| Field | Meaning | -|---|---| -| `entry` | Which operation was negotiated. `install` means the agent was placed into the framework's workspace and registered; `fetch_raw` means its files were downloaded to a directory and **nothing was installed** | -| `frameworks` | The agent frameworks that plugin build understands | -| `operations` | What it can actually do in this version | -| `planned` | Names it declares but has not implemented, and when they land. Calling one returns `ok=False` naming the release, rather than failing obscurely | +Omit `--trust-remote-code` to inspect a plugin's scope **without executing any of it**: downloading runs nothing, so the command fetches the package, verifies the manifest, prints repository, revision, version, entry module, frameworks, operations, planned work and the manifest digest, then stops before the import. -The last line is the one to check when a result looks incomplete: `Fetched … to ` means the files are on disk and the framework has not been touched, while `Installed … under ` means it has been. +##### Security model -To inspect a plugin's scope **without executing any of its code**, omit `--trust-remote-code`. Downloading does not run anything, so the command fetches the package, verifies its manifest, prints the full summary — repository, revision, version, entry module, frameworks, operations, planned work and the manifest digest — and stops before the import: +Importing a plugin executes code this package did not ship, so it is gated three times, cheapest first: -```bash -ms-hub agent install -r user/my-agent --plugin-revision v0.2.0 -``` +1. **Known source.** Defaults to `modelscope/agent-hub-plugin`; `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO` overrides it. That is a convenience, not a bypass — whichever id wins still passes the allow-list. +2. **Owner allow-list**, checked *before* anything is downloaded. It is a **compile-time constant** (`modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS`, currently `modelscope` and `AI-ModelScope`) with deliberately **no environment override**: this list is the trust anchor for a command that runs downloaded code, and an anchor any parent process can rewrite is not an anchor. Widening it is a reviewed code change. Matching is case-insensitive because that is how the registry treats identity — it resolves `ModelScope/x` and `modelscope/x` to one repository — so an exact comparison would not stop a look-alike, it would only reject the casing copied from the website. +3. **Trust opt-in**, checked after the manifest is verified so the refusal can show exactly what is about to run. A flag or an environment variable, **never persisted**. Satisfying it through the environment logs a warning, because that variable applies to every install in the process and an environment you did not build can opt you in. Opting in also hands the plugin your `--endpoint` and your API token, since it needs credentials to fetch the agent. -##### Security model +Between gates 2 and 3, `plugin.json`'s `content_sha256` is checked against every file on disk, in both directions, and a key pointing outside the package is refused. Be precise about what that buys: it proves the bytes are the bytes the manifest described, and it makes the digest in the trust prompt meaningful, so what you agreed to and what gets imported cannot diverge. **It is not authenticity** — the manifest is unsigned and ships beside the code it describes, so whoever controls the repository controls the hashes. The trust anchor is the allow-list plus the opt-in. The hub's own file listing is not used for this either: it has been observed reporting a git blob SHA-1 in a `sha256` field. -Importing a plugin executes code this package did not ship, so the path is gated three times, in increasing order of cost: +##### Writing a plugin -1. **Known source.** The plugin repository defaults to `modelscope/agent-hub-plugin`, and `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO` overrides it. The default is a convenience, not a bypass: whichever id wins goes through the same allow-list, so a typo is still refused before anything is downloaded. When the default itself cannot be fetched, the error says it was the default and names both overrides, because a repository the user never chose should not fail inscrutably. -2. **Owner allow-list.** Checked *before* anything is downloaded, so an untrusted source is refused without touching the network. Matching is **case-insensitive**, because that is how the registry treats identity: it resolves `ModelScope/x` and `modelscope/x` to the same repository and normalises the owner, so two owners differing only in case cannot both exist. An exact comparison would therefore not stop a look-alike account — it would only reject the casing somebody copied from the website, which is a real trap here since the organisation *displays* as `ModelScope` while its identifier is `modelscope`. Owners are echoed back as typed, so messages keep the user's spelling. +A plugin is a **model** repository (`snapshot_download` rejects `repo_type='agent'`) with `plugin.json` at its root beside an importable package — `my_plugin/__init__.py`, or a single `my_plugin.py` — named by `entry_module`: - ```bash - # Default: modelscope,AI-ModelScope,mushenL. Override (comma-separated): - export MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS="modelscope,my-org" - ``` +`plugin.json` — two fields are required, the rest are display only: - Overriding replaces the list rather than extending it, which is also how a development owner is dropped once it is no longer needed. A refusal names the owners currently trusted and prints this variable, so the fix is discoverable from the error alone. -3. **Trust opt-in.** Checked after the manifest is verified, so the refusal can show exactly what is about to run — repository, revision, version, entry module, frameworks, declared operations and the manifest digest. The opt-in is a flag or an environment variable and is **never persisted**: "allow this code to run" is not a preference worth remembering on the user's behalf. +```json +{ + "entry_module": "my_plugin", + "content_sha256": {"my_plugin/__init__.py": "", "...": "..."}, + "version": "1.0.0", + "frameworks": ["qwenpaw"], + "api": ["install"], + "roadmap": {"upload": "P1"} +} +``` -Between gates 2 and 3, `plugin.json`'s `content_sha256` is checked against every file on disk. A package whose contents do not match the digest published with them is refused, as is one with no digest at all — without it the import would be unconditional code execution. The hub's own file listing is deliberately *not* used for this: it has been observed reporting a git blob SHA-1 in a `sha256` field, which makes it unreliable as an integrity source. +`content_sha256` must cover **every** file in the package except `plugin.json` itself (it cannot hash itself), `.gitattributes` (the hub injects it) and `__pycache__`. Anything missing, mismatched or unlisted is refused. `version` / `frameworks` / `api` / `roadmap` are never validated — they feed the trust prompt and the `scope :` line. -Downloading a plugin does not execute anything, so fetching an untrusted package is safe; only the import is gated. +The entry module must expose at least one of `install`, `fetch_raw`, `download`, tried in that order. Export `capabilities()` returning `{"operations": [...], "frameworks": [...], "planned": {...}}`: it is authoritative, so a name you ship but did not implement is skipped instead of selected. Without it, selection falls back to "first callable attribute wins", which will pick a stub. A `capabilities()` that raises is an error, not an empty declaration. + +Your operation is called with keyword arguments narrowed to its signature, from: `repo`, `name`, `framework`, `source_framework`, `local_dir`, `dest`, `dry_run`, `yes`, `force`, `quiet`, `endpoint`, `token`. Unset optionals are dropped so your defaults apply; `False` booleans are kept; `dest` is always resolved (`--local-dir`, else a staging directory). Accept `**kwargs` to be forward-compatible. + +Return an object carrying **`ok`** — required, not defaulted, because it is the only signal deciding whether the user is told the agent was installed. Add `error` and `exit_code` on failure, and `files_written` plus `root` for the success message. Raising is also fine: it becomes `ok=False`, exit 1. + +Two constraints follow from how loading works. Use **relative imports** inside your package: it is registered under a directory-scoped alias, not its own name, so two plugins cannot be served each other's cached code. And do not assume the plugin directory is still on `sys.path` after your operation returns — it is scoped to the call, for the reason given above. Your staging directory is yours to clean up, not the hub's. ##### Python API ```python from modelscope_hub.agent import install_agent -outcome = install_agent( - "user/my-agent", - plugin_repo="modelscope/agent-hub-plugin", - plugin_revision="v0.2.0", - trust_remote_code=True, -) +outcome = install_agent("user/my-agent", plugin_revision="v0.3.1", trust_remote_code=True) print(outcome.ok, outcome.operation, outcome.exit_code, outcome.error) ``` -The lower-level steps are exported too (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `select_operation`, `default_staging_dir`) for callers that want to inspect a plugin without running it. - -The plugin's entry operation is negotiated rather than hard-coded, in preference order `install` → `fetch_raw` → `download`, and `capabilities()['operations']` is authoritative when the plugin declares it — so a plugin that ships a name without implementing it is not selected, and the hub does not need re-releasing when a plugin grows a richer entry point. Arguments are narrowed to what the plugin's signature accepts, so a plugin adding new keywords does not break older hubs. +The steps are exported individually (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `plugin_syspath`, `select_operation`, `default_staging_dir`) for callers that want to inspect a plugin without running it. -`fetch_raw` is a transport, not an installer: it writes the repository's bytes into a directory the caller names and stops, with no workspace registration and no inbound rewriting. Such a plugin deliberately has **no default destination**, because the only sensible-looking default is a framework workspace, and a workspace holds the user's own credentials (`agent.json` channels, `settings.json` providers, `mcp.json` env blocks) that an overwrite would silently destroy. The hub therefore resolves `dest` for it — `--local-dir` when given, otherwise `default_staging_dir(repo)` — and operations that do not declare `dest` never see it. +A `fetch_raw`-style plugin deliberately has **no default destination**: the only sensible-looking default is a framework workspace, and a workspace holds the user's own credentials (`agent.json` channels, `settings.json` providers, `mcp.json` env blocks) that an overwrite would silently destroy. The hub resolves `dest` for it instead. @@ -940,7 +938,6 @@ Token is persisted locally after `ms-hub login` and auto-loaded in subsequent se | `MODELSCOPE_CACHE` | `~/.cache/modelscope` | Local cache directory | | `MODELSCOPE_HOME` | `~/.modelscope` | SDK config directory | | `MODELSCOPE_AGENT_PLUGIN_REPO` | `modelscope/agent-hub-plugin` | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; `--plugin-repo` wins over it | -| `MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS` | `modelscope,AI-ModelScope,mushenL` | Comma-separated owners allowed to provide the agent plugin; replaces the default list, matched case-insensitively | | `MODELSCOPE_AGENT_TRUST_REMOTE_CODE` | `false` | Let `ms-hub agent install` execute plugin code without `--trust-remote-code` | | `MODELSCOPE_PREFER_AI_SITE` | `false` | Prefer `modelscope.ai` over `modelscope.cn` | diff --git a/src/modelscope_hub/agent/__init__.py b/src/modelscope_hub/agent/__init__.py index 11a5f6c..8669699 100644 --- a/src/modelscope_hub/agent/__init__.py +++ b/src/modelscope_hub/agent/__init__.py @@ -32,6 +32,7 @@ fetch_plugin, install_agent, load_plugin, + plugin_syspath, resolve_plugin_repo, select_operation, verify_manifest, @@ -52,6 +53,7 @@ "fetch_plugin", "verify_manifest", "load_plugin", + "plugin_syspath", "select_operation", "default_staging_dir", ] diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index cad3ba6..0a16628 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -11,20 +11,32 @@ Integrity comes from ``plugin.json``'s ``content_sha256``, not from the hub's own file listing -- that listing has been observed reporting a git blob SHA-1 in a -``sha256`` field. +``sha256`` field. Be precise about what that buys: it proves the bytes on disk are +the bytes the manifest described, and it gives the trust prompt a stable +fingerprint, so the decision and the import cannot diverge. It is **not** +authenticity. The manifest ships inside the same unsigned repository as the code +it describes, so whoever controls the repository controls the hashes and can make +anything verify. The trust anchor is the owner allow-list plus the opt-in; nothing +here vouches for who wrote the plugin. Signing would change that and is not done +yet. """ from __future__ import annotations import hashlib import importlib +import importlib.util import inspect import json +import logging import os +import re import sys +from collections.abc import Iterator +from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Any from .. import constants @@ -32,6 +44,8 @@ from ..errors import InvalidParameter, NotSupportedError from ..utils.file_utils import compute_hash, get_cache_dir +logger = logging.getLogger("modelscope_hub.agent") + MANIFEST_NAME = "plugin.json" #: Negotiated rather than hard-coded, so a plugin growing a richer entry point @@ -154,8 +168,10 @@ def assert_trusted_owner(repo_id: str) -> tuple[str, str]: f"Trusted owners: {', '.join(sorted(trusted)) or '(none)'}." ) error.suggestion = ( - f"Extend the allow-list with {constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS}" - "=owner1,owner2 (comma-separated), then retry." + "The allow-list is a compile-time constant " + "(modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS), not an " + "environment variable: it is the trust anchor for a command that runs " + "downloaded code, so widening it is a reviewed code change." ) raise error return owner, name @@ -206,8 +222,13 @@ def fetch_plugin( def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: """Check the downloaded package against its own ``plugin.json``. - Strict on purpose: this is what makes the subsequent import something other - than unconditional code execution. + Strict on purpose, and worth being clear about what strictness buys: it proves + the files on disk are the files the manifest described, and it makes the + digest shown by :func:`require_trust` mean something, so what the user agreed + to and what gets imported cannot diverge. It does not prove anything about + authorship -- the manifest is unsigned and ships beside the code it describes, + so a repository's owner can make any content verify. That is the owner + allow-list's job. """ manifest_path = directory / MANIFEST_NAME if not manifest_path.is_file(): @@ -230,9 +251,22 @@ def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: "cannot be verified. Refusing to load it." ) - missing, mismatched, unexpected = [], [], [] + missing, mismatched, unexpected, escaped = [], [], [], [] + directory_resolved = directory.resolve() for rel, expected in sorted(recorded.items()): + # Manifest keys are attacker-controlled. ``directory / "/etc/passwd"`` + # discards the directory entirely, so an absolute or parent-climbing key + # would have this loader read -- and hash -- a file outside the package. + # Nothing is returned to the caller, so it is not a disclosure, but it is + # a read the manifest has no business requesting. + parts = PurePosixPath(rel).parts + if PurePosixPath(rel).is_absolute() or ".." in parts: + escaped.append(rel) + continue target = directory / rel + if not target.resolve().is_relative_to(directory_resolved): + escaped.append(rel) + continue if not target.is_file(): missing.append(rel) continue @@ -249,6 +283,8 @@ def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: unexpected.append(rel) problems = [] + if escaped: + problems.append(f"{len(escaped)} manifest key(s) point outside the package: {', '.join(escaped[:5])}") if missing: problems.append(f"missing {len(missing)} file(s): {', '.join(missing[:5])}") if mismatched: @@ -267,47 +303,162 @@ def require_trust(spec: PluginSpec, *, trust_remote_code: bool) -> None: a flag or an environment variable and is never persisted -- "allow this code to run" is not a preference worth remembering on the user's behalf. """ - if trust_remote_code or constants.AGENT_TRUST_REMOTE_CODE: + if trust_remote_code: + return + if constants.AGENT_TRUST_REMOTE_CODE: + logger.warning( + "Executing plugin %s@%s because %s is set, not because this invocation " + "asked for it. That variable applies to every install in this process, " + "so an environment you did not build can opt you in.", + spec.repo_id, + spec.revision, + constants.ENV_AGENT_TRUST_REMOTE_CODE, + ) return raise NotSupportedError( "refusing to execute plugin code without an explicit opt-in. The plugin " "resolved to:\n" + spec.describe() + "\n\n" + "Opting in does two things: it imports and runs that code, and it passes " + "the plugin your --endpoint and your API token, since it needs credentials " + "to fetch the agent. Only continue if you trust the owner to hold both.\n\n" "Re-run with --trust-remote-code to import and run it, or set " f"{constants.ENV_AGENT_TRUST_REMOTE_CODE}=1." ) +def _module_alias(spec: PluginSpec) -> str: + """A ``sys.modules`` name unique to the code being loaded. + + ``importlib.import_module(entry_module)`` goes through the global cache, so + loading two plugins whose entry modules share a name in one process would + silently run the first one's code for the second. + + The discriminator has to be the *directory*, not the repository and revision: + those do not identify the bytes. Two checkouts of one repository at one + revision -- a re-download into a different cache, a development tree beside a + published one -- are different code with the same identity, and aliasing on + the pair collides. The entry module name stays in the alias so a traceback is + still readable. + """ + resolved = Path(spec.directory).resolve() + digest = hashlib.sha256(str(resolved).encode("utf-8")).hexdigest()[:12] + safe = re.sub(r"[^A-Za-z0-9_]", "_", spec.entry_module) + return f"_ms_agent_plugin_{safe}_{digest}" + + +@contextmanager +def plugin_syspath(directory: Path) -> Iterator[None]: + """Keep *directory* importable for the duration of the block, then undo it. + + Scoped rather than permanent on purpose. A plugin directory parked at + ``sys.path[0]`` lets any module it ships shadow the standard library or a + dependency for the rest of the process, and shipping one is not even a rule + violation -- every file has to be listed in ``content_sha256``, so a + ``json.py`` or ``requests.py`` passes the integrity check like anything else. + A one-shot CLI barely notices; a long-lived process calling + :func:`install_agent` would stay poisoned. + + It cannot be narrowed to the import alone: plugins import their own sibling + packages lazily, at call time, so the directory has to stay reachable until + the operation returns. + """ + root = str(directory) + inserted = root not in sys.path + if inserted: + sys.path.insert(0, root) + try: + yield + finally: + if inserted: + try: + sys.path.remove(root) + except ValueError: + pass + + def load_plugin(spec: PluginSpec) -> Any: """Import the plugin's entry module from its downloaded directory. The directory goes at the *front* of ``sys.path`` so the fetched revision - wins over any same-named installed distribution. + wins over any same-named installed distribution, and the module is registered + under :func:`_module_alias` rather than its own name so a second plugin cannot + be served the first one's cached code. Callers that will also *invoke* the + plugin should hold :func:`plugin_syspath` open for the whole operation. + + Residual limitation, not solved here: a plugin's sibling top-level packages + (``agent_hub_core`` beside ``agent_hub_plugin``, say) are imported by their own + names and still land in the global cache, so two plugins shipping different + copies of one would collide. Isolating that needs a subprocess, which in turn + needs a serialisable result contract. """ root = str(spec.directory) if root not in sys.path: sys.path.insert(0, root) + + alias = _module_alias(spec) + cached = sys.modules.get(alias) + if cached is not None: + return cached + + package_init = Path(spec.directory) / spec.entry_module / "__init__.py" + single_file = Path(spec.directory) / f"{spec.entry_module}.py" + if package_init.is_file(): + target, search_locations = package_init, [str(package_init.parent)] + elif single_file.is_file(): + target, search_locations = single_file, None + else: + if root in sys.path: + sys.path.remove(root) + raise ImportError(f"entry module {spec.entry_module!r} is neither a package nor a module in {root}") + + module_spec = importlib.util.spec_from_file_location(alias, target, submodule_search_locations=search_locations) + if module_spec is None or module_spec.loader is None: + if root in sys.path: + sys.path.remove(root) + raise ImportError(f"cannot build an import spec for {target}") + + module = importlib.util.module_from_spec(module_spec) + # Registered before executing so the plugin's own relative and recursive + # imports resolve to this module rather than re-entering it. + sys.modules[alias] = module try: - return importlib.import_module(spec.entry_module) - except ImportError: + module_spec.loader.exec_module(module) + except BaseException: + sys.modules.pop(alias, None) if root in sys.path: sys.path.remove(root) raise + return module def select_operation(module: Any) -> tuple[str, Any]: """Pick the entry operation the plugin actually supports. ``capabilities()`` is authoritative when present, so a plugin that ships a - name without implementing it is not selected. + name without implementing it is not selected. When it is *absent* selection + falls back to presence, which is what lets a minimal plugin work. When it is + present but fails, that is a broken plugin rather than an undeclaring one, so + it is an error: silently falling back to presence would pick whatever name + happens to exist, including a placeholder the plugin chose not to declare. """ declared = None capabilities = getattr(module, "capabilities", None) if callable(capabilities): try: payload = capabilities() - declared = set((payload or {}).get("operations") or ()) - except Exception: - declared = None + except Exception as exc: + raise NotSupportedError( + f"plugin {module.__name__}.capabilities() raised " + f"{exc.__class__.__name__}: {exc}. Without it there is no way to tell " + "which operations the plugin implements, so refusing to guess." + ) from exc + operations = payload.get("operations") if isinstance(payload, dict) else None + if operations is None: + raise NotSupportedError( + f"plugin {module.__name__}.capabilities() returned no 'operations' " + f"(got {type(payload).__name__}); cannot tell which entry points it implements." + ) + declared = set(operations) for name in ENTRY_OPERATIONS: func = getattr(module, name, None) @@ -407,8 +558,8 @@ def install_agent( exc.suggestion = ( f"{plugin_repo_id} is the built-in default. If it is not published yet, " f"or you built your own, pass --plugin-repo owner/name (or set " - f"{constants.ENV_AGENT_PLUGIN_REPO}) -- its owner must also be listed in " - f"{constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS}." + f"{constants.ENV_AGENT_PLUGIN_REPO}). Its owner must be one of " + f"{', '.join(sorted(constants.AGENT_PLUGIN_TRUSTED_OWNERS))}." ) raise manifest = verify_manifest(directory, plugin_repo_id) @@ -423,49 +574,52 @@ def install_agent( ) require_trust(spec, trust_remote_code=trust_remote_code) - try: - module = load_plugin(spec) - operation, func = select_operation(module) - except NotSupportedError: - raise - except Exception as exc: - return InstallOutcome( - ok=False, - error=f"failed to load plugin {plugin_repo_id}: {exc.__class__.__name__}: {exc}", - plugin=spec, - exit_code=1, - ) + # The plugin directory is importable for exactly as long as the plugin runs, + # not for the rest of the process -- see :func:`plugin_syspath`. + with plugin_syspath(spec.directory): + try: + module = load_plugin(spec) + operation, func = select_operation(module) + except NotSupportedError: + raise + except Exception as exc: + return InstallOutcome( + ok=False, + error=f"failed to load plugin {plugin_repo_id}: {exc.__class__.__name__}: {exc}", + plugin=spec, + exit_code=1, + ) - candidates: dict[str, Any] = { - "repo": repo, - "name": name, - "framework": framework, - "source_framework": framework, - "local_dir": local_dir, - # A fetch-only plugin writes where it is told and has no default, so the - # destination is always resolved here: the caller's --local-dir, else a - # staging directory. Operations that do not declare ``dest`` never see it. - "dest": local_dir or str(default_staging_dir(repo)), - "dry_run": dry_run, - "yes": yes, - "force": force, - "quiet": quiet, - "endpoint": endpoint, - "token": token, - } - # Unset optionals are dropped so the plugin applies its own defaults; a False - # boolean is kept because that is a decision the caller made. - provided = {key: value for key, value in candidates.items() if value is not None} - try: - result = func(**_accepted_kwargs(func, provided)) - except Exception as exc: - return InstallOutcome( - ok=False, - error=f"plugin {operation}() failed: {exc.__class__.__name__}: {exc}", - operation=operation, - plugin=spec, - exit_code=1, - ) + candidates: dict[str, Any] = { + "repo": repo, + "name": name, + "framework": framework, + "source_framework": framework, + "local_dir": local_dir, + # A fetch-only plugin writes where it is told and has no default, so the + # destination is always resolved here: the caller's --local-dir, else a + # staging directory. Operations that do not declare ``dest`` never see it. + "dest": local_dir or str(default_staging_dir(repo)), + "dry_run": dry_run, + "yes": yes, + "force": force, + "quiet": quiet, + "endpoint": endpoint, + "token": token, + } + # Unset optionals are dropped so the plugin applies its own defaults; a False + # boolean is kept because that is a decision the caller made. + provided = {key: value for key, value in candidates.items() if value is not None} + try: + result = func(**_accepted_kwargs(func, provided)) + except Exception as exc: + return InstallOutcome( + ok=False, + error=f"plugin {operation}() failed: {exc.__class__.__name__}: {exc}", + operation=operation, + plugin=spec, + exit_code=1, + ) # ``ok`` is required, not defaulted. It is the only signal deciding whether # the user is told the agent was installed, so defaulting it to True let a diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index e7f195e..a1081fb 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -21,10 +21,9 @@ from ..agent import AgentApi, agent_last_modified, agent_visibility_label, install_agent, is_lfs_file from ..constants import ( + AGENT_PLUGIN_TRUSTED_OWNERS, DEFAULT_AGENT_PLUGIN_REPO, - DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS, ENV_AGENT_PLUGIN_REPO, - ENV_AGENT_PLUGIN_TRUSTED_OWNERS, ENV_AGENT_TRUST_REMOTE_CODE, Visibility, ) @@ -452,11 +451,12 @@ def register(subparsers: SubParsers) -> None: "operations it implements, and the ones it declares as not yet available. Run without " "--trust-remote-code to see that summary plus the resolved plugin and its manifest digest " "without executing any downloaded code.\n\n" - f"Loading a plugin imports code this package did not ship, so its owner must be on the " - f"allow-list ({ENV_AGENT_PLUGIN_TRUSTED_OWNERS}, default: " - f"{DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS}) and execution requires --trust-remote-code " - f"(or {ENV_AGENT_TRUST_REMOTE_CODE}=1). The plugin itself defaults to " - f"{DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo or {ENV_AGENT_PLUGIN_REPO} overrides it." + f"Loading a plugin imports code this package did not ship, so its owner must be on a " + f"compile-time allow-list ({', '.join(sorted(AGENT_PLUGIN_TRUSTED_OWNERS))}) and execution " + f"requires --trust-remote-code (or {ENV_AGENT_TRUST_REMOTE_CODE}=1). Opting in also hands " + f"the plugin your --endpoint and your API token, because it needs credentials to fetch the " + f"agent. The plugin itself defaults to {DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo or " + f"{ENV_AGENT_PLUGIN_REPO} overrides it, but the owner still has to be on the allow-list." ), ) p_install.add_argument( @@ -495,7 +495,12 @@ def register(subparsers: SubParsers) -> None: help="Allow the downloaded plugin to be imported and executed. Without it (or " "$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1) the command reports what it would run and stops.", ) - p_install.add_argument("--dry-run", action="store_true", help="Report what would happen, change nothing") + p_install.add_argument( + "--dry-run", + action="store_true", + help="Ask the plugin to report instead of change anything. The plugin is still imported, so its " + "module-level code runs; to inspect one without executing it, omit --trust-remote-code", + ) p_install.add_argument("-y", "--yes", action="store_true", help="Answer the plugin's prompts yes") p_install.add_argument("--force", action="store_true", help="Let the plugin overwrite an existing agent") p_install.add_argument("-q", "--quiet", action="store_true", help="Suppress the plugin's progress output") diff --git a/src/modelscope_hub/constants.py b/src/modelscope_hub/constants.py index df1acc6..69615b7 100644 --- a/src/modelscope_hub/constants.py +++ b/src/modelscope_hub/constants.py @@ -961,45 +961,24 @@ def get_upload_ignore_file_pattern() -> str | None: # :mod:`modelscope_hub.agent._plugin`. # --------------------------------------------------------------------------- ENV_AGENT_PLUGIN_REPO: str = "MODELSCOPE_AGENT_PLUGIN_REPO" -ENV_AGENT_PLUGIN_TRUSTED_OWNERS: str = "MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS" ENV_AGENT_TRUST_REMOTE_CODE: str = "MODELSCOPE_AGENT_TRUST_REMOTE_CODE" -#: Owners allowed to provide the agent plugin. ``modelscope`` and -#: ``AI-ModelScope`` are the published homes for it; ``mushenL`` is a personal -#: account used while the plugin is still being developed there, and should be -#: dropped before this ships. Removing it is this line alone -- nothing else reads -#: the default, and no test pins it (the gate tests set the resolved constant -#: themselves). Until then an override is enough: -#: ``MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS=modelscope,AI-ModelScope``. -DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS: str = "modelscope,AI-ModelScope,mushenL" +#: Owners allowed to provide the agent plugin. +#: +#: A compile-time constant with **no environment override**, on purpose. This list +#: is the trust anchor for a command that executes downloaded code, and an anchor +#: any parent process can rewrite through the environment is not an anchor: a +#: script that can set env vars could point ``ms agent install`` at a repository +#: it controls. Deciding who is trusted is a reviewed code change. +#: +#: ``MODELSCOPE_AGENT_PLUGIN_REPO`` *is* overridable and that is safe: it chooses +#: which repository to fetch, but the owner still has to appear here, so it can +#: pick among already-trusted owners without widening trust. +AGENT_PLUGIN_TRUSTED_OWNERS: frozenset[str] = frozenset({"modelscope", "AI-ModelScope"}) + DEFAULT_AGENT_PLUGIN_REPO: str = "modelscope/agent-hub-plugin" DEFAULT_AGENT_PLUGIN_REVISION: str = "master" -_AGENT_TRUSTED_OWNERS_DESCRIPTION = "Comma-separated owners allowed to provide the agent plugin" - - -def _env_csv_frozenset_exact( - name: str, - default: str, - description: str, - category: str, - *deprecated_names: str, -) -> frozenset[str]: - """Read a comma-separated set from the environment, preserving case. - - Case is preserved so the list can be shown back to a user exactly as they or - the default wrote it. It is **not** an identity rule: the registry resolves - repository ids case-insensitively and normalises them (``ModelScope/x`` and - ``modelscope/x`` are one repository), so matching happens case-insensitively - at the comparison site in :mod:`modelscope_hub.agent._plugin`. Do not - "simplify" this into :func:`_env_csv_frozenset`, which upper-cases every item - and would turn ``AI-ModelScope`` into ``AI-MODELSCOPE`` in messages. - """ - _env_register(name, default, description, category, deprecated_names=deprecated_names) - raw = _env(name, *deprecated_names) or default - return frozenset(item.strip() for item in raw.split(",") if item.strip()) - - _env_register( ENV_AGENT_PLUGIN_REPO, DEFAULT_AGENT_PLUGIN_REPO, @@ -1013,12 +992,6 @@ def _env_csv_frozenset_exact( "Core", ) -AGENT_PLUGIN_TRUSTED_OWNERS: frozenset[str] = _env_csv_frozenset_exact( - ENV_AGENT_PLUGIN_TRUSTED_OWNERS, - DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS, - _AGENT_TRUSTED_OWNERS_DESCRIPTION, - "Core", -) AGENT_TRUST_REMOTE_CODE: bool = _env_bool( ENV_AGENT_TRUST_REMOTE_CODE, False, @@ -1038,7 +1011,6 @@ def _env_csv_frozenset_exact( "DATASET_LFS_SUFFIX", "DEFAULT_AGENT_PLUGIN_REPO", "DEFAULT_AGENT_PLUGIN_REVISION", - "DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS", "DEFAULT_CACHE_DIR_NAME", "DEFAULT_CREDENTIALS_PATH", "DEFAULT_DATASET_REVISION", @@ -1059,7 +1031,6 @@ def _env_csv_frozenset_exact( "DOWNLOAD_RETRY_TIMES", "DOWNLOAD_TIMEOUT", "ENV_AGENT_PLUGIN_REPO", - "ENV_AGENT_PLUGIN_TRUSTED_OWNERS", "ENV_AGENT_TRUST_REMOTE_CODE", "ENV_FILE_LOCK", "ENV_CACHE", diff --git a/tests/cli/test_agent_install.py b/tests/cli/test_agent_install.py index 1a3d3f5..76d6bda 100644 --- a/tests/cli/test_agent_install.py +++ b/tests/cli/test_agent_install.py @@ -209,7 +209,8 @@ def refused(repo_id, **kwargs): combined = out + err assert constants.DEFAULT_AGENT_PLUGIN_REPO in combined assert "--plugin-repo" in combined - assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in combined + for owner in sorted(constants.AGENT_PLUGIN_TRUSTED_OWNERS): + assert owner in combined def test_untrusted_plugin_owner_exits_2(): @@ -218,7 +219,11 @@ def test_untrusted_plugin_owner_exits_2(): ) assert code == 2 assert "evil" in err - assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in out + err + combined = out + err + # The refusal must say the list is compiled in, so nobody hunts for an + # environment variable that no longer exists. + assert "AGENT_PLUGIN_TRUSTED_OWNERS" in combined + assert "compile-time" in combined def test_trust_gate_refuses_and_explains(monkeypatch, tmp_path): diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index 415ce7d..7d80d82 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -14,6 +14,7 @@ from __future__ import annotations import hashlib +import importlib import json import re import sys @@ -127,6 +128,17 @@ def load_entry(directory: Path, module_name: str): return _plugin.load_plugin(spec_for(directory, manifest={"entry_module": module_name})) +def loaded(directory: Path, module_name: str): + """The module ``install_agent`` loaded from *directory*. + + Reached through the alias, not ``import ``: registration is + directory-scoped on purpose, so the plain name is never in ``sys.modules`` and + the plugin directory is off ``sys.path`` again once the call returns. + """ + alias = _plugin._module_alias(spec_for(directory, manifest={"entry_module": module_name})) + return sys.modules[alias] + + # --------------------------------------------------------------------------- # resolve_plugin_repo # --------------------------------------------------------------------------- @@ -166,7 +178,29 @@ def test_assert_trusted_owner_rejects_others(allow_list, owner): with pytest.raises(InvalidParameter) as excinfo: _plugin.assert_trusted_owner(f"{owner}/agent-hub-plugin") assert owner in str(excinfo.value) - assert constants.ENV_AGENT_PLUGIN_TRUSTED_OWNERS in excinfo.value.suggestion + assert "AGENT_PLUGIN_TRUSTED_OWNERS" in excinfo.value.suggestion + + +def test_the_allow_list_cannot_be_widened_from_the_environment(monkeypatch): + """The allow-list is the trust anchor for a command that executes downloaded + code, so a parent process must not be able to move it. The override that used + to exist is gone; this pins that it stays gone rather than being reintroduced + as a convenience.""" + monkeypatch.setenv("MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS", "evilcorp") + importlib.reload(constants) + try: + assert "evilcorp" not in constants.AGENT_PLUGIN_TRUSTED_OWNERS + assert not hasattr(constants, "ENV_AGENT_PLUGIN_TRUSTED_OWNERS") + with pytest.raises(InvalidParameter): + _plugin.assert_trusted_owner("evilcorp/agent-hub-plugin") + finally: + importlib.reload(constants) + + +def test_the_shipped_allow_list_has_no_personal_account(): + """A personal account in the default list is a supply-chain entry point: if it + is compromised, anything it publishes passes the owner gate.""" + assert constants.AGENT_PLUGIN_TRUSTED_OWNERS == frozenset({"modelscope", "AI-ModelScope"}) @pytest.mark.parametrize("owner", ["mushenl", "MUSHENL", "ModelScope", "MODELSCOPE", "ai-modelscope"]) @@ -189,14 +223,6 @@ def test_assert_trusted_owner_empty_list_blocks_everything(monkeypatch): _plugin.assert_trusted_owner("mushenL/agent-hub-plugin") -def test_env_csv_helper_preserves_case(monkeypatch): - """The only coverage of the environment parsing behind the allow-list; the - gate tests above patch the resolved constant instead.""" - monkeypatch.setenv("MODELSCOPE_TEST_OWNERS", " mushenL , modelscope ,, ") - got = constants._env_csv_frozenset_exact("MODELSCOPE_TEST_OWNERS", "fallback", "test", "Core") - assert got == frozenset({"mushenL", "modelscope"}) - - # --------------------------------------------------------------------------- # verify_manifest # --------------------------------------------------------------------------- @@ -278,6 +304,24 @@ def test_verify_manifest_exempts_non_plugin_files(tmp_path): assert ".gitattributes" not in str(excinfo.value) +@pytest.mark.parametrize("key", ["/etc/hosts", "../../etc/hosts", "pkg/../../outside.py"]) +def test_verify_manifest_refuses_keys_pointing_outside_the_package(tmp_path, key): + """Manifest keys are attacker-controlled and become paths. ``directory / key`` + with an absolute key discards the directory outright, so a hostile manifest + could have the loader hash a file anywhere on disk. Nothing is returned to the + caller, so it is not a disclosure -- but it is a read the manifest has no + business requesting, and a package that asks for it is not a corrupt download. + """ + directory = make_plugin(tmp_path) + manifest = json.loads((directory / "plugin.json").read_text()) + manifest["content_sha256"][key] = "0" * 64 + (directory / "plugin.json").write_text(json.dumps(manifest)) + + with pytest.raises(NotSupportedError) as excinfo: + _plugin.verify_manifest(directory, PLUGIN_REPO) + assert "outside the package" in str(excinfo.value) + + # --------------------------------------------------------------------------- # require_trust # --------------------------------------------------------------------------- @@ -300,6 +344,21 @@ def test_require_trust_allows_with_flag_or_env(tmp_path, monkeypatch, via): ) +def test_require_trust_warns_when_the_environment_opted_in(tmp_path, monkeypatch, caplog): + """The variable applies to every install in the process, so an environment the + user did not build can opt them in without a decision being made here. The + flag path stays silent: that one was a choice.""" + monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", True) + with caplog.at_level("WARNING", logger="modelscope_hub.agent"): + _plugin.require_trust(spec_for(make_plugin(tmp_path)), trust_remote_code=False) + assert constants.ENV_AGENT_TRUST_REMOTE_CODE in caplog.text + + caplog.clear() + with caplog.at_level("WARNING", logger="modelscope_hub.agent"): + _plugin.require_trust(spec_for(make_plugin(tmp_path / "b")), trust_remote_code=True) + assert caplog.text == "" + + # --------------------------------------------------------------------------- # fetch_plugin # --------------------------------------------------------------------------- @@ -347,10 +406,34 @@ def boom(repo_id, **kwargs): def test_load_plugin_imports_the_declared_entry_module(tmp_path): directory = make_plugin(tmp_path, entry_module="entry_a") module = _plugin.load_plugin(spec_for(directory)) - assert module.__name__ == "entry_a" + assert module.__file__ == str(directory / "entry_a.py") + # Registered under a directory-scoped alias, not its own name -- see the next + # test for why that matters. + assert module.__name__.startswith("_ms_agent_plugin_entry_a_") assert str(directory) in sys.path sys.path.remove(str(directory)) - sys.modules.pop("entry_a", None) + sys.modules.pop(module.__name__, None) + + +def test_load_plugin_does_not_serve_one_directory_another(tmp_path): + """Two plugins sharing a repository id, revision and entry module name must + not share code. ``import_module`` would have returned the first from + ``sys.modules`` for the second, so installing plugin B ran plugin A.""" + first = make_plugin(tmp_path / "one", dirname="p", entry_module="same_name", entry_source="MARKER = 'first'\n") + second = make_plugin(tmp_path / "two", dirname="p", entry_module="same_name", entry_source="MARKER = 'second'\n") + loaded = [] + try: + for directory in (first, second): + sys.path.insert(0, str(directory)) + loaded.append(_plugin.load_plugin(spec_for(directory))) + assert [m.MARKER for m in loaded] == ["first", "second"] + assert loaded[0] is not loaded[1] + finally: + for m in loaded: + sys.modules.pop(m.__name__, None) + for directory in (first, second): + if str(directory) in sys.path: + sys.path.remove(str(directory)) def test_load_plugin_restores_sys_path_on_failure(tmp_path): @@ -371,6 +454,25 @@ def test_load_plugin_restores_sys_path_on_failure(tmp_path): assert sys.path == before +def test_plugin_syspath_is_scoped_to_the_block(tmp_path): + """A plugin directory left at sys.path[0] lets any file it ships shadow the + standard library or a dependency for the rest of the process -- and shipping + one is not a rule violation, since every file has to be listed in the + manifest. Harmless in a one-shot CLI; a long-lived process calling + install_agent would stay poisoned.""" + directory = make_plugin(tmp_path) + before = list(sys.path) + with _plugin.plugin_syspath(directory): + assert str(directory) in sys.path + assert sys.path[0] == str(directory) + assert sys.path == before + + +def test_install_agent_leaves_no_plugin_directory_on_sys_path(wired): + _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + assert str(wired) not in sys.path + + def test_select_operation_prefers_install(tmp_path): module = load_entry(make_plugin(tmp_path, entry_module="sel_install"), "sel_install") name, func = _plugin.select_operation(module) @@ -415,6 +517,28 @@ def install(repo, **kwargs): assert "sel_none" in str(excinfo.value) +def test_select_operation_refuses_a_broken_capabilities(tmp_path): + """A ``capabilities()`` that exists and fails means the plugin is broken, not + that it declares nothing. Swallowing it downgraded selection to "first + callable attribute wins", which can pick a placeholder the plugin deliberately + left undeclared.""" + source = textwrap.dedent( + """ + def capabilities(): + raise RuntimeError("manifest and code disagree") + + def install(repo, **kwargs): + raise AssertionError("must not be chosen") + """ + ).lstrip() + directory = make_plugin(tmp_path, entry_module="broken_caps", entry_source=source) + module = load_entry(directory, "broken_caps") + with pytest.raises(NotSupportedError) as excinfo: + _plugin.select_operation(module) + assert "capabilities() raised" in str(excinfo.value) + assert "manifest and code disagree" in str(excinfo.value) + + def test_select_operation_without_capabilities_uses_presence(tmp_path): source = "def download(repo, **kwargs):\n return 'ok'\n" directory = make_plugin(tmp_path, entry_module="sel_nocaps", entry_source=source) @@ -495,7 +619,7 @@ def wired(monkeypatch, tmp_path): directory = make_plugin(tmp_path, entry_module="e2e_plugin") monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) yield directory - sys.modules.pop("e2e_plugin", None) + sys.modules.pop(_plugin._module_alias(spec_for(directory)), None) def test_install_agent_happy_path_and_option_forwarding(wired): @@ -505,11 +629,9 @@ def test_install_agent_happy_path_and_option_forwarding(wired): assert outcome.plugin.repo_id == PLUGIN_REPO assert outcome.plugin.version == "9.9.9" - # Importable only now: load_plugin put the directory on sys.path. - import e2e_plugin - - assert e2e_plugin.CALLS[-1][:2] == ("install", "owner/my-agent") - forwarded = e2e_plugin.CALLS[-1][2] + entry = loaded(wired, "e2e_plugin") + assert entry.CALLS[-1][:2] == ("install", "owner/my-agent") + forwarded = entry.CALLS[-1][2] # Unset optionals are dropped so the plugin applies its own defaults, but a # False boolean is a decision the caller made and is forwarded. assert {key: forwarded[key] for key in ("dry_run", "yes", "force", "quiet")} == { @@ -535,7 +657,7 @@ def test_install_agent_happy_path_and_option_forwarding(wired): plugin_repo=PLUGIN_REPO, trust_remote_code=True, ) - forwarded = e2e_plugin.CALLS[-1][2] + forwarded = entry.CALLS[-1][2] assert forwarded["name"] == "sub" assert forwarded["local_dir"] == "/tmp/ws" assert forwarded["dest"] == "/tmp/ws" @@ -705,7 +827,7 @@ def fetch_only(wired, monkeypatch): ) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) yield directory - sys.modules.pop("fetch_plugin", None) + sys.modules.pop(_plugin._module_alias(spec_for(directory)), None) def test_install_agent_drives_a_fetch_only_plugin(fetch_only): @@ -720,9 +842,7 @@ def test_install_agent_drives_a_fetch_only_plugin(fetch_only): assert outcome.ok, outcome.error assert outcome.operation == "fetch_raw" - import fetch_plugin - - call = fetch_plugin.CALLS[-1] + call = loaded(fetch_only, "fetch_plugin").CALLS[-1] assert call["repo"] == "owner/my-agent" assert call["framework"] == "qwenpaw" assert Path(call["dest"]).parent.name == "agent-staging" @@ -739,9 +859,7 @@ def test_install_agent_maps_local_dir_onto_dest(fetch_only): ) assert outcome.ok, outcome.error - import fetch_plugin - - assert fetch_plugin.CALLS[-1]["dest"] == "/tmp/joint/staging" + assert loaded(fetch_only, "fetch_plugin").CALLS[-1]["dest"] == "/tmp/joint/staging" def test_dest_is_not_forwarded_to_an_operation_that_does_not_accept_it(wired, monkeypatch): @@ -771,7 +889,7 @@ def download(repo, *, local_dir=None, dry_run=False): assert outcome.ok, outcome.error assert outcome.operation == "download" - import legacy_plugin - - assert legacy_plugin.CALLS[-1] == {"repo": "owner/my-agent", "local_dir": "/tmp/ws"} - sys.modules.pop("legacy_plugin", None) + assert loaded(directory, "legacy_plugin").CALLS[-1] == { + "repo": "owner/my-agent", + "local_dir": "/tmp/ws", + } From a71914c9e84ff0f72d891c6929e233c41eb64e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Sun, 20 Sep 2026 18:10:25 +0800 Subject: [PATCH 08/10] [Feature] Make the owner allow-list the whole authorisation The allow-list is compile-time and names only official organisations, and nothing lets a caller point this command at a plugin whose owner is not on it. A per-invocation opt-in on top of that asks the user to confirm a decision that was already made by whoever shipped the release, and implies third-party plugins are supported when they are not. So --trust-remote-code and MODELSCOPE_AGENT_TRUST_REMOTE_CODE are both gone, and an allow-listed plugin is downloaded and executed with no confirmation. Support for third-party plugins, and an opt-in with it, is deferred; a test pins that neither the flag nor the variable comes back silently. The environment variable goes for a second reason: it applied to every install in the process, so an environment the user did not build could opt them in. The warning added for that case is gone with it. What replaces the gate is an audit line, logged before the import so a crash during it still leaves a record of which build was being loaded. require_trust had nothing left to require, so it is replaced by log_execution rather than left as a function that always returns. Also makes the two paths where a plugin is downloaded and verified but never runs say so. "failed to load plugin " and "exposes none of install, fetch_raw, download" both read like a network or configuration problem, and neither tells the user the two things that matter: the package fetched fine, and no agent was fetched or installed. Both now say "downloaded and verified, but was NOT executed". --dry-run was the other thing that looked like an inspect-only mode and is not: it still downloads, imports and calls the plugin, and only the plugin's writes are suppressed. Its help said "change nothing", and the README offered omitting the trust flag as the way to inspect a plugin without executing it -- that mode no longer exists, so both are corrected rather than left describing a behaviour the command does not have. README drops the authoring section and the third gate: only official plugins are supported, so documenting how to write one advertised a path that cannot be used. The package contract is not lost, it moved to the _plugin module docstring where a maintainer will find it. The install section is 63 lines, down from 92 before this feature, and the PR's README diff is +73 rather than +104. 1039 tests pass; the one failure is the pre-existing test_compat_constants_completeness. --- README.md | 50 ++-------- src/modelscope_hub/agent/_plugin.py | 150 +++++++++++++++++++--------- src/modelscope_hub/cli/agent.py | 36 +++---- src/modelscope_hub/constants.py | 17 ---- tests/cli/test_agent_install.py | 40 ++++---- tests/test_agent_plugin.py | 111 ++++++++++++-------- 6 files changed, 212 insertions(+), 192 deletions(-) diff --git a/README.md b/README.md index 2e263e2..21655cb 100644 --- a/README.md +++ b/README.md @@ -34,8 +34,8 @@ The official Python SDK & CLI for [ModelScope Hub](https://modelscope.cn) — do ## News **Unreleased** -- **Feature**: `ms-hub agent install -r owner/name` fetches a framework plugin from a model repository and delegates to the entry operation it declares, plus the `modelscope_hub.agent.install_agent` SDK entry. The hub gains no framework knowledge; see [Writing a plugin](#writing-a-plugin). -- **Quality**: loading a plugin executes code this package did not ship, so it is gated by a compile-time owner allow-list checked before any download, a `content_sha256` manifest verified against every file before import, and a `--trust-remote-code` opt-in that is never persisted. +- **Feature**: `ms-hub agent install -r owner/name` fetches the official framework plugin from a model repository and delegates to the entry operation it declares, plus the `modelscope_hub.agent.install_agent` SDK entry. The hub gains no framework knowledge. +- **Quality**: loading a plugin executes code this package did not ship, so it is gated by a compile-time owner allow-list checked before any download and a `content_sha256` manifest verified against every file before import. **v0.4.0** (2026-09-01) - **Feature**: complete OpenAPI coverage for Agent-IDP, MCP, and Studios — Agent Ed25519 identities, OIDC discovery/JWKS and signed JWT issuance (`HubApi`, `ms-hub agent-idp`); Studio lists, variables and configuration options; hosted MCP discovery; protected visibility and runtime metadata; read-only tokens can log in and rejected writes name the required tier. Agent private JWKs are only written to an explicitly requested owner-only file. @@ -650,7 +650,7 @@ Remote agent repositories: raw file transfer (`download`, `upload`, `list`) and ms-hub agent download -r user/my-agent --local-dir ./my-agent # download raw files ms-hub agent upload -r user/my-agent --local-dir ./my-agent # upload raw ms-hub agent install -r user/my-agent \ - --plugin-repo modelscope/agent-hub-plugin --trust-remote-code # install into the framework + # install into the framework ``` `download` / `upload` / `list` transfer files as-is, with **no framework awareness**. @@ -698,7 +698,7 @@ ms-hub agent upload -r user/my-agent --local-dir ./my-agent --dry-run Download an agent and hand it to its **framework plugin**. What the plugin does with it is negotiated: one with an `install` entry point places the agent into the framework's workspace, one that only transports bytes writes the repository's files into a destination directory and leaves placement to whatever runs next. The command reports which happened (`Installed …` vs `Fetched …`). ```bash -ms-hub agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin --trust-remote-code +ms-hub agent install -r user/my-agent ms-hub agent install -r user/my-agent --plugin-revision v0.3.1 -n sub-agent --local-dir ~/ws ``` @@ -707,11 +707,10 @@ ms-hub agent install -r user/my-agent --plugin-revision v0.3.1 -n sub-agent --lo | `-r, --repo REPO` | yes | Agent repository to install (`owner/name`) | | `--plugin-repo OWNER/NAME` | no | Plugin model repository. Resolution: this flag, then `$MODELSCOPE_AGENT_PLUGIN_REPO`, then the built-in default `modelscope/agent-hub-plugin`. The owner must be on the allow-list either way | | `--plugin-revision REV` | no | Plugin revision (default: `master`; pin a tag for reproducible installs) | -| `--trust-remote-code` | no | Required to import and run the plugin, unless `$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1` | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | | `--framework FW` | no | Override the plugin's framework detection | | `--local-dir DIR` | no | Where the agent repository is **downloaded** — not where it is installed. An installing plugin then places the agent in the framework's own home (`~/.ms_agent`, `~/.qwenpaw`) and leaves the download in place, since a directory you named is never treated as scratch. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` | -| `--dry-run` | no | Ask the plugin to report instead of change anything. The plugin is still **imported**, so its module-level code runs; to inspect one without executing it, omit `--trust-remote-code` | +| `--dry-run` | no | Ask the plugin to report instead of change anything. The plugin is still downloaded, imported and run — only its writes are suppressed | | `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | Exit codes: `0` success, `2` a gate refused or the command line is wrong, and otherwise **the plugin's own code** — the install layer gives `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed) and `6` (framework mismatch) distinct meanings, and collapsing them to `1` would discard the only machine-readable signal a caller has. @@ -729,44 +728,18 @@ Installed user/my-agent `entry` is the line to read when a result looks incomplete: `Installed` means the framework was touched, `Fetched … to ` means only that files are on disk. `planned` names operations the plugin declares but has not implemented; calling one returns `ok=False` naming the release rather than failing obscurely. -Omit `--trust-remote-code` to inspect a plugin's scope **without executing any of it**: downloading runs nothing, so the command fetches the package, verifies the manifest, prints repository, revision, version, entry module, frameworks, operations, planned work and the manifest digest, then stops before the import. - ##### Security model -Importing a plugin executes code this package did not ship, so it is gated three times, cheapest first: - -1. **Known source.** Defaults to `modelscope/agent-hub-plugin`; `--plugin-repo` or `$MODELSCOPE_AGENT_PLUGIN_REPO` overrides it. That is a convenience, not a bypass — whichever id wins still passes the allow-list. -2. **Owner allow-list**, checked *before* anything is downloaded. It is a **compile-time constant** (`modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS`, currently `modelscope` and `AI-ModelScope`) with deliberately **no environment override**: this list is the trust anchor for a command that runs downloaded code, and an anchor any parent process can rewrite is not an anchor. Widening it is a reviewed code change. Matching is case-insensitive because that is how the registry treats identity — it resolves `ModelScope/x` and `modelscope/x` to one repository — so an exact comparison would not stop a look-alike, it would only reject the casing copied from the website. -3. **Trust opt-in**, checked after the manifest is verified so the refusal can show exactly what is about to run. A flag or an environment variable, **never persisted**. Satisfying it through the environment logs a warning, because that variable applies to every install in the process and an environment you did not build can opt you in. Opting in also hands the plugin your `--endpoint` and your API token, since it needs credentials to fetch the agent. - -Between gates 2 and 3, `plugin.json`'s `content_sha256` is checked against every file on disk, in both directions, and a key pointing outside the package is refused. Be precise about what that buys: it proves the bytes are the bytes the manifest described, and it makes the digest in the trust prompt meaningful, so what you agreed to and what gets imported cannot diverge. **It is not authenticity** — the manifest is unsigned and ships beside the code it describes, so whoever controls the repository controls the hashes. The trust anchor is the allow-list plus the opt-in. The hub's own file listing is not used for this either: it has been observed reporting a git blob SHA-1 in a `sha256` field. - -##### Writing a plugin - -A plugin is a **model** repository (`snapshot_download` rejects `repo_type='agent'`) with `plugin.json` at its root beside an importable package — `my_plugin/__init__.py`, or a single `my_plugin.py` — named by `entry_module`: - -`plugin.json` — two fields are required, the rest are display only: - -```json -{ - "entry_module": "my_plugin", - "content_sha256": {"my_plugin/__init__.py": "", "...": "..."}, - "version": "1.0.0", - "frameworks": ["qwenpaw"], - "api": ["install"], - "roadmap": {"upload": "P1"} -} -``` - -`content_sha256` must cover **every** file in the package except `plugin.json` itself (it cannot hash itself), `.gitattributes` (the hub injects it) and `__pycache__`. Anything missing, mismatched or unlisted is refused. `version` / `frameworks` / `api` / `roadmap` are never validated — they feed the trust prompt and the `scope :` line. +Importing a plugin executes code this package did not ship, so where it may come from is decided **at compile time**, not at the command line: -The entry module must expose at least one of `install`, `fetch_raw`, `download`, tried in that order. Export `capabilities()` returning `{"operations": [...], "frameworks": [...], "planned": {...}}`: it is authoritative, so a name you ship but did not implement is skipped instead of selected. Without it, selection falls back to "first callable attribute wins", which will pick a stub. A `capabilities()` that raises is an error, not an empty declaration. +1. **Owner allow-list.** A constant in `modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS` — currently `modelscope` and `AI-ModelScope` — checked *before* anything is downloaded, so a source outside it is refused without touching the network. There is deliberately **no environment override**: this list is the authorisation for running downloaded code, and an anchor any parent process can rewrite is not an anchor. Widening it is a reviewed code change. Matching is case-insensitive because that is how the registry treats identity — it resolves `ModelScope/x` and `modelscope/x` to one repository — so an exact comparison would not stop a look-alike, it would only reject the casing copied from the website. +2. **Manifest integrity.** `plugin.json`'s `content_sha256` is checked against every file on disk before import, in both directions, and a key pointing outside the package is refused. Be precise about what that buys: it proves the bytes are the bytes the manifest described. **It is not authenticity** — the manifest is unsigned and ships beside the code it describes, so whoever controls the repository controls the hashes. The allow-list is what vouches for origin. The hub's own file listing is not used for this either: it has been observed reporting a git blob SHA-1 in a `sha256` field. -Your operation is called with keyword arguments narrowed to its signature, from: `repo`, `name`, `framework`, `source_framework`, `local_dir`, `dest`, `dry_run`, `yes`, `force`, `quiet`, `endpoint`, `token`. Unset optionals are dropped so your defaults apply; `False` booleans are kept; `dest` is always resolved (`--local-dir`, else a staging directory). Accept `**kwargs` to be forward-compatible. +There is **no per-invocation confirmation**. An allow-listed plugin is downloaded and run, because the decision to trust it was made by whoever shipped this release rather than by the user at the prompt. Which build is about to execute is logged before the import, and reported as `plugin:` / `entry:` / `scope:` lines afterwards. -Return an object carrying **`ok`** — required, not defaulted, because it is the only signal deciding whether the user is told the agent was installed. Add `error` and `exit_code` on failure, and `files_written` plus `root` for the success message. Raising is also fine: it becomes `ok=False`, exit 1. +The plugin receives your `--endpoint` and your API token, since it needs credentials to fetch the agent. -Two constraints follow from how loading works. Use **relative imports** inside your package: it is registered under a directory-scoped alias, not its own name, so two plugins cannot be served each other's cached code. And do not assume the plugin directory is still on `sys.path` after your operation returns — it is scoped to the call, for the reason given above. Your staging directory is yours to clean up, not the hub's. +Only official plugins are supported. Third-party plugin support — and with it a user-facing opt-in such as `--trust-remote-code` — is deferred; the package format is documented for maintainers in the `modelscope_hub.agent._plugin` module docstring. ##### Python API @@ -938,7 +911,6 @@ Token is persisted locally after `ms-hub login` and auto-loaded in subsequent se | `MODELSCOPE_CACHE` | `~/.cache/modelscope` | Local cache directory | | `MODELSCOPE_HOME` | `~/.modelscope` | SDK config directory | | `MODELSCOPE_AGENT_PLUGIN_REPO` | `modelscope/agent-hub-plugin` | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; `--plugin-repo` wins over it | -| `MODELSCOPE_AGENT_TRUST_REMOTE_CODE` | `false` | Let `ms-hub agent install` execute plugin code without `--trust-remote-code` | | `MODELSCOPE_PREFER_AI_SITE` | `false` | Prefer `modelscope.ai` over `modelscope.cn` | **Network:** diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index 0a16628..3aebb6a 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -9,16 +9,73 @@ it cannot know where such a plugin should write, and the plugin deliberately has no default of its own. +Trust model +----------- +The **owner allow-list is the authorisation**. It is a compile-time constant +naming only official organisations, and nothing lets a caller point this command +at a plugin whose owner is not on it, so by the time a package has been fetched +the decision to run it was already made by whoever shipped this release. There is +consequently no per-invocation opt-in: an allow-listed plugin is always downloaded +*and* executed, and there is no inspect-only mode. A ``--trust-remote-code`` style +flag is deferred to whichever release supports third-party plugins; exposing one +now would imply a choice the allow-list has already made. + Integrity comes from ``plugin.json``'s ``content_sha256``, not from the hub's own file listing -- that listing has been observed reporting a git blob SHA-1 in a ``sha256`` field. Be precise about what that buys: it proves the bytes on disk are -the bytes the manifest described, and it gives the trust prompt a stable -fingerprint, so the decision and the import cannot diverge. It is **not** -authenticity. The manifest ships inside the same unsigned repository as the code -it describes, so whoever controls the repository controls the hashes and can make -anything verify. The trust anchor is the owner allow-list plus the opt-in; nothing -here vouches for who wrote the plugin. Signing would change that and is not done -yet. +the bytes the manifest described, and it gives the audit line in +:func:`log_execution` a stable fingerprint. It is **not** authenticity. The +manifest ships inside the same unsigned repository as the code it describes, so +whoever controls the repository controls the hashes and can make anything verify. +The allow-list is what vouches for the plugin's origin; nothing here vouches for +its contents beyond "unchanged since it was listed". Signing would change that and +is not done yet. + +Plugin package contract +----------------------- +Maintainer-facing record of the format; it is deliberately not in the README, +which documents only the supported path of installing an official plugin. + +A plugin is a **model** repository (``snapshot_download`` rejects +``repo_type='agent'``) with ``plugin.json`` at its root beside an importable +package or module named by ``entry_module``. + +``plugin.json`` -- two fields are required, the rest are display only and never +validated: + +* ``entry_module`` (str) -- imported from the download root via + ``sys.path.insert(0, root)``, so relative imports inside a package work. +* ``content_sha256`` (dict) -- sha256 of every file, keyed by posix path relative + to the root. Checked in both directions: missing, mismatched and unlisted files + all fail. Exempt: ``plugin.json`` itself (it cannot hash itself), + ``.gitattributes`` (the hub injects it) and ``__pycache__``. Keys are validated + as paths before use, since they are attacker-controlled. +* ``version``, ``frameworks``, ``api``, ``roadmap`` -- feed :meth:`PluginSpec.describe` + and :meth:`PluginSpec.scope`, nothing else. + +The entry module must expose at least one of ``install``, ``fetch_raw``, +``download``, tried in that order. ``capabilities()`` returning +``{"operations": [...], "frameworks": [...], "planned": {...}}`` is authoritative +when present, so a name that is shipped but not implemented is skipped rather than +selected; when it is absent, selection falls back to presence, and when it raises +that is an error rather than an empty declaration. + +The chosen operation is called with keyword arguments narrowed to its signature, +from: ``repo``, ``name``, ``framework``, ``source_framework``, ``local_dir``, +``dest``, ``dry_run``, ``yes``, ``force``, ``quiet``, ``endpoint``, ``token``. +Unset optionals are dropped so the plugin's own defaults apply; ``False`` booleans +are kept; ``dest`` is always resolved. Its return value must carry ``ok`` -- +required, not defaulted, because it is the only signal deciding whether the user +is told the agent was installed -- plus ``error`` and ``exit_code`` on failure and +``files_written`` / ``root`` for the success message. Raising is also handled. + +Two constraints follow from how loading works. The package must use **relative +imports** internally, because it is registered under a directory-scoped alias +rather than its own name so two plugins cannot be served each other's cached +code. And it must not assume its directory stays on ``sys.path`` after the +operation returns: the entry is scoped to the call, since a directory parked at +``sys.path[0]`` lets any file it ships shadow the standard library. Cleaning up +its own staging directory is the plugin's job, not this module's. """ from __future__ import annotations @@ -296,34 +353,22 @@ def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: return manifest -def require_trust(spec: PluginSpec, *, trust_remote_code: bool) -> None: - """Refuse to import the plugin unless execution was opted into. +def log_execution(spec: PluginSpec) -> None: + """Record which build is about to be imported, before it is imported. + + There is no per-invocation opt-in to wait for any more. The owner allow-list + is the authorisation: it is compile-time, it names only official + organisations, and nothing lets a user point this command at a plugin whose + owner is not on it. Executing downloaded code still deserves an audit line + naming the exact build, emitted *before* the import so that a crash during it + leaves a trace of what was being loaded. - The refusal lists what *would* run so the decision is informed. The opt-in is - a flag or an environment variable and is never persisted -- "allow this code - to run" is not a preference worth remembering on the user's behalf. + A per-invocation opt-in (``--trust-remote-code``) is deliberately deferred: + it belongs with third-party plugins, which this release does not support, and + exposing it now would imply a choice the allow-list has already made. + Reintroducing it means gating here again and refusing instead of logging. """ - if trust_remote_code: - return - if constants.AGENT_TRUST_REMOTE_CODE: - logger.warning( - "Executing plugin %s@%s because %s is set, not because this invocation " - "asked for it. That variable applies to every install in this process, " - "so an environment you did not build can opt you in.", - spec.repo_id, - spec.revision, - constants.ENV_AGENT_TRUST_REMOTE_CODE, - ) - return - raise NotSupportedError( - "refusing to execute plugin code without an explicit opt-in. The plugin " - "resolved to:\n" + spec.describe() + "\n\n" - "Opting in does two things: it imports and runs that code, and it passes " - "the plugin your --endpoint and your API token, since it needs credentials " - "to fetch the agent. Only continue if you trust the owner to hold both.\n\n" - "Re-run with --trust-remote-code to import and run it, or set " - f"{constants.ENV_AGENT_TRUST_REMOTE_CODE}=1." - ) + logger.info("Executing agent plugin:\n%s", spec.describe()) def _module_alias(spec: PluginSpec) -> str: @@ -516,24 +561,29 @@ def install_agent( quiet: bool = False, plugin_repo: str | None = None, plugin_revision: str | None = None, - trust_remote_code: bool = False, endpoint: str | None = None, token: str | None = None, cache_dir: str | None = None, ) -> InstallOutcome: """Fetch or install *repo*'s agent through its framework plugin. - Which of the two happens is the plugin's answer, not this function's: the - entry operation is negotiated in :func:`select_operation`, so a plugin that - installs into the workspace installs, and one that only transports bytes + An allow-listed plugin is always downloaded **and executed** -- there is no + inspect-only mode. Authorisation is the compile-time owner allow-list, not a + per-invocation opt-in, so by the time this function is past + :func:`assert_trusted_owner` the decision has already been made by whoever + shipped this package. + + Which of fetch or install happens is the plugin's answer, not this function's: + the entry operation is negotiated in :func:`select_operation`, so a plugin + that installs into the workspace installs, and one that only transports bytes stages them in ``local_dir`` (or :func:`default_staging_dir`) for the install layer to place. *repo* is passed through uninterpreted beyond requiring ``owner/name``. - Plugin failures come back as data (``ok`` False); the three gates - (:func:`resolve_plugin_repo`, :func:`assert_trusted_owner`, - :func:`require_trust`) raise instead, so the CLI can map a misconfigured - command line to exit 2 and keep it distinct from a failed install. + Plugin failures come back as data (``ok`` False); the two gates + (:func:`resolve_plugin_repo`, :func:`assert_trusted_owner`) raise instead, so + the CLI can map a misconfigured command line to exit 2 and keep it distinct + from a failed install. """ if not repo or not repo.strip(): raise InvalidParameter("--repo is required, in 'owner/name' form.") @@ -572,7 +622,7 @@ def install_agent( manifest=manifest, entry_module=str(manifest["entry_module"]), ) - require_trust(spec, trust_remote_code=trust_remote_code) + log_execution(spec) # The plugin directory is importable for exactly as long as the plugin runs, # not for the rest of the process -- see :func:`plugin_syspath`. @@ -580,12 +630,22 @@ def install_agent( try: module = load_plugin(spec) operation, func = select_operation(module) - except NotSupportedError: - raise + except NotSupportedError as exc: + # Imported but nothing was runnable. Say that the download succeeded + # and that no agent work happened, or the message reads like a + # network failure and sends the user off checking the wrong thing. + raise NotSupportedError( + f"plugin {plugin_repo_id}@{spec.revision} downloaded and verified, but was " + f"NOT executed -- no agent was fetched or installed. {exc}" + ) from exc except Exception as exc: return InstallOutcome( ok=False, - error=f"failed to load plugin {plugin_repo_id}: {exc.__class__.__name__}: {exc}", + error=( + f"plugin {plugin_repo_id}@{spec.revision} downloaded and verified, but could " + f"not be imported, so it was NOT executed and no agent was fetched or " + f"installed: {exc.__class__.__name__}: {exc}" + ), plugin=spec, exit_code=1, ) diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index a1081fb..2102797 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -24,7 +24,6 @@ AGENT_PLUGIN_TRUSTED_OWNERS, DEFAULT_AGENT_PLUGIN_REPO, ENV_AGENT_PLUGIN_REPO, - ENV_AGENT_TRUST_REMOTE_CODE, Visibility, ) from ..errors import APIError @@ -273,7 +272,6 @@ def _cmd_install( quiet, plugin_repo, plugin_revision, - trust_remote_code, endpoint, token, ) -> int: @@ -299,7 +297,6 @@ def _cmd_install( quiet=quiet, plugin_repo=plugin_repo, plugin_revision=plugin_revision, - trust_remote_code=trust_remote_code, endpoint=endpoint, token=token, ) @@ -345,7 +342,7 @@ def register(subparsers: SubParsers) -> None: " download -r REPO [--local-dir DIR] [--revision REV]\n" " upload -r REPO [--local-dir DIR] [--revision REV] [--dry-run]\n" " list [--owner OWNER] [--page N] [--page-size N]\n" - " install -r REPO --plugin-repo OWNER/NAME --trust-remote-code\n" + " install -r REPO [--plugin-repo OWNER/NAME]\n" " [-n NAME] [--framework FW] [--local-dir DIR] [--plugin-revision REV]\n" " [--dry-run] [-y] [--force] [-q]\n" "\n" @@ -357,8 +354,7 @@ def register(subparsers: SubParsers) -> None: " ms agent download -r user/my-agent --local-dir ./my-agent\n" " ms agent upload -r user/my-agent --local-dir ./my-agent\n" " ms agent list --owner user\n" - " ms agent install -r user/my-agent --plugin-repo modelscope/agent-hub-plugin \\\n" - " --trust-remote-code\n" + " ms agent install -r user/my-agent\n" ) agent_parser = subparsers.add_parser( "agent", @@ -448,15 +444,14 @@ def register(subparsers: SubParsers) -> None: "reports which of the two happened.\n\n" "Supported scope comes from the plugin, not from this package, so it cannot go stale here: " "every run prints a 'scope :' line naming the frameworks that plugin build covers, the " - "operations it implements, and the ones it declares as not yet available. Run without " - "--trust-remote-code to see that summary plus the resolved plugin and its manifest digest " - "without executing any downloaded code.\n\n" - f"Loading a plugin imports code this package did not ship, so its owner must be on a " - f"compile-time allow-list ({', '.join(sorted(AGENT_PLUGIN_TRUSTED_OWNERS))}) and execution " - f"requires --trust-remote-code (or {ENV_AGENT_TRUST_REMOTE_CODE}=1). Opting in also hands " - f"the plugin your --endpoint and your API token, because it needs credentials to fetch the " - f"agent. The plugin itself defaults to {DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo or " - f"{ENV_AGENT_PLUGIN_REPO} overrides it, but the owner still has to be on the allow-list." + "operations it implements, and the ones it declares as not yet available.\n\n" + f"The plugin is official code selected by a compile-time owner allow-list " + f"({', '.join(sorted(AGENT_PLUGIN_TRUSTED_OWNERS))}), which is checked before anything is " + f"downloaded and is the whole authorisation -- an allow-listed plugin is fetched and run, " + f"with no separate confirmation. It defaults to {DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo " + f"or {ENV_AGENT_PLUGIN_REPO} picks a different one, but its owner still has to be listed. " + f"The plugin receives your --endpoint and API token, since it needs credentials to fetch " + f"the agent." ), ) p_install.add_argument( @@ -489,17 +484,11 @@ def register(subparsers: SubParsers) -> None: default=None, help="Plugin revision to fetch (default: master; pin a tag for reproducible installs)", ) - p_install.add_argument( - "--trust-remote-code", - action="store_true", - help="Allow the downloaded plugin to be imported and executed. Without it (or " - "$MODELSCOPE_AGENT_TRUST_REMOTE_CODE=1) the command reports what it would run and stops.", - ) p_install.add_argument( "--dry-run", action="store_true", - help="Ask the plugin to report instead of change anything. The plugin is still imported, so its " - "module-level code runs; to inspect one without executing it, omit --trust-remote-code", + help="Ask the plugin to report instead of change anything. The plugin is still downloaded, " + "imported and run -- only its writes are suppressed", ) p_install.add_argument("-y", "--yes", action="store_true", help="Answer the plugin's prompts yes") p_install.add_argument("--force", action="store_true", help="Let the plugin overwrite an existing agent") @@ -572,7 +561,6 @@ def execute(self) -> None: quiet=args.quiet, plugin_repo=args.plugin_repo, plugin_revision=args.plugin_revision, - trust_remote_code=args.trust_remote_code, endpoint=endpoint, token=token, ) diff --git a/src/modelscope_hub/constants.py b/src/modelscope_hub/constants.py index 69615b7..5bfdc0a 100644 --- a/src/modelscope_hub/constants.py +++ b/src/modelscope_hub/constants.py @@ -961,7 +961,6 @@ def get_upload_ignore_file_pattern() -> str | None: # :mod:`modelscope_hub.agent._plugin`. # --------------------------------------------------------------------------- ENV_AGENT_PLUGIN_REPO: str = "MODELSCOPE_AGENT_PLUGIN_REPO" -ENV_AGENT_TRUST_REMOTE_CODE: str = "MODELSCOPE_AGENT_TRUST_REMOTE_CODE" #: Owners allowed to provide the agent plugin. #: @@ -985,24 +984,9 @@ def get_upload_ignore_file_pattern() -> str | None: "Model repository id ('owner/name') of the agent plugin used by 'ms agent install'", "Core", ) -_env_register( - ENV_AGENT_TRUST_REMOTE_CODE, - "false", - "Let 'ms agent install' execute plugin code without --trust-remote-code", - "Core", -) - -AGENT_TRUST_REMOTE_CODE: bool = _env_bool( - ENV_AGENT_TRUST_REMOTE_CODE, - False, - "Let 'ms agent install' execute plugin code without --trust-remote-code", - "Core", -) - __all__ = [ "AGENT_PLUGIN_TRUSTED_OWNERS", - "AGENT_TRUST_REMOTE_CODE", "API_CONNECT_TIMEOUT", "API_MAX_RETRIES", "API_TIMEOUT", @@ -1031,7 +1015,6 @@ def get_upload_ignore_file_pattern() -> str | None: "DOWNLOAD_RETRY_TIMES", "DOWNLOAD_TIMEOUT", "ENV_AGENT_PLUGIN_REPO", - "ENV_AGENT_TRUST_REMOTE_CODE", "ENV_FILE_LOCK", "ENV_CACHE", "ENV_INTRA_CLOUD_ACCELERATION", diff --git a/tests/cli/test_agent_install.py b/tests/cli/test_agent_install.py index 76d6bda..e5dbb69 100644 --- a/tests/cli/test_agent_install.py +++ b/tests/cli/test_agent_install.py @@ -60,14 +60,13 @@ def install(repo, **kwargs): PLUGIN_REPO, "--plugin-revision", "v1.0.0", - "--trust-remote-code", "--dry-run", "-y", "--force", "-q", ] -MINIMAL = ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO, "--trust-remote-code"] +MINIMAL = ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO] def build_plugin(root: Path, *, entry_module: str = "cli_fake_plugin") -> Path: @@ -106,9 +105,7 @@ def spec(revision: str = "v1.0.0") -> PluginSpec: @pytest.fixture(autouse=True) def _clean_env(monkeypatch): monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({TRUSTED, "modelscope"})) - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) - monkeypatch.delenv(constants.ENV_AGENT_TRUST_REMOTE_CODE, raising=False) yield sys.modules.pop("cli_fake_plugin", None) @@ -140,13 +137,11 @@ def test_parser_wires_install(parser): "/tmp/ws", ) assert (args.plugin_repo, args.plugin_revision) == (PLUGIN_REPO, "v1.0.0") - assert args.trust_remote_code is True assert args.dry_run and args.yes and args.force and args.quiet def test_parser_install_defaults(parser): args = parser.parse_args(["agent", "install", "-r", AGENT_REPO]) - assert args.trust_remote_code is False assert args.plugin_repo is None assert args.plugin_revision is None assert args.dry_run is False @@ -167,7 +162,6 @@ def test_forwards_every_option_to_the_sdk(stub_sdk): assert stub_sdk["local_dir"] == "/tmp/ws" assert stub_sdk["plugin_repo"] == PLUGIN_REPO assert stub_sdk["plugin_revision"] == "v1.0.0" - assert stub_sdk["trust_remote_code"] is True assert stub_sdk["dry_run"] and stub_sdk["yes"] assert stub_sdk["force"] and stub_sdk["quiet"] @@ -204,7 +198,7 @@ def refused(repo_id, **kwargs): raise NotSupportedError(f"failed to download agent plugin {repo_id}@master: record not found") monkeypatch.setattr(_plugin, "fetch_plugin", refused) - code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--trust-remote-code"]) + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO]) assert code != 0 combined = out + err assert constants.DEFAULT_AGENT_PLUGIN_REPO in combined @@ -214,9 +208,7 @@ def refused(repo_id, **kwargs): def test_untrusted_plugin_owner_exits_2(): - code, out, err = run_cli( - ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", "evil/plugin", "--trust-remote-code"] - ) + code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--plugin-repo", "evil/plugin"]) assert code == 2 assert "evil" in err combined = out + err @@ -226,27 +218,29 @@ def test_untrusted_plugin_owner_exits_2(): assert "compile-time" in combined -def test_trust_gate_refuses_and_explains(monkeypatch, tmp_path): - """Without the opt-in the command stops before importing, and says what it - would have run.""" +def test_an_allow_listed_plugin_runs_with_no_confirmation(monkeypatch, tmp_path): + """The allow-list is the whole authorisation, so an allow-listed plugin is + downloaded and executed with nothing for the user to confirm -- and the + command no longer accepts a flag that would imply otherwise.""" directory = build_plugin(tmp_path) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO]) - assert code == 2 - combined = out + err - for expected in ("--trust-remote-code", PLUGIN_REPO, "9.9.9", "cli_fake_plugin"): - assert expected in combined + assert code == 0, err + assert "Installed" in out -def test_trust_gate_can_be_satisfied_by_env(monkeypatch, tmp_path): +def test_the_trust_flag_is_gone(monkeypatch, tmp_path): + """Deferred with third-party plugin support. Passing it must be an argparse + error rather than a silently ignored extra, so nobody believes they opted in.""" directory = build_plugin(tmp_path) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", True) - code, out, err = run_cli(["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO]) - assert code == 0, err - assert "Installed" in out + code, out, err = run_cli( + ["agent", "install", "-r", AGENT_REPO, "--plugin-repo", PLUGIN_REPO, "--trust-remote-code"] + ) + assert code != 0 + assert "unrecognized arguments" in (out + err) or "--trust-remote-code" in (out + err) # --------------------------------------------------------------------------- diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index 7d80d82..6938226 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -323,40 +323,30 @@ def test_verify_manifest_refuses_keys_pointing_outside_the_package(tmp_path, key # --------------------------------------------------------------------------- -# require_trust +# execution audit / no opt-in # --------------------------------------------------------------------------- -def test_require_trust_blocks_without_opt_in(tmp_path, monkeypatch): - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) - with pytest.raises(NotSupportedError) as excinfo: - _plugin.require_trust(spec_for(make_plugin(tmp_path)), trust_remote_code=False) - # The refusal must say what would have run, not just "no". - message = str(excinfo.value) - for expected in (PLUGIN_REPO, "9.9.9", "fake_plugin", "--trust-remote-code"): - assert expected in message - - -@pytest.mark.parametrize("via", ["flag", "env"]) -def test_require_trust_allows_with_flag_or_env(tmp_path, monkeypatch, via): - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", via == "env") - _plugin.require_trust( - spec_for(make_plugin(tmp_path)), - trust_remote_code=(via == "flag"), - ) - - -def test_require_trust_warns_when_the_environment_opted_in(tmp_path, monkeypatch, caplog): - """The variable applies to every install in the process, so an environment the - user did not build can opt them in without a decision being made here. The - flag path stays silent: that one was a choice.""" - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", True) - with caplog.at_level("WARNING", logger="modelscope_hub.agent"): - _plugin.require_trust(spec_for(make_plugin(tmp_path)), trust_remote_code=False) - assert constants.ENV_AGENT_TRUST_REMOTE_CODE in caplog.text - - caplog.clear() - with caplog.at_level("WARNING", logger="modelscope_hub.agent"): - _plugin.require_trust(spec_for(make_plugin(tmp_path / "b")), trust_remote_code=True) - assert caplog.text == "" +def test_log_execution_records_the_build_before_import(tmp_path, caplog): + """There is no opt-in to wait for any more -- the allow-list is the + authorisation -- but executing downloaded code still deserves an audit line + naming the exact build, emitted before the import so a crash during it leaves + a trace of what was being loaded.""" + spec = spec_for(make_plugin(tmp_path)) + with caplog.at_level("INFO", logger="modelscope_hub.agent"): + _plugin.log_execution(spec) + for expected in (spec.repo_id, spec.revision, spec.entry_module, "9.9.9"): + assert expected in caplog.text + + +def test_no_trust_opt_in_is_exposed(): + """Deferred on purpose: the allow-list already decided, so a per-invocation + flag would imply a choice the user does not have. Pin that it stays gone until + third-party plugins are supported.""" + import inspect + + assert not hasattr(constants, "AGENT_TRUST_REMOTE_CODE") + assert not hasattr(constants, "ENV_AGENT_TRUST_REMOTE_CODE") + assert not hasattr(_plugin, "require_trust") + assert "trust_remote_code" not in inspect.signature(_plugin.install_agent).parameters # --------------------------------------------------------------------------- @@ -469,7 +459,7 @@ def test_plugin_syspath_is_scoped_to_the_block(tmp_path): def test_install_agent_leaves_no_plugin_directory_on_sys_path(wired): - _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) assert str(wired) not in sys.path @@ -611,7 +601,6 @@ def variadic(**kwargs): def wired(monkeypatch, tmp_path): """Point ``install_agent`` at a real plugin tree with the network stubbed.""" monkeypatch.setattr(constants, "AGENT_PLUGIN_TRUSTED_OWNERS", frozenset({TRUSTED})) - monkeypatch.setattr(constants, "AGENT_TRUST_REMOTE_CODE", False) monkeypatch.delenv(constants.ENV_AGENT_PLUGIN_REPO, raising=False) # install_agent resolves a default staging directory under the cache; keep it # out of the real user home. @@ -623,7 +612,7 @@ def wired(monkeypatch, tmp_path): def test_install_agent_happy_path_and_option_forwarding(wired): - outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) assert outcome.ok, outcome.error assert (outcome.operation, outcome.exit_code) == ("install", 0) assert outcome.plugin.repo_id == PLUGIN_REPO @@ -655,7 +644,6 @@ def test_install_agent_happy_path_and_option_forwarding(wired): endpoint="https://pre.modelscope.cn", token="tok", plugin_repo=PLUGIN_REPO, - trust_remote_code=True, ) forwarded = entry.CALLS[-1][2] assert forwarded["name"] == "sub" @@ -681,7 +669,7 @@ def no_network(*args, **kwargs): monkeypatch.setattr(compat, "snapshot_download", no_network) with pytest.raises(InvalidParameter): - _plugin.install_agent(repo, plugin_repo=PLUGIN_REPO, trust_remote_code=True) + _plugin.install_agent(repo, plugin_repo=PLUGIN_REPO) def test_install_agent_reports_plugin_failure(wired, monkeypatch): @@ -708,7 +696,7 @@ def install(repo, **kwargs): directory = make_plugin(wired.parent, dirname="fail_plugin", entry_module="fail_plugin", entry_source=source) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) - outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) assert not outcome.ok assert outcome.error == "framework not installed" # The install layer's own codes (3/4/5/6) carry meaning and must survive. @@ -730,13 +718,51 @@ def install(repo, **kwargs): directory = make_plugin(wired.parent, dirname="boom_plugin", entry_module="boom_plugin", entry_source=source) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) - outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) assert not outcome.ok assert "boom" in outcome.error assert outcome.exit_code == 1 sys.modules.pop("boom_plugin", None) +def test_an_unimportable_plugin_says_it_was_downloaded_but_not_run(wired, monkeypatch): + """The download and the manifest check both passed, so a bare "failed to load" + reads like a network problem and hides the two facts that matter: the package + is fine, and no agent work happened.""" + source = "import definitely_not_installed_xyz\n" + directory = make_plugin(wired.parent, dirname="bad_import", entry_module="bad_import", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) + assert not outcome.ok + assert outcome.exit_code == 1 + assert "downloaded and verified" in outcome.error + assert "NOT executed" in outcome.error + assert "no agent was fetched or installed" in outcome.error + + +def test_a_plugin_with_no_usable_operation_says_it_was_not_run(wired, monkeypatch): + source = textwrap.dedent( + """ + def capabilities(): + return {"operations": ()} + + def install(repo, **kwargs): + raise AssertionError("must not be chosen") + """ + ).lstrip() + directory = make_plugin(wired.parent, dirname="no_op", entry_module="no_op", entry_source=source) + monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) + + with pytest.raises(NotSupportedError) as excinfo: + _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) + message = str(excinfo.value) + assert "downloaded and verified" in message + assert "NOT executed" in message + assert "no agent was fetched or installed" in message + sys.modules.pop("no_op", None) + + @pytest.mark.parametrize("returned", ["None", "'done'", "{}"]) def test_install_agent_refuses_a_result_without_ok(wired, monkeypatch, returned): """``ok`` is the only signal that decides whether the user is told the agent @@ -755,7 +781,7 @@ def install(repo, **kwargs): directory = make_plugin(wired.parent, dirname="no_ok_plugin", entry_module="no_ok_plugin", entry_source=source) monkeypatch.setattr(_plugin, "fetch_plugin", lambda repo_id, **kwargs: directory) - outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO, trust_remote_code=True) + outcome = _plugin.install_agent("owner/my-agent", plugin_repo=PLUGIN_REPO) assert not outcome.ok assert outcome.exit_code == 1 assert "no 'ok' attribute" in outcome.error @@ -837,7 +863,6 @@ def test_install_agent_drives_a_fetch_only_plugin(fetch_only): "owner/my-agent", framework="qwenpaw", plugin_repo=PLUGIN_REPO, - trust_remote_code=True, ) assert outcome.ok, outcome.error assert outcome.operation == "fetch_raw" @@ -855,7 +880,6 @@ def test_install_agent_maps_local_dir_onto_dest(fetch_only): "owner/my-agent", local_dir="/tmp/joint/staging", plugin_repo=PLUGIN_REPO, - trust_remote_code=True, ) assert outcome.ok, outcome.error @@ -884,7 +908,6 @@ def download(repo, *, local_dir=None, dry_run=False): "owner/my-agent", local_dir="/tmp/ws", plugin_repo=PLUGIN_REPO, - trust_remote_code=True, ) assert outcome.ok, outcome.error assert outcome.operation == "download" From 98c4da672c8c53181df8e2d0e1042c4d42cd3b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Sun, 20 Sep 2026 18:39:55 +0800 Subject: [PATCH 09/10] [Doc] Stop documenting the opt-in that was removed The README's Python API example still passed trust_remote_code=True to install_agent, which no longer accepts it, so the documented call raised TypeError. Two docstrings also cross-referenced require_trust, deleted when the allow-list became the whole authorisation, leaving broken :func: links. Naming the deferred flag in user-facing docs invites users to reach for a choice the allow-list has already made, so it is now described only as a deferred capability; the maintainer-facing module docstring keeps the rationale. A test asserts the flag name stays out of the README. --- README.md | 4 ++-- src/modelscope_hub/agent/_plugin.py | 15 ++++++++------- tests/test_agent_plugin.py | 6 ++++++ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 21655cb..41e62be 100644 --- a/README.md +++ b/README.md @@ -739,14 +739,14 @@ There is **no per-invocation confirmation**. An allow-listed plugin is downloade The plugin receives your `--endpoint` and your API token, since it needs credentials to fetch the agent. -Only official plugins are supported. Third-party plugin support — and with it a user-facing opt-in such as `--trust-remote-code` — is deferred; the package format is documented for maintainers in the `modelscope_hub.agent._plugin` module docstring. +Only official plugins are supported; the allow-list is the whole authorisation and there is nothing for a user to opt into. Support for plugins from other owners is deferred, and the package format is documented for maintainers in the `modelscope_hub.agent._plugin` module docstring. ##### Python API ```python from modelscope_hub.agent import install_agent -outcome = install_agent("user/my-agent", plugin_revision="v0.3.1", trust_remote_code=True) +outcome = install_agent("owner/my-agent", plugin_revision="v0.3.1") print(outcome.ok, outcome.operation, outcome.exit_code, outcome.error) ``` diff --git a/src/modelscope_hub/agent/_plugin.py b/src/modelscope_hub/agent/_plugin.py index 3aebb6a..3033564 100644 --- a/src/modelscope_hub/agent/_plugin.py +++ b/src/modelscope_hub/agent/_plugin.py @@ -244,8 +244,9 @@ def fetch_plugin( ) -> Path: """Download the plugin package and return its directory. - Transfer executes nothing, so fetching an untrusted plugin is safe; only the - import is gated, by :func:`require_trust`. + Transfer executes nothing, and the owner gate in :func:`assert_trusted_owner` + has already run by the time this is reached -- a package outside the + allow-list is refused without touching the network. """ from ..compat import snapshot_download @@ -281,11 +282,11 @@ def verify_manifest(directory: Path, repo_id: str) -> dict[str, Any]: Strict on purpose, and worth being clear about what strictness buys: it proves the files on disk are the files the manifest described, and it makes the - digest shown by :func:`require_trust` mean something, so what the user agreed - to and what gets imported cannot diverge. It does not prove anything about - authorship -- the manifest is unsigned and ships beside the code it describes, - so a repository's owner can make any content verify. That is the owner - allow-list's job. + digest in the :func:`log_execution` audit line mean something, so what was + recorded as about to run and what actually got imported cannot diverge. It + does not prove anything about authorship -- the manifest is unsigned and ships + beside the code it describes, so a repository's owner can make any content + verify. That is the owner allow-list's job. """ manifest_path = directory / MANIFEST_NAME if not manifest_path.is_file(): diff --git a/tests/test_agent_plugin.py b/tests/test_agent_plugin.py index 6938226..f32e6c0 100644 --- a/tests/test_agent_plugin.py +++ b/tests/test_agent_plugin.py @@ -348,6 +348,12 @@ def test_no_trust_opt_in_is_exposed(): assert not hasattr(_plugin, "require_trust") assert "trust_remote_code" not in inspect.signature(_plugin.install_agent).parameters + # Nor in the user-facing docs: naming an opt-in that does not exist invites + # users to reach for it, and a stale example is a call that raises TypeError. + readme = (Path(__file__).resolve().parents[1] / "README.md").read_text(encoding="utf-8") + assert "trust_remote_code" not in readme + assert "trust-remote-code" not in readme + # --------------------------------------------------------------------------- # fetch_plugin From b71c45d2d065fd57a4bde45ed646ab9642a902ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=A8=E5=A0=83?= Date: Sun, 20 Sep 2026 21:43:29 +0800 Subject: [PATCH 10/10] [Doc] Keep the install docs to what a user needs The README carried the migration knobs and design rationale alongside the reference: the plugin-repo env var and flag rows, a second example that taught nothing the options table does not, the plugin's own home directories under --local-dir, and an overview paragraph restating the section below it. The --help description made the same points at 187 words. Docs now cover the supported path only. The env var and --plugin-repo stay in the code -- the plugin has no home under an official organisation yet, so the override is what development and the regression scripts run on -- but they left the user-facing surfaces; the error hint still names them when the built-in default is not published. --- README.md | 34 +++++++++++------------------ src/modelscope_hub/cli/agent.py | 38 +++++++++++++-------------------- 2 files changed, 27 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 41e62be..18294d0 100644 --- a/README.md +++ b/README.md @@ -649,13 +649,12 @@ Remote agent repositories: raw file transfer (`download`, `upload`, `list`) and ```bash ms-hub agent download -r user/my-agent --local-dir ./my-agent # download raw files ms-hub agent upload -r user/my-agent --local-dir ./my-agent # upload raw -ms-hub agent install -r user/my-agent \ - # install into the framework +ms-hub agent install -r user/my-agent # install into the framework ``` `download` / `upload` / `list` transfer files as-is, with **no framework awareness**. -`install` is different: it does not know any framework's file layout either. It resolves *which* plugin to use, fetches that plugin from a model repository, verifies it against its own manifest, and hands the agent id over — the plugin decides how the agent is registered and what the framework needs afterwards, and which entry operation runs is negotiated from what the plugin declares. See [`ms-hub agent install`](#ms-hub-agent-install) for the security model. +`install` is different: it fetches an official plugin and hands the agent id to it, leaving every framework decision to the plugin. See [`ms-hub agent install`](#ms-hub-agent-install) for the details and the security model. > **Framework-aware operations** (cross-framework `convert`, `watch`/bidirectional sync, `status`, `backups`, `restore`, `stop`) live in **[modelscope-agent](https://github.com/modelscope/ms-agent)** — use `ms-agent agent ...` instead. For example, to download and convert in one step: `ms-agent agent download -f qoder -r user/my-agent --target-framework qwenpaw`. @@ -695,29 +694,27 @@ ms-hub agent upload -r user/my-agent --local-dir ./my-agent --dry-run #### `ms-hub agent install` -Download an agent and hand it to its **framework plugin**. What the plugin does with it is negotiated: one with an `install` entry point places the agent into the framework's workspace, one that only transports bytes writes the repository's files into a destination directory and leaves placement to whatever runs next. The command reports which happened (`Installed …` vs `Fetched …`). +Download an agent and hand it to its **framework plugin**. A plugin with an `install` entry point places the agent into the framework's workspace; one that only transports bytes writes the repository's files into a destination directory and leaves placement to whatever runs next. The command reports which happened (`Installed …` vs `Fetched …`). ```bash ms-hub agent install -r user/my-agent -ms-hub agent install -r user/my-agent --plugin-revision v0.3.1 -n sub-agent --local-dir ~/ws ``` | Option | Required | Description | |--------|----------|-------------| | `-r, --repo REPO` | yes | Agent repository to install (`owner/name`) | -| `--plugin-repo OWNER/NAME` | no | Plugin model repository. Resolution: this flag, then `$MODELSCOPE_AGENT_PLUGIN_REPO`, then the built-in default `modelscope/agent-hub-plugin`. The owner must be on the allow-list either way | | `--plugin-revision REV` | no | Plugin revision (default: `master`; pin a tag for reproducible installs) | | `-n, --name NAME` | no | Sub-agent name, passed through to the plugin | | `--framework FW` | no | Override the plugin's framework detection | -| `--local-dir DIR` | no | Where the agent repository is **downloaded** — not where it is installed. An installing plugin then places the agent in the framework's own home (`~/.ms_agent`, `~/.qwenpaw`) and leaves the download in place, since a directory you named is never treated as scratch. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` | +| `--local-dir DIR` | no | Where the agent repository is **downloaded**, not where it is installed. Omitted, downloads go to `$MODELSCOPE_CACHE/agent/agent-staging/---/` and are cleaned up on success | | `--dry-run` | no | Ask the plugin to report instead of change anything. The plugin is still downloaded, imported and run — only its writes are suppressed | | `-y, --yes` / `--force` / `-q, --quiet` | no | Passed through to the plugin | -Exit codes: `0` success, `2` a gate refused or the command line is wrong, and otherwise **the plugin's own code** — the install layer gives `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed) and `6` (framework mismatch) distinct meanings, and collapsing them to `1` would discard the only machine-readable signal a caller has. +Exit codes: `0` success, `2` a gate refused or the command line is wrong, otherwise **the plugin's own code** — the install layer uses `3` (already exists), `4` (refused to overwrite), `5` (install or self-check failed), `6` (framework mismatch). ##### Supported scope -This package supports **no frameworks**; which agents it can handle is a property of the plugin build it fetches. So the authoritative list is printed every run rather than maintained here, where it would go stale: +This package supports **no frameworks**; what can be installed is a property of the plugin build it fetches, so the authoritative list is printed every run: ``` plugin: modelscope/agent-hub-plugin@v0.3.1 (version 0.3.1) @@ -726,20 +723,18 @@ scope : frameworks ms-agent, qwenpaw | operations fetch_raw, install, list_backu Installed user/my-agent ``` -`entry` is the line to read when a result looks incomplete: `Installed` means the framework was touched, `Fetched … to ` means only that files are on disk. `planned` names operations the plugin declares but has not implemented; calling one returns `ok=False` naming the release rather than failing obscurely. +`planned` names operations the plugin declares but has not implemented; calling one returns `ok=False` naming the planned release. ##### Security model -Importing a plugin executes code this package did not ship, so where it may come from is decided **at compile time**, not at the command line: +Where the plugin may come from is fixed at compile time, not at the command line: -1. **Owner allow-list.** A constant in `modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS` — currently `modelscope` and `AI-ModelScope` — checked *before* anything is downloaded, so a source outside it is refused without touching the network. There is deliberately **no environment override**: this list is the authorisation for running downloaded code, and an anchor any parent process can rewrite is not an anchor. Widening it is a reviewed code change. Matching is case-insensitive because that is how the registry treats identity — it resolves `ModelScope/x` and `modelscope/x` to one repository — so an exact comparison would not stop a look-alike, it would only reject the casing copied from the website. -2. **Manifest integrity.** `plugin.json`'s `content_sha256` is checked against every file on disk before import, in both directions, and a key pointing outside the package is refused. Be precise about what that buys: it proves the bytes are the bytes the manifest described. **It is not authenticity** — the manifest is unsigned and ships beside the code it describes, so whoever controls the repository controls the hashes. The allow-list is what vouches for origin. The hub's own file listing is not used for this either: it has been observed reporting a git blob SHA-1 in a `sha256` field. +1. **Owner allow-list** — `modelscope_hub.constants.AGENT_PLUGIN_TRUSTED_OWNERS`, currently `modelscope` and `AI-ModelScope`. Checked before anything is downloaded, with no environment override and no per-invocation confirmation: an allow-listed plugin is fetched and run. Widening the list is a reviewed code change. +2. **Manifest integrity** — `plugin.json`'s `content_sha256` is verified against the files on disk before import. That proves the bytes are the ones the manifest described; it is **not** authenticity, since the manifest is unsigned and ships beside the code it describes. Origin rests on the allow-list alone. -There is **no per-invocation confirmation**. An allow-listed plugin is downloaded and run, because the decision to trust it was made by whoever shipped this release rather than by the user at the prompt. Which build is about to execute is logged before the import, and reported as `plugin:` / `entry:` / `scope:` lines afterwards. +Which build is about to run is logged before the import. The plugin receives your `--endpoint` and your API token, since it needs credentials to fetch the agent. -The plugin receives your `--endpoint` and your API token, since it needs credentials to fetch the agent. - -Only official plugins are supported; the allow-list is the whole authorisation and there is nothing for a user to opt into. Support for plugins from other owners is deferred, and the package format is documented for maintainers in the `modelscope_hub.agent._plugin` module docstring. +Plugins from other owners are not supported. The package format is documented for maintainers in the `modelscope_hub.agent._plugin` module docstring. ##### Python API @@ -750,10 +745,6 @@ outcome = install_agent("owner/my-agent", plugin_revision="v0.3.1") print(outcome.ok, outcome.operation, outcome.exit_code, outcome.error) ``` -The steps are exported individually (`resolve_plugin_repo`, `assert_trusted_owner`, `fetch_plugin`, `verify_manifest`, `load_plugin`, `plugin_syspath`, `select_operation`, `default_staging_dir`) for callers that want to inspect a plugin without running it. - -A `fetch_raw`-style plugin deliberately has **no default destination**: the only sensible-looking default is a framework workspace, and a workspace holds the user's own credentials (`agent.json` channels, `settings.json` providers, `mcp.json` env blocks) that an overwrite would silently destroy. The hub resolves `dest` for it instead. - ### `ms-hub agent-idp` @@ -910,7 +901,6 @@ Token is persisted locally after `ms-hub login` and auto-loaded in subsequent se | `MODELSCOPE_ENDPOINT` | `https://modelscope.cn` | API endpoint URL | | `MODELSCOPE_CACHE` | `~/.cache/modelscope` | Local cache directory | | `MODELSCOPE_HOME` | `~/.modelscope` | SDK config directory | -| `MODELSCOPE_AGENT_PLUGIN_REPO` | `modelscope/agent-hub-plugin` | Model repository (`owner/name`) of the agent plugin used by `ms-hub agent install`; `--plugin-repo` wins over it | | `MODELSCOPE_PREFER_AI_SITE` | `false` | Prefer `modelscope.ai` over `modelscope.cn` | **Network:** diff --git a/src/modelscope_hub/cli/agent.py b/src/modelscope_hub/cli/agent.py index 2102797..0aced6c 100644 --- a/src/modelscope_hub/cli/agent.py +++ b/src/modelscope_hub/cli/agent.py @@ -23,7 +23,6 @@ from ..constants import ( AGENT_PLUGIN_TRUSTED_OWNERS, DEFAULT_AGENT_PLUGIN_REPO, - ENV_AGENT_PLUGIN_REPO, Visibility, ) from ..errors import APIError @@ -436,22 +435,16 @@ def register(subparsers: SubParsers) -> None: help="Install an agent into its framework via the agent plugin", formatter_class=RawDescriptionHelpFormatter, description=( - "Download an agent repository and hand it to the framework plugin.\n\n" - "What the plugin does with it is negotiated, not assumed: a plugin with an install entry " - "point places the agent into the framework's workspace and completes the framework's own " - "registration steps, while one that only transports bytes writes the repository's files " - "into a destination directory and leaves placement to whatever runs next. The command " - "reports which of the two happened.\n\n" - "Supported scope comes from the plugin, not from this package, so it cannot go stale here: " - "every run prints a 'scope :' line naming the frameworks that plugin build covers, the " - "operations it implements, and the ones it declares as not yet available.\n\n" - f"The plugin is official code selected by a compile-time owner allow-list " - f"({', '.join(sorted(AGENT_PLUGIN_TRUSTED_OWNERS))}), which is checked before anything is " - f"downloaded and is the whole authorisation -- an allow-listed plugin is fetched and run, " - f"with no separate confirmation. It defaults to {DEFAULT_AGENT_PLUGIN_REPO}; --plugin-repo " - f"or {ENV_AGENT_PLUGIN_REPO} picks a different one, but its owner still has to be listed. " - f"The plugin receives your --endpoint and API token, since it needs credentials to fetch " - f"the agent." + "Download an agent repository and hand it to the framework plugin. A plugin with an " + "install entry point places the agent into the framework's workspace; one that only " + "transports bytes writes the files into a destination directory and leaves placement " + "to whatever runs next. Every run prints a 'scope :' line with what that plugin build " + "supports.\n\n" + f"The plugin is official code chosen by a compile-time owner allow-list " + f"({', '.join(sorted(AGENT_PLUGIN_TRUSTED_OWNERS))}), checked before any download and " + f"the whole authorisation: an allow-listed plugin is fetched and run with no separate " + f"confirmation. It receives your --endpoint and API token, since it needs credentials " + f"to fetch the agent." ), ) p_install.add_argument( @@ -467,17 +460,16 @@ def register(subparsers: SubParsers) -> None: p_install.add_argument( "--local-dir", default=None, - help="Where the agent repository is downloaded. A plugin that only fetches leaves the files " - "there and stops; one that installs then places the agent in the framework's own home " - "(e.g. ~/.ms_agent, ~/.qwenpaw) and leaves the download behind, since a directory you named " - "is never treated as scratch. Omitted, downloads go to " + help="Where the agent repository is downloaded, not where it is installed: an installing " + "plugin still puts the agent in the framework's own home (e.g. ~/.ms_agent, ~/.qwenpaw) " + "and leaves your directory alone. Omitted, downloads go to " "$MODELSCOPE_CACHE/agent/agent-staging/ and are cleaned up on success.", ) p_install.add_argument( "--plugin-repo", default=None, - help=f"Plugin model repository, owner/name (default: ${ENV_AGENT_PLUGIN_REPO}, else " - f"{DEFAULT_AGENT_PLUGIN_REPO}). Its owner must be on the allow-list.", + help=f"Plugin model repository, owner/name (default: {DEFAULT_AGENT_PLUGIN_REPO}). " + f"Its owner must be on the allow-list.", ) p_install.add_argument( "--plugin-revision",