Quellcode durchsuchen

fix(config): guard disabled_* fields against non-array values

- guard every reader of disabled_tools/disabled_agents/disabled_mcps/disabled_skills with Array.isArray() before .filter()/.includes()/iteration, falling back to "nothing disabled" instead of throwing

- affected sites: minimumExpectedToolCount() and the tool/MCP health-check block in src/index.ts, createBuiltinMcps() in src/mcp/index.ts, the orchestrator wait_for_user flag and getDisabledAgents() in src/agents/index.ts, getSkillPermissionsForAgent() in src/cli/skills.ts, and validateFinalImageRouting() in src/config/loader.ts

- add a defensive normalization pass at the end of loadPluginConfig() that strips these fields if present but not an array, warning via the existing onWarning/console.warn path; documented as currently unreachable via normal file loading (Zod already rejects the whole file) and retained as defense-in-depth

- add regression tests for malformed ('', null, {}) input at every guarded call site

- add/complete docstrings on the functions touched by this fix

Fixes #894

Signed-off-by: Major Hayden <major@mhtx.net>
Major Hayden vor 3 Wochen
Ursprung
Commit
8d8a8a9acc

+ 61 - 0
src/agents/index.test.ts

@@ -1166,3 +1166,64 @@ describe('AgentOverrideConfigSchema permission validation', () => {
     expect(result.success).toBe(false);
   });
 });
+
+describe('getDisabledAgents with malformed config', () => {
+  test('falls back to DEFAULT_DISABLED_AGENTS when disabled_agents is not an array', () => {
+    const config: PluginConfig = {
+      disabled_agents: 'not-an-array' as any,
+    };
+    const disabled = getDisabledAgents(config);
+    const expected = getDisabledAgents(undefined);
+    expect(disabled).toEqual(expected);
+  });
+
+  test('falls back to DEFAULT_DISABLED_AGENTS when disabled_agents is an object', () => {
+    const config: PluginConfig = {
+      disabled_agents: { invalid: 'object' } as any,
+    };
+    const disabled = getDisabledAgents(config);
+    const expected = getDisabledAgents(undefined);
+    expect(disabled).toEqual(expected);
+  });
+
+  test('handles valid array normally', () => {
+    const config: PluginConfig = {
+      disabled_agents: ['explorer'],
+    };
+    const disabled = getDisabledAgents(config);
+    expect(disabled.has('explorer')).toBe(true);
+  });
+});
+
+describe('createAgents with malformed disabled_tools', () => {
+  test('does not throw when disabled_tools is not an array', () => {
+    const config: PluginConfig = {
+      disabled_tools: 'not-an-array' as any,
+    };
+    expect(() => createAgents(config)).not.toThrow();
+  });
+
+  test('does not throw when disabled_tools is an object', () => {
+    const config: PluginConfig = {
+      disabled_tools: {} as any,
+    };
+    expect(() => createAgents(config)).not.toThrow();
+  });
+
+  test('orchestrator is created with wait_for_user enabled when disabled_tools is malformed', () => {
+    const config: PluginConfig = {
+      disabled_tools: 'not-an-array' as any,
+    };
+    const agents = createAgents(config);
+    const orchestrator = agents.find((a) => a.name === 'orchestrator');
+    expect(orchestrator).toBeDefined();
+    // When disabled_tools is malformed (treated as empty array), wait_for_user
+    // should be enabled, which is reflected in the prompt text
+    expect(orchestrator?.config.prompt).toContain(
+      'call `wait_for_user` as your final tool action',
+    );
+    expect(orchestrator?.config.prompt).not.toContain(
+      '`wait_for_user` is disabled',
+    );
+  });
+});

+ 7 - 3
src/agents/index.ts

@@ -541,7 +541,10 @@ export function createAgents(
     undefined,
     disabled,
     councillorAgents.length > 0 ? ['council'] : undefined,
-    !config?.disabled_tools?.includes('wait_for_user'),
+    !(
+      Array.isArray(config?.disabled_tools) &&
+      config.disabled_tools.includes('wait_for_user')
+    ),
   );
 
   const inlineOrchestratorPrompt = orchestratorOverride?.prompt;
@@ -756,8 +759,9 @@ export function getAgentConfigs(
  */
 export function getDisabledAgents(config?: PluginConfig): Set<string> {
   const userDisabled = config?.disabled_agents;
-  const disabledSource =
-    userDisabled !== undefined ? userDisabled : DEFAULT_DISABLED_AGENTS;
+  const disabledSource = Array.isArray(userDisabled)
+    ? userDisabled
+    : DEFAULT_DISABLED_AGENTS;
   const disabled = new Set<string>();
   for (const name of disabledSource) {
     if (!PROTECTED_AGENTS.has(name)) {

+ 36 - 0
src/cli/skills.test.ts

@@ -51,3 +51,39 @@ describe('skills permissions', () => {
     expect(wildcardPerms['*']).toBe('allow');
   });
 });
+
+describe('getSkillPermissionsForAgent with malformed disabledSkillNames', () => {
+  it('does not throw when disabledSkillNames is not an array', () => {
+    expect(() =>
+      getSkillPermissionsForAgent(
+        'orchestrator',
+        undefined,
+        'not-an-array' as any,
+      ),
+    ).not.toThrow();
+  });
+
+  it('treats non-array disabledSkillNames as empty array', () => {
+    const permsWithDisabled = getSkillPermissionsForAgent(
+      'orchestrator',
+      undefined,
+      ['simplify'],
+    );
+    const permsWithMalformed = getSkillPermissionsForAgent(
+      'orchestrator',
+      undefined,
+      'not-an-array' as any,
+    );
+    // When simplify is disabled, it should be explicitly denied
+    expect(permsWithDisabled.simplify).toBe('deny');
+    // When disabledSkillNames is malformed (treated as empty), simplify should be allowed
+    expect(permsWithMalformed['*']).toBe('allow');
+  });
+
+  it('handles object as disabledSkillNames gracefully', () => {
+    const perms = getSkillPermissionsForAgent('orchestrator', undefined, {
+      invalid: 'object',
+    } as any);
+    expect(perms['*']).toBe('allow');
+  });
+});

+ 3 - 1
src/cli/skills.ts

@@ -37,7 +37,9 @@ export function getSkillPermissionsForAgent(
   skillList?: string[],
   disabledSkillNames?: string[],
 ): Record<string, 'allow' | 'ask' | 'deny'> {
-  const disabledSkills = new Set(disabledSkillNames ?? []);
+  const disabledSkills = new Set(
+    Array.isArray(disabledSkillNames) ? disabledSkillNames : [],
+  );
 
   // Orchestrator gets all skills by default, others are restricted
   const permissions: Record<string, 'allow' | 'ask' | 'deny'> = {

+ 87 - 0
src/config/loader.test.ts

@@ -538,6 +538,93 @@ describe('onWarning callback', () => {
     const config = loadPluginConfig(projectDir);
     expect(config.agents?.oracle?.model).toBe('model');
   });
+
+  test('rejects config with non-array disabled_tools (schema validation)', () => {
+    const projectDir = path.join(tempDir, 'project');
+    const projectConfigDir = path.join(projectDir, '.opencode');
+    fs.mkdirSync(projectConfigDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(projectConfigDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({
+        disabled_tools: 'not-an-array',
+        agents: { oracle: { model: 'test/model' } },
+      }),
+    );
+
+    const warnings: ConfigLoadWarning[] = [];
+    const config = loadPluginConfig(projectDir, {
+      onWarning: (warning) => warnings.push(warning),
+    });
+
+    // Schema validation rejects the entire file, so config is empty
+    expect(config).toEqual({});
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]?.kind).toBe('invalid-schema');
+    expect(warnings[0]?.message).toBe('Config does not match schema');
+  });
+
+  test('rejects config with non-array disabled_agents (schema validation)', () => {
+    const projectDir = path.join(tempDir, 'project');
+    const projectConfigDir = path.join(projectDir, '.opencode');
+    fs.mkdirSync(projectConfigDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(projectConfigDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({
+        disabled_agents: { invalid: 'object' },
+      }),
+    );
+
+    const warnings: ConfigLoadWarning[] = [];
+    const config = loadPluginConfig(projectDir, {
+      onWarning: (warning) => warnings.push(warning),
+    });
+
+    expect(config).toEqual({});
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]?.kind).toBe('invalid-schema');
+  });
+
+  test('rejects config with non-array disabled_mcps (schema validation)', () => {
+    const projectDir = path.join(tempDir, 'project');
+    const projectConfigDir = path.join(projectDir, '.opencode');
+    fs.mkdirSync(projectConfigDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(projectConfigDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({
+        disabled_mcps: 123,
+      }),
+    );
+
+    const warnings: ConfigLoadWarning[] = [];
+    const config = loadPluginConfig(projectDir, {
+      onWarning: (warning) => warnings.push(warning),
+    });
+
+    expect(config).toEqual({});
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]?.kind).toBe('invalid-schema');
+  });
+
+  test('rejects config with non-array disabled_skills (schema validation)', () => {
+    const projectDir = path.join(tempDir, 'project');
+    const projectConfigDir = path.join(projectDir, '.opencode');
+    fs.mkdirSync(projectConfigDir, { recursive: true });
+    fs.writeFileSync(
+      path.join(projectConfigDir, 'oh-my-opencode-slim.json'),
+      JSON.stringify({
+        disabled_skills: true,
+      }),
+    );
+
+    const warnings: ConfigLoadWarning[] = [];
+    const config = loadPluginConfig(projectDir, {
+      onWarning: (warning) => warnings.push(warning),
+    });
+
+    expect(config).toEqual({});
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]?.kind).toBe('invalid-schema');
+  });
 });
 
 describe('deepMerge behavior', () => {

+ 47 - 1
src/config/loader.ts

@@ -200,6 +200,17 @@ function findConfigPathInDirs(
   return null;
 }
 
+/**
+ * Validate that `image_routing: "auto"` has a live observer agent to route
+ * images to. Emits a warning (via `onWarning`/`console.warn`) and returns
+ * `false` if "auto" routing is configured but the observer agent is
+ * disabled, since images would then have nowhere to go.
+ *
+ * @param config - Plugin configuration to validate
+ * @param configPath - Path of the config file, used in the warning payload
+ * @param options - Optional load options including the onWarning callback
+ * @returns `true` if the routing configuration is valid, `false` otherwise
+ */
 function validateFinalImageRouting(
   config: PluginConfig,
   configPath: string,
@@ -207,7 +218,9 @@ function validateFinalImageRouting(
 ): boolean {
   if (config.image_routing !== 'auto') return true;
 
-  const disabledAgents = config.disabled_agents ?? DEFAULT_DISABLED_AGENTS;
+  const disabledAgents = Array.isArray(config.disabled_agents)
+    ? config.disabled_agents
+    : DEFAULT_DISABLED_AGENTS;
   if (!disabledAgents.includes('observer')) return true;
 
   const message =
@@ -402,6 +415,39 @@ export function loadPluginConfig(
     options,
   );
 
+  // Normalize disabled_* config keys to ensure they are arrays or undefined.
+  // This loop is currently unreachable via the normal file-loading path:
+  // PluginConfigSchema.safeParse() rejects the WHOLE config object if any
+  // disabled_* field is non-array (no .catch() on these fields), so
+  // loadConfigFromPath returns null and the file falls back to {} BEFORE this
+  // loop ever runs. Retained only as defense-in-depth against a future schema
+  // relaxation (e.g. adding .catch() to these fields) or a construction path
+  // that bypasses safeParse entirely — not as a proven/tested fix for the
+  // originally reported crash (root cause not reproduced).
+  const ARRAY_CONFIG_KEYS = [
+    'disabled_agents',
+    'disabled_tools',
+    'disabled_mcps',
+    'disabled_skills',
+  ] as const;
+
+  const configPathForWarning = projectConfigPath ?? userConfigPath ?? '';
+  for (const key of ARRAY_CONFIG_KEYS) {
+    const value = config[key as keyof PluginConfig];
+    if (value !== undefined && !Array.isArray(value)) {
+      const message = `Config key "${key}" must be an array; ignoring invalid value.`;
+      options?.onWarning?.({
+        path: configPathForWarning,
+        kind: 'invalid-schema',
+        message,
+      });
+      if (!options?.silent) {
+        console.warn(`[oh-my-opencode-slim] ${message}`);
+      }
+      delete config[key as keyof PluginConfig];
+    }
+  }
+
   return config;
 }
 

+ 8 - 0
src/index.test.ts

@@ -10,6 +10,14 @@ describe('plugin health thresholds', () => {
     );
     expect(minimumExpectedToolCount(['unknown_tool'])).toBe(5);
   });
+
+  test('never throws when disabledTools is not an array', () => {
+    // Regression test: a malformed/non-array config.disabled_tools value
+    // must degrade to "nothing disabled" instead of crashing plugin init.
+    expect(minimumExpectedToolCount('' as any)).toBe(5);
+    expect(minimumExpectedToolCount(null as any)).toBe(5);
+    expect(minimumExpectedToolCount({} as any)).toBe(5);
+  });
 });
 
 describe('plugin env disable', () => {

+ 21 - 4
src/index.ts

@@ -115,12 +115,26 @@ const BASELINE_TOOL_NAMES = new Set([
   'ast_grep_replace',
 ]);
 
-/** @internal Exposed for deterministic health-threshold tests. */
+/**
+ * Compute the minimum tool count the health check should expect, accounting
+ * for baseline tools the user has intentionally disabled.
+ *
+ * @internal Exposed for deterministic health-threshold tests.
+ * @param disabledTools - Tool names disabled via config; non-array/malformed
+ *   values (which should never occur post-validation, but are not trusted at
+ *   runtime) are treated as "nothing disabled".
+ * @returns The adjusted minimum expected tool count
+ */
 export function minimumExpectedToolCount(
   disabledTools: readonly string[] = [],
 ): number {
+  // Config values come from user-edited JSON/JSONC (and can be re-derived
+  // via runtime preset switches); never trust the declared type at
+  // runtime. Fall back to "no disabled tools" instead of crashing plugin
+  // init if this isn't actually an array.
+  const safeDisabledTools = Array.isArray(disabledTools) ? disabledTools : [];
   const disabledBaselineTools = new Set(
-    disabledTools.filter((toolName) => BASELINE_TOOL_NAMES.has(toolName)),
+    safeDisabledTools.filter((toolName) => BASELINE_TOOL_NAMES.has(toolName)),
   );
   return HEALTH_CHECK.minTools - disabledBaselineTools.size;
 }
@@ -459,7 +473,10 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       ast_grep_search,
       ast_grep_replace,
     };
-    if (config.disabled_tools && config.disabled_tools.length > 0) {
+    if (
+      Array.isArray(config.disabled_tools) &&
+      config.disabled_tools.length > 0
+    ) {
       const disabledTools = new Set(config.disabled_tools);
       tools = Object.fromEntries(
         Object.entries(tools).filter(([name]) => !disabledTools.has(name)),
@@ -484,7 +501,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
   const mcpCount = Object.keys(mcps).length;
   // Skip MCP threshold when user explicitly disabled all built-in MCPs
   const mcpThreshold =
-    config.disabled_mcps && config.disabled_mcps.length > 0
+    Array.isArray(config.disabled_mcps) && config.disabled_mcps.length > 0
       ? 0
       : HEALTH_CHECK.minMcps;
   const toolThreshold = minimumExpectedToolCount(config.disabled_tools);

+ 11 - 0
src/mcp/index.test.ts

@@ -80,4 +80,15 @@ describe('createBuiltinMcps', () => {
     expect(gh_grep).toBeDefined();
     expect('url' in gh_grep).toBe(true);
   });
+
+  test('never throws when disabledMcps is not an array', () => {
+    // Regression test: a malformed/non-array config.disabled_mcps value
+    // must degrade to "nothing disabled" instead of crashing plugin init.
+    const mcps = createBuiltinMcps('' as any);
+    const names = Object.keys(mcps);
+
+    expect(names.length).toBe(2);
+    expect(names).toContain('context7');
+    expect(names).toContain('gh_grep');
+  });
 });

+ 4 - 1
src/mcp/index.ts

@@ -16,9 +16,12 @@ const allBuiltinMcps: Record<McpName, McpConfig> = {
 export function createBuiltinMcps(
   disabledMcps: readonly string[] = [],
 ): Record<string, McpConfig> {
+  // Never trust the declared type of user-config-derived values at
+  // runtime; fall back to "nothing disabled" instead of throwing.
+  const safeDisabledMcps = Array.isArray(disabledMcps) ? disabledMcps : [];
   return Object.fromEntries(
     Object.entries(allBuiltinMcps).filter(
-      ([name]) => !disabledMcps.includes(name),
+      ([name]) => !safeDisabledMcps.includes(name),
     ),
   );
 }