refactor(gateway): extract BaseInstructionProvider and structured ValidationError

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, McpServerConfig>
-): 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<string, McpServerConfig>
       );
-      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();

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<string> {
+    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> | 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<string> {
+  protected async buildInstructions(
+    context: InstructionContext
+  ): Promise<string> {
     // 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() || "";