diff --git a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx index 35abf2e9c..ffefa8826 100644 --- a/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx +++ b/packages/opencode/src/plugin/tui/altimate/skill-ops.tsx @@ -34,6 +34,41 @@ 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 " affordance. Extracted so the UI's install-preview and the +// installer can't drift (an earlier `looksInstallable` that only checked `q.includes("/")` +// showed the Install option for skill-search queries like "dbt/snowflake" which the +// installer then rejected as "Path not found"). Returns null for shapes the installer +// wouldn't accept without ambiguity — search terms, relative paths, `~` — 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 " 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 = { "dbt-develop": "dbt", @@ -143,12 +178,19 @@ 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") { + // Build the clone URL from the *normalized* source so a `.git` suffix or trailing + // dot in the typed query doesn't produce `owner/repo.git.git` (bug reported by + // OpenCodeReview): the classifier tolerates those shapes but the raw `normalized` + // still carries them until stripped. + const cleaned = normalizeInstallSource(normalized) + const url = cleaned.startsWith("http") ? cleaned : `https://github.com/${cleaned}.git` const label = url.replace(/https?:\/\/github\.com\//, "").replace(/\.git$/, "") onProgress?.(`Cloning ${label}...`) const cache = cacheDir() @@ -295,7 +337,7 @@ 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) @@ -303,6 +345,7 @@ function DialogSkillCreate(props: { api: TuiPluginApi }) { ( @@ -350,7 +393,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 +402,7 @@ function DialogSkillInstall(props: { api: TuiPluginApi }) { ( @@ -552,7 +596,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[]>(() => { + // 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[]>(() => { const list = skills() ?? [] const maxWidth = Math.max(0, ...list.map((s) => s.name.length)) return list.map((skill) => { @@ -570,13 +625,49 @@ function DialogSkillList(props: { api: TuiPluginApi; onCurrent: (skill: string | }) }) + const options = createMemo[]>(() => { + 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 " 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 = { + 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 ( 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 +680,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 = () => 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) { currentLookup = lookup } function showList(api: TuiPluginApi) { - api.ui.dialog.replace(() => ( - (currentSkill = skill)} /> - )) + api.ui.dialog.replace( + () => (currentSkill = skill)} />, + () => { + currentFilter = () => "" + }, + ) } -function showCreate(api: TuiPluginApi) { +function showCreate(api: TuiPluginApi, initialValue?: string) { api.ui.dialog.replace( - () => , + () => , () => setTimeout(() => showList(api), 0), ) } -function showInstall(api: TuiPluginApi) { +function showInstall(api: TuiPluginApi, initialValue?: string) { api.ui.dialog.replace( - () => , + () => , () => 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 +754,7 @@ const tui: TuiPlugin = async (api) => { category: "Altimate", namespace: "palette", run() { - showCreate(api) + showCreate(api, currentFilter().trim() || undefined) }, }, { @@ -659,7 +764,7 @@ const tui: TuiPlugin = async (api) => { category: "Altimate", namespace: "palette", run() { - showInstall(api) + showInstall(api, currentFilter().trim() || undefined) }, }, ], diff --git a/packages/opencode/test/altimate/skill-install-classifier.test.ts b/packages/opencode/test/altimate/skill-install-classifier.test.ts new file mode 100644 index 000000000..1a9bd8d71 --- /dev/null +++ b/packages/opencode/test/altimate/skill-install-classifier.test.ts @@ -0,0 +1,92 @@ +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 " +// 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 +// skill-search queries like `dbt/snowflake` — the installer then rejected them as +// "Path not found". These tests pin the classifier's edge cases so the two 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 multi-slash paths that aren't clean owner/repo (search terms, sub-paths)", () => { + // Historically `q.includes("/")` misfired on these — a skill search for "dbt/snowflake" + // would surface an Install option that installSkillDirect then refused. + expect(classifyInstallSource("owner/repo/subpath")).toBeNull() + expect(classifyInstallSource("dbt/snowflake/thing")).toBeNull() + }) + + 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("") + }) +}) diff --git a/packages/opencode/test/upstream/fork-feature-guards.test.ts b/packages/opencode/test/upstream/fork-feature-guards.test.ts index 0582d9fe8..65d402373 100644 --- a/packages/opencode/test/upstream/fork-feature-guards.test.ts +++ b/packages/opencode/test/upstream/fork-feature-guards.test.ts @@ -197,6 +197,24 @@ describe("fork feature presence guards (merge drop detection)", () => { expect(skill).toMatch(/url:\s*"\/skill"[\s\S]*query:\s*\{\s*reload:\s*"true"\s*\}/) }) + // Minimal merge-drop guards for the synthetic "Install " top-of-list option in + // /skills. A silent upstream drop of any of these would remove a user-facing feature + // (ctrl+i can't be trusted — its wire byte 0x09 collides with Tab — so Enter on this + // synthetic row is the only reliable install path). Behavioural coverage of the + // classifier lives in `test/altimate/skill-install-classifier.test.ts`; keep this test + // narrow so cosmetic refactors don't churn it. + test("DialogSkillList wires synthetic install option + prefill plumbing", async () => { + const skill = await read("src/plugin/tui/altimate/skill-ops.tsx") + // Shared classifier + sentinel exist at module scope (installer and list must not drift). + expect(skill).toMatch(/classifyInstallSource/) + expect(skill).toMatch(/INSTALL_ACTION_VALUE/) + // Enter on the synthetic row routes to the install flow with the typed filter text. + expect(skill).toMatch(/item\.value === INSTALL_ACTION_VALUE[\s\S]{0,120}showInstall\(api, filter\(\)/) + // Install/create sub-dialogs receive the typed text as a prefill. + expect(skill).toMatch(/DialogSkillInstall[\s\S]{0,80}initialValue/) + expect(skill).toMatch(/DialogSkillCreate[\s\S]{0,80}initialValue/) + }) + test("re-homed TUI upstream fixes keep prompt/update/child-session behavior", async () => { const promptEnhance = await read("src/plugin/tui/altimate/prompt-enhance.tsx") expect(promptEnhance).toMatch(