From 01973bb4b064ea2e47982b6af327e0d377da6285 Mon Sep 17 00:00:00 2001 From: Dan Shapiro Date: Fri, 27 Mar 2026 17:51:27 -0700 Subject: [PATCH 1/2] fix(mcp): validate params and reject unknown parameters with helpful hints When called with unrecognized params (e.g. new-tab with url), the MCP tool now returns an error listing valid parameters. A specific hint suggests open-browser when url is passed to new-tab. Help text updated to clarify the distinction between new-tab and open-browser, with a new playbook for opening URLs. --- server/mcp/freshell-tool.ts | 87 +++++++++++++++++++++- test/unit/server/mcp/freshell-tool.test.ts | 61 +++++++++++++++ 2 files changed, 145 insertions(+), 3 deletions(-) diff --git a/server/mcp/freshell-tool.ts b/server/mcp/freshell-tool.ts index 39d7bbbb..b6d818cb 100644 --- a/server/mcp/freshell-tool.ts +++ b/server/mcp/freshell-tool.ts @@ -230,6 +230,74 @@ async function handleDisplay(format: string, target?: string): Promise { .replace(/#\{([^}]+)\}/g, (_match, token) => values[token] ?? 'N/A') } +// --------------------------------------------------------------------------- +// Parameter validation: known params per action +// --------------------------------------------------------------------------- + +const ACTION_PARAMS: Record = { + 'new-tab': { required: [], optional: ['name', 'mode', 'shell', 'cwd', 'browser', 'editor', 'resume', 'prompt'] }, + 'list-tabs': { required: [], optional: [] }, + 'select-tab': { required: ['target'], optional: [] }, + 'kill-tab': { required: ['target'], optional: [] }, + 'rename-tab': { required: ['target', 'name'], optional: [] }, + 'has-tab': { required: ['target'], optional: [] }, + 'next-tab': { required: [], optional: [] }, + 'prev-tab': { required: [], optional: [] }, + 'split-pane': { required: [], optional: ['target', 'direction', 'mode', 'shell', 'cwd', 'browser', 'editor'] }, + 'list-panes': { required: [], optional: ['target'] }, + 'select-pane': { required: ['target'], optional: [] }, + 'rename-pane': { required: ['target', 'name'], optional: [] }, + 'kill-pane': { required: ['target'], optional: [] }, + 'resize-pane': { required: ['target'], optional: ['x', 'y', 'sizes'] }, + 'swap-pane': { required: ['target', 'with'], optional: [] }, + 'respawn-pane': { required: ['target'], optional: ['mode', 'shell', 'cwd'] }, + 'send-keys': { required: [], optional: ['target', 'keys', 'literal'] }, + 'capture-pane': { required: [], optional: ['target', 'S', 'J', 'e'] }, + 'wait-for': { required: [], optional: ['target', 'pattern', 'stable', 'exit', 'prompt', 'timeout'] }, + 'run': { required: ['command'], optional: ['capture', 'detached', 'timeout', 'name', 'cwd'] }, + 'summarize': { required: [], optional: ['target'] }, + 'display': { required: [], optional: ['target', 'format'] }, + 'list-terminals': { required: [], optional: [] }, + 'attach': { required: ['target', 'terminalId'], optional: [] }, + 'open-browser': { required: ['url'], optional: ['name'] }, + 'navigate': { required: ['target', 'url'], optional: [] }, + 'screenshot': { required: ['scope'], optional: ['target', 'name'] }, + 'list-sessions': { required: [], optional: [] }, + 'search-sessions': { required: ['query'], optional: [] }, + 'lan-info': { required: [], optional: [] }, + 'health': { required: [], optional: [] }, + 'help': { required: [], optional: [] }, +} + +const COMMON_CONFUSIONS: Record> = { + 'new-tab': { + url: "Unknown parameter 'url' for action 'new-tab'. Did you mean to use 'open-browser' to open a URL? Or pass the URL as 'browser' to create a browser pane in a new tab.", + }, +} + +function validateParams(action: string, params: Record | undefined): { error: string; hint: string } | null { + const schema = ACTION_PARAMS[action] + if (!schema) return null + + const allValid = [...schema.required, ...schema.optional] + const givenKeys = Object.keys(params || {}) + const unknownKeys = givenKeys.filter(k => !allValid.includes(k)) + + if (unknownKeys.length === 0) return null + + const specificHint = COMMON_CONFUSIONS[action] + for (const key of unknownKeys) { + if (specificHint?.[key]) { + return { error: specificHint[key], hint: `Valid params for '${action}': ${allValid.join(', ') || '(none)'}` } + } + } + + return { + error: `Unknown parameter${unknownKeys.length > 1 ? 's' : ''} '${unknownKeys.join("', '")}' for action '${action}'.`, + hint: `Valid params: ${allValid.join(', ') || '(none)'}`, + } +} + // --------------------------------------------------------------------------- // Action router // --------------------------------------------------------------------------- @@ -239,9 +307,10 @@ const HELP_TEXT = `Freshell MCP tool -- full reference ## Command reference Tab commands: - new-tab Create a tab. Params: name?, mode?, shell?, cwd?, browser?, editor?, resume?, prompt? + new-tab Create a tab with a terminal pane (default). Params: name?, mode?, shell?, cwd?, browser?, editor?, resume?, prompt? mode values: shell (default), claude, codex, kimi, opencode, or any supported CLI. prompt: text to send to the terminal after creation (via send-keys with literal mode). + To open a URL in a browser pane, use 'open-browser' instead. list-tabs List all tabs. Returns { tabs: [...], activeTabId }. select-tab Activate a tab. Params: target (tab ID or title) kill-tab Close a tab. Params: target @@ -279,8 +348,9 @@ Terminal I/O: attach Attach a terminal to a pane. Params: target (pane ID), terminalId Browser/navigation: - open-browser Open a URL in a new browser tab. Params: url, name? - navigate Navigate a browser pane to a URL. Params: target (pane ID), url + open-browser Open a URL in a new browser tab to display web pages or images. + Params: url (required), name? (optional) + navigate Navigate an existing browser pane to a URL. Params: target (pane ID), url Screenshot: screenshot Take a screenshot. Params: scope (pane|tab|view), target?, name? (defaults to "screenshot") @@ -339,6 +409,15 @@ Meta: // Or split an existing pane freshell({ action: "split-pane", params: { editor: "/absolute/path/to/file.ts" } }) +## Playbook: open a URL in a browser pane + + // Open a URL in a new browser tab (correct way) + freshell({ action: "open-browser", params: { url: "https://example.com", name: "My Page" } }) + + // Navigate an existing browser pane to a different URL + freshell({ action: "navigate", params: { target: paneId, url: "https://other.com" } }) + + ## Screenshot guidance - Use a dedicated canary tab when validating screenshot behavior so live project panes are not contaminated. @@ -386,6 +465,8 @@ export async function executeAction( params?: Record, ): Promise { try { + const paramError = validateParams(action, params) + if (paramError) return paramError return await routeAction(action, params) } catch (err: any) { if (err instanceof MissingParamError) { diff --git a/test/unit/server/mcp/freshell-tool.test.ts b/test/unit/server/mcp/freshell-tool.test.ts index fb5d5ca6..14d192ad 100644 --- a/test/unit/server/mcp/freshell-tool.test.ts +++ b/test/unit/server/mcp/freshell-tool.test.ts @@ -1137,3 +1137,64 @@ describe('executeAction -- error handling', () => { expect(result.hint).toContain('Freshell') }) }) + +describe('executeAction -- parameter validation', () => { + it('new-tab with url param returns error suggesting open-browser', async () => { + const result = await executeAction('new-tab', { url: 'https://example.com' }) + expect(result).toHaveProperty('error') + expect(result.error).toContain('open-browser') + expect(result).toHaveProperty('hint') + expect(result.hint).toContain('Valid params') + }) + + it('new-tab with url and name returns error about url', async () => { + const result = await executeAction('new-tab', { url: 'https://example.com', name: 'My Tab' }) + expect(result).toHaveProperty('error') + expect(result.error).toContain('url') + expect(result.error).toContain('open-browser') + }) + + it('unknown param on any action returns error listing valid params', async () => { + const result = await executeAction('screenshot', { scope: 'pane', badparam: 'value' }) + expect(result).toHaveProperty('error') + expect(result.error).toContain('badparam') + expect(result).toHaveProperty('hint') + expect(result.hint).toContain('Valid params') + }) + + it('multiple unknown params returns error listing all of them', async () => { + const result = await executeAction('health', { foo: 1, bar: 2 }) + expect(result).toHaveProperty('error') + expect(result.error).toContain('foo') + expect(result.error).toContain('bar') + expect(result.hint).toContain('(none)') + }) + + it('valid params pass through without error', async () => { + mockClient.post.mockResolvedValue({ id: 't1' }) + const result = await executeAction('new-tab', { name: 'Work', mode: 'claude' }) + expect(result).not.toHaveProperty('error') + expect(mockClient.post).toHaveBeenCalledWith('/api/tabs', expect.objectContaining({ name: 'Work', mode: 'claude' })) + }) + + it('action without params schema (tmux alias) skips validation', async () => { + mockClient.post.mockResolvedValue({ id: 't1' }) + const result = await executeAction('new-window', { unknownParam: 'value' }) + // new-window is a tmux alias, not in ACTION_PARAMS directly, so no validation + expect(result).not.toHaveProperty('error') + }) + + it('empty params on paramless action succeeds', async () => { + mockClient.get.mockResolvedValue({ ok: true }) + const result = await executeAction('health') + expect(result).not.toHaveProperty('error') + }) + + it('help text mentions open-browser for URLs', async () => { + const result = await executeAction('help') + const text = typeof result === 'string' ? result : JSON.stringify(result) + expect(text).toContain("use 'open-browser'") + expect(text).toContain('open-browser') + expect(text).toContain('Playbook: open a URL') + }) +}) From 927bb35ab1e748893a49b992a4db28359932224c Mon Sep 17 00:00:00 2001 From: Matt Leaverton Date: Mon, 30 Mar 2026 11:17:56 -0500 Subject: [PATCH 2/2] fix: validate params through tmux aliases Tmux aliases (new-window, split-window, etc.) now validate params against the resolved action before routing. Previously they bypassed validateParams entirely. Co-Authored-By: Claude Opus 4.6 (1M context) --- server/mcp/freshell-tool.ts | 7 ++++++- test/unit/server/mcp/freshell-tool.test.ts | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/server/mcp/freshell-tool.ts b/server/mcp/freshell-tool.ts index b6d818cb..0c048179 100644 --- a/server/mcp/freshell-tool.ts +++ b/server/mcp/freshell-tool.ts @@ -750,8 +750,13 @@ async function routeAction( // For screenshot aliases, inject scope from the alias name if (action.startsWith('screenshot-')) { const scope = action.replace('screenshot-', '') - return routeAction(resolved, { ...params, scope }) + const mergedParams = { ...params, scope } + const aliasParamError = validateParams(resolved, mergedParams) + if (aliasParamError) return aliasParamError + return routeAction(resolved, mergedParams) } + const aliasParamError = validateParams(resolved, params) + if (aliasParamError) return aliasParamError return routeAction(resolved, params) } return { diff --git a/test/unit/server/mcp/freshell-tool.test.ts b/test/unit/server/mcp/freshell-tool.test.ts index 14d192ad..cf3487be 100644 --- a/test/unit/server/mcp/freshell-tool.test.ts +++ b/test/unit/server/mcp/freshell-tool.test.ts @@ -1177,11 +1177,23 @@ describe('executeAction -- parameter validation', () => { expect(mockClient.post).toHaveBeenCalledWith('/api/tabs', expect.objectContaining({ name: 'Work', mode: 'claude' })) }) - it('action without params schema (tmux alias) skips validation', async () => { - mockClient.post.mockResolvedValue({ id: 't1' }) + it('tmux alias validates params against the resolved action', async () => { const result = await executeAction('new-window', { unknownParam: 'value' }) - // new-window is a tmux alias, not in ACTION_PARAMS directly, so no validation + expect(result).toHaveProperty('error') + expect(result.error).toContain('unknownParam') + }) + + it('tmux alias with valid params routes through', async () => { + mockClient.post.mockResolvedValue({ id: 't1' }) + const result = await executeAction('new-window', { name: 'Work', mode: 'claude' }) expect(result).not.toHaveProperty('error') + expect(mockClient.post).toHaveBeenCalledWith('/api/tabs', expect.objectContaining({ name: 'Work', mode: 'claude' })) + }) + + it('tmux alias surfaces common confusion hints', async () => { + const result = await executeAction('new-window', { url: 'https://example.com' }) + expect(result).toHaveProperty('error') + expect(result.error).toContain('open-browser') }) it('empty params on paramless action succeeds', async () => {