fix(onboard): scope installSelected gate to skills actually presented in multiselect

lonexreb · lonexreb · commit f0fb38c93003 · 2026-05-04T23:46:44.000-05:00
Copilot review on PR #74891 noted that the new installSelected gate at src/commands/onboard-skills.ts:219 silently skipped the API-key prompt for skills that have missing bins + primaryEnv but no install options. Such skills are never offered in the install multiselect (they're filtered out of installable because install.length === 0, and out of envOnlyConfigurable because missing.bins.length > 0), so they can never be in installSelected — the gate was unconditional for them. Track presentableNames as a Set of skills that actually appeared in the multiselect, then only skip when the skill was both presented AND not picked. Skills that were never presented (the documented bin-only no-install pattern in docs/tools/skills.md) keep getting prompted for their required env key. Add a regression test that builds such a skill (install: []) and asserts the API-key prompt still fires.
diff --git a/src/commands/onboard-skills.test.ts b/src/commands/onboard-skills.test.ts
@@ -310,4 +310,45 @@ describe("setupSkills", () => {
     // Install loop must not run installSkill for env-only entries.
     expect(mocks.installSkill).not.toHaveBeenCalled();
   });
+
+  it("still prompts for an API key for a skill with missing bins + primaryEnv but no install options (codex follow-up on PR #74891)", async () => {
+    if (process.platform === "win32") {
+      return;
+    }
+
+    // Documented pattern in docs/tools/skills.md: a skill can require bins +
+    // primaryEnv without an install block. Such skills are NEVER offered in
+    // the install multiselect (they're filtered out of `installable` because
+    // install.length === 0, and out of `envOnlyConfigurable` because
+    // missing.bins.length > 0). The previous gate `!installSelected.has(...)`
+    // silently skipped the API-key prompt for those skills. After the fix,
+    // the gate only fires when the skill was actually presented in the
+    // multiselect, so non-installable bin+env skills still get prompted for
+    // their required env key.
+    const noInstallSkill = createBundledSkill({
+      name: "openai-whisper-no-install",
+      description: "Skill that requires bins + an API key but has no install options",
+      bins: ["nonexistent-bin"],
+      installLabel: "unused",
+      primaryEnv: "OPENAI_API_KEY",
+      envMissing: ["OPENAI_API_KEY"],
+    });
+    // Override the install array to be empty — the documented bin-only
+    // no-install pattern. The helper always sets a brew install entry.
+    (noInstallSkill as { install: unknown[] }).install = [];
+
+    mockMissingBrewStatus([noInstallSkill]);
+
+    // No multiselect interaction expected — when configurable is empty the
+    // prompter never gets called for it; the API-key prompt should still run
+    // for the unpresentable skill below it.
+    const { prompter } = createPrompter({ multiselect: [] });
+    await setupSkills({} as OpenClawConfig, "/tmp/ws", runtime, prompter);
+
+    const confirmCalls = (
+      prompter.confirm as unknown as { mock: { calls: Array<[{ message: string }]> } }
+    ).mock.calls;
+    const askedForApiKey = confirmCalls.some(([arg]) => arg.message.includes("Set OPENAI_API_KEY"));
+    expect(askedForApiKey).toBe(true);
+  });
 });
diff --git a/src/commands/onboard-skills.ts b/src/commands/onboard-skills.ts
@@ -89,6 +89,13 @@ export async function setupSkills(
       Boolean(skill.primaryEnv) && skill.missing.env.length > 0 && skill.missing.bins.length === 0,
   );
   const configurable = [...installable, ...envOnlyConfigurable];
+  // Track names of skills the user actually saw in the install multiselect so
+  // the API-key gate below only fires for skills that were genuinely offered.
+  // Skills with missing bins + primaryEnv but no install options
+  // (`install.length === 0` so they aren't in `installable`, and
+  // `missing.bins.length > 0` so they aren't in `envOnlyConfigurable`) never
+  // appear in the multiselect — those should still get API-key prompts.
+  const presentableNames = new Set(configurable.map((skill) => skill.name));
   let next: OpenClawConfig = cfg;
   const installSelected = new Set<string>();
   if (configurable.length > 0) {
@@ -216,7 +223,12 @@ export async function setupSkills(
     if (!skill.primaryEnv || skill.missing.env.length === 0) {
       continue;
     }
-    if (!installSelected.has(skill.name)) {
+    // Only skip the API-key prompt when the skill was actually presented in
+    // the install multiselect and the user did not pick it. Skills that were
+    // never presented (bins required + primaryEnv but no install options)
+    // still get prompted — otherwise we silently stop asking for required
+    // env keys for a documented pattern (see docs/tools/skills.md).
+    if (presentableNames.has(skill.name) && !installSelected.has(skill.name)) {
       continue;
     }
     const wantsKey = await prompter.confirm({
