fix(config): add-only skill directives keep default grants; fold at runtime resolution

docs/project-local-customization.md

@@ -76,7 +76,7 @@ The `skills` array is replacement-based: when a project config defines `agents.<
 
 Effective result: `codemap`, `deepwork`, `project-architecture`, `project-testing` — the global list is not duplicated.
 
-Resolution order is deterministic: resolve the inherited/configured `skills` list, then apply `skills_add`, then apply `skills_remove`. Duplicates are removed (first occurrence wins), and `skills_remove` wins over `skills_add` for the same skill. When the effective list contains `"*"`, removals are expressed with the existing `!name` exclusion syntax (e.g. effective `["*", "!codemap"]`). The directives are folded into `skills` during config resolution and stripped from the final agent configuration, so agent definitions and hooks only ever see a plain `skills` list.
+Resolution order is deterministic: resolve the inherited/configured `skills` list, then apply `skills_add`, then apply `skills_remove`. Duplicates are removed (first occurrence wins), and `skills_remove` wins over `skills_add` for the same skill. When the effective list contains `"*"`, removals are expressed with the existing `!name` exclusion syntax (e.g. effective `["*", "!codemap"]`). The directives are folded into `skills` during agent resolution — after all layers (user config, project config, presets, runtime `/preset` switching) have determined the effective `skills` value — and stripped from the final agent configuration, so agent definitions and hooks only ever see a plain `skills` list. On an agent without a `skills` list, directives resolve against that agent's default grants, so `skills_add` keeps the defaults and appends.
 
 See [Skills Assignment](skills.md#adding-or-removing-skills-on-top-of-an-inherited-list) for the full rule set, including behavior when no `skills` list is configured.
 

docs/skills.md

@@ -333,4 +333,5 @@ Control which skills each agent can use in `~/.config/opencode/oh-my-opencode-sl
 **Rules:**
 - Duplicates are removed (first occurrence wins) before removals are applied
 - If the result contains `"*"`, each removed name is appended as `"!<name>"` so the exclusion beats the wildcard grant
-- A removal on an agent without a `skills` list starts from that agent's default grants (orchestrator: all skills)
+- On an agent without a `skills` list, directives resolve against that agent's default grants (orchestrator: all skills), so `skills_add` keeps the defaults and appends, and `skills_remove` prunes from them
+- A removal entry of the form `"!<name>"` removes the exclusion token itself (it lifts an existing exclusion); the `"*"` token is never expanded

src/cli/skills.ts

@@ -102,11 +102,10 @@ export function getSkillPermissionsForAgent(
 /**
  * Fold per-agent skill directives into an effective skills list.
  *
- * 1. Without a base `skills` list and without additions, there is nothing
- *    to resolve unless a removal is requested. A removal without a base
- *    starts from the agent's default grants (orchestrator defaults to
- *    allow-all, so its working base is `['*']`). Additions without a base
- *    start from an empty list - they do not inherit default grants.
+ * 1. Without a base `skills` list the working base is the agent's default
+ *    grants (orchestrator defaults to allow-all, so its working base is
+ *    `['*']`), so `skills_add` alone keeps the defaults and appends, and
+ *    `skills_remove` alone prunes from the defaults.
  * 2. The working base and `add` are concatenated, deduped (first
  *    occurrence wins), then removal entries are filtered out.
  * 3. Removals operate on final skill tokens, never expanding `'*'`: a
@@ -135,16 +134,14 @@ export function resolveEffectiveSkills(
     return undefined;
   }
 
-  // A removal without a base list starts from the agent's default grants
-  // (orchestrator defaults to allow-all). Additions without a base start
-  // from an empty list and do not inherit default grants.
+  // Without a base list the working base is the agent's default grants
+  // (orchestrator defaults to allow-all), so additions build on top of
+  // what the agent already gets and removals prune from it.
   const workingBase =
     base ??
-    (addList.length === 0
-      ? agentName === 'orchestrator'
-        ? ['*']
-        : getDefaultGrantedSkillNames(agentName)
-      : []);
+    (agentName === 'orchestrator'
+      ? ['*']
+      : getDefaultGrantedSkillNames(agentName));
 
   // Removals operate on final skill tokens: a plain name removes that
   // name, and a '!name' entry removes the exclusion token itself (lifting

src/config/loader.ts

@@ -11,7 +11,6 @@ import {
   PluginConfigSchema,
   WebfetchConfigSchema,
 } from './schema';
-import { normalizeAgentSkillDirectives } from './utils';
 
 /**
  * Warning kinds produced during config loading.
@@ -729,11 +728,11 @@ export function loadPluginConfig(
     }
   }
 
-  // Fold per-agent skill directives (skills_add/skills_remove) into the
-  // effective skills list so downstream consumers see plain `skills`.
-  if (config.agents) {
-    config.agents = normalizeAgentSkillDirectives(config.agents);
-  }
+  // Note: per-agent skill directives (skills_add/skills_remove) are left
+  // raw in the returned config. They are folded into the effective skills
+  // list by RuntimeConfig.agents(), the single resolution point, so runtime
+  // /preset switching re-resolves them from the raw preset layers instead
+  // of operating on an already-baked skills array.
 
   // Normalize companion config defaults
   if (config.companion) {

src/config/skills-add-remove.test.ts

@@ -68,6 +68,15 @@ describe('skills_add / skills_remove directives', () => {
     expect('skills_remove' in (entry ?? {})).toBe(false);
   }
 
+  // Effective skills for a loaded config, resolved exactly the way the
+  // plugin consumes them (RuntimeConfig.agents()).
+  function effectiveAgent(
+    loaded: PluginConfig,
+    name = 'oracle',
+  ): AgentOverrideConfig {
+    return runtimeFor(loaded).agents()[name] as AgentOverrideConfig;
+  }
+
   // Resolver unit tests -----------------------------------------------------
 
   test('resolveEffectiveSkills: base + add with remove winning over add', () => {
@@ -88,10 +97,16 @@ describe('skills_add / skills_remove directives', () => {
     );
   });
 
-  test('resolveEffectiveSkills: additions without a base', () => {
+  test('resolveEffectiveSkills: additions without a base keep default grants', () => {
     expect(
       resolveEffectiveSkills('oracle', undefined, ['x', 'y'], undefined),
-    ).toEqual(['x', 'y']);
+    ).toEqual([...getDefaultGrantedSkillNames('oracle'), 'x', 'y']);
+  });
+
+  test('resolveEffectiveSkills: additions without a base, orchestrator keeps allow-all', () => {
+    expect(
+      resolveEffectiveSkills('orchestrator', undefined, ['x'], undefined),
+    ).toEqual(['*', 'x']);
   });
 
   test('resolveEffectiveSkills: removal only, orchestrator defaults to allow-all', () => {
@@ -184,6 +199,8 @@ describe('skills_add / skills_remove directives', () => {
   });
 
   // Loader E2E ---------------------------------------------------------------
+  // The loader keeps directives raw; the effective list is resolved by
+  // RuntimeConfig.agents(), so E2E assertions go through runtimeFor().
 
   test('loader: global skills + project skills_add', () => {
     writeUserConfig({
@@ -196,13 +213,14 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual([
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual([
       'codemap',
       'deepwork',
       'nexus-backend',
       'nexus-frontend',
     ]);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: global skills + project skills_remove', () => {
@@ -214,8 +232,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['codemap']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['codemap']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: simultaneous add + remove with duplicates in one layer', () => {
@@ -230,8 +249,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['a', 'c']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['a', 'c']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: agent without existing skills gains skills via skills_add', () => {
@@ -240,8 +260,13 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['x', 'y']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual([
+      ...getDefaultGrantedSkillNames('oracle'),
+      'x',
+      'y',
+    ]);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: removal only, no base list', () => {
@@ -253,12 +278,14 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.orchestrator?.skills).toEqual(['*', '!foo']);
-    expectNoDirectiveKeys(loaded.agents?.orchestrator);
-    expect(loaded.agents?.oracle?.skills).toEqual(
+    const orchestrator = effectiveAgent(loaded, 'orchestrator');
+    expect(orchestrator.skills).toEqual(['*', '!foo']);
+    expectNoDirectiveKeys(orchestrator);
+    const oracle = effectiveAgent(loaded);
+    expect(oracle.skills).toEqual(
       getDefaultGrantedSkillNames('oracle').filter((n) => n !== 'codemap'),
     );
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    expectNoDirectiveKeys(oracle);
   });
 
   test('loader: custom agent inherits project skills_add', () => {
@@ -270,8 +297,12 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.['my-agent']?.skills).toEqual(['proj-skill']);
-    expectNoDirectiveKeys(loaded.agents?.['my-agent']);
+    const effective = effectiveAgent(loaded, 'my-agent');
+    expect(effective.skills).toEqual([
+      ...getDefaultGrantedSkillNames('my-agent'),
+      'proj-skill',
+    ]);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: preset skills + project skills_add', () => {
@@ -284,8 +315,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['a', 'b', 'c']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['a', 'b', 'c']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: preset skills + project skills_remove', () => {
@@ -298,8 +330,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['a']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['a']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: root skills replace preset skills, directive still applies', () => {
@@ -313,8 +346,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['x', 'c']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['x', 'c']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: preset-layer removal survives field-level merge', () => {
@@ -329,8 +363,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['x']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['x']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: wildcard base + project removal', () => {
@@ -342,8 +377,9 @@ describe('skills_add / skills_remove directives', () => {
     });
 
     const loaded = loadPluginConfig(projectDir, { silent: true });
-    expect(loaded.agents?.oracle?.skills).toEqual(['*', '!foo']);
-    expectNoDirectiveKeys(loaded.agents?.oracle);
+    const effective = effectiveAgent(loaded);
+    expect(effective.skills).toEqual(['*', '!foo']);
+    expectNoDirectiveKeys(effective);
   });
 
   test('loader: plain skills entry without directives is unchanged', () => {
@@ -356,6 +392,22 @@ describe('skills_add / skills_remove directives', () => {
     expectNoDirectiveKeys(loaded.agents?.oracle);
   });
 
+  test('loader: config keeps raw directives for runtime resolution', () => {
+    writeUserConfig({
+      agents: { oracle: { skills: ['a'] } },
+    });
+    writeProjectConfig({
+      agents: { oracle: { skills_add: ['b'] } },
+    });
+
+    const loaded = loadPluginConfig(projectDir, { silent: true });
+    expect(loaded.agents?.oracle).toEqual({
+      skills: ['a'],
+      skills_add: ['b'],
+    });
+    expect(effectiveAgent(loaded).skills).toEqual(['a', 'b']);
+  });
+
   // Schema validation --------------------------------------------------------
 
   test('schema: rejects invalid skills_add / skills_remove values', () => {
@@ -408,6 +460,42 @@ describe('skills_add / skills_remove directives', () => {
     expectNoDirectiveKeys(switched);
   });
 
+  test('runtime: higher runtime preset replaces startup-preset directive', () => {
+    writeUserConfig({
+      preset: 'p1',
+      presets: {
+        p1: { oracle: { skills_add: ['b'] } },
+        p2: { oracle: { skills_add: ['c'] } },
+      },
+      agents: { oracle: { skills: ['a'] } },
+    });
+
+    const runtime = runtimeFor(loadPluginConfig(projectDir, { silent: true }));
+    expect(runtime.agents().oracle.skills).toEqual(['a', 'b']);
+
+    runtime.setRuntimePreset('p2');
+    const switched = runtime.agents().oracle;
+    expect(switched.skills).toEqual(['a', 'c']);
+    expectNoDirectiveKeys(switched);
+  });
+
+  test('runtime: empty skills_add in higher preset suppresses startup directive', () => {
+    writeUserConfig({
+      preset: 'p1',
+      presets: {
+        p1: { oracle: { skills_add: ['b'] } },
+        p2: { oracle: { skills_add: [] } },
+      },
+      agents: { oracle: { skills: ['a'] } },
+    });
+
+    const runtime = runtimeFor(loadPluginConfig(projectDir, { silent: true }));
+    expect(runtime.agents().oracle.skills).toEqual(['a', 'b']);
+
+    runtime.setRuntimePreset('p2');
+    expect(runtime.agents().oracle.skills).toEqual(['a']);
+  });
+
   // createAgents integration ----------------------------------------------------
 
   test('createAgents: folded skills become permission grants', () => {
@@ -425,4 +513,20 @@ describe('skills_add / skills_remove directives', () => {
     expect(skillPermissions?.['my-skill']).toBe('allow');
     expect(skillPermissions?.simplify).toBe('allow');
   });
+
+  test('createAgents: add-only directive keeps default grants', () => {
+    const config: PluginConfig = {
+      agents: {
+        oracle: { skills_add: ['my-skill'] },
+      },
+    };
+    const agents = createAgents(runtimeFor(config));
+    const oracle = agents.find((a) => a.name === 'oracle');
+    expect(oracle).toBeDefined();
+    const skillPermissions = (
+      oracle?.config.permission as Record<string, unknown>
+    )?.skill as Record<string, unknown> | undefined;
+    expect(skillPermissions?.['my-skill']).toBe('allow');
+    expect(skillPermissions?.simplify).toBe('allow');
+  });
 });