Conversation
added 5 commits
September 16, 2026 18:14
`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.
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/<owner>--<name>-<timestamp>, 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 <dir>" 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.
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.
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.
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.
| # :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" |
Collaborator
There was a problem hiding this comment.
这里是否是环境变量可配的，是否会发生有恶意脚本写了MODELSCOPE_AGENT_PLUGIN_TRUSTED_OWNERS，然后驱动用户拉下来恶意 plugin 的问题？如果没有必要这里可以不作为环境变量
| #: 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" |
| "Core", | ||
| ) | ||
|
|
||
| AGENT_PLUGIN_TRUSTED_OWNERS: frozenset[str] = _env_csv_frozenset_exact( |
Collaborator
There was a problem hiding this comment.
对于 constant 文件来说，这些逻辑有点太复杂了
| ``` | ||
|
|
||
| Reading that, for the plugin published at the time of writing: | ||
|
|
added 5 commits
September 20, 2026 11:08
`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.
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.
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 <repo>" 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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
ms agent install -r owner/name, which downloads an agent into its framework's local workspace by delegating to a framework plugin published as a model repository, plus themodelscope_hub.agent.install_agentSDK entry behind it.ms agent install -r user/my-agent \ --plugin-repo modelscope/agent-hub-plugin --trust-remote-codeThe 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 are all the plugin's decisions.
Why
installbelongs in a command group documented as "no framework awareness"cli/agent.pystates that framework-aware operations live in modelscope-agent.installis framework-dependent but not framework-aware: it never reads a workspace layout, a file pattern or a framework's config format. It reads a repository id, fetches a package, and calls one function. The distinction is what keeps the boundary intact, and the module docstring now says so explicitly. Flagging it here because it is the first question a reviewer should ask of this PR.Security model
Importing a plugin executes code this distribution did not ship, so the path is gated three times, in increasing order of cost:
--plugin-repoorMODELSCOPE_AGENT_PLUGIN_REPO. There is 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._env_csv_frozensetupper-cases its items, which would letmushenlpass a list trustingmushenL, hence the new_env_csv_frozenset_exact.Between gates 2 and 3,
plugin.json'scontent_sha256is 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 as the integrity source: it has been observed returning a git blob SHA-1 (40 hex) in a
sha256field for some content, which makes it unreliable. Reproduction and analysis are in the plugin publisher's notes; happy to file it separately if useful.Design points worth a reviewer's opinion
python -m <entry> ..., results over stdout JSON) would isolate it, at the cost of ~0.3s startup and result serialisation. Went with the in-process form because the gates above are the documented contract andtrust_remote_codeis an explicit acknowledgement; the stricter form is a small change if maintainers prefer it.DEFAULT_AGENT_PLUGIN_TRUSTED_OWNERS = "mushenL,modelscope"—mushenLis where the plugin is published today. This is an interim value and should almost certainly becomemodelscopeonly (or empty) before merge. Called out explicitly rather than buried in a constant.installis preferred,downloadaccepted as a fallback, andcapabilities()['operations']is authoritative when declared — so a plugin that ships 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 a plugin adding keywords does not break older hubs.Two bugs found by running against a published plugin, not by review
.gitattributesinto every repository. It is never in an author's manifest, so the unlisted-file check made every real package fail verification. Now exempt, alongsideplugin.json(cannot hash itself) and__pycache__(written locally by a previous import) — and the exemption is exact, a genuinely unlisted file is still refused."/" in repocheck let/nonameandowner/through to a real API call. Both now rejected by the shared_split_repo_id, with a test asserting no network is reached.Testing
53 tests, mock-only per CI's
MODELSCOPE_RUN_REMOTE_TESTS=false. The 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; onlysnapshot_downloadis stubbed.Gate logic is tested once at function level and once through the CLI for exit-code mapping, with no assertion duplicated across the two levels.
Gates run locally:
ruff check src/ tests/ruff format --check(changed files)mypy src/modelscope_hub/main(11 pre-existing from missingtypes-requestsin my env)pytest tests/ -k "not remote" --ignore=tests/integrationThe single failure in my environment,
test_compat_constants_completeness.py::test_matches_installed_modelscope_when_available, reproduces on unmodifiedmain— it compares against an installedmodelscope1.39.1 whoseUPLOAD_REACT_ROUND3_FILE_DELAYdiffers. Not related to this change.Also 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 any download; the allow-list override takes effect.
Out of scope
download/upload/list.agent/_api.pypermission work from [Fix] Fix Agent Permission Handling and MCP Pagination #66 is untouched;repo_info,create_repoandlist_repo_filessignatures are unchanged.Diff shape
7 files, +1596 / −15. Three are new (
agent/_plugin.pyand two test files); the four modified files lose 15 lines between them, mostly docstring and epilog text, so existing behaviour is not rewritten.