fix(extension): auto-detect agents/skills/commands directories for Gemini extensions - #4859

Passing these fields through is not enough for non-default paths. ExtensionManager still loads extension skills and agents from ${extensionPath}/skills and ${extensionPath}/agents unconditionally, so a Gemini extension that explicitly declares "agents": "custom-agents" or "skills": "custom-skills" will get those paths written to qwen-extension.json but the resources still will not be loaded. This needs either collection into the default folders during conversion (similar to the Claude converter) or loader support for the configured paths before exposing the fields.

Fixed — removed the passthrough of agents/skills/commands from convertGeminiToQwenConfig entirely. Since ExtensionManager always loads from hardcoded ${extensionPath}/agents etc., exposing custom paths in the config would be misleading. The auto-detection in convertGeminiExtensionPackage still handles the standard directory case correctly.

This passthrough has a conversion gap for explicitly declared command paths. The package converter only runs TOML-to-Markdown conversion for the hard-coded commands/ directory, but after this line Qwen will respect a custom commands value from gemini-extension.json. For "commands": "custom-cmds", any Gemini TOML commands in custom-cmds remain unconverted, so FileCommandLoader will scan that directory for .md files and load nothing. Please convert the declared command path(s), or only emit commands when the target has been converted into a Qwen-loadable Markdown directory.

Fixed — same as above, removed the commands passthrough. Custom command paths would bypass TOML-to-Markdown conversion and fail to load. Auto-detection now only emits "commands": "commands" when the default directory exists as a non-empty directory.

[Suggestion] Inconsistent empty-directory guard across the three auto-detection blocks. The commands check (below) includes fs.readdirSync(...).length > 0, but agents and skills only check fs.existsSync(). An empty agents/ or skills/ directory (e.g., scaffolding placeholder) would be falsely detected and registered in the config.

Apply the same guard to the skills block as well.

— qwen3.7-max via Qwen Code /review

Fixed — added fs.readdirSync(agentsDir).length > 0 guard to both agents and skills blocks, matching the existing commands check. All three now consistently skip empty directories.

[Suggestion] commandsDirAfterConvert is path.join(tmpDir, 'commands') — identical to commandsDir from Step 2 (line 91). Both resolve to the same path. The redundant variable hurts readability and creates mild confusion about whether the path could differ post-conversion. Reuse commandsDir instead.

Then use commandsDir in the check below.

— qwen3.7-max via Qwen Code /review

Fixed — removed commandsDirAfterConvert and reused commandsDir from Step 2.

[Suggestion] The comment "including auto-detected fields" is misleading in the context of convertGeminiToQwenConfig. This function simply passes fields through from gemini-extension.json — auto-detection happens later in convertGeminiExtensionPackage (Step 3). A reader of this function in isolation would incorrectly conclude that auto-detection logic exists inside it. Revert to // Direct field mapping.

— qwen3.7-max via Qwen Code /review

Fixed — reverted to // Direct field mapping. The auto-detection fields passthrough has been removed from this function entirely.

[MEDIUM] fs.existsSync returns true for regular files as well as directories. If an extension has a regular file named agents (not a directory), this would incorrectly add "agents": "agents" to the config. Downstream, loadSubagentFromDir calls fs.readdir which would throw ENOTDIR.

The codebase uses statSync().isDirectory() elsewhere (e.g. extensionManager.ts:581, extensionManager.ts:630). Consider applying the same pattern here and for skillsDir/commandsDirAfterConvert below.

Fixed — added fs.statSync(agentsDir).isDirectory() checks to all three auto-detection blocks, consistent with the pattern used in extensionManager.ts.

[MEDIUM] Inconsistent empty-directory handling: the commands auto-detection checks fs.readdirSync(...).length > 0 to skip empty directories, but the agents and skills checks above (lines 99, 107) don't. This means an empty agents/ or skills/ directory will be declared in the config while an empty commands/ won't.

If the intent is to skip empty directories for all three, add the same check to agents and skills. If the intent is different, a comment explaining why commands gets the extra guard would help.

Fixed — all three blocks now have consistent guards: existsSync + isDirectory() + readdirSync().length > 0.

[LOW] commandsDirAfterConvert is the same path as commandsDir declared above at line 89 (path.join(tmpDir, 'commands')). The variable could be reused to avoid the redundant declaration.

Fixed — removed the redundant variable, now reusing commandsDir from Step 2.

[Critical] path.join(extensionPath, configValue) does not validate that the resolved path stays within extensionPath. A malicious extension can set "agents": "../../.ssh" in its manifest to escape the extension directory. loadSubagentFromDir and loadSkillsFromDir then read .md file contents from arbitrary filesystem locations and inject them as LLM system prompts.

Before this PR, paths were hardcoded (${extensionPath}/agents), so this is a new attack surface. The codebase already has isPathWithinRoot (workspaceContext.ts:274) that can be reused.

— qwen3.7-max via Qwen Code /review

[Critical] These three tests compare path.join output against POSIX-separator string literals. On Windows, path.join('/ext', 'custom-agents') returns \ext\custom-agents, so all three assertions in this describe block fail on win32 — this is the root cause of the failing Test (windows-latest, Node 22.x) CI job on this PR. Build the expectations with path.join here and in the other two tests (lines 1127–1129 and 1133–1135):

— claude-fable-5 via Claude Code /qreview

[Critical] This auto-detection step is behaviorally inert for the bug the PR claims to fix. It only ever writes the literal default values ("agents": "agents", etc.), and resolveExtensionResourceDir(p, 'agents', 'agents') resolves to exactly the same path as the undefined fallback — which is also exactly the path the loader hardcoded on main before this PR (${effectiveExtensionPath}/agents, scanned unconditionally). So for every converted Gemini extension, load behavior is identical with or without these manifest fields. gsd-core's 33 agents are flat top-level .md files in agents/ — precisely the layout main already scans — so the "before: 2 of 33, after: 33" evidence cannot be produced by this diff; the real root cause of that symptom (if it reproduces on current main) is likely elsewhere, e.g. per-file frontmatter validation in loadSubagentFromDir.

Please either (a) reproduce the 2-of-33 symptom on a build of current main and add a regression test that fails without this diff, or (b) re-scope the PR to "honor manifest-declared resource dirs in ExtensionManager" (the genuinely new behavior here) and drop this auto-detection step as dead weight.

— claude-fable-5 via Claude Code /qreview

[Critical] The PR's headline behavior — the loader honoring manifest-declared dirs (here at lines 705–732 and in the install/update consent flow at lines 1003–1027) — has zero behavioral test coverage. The only new tests are three pure path-arithmetic unit tests of resolveExtensionResourceDir, which would still pass if the helper were never called from these call sites. That is exactly the written-but-never-consumed gap this PR's first iteration shipped (caught by human review, not tests). The existing fixtures make this cheap: write a manifest with "agents": "custom-agents" / "skills" / "commands", create files in the custom dirs (and decoys in the default dirs), then assert via refreshCache()/getLoadedExtensions() that extension.agents/skills/commands come from the declared dirs.

— claude-fable-5 via Claude Code /qreview

[Suggestion] Two unvalidated value classes silently change behavior here:

1. Empty string: "commands": "" passes the typeof check and path.join(p, '') returns p itself, so the loaders scan the extension root — loadCommandsFromDir globs **/*.md with dot: true, turning README.md/docs into phantom commands, and loadSubagentFromDir parses top-level .md files as agents. Pre-PR, '' fell through to the default dir, and FileCommandLoader (truthiness check) still treats '' as unset — so the two consumers of the same field now disagree.
2. Declared-but-missing dir: a stale "agents": "my-agents" in an existing manifest (these fields were documented but inert for agents/skills until this PR) now resolves with no existence check, and all three loaders swallow the miss silently (loadCommandsFromDir suppresses ENOENT, loadSubagentFromDir bare-catches, loadSkillsFromDir debug-logs only) — resources silently vanish after upgrade with nothing in default logs.

Suggest treating empty/whitespace strings as unset, and emitting a non-debug warning when an explicitly declared dir does not exist:

— claude-fable-5 via Claude Code /qreview

[Suggestion] This helper and FileCommandLoader.getExtensionCommandsPaths (packages/cli/src/services/FileCommandLoader.ts:209–225) now resolve the same manifest field with conflicting semantics: FileCommandLoader honors string[] values (every entry) and absolute paths (path.isAbsolute check); this helper silently ignores arrays (falls back to the default dir) and mangles absolute strings (path.join('/ext', '/abs') → /ext/abs). Note also that loadExtensionConfig (line 834) runs recursivelyHydrateStrings over the config, so a documented-style value like "commands": "${extensionPath}/commands" hydrates to an absolute path and resolves to <ext>/<ext>/commands here — loading nothing.

Consequence: for array/absolute/hydrated manifests, the install-consent prompt and extension.commands listing show a different command set than what FileCommandLoader actually registers at runtime — the user consents to a list that doesn't match what loads. Suggest a single shared resolver used by both consumers: expand arrays, handle path.isAbsolute, and apply the containment validation from the open comment at line 240 in that one place.

— claude-fable-5 via Claude Code /qreview

[Suggestion] These three fields are dead code — nothing reads them: convertGeminiToQwenConfig deliberately drops them (per the earlier review threads), and the auto-detect mutations below operate on the returned Qwen ExtensionConfig, which already declares these fields. Worse, they advertise support that doesn't exist: a Gemini extension declaring "agents": "my-agents" has its files copied into the converted package, the declaration silently dropped, auto-detection probing only the default agents/ — so those agents vanish with zero diagnostics, the same symptom class this PR sets out to fix. The original rationale for dropping the passthrough ("ExtensionManager loads from hardcoded paths") was invalidated by this same PR's loader change.

Suggest either removing the three fields, or passing through string-valued agents/skills now that the loader honors them (commands must stay dropped — TOML→MD conversion only processes the default commands/ dir — and deserves a comment saying so).

— claude-fable-5 via Claude Code /qreview

[Suggestion] These tests assert the in-memory config return value, but the production pipeline discards it — convertGeminiOrClaudeExtension keeps only .convertedDir, and installExtension re-reads qwen-extension.json from disk (extensionManager.ts:965). Coverage currently holds only by statement ordering (the file is written after the mutations); swapping Step 3/Step 4 would break production while these tests stay green. Add at least one assertion against the persisted manifest:

const persisted = JSON.parse(
  actualFs.readFileSync(path.join(convertedDir, 'qwen-extension.json'), 'utf-8'),
);
expect(persisted.agents).toBe('agents');

— claude-fable-5 via Claude Code /qreview