feat(tui): install skills via typed query in the /skills list (#1005)

* feat(tui): install skills via typed query in the /skills list

The `/skills` list didn't offer a discoverable way to install from a
GitHub repo / URL / absolute path — the only entry point was `ctrl+i`,
whose wire byte (0x09) collides with Tab on default terminals, so many
users couldn't trigger it.

Surface a synthetic "Install <query>" row at the top of the list when the
filter matches an installable shape, so pressing Enter opens the install
dialog prefilled with the typed text. Backed by a shared
`classifyInstallSource` classifier used by both the list preview and
`installSkillDirect` — an earlier `q.includes("/")` check drifted from
the installer and offered the row for skill-search queries like
`dbt/snowflake` that the installer then rejected as "Path not found".

- Add module-scope `classifyInstallSource` (recognises github URLs, clean
  `owner/repo` shorthand, POSIX absolute paths, Windows drive-letter
  paths — rejects short strings, sub-paths, `~`, relatives).
- Route the sentinel through `onSelect` to `showInstall`, forward the
  captured filter text as `initialValue` on `DialogSkillInstall` and
  `DialogSkillCreate`.
- Filter the sentinel out of `onMove` so highlighting the synthetic row
  doesn't set `currentSkill` to the sentinel string (would trip
  `ctrl+a`'s action picker into a degenerate lookup miss).
- Harden `showActions` to bail on the sentinel or a lookup miss as
  belt-and-braces.
- Build options via a spread instead of `Array.prototype.unshift` so the
  memo stays pure (Solid dev-mode double-eval would otherwise
  double-prepend).
- Slim the fork-feature-guards test to a minimal presence check
  (sentinel, classifier symbol, sentinel-route in `onSelect`, prefill
  plumbing) — behavioural coverage of the classifier moves to a new
  `test/altimate/skill-install-classifier.test.ts` unit suite.

Verified: `bun run typecheck` clean; new classifier suite 10/10;
fork-feature-guards 18/18; `test/session` 731/731.

* fix(tui): address OpenCodeReview findings on skills-install classifier

Two medium findings from the automated review pass on this PR:

1. `installSkillDirect` used the raw `normalized` input when building a
   clone URL, but `classifyInstallSource` tolerates a trailing `.git`
   suffix / whitespace / trailing dots. An input like `owner/repo.git`
   passed the classifier's `owner-repo` check and then produced
   `https://github.com/owner/repo.git.git`. Extracted the trim/strip
   logic into `normalizeInstallSource` and use it in both the classifier
   and the URL builder so the two can't disagree on shape.

2. The `options` `createMemo` read the `filter()` signal, so every
   keystroke re-executed the full `list.map` — including
   `detectToolReferences` (regex parse per skill). Split into a
   `baseOptions` memo that depends only on `skills()` and a derived
   `options` memo that composes the synthetic Install row on top. Now a
   filter change only re-checks `classifyInstallSource(q)` and prepends
   one item; `detectToolReferences` only re-runs when the underlying
   skills list changes.

Also adds a `normalizeInstallSource` unit-test suite (4 cases pinning
the `.git` / trailing-dot / whitespace behaviour) alongside the existing
classifier tests.

Verified: `bun run typecheck` clean; classifier suite 14/14 (54 expects);
fork-feature-guards 18/18 (61 expects).

* fix(tui): route owner/repo shorthand by classifier kind, not http-prefix probe

Second review pass on this PR — one live-reproduced bug + two auxiliary
finds:

1. `installSkillDirect` branched clone-URL construction on
   `cleaned.startsWith("http")`, which misfired for owners whose name
   literally begins with `http` (`httpie/httpie`,
   `http-party/http-server`, `httpwg/http-extensions`): they classify as
   `owner-repo` yet passed the prefix probe, so the installer tried to
   `git clone httpie/httpie` and failed. Fix: branch on the already-
   computed `kind` from `classifyInstallSource`. As a bonus, the
   `github-url` branch now preserves the trailing `.git` the user typed
   (matters for self-hosted git servers that require it), while the
   `owner-repo` branch continues to normalize.

2. Merge-drop guard in `fork-feature-guards.test.ts` didn't cover the
   memo block that actually creates the synthetic Install row —
   deleting the block still passed all guards (verified by mutation).
   Add a proximity-anchored `classifyInstallSource(q) → value:
   INSTALL_ACTION_VALUE` match; the 300-char bound is deliberately
   tight so a real deletion collapses it to zero and fails loudly.

3. Classifier comments and one test title claimed `dbt/snowflake` was
   rejected, but the classifier returns `owner-repo` for any clean
   two-segment string — intentional, since skill names can't contain a
   slash. Correct the docstrings and pin the actual behaviour with
   explicit `expect(...dbt/snowflake).toBe("owner-repo")` and
   `expect(...httpie/httpie).toBe("owner-repo")` assertions.

Not addressed here: the pre-existing `/skills` slash-command collision
between the upstream `prompt.skills` and this plugin's
`altimate.skill.list`. Already on `main`, already tracked as a comment
on the Jira ticket, out of scope for this PR.

Verified: `bun run typecheck` clean; classifier + fork-guard suites
34/34 (100 expects); `test/session` 731/731 (1636 expects).

---------

Co-authored-by: Haider <haider@altimate.ai>

Author: sahrizvi

packages/opencode/src/plugin/tui/altimate/skill-ops.tsx

@@ -34,6 +34,44 @@ import fs from "fs/promises"
 
 const id = "altimate:skill-ops"
 
+// altimate_change start — single classifier shared by installSkillDirect and the skill-list
+// dialog's "Install <query>" affordance. Extracted so the UI's install-preview and the
+// installer can't drift (an earlier `looksInstallable` that only checked `q.includes("/")`
+// surfaced the Install option for shapes the installer then rejected as "Path not found",
+// e.g. `owner/repo/subpath`). A clean two-segment `owner/repo` is intentionally treated
+// as GitHub shorthand — indistinguishable from a two-token search without a network call,
+// and skill names cannot contain a slash so no real search result is hijacked. Returns
+// null for shapes the installer wouldn't accept without ambiguity — three-segment paths,
+// relative paths, `~`, bare identifiers — so we only surface the synthetic Install row
+// when Enter will actually try to install.
+export type InstallSourceKind = "github-url" | "owner-repo" | "absolute-path"
+const OWNER_REPO_REGEX = /^[a-zA-Z0-9_-]+\/[a-zA-Z0-9._-]+$/
+
+// Strip the surface variation the classifier tolerates — whitespace, trailing dots,
+// and a `.git` suffix — before comparing. Exported so callers that consume the
+// classified string (e.g. installSkillDirect building a clone URL) can use the exact
+// same shape as the classifier, avoiding double-suffix bugs like `owner/repo.git.git`.
+export function normalizeInstallSource(source: string): string {
+  return source.trim().replace(/\.+$/, "").replace(/\.git$/, "")
+}
+
+export function classifyInstallSource(source: string): InstallSourceKind | null {
+  const trimmed = normalizeInstallSource(source)
+  if (trimmed.length < 3) return null
+  if (trimmed.startsWith("http://") || trimmed.startsWith("https://")) return "github-url"
+  if (OWNER_REPO_REGEX.test(trimmed)) return "owner-repo"
+  if (trimmed.startsWith("/")) return "absolute-path"
+  if (/^[a-zA-Z]:[\\/]/.test(trimmed)) return "absolute-path"
+  return null
+}
+
+// Sentinel value for the synthetic top-of-list "Install <query>" option. Namespaced so it
+// can't collide with any skill name (skill names match `^[a-z][a-z0-9]*(-[a-z0-9]+)*$`).
+// Module-scope so it's stable across renders and reachable by every function that needs
+// to check whether an item.value is the sentinel (onSelect, onMove, showActions).
+const INSTALL_ACTION_VALUE = "__altimate:skill:install-from-query__"
+// altimate_change end
+
 // Categorize skills by domain for cleaner grouping in the list.
 const SKILL_CATEGORIES: Record<string, string> = {
   "dbt-develop": "dbt",
@@ -143,12 +181,27 @@ async function installSkillDirect(
     normalized = `https://github.com/${ghWebMatch[1]}.git`
   }
 
-  if (
-    normalized.startsWith("http://") ||
-    normalized.startsWith("https://") ||
-    normalized.match(/^[a-zA-Z0-9_-]+\/[a-zA-Z0-9._-]+$/)
-  ) {
-    const url = normalized.startsWith("http") ? normalized : `https://github.com/${normalized}.git`
+  // Classify the source. Anything the classifier recognises (github-url / owner-repo /
+  // absolute-path) takes the corresponding branch. Anything it doesn't (relative path,
+  // `~`, bare identifier) falls through to the path branch below and is resolved against
+  // cwd — that's the historical behaviour, preserved as a fallback for callers that
+  // reach the installer without going through the UI's install-preview.
+  const kind = classifyInstallSource(normalized)
+  if (kind === "github-url" || kind === "owner-repo") {
+    // Branch on the already-computed `kind`, not on a `startsWith("http")` probe of the
+    // normalized string. Two bugs are avoided that way:
+    //   1. An owner whose name literally starts with `http` (e.g. `httpie/httpie`,
+    //      `http-party/http-server`) classifies as `owner-repo` but its normalized form
+    //      also starts with `http` — a raw prefix test would treat it as an "already a
+    //      URL" and try to `git clone httpie/httpie`, which fails.
+    //   2. `normalizeInstallSource` strips a trailing `.git`; for owner-repo we want that
+    //      (so `owner/repo.git` doesn't produce `owner/repo.git.git`), but for an explicit
+    //      `github-url` we want to preserve whatever the user typed — self-hosted git
+    //      servers require the exact suffix form.
+    const url =
+      kind === "github-url"
+        ? normalized.trim().replace(/\.+$/, "")
+        : `https://github.com/${normalizeInstallSource(normalized)}.git`
     const label = url.replace(/https?:\/\/github\.com\//, "").replace(/\.git$/, "")
     onProgress?.(`Cloning ${label}...`)
     const cache = cacheDir()
@@ -295,14 +348,15 @@ async function reloadAndVerify(api: TuiPluginApi, expectedNames: string[]): Prom
 
 // ── Sub-dialogs ─────────────────────────────────────────────────────────────────────────────────
 
-function DialogSkillCreate(props: { api: TuiPluginApi }) {
+function DialogSkillCreate(props: { api: TuiPluginApi; initialValue?: string }) {
   const { api } = props
   const theme = () => api.theme.current
   const [busy, setBusy] = createSignal(false)
   return (
     <api.ui.DialogPrompt
       title="Create Skill"
       placeholder="my-tool"
+      value={props.initialValue}
       busy={busy()}
       busyText="Creating skill..."
       description={() => (
@@ -350,7 +404,7 @@ function DialogSkillCreate(props: { api: TuiPluginApi }) {
   )
 }
 
-function DialogSkillInstall(props: { api: TuiPluginApi }) {
+function DialogSkillInstall(props: { api: TuiPluginApi; initialValue?: string }) {
   const { api } = props
   const theme = () => api.theme.current
   const [busy, setBusy] = createSignal(false)
@@ -359,6 +413,7 @@ function DialogSkillInstall(props: { api: TuiPluginApi }) {
     <api.ui.DialogPrompt
       title="Install Skill (owner/repo, URL, or path)"
       placeholder="anthropics/skills"
+      value={props.initialValue}
       busy={busy()}
       busyText="Installing skill..."
       description={() => (
@@ -552,7 +607,18 @@ function DialogSkillList(props: { api: TuiPluginApi; onCurrent: (skill: string |
   // Expose the lookup to the keymap-layer commands (ctrl+a/test/etc).
   registerLookup(api, skillMap)
 
-  const options = createMemo<TuiDialogSelectOption<string>[]>(() => {
+  // altimate_change start — capture the current filter text so an install/create
+  // action triggered from inside the list can prefill the sub-dialog with what the user typed
+  // (e.g. a GitHub URL typed into the search box), instead of dropping it on the floor.
+  const [filter, setFilter] = createSignal("")
+  currentFilter = filter
+  // altimate_change end
+
+  // Base list — depends only on `skills()`. Kept separate from the `options` memo below
+  // so that per-keystroke filter changes don't re-run `detectToolReferences` (regex parse
+  // per skill) across the whole list. On projects with many installed skills the previous
+  // fused memo produced noticeable typing lag.
+  const baseOptions = createMemo<TuiDialogSelectOption<string>[]>(() => {
     const list = skills() ?? []
     const maxWidth = Math.max(0, ...list.map((s) => s.name.length))
     return list.map((skill) => {
@@ -570,13 +636,49 @@ function DialogSkillList(props: { api: TuiPluginApi; onCurrent: (skill: string |
     })
   })
 
+  const options = createMemo<TuiDialogSelectOption<string>[]>(() => {
+    const items = baseOptions()
+    // altimate_change start — when the filter looks like a shape installSkillDirect will
+    // accept (github URL, `owner/repo` shorthand, or absolute path), prepend a synthetic
+    // "Install <query>" top option. Selecting it (Enter) routes to installSkillDirect.
+    // Non-mutating build (spread instead of unshift) so this stays a pure memo — Solid
+    // dev-mode double-eval would otherwise double-prepend.
+    // ctrl+i on the wire is byte 0x09 = Tab, so we can't offer a reliable ctrl+i binding
+    // on default terminals; Enter on this synthetic row is the discoverable substitute.
+    const q = filter().trim()
+    if (classifyInstallSource(q) !== null) {
+      const installOption: TuiDialogSelectOption<string> = {
+        title: `Install ${q}`,
+        description: "Press Enter to install from this GitHub repo, URL, or path",
+        footer: undefined,
+        value: INSTALL_ACTION_VALUE,
+        category: "Install",
+      }
+      return [installOption, ...items]
+    }
+    // altimate_change end
+    return items
+  })
+
   return (
     <api.ui.DialogSelect
-      title="Skills (ctrl+a actions · ctrl+n new · ctrl+i install)"
-      placeholder="Search skills..."
+      title="Skills"
+      placeholder="Search skills, or type a repo/URL and press Enter to install..."
       options={options()}
-      onMove={(item) => props.onCurrent(item.value)}
+      onFilter={(q) => setFilter(q)}
+      // altimate_change start — filter the sentinel out of onCurrent so highlighting
+      // the synthetic Install row doesn't set `currentSkill` to the sentinel string
+      // (which would trip ctrl+a's showActions into opening a degenerate action picker
+      // on a non-existent skill named INSTALL_ACTION_VALUE).
+      onMove={(item) => props.onCurrent(item.value === INSTALL_ACTION_VALUE ? undefined : item.value)}
+      // altimate_change end
       onSelect={(item) => {
+        // altimate_change start — synthetic install option routes to installer.
+        if (item.value === INSTALL_ACTION_VALUE) {
+          showInstall(api, filter().trim() || undefined)
+          return
+        }
+        // altimate_change end
         props.onCurrent(item.value)
         // Selecting a skill opens its action picker (the pre-merge default action was the picker).
         openActionPicker(api, skillMap().get(item.value), item.value, () => showList(api))
@@ -589,34 +691,48 @@ function DialogSkillList(props: { api: TuiPluginApi; onCurrent: (skill: string |
 // Both are module-level so the layer (registered once at plugin init) can reach the live values.
 let currentSkill: string | undefined
 let currentLookup: () => Map<string, SkillInfo> = () => new Map()
+// altimate_change start — expose the current list dialog's filter text so the outer
+// keymap-layer install/create commands (ctrl+i/ctrl+n) can prefill the sub-dialog with whatever
+// the user has typed into the search box. Reset to a no-op after the list closes so a fresh
+// keybind press from an unrelated context doesn't leak stale text.
+let currentFilter: () => string = () => ""
+// altimate_change end
 
 function registerLookup(_api: TuiPluginApi, lookup: () => Map<string, SkillInfo>) {
   currentLookup = lookup
 }
 
 function showList(api: TuiPluginApi) {
-  api.ui.dialog.replace(() => (
-    <DialogSkillList api={api} onCurrent={(skill) => (currentSkill = skill)} />
-  ))
+  api.ui.dialog.replace(
+    () => <DialogSkillList api={api} onCurrent={(skill) => (currentSkill = skill)} />,
+    () => {
+      currentFilter = () => ""
+    },
+  )
 }
 
-function showCreate(api: TuiPluginApi) {
+function showCreate(api: TuiPluginApi, initialValue?: string) {
   api.ui.dialog.replace(
-    () => <DialogSkillCreate api={api} />,
+    () => <DialogSkillCreate api={api} initialValue={initialValue} />,
     () => setTimeout(() => showList(api), 0),
   )
 }
 
-function showInstall(api: TuiPluginApi) {
+function showInstall(api: TuiPluginApi, initialValue?: string) {
   api.ui.dialog.replace(
-    () => <DialogSkillInstall api={api} />,
+    () => <DialogSkillInstall api={api} initialValue={initialValue} />,
     () => setTimeout(() => showList(api), 0),
   )
 }
 
 function showActions(api: TuiPluginApi) {
-  if (!currentSkill) return
-  openActionPicker(api, currentLookup().get(currentSkill), currentSkill, () => showList(api))
+  // Belt-and-braces against the synthetic Install row leaking into `currentSkill` via a
+  // future refactor that forgets the DialogSkillList `onMove` filter — refuse to open a
+  // per-skill action picker unless we can resolve a real skill entry from the lookup.
+  if (!currentSkill || currentSkill === INSTALL_ACTION_VALUE) return
+  const info = currentLookup().get(currentSkill)
+  if (!info) return
+  openActionPicker(api, info, currentSkill, () => showList(api))
 }
 
 const tui: TuiPlugin = async (api) => {
@@ -649,7 +765,7 @@ const tui: TuiPlugin = async (api) => {
         category: "Altimate",
         namespace: "palette",
         run() {
-          showCreate(api)
+          showCreate(api, currentFilter().trim() || undefined)
         },
       },
       {
@@ -659,7 +775,7 @@ const tui: TuiPlugin = async (api) => {
         category: "Altimate",
         namespace: "palette",
         run() {
-          showInstall(api)
+          showInstall(api, currentFilter().trim() || undefined)
         },
       },
     ],

packages/opencode/test/altimate/skill-install-classifier.test.ts

@@ -0,0 +1,115 @@
+import { describe, expect, test } from "bun:test"
+import { classifyInstallSource, normalizeInstallSource } from "@/plugin/tui/altimate/skill-ops"
+
+// classifyInstallSource is the shared classifier for the DialogSkillList's "Install <query>"
+// affordance and installSkillDirect. Keeping the two in lockstep matters: an earlier version
+// of the list used `q.includes("/") && q.length >= 3`, which offered the Install option for
+// three-segment paths and other shapes the installer then rejected as "Path not found".
+// A clean two-segment `owner/repo` is intentionally accepted as GitHub shorthand — skill
+// names can't contain a slash so no search result is hijacked. These tests pin the
+// classifier's edge cases so the UI's install-preview and the installer can't drift.
+describe("classifyInstallSource", () => {
+  test("recognises github owner/repo shorthand", () => {
+    expect(classifyInstallSource("anthropics/skills")).toBe("owner-repo")
+    expect(classifyInstallSource("owner/repo.git")).toBe("owner-repo")
+    expect(classifyInstallSource("Owner_1/repo-2.name")).toBe("owner-repo")
+  })
+
+  test("recognises http(s) URLs", () => {
+    expect(classifyInstallSource("https://github.com/anthropics/skills.git")).toBe("github-url")
+    expect(classifyInstallSource("http://example.com/thing")).toBe("github-url")
+  })
+
+  test("recognises POSIX absolute paths", () => {
+    expect(classifyInstallSource("/tmp/my-skill")).toBe("absolute-path")
+    expect(classifyInstallSource("/home/user/skills/foo")).toBe("absolute-path")
+  })
+
+  test("recognises Windows drive-letter paths", () => {
+    expect(classifyInstallSource("C:\\Users\\me\\skills")).toBe("absolute-path")
+    expect(classifyInstallSource("D:/skills/foo")).toBe("absolute-path")
+  })
+
+  test("rejects short strings that the installer would refuse", () => {
+    expect(classifyInstallSource("a")).toBeNull()
+    expect(classifyInstallSource("ab")).toBeNull()
+    expect(classifyInstallSource("")).toBeNull()
+  })
+
+  test("rejects three-segment paths (sub-paths inside a repo)", () => {
+    // Historically `q.includes("/")` misfired on these — the list surfaced an Install
+    // option that installSkillDirect then refused with "Path not found".
+    expect(classifyInstallSource("owner/repo/subpath")).toBeNull()
+    expect(classifyInstallSource("dbt/snowflake/thing")).toBeNull()
+  })
+
+  test("clean two-segment strings are treated as owner/repo shorthand", () => {
+    // Intentional feature, not a bug: skill names can't contain a slash (see the
+    // `^[a-z][a-z0-9]*(-[a-z0-9]+)*$` grammar enforced by createSkillDirect), so any
+    // two-segment string typed into the filter cannot collide with a real skill name
+    // and is unambiguously a GitHub `owner/repo` shorthand.
+    expect(classifyInstallSource("dbt/snowflake")).toBe("owner-repo")
+    expect(classifyInstallSource("anthropics/skills")).toBe("owner-repo")
+  })
+
+  test("owners whose name starts with `http` still classify as owner-repo, not URL", () => {
+    // Regression pin for a real bug where installSkillDirect used
+    // `cleaned.startsWith("http")` to route between "already a URL" and "build a
+    // github.com URL": that misrouted owner names literally beginning with `http`
+    // (e.g. `httpie/httpie`, `http-party/http-server`) as pre-formed URLs, then failed
+    // to clone. The fix branches on the classifier's `kind` instead of a prefix probe;
+    // this test locks in the classifier's side of the contract.
+    expect(classifyInstallSource("httpie/httpie")).toBe("owner-repo")
+    expect(classifyInstallSource("http-party/http-server")).toBe("owner-repo")
+    expect(classifyInstallSource("httpwg/http-extensions")).toBe("owner-repo")
+  })
+
+  test("rejects `~`-prefixed and relative paths — ambiguous with skill names", () => {
+    expect(classifyInstallSource("~/skills/foo")).toBeNull()
+    expect(classifyInstallSource("./local")).toBeNull()
+    expect(classifyInstallSource("../thing")).toBeNull()
+  })
+
+  test("rejects bare identifiers with no slash and no path prefix", () => {
+    expect(classifyInstallSource("skill-name")).toBeNull()
+    expect(classifyInstallSource("dbt-snowflake")).toBeNull()
+  })
+
+  test("strips trailing dots and `.git` before classifying", () => {
+    expect(classifyInstallSource("owner/repo.git")).toBe("owner-repo")
+    expect(classifyInstallSource("https://github.com/x/y.git")).toBe("github-url")
+    expect(classifyInstallSource("owner/repo.")).toBe("owner-repo")
+  })
+
+  test("trims surrounding whitespace before classifying", () => {
+    expect(classifyInstallSource("  owner/repo  ")).toBe("owner-repo")
+    expect(classifyInstallSource("\thttps://github.com/x/y\n")).toBe("github-url")
+  })
+})
+
+// normalizeInstallSource is the shared trim/strip helper that installSkillDirect uses
+// before building a clone URL from an `owner/repo` shorthand. Without it, an input like
+// `owner/repo.git` (accepted by the classifier because it strips `.git`) would produce
+// `https://github.com/owner/repo.git.git` — real bug flagged in review.
+describe("normalizeInstallSource", () => {
+  test("strips a trailing `.git` suffix", () => {
+    expect(normalizeInstallSource("owner/repo.git")).toBe("owner/repo")
+    expect(normalizeInstallSource("https://github.com/x/y.git")).toBe("https://github.com/x/y")
+  })
+
+  test("strips trailing dots", () => {
+    expect(normalizeInstallSource("owner/repo.")).toBe("owner/repo")
+    expect(normalizeInstallSource("owner/repo...")).toBe("owner/repo")
+  })
+
+  test("trims surrounding whitespace", () => {
+    expect(normalizeInstallSource("  owner/repo  ")).toBe("owner/repo")
+    expect(normalizeInstallSource("\n\towner/repo\r\n")).toBe("owner/repo")
+  })
+
+  test("returns the source unchanged when nothing to strip", () => {
+    expect(normalizeInstallSource("owner/repo")).toBe("owner/repo")
+    expect(normalizeInstallSource("/tmp/skills")).toBe("/tmp/skills")
+    expect(normalizeInstallSource("")).toBe("")
+  })
+})