From bfb699687108320cc444f1dc321e4faf1b3d2d98 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Fri, 2 Oct 2026 12:55:16 -0400 Subject: [PATCH 1/3] fix(skills): close four behaviour gaps with the Python SDK - FDv2SkillStore.start() resumes a store that gave up (fatal status or exhausted retry budget), clearing failed and resetting the retry budget. It was a silent no-op. close() stays final. - SkillObjectSet.get misses a pin for a version that is not held when valid versions exist, instead of serving a version-less entry and recording an integrity failure against a healthy skill. - writeSkills caps the manifest read at MAX_MANIFEST_BYTES (8 MiB) and treats a larger file as corrupt. readRegularFile now requires maxBytes. - getSkills and allSkills log one warning per call when content was withheld, with a stronger message when nothing verified. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 4 +- packages/client/agents.md | 4 +- .../client/src/__tests__/skills-fdv2.test.ts | 50 ++++++++++++++++ .../skills-fs-byte-integrity.test.ts | 10 ++-- .../client/src/__tests__/skills-fs.test.ts | 35 +++++++++++- packages/client/src/__tests__/skills.test.ts | 57 +++++++++++++++++++ packages/client/src/skills-core.ts | 25 ++++++++ packages/client/src/skills-fdv2.ts | 30 ++++++---- packages/client/src/skills-fs.ts | 28 ++++++--- packages/client/src/skills.ts | 10 +++- 10 files changed, 221 insertions(+), 32 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 9bff5b6c..407cc6e9 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -325,7 +325,7 @@ const report = await writeSkills('*', '.claude/skills', { | `skillRefs(config)` | Project a config's `skills` array into typed `SkillReference[]`. Pure — no client, no store, no telemetry. `[]` when absent. | | `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. | +| `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. | | `allSkills()` | Every verified skill the store holds, newest version per key. | | `writeSkills(skills, root, options?)` | Materialize to `//SKILL.md`. Accepts `Skill` / `SkillReference` / key strings, or the literal `'*'`. Returns a `ReconcileReport`. Throws for a caller error (an unusable `root`, a bare string other than `'*'`), distinct from the per-skill `error` actions in the report. | | `InMemorySkillStore` | An in-memory `SkillStore` for local development and testing: `put(raw)`, `getObject(kind, key, version?)`, `allObjects(kind)`, `addListener(kind, fn)`, `removeListener(kind, fn)`. Holds several versions of a key: `getObject` answers a pin with exactly that version and an omitted version with the newest. `addListener` throws for any kind but `'skill'`. | @@ -419,7 +419,7 @@ try { - **Causes:** a view-scoped SDK key, which cannot be assigned a skill payload (use a key that is not view-scoped), or Agent Skills delivery not being enabled for your account (contact LaunchDarkly support). - **What the store does:** retrying cannot help, so it gives up. `failed` carries the reason, `lastError` is set, and `waitForSkills` resolves `false` immediately instead of at your timeout. The 422 does not count toward `connectionFailures`, which tracks recoverable failures only. - **Not the empty case:** an environment with zero skills is served an empty payload that commits normally. -- **Recovery:** `start()` does nothing on a store that has given up, so a process that booted while the cause was in effect picks up skills only after a restart. +- **Recovery:** once the cause is fixed, call `start()` on the same store. Only `close()` is final: a restart clears `failed`, resets the retry budget, and held content stays readable throughout. Restarting the process also works. **Nothing above the store changes.** The accessors, integrity verification, and `writeSkills` see raw objects through the `SkillStore` interface and cannot tell which store produced them. diff --git a/packages/client/agents.md b/packages/client/agents.md index 70ff7e04..5bae4039 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -126,7 +126,7 @@ The Python SDK enforces the same pair with the same wording; change both or neit **The store refuses two things loudly rather than degrading.** - `addListener` throws for any kind but `'skill'`, in `FDv2SkillStore` and `InMemorySkillStore` alike: a listener on another kind would silently never fire. `removeListener` accepts any kind, so a consumer can detach unconditionally. -- `close` is final: `start` throws afterwards rather than opening a second delivery loop. Closing leaves `failed` as `null` (it is the caller's decision, not a delivery failure), and both a closed store and one that gave up answer `waitForSkills` immediately. +- `close` is final: `start` throws afterwards rather than opening a second delivery loop. A store that gave up on its own is different: `start` runs delivery again with a fresh retry budget and clears `failed`, and is a no-op while delivery is running. Closing leaves `failed` as `null` (it is the caller's decision, not a delivery failure), and both a closed store and one that gave up answer `waitForSkills` immediately. --- @@ -503,7 +503,7 @@ Without `watchSkills`, the revocation bound is process lifetime: a skill revoked ### 5. Relaxing a path or manifest check in `skills-fs.ts` -`keyRejectionReason` and `unsafePathReason` are shared by the write and prune paths so the two cannot disagree about which paths this SDK may destroy, and both are **non-relaxable**. The same goes for the manifest rules: a destructive operation is allowed only on a path the manifest lists under a matching key, and a corrupt manifest suppresses every destructive action. Each has a dedicated abuse-case test in `src/__tests__/skills-fs.test.ts`; if one starts failing, the defense changed, not the test. +`keyRejectionReason` and `unsafePathReason` are shared by the write and prune paths so the two cannot disagree about which paths this SDK may destroy, and both are **non-relaxable**. The same goes for the manifest rules: a destructive operation is allowed only on a path the manifest lists under a matching key, and a corrupt manifest (including one larger than `MAX_MANIFEST_BYTES`, 8 MiB) suppresses every destructive action. Each has a dedicated abuse-case test in `src/__tests__/skills-fs.test.ts`; if one starts failing, the defense changed, not the test. Three specifics a later contributor is most likely to widen: diff --git a/packages/client/src/__tests__/skills-fdv2.test.ts b/packages/client/src/__tests__/skills-fdv2.test.ts index 2de86a26..fddeda68 100644 --- a/packages/client/src/__tests__/skills-fdv2.test.ts +++ b/packages/client/src/__tests__/skills-fdv2.test.ts @@ -1194,6 +1194,16 @@ describe('the held object set', () => { expect(filled().get('malformed', 7)?.content).toBe('q'); }); + it('misses a pin for a missing version rather than serving a version-less entry', () => { + // With valid versions held, the version-less entry is not this pin's object: + // serving it would record an integrity failure against a healthy skill. + const set = filled(); + set.put({ key: 'a', version: 'not-a-version', content: 'q' }); + expect(set.get('a', 99)).toBeNull(); + expect(set.get('a', 1)?.content).toBe('x'); + expect(set.get('a', null)?.content).toBe('y'); + }); + it('answers null for a key it does not hold', () => { expect(filled().get('missing', null)).toBeNull(); expect(filled().get('a', 99)).toBeNull(); @@ -2285,6 +2295,46 @@ describe('failure handling', () => { expect(await readFile(path.join(stale, 'SKILL.md'), 'utf8')).toBe('not ours to delete'); }); + it('resumes a store that gave up on a 422 when start is called again', async () => { + // A 422 is never retried on its own, but once the cause is fixed (a new key, + // or delivery enabled for the account) the application can resume the same + // store without restarting the process. + endpoint.defaultPollStatus = 422; + const store = pollStore(); + store.start(); + expect(await waitUntil(() => store.failed !== null)).toBe(true); + + endpoint.queuePoll(fullPayload([['put-object', putSkill()]])); + expect(store.start()).toBe(store); + expect(store.failed).toBeNull(); + expect(await store.waitForSkills(5000)).toBe(true); + expect(store.getObject('skill', 'pdf-extraction')).not.toBeNull(); + }); + + it('points the give-up log line at start(), not only a process restart', async () => { + endpoint.defaultPollStatus = 422; + const store = pollStore(); + store.start(); + expect(await waitUntil(() => store.failed !== null)).toBe(true); + expect(consoleErrors()).toMatch(/call start\(\) on this store/); + }); + + it('gives a restarted store a fresh retry budget', async () => { + // Without the reset, the restarted run would inherit the spent count and give + // up on its first recoverable failure without retrying once. + const store = pollStore({ maxConsecutiveFailures: 1 }); + endpoint.queuePoll([], { status: 500 }); + endpoint.queuePoll([], { status: 500 }); + store.start(); + expect(await waitUntil(() => store.failed !== null)).toBe(true); + + endpoint.queuePoll([], { status: 500 }); + endpoint.queuePoll(fullPayload([['put-object', putSkill()]])); + store.start(); + expect(await store.waitForSkills(5000)).toBe(true); + expect(store.failed).toBeNull(); + }); + it('makes backoff exponential and capped', () => { expect(backoffDelayMs(1, 1000, 30_000, 0)).toBe(1000); expect(backoffDelayMs(2, 1000, 30_000, 0)).toBe(2000); diff --git a/packages/client/src/__tests__/skills-fs-byte-integrity.test.ts b/packages/client/src/__tests__/skills-fs-byte-integrity.test.ts index edb07b47..22640a08 100644 --- a/packages/client/src/__tests__/skills-fs-byte-integrity.test.ts +++ b/packages/client/src/__tests__/skills-fs-byte-integrity.test.ts @@ -237,16 +237,16 @@ describe('the adoption comparison read is bounded', () => { expect(hook.reads.get(SKILL_MD)).toBe(encoded.byteLength); }); - it('reads the manifest unbounded, because its length is not predictable', async () => { - // The counterpart: `maxBytes` is opt-in, and the manifest does not opt in. - // It is parsed rather than compared, no caller can predict its length, and it - // is the one file under the root this SDK writes itself. + it('reads the manifest through a bounded read, not to EOF', async () => { + // Its length is not predictable, but it lives in a directory the SDK does not + // own exclusively, so the read is capped at `MAX_MANIFEST_BYTES`. await writeSkills([skill('a')], root); disarm(); await writeSkills([skill('a')], root); - expect(hook.wholeFileReads.has('.launchdarkly-skills.json')).toBe(true); + expect(hook.wholeFileReads.has('.launchdarkly-skills.json')).toBe(false); + expect(hook.reads.get('.launchdarkly-skills.json')).toBeGreaterThan(0); }); }); diff --git a/packages/client/src/__tests__/skills-fs.test.ts b/packages/client/src/__tests__/skills-fs.test.ts index eb3c2ecd..f878b7e1 100644 --- a/packages/client/src/__tests__/skills-fs.test.ts +++ b/packages/client/src/__tests__/skills-fs.test.ts @@ -29,7 +29,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { fsOps, SUPPORTS_DIR_FD, SUPPORTS_PROC_FD } from '../safe-fs.js'; import { _clearState, _setEmitterForTesting, _setStore, InMemorySkillStore, skillRefs } from '../skills.js'; import { MAX_SKILL_CONTENT_BYTES, recordRevoked } from '../skills-core.js'; -import { writeSkills } from '../skills-fs.js'; +import { MAX_MANIFEST_BYTES, writeSkills } from '../skills-fs.js'; import type { RawSkillObject, ReconcileAction, ReconcileReport, Skill, SkillStore } from '../types.js'; import { createSkill, isValidSkillKey, parseAiConfig } from '../types.js'; @@ -1967,6 +1967,39 @@ const CORRUPT_MANIFESTS: Array<[string, unknown]> = [ const LIVE_ENTRY_MANIFESTS = CORRUPT_MANIFESTS.filter(([name]) => name.endsWith('_live_entries')); +describe('writeSkills oversize manifest', () => { + // The manifest is a plain file in a directory the SDK does not own + // exclusively, so an unbounded read of it could exhaust the process. + it('refuses a manifest over the cap without destroying anything', async () => { + const target = await placeManaged(root, 'a', SKILL_BODY); + // Derived from the cap, so raising the bound cannot leave this test passing. + await writeManifest(root, { manifestVersion: 1, entries: {}, pad: 'x'.repeat(MAX_MANIFEST_BYTES + 1) }); + const unlinks = interceptUnlink(); + + const report = await writeSkills([], root); + + expect(report.ok).toBe(false); + expect(errorMessages(report).some((m) => m.includes('cap'))).toBe(true); + expect(unlinks).toEqual([]); + expect(await readFile(target, 'utf-8')).toBe(SKILL_BODY); + // Left for an operator rather than overwritten. + expect((await readManifest(root)).pad).toBeDefined(); + }); + + it('still reads a manifest exactly at the cap', async () => { + const manifest: Record = { manifestVersion: 1, entries: {} }; + const overhead = Buffer.byteLength(JSON.stringify({ ...manifest, pad: '' }), 'utf-8'); + manifest.pad = 'x'.repeat(MAX_MANIFEST_BYTES - overhead); + await writeManifest(root, manifest); + expect((await stat(manifestPath(root))).size).toBe(MAX_MANIFEST_BYTES); + + const report = await writeSkills([], root); + + expect(report.ok).toBe(true); + expect((await readManifest(root)).pad).toBeDefined(); + }); +}); + describe('writeSkills corrupt manifest', () => { it.each(CORRUPT_MANIFESTS)('performs no destructive action and reports an error: %s', async (_label, raw) => { const target = path.join(root, 'a', SKILL_MD); diff --git a/packages/client/src/__tests__/skills.test.ts b/packages/client/src/__tests__/skills.test.ts index 07e85152..2b26c243 100644 --- a/packages/client/src/__tests__/skills.test.ts +++ b/packages/client/src/__tests__/skills.test.ts @@ -1218,6 +1218,63 @@ describe('getSkills', () => { }); }); +// ─── Withholding summary ─────────────────────────────────────────────── + +describe('withholding summary', () => { + let store: InMemorySkillStore; + let warnSpy: ReturnType; + const warnings = (): string[] => warnSpy.mock.calls.map((call) => String(call[0])); + const tampered = (key: string): RawSkillObject => rawSkill({ key, contentHash: '0'.repeat(64) }); + + beforeEach(() => { + store = new InMemorySkillStore(); + _setStore(store); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('warns once, naming the content hash, when allSkills withholds everything', async () => { + store.put(tampered('a')); + expect(await allSkills()).toEqual([]); + expect(warnings()).toHaveLength(1); + expect(warnings()[0]).toMatch(/contentHash/); + }); + + it('warns with the counts when allSkills withholds some', async () => { + store.put(rawSkill({ key: 'good' })); + store.put(tampered('bad')); + expect((await allSkills()).map((s) => s.key)).toEqual(['good']); + expect(warnings()).toHaveLength(1); + expect(warnings()[0]).toMatch(/1 of 2/); + }); + + it('warns once per getSkills batch, not once per skill', async () => { + store.put(tampered('a')); + store.put(tampered('b')); + expect(await getSkills(['a', 'b'])).toEqual([]); + expect(warnings()).toHaveLength(1); + }); + + it('does not warn when nothing was withheld', async () => { + store.put(rawSkill({ key: 'a' })); + expect(await getSkills(['a'])).toHaveLength(1); + expect(await allSkills()).toHaveLength(1); + expect(warnings()).toEqual([]); + }); + + it('does not count a key as withheld when another version of it resolved', async () => { + // A malformed object beside a well-formed version of the same key. + store.put(rawSkill({ key: 'a', version: 1 })); + store.put(rawSkill({ key: 'a', version: 'not-a-version' as unknown as number })); + expect((await allSkills()).map((s) => s.key)).toEqual(['a']); + expect(warnings()).toEqual([]); + }); +}); + // ─── allSkills ───────────────────────────────────────────────────────── describe('allSkills', () => { diff --git a/packages/client/src/skills-core.ts b/packages/client/src/skills-core.ts index b34f9c6d..5f3a4f1c 100644 --- a/packages/client/src/skills-core.ts +++ b/packages/client/src/skills-core.ts @@ -440,6 +440,31 @@ export function verifyRawSkill(raw: unknown): Skill | null { } /** The one wording for "the store could not answer", used by every path. */ +/** + * Logs one warning per batch when content was withheld, with the counts. + * + * Makes withholding visible, especially when nothing verified and the empty + * result would look like "no skills". + */ +export function logWithholdingSummary(subject: string, requested: number, resolved: number): void { + const withheld = requested - resolved; + if (withheld <= 0) return; + if (resolved === 0) { + // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; an empty result must not read as "no skills" + console.warn( + `[LaunchDarkly] All ${requested} ${subject} were withheld and no skill content is available. Every ` + + 'object failed verification — check that the delivered objects carry a contentHash matching the ' + + 'sha256 of their content.', + ); + return; + } + // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; withheld skills must be visible + console.warn( + `[LaunchDarkly] ${withheld} of ${requested} ${subject} were withheld and are unavailable; see the ` + + 'preceding errors for the per-skill reason.', + ); +} + export function storeThrew(error: unknown): string { const name = error instanceof Error ? error.constructor.name : 'unknown error'; const message = error instanceof Error ? error.message : String(error); diff --git a/packages/client/src/skills-fdv2.ts b/packages/client/src/skills-fdv2.ts index 8be5a832..b9c26de2 100644 --- a/packages/client/src/skills-fdv2.ts +++ b/packages/client/src/skills-fdv2.ts @@ -501,16 +501,15 @@ export class SkillObjectSet { /** * The object for `key` at `version`, or the newest held when `version` is null. - * Falls back to the version-less entry so verification can report it. + * + * When only a version-less entry exists it is served so verification can + * report it. A pin that misses while valid versions exist is a plain miss. */ get(key: string, version: number | null): RawSkillObject | null { const held = this.versions.get(key); - if (version !== null) return held?.get(version) ?? this.loose.get(key) ?? null; - if (held && held.size > 0) { - const newest = Math.max(...held.keys()); - return held.get(newest) ?? null; - } - return this.loose.get(key) ?? null; + if (!held || held.size === 0) return this.loose.get(key) ?? null; + if (version !== null) return held.get(version) ?? null; + return held.get(Math.max(...held.keys())) ?? null; } /** @@ -1533,8 +1532,8 @@ export class FDv2SkillStore implements SkillStore { * * Does not await: use `waitForSkills` when boot ordering matters. * - * Throws if the store has been closed. Once delivery has stopped on its own - * (`failed` is set), calling `start` again does nothing. + * Throws if the store has been closed. A store whose delivery stopped on its + * own (`failed` is set) can be started again, with a fresh retry budget. */ start(): this { if (this.closed) { @@ -1543,7 +1542,10 @@ export class FDv2SkillStore implements SkillStore { 'A closed store still answers from the content it received, so retrieval needs no restart.', ); } - if (this.loop !== null) return this; + if (this.loop !== null && this.failedReason === null) return this; + // A loop that gave up has already returned, so a new one cannot overlap it. + this.failedReason = null; + this.failures = 0; this.controller = new AbortController(); this.loop = this.run(this.controller.signal); return this; @@ -1627,7 +1629,10 @@ export class FDv2SkillStore implements SkillStore { return this.firstPayload; } - /** Why delivery stopped for good, or `null` while it is running. */ + /** + * Why delivery stopped, or `null` while it is running. Cleared when `start` + * runs delivery again. + */ get failed(): string | null { return this.failedReason; } @@ -1789,7 +1794,8 @@ export class FDv2SkillStore implements SkillStore { this.reader.diagnostics.lastError = reason; error( `Skill delivery has stopped and will not retry: ${reason}. The store keeps serving the last content it ` + - 'received; skills will not update until the process restarts with a working connection.', + 'received, and skills will not update until delivery runs again: call start() on this store once the ' + + 'cause is fixed, or restart the process.', ); // Release `waitForSkills` callers now; they resolve `false`. this.releaseWaiters(); diff --git a/packages/client/src/skills-fs.ts b/packages/client/src/skills-fs.ts index 7079336c..c08570ff 100644 --- a/packages/client/src/skills-fs.ts +++ b/packages/client/src/skills-fs.ts @@ -71,6 +71,13 @@ export const MANIFEST_FILENAME = '.launchdarkly-skills.json'; /** Manifest schema version this release writes, and the highest it can read. */ export const MANIFEST_VERSION = 1; +/** + * Hard cap on the manifest read, far above any real manifest. A larger file is + * treated as corrupt rather than read into memory. Not exported from the + * package index. + */ +export const MAX_MANIFEST_BYTES = 8 * 1024 * 1024; + /** The single file each skill materializes to, under `//`. */ export const SKILL_FILENAME = 'SKILL.md'; @@ -621,9 +628,9 @@ async function loadManifest( ): Promise<{ manifest: Record; error: string | null }> { const fresh = { manifestVersion: MANIFEST_VERSION, entries: {} }; - let text: string; + let raw: Buffer; try { - text = (await readRegularFile(path.join(root.address, MANIFEST_FILENAME))).toString('utf-8'); + raw = await readRegularFile(path.join(root.address, MANIFEST_FILENAME), MAX_MANIFEST_BYTES); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { manifest: fresh, error: null }; return { @@ -631,6 +638,14 @@ async function loadManifest( error: `the skills manifest ${MANIFEST_FILENAME} could not be read: ${messageOf(error)}`, }; } + // The read stops one byte past the cap, which is enough to detect an overage. + if (raw.byteLength > MAX_MANIFEST_BYTES) { + return { + manifest: {}, + error: `the skills manifest ${MANIFEST_FILENAME} is larger than the ${MAX_MANIFEST_BYTES} byte cap; refusing every destructive action`, + }; + } + const text = raw.toString('utf-8'); // Non-UTF-8 bytes decode as U+FFFD, so they surface as invalid JSON. let data: unknown; @@ -850,18 +865,17 @@ async function writeOne( * - `O_NOFOLLOW`: refuses a trailing symlink. * - The type check uses `stat` on the handle, not the path. * - * With `maxBytes`, reads at most `maxBytes + 1` bytes; the extra byte keeps a - * longer file from hashing equal to its prefix. `undefined` reads to EOF. + * Reads at most `maxBytes + 1` bytes; the extra byte keeps a longer file from + * hashing equal to its prefix. `maxBytes` is required so no call site can read + * unbounded. * * Throws for anything the caller must turn into a refusal. */ -async function readRegularFile(target: string, maxBytes?: number): Promise { +async function readRegularFile(target: string, maxBytes: number): Promise { const flags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0) | (fsConstants.O_NONBLOCK ?? 0); const handle = await open(target, flags); try { if (!(await handle.stat()).isFile()) throw new Error('the path is not a regular file'); - if (maxBytes === undefined) return await handle.readFile(); - // Buffer sized to the bound, not the file; looped since `read` may return short. const limit = maxBytes + 1; const buffer = Buffer.alloc(limit); diff --git a/packages/client/src/skills.ts b/packages/client/src/skills.ts index 4cf12050..ac1db34a 100644 --- a/packages/client/src/skills.ts +++ b/packages/client/src/skills.ts @@ -12,6 +12,7 @@ import { allRawObjects, clearState, + logWithholdingSummary, newestByKey, referenceTarget, requireStore, @@ -275,7 +276,7 @@ export async function getSkillResult(key: string, options: { version?: number } * @param refs `SkillReference` values and/or bare key strings (a string means the * newest version). * @returns The skills found, in input order. Entries that are missing, at the - * wrong version, or fail verification are omitted. + * wrong version, or fail verification are omitted, and a warning logs how many. * @throws TypeError if `refs` is a single string; pass `[key]` instead. * @throws Error if no skill store is configured. */ @@ -295,6 +296,7 @@ export async function getSkills(refs: ReadonlyArray): P const { skill } = resolveFromStore(store, key, wanted); if (skill) skills.push(skill); } + logWithholdingSummary('requested skills', refs.length, skills.length); return skills; } @@ -302,7 +304,7 @@ export async function getSkills(refs: ReadonlyArray): P * Retrieves every verified skill the store currently holds. * * Returns the newest version of each key. Skills that fail verification are - * omitted. + * omitted, and a warning logs how many. * * @throws Error if no skill store is configured. */ @@ -311,10 +313,12 @@ export async function allSkills(): Promise { if (error !== null) return []; // The store may hold several versions per key; keep only the newest. + const candidates = newestByKey(objects); const skills: Skill[] = []; - for (const { raw } of newestByKey(objects)) { + for (const { raw } of candidates) { const skill = verifyRawSkill(raw); if (skill) skills.push(skill); } + logWithholdingSummary('skills held by the store', candidates.length, skills.length); return skills; } From 76c300ccc331e994a918b1d49e2c3dc798e8963f Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Mon, 5 Oct 2026 12:44:07 -0400 Subject: [PATCH 2/3] fix(skills): count only served skills in the getSkills withholding warning getSkills passed the full request count to logWithholdingSummary, so an absent key, a pin miss, a wrong-version answer, or a store that threw was reported as withheld. When nothing resolved, the warning blamed contentHash: a getSkills call against an empty store at boot raised a false integrity alarm. Only resolutions where the store served an object (ok or integrity_failure) now count toward the summary. allSkills already counted only what the store held. Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 2 +- packages/client/src/__tests__/skills.test.ts | 32 ++++++++++++++++++++ packages/client/src/skills.ts | 11 +++++-- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 407cc6e9..34723c54 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -325,7 +325,7 @@ const report = await writeSkills('*', '.claude/skills', { | `skillRefs(config)` | Project a config's `skills` array into typed `SkillReference[]`. Pure — no client, no store, no telemetry. `[]` when absent. | | `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. | +| `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. | | `allSkills()` | Every verified skill the store holds, newest version per key. | | `writeSkills(skills, root, options?)` | Materialize to `//SKILL.md`. Accepts `Skill` / `SkillReference` / key strings, or the literal `'*'`. Returns a `ReconcileReport`. Throws for a caller error (an unusable `root`, a bare string other than `'*'`), distinct from the per-skill `error` actions in the report. | | `InMemorySkillStore` | An in-memory `SkillStore` for local development and testing: `put(raw)`, `getObject(kind, key, version?)`, `allObjects(kind)`, `addListener(kind, fn)`, `removeListener(kind, fn)`. Holds several versions of a key: `getObject` answers a pin with exactly that version and an omitted version with the newest. `addListener` throws for any kind but `'skill'`. | diff --git a/packages/client/src/__tests__/skills.test.ts b/packages/client/src/__tests__/skills.test.ts index 2b26c243..e04ca4d0 100644 --- a/packages/client/src/__tests__/skills.test.ts +++ b/packages/client/src/__tests__/skills.test.ts @@ -1259,6 +1259,38 @@ describe('withholding summary', () => { expect(warnings()).toHaveLength(1); }); + it('does not warn when getSkills asks for keys the store does not hold', async () => { + // An empty store at boot is not an integrity problem. + expect(await getSkills(['a', 'b'])).toEqual([]); + expect(warnings()).toEqual([]); + }); + + it('does not warn when a getSkills pin misses', async () => { + store.put(rawSkill({ key: 'a', version: 2 })); + expect(await getSkills([{ key: 'a', version: 1 }])).toEqual([]); + expect(warnings()).toEqual([]); + }); + + it('does not warn when the store throws during getSkills', async () => { + const throwing: SkillStore = { + getObject: () => { + throw new Error('down'); + }, + allObjects: () => ({}), + }; + _setStore(throwing); + expect(await getSkills(['a'])).toEqual([]); + expect(warnings()).toEqual([]); + }); + + it('counts only what the store served when getSkills mixes misses and withholding', async () => { + store.put(rawSkill({ key: 'good' })); + store.put(tampered('bad')); + expect((await getSkills(['good', 'bad', 'missing'])).map((s) => s.key)).toEqual(['good']); + expect(warnings()).toHaveLength(1); + expect(warnings()[0]).toMatch(/1 of 2/); + }); + it('does not warn when nothing was withheld', async () => { store.put(rawSkill({ key: 'a' })); expect(await getSkills(['a'])).toHaveLength(1); diff --git a/packages/client/src/skills.ts b/packages/client/src/skills.ts index ac1db34a..087db2c5 100644 --- a/packages/client/src/skills.ts +++ b/packages/client/src/skills.ts @@ -276,7 +276,8 @@ export async function getSkillResult(key: string, options: { version?: number } * @param refs `SkillReference` values and/or bare key strings (a string means the * newest version). * @returns The skills found, in input order. Entries that are missing, at the - * wrong version, or fail verification are omitted, and a warning logs how many. + * wrong version, or fail verification are omitted. A warning logs how many + * failed verification; misses are not counted. * @throws TypeError if `refs` is a single string; pass `[key]` instead. * @throws Error if no skill store is configured. */ @@ -291,12 +292,16 @@ export async function getSkills(refs: ReadonlyArray): P const store = requireStore(); const skills: Skill[] = []; + // Only what the store served counts toward the summary: a miss or an outage is + // not a verification failure, and the summary would report it as one. + let served = 0; for (const ref of refs) { const [key, wanted] = referenceTarget(ref); - const { skill } = resolveFromStore(store, key, wanted); + const { skill, reason } = resolveFromStore(store, key, wanted); if (skill) skills.push(skill); + if (reason === 'ok' || reason === 'integrity_failure') served += 1; } - logWithholdingSummary('requested skills', refs.length, skills.length); + logWithholdingSummary('requested skills the store served', served, skills.length); return skills; } From e071aef806a46c59e31f9617d848eeaad6b7240f Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Mon, 5 Oct 2026 15:29:10 -0400 Subject: [PATCH 3/3] Fixup --- packages/client/src/skills-core.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/client/src/skills-core.ts b/packages/client/src/skills-core.ts index 5f3a4f1c..e938576f 100644 --- a/packages/client/src/skills-core.ts +++ b/packages/client/src/skills-core.ts @@ -439,7 +439,6 @@ export function verifyRawSkill(raw: unknown): Skill | null { }); } -/** The one wording for "the store could not answer", used by every path. */ /** * Logs one warning per batch when content was withheld, with the counts. * @@ -465,6 +464,7 @@ export function logWithholdingSummary(subject: string, requested: number, resolv ); } +/** The one wording for "the store could not answer", used by every path. */ export function storeThrew(error: unknown): string { const name = error instanceof Error ? error.constructor.name : 'unknown error'; const message = error instanceof Error ? error.message : String(error);