fix(client): a broken store answer is not an absent skill (#57)

Ports two Agent Skills store-answer fixes from
launchdarkly/python-ai-sdk (`split/skills-materialization`,
launchdarkly/python-ai-sdk#53) into the Node client.

## What changed

**A non-object listing is a broken store, not an empty one**
(`packages/client/src/skills-core.ts`). `allRawObjects` used to collapse
anything `store.allObjects()` returned that was not an object down to
`{}` with no error. A store that served nothing usable was therefore
indistinguishable from a store holding no skills, which the `'*'`
reconcile read as "every skill was revoked" and pruned accordingly. It
now returns `{ objects, error }`, logs an error naming the returned type
(`the skill store listed skills as null rather than an object`), and
puts that answer on the same footing as a store that threw. A throwing
store is caught in the same place with the same wording (`storeThrew`),
so `allSkills` and the reconcile's `resolveAll` no longer each re-derive
the log line and the message. `allSkills` still returns `[]` on either
failure; `writeSkills('*')` reports an incomplete run and prunes
nothing. Mirrors Python's `list_raw_objects` returning `(objects,
error)`.

**An answer under a different key is withheld** (`resolveFromStore`).
Identity is read off the object itself, so a store answering under a
different key used to hand the caller a `Skill` carrying someone else's
key. The reconcile then wrote that other key's path and, since prune
keys off the request, deleted it in the same pass while reporting `ok`.
After verification, `skill.key !== key` is now withheld with `skill
'<key>' is not available: the store answered under key '<skill.key>'`,
using the same shape as the existing version-mismatch branch.

**Tests.** Accessor side from Python `5817d89` (aliased key withheld by
`getSkill` / `getSkillResult`; non-object listing reported via
`allRawObjects` with the type named) and reconcile side from `4c6d965`
(a non-object listing leaves every managed file and manifest entry
alone; an aliased answer writes nothing, is reported against the
requested key, and never reaches the other key's file). All six fail
against the unpatched source.

Commits mirror Python `5817d89` and `4c6d965`.

## Notes for review

- **Reason token for the key mismatch.** Python's `Resolution` has no
typed outcome, so there is nothing to port there. Here it reuses
`wrong_version`, per the existing branch's shape: the store held an
answer, but not the one that was asked for. If a dedicated token is
wanted it is a cross-language vocabulary change and belongs in its own
PR.
- **`e2b54fd` (an unheld version pin is a miss, not an integrity
failure) is deliberately not ported.** Python's `InMemorySkillStore`
files several versions per key plus a version-less slot, and that commit
changes which one a pinned miss falls back to. Node's
`InMemorySkillStore` holds exactly one object per key and ignores the
version argument, letting `resolveFromStore` refuse a wrong-version
answer afterwards. There is no fallback slot to fix, so the change has
no direct equivalent; whether Node should grow multi-version semantics
is a separate decision.
- **#50 (`xie/skills-fdv2-transport`) sits above
this branch and will need a rebase once this lands**, since
`allRawObjects` changed its return type and `resolveAll` / `allSkills`
changed accordingly. Not rebased here.

## Verification

- `vitest run` in `packages/client`: 652 passed, 2 skipped
- `tsc --noEmit`: clean
- `biome check`: clean

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Hardens the Node client against **untrusted skill store** behavior
(ported from the Python SDK): broken listings and key aliasing no longer
look like “no skills” or succeed while corrupting disk state.
> 
> **Non-object listings** — `allRawObjects` now returns `{ objects,
error }` instead of silently `{}`. Throws and invalid list shapes (null,
array, etc.) are logged with consistent wording via **`storeThrew`**,
and **`writeSkills('*')`** treats a listing error as an **incomplete
run** so **prune does not delete** managed files. **`allSkills`** still
returns `[]` on failure but the error is available on the listing path.
> 
> **Key aliasing** — After verification, **`resolveFromStore`**
withholds answers where **`skill.key !==` the requested key**, with the
same outcome shape as version mismatch (`wrong_version`). That blocks
**`getSkill`** from returning the wrong identity and stops
**`writeSkills`** from writing under one key and pruning under another
in the same reconcile.
> 
> **Tests** cover accessor and filesystem reconcile paths for both
fixes.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
ee5d469. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: XieX

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

@@ -719,6 +719,87 @@ describe('writeSkills resilience', () => {
     );
   });
 
+  it('never prunes on a non-object listing', async () => {
+    // A store that cannot list is not a store holding nothing. A listing
+    // collapsed to "no skills" is indistinguishable from every skill having
+    // been revoked, and prune would then delete every managed file and report a
+    // clean run. The listing failure has to reach the prune gate as an
+    // incomplete run.
+    const noListing: SkillStore = {
+      getObject() {
+        return null;
+      },
+      allObjects() {
+        return null as unknown as Record<string, RawSkillObject>;
+      },
+    };
+    const existing = await placeManaged(root, 'pdf-extraction', SKILL_BODY);
+    _setStore(noListing);
+
+    const report = await writeSkills('*', root);
+
+    expect(report.ok).toBe(false);
+    expect(await readFile(existing, 'utf-8')).toBe(SKILL_BODY);
+    expect(report.actions.map((a) => a.action)).toEqual(['error']);
+    expect(errorMessages(report).some((m) => m.includes('rather than an object'))).toBe(true);
+    // The entry survives, so the next reconcile picks it up.
+    expect(Object.keys((await readManifest(root)).entries as Record<string, unknown>)).toContain(
+      `pdf-extraction/${SKILL_MD}`,
+    );
+  });
+
+  it('writes nothing for an answer served under another key', async () => {
+    // The file is named after the key the object carries, so a store answering
+    // under a different key would write one path and prune another. Left
+    // unchecked, the run wrote the aliased key, then deleted it in the same
+    // pass because prune keys off the request — and reported ok. The requested
+    // key has to be the one the outcome is reported against.
+    const aliasing: SkillStore = {
+      getObject() {
+        return rawSkill('other-key');
+      },
+      allObjects() {
+        return {};
+      },
+    };
+    _setStore(aliasing);
+
+    const report = await writeSkills(['requested-key'], root);
+
+    expect(report.ok).toBe(false);
+    expect(report.actions.map((a) => a.action)).toEqual(['error']);
+    // Reported against the key that was asked for, not the one served.
+    expect(actionsByKey(report)['requested-key'].action).toBe('error');
+    expect(await exists(path.join(root, 'other-key'))).toBe(false);
+    expect((await readManifest(root)).entries).toEqual({});
+  });
+
+  it('an answer served under another key never reaches that key’s file', async () => {
+    // Both keys are requested here, so nothing is prunable and the write itself
+    // is what is under test: unchecked, the object served under the alias is
+    // written to the *other* key's path, clobbering the content that key's own
+    // lookup resolved — and the run still reports ok.
+    const aliased = 'aliased\n';
+    const aliasing: SkillStore = {
+      getObject(_kind, key) {
+        return key === 'other-key' ? rawSkill('other-key') : rawSkill('other-key', 2, aliased);
+      },
+      allObjects() {
+        return {};
+      },
+    };
+    const existing = await placeManaged(root, 'other-key', SKILL_BODY);
+    _setStore(aliasing);
+
+    // The alias is resolved last, so an unchecked write would land on top.
+    const report = await writeSkills(['other-key', 'requested-key'], root);
+
+    expect(report.ok).toBe(false);
+    expect(await readFile(existing, 'utf-8')).toBe(SKILL_BODY);
+    expect(actionsByKey(report)['requested-key'].action).toBe('error');
+    expect(actionsByKey(report)['other-key'].action).toBe('skipped_current');
+  });
+
   it('never corrupts the manifest on an unavailable run', async () => {
     await placeManaged(root, 'a', SKILL_BODY);
     const before = await readManifest(root);

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

@@ -46,7 +46,7 @@ import {
   InMemorySkillStore,
   skillRefs,
 } from '../skills.js';
-import { MAX_SKILL_CONTENT_BYTES } from '../skills-core.js';
+import { allRawObjects, MAX_SKILL_CONTENT_BYTES, requireStore } from '../skills-core.js';
 import type { RawSkillObject, Skill, SkillOutcomeReason, SkillStore } from '../types.js';
 import {
   createReconcileAction,
@@ -603,6 +603,29 @@ describe('getSkill', () => {
   it('returns null for a missing key, never raising', async () => {
     expect(await getSkill('nope')).toBeNull();
   });
+
+  it('withholds a store answering under a different key', async () => {
+    // The key needs the same post-fetch defense the version already has.
+    // Identity is read off the object itself, and the store is untrusted. An
+    // answer served under a different key would otherwise be handed back under
+    // the key the caller asked for while carrying its own.
+    const aliasing: SkillStore = {
+      getObject() {
+        return rawSkill({ key: 'other-key' });
+      },
+      allObjects() {
+        return {};
+      },
+    };
+    _setStore(aliasing);
+
+    expect(await getSkill('asked-for')).toBeNull();
+
+    const outcome = await getSkillResult('asked-for');
+    expect(outcome.skill).toBeNull();
+    expect(outcome.reason).toBe('wrong_version');
+    expect(outcome.detail).toBe("skill 'asked-for' is not available: the store answered under key 'other-key'");
+  });
 });
 
 // ─── getSkills ─────────────────────────────────────────────────────────
@@ -680,6 +703,55 @@ describe('allSkills', () => {
     _setStore(new InMemorySkillStore());
     expect(await allSkills()).toEqual([]);
   });
+
+  it('reports a non-object listing as a broken store, not an empty one', async () => {
+    // `allSkills` has no way to report the difference, so it returns an empty
+    // list either way — but the reason has to reach the caller that does act on
+    // it. Collapsing the answer to "no skills" reads downstream as "every skill
+    // was revoked".
+    const spy = vi.spyOn(console, 'error').mockImplementation(() => {});
+    try {
+      const brokenListing: SkillStore = {
+        getObject() {
+          return null;
+        },
+        allObjects() {
+          return null as unknown as Record<string, RawSkillObject>;
+        },
+      };
+      _setStore(brokenListing);
+
+      expect(await allSkills()).toEqual([]);
+
+      const { objects, error } = allRawObjects(requireStore());
+      expect(objects).toEqual({});
+      expect(error).toBe('the skill store listed skills as null rather than an object');
+      expect(spy.mock.calls.map(([line]) => String(line))).toEqual([
+        '[LaunchDarkly] Skill store listed skills as null rather than an object',
+        '[LaunchDarkly] Skill store listed skills as null rather than an object',
+      ]);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  it('names the type a broken listing came back as', () => {
+    const spy = vi.spyOn(console, 'error').mockImplementation(() => {});
+    try {
+      const listingAs = (value: unknown): SkillStore => ({
+        getObject: () => null,
+        allObjects: () => value as Record<string, RawSkillObject>,
+      });
+      expect(allRawObjects(listingAs([])).error).toBe('the skill store listed skills as array rather than an object');
+      expect(allRawObjects(listingAs('x')).error).toBe('the skill store listed skills as string rather than an object');
+      expect(allRawObjects(listingAs(undefined)).error).toBe(
+        'the skill store listed skills as undefined rather than an object',
+      );
+      expect(allRawObjects(new InMemorySkillStore())).toEqual({ objects: {}, error: null });
+    } finally {
+      spy.mockRestore();
+    }
+  });
 });
 
 // ─── Integrity verification ────────────────────────────────────────────

packages/client/src/skills-core.ts

@@ -419,10 +419,49 @@ export function verifyRawSkill(raw: unknown): Skill | null {
   });
 }
 
-/** Lists every raw object the store holds. Propagates whatever it throws. */
-export function allRawObjects(store: SkillStore): Record<string, RawSkillObject> {
-  const objects = store.allObjects(SKILL_OBJECT_KIND);
-  return typeof objects === 'object' && objects !== null && !Array.isArray(objects) ? objects : {};
+/** 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);
+  return `the skill store threw ${name}: ${message}`;
+}
+
+/** Every raw object the store holds, or the reason it could not answer. */
+export type RawListing = {
+  readonly objects: Record<string, RawSkillObject>;
+  /** `null` when the store answered; otherwise why it could not. */
+  readonly error: string | null;
+};
+
+/**
+ * Lists every raw object the store holds, or reports why it could not.
+ *
+ * A throwing store is caught here rather than propagated so that `allSkills` and
+ * the `'*'` reconcile path log and word the failure identically. Letting the
+ * exception out instead would make each of them re-derive the log line and the
+ * message, which is the drift this module exists to prevent.
+ *
+ * An answer that is not an object is a broken store, on the same footing as one
+ * that threw — **not** an empty one. Collapsing it to `{}` would make a store
+ * that served nothing usable indistinguishable from a store that holds no
+ * skills, which reads downstream as "every skill was revoked".
+ */
+export function allRawObjects(store: SkillStore): RawListing {
+  let objects: unknown;
+  try {
+    objects = store.allObjects(SKILL_OBJECT_KIND);
+  } catch (error) {
+    // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; a failing store must be visible
+    console.error(`[LaunchDarkly] Skill store threw while listing skills: ${storeThrew(error)}`);
+    return { objects: {}, error: storeThrew(error) };
+  }
+  if (typeof objects !== 'object' || objects === null || Array.isArray(objects)) {
+    const typeName = Array.isArray(objects) ? 'array' : objects === null ? 'null' : typeof objects;
+    // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; a failing store must be visible
+    console.error(`[LaunchDarkly] Skill store listed skills as ${typeName} rather than an object`);
+    return { objects: {}, error: `the skill store listed skills as ${typeName} rather than an object` };
+  }
+  return { objects: objects as Record<string, RawSkillObject>, error: null };
 }
 
 // ---------------------------------------------------------------------------
@@ -470,18 +509,19 @@ export type Resolution = {
  * versions of one key and only it can pick between them; `null` asks for the
  * newest. The equality check afterwards is kept as a **defense**, not as the
  * selection mechanism: the store is untrusted, so an answer that is not the
- * version that was asked for is withheld rather than returned.
+ * version that was asked for is withheld rather than returned. The key is
+ * checked the same way and for the same reason: identity is read off the object
+ * itself, so an answer served under a different key would otherwise be returned
+ * under the caller's key while carrying its own.
  */
 export function resolveFromStore(store: SkillStore, key: string, wantedVersion: number | null): Resolution {
   let raw: RawSkillObject | null | undefined;
   try {
     raw = store.getObject(SKILL_OBJECT_KIND, key, wantedVersion);
   } catch (error) {
-    const name = error instanceof Error ? error.constructor.name : 'unknown error';
-    const message = error instanceof Error ? error.message : String(error);
     // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; a failing store must be visible
-    console.error(`[LaunchDarkly] Skill store threw while retrieving '${key}': ${message}`);
-    return { error: `the skill store threw ${name}: ${message}`, reason: 'store_unavailable', unavailable: true };
+    console.error(`[LaunchDarkly] Skill store threw while retrieving '${key}': ${storeThrew(error)}`);
+    return { error: storeThrew(error), reason: 'store_unavailable', unavailable: true };
   }
 
   if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) {
@@ -492,6 +532,12 @@ export function resolveFromStore(store: SkillStore, key: string, wantedVersion:
   if (skill === null) {
     return { error: `skill '${key}' failed integrity verification and was withheld`, reason: 'integrity_failure' };
   }
+  if (skill.key !== key) {
+    return {
+      error: `skill '${key}' is not available: the store answered under key '${skill.key}'`,
+      reason: 'wrong_version',
+    };
+  }
   if (wantedVersion !== null && skill.version !== wantedVersion) {
     return {
       error: `skill '${key}' version ${wantedVersion} is not available (the store holds version ${skill.version})`,

packages/client/src/skills-fs.ts

@@ -504,13 +504,8 @@ function resolveAll(deadline: number, onUnavailable: OnUnavailable): { requests:
   // Deliberately not via allSkills(), which reports a throwing store as an empty
   // result — that would look like "every skill was revoked" and let prune delete
   // the lot.
-  let objects: Record<string, unknown>;
-  try {
-    objects = allRawObjects(store);
-  } catch (error) {
-    const name = error instanceof Error ? error.constructor.name : 'unknown error';
-    return unavailableRun(unavailable(`the skill store threw ${name}: ${messageOf(error)}`), onUnavailable);
-  }
+  const { objects, error } = allRawObjects(store);
+  if (error !== null) return unavailableRun(unavailable(error), onUnavailable);
 
   return {
     requests: Object.entries(objects).map(([key, raw]) => pendingForRaw(key, raw)),

packages/client/src/skills.ts

@@ -253,18 +253,8 @@ export async function getSkills(refs: ReadonlyArray<SkillReference | string>): P
  * configured.
  */
 export async function allSkills(): Promise<Skill[]> {
-  const store = requireStore();
-
-  let objects: Record<string, RawSkillObject>;
-  try {
-    objects = allRawObjects(store);
-  } catch (error) {
-    // biome-ignore lint/suspicious/noConsole: this package has no logger abstraction; a failing store must be visible
-    console.error(
-      `[LaunchDarkly] Skill store threw while listing skills: ${error instanceof Error ? error.message : String(error)}`,
-    );
-    return [];
-  }
+  const { objects, error } = allRawObjects(requireStore());
+  if (error !== null) return [];
 
   const skills: Skill[] = [];
   for (const raw of Object.values(objects)) {