feat(extensions): interactive multi-tab /extensions manager (Installed / Discover / Sources) - #4850
Conversation
…rketplaces) Upgrade the /extensions management dialog from a linear wizard into a multi-tab interactive dialog aligned with Claude Code's /plugin command. UI (packages/cli): - Discover: pull installable plugins from configured marketplaces, multi-select (Space), batch install (i) with Global/Project/Local scope, open homepage, per-plugin details. - Installed: plugins + standalone MCP servers grouped by Favorites/Local/User/Project/Disabled; Space toggles enable/disable, f toggles favorite, Enter opens details with an action menu (toggle/favorite/mark-for-update/update/uninstall). - Marketplaces: add/list/view/remove marketplace sources (owner/repo, SSH, HTTP JSON, local path). - Tabbed shell with Tab/arrow switching and a focus-lock contract so a tab owns Escape while in a sub-view. Core (packages/core): - ExtensionPreferencesStore: favorites + per-extension scope intent. - MarketplaceRegistryStore + discoverPlugins(): persistent marketplace source registry and cross-source discovery. - loadMarketplaceConfigFromSource() in marketplace.ts (GitHub/local/HTTP-JSON). - ExtensionManager methods for marketplaces, discovery, favorites and scopes; preference cleanup on uninstall. Scope mapping: Global -> User; Project/Local -> workspace-scoped enablement (install then re-scope so the choice actually restricts where it is active). The Errors tab is intentionally deferred per the spec. Tests: 19 core unit tests (preferences/registry/discovery) and 11 tabbed dialog integration tests; existing extension suites updated. typecheck, lint and i18n checks pass.
Rework the Discover tab's Enter detail view to match Claude Code's "Plugin details" page in both layout and interaction: - Layout: "Plugin details" header, title, "from <marketplace>", last updated / version, description, "By: <author>", a "Will install:" component summary (Skills/Commands/Agents/MCP servers), and a trust warning. - Interaction: the scope choice is now an inline action selector on the detail page (Install for you / for all collaborators / in this repo only / Open homepage / Back to plugin list), selected with Enter — replacing the previous i/h shortcuts and the separate scope step. - Footer shows "Enter to select · Esc to go back" while a tab sub-view is open. Core: DiscoveredPlugin now carries declared `components` and a best-effort `lastUpdated`, surfaced by discoverPlugins(). Adds a core test for component/lastUpdated extraction and a UI test for the detail layout + inline selector.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Match CC's "Add Marketplace" screen: a bold "Add Marketplace" header, an "Enter marketplace source:" prompt, and an "Examples:" bullet list (owner/repo · git@…:owner/repo.git (SSH) · https://…/marketplace.json · ./path/to/marketplace) above a bare cursor input (placeholder removed). Update the Marketplaces add-view tests accordingly.
…t with CC Discover reliability and UX fixes: - Fix the "Discovering plugins…" hang: marketplace network fetches had no timeout, so a slow/unreachable source could block discovery forever. Add a 10s per-request timeout (resolve null) plus socket drain on non-200. - Cache the fetched listing in ExtensionManager for the session so revisiting the tab no longer refetches over the network; `installed` flags are recomputed cheaply, and the cache is invalidated on add/remove marketplace. CC-aligned Discover list: - Windowed/scrolling viewport (no longer renders the entire 200+ list at once) with "↑ more above" / "↓ more below" hints, sized to the terminal. - Type-to-search filter with a search box and a "Discover plugins (pos/total)" count header. - Item layout: cursor "›", ○/●/✓ checkbox, bold title · marketplace · "<N> installs", with a truncated description line. - Space toggles selection, Enter views detail (or installs the selected set); the conflicting "i" shortcut was removed in favor of search. Core: DiscoveredPlugin gains a best-effort `installs` count. Adds tests for windowing, search filtering, and install-count extraction.
Rework the Marketplaces tab detail to match Claude Code:
- Show marketplace name, source, "N available plugins", and the plugins
from this marketplace that are installed ("Installed plugins (K):" with
descriptions) — instead of dumping a truncated list of all plugins.
- Replace the ad-hoc "d to remove" hint with an action selector:
Browse plugins (N) · Update marketplace [(last updated DATE)] ·
Remove marketplace.
- "Browse plugins" switches to the Discover tab filtered to this
marketplace (only its plugins); the filter clears on manual tab switch
and is shown in the Discover header.
- "Update marketplace" re-fetches the marketplace config, stamps a fresh
"last updated", and invalidates the discovery cache.
Core: MarketplaceSource gains lastUpdatedAt; addMarketplace stamps it and
ExtensionManager.markMarketplaceUpdated() refreshes it + clears the
discovery cache.
Adds tests for the marketplace detail layout and the Browse-to-Discover
filtering flow.
…on adds
- Installed list: plugin rows now show their type + version ("Extension
v0.7.0"), parallel to MCP rows ("MCP"), instead of a bare version.
- Add Marketplace: when the source is not a Claude marketplace but is a
valid single extension source (Gemini/Claude/git/npm), the error now
guides the user to install it directly ("... looks like a single
extension, not a marketplace. Install it with: /extensions install X")
instead of the generic "expected marketplace.json" message.
The Marketplaces add flow advertises git@github.com:owner/repo.git (SSH) as a supported format, but loadMarketplaceConfigFromSource relied on parseGitHubRepoForReleases, which rejects the git@ scp-like form. Extract owner/repo directly from the git@github.com:owner/repo(.git) form before falling back to the URL parser, so SSH marketplace sources actually resolve. Adds a regression test.
…ketplaces tab
- Terminology: use 'Extension' instead of 'Plugin' across the dialog
(Discover extensions, Extension details, Back to extension list, etc.).
- Tabs reordered to Installed, Discover, Marketplaces; the dialog now
opens on Installed by default.
- Marketplaces tab is now a sources hub:
- new 'Install new extension' action (installs a single Gemini/Qwen/
Claude/git/npm extension directly via parseInstallSource).
- 'Add new marketplace' annotated as a Claude plugin marketplace.
- items grouped into 'Extensions' and 'Marketplaces' sections; an
extension row opens a compact detail with Uninstall.
Updates the dialog tests for the new wording, tab order and layout.
… Marketplaces footer - Add a shared ExtensionActionsView (info + components + action menu + scope-select + uninstall-confirm) used by both the Installed and Marketplaces tabs, so the Marketplaces extension detail now offers the full set (Enable/Disable, Favorite, Mark for Update, Update Now, Uninstall) instead of just Uninstall/Back. - Add a new 'Change scope' action (Global/Project/Local) that re-scopes enablement (User vs workspace), available in both tabs. - Context-aware Marketplaces footer: shows 'Enter details' for an extension row, 'Enter open · d remove marketplace' for a marketplace row, and a neutral hint for the action rows — no longer says 'd remove marketplace' when an extension is selected. Adds a test for the full extension actions in the Marketplaces detail.
…+ fix enable/disable in Sources detail - Rename the user-visible tab label 'Marketplaces' -> 'Sources' (TabBar + TABS). The in-tab 'Marketplaces' section header (grouping marketplace sources) is kept. Also update the Discover empty-state hint to point at the 'Sources' tab. - Hide the Add/Remove Favorites action in the Sources extension detail via a showFavorite prop (default true; Installed keeps it). - Fix a stale enable/disable label in the Sources extension detail: ExtensionActionsView re-read enablement through the manager cache keyed by a tick, but refreshCache() briefly empties that cache, so the read raced and fell back to the stale extension prop (isActive: true). It now holds authoritative local state (enabled/isFavorite/scope) updated optimistically after each action — no cache read-back. The Installed tab was immune only because it fed a fresh extension object each load. Adds regression tests for the enable/disable toggle staying in sync and for change-scope re-scoping + re-enabling a disabled extension.
…elector - Add an 'Add new' section heading above the '+ Install new extension' and '+ Add new marketplace' rows on the Sources tab, so those two actions are grouped like the Extensions and Marketplaces sections. - In the Change scope selector, default the cursor to the extension's current scope and show a 'Current: <scope>' line. Previously it always defaulted to Global, so after changing scope it was unclear whether the change took effect. Applies to both the Sources and Installed extension detail (shared ExtensionActionsView). Updates tests to assert the 'Add new' section title renders and that re-entering the scope selector reflects the now-current scope.
…TW i18n - Move the 'Note: Uninstall permanently removes this extension.' warning out of the detail-view action list and into the uninstall confirmation step (replacing the near-synonymous 'This action cannot be undone.'). - Fix the Chinese/English mix in the extensions manager: the new multi-tab UI added ~104 English strings that had no locale entries, so they fell back to the English key at runtime. Add Simplified (zh) and Traditional (zh-TW) translations for all of them, plus the matching en.js base keys (en.js is the canonical superset; zh/zh-TW require strict key parity per check-i18n). Placeholders, keyboard tokens (Tab/Enter/Esc/Space/↑↓/·) and the ⚠ glyph are preserved across all locales.
Rename the Installed-tab scope group headers from User/Project/Local to 'X level' (用户级/项目级/本地级) so the grouping reads as scope levels. Adds the new keys to en/zh/zh-TW locales.
The Installed-tab MCP item detail was a read-only view (name/type/scope/ transport/status) with a meaningless 'Enter to select' and no actions. Replace it with McpServerActionsView, which reuses the /mcp dialog's ServerDetailStep, ToolListStep, ToolDetailStep and AuthenticateStep so the behaviour matches /mcp exactly: live connection status, View tools, Enable/Disable, Reconnect (when disconnected), Re-authenticate and Clear authentication. Handlers mirror MCPManagementDialog (mcp.excluded settings + toolRegistry discover/disable/disconnect + MCPOAuthTokenStorage). Delete the now-unused McpDetailView and its obsolete locale keys; add the two new status strings to en/zh/zh-TW.
Review follow-up: buildServer hardcoded promptCount to 0, diverging from /mcp's fetchServerData. Query the prompt registry like the original so the reused MCPServerDisplayInfo is computed consistently.
BZ-D
changed the title
…lace to source The Sources tab treats both single-extension sources and Claude plugin marketplaces as 'sources', so the source-management layer is renamed for consistency: MarketplaceSource -> ExtensionSource marketplaceRegistry(.ts) -> sourceRegistry(.ts) MarketplaceRegistryStore -> SourceRegistryStore add/get/remove/markMarketplaceUpdated -> add/get/remove/markSourceUpdated loadMarketplace/updateMarketplace -> loadSource/updateSource MarketplacesTab -> SourcesTab; EXTENSIONS_TABS.MARKETPLACES -> SOURCES + the source-detail UI handlers. Terms that refer to the Claude marketplace manifest *format* are kept, since a marketplace is one source type: ClaudeMarketplaceConfig, loadMarketplaceConfigFromSource, the .claude-plugin/marketplace.json path, DiscoveredPlugin.marketplaceName, and the in-tab 'Marketplaces' group label.
…vive the rename The source/* rename accidentally renamed the persisted registry file from marketplaces.json to sources.json, so previously added sources (e.g. a Claude marketplace) appeared to vanish — the data was intact in marketplaces.json but the code read sources.json. Restore the marketplaces.json filename for backward compatibility.
Previously runInstall always returned to the list after attempting an install. Now it only returns to the list on success; on failure it stays on the extension detail page so the error message remains visible and the user can retry without re-navigating.
| res.on('error', () => done(null)); | ||
| }); | ||
| req.on('error', () => done(null)); | ||
| req.setTimeout(MARKETPLACE_FETCH_TIMEOUT_MS, () => { |
There was a problem hiding this comment.
[Critical] req.setTimeout is a socket-idle timeout, not an absolute wall-clock deadline — the timer resets every time a chunk is received. A server trickling 1 byte every 9 seconds will never trigger the 10-second idle timer, causing fetchUrl (and thus the entire Discover tab via Promise.all across all sources) to hang indefinitely with no user-visible diagnostic beyond a permanent "Loading..." spinner.
| req.setTimeout(MARKETPLACE_FETCH_TIMEOUT_MS, () => { | |
| req.setTimeout(MARKETPLACE_FETCH_TIMEOUT_MS, () => { | |
| req.destroy(); | |
| done(null); | |
| }); | |
| // Hard wall-clock deadline — req.setTimeout only resets on socket activity. | |
| const hardTimer = setTimeout(() => { | |
| req.destroy(); | |
| done(null); | |
| }, MARKETPLACE_FETCH_TIMEOUT_MS); | |
| req.on('close', () => clearTimeout(hardTimer)); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — added an absolute wall-clock deadline (a setTimeout cleared in done()) alongside the existing socket-idle req.setTimeout.
| {extensions.length === 0 && sources.length === 0 ? ( | ||
| <Box marginTop={1}> | ||
| <Text color={theme.text.secondary}> | ||
| {t('No extensions or sources added yet.')} |
There was a problem hiding this comment.
[Critical] i18n key mismatch: this calls t('No extensions or sources added yet.') but all three locale dictionaries (en.js, zh.js, zh-TW.js) define the key as 'No extensions or marketplaces added yet.'. The translation lookup misses, so Chinese (zh/zh-TW) users see the raw English key string instead of the localized text.
| {t('No extensions or sources added yet.')} | |
| {t('No extensions or marketplaces added yet.')} |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Couldn't reproduce on current HEAD — the empty state renders t('No marketplaces added yet.') (defined in en/zh/zh-TW), and 'No extensions or sources added yet.' doesn't appear anywhere in the tree. Looks fixed in a push after the reviewed SHA.
…places
Some Claude marketplace plugins live in a subdirectory of a git repo and use a
'git-subdir' source ({url, path, ref, sha}), which resolvePluginSource didn't
handle — installing failed with 'Unsupported plugin source type'. Add the
git-subdir branch: clone the repo (pinned to ref/sha when provided) and return
the subdirectory as the plugin source.
Verified against github.com/42Crunch-AI/claude-plugins @ v1.5.5: the cloned
plugins/api-security-testing subdir is a valid plugin (.claude-plugin/plugin.json).
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] The new git-subdir source type in resolvePluginSource has zero test coverage. No existing tests exercise any object-type source (github, url, or git-subdir) — the function is only tested via local-directory paths. At minimum, add integration tests (via convertClaudePluginPackage) covering: successful clone with ref/sha/neither, subdirectory-not-found error, and clone-failure propagation.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| originSource: 'Claude', | ||
| }; | ||
| await cloneFromGit(installMetadata, pluginDir); | ||
| const subDir = path.join(pluginDir, source.path); |
There was a problem hiding this comment.
[Critical] Path traversal via unvalidated source.path. path.join(pluginDir, source.path) does not confine the resolved path to pluginDir — a malicious marketplace manifest can set path: "../../.ssh" to escape the sandbox. fs.existsSync only checks whether the escaped path exists, not whether it lies within pluginDir.
| const subDir = path.join(pluginDir, source.path); | |
| const subDir = path.resolve(pluginDir, source.path); | |
| const resolvedBase = path.resolve(pluginDir); | |
| if (!subDir.startsWith(resolvedBase + path.sep) && subDir !== resolvedBase) { | |
| throw new Error( | |
| `Plugin path "${source.path}" escapes the plugin directory`, | |
| ); | |
| } | |
| if (!fs.existsSync(subDir)) { |
Also validate that source.path is non-empty — an empty string causes path.join to return pluginDir itself, silently installing the repo root instead of a subdirectory.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — source.path is confined to the clone via path.resolve + startsWith(repoRoot + sep), and empty/./absolute values are rejected before use.
| if (source.source === 'git-subdir') { | ||
| // The plugin lives in a subdirectory of a git repository. Clone the repo | ||
| // (pinned to the provided ref/sha when present) and return the subdir. | ||
| const installMetadata: ExtensionInstallMetadata = { |
There was a problem hiding this comment.
[Critical] SHA-based clone incompatible with shallow clones on non-GitHub remotes. cloneFromGit does --depth 1 shallow clone then git.fetch(remote, sha). Fetching by SHA from a shallow clone requires server-side uploadpack.allowReachableSHA1InWant support, which self-hosted GitLab, Gitea, and Bitbucket Server may not enable. When a marketplace manifest specifies sha (the precise, immutable pin), installation silently fails with a generic Failed to clone Git repository error.
Consider either using git fetch --depth=... origin <sha> instead of the shallow-clone-then-fetch pattern, or documenting this limitation clearly so marketplace authors know SHA pins only work on GitHub.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Deferring — narrow edge (only when a manifest pins sha AND the remote lacks allowReachableSHA1InWant). I'll document the limitation / add an unshallow fallback in a follow-up.
| const installMetadata: ExtensionInstallMetadata = { | ||
| source: source.url, | ||
| type: 'git', | ||
| ref: source.ref || source.sha, |
There was a problem hiding this comment.
[Suggestion] ref silently takes precedence over sha when both are present. ref: source.ref || source.sha means sha is ignored if ref is truthy, but the TypeScript type allows both fields to coexist. A future maintainer reading the type may assume sha acts as a hard constraint, while the runtime silently discards it.
Consider either making ref and sha mutually exclusive in the type ({ ref?: string } | { sha?: string }), or adding a runtime check that warns when both are provided.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — now ref: source.sha || source.ref, so the immutable SHA pin takes precedence.
| const subDir = path.join(pluginDir, source.path); | ||
| if (!fs.existsSync(subDir)) { | ||
| throw new Error( | ||
| `Plugin subdirectory "${source.path}" not found in ${source.url}`, |
There was a problem hiding this comment.
[Suggestion] Error message for missing subdirectory does not include the ref/sha used for the clone. When source.path doesn't exist in the cloned repo, the error only reports the URL — if the user specified ref: "main" but the subdirectory only exists on a feature branch, they have no way to diagnose this from the error.
Include the ref in the error: `Plugin subdirectory "${source.path}" not found in ${source.url} (ref: ${source.ref || source.sha || 'HEAD'})`
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — the not-found error now includes (ref: ${source.ref ?? source.sha ?? 'HEAD'}).
Simplify the install/visibility scope model from three options (user / project / local) down to two (user / project). The 'local' option duplicated the workspace-level enablement of 'project' without providing a meaningfully different storage location, so it was UI clutter rather than a real feature. - core: ExtensionScope = 'user' | 'project'; read() filters unknown values via a type guard, so any stale 'local' (or otherwise invalid) entry in extension-preferences.json is dropped and falls back to 'user' downstream. - UI: remove the 'local' option from the Discover install menu, the change- scope picker, and the Installed tab's group ordering. - copy: rename 'Project (All Collaborators)' to 'Project (Workspace)' and 'Install for all collaborators on this repository' to 'Install for the current workspace', matching the new two-tier model. - i18n: clean up the now-unused 'Local *' keys in en / zh / zh-TW and retranslate the renamed keys. - tests: update the scope-change spec and replace the legacy-scope migration test with one that exercises the unknown-value filter.
| </Text> | ||
| </Box> | ||
| <Text bold color={titleColor}> | ||
| {plugin.name} |
There was a problem hiding this comment.
[Critical] ANSI escape sequence injection via marketplace plugin metadata. Plugin name, description, author, and marketplaceName from marketplace JSON are rendered directly inside Ink <Text> components without any sanitization. A malicious marketplace can embed ANSI escape codes (e.g., \x1b[2K\x1b[1A) in these fields to clear terminal lines, move the cursor, erase security warnings, or spoof UI elements.
This affects DiscoverTab, SourcesTab, and PluginDetailView — all render marketplace-sourced strings verbatim.
| {plugin.name} | |
| // Strip ANSI from all marketplace-sourced strings before rendering. | |
| // Use strip-ansi or a simple regex: /\x1b\[[0-9;]*[a-zA-Z]/g | |
| const safeName = plugin.name.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, ''); |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — marketplace strings are stripped of ANSI/VT/control chars at the pluginsFromConfig boundary (covers the Discover list + detail), and the Sources detail wraps untrusted names in stripUnsafeCharacters. Install resolution still uses the raw fields, so this is display-only.
| return; | ||
| } | ||
| const chunks: Buffer[] = []; | ||
| res.on('data', (chunk) => chunks.push(chunk)); |
There was a problem hiding this comment.
[Suggestion] Unbounded HTTP response body in fetchUrl enables OOM via malicious marketplace. The chunks array accumulates the entire response without any size cap. A malicious marketplace server can stream gigabytes of data within the 10-second timeout window, exhausting process memory.
Before this PR, fetchUrl was only called against GitHub API and raw.githubusercontent.com. Now it fetches arbitrary HTTPS URLs supplied as marketplace sources, and these are re-fetched every time the user opens the Discover tab.
| res.on('data', (chunk) => chunks.push(chunk)); | |
| const MAX_BODY = 5 * 1024 * 1024; // 5 MB | |
| let totalBytes = 0; | |
| res.on('data', (chunk) => { | |
| totalBytes += chunk.length; | |
| if (totalBytes > MAX_BODY) { | |
| req.destroy(); | |
| done(null); | |
| return; | |
| } | |
| chunks.push(chunk); | |
| }); |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — the response body is capped at 10 MB (the request is destroyed and resolves null once exceeded).
| try { | ||
| manager.setExtensionScope(name, newScope); | ||
| // Apply enablement: Global -> User; Project/Local -> workspace only. | ||
| if (newScope === 'user') { |
There was a problem hiding this comment.
[Suggestion] Scope change from project to user does not disable workspace enablement. When newScope === 'user', the code only calls enableExtension(name, SettingScope.User) but does NOT call disableExtension(name, SettingScope.Workspace) first. The reverse direction (user → project) correctly disables User then enables Workspace.
After a project-to-user scope change, the extension has enablement overrides at both scopes. If the user later disables the extension via the Installed tab (which targets User scope), the stale Workspace enablement persists and the extension remains active.
| if (newScope === 'user') { | |
| if (newScope === 'user') { | |
| await manager.disableExtension(name, SettingScope.Workspace); | |
| await manager.enableExtension(name, SettingScope.User); | |
| } else { |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
I believe this is intentional rather than a bug: switching to user scope is a global enable, and setExtensionScope already records the scope intent. The user→project branch disables User precisely because workspace-only enablement requires it. Happy to revisit with a concrete stale-override repro.
| if (src && src.source === 'url') { | ||
| return src.url; | ||
| } | ||
| return plugin.name; |
There was a problem hiding this comment.
[Suggestion] resolveInstallSource does not handle the git-subdir source variant. This PR adds a git-subdir variant to ClaudePluginSource (in claude-converter.ts), but resolveInstallSource only handles string, { source: 'github' }, and { source: 'url' }. A plugin with { source: 'git-subdir', url, path, ref? } falls through all conditions and returns just plugin.name — a bare name that is not a valid install source.
The Discover tab would show the plugin but installing it would fail with a parse error.
| return plugin.name; | |
| if (src && src.source === 'git-subdir') { | |
| return `${src.url}:${plugin.name}`; // or filter out during discovery | |
| } | |
| return plugin.name; |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Valid gap, but deferring: the suggested return src.url would clone+install the repo root, silently dropping path/ref — worse than today's clear Install source not found. Proper support needs the subdir/ref carried through the install metadata. Only affects git-subdir plugins served by an http-type marketplace; tracking as a follow-up.
There was a problem hiding this comment.
Thanks — I dug into this again rather than just re-deferring, and I think the return src.url one-liner would introduce a silent wrong-install, so I'd like to push back and propose a proper follow-up instead.
Why return src.url is unsafe here: a git-subdir source is { url, path, ref?, sha? } — the plugin lives in the subdirectory path of the repo, optionally pinned to ref/sha. But ExtensionInstallMetadata (config.ts:540) only has source / type / ref — there is no subdirectory field, and parseInstallSource has no subdir syntax. So return src.url drops both path and the pin, and the installer clones the repo root: it then either installs the whole repo as a standalone extension or (if the repo happens to ship its own marketplace.json) prompts the user to pick an unrelated plugin. In every case it installs something other than the subdir plugin the entry describes — strictly worse than today's explicit "not found" failure.
Scope of the actual gap: string- and github-form sources from an HTTP marketplace already install git-subdir plugins correctly, because their installSource is a repo:plugin form that routes back through convertClaudePluginPackage → resolvePluginSource, which fully handles git-subdir (now covered by the new tests in 158002c77). The unsupported case is narrowly a direct { source: 'git-subdir' } entry in an http-type (direct-JSON) marketplace.
Proper fix: thread the subdirectory + ref through the install pipeline — add a path/subdir field to ExtensionInstallMetadata and teach the git clone to check out the subdir at the pinned ref. That's a self-contained capability with its own tests, which I'd prefer to land as a follow-up rather than fold into this already-large PR. If you'd rather have it in-scope here, say the word and I'll add it (with tests) in the next push.
| item.name, | ||
| ]); | ||
| } | ||
| await toolRegistry?.disableMcpServer(item.name); |
There was a problem hiding this comment.
[Suggestion] toggleMcp enable path updates runtime excluded list but disable path does not. When enabling an MCP server, the code explicitly removes the server from the runtime excluded list via config.setExcludedMcpServers(...). When disabling, it only calls toolRegistry?.disableMcpServer(name) without updating the runtime excluded list.
Other code that checks config.getExcludedMcpServers() to determine if a server is disabled may get an incorrect answer after toggling disable from this UI.
| await toolRegistry?.disableMcpServer(item.name); | |
| // In the disable branch (item.isActive === true), add: | |
| const runtimeExcluded = config.getExcludedMcpServers() ?? []; | |
| if (!runtimeExcluded.includes(item.name)) { | |
| config.setExcludedMcpServers([...runtimeExcluded, item.name]); | |
| } | |
| await toolRegistry?.disableMcpServer(item.name); |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Couldn't reproduce — the disable branch persists mcp.excluded to settings, and toolRegistry.disableMcpServer() itself appends the server to config.getExcludedMcpServers() (tool-registry.ts), so the runtime list is updated on disable.
| try { | ||
| const metadata = await parseInstallSource(plugin.installSource); | ||
| const ext = await extensionManager.installExtension(metadata); | ||
| extensionManager.setExtensionScope(ext.name, scope); |
There was a problem hiding this comment.
[Suggestion] setExtensionScope failure inside install loop marks a successfully-installed extension as failed. If setExtensionScope throws after installExtension succeeds, the catch block counts the entire operation as a failure even though the extension is fully installed and functional. The user sees an error like "Installed 0, failed 1" and may attempt to re-install.
| extensionManager.setExtensionScope(ext.name, scope); | |
| const ext = await extensionManager.installExtension(metadata); | |
| try { | |
| extensionManager.setExtensionScope(ext.name, scope); | |
| } catch (scopeError) { | |
| debugLogger.error('Failed to record scope preference:', scopeError); | |
| // Extension is installed; scope preference is non-critical. | |
| } |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — install and the post-install scope/enablement step are now separate try/catch blocks; a scope-preference failure is logged but no longer flips a successful install to "failed".
| } | ||
| const src = plugin.source; | ||
| if (typeof src === 'string') { | ||
| return src.includes(':') ? src : `${src}:${plugin.name}`; |
There was a problem hiding this comment.
[Suggestion] Remote http marketplace can reference arbitrary local paths via resolveInstallSource. For http-type marketplaces, when plugin.source is a string containing :, it's returned as-is. A malicious marketplace JSON could declare "source": "/opt/internal-tool" — parseInstallSource then calls stat(repo) which succeeds for existing paths, treating it as a local extension.
A remote marketplace should not be able to reference local filesystem directories.
| return src.includes(':') ? src : `${src}:${plugin.name}`; | |
| if (typeof src === 'string') { | |
| if (path.isAbsolute(src)) { | |
| debugLogger.warn(`Rejecting local path source from remote marketplace: ${src}`); | |
| return plugin.name; | |
| } | |
| return src.includes(':') ? src : `${src}:${plugin.name}`; | |
| } |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — string sources that are absolute or start with ./~ are rejected for remote marketplaces (returns the bare name → clear error rather than a local install).
| return; | ||
| } | ||
| try { | ||
| await open(plugin.homepage); |
There was a problem hiding this comment.
[Suggestion] openHomepage accepts file:// URLs from marketplace manifest. The function passes plugin.homepage directly to open() without validating the URL scheme. The open package supports file:// URLs, which opens the referenced local file in the OS-default application.
A malicious marketplace could set "homepage": "file:///etc/passwd" or "homepage": "file:///Users/victim/.ssh/id_rsa".
| await open(plugin.homepage); | |
| const openHomepage = useCallback( | |
| async (plugin: DiscoveredPlugin) => { | |
| if (!plugin.homepage) return; | |
| try { | |
| const url = new URL(plugin.homepage); | |
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | |
| onStatus({ type: 'error', text: t('Failed to open {{url}}', { url: plugin.homepage }) }); | |
| return; | |
| } | |
| await open(plugin.homepage); | |
| } catch { |
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Fixed in 05eb44f — openHomepage now parses the URL and only follows http:/https:.
| settled = true; | ||
| resolve(value); | ||
| }; | ||
| const req = https.get(url, { headers }, (res) => { |
There was a problem hiding this comment.
[Suggestion] fetchUrl uses node:https exclusively — http:// marketplace URLs silently fail. loadMarketplaceConfigFromSource accepts both http:// and https:// prefixes (Priority 2 branch), but fetchUrl only imports node:https. When a user adds an http:// marketplace (e.g., a local network server), https.get('http://...') produces a TLS error that is silently caught and resolved as null. The user sees only "Could not load this marketplace." with no diagnostic.
Either import node:http and dispatch based on URL protocol, or reject http:// URLs early with a clear error message.
— qwen3.7-plus via Qwen Code /review
There was a problem hiding this comment.
Deferring — http:// marketplaces are an insecure transport; rather than silently failing I'll either reject them with a clear message or add explicit node:http dispatch in a follow-up.
Press Ctrl+R in the Extensions Manager Discover tab to bypass the discover cache and re-fetch all marketplace sources. The refresh hint is merged into the dialog footer, and a success status is shown after the refresh completes.
The Discover tab list reused the global SELECTION_UP/DOWN matchers, which include bare j/k as Vim-style navigation. Combined with type-to-search input, that made it impossible to type j or k into the search query. Switch the Discover list navigation to explicit arrow keys plus Ctrl+P/Ctrl+N, so bare j/k fall through to the printable-character branch and append to the query. Other extension tabs (Installed, Sources) remain pure lists and keep the Vim navigation.
|
Thanks for the thorough macOS real-build verification @wenshao 🙏 — especially the byte-level confirmation that the OSC title-injection is neutralized pre-consent. Followed up on your two non-blocking notes: 1.
This locks in the sanitization at the 2. The Also re the red If there's nothing else outstanding, an approval would be much appreciated 🙏 — happy to iterate on anything that remains. 中文多谢 @wenshao 在 macOS 上的真实构建验证 🙏,尤其是字节级确认了 OSC 改标题注入在安装前已被消解。 两条非阻塞提示都已跟进: 1. 2. 另外红色的 如无其他问题,烦请帮忙 approve 🙏。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Stale JSDoc on sanitizeForErrorMessage in workflow-orchestrator.ts: the JSDoc still says "Control characters are replaced with a single space" but the new stripAnsiAndControl removes them entirely (empty string). The inner comment acknowledges this ("Removing rather than spacing"), but the outer JSDoc was not updated. The call-site comment at ~line 487 has the same stale description.
— qwen3.7-max via Qwen Code /review
| // A transient read error (permission/too-many-files/…) — the file may be | ||
| // perfectly valid, so do NOT quarantine it here; only parse failures | ||
| // above do that. Return the default for this read. | ||
| debugLogger.error('Error reading extension preferences:', error); |
There was a problem hiding this comment.
[Suggestion] Silent empty state on transient read errors. Both this catch and the analogous one in sourceRegistry.ts only log via debugLogger.error(), which is gated behind QWEN_DEBUG_LOG_FILE. When a transient error (EACCES/EMFILE/EISDIR) hits, the user's favorites/scopes/disabled-server state vanishes silently with no visible diagnostic.
quarantineCorruptFile already established the pattern of writing to process.stderr because the debug logger is gated. The same approach would work here:
| debugLogger.error('Error reading extension preferences:', error); | |
| process.stderr.write( | |
| `[warn] Could not read extension preferences at ${this.filePath}: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }. Using defaults for this session.\n`, | |
| ); | |
| debugLogger.error('Error reading extension preferences:', error); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 631e271fb. The transient-read catch now writes a [warn] line to process.stderr (matching the quarantineCorruptFile pattern) before the gated debugLogger.error, so a vanished favorites/scopes state has a visible trail.
| } | ||
| // A transient read error (permission/too-many-files/…) — the file may be | ||
| // valid, so do NOT quarantine it; only a parse failure below does that. | ||
| debugLogger.error('Error reading marketplace registry:', error); |
There was a problem hiding this comment.
[Suggestion] Same as extensionPreferences.ts — transient read errors produce no stderr output. If the marketplace registry file hits EACCES or EMFILE, the user's source list vanishes silently. Add a process.stderr.write matching the quarantineCorruptFile pattern so the failure has a visible trail.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 631e271fb. Added the same process.stderr.write warning to the marketplace-registry transient-read catch in sourceRegistry.ts.
| // Transient read errors (EACCES/EMFILE/EISDIR/…) fall through to the | ||
| // outer catch, which must NOT quarantine an otherwise-valid file. | ||
| debugLogger.error('Corrupt extension preferences:', parseError); | ||
| quarantineCorruptFile(this.filePath); |
There was a problem hiding this comment.
[Suggestion] Test coverage gaps for the quarantine narrowing. The core behavioral change here — parse errors quarantine, transient read errors do NOT — has no test verification:
- No test asserts
quarantineCorruptFileactually ran on a corrupt JSON file (i.e., the.corruptedsibling exists after a parse failure). - No test simulates a transient read error (mock
readFileSyncto throw EACCES) and asserts the file is NOT quarantined.
The same gaps exist in sourceRegistry.test.ts. Without these tests, a future refactor could silently re-introduce the old "quarantine on any error" bug — which would wipe user preferences on a temporary permission error.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 631e271fb. Both extensionPreferences.test.ts and sourceRegistry.test.ts now assert: (1) a parse failure quarantines to a .corrupted sibling and preserves the original bytes; (2) a transient read error (EISDIR via a directory at the path — same class as EACCES/EMFILE) does NOT quarantine, leaves the file intact, and still warns on stderr. A future "quarantine on any error" regression would now fail these.
| await manager.enableExtension(name, SettingScope.User); | ||
| } else { | ||
| await manager.disableExtension(name, SettingScope.User); | ||
| await manager.enableExtension(name, SettingScope.Workspace); |
There was a problem hiding this comment.
[Critical] Missing rollback when enableExtension(Workspace) fails after disableExtension(User) succeeds. The CLI install.ts (lines 108-120) has an explicit try/catch that re-enables at User scope on Workspace-enable failure, but this UI path does not. If the Workspace enable throws, the extension is left disabled at all scopes — the user sees an error toast but the extension is silently dead.
| await manager.enableExtension(name, SettingScope.Workspace); | |
| await manager.disableExtension(name, SettingScope.User); | |
| try { | |
| await manager.enableExtension(name, SettingScope.Workspace); | |
| } catch (enableError) { | |
| try { | |
| await manager.enableExtension(name, SettingScope.User); | |
| } catch { | |
| // Best-effort rollback | |
| } | |
| throw enableError; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 631e271fb. handleScope now wraps the Workspace enable in a try/catch and rolls the User enable back on failure (mirroring the CLI install.ts pattern), so a failed Workspace enable can no longer leave the extension disabled at every scope. I also moved setExtensionScope to run only after enablement succeeds, so the prefs can't point at a scope the extension isn't actually enabled at.
| // workspace-scoped choice, re-scope enablement to this workspace | ||
| // only: disable the global enable and enable for the workspace path. | ||
| if (scope !== 'user') { | ||
| await extensionManager.disableExtension( |
There was a problem hiding this comment.
[Critical] Same missing rollback pattern: project-scoped install disables User then enables Workspace with no try/catch. Worse than ExtensionActionsView because the outer catch (line 249) logs and swallows the error — the install counts as success while the extension silently doesn't activate.
| await extensionManager.disableExtension( | |
| if (scope !== 'user') { | |
| await extensionManager.disableExtension( | |
| ext.name, | |
| SettingScope.User, | |
| ); | |
| try { | |
| await extensionManager.enableExtension( | |
| ext.name, | |
| SettingScope.Workspace, | |
| ); | |
| } catch (enableError) { | |
| try { | |
| await extensionManager.enableExtension( | |
| ext.name, | |
| SettingScope.User, | |
| ); | |
| } catch { | |
| // Best-effort rollback | |
| } | |
| throw enableError; | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 631e271fb. The project-scope branch now rolls the User enable back if the Workspace enable throws — important here precisely because (as you noted) the outer catch only logs and the install still reports success, so without the rollback the extension would be silently dead. setExtensionScope is now recorded only after enablement succeeds.
| ); | ||
| const scope = normalizeScope(args.scope); | ||
| if (args.scope) { | ||
| extensionManager.setExtensionScope(extension.name, scope); |
There was a problem hiding this comment.
[Suggestion] setExtensionScope(name, scope) is persisted before the enable/disable try/catch. If Workspace enable fails, the rollback correctly re-enables at User scope, but the scope preference still records 'project'. The Installed tab will show the extension in the "Project level" group even though it's actually enabled at User scope.
Move setExtensionScope to after the enablement succeeds, or roll it back in the catch alongside the enablement rollback.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 631e271fb. Moved setExtensionScope(extension.name, scope) to after the enable/disable block. On a Workspace-enable failure the rollback re-enables User scope and the scope preference is never written (defaults to user), so the Installed tab no longer mislabels it as "Project level". Same reorder applied to the two UI install paths.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-reviewed against the current head b5fa71a3f9 (round 4) after the latest push.
✅ Round 4 resolved the previously-open items (verified at HEAD): shared stripAnsiAndControl in textUtils.ts (the sanitizeForError/sanitizeDisplay duplication is closed); quarantineCorruptFile now surfaces on stderr instead of only the gated debug log; quarantine narrowed to genuine parse failures (transient EACCES/EMFILE/… no longer quarantine a valid file); install.ts rollback-failure now surfaced and ref/sha interpolation sanitized; new tests for the gemini symlink-escape branch, corruptFile (rename + byte preservation), and the install rollback (success + double-failure).
🔴 Still open — the one item round 4 did NOT touch: ExtensionActionsView.tsx handleScope still runs disableExtension(User) → enableExtension(Workspace) with no rollback (only an error toast at the catch). If the Workspace enable throws (settings write EACCES/EROFS, or a tool-refresh failure), the extension is left disabled at every scope with no recovery — the exact hazard install.ts now has a rollback for. The earlier Critical filed inline at ExtensionActionsView.tsx:229 stands; please mirror the install.ts pattern (wrap the Workspace enable, best-effort re-enable User on failure). My earlier CHANGES_REQUESTED remains until this is addressed.
🟡 New finding (inline): persisted-version ANSI/control-char injection — sanitized in Discover, rendered raw in the Installed tab. See the inline comment on InstalledTab.tsx.
ℹ️ Pre-existing sinks newly exposed by the marketplace install flow (out-of-diff, not blocking — flagged here, not inline):
- Consent prompt (
extensionConsentString→MarkdownDisplay/RenderInline): untrusted pluginname/description/MCP/skill fields reach the terminal without C0/C1 stripping (descriptiononly getsstripAnsi, which leaves BEL/BS/CR). The consent dialog is the last gate before install, so a crafted plugin can spoof it.consent.tsis unchanged by this PR, but the marketplace flow is what newly routes attacker-controlled metadata into it. cloneFromGitapplies no scheme allowlist to the marketplace-suppliedsource.url(newgit-subdir/urlbranches). Note: theext::RCE vector is blocked by git's defaultprotocol.ext.allow=never— I verified locally (git 2.50.1 →fatal: transport 'ext' not allowed), so this is not RCE. Butfile://(local clone) andgit://remain reachable with a marketplace-controlled URL; an explicit scheme allowlist would be reasonable defense-in-depth.
Verdict: Comment — round 4 is solid progress; handleScope rollback is the single remaining blocker.
中文版
已针对 round 4 的当前 head b5fa71a3f9（最新一次 push 之后）重新评审。
✅ Round 4 已修复上一轮遗留项（在 HEAD 上已核实）：textUtils.ts 抽出共享的 stripAnsiAndControl（sanitizeForError/sanitizeDisplay 的重复实现已收敛）；quarantineCorruptFile 现在会写 stderr，不再只走被 QWEN_DEBUG_LOG_FILE 网关挡住的 debug 日志；隔离逻辑收窄为仅在真正 JSON 解析失败时触发（瞬时 EACCES/EMFILE 等不再误隔离有效文件）；install.ts 回滚失败已对外提示，ref/sha 插值已做净化；新增了 gemini 符号链接越界分支、corruptFile（重命名 + 字节保全）、install 回滚（成功 + 双重失败）的测试。
🔴 仍未解决 —— round 4 唯一没动的文件： ExtensionActionsView.tsx 的 handleScope 依旧是 disableExtension(User) → enableExtension(Workspace) 两步操作且没有回滚（catch 里只弹一个错误 toast）。若 Workspace 启用步骤抛错（设置写入 EACCES/EROFS，或刷新工具失败），扩展会被留在所有 scope 都禁用的状态且无法自动恢复 —— 正是 install.ts 现在已加回滚来防的那个隐患。之前在 ExtensionActionsView.tsx:229 提的 Critical 依然成立；请照搬 install.ts 的写法（包住 Workspace 启用，失败时尽力把 User 重新启用回来）。在修复前，我之前的 CHANGES_REQUESTED 保持不变。
🟡 新发现（行内评论）： 持久化的 version 存在 ANSI/控制字符注入 —— Discover 里做了净化，但 Installed 标签页是原样渲染。详见 InstalledTab.tsx 的行内评论。
ℹ️ 被 marketplace 安装流程新暴露的既有 sink（不在本 PR diff 内、非阻塞 —— 这里一并提示，不单独行内）：
- 同意确认弹窗（
extensionConsentString→MarkdownDisplay/RenderInline）：不可信插件的name/description/MCP/skill 字段未经 C0/C1 净化就进入终端（description只过了stripAnsi，BEL/BS/CR 仍残留）。该弹窗是安装前的最后一道关卡，被构造的插件可以伪造它。consent.ts本身没改，但 marketplace 流程是把攻击者可控元数据新引入到这里的来源。 cloneFromGit对 marketplace 提供的source.url没有任何 scheme 白名单（新增的git-subdir/url分支）。说明：ext::这个 RCE 向量被 git 默认的protocol.ext.allow=never挡住了 —— 本地实测确认（git 2.50.1 →fatal: transport 'ext' not allowed），所以不是 RCE。但file://（克隆本地仓库）和git://仍可被 marketplace 控制的 URL 触达；加一个显式 scheme 白名单作为纵深防御是合理的。
结论：Comment —— round 4 进展扎实；handleScope 回滚是唯一剩下的阻塞项。
— claude-opus-4-8 via Claude Code /qreview
| item.kind === 'mcp' | ||
| ? t('MCP') | ||
| : t('Extension v{{version}}', { | ||
| version: item.extension.version, |
There was a problem hiding this comment.
[Suggestion] Persisted version reaches the terminal unsanitized — ANSI/control-char injection on the Installed tab.
convertClaudeToQwenConfig persists a marketplace plugin's version verbatim (claude-converter.ts:417), and on load only name is validated (validateName) — version is neither validated nor sanitized. The Discover tab scrubs these fields via sanitizeDisplay/stripAnsiAndControl, but here (and at PluginDetailView.tsx:128) the persisted version is rendered raw in an Ink <Text>.
A marketplace plugin with "name":"safe-name" (passes validateName) and "version":"1.0.0\u001b[2J\u001b[H…" injects escape sequences (screen-clear, cursor moves, a forged ✓ verified banner) on every Installed-tab render after install. Same class as the Discover-side ANSI injection fixed earlier in this PR — just on the new persisted/Installed render path that the Discover sanitization doesn't cover.
| version: item.extension.version, | |
| version: stripUnsafeCharacters(item.extension.version ?? ''), |
Also add the import (as SourcesTab.tsx:25 already does): import { stripUnsafeCharacters } from '../../../utils/textUtils.js';, and apply the same wrap at PluginDetailView.tsx:128. Sanitizing at the persistence boundary in convertClaudeToQwenConfig would fix all sinks at once and also defend already-installed extensions.
— claude-opus-4-8 via Claude Code /qreview
There was a problem hiding this comment.
Fixed in 631e271fb. Wrapped the persisted version with stripUnsafeCharacters at both raw sinks — InstalledTab.tsx (the Extension v{{version}} badge) and PluginDetailView.tsx (the Version row). I chose render-time sanitization over the persistence boundary because it also defends already-installed extensions whose raw version is already on disk. (The third version render in steps/ExtensionDetailStep.tsx is the retired step architecture — not mounted by any live tab/view, so left untouched.)
…ion sanitization, visible read warnings - Critical: the UI scope-change (ExtensionActionsView) and project-scope install (DiscoverTab) disabled User then enabled Workspace with no rollback — a failed Workspace enable left the extension disabled at all scopes (silently dead, and in DiscoverTab the outer catch swallowed it so the install still reported success). Mirror the CLI install.ts pattern: roll the User enable back on failure. - Persist the scope preference only AFTER enablement succeeds (install.ts + both UI paths), so a rolled-back enable can't leave prefs pointing at a scope the extension isn't actually enabled at (Installed tab mislabel). - Security: the persisted 'version' is rendered raw on the Installed tab and PluginDetailView — only 'name' is validated on load, so a marketplace plugin could inject ANSI/control sequences post-install on every render. Scrub via stripUnsafeCharacters at both sinks (covers already-installed extensions; the Discover-side sanitization doesn't reach this path). - Transient read errors in extensionPreferences/sourceRegistry only logged via the gated debugLogger, so a user's favorites/scopes/sources could vanish with no trail. Add an stderr warning matching quarantineCorruptFile. - Tests: assert quarantine runs on a parse error (.corrupted sibling) and does NOT run on a transient read error (EISDIR), for both stores.
|
Thanks @wenshao and the ✅ Critical — scope-change rollback (parity with CLI
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
All findings from my previous review are resolved in round 5 (631e271fba) — verified at HEAD. ✅
- Scope-change rollback (Critical) —
ExtensionActionsView.handleScopenow wraps the Workspace enable in try/catch and best-effort re-enables User on failure, and records the scope preference only after enablement succeeds. The siblingDiscoverTabinstall path got the same rollback, andinstall.tswas reordered to persist scope only after enablement. Correct. - Persisted-
versionANSI injection (Suggestion) — both sinks (InstalledTab.tsx,PluginDetailView.tsx) now wrapversioninstripUnsafeCharacters(… ?? ''), with the import added. Correct. - Transient-read silent empty state —
extensionPreferences.tsandsourceRegistry.tsnow surface read failures on stderr. New tests cover parse-error quarantine vs. transient-read (EISDIR) non-quarantine, plus adiscoverPluginsANSI-stripping assertion. Correct.
This feature has been through a thorough multi-round security review (path/symlink confinement, ANSI sanitization at every marketplace-metadata boundary, fetch size/time caps, corrupt-file quarantine, non-atomic scope rollbacks) and the in-diff code is now clean. CI green (30 checks).
Recommended follow-up (non-blocking, pre-existing & out-of-diff): the install consent prompt (extensionConsentString → MarkdownDisplay/RenderInline) still renders untrusted plugin description/name/MCP/skill fields without C0/C1 stripping (description only gets stripAnsi). It is pre-existing shared code that this PR newly routes marketplace metadata into; worth sanitizing in a follow-up so the consent dialog can't be spoofed. Not a blocker for this PR.
Verdict: Approve.
中文版
上一轮 review 提的问题在 round 5（631e271fba）里都已修复，已在 HEAD 上核实。✅
- scope 切换回滚（Critical） ——
ExtensionActionsView.handleScope现在用 try/catch 包住 Workspace 启用，失败时尽力把 User 重新启用回来，并且只在启用成功后才记录 scope 偏好。同源的DiscoverTab安装路径加了相同的回滚，install.ts也调整为启用成功后再持久化 scope。修法正确。 - 持久化
version的 ANSI 注入（Suggestion） —— 两个渲染点（InstalledTab.tsx、PluginDetailView.tsx）现在都用stripUnsafeCharacters(… ?? '')包住version，并补了 import。正确。 - 瞬时读失败导致静默空状态 ——
extensionPreferences.ts和sourceRegistry.ts现在会把读失败写到 stderr。新增测试覆盖了"解析失败→隔离"与"瞬时读失败（EISDIR）→不隔离"两条分支，外加discoverPlugins的 ANSI 净化断言。正确。
这个特性经过了多轮严格的安全 review（路径/符号链接约束、对所有 marketplace 元数据边界做 ANSI 净化、fetch 大小/超时上限、损坏文件隔离、非原子 scope 回滚），diff 内的代码现在是干净的。CI 全绿（30 项）。
建议的后续项（不阻塞、属既有代码且不在本 diff 内）： 安装同意确认弹窗（extensionConsentString → MarkdownDisplay/RenderInline）仍然把不可信插件的 description/name/MCP/skill 字段未经 C0/C1 净化就渲染（description 只过了 stripAnsi）。这是本 PR 新引流 marketplace 元数据进入的既有共享代码；建议在后续 PR 里净化，避免同意弹窗被伪造。不阻塞本 PR。
结论：Approve。
— claude-opus-4-8 via Claude Code /qreview
✅ Re-verification at new head
|
Re-check (delta 61a69e2729..631e271fba, +483/-50, 17 files) |
Result |
|---|---|
npm ci + npm run build (macOS) |
✅ exit 0 |
| Changed core suites (9 files) | ✅ 276 passed |
| Changed CLI suites (3 files) | ✅ 47 passed |
| Mutation test on the new ANSI/control tests | ✅ disabling the sanitizer fails exactly them → non-vacuous |
| Real-TUI: shared-sanitizer refactor + round-5 version sanitization | ✅ still strips ANSI/OSC at runtime |
| CI | Lint ✅ · 3× Test + CodeQL still pending (recent push) |
1. What changed since my last report
Four commits, all hardening in response to review:
631e271fba round 5 — scope-change rollback, version sanitization, visible read warnings
a2d63d7807 test — assert discoverPlugins strips ANSI/control chars from display fields <- closes my flagged gap
b5fa71a3f9 round 4 — shared sanitizer, narrowed quarantine, visible warnings, tests
a7b2df2fb4 test — normalize realpathSync mock so the gemini guard passes on Windows
- Shared sanitizer: the per-file
stripVTControlCharacters + C0/C1copies are unified into one exportedstripAnsiAndControl()inutils/textUtils.ts;sourceRegistry,claude-converter, andworkflow-orchestratornow all delegate to it. The orchestrator's old local regex (\x00–\x1f+\x7f) actually missed the C1 range (\x80–\x9f); the shared one fixes that — a real (small) bug closed as a side effect of de-duplication. - Narrowed quarantine (data-loss fix):
SourceRegistryStore.read()andExtensionPreferencesStore.read()previously moved the file aside on any read error. They now quarantine only on a JSON parse failure; a transient read error (EACCES / EMFILE / …) keeps the file and emits a visiblestderrwarning (sincedebugLogger.erroris gated behindQWEN_DEBUG_LOG_FILE, which is unset for almost everyone → previously a silent data-vanish). - Round 5:
versionis now sanitized alongside the other display fields; the converter error path also sanitizesref ?? sha ?? 'HEAD'(was interpolated raw).
2. Delta tests (macOS)
core (9): sourceRegistry 18 · textUtils 25 · corruptFile 2 · extensionPreferences 11
gemini-converter 11 · claude-converter 35 · marketplace 23 · extensionManager 50
workflow-orchestrator 101 -> 276 passed
cli (3): ExtensionsManagerDialog 23 · extensions/install · extensions/sources 11 -> 47 passed
3. The flagged gap is now genuinely closed (mutation test) ⭐
The new discoverPlugins > strips ANSI/control sequences from untrusted display fields test even notes "Payload mirrors the PoC from the PR #4850 verification report" — it feeds an ANSI/OSC/BEL payload through every display field and asserts each comes out clean.
To confirm it isn't vacuous, I disabled the shared sanitizer (stripAnsiAndControl → return text) and re-ran:
× discoverPlugins > strips ANSI/control sequences from untrusted display fields
-> expected 'Hostile<ESC>[2K' to be 'Hostile'
× stripAnsiAndControl > strips ANSI/VT escape sequences
× stripAnsiAndControl > strips OSC 8 hyperlink sequences but keeps the link text
× stripAnsiAndControl > removes residual C0 control chars and DEL
× stripAnsiAndControl > removes C1 control chars (the range a drifted local copy missed)
Failed Tests 5
Exactly the sanitization tests fail (including one that specifically guards the C1 range the orchestrator regex used to miss), and nothing else → the coverage is real. Restored → all green again.
4. Real-TUI re-confirmation (the refactor didn't regress runtime behavior)
Same hostile-marketplace PoC, real built binary, isolated $HOME. After the shared-sanitizer refactor and the round-5 version sanitization, the pre-consent Discover view and the detail view still neutralize everything:
Discover list : name -> "evil-plugin" desc -> "Totally safe — trust me"
Detail view : Version: 9.9.9 By: mallory homepage -> https://evil.com
byte-level (capture -e): PWNED 0 · OSC ]0; 0 · cursor-up [1A 0 · clear-line [2K 0 · blink [5m 0
terminal pane title : "Qwen - project" (NOT "PWNED" — OSC title-injection neutralized)
The three tabs (Installed grouping + nested MCP status, Discover, Sources) all still render and navigate correctly.
Verdict
The rounds-4/5 delta is clean, well-reasoned hardening that resolves the only open item from my prior pass. Build green, all changed suites green, the new security tests are non-vacuous, and runtime sanitization is intact. Recommendation stands: merge once the (currently pending) CI Test matrix goes green. Note the macOS Test job may again trip the unrelated cronScheduler durable-ownership timing flake described in my earlier comment — that remains a false signal, not this PR.
🇨🇳 中文版（点击展开）
✅ 在新 head 631e271fba 上的复验(维护者)—— 结论不变:建议合并
本评论取代我此前的验证(那次针对 head 61a69e2729)。作者之后又推了 4 个 commit(review 第 4、5 轮);我在 macOS 上从源码重新构建并复验了增量。要点:第 4/5 轮全是 review 驱动的加固,而且堵上了我上次报告标注的唯一缺口 —— sanitizeDisplay() 现在有了直接、非空泛的单测覆盖。
复检项(增量 61a69e2729..631e271fba,+483/-50,17 文件) |
结果 |
|---|---|
npm ci + npm run build(macOS) |
✅ exit 0 |
| 改动的 core 套件(9 文件) | ✅ 276 通过 |
| 改动的 CLI 套件(3 文件) | ✅ 47 通过 |
| 对新增 ANSI/控制字符测试的 mutation test | ✅ 禁用 sanitizer 后恰好这些失败 → 非空泛 |
| 实机 TUI:共享 sanitizer 重构 + 第 5 轮 version 净化 | ✅ 运行时仍剥离 ANSI/OSC |
| CI | Lint ✅ · 三平台 Test + CodeQL 仍 pending(刚推送) |
1. 自上次报告以来改了什么
四个 commit,全是回应 review 的加固:
631e271fba 第 5 轮 — scope 变更回滚、version 净化、可见读取警告
a2d63d7807 测试 — 断言 discoverPlugins 从显示字段剥离 ANSI/控制字符 <- 堵上我标注的缺口
b5fa71a3f9 第 4 轮 — 共享 sanitizer、收窄 quarantine、可见警告、测试
a7b2df2fb4 测试 — 规范化 realpathSync mock 让 gemini guard 在 Windows 通过
- 共享 sanitizer: 各文件里
stripVTControlCharacters + C0/C1的副本被统一成utils/textUtils.ts里一个导出的stripAnsiAndControl();sourceRegistry、claude-converter、workflow-orchestrator现在都委托给它。orchestrator 旧的本地正则(\x00–\x1f+\x7f)其实漏了 C1 范围(\x80–\x9f),共享版顺手修掉了 —— 去重附带修了个(小)真实 bug。 - 收窄 quarantine(数据丢失修复):
SourceRegistryStore.read()和ExtensionPreferencesStore.read()之前对任何读错误都把文件挪走。现在只在 JSON 解析失败时 quarantine;瞬时读错误(EACCES / EMFILE / …)保留文件并打可见的stderr警告(因为debugLogger.error被QWEN_DEBUG_LOG_FILE门控,几乎所有人都没设 → 之前是静默丢数据)。 - 第 5 轮:
version现在和其他显示字段一起被净化;转换器错误路径也净化了ref ?? sha ?? 'HEAD'(之前是裸插值)。
2. 增量测试(macOS)
core (9): sourceRegistry 18 · textUtils 25 · corruptFile 2 · extensionPreferences 11
gemini-converter 11 · claude-converter 35 · marketplace 23 · extensionManager 50
workflow-orchestrator 101 -> 276 通过
cli (3): ExtensionsManagerDialog 23 · extensions/install · extensions/sources 11 -> 47 通过
3. 标注的缺口现已真正堵上(mutation test)⭐
新测试 discoverPlugins > strips ANSI/control sequences from untrusted display fields 的注释甚至写着 "Payload mirrors the PoC from the PR #4850 verification report" —— 它把一份 ANSI/OSC/BEL 载荷喂过每个显示字段,断言每个都干净输出。
为确认它非空泛,我把共享 sanitizer 禁用(stripAnsiAndControl → return text)后重跑:
× discoverPlugins > strips ANSI/control sequences from untrusted display fields
-> expected 'Hostile<ESC>[2K' to be 'Hostile'
× stripAnsiAndControl > strips ANSI/VT escape sequences
× stripAnsiAndControl > strips OSC 8 hyperlink sequences but keeps the link text
× stripAnsiAndControl > removes residual C0 control chars and DEL
× stripAnsiAndControl > removes C1 control chars (the range a drifted local copy missed)
Failed Tests 5
恰好这些净化测试失败(其中一个专门守护 orchestrator 正则曾漏掉的 C1 范围),其余无碍 → 覆盖是真实的。还原后重新全绿。
4. 实机 TUI 复核(重构没有破坏运行时行为)
同一份恶意 marketplace PoC、真实构建二进制、隔离 $HOME。在共享 sanitizer 重构以及第 5 轮 version 净化之后,安装前的 Discover 视图与详情视图仍然消解了所有载荷:
Discover 列表 : name -> "evil-plugin" desc -> "Totally safe — trust me"
详情视图 : Version: 9.9.9 By: mallory homepage -> https://evil.com
字节级(capture -e): PWNED 0 · OSC ]0; 0 · 光标上移 [1A 0 · 清行 [2K 0 · 闪烁 [5m 0
终端 pane 标题 : "Qwen - project" (不是 "PWNED" —— OSC 改标题注入被消解)
三个 tab(Installed 分组 + 嵌套 MCP 状态、Discover、Sources)仍正确渲染和导航。
结论
第 4/5 轮的增量是干净、有据的加固,解决了我上一轮唯一的遗留项。构建绿、所有改动套件绿、新增安全测试非空泛、运行时净化完好。结论不变:建议合并 —— 待(当前 pending 的)CI Test 矩阵转绿即可。注意 macOS Test job 可能再次踩到我上条评论里描述的与本 PR 无关的 cronScheduler durable-ownership 计时 flaky —— 那仍是误报,不是本 PR。
Maintainer re-verification · head 631e271fba (prev 61a69e2729) · macOS 26.5 / Node 22.22.2.
|
@qwen-code /triage |
| {marketplaceFilter ? ( | ||
| <Text color={theme.text.secondary}> | ||
| {t(' · {{marketplace}} (Tab to clear)', { | ||
| marketplace: marketplaceFilter, |
There was a problem hiding this comment.
[Critical] ANSI/terminal escape injection via marketplaceFilter. When the user clicks "Browse extensions" on the Sources tab, the raw marketplace name (detailSource.name from ExtensionSource) is passed as marketplaceFilter and rendered unsanitized here. The marketplace name originates from the remote marketplace.json's config.name field (untrusted), stored raw by addSource in extensionManager.ts.
The prior ANSI fix (round 1) sanitized plugin metadata (DiscoveredPlugin.marketplaceName) at the sourceRegistry.ts boundary, but this is a separate injection vector: the marketplace's own name, passed cross-component through onBrowse, bypasses the sanitizeDisplay applied to plugin fields.
A hostile marketplace can embed ANSI/OSC escape sequences in its name field. When browsed from the Sources tab, the sequences reach the terminal unsanitized — potentially spoofing UI elements, clearing lines, or moving the cursor.
| marketplace: marketplaceFilter, | |
| marketplace: stripUnsafeCharacters(marketplaceFilter), |
Alternatively, sanitize at the handleBrowseSource callback or in addSource so the raw name is never persisted with escape sequences.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 158002c77. Sanitized the marketplace name in handleBrowseSource (ExtensionsManagerDialog) via stripUnsafeCharacters before it becomes the Discover filter. I chose that boundary over the render site because the filter is also compared against the already-sanitized DiscoveredPlugin.marketplaceName — so sanitizing here blocks the ANSI injection AND keeps the filter comparison matching (a raw control-char name would otherwise match zero plugins).
| </Text> | ||
| {marketplaceFilter ? ( | ||
| <Text color={theme.text.secondary}> | ||
| {t(' · {{marketplace}} (Tab to clear)', { |
There was a problem hiding this comment.
[Critical] The hint says (Tab to clear) but the DiscoverTab's useKeypress handler has no Tab key handler. Pressing Tab triggers the parent's cycleTab, which switches to the next tab rather than clearing the filter in place. Worse, the catch-all key.sequence >= ' ' condition at the list keypress handler would append a literal \t character to the search query instead.
| {t(' · {{marketplace}} (Tab to clear)', { | |
| {t(' · {{marketplace}} (Esc to clear)', { |
Or add a dedicated key handler (e.g., Esc or a specific key combo) that calls an onClearFilter callback to set marketplaceFilter to null without switching tabs.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 158002c77. Rather than change the hint, I made it true: on the Discover tab with an active marketplace filter, Tab now clears the filter in place (revealing all extensions) instead of cycling tabs. This lives in the parent ExtensionsManagerDialog Tab handler — useKeypress broadcasts to all subscribers with no propagation control, so handling Tab in the child would race the parent's cycleTab; doing it in the parent (which owns both the filter state and the Tab handler) is conflict-free. (Note: a bare Tab is \x09 < 0x20`, so the list catch-all never appended it to the query — but the in-place clear is the real fix.)
| SettingScope.User, | ||
| ); | ||
| } catch { | ||
| // Best-effort rollback. |
There was a problem hiding this comment.
[Critical] Rollback failure is silently swallowed. The CLI's install.ts (line 122-128) surfaces rollback failures with writeStderrLine, warning the user the extension may be disabled at all scopes. This UI code silently swallows the rollback error with a bare catch.
Scenario: install succeeds → disableExtension(User) lands → enableExtension(Workspace) fails → rollback enableExtension(User) also fails. The user sees either a success message or a generic error about the workspace-enable failure, but nothing about the rollback failure that left the extension disabled at every scope.
| // Best-effort rollback. | |
| } catch (rollbackError) { | |
| debugLogger.error('Rollback failed:', rollbackError); | |
| errors.push(`${plugin.name}: install succeeded but scope rollback failed — the extension may be disabled at all scopes: ${getErrorMessage(rollbackError)}`); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 158002c77. The inner rollback catch now captures the failure: it debug-logs and pushes a descriptive entry to the batch errors[] (new i18n key) so the user is told the extension may be disabled at all scopes and to re-enable it from the Installed tab, instead of the install silently reporting success.
| try { | ||
| await manager.enableExtension(name, SettingScope.User); | ||
| } catch { | ||
| // Best-effort rollback. |
There was a problem hiding this comment.
[Critical] Same rollback swallowing as DiscoverTab.tsx:258. The CLI's install.ts (lines 122-128) surfaces rollback failures with writeStderrLine, but this UI code silently swallows them. If the rollback fails, the extension ends up disabled at all scopes while the user only sees the original enable failure in the error toast.
Consider surfacing the rollback failure so the user knows to manually re-enable the extension:
| // Best-effort rollback. | |
| } catch (rollbackError) { | |
| debugLogger.error('Rollback failed:', rollbackError); | |
| throw new Error(`${getErrorMessage(error)}\n\nRollback also failed — "${name}" may be disabled at all scopes. Please enable it manually from the Installed tab.`); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 158002c77. Same treatment as DiscoverTab: when the rollback enableExtension(User) also fails, handleScope now throws an explicit, translated error telling the user the extension may be disabled at all scopes and to re-enable it from the Installed tab — rather than surfacing only the bare workspace-enable error.
| ); | ||
| } | ||
|
|
||
| /** Max time to wait for a single marketplace network request. */ |
There was a problem hiding this comment.
[Suggestion] The new security guards (wall-clock timeout + body-size cap) have no test coverage. These are the exact threat-model mitigations described in the code comments — a slow/trickling server and a hostile server streaming unbounded data — yet nothing verifies they actually work.
Add two tests:
- Mock
https.getwith a server that never sendsend— assert the promise resolves tonullwithinMARKETPLACE_FETCH_TIMEOUT_MS(usevi.useFakeTimers()). - Mock
https.getwith a server that streams >10 MB in chunks — assertreq.destroy()is called and the result isnull.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 158002c77 (marketplace.test.ts). Two tests drive fetchUrl through a single direct-JSON fetch (https://example.com/marketplace.json): (1) a stalled server that connects but never emits data/end → vi.useFakeTimers() + advanceTimersByTimeAsync past the 10s wall-clock deadline asserts it resolves to null and req.destroy() is called; (2) a server streaming an 11 MB chunk asserts the body-size cap aborts mid-stream (req.destroy() called, result null).
| return `${marketplace.source}:${plugin.name}`; | ||
| } | ||
| const src = plugin.source; | ||
| if (typeof src === 'string') { |
There was a problem hiding this comment.
[Suggestion] The local-path injection guard for remote (http) marketplaces — rejecting source strings that are absolute paths, start with ., or start with ~ — has no test coverage. This security guard prevents a hostile remote marketplace from pointing the installer at local filesystem paths.
Since resolveInstallSource is module-private, test it via discoverPlugins:
it('rejects local-path sources from a remote (http) marketplace', async () => {
vi.mocked(loadMarketplaceConfigFromSource).mockResolvedValue(
config('Remote', [
{ name: 'evil', version: '1.0.0', source: '/etc/passwd' },
{ name: 'rel', version: '1.0.0', source: '../../secret' },
{ name: 'home', version: '1.0.0', source: '~/.ssh/id_rsa' },
]),
);
const discovered = await discoverPlugins(
[{ name: 'Remote', source: 'https://x/m.json', type: 'http' }],
new Set(),
);
expect(discovered.find(p => p.name === 'evil')!.installSource).toBe('evil');
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 158002c77 (sourceRegistry.test.ts). A new discoverPlugins test feeds a remote (http) marketplace plugins with /etc/passwd, ../../secret, and ~/.ssh/id_rsa sources (both bare-string and { source: 'url' } forms) and asserts each falls back to the bare plugin name (no install redirect), while a genuine https:// URL is preserved.
| // workspace-scoped choice, re-scope enablement to this workspace | ||
| // only: disable the global enable and enable for the workspace path. | ||
| if (scope !== 'user') { | ||
| await extensionManager.disableExtension( |
There was a problem hiding this comment.
[Critical] Missing scope-change rollback guard.
When scope !== 'user', this disables at User then enables at Workspace. If the Workspace enable fails, the outer catch only logs — the extension is left disabled at all scopes, but the user already saw "Installed N extension(s)."
install.ts:97-130 handles this correctly with an explicit rollback:
| await extensionManager.disableExtension( | |
| if (scope !== 'user') { | |
| await extensionManager.disableExtension( | |
| ext.name, | |
| SettingScope.User, | |
| ); | |
| try { | |
| await extensionManager.enableExtension( | |
| ext.name, | |
| SettingScope.Workspace, | |
| ); | |
| } catch (enableError) { | |
| try { | |
| await extensionManager.enableExtension( | |
| ext.name, | |
| SettingScope.User, | |
| ); | |
| } catch { | |
| // rollback also failed | |
| } | |
| throw enableError; | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Already addressed in 631e271fb (round 5) — this thread targets a pre-round-5 snapshot. The Discover batch-install path already has exactly this disableExtension(User) → try enableExtension(Workspace) → rollback-to-User pattern. Round 6 (158002c77) went further and now also surfaces a rollback-failure to the user (see the related thread on line ~258).
| if (newScope === 'user') { | ||
| await manager.enableExtension(name, SettingScope.User); | ||
| } else { | ||
| await manager.disableExtension(name, SettingScope.User); |
There was a problem hiding this comment.
[Critical] Same scope-change rollback gap as DiscoverTab.tsx:239.
disableExtension(User) runs before enableExtension(Workspace). If the workspace enable throws, the catch at line 243 shows an error toast but does not re-enable at User scope. The extension is left disabled everywhere, and the scope preference metadata says "project" but no enablement exists.
Mirror the install.ts rollback pattern:
| await manager.disableExtension(name, SettingScope.User); | |
| await manager.disableExtension(name, SettingScope.User); | |
| try { | |
| await manager.enableExtension(name, SettingScope.Workspace); | |
| } catch (enableError) { | |
| try { | |
| await manager.enableExtension(name, SettingScope.User); | |
| } catch { | |
| // rollback also failed | |
| } | |
| throw enableError; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Already addressed in 631e271fb (round 5) — pre-round-5 snapshot. handleScope in ExtensionActionsView already rolls the User enable back when the Workspace enable fails, and records the scope preference only after enablement succeeds. Round 6 (158002c77) additionally surfaces the case where the rollback itself fails.
| if (src && src.source === 'github') { | ||
| return `${src.repo}:${plugin.name}`; | ||
| } | ||
| if (src && src.source === 'url') { |
There was a problem hiding this comment.
[Critical] {source: 'url'} branch bypasses the local-path guard applied to string sources.
The string-source branch (lines 168-175) explicitly rejects local filesystem paths from remote marketplaces (isAbsolute, startsWith('.'), startsWith('~')). But this branch returns src.url without any validation, allowing a hostile http-type marketplace to redirect the installer at arbitrary local paths (e.g. /home/victim/.ssh or ../../etc).
Apply the same local-path guard:
| if (src && src.source === 'url') { | |
| if (src && src.source === 'url') { | |
| if ( | |
| typeof src.url === 'string' && | |
| (path.isAbsolute(src.url) || src.url.startsWith('.') || src.url.startsWith('~')) | |
| ) { | |
| debugLogger.warn( | |
| `Ignoring local path source "${src.url}" from remote marketplace "${marketplace.source}".`, | |
| ); | |
| return plugin.name; | |
| } | |
| return src.url; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 158002c77. The { source: 'url' } branch in resolveInstallSource now applies the same local-path guard as the string branch (rejecting absolute / .-prefixed / ~-prefixed values and falling back to the plugin name), so a remote http marketplace can no longer redirect the installer at a local path via the structured form. Covered by the new sourceRegistry.test.ts test.
| return pluginDir; | ||
| } | ||
|
|
||
| if (source.source === 'git-subdir') { |
There was a problem hiding this comment.
[Critical] The git-subdir source type branch (~45 lines) has no test coverage.
This branch handles cloning a repo, pinning to a ref/sha, validating subdirectory confinement (realPathWithin), and guarding against symlink escapes. None of these security-critical code paths — the happy path, path-escape rejection, absolute-path rejection, missing-subdir error, or symlink-escape guard — are tested.
A regression could silently break path confinement, allowing a hostile marketplace entry with path: '../../.ssh' to escape the clone root.
Suggested tests in claude-converter.test.ts:
- Valid
git-subdirsource with a real subdirectory - A
paththat escapes the repo root (e.g.../../etc) - An absolute
path - A subdir that is a symlink pointing outside the clone
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 158002c77 (claude-converter.test.ts). A new git-subdir suite mocks cloneFromGit to populate the clone dir against a real fs and covers: the happy path (clone + subdir returned, asserting the immutable sha is preferred over the named ref), path-escape (../../etc) rejection, absolute-path rejection, missing-subdir rejection, and the symlink-escape guard (realPathWithin).
|
Thanks for the PR, @BZ-D! (Re-run at head Template looks good ✓ — all required sections present (What, Why, Reviewer Test Plan, Evidence, Risk & Scope, Linked Issues). On direction: This is squarely on the extensions/marketplace roadmap. CHANGELOG shows active investment in the extension system ( On approach: At 53 files / +8616 lines, this is a large PR. But the changes are cohesive: every file serves the extensions management feature. The MCP component modifications are refactoring for reuse (extracting shared components), not scope creep. The security hardening layer (ANSI sanitization via shared Round 6 (+302/-8, 10 files) is a focused security and data-integrity hardening pass: closes a real Moving on to code review. 🔍 中文说明感谢贡献，@BZ-D！（在 head 模板完整 ✓ — 所有必填章节齐全。 方向： 完全契合扩展/市场路线图。CHANGELOG 显示项目在扩展系统上的持续投入。将 方案： 53 个文件 / +8616 行，PR 体量较大。但改动内聚：每个文件都服务于扩展管理功能。MCP 组件修改是为了复用的重构，不是范围蔓延。安全加固层（通过共享 第 6 轮（+302/-8，10 文件）是聚焦的安全和数据完整性加固：堵上了真实的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review (re-run at head
|
| Suite | Tests | Status |
|---|---|---|
sourceRegistry.test.ts |
19 | ✅ |
marketplace.test.ts |
25 | ✅ |
claude-converter.test.ts |
40 | ✅ |
extensionPreferences.test.ts |
11 | ✅ |
extensionManager.test.ts |
50 | ✅ |
textUtils.test.ts |
25 | ✅ |
ExtensionsManagerDialog.test.tsx |
23 | ✅ |
ExtensionListStep.test.tsx |
5 | ✅ |
ActionSelectionStep.test.tsx |
5 | ✅ |
| CLI extensions commands (12 suites) | 122 | ✅ |
| Total | 325 | ✅ |
Typecheck — clean on all packages.
CLI command verification (tmux capture-pane):
$ qwen extensions sources list
No marketplace sources added yet.
$ qwen extensions list
No extensions installed.
$ qwen extensions sources --help
Commands:
qwen extensions sources add <source> Adds a marketplace source (Claude format).
qwen extensions sources remove <name> Removes a marketplace source.
qwen extensions sources list Lists configured marketplace sources.
qwen extensions sources update <name> Re-fetches a marketplace source and its plugin listing.
Interactive TUI: The Ink-based TUI renders to the alternate screen buffer, which makes capture-pane unreliable in this headless environment. The collaborator (wenshao) ran a full tmux real-user E2E pass at the same head covering all three tabs, install/uninstall/enable-disable across restarts, scope changes, MCP toggle, the CLI command group, zh rendering, ANSI injection resistance (byte-level PWNED 0 · OSC ]0; 0 · [2K 0), and mutation test on the url-source guard — and reported all passing with APPROVE.
中文说明
代码审查（在 head 158002c773 重审）
独立方案（阅读第 6 轮 diff 之前）：对于市场管理器审查反馈的修复轮次，我预期：(1) 用与字符串 source 分支相同的守卫堵住 { source: 'url' } 本地路径绕过，(2) 把回滚失败显式化而非静默吞掉，(3) 在 marketplace name 成为 Discover 过滤值前做净化，(4) 添加移除守卫就会失败的测试。
与 diff 对比： 第 6 轮完全匹配所有四项预期。url-source guard（sourceRegistry.ts:182-196）使用同样的检查逻辑。回滚失败上报（DiscoverTab.tsx、ExtensionActionsView.tsx）两处都显式抛出带 i18n 的错误消息。Marketplace name 净化（ExtensionsManagerDialog.tsx:114-119）在 handleBrowseSource 中应用 stripUnsafeCharacters。Tab 清除行为修复匹配 (Tab to clear) 提示。
未发现关键阻断问题。
测试结果
- 单元测试：325 项全部通过 ✅（core 170 + CLI UI 33 + CLI 命令 122）
- 类型检查：所有包 clean ✅
- CLI 命令：
sources子命令组（add/list/update/remove）正确注册，help 输出完整 ✅，sources list和extensions list正确输出空状态 - 交互式 TUI：Ink 备用屏幕渲染，无头环境 capture-pane 不可靠。协作者 wenshao 在同一 head 完成完整 tmux E2E 并通过 APPROVE。
— Qwen Code · qwen3.7-max
|
Stepping back to look at the whole picture: this PR has been through 6 rounds of review from maintainers wenshao and DragonnZhang, covering security (ANSI injection, path traversal, fetch hardening), correctness (scope-change rollback, corrupt file handling), and architecture (component reuse, i18n parity). The latest round (6) closes a real install-source path-traversal bypass and surfaces a previously-silent double-failure, both with non-vacuous tests. My independent proposal from Stage 2a matched the PR's approach — the PR actually went further with the rollback-failure surfacing pattern and the corrupt-file quarantine abstraction, both of which are the kind of defensive code I'd want to see in a feature that processes untrusted third-party marketplace data. The scope is large but justified: 15 new files are all purpose-built for this feature, and the 38 modified files are either integration points (DialogManager, i18n, MCP components for reuse) or enhancements to existing extension logic (converter, manager, config). No drive-by refactors, no scope creep. 325 unit tests pass, typecheck is clean, CLI commands work as expected. The interactive TUI couldn't be verified via capture-pane in this headless environment (Ink alt-screen rendering), but the maintainer's comprehensive E2E pass — including byte-level ANSI injection verification and a mutation test proving the url-source guard is genuinely tested — covers that gap thoroughly. If I had to maintain this in six months, I'd thank the author — the code is well-organized, the security boundaries are clearly marked with comments explaining why, and the test coverage is thorough. Approving. ✅ 中文说明整体审视：本 PR 已经历维护者 wenshao 和 DragonnZhang 的 6 轮审查，涵盖安全（ANSI 注入、路径穿越、fetch 加固）、正确性（scope 变更回滚、损坏文件处理）和架构（组件复用、i18n 一致性）。最新第 6 轮堵上了一个真实的安装源路径穿越绕过，并把一个静默的双重失败显式化，二者都带非空泛测试。 我在 Stage 2a 的独立方案与 PR 实现一致——PR 实际上更进一步，增加了回滚失败上报模式和损坏文件隔离抽象，这些正是处理不受信任的第三方市场数据时应有的防御性代码。 体量虽大但合理：15 个新文件均为此功能专属，38 个修改文件要么是集成点（DialogManager、i18n、MCP 组件复用），要么是现有扩展逻辑的增强（转换器、管理器、配置）。无顺手重构，无范围蔓延。 325 项单元测试通过，类型检查 clean，CLI 命令工作正常。交互式 TUI 因 Ink 备用屏幕渲染无法通过 capture-pane 验证，但维护者的全面 E2E 测试——包括字节级 ANSI 注入验证和证明 url-source guard 非空泛的 mutation test——覆盖了此缺口。 六个月后维护此代码，我会感谢作者——代码组织良好，安全边界有清晰的 why 注释，测试覆盖充分。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…on, rollback-failure surfacing, url-source guard, security tests
Code:
- ANSI injection via the Discover marketplace filter: the marketplace name
(untrusted, from a remote marketplace.json) flowed through onBrowse to the
Discover hint render unsanitized. Scrub it in handleBrowseSource — this also
fixes the filter comparison (it is matched against the already-sanitized
DiscoveredPlugin.marketplaceName).
- '(Tab to clear)' hint was misleading: Tab cycled tabs rather than clearing
the marketplace filter in place. On Discover with an active filter, Tab now
clears the filter in place, matching the hint.
- Scope-change rollback failures were silently swallowed by a bare catch in
both the Discover batch install and ExtensionActionsView, unlike the CLI
install.ts which surfaces them. Both now report the rollback failure so the
user knows the extension may be disabled at all scopes (new i18n keys for
en/zh/zh-TW).
- resolveInstallSource: the structured { source: 'url' } branch bypassed the
local-path guard applied to string sources, letting a remote http
marketplace redirect the installer at a local filesystem path. Apply the
same guard.
Tests:
- marketplace fetchUrl: wall-clock deadline (stalled server) and body-size cap
(oversized stream) now covered.
- sourceRegistry: remote-marketplace local-path rejection covered for both the
string and { source: 'url' } source forms.
- claude-converter git-subdir: clone+sha-pin happy path plus path-escape,
absolute-path, missing-subdir, and symlink-escape rejections covered.
Note: the bot's 'missing scope rollback' threads target a pre-631e271fb
snapshot — that rollback already landed in round 5.
|
Thanks for round 6 @wenshao and the ✅ Fixed — security / correctness
✅ Added — security tests
↩️ Already addressed (bot reviewed a pre-round-5 snapshot)
Validation: core extension suites 95 ✅ (sourceRegistry 20 · marketplace 25 · claude-converter 40 · extensionPreferences 11 — includes the new tests); cli Re-requesting review 🙏. 中文第 6 轮已在
验证:core 扩展套件 95 ✅、cli 47 ✅(NO_COLOR)、check-i18n / tsc / eslint / prettier 全绿。 |
|
@qwen-code /triage |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Round 6 review — build green, eslint clean, 309 core + 155 CLI extension tests pass. No high-confidence critical issues remain after 5 prior rounds of hardening. One new suggestion on an unhandled source variant, plus low-confidence terminal-only findings (code duplication, test coverage gaps, debuggability) listed in the Needs Human Review section of the terminal output.
| } | ||
| return plugin.name; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] resolveInstallSource does not handle the git-subdir source variant defined in ClaudePluginSource (claude-converter.ts:98). When a remote (HTTP) marketplace plugin declares { source: 'git-subdir', url, path, ref?, sha? }, the function falls through to return plugin.name — a bare name that parseInstallSource cannot resolve to an installable artifact.
The sibling resolvePluginSource in claude-converter.ts:1088 correctly handles git-subdir during the install phase, but the Discover tab's install flow depends on this function to produce the installSource string. Without a matching branch, git-subdir plugins from HTTP marketplaces fail to install from the Discover tab.
| if (src && src.source === 'url') { | |
| return src.url; | |
| } | |
| if (src && src.source === 'git-subdir') { | |
| return src.url; | |
| } | |
| return plugin.name; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — I looked into this again rather than re-deferring, and the return src.url one-liner would introduce a silent wrong-install, so I'd like to push back and propose a proper follow-up instead.
Why return src.url is unsafe: a git-subdir source is { url, path, ref?, sha? } — the plugin lives in subdirectory path, optionally pinned to ref/sha. But ExtensionInstallMetadata (config.ts:540) only has source / type / ref — there is no subdirectory field, and parseInstallSource has no subdir syntax. So return src.url drops both path and the pin and clones the repo root: it then installs the whole repo as a standalone extension, or (if the repo ships its own marketplace.json) prompts the user to pick an unrelated plugin. Either way it installs something other than the subdir plugin the entry describes — strictly worse than today's explicit failure.
Scope of the gap: string- and github-form sources from an HTTP marketplace already install git-subdir correctly, because their installSource is a repo:plugin form that routes back through convertClaudePluginPackage → resolvePluginSource, which fully handles git-subdir (now covered by the new tests in 158002c77). The unsupported case is narrowly a direct { source: 'git-subdir' } entry in an http-type (direct-JSON) marketplace.
Proper fix: thread the subdir + ref through the install pipeline (add a path/subdir field to ExtensionInstallMetadata and teach the git clone to check out the subdir at the pinned ref) — a self-contained capability with its own tests that I'd prefer to land as a follow-up rather than fold into this already-large PR. If you'd rather have it in-scope here, say the word and I'll add it (with tests) in the next push. (Mirrors my reply on the original thread for this same item.)
✅ Re-verification at new head
|
Re-check (delta 631e271fba..158002c773) |
Result |
|---|---|
npm ci + npm run build (macOS) |
✅ exit 0 |
| Round-6 core suites (+235 lines of new security tests) | ✅ 170 passed |
| Mutation test on the url-source guard | ✅ removing it fails the new test → non-vacuous |
| Real-TUI smoke on the round-6 binary | ✅ 3 tabs OK, ANSI sanitization intact, no regression |
| CI | Lint ✅ · 3× Test + CodeQL still pending |
1. What round 6 changed
- url-source path-traversal guard (the security fix).
resolveInstallSourcealready refused local-filesystem paths from a remotehttpmarketplace in the bare-stringsourceform (/etc/passwd,../x,~/x). Round 6 extends the identical guard to the structured{ source: 'url', url: … }form, which previously bypassed it — a hostile marketplace could set{ source: 'url', url: '/etc/shadow' }and redirect the installer at a local path. Now it falls back to the plugin name; genuinehttps://…URLs are preserved. - Rollback-failure surfacing (data-integrity UX). The scope-change flow disables at User scope then enables at Workspace scope; if the enable fails it rolls the User enable back. If the rollback itself also failed, both
ExtensionActionsViewandDiscoverTabpreviously swallowed it (catch {}) — leaving the extension disabled at every scope with the user told it succeeded. Both now surface it explicitly ("…may be disabled at all scopes; re-enable it from the Installed tab"). - Marketplace-name filter sanitization (defense-in-depth: the Browse→Discover filter value is run through
stripUnsafeCharacters), plus i18n strings and +235 lines of new security tests (claude-converter+133,marketplace+57,sourceRegistry+45).
2. Tests (macOS)
core (6): sourceRegistry 19 · marketplace 25 · claude-converter 40 · extensionPreferences 11
extensionManager 50 · textUtils 25 -> 170 passed
3. Mutation test — the url-source guard is genuinely tested ⭐
The new discoverPlugins > rejects local-path sources from a remote (http) marketplace test asserts both source forms (bare-string and structured url) for absolute / relative / ~ paths fall back to the plugin name, while a real https://… URL is preserved. I reverted just the round-6 guard ({ source:'url' } branch back to return src.url) and re-ran:
× discoverPlugins > rejects local-path sources from a remote (http) marketplace
-> expected '/etc/shadow' to be 'urlabs' (installer would be pointed at /etc/shadow)
Tests 1 failed | 18 passed (19)
Exactly that test fails, and the failure value /etc/shadow is the vulnerability the guard closes. Restored -> 19/19 green.
4. Real-TUI smoke (no regression from the DiscoverTab / ExtensionActionsView edits)
Round-6 binary, isolated $HOME, hostile-marketplace fixture. All three tabs still render and navigate; the hostile plugin still sanitizes (evil<ESC>[31m-x<ESC>[2K -> evil-x, desc -> safe ok); byte-level PWNED 0 · OSC ]0; 0 · [2K 0; pane title stays "Qwen - project". The round-6 changes are guard / error-path logic, so the decisive proof is the mutation above; the smoke confirms the UI edits didn't regress rendering.
Verdict
Round 6 is a clean security + data-integrity hardening round: a real install-source path-traversal bypass closed (with a non-vacuous test), a silent double-failure made visible, all green locally. Recommendation stands: merge once the pending CI matrix is green. (As before, a red macOS Test job would most likely be the unrelated cronScheduler durable-ownership timing flake, not this PR.)
🇨🇳 中文版（点击展开）
✅ 在新 head 158002c773 上的复验(维护者)—— 结论不变:建议合并
本评论取代我第 4/5 轮的验证(head 631e271fba)。之后新增一个 commit —— review 第 6 轮(158002c773,+302/-8,10 文件)。要点:它堵上了一个真实的路径穿越绕过(结构化 url 安装源),并把一个此前静默的数据完整性失败显式化,二者都带新测试。已在 macOS 复验。
复检项(增量 631e271fba..158002c773) |
结果 |
|---|---|
npm ci + npm run build(macOS) |
✅ exit 0 |
| 第 6 轮 core 套件(+235 行新安全测试) | ✅ 170 通过 |
| 对 url-source guard 的 mutation test | ✅ 移除后新测试失败 → 非空泛 |
| round-6 二进制实机冒烟 | ✅ 三 tab 正常、ANSI 净化完好、无回归 |
| CI | Lint ✅ · 三平台 Test + CodeQL 仍 pending |
1. 第 6 轮改了什么
- url-source 路径穿越 guard(安全修复)。
resolveInstallSource此前已对远程httpmarketplace 的裸字符串source形式拒绝本地路径(/etc/passwd、../x、~/x)。第 6 轮把同样的 guard 扩展到结构化{ source:'url', url:… }形式 —— 它此前能绕过:恶意 marketplace 可设{ source:'url', url:'/etc/shadow' }把安装器重定向到本地路径。现在会 fallback 到 plugin name;真实https://…URL 保留。 - rollback 失败上报(数据完整性 UX)。 scope 变更流程先 disable User 再 enable Workspace;若 enable 失败则回滚 User enable。若回滚本身也失败,
ExtensionActionsView和DiscoverTab此前都把它静默吞掉(catch {})—— 扩展在每个 scope 都被禁用,却告诉用户成功了。现在两处都显式上报("可能在所有 scope 都被禁用;从 Installed tab 重新启用")。 - marketplace-name 过滤净化(纵深防御:Browse→Discover 的过滤值过一遍
stripUnsafeCharacters),外加 i18n 字符串和 +235 行新安全测试(claude-converter+133、marketplace+57、sourceRegistry+45)。
2. 测试(macOS)
core (6): sourceRegistry 19 · marketplace 25 · claude-converter 40 · extensionPreferences 11
extensionManager 50 · textUtils 25 -> 170 通过
3. Mutation test —— url-source guard 确有测试守护 ⭐
新测试 discoverPlugins > rejects local-path sources from a remote (http) marketplace 断言两种 source 形式(裸字符串与结构化 url)的绝对/相对/~ 路径都 fallback 到 plugin name,而真实 https://… URL 保留。我只把第 6 轮的 guard 还原({ source:'url' } 分支退回 return src.url)后重跑:
× discoverPlugins > rejects local-path sources from a remote (http) marketplace
-> expected '/etc/shadow' to be 'urlabs' (安装器会被指向 /etc/shadow)
Tests 1 failed | 18 passed (19)
恰好该测试失败,且失败值 /etc/shadow 正是 guard 所堵的漏洞。还原后 -> 19/19 绿。
4. 实机冒烟(DiscoverTab / ExtensionActionsView 改动无回归)
round-6 二进制、隔离 $HOME、恶意 marketplace fixture。三个 tab 仍正确渲染和导航;恶意插件仍被净化(evil<ESC>[31m-x<ESC>[2K -> evil-x,描述 -> safe ok);字节级 PWNED 0 · OSC ]0; 0 · [2K 0;pane 标题仍为 "Qwen - project"。第 6 轮是守卫/错误路径逻辑,决定性证明是上面的 mutation;冒烟确认 UI 改动没有破坏渲染。
结论
第 6 轮是干净的安全 + 数据完整性加固:堵上了一个真实的安装源路径穿越绕过(带非空泛测试),把一个静默的双重失败变可见,本地全绿。结论不变:建议合并 —— 待 pending 的 CI 矩阵转绿即可。(同前,若 macOS Test 变红,大概率仍是与本 PR 无关的 cronScheduler durable-ownership 计时 flaky。)
Maintainer re-verification · head 158002c773 (prev 631e271fba) · macOS 26.5 / Node 22.22.2.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes | 9 review agents | 18/19 findings verified | 1 reverse-audit round
This review identified 5 new findings not yet covered in the existing PR discussion threads. Many other issues found by our agents were already discussed and/or fixed in earlier review rounds — thanks to the reviewers for the thorough prior passes.
Critical (3)
-
marketplace.ts:262—loadMarketplaceConfigFromSource("owner/repo")passes throughstat()at Priority 1 before the owner/repo GitHub shorthand is checked at Priority 4. If a local directory matchingowner/repoexists in cwd, the function silently returns the local marketplace config instead of fetching from GitHub. Fix: add a shorthand-detection heuristic beforestat()to skip it for likelyowner/repopatterns. -
sourceRegistry.ts:345—discoverPluginsdedup key${plugin.marketplaceName}/${plugin.name}uses display names, not source URLs. Two distinct plugins from different sources that share the same marketplace + plugin name collide, and one is silently dropped. Fix: include the source URL or registry identifier in the dedup key. -
config.ts:3254—isMcpServerDisabled()only checksextensionPreferences.disabledMcpServers, missing runtime MCP servers. An extension-bundled server name that coincidentally matches a disabled entry reportsdisabled: trueeven when loaded via the runtime path.
Suggestion (2)
-
extensionPreferences.ts:105—favoritesarray validated withArray.isArrayonly, no element-type filter. Sibling fields (scopes,disabledMcpServers) validate both container and element types. Fix:Array.isArray(parsed.favorites) && parsed.favorites.every((v: unknown) => typeof v === 'string'). -
sourceRegistry.test.ts— Novi.mock('fs')setup. Tests create real temp files viaos.tmpdir(), which is fragile (race conditions, leftover files). SiblingextensionPreferences.test.tsusesvi.mock('fs'). Recommend matching that pattern.
Findings deduplicated against 160+ existing PR comments. Build and all 378 unit tests pass.
— qwen3.7-max via Qwen Code /review
| const trimmed = source.trim(); | ||
|
|
||
| // Priority 1: local path (directory with .claude-plugin/marketplace.json, | ||
| // or a direct marketplace.json file). |
There was a problem hiding this comment.
[Critical] Priority 1 local-path check shadows the owner/repo GitHub shorthand (Priority 4).
loadMarketplaceConfigFromSource("owner/repo") passes "owner/repo" to stat() at Priority 1. On Linux, stat("owner/repo") succeeds if a local directory ./owner/repo/ exists — and the function returns the local marketplace config instead of resolving the GitHub shorthand at Priority 4.
This is a subtle correctness issue: a user running the CLI from a directory that happens to contain a folder matching owner/repo silently gets the wrong marketplace data.
Fix: before the stat() call, add a heuristic that skips Priority 1 for sources that look like a GitHub shorthand:
| // or a direct marketplace.json file). | |
| // Priority 1: local path — skip for likely GitHub shorthands (owner/repo) | |
| const looksLikeShorthand = /^[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+$/.test(trimmed); | |
| if (!looksLikeShorthand) { | |
| try { | |
| const stats = await stat(trimmed); | |
| if (stats.isDirectory()) { | |
| return await readLocalMarketplaceConfig(trimmed); | |
| } | |
| if (stats.isFile()) { | |
| try { | |
| const content = await fs.promises.readFile(trimmed, 'utf-8'); | |
| return JSON.parse(content) as ClaudeMarketplaceConfig; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| } catch { | |
| // Not a local path — fall through to next priority | |
| } | |
| } |
Alternatively, reorder priorities so the owner/repo check runs before the local-path stat.
— qwen3.7-max via Qwen Code /review
| const seen = new Set<string>(); | ||
| const deduped: DiscoveredPlugin[] = []; | ||
| for (const plugin of results.flat()) { | ||
| const key = `${plugin.marketplaceName}/${plugin.name}`; |
There was a problem hiding this comment.
[Critical] discoverPlugins dedup key uses display names, causing cross-source collisions.
The dedup key ${plugin.marketplaceName}/${plugin.name} uses display names, not source URLs. Two distinct plugins from different sources that share the same marketplace + plugin name collide, and one is silently dropped.
Example: Source A (GitHub) and Source B (HTTP marketplace) both declare a plugin named "my-ext" under marketplace "MyMarket". The second is dropped even though they may be entirely different plugins.
Fix: include the source URL or registry identifier in the dedup key:
| const key = `${plugin.marketplaceName}/${plugin.name}`; | |
| const key = `${plugin.source ?? 'unknown'}:${plugin.marketplaceName}/${plugin.name}`; |
Or, if DiscoveredPlugin carries a sourceUrl / registryUrl field, use that instead.
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
| const prefs: ExtensionPreferences = { | ||
| favorites: Array.isArray(parsed.favorites) ? parsed.favorites : [], |
There was a problem hiding this comment.
[Suggestion] favorites array is not validated for element types, unlike scopes and disabledMcpServers.
Array.isArray(parsed.favorites) accepts any array, including [1, null, true, "real-ext"]. The sibling fields use proper element-type guards:
// scopes — validates each entry
scopes: isRecord(parsed.scopes) ? Object.fromEntries(...) : {}
// disabledMcpServers — validates each entry
disabledMcpServers: isRecord(parsed.disabledMcpServers) ? Object.fromEntries(...) : {}Fix:
| favorites: Array.isArray(parsed.favorites) ? parsed.favorites : [], | |
| favorites: Array.isArray(parsed.favorites) && parsed.favorites.every((v: unknown) => typeof v === 'string') ? parsed.favorites : [], |
While non-string elements would never match .includes(name) (since name is a string), the inconsistency can mask bugs at deserialization time.
— qwen3.7-max via Qwen Code /review
| @@ -3252,7 +3252,25 @@ export class Config { | |||
| } | |||
|
|
|||
| isMcpServerDisabled(serverName: string): boolean { | |||
There was a problem hiding this comment.
[Critical] isMcpServerDisabled() doesn't check runtimeMcpServers, leading to false positives.
Runtime MCP servers (registered programmatically by extensions or plugins, not from config files) are not checked here. If an extension registers a runtime server named "foo", and an unrelated extension-bundled server named "foo" is disabled in preferences, isMcpServerDisabled('foo') returns true — incorrectly suppressing the runtime server.
The check should distinguish runtime from extension-bundled servers:
| isMcpServerDisabled(serverName: string): boolean { | |
| isMcpServerDisabled(serverName: string): boolean { | |
| const prefs = this.getExtensionPreferences(); | |
| const isDisabled = prefs.disabledMcpServers[serverName] === true; | |
| // Runtime MCP servers are not managed by extension preferences | |
| if (this.isRuntimeMcpServer?.(serverName)) { | |
| return false; | |
| } | |
| return isDisabled; | |
| } |
— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,316 @@ | |||
| /** | |||
There was a problem hiding this comment.
[Suggestion] sourceRegistry.test.ts lacks vi.mock('fs') — tests hit real filesystem.
The test file imports SourceRegistryStore which uses fs.readFileSync/fs.writeFileSync internally. Unlike sibling extensionPreferences.test.ts, there is no vi.mock('fs') setup.
Tests create real temp files via os.tmpdir(), which works but is fragile:
- Race conditions with concurrent test runs
- Leftover temp files on crash
- Platform-specific path behavior differences
Recommend adding vi.mock('fs') with memfs or vitest's built-in vi.fs() mock to match the sibling test pattern:
vi.mock('fs');
// or
vi.mock('node:fs');
vi.mock('node:fs/promises');— qwen3.7-max via Qwen Code /review
What this PR does
Turns
/extensionsinto an interactive, multi-tab manager instead of a flat read-only list. Three tabs — Installed, Discover, Sources — cover the full lifecycle of finding, installing, configuring and removing extensions (and standalone MCP servers).Installed
/mcpdialog components (ServerDetailStep/ToolListStep/AuthenticateStep), so an installed MCP server shows live connection status and offers View tools, Enable/Disable, Reconnect, Re-authenticate and Clear authentication. Extension-bundled MCP servers nest under their parent extension and can be disabled per-server.Discover
Ctrl+Rre-fetches the listings.Sources
qwen extensions sources add/list/update/removeCLI group.Core
owner/repo, https/git URLs,git@SSH, scoped npm packages, local paths, and Claude marketplace manifests (.claude-plugin/marketplace.json).extensionPreferences.ts): persisted favorites and per-extension scope intent that drive the Installed-tab grouping.--scopeonextensions install.i18n
zh), Traditional Chinese (zh-TW) and English (en) translations for all new strings; key parity is enforced bynpm run check-i18n.Why it's needed
The previous
/extensionswas a read-only list — installing, enabling/disabling, re-scoping, updating, removing, browsing marketplaces, and managing extension-bundled MCP servers all required separate CLI commands or weren't possible at runtime. This consolidates the whole extension lifecycle into one hot-reloading TUI aligned with the project's extensions/marketplace roadmap and with the existing/mcpmanager (whose components it reuses), so users can discover and manage Gemini- and Claude-format extensions without leaving the session.Reviewer Test Plan
How to verify
npm run build, launch the CLI, run/extensions(or/extensions manage).Enterfor detail, install at a chosen scope,Ctrl+Rto refresh. Confirm the installed plugin is converted toqwen-extension.jsonand its commands/skills become available.Spaceenable/disable,ffavorite,Enteraction menu (change scope / mark update / uninstall). Confirm groups re-sort and favorites/scope/enablement persist across restart. Open an MCP server's detail and confirm live status + View tools / Reconnect / auth actions.~/.qwen/extensions/marketplaces.jsonis updated.qwen extensions sources add/list/update/removeandqwen extensions install <src> --scope project.Evidence (Before & After)
claude-converter,marketplace,sourceRegistry(61 tests) and the fullcliextensions suites incl.ExtensionsManagerDialogandsources(152 tests) — pass, plusprettier/eslint/tscon the changed files. A full TUI E2E was not run locally.Tested on
macOS ✅ = unit/lint/type checks for the changed code, run locally this round. Linux ✅ = collaborator E2E (linked above). Windows⚠️ = not manually verified locally; relying on CI (ubuntu/macos/windows unit + CodeQL green).
Environment (optional)
npm run build+ built CLI in a tmux TUI session (collaborator); unit tests viavitest(local, macOS).Risk & Scope
05eb44f6(ANSI sanitization, path-traversal confinement, fetch size/time caps, homepage scheme check) is the security-relevant part to scrutinize.git-subdirinstall from anhttp-type marketplace is a known gap (clear error, not a wrong install) tracked as a follow-up.Linked Issues
N/A — no tracking issue; aligns with the extensions/marketplace roadmap.