fix(extensions): preserve Claude hooks in dual-manifest extensions - #8626
fix(extensions): preserve Claude hooks in dual-manifest extensions#8626destire-mio wants to merge 32 commits into
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @destire-mio!
The underlying problem is real — #8539 was reproduced end-to-end in the issue thread and a fix in this direction was explicitly welcomed there. But the PR body doesn't follow the PR template, so triage stops here before code review.
What's missing:
## What this PR doesand## Why it's needed— yourSummary/Root causecontent covers this; it just needs the template headings.## Reviewer Test Planwith### How to verify,### Evidence (Before & After), and the### Tested ontable. This is the part maintainers weigh most: how should a reviewer confirm a dual-manifest extension now loads its Claude hooks? The E2E plan you committed under.qwen/e2e-tests/is a good foundation for this section.## Risk & Scope— main risk/tradeoff, what was not validated, breaking changes.## Linked Issues— currently justFixes #8539in prose.- The Chinese translation in a
<details>block, as the template requires.
Please rework the PR body to follow the template, then trigger a re-run with @qwen-code /triage and we'll pick it up from there.
中文说明
感谢提交 PR，@destire-mio！
问题是真实存在的 —— #8539 已在 issue 中被端到端复现，且明确欢迎这个方向的修复。但 PR 描述没有遵循 PR 模板，所以审查在进入代码阶段前先停在这里。
缺失的部分：
## What this PR does和## Why it's needed—— 现有Summary/Root cause的内容已覆盖，只需套用模板标题。## Reviewer Test Plan，包含### How to verify、### Evidence (Before & After)和### Tested on表格。这是维护者最看重的部分：reviewer 该如何确认双清单扩展现在能加载其 Claude 钩子？你提交在.qwen/e2e-tests/下的 E2E 计划很适合作为该节的基础。## Risk & Scope—— 主要风险/权衡、未验证的内容、破坏性变更。## Linked Issues—— 目前只有一句Fixes #8539。- 模板要求的
<details>中文翻译。
请按模板重写 PR 描述，然后用 @qwen-code /triage 重新触发审查，我们会继续跟进。
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the new extension-converter tests is unverified.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| ).toBe( | ||
| expectedHookPath | ||
| ? path.join(installedDir, expectedHookPath) | ||
| : undefined, |
There was a problem hiding this comment.
[Critical] The installed-config hook assertions compare the hydrated ${CLAUDE_PLUGIN_ROOT} command against a path.join-built expectation with strict .toBe, without normalizing path separators. hydrateString is plain string substitution — the literal / after the variable survives hydration, so on Windows the hydrated command mixes separators (C:\...\ponytail/scripts/session-start.sh) while path.join builds all-backslashes — Failure scenario: merge-queue run on the required test_windows job → 6 of 7 cases in this file fail (these five it.each cases plus the same unnormalized comparison at the subdirectory assertion, line 466) → required check red, merge blocked. Probe-confirmed with a simulated Windows installedDir; the codebase precedent (extensionManager.test.ts 'uses the installed path for Claude plugin root replacement') wraps the substituted string in path.normalize() before comparing. Suggested fix (both sites):
const command = (
installedConfig.hooks?.['SessionStart']?.[0].hooks?.[0] as {
command?: string;
}
)?.command;
expect(command ? path.normalize(command) : command).toBe(
expectedHookPath
? path.join(installedDir, expectedHookPath)
: undefined,
);— qwen3.8-max via Qwen Code /review (v0.21.6)
| return { | ||
| installSource: `${marketplace.source}:${plugin.name}`, | ||
| pluginSourceKind: 'marketplace-entry', | ||
| }; |
There was a problem hiding this comment.
[Critical] The unconditional 'marketplace-entry' tag routes marketplace entries that omit the source field into convertClaudePluginPackage, where resolvePluginSource dereferences source.source on undefined and throws a raw TypeError — Failure scenario: a git/github/local marketplace entry {name: 'myplugin'} with no source and a root Qwen/Gemini manifest → Discover install (and CLI repo:plugin, which defaults the kind identically) hard-fails post-PR with a raw TypeError; pre-PR the same install succeeded via root-manifest precedence. A/B-proven at the reviewed commit. The codebase anticipates the shape (selectedMarketplacePluginLocation optional-chains on it; this PR's own http fall-through handles sourceless entries), and nothing on the path validates it. Updates of pre-PR installs are unaffected. Suggested fix: treat a missing source as the marketplace root — return 'root' from selectedMarketplacePluginLocation when the entry exists but has no source, and return marketplaceDir from resolvePluginSource for source === undefined instead of falling through to source.source.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| newExtensionDir = geminiConversion.convertedDir; | ||
| originSource = 'Gemini'; |
There was a problem hiding this comment.
[Critical] The merge branch installs Claude hooks but tags the artifact originSource: 'Gemini', so the install-time Claude file-adaptation pass (performVariableReplacement, gated on originSource === 'Claude') is skipped for exactly the artifacts this PR newly activates Claude hooks in — Failure scenario: a dual-manifest repo whose hook scripts parse the Claude transcript shape (jq '.message.content | map(select(.type == "text"))') or reference ~/.claude/ paths → hooks execute unadapted: the jq filter matches nothing in Qwen's .message.parts transcript and ~/.claude paths don't exist → the preserved hooks silently fail or produce empty output. Runtime-proven with a flipping control: a pure-Claude install adapts identical scripts (.message.parts, ~/.qwen); a dual-manifest install does not. Load-time hydration only fixes ${CLAUDE_PLUGIN_ROOT} in hook-command strings — never file contents. Suggested fix: run the adaptation whenever Claude-side hooks were merged (have the merge branch report a Claude component / distinct origin marker and widen the gate on it) — admitting 'Gemini' wholesale would also cover pure-Gemini installs.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const mergedConfig = { | ||
| ...geminiConversion.config, | ||
| hooks: claudeConversion.config.hooks ?? geminiConversion.config.hooks, | ||
| }; |
There was a problem hiding this comment.
[Critical] Writing the Claude hooks into the merged config suppresses loadExtension's hooks/hooks.json directory fallback (if (!extension.hooks)) for the installed artifact — Failure scenario: a dual-manifest repo whose Gemini-side hooks live in the conventional hooks/hooks.json directory (pre-PR the only way a Gemini-shaped install could have hooks — the Gemini config has no hooks key) and whose Claude plugin.json declares inline hooks → post-PR the directory hooks silently stop running at every session start. Runtime-proven: the installed artifact still contains hooks/hooks.json, but loading yields only the Claude events; a pre-PR-shape control loads the directory hooks via the fallback. Suggested fix: when merging, also load hooks/hooks.json from the source if present and merge its per-event hook arrays with the Claude-side hooks, instead of letting the inline hooks key shadow the directory convention.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| it('does not re-prompt for a marketplace plugin when the source is an extension root', async () => { | ||
| const sourceDir = path.join(tempWorkspaceDir, 'direct-root-source'); | ||
| writeExtractedExtension(sourceDir, 'direct-root-extension'); |
There was a problem hiding this comment.
[Suggestion] No test replays issue #8539's actual install vector through installExtension with a 'marketplace-entry' kind — kind defaulting in parseInstallSource, prompt gating, kind pass-through (~line 1817) and metadata persistence are unasserted through the manager; the converter is tested directly instead — Concrete cost: a future change to any of those seams could silently re-break the exact command from issue #8539 (qwen extensions install https://github.com/DietrichGebert/ponytail:ponytail) with no failing test. A live replay confirms the path works today, so this is a coverage gap, not a live bug. Suggested fix: add a case installing a dual-manifest fixture with {type: 'local', source, originSource: 'Claude', pluginName, marketplaceConfig} (no pluginSourceKind, mirroring the CLI default) asserting the installed config carries the Claude hooks + Gemini resources and the persisted metadata carries the kind.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| } finally { | ||
| if (claudeConversion) { | ||
| removeConvertedDirectory(claudeConversion.convertedDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] Both cleanup paths in the merge branch are untested — the finally removal of the Claude staging dir (success and failure) and the catch removal of the Gemini staging dir when the Claude conversion throws — Concrete cost: mutation-verified — removing both keeps 171 tests green; every successful dual-manifest install would leak a full temp copy of the extension, and a malformed plugin.json would additionally leak the Gemini staging dir while failing the install, undetected. Same leak class the claude-converter cleanup test was written to prevent, one level up. Suggested fix: spy on ExtensionStorage.createTmpDir in one dual-manifest case asserting every staging dir except the returned one is removed, plus one case with an invalid plugin.json asserting rejection and no surviving staging dir.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| claudeConversion = rootMarketplacePluginName | ||
| ? await convertClaudePluginPackage( | ||
| extensionDir, | ||
| rootMarketplacePluginName, |
There was a problem hiding this comment.
[Suggestion] The merge branch turns any Claude-side manifest defect into a hard install failure — convertClaudePluginStandalone performs an unguarded JSON.parse and throws on non-object bodies — removing the pre-PR tolerance where the Gemini side installed independently (pre-PR branch order checked the Gemini manifest first and never read the Claude manifest) — Failure scenario: a dual-manifest extension whose plugin.json is invalid JSON / empty / null / [] (placeholder or corrupt checkout) → install fails outright post-PR, and manual/auto updates of already-installed legacy dual-manifest extensions fail via the update-from-state path; pre-PR the same fixture installed via the Gemini path. A/B-proven at the reviewed commit. The tolerance removal is not documented as an intended design change. Suggested fix: wrap the Claude conversion in the merge branch in try/catch; on failure debugLogger.warn(...) and fall back to the Gemini-only conversion result.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expectedHookPath: 'scripts/session-start.sh', | ||
| pluginSourceKind: 'extension-root' as const, | ||
| includeMarketplace: true, |
There was a problem hiding this comment.
[Suggestion] The only 'extension-root' it.each case picks an entry name that does NOT match pluginName, so it executes the identical path as the kind-less legacy case — no test anywhere exercises the !isExplicitExtensionRoot guard on rootMarketplacePluginName ('extension-root' × a root-located matching entry) — Concrete cost: mutation-proven — deleting the guard flips a probe pinning the seam (the entry's hooks overlay silently replaces the root plugin.json hooks) while every existing test stays green. The fork is reachable: http-marketplace Discover installs are tagged 'extension-root' while CLI repo:plugin defaults to 'marketplace-entry', so the same dual-manifest repo installs different hooks by entry route; metadata persistence makes it live on updates too. Suggested fix: add a case with pluginSourceKind: 'extension-root', marketplacePluginName matching pluginName and marketplaceSource: './', asserting the root plugin.json hooks win.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| } finally { | ||
| try { | ||
| await fs.promises.rm(pluginDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
[Suggestion] The failure-path cleanup of the staging dir is untested — the only test observing createTmpDir exercises the success path exclusively, and pre-PR the staging dir lived inside the cloned marketplace dir (reclaimed transitively even when conversion threw) while post-PR it is a standalone mkdtemp dir in os.tmpdir() whose only cleanup is this finally — Concrete cost: empirically demonstrated — hoisting resolvePluginSource out of the try (a narrowed-try mutation) keeps all 53 tests green while a staging dir leaks after a rejected conversion. The current code is correct; this is a test gap, not a live leak. Suggested fix: add a case that spies createTmpDir, forces a throw after staging-dir creation (a strict: true entry lacking plugin.json, or an escaping relative source), and asserts every created staging dir no longer exists.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const converted = await convertGeminiOrClaudeExtension( | ||
| extensionDir, | ||
| 'ponytail', | ||
| undefined, | ||
| undefined, | ||
| 'marketplace-entry', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The only 'marketplace-entry' case uses a subdirectory entry, so the !selectedMarketplaceEntryUsesRoot guard — which keeps root-located (source: './') entries on the dual-manifest merge branch — has zero coverage (mirror-image seam to the !isExplicitExtensionRoot comment above) — Concrete cost: mutation-proven — removing the guard keeps 239 tests in the extension files and 593 across src/extension green, and a probe flips originSource Gemini→Claude, loses Gemini settings/contextFileName, and stops TOML→MD command conversion. Reachable via non-http Discover entries and CLI repo:plugin installs against a dual-manifest repo whose entry sits at source: './'. Suggested fix: add a case with pluginSourceKind: 'marketplace-entry', a matching entry name and marketplaceSource: './', asserting originSource stays 'Gemini' and Gemini settings/contextFileName survive.
— qwen3.8-max via Qwen Code /review (v0.21.6)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the updated extension-converter tests is unverified.
— qwen3.8-max via Qwen Code /review (v0.21.7)
|
|
||
| export interface ClaudeMarketplacePluginConfig extends ClaudePluginConfig { | ||
| source: string | ClaudePluginSource; | ||
| source?: string | ClaudePluginSource; |
There was a problem hiding this comment.
[Critical] Making ClaudeMarketplacePluginConfig.source optional breaks an untouched consumer: packages/cli/src/serve/routes/workspace-extensions.ts:351-356 dereferences plugin.source without an undefined guard — Failure scenario: npm run build --workspace=packages/cli fails at this commit with 5 errors (TS18048 ×3, TS2339 ×2 at workspace-extensions.ts:353-355) while the merge base builds green, so the branch cannot merge as-is. Beyond the compile break, sourceless marketplace entries are now an actively produced shape (this PR's resolvePluginSource and selectedMarketplacePluginLocation treat them as root plugins, and claude-converter.test.ts tests them): once the type error is patched naively, a daemon install of a marketplace containing a sourceless entry that reaches the plugin-choice prompt throws TypeError: Cannot read properties of undefined (reading 'source'). The other two requestChoicePlugin implementations (TUI AppContainer.tsx, non-interactive consent.ts) only read name/description and are unaffected. Suggested fix (in the consumer, matching its existing conditional-spread style):
...(plugin.source
? {
source: redactExtensionDisplaySource(
typeof plugin.source === 'string'
? plugin.source
: plugin.source.source === 'github'
? plugin.source.repo
: plugin.source.url,
),
}
: {}),— qwen3.8-max via Qwen Code /review (v0.21.7)
| if ( | ||
| isExplicitMarketplaceEntry && | ||
| pluginName && | ||
| !selectedMarketplaceEntryUsesRoot | ||
| ) { |
There was a problem hiding this comment.
[Critical] The explicit marketplace-entry branch takes precedence over the Qwen/Gemini root-manifest branches, while parseInstallSource (marketplace.ts:511) defaults every repo:plugin install to kind 'marketplace-entry' — the CLI install.ts, /extensions install, SourcesTab.tsx, and the daemon routes all pass no kind — Failure scenario: qwen extensions install owner/gemini-ext:gemini-ext against a classic Gemini extension repo (root gemini-extension.json, no .claude-plugin/) → selectedMarketplacePluginLocation returns 'missing-marketplace', this branch calls convertClaudePluginPackage, and the install hard-fails with Marketplace configuration not found at .../.claude-plugin/marketplace.json (an opaque temp-clone path); pre-PR the Gemini branch ran first and the same command installed fine. Probe-verified at this commit: Gemini-only, Qwen-only, and dual-manifest-without-marketplace repos all throw with :name, while the no-suffix controls install OK. This is the same defect class as the sourceless-entry break fixed this round, for the absent-marketplace variant; no test covers a :name install of a non-marketplace repo.
| if ( | |
| isExplicitMarketplaceEntry && | |
| pluginName && | |
| !selectedMarketplaceEntryUsesRoot | |
| ) { | |
| if ( | |
| isExplicitMarketplaceEntry && | |
| pluginName && | |
| marketplaceLocation !== 'missing-marketplace' && | |
| !selectedMarketplaceEntryUsesRoot | |
| ) { |
— qwen3.8-max via Qwen Code /review (v0.21.7)
| installMetadata.pluginSourceKind, | ||
| ); | ||
| extensionDir = converted.extensionDir; | ||
| if (extensionDir !== tempDir) { |
There was a problem hiding this comment.
[Critical] The update check forwards pluginSourceKind in both archive branches, but the non-archive local-directory path in this same function performs no conversion at all — so for a marketplace-entry install it version-compares the repo root's manifest against the installed subplugin — Failure scenario: a Qwen-rooted dual-format repo (root qwen-extension.json v1.0.0 + marketplace entry foo at ./plugins/foo v2.0.0) installed via dir:foo resolves the subplugin (v2.0.0) through this PR's explicit-entry branch, but qwen extensions update foo hits the directory path and calls loadExtensionConfig on the raw repo root (v1.0.0). Probe-verified at this commit: a just-installed extension from an unchanged source immediately reports UPDATE_AVAILABLE, and running the update re-resolves the same subplugin — a permanent no-op update loop; with equal versions, a real version bump inside plugins/foo is masked as UP_TO_DATE because only the root artifact is ever read. Pre-PR the install resolved the root too, so the check was self-consistent. Gemini-rooted and pure-Claude directory repos don't surface this (no root qwen-extension.json → NOT_UPDATABLE, same as pre-PR). Suggested fix: in the non-archive case of type === 'local', run the same convertGeminiOrClaudeExtension(extensionDir, installMetadata.pluginName, installMetadata.networkPolicy, signal, installMetadata.pluginSourceKind) before loadExtensionConfig, reusing the existing convertedDir cleanup — symmetric with the two archive branches patched here.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if ( | ||
| (originSource === 'Claude' && fs.existsSync(hooksDir)) || | ||
| (originSource === 'Claude' && | ||
| ((originSource === 'Claude' || requiresClaudeFileAdaptation) && | ||
| fs.existsSync(hooksDir)) || |
There was a problem hiding this comment.
[Suggestion] Under-adaptation: the widened gate still keys on hooks artifacts on disk, so dual-manifest extensions whose merged Claude hooks are an inline object (or a file outside hooks/) run their hooks but never get file adaptation — Failure scenario: a dual-manifest extension whose .claude-plugin/plugin.json declares hooks inline and ships a script parsing ~/.claude/transcript: requiresClaudeFileAdaptation is true and the hooks execute after install, but staging has no hooks/ dir and the merged config.hooks is an object (configHooksPath null), so the gate is false — probe-verified: the installed script kept jq '.message.content ...' ~/.claude/transcript verbatim and would parse a nonexistent transcript at runtime; prepending requiresClaudeFileAdaptation || to the gate made it come out adapted (~/.qwen/transcript, .message.parts). Note this interacts with the sibling comment below (over-adaptation): consider opening the gate on the flag alone but scoping the replacement (see there).
| if ( | |
| (originSource === 'Claude' && fs.existsSync(hooksDir)) || | |
| (originSource === 'Claude' && | |
| ((originSource === 'Claude' || requiresClaudeFileAdaptation) && | |
| fs.existsSync(hooksDir)) || | |
| if ( | |
| requiresClaudeFileAdaptation || | |
| ((originSource === 'Claude' || requiresClaudeFileAdaptation) && | |
| fs.existsSync(hooksDir)) || |
— qwen3.8-max via Qwen Code /review (v0.21.7)
| fs.existsSync(hooksDir)) || | ||
| ((originSource === 'Claude' || requiresClaudeFileAdaptation) && | ||
| configHooksPath && |
There was a problem hiding this comment.
[Suggestion] Over-adaptation (opposite pole of the comment above): for Gemini-origin dual-manifest installs, requiresClaudeFileAdaptation opens this gate to performVariableReplacement(stagingPath, destinationPath), whose **/*.md / **/*.sh globs rewrite Claude idioms across EVERY file of the extension — including Gemini-side files unrelated to the merged Claude hooks. Pre-PR, Gemini-origin installs never ran this rewriting — Failure scenario: probe-verified at this commit — a Gemini-side GEMINI_NOTES.md and scripts/gemini-helper.sh reading ~/.claude/settings.json were rewritten to ~/.qwen/settings.json during install of a dual-manifest fixture, so a Gemini helper that legitimately operates on Claude files (plausible exactly in repos shipping Claude compatibility) breaks at runtime; reverting the gate to the pre-PR originSource === 'Claude' condition kept all files untouched. Suggested fix: when originSource !== 'Claude', restrict the replacement to the files the merged hook commands reference (or at minimum the hooks/ tree plus those scripts) instead of globbing the entire staging directory.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| mergedConfig.hooks = preserveHookVariables | ||
| ? hooksData | ||
| : substituteHookVariables(hooksData, pluginSource); |
There was a problem hiding this comment.
[Suggestion] The default (preserveHookVariables = false) path substitutes the staging temp dir into hook commands, and this diff's new finally deletes that dir before the function returns — the converted qwen-extension.json ships hook commands pointing at a directory that no longer exists — Failure scenario: probe-verified at this commit: convertClaudePluginPackage(root, 'p') on a source: './sub' fixture emitted command: /tmp/qwen-extensionXXXX/scripts/start.sh while that directory was already deleted; the next caller using the default gets hooks that fail with a bare not-found error and no trail back to the converter. All five in-repo production call sites pass true, but the converters are exported from the core package index (core/src/index.ts → extension/index.ts), so the trap default sits on the public API surface — the diff changed the default's meaning from "path with the clone's lifetime" to "path dead on return". Suggested fix: drop the default (preserveHookVariables: boolean, forcing callers to choose), or stop substituting at conversion entirely — load-time hydration resolves ${CLAUDE_PLUGIN_ROOT} for every production path.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| ).toBe( | ||
| expectedHookPath | ||
| ? '${CLAUDE_PLUGIN_ROOT}/' + expectedHookPath | ||
| : undefined, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The matrix inspects only SessionStart[0].hooks[0] here and in the installed-config assertion below, and never asserts the number of hook definitions — so the "imports only selected root Claude hooks" exclusivity in this test's own title is not pinned — Failure scenario: if mergeClaudeConfigs/buildQwenExtensionFromPlugin regressed from replacing plugin.json hooks with marketplace-entry hooks to concatenating both, the selected definition stays at index 0 in the exclusivity cases (named root, explicit matching marketplace root, explicit matching extension root), so every assertion in this file stays green while both SessionStart hooks fire on every session start. The same file already demonstrates the stronger pattern in merges conventional Gemini hooks with Claude hooks by event:
expect(
config.hooks?.['SessionStart']?.map(
(d) => (d.hooks?.[0] as { command?: string })?.command,
),
).toEqual(
expectedHookPath
? ['${CLAUDE_PLUGIN_ROOT}/' + expectedHookPath]
: undefined,
);(mirrored on the installedConfig assertion).
— qwen3.8-max via Qwen Code /review (v0.21.7)
| expect(fs.existsSync(extensionDir)).toBe(false); | ||
| const installedCommand = ( |
There was a problem hiding this comment.
[Suggestion] Dead assertion: this checks that the source fixture dir is gone, but the test itself deleted it with fs.rmSync(extensionDir, ...) at line 286 — it can never fail and verifies nothing about the code under test — Failure scenario: placed right after manager.loadExtensionConfig(...), it reads as if it verifies an install-flow property, yet any behavior change in the converter or load path leaves it green. The afterEach at line 32 already removes extensionDir, so if a maintainer later deletes the now-redundant manual rmSync, this assertion flips red with no behavior change — mimicking a regression and sending debugging effort after a test artifact. Suggested fix: delete the assertion (or drop the manual rmSync and assert the source dir survives conversion+install, if non-destruction is the intent).
| expect(fs.existsSync(extensionDir)).toBe(false); | |
| const installedCommand = ( | |
| const installedCommand = ( |
— qwen3.8-max via Qwen Code /review (v0.21.7)
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31243205337)._ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: reverse audit — stopped before round 2 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if ( | ||
| !fs.existsSync(marketplacePath) || | ||
| !realPathWithin(marketplacePath, extensionDir) | ||
| ) { | ||
| return 'other'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-2: The new symlink-confinement guards (selectedMarketplacePluginLocation for .claude-plugin/marketplace.json, and loadConventionalHooks for hooks/hooks.json) have no test, while the equivalent plugin.json/marketplace.json guards in claude-converter do (e.g. claude-converter.test.ts:995) — Failure scenario: an untrusted extension archive ships .claude-plugin/marketplace.json as a symlink to a host file; if a refactor drops realPathWithin here, the symlink is followed — host-file content decides whether the selected plugin is treated as the marketplace root (changing the install route), and the extension can probe whether arbitrary host paths parse as marketplace-shaped JSON. Probe-verified at this commit: with the guard removed, an escaping symlink resolves to 'root' and silently reroutes the install, while all existing tests stay green. Suggested fix: add a case where .claude-plugin/marketplace.json is a symlink escaping the package and assert the install still resolves via the standalone/root-plugin path, mirroring claude-converter.test.ts:995.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| it('keeps a valid Gemini conversion when Claude metadata is malformed', async () => { | ||
| fs.writeFileSync( | ||
| path.join(extensionDir, 'gemini-extension.json'), |
There was a problem hiding this comment.
[Suggestion] R3-3: The malformed-Claude-metadata test exercises the dual-merge swallow path (inner catch → Gemini fallback) but asserts nothing about temp-dir hygiene — Failure scenario: on this path claudeConversion stays undefined and the outer finally is a no-op, so the no-orphan invariant rests on the callee throwing before ExtensionStorage.createTmpDir() or on its internal rmSync catch. Probe-verified at this commit: simulating the internal-cleanup removal leaks a temp dir while all 18 tests stay green; a refactor that moves tmp-dir allocation ahead of manifest validation leaks a temp directory on every install of a valid-Gemini/malformed-Claude extension. Suggested fix: spy on ExtensionStorage.createTmpDir as the two cleanup tests do and assert every created temp dir no longer exists (or exactly one — the surviving Gemini conversion — remains) after convertGeminiOrClaudeExtension returns.
— qwen3.8-max via Qwen Code /review (v0.21.7)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI at this commit and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| return { | ||
| installSource: src.includes(':') ? src : `${src}:${plugin.name}`, | ||
| pluginSourceKind: 'extension-root', | ||
| }; |
There was a problem hiding this comment.
[Critical] resolveInstallSource stamps pluginSourceKind: 'extension-root' on every http-marketplace string source — including owner/repo:pluginName selector forms (the documented Claude marketplace form for a plugin inside another repo) — so convertGeminiOrClaudeExtension skips the subplugin-selection branches and a marketplace-shaped target repo falls through every branch — Failure scenario: an http marketplace entry with source: 'someone/repo:pdf' (or source: {type:'github', repo:...}): the TUI Discover tab forwards the kind, isExplicitExtensionRoot blocks both subplugin-selection branches, and a pure marketplace root (no plugin.json/qwen-extension.json) returns the raw clone — loadExtensionConfig throws Configuration file not found, the install aborts; if the root carries a manifest, the root extension is silently installed instead of the selected plugin. Execution-verified against the worktree: kind extension-root returns the raw root (install fails), while the same input without the kind converts the entry pdf and installs successfully — the CLI/daemon path (which defaults to marketplace-entry) works, so the Discover path is internally inconsistent.
| return { | |
| installSource: src.includes(':') ? src : `${src}:${plugin.name}`, | |
| pluginSourceKind: 'extension-root', | |
| }; | |
| return { | |
| installSource: src.includes(':') ? src : `${src}:${plugin.name}`, | |
| pluginSourceKind: src.includes(':') ? 'marketplace-entry' : 'extension-root', | |
| }; |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| installKind: 'explicit matching extension root', | ||
| pluginName: 'ponytail', | ||
| marketplacePluginName: 'ponytail', | ||
| marketplaceSource: './', |
There was a problem hiding this comment.
[Suggestion] The dual-manifest matrix never combines pluginSourceKind: 'extension-root' with a matching marketplace entry at a non-root source — the one configuration where the extension-root kind's semantics actually diverge from marketplace-entry — so the new kind dispatch is unpinned at the converter layer — Failure scenario: both existing extension-root cases route around the subplugin-precedence branch for accidental reasons (case 4's entry name doesn't match; case 9's source is the root), so a refactor that lets extension-root enter the subplugin-precedence branch (e.g. widening the guard to isExplicitMarketplaceEntry || isExplicitExtensionRoot) silently installs the subplugin instead of the root for that configuration. Probe-verified: the missing case added to the matrix is green on the PR code and flips red under that mutation. The new github.test.ts archive-update test pins the same precedence at the update level; this matrix case closes the converter-level hole.
| installKind: 'explicit matching extension root', | |
| pluginName: 'ponytail', | |
| marketplacePluginName: 'ponytail', | |
| marketplaceSource: './', | |
| installKind: 'explicit matching non-root entry', | |
| pluginName: 'ponytail', | |
| marketplacePluginName: 'ponytail', | |
| marketplaceSource: './plugins/ponytail', | |
| pluginSourceKind: 'extension-root' as const, |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const sourceBeforeConversion = extensionDir; | ||
| const converted = await convertGeminiOrClaudeExtension( | ||
| sourceBeforeConversion, |
There was a problem hiding this comment.
[Suggestion] The checkForExtensionUpdate restructure moves convertGeminiOrClaudeExtension out of the isSupportedArchivePath guard, so non-archive local directory sources — which pre-change were loaded directly with no conversion — now run a full conversion on every update check — Failure scenario: an extension installed from a local directory whose root is a Gemini extension or Claude plugin (both newly installable via this PR): every update check (extensions-UI mount, the check-updates route, the update command) recursively copies the live source tree (including .git for the Gemini path) into a temp dir, converts, reads the version, then deletes the copy — probe-verified for both roots. For a dev directory with build artifacts or a large .git, each check performs a full-tree copy + write + delete; and a malformed Gemini manifest that previously yielded the benign NOT_UPDATABLE now throws mid-conversion. The side effect is untested: the new regression test covers the archive branch only.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| installKind: 'named root', | ||
| pluginName: 'ponytail', | ||
| marketplacePluginName: 'ponytail', | ||
| marketplaceSource: './', |
There was a problem hiding this comment.
[Suggestion] The legacy subplugin-selection branch (else if (pluginName && !isExplicitExtensionRoot) { convertClaudePluginPackage(...) }) is never exercised by any test — probe-verified: disabling the branch leaves 220/220 tests green across the three files that call convertGeminiOrClaudeExtension — Failure scenario: both github.ts update paths pass installMetadata.pluginName with pluginSourceKind: undefined for pre-PR owner/repo:name installs of pure-Claude marketplace repos (.claude-plugin/marketplace.json with subplugins, no root manifest), so a future refactor that deletes or re-gates the branch would silently change what an update of such a legacy install produces — the converter returns the unconverted marketplace root and the update degrades to NOT_UPDATABLE, or compares/installs the repo root instead of the named subplugin — with the entire suite staying green. Behavior probe: with the branch, the subplugin (child manifest version 2.0.0) is converted; without it, reading the root's qwen-extension.json throws ENOENT.
| installKind: 'named root', | |
| pluginName: 'ponytail', | |
| marketplacePluginName: 'ponytail', | |
| marketplaceSource: './', | |
| installKind: 'legacy subplugin selection', | |
| pluginName: 'ponytail', | |
| pluginSourceKind: undefined, |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| newExtensionDir = geminiConversion.convertedDir; | ||
| originSource = 'Gemini'; | ||
| requiresClaudeFileAdaptation = Boolean(claudeHooks); |
There was a problem hiding this comment.
[Suggestion] requiresClaudeFileAdaptation = Boolean(claudeHooks) is false when the dual-manifest repo's Claude hooks use the conventional hooks/hooks.json layout (plugin.json without a hooks field — the standard Claude plugin shape), so the Claude hooks this branch imports ship un-adapted — Failure scenario: a dual-manifest repo (gemini-extension.json + plugin.json with no hooks field + hooks/hooks.json at root): convertClaudeToQwenConfig maps the missing/string hooks field to undefined, the hooks are imported as geminiHooks via loadConventionalHooks and inlined, and the flag is computed false — the adaptation gate's originSource === 'Claude' disjunct fails (it is 'Gemini') and configHooksPath is null, so performVariableReplacement never runs even though the conventional staging/hooks dir exists. The staged .md/.sh hook files ship raw (jq '.message.content ...', ~/.claude/transcript), while load-time substituteHookVariables only rewrites ${CLAUDE_PLUGIN_ROOT} in command strings — probe-verified: layout A (conventional) gets requiresClaudeFileAdaptation: false and a raw script; layout B (plugin.json object form) flips to true. The same repo with hooks declared in plugin.json is adapted correctly.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| signal, | ||
| true, | ||
| ) | ||
| : await convertClaudePluginStandalone(extensionDir, true); |
There was a problem hiding this comment.
[Suggestion] The dual-manifest branch forwards networkPolicy and signal to convertClaudePluginPackage but drops the signal on the convertClaudePluginStandalone arm — and convertClaudePluginStandalone/buildQwenExtensionFromPlugin take no signal parameter at all — so the branch's only abort checkpoints for that path are before the Claude conversion and after it fully completes — Failure scenario: a user cancels (abort) during install/update of a dual-manifest repo whose Claude side takes the standalone path (any gemini-extension.json + plugin.json repo without a root-pointing marketplace entry — the common dual-manifest shape): the abort surfaces only after the full recursive copy, agent-file rewrites, and config write complete, so on a large repo the cancel appears to hang and the conversion's I/O is wasted. The convertClaudePluginPackage arm aborts mid-flight — an inconsistency this branch introduces; the PR's own abort test fires at the second createTmpDir and passes only because the abort is noticed at the post-conversion checkpoint, encoding the same latency.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| } else if (isGeminiExtension && hasClaudePlugin) { | ||
| const geminiConversion = await convertGeminiExtensionPackage(extensionDir); |
There was a problem hiding this comment.
[Suggestion] Every dual-manifest conversion runs convertGeminiExtensionPackage (full recursive copy of the repo, .git kept) and then convertClaudePluginStandalone → buildQwenExtensionFromPlugin (a second full recursive copy of the same tree), and only the Claude side's config.hooks is consumed before its converted dir is deleted in the finally — Failure scenario: installing or updating any dual-manifest Gemini repo (the PR's headline feature) performs ~2× the tree copy IO plus a write-plus-delete, where pre-PR the same repo took a single Gemini copy. For a repo with a large .git or vendored assets, each install/update transiently writes the whole tree twice and deletes one copy; the second copy is pure waste because the standalone conversion's only consumed output is the in-memory config.hooks (the string-hooks-file case needs at most a file read, not a tree copy). Distinct from the tmp-capacity concern for remote sources; this applies to the local-source dual-manifest path introduced by this diff.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const sourceBeforeConversion = extensionDir; | ||
| const converted = await convertGeminiOrClaudeExtension( | ||
| sourceBeforeConversion, | ||
| installMetadata.pluginName, |
There was a problem hiding this comment.
[Suggestion] The checkForExtensionUpdate restructure moved conversion out of the isSupportedArchivePath guard, so update checks of local-directory marketplace installs whose selected entry has a remote source now perform a live network clone/download on every check — Failure scenario: qwen extension install ./local-marketplace-repo:selected where the entry's source is {source:'github', repo:...} / {source:'url', url:...} / git-subdir: at every checkForAllExtensionUpdates sweep (startup, extensionManager.ts:2599), the check runs convertClaudePluginPackage → resolvePluginSource → cloneFromGit/downloadFromGitHubRelease against the entry's remote, into OS tmp, for an extension whose install root is a local directory — probe-verified: 1 https.get + 1 git clone attempted during the check, degrading to NOT_UPDATABLE on network failure, while the pre-PR shape performed zero network and read the install directly. Pre-PR the same metadata hit no conversion. The startup sweep now makes network requests the user never initiated (stalling the allSettled pass on slow/unreachable remotes), and the check follows whatever repo the entry currently names, with no pin to what was installed.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| function removeConvertedDirectory(directory: string): void { | ||
| try { | ||
| fs.rmSync(directory, { recursive: true, force: true }); |
There was a problem hiding this comment.
[Suggestion] The new dual-manifest branch's finally deletes the standalone Claude conversion — a full recursive copy of the repo — synchronously on every successful conversion, and the outer catch synchronously deletes the Gemini conversion copy on every error/abort; removeConvertedDirectory introduces the only synchronous full-tree delete in the flow, where every sibling cleanup is await fs.promises.rm — Failure scenario: install (or any update check via github.ts, which runs the same conversion on every local-source update) of a dual-manifest repo with a large tree — vendored assets, or a big .git on the error path — blocks the Node event loop for the duration of the recursive rmSync (hundreds of ms to seconds on tens of thousands of files). Benchmark: a 30,000-file tree froze the loop ~3s (fs.rmSync) with zero timer ticks firing, vs fs.promises.rm completing responsive. During that window the TUI renders, other extension loads, and concurrent operations all stall — pure overhead on the happy path.
| function removeConvertedDirectory(directory: string): void { | |
| try { | |
| fs.rmSync(directory, { recursive: true, force: true }); | |
| async function removeConvertedDirectory(directory: string): Promise<void> { | |
| try { | |
| await fs.promises.rm(directory, { recursive: true, force: true }); | |
| } catch {} | |
| } |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| expect(unchanged).toBe(ExtensionUpdateState.UP_TO_DATE); | ||
| expect(outdated).toBe(ExtensionUpdateState.UPDATE_AVAILABLE); |
There was a problem hiding this comment.
[Suggestion] The new update-check test pins the marketplace-entry-beats-Qwen-root precedence only through a version asymmetry in its fixture (root qwen-extension.json = 1.0.0, subplugin plugin.json = 2.0.0); it never asserts the identity of the config the conversion returned, so a fixture edit that equalizes the versions silently unpins the branch-order guarantee — Failure scenario: probe-verified — with equalized versions, both branch orders yield a version-2.0.0 config, so latestConfig.version !== extension.version produces identical results whether the subplugin or the marketplace root wins; the name discriminator ('selected' vs 'marketplace-root') is never asserted. A future maintainer who bumps the fixture's root version (e.g. while reusing the fixture) makes both assertions pass regardless of branch order, and a converter refactor that moves hasQwenConfig above the subplugin-precedence branch ships green — silently installing the marketplace repo root instead of the user-selected entry for Qwen-rooted marketplace repos.
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the updated extension-converter tests is unverified. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally. Not reviewed: reverse audit — stopped before round 5 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| installMetadata.pluginSourceKind, | ||
| ); | ||
| const extensionDir = converted.extensionDir; | ||
| if (extensionDir !== tempDir) { |
There was a problem hiding this comment.
[Suggestion] The archive-url branch of checkForExtensionUpdate forwards pluginSourceKind in this diff, but no test exercises that call site with pluginName/pluginSourceKind set — the two new forwarding tests cover only the local branch, and all pre-existing archive-url tests omit both fields — Failure scenario: parseInstallSource('https://example.com/ext.zip:my-plugin') yields {type: 'archive-url', pluginName, pluginSourceKind: 'marketplace-entry'} via the new default in marketplace.ts; if this forwarding regresses, an archive-url-installed extension whose marketplace entry selects a subdirectory of a dual-manifest archive compares the root manifest's version instead of the selected plugin's during update checks → wrong UP_TO_DATE (missed update) or spurious UPDATE_AVAILABLE, while all 88 existing github.test.ts tests stay green. Probe+mutation verified at this commit: removing the argument flips a targeted probe while the existing suite stays green. (This is the residual tail of the earlier update-check coverage thread — the local branch is now covered.) Suggested fix: mirror forwards extension-root kind when checking a local archive update with type: 'archive-url' metadata (reuse the existing mockHttpsResponses/createZipBuffer pattern) and pluginName + pluginSourceKind set, asserting the selected plugin's version drives the result.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // Claude marketplaces allow an entry without `source`; that entry refers | ||
| // to the marketplace root itself. | ||
| if (source === undefined || source === null) return 'root'; |
There was a problem hiding this comment.
[Suggestion] The explicit-JSON-null disjunct is untested here and at the matching site in claude-converter.ts resolvePluginSource (~line 1045); every sourceless fixture is built with marketplaceSource: undefined, which JSON.stringify drops, so the tests only ever exercise a missing key — Failure scenario: a hand-authored marketplace.json containing "source": null (valid JSON) resolves to the marketplace root today; if either check is later simplified to source === undefined, selectedMarketplacePluginLocation returns 'other' for that entry, so an explicit marketplace-entry install of a dual-manifest root plugin takes the convertClaudePluginPackage path and fails or imports the wrong plugin — with all current tests green. Suggested fix: add a source: null variant to the sourceless marketplace root it.each case in extension-converter.test.ts (and optionally to claude-converter's treats a marketplace entry without source as the marketplace root).
— qwen3.8-max via Qwen Code /review (v0.21.7)
| expect(fs.readdirSync(extensionDir).sort()).toEqual(sourceEntriesBefore); | ||
| convertedDir = converted.extensionDir; |
There was a problem hiding this comment.
[Suggestion] This 'source directory is left unmodified' guard compares only top-level entry names (fs.readdirSync is non-recursive), so nested in-place mutation of the source tree passes it silently (same pattern at the subdirectory-selection test below) — Failure scenario: the converter's TOML→MD step deletes commands/*.toml and writes commands/*.md; if a future refactor ran that conversion (or any hooks/manifest rewrite) against the source dir instead of the tmp copy — for a type: 'local' install extensionDir is the user's real folder — both readdir assertions would still pass ('commands' keeps its name) while the user's tree is mutated. These two assertions are the only source-integrity guards the PR adds. Suggested fix: compare a recursive snapshot instead — e.g. collect all relative paths with fs.globSync('**/*', { cwd: extensionDir }) before/after (or hash file contents) and assert equality.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| convertedDir = converted.extensionDir; | ||
| expect(fs.readdirSync(extensionDir).sort()).toEqual(sourceEntriesBefore); |
There was a problem hiding this comment.
[Suggestion] Same shallow guard as the it.each case above: fs.readdirSync compares top-level entry names only, so nested in-place mutation of the source tree passes silently — Failure scenario: the converter's TOML→MD step deletes commands/*.toml and writes commands/*.md; if a future refactor ran that conversion against the source dir instead of the tmp copy — for a type: 'local' install extensionDir is the user's real folder — this assertion would still pass while the user's tree is mutated. Suggested fix: compare a recursive snapshot (e.g. fs.globSync('**/*', { cwd: extensionDir }) or file hashes) before/after instead of top-level names.
— qwen3.8-max via Qwen Code /review (v0.21.7)
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/8626/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
# Conflicts: # packages/core/src/extension/claude-converter.ts # packages/core/src/extension/extension-converter.ts # packages/core/src/extension/extensionManager.ts # packages/core/src/extension/github.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the updated extension tests is unverified.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally.
Not explored to full depth (tool budget reached): This PR fixes dual-manifest extension handling in Qwen Co...: did not trace the hook runner's child-process env (whether CLAUDE_PLUGIN_ROOT is exported for scripts that reference the variable internally rather than in th…; This PR fixes dual-manifest extension handling in Qwen Co...: did not trace consent-UI rendering of hook commands, which show the transient converted-dir path after loadExtensionConfig hydration (pre-existing mechanism, ….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| function mergeHooks( | ||
| ...sources: Array<ExtensionHooks | undefined> | ||
| ): ExtensionHooks | undefined { | ||
| const merged: ExtensionHooks = {}; |
There was a problem hiding this comment.
[Critical] mergeHooks crashes with a TypeError when a hooks object uses an Object.prototype property name (constructor, toString, __proto__, ...) as an event key: merged is a plain object, so merged[event] ?? [] reads the inherited prototype member instead of defaulting to [], and existing.map is undefined. — Failure scenario: install a dual-manifest extension whose hooks JSON — either a conventional hooks/hooks.json picked up by loadConventionalHooks or the Claude plugin's inline/file hooks — contains { "hooks": { "constructor": [] } } → TypeError: existing.map is not a function → the outer catch deletes the Gemini conversion and rethrows → the whole install fails for a repo that pre-PR installed fine as a plain Gemini extension. Probe-confirmed for constructor/toString/__proto__/hasOwnProperty through both hook sources; such keys are legal JSON and can appear in third-party hook files. Note the write side too: merged['__proto__'] = ... on a plain object sets the prototype rather than an own key, so a null-prototype object fixes both ends (Object.keys still works on it).
| const merged: ExtensionHooks = {}; | |
| const merged = Object.create(null) as ExtensionHooks; |
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (fallbackPluginName) { | ||
| return { | ||
| installSource: `${source}:${fallbackPluginName}`, |
There was a problem hiding this comment.
[Critical] The newly appended :<pluginName> alias on scheme-prefixed sources (git@, sso://) does not round-trip through parseSourceAndPluginName when the plugin name starts with a digit: the parser's port heuristic (!/\d+/ prefix test) rejects digit-leading plugin names, so the whole aliased string becomes the repo and the installer attempts to clone an invalid git URL. — Failure scenario: a remote (http) marketplace entry { name: '2048-game', source: 'git@github.com:someone/repo.git' } → resolveInstallSource appends the alias → 'git@github.com:someone/repo.git:2048-game' (kind extension-root) → at install time parseInstallSource re-parses, the port guard rejects '2048-game', no pluginName is extracted and repo is the whole mangled string → isGitUrl still matches the git@ prefix → clone of an invalid scp-style URL → install fails. Pre-PR the same entry installed fine (colon-bearing sources were returned unchanged — see the removed line in this diff). Probe-confirmed for git@ and sso:// sources; letter-leading controls round-trip cleanly, which is why the new git-root/sso-root tests pass. The same defect class triggers for names containing / or :. — Suggested fix (in parseSourceAndPluginName, marketplace.ts — the fix spans files, so no suggestion block here): tighten the port heuristic to reject only fully-numeric segments, !/\d+$/.test(potentialPluginName) instead of !/\d+/; probe-verified that this restores the round trip while a genuine port (:8443) stays rejected.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const mergedConfig = { | ||
| ...geminiConversion.config, | ||
| hooks: mergeHooks(geminiHooks, claudeHooks), | ||
| }; |
There was a problem hiding this comment.
[Suggestion] The dual-manifest merge spreads only geminiConversion.config, so the marketplace entry's version is discarded when the selected entry points at the repository root — Concrete cost: for entry { name: 'ponytail', version: '2.1.0', source: './' } over a dual-manifest repo whose gemini-extension.json says 1.0.0, probe observation: installed config name=ponytail version=1.0.0 while Discover shows 2.1.0 (pluginsFromConfig renders plugin.version). checkForExtensionUpdate re-converts with the same kind and compares Gemini versions on both sides, so entry-version-only bumps (the canonical Claude marketplace update pattern) are permanently UP_TO_DATE for local/archive-url installs; Claude-only root entries do not have this problem (entry version wins in mergeClaudeConfigs), so version semantics diverge based on whether a sibling Gemini manifest exists. Git-type installs compare hashes and are unaffected. — Suggested fix: when a root marketplace entry selected the merge (rootMarketplacePluginName is set), overlay the entry's version onto the merged config, mirroring the entry-overrides-plugin.json precedence already applied for Claude-only entries.
— qwen3.8-max via Qwen Code /review (v0.21.8)
# Conflicts: # packages/core/src/extension/extension-converter.test.ts # packages/core/src/extension/extension-converter.ts # packages/core/src/extension/extensionManager.test.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the new extension conversion/source-registry path handling is unverified.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — I finished within budget. One deliberate scope boundary: the diff hunks after line 728 (rest of buildQwenExtensionFromPlugin , the convertClaudePluginS…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 4 more.
Test Plan (not a blocker): src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the new extension conversion/source-registry path handling is unverified。
未审查：build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — I finished within budget. One deliberate scope boundary: the diff hunks after line 728 (rest of buildQwenExtensionFromPlugin , the convertClaudePluginS…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 4 条。
Test Plan（非阻断）：src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const converted = await buildQwenExtensionFromPlugin( | ||
| extensionDir, | ||
| config as ClaudePluginConfig, | ||
| false, | ||
| signal, | ||
| ); |
There was a problem hiding this comment.
[Critical] R6-1: convertQoderPlugin passes preserveHookVariables=false, so buildQwenExtensionFromPlugin bakes ${CLAUDE_PLUGIN_ROOT} in ALL resolved hooks (inline objects included) to the ephemeral conversion tmp dir. The installer later copies the converted dir into place and deletes it; load-time hydration and performVariableReplacement only replace literal tokens — none remain — so installed hook commands permanently reference a deleted temp directory. — Failure scenario: probe-verified end-to-end — installing a Qoder plugin whose hooks use ${CLAUDE_PLUGIN_ROOT} produced installed command /tmp/qwen-extensionXXXX/scripts/hook.sh with the target dir already deleted → ENOENT at every hook event, for every install type (local/link installs worked pre-PR). Flipping this call site to true kept the variable and hydrated it to the installed dir at load (probe flipped).
| const converted = await buildQwenExtensionFromPlugin( | |
| extensionDir, | |
| config as ClaudePluginConfig, | |
| false, | |
| signal, | |
| ); | |
| const converted = await buildQwenExtensionFromPlugin( | |
| extensionDir, | |
| config as ClaudePluginConfig, | |
| true, | |
| signal, | |
| ); |
中文说明
convertQoderPlugin 传入 preserveHookVariables=false，导致 buildQwenExtensionFromPlugin 将所有已解析 hooks（包括内联对象）中的 ${CLAUDE_PLUGIN_ROOT} 提前替换为临时转换目录路径。安装器随后把转换目录复制到最终位置并将其删除；加载期的 hydration 和 performVariableReplacement 只替换字面量 token——此时已无 token 可替换——因此安装后的 hook 命令永久指向一个已被删除的临时目录。失败场景：端到端探针验证——安装使用 ${CLAUDE_PLUGIN_ROOT} 的 Qoder 插件后，安装产物中的命令为 /tmp/qwen-extensionXXXX/scripts/hook.sh，而该目录已被删除 → 每次 hook 触发都 ENOENT，影响所有安装类型（PR 之前 local/link 安装是可用的）。将该调用点改为 true 后变量得以保留，并在加载期 hydrate 到已安装目录（探针已验证翻转）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| installMetadata.pluginName, | ||
| installMetadata.networkPolicy, | ||
| signal, | ||
| installMetadata.pluginSourceKind, | ||
| ); |
There was a problem hiding this comment.
[Critical] R6-2: The new externalContent === true → NOT_UPDATABLE guard protects only the local branch of checkForExtensionUpdate. This archive-url branch — which this same PR threads pluginSourceKind into — has no equivalent guard, while the git/github-release branch already has one, so update sweeps implicitly refetch unpinned external content for archive-url marketplace-entry installs. — Failure scenario: probe-verified — install from an archive URL whose payload is a Claude marketplace, selecting an entry whose source is external (install records externalContent: true); on the next sweep this branch re-downloads and reaches resolvePluginSource, which attempted a real git clone of the unpinned nested repo — the exact implicit refetch the new local-branch comment forbids — then compares versions against drifting HEAD content (flapping UPDATE_AVAILABLE). Adding the same guard flips the probe to NOT_UPDATABLE with zero clone attempts.
Suggested fix (mirror the local-branch guard at the top of this branch, before any download/conversion):
if (installMetadata.externalContent === true) {
return ExtensionUpdateState.NOT_UPDATABLE;
}中文说明
新增的 externalContent === true → NOT_UPDATABLE 守卫只保护了 checkForExtensionUpdate 的 local 分支。当前这个 archive-url 分支——本 PR 同样把 pluginSourceKind 传入了这里——没有等价守卫，而 git/github-release 分支已有该守卫，因此更新扫描会对 archive-url 的 marketplace-entry 安装隐式重新拉取未固定的外部内容。失败场景：探针验证——从一个 payload 为 Claude marketplace 的 archive URL 安装、且所选 entry 的 source 为外部来源（安装记录 externalContent: true）；下一次更新扫描时该分支重新下载并进入 resolvePluginSource，实际尝试对未固定的嵌套仓库执行 git clone——正是新 local 分支注释所禁止的隐式重拉——随后与漂移中的 HEAD 内容比较版本（导致 UPDATE_AVAILABLE 反复跳变）。加上同样的守卫后探针翻转为 NOT_UPDATABLE 且零 clone 尝试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (src && src.source === 'github') { | ||
| return `${src.repo}:${plugin.name}`; | ||
| return classifyRemotePluginSource(src.repo, plugin.name); | ||
| } |
There was a problem hiding this comment.
[Critical] R6-20: The github/url branches pass src.repo / src.url to classifyRemotePluginSource without validating the field is a string. Marketplace JSON is untrusted external content parsed with bare JSON.parse (no shape validation at any of the four sites), so one malformed entry ({source:{source:'url'}} missing url, or {source:'github'} missing repo) throws a TypeError in parseSourceAndPluginName that propagates out of pluginsFromConfig into discoverPlugins' per-source catch, which returns [] — silently dropping the ENTIRE marketplace listing, including all valid sibling entries. Pre-PR the same inputs survived discovery. — Failure scenario: probe-confirmed at this commit: one malformed entry plus one valid sibling yields discoverPlugins → [] for both missing-url and missing-repo shapes (valid sibling dropped in both); adding typeof guards flips the count 0 → 2. The only log is debugLogger.error, gated behind QWEN_DEBUG_LOG_FILE — silent by default.
| if (src && src.source === 'github') { | |
| return `${src.repo}:${plugin.name}`; | |
| return classifyRemotePluginSource(src.repo, plugin.name); | |
| } | |
| if (src && src.source === 'github' && typeof src.repo === 'string') { | |
| return classifyRemotePluginSource(src.repo, plugin.name); | |
| } |
(apply the same typeof src.url === 'string' treatment to the url branch below, or wrap the per-plugin map body in pluginsFromConfig so one bad entry can't sink its siblings)
中文说明
github/url 分支未校验字段是否为字符串就把 src.repo / src.url 传给 classifyRemotePluginSource。Marketplace JSON 是不受信任的外部内容，四处解析点均为裸 JSON.parse（无任何形状校验），因此一个畸形 entry（{source:{source:'url'}} 缺少 url，或 {source:'github'} 缺少 repo）会在 parseSourceAndPluginName 中抛出 TypeError，并沿 pluginsFromConfig 传播到 discoverPlugins 的按源 catch，后者返回 [] —— 整个 marketplace 列表（包括所有有效兄弟 entry）被静默丢弃。PR 之前相同输入可以正常出现在 Discover 中。失败场景：已在本 commit 探针确认——一个畸形 entry 加一个有效兄弟 entry，在缺 url 与缺 repo 两种形状下 discoverPlugins 均返回 []（有效兄弟一并被丢弃）；加上 typeof 守卫后数量由 0 → 2。唯一日志是受 QWEN_DEBUG_LOG_FILE 门控的 debugLogger.error —— 默认完全静默。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * Copyright 2025 Google LLC | ||
| * SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
[Suggestion] R6-3: This diff reassigns the file's header from Copyright 2026 Qwen Team to Copyright 2025 Google LLC while expanding it from ~19 to 995+ lines of new Qwen-authored tests. Recently ADDED Qwen-authored extension files keep Qwen headers (extension-store.ts, archive-safety.ts, zip-extraction.ts, agent-plugins-v1/index.ts: Copyright 2026 Qwen Team; qoder-converter.ts/sourceRegistry.ts: Copyright 2025 Qwen); Copyright 2025 Google LLC is the gemini-cli-heritage marker. — Concrete cost: misattribution of ~975 newly authored lines; looks like an accidental copy of the neighboring claude-converter.test.ts header.
| * @license | |
| * Copyright 2026 Qwen Team | |
| * Copyright 2025 Google LLC | |
| * SPDX-License-Identifier: Apache-2.0 | |
| * @license | |
| * Copyright 2026 Qwen Team | |
| * SPDX-License-Identifier: Apache-2.0 |
中文说明
本 diff 在将该文件从约 19 行扩展到 995+ 行新编写的 Qwen 测试的同时，把文件头从 Copyright 2026 Qwen Team 改成了 Copyright 2025 Google LLC。近期新增的 Qwen 扩展文件均保留 Qwen 头（extension-store.ts、archive-safety.ts、zip-extraction.ts、agent-plugins-v1/index.ts：Copyright 2026 Qwen Team；qoder-converter.ts/sourceRegistry.ts：Copyright 2025 Qwen）；Copyright 2025 Google LLC 是 gemini-cli 承继文件的标记。具体代价：约 975 行新代码被错误署名；看起来是误抄了相邻 claude-converter.test.ts 的文件头。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| JSON.stringify({ | ||
| name: 'untrusted', | ||
| owner: { name: 'Untrusted' }, | ||
| plugins: [{ name: 'safe-root', source: './' }], |
There was a problem hiding this comment.
[Suggestion] R6-4: The escaping-marketplace fixture entry carries no hooks and no version, so this test only proves conversion doesn't throw — it cannot observe whether the manifest was consulted. — Failure scenario: both arms traced in code — if selectedMarketplacePlugin's realPathWithin containment check were dropped/weakened, the escaping entry resolves to location: 'root'; mergeClaudeConfigs overlays entry fields only when defined, and this entry defines none, so every assertion still passes unchanged while the escaping manifest can now supply hooks/version to the merged config — the regression this test is named to catch ships undetected.
Suggested fix: give the escaping entry observable fields — plugins: [{ name: 'safe-root', source: './', version: '9.9.9', hooks: './hooks/marketplace-hooks.json' }], create hooks/marketplace-hooks.json inside extensionDir with a distinct command, and additionally assert config.version remains '1.0.0'; under the regression the test would then fail.
中文说明
逃逸 marketplace 的 fixture entry 未携带 hooks 和 version，因此该测试只能证明转换不抛错——无法观测 manifest 是否真的被读取。失败场景：两种分支均已在代码中推演——如果 selectedMarketplacePlugin 的 realPathWithin 包含性检查被删除或弱化，逃逸 entry 会解析为 location: 'root'；mergeClaudeConfigs 仅在字段有定义时才覆盖，而该 entry 没有任何字段，于是所有断言依旧通过，逃逸的 manifest 却已能向合并配置提供 hooks/version —— 该测试本要捕获的回归将在无人察觉的情况下溜走。建议：给逃逸 entry 加上可观测字段——plugins: [{ name: 'safe-root', source: './', version: '9.9.9', hooks: './hooks/marketplace-hooks.json' }]，在 extensionDir 内创建带不同命令的 hooks/marketplace-hooks.json，并额外断言 config.version 仍为 '1.0.0'；这样回归发生时测试就会失败。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const pluginSourceKind = | ||
| options.pluginSourceKind ?? (pluginName ? 'marketplace-entry' : undefined); | ||
| if (pluginSourceKind) { | ||
| installMetadata.pluginSourceKind = pluginSourceKind; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R6-18: This defaults pluginSourceKind to 'marketplace-entry' whenever a pluginName parses, even when parsing proved no marketplace config exists (the new test blesses the stamp with all HTTPS fetches mocked to 404). Aliased installs of standalone plugin roots then route into the legacy pluginName branch → convertClaudePluginPackage → Marketplace configuration not found, while the same repo succeeds via the discovery extension-root path — contradicting the rule this same PR documents in classifyRemotePluginSource ("must not turn a plain repo/transport source into a marketplace selector"). — Failure scenario: probe-verified — a repo with only .claude-plugin/plugin.json, install alias myalias: kind 'marketplace-entry' THREW Marketplace configuration not found at …/.claude-plugin/marketplace.json; kind 'extension-root' SUCCEEDED (originSource Claude). Both CLI call sites pass no kind, so qwen extensions install git@github.com:owner/plugin.git:myalias fails. (The failure predates this PR — but the extension-root mechanism this PR introduced to fix root-alias installs is bypassed by its own default.)
Suggested fix: default the kind only when a marketplace was actually resolved — e.g. gate the default on the fetched marketplace config plus pluginName — so marketplace-less aliases keep legacy behavior and stay consistent with the discovery path.
中文说明
只要解析出 pluginName，这里就把 pluginSourceKind 默认为 'marketplace-entry'——即使解析过程已证明不存在 marketplace 配置（新增测试在所有 HTTPS 请求被 mock 为 404 的情况下仍认可该打标）。独立插件根的带别名安装随后进入遗留 pluginName 分支 → convertClaudePluginPackage → Marketplace configuration not found，而同一仓库经 Discover 的 extension-root 路径却能成功——与本 PR 自己在 classifyRemotePluginSource 中写明的规则（“不得把普通 repo/transport 来源变成 marketplace selector”）相矛盾。失败场景：探针验证——仅有 .claude-plugin/plugin.json 的仓库、安装别名 myalias：kind 为 'marketplace-entry' 时抛出 Marketplace configuration not found at …/.claude-plugin/marketplace.json；kind 为 'extension-root' 时成功（originSource Claude）。两个 CLI 调用点都不传 kind，因此 qwen extensions install git@github.com:owner/plugin.git:myalias 会失败。（该失败在本 PR 之前就存在——但本 PR 为修复根别名安装而引入的 extension-root 机制被它自己的默认值绕过了。）建议：仅在确实解析到 marketplace 时才默认该 kind——例如以获取到的 marketplace 配置加 pluginName 作为默认条件——使无 marketplace 的别名保持遗留行为，并与 Discover 路径一致。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| export function discoveredPluginInstallIdentity( | ||
| installSource: string, | ||
| pluginSourceKind?: ExtensionPluginSourceKind, |
There was a problem hiding this comment.
[Suggestion] R6-19: Local marketplaces added with a relative path never identity-match their installs: addSource persists the user's input verbatim (./mkt), the install flow resolves local sources to absolute paths before persisting metadata, and this function canonicalizes neither side. Its shorthand regex also misclassifies dot-paths — . satisfies [a-zA-Z0-9_.-]+, rewriting ./mkt to a bogus https://github.com/./mkt. — Failure scenario: probe-verified end-to-end — source added as ./mkt, entry plug installed → discovery identity {"source":"https://github.com/./mkt",…} vs installed {"source":"/abs/cwd/mkt",…} → installed: false, while the absolute-path control yields installed: true. The name fallback masks this only when entry name === manifest name; reinstalling errors with "already installed". The user-visible outcome equals pre-PR (name-only matching also missed) — hence Suggestion — but the fabricated GitHub-URL identity is worth fixing before any future consumer displays or acts on it.
Suggested fix: canonicalize filesystem sources before identity comparison (path.resolve non-URL sources on both the addSource/registry side and here), and exclude dot-leading segments from the owner/repo shorthand test.
中文说明
以相对路径添加的本地 marketplace 永远无法与其安装完成身份匹配：addSource 原样持久化用户输入（./mkt），安装流程在持久化元数据前会把本地来源解析为绝对路径，而本函数对两侧都不做规范化。其 shorthand 正则还会误判点路径——. 满足 [a-zA-Z0-9_.-]+，把 ./mkt 改写为伪造的 https://github.com/./mkt。失败场景：端到端探针验证——以 ./mkt 添加源并安装 entry plug → 发现侧身份 {"source":"https://github.com/./mkt",…} 对安装侧 {"source":"/abs/cwd/mkt",…} → installed: false，而绝对路径的对照组为 installed: true。名称回退只在 entry 名 === manifest 名时才能掩盖该问题；再次安装会报 “already installed”。用户可见结果与 PR 前相同（纯名称匹配同样会漏）——因此定为 Suggestion——但伪造的 GitHub URL 身份值得在任何未来消费者展示或据此操作之前修复。建议：在身份比较前规范化文件系统来源（在 addSource/注册侧与此处都对非 URL 来源做 path.resolve），并在 owner/repo shorthand 判断中排除点开头的段。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| potentialPluginName && | ||
| !potentialPluginName.includes('/') && | ||
| !/^\d+/.test(potentialPluginName) | ||
| !/^\d+$/.test(potentialPluginName) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R6-21: All-digit plugin names break the installed↔discovery identity round-trip for POST-PR installs: install succeeds (the non-URL parse branch has no digit check and persists pluginName: '2048'), but the installed-side reconstruction in ExtensionManager.discoverPlugins re-parses source:pluginName through the URL branch, where this /^\d+$/ heuristic swallows the all-digit name as a port — identities never match. Distinct from the R5-2 fix territory (classifyRemotePluginSource aliasing, where install itself breaks): here install succeeds, and a fix confined to classifyRemotePluginSource would not repair the round-trip. — Failure scenario: probe-verified through the real discoveredPluginInstallIdentity: marketplace owner/repo shorthand lists entry 2048 → installed-side identity {"source":"https://github.com/someone/mkt:2048","pluginSourceKind":"marketplace-entry"} vs discovery {"source":"https://github.com/someone/mkt","pluginName":"2048",…} → installed: false; the same entry named game-2048 yields installed: true. The entry keeps showing as installable whenever the name fallback misses; reinstalling errors or conflicts.
Suggested fix: don't round-trip the installed identity through the ambiguous string parser — build it from the metadata fields directly (e.g. a {source, pluginName, pluginSourceKind} form of discoveredPluginInstallIdentity called with metadata.source and metadata.pluginName separately).
中文说明
纯数字插件名会破坏 PR 后安装的“安装侧↔发现侧”身份往返：安装本身成功（非 URL 解析分支没有数字检查，持久化 pluginName: '2048'），但 ExtensionManager.discoverPlugins 中的安装侧重构会把 source:pluginName 重新经由 URL 分支解析，此处的 /^\d+$/ 启发式把纯数字名当作端口吞掉——两侧身份永远无法相等。这与 R5-2 修复的领域不同（classifyRemotePluginSource 别名追加问题中安装本身会失败）：这里安装成功，且只修 classifyRemotePluginSource 无法修复该往返。失败场景：已经由真实 discoveredPluginInstallIdentity 探针验证——owner/repo shorthand marketplace 列出 entry 2048 → 安装侧身份 {"source":"https://github.com/someone/mkt:2048","pluginSourceKind":"marketplace-entry"} 对发现侧 {"source":"https://github.com/someone/mkt","pluginName":"2048",…} → installed: false；同名 entry 改为 game-2048 则为 installed: true。只要名称回退不命中，该 entry 就会持续显示为可安装；再次安装会报错或冲突。建议：不要把安装侧身份经由有歧义的字符串解析器往返——直接用元数据字段构造（例如给 discoveredPluginInstallIdentity 一个 {source, pluginName, pluginSourceKind} 形式，分别传入 metadata.source 与 metadata.pluginName）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| ); | ||
| }); | ||
|
|
||
| it('detects a direct-root Agent Plugin even when it has an install alias', async () => { |
There was a problem hiding this comment.
[Suggestion] R6-22: The new detection gate's legacy arm — alias given with pluginSourceKind undefined — is never tested against a contending supported root manifest; the suite pins only the no-alias, 'extension-root', and 'marketplace-entry' arms. That arm is the cohort of every pre-PR aliased install (the kind is added by this PR; extensionManager.ts forwards it undefined for legacy metadata). — Failure scenario: probe-proven against the built PR — fixture with a supported-schema root plugin.json + marketplace.json listing legacy-alias → ./plugins/legacy-child: kindless convertCompatibleExtension(dir, 'legacy-alias') currently converts the sub-plugin (originSource Claude) — but a one-line extension of detection to kindless aliases flips it to originSource AgentPlugins returning the repo root, silently swapping installed content, with 55/55 tests still green.
Suggested fix: add a test here: supported plugin.json at root + .claude-plugin/marketplace.json whose entry points to a subdirectory; call convertCompatibleExtension(pluginRoot, 'legacy-alias') with no kind; assert originSource is 'Claude' (selector-first preserved), not 'AgentPlugins'.
中文说明
新检测门的遗留分支——给定别名但 pluginSourceKind 为 undefined——从未在与一个受支持根清单竞争的 fixture 上被测试；测试套件只固定了无别名、'extension-root' 与 'marketplace-entry' 三个分支。该分支正是所有 PR 前带别名安装的群体（kind 由本 PR 新增；extensionManager.ts 对遗留元数据传 undefined）。失败场景：已在 PR 构建产物上探针证明——supported schema 的根 plugin.json + 列出 legacy-alias → ./plugins/legacy-child 的 marketplace.json：无 kind 的 convertCompatibleExtension(dir, 'legacy-alias') 目前转换子插件（originSource Claude）——但把检测扩展到无 kind 别名只需一行改动，就会翻转为 originSource AgentPlugins 并返回仓库根，静默替换安装内容，而 55/55 测试仍然全绿。建议：在此新增测试——根目录放 supported plugin.json + entry 指向子目录的 .claude-plugin/marketplace.json；以无 kind 调用 convertCompatibleExtension(pluginRoot, 'legacy-alias')；断言 originSource 为 'Claude'（selector 优先得以保留），而非 'AgentPlugins'。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const conventionalHooks = loadConventionalHooks( | ||
| geminiConversion.convertedDir, | ||
| ); | ||
| const geminiHooks = geminiConversion.config.hooks ?? conventionalHooks; |
There was a problem hiding this comment.
[Suggestion] R6-26: geminiConversion.config.hooks is a dead operand: convertGeminiToQwenConfig returns exactly {name, version, mcpServers, contextFileName, settings} — GeminiExtensionConfig has no hooks field and the fields are picked explicitly, so even an untrusted gemini-extension.json carrying a hooks key cannot carry it through. The inline-Gemini-hooks side the ?? expresses can never participate; geminiHooks is structurally always conventionalHooks. — Concrete cost: a maintainer investigating "hooks declared inline in gemini-extension.json are missing from the merged install" is told by this line that Gemini inline hooks flow into mergeHooks — they never can; it also makes the inline arm of the conventional-vs-inline precedence unreachable (the earlier R3-round coverage observation assumed that arm exists).
| const geminiHooks = geminiConversion.config.hooks ?? conventionalHooks; | |
| const geminiHooks = conventionalHooks; |
(or, if Gemini-side inline hooks SHOULD merge, extend GeminiExtensionConfig/convertGeminiToQwenConfig to map them — author's call)
中文说明
geminiConversion.config.hooks 是一个死操作数：convertGeminiToQwenConfig 只返回 {name, version, mcpServers, contextFileName, settings} —— GeminiExtensionConfig 没有 hooks 字段，且字段是显式挑选的，因此即便不受信任的 gemini-extension.json 携带 hooks 键也传不进来。?? 所表达的“内联 Gemini hooks”一侧永远无法参与；geminiHooks 在结构上恒等于 conventionalHooks。具体代价：维护者排查“gemini-extension.json 中内联声明的 hooks 在合并安装中丢失”时，这一行会让他以为 Gemini 内联 hooks 会流入 mergeHooks —— 实际永远不会；它还使 conventional-vs-inline 优先级中内联一侧不可达（早先轮次关于覆盖率的观察假定该分支存在）。（或者，如果 Gemini 侧内联 hooks 确实应当合并，请扩展 GeminiExtensionConfig/convertGeminiToQwenConfig 来映射它们——由作者决定。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows path-separator behavior of the new extension conversion tests is unverified.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run.
Test Plan (not a blocker): src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows path-separator behavior of the new extension conversion tests is unverified。
未审查：build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run。
Test Plan（非阻断）：src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| potentialPluginName && | ||
| !potentialPluginName.includes('/') && | ||
| !/^\d+/.test(potentialPluginName) | ||
| !/^\d+$/.test(potentialPluginName) |
There was a problem hiding this comment.
[Critical] R7-1: All-digit plugin names break the appended install-alias round-trip for scheme-prefixed sources — classifyRemotePluginSource (sourceRegistry.ts) appends :<pluginName> as a direct-root alias, but this port heuristic rejects a fully-numeric suffix on every scheme, so the install-time re-parse cannot strip the alias. — Failure scenario: an http marketplace entry named 2048 with string source git@github.com:owner/repo.git (or any sso:// source) yields git@github.com:owner/repo.git:2048; this branch rejects 2048 as a port and returns the whole mangled string as repo with no pluginName; parseInstallSource then classifies it as a git URL and the clone of a nonexistent URL fails — the entry Discover shows as installable can never be installed. Regression introduced by this diff: pre-PR the colon-bearing source was returned bare and installed fine. Probe-verified end-to-end (producer → consumer → flip); the new tests cover digit-leading names (2048-game, 2048-sso) but not all-digit names. Suggested fix — scope the numeric rejection to the schemes that actually have ports; probe-verified this restores the round trip with all 64 source-grammar tests still green (:8443 still parses as a port, and the producer never appends aliases to http(s) sources):
| potentialPluginName && | |
| !potentialPluginName.includes('/') && | |
| !/^\d+/.test(potentialPluginName) | |
| !/^\d+$/.test(potentialPluginName) | |
| potentialPluginName && | |
| !potentialPluginName.includes('/') && | |
| !((scheme === 'http://' || scheme === 'https://') && /^\d+$/.test(potentialPluginName)) |
中文说明
[Critical] R7-1：全数字插件名会破坏 scheme 前缀来源上追加安装别名的往返解析——classifyRemotePluginSource（sourceRegistry.ts）会追加 :<pluginName> 作为直接根别名，但此处的端口启发式在所有 scheme 下都拒绝纯数字后缀，导致安装期的重新解析无法剥离该别名。失败场景：一个名为 2048 的 http marketplace entry，其字符串 source 为 git@github.com:owner/repo.git（或任意 sso:// 来源），会生成 git@github.com:owner/repo.git:2048；该分支将 2048 当作端口拒绝，把整个被破坏的字符串作为 repo 返回且没有 pluginName；parseInstallSource 随后将其归类为 git URL，clone 一个不存在的 URL 必然失败——Discover 中显示为可安装的 entry 实际上永远无法安装。这是本 PR 引入的回归：PR 之前含冒号的来源会原样返回且可正常安装。已端到端探针验证（生产者 → 消费者 → 翻转验证）；新增测试覆盖了数字开头的名称（2048-game、2048-sso），但未覆盖纯数字名称。建议修复——将纯数字拒绝限定在实际存在端口的 scheme 上；探针验证该修复可恢复往返解析，且全部 64 个来源语法测试仍然通过（:8443 仍按端口解析，且生产者从不对 http(s) 来源追加别名）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the new extension conversion tests is unverified.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": did not walk the extensionManager.test.ts hunk that begins at the very end of my chunk (only its import line of parseInstallSource falls inside it) beyond con…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget., and 11 more.
Test Plan (not a blocker): src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the new extension conversion tests is unverified。
未审查：build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：did not walk the extensionManager.test.ts hunk that begins at the very end of my chunk (only its import line of parseInstallSource falls inside it) beyond con…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.，另有 11 条。
Test Plan（非阻断）：src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const isDirectUrl = /^https?:\/\//i.test(src); | ||
| return classifyRemotePluginSource( | ||
| src, | ||
| isDirectUrl ? undefined : plugin.name, | ||
| ); |
There was a problem hiding this comment.
[Critical] R8-14: The local-path guard misses CWD-relative paths with no leading ./ or ~ — a remote marketplace entry whose source is foo/bar (or foo/../bar) passes this predicate, and this PR's new pluginSourceKind: 'extension-root' newly turns it into a working local-directory install. — Failure scenario: entry {"name":"x","source":"foo/bar"} from a remote http marketplace → installSource: 'foo/bar:x', kind 'extension-root' → parseInstallSource stats foo/bar against the CLI/daemon CWD; if a directory with a root manifest exists there it resolves to type:'local' and the installer copies that local directory in as an enabled extension (hooks included) — probe-verified end-to-end. The merge base threw Marketplace configuration not found for the same input, and this PR tightened the caught paths, so the delta for the missed bare-relative class is exactly this bypass of the guard's documented purpose. The daemon install routes reject non-git/npm types, so the live vector is the TUI Discover path. A predicate cannot distinguish owner/repo shorthand from a CWD-relative path (seg/seg is ambiguous by construction) — suggested fix: reject in the Discover install path any parseInstallSource result with type === 'local' whose parsed repo string was relative, so discovered entries from remote marketplaces never resolve via CWD-relative stat (this also covers the sibling {source:'github'} branch below once its guard is added).
中文说明
R8-14：本地路径守卫遗漏了不带 ./ 或 ~ 前缀的 CWD 相对路径——远程 marketplace 中 source 为 foo/bar（或 foo/../bar）的 entry 可以通过该谓词，而本 PR 新增的 pluginSourceKind: 'extension-root' 使其首次可以成功安装为本地目录。失败场景：来自远程 http marketplace 的 entry {"name":"x","source":"foo/bar"} → installSource: 'foo/bar:x'、kind 为 'extension-root' → parseInstallSource 以 CLI/daemon 的 CWD 为基准对 foo/bar 执行 stat；若该目录下存在根清单，则解析为 type:'local'，安装器会把该本地目录作为已启用扩展复制安装（包含 hooks）——已端到端探针验证。merge base 对相同输入抛出 Marketplace configuration not found，且本 PR 收紧了被守卫命中的路径，因此对漏掉的裸相对路径类别而言，差量恰恰是绕过了该守卫所声明的防护目的。daemon 安装路由拒绝非 git/npm 类型，故实际可达路径是 TUI Discover。谓词层面无法区分 owner/repo 简写与 CWD 相对路径（seg/seg 在构造上就是歧义的）——建议修复：在 Discover 安装路径中拒绝 parseInstallSource 结果为 type === 'local' 且解析出的 repo 字符串为相对路径的情况，使来自远程 marketplace 的发现项永远不会通过 CWD 相对 stat 解析（下方 {source:'github'} 分支补上守卫后同样适用）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (src && src.source === 'github' && typeof src.repo === 'string') { | ||
| return classifyRemotePluginSource(src.repo, plugin.name); | ||
| } |
There was a problem hiding this comment.
[Critical] R8-8: This structured { source: 'github' } branch has no local-path guard, so a remote marketplace can point the installer at an arbitrary local filesystem path — the exact invariant the sibling string and { source: 'url' } branches enforce with a documented threat model. — Failure scenario: entry {source:{source:'github', repo:'/home/user/secrets'}} → /home/user/secrets:x → parseInstallSource stats the path → type:'local', and the installer copies the victim's local directory in as an enabled extension (probe-verified, hooks included); the sibling branches reject the identical input. The missing guard predates this PR, but the new extension-root kind makes the install succeed where the base threw Marketplace configuration not found (probe-verified flip).
| if (src && src.source === 'github' && typeof src.repo === 'string') { | |
| return classifyRemotePluginSource(src.repo, plugin.name); | |
| } | |
| if (src && src.source === 'github' && typeof src.repo === 'string') { | |
| if ( | |
| path.isAbsolute(src.repo) || | |
| src.repo.startsWith('.') || | |
| src.repo.startsWith('~') | |
| ) { | |
| debugLogger.warn( | |
| `Ignoring local path source "${src.repo}" from remote marketplace "${marketplace.source}".`, | |
| ); | |
| return { installSource: '', pluginSourceKind: 'extension-root' }; | |
| } | |
| return classifyRemotePluginSource(src.repo, plugin.name); | |
| } |
中文说明
R8-8：该结构化 { source: 'github' } 分支没有本地路径守卫，因此远程 marketplace 可以把安装器指向任意本地文件系统路径——而同级的字符串分支与 { source: 'url' } 分支正以明确的威胁模型注释强制实施这一不变量。失败场景：entry {source:{source:'github', repo:'/home/user/secrets'}} → /home/user/secrets:x → parseInstallSource 对该路径执行 stat → type:'local'，安装器会把受害者本地目录作为已启用扩展复制安装（已探针验证，包含 hooks）；同级分支会拒绝完全相同的输入。缺失的守卫在本 PR 之前已存在，但新的 extension-root kind 使该安装在 base 抛出 Marketplace configuration not found 的位置得以成功（探针已验证翻转）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const isGeminiExtension = | ||
| !hasQwenConfig && isGeminiExtensionConfig(extensionDir); |
There was a problem hiding this comment.
[Critical] R8-11: Hoisting isGeminiExtensionConfig(extensionDir) out of the lazy else-if chain makes its unguarded JSON.parse/readFileSync run on every conversion — including the Agent Plugins paths that previously never read gemini-extension.json. — Failure scenario: a repo whose root plugin.json carries the supported Agent Plugins $schema plus a malformed root gemini-extension.json (bad JSON / a directory / unreadable) installed fine at the merge base — the chain reached 'supported' before any gemini check; post-PR the eager probe throws a raw SyntaxError/EISDIR out of convertCompatibleExtension and the install fails (probe-verified; deferring the probe flips all four probe cases). An 'unsupported' agent repo with the same stray file now surfaces a JSON parse error instead of the intended Unsupported Agent Plugins schema. Supported schema: … message. Suggested fix: defer the probe into the branch that needs it (compute isGeminiExtension only after the agent-plugin and qwen-config branches), or make isGeminiExtensionConfig fail closed (return false on read/parse errors), the way selectedMarketplacePlugin guards its reads.
中文说明
R8-11：将 isGeminiExtensionConfig(extensionDir) 从惰性的 else-if 链中提升出来，导致其未加守卫的 JSON.parse/readFileSync 在每次转换时都执行——包括此前从不读取 gemini-extension.json 的 Agent Plugins 路径。失败场景：一个根 plugin.json 带有受支持 Agent Plugins $schema、同时根目录还有一个畸形 gemini-extension.json（非法 JSON / 是个目录 / 不可读）的仓库，在 merge base 可正常安装——链在到达任何 gemini 检查前就命中了 'supported'；本 PR 之后该提前执行的探测会抛出裸的 SyntaxError/EISDIR，导致安装失败（已探针验证；将探测延回分支后四个探针用例全部翻转）。带有同样杂散文件的 'unsupported' agent 仓库现在也会抛出 JSON 解析错误，而不是预期的 Unsupported Agent Plugins schema. Supported schema: … 提示。建议修复：把探测延回到需要它的分支内（在 agent-plugin 与 qwen-config 分支之后再计算 isGeminiExtension），或让 isGeminiExtensionConfig 失败即闭合（读取/解析出错时返回 false），与 selectedMarketplacePlugin 对自身读取的守卫方式一致。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| !selectedMarketplaceEntryUsesRoot | ||
| ) { | ||
| const converted = await convertClaudePluginPackage( |
There was a problem hiding this comment.
[Suggestion] R8-1: The explicit-marketplace-entry branch body (lines 253-266) is a verbatim copy of the legacy named-install branch body (lines 340-353): same convertClaudePluginPackage(..., true) call, same post-conversion Agent Plugins manifest deletion, same originSource/externalContent assignments (byte-compared: zero differences). — Concrete cost: any future change to Claude package post-processing must be applied in two places in the same function; updating only one silently diverges install behavior between explicit marketplace entries and legacy named installs. This PR itself demonstrates the coupling — the preserveHookVariables change had to update both call sites. Suggested fix: extract the shared body into a small local helper taking (extensionDir, pluginName, networkPolicy, signal) and returning { convertedDir, externalContent }, called from both branches.
中文说明
R8-1：显式 marketplace-entry 分支体（253-266 行）是旧式具名安装分支体（340-353 行）的逐字拷贝：相同的 convertClaudePluginPackage(..., true) 调用、相同的转换后 Agent Plugins 清单删除、相同的 originSource/externalContent 赋值（逐字节比对：零差异）。具体代价：未来对 Claude 包后处理的任何修改都必须在同一函数内改两处；只改一处会让显式 marketplace entry 与旧式具名安装的行为悄然分叉。本 PR 自身就证明了这种耦合——preserveHookVariables 修改必须同时更新两个调用点。建议修复：把共享分支体抽取为一个小的本地辅助函数，接收 (extensionDir, pluginName, networkPolicy, signal) 并返回 { convertedDir, externalContent }，由两个分支调用。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (typeof parsedHooks !== 'object' || parsedHooks === null) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R8-2: Hooks-document shape-validation gap — this file-parse branch accepts a top-level JSON array (typeof [] === 'object' passes, 'hooks' in [] is false, so the array itself is returned), and discards non-object bodies (null, scalars) without the Failed to parse hooks file warn the deleted code emitted for the same input. Two sibling branches carry the same defect class: the inline branch at lines 642-643 (returns any truthy non-string hooks unvalidated) and loadConventionalHooks in extension-converter.ts:144-148 (accepts top-level arrays). loadClaudePluginManifest, added in this same diff, rejects arrays — the inconsistency is inside this PR. — Failure scenario: array-typed hooks documents land as numeric-keyed structures no HookEventName can match, so hooks silently never fire — pure-Claude installs write "hooks": [...] into qwen-extension.json; on the new dual-manifest path mergeHooks silently drops every entry while requiresClaudeFileAdaptation flips on; a hooks.json body of null installs with zero hooks and no diagnostic anywhere. Probe-verified; the guard below flips each case (also consider restoring the warn breadcrumb for non-object documents, matching the parse-error catch).
| if (typeof parsedHooks !== 'object' || parsedHooks === null) { | |
| return undefined; | |
| } | |
| if ( | |
| typeof parsedHooks !== 'object' || | |
| parsedHooks === null || | |
| Array.isArray(parsedHooks) | |
| ) { | |
| return undefined; | |
| } |
中文说明
R8-2：hooks 文档形状校验缺口——该文件解析分支会接受顶层 JSON 数组（typeof [] === 'object' 通过、'hooks' in [] 为 false，于是数组本身被返回），并且对非对象主体（null、标量）直接静默丢弃，而删除的旧代码对相同输入会输出 Failed to parse hooks file 警告。同一缺陷类还存在于两个兄弟分支：642-643 行的内联分支（未校验地返回任意非字符串真值 hooks）以及 extension-converter.ts:144-148 的 loadConventionalHooks（接受顶层数组）。同一 diff 中新增的 loadClaudePluginManifest 会拒绝数组——该不一致就在本 PR 内部。失败场景：数组形状的 hooks 文档最终成为数字键结构，任何 HookEventName 都无法匹配，hooks 静默永不触发——纯 Claude 安装会把 "hooks": [...] 写入 qwen-extension.json；在新的双清单路径上 mergeHooks 静默丢弃所有条目，而 requiresClaudeFileAdaptation 却被置为 true；hooks.json 内容为 null 时安装结果没有任何 hooks 且无任何诊断。已探针验证；下面的守卫可翻转所有情形（另建议为非对象文档恢复警告输出，与 parse 错误的 catch 保持一致）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (!hooks) return undefined; | ||
| if (typeof hooks !== 'string') return hooks; |
There was a problem hiding this comment.
[Suggestion] R8-2 (sibling site): the inline branch returns any truthy non-string hooks value unvalidated — an inline "hooks": [...] array in plugin.json is passed through as ClaudeHooks. — Failure scenario: the array is written into qwen-extension.json (pure-Claude path) or silently dropped by mergeHooks while requiresClaudeFileAdaptation flips on (dual-manifest path); numeric keys never match a HookEventName, so every hook silently never fires, with no diagnostic. Probe-verified. Part of the same shape-validation gap as the comment below on the file-parse branch.
| if (!hooks) return undefined; | |
| if (typeof hooks !== 'string') return hooks; | |
| if (!hooks) return undefined; | |
| if (typeof hooks !== 'string') { | |
| if (typeof hooks !== 'object' || Array.isArray(hooks)) return undefined; | |
| return hooks; | |
| } |
中文说明
R8-2（兄弟位置）：内联分支未校验地返回任意非字符串真值 hooks——plugin.json 中的内联 "hooks": [...] 数组会被原样作为 ClaudeHooks 传出。失败场景：该数组或被写入 qwen-extension.json（纯 Claude 路径），或被 mergeHooks 静默丢弃而 requiresClaudeFileAdaptation 被置为 true（双清单路径）；数字键永远无法匹配 HookEventName，所有 hooks 静默永不触发且无诊断。已探针验证。与下方文件解析分支的评论属于同一形状校验缺口。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (typeof parsed !== 'object' || parsed === null) return undefined; | ||
| const hooks = (parsed as { hooks?: unknown }).hooks ?? parsed; |
There was a problem hiding this comment.
[Suggestion] R8-2 (sibling site): loadConventionalHooks accepts a top-level JSON array — typeof [] === 'object' passes and [].hooks is undefined, so the array itself is returned as ExtensionHooks. — Failure scenario: a dual-manifest extension whose hooks/hooks.json parses to a top-level array gets every entry skipped by mergeHooks (index keys, non-array values), so its conventional hooks are silently dropped from the merged qwen-extension.json with no warn (parse errors in this same function do warn), while requiresClaudeFileAdaptation still flips on for hooks never merged. Probe-verified. Suggested fix (also covers the inner {hooks: [...]} shape):
if (
typeof parsed !== 'object' ||
parsed === null ||
Array.isArray(parsed)
) {
return undefined;
}
const hooks = (parsed as { hooks?: unknown }).hooks ?? parsed;
return typeof hooks === 'object' && hooks !== null && !Array.isArray(hooks)
? (hooks as ExtensionHooks)
: undefined;中文说明
R8-2（兄弟位置）：loadConventionalHooks 接受顶层 JSON 数组——typeof [] === 'object' 通过且 [].hooks 为 undefined，于是数组本身被作为 ExtensionHooks 返回。失败场景：双清单扩展的 hooks/hooks.json 解析为顶层数组时，mergeHooks 会跳过所有条目（索引键、非数组值），其约定式 hooks 被静默丢弃、不会进入合并后的 qwen-extension.json，且无警告（同一函数内的 parse 错误却有警告），而 requiresClaudeFileAdaptation 仍会为这些从未合并的 hooks 置 true。已探针验证。建议修复（同时覆盖内层 {hooks: [...]} 形状）：见代码块。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if ( | ||
| potentialPluginName && | ||
| !potentialPluginName.includes('/') && | ||
| !/^\d+/.test(potentialPluginName) | ||
| !( | ||
| (scheme === 'http://' || scheme === 'https://') && | ||
| /^\d+$/.test(potentialPluginName) | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R8-3: Residual gap in the numeric-name fix — all-digit plugin names are still parsed as ports for http/https sources, so the install-alias round-trip breaks for the one scheme family this rewrite targets. — Failure scenario: a marketplace added via full https:// URL hosting a plugin named 2048 yields https://github.com/owner/market:2048 (and the structured-github branch with a full-URL repo does the same); re-parse treats the all-digit suffix as a port, returns the whole mangled string as repo with no pluginName, and the installer clones a nonexistent URL — probe-verified. Pre-existing (the removed lines built identical strings and the old rule rejected them identically) and this PR strictly narrows the broken class — hence Suggestion. Ports only appear in the authority, before any / in the host portion, so a tighter rule is safe: probe-verified that treating a fully-numeric suffix as a port only when the segment between the scheme and the colon contains no / restores the round trip while https://example.com:8443 still parses as a port and all 67 source-grammar tests stay green.
中文说明
R8-3：数字名修复的残留缺口——全数字插件名在 http/https 来源上仍会被解析为端口，使得本次重写所针对的这一 scheme 族的安装别名往返解析仍然失败。失败场景：以完整 https:// URL 添加的 marketplace 中名为 2048 的插件会生成 https://github.com/owner/market:2048（结构化 github 分支在 repo 为完整 URL 时同样如此）；重新解析时全数字后缀被视为端口，整个被破坏的字符串作为 repo 返回且没有 pluginName，安装器去 clone 一个不存在的 URL——已探针验证。该问题在本 PR 之前已存在（删除的行构造了相同字符串，旧规则以相同方式拒绝），且本 PR 严格收窄了受损类别——故为 Suggestion。端口只出现在 authority 中、host 部分的任何 / 之前，因此更收紧的规则是安全的：探针验证表明，仅当 scheme 与冒号之间的片段不含 / 时才把全数字后缀视为端口，即可恢复往返解析，同时 https://example.com:8443 仍按端口解析，全部 67 个来源语法测试保持通过。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the extension conversion/source-registry tests is unverified.
Not reviewed: build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run.
Not explored to full depth (tool budget reached): "Context: This PR fixes dual-manifest extension handling in…": none — all checks above completed within budget.; chunk 2: none — all checks I started completed within budget.; chunk 6: none — all planned checks completed within budget..
Test Plan (not a blocker): src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more.
[Critical] R8-5 (carried from the round-8 ledger, re-confirmed this round by probe + A/B against the merge base): all-digit plugin names appended as install aliases to https-normalized sources cannot round-trip — normalizeRemotePluginSource rewrites owner/repo shorthand and {source:'github'} repos to https://github.com/..., classifyRemotePluginSource (sourceRegistry.ts, alias-append branch) appends :<name>, and parseSourceAndPluginName's port exception keeps a fully-numeric suffix inside the repo URL for http/https schemes, so pluginName comes back undefined and the clone URL is corrupted (https://github.com/someone/numeric-root:2048). A remote http(s) marketplace entry such as { name: '2048', source: 'someone/numeric-root' } is shown as installable in Discover, but every install attempt fails at git resolution with an opaque error; the embedded-selector variant ('someone/repo:2048' plus an entry name) is likewise corrupted and misclassified as extension-root. A/B-proven regression: at the merge base the same shapes parsed to a clean repo + preserved alias. The inline comment could not be attached because an existing thread already occupies the anchor line (sourceRegistry.ts:207); that thread states the digit-leading variant, which is fixed — this fully-numeric-over-http(s) variant is the still-open residual (also tracked by the R8-3 thread at marketplace.ts:75). The PR's all-digit alias tests cover only git@/sso:// schemes. Suggested fix: do not append the alias when the (normalized) source is http(s) and the name is fully numeric; a complete fix also needs round-trip-safe handling for embedded numeric selectors, plus a round-trip test for this shape.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the extension conversion/source-registry tests is unverified。
未审查：build-and-test — 'Test (macos-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run。
未探索到全部深度（达到工具调用预算）："Context: This PR fixes dual-manifest extension handling in…"：none — all checks above completed within budget.；chunk 2：none — all checks I started completed within budget.；chunk 6：none — all planned checks completed within budget.。
Test Plan（非阻断）：src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more。
[Critical] R8-5 (carried from the round-8 ledger, re-confirmed this round by probe + A/B against the merge base): all-digit plugin names appended as install aliases to https-normalized sources cannot round-trip — normalizeRemotePluginSource rewrites owner/repo shorthand and {source:'github'} repos to https://github.com/..., classifyRemotePluginSource (sourceRegistry.ts, alias-append branch) appends :<name>, and parseSourceAndPluginName's port exception keeps a fully-numeric suffix inside the repo URL for http/https schemes, so pluginName comes back undefined and the clone URL is corrupted (https://github.com/someone/numeric-root:2048). A remote http(s) marketplace entry such as { name: '2048', source: 'someone/numeric-root' } is shown as installable in Discover, but every install attempt fails at git resolution with an opaque error; the embedded-selector variant ('someone/repo:2048' plus an entry name) is likewise corrupted and misclassified as extension-root. A/B-proven regression: at the merge base the same shapes parsed to a clean repo + preserved alias. The inline comment could not be attached because an existing thread already occupies the anchor line (sourceRegistry.ts:207); that thread states the digit-leading variant, which is fixed — this fully-numeric-over-http(s) variant is the still-open residual (also tracked by the R8-3 thread at marketplace.ts:75). The PR's all-digit alias tests cover only git@/sso:// schemes. Suggested fix: do not append the alias when the (normalized) source is http(s) and the name is fully numeric; a complete fix also needs round-trip-safe handling for embedded numeric selectors, plus a round-trip test for this shape.
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const usesPluginVariables = | ||
| originSource === 'Claude' || originSource === 'Qoder'; | ||
| originSource === 'Claude' || | ||
| originSource === 'Qoder' || | ||
| requiresClaudeFileAdaptation; |
There was a problem hiding this comment.
[Suggestion] R9-1: The new requiresClaudeFileAdaptation term cannot take effect when the imported Claude hooks don't correspond to a physical hooks/ directory or a string hooks path — the unchanged gate below still requires one of those before running performVariableReplacement. In the dual-manifest merge branch the merged hooks are always an object (mergeHooks result), so configHooksPath is null there; when the repo has no hooks/ directory — Claude hooks declared inline (object form) in plugin.json, or via a hooks file at a non-conventional path — adaptation is skipped even though the flag is true. This is a residual sub-case of the originSource-gate blocker this PR fixed via requiresClaudeFileAdaptation (comment 3728569319); the new adaptation test only exercises the conventional hooks/ layout. — Failure scenario: probe-verified at this commit — a dual-manifest extension with inline Claude hooks and no hooks/ directory installs hook scripts still parsing Claude's transcript shape (jq .message.content | map(select(.type == "text"))) and referencing ~/.claude/ paths; in Qwen the filter matches nothing (.message.parts transcript) and the paths don't exist, so the preserved hooks silently fail or produce empty output. Adding requiresClaudeFileAdaptation || to the disjunction flips the probe to fully adapted output (.message.parts, ~/.qwen). Suggested fix: run the adaptation when requiresClaudeFileAdaptation || (usesPluginVariables && (hooksDir exists || configHooksPath exists)).
中文说明
[Suggestion] R9-1：当导入的 Claude hooks 没有对应的实体 hooks/ 目录或字符串型 hooks 路径时，新增的 requiresClaudeFileAdaptation 项无法生效——下方未改动的门控仍要求二者之一存在才会执行 performVariableReplacement。在双清单合并分支中，合并后的 hooks 恒为对象（mergeHooks 的结果），因此 configHooksPath 为 null；当仓库没有 hooks/ 目录时——例如 Claude hooks 以对象形式内联声明在 plugin.json 中，或 hooks 文件位于非常规路径——即使该标志为 true，文件适配也会被跳过。这是本 PR 通过 requiresClaudeFileAdaptation 修复的 originSource 门控阻断问题（评论 3728569319）的残留子情形；新增的适配测试只覆盖了常规 hooks/ 布局。失败场景：已在本 commit 探针验证——一个内联声明 Claude hooks 且没有 hooks/ 目录的双清单扩展，安装后的 hook 脚本仍在解析 Claude 的 transcript 结构（jq .message.content | map(select(.type == "text"))）并引用 ~/.claude/ 路径；在 Qwen 中该过滤器匹配不到任何内容（transcript 为 .message.parts）且路径不存在，导致保留的 hooks 静默失败或输出为空。将 requiresClaudeFileAdaptation || 加入该析取条件后探针翻转为完全适配的输出（.message.parts、~/.qwen）。建议修复：在 requiresClaudeFileAdaptation || (usesPluginVariables && (hooksDir exists || configHooksPath exists)) 时执行适配。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| } else if ( | ||
| isExplicitMarketplaceEntry && | ||
| pluginName && | ||
| marketplaceSelection.location !== 'missing-marketplace' && | ||
| !selectedMarketplaceEntryUsesRoot | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R8-1: The explicit-marketplace-entry branch body below is still a verbatim copy of the legacy named-install branch body (~lines 340-353 in the same function): same convertClaudePluginPackage(..., true) call, same post-conversion Agent Plugins manifest deletion, same originSource/externalContent assignments. Re-checked at this commit — still stands. — Concrete cost: any future change to Claude package post-processing must be applied in two places in the same function; updating only one silently diverges install behavior between explicit marketplace entries and legacy named installs. This PR itself demonstrates the coupling — the preserveHookVariables change had to update both call sites. Suggested fix: extract the shared body into a small local helper taking (extensionDir, pluginName, networkPolicy, signal) and returning { convertedDir, externalContent }, called from both branches.
中文说明
[Suggestion] R8-1：下方显式 marketplace-entry 分支体仍是同函数内旧式具名安装分支体（约 340-353 行）的逐字拷贝：相同的 convertClaudePluginPackage(..., true) 调用、相同的转换后 Agent Plugins 清单删除、相同的 originSource/externalContent 赋值。已在本 commit 复查——仍然存在。具体代价：未来对 Claude 包后处理的任何修改都必须在同一函数内改两处；只改一处会让显式 marketplace entry 与旧式具名安装的行为悄然分叉。本 PR 自身就证明了这种耦合——preserveHookVariables 修改必须同时更新两个调用点。建议修复：把共享分支体抽取为一个小的本地辅助函数，接收 (extensionDir, pluginName, networkPolicy, signal) 并返回 { convertedDir, externalContent }，由两个分支调用。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const hooksData = | ||
| 'hooks' in parsedHooks | ||
| ? (parsedHooks as { hooks?: unknown }).hooks | ||
| : parsedHooks; |
There was a problem hiding this comment.
[Suggestion] R8-2: Hooks-document shape-validation gap — this file-parse branch still accepts a top-level JSON array (typeof [] === 'object' passes, 'hooks' in [] is false, so the array itself is returned) and discards non-object bodies (null, scalars) without the Failed to parse hooks file warn the deleted code emitted for the same input. loadClaudePluginManifest, added in this same diff, rejects arrays — the inconsistency is inside this PR. Re-checked at this commit — still stands (sibling sites: the inline branch at lines 642-643 and loadConventionalHooks in extension-converter.ts, each covered by its own thread). — Failure scenario: array-typed hooks documents land as numeric-keyed structures no HookEventName can match, so hooks silently never fire — pure-Claude installs write "hooks": [...] into qwen-extension.json; on the dual-manifest path mergeHooks silently drops every entry while requiresClaudeFileAdaptation flips on; a hooks-file body of null installs with zero hooks and no diagnostic anywhere. Suggested fix: reject arrays alongside non-objects — if (typeof parsedHooks !== 'object' || parsedHooks === null || Array.isArray(parsedHooks)) return undefined; (and consider restoring the warn breadcrumb for non-object documents).
中文说明
[Suggestion] R8-2：hooks 文档形状校验缺口——该文件解析分支仍会接受顶层 JSON 数组（typeof [] === 'object' 通过、'hooks' in [] 为 false，于是数组本身被返回），并且对非对象主体（null、标量）直接静默丢弃，而删除的旧代码对相同输入会输出 Failed to parse hooks file 警告。同一 diff 中新增的 loadClaudePluginManifest 会拒绝数组——该不一致就在本 PR 内部。已在本 commit 复查——仍然存在（兄弟位置：642-643 行的内联分支与 extension-converter.ts 中的 loadConventionalHooks，各有对应评论跟踪）。失败场景：数组形状的 hooks 文档最终成为数字键结构，任何 HookEventName 都无法匹配，hooks 静默永不触发——纯 Claude 安装会把 "hooks": [...] 写入 qwen-extension.json；在双清单路径上 mergeHooks 静默丢弃所有条目而 requiresClaudeFileAdaptation 却被置为 true；hooks 文件内容为 null 时安装结果没有任何 hooks 且无任何诊断。建议修复：在拒绝非对象的同时拒绝数组——if (typeof parsedHooks !== 'object' || parsedHooks === null || Array.isArray(parsedHooks)) return undefined;（另建议为非对象文档恢复警告输出）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the extension conversion/source-parsing changes is unverified.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run.
Not explored to full depth (tool budget reached): "agent 8b": none — all five invariant groups were walked; no checks left unfinished.; "agent reverse-audit (round 1)": none — but one check was concluded by inference rather than execution: I did not run an end-to-end install of the conventional-hooks-only fixture to observe the…; chunk 5: executed test run of packages/core/src/extension/extension-converter.test.ts not performed — the review worktree has no node_modules or built dist/, and npm ci ….
Test Plan (not a blocker): src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/core/src/extension/sourceRegistry.ts:207 — [review] appended install alias has no charset validation — ':'/'/' in entry names corrupts the round-trippackages/core/src/extension/claude-converter.test.ts:296 — [review] sourceless-entry test is vacuous — a wrong-directory resolution mutant keeps it greenpackages/core/src/extension/github.test.ts:1198 — [review] local externalContent update-guard test is vacuous — deleting the guard keeps it greenpackages/core/src/extension/sourceRegistry.ts:277 — [review] git-subdir marketplace entries fall through to the non-installable fallback in http marketplacesdocs/users/features/hooks.md:35 — [review] documented merge scope is inaccurate for legacy kindless installspackages/core/src/extension/extension-converter.test.ts:974 — [review] abort test does not exercise the dual-branch cleanup catch it namespackages/core/src/extension/sourceRegistry.ts:182 — [review] scoped-npm marketplace entry sources are misclassified as local pathspackages/core/src/extension/extensionManager.ts:2340 — [review] R9-1 still stands — requiresClaudeFileAdaptation cannot take effect for inline-object Claude hookspackages/core/src/extension/extension-converter.ts:254 — [review] R8-1 still stands — explicit-marketplace-entry branch body is a verbatim copy of the legacy branchpackages/core/src/extension/claude-converter.ts:658 — [review] R8-2 still stands — hooks-document shape-validation gap (top-level array accepted)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the required check 'Test (windows-latest, Node 22.x)' was skipped in CI at this commit and its suite did not run locally (Linux-only runner); the Windows behavior of the extension conversion/source-parsing changes is unverified。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI at this commit and its suite did not run locally; the CLI changes (serve route, DiscoverTab) are not covered by the scoped unit run。
未探索到全部深度（达到工具调用预算）："agent 8b"：none — all five invariant groups were walked; no checks left unfinished.；"agent reverse-audit (round 1)"：none — but one check was concluded by inference rather than execution: I did not run an end-to-end install of the conventional-hooks-only fixture to observe the…；chunk 5：executed test run of packages/core/src/extension/extension-converter.test.ts not performed — the review worktree has no node_modules or built dist/, and npm ci …。
Test Plan（非阻断）：src/extension/sourceRegistry.test.ts — no such file or directory; src/extension/marketplace.test.ts — no such file or directory; src/extension/claude-converter.test.ts — no such file or directory; src/extension/extension-converter.test.ts — no such file or directory; src/extension/extensionManager.test.ts — no such file or directory; and 4 more。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const isHttpPort = | ||
| (scheme === 'http://' || scheme === 'https://') && | ||
| /^\d+$/.test(potentialPluginName) && |
There was a problem hiding this comment.
[Critical] R10-1: The new isHttpPort heuristic models an HTTP(S) authority suffix as either an all-digit port or a plugin name and nothing else, regressing the removed /^\d+/ prefix check. Install sources with userinfo credentials (http://user:1234@host:8080), a port followed by a query or fragment on a path-less URL (https://host:8080?token=abc), or a bracketed IPv6 host without a port (https://[::1]) are all split into repo + pluginName where the pre-PR code kept them intact. The userinfo entrance breaks an install that worked at the merge base: with a repo served at the endpoint root, git clone http://user:1234@127.0.0.1:8080 cloned fine pre-PR, but at this commit the source parses to repo http://user and the clone fails with Could not resolve host: user. The query/fragment entrance drops the port and query (the clone then targets the host's default port instead of 8080) and fabricates 8080?token=abc as a pluginName whose kind defaults to 'marketplace-entry', steering conversion into a marketplace lookup that never existed. The bracketed-IPv6 shape is the same class (no working base flow was demonstrated for that entrance — the parse corruption is the evidence there).
| const isHttpPort = | |
| (scheme === 'http://' || scheme === 'https://') && | |
| /^\d+$/.test(potentialPluginName) && | |
| const isHttpPort = | |
| (scheme === 'http://' || scheme === 'https://') && | |
| /^\d+$/.test(potentialPluginName) && | |
| !/[?#@]/.test(potentialPluginName) && |
Never split an authority suffix containing ?, #, or @ — probe-verified to restore the userinfo, query, and fragment entrances while the intended port / numeric-alias-after-path / owner-repo-alias splits still apply. Bracketed IPv6 literals additionally need bracket-awareness (never split a suffix inside [...]).
Witness (probe + wire-level A/B against a real git-http-backend, merge base vs HEAD):
HEAD parse: "https://user:1234@example.com" => {"repo":"https://user","pluginName":"1234@example.com"}
BASE parseInstallSource("https://example.com:8080?token=abc") => {"source":"https://example.com:8080?token=abc","type":"git"}
HEAD parseInstallSource(same) => {"source":"https://example.com","pluginName":"8080?token=abc","pluginSourceKind":"marketplace-entry"}
BASE-arm clone http://user:1234@127.0.0.1:8080 => CLONE_OK (GET /info/refs + POST /git-upload-pack)
HEAD-arm clone http://user => fatal: Could not resolve host: user
flip: with the ?/#/@ guard => all three entrances INTACT; port and numeric-alias splits unchanged
中文说明
新的 isHttpPort 启发式把 HTTP(S) authority 后缀只建模为「纯数字端口」或「插件名」两种情况，回退了被移除的 /^\d+/ 前缀检查。携带 userinfo 凭据的安装来源（http://user:1234@host:8080）、无路径 URL 上端口后跟 query/fragment（https://host:8080?token=abc）、以及不带端口的 IPv6 方括号主机（https://[::1]）都会被拆成 repo + pluginName，而 PR 之前这些字符串保持完整。userinfo 入口破坏了一个在 merge base 可用的安装：当端点根路径提供 git 仓库时，git clone http://user:1234@127.0.0.1:8080 在 PR 之前可正常克隆；在本 commit 上该来源被解析为 repo http://user，克隆以 Could not resolve host: user 失败。query/fragment 入口会丢弃端口与查询参数（克隆转向主机默认端口而非 8080），并把 8080?token=abc 伪造成 pluginName（kind 默认为 'marketplace-entry'），将转换引入一个根本不存在的 marketplace 查找。IPv6 方括号形状属于同一类（该入口未证实存在可用的 base 流程——其证据是解析破坏本身）。建议修复：权威后缀包含 ?、# 或 @ 时一律不拆分（已探针验证可恢复上述三个入口，同时保留端口/路径后数字别名/owner-repo 别名的预期拆分）；IPv6 方括号还需括号感知（不在 [...] 内部拆分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
3 participants
What this PR does
Preserves Qwen/Gemini extension resources while importing Claude-compatible hooks from dual-manifest extensions. It also keeps marketplace-entry selection distinct from direct extension-root installs across Discover, CLI install, update checks, and persisted metadata; preserves
${CLAUDE_PLUGIN_ROOT}until installation-time hydration; eliminates the redundant second full-tree conversion for dual-manifest hook imports; makes recursive conversion copies cancellation-aware; and cleans temporary conversion directories on success, failure, and cancellation paths.Why it's needed
The converter previously chose one manifest path and overloaded
pluginNamefor two different meanings: selecting an entry from a Claude marketplace and naming a direct extension root. As a result, a dual-manifest extension could keep its Gemini resources or load its Claude hooks, but not reliably do both, while direct GitHub, URL,git@, orsso://roots could be reinterpreted as marketplace repositories. This fixes issue #8539 without executing untrusted third-party hooks during installation or validation.Reviewer Test Plan
How to verify
From
packages/core, run:From
packages/cli, run:Confirm the focused cases cover these outcomes:
marketplace-entry; bare/structured GitHub, URL,git@, andsso://sources remainextension-root, including digit-leading aliases;Object.prototypekeys do not crash or mutate the output object's prototype;Optional trusted-fixture E2E: follow
.qwen/e2e-tests/8539-dual-manifest-extension-hooks.md. Do not use an unreviewed third-party extension for this check.Evidence (Before & After)
N/A — this is a non-visual extension conversion and source-selection fix. The focused regression fixtures reproduce the previous manifest-selection failures and assert the corrected installed configuration.
Tested on
Environment (optional)
macOS arm64, Node.js 26.5.0. On the current head synced with
mainatabfd44369860b53aff1b0607ff983be6a0a98bfa, the focused Core extension matrix passed 423/423 and Discover passed 3/3; Core/CLI typecheck, focused ESLint/Prettier, andgit diff --checkalso passed.Risk & Scope
pluginSourceKindremains optional, and legacy install metadata continues through the compatibility path.Linked Issues
Fixes #8539
中文说明
此 PR 做了什么
在双清单扩展中保留 Qwen/Gemini 的扩展资源，同时导入 Claude 兼容 hooks。它还在 Discover、CLI 安装、更新检查和持久化元数据中明确区分 marketplace entry 与直接 extension root；将
${CLAUDE_PLUGIN_ROOT}保留到安装阶段再解析；消除双清单 hook 导入中多余的第二次全量转换，让递归复制可响应取消，并在成功、失败和取消路径中清理临时转换目录。为什么需要此修改
此前转换器只选择一条清单路径，并让
pluginName同时表示两个不同概念：从 Claude marketplace 选择子项，以及为直接扩展根目录提供名称。因此双清单扩展无法稳定地同时保留 Gemini 资源与加载 Claude hooks，而直接 GitHub、URL、git@或sso://根源也可能被误解为 marketplace 仓库。本修改在不执行不受信任第三方 hooks 的前提下修复 #8539。Reviewer 测试计划
如何验证
在
packages/core中运行：在
packages/cli中运行：确认聚焦用例覆盖以下结果：
marketplace-entry；裸/结构化 GitHub、URL、git@与sso://来源保持extension-root，包括数字开头的 alias；Object.prototype属性同名的 hook event 不会崩溃或修改输出对象原型；可选的可信 fixture E2E：按照
.qwen/e2e-tests/8539-dual-manifest-extension-hooks.md操作。不要用未经审阅的第三方扩展执行此验证。修改前后证据
N/A——这是非可视化的扩展转换与来源选择修复。聚焦回归 fixture 会复现此前的清单选择失败，并断言修复后的安装配置。
测试平台
环境（可选）
macOS arm64，Node.js 26.5.0。当前 head 已同步到
main的abfd44369860b53aff1b0607ff983be6a0a98bfa；Core 聚焦扩展矩阵为 423/423，Discover 为 3/3，Core/CLI typecheck、聚焦 ESLint/Prettier 与git diff --check均通过。风险与范围
pluginSourceKind仍为可选字段，旧安装元数据继续走兼容路径。关联 Issue
Fixes #8539