From 42968ff008b5a72e8b10ad9cf609ac50f7ff9531 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Wed, 7 Oct 2026 11:25:50 -0400 Subject: [PATCH] fix(client): validate skills in skillRefs, not in parseAiConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseAiConfig` rejected a whole AI Config when its `skills` field was malformed, so a bad experimental field failed `config().invoke()`, `stream()`, graph nodes and `extractVariation` for apps that never use Agent Skills. That breaks TESTING.md §0.3: experimental behaviour must not break a core call. The parser now passes `skills` through unchecked, and `AiConfigRep.skills` is typed `unknown` to say so. `skillRefs` validates it instead and throws `TypeError` when the field is present but malformed, including `skills: null` and a single bad entry. It used to drop bad entries with a warning, which was safe only because the parser had already rejected them. Throwing keeps the original reason for the check: `writeSkills` with `prune: true` never receives an empty or partial list for a field that is present. An absent field, or a nullish config, still returns `[]`. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 2 +- packages/client/agents.md | 7 +- packages/client/etc/ai-server.api.md | 5 +- .../client/src/__tests__/lifecycle.test.ts | 24 ++++ packages/client/src/__tests__/schema.test.ts | 113 ++++-------------- .../client/src/__tests__/skills-fs.test.ts | 13 +- packages/client/src/__tests__/skills.test.ts | 97 +++++++++------ packages/client/src/skills.ts | 69 ++++++----- packages/client/src/types.ts | 37 ++---- 9 files changed, 169 insertions(+), 198 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 7abc0580..9261c26a 100644 --- a/packages/client/README.md +++ b/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. | diff --git a/packages/client/agents.md b/packages/client/agents.md index 9bb978bc..55dc4840 100644 --- a/packages/client/agents.md +++ b/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: diff --git a/packages/client/etc/ai-server.api.md b/packages/client/etc/ai-server.api.md index 4a45ad1f..5fe70199 100644 --- a/packages/client/etc/ai-server.api.md +++ b/packages/client/etc/ai-server.api.md @@ -32,10 +32,7 @@ export type AiConfigRep = { name: string; }; outputFormat?: Record; - skills?: ReadonlyArray<{ - readonly key: string; - readonly version: number; - }>; + skills?: unknown; }; // @public diff --git a/packages/client/src/__tests__/lifecycle.test.ts b/packages/client/src/__tests__/lifecycle.test.ts index ad950c53..3fb7a9f6 100644 --- a/packages/client/src/__tests__/lifecycle.test.ts +++ b/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({ diff --git a/packages/client/src/__tests__/schema.test.ts b/packages/client/src/__tests__/schema.test.ts index 2352896b..876b90b1 100644 --- a/packages/client/src/__tests__/schema.test.ts +++ b/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); }); }); }); diff --git a/packages/client/src/__tests__/skills-fs.test.ts b/packages/client/src/__tests__/skills-fs.test.ts index f878b7e1..a1c9ee59 100644 --- a/packages/client/src/__tests__/skills-fs.test.ts +++ b/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({ diff --git a/packages/client/src/__tests__/skills.test.ts b/packages/client/src/__tests__/skills.test.ts index 4905483f..7af2b40a 100644 --- a/packages/client/src/__tests__/skills.test.ts +++ b/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 () => { diff --git a/packages/client/src/skills.ts b/packages/client/src/skills.ts index 1d477768..02011c01 100644 --- a/packages/client/src/skills.ts +++ b/packages/client/src/skills.ts @@ -23,7 +23,13 @@ import { verifyRawSkill, } from './skills-core.js'; import type { AiConfigRep, RawSkillObject, Skill, SkillOutcome, SkillReference, SkillStore } from './types.js'; -import { createSkillOutcome, createSkillReference, isValidSkillKey, isValidSkillVersion } from './types.js'; +import { + createSkillOutcome, + createSkillReference, + isValidSkillKey, + isValidSkillVersion, + SKILL_KEY_MAX_LENGTH, +} from './types.js'; // --------------------------------------------------------------------------- // Injection points @@ -213,46 +219,53 @@ export class InMemorySkillStore implements SkillStore { // --------------------------------------------------------------------------- /** - * Warns that one `skills` entry was dropped. Never silent: `writeSkills` with - * `prune: true` would delete a dropped skill's files. + * Why a present `skills` field is malformed, or `null` when it is valid. One + * malformed reference rejects the whole field, never a partial list. */ -function warnDropped(index: number, why: string): void { - // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; a dropped skill reference must be visible - console.warn(`[LaunchDarkly] skills[${index}]${why}; it was dropped from the projection`); +function skillsFieldRejectionReason(raw: unknown): string | null { + if (!Array.isArray(raw)) return 'skills must be an array of {key, version} objects'; + + for (const [index, entry] of raw.entries()) { + if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) { + return `skills[${index}] must be an object with key and version`; + } + const { key, version } = entry as { key?: unknown; version?: unknown }; + if (!isValidSkillKey(key)) { + return `skills[${index}].key must be a string matching ^[a-z0-9][a-z0-9-]*$ of at most ${SKILL_KEY_MAX_LENGTH} characters`; + } + if (!isValidSkillVersion(version)) return `skills[${index}].version must be an integer >= 1`; + } + return null; } /** * Returns the skill references attached to a resolved AI Config. * * Pure: no network, store, or telemetry. Returns `[]` when the config has no - * skills. Typical use: `await getSkills(skillRefs(config))`. + * `skills` field, or when `config` is not an object (for example `null` from a + * failed `inspectConfig`). Typical use: `await getSkills(skillRefs(config))`. + * + * `parseAiConfig` does not validate `skills`, so a malformed field does not + * fail core config calls. It is validated here instead, and rejected whole: + * `writeSkills` with `prune: true` would delete the files of any skill missing + * from the list, so a partial or empty list is never returned for a field that + * is present. * - * Invalid entries (possible only in a hand-built object; `parseAiConfig` rejects - * them) are dropped with a warning, because `writeSkills` with `prune: true` - * would delete a dropped skill's files. + * @throws TypeError if `skills` is present but is not an array of + * `{ key, version }` objects with a valid key and an integer version >= 1, + * including `skills: null`. */ export function skillRefs(config: AiConfigRep | null | undefined): SkillReference[] { if (typeof config !== 'object' || config === null) return []; - const raw = (config as { skills?: unknown }).skills; - if (!Array.isArray(raw)) return []; + const raw: unknown = config.skills; + if (raw === undefined) return []; - const refs: SkillReference[] = []; - for (const [index, entry] of raw.entries()) { - if (typeof entry !== 'object' || entry === null) { - warnDropped(index, ' is not a { key, version } object'); - continue; - } - const { key, version } = entry as { key?: unknown; version?: unknown }; - if (!isValidSkillKey(key)) { - warnDropped(index, '.key must be 1–256 characters matching /^[a-z0-9][a-z0-9-]*$/'); - } else if (!isValidSkillVersion(version)) { - warnDropped(index, '.version must be an integer >= 1'); - } else { - refs.push(createSkillReference({ key, version })); - } - } - return refs; + const rejection = skillsFieldRejectionReason(raw); + if (rejection !== null) throw new TypeError(`Invalid skills field in AI Config: ${rejection}`); + return (raw as Array<{ key: string; version: number }>).map(({ key, version }) => + createSkillReference({ key, version }), + ); } // --------------------------------------------------------------------------- diff --git a/packages/client/src/types.ts b/packages/client/src/types.ts index d11c8be6..ddc5ba2f 100644 --- a/packages/client/src/types.ts +++ b/packages/client/src/types.ts @@ -153,11 +153,14 @@ export type AiConfigRep = { */ outputFormat?: Record; /** - * Optional version-pinned skill references attached to this variation, - * validated by `parseAiConfig`. Typed structurally: Agent Skills is - * experimental, so this core type does not name its types. + * Optional version-pinned skill references attached to this variation, as + * delivered. `parseAiConfig` does not validate it, so a malformed field never + * fails a core call; read it with `skillRefs` from + * `@launchdarkly/ai-server/experimental`, which does. Typed `unknown` because + * it is unvalidated, and because Agent Skills is experimental, so this core + * type does not name its types. */ - skills?: ReadonlyArray<{ readonly key: string; readonly version: number }>; + skills?: unknown; }; type ParseResult = { success: true; data: T } | { success: false; error: { message: string } }; @@ -415,25 +418,6 @@ export function isValidSkillVersion(version: unknown): version is number { return typeof version === 'number' && Number.isInteger(version) && version >= 1; } -/** - * Validates the optional `skills` array. Returns an error message or `null`. - * - * Fails closed: one malformed reference fails the whole config, rather than - * silently materializing a partial skill set. - */ -function parseSkills(raw: unknown): string | null { - if (!Array.isArray(raw)) return 'skills must be an array of {key, version} objects'; - - for (const [index, entry] of raw.entries()) { - if (!isObject(entry)) return `skills[${index}] must be an object with key and version`; - if (!isValidSkillKey(entry.key)) { - return `skills[${index}].key must be a string matching ^[a-z0-9][a-z0-9-]*$ of at most ${SKILL_KEY_MAX_LENGTH} characters`; - } - if (!isValidSkillVersion(entry.version)) return `skills[${index}].version must be an integer >= 1`; - } - return null; -} - function parseTool(raw: unknown, key: string): string | null { if (!isObject(raw)) return `tools.${key} must be an object`; if (typeof raw.name !== 'string') return `tools.${key}.name must be a string`; @@ -485,10 +469,9 @@ export function parseAiConfig(raw: unknown): ParseResult { return { success: false, error: { message: 'outputFormat must be an object (JSON Schema)' } }; } - if (raw.skills !== undefined) { - const err = parseSkills(raw.skills); - if (err) return { success: false, error: { message: err } }; - } + // `skills` is passed through unvalidated. Agent Skills is experimental, so a + // malformed field must not fail a core config call (TESTING.md §0.3); + // `skillRefs` rejects it where the references are used. return { success: true, data: raw as AiConfigRep }; }