diff --git a/packages/client/README.md b/packages/client/README.md index e375f066..764686ac 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 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'`. | @@ -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. diff --git a/packages/client/agents.md b/packages/client/agents.md index a31f4bfb..27de12c0 100644 --- a/packages/client/agents.md +++ b/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: diff --git a/packages/client/src/__tests__/skills-fdv2.test.ts b/packages/client/src/__tests__/skills-fdv2.test.ts index c5f4b9bd..ceadaa96 100644 --- a/packages/client/src/__tests__/skills-fdv2.test.ts +++ b/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, 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..e04ca4d0 100644 --- a/packages/client/src/__tests__/skills.test.ts +++ b/packages/client/src/__tests__/skills.test.ts @@ -1218,6 +1218,95 @@ 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 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', () => { diff --git a/packages/client/src/skills-core.ts b/packages/client/src/skills-core.ts index b34f9c6d..e938576f 100644 --- a/packages/client/src/skills-core.ts +++ b/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'; diff --git a/packages/client/src/skills-fdv2.ts b/packages/client/src/skills-fdv2.ts index cafcc2d5..841487c1 100644 --- a/packages/client/src/skills-fdv2.ts +++ b/packages/client/src/skills-fdv2.ts @@ -506,16 +506,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; } /** @@ -1586,8 +1585,9 @@ 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 its failure count and + * backoff started over. */ start(): this { if (this.closed) { @@ -1596,7 +1596,12 @@ 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.reader.diagnostics.connectionFailures = 0; + this.backoffAttempt = 0; this.controller = new AbortController(); this.loop = this.run(this.controller.signal); return this; @@ -1680,7 +1685,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; } @@ -1847,7 +1855,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 b1f1bf50..95205b54 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'; @@ -622,9 +629,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 { @@ -632,6 +639,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; @@ -851,18 +866,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..087db2c5 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,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. + * 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. */ @@ -290,11 +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 the store served', served, skills.length); return skills; } @@ -302,7 +309,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 +318,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; }