diff --git a/packages/gateway/src/routes/public/agent.ts b/packages/gateway/src/routes/public/agent.ts index 7cf06129a..5af75c8e8 100644 --- a/packages/gateway/src/routes/public/agent.ts +++ b/packages/gateway/src/routes/public/agent.ts @@ -172,42 +172,76 @@ const AgentIdParamSchema = z.object({ // Validation Helpers // ============================================================================= -function validateDomainPattern(pattern: string): string | null { +/** + * Structured validation error. Each entry identifies the offending field path + * (e.g. `networkConfig.allowedDomains[0]`, `mcpServers.foo.url`) plus a human + * message. Callers format these via `formatValidationError` at the response + * boundary so the wire representation stays a single flat string. + */ +type ValidationError = { path: string; message: string }[]; + +/** + * Collapse a structured ValidationError into the flat `error` string the + * public API contract expects. We surface the first failure's message + * verbatim — historically these validators short-circuited on the first + * error and callers displayed only that message, so preserving this keeps + * the HTTP response body unchanged. + */ +function formatValidationError(err: ValidationError): string { + const first = err[0]; + if (!first) return "Validation failed"; + return first.message; +} + +function validateDomainPattern(pattern: string, path: string): ValidationError { if (!pattern || typeof pattern !== "string") { - return "Domain pattern must be a non-empty string"; + return [{ path, message: "Domain pattern must be a non-empty string" }]; } const trimmed = pattern.trim().toLowerCase(); - if (trimmed === "*") return "Bare wildcard '*' is not allowed"; - if (trimmed.includes("://")) - return `Domain pattern cannot contain protocol: ${pattern}`; - if (trimmed.includes("/")) - return `Domain pattern cannot contain path: ${pattern}`; + if (trimmed === "*") { + return [{ path, message: "Bare wildcard '*' is not allowed" }]; + } + if (trimmed.includes("://")) { + return [ + { path, message: `Domain pattern cannot contain protocol: ${pattern}` }, + ]; + } + if (trimmed.includes("/")) { + return [ + { path, message: `Domain pattern cannot contain path: ${pattern}` }, + ]; + } if (trimmed.includes(":") && !trimmed.includes("[")) { - return `Domain pattern cannot contain port: ${pattern}`; + return [ + { path, message: `Domain pattern cannot contain port: ${pattern}` }, + ]; } if (trimmed.startsWith("*.") || trimmed.startsWith(".")) { const domain = trimmed.startsWith("*.") ? trimmed.substring(2) : trimmed.substring(1); if (!domain.includes(".")) { - return `Wildcard pattern too broad: ${pattern}`; + return [{ path, message: `Wildcard pattern too broad: ${pattern}` }]; } } else if (!trimmed.includes(".")) { - return `Invalid domain pattern: ${pattern}`; + return [{ path, message: `Invalid domain pattern: ${pattern}` }]; } - return null; + return []; } -function validateNetworkConfig(config: NetworkConfig): string | null { - for (const domains of [config.allowedDomains, config.deniedDomains]) { - if (domains) { - for (const domain of domains) { - const error = validateDomainPattern(domain); - if (error) return error; - } +function validateNetworkConfig(config: NetworkConfig): ValidationError { + const fields: Array<[string, string[] | undefined]> = [ + ["networkConfig.allowedDomains", config.allowedDomains], + ["networkConfig.deniedDomains", config.deniedDomains], + ]; + for (const [fieldPath, domains] of fields) { + if (!domains) continue; + for (let i = 0; i < domains.length; i++) { + const errors = validateDomainPattern(domains[i]!, `${fieldPath}[${i}]`); + if (errors.length > 0) return errors; } } - return null; + return []; } function normalizeNetworkConfig(config: NetworkConfig): NetworkConfig { @@ -220,16 +254,27 @@ function normalizeNetworkConfig(config: NetworkConfig): NetworkConfig { function validateMcpServerConfig( id: string, config: McpServerConfig -): string | null { +): ValidationError { + const basePath = `mcpServers.${id}`; if (!config.url && !config.command) { - return `MCP ${id}: must specify either 'url' or 'command'`; + return [ + { + path: basePath, + message: `MCP ${id}: must specify either 'url' or 'command'`, + }, + ]; } if ( config.url && !config.url.startsWith("http://") && !config.url.startsWith("https://") ) { - return `MCP ${id}: url must be http:// or https://`; + return [ + { + path: `${basePath}.url`, + message: `MCP ${id}: url must be http:// or https://`, + }, + ]; } if (config.command) { const dangerousCommands = [ @@ -244,23 +289,30 @@ function validateMcpServerConfig( ]; const baseCommand = config.command.split("/").pop()?.split(" ")[0] || ""; if (dangerousCommands.includes(baseCommand)) { - return `MCP ${id}: command '${baseCommand}' is not allowed`; + return [ + { + path: `${basePath}.command`, + message: `MCP ${id}: command '${baseCommand}' is not allowed`, + }, + ]; } } - return null; + return []; } function validateMcpConfig( mcpServers: Record -): string | null { +): ValidationError { for (const [id, config] of Object.entries(mcpServers)) { if (!/^[a-zA-Z0-9_-]+$/.test(id)) { - return `MCP ID '${id}' is invalid`; + return [ + { path: `mcpServers.${id}`, message: `MCP ID '${id}' is invalid` }, + ]; } - const error = validateMcpServerConfig(id, config); - if (error) return error; + const errors = validateMcpServerConfig(id, config); + if (errors.length > 0) return errors; } - return null; + return []; } // ============================================================================= @@ -563,16 +615,26 @@ export function createAgentApi( // Validate network config if (normalizedNetworkConfig) { - const error = validateNetworkConfig(normalizedNetworkConfig); - if (error) return c.json({ success: false, error }, 400); + const errors = validateNetworkConfig(normalizedNetworkConfig); + if (errors.length > 0) { + return c.json( + { success: false, error: formatValidationError(errors) }, + 400 + ); + } } // Validate MCP config if (mcpServers) { - const error = validateMcpConfig( + const errors = validateMcpConfig( mcpServers as Record ); - if (error) return c.json({ success: false, error }, 400); + if (errors.length > 0) { + return c.json( + { success: false, error: formatValidationError(errors) }, + 400 + ); + } } const isEphemeral = !requestedAgentId?.trim(); diff --git a/packages/gateway/src/services/instruction-service.ts b/packages/gateway/src/services/instruction-service.ts index ae68c96e7..cffc2dab8 100644 --- a/packages/gateway/src/services/instruction-service.ts +++ b/packages/gateway/src/services/instruction-service.ts @@ -26,56 +26,104 @@ interface SessionContextData { mcpStatus: McpStatus[]; } +/** + * Shared base class for InstructionProviders. + * + * Subclasses implement `buildInstructions(context)` with their domain logic. + * The base wraps every call in a try-catch + structured logging, so unexpected + * errors yield an empty string instead of crashing the session context + * assembly. This removes the identical boilerplate each subclass used to + * declare. + */ +abstract class BaseInstructionProvider implements InstructionProvider { + abstract readonly name: string; + abstract readonly priority: number; + + async getInstructions(context: InstructionContext): Promise { + try { + return await this.buildInstructions(context); + } catch (error) { + logger.error(`Failed to build ${this.name} instructions`, { error }); + return ""; + } + } + + protected abstract buildInstructions( + context: InstructionContext + ): Promise | string; +} + /** * Provides instructions from enabled skills for the agent. * Fetches skill content from AgentSettings and injects as instructions. * Falls back to generic skills.sh discovery instructions if no skills configured. */ -class SkillsInstructionProvider implements InstructionProvider { - name = "skills"; - priority = 15; +class SkillsInstructionProvider extends BaseInstructionProvider { + readonly name = "skills"; + readonly priority = 15; - constructor(private agentSettingsStore?: AgentSettingsStore) {} + constructor(private agentSettingsStore?: AgentSettingsStore) { + super(); + } - async getInstructions(context: InstructionContext): Promise { + protected async buildInstructions( + context: InstructionContext + ): Promise { // If no settings store or agentId, return generic skills.sh instructions if (!this.agentSettingsStore || !context.agentId) { return this.getGenericSkillsInstructions(); } + // Settings lookup uses a local try/catch because the domain-specific + // fallback here is "return the generic skills blurb", not "empty string". + // The base class's catch-all still guards against any bug outside this + // block. + let enabledSkills: Array<{ + name: string; + description?: string; + repo: string; + content?: string; + modelPreference?: string; + thinkingLevel?: string; + instructions?: string; + }> = []; try { const settings = await this.agentSettingsStore.getSettings( context.agentId ); const skills = settings?.skillsConfig?.skills || []; - const enabledSkills = skills.filter((s) => s.enabled && s.content); + enabledSkills = skills.filter((s) => s.enabled && s.content); + } catch (error) { + logger.error("Failed to load skill settings", { error }); + return this.getGenericSkillsInstructions(); + } - if (enabledSkills.length === 0) { - return this.getGenericSkillsInstructions(); - } + if (enabledSkills.length === 0) { + return this.getGenericSkillsInstructions(); + } - // Progressive disclosure: inject only metadata (name + description + model/thinking tags) - // to reduce prompt size. Agent reads full SKILL.md on demand. - const skillSummaries = enabledSkills - .map((skill) => { - const desc = skill.description ? ` - ${skill.description}` : ""; - const tags: string[] = []; - if (skill.modelPreference) { - tags.push(`[model: ${skill.modelPreference}]`); - } - if (skill.thinkingLevel) { - tags.push(`[thinking: ${skill.thinkingLevel}]`); - } - const tagStr = tags.length > 0 ? ` ${tags.join(" ")}` : ""; - const line = `- **${skill.name}**${desc} (\`${skill.repo}\`)${tagStr}`; - if (skill.instructions?.trim()) { - return `${line}\n → ${skill.instructions.trim()}`; - } - return line; - }) - .join("\n"); + // Progressive disclosure: inject only metadata (name + description + model/thinking tags) + // to reduce prompt size. Agent reads full SKILL.md on demand. + const skillSummaries = enabledSkills + .map((skill) => { + const desc = skill.description ? ` - ${skill.description}` : ""; + const tags: string[] = []; + if (skill.modelPreference) { + tags.push(`[model: ${skill.modelPreference}]`); + } + if (skill.thinkingLevel) { + tags.push(`[thinking: ${skill.thinkingLevel}]`); + } + const tagStr = tags.length > 0 ? ` ${tags.join(" ")}` : ""; + const line = `- **${skill.name}**${desc} (\`${skill.repo}\`)${tagStr}`; + if (skill.instructions?.trim()) { + return `${line}\n → ${skill.instructions.trim()}`; + } + return line; + }) + .join("\n"); - return `# Enabled Skills + return `# Enabled Skills The following skills are installed and available. When a task matches a skill, read the full skill instructions before using it. Skills tagged with [model: ...] prefer a specific model — delegate to the corresponding coding agent when available. @@ -86,10 +134,6 @@ ${skillSummaries} --- ${this.getGenericSkillsInstructions()}`; - } catch (error) { - logger.error("Failed to get skills instructions", { error }); - return this.getGenericSkillsInstructions(); - } } private getGenericSkillsInstructions(): string { @@ -102,11 +146,11 @@ Your available skills are listed above. To read full instructions for a skill, u /** * Provides information about network access rules and allowed domains */ -class NetworkInstructionProvider implements InstructionProvider { - name = "network"; - priority = 20; +class NetworkInstructionProvider extends BaseInstructionProvider { + readonly name = "network"; + readonly priority = 20; - getInstructions(_context: InstructionContext): string { + protected buildInstructions(_context: InstructionContext): string { const allowedDomains = process.env.WORKER_ALLOWED_DOMAINS?.trim() || ""; const disallowedDomains = process.env.WORKER_DISALLOWED_DOMAINS?.trim() || "";