fix(client): validate skills in skillRefs, not in parseAiConfig

packages/client/README.md

@@ -325,7 +325,7 @@ const report = await writeSkills('*', '.claude/skills', {
 | Export | Description |
 |---|---|
 | `setSkillStore(store)` | Set the `SkillStore` the accessors, `writeSkills`, and `watchSkills` read from. Applies on every call; a nullish argument never clears the configured store, and `shutdown()` does. Throws `TypeError` for anything else without `getObject` and `allObjects` methods. Replacing a store does not close the old one, and a running watcher keeps the store it started with. Not an `initClient` option. |
-| `skillRefs(config)` | Project a config's `skills` array into typed `SkillReference[]`. Pure — no client, no store, no telemetry. `[]` when absent. |
+| `skillRefs(config)` | Project a config's `skills` array into typed `SkillReference[]`. Pure — no client, no store, no telemetry. `[]` when the field is absent or `config` is nullish. Throws `TypeError` when the field is present but malformed (including `null`, or one bad entry), so `writeSkills` never receives a partial list that would prune skills the config still references. `parseAiConfig` does not check `skills`, so a malformed field never fails `config().invoke()` or other core calls. |
 | `getSkill(key, { version? })` | One verified skill. Omit `version` for the newest available. Resolves to `null` when the skill is unavailable; throws only when no store is configured. |
 | `getSkillResult(key, { version? })` | The same retrieval, reporting **why**: resolves to `{ skill, reason, detail }`, where `reason` is `ok` / `absent` / `integrity_failure` / `store_unavailable` / `wrong_version`. Throws only when no store is configured. See [fail closed on tampering](#fail-closed-on-tampering-getskillresult). |
 | `getSkills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted, and a warning logs how many failed verification. |

packages/client/agents.md

@@ -280,8 +280,11 @@ export { createSkill, createSkillOutcome, createSkillReference } from './types.j
 The stage rules (shared spec §0.3), which every change here must keep:
 
 - **Core never names experimental.** No root export, core type, function signature, or option may
-  mention an experimental name. `AiConfigRep.skills` is therefore typed structurally rather than as
+  mention an experimental name. `AiConfigRep.skills` is therefore typed `unknown` rather than as
   `SkillReference[]`, and the store is set with `setSkillStore`, not an `initClient` option.
+- **An experimental field never fails a core call.** `parseAiConfig` passes `skills` through
+  unvalidated; `skillRefs` validates it and throws on a present but malformed field, rather than
+  returning a partial list that would authorize a prune.
   Experimental code may import core freely.
 - **Core reaches experimental only through an internal hook**, and a failure there is caught and
   logged, never thrown into the core call. Core never imports an experimental module: an experimental
@@ -555,7 +558,7 @@ Three specifics a later contributor is most likely to widen:
 
 - **The 22 Windows reserved device names** — `con`, `prn`, `aux`, `nul`, `com1`–`com9`, `lpt1`–`lpt9` — are rejected by `keyRejectionReason` on every platform.
   - No `process.platform === 'win32'` gate: a root written by a Linux container and read from a Windows host is an ordinary deployment, and with no Windows CI runner a platform branch would be untested.
-  - Not in `isValidSkillKey` or `SKILL_KEY_PATTERN` either. `parseAiConfig` fails closed on a bad `skills` entry, so a grammar rejection would invalidate the *whole* AI Config for a customer who never touches Windows. `skillRefs` would also drop the reference (with a warning), shortening what it hands `writeSkills` so prune deletes the skill's on-disk copy — "fails to write on Windows" becomes "deleted on Linux". The 255-byte path-component bound lives in this layer for the same reason.
+  - Not in `isValidSkillKey` or `SKILL_KEY_PATTERN` either. `skillRefs` fails closed on a bad `skills` entry, so a grammar rejection would reject *every* skill reference for a customer who never touches Windows. A grammar that dropped the entry instead would shorten what `skillRefs` hands `writeSkills`, so prune deletes the skill's on-disk copy — "fails to write on Windows" becomes "deleted on Linux". The 255-byte path-component bound lives in this layer for the same reason.
   - The set is exact: `com0` and `lpt0` are not reserved, and no case folding or suffix stripping is needed because the key grammar admits no uppercase, no `.`, and no `$`.
 - **Adoption narrows the clobber refusal; it is not a hole in it.** A file at a managed path with no manifest entry is adopted (recorded, reported `skipped_current`) only when its on-disk sha256 equals the resolved content hash. That lets a reconcile killed between the content writes and the final manifest write heal instead of wedging. Adopt on anything weaker than an exact hash match and the guarantee is gone. `skipped_current` is reused because a new `ReconcileActionKind` member would break every consumer with an exhaustive `switch`.
 - **`readRegularFile` is what makes that read safe**, and every part is load-bearing:

packages/client/etc/ai-server.api.md

@@ -32,10 +32,7 @@ export type AiConfigRep = {
         name: string;
     };
     outputFormat?: Record<string, unknown>;
-    skills?: ReadonlyArray<{
-        readonly key: string;
-        readonly version: number;
-    }>;
+    skills?: unknown;
 };
 
 // @public

packages/client/src/__tests__/lifecycle.test.ts

@@ -589,6 +589,30 @@ describe('lifecycle', () => {
       expect(result.meta?.variationKey).toBe('v1');
     });
 
+    it('does not fail on a malformed skills field', async () => {
+      // Agent Skills is experimental, so its field cannot break a core call
+      // (TESTING.md §0.3). `skillRefs` rejects it instead.
+      const mockClient = makeMockClient();
+      mockClient.variation = vi.fn().mockResolvedValue({
+        _ldMeta: { enabled: true, variationKey: 'v1', version: 1, mode: 'messages' },
+        model: { name: 'gpt-4o' },
+        provider: { name: 'OpenAI' },
+        instructions: 'You are helpful.',
+        skills: [{ key: 'My_Skill', version: 0 }],
+      });
+      mockLdInit.mockReturnValue(mockClient);
+      process.env.LD_SDK_KEY = 'test-key';
+
+      const { inspectConfig, extractVariation } = await import('../lifecycle.js');
+      const ctx = { kind: 'user' as const, key: 'user-1' };
+      const result = await inspectConfig('my-flag', ctx);
+      const extracted = await extractVariation('my-flag', ctx);
+
+      expect(result.enabled).toBe(true);
+      expect(result.config?.model.name).toBe('gpt-4o');
+      expect(extracted.config.model.name).toBe('gpt-4o');
+    });
+
     it('preserves modelKey and modelVersion from _ldMeta on meta', async () => {
       const mockClient = makeMockClient();
       mockClient.variation = vi.fn().mockResolvedValue({

packages/client/src/__tests__/schema.test.ts

@@ -128,17 +128,17 @@ describe('parseAiConfig', () => {
   // skill set without telling anyone.
 
   describe('skills', () => {
+    // Agent Skills is experimental, so a malformed `skills` field must not fail
+    // a core config call (TESTING.md §0.3). The field is passed through
+    // unvalidated, and `skillRefs` rejects it where the references are used
+    // (see skills.test.ts).
     const valid = { ...base, instructions: 'You are helpful.' };
 
-    it('accepts a config with no skills field (backward compatibility)', () => {
+    it('accepts a config with no skills field', () => {
       expect(parseAiConfig(valid).success).toBe(true);
     });
 
-    it('accepts an empty skills array', () => {
-      expect(parseAiConfig({ ...valid, skills: [] }).success).toBe(true);
-    });
-
-    it('accepts valid entries and round-trips them unmodified', () => {
+    it('passes valid entries through unmodified', () => {
       const skills = [
         { key: 'pdf-extraction', version: 2 },
         { key: 'a1', version: 1 },
@@ -149,95 +149,26 @@ describe('parseAiConfig', () => {
     });
 
     it.each([
+      ['null', null],
       ['a string', 'pdf-extraction'],
       ['an object', { key: 'pdf-extraction', version: 1 }],
-      ['a number', 3],
-      ['null', null],
-    ])('rejects a non-array skills value: %s', (_label, skills) => {
-      expect(parseAiConfig({ ...valid, skills }).success).toBe(false);
-    });
-
-    it.each([
       ['a bare string entry', ['pdf-extraction']],
-      ['a null entry', [null]],
-      ['a nested array entry', [['pdf-extraction', 1]]],
-    ])('rejects a non-object entry: %s', (_label, skills) => {
-      expect(parseAiConfig({ ...valid, skills }).success).toBe(false);
-    });
-
-    it.each([
-      ['key absent', { version: 1 }],
-      ['key is a number', { key: 7, version: 1 }],
-      ['key is null', { key: null, version: 1 }],
-    ])('rejects a missing or non-string key: %s', (_label, entry) => {
-      expect(parseAiConfig({ ...valid, skills: [entry] }).success).toBe(false);
-    });
-
-    it.each([
-      ['uppercase', 'Evil'],
-      ['leading dash', '-skill'],
-      ['leading dot', '.hidden'],
-      ['path separator', 'a/b'],
-      ['backslash', 'a\\b'],
-      ['traversal', '../evil'],
-      ['empty string', ''],
-      ['embedded space', 'has space'],
-      ['underscore', 'has_underscore'],
-      ['trailing newline', 'pdf-extraction\n'],
-    ])('rejects a key violating the pattern: %s', (_label, key) => {
-      expect(parseAiConfig({ ...valid, skills: [{ key, version: 1 }] }).success).toBe(false);
-    });
-
-    it('accepts a key of exactly 256 characters', () => {
-      // The accepting side of the <= 256 bound. It
-      // is only observable at the pure layers: a key becomes one directory name
-      // and NAME_MAX is 255, so `writeSkills` can never reach it.
-      const key = 'a'.repeat(256);
-      expect(parseAiConfig({ ...valid, skills: [{ key, version: 1 }] }).success).toBe(true);
-    });
-
-    it('rejects a key of 257 characters', () => {
-      const key = 'a'.repeat(257);
-      expect(parseAiConfig({ ...valid, skills: [{ key, version: 1 }] }).success).toBe(false);
-    });
-
-    it.each([
-      ['version absent', {}],
-      ['version 0', { version: 0 }],
-      ['version negative', { version: -1 }],
-      ['version non-integer', { version: 2.5 }],
-      ['version as string', { version: '2' }],
-      // A boolean is not an acceptable integer even in languages where it is
-      // integer-like — the rule is identical across language implementations.
-      ['version as boolean', { version: true }],
-      ['version null', { version: null }],
-      ['version NaN', { version: Number.NaN }],
-      ['version Infinity', { version: Number.POSITIVE_INFINITY }],
-    ])('rejects a missing or invalid version: %s', (_label, overrides) => {
-      const entry = { key: 'pdf-extraction', ...overrides };
-      expect(parseAiConfig({ ...valid, skills: [entry] }).success).toBe(false);
-    });
-
-    it('accepts version 1 as the lower bound', () => {
-      expect(parseAiConfig({ ...valid, skills: [{ key: 'a', version: 1 }] }).success).toBe(true);
-    });
-
-    it('names the skills field in the failure message', () => {
-      const result = parseAiConfig({ ...valid, skills: [{ key: 'Evil', version: 1 }] });
-      expect(result.success).toBe(false);
-      if (!result.success) expect(result.error.message).toContain('skills');
-    });
-
-    it('fails the whole config when a single entry among several is invalid', () => {
-      const result = parseAiConfig({
-        ...valid,
-        skills: [
-          { key: 'good-one', version: 1 },
-          { key: 'BAD', version: 1 },
-          { key: 'good-two', version: 2 },
+      ['an absent key', [{ version: 1 }]],
+      ['an uppercase key', [{ key: 'Evil', version: 1 }]],
+      ['a traversal key', [{ key: '../evil', version: 1 }]],
+      ['version 0', [{ key: 'a', version: 0 }]],
+      ['an absent version', [{ key: 'a' }]],
+      [
+        'one bad entry among good ones',
+        [
+          { key: 'good', version: 1 },
+          { key: 'My_Skill', version: 1 },
         ],
-      });
-      expect(result.success).toBe(false);
+      ],
+    ])('does not fail the parse for a malformed skills field: %s', (_label, skills) => {
+      const result = parseAiConfig({ ...valid, skills });
+      expect(result.success).toBe(true);
+      if (result.success) expect(result.data.skills).toEqual(skills);
     });
   });
 });

packages/client/src/__tests__/skills-fs.test.ts

@@ -1801,13 +1801,12 @@ describe('writeSkills Windows reserved device names', () => {
 
   it('a reserved name is still a valid key at the grammar level', () => {
     // This is the whole reason the check lives in the filesystem layer rather
-    // than in `isValidSkillKey`. `parseAiConfig` fails closed on a bad skills
-    // entry, so a grammar-level rejection would invalidate the *entire* AI
-    // Config — model, provider, instructions, tools — for a Linux or macOS
-    // customer, over a constraint that only exists on Windows. And `skillRefs`
-    // would silently drop the reference, which lets prune delete the skill's
-    // on-disk copy: "this fails to write on Windows" would become "this gets
-    // deleted on Linux".
+    // than in `isValidSkillKey`. `skillRefs` fails closed on a bad skills
+    // entry, so a grammar-level rejection would reject *every* skill reference
+    // for a Linux or macOS customer, over a constraint that only exists on
+    // Windows. A grammar that dropped the entry instead would let prune delete
+    // the skill's on-disk copy: "this fails to write on Windows" would become
+    // "this gets deleted on Linux".
     for (const key of WINDOWS_RESERVED_KEYS) expect(isValidSkillKey(key)).toBe(true);
 
     const parsed = parseAiConfig({

packages/client/src/__tests__/skills.test.ts

@@ -563,46 +563,67 @@ describe('skillRefs', () => {
     expect(skillRefs(undefined)).toEqual([]);
   });
 
-  it('drops a malformed entry and logs one warning per drop', async () => {
-    // The silence is what makes this load-bearing rather than cosmetic. The
-    // projection's output is what a caller hands `writeSkills`, and a shortened
-    // list is indistinguishable there from "that skill is no longer requested" —
-    // so with `prune: true` (the default) a silently dropped entry *deletes the
-    // skill's files*. One warning per drop, each naming the position, is what
-    // lets an operator find the offending entry.
-    const spy = vi.spyOn(console, 'warn').mockImplementation(() => {});
-    try {
-      const refs = skillRefs({
-        ...base,
-        skills: [
-          { key: 'ok', version: 1 },
-          // Not an object at all.
-          'nope' as unknown as { key: string; version: number },
-          // An invalid key.
-          { key: 'Not/A/Key', version: 1 },
-          // An invalid version.
-          { key: 'also-ok', version: 0 },
-          { key: 'also-ok', version: 2 },
-        ],
-      });
+  it.each([
+    ['null', null],
+    ['a string', 'pdf-extraction'],
+    ['an object', { key: 'pdf-extraction', version: 1 }],
+    ['a number', 3],
+    ['a bare string entry', ['pdf-extraction']],
+    ['a null entry', [null]],
+    ['a nested array entry', [['pdf-extraction', 1]]],
+    ['an absent key', [{ version: 1 }]],
+    ['a numeric key', [{ key: 7, version: 1 }]],
+    ['a null key', [{ key: null, version: 1 }]],
+    ['an uppercase key', [{ key: 'Evil', version: 1 }]],
+    ['a leading-dash key', [{ key: '-skill', version: 1 }]],
+    ['a leading-dot key', [{ key: '.hidden', version: 1 }]],
+    ['a path-separator key', [{ key: 'a/b', version: 1 }]],
+    ['a backslash key', [{ key: 'a\\b', version: 1 }]],
+    ['a traversal key', [{ key: '../evil', version: 1 }]],
+    ['an empty key', [{ key: '', version: 1 }]],
+    ['a key with a space', [{ key: 'has space', version: 1 }]],
+    ['an underscore key', [{ key: 'has_underscore', version: 1 }]],
+    ['a key with a trailing newline', [{ key: 'pdf-extraction\n', version: 1 }]],
+    ['a 257-character key', [{ key: 'a'.repeat(257), version: 1 }]],
+    ['an absent version', [{ key: 'a' }]],
+    ['version 0', [{ key: 'a', version: 0 }]],
+    ['a negative version', [{ key: 'a', version: -1 }]],
+    ['a non-integer version', [{ key: 'a', version: 2.5 }]],
+    ['a string version', [{ key: 'a', version: '2' }]],
+    ['a boolean version', [{ key: 'a', version: true }]],
+    ['a null version', [{ key: 'a', version: null }]],
+    ['a NaN version', [{ key: 'a', version: Number.NaN }]],
+    ['an Infinity version', [{ key: 'a', version: Number.POSITIVE_INFINITY }]],
+  ])('throws for a malformed skills field: %s', (_label, skills) => {
+    // `parseAiConfig` passes `skills` through, so it is validated here.
+    // Returning `[]` or a shortened list instead would let `writeSkills` with
+    // `prune: true` (the default) delete skills the config still references.
+    // `skills: null` is included: read as "no skills" it would prune everything.
+    expect(() => skillRefs({ ...base, skills })).toThrow(TypeError);
+    expect(() => skillRefs({ ...base, skills })).toThrow(/skills/);
+  });
+
+  it('rejects the whole field when one entry among several is invalid', () => {
+    const skills = [
+      { key: 'good-one', version: 1 },
+      { key: 'BAD', version: 1 },
+      { key: 'good-two', version: 2 },
+    ];
+    expect(() => skillRefs({ ...base, skills })).toThrow(/skills\[1\]\.key/);
+  });
 
-      // The usable entries survive, in order.
-      expect(refs).toEqual([
-        { key: 'ok', version: 1 },
-        { key: 'also-ok', version: 2 },
-      ]);
+  it('does not echo the rejected key in the error', () => {
+    expect(() => skillRefs({ ...base, skills: [{ key: 'Secret-Name', version: 1 }] })).toThrow(
+      expect.objectContaining({ message: expect.not.stringContaining('Secret-Name') }),
+    );
+  });
 
-      const lines = spy.mock.calls.map(([line]) => String(line));
-      expect(lines).toHaveLength(3);
-      // Each names its own position, so three drops are three distinct reports
-      // rather than one summary an operator cannot act on.
-      expect(lines[0]).toContain('skills[1]');
-      expect(lines[1]).toContain('skills[2]');
-      expect(lines[2]).toContain('skills[3]');
-      for (const line of lines) expect(line).toContain('dropped from the projection');
-    } finally {
-      spy.mockRestore();
-    }
+  it('accepts a key of exactly 256 characters and version 1', () => {
+    // The accepting side of the <= 256 bound. It is only observable at the pure
+    // layers: a key becomes one directory name and NAME_MAX is 255, so
+    // `writeSkills` can never reach it.
+    const key = 'a'.repeat(256);
+    expect(skillRefs({ ...base, skills: [{ key, version: 1 }] })).toEqual([{ key, version: 1 }]);
   });
 
   it('warns about nothing when every entry is usable', async () => {