diff --git a/packages/cli/src/ui/components/SuggestionsDisplay.test.tsx b/packages/cli/src/ui/components/SuggestionsDisplay.test.tsx index 023d1080907..15469669337 100644 --- a/packages/cli/src/ui/components/SuggestionsDisplay.test.tsx +++ b/packages/cli/src/ui/components/SuggestionsDisplay.test.tsx @@ -71,6 +71,52 @@ describe('SuggestionsDisplay', () => { expect(output.split('\n').length).toBeLessThanOrEqual(2); }); + it('keeps the full MCP resource reference on one line and truncates its description (reverse mode)', () => { + // Two resources that share a long `server:scheme://` prefix and differ only + // in the tail — the discriminating part. The reference (label) must stay + // intact on a single line so the two rows are distinguishable; the + // description yields the width and truncates instead. + const a = 'asys-mcp-http:asight://skills/ppu_bubble_analysis'; + const b = 'asys-mcp-http:asight://skills/ppu_operator_performance'; + const { lastFrame } = render( + , + ); + + const output = lastFrame() ?? ''; + // Each full reference appears verbatim (the tail is NOT truncated away), so + // the two rows can be told apart. + expect(output).toContain(a); + expect(output).toContain(b); + // The description is what gets cut — its tail must be gone, ellipsized. + expect(output).toContain('…'); + expect(output).not.toContain('loaded trace report.'); + // One visible row per suggestion: the label is not wrapped onto extra lines. + expect( + output.split('\n').filter((l) => l.includes('asys-mcp-http:')), + ).toHaveLength(2); + }); + it('collapses newlines in multi-line descriptions so a row stays one line', () => { const description = [ 'First line of the skill description.', diff --git a/packages/cli/src/ui/components/SuggestionsDisplay.tsx b/packages/cli/src/ui/components/SuggestionsDisplay.tsx index 1fe417cd821..d8f4600905d 100644 --- a/packages/cli/src/ui/components/SuggestionsDisplay.tsx +++ b/packages/cli/src/ui/components/SuggestionsDisplay.tsx @@ -55,6 +55,13 @@ interface SuggestionsDisplayProps { export const MAX_SUGGESTIONS_TO_SHOW = 8; export { MAX_WIDTH }; +/** + * In @-mention mode a wide resource-reference column must still leave the row's + * description at least this many columns, so an unusually long reference can't + * shrink the description away entirely. + */ +const MIN_DESCRIPTION_WIDTH = 12; + /** * Collapse all runs of whitespace (including newlines from multi-line * SKILL.md/command descriptions) into single spaces so a description renders @@ -101,8 +108,25 @@ export function SuggestionsDisplay({ const maxLabelLength = Math.max( ...suggestions.map((s) => getFullLabel(s).length), ); - const commandColumnWidth = - mode === 'slash' ? Math.min(maxLabelLength, Math.floor(width * 0.5)) : 0; + // Width of the left label column. In slash mode every row shares one + // half-width command column. In @-mention (reverse) mode only rows WITH a + // description (MCP resources/servers) share a column — sized to the longest + // such reference so the references stay intact and their descriptions line + // up, capped so the description keeps a minimum readable width — while plain + // file rows (no description) keep the full row width. The reference takes + // priority over its description, which truncates. + const describedLabelLengths = suggestions + .filter((s) => s.description) + .map((s) => getFullLabel(s).length); + const labelColumnWidth = + mode === 'slash' + ? Math.min(maxLabelLength, Math.floor(width * 0.5)) + : describedLabelLengths.length > 0 + ? Math.min( + Math.max(...describedLabelLengths), + Math.max(width - MIN_DESCRIPTION_WIDTH - 2, 1), + ) + : 0; return ( @@ -117,7 +141,7 @@ export function SuggestionsDisplay({ const isLong = displayLabel.length >= MAX_WIDTH; const expansionIndicatorWidth = isActive && isLong ? 3 : 0; const descriptionColumnWidth = Math.max( - width - commandColumnWidth - 2 - expansionIndicatorWidth, + width - labelColumnWidth - 2 - expansionIndicatorWidth, 1, ); const labelElement = ( @@ -133,8 +157,8 @@ export function SuggestionsDisplay({ return ( diff --git a/packages/cli/src/ui/hooks/useAtCompletion.test.ts b/packages/cli/src/ui/hooks/useAtCompletion.test.ts index da4a7986203..407b5f1e080 100644 --- a/packages/cli/src/ui/hooks/useAtCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useAtCompletion.test.ts @@ -959,4 +959,166 @@ describe('useAtCompletion', () => { expect(values).toContain('my-notes.txt'); }); }); + + describe('Global MCP resource completion', () => { + it('matches resources globally for a bare @ with no server prefix', async () => { + testRootDir = await createTmpDir({ 'unrelated.txt': '' }); + const resourceConfig = { + ...mockConfig, + getMcpServers: () => ({ 'asys-mcp-http': {} }), + getResourceRegistry: () => ({ + getResourcesByServer: () => [], + getAllResources: () => [ + { + uri: 'asight://skills/ppu_bubble', + name: 'bubble', + serverName: 'asys-mcp-http', + }, + { + uri: 'asight://skills/ppu_op', + name: 'op', + serverName: 'asys-mcp-http', + }, + ], + }), + } as unknown as Config; + + const { result } = renderHook(() => + useTestHarnessForAtCompletion( + true, + 'asight', + resourceConfig, + testRootDir, + ), + ); + + await waitFor(() => { + expect(result.current.suggestions.length).toBeGreaterThan(0); + }); + // 'asight' is not a configured server name and carries no ':' — yet both + // resources match by URI prefix and are injected as @server:uri. + const values = result.current.suggestions.map((s) => s.value); + expect(values).toContain('asys-mcp-http:asight://skills/ppu_bubble'); + expect(values).toContain('asys-mcp-http:asight://skills/ppu_op'); + }); + + it('matches a resource globally by its friendly name/title', async () => { + testRootDir = await createTmpDir({ 'file.txt': '' }); + const resourceConfig = { + ...mockConfig, + getMcpServers: () => ({ demo: {} }), + getResourceRegistry: () => ({ + getResourcesByServer: () => [], + getAllResources: () => [ + { + uri: 'file:///x/spec.md', + name: 'spec', + title: 'Project Spec', + serverName: 'demo', + }, + ], + }), + } as unknown as Config; + + const { result } = renderHook(() => + useTestHarnessForAtCompletion( + true, + 'Project', + resourceConfig, + testRootDir, + ), + ); + + await waitFor(() => { + expect(result.current.suggestions.length).toBeGreaterThan(0); + }); + expect(result.current.suggestions.map((s) => s.value)).toContain( + 'demo:file:///x/spec.md', + ); + }); + + it('prepends globally-matched resources before file results', async () => { + // A file AND a resource both match 'doc'. + testRootDir = await createTmpDir({ 'doc.txt': '' }); + const resourceConfig = { + ...mockConfig, + getMcpServers: () => ({ demo: {} }), + getResourceRegistry: () => ({ + getResourcesByServer: () => [], + getAllResources: () => [ + { uri: 'doc://readme', name: 'r', serverName: 'demo' }, + ], + }), + } as unknown as Config; + + const { result } = renderHook(() => + useTestHarnessForAtCompletion(true, 'doc', resourceConfig, testRootDir), + ); + + await waitFor(() => { + expect(result.current.suggestions.length).toBeGreaterThan(1); + }); + const values = result.current.suggestions.map((s) => s.value); + const resIdx = values.indexOf('demo:doc://readme'); + const fileIdx = values.indexOf('doc.txt'); + expect(resIdx).toBeGreaterThanOrEqual(0); + expect(fileIdx).toBeGreaterThanOrEqual(0); + // Resources come first so a file flood can't bury them. + expect(resIdx).toBeLessThan(fileIdx); + }); + + it('does not surface global resources for the empty @ trigger (files only)', async () => { + testRootDir = await createTmpDir({ 'file.txt': '' }); + const resourceConfig = { + ...mockConfig, + getMcpServers: () => ({ demo: {} }), + getResourceRegistry: () => ({ + getResourcesByServer: () => [], + getAllResources: () => [ + { uri: 'res://x', name: 'x', serverName: 'demo' }, + ], + }), + } as unknown as Config; + + const { result } = renderHook(() => + useTestHarnessForAtCompletion(true, '', resourceConfig, testRootDir), + ); + + await waitFor(() => { + expect(result.current.suggestions.length).toBeGreaterThan(0); + }); + const values = result.current.suggestions.map((s) => s.value); + expect(values).toContain('file.txt'); + expect(values).not.toContain('demo:res://x'); + }); + + it('does not surface global resources in an untrusted folder', async () => { + testRootDir = await createTmpDir({ 'file.txt': '' }); + const resourceConfig = { + ...mockConfig, + isTrustedFolder: () => false, + getMcpServers: () => ({ demo: {} }), + getResourceRegistry: () => ({ + getResourcesByServer: () => [], + getAllResources: () => [ + { uri: 'asight://secret', name: 's', serverName: 'demo' }, + ], + }), + } as unknown as Config; + + const { result } = renderHook(() => + useTestHarnessForAtCompletion( + true, + 'asight', + resourceConfig, + testRootDir, + ), + ); + + await new Promise((r) => setTimeout(r, 300)); + expect(result.current.suggestions.map((s) => s.value)).not.toContain( + 'demo:asight://secret', + ); + }); + }); }); diff --git a/packages/cli/src/ui/hooks/useAtCompletion.ts b/packages/cli/src/ui/hooks/useAtCompletion.ts index 49bfa1365d1..b83ec9606bd 100644 --- a/packages/cli/src/ui/hooks/useAtCompletion.ts +++ b/packages/cli/src/ui/hooks/useAtCompletion.ts @@ -13,24 +13,87 @@ import { matchMcpServerPrefix, buildMcpResourceRef } from './mcpResourceRef.js'; import { t } from '../../i18n/index.js'; /** - * `@server:uri` MCP resource completion. Returns suggestions when `pattern` - * is of the form `:` and `` is a configured MCP - * server (so a plain file path containing ':' is never hijacked); returns - * `null` otherwise to let the caller fall through to filesystem search. + * Resource → suggestion input shape. Structurally satisfied by core's + * `DiscoveredMCPResource` (typed locally to avoid a core import / rebuild). + */ +type CompletableResource = { + uri: string; + name?: string; + title?: string; + serverName: string; +}; + +/** + * Lower rank = better match; `Infinity` means no match (filtered out). Shared by + * the per-server and global resource paths so their ranking can't drift, best + * first: URI prefix, then friendly-name prefix, then URI substring, then name + * substring. `query` must already be lower-cased. + */ +function rankResourceMatch( + uri: string, + friendly: string, + query: string, +): number { + if (uri.startsWith(query)) return 0; + if (friendly.startsWith(query)) return 1; + if (uri.includes(query)) return 2; + if (friendly.includes(query)) return 3; + return Infinity; +} + +/** + * Rank `resources` against `query` (already lower-cased) and project the matches + * onto completion suggestions, best first (ties break by the canonical + * `@server:uri` reference for a stable order). * - * The partial after the colon is matched case-INsensitively against each - * resource's URI AND its friendly name/title (the same `title || name` the - * `/mcp` dialog shows), so a user who only remembers the human-readable name — - * not the URI — still gets completions. An empty partial matches every - * resource (`''` is a prefix of every string). Ranking, best first: URI prefix, - * then name/title prefix, then URI substring, then name/title substring; ties - * break alphabetically by URI for a stable order. + * The partial is matched case-INsensitively against each resource's URI AND its + * friendly name/title (the same `title || name` the `/mcp` dialog shows), so a + * user who only remembers the human-readable name — not the URI — still gets + * completions. An empty `query` matches every resource (`''` is a substring of + * every string); callers gate that where it is unwanted. * * The injected `value` is always the canonical `@server:uri` reference (the * friendly name is not a referenceable identifier); the name rides along as the - * suggestion `description` so a name-only match is self-explanatory. The - * resource list comes from the post-discovery `ResourceRegistry`, so an empty - * result before discovery completes simply shows no suggestions. + * suggestion `description` only when it adds information beyond the URI (mirrors + * the `/mcp` resource list, which dims a redundant name). + */ +function rankResourcesToSuggestions( + resources: CompletableResource[], + query: string, +): Suggestion[] { + return resources + .map((resource) => { + const friendly = resource.title || resource.name || ''; + return { + resource, + friendly, + ref: buildMcpResourceRef(resource.serverName, resource.uri), + rank: rankResourceMatch( + resource.uri.toLowerCase(), + friendly.toLowerCase(), + query, + ), + }; + }) + .filter((m) => m.rank !== Infinity) + .sort((a, b) => a.rank - b.rank || a.ref.localeCompare(b.ref)) + .slice(0, MAX_SUGGESTIONS_TO_SHOW * 3) + .map((m) => ({ + label: m.ref, + value: m.ref, + description: + m.friendly && m.friendly !== m.resource.uri ? m.friendly : undefined, + isDirectory: false, + })); +} + +/** + * `@server:uri` per-server MCP resource completion. Returns suggestions when + * `pattern` is of the form `:` and `` is a configured + * MCP server (so a plain file path containing ':' is never hijacked); returns + * `null` otherwise to let the caller fall through to filesystem search (and the + * global path below). The resource list comes from the post-discovery + * `ResourceRegistry`, so an empty result before discovery simply shows nothing. */ function getMcpResourceSuggestions( config: Config | undefined, @@ -46,46 +109,33 @@ function getMcpResourceSuggestions( const mcpServers = config.getMcpServers?.() || {}; const match = matchMcpServerPrefix(pattern, Object.keys(mcpServers)); if (!match) return null; - const serverName = match.serverName; - const query = match.rest.toLowerCase(); - - // Lower rank = better match; `Infinity` means no match and is filtered out. - const rankOf = (uri: string, friendly: string): number => { - if (uri.startsWith(query)) return 0; - if (friendly.startsWith(query)) return 1; - if (uri.includes(query)) return 2; - if (friendly.includes(query)) return 3; - return Infinity; - }; - const resources = - config.getResourceRegistry?.()?.getResourcesByServer(serverName) ?? []; - const matches = resources - .map((resource) => { - const friendly = resource.title || resource.name || ''; - return { - resource, - friendly, - rank: rankOf(resource.uri.toLowerCase(), friendly.toLowerCase()), - }; - }) - .filter((m) => m.rank !== Infinity) - .sort( - (a, b) => a.rank - b.rank || a.resource.uri.localeCompare(b.resource.uri), - ); - - return matches.slice(0, MAX_SUGGESTIONS_TO_SHOW * 3).map((m) => { - const ref = buildMcpResourceRef(serverName, m.resource.uri); - return { - label: ref, - value: ref, - // Only surface the friendly name when it adds information beyond the URI - // (mirrors the `/mcp` resource list, which dims a redundant name). - description: - m.friendly && m.friendly !== m.resource.uri ? m.friendly : undefined, - isDirectory: false, - }; - }); + config.getResourceRegistry?.()?.getResourcesByServer(match.serverName) ?? + []; + return rankResourcesToSuggestions(resources, match.rest.toLowerCase()); +} + +/** + * Bare `@` GLOBAL MCP resource completion. When the partial carries no + * `:` prefix (so `getMcpResourceSuggestions` doesn't apply), match it + * against EVERY discovered resource across all servers, so a user can pull up a + * resource by a memorable fragment of its URI/name without first recalling which + * server exposes it. The injected `value` is still the canonical `@server:uri`. + * + * Returns `[]` (never `null`): like `getMcpServerSuggestions`, these are + * surfaced ALONGSIDE the filesystem results, never replacing them. The empty + * partial (bare `@`) is intentionally excluded — every resource would otherwise + * match — keeping the bare `@` a files-only view. + */ +function getGlobalMcpResourceSuggestions( + config: Config | undefined, + pattern: string, +): Suggestion[] { + if (!config) return []; + if (config.isTrustedFolder?.() === false) return []; + if (pattern.length === 0) return []; + const resources = config.getResourceRegistry?.()?.getAllResources?.() ?? []; + return rankResourcesToSuggestions(resources, pattern.toLowerCase()); } /** @@ -344,20 +394,29 @@ export function useAtCompletion(props: UseAtCompletionProps): void { return; } - // No `:` selected yet — offer matching MCP servers (that expose - // resources) ALONGSIDE the filesystem results so the user can discover a - // server without knowing a resource URI up front. Computed synchronously - // and prepended below; never hides files. + // No `:` prefix yet — offer, ALONGSIDE the filesystem results + // (never hiding files): matching MCP servers (discovery, so the user can + // drill in without knowing a URI) AND resources matched globally by + // URI/name across all servers. Both computed synchronously and prepended + // below. const serverSuggestions = getMcpServerSuggestions(config, state.pattern); + const globalResourceSuggestions = getGlobalMcpResourceSuggestions( + config, + state.pattern, + ); + const mcpSuggestions = [ + ...serverSuggestions, + ...globalResourceSuggestions, + ]; if (!fileSearch.current) { - // File index not ready yet; still surface any server matches so - // discovery doesn't have to wait on the crawler. - if (serverSuggestions.length > 0) { + // File index not ready yet; still surface any MCP matches so they + // don't have to wait on the crawler. + if (mcpSuggestions.length > 0) { if (slowSearchTimer.current) { clearTimeout(slowSearchTimer.current); } - dispatch({ type: 'SEARCH_SUCCESS', payload: serverSuggestions }); + dispatch({ type: 'SEARCH_SUCCESS', payload: mcpSuggestions }); } return; } @@ -397,14 +456,14 @@ export function useAtCompletion(props: UseAtCompletionProps): void { })); dispatch({ type: 'SEARCH_SUCCESS', - payload: [...serverSuggestions, ...fileSuggestions], + payload: [...mcpSuggestions, ...fileSuggestions], }); } catch (error) { if (!(error instanceof Error && error.name === 'AbortError')) { - // A file-search failure shouldn't swallow server matches we already + // A file-search failure shouldn't swallow MCP matches we already // have; show those rather than dropping to an error state. - if (serverSuggestions.length > 0) { - dispatch({ type: 'SEARCH_SUCCESS', payload: serverSuggestions }); + if (mcpSuggestions.length > 0) { + dispatch({ type: 'SEARCH_SUCCESS', payload: mcpSuggestions }); } else { dispatch({ type: 'ERROR' }); }