diff --git a/.changelog/next/added-issue-4143.md b/.changelog/next/added-issue-4143.md new file mode 100644 index 0000000000..700447523a --- /dev/null +++ b/.changelog/next/added-issue-4143.md @@ -0,0 +1 @@ +- AI Providers editor now warns when a custom CLI/TUI command isn't on the CoS Agent Runner's allowlist — the provider still saves and runs in direct-spawn mode, it just can't be launched by /spawn or /spawn-tui diff --git a/client/src/pages/AIProviders.jsx b/client/src/pages/AIProviders.jsx index 9a35da428e..cc506be0b0 100644 --- a/client/src/pages/AIProviders.jsx +++ b/client/src/pages/AIProviders.jsx @@ -1,8 +1,9 @@ import { useState, useEffect, useCallback } from 'react'; +import { AlertTriangle } from 'lucide-react'; import toast from '../components/ui/Toast'; import * as api from '../services/api'; import socket from '../services/socket'; -import { filterSelectableModels, filterGenerationModels, isEmbeddingModel, mergeModelLists, configuredDefaultIn, localBackendForProvider, modelOptionLabel, providerTypeClass, isTuiProvider, isApiProvider, isProcessProvider, supportsModelRefresh, isGrokBuildCli, isLocalEndpoint, effectiveModelContextWindow } from '../utils/providers'; +import { filterSelectableModels, filterGenerationModels, isEmbeddingModel, mergeModelLists, configuredDefaultIn, localBackendForProvider, modelOptionLabel, providerTypeClass, isTuiProvider, isApiProvider, isProcessProvider, supportsModelRefresh, isGrokBuildCli, isLocalEndpoint, effectiveModelContextWindow, isRunnerAllowedCommand } from '../utils/providers'; import useLocalModels from '../hooks/useLocalModels'; import BrailleSpinner from '../components/BrailleSpinner'; import EmptyState from '../components/EmptyState'; @@ -21,6 +22,10 @@ import CodeReviewDefaultsPanel from '../components/providers/CodeReviewDefaultsP import Modal from '../components/ui/Modal'; import { FormField } from '../components/ui/FormField'; +// One phrasing for "this command isn't on the CoS Agent Runner's allowlist", +// shared by the provider-card badge tooltip and the editor's inline banner. +const RUNNER_NOT_ALLOWED_HINT = 'This command is not on the CoS Agent Runner’s allowlist, so /spawn and /spawn-tui will refuse it. The provider still works everywhere else (direct spawn, chat, pipeline). The allowlist is curated in the PortOS source, not in this form.'; + // Privacy disclosure for the Grok Build CLI/TUI: its harness uploads the entire // working repo to xAI (GCP) as it works unless the user opts out. Shown both on // the provider card and in the create/edit form (before enabling) — see @@ -39,6 +44,9 @@ disable_codebase_upload = true`} export default function AIProviders() { const [providers, setProviders] = useState([]); + // The CoS Agent Runner's exec allowlist, published by GET /api/providers. + // `null` = not fetched yet (or the fetch failed) — never warn from that state. + const [runnerAllowedCommands, setRunnerAllowedCommands] = useState(null); const [statuses, setStatuses] = useState({}); // runtime availability by providerId (separate from the `enabled` toggle) const [recovering, setRecovering] = useState({}); const [activeProviderId, setActiveProviderId] = useState(null); @@ -110,6 +118,12 @@ export default function AIProviders() { setLoadError(false); setProviders(providersData.providers || []); setActiveProviderId(providersData.activeProvider); + // Keep `null` (not an empty array) when an older server omits the field, + // so the "off the allowlist" warning stays silent rather than firing on + // every command. + setRunnerAllowedCommands(Array.isArray(providersData.runnerAllowedCommands) + ? providersData.runnerAllowedCommands + : null); } setApps(appsData); setRuns(runsData.runs || []); @@ -534,6 +548,17 @@ export default function AIProviders() { UNAVAILABLE{statuses[provider.id]?.reason ? ` · ${statuses[provider.id].reason}` : ''} )} + {/* Off the CoS Agent Runner's exec allowlist: the provider still + works for direct spawn, it just can't be launched by /spawn + or /spawn-tui. Informational — never a save-time rejection. */} + {isProcessProvider(provider) && isRunnerAllowedCommand(provider.command, runnerAllowedCommands) === false && ( + + NO AGENT RUNNER + + )} {provider.enabled && statuses[provider.id]?.available === false && ( @@ -748,6 +773,7 @@ export default function AIProviders() { { setShowForm(false); setEditingProvider(null); }} onSave={() => { setShowForm(false); setEditingProvider(null); loadData(); }} /> @@ -757,7 +783,7 @@ export default function AIProviders() { ); } -function ProviderForm({ provider, onClose, onSave, allProviders = [] }) { +function ProviderForm({ provider, onClose, onSave, allProviders = [], runnerAllowedCommands = null }) { const [formData, setFormData] = useState({ name: provider?.name || '', type: provider?.type || 'cli', @@ -952,6 +978,22 @@ function ProviderForm({ provider, onClose, onSave, allProviders = [] }) { required={formData.type === 'cli' || formData.type === 'tui'} className="w-full px-3 py-2 bg-port-bg border border-port-border rounded-lg text-white focus:border-port-accent focus:outline-hidden" /> + {/* Informational only — an off-allowlist command saves fine and runs + fine in direct-spawn mode; it just can't be launched by the CoS + Agent Runner. Rejecting the save would break that valid config. */} + {isRunnerAllowedCommand(formData.command, runnerAllowedCommands) === false && ( + +

+ {formData.command} is not on the CoS Agent Runner’s + command allowlist, so /spawn and{' '} + /spawn-tui will refuse it. Saving is fine — the provider still + runs in direct-spawn mode and everywhere else. +

+

+ Allowlisted: {runnerAllowedCommands.join(', ')} +

+ + )} diff --git a/client/src/pages/AIProviders.test.jsx b/client/src/pages/AIProviders.test.jsx index c7c7cef777..8c5edc8035 100644 --- a/client/src/pages/AIProviders.test.jsx +++ b/client/src/pages/AIProviders.test.jsx @@ -190,3 +190,75 @@ describe('handleAddAllSamples partial failure handling', () => { }); }); +describe('CoS Agent Runner allowlist warning', () => { + const cliProvider = (command) => ({ + id: 'p1', name: 'Custom Agent', type: 'cli', enabled: true, command, args: [], + }); + + beforeEach(() => { + vi.clearAllMocks(); + api.getApps.mockResolvedValue([]); + api.getRuns.mockResolvedValue({ runs: [] }); + api.getProviderStatuses.mockResolvedValue({ providers: {} }); + }); + + it('badges a provider whose command is off the published allowlist', async () => { + api.getProviders.mockResolvedValue({ + providers: [cliProvider('my-custom-agent')], + activeProvider: 'p1', + runnerAllowedCommands: ['claude', 'codex'], + }); + + renderPage(); + + expect(await screen.findByText('NO AGENT RUNNER')).toBeInTheDocument(); + }); + + it('does not badge a provider whose command IS on the allowlist', async () => { + api.getProviders.mockResolvedValue({ + providers: [cliProvider('/usr/local/bin/claude')], + activeProvider: 'p1', + runnerAllowedCommands: ['claude', 'codex'], + }); + + renderPage(); + + expect(await screen.findByText('Custom Agent')).toBeInTheDocument(); + expect(screen.queryByText('NO AGENT RUNNER')).not.toBeInTheDocument(); + }); + + // A server that predates #4143 omits `runnerAllowedCommands`; an unfetchable + // list must read as "can't tell", never as "nothing is allowed". + it('stays silent when the server omits runnerAllowedCommands', async () => { + api.getProviders.mockResolvedValue({ + providers: [cliProvider('my-custom-agent')], + activeProvider: 'p1', + }); + + renderPage(); + + expect(await screen.findByText('Custom Agent')).toBeInTheDocument(); + expect(screen.queryByText('NO AGENT RUNNER')).not.toBeInTheDocument(); + }); + + it('warns inline in the editor as the command is typed, without blocking Save', async () => { + api.getProviders.mockResolvedValue({ + providers: [cliProvider('claude')], + activeProvider: 'p1', + runnerAllowedCommands: ['claude', 'codex'], + }); + + renderPage(); + + fireEvent.click(await screen.findByRole('button', { name: 'Edit' })); + + const commandInput = await screen.findByDisplayValue('claude'); + expect(screen.queryByText(/command allowlist/)).not.toBeInTheDocument(); + + fireEvent.change(commandInput, { target: { value: 'my-custom-agent' } }); + + expect(await screen.findByText(/command allowlist/)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save' })).not.toBeDisabled(); + }); +}); + diff --git a/client/src/utils/README.md b/client/src/utils/README.md index 2a75391061..08f51cb04f 100644 --- a/client/src/utils/README.md +++ b/client/src/utils/README.md @@ -41,7 +41,7 @@ grep -i "what you want to do" client/src/utils/README.md | `urlNormalize` | `isUrl` detection, `normalizeUrl` (optional git/`requireDot` modes), `isHttpUrl` (explicit http(s) only — safe-href check), and `tiktokVideoId` / `tiktokEmbedSrc` (host-anchored TikTok video-id extraction + its Embed Player URL, so a reference embeds without loading TikTok's embed.js). | | `platform` | `isMac` detection and `modKey` (⌘/Ctrl) for keyboard-shortcut display. | | `navWorkingSet` | Recent/pinned nav persistence (`recordVisit`, `togglePin`, `isPinned`) plus `resolveRecentNavEntries` for mapping stored deep links back to their longest matching nav-manifest entry. | -| `providers` | AI-provider type predicates and helpers (`isCliProvider`, `isApiProvider`, `isCodexProvider`, `isAntigravityProvider`, `filterSelectableModels`, `resolveCliEffort` (mirror — what a stored effort actually runs as, so the picker can name a clamped level), `configuredDefaultIn` — the sentinel a provider's catalog carries, so a picker can render an option matching a sentinel-valued tier instead of a blank select — `getProviderTimeout`, `resolveEffectiveProvider` — the provider a record actually runs on (its pin, else the active provider) plus whether it fell back, so a "Default" option can name what it resolves to — `resolveSeriesRunLlm` (mirror of server `seriesLlmOverride.js`: which provider/model a Pipeline **series** run resolves to — per-run override → `series.llm` → active provider) and `providerModelLabel` (the one "Provider / model" phrasing), configured-default sentinels, and the claude/codex/agy thinking-effort levels — `effortLevelsForProvider`, mirror of server `providerModels.js`). | +| `providers` | AI-provider type predicates and helpers (`isCliProvider`, `isApiProvider`, `isCodexProvider`, `isAntigravityProvider`, `filterSelectableModels`, `resolveCliEffort` (mirror — what a stored effort actually runs as, so the picker can name a clamped level), `configuredDefaultIn` — the sentinel a provider's catalog carries, so a picker can render an option matching a sentinel-valued tier instead of a blank select — `getProviderTimeout`, `resolveEffectiveProvider` — the provider a record actually runs on (its pin, else the active provider) plus whether it fell back, so a "Default" option can name what it resolves to — `resolveSeriesRunLlm` (mirror of server `seriesLlmOverride.js`: which provider/model a Pipeline **series** run resolves to — per-run override → `series.llm` → active provider) and `providerModelLabel` (the one "Provider / model" phrasing), configured-default sentinels, and the claude/codex/agy thinking-effort levels — `effortLevelsForProvider`, mirror of server `providerModels.js`). Also `isRunnerAllowedCommand(command, allowedCommands)` — would the CoS Agent Runner (`/spawn`, `/spawn-tui`) accept this command? Mirrors only the *normalization* in `server/cos-runner/allowedCommands.js` (the list itself arrives as `runnerAllowedCommands` on `GET /api/providers`, because the allowlist is an exec boundary and stays hand-curated server-side); returns `null` for "list not fetched / field blank" so a failed fetch never renders a warning. Pinned by `server/cos-runner/allowedCommands.parity.test.js`. | | `layeredIntelligenceReasons` | Canonical gloss for the Layered Intelligence loop's run-outcome reason tokens, shared by the on-demand toast and the durable "Last run" line (`formatLiReason`, `liReasonTone`, `LI_NEUTRAL_REASONS`). | ## Module loading / resilience diff --git a/client/src/utils/providers.js b/client/src/utils/providers.js index cb8ab02e1a..1b0c42d0c1 100644 --- a/client/src/utils/providers.js +++ b/client/src/utils/providers.js @@ -724,6 +724,51 @@ export const enabledApiProviderFilter = (provider) => Boolean(provider?.enabled) */ export const isProcessProvider = (provider) => isCliProvider(provider) || isTuiProvider(provider); +/** + * Base name of a spawn command, normalized the way the CoS Agent Runner's + * allowlist check does before its membership test: strip any directory + * prefix, then a trailing Windows `.exe`. Mirror of `isAllowedCommand`'s + * normalization in `server/cos-runner/allowedCommands.js`, pinned by + * `server/cos-runner/allowedCommands.parity.test.js`. + * + * The server uses `path.basename`, which is platform-specific — on a POSIX + * host a backslash is NOT a separator. This mirror always treats both `/` and + * `\` as separators, so a Windows-style path typed into the editor on a POSIX + * install reads as "allowed" when the server would spawn-time reject it. That + * direction is deliberate: this drives an informational warning, and a false + * *warning* about a path the user's own platform handles fine is worse than a + * missing one for a path shape that platform can't run anyway. + */ +const runnerCommandBaseName = (command) => { + const base = String(command).replace(/[/\\]+$/, '').split(/[/\\]/).pop(); + // Only `.exe` — a `.cmd`/`.bat` npm shim is deliberately NOT stripped, + // matching the server: the spawn path runs with `shell: false` and cannot + // execute a batch shim, so accepting it would only move the failure later. + return base.toLowerCase().endsWith('.exe') ? base.slice(0, -4) : base; +}; + +/** + * Would the CoS Agent Runner (`/spawn`, `/spawn-tui`) accept this command? + * + * `allowedCommands` is the server-published list (`runnerAllowedCommands` on + * `GET /api/providers`) — the client never carries its own copy, because the + * allowlist is the runner's exec boundary and must stay hand-curated + * server-side rather than derived from user-writable provider config. + * + * Returns `null` for "can't tell" — the list hasn't been fetched, or the field + * is still blank — which is distinct from `false` ("fetched, and this command + * is definitely off the list"). Only an explicit `false` should render a + * warning; a failed fetch must not accuse a perfectly good command. + * + * The command is matched UNTRIMMED (past the blank guard), because the editor + * persists it untrimmed too — `'claude '` really would fail the runner's check. + */ +export const isRunnerAllowedCommand = (command, allowedCommands) => { + if (!Array.isArray(allowedCommands) || allowedCommands.length === 0) return null; + if (typeof command !== 'string' || command.trim() === '') return null; + return allowedCommands.includes(runnerCommandBaseName(command)); +}; + /** * Whether `provider` is served by an Ollama daemon rather than its nominal * cloud/CLI backend: the built-in `ollama` API provider itself (id match), an diff --git a/docs/API.md b/docs/API.md index 3e3141db27..7e7aced7e9 100644 --- a/docs/API.md +++ b/docs/API.md @@ -59,7 +59,7 @@ PortOS is designed for personal/developer use on trusted networks. It implements | Method | Endpoint | Description | |--------|----------|-------------| -| GET | `/providers` | List all AI providers | +| GET | `/providers` | List all AI providers. Also returns `runnerAllowedCommands` — the CoS Agent Runner's exec allowlist, read-only, so the editor can warn that a custom `command` won't spawn via `/spawn` / `/spawn-tui`. | | POST | `/providers` | Add new provider | | PUT | `/providers/:id` | Update provider | | DELETE | `/providers/:id` | Delete provider | diff --git a/server/cos-runner/allowedCommands.parity.test.js b/server/cos-runner/allowedCommands.parity.test.js new file mode 100644 index 0000000000..36b67450e1 --- /dev/null +++ b/server/cos-runner/allowedCommands.parity.test.js @@ -0,0 +1,71 @@ +/** + * Parity pin for the AI Providers editor's runner-allowlist warning (#4143). + * + * The client never carries its own copy of the allowlist — it receives it as + * `runnerAllowedCommands` on `GET /api/providers`, so the list itself cannot + * drift. What it DOES mirror is the normalization `isAllowedCommand` applies + * before the membership test (strip directory prefix, strip a trailing + * `.exe`), and that mirror is what this file pins. + */ + +import { describe, it, expect } from 'vitest'; +import { ALLOWED_COMMANDS, isAllowedCommand } from './allowedCommands.js'; +import { isRunnerAllowedCommand } from '../../client/src/utils/providers.js'; + +const allowlist = [...ALLOWED_COMMANDS].sort(); +const sampleAllowed = allowlist[0]; + +describe('runner allowlist client mirror', () => { + // Forward-slash / bare-name / .exe forms only: `path.basename` is + // platform-specific, so a backslash is NOT a separator on a POSIX host while + // the client mirror always treats it as one. That deliberate divergence is + // documented on `runnerCommandBaseName` and asserted separately below. + const cases = [ + sampleAllowed, + `/usr/local/bin/${sampleAllowed}`, + `./${sampleAllowed}`, + `${sampleAllowed}.exe`, + `${sampleAllowed}.EXE`, + `/opt/bin/${sampleAllowed}.exe`, + `${sampleAllowed}/`, + `${sampleAllowed}.cmd`, + `${sampleAllowed}.bat`, + `${sampleAllowed} `, + `my-${sampleAllowed}`, + 'definitely-not-a-real-agent-cli', + '/usr/bin/rm', + 'rm -rf /', + '/', + ]; + + it.each(cases)('agrees with isAllowedCommand for %j', (command) => { + // The client's third state (`null` = list not fetched / field blank) never + // occurs here: a real list is passed and every case is non-blank. + expect(isRunnerAllowedCommand(command, allowlist)).toBe(isAllowedCommand(command)); + }); + + it('reports every shipped allowlist entry as allowed', () => { + for (const command of allowlist) { + expect(isRunnerAllowedCommand(command, allowlist)).toBe(true); + } + }); + + it('returns null (not false) when the allowlist has not been fetched', () => { + expect(isRunnerAllowedCommand(sampleAllowed, null)).toBeNull(); + expect(isRunnerAllowedCommand(sampleAllowed, undefined)).toBeNull(); + expect(isRunnerAllowedCommand(sampleAllowed, [])).toBeNull(); + }); + + it('returns null (not false) for a blank command field', () => { + expect(isRunnerAllowedCommand('', allowlist)).toBeNull(); + expect(isRunnerAllowedCommand(' ', allowlist)).toBeNull(); + expect(isRunnerAllowedCommand(null, allowlist)).toBeNull(); + }); + + it('treats a backslash as a separator even where POSIX path.basename would not', () => { + // Client-only behavior, by design: this drives an informational warning, + // and a false warning about a Windows path on a Windows install would be + // worse than a missing one for a path shape POSIX cannot spawn anyway. + expect(isRunnerAllowedCommand(`C:\\bin\\${sampleAllowed}.exe`, allowlist)).toBe(true); + }); +}); diff --git a/server/routes/providers.js b/server/routes/providers.js index 84c10f2e76..9353af9849 100644 --- a/server/routes/providers.js +++ b/server/routes/providers.js @@ -3,6 +3,24 @@ import { asyncHandler, ServerError } from '../lib/errorHandler.js'; import { testVision, runVisionTestSuite, checkVisionHealth } from '../services/visionTest.js'; import { providerSchema, providerActiveSchema, validate } from '../lib/aiToolkit/validation.js'; import { withRefreshCapability } from '../lib/aiToolkit/internal/modelFetchers.js'; +import { ALLOWED_COMMANDS } from '../cos-runner/allowedCommands.js'; + +/** + * The CoS Agent Runner's exec allowlist, published read-only so the AI + * Providers editor can warn that a custom `command` will never spawn via + * `/spawn` / `/spawn-tui` (#4143). Direct (non-runner) spawn does NOT consult + * this list, so an off-list command is a legitimate config — informational + * only, never a save-time rejection. + * + * Published as a list rather than a per-provider `runnerAllowed` flag on + * purpose: the editor has to warn about the command the user is TYPING, which + * has no persisted provider to decorate. Sorted so the payload is stable. + * + * This is a one-way read: the allowlist stays hand-curated in + * `cos-runner/allowedCommands.js` and is never derived from the user-writable + * `data/providers.json`, or a config write could choose the exec target. + */ +const RUNNER_ALLOWED_COMMANDS = [...ALLOWED_COMMANDS].sort(); /** * Sanitize a provider object for client responses. @@ -59,7 +77,8 @@ export function createPortOSProviderRoutes(aiToolkit) { const data = await providerService.getAllProviders(); res.json({ activeProvider: data.activeProvider, - providers: data.providers.map(presentProvider) + providers: data.providers.map(presentProvider), + runnerAllowedCommands: RUNNER_ALLOWED_COMMANDS }); }));