fix(skills): close four behaviour gaps with the Python SDK

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 failed verification. |
 | `allSkills()` | Every verified skill the store holds, newest version per key. |
 | `writeSkills(skills, root, options?)` | Materialize to `<root>/<key>/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,9 +419,9 @@ 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`, starts `connectionFailures` and the backoff over, and held content stays readable throughout. Restarting the process also works.
 
-**A response over the transport's memory bound also stops delivery.** The store holds at most 64 Mi characters from one poll body or one streamed event, far above any real payload. Past that, nothing from the response is applied, and it is accounted like a 422: `failed` and `lastError` are set, `connectionFailures` does not move, and the store keeps serving what it already held. The payload's size belongs to the environment, not the connection, so a retry would download it again only to be refused the same way. Recovery is the same as for a 422: a store that has given up does not resume, so once the payload is back under the bound, restart the process or construct a new store.
+**A response over the transport's memory bound also stops delivery.** The store holds at most 64 Mi characters from one poll body or one streamed event, far above any real payload. Past that, nothing from the response is applied, and it is accounted like a 422: `failed` and `lastError` are set, `connectionFailures` does not move, and the store keeps serving what it already held. The payload's size belongs to the environment, not the connection, so a retry would download it again only to be refused the same way. Recovery is the same as for a 422: once the payload is back under the bound, call `start()` on the same store, or restart the process.
 
 **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.
 

packages/client/agents.md

@@ -129,7 +129,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 the failure count and backoff started over, 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.
 
 ---
 
@@ -508,7 +508,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:
 

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

@@ -1240,6 +1240,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();
@@ -2569,6 +2579,48 @@ 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('starts the failure count over when a store is restarted', async () => {
+    // Recoverable failures before the fatal one are history once delivery runs
+    // again, so a restarted store does not report them as its own.
+    endpoint.queuePoll([], { status: 500 });
+    endpoint.defaultPollStatus = 422;
+    const store = pollStore();
+    store.start();
+    expect(await waitUntil(() => store.failed !== null)).toBe(true);
+    expect(store.diagnostics.connectionFailures).toBe(1);
+
+    endpoint.defaultPollStatus = 304;
+    endpoint.queuePoll(fullPayload([['put-object', putSkill()]]));
+    store.start();
+    expect(store.diagnostics.connectionFailures).toBe(0);
+    expect(await store.waitForSkills(5000)).toBe(true);
+    expect(store.failed).toBeNull();
+  });
+
   /**
    * A response body that opens with `prefix` and then runs past the transport
    * bound without a newline, in 4 MiB reads. Finite, a little past the bound,

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);
   });
 });
 

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<string, unknown> = { 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);

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

@@ -1218,6 +1218,95 @@ describe('getSkills', () => {
   });
 });
 
+// ─── Withholding summary ───────────────────────────────────────────────
+
+describe('withholding summary', () => {
+  let store: InMemorySkillStore;
+  let warnSpy: ReturnType<typeof vi.spyOn>;
+  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 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);
+    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', () => {

packages/client/src/skills-core.ts

@@ -439,6 +439,31 @@ export function verifyRawSkill(raw: unknown): Skill | null {
   });
 }
 
+/**
+ * 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.',
+  );
+}
+
 /** 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';