Repository navigation
feat(skills): Agent Skills - #71
Merged
Merged
Conversation
First of a stack that lands Agent Skills as one-way-dependent layers. This is the leaf: nothing here imports another skills module, so it reads on its own. `types.ts` gains the four frozen value types — `SkillReference`, `Skill`, `ReconcileAction`, `ReconcileReport` — plus the untrusted wire shape `RawSkillObject`, the structural `SkillStore` seam, the freezing factories, and the key/version validators. `parseAiConfig` now applies them to a variation's `skills` array, and it fails **closed**: one malformed reference fails the whole parse, because silently dropping it would materialize a partial skill set without telling anyone. Every one of these names, and the camelCase wire field `contentHash`, is an identical string to the Python SDK. A polyglot fleet has to agree on the shape it exchanges, so changing one is a cross-language breaking change. `Skill.content` is `Uint8Array` — the verified verbatim bytes, exactly what was hashed, with no encoding or file-format claim. The SDK never interprets skill content: there is deliberately no frontmatter parser and no YAML dependency anywhere in the package. Consumers that want structure parse the bytes themselves. Client package: 294 -> 342 tests. typecheck and biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ific Review feedback: code comments in this SDK should not lean on another SDK as the explanation. SkillOutcomeReason's five-token vocabulary is a real cross-language contract, so the constraint stays — it is just stated as a property of every language implementation rather than of the Python SDK specifically. Same for the integer rule in the schema test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The primitives the skills materialization layer will be built on, landed on their own because they know nothing about skills and can be read without any of it. Nothing is exported from the package root — this is internal machinery. `atomicWrite` creates its temp file exclusively (`O_EXCL`) in the target's own directory, so the rename is same-filesystem and cannot be redirected through a staging area an attacker controls, fsyncs the file, renames at mode `0644`, and fsyncs the directory. `openDirectoryNoFollow` opens with `O_NOFOLLOW | O_DIRECTORY`, so a symlinked directory is refused by the kernel rather than by a check that could be raced. `unlinkNoFollow` is the single managed-file delete. Both destructive calls go through the `fsOps` record so exactly those two operations, and nothing else, are interceptable from a test. On the TOCTOU exposure, stated plainly rather than papered over: Node exposes no `*at()` family — no `renameat`, `unlinkat`, or `openat`, and no `rename` on `FileHandle` — so a destructive operation cannot be addressed relative to a pinned directory descriptor. `SUPPORTS_DIR_FD` is a real feature probe of that family, and it is `false` here. What narrows the window instead is a `(dev, ino)` re-check against the pinned handle immediately before each destructive step, on top of the `O_NOFOLLOW` opens and the exclusive same-directory temp. That does not close the window. An attacker who already has write permission on the managed root can still win the race, and the two swap-race cases that would prove otherwise are skipped off the same probe the implementation gates on — written out in full so they become live the moment the probe flips. The `(dev, ino)` re-check is, on this runtime, the whole defense against a directory swapped between validation and a path-based rename or unlink, and it is invisible from the layer above: it could be deleted outright with every higher-level test still green. So it is staged here at the primitive layer instead — pin the handle, swap the directory underneath it, invoke the primitive. The checks here are also deliberately redundant with the ones the materialization layer will apply. A symlinked skill directory will be refused twice. Testing each layer on its own is what keeps that redundancy from rotting into a single point of failure. Client package: 360 -> 373 tests. typecheck and biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review feedback on the safe-fs primitives. TEMP_NAME_ATTEMPTS was 128 with no justification behind it. Temp names carry 64 bits of randomness from randomBytes(8), so an EEXIST collision is not something that occurs in practice — the loop is a bound, not a probabilistic strategy. Lowered to 5 and the comment now says why. Comment sweep over both files: - Drop the reference to the Python SDK's use of the *at() family; the behavior of the family is what matters here. - Drop forward references to skills-fs.ts and skills-fs.test.ts, neither of which exists at this commit. - Drop the development narrative: why the module was split out, "its own file for two reasons", the mutation-testing result, and the defenses of test strategy and of probing instead of hardcoding. - Tighten the remaining prose, keeping the non-obvious technical content (O_NOFOLLOW vs lstat, recursive mkdir treating a symlink as present, same-directory temp for an atomic rename, why fsOps exists, and the residual TOCTOU exposure at SUPPORTS_DIR_FD). Comment lines 126 -> 106 and 54 -> 40. 355 client tests, typecheck, and biome all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The whole non-filesystem runtime, and the first commit in the stack where the feature does something end to end. `skills-core.ts` holds what the two layers above it share: the `SkillStore` and telemetry seams, the module state behind them, integrity verification, and store resolution. It imports neither of those layers. Keeping the store and the emitter here is what makes it impossible for the accessor layer and the filesystem layer to disagree about whether one is configured, so the dependency edge that would close that cycle must not be added. `skills.ts` is the public half: `skillRefs`, `getSkill`/`getSkills`/`allSkills`, `InMemorySkillStore`, and the documented injection points. `lifecycle.ts` accepts `skillStore` on both `initClient` overloads — the BYOC overload gains an options argument, which is how an edge-runtime caller configures one. It is applied before the idempotency check and on every call, so a client that initialized lazily, or without a store, can be given one afterwards; a nullish value never clears a configured store. `shutdown()` clears the skills state unconditionally and ahead of its own early return, because that state can exist without a client. Content delivery is not wired up. Everything runs against the `SkillStore` seam and the shipped default is absent, so the accessors throw an actionable error until one is configured. `InMemorySkillStore` covers local development, tests, and bring-your-own-content. The real transport drops in behind the same interface without touching the public API. Nothing a store serves is trusted. The transport is outside the trust boundary, so key, version, size, and content hash are revalidated at the accessor boundary on every pass — a `Skill` only exists once verification has passed, and `writeSkills` will re-verify anyway. Telemetry goes through a private no-op emitter and never `client.track()`. These are LaunchDarkly product-analytics signals, not customer analytics: `track()` would require an LD context, spend the customer's event volume, and land in their data export. Exactly three signal names exist, they are an allowlist rather than a floor, and a sweep over the recorded names enforces that. No skill content and no filesystem paths reach a signal — hashes and byte counts only. The wire object delivers `content` as a JSON string; `verifiedBytes` encodes it to UTF-8 bytes exactly once (TextEncoder), hashes the bytes, and the verified bytes are what the `Skill` carries — "skills are opaque byte buffers" is true by construction from here on. The pre-write pass hands a `Skill`'s bytes straight back in and they are hashed as-is. One defense here is invisible from the outside and could be deleted with every other test still green: the UTF-8 round-trip guard in `verifiedBytes`. TextEncoder silently substitutes U+FFFD for an unpaired surrogate where Python raises, so without the guard a store supplying the sha256 of the *substituted* bytes has fabricated content pass verification. Its test pins `contentHash` to exactly that hash, so the hash comparison is provably not what rejects it. Client package: 342 -> 395 tests. typecheck and biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A default `TextDecoder` consumes a leading U+FEFF (`ignoreBOM` defaults
to false, which means "handle the BOM" — i.e. strip it). The UTF-8
round-trip guard in `verifiedBytes` compared that decoded string against
the original, so authentic content starting with a BOM round-tripped to a
shorter string and was withheld as `not_utf8` even though its bytes
hashed correctly — surfacing to callers as `integrity_failure`.
Decoding with `{ ignoreBOM: true }` passes U+FEFF through, so the
comparison only fires on content that genuinely has no UTF-8 encoding.
Verified that the lone-surrogate case this guard exists for is still
caught, since TextEncoder substitutes U+FFFD there regardless of the BOM
setting. Regression test lands with the other verification tests in the
next PR in the stack.
Reported by Cursor Bugbot on #31.
Also, per review feedback, the integrity vocabularies and the byte-identical
JSON requirement are now stated as cross-SDK properties rather than as
facts about the Python SDK. The constraints are unchanged: the eight
reason_code tokens and alphabetical key insertion order are still
load-bearing, and the comments still say so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Splitting these out of the accessor commit rather than reviewing 1,500 lines at once. Both suites test code that already landed; they are the adversarial half of it. Integrity verification covers what a hostile or broken store can serve: a content hash that does not match, a hash of the wrong shape, content over the 64 KB cap, a key or version that fails revalidation, and the UTF-8 round-trip case where `TextEncoder` substitutes U+FFFD for an unpaired surrogate. That last one pins `contentHash` to the sha256 of the *substituted* bytes, so the hash comparison cannot be what rejects it — only the round-trip guard can be, which is what makes the test fail against an implementation missing it. The telemetry suite asserts the seam's shape rather than any one signal. The allowlist arrives with it: a sweep over every recorded name proves the set is exactly three and that the two names this SDK must never emit — `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` — appear nowhere. Also covered: no skill content and no filesystem path reaches a signal, and an emitter that throws cannot fail the operation it was observing. Client package: 395 -> 433 tests. typecheck and biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the regression test for the BOM fix in the previous commit, placed next to the lone-surrogate test it is the counterpart of: authentic content beginning with U+FEFF verifies and returns its exact bytes. Fails without `ignoreBOM: true`. Comment sweep, per review feedback: the vocabulary and key-order tests now describe their cross-SDK contract without naming the Python SDK, and the surrogate test explains the U+FFFD substitution as a Node behavior rather than by contrast with str.encode. No assertions changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The highest-blast-radius layer in the feature: this writes LaunchDarkly-delivered
content to a customer's disk. It lands with the tests that prove its defenses
rather than after them, because several of those defenses are the kind that a
passing functional suite would not notice going missing.
`writeSkills(skills, root, options?)` reconciles `<root>/<key>/SKILL.md` against
a `.launchdarkly-skills.json` manifest and reports every outcome in a
`ReconcileReport` — nothing is silent. It accepts `Skill` values,
`SkillReference` values, and bare key strings. `timeout` is in **seconds**,
defaulting to 10, matching the Python signature rather than the usual TypeScript
`timeoutMs` instinct.
The manifest format is byte-level identical across languages: same filename,
same sorted-key ASCII-escaped serialization. A polyglot fleet reconciling one
directory has to produce the same bytes from either language, so the filename,
the format, and the exported constants are a cross-language breaking change to
alter. It is written through the same atomic path as a skill file.
It fails closed, and the checks are not negotiable. `keyRejectionReason` and
`unsafePathReason` are shared by the write and the prune paths precisely so the
two cannot disagree about which paths this SDK may destroy. Keys are
re-validated locally against the same anchored pattern, with a tighter
path-component bound than the data model's, since no mainstream filesystem
accepts a 256-byte component. Content is hash-verified again immediately before
the write, because a `Skill` can be constructed by a caller and not just by an
accessor. Symlinked roots, directories, and targets are refused. A destructive
operation is permitted only on a path the manifest lists under a matching key,
and a corrupt manifest suppresses every destructive action.
Two things the fail-closed posture needs in order not to fail *badly*.
A partial reconcile heals itself. The manifest is written atomically, once,
last, so a process killed after a skill file lands but before that write leaves
the file at a managed path with no entry — which is exactly what clobber
protection treats as a customer-placed file, so every later reconcile refuses it
and the skill is wedged until a human intervenes. Boot-time execution under a
10-second budget makes that window realistic. So a colliding file whose on-disk
sha256 equals the resolved content hash is **adopted**: recorded in the manifest
and reported `skipped_current`, the existing action kind whose documented
meaning — the bytes on disk already are the resolved content — is precisely this
case. It cannot weaken the guarantee, because the only bytes ever adopted are
bytes LaunchDarkly resolved; differing unmanaged bytes are still refused,
untouched. Adoption does make the file prunable later, which is correct: a
subsequent prune then removes content LaunchDarkly delivered, exactly what would
have happened had the crash not occurred.
And the read that comparison needs is now a guarded one. `readRegularFile` opens
with `O_NOFOLLOW | O_NONBLOCK`, `fstat`s the **handle** rather than the path, and
refuses anything that is not a regular file — because `readFile` on a FIFO with
no writer never returns, and a FIFO or a device node is neither a symlink nor a
directory, so `unsafePathReason` does not see one. That hazard predates adoption
on the managed path; adoption widens the read to genuinely foreign files, which
is what makes it a prerequisite rather than a cleanup. A read that fails is a
refusal and never a fall-through to the write: not knowing what is on disk is
the last state in which to overwrite it. No `O_BINARY`, unlike the Python twin —
Node performs no CRLF translation on a descriptor.
Two smaller additions in the same layer. Orphaned temp files are swept during the
reconcile: `atomicWrite` removes its own temp on any error it sees, but not after
a SIGKILL, and prune walks manifest entries only, so an orphan is invisible
forever *and* blocks the `rmdir` that would clean up an emptied skill directory.
The sweep is bounded on every axis — a key that passes `keyRejectionReason`, a
directory opened `O_NOFOLLOW` and pinned, only names matching the pattern
`safe-fs.ts` derives from its own generator (anchored at both ends, so it cannot
drift into matching something this SDK did not write), only regular files, and
through the same pinned-handle unlink the prune path uses. A failure is a
reported action, never a throw.
Second: the 22 Windows reserved device names — `con`, `prn`, `aux`, `nul`,
`com1`-`com9`, `lpt1`-`lpt9` — are rejected in `keyRejectionReason`, and
deliberately **not** in `isValidSkillKey`. `parseAiConfig` fails closed on a bad
`skills` entry, so a grammar-level rejection would invalidate the entire AI
Config — model, provider, instructions, tools — for a Linux or macOS customer
over a constraint that only exists on Windows; worse, `skillRefs` would silently
drop the reference, and a dropped reference lets prune delete the skill's on-disk
copy. "Fails to write on Windows" would become "gets deleted on Linux". The
255-byte path-component bound already lives in this layer for exactly that
reason. Unconditional rather than platform-gated, because a managed root written
by a Linux container and read from a Windows host is an ordinary deployment, and
because neither repo has a Windows CI runner — a `process.platform` branch would
be the one thing here no test could reach. The honest cost: a customer who
legitimately names a skill `aux` now gets a reported `error` where it would
previously have worked off Windows.
The abuse matrix here is what holds those rules in place:
- Path traversal, over seventeen hostile keys — parent traversal, absolute
paths, backslashes, drive letters, an NTFS alternate data stream, an embedded
null byte, uppercase, leading and trailing whitespace, a 257-byte key. Each
asserts that **no filesystem operation was attempted**, not merely that none
succeeded: the OS would reject several of these on its own, so a failed write
is not evidence of a defense.
- Symlink attacks on the root, on a skill directory, and on the target file,
plus the swap-race pair. Those two are skipped off `SUPPORTS_DIR_FD` and
written out in full, so they become live if Node ever grows the `*at()`
family; the capability probe is itself tested, so a probe that silently
reported "unsupported" could not also silently skip the cases that would have
caught it.
- Clobber protection: a file at a managed path with no manifest entry is never
overwritten unless its bytes already are the resolved content. Adoption,
byte-difference refusal, and a mixed run that adopts one skill and refuses
another are all covered, as is the follow-on reconcile being an ordinary no-op.
- Targets that must not be read as ordinary files: a FIFO (with an explicit test
timeout, so a regression fails rather than stalls CI), a directory standing
where `SKILL.md` belongs, and an unreadable file — which must refuse rather
than overwrite, with a reason distinguishable from the collision refusal.
- All 22 reserved device names, through both the write and the prune path, plus
the assertion that makes the layer choice load-bearing: `isValidSkillKey('con')`
is still `true` and `parseAiConfig` still accepts a config referencing it.
`com0` and `lpt0` are not reserved and still write.
- Corrupt manifest, over eight variants. Six are unparseable, which makes
"performed no destructive action" arithmetic rather than a defense. The two
`*_live_entries` variants are the ones that test the rule — corrupt only in
`manifestVersion`, with a valid entries map listing the managed path under a
matching key, so the implementation has everything it needs to overwrite and
to prune, and must refuse anyway.
Also lands the last of the package-root exports and the assertions that pin
their exact values, since a caller needs `MANIFEST_FILENAME` to gitignore the
manifest and `MAX_SKILL_CONTENT_BYTES` to pre-check content.
One residual is documented rather than implemented: the 255-byte bound is
per-component and does not bound the total path, so root + key + `/SKILL.md` can
still exceed Windows' 260-character `MAX_PATH` with a legal 200-character key.
The SDK cannot validate that — the root is the customer's.
Client package: 452 -> 555 tests, +2 skipped (the swap-race pair). typecheck and
biome clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`loadManifest` used a plain `readFile`, so a FIFO planted at `<root>/.launchdarkly-skills.json` never returned: the awaited promise never settled, a libuv threadpool thread stayed held, and the process could not exit. The `timeout` budget could not rescue it — it is cooperative, and `loadManifest` runs before the first deadline check. The manifest sits in the skills root, so it is reachable by exactly the swap `readRegularFile` was added in this branch to defend against on skill files; guarding one and not the other in the same directory was an inconsistency rather than a different risk tier. `O_NONBLOCK` makes the open return immediately and the handle `stat` refuses anything that is not a regular file, which lands in the existing corrupt-manifest branch: no destructive action, and the file left alone. `O_NOFOLLOW` also means a symlinked manifest is now refused rather than followed — deliberate, since the atomic rewrite already replaces a symlink at that path with a real file. Both cases are covered, the FIFO test with an explicit 5s timeout so a regression fails rather than stalling CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per review feedback, removes the Python SDK as the explanation for the
manifest's on-disk form while keeping the constraint it was there to
justify. Sorted keys and the non-ASCII `\uXXXX` escape are still
described as load-bearing — a root that more than one SDK reconciles
would otherwise churn the file's bytes on alternating runs — just stated
in terms of the shared on-disk form rather than json.dumps.
Also drops the development narrative from two doc comments ("Split out
of skills.ts", "Split out of writeOne"), which described past refactors
rather than the code as it stands, and the "unlike the Python twin" aside
on the missing O_BINARY.
No code changed. 582 tests pass, 2 skipped off SUPPORTS_DIR_FD as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The functional half of the materialization suite, split out so it is not read alongside the implementation and the abuse matrix in one sitting. No source changes — everything here covers code that already landed, and the gap it closes is real: until now `writeSkills` had its defenses proven but not its ordinary behaviour. - Basic writes and the returned report: what lands on disk, at what path, and what each `ReconcileAction` says about it. - Manifest: the serialization is asserted byte for byte, since it is a cross-language on-disk contract. Written literally rather than by importing the implementation's own constants — a test that imported them could not detect a change to them. - Reconcile semantics: `written`, `updated`, `skipped_current`, `removed`, and the `prune` and `onUnavailable` behaviours, including that a retrieval failure under `keep` leaves existing content alone. - Root handling, and the bare-string guard on the argument surface. - Atomicity and permissions: the temp file is created in the target's own directory so the rename is same-filesystem rather than cross-device, the mode is `0644`, and a failure injected between write and rename leaves no partial file and produces an `error` action. - Resilience: an unreadable directory, a failing unlink, a manifest that cannot be rewritten. One skill failing never takes the run down, and every failure appears in the report. - Verify-then-write: content is re-verified immediately before the write, so a `Skill` a caller built by hand gets the same treatment as one an accessor produced. - Orphaned temp files: the sweep removes an orphan a killed reconcile left behind, and does so ahead of the prune, so the `rmdir` that empties a skill directory is no longer blocked by one. Most of the block is about what the sweep must *not* touch — a customer file, a temp-shaped name that fails the anchored pattern at either end, a temp for another target, a symlink wearing a temp name, anything in the managed root itself, and everything at all when the manifest is corrupt. - The write half of the telemetry seam, with the same allowlist sweep the accessor half applies: three signal names, no content, no paths. Client package: 555 -> 619 tests (+2 skipped). typecheck, biome, and sherif clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per review feedback. The two manifest serialization tests asserted the right bytes but named the Python SDK in their titles and rationale. They now describe the shared on-disk form directly — two-space indent, sorted keys, no trailing newline, non-ASCII escaped as \uXXXX — which is what the assertions actually check. No assertions changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documents the feature now that all of it exists. The README section and the contributor notes both span every layer in the stack, so they land once rather than as six partial edits to the same two files. `README.md` gains the public API — the accessors, `writeSkills`, the store seam, the exported constants, and the types — and states two things plainly rather than leaving a reader to discover them. Content delivery is not wired up in this release: everything runs against `SkillStore`, the shipped default is absent, and the accessors throw an actionable error until one is configured. And the security posture, including the limitation: Node exposes no `renameat`/`unlinkat`, so the final rename cannot be performed relative to a pinned directory descriptor, and an attacker who already has write permission on the managed root can still win a race to redirect a write or a delete outside it. Keep the managed root writable only by the process running the SDK. Two behaviours a reader would otherwise be surprised by are stated where the conservatism itself is described. First, the one narrow exception to "a file you placed yourself is never overwritten": a colliding file whose bytes *already are* the resolved content is adopted rather than refused, which is what lets a reconcile killed between the content writes and the final manifest write heal itself instead of wedging those skills forever. Nothing is overwritten, because nothing needs to be. Differing bytes, and bytes that cannot be read at all, are still refused and left untouched. Second, the two key constraints this layer imposes that the data model does not — the 255-byte path-component bound and the 22 Windows reserved device names — including why a key like `aux` still parses everywhere else, and the residual the SDK cannot check: the bound is per component, so a deep root plus a long key can still exceed Windows' `MAX_PATH`. `agents.md` records what a future change has to preserve: the module ownership rows, the one-way dependency direction (`types` <- `skills-core` <- `skills`, and `types` + `safe-fs` + `skills-core` <- `skills-fs`) and why closing a cycle there would let the accessor and filesystem layers disagree about whether a store is configured, and a Dependencies table giving a reason for every runtime dependency and optional peer. Skill content is documented as what it now is: `Skill.content` is a `Uint8Array` of the verified verbatim bytes, opaque to the SDK — no frontmatter accessor, no YAML dependency, and consumers who want structure parse the bytes themselves. Four new pitfalls, each one a mistake that would pass review: interpreting skill content anywhere in the SDK, reading `writeSkills`'s `timeout` as milliseconds, relaxing a path or manifest check in `skills-fs.ts`, and bypassing `fsOps` for a destructive call — which makes the operation invisible to the assertions that prove no operation was attempted, so they then pass vacuously. The path-check pitfall carries the three specifics most likely to be widened by someone who has not read the reasoning: why the reserved-name rejection is unconditional rather than platform-gated and why it lives in `keyRejectionReason` rather than in the key grammar (a grammar-level rejection would invalidate the whole AI Config, and a dropped `skillRefs` entry lets prune delete the on-disk copy — "fails to write on Windows" becoming "gets deleted on Linux"); that adoption is a narrowing of the clobber refusal and requires an exact hash match; and that every flag in `readRegularFile` is load-bearing, `O_NONBLOCK` most of all, since opening a FIFO with no writer blocks forever. Also renames one pre-existing test to describe its behaviour directly instead of citing a specification section. Workspace 1148 passing (+2 skipped). typecheck, biome, and sherif clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… remainder The Node half of the four items left open in the response to the Agent Skills security design review. Three are documentation, one is tests; no behavior changes, and `safe-fs.ts`'s implementation is deliberately untouched. **Privilege separation** (row 9 docs half, and the agreed counter-proposal for row 26). The recommended deployment runs the reconcile as a different identity than the agent, which is the whole reason the `0644`/`0755` modes deny anything: the agent reads its instructions and cannot rewrite them, or the manifest. That is the mitigation for AZ-1, a prompt-injected agent editing its own skills. Write access to the manifest is the worse half — it is what tells the *next* reconcile which paths the SDK may delete — which is why the prune path re-validates every entry from scratch rather than trusting it. It matters more here than in Python, because this runtime's residual race *requires* write permission on the managed root, and privilege separation is what denies it. The README's security-posture note now points at the new section instead of ending on a bare "keep the root writable only by the SDK". The README also hands the operator the check to run, because the SDK cannot run it: it knows only its own identity, which trivially has write access, having just written there. So `ReconcileReport` grows no writability field — the review asked for one and we declined, since any check the SDK could make would answer a different question than the one asked and manufacture false confidence exactly where caution is wanted. `agents.md` records that reasoning so the field is not added later by someone reading its absence as an oversight. **Three hostile-manifest prune tests** (row 12 remainder): a well-formed manifest listing `/etc/passwd`, `../../../etc/passwd`, and a path under a parent that has since become a symlink. The prune path already refuses all three, so these turn asserted into verified, and they mirror the Python suite case for case. Two things make them worth more than their line count. They are deliberately *well-formed* — the corrupt-manifest block above them proves nothing here, because a corrupt manifest suppresses every destructive action wholesale, whereas these manifests give the implementation everything it needs to prune. And "deleted nothing" is asserted through an `fsOps.unlink` spy rather than by checking that `/etc/passwd` still exists: the test process cannot delete that file anyway, so the obvious assertion would pass against an implementation with no path check at all. **One sentence on** `'*'` (row 16 remainder). It materializes the whole project library, so every skill's `description` enters the agent's context — including skills no AI Config references and skills belonging to other teams. **The platform bound is now explicit** (row 2 residual), in `safe-fs.ts`, `agents.md` and the README — and it is a sharper bound here than in Python. Node exposes no `*at()` family on *any* platform, so the racy per-component `lstat` floor is not the Windows fallback it is in Python; it is the only implementation, Linux included. The README previously said this of the rename alone; it is true of every destructive step. Windows reparse-point checks (`GetFileAttributesW` / `FILE_FLAG_OPEN_REPARSE_POINT`) are not implemented, by decision: Windows is not a supported or tested platform for this release, neither repository has a Windows CI runner, and Node offers no primitive that would make such checks meaningful here anyway. This retroactively lowers the priority of the row 25 reserved-device-name work, noted where that code lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two untrusted-store answers were read as data rather than as failures.
`allRawObjects` collapsed a non-object listing to `{}` with no error, so
a store that served nothing usable was indistinguishable from one holding
no skills. `resolveFromStore` read identity off the object without
checking it against the key that was asked for, so an answer served under
a different key came back under the caller's key while carrying its own.
Both are now withheld and reported, alongside the version check that
already guarded the same way. `allRawObjects` now returns the listing
together with the reason it could not answer, with a throwing store
caught and worded in the same place, so `allSkills` and the `'*'`
reconcile path no longer each re-derive the log line and the message.
Ports launchdarkly/python-ai-sdk 5817d89.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Covers the reconcile side of the withheld-answer fix: a listing that is not an object leaves every managed file alone rather than reading as a full revocation, and an answer served under a different key writes nothing, is reported against the key that was asked for, and does not reach that other key's file. Each one previously deleted a file and reported a clean run. Ports launchdarkly/python-ai-sdk 4c6d965. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es not close Three expected-failure tests (skills-fs-root-swap.test.ts) for SEC-8985 row 2. resolveRoot validates the root once and returns a path; nothing holds it open. Each write and prune re-opens <root>/<key> by path with O_NOFOLLOW, which guards only the final component, so a root swapped for a symlink after validation redirects the open into the attacker's directory. assertUnswapped does not notice, because it lstat's the same swapped path the handle came from. Precondition is write permission on the root's parent, which the README privilege-separation checklist does not mention, so the documented mitigation for Node's residual exposure does not cover this. The tests state the contract (nothing lands outside the root, no outside file is overwritten, no outside file is removed) under a knownGap() wrapper: pytest's strict xfail narrowed to assertion errors, which it.fails is not, so a harness breakage is a real failure and the wrapper fails the run the day the body passes. Separate file because the swap must land before the per-skill directory is opened, which means intercepting mkdir/open from node:fs/promises with a file-wide vi.mock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 2b9025e)
The expected-failure marker made the suite green while the contract was violated. A red run is the demonstration: the tests assert the contract, the code does not meet it, and they go green when the root is pinned for the reconcile, with nothing to remove. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 31d47db)
SEC-8985 row 2. `resolveRoot` validated the managed root and returned a path; nothing held it open. Every write and prune then opened `<root>/<key>` by path with `O_NOFOLLOW`, which guards only the final component, so the root was re-resolved on each one and a root swapped for a symlink after validation redirected the open into the attacker's directory. `assertUnswapped` agreed with itself, because it lstat'd the same swapped path the handle came from. Only the manifest write pinned the root, and by then the skill files were already outside it. The precondition is write access to the root's *parent* — `.claude` for a root of `.claude/skills`, which the agent identity typically owns. The root is now opened `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` once, fstat'd, and held for the whole reconcile, and every child is addressed as `/proc/self/fd/<fd>/<name>`. The kernel resolves that prefix to the pinned inode rather than to the name, which gives it the property an `*at()` call would have; the per-skill directory is pinned in turn for the files inside it. Gated on a new `SUPPORTS_PROC_FD` probe (Linux plus a usable `/proc/self/fd`, verified at load by resolving a live descriptor back to its own inode), so macOS and Windows keep the per-component lstat floor unchanged — `assertUnswapped` is skipped only for a directory the caller addressed through a descriptor, which is why the floor's own tests still exercise it on Linux. Containment checks in `unsafePathReason` stay as defense in depth; they are no longer the boundary here. A swap landing before the pin fails the open and is reported as a run-level error rather than thrown, since the root was usable when the caller named it. `atomicWriteIn` is gone: the root is always pinned by the caller now, so a primitive whose job was to re-open a directory by path is exactly the footgun this bug was made of. Descriptor addresses are stripped out of `errno` messages, which leaves the offending file named relative to whichever directory was pinned. Tests: the two `<root>/<key>` races in skills-fs.test.ts now run wherever either capability probe is true, so they are live on Linux. The root-swap file from Security's PR #49 keeps its assertions; its trigger matches the final component plus the shape of its parent rather than one exact path, so the same bodies fire the swap against both the old and new addressing — pinning the trigger to `path.join(root, key)` would have made them pass vacuously the moment the fix landed. Verified in a Linux container: all four fail against the pre-fix code with real assertion failures (the write escapes, the outside victim is clobbered, the outside victim is unlinked) and pass against this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes the internal-process references from this PR's comments and docs while keeping every technical constraint they carried: - The SEC-8985 ticket reference in the root-swap test header. The defect description around it is what a reader needs and stays as-is. - "the security review names", "reopening the security review", and "the security review asked for one; we declined" in agents.md and the prune test. The decisions and their reasoning stay; only the appeal to an internal process is gone. - The Windows bullet's reference to another repository's CI runner, and the Python contrast for where the racy floor sits. - Two re-introduced "Split out of ..." refactor narratives and the "retroactively lowers the priority" framing, which described project sequencing rather than the code. Note on the rebase onto the restacked #33: that PR's Bugbot fix routes the manifest read through `readRegularFile`, and this PR independently rewrote the same line to address the manifest via `root.address` with a plain `readFile`. The conflict resolution keeps both — `/proc/self/fd` addressing defeats a directory *swap*, but does nothing about a FIFO already sitting at the resolved path, so dropping `readRegularFile` would have restored the hang. The manifest FIFO test times out without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Node port of the Python SDK's FDv2 transport, behind the same
SkillStore seam. Deliberately mechanical: same protocol boundary, same
constants, same wire semantics, same diagnostics vocabulary. Nothing
above the seam changed — the accessors, integrity verification, and
writeSkills are untouched.
The design's original Plan A — a bespoke poller against a gonfalon
/private/flagdlv route — is dead. That route is authenticated by Cognito
machine-token OAuth scopes with no per-tenant authorization, and the
security review ruled out both relaxing its auth and shipping a machine
credential to customer hosts. This uses GET /sdk/poll and GET /sdk/stream
with the environment's server-side SDK key instead, which is also the
channel payload signing will eventually cover.
skills-fdv2.ts — FDv2SkillStore: authenticate, poll or stream, maintain
selector/basis state, deserialize inline-resource/skill objects, hold
them keyed by (key, objectVersion), and serve the seam. Bounded retries
with capped jittered backoff, Retry-After honoured, If-None-Match/304.
Platform globals only — fetch, AbortController, TextDecoder — so the
content path adds no dependency.
The trap, stated loudly and asserted in both directions: objectVersion is
the skill's own version — the one {key, version} pins — while version is
the payload version. Confusing them fails silently. seamObjectFromPut is
the only place the translation happens.
Flag and segment objects arrive on the same connection and are skipped
cleanly rather than throwing; throwing is the unknown-kind reconnect loop
this feature must not reproduce. Changes commit at payload-transferred,
so an interrupted full transfer leaves last known good intact.
One deliberate divergence from Python, documented on the class: the
snapshot collapses to one object per key at its newest version, because
this SDK has no newestByKey above the seam yet and <root>/<key>/SKILL.md
is a single path. End-to-end behaviour matches Python; getObject still
resolves a pinned version out of the full set.
contentHash is read from the envelope and verification semantics are
unchanged. The field has not shipped on the write path yet, so its
absence is made loudly diagnosable — an error per object naming
missing_content_hash, a summary per wholly-hashless payload, and a
diagnostics counter — rather than a silent empty store. There is
deliberately no fallback that skips verification.
skills-watch.ts — watchSkills/SkillWatcher pull the change-listener
re-reconcile forward out of phase 4. A delete-object reaches a live
stream in seconds, so a revoked skill's SKILL.md now leaves the disk
within a debounce interval instead of at the next restart, which
materially improves the review's AV-1. onUnavailable: 'keep' stays the
default, as the review endorses.
Server-side only: a mobile key or client-side environment ID throws from
the constructor. Skill content is customer-confidential and payload
assignment is shared across auth types, so this is the SDK-side half of
that boundary.
close() aborts the signal rather than only setting a flag, and every
timer is unref'ed, so a background store is never why node stays up.
Two stale doc comments on the SkillStore type corrected: the transport is
no longer "future", and addListener now has a consumer.
103 tests against an in-process fake FDv2 endpoint that implements the
contract: put/delete, objectVersion vs version, mixed and unknown-kind
payloads, 304, basis round-tripping, reconnect/backoff, Retry-After,
bounded retries, the snapshot collapse, and a missing-contentHash
envelope producing withheld skills with the right reason code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SDK-facing FDv2 channel delivers skills the way streamer #4681 and gonfalon #70638 spell them: object kinds are open strings, the agent-skill payload is classified `generic`, and every generic object carries only `key`, `kind`, `version` and `object`, exactly like a flag. A skill arrives under kind `skill` with its own version folded into the key as `<key>:<version>`. There is no `category` field and no `objectVersion` field; both came from an earlier streamer draft that never shipped. Identification is now the kind alone. The wire key is split in one place, `splitWireKey`, and both the put and the delete translation go through it. A key that will not split cleanly is held rather than dropped — version-less, or with the offending text as its version — so verification withholds it with `invalid_version` under a key the caller recognises; only a key with nothing before the delimiter is dropped, since there is no identity to hold it under. `SDK_DATA_MODEL_VERSION` and the `dataModelVersion` option go with it: the connection's `mv` parameter only accepts flag model versions, and generic payloads ignore it, so the request no longer sends one. The 403 and 400-class advice now says what a customer can act on rather than naming internal flags and unmerged branches. Ports python-ai-sdk efc4ca7. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The protocol reader took payloads[0]'s intentCode and applied it to the skill object set, which is what the delivery protocol requires — one payload per credential, read the first intent, tolerate the rest — but it left the assumption behind that rule undocumented and unguarded. If the one-payload guarantee ever widens, an xfer-full for another payload would start an empty pending set and the next payload-transferred would publish it: every skill reported revoked, and with pruning on, a customer's files deleted. The first payload is still the payload that is read. What is new is that the reader now knows which payload skills actually arrive on — learnt from the intent's id, or from the (p:<id>:<version>) selector, since no object or transfer event carries a payload id of its own — and declines to apply a transfer of any other, holding last known good, warning once, and counting it in diagnostics.payloadsIgnored. An intent describing more than one payload warns once on its own, because that is the one case the comparison cannot catch: another payload's transfer arriving before any skill has been seen has nothing to be compared against. Behaviour under one-payload delivery is unchanged, and a full transfer of the skill payload still empties it — every skill deleted is a real state the guard must not mask. Ports python-ai-sdk 1cf5235. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ures a live stream actually has Four fixes to the delivery loop, each of which stopped delivery — updates and revocations alike — for the process lifetime under an ordinary condition: - The consecutive-failure counter was reset only after `streamOnce` returned, and a stream never returns normally: it ends by being dropped. Every healthy, server-recycled connection therefore counted as a failure and a healthy server was given up on after maxConsecutiveFailures + 1 recycles. The count now resets at each committed payload inside `apply`, and still after a successful poll. - Only the connect was wrapped as recoverable, so a read timeout, reset or truncated chunk in the body reached the loop as whatever `fetch` threw and was read as a bug. `iterSse` now wraps the read itself — and only the read, so a protocol-reader or dispatch error still surfaces as the bug it is — and cancels the body on exit so an early stop does not leave a socket behind the reconnect. - `Retry-After` was honoured verbatim. The header may come from a proxy rather than LaunchDarkly, and a value in the hours would park revocation for that long; every retry delay is now clamped to `maxBackoffMs`. - There was no read timeout at all, so a stream that went quiet hung forever. `readTimeoutMs` is the one network timeout: in `'poll'` mode it bounds the whole request (default 10s), in `'stream'` mode the gap between reads (default 300s, well past LaunchDarkly's heartbeat). It is implemented as a `ReadDeadline` composed with the store's own abort signal, so closing the store during a connect or a read is still one abort, which the tests now assert against a host that accepts and never answers. Ports python-ai-sdk b6a25f9's loop fixes and c285ff5. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…hat names the delivery store `removeListener` joins `addListener` as the optional second half of change notification, on the `SkillStore` seam, on `InMemorySkillStore` and on `FDv2SkillStore`. `SkillWatcher.close` needs it to detach; without it a store held every watcher ever created for the rest of its life, and a closed watcher was still woken by every commit. The watcher probes for it, so a store without it keeps working at the cost of the listener staying registered. Both stores notify over a copy of the listener list, so a removal from inside a listener takes effect from the next commit rather than shifting its neighbours out from under the iteration. The no-store message now names `FDv2SkillStore` first, because it is the answer in production, and drops the sentence saying the delivery transport ships in a follow-up release, since it is in this one. Ports the interface half of python-ai-sdk dd2d55c and b6a25f9. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
agents.md described the earlier streamer draft: `objectVersion` beside `version`, `inline-resource` plus `category`, `basis` plus `mv`. It now describes what the channel delivers — the skill's version folded into the wire key, identification by kind alone, no `mv` — and adds the three transport decisions that arrived with the loop fixes: the payload identity guard, the single read timeout, and which failures are recoverable. README's "Receiving skills from LaunchDarkly" gains `readTimeoutMs` with its per-mode default, `removeListener` on both stores and the seam, the `StoreDiagnostics` fields including `payloadsIgnored`, and the 403 wording that matches what the store now logs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
All four are real, and each gets a test that fails without its fix.
**Snapshot keys break prune withhold.** `SkillObjectSet.snapshot` keyed its
entries `key:version`, but `writeSkills('*')` derives its prune keep-set from
the keys `allObjects` is keyed by. `pendingForRaw` could not parse `key:version`
as a skill key, so an unverifiable object fell out of the keep-set and prune
deleted the last known-good copy — the exact outcome withholding exists to
prevent, and one the `contentHash` gap makes universal, since today every object
arrives hashless. `snapshot` now keys by the bare skill key (matching
`InMemorySkillStore`), and `pendingForRaw` reads the object's own `key` before
falling back to the map key, so no store's choice of map key can reach the same
end. `allRaw` remains the way to match a held object back to its wire event.
**Empty transfer skips listener notify.** A full transfer revokes by omission —
whatever it did not carry is gone, and no `delete-object` says so — so a full
transfer that dropped every skill committed an empty store with an empty change
list and woke nobody, leaving revoked files on disk until the process restarted.
`payloadTransferred` now diffs the committed set against the pending one before
the swap and reports the departures as tombstones, which also gives listeners
that read versions both halves of a version move.
**Foreign payload overwrites delivery basis.** A declined transfer returned the
foreign selector, which `apply` then wrote into `basis`, asking the next poll or
stream to resume from a payload this layer had just thrown away. Skill updates
could stop arriving while every diagnostic still read healthy. A declined
payload no longer moves the resume point.
**Watcher misses updates during initial write.** `watchSkills` registered the
change listener after the initial reconcile, so a payload committing during that
reconcile's filesystem I/O reached nobody. The listener now goes on first, and
the initial reconcile runs through the watcher's own chain via `runInitial`, so a
notify arriving mid-run queues behind it rather than interleaving a second
reconcile on one root. A failed initial reconcile closes the watcher instead of
leaving a listener on a root that never reconciled.
Note this touches `skills-fs.ts`, which the PR description describes as
untouched: the `pendingForRaw` half of the first fix is a pre-existing weakness
in the seam contract that the FDv2 store is simply the first store to expose.
Full suite 1337 passing; `tsc --noEmit` and `biome check` clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…public SDK Removes references to internal process, tickets, design history and the Python SDK from the FDv2 transport, the watcher, the store seam docblock, and the tests added with them. Trims docblocks that restated the same rationale at class, method and inline level. The runtime hashless-object advice no longer names an internal ticket or a shipping timeline. No behavioural change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Follow-on to the sweep in a333b1d, covering what it left behind. The hashless-object advice is written to console.error on every withheld skill, so it is customer-facing text. It still claimed the SDK "cannot work around this" and that accessors stay empty "until LaunchDarkly delivers it with a contentHash" — a shipping prediction in a log line. Cut to the reason code, what it means, and where to go. The hashless tests assert on 'missing_content_hash', 'contentHash' and the separate whole-payload summary, all of which survive. Also removes three references customers have no use for: the `_warnedHashless` note about clearing it from the tests, `ProtocolReader`'s pointer at `skills-fdv2.test.ts`, and "test seam: a transport double" on `requester`, which is a public option whose docblock shows up as hover text. Trims the `NO_STORE_MESSAGE` maintainer note and three blocks in `skills-fs.ts` / `skills-watch.ts` that explained a rule by narrating the defect it fixed; the rules are kept. Reunites the skills export table in the README. The new "Receiving skills from LaunchDarkly" section had been inserted mid-table, splitting seven rows into a second table after ~35 lines of prose. The section now follows the whole table as a sibling of "Observability"; its heading is unchanged, so both in-page links still resolve. No behavioural change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`DEFAULT_BASE_URI`, `DEFAULT_STREAM_URI` and `DEFAULT_DEBOUNCE_MS` become `SKILLS_DEFAULT_BASE_URI`, `SKILLS_DEFAULT_STREAM_URI` and `SKILLS_DEFAULT_DEBOUNCE_MS`. TypeScript publishes every experimental feature from one entry point, and Evaluations has its own `DEFAULT_BASE_URI` for a different host, so the unprefixed names would collide once it lands there. Renaming is cheap while the names are experimental and unreleased. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`parseAiConfig` rejected a whole AI Config when its `skills` field was malformed, so a bad experimental field failed `config().invoke()`, `stream()`, graph nodes and `extractVariation` for apps that never use Agent Skills. That breaks TESTING.md §0.3: experimental behaviour must not break a core call. The parser now passes `skills` through unchecked, and `AiConfigRep.skills` is typed `unknown` to say so. `skillRefs` validates it instead and throws `TypeError` when the field is present but malformed, including `skills: null` and a single bad entry. It used to drop bad entries with a warning, which was safe only because the parser had already rejected them. Throwing keeps the original reason for the check: `writeSkills` with `prune: true` never receives an empty or partial list for a field that is present. An absent field, or a nullish config, still returns `[]`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
## Summary A malformed `skills` field no longer fails core config calls. Today `parseAiConfig` rejects the whole AI Config when `skills` is malformed. `config().invoke()`, `stream()`, graph nodes and `extractVariation` then raise, and `inspectConfig` returns `config: null`, even for apps that never use Agent Skills. Agent Skills is experimental, and TESTING.md §0.3 says experimental behaviour must not break a core call. - **`parseAiConfig` passes `skills` through unchecked.** - **`skillRefs` validates the field and throws `TypeError`** when it is present but malformed, including `skills: null` and a single bad entry. Before, it dropped bad entries with a warning, which was only safe because the parser had already rejected them. - **An absent field, or a nullish config, still returns `[]`.** - **`AiConfigRep.skills` is typed `unknown`**, since the parser no longer checks it. The root API report changes accordingly. Skills has never been in an npm release, so no published code reads the old type. Throwing rather than dropping keeps the reason the parser checked in the first place. `writeSkills` with the default `prune: true` deletes every managed skill missing from the list it is given, so `skillRefs` never returns an empty or partial list for a field that is present. Stacked on #114. Python counterpart: launchdarkly/python-ai-sdk#146. ## Tests - **Parser:** a malformed `skills` field of every kind (null, non-array, bad entry, one bad entry among good ones) parses successfully and passes through unchanged. - **`skillRefs`:** the cases the parser used to reject, including `null`, now throw `TypeError` naming the field. One bad entry rejects the whole field. The error does not echo the rejected key. - **Core calls:** `inspectConfig` and `extractVariation` succeed on a variation with malformed `skills`. ## Open question `skillRefs(null)` still returns `[]`, as documented. That is the case where a failed `inspectConfig` feeds an empty list to a pruning `writeSkills`. Throwing would remove that trap for every caller. That is a contract change, so it is not in this PR. ## Test plan - [x] Build, typecheck, Biome, Sherif, `api:check` - [x] All workspace tests: client 1240 passed, 10 skipped - [x] `test:integration`: 28 passed 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Moves **Agent Skills** validation out of the core config parser so a bad `skills` payload cannot break `config().invoke()`, `inspectConfig`, or `extractVariation`. > > **`parseAiConfig`** no longer validates `skills`; the field is passed through as-is. **`AiConfigRep.skills`** is now typed **`unknown`** (API report updated). **`skillRefs`** owns validation: it still returns `[]` when `skills` is absent or `config` is nullish, but throws **`TypeError`** when `skills` is present yet malformed (including `null`, wrong shape, or one bad entry among good ones)—replacing the old warn-and-drop behavior so **`writeSkills`** with default **`prune: true`** never gets a shortened list that could delete on-disk skills the config still references. > > Tests and docs (README, `agents.md`) reflect the experimental-field rule: core calls tolerate malformed `skills`; consumers validate at **`skillRefs`**. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 42968ff. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…onfig resolves When `inspectConfig` fails, `info.config` is `null`, `skillRefs(null)` returns `[]`, and `writeSkills` with the default prune then removes every managed skill while reporting `ok: true`. The quickstart's real-use path now returns before that, matching the Python README. Also test a store with only `allObjects`, so a `setSkillStore` check that skips `getObject` fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… own digests (#102) `contentHash` is specified as sha256, lowercase hex, over the content's exact UTF-8 bytes, with no canonicalization on either side. That makes agreement between this SDK and LaunchDarkly's delivery service part of the rule rather than an implementation detail — it was accepted in place of canonicalizing the content, so it wants a regression test. Nothing in the suite held the two to it. Every raw skill object in these tests is built by a helper that hashes its content with the same expression the implementation uses, so a change to the hashing rule moved the expectation along with it and nothing failed. ## What this adds **Two literal digests as the oracle.** Both are what the delivery service computes for the content beside them, pinned as literals and never recomputed with `createHash` — that is the entire point, since recomputing with the expression under test cannot detect the rule changing. | content | digest | |---|---| | `# hello` | `ea67f39f…cd06681` | | `# héllo\r\n\ntrailing no newline` | `2b7c050d…40b9a558` | The second carries three hazards at once: a non-ASCII character, a CRLF, and no trailing newline — the things a normalizing round trip alters without changing what the document means. It is written with a `é` escape rather than the literal character, so a tool that re-normalized the source file could not quietly change the test input. **Both halves, in both places the rule is written down.** Agreeing on a digest proves nothing unless disagreeing is detectable, so each vector's content is also altered three ways — CRLF→LF, an appended trailing newline, and NFD decomposition — and paired with the *unaltered* digest, which must fail. Two commits, because there are two independent hashing expressions: - `skills-core.ts` — `verifiedBytes`, shared by the accessor and the pre-write re-verify. Pinned in `skills.test.ts` (5 cases). - `skills-fs.ts` — the adoption comparison, which hashes the bytes it reads off disk with an expression of its own. Pinned in `skills-fs-byte-integrity.test.ts` (4 cases). The second one has teeth beyond digest agreement: adoption decides whether an *unmanaged* file is claimed or refused. A comparison that normalized would adopt a planted file whose bytes merely *normalize* to the delivered content, leave those foreign bytes on disk, and report `skipped_current` — an action documented to mean that the bytes on disk already are the resolved content. **The fixture helpers now say what they are.** All five recompute the rule with the implementation's own expression, which is what makes them useless as an oracle. Each carries a docblock saying so and naming where the rule is actually pinned, so the recomputation reads as deliberate fixture-building rather than as coverage — and so nobody "simplifies" the pinned literals into a call to `hash`. ## Verification By mutation, which is the bar the surrounding tests were held to. Each mutation patched only the hashing step, then the source was restored and confirmed clean before committing. `verifiedBytes`: | mutation | turns red | |---|---| | CRLF → LF before hashing | the non-ASCII/CRLF vector | | trailing newline appended | both pinned vectors | | NFC-normalize before hashing | the NFD withholding case | The adoption comparison: | mutation | turns red | |---|---| | CRLF → LF before hashing | exact-match adoption | | trailing newline appended | exact-match adoption | | NFC-normalize before hashing | the NFD-planted refusal — it wrongly adopts | In both tables the NFC mutation is caught *only* by the withholding/refusal half, which is what confirms the teeth are not redundant with the vectors. `biome check` clean, `tsc --noEmit` clean, client suite 1047 passed / 10 skipped. Tests only — no `src/` changes. ## Parity The Python SDK carries the same two suites (launchdarkly/python-ai-sdk#125), against the same two literal digests. A test that pins a cross-service contract in one SDK and not the other is the same class of gap as not pinning it at all. Based on `xie/skills-kinds-param` rather than `main`, which does not have the skills feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds **regression tests** so the Agent Skills `contentHash` rule (SHA-256 over exact UTF-8 bytes, no normalization) cannot drift from LaunchDarkly’s delivery service without failing CI. > > In **`skills.test.ts`**, a new **`contentHash contract`** suite pins two **literal service digests** (ASCII and a hazard string with CRLF, non-ASCII, and no trailing newline) and asserts **`getSkill`** accepts them. It also checks that content altered only by CRLF→LF, an extra trailing newline, or NFD still **withholds** skills and logs **`hash_mismatch`**. > > In **`skills-fs-byte-integrity.test.ts`**, a parallel suite pins the **on-disk adoption comparison** (separate from `verifiedBytes`): exact-byte files are adopted as **`skipped_current`**, while the same three normalizations on planted files are **refused** and left out of the manifest. > > **Docblocks** on test `hash()` helpers in five files state they mirror implementation hashing and are **not** an oracle—so maintainers do not replace pinned literals with recomputed digests. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit bfb5bbd. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…o xie/pr103-merge-865b2ec # Conflicts: # packages/client/src/lifecycle.ts
…114) Refactors Agent Skills to ship as an **experimental** feature. Skills features are exported **only** from `@launchdarkly/ai-server/experimental` and are absent from the package root. Additionally, the skill store is now configured via a `setSkillStore(store)` setter rather than injected as an option to `initClient`. Passing the removed `skillStore` option to `initClient` (from JavaScript, or past the type) logs a generic warning, `Ignoring unrecognized initClient option(s): skillStore`. The same warning covers any key `initClient` does not read and does not name the experimental feature or its setter, matching the Python SDK. Other changes: - `AiConfigRep.skills` was `SkillReference[]`; it is now typed structurally (`ReadonlyArray<{ readonly key: string; readonly version: number }>`). - **Core never imports experimental.** `shutdown()` clears Skills state through a shutdown-hook registry (`src/shutdown-hooks.ts`) kept on a `globalThis` symbol slot. `skills-core.ts` registers its cleanup when it loads, so the root entry point imports no Skills module, and the built `dist/index.{js,cjs}` contain no Skills code. Each hook runs in its own try/catch: a throw is logged and fails neither the other hooks nor `shutdown()`. - **`@launchdarkly/ai-node/experimental`.** ai-node re-exports the root with `export *`, so Skills left it too. It now has its own `./experimental` subpath re-exporting `@launchdarkly/ai-server/experimental`, so Node users who install only ai-node can reach Skills under strict package managers (pnpm, Yarn PnP). - **`typesVersions`.** Both packages map `experimental` to `dist/experimental.d.ts`, so the subpath's types resolve under `moduleResolution: node` (node10), which ignores `exports` and is still the default for `module: commonjs`. - **`setSkillStore` checks its argument.** A non-nullish value without `getObject` and `allObjects` methods throws `TypeError`, matching the Python SDK. - **API reports.** The repo had no API Extractor setup, so this adds one. API Extractor 7.x takes one entry point per config, so there are two configs and two checked-in reports: `etc/ai-server.api.md` and `etc/ai-server-experimental.api.md`. New `api:check` / `api:update` scripts; CI runs `api:check` in the typecheck job. The reports are generated from `tsc` declaration output rather than tsup's, whose hash-named chunks would churn the report. - **Docs.** README and `agents.md` use the experimental import path and `setSkillStore`. The `agents.md` export blocks are regenerated for both entry points, and the stage rules are written down, including that `/experimental` is the one sub-path anyone may import (deep `dist/*` imports stay banned). The README does not promise an "Experimental" changelog section, since release-please doesn't produce one yet (open question 5). ## Build note: no code splitting Code splitting would let both entry points share one copy of each module, but in CJS tsup rewrites the dynamic `import()` of the optional peers into `require()`. That is the AIC-3370 webpack regression, and the commonjs-webpack integration test caught it. Splitting stays off, so modules both entries reach (`types.ts`, `shutdown-hooks.ts`) are copied into each. The Skills modules are only in the experimental bundle now that the root no longer imports them. This is safe because the skill store, the shutdown hooks, and the client singleton live on `globalThis` symbol slots (the A.12 design for multiple module instances). Verified: a store set through the experimental bundle is cleared by the root `shutdown()`. ## Tests - The experimental entry point's runtime exports equal the documented set exactly (an allowlist, which also covers the §3.23.7 "no reader over `content`" rule). None of those names is reachable from the root. - The root API report names no Skills type and no `skillStore`; the experimental report names every Skills name. - The `setSkillStore` behaviour above, including that the SDK `init` runs exactly once. - Shutdown hooks: every registered hook runs, including before a client exists; a throwing hook is logged and fails neither the other hooks nor client teardown; re-registering a name replaces the hook. Loading the package root registers no Skills hook, so the root imports no Skills module (checked by temporarily adding the import, which makes the test fail). - `initClient` warns about an unrecognized option, `skillStore` included, on both overloads, and stays quiet for the documented options. - ai-node's experimental entry point re-exports every `@launchdarkly/ai-server/experimental` name as the same reference, and its root has none of them. - The package root's runtime exports equal a checked-in snapshot. - The integration test type-checks root and experimental imports against the packed tarballs under `node10`, `node16` and `bundler`, and asserts every loaded declaration is the consumer's own. - The integration test resolves `@launchdarkly/ai-server/experimental` and `@launchdarkly/ai-node/experimental` through `require` and `import`, asserts `getSkill` is absent from both roots, and checks both subpaths' manifest entries and packed files. Build, typecheck, Biome, Sherif, all workspace tests, `test:integration`, and `api:check` pass locally. The §3.23.2 swap-race tests skip on macOS and run only on Linux CI, as before. ## Open questions - **`parseAiConfig` still validates `skills`.** It fails a whole config when its `skills` array is malformed (`types.ts`), so a bad Skills field can fail an ordinary `config().invoke()`. That conflicts with "experimental behaviour must not break a core call". launchdarkly/python-ai-sdk#144 has the same open question; it should be decided once for both SDKs. - **Land order.** This conflicts with #105 in `lifecycle.ts` and `skills.test.ts`. Whichever lands second keeps #105's retry and shutdown shape, calls `runShutdownHooks()` instead of `_clearState()`, and drops `resolveClient`. `main`'s TESTING.md still requires Skills at the root, so this lands with launchdarkly/ai-sdks-monorepo#33, not before it. 🤖 Generated with [Claude Code](https://claude.com/claude-code), updated by @XieX <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Moves **Agent Skills** off the stable package root onto published **`@launchdarkly/ai-server/experimental`** and **`@launchdarkly/ai-node/experimental`** subpaths (dual ESM/CJS builds, `typesVersions` for node10). Consumers import skills APIs from `/experimental`; the root barrel no longer exports them. > > **Configuration and core/experimental boundary:** Skill storage is configured with **`setSkillStore(store)`** instead of `initClient({ skillStore })`. Unknown `initClient` keys (including `skillStore`) log a generic warning. **`AiConfigRep.skills`** is **`unknown`** and **`parseAiConfig`** no longer validates it, so malformed skills do not fail core config paths; **`skillRefs`** validates and **throws** on bad data (fail-closed for prune). Core clears skills state via **`shutdown-hooks`** on `globalThis` without importing experimental modules. > > **Tooling and tests:** Adds **API Extractor** reports for root and experimental surfaces, **`api:check`** in CI, and expands **commonjs-webpack** integration coverage (subpath resolution, packed artifacts, consumer `tsc` under multiple `moduleResolution` modes). Renames skills default URI/debounce exports to **`SKILLS_DEFAULT_*`**. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f2f9f51. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## TL;DR A JSDoc-only change. Since #107, `start()` revives a store that gave up, but the `waitForSkills` doc still listed giving up as the end of delivery. It now says a wait begun after the restart waits again, and that only `close` is final. Found in review of launchdarkly/ai-sdks-monorepo#40 (review). The behaviour is already there: `start()` clears `failedReason`, and that's what `waitForSkills` checks before returning early. This matches TESTING.md §3.25 in ai-sdks-monorepo#40 and the same docstring change in launchdarkly/python-ai-sdk#139. ## Test plan - [x] Pre-commit hook (`biome lint`, `biome check`) passes - No code change, so no tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Updates the **`waitForSkills`** JSDoc on `FDv2SkillStore` so it matches restart behavior introduced when **`start()`** can revive a store that gave up on delivery. > > The doc now states that **`false`** from an immediate return applies while delivery has ended via **`close`** or a non-retried failure (`**failed**`), but after **`start()`** runs again a caller **`waitForSkills`** will block for a payload again—only **`close`** is final. No runtime changes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f378cbb. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
… into xie/pr105-merge-base # Conflicts: # packages/client/src/lifecycle.ts
#105 removed the early return, so shutdown() no longer clears skills state "before" one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o xie/pr103-merge-865b2ec # Conflicts: # packages/client/agents.md # packages/client/src/__tests__/skills.test.ts
…#103) Stacked on #105 (`xie/initclient-store-timing`). This PR used to fix three telemetry-lifecycle bugs. Two of them, releasing the OTel globals on shutdown and retrying a failed init, now live on #105 in `3aed185` and `50aaa8d`. What remains is the first: Bugbot's report on #98 ([comment](#98 (comment))). #105 has since moved on, and each change is merged in here: - `52c1d0f` and `6720f6f` keep an init abandoned by `shutdown()` from adopting its client or leaving a provider behind. - `865b2ec` drops `initClient`'s `skillStore` option. #114 replaced it with `setSkillStore` on the experimental entry point. - #105 merged its base, which brought in #114, and `6575893` fixes an `agents.md` line about `shutdown()`. The code change here is unchanged. Only the docs needed reconciling: the BYOC sentence now sits in #105's rewritten `agents.md` bullet. ## A repeat BYOC init swapped the stored client `initClient`'s BYOC branch sat *above* the singleton idempotency check. So a repeat `initClient(client)`: - **replaced the stored client** with the new one, although the JSDoc said a second call returns the existing client and ignores every other option, and - **re-ran telemetry setup** with the new call's options, which could not take effect because OTel's globals were already held. The fix moves the check ahead of both init paths, matching `_resolve_client` in the Python SDK. #105's one-provider guard in `setupTelemetry` already stops a repeat call from building a second provider. So, on top of #105, the client swap is the only behaviour this PR still changes. > [!NOTE] > **Behaviour change:** a repeat BYOC call with a different client now returns the first client instead of swapping in the new one, as the JSDoc already promised. Call `shutdown()` first to hand the SDK a different client. The `initClient` JSDoc, `AGENTS.md` and `packages/client/agents.md` now describe this. This PR used to correct a `skills.test.ts` comment that said BYOC "deliberately replaces the singleton". #105 has since rewritten that test, so the comment is gone and this PR no longer touches the file. ## Tests - `returns the first client on a repeat BYOC call rather than swapping it`: fails against #105's `lifecycle.ts`. - `does not re-run telemetry setup on a repeat BYOC call`, `swaps in a new BYOC client after shutdown`, and `flushes registered AI package information on a repeat BYOC call`: regression coverage. These already pass on #105. - I re-checked on #105's current tip, `6575893`: the first test still fails against its `lifecycle.ts`, and the other three pass. - `packages/client`: `vitest run` (1274 passed, 10 skipped), `tsc --noEmit`. - Repo root, after `yarn build`: `vitest run` (2027 passed, 10 skipped, 49 files) and `yarn code:check`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Fixes BYOC `initClient` idempotency** so repeat calls match documented behavior and the Node SDK path. > > `initClient` now checks for an existing singleton **before** the BYOC branch. A second `initClient(client, options?)` **returns the first client** (even if you pass a different instance), **does not replace** the stored client, and **does not re-run** telemetry with new options—only **`flushAiSdkInfo`** runs. Call **`shutdown()`** first to register a different BYOC client. > > Docs in **`AGENTS.md`**, **`packages/client/agents.md`**, and **`initClient` JSDoc** describe this. **`lifecycle.test.ts`** adds regression tests (first client wins, single OTLP setup, swap after shutdown, sdk-info flush on repeat). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 38c321e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…el globals it registered (#105) ## Summary Makes a failed or abandoned `initClient` leave nothing behind, so the next call starts fresh instead of replaying the failure, and lets telemetry be set up again after `shutdown()`. Each fix has its own section below. **Rescoped (`865b2ec`).** This PR first moved `initClient`'s `skillStore` install to after a successful init. #114 replaces the `skillStore` option with `setSkillStore` on the experimental entry point, so that change is dropped. The `skillStore` handling, its JSDoc and its test are back to the base branch's, and the `resolveClient` split, which existed only to give that install one success point, is inlined into `initClient`. The other fixes are unchanged. The spec change is launchdarkly/ai-sdks-monorepo#35 (`TESTING.md` §3.9, §3.16). **Merging with #114.** There's one conflict, at the top of `shutdown()`. Take #114's `runShutdownHooks()` in place of `_clearState()`, and keep this PR's `singleton.initPromise = null` without #114's `if (!singleton.client) return;`, because this PR moves that early return below `shutdownTelemetry()`. Everything else, including the `initClient` changes, merges cleanly. ## A failed init can be retried (`50aaa8d`) The `initClient` JSDoc says a rejected call leaves no global state behind. But `singleton.initPromise` cached the first `initBaseClient` promise, rejection included. So a call made before `LD_SDK_KEY` was set failed for the life of the process, even when a later call passed `sdkKey`. `inspectConfig` swallows that error, so the app silently served disabled configs. - `initClient` clears the init promise when it rejects, unless something has replaced it meanwhile, so the next call starts a fresh attempt. - `initBaseClient` closes the client it built when `waitForInitialization` or `waitForTelemetry` rejects. Otherwise each retry would leak a streaming connection. - `shutdown()` clears the init promise before its no-client early return. ## Release the OTel globals this SDK registered, and only those (`3aed185`) Each OTel global is one-shot: a second registration is refused (`Attempted duplicate registration of API: trace`) until the first is disabled. `shutdownTelemetry()` shut the provider down but never released the global, which caused two problems: 1. **init → shutdown → init exported nothing.** The global still pointed at the provider that had been shut down, and the new provider's registration was refused. 2. **Retrying made it worse.** Since the commit above, a failure *after* `setupTelemetry` (no `node-server-sdk`, or a `waitForInitialization` timeout) is retried. The retry built a second provider, which was refused registration but replaced the module handle. The first provider leaked and was never shut down. Changes, mirroring `_owns_otel_globals` / `_release_otel_globals` in the Python SDK: - `setupTelemetry` registers the tracer provider, context manager and propagator one at a time, as `provider.register()` does, and records which globals each setter actually took. `register()` throws those booleans away. If our context manager is refused, it is disabled again. If our tracer provider is refused, we log a `console.warn`, because the telemetry options then have no effect. - `shutdownTelemetry()` releases only the globals we took, so it never tears down a host application's own provider. Ownership is tracked per API rather than as one flag, so a host that registered only a context manager keeps it. - An init that fails after building a provider tears it down. The retry then builds its own provider with the retry's options: `sdkKey` becomes `highlight.project_id`, so reusing the old provider would stamp the wrong project. - `setupTelemetry` builds at most one provider at a time. Without this, a repeat BYOC call would replace the live provider's handle with a refused one and mark us as not owning the globals, which brings bug 1 back. Tests: the `@opentelemetry/api` mock now models one-shot registration. Seven new tests cover init/shutdown/init, a host provider registered first, a host context manager only, repeat BYOC, a timeout followed by a retry, missing `node-server-sdk` followed by a retry, and the retry using its own options. Reverting each part of the fix makes at least one of them fail. Separately, the three OTel-not-installed tests called `vi.doUnmock('@opentelemetry/sdk-trace-node')`, which gave every later test the real provider. They now restore the mock instead. ## An init abandoned by `shutdown()` must not adopt its client (`52c1d0f`) Reported by Bugbot [here](#105 (comment)). `shutdown()` drops an in-flight init promise so the next call starts fresh, but the attempt keeps running. When it finished, `initClient` assigned its client to the singleton unconditionally, with three effects: - It replaced the newer client, leaking that client's connection. - The abandoned call still resolved, as if it had succeeded. - `shutdown()` returned early when there was no client, so the attempt's provider stayed registered. The next init reused it with the abandoned attempt's `sdkKey` as `highlight.project_id`. Changes: - Each options-path attempt now adopts its client itself, once (`startInit`). The check that it is still the current attempt and the assignment run in the same synchronous step. An attempt that `shutdown()` or a BYOC call replaced closes its own client and rejects. - `shutdown()` tears down telemetry whether or not a client exists. - A failed attempt tears down its provider only while that provider is still the current one. An abandoned attempt that fails later therefore can't shut down a provider a newer attempt has built. ## No provider for an init abandoned during telemetry setup (`6720f6f`) Reported by Bugbot [here](#105 (comment)). If `shutdown()` ran while `setupTelemetry` was still awaiting its dynamic imports, there was no provider yet to tear down. The abandoned attempt then built one anyway, and the next init reused it, again with the abandoned attempt's `sdkKey`. `shutdownTelemetry()` now counts teardowns, including those with no provider. A setup that sees the count change while its imports are pending builds nothing. So an attempt that started before a teardown can never outlive it with a provider. One side effect: an app that calls `shutdownTelemetry()` directly while an init is in flight gets an init with no telemetry, which is what it asked for. Tests: five new ones cover a `shutdown()` that lands mid-init, a BYOC call that lands mid-init, the in-flight provider teardown, an abandoned attempt that fails while a newer one is in flight, and a `shutdown()` before setup's imports resolve. The existing in-flight test now also asserts that the abandoned call rejects. Reverting each part of either fix makes at least one test fail. ## Related #103, now stacked on this PR, keeps only its first fix: a repeat BYOC `initClient` returns the existing client. Its second and third fixes are superseded by this PR. ## Verification - `packages/client`: `vitest run` (1234 passed, 10 skipped). - Repo root: `yarn build`, `yarn typecheck`, `yarn code:check`, `yarn test` (every workspace) and `yarn test:integration`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Hardens **`initClient` / `shutdown` / `shutdownTelemetry`** so failed, abandoned, or repeated initialization does not leave stale clients, cached rejections, or broken OpenTelemetry state. > > **Failed init is retryable.** Rejected `initClient` calls no longer stick in `initPromise`; the LD client built for a failed attempt is closed, and telemetry built for that attempt is torn down when it is still the active provider. That fixes permanent failure when `LD_SDK_KEY` arrives late (including paths where `inspectConfig` swallows the error). > > **OTel lifecycle matches one-shot globals.** `setupTelemetry` registers tracer provider, context manager, and propagators individually, records which globals it actually took, warns when a host app already owns the tracer provider, and builds at most one provider. `shutdownTelemetry` shuts down the provider and **`disable()`s only the globals this SDK registered**, so init → shutdown → init works and coexisting host instrumentation is not torn down. In-flight setup abandoned by `shutdown()` (including during dynamic imports) is detected via a teardown counter so a late-built provider is not reused with stale options. > > **Concurrent / BYOC edge cases.** Options-path in-flight inits adopt their client via `startInit`; if `shutdown()` or a later `initClient` replaces the attempt, the old one closes its client and rejects with an abandonment error instead of overwriting the winner. Repeat **BYOC** calls return the first client and do not re-run telemetry; `shutdown()` always clears the in-flight promise and runs telemetry teardown even when no client exists yet. > > Docs (`AGENTS.md`, README, `agents.md`) and extensive `lifecycle.test.ts` coverage reflect the new behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit cf49bd7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4de1cd7. Configure here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`main` added `camelizeModelParameters`, `normalizeModelParameters` and `pickForwardedModelParameters` to the package root (#73). Add them to the root-export snapshot and regenerate the root API report, so the snapshot test and `api:check` pass after the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
This is the feature branch that all Agent Skills PRs were merged into for one final review before merging to main, in preparation for inclusion in a release. The repo squashes with the PR title and body as the commit message, and release-please reads the (Moved here from the PR description so it doesn't end up in the squash commit message.) |
3 of 4 tasks
XieX
changed the title
XieX
added a commit
that referenced
this pull request
Oct 8, 2026
… needs (#119) ## Summary Pins `@launchdarkly/ai-server` to a specific version rather than `"*"`, and uses the `node-workspace` plugin for `release-please` to automatically update it as part of the release workflow. ## Details Every package that depends on `@launchdarkly/ai-server` asks for `"*"`. A consumer whose lockfile already holds an older ai-server keeps it when they upgrade a handler or `@launchdarkly/ai-node`, because `"*"` is still satisfied. That is already a problem on main: - Every handler imports `normalizeModelParameters` / `pickForwardedModelParameters`, which published `@launchdarkly/ai-server@0.3.0` does not export. As it stands, the next release (#85) would publish handlers that fail at import against a locked 0.3.0. - #71's `@launchdarkly/ai-node/experimental` re-exports `@launchdarkly/ai-server/experimental`. A pre-release review installed the packed ai-node with the published ai-server 0.3.0 and got `ERR_PACKAGE_PATH_NOT_EXPORTED`. ### Change - `release-please-config.json`: add the `node-workspace` plugin, so each release PR rewrites dependents' ranges to the version it releases. - All nine dependents: `"*"` → `"^0.3.0"`. The plugin keeps a `^`/`~`/`>=` prefix, but it turns `"*"` into an exact pin (`0.4.0`) and keeps it exact from then on. ### Why not just pin `^0.4.0` now Yarn v1 links a workspace sibling only when its version satisfies the range. Local `packages/client` stays at 0.3.0 until the release PR bumps it, so `^0.4.0` makes yarn go to npm, where 0.4.0 doesn't exist, and `yarn install --frozen-lockfile` fails with `Couldn't find any versions for "@launchdarkly/ai-server" that matches "^0.4.0"`. `^0.3.0` links the local package now, and `^0.4.0` links it once the release PR has bumped it. `yarn.lock` is unchanged either way, since yarn v1 doesn't lock workspace packages. ### Policy change to be aware of `node-workspace` gives every dependent a patch release, with a "Dependencies" changelog entry, whenever ai-server releases. It does this even when the dependent has no changes of its own, and release-please 17.6.0 has no option to turn it off (`always-link-local` is parsed but never read: googleapis/release-please#2876). The nine dependents are already in the pending release, so this release is unaffected. ### Verification - `yarn install --frozen-lockfile`, `yarn lint:pkg` (sherif), and the biome pre-commit hooks pass. - I ran `release-please release-pr --dry-run` (17.6.0, the version release-please-action v5.0.0 bundles) against this branch. It proposes the same versions as #85: ai-server 0.4.0, ai-node 0.3.0, ai-otel 0.2.0, handlers 0.4.0, vercel-* 0.2.0. Each of the nine dependents gets `@launchdarkly/ai-server bumped from ^0.3.0 to ^0.4.0` in its `package.json` and changelog. Merge this before #85. It doesn't conflict with #71, which doesn't touch these dependency lines or the release config. The Python SDK has the same unpinned `launchdarkly-ai-server` dependencies, but doesn't need a similar change yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code), updated by @XieX <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Replaces wildcard `@launchdarkly/ai-server` dependencies (`"*"`) with **`^0.3.0`** across nine workspace packages (`ai-node`, claude/openai/langchain/vercel agents and messages). That forces installs/upgrades to pull a compatible **ai-server** instead of keeping an older lockfile version that still satisfies `*`, which was breaking imports and experimental re-exports against published **0.3.0**. > > Adds the **`node-workspace`** plugin to **`release-please-config.json`** so future release PRs automatically bump dependents’ **ai-server** ranges when **packages/client** releases (e.g. `^0.3.0` → `^0.4.0` on the next cut). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1f3b9d3. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Fixes the must-fix-before-merge findings from the pre-release review of
Agent Skills (reviewed at `c27005c`). There is one commit per fix, and
each fix has a test that fails without it.
## Fixes
1. **The process exited while `await store.waitForSkills()` was
pending** (`ca79d2f`). The wait's `setTimeout` was unreffed. When
LaunchDarkly is unreachable, the backoff and deadline timers (also
unreffed) are the only other handles, so `node` exited mid-`await`: exit
13 under ESM top-level await, and exit 0 in CommonJS without running the
next line. The wait's timer is now referenced. It is bounded by the
timeout and cleared when the wait is released. The backoff and deadline
timers stay unreffed.
*Test:* child-process scripts, run through `tsx` against the source: ESM
top-level await, a CommonJS async function, and a never-started store.
Each asserts exit 0 and that the wait resolves `false` at the timeout.
2. **A streamed event whose `data` is not JSON was dropped, and the
transfer still committed** (`88e21c1`). A corrupted `delete-object` kept
a revoked skill served, because the reconnect sent the advanced `basis`.
A corrupted `put-object` inside `xfer-full` revoked that skill by
omission. `iterSse` now throws `RecoverableTransportError`, so the
in-flight payload is abandoned and the reconnect resumes from the last
committed basis. This matches js-sdk-common's `PayloadStreamReader`,
which reports `InvalidData`; `StreamingProcessorFDv2` then stops the
source. Only the six events the reader acts on are parsed
(`server-intent`, `put-object`, `delete-object`, `payload-transferred`,
`goodbye`, `error`). A `heart-beat` or an unknown event is yielded with
`null` data, so its data can't end the connection (`4f2375f`).
*Tests:* commit `{a:1, b:1}`, then send `xfer-changes` with `put c:1`, a
corrupt `delete-object a:1`, and `payload-transferred`. The test asserts
that the reconnect carries `basis-1`, that listeners see `c:1` once
(from the clean retransmission), and that the retransmission revokes
`a`. A second test commits `a:1`, then sends an `xfer-full` with a clean
`put b:1` and a corrupt `put a:1`. It asserts the reconnect carries
`basis-1`, `a` is never revoked, and `b` reaches listeners once.
`iterSse` framing tests cover the corrupt `delete-object`, plus a
`heart-beat` and an unknown event with `data: not json`. Framing tests
also pin the parse set (all six), and a store-level test streams a
catastrophic goodbye through `iterSse` (`7978770`). Empty data on a
known event is not JSON either (`fe8e5ec`). Read as `null`, a
`delete-object` was ignored and a `payload-transferred` committed with
no selector. Two protocol-reader tests pin the experimental-stage rule
for a put or delete with no usable `key`: it is warned and ignored, and
its transfer commits (TESTING.md §3.25, ai-sdks-monorepo#48).
3. **A `goodbye` with `catastrophe: true` was fatal until `start()`**
(`c71345e`). Neither base JS package reads `catastrophe`, and the
streamer sets it on its own connection-error path. It is now a
recoverable disconnect. It is counted even after a completed exchange
(never `expected`), recorded in `lastError`, and logged at error level
whatever `silent` says.
*Test:* the reader test that pinned the fatal reading now asserts the
new outcome. A store-level test covers a commit followed by a
catastrophic goodbye: the store reconnects, `failed` stays `null`, and
the goodbye is logged at error level.
4. **`skillRefs(null)` returned `[]`** (`9466bd8`). `inspectConfig`
answers `config: null` on any failure, so
`writeSkills(skillRefs(info.config), root)` pruned every managed skill
during an outage. A config that is not an object, `null` and `undefined`
included, now throws `TypeError`. An object with no `skills` field still
returns `[]`. The TSDoc, the README pipeline example and the API table
are updated.
*Test:* non-object configs (null, undefined, string, number, boolean,
array), plus a failed `inspectConfig` whose `config` throws.
5. **Docs** (`fa44c1a`). The README promised that
`ld.skills.integrity_failure` would not change "outside a major
release". It now says that, while the feature is experimental, the
record may change in a minor release with a changelog entry under
**Experimental**.
## Cross-SDK and spec
- **python-ai-sdk#148** makes the same changes. Python's
`skill_refs(None)` now raises `ValueError` (JS throws `TypeError`), so
the two SDKs agree apart from the error class, which A.12 records.
- **ai-sdks-monorepo#48** carries the spec text: the
catastrophic-goodbye carve-out in §3.25, the experimental-stage wording
for the integrity event in §3.24 and A.12, the non-object rule in §3.20,
and the non-JSON rule in §3.25.
## Checks run locally
`yarn build`, `yarn test` (client: 1325 passed, 10 skipped), `yarn
typecheck`, `yarn code:check`, and `yarn workspace
@launchdarkly/ai-server api:check`. The only API-report changes are
source line numbers in a comment.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Overview**
> Addresses pre-release **must-fix** issues in experimental Agent
Skills: safer config projection, more resilient FDv2 streaming, and
process-lifetime behavior during outages.
>
> **`skillRefs`** no longer treats a failed `inspectConfig` (`config:
null`) as “no skills” — non-object configs (including
`null`/`undefined`) now throw **`TypeError`** so
`writeSkills(skillRefs(config), …)` cannot prune every managed skill
during an LD outage. README/docs and tests are updated accordingly.
>
> **FDv2 SSE (`iterSse`)** now fails the connection with
**`RecoverableTransportError`** when a *known* protocol event’s `data`
is missing or not JSON (instead of skipping it and still committing
`payload-transferred`). That abandons the in-flight transfer and
reconnects from the last committed `basis`, fixing cases where a bad
`delete-object` left revoked skills served or a bad `put-object` in
`xfer-full` revoked by omission. Only the six reader-handled events are
JSON-parsed; heartbeats/unknown events stay `null` data.
>
> **Catastrophic `goodbye`** is no longer fatal (delivery keeps
retrying); it is always counted in **`connectionFailures`**, sets
**`lastError`**, and logs at error level even when `silent`.
>
> **`waitForSkills`** keeps its timeout timer **referenced** (other
store timers stay unreffed) so Node does not exit mid-`await` when
delivery is down and nothing else holds the event loop.
>
> Docs clarify that **`ld.skills.integrity_failure`** may change in
minor releases while Agent Skills is experimental.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
fe8e5ec. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
andrewklatzke
pushed a commit
that referenced
this pull request
Oct 8, 2026
…ing (#123) ## Summary Lists Agent Skills changes under an **Experimental** heading in each package's changelog, as TESTING.md §0.1 promises for an experimental feature. Spec: launchdarkly/ai-sdks-monorepo#50. Python twin: launchdarkly/python-ai-sdk#150. ## How `release-please-config.json` gains `changelog-sections`. Rows matching `feat`, `fix` and `perf` with scope `skills` map to "Experimental". The rest of the list reproduces the defaults release-please 17.6.0 uses for the Node strategy (the `conventionalcommits` preset's list), so every other commit renders exactly as before. Matching on scope works because release-please hands `changelog-sections` to `conventional-changelog-conventionalcommits` 6.1.0 unchanged (`manifest.ts:1397`), and the preset's `findTypeEntry` checks an entry's `scope` (`writer-opts.js:60-71`). The first match wins, so the scoped rows come first, and "Experimental" is therefore the first heading. `scope` isn't in release-please's documented config schema. Rerun the checks below when upgrading release-please. `AGENTS.md` gains a "PR Titles and the Changelog" section with the convention: - an experimental change uses its feature's scope, and only that one scope; - a PR that mixes core and experimental changes is split, or uses `BEGIN_COMMIT_OVERRIDE`; - a breaking experimental change uses `!` while the package is 0.x, and drops it after 1.0. The block sits at the end of the file so it doesn't conflict with #119, which adds `plugins` after `bootstrap-sha` (`git merge-tree` is clean). ## Verification - **Rendering:** I ran release-please 17.6.0's own commit parser and `DefaultChangelogNotes` against this config, with synthetic commits. - Non-experimental commits render byte-for-byte the same as with the defaults. - `feat(skills)`, `fix(skills)`, `perf(skills)` and `feat(skills)!` land under Experimental. A breaking one also appears in "⚠ BREAKING CHANGES". - `docs(skills)` stays hidden, `feat(skills,client)` lands under Features, and `feat(client)` stays under Features. - **Dry run:** `release-please release-pr --dry-run` against this branch proposes the same packages, versions and notes as release PR #85, apart from the date. Nothing on `main` uses the `skills` scope yet. - **Agent Skills:** #71 is retitled `feat(skills): Agent Skills`, so its squash commit lands under Experimental. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Configures **release-please** so Agent Skills work (`feat`/`fix`/`perf` with scope `skills`) appears under an **Experimental** changelog heading, while the rest of the section list mirrors the default Node/conventional-commits preset so other commits render unchanged. > > Adds **PR Titles and the Changelog** to `AGENTS.md`, documenting squash-merge titles, the single `skills` scope, mixed core/experimental PRs (`BEGIN_COMMIT_OVERRIDE`), breaking experimental commits while packages are 0.x, and removing experimental rows when a feature graduates. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5c1e697. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…onds (#121) Stacked on #120. This moves every `FDv2SkillStore` time option to seconds, for consistency with the Python SDK, the base Node SDK (`pollInterval`, `timeout`, `waitForInitialization({ timeoutSeconds })`) and `writeSkills`' existing `timeout`. Technically we don't _have_ to merge this before releasing the SDK as experimental, since the API can change after that, but in the interest of reducing consumer code churn I put it together now. ## What changed Each option is **renamed rather than reinterpreted**, so no existing name changes meaning: | Before (ms) | After (seconds) | Default | |---|---|---| | `pollIntervalMs` | `pollIntervalSeconds` | `30` | | `readTimeoutMs` | `readTimeoutSeconds` | `10` poll / `300` stream | | `initialBackoffMs` | `initialBackoffSeconds` | `1` | | `maxBackoffMs` | `maxBackoffSeconds` | `30` | | `waitForSkills(timeoutMs)` | `waitForSkills({ timeoutSeconds })` | `10` | | `DEFAULT_POLL_TIMEOUT_MS`, `DEFAULT_STREAM_READ_TIMEOUT_MS` | `DEFAULT_POLL_TIMEOUT_SECONDS`, `DEFAULT_STREAM_READ_TIMEOUT_SECONDS` | module constants, not on the entry point | - **`waitForSkills` takes an options object** because a positional argument can't be renamed: `waitForSkills(10_000)` would silently wait almost three hours. A bare number, `null` or a string rejects with a `TypeError` that names the new form. - **The retired `...Ms` keys are not read.** TypeScript rejects them in an object literal; from JavaScript they are ignored and the default applies. - **`watchSkills`' `debounceMs` stays in milliseconds**, along with `SKILLS_DEFAULT_DEBOUNCE_MS`. It is now the one millisecond option in the skills API. - **Validation is unchanged.** Values must be positive and finite (non-negative for the wait), `NaN` and `Infinity` are rejected, and the checks run in seconds. - **Values become milliseconds only in `secondsToMs`**, at the `setTimeout` or read deadline they set. It rounds rather than truncates: `1.005 * 1000` is `1004.9999999999999`, and the base SDK's `Math.trunc` would give 1004. A positive option is floored at 1 ms, so a sub-millisecond value never becomes a zero delay. ## Tests - The existing suite is converted to the new names and units. - New tests cover `waitForSkills` rejecting a bare number, `null` and a string, the 10-second default, a timeout of zero, positive `Infinity`, and the `secondsToMs` table. - An end-to-end test checks that `pollIntervalSeconds: 1.005` sleeps 1005 ms. Swapping `Math.round` for `Math.trunc` turns both precision tests red. `yarn build`, `yarn test` (client: 1318 passed, 10 skipped), `yarn typecheck`, `yarn code:check` and `api:check` all pass. `etc/ai-server-experimental.api.md` is updated. ## Spec text that now needs updating (ai-sdks-monorepo `TESTING.md`; not edited here) - §3.25, the backoff bullet: "`initial_backoff` / `initialBackoffMs` and `max_backoff` / `maxBackoffMs` raise in the constructor…". These should become `initialBackoffSeconds` / `maxBackoffSeconds`. - A.12 has no row for the transport time options or for `waitForSkills`' shape. Python uses `poll_interval`, `read_timeout`, `initial_backoff`, `max_backoff` and `wait_for_skills(timeout)`, while TypeScript now uses the `...Seconds` names and `waitForSkills({ timeoutSeconds })`. §0.3 says "Both languages use the same names, even while experimental", so A.12 should record this as a sanctioned difference: the unit is shared and the suffix is TypeScript-only. §3.26 already makes the same argument for `debounceMs` ("a bare `debounce` number in a language without keyword arguments does not say what unit it is in"). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Breaking change (experimental):** `FDv2SkillStore` timing is expressed in **seconds** and aligned with the base Node/Python SDKs and `writeSkills`' `timeout`. Millisecond option names are **renamed**, not reinterpreted (`pollIntervalSeconds`, `readTimeoutSeconds`, `initialBackoffSeconds`, `maxBackoffSeconds`; defaults stay 30s / 10s poll / 300s stream / 1s / 30s). > > `waitForSkills` now takes **`{ timeoutSeconds }`** (default 10) instead of a positional millisecond argument. A bare number, `null`, or string **throws `TypeError`** so old calls like `waitForSkills(10_000)` cannot silently wait ~3 hours. Legacy `...Ms` constructor keys are **ignored** with a one-time warning. > > Implementation adds **`secondsToMs`** (round, 1 ms floor for positive delays, cap at `2^31−1` for `setTimeout`) used only at timer/deadline sites. **`watchSkills` `debounceMs` is unchanged** — the only ms-based skills API surface. > > Docs (`README`, `agents.md`), public API report, tests, and an error hint in `writeSkills` when the store is uninitialized are updated accordingly. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 64da00d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Andrew Klatzke <aklatzke@launchdarkly.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Brings in #115 (helper usage), #123 (Experimental changelog heading), #78 (bundler import diagnostics) and #119 (node-workspace ranges). lifecycle.ts conflict: keep the branch's init/shutdown structure and add main's usage flush beside each flushAiSdkInfo call (already-initialized, BYOC, and after a successful options-path await), so a failed or abandoned init never flushes and a retry flushes once. shutdown() resets usage with sdk-info, only when a client existed, as main's early return did; held helpers survive a no-client shutdown. shutdownTelemetry() keeps the branch's OTel-globals teardown and gains main's ESM warning reset. Root surface: add main's configInternal, createHandlerInternal, graphInternal, inspectConfigInternal and reportUsage to ROOT_VALUES and regenerate etc/ai-server.api.md. The experimental report is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…handle (#129) Same finding as the Bugbot comment on launchdarkly/python-ai-sdk#87 (comment 4232816271). The gap is identical in this SDK. **What was wrong:** `openOrCreateDirectory` in `safe-fs.ts` created `<root>/<key>/` with `mkdir(directory, 0o755)`, but the process umask still masks that mode, and nothing set it afterwards. Under `umask 0077` the directory came out `0700`, so an agent running as a different identity couldn't read `SKILL.md`, even though `writeSkills` reported success. The README's privilege-separation section promises per-skill directories at `0755` no matter what the umask is. **Fix:** after a successful create, the code sets the mode to `0755` with `FileHandle.chmod` (that is, `fchmod`) on the handle `openDirectoryNoFollow` already returns, the same way `FILE_MODE` is applied to files. It never chmods the path, because a swap between the mkdir and the open could redirect a path-based chmod. - The mode is only changed when this call created the directory. An existing `<root>/<key>/` keeps its mode. - The managed root's mode is still up to the user, as the README says. It's created by a separate `mkdir(root)` in `skills-fs.ts` and isn't touched here. - **A setgid bit the new directory inherited is kept.** On Linux, a directory created under a setgid parent gets the parent's `S_ISGID` bit, and `fchmod` uses exactly the bits passed, so a bare `0755` cleared it. Files written inside would then get the reconcile identity's primary group instead of the shared group. The mode is now `(mode & S_ISGID) | 0755`, with the current mode read through `handle.stat()` on the same descriptor. macOS doesn't propagate the bit, so nothing changes there. - The symlink and not-a-directory refusals are unchanged. If the chmod fails, the handle is closed before the error is rethrown. **Tests (skipped on win32):** - `creates per-skill directories as 0755 even under a restrictive umask` runs `writeSkills` under `process.umask(0o077)` and asserts the directory is `0755` and `SKILL.md` is `0644`. It fails without the fix. - `keeps a setgid bit the new skill directory inherited` makes the root `2755` and asserts the new skill directory is `2755`. It's skipped unless a probe directory under that root actually inherits the bit, so it runs on Linux and skips on macOS. On Linux (Docker, `node:22`) it fails without the setgid change. `fs.constants` has no `S_ISGID`, so `safe-fs.ts` defines it locally. - `leaves the mode of an existing directory alone` checks that an existing `0700` directory stays `0700`. Full suite: 1394 passed on macOS (the setgid test skips) and 1405 passed on Linux. biome and tsc are clean. The Python fix is in a separate PR, linked below. The spec lines are in launchdarkly/ai-sdks-monorepo#53 (merged) and its setgid follow-up. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Agent Skills
Skills are versioned SKILL.md documents managed in LaunchDarkly and attached to AI Config variations by reference. This adds the full server-side surface for them: the SDK reports which skills a resolved config references, retrieves their content over LaunchDarkly's FDv2 delivery channel, verifies it, and materializes it onto disk as //SKILL.md — where the Claude Agent SDK and anything else following that convention discovers it.
Experimental. Every skills name is imported from
@launchdarkly/ai-server/experimental(or@launchdarkly/ai-node/experimental, which re-exports it); none is exported from either package root, which the checked-in API reports underpackages/client/etc/pin (api:checkruns in CI). Any of them may change in a minor release. The store is configured withsetSkillStore(store), not throughinitClient: askillStoreoption is warned about as unrecognized and ignored.Release notes
release-please reads the block below in place of this PR's title, one changelog entry per line. The skills feature comes first; the rest are the core, non-skills behaviour changes this branch also makes.
BEGIN_COMMIT_OVERRIDE
feat(skills): add experimental Agent Skills under
@launchdarkly/ai-server/experimental, re-exported from@launchdarkly/ai-node/experimentalfeat(client):
initClient(client, options)accepts telemetry options for a pre-initialized clientfeat(client): warn about unrecognized
initClientoptions instead of ignoring them silentlyfix(client):
shutdownTelemetry()releases the OpenTelemetry globals the SDK registered, so a laterinitClientexports spans againfix(client): an
initClientthat fails, or thatshutdown()interrupts, closes the client it built, and a failure is no longer cached, so the next call retriesfix(client): a repeat
initClient(client)returns the client already set instead of replacing itEND_COMMIT_OVERRIDE
Also in the telemetry change: when another library registered an OpenTelemetry tracer provider first, the SDK now warns and leaves that provider's globals alone, and
shutdownTelemetry()disables only the globals this SDK took.Note
Overview
Adds experimental Agent Skills on
@launchdarkly/ai-server/experimental(and@launchdarkly/ai-node/experimental): resolve skill refs from AI configs, verify hash-backed content via a pluggableSkillStore, deliver from LaunchDarkly throughFDv2SkillStore, and reconcileSKILL.mdfiles on disk withwriteSkills/watchSkills. Nothing skills-related is exported from package roots;AiConfigRep.skillsstaysunknownand core parsing leaves it unvalidated soconfig()/inspectConfig()are unaffected.Packaging & CI: dual
tsupentry points,exports['./experimental']plustypesVersionsfor node10 resolution, API Extractor reports underpackages/client/etc/, and a new CI stepapi:check.Lifecycle / telemetry (non-skills):
initClient(client, options?)forwards telemetry options on BYOC; repeat init is idempotent and does not swap clients; failed or abandoned inits close resources and can retry;shutdown()runs registered shutdown hooks (skills state clears without root importing experimental);shutdownTelemetry()releases only the OTel globals this SDK registered; unrecognizedinitClientkeys log a warning.Integration tests now assert experimental subpaths, tarball contents, and consumer
tscunder severalmoduleResolutionmodes.Reviewed by Cursor Bugbot for commit 960ecc3. Bugbot is set up for automated code reviews on this repo. Configure here.