Agent Skills - #71
Draft
XieX wants to merge 14 commits into
Draft
Agent Skills#71XieX wants to merge 14 commits into
XieX wants to merge 14 commits into
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>
> **Stacked PR 6 of 7 — merge bottom-up into `xie/agent-skills-feature-ac9ac7`.** > 1. #29 — value types > 2. #30 — symlink-refusing filesystem primitives > 3. #31 — store seam, verification, accessors > 4. #32 — integrity + telemetry tests > 5. #33 — writeSkills + abuse matrix > 6. **#34 — remaining writeSkills tests ← you are here** > 7. #35 — docs > > Each PR targets the one below it, so GitHub already shows only this PR's own diff. --- Sixth of seven. Test-only — the functional half of the materialization suite, split out so it isn't read alongside the implementation and the abuse matrix in one sitting. This closes the coverage gap the previous PR flagged: until now `writeSkills` had its defenses proven but not its ordinary behaviour. ## What's covered - **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's 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. Directly constructed `Skill`s carry `Uint8Array` content, and the pass hashes those bytes as-is; the `content_bytes` telemetry property is asserted to be the encoded byte count. - **Orphaned temp files** — the sweep added in #33 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. ## Verification Client package 555 → 619 tests (+2 skipped). Workspace 1148 passing. `typecheck`, `biome`, and `sherif` clean. The 2 skips are expected: the TOCTOU swap-race pair, gated off `SUPPORTS_DIR_FD`, which is `false` on Node because it exposes no `*at()` syscall family. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Test-only expansion** of `skills-fs.test.ts`: closes the gap where `writeSkills` had security/abuse coverage but not ordinary materialization behavior. > > Adds large new Vitest blocks for **basic writes** (Skill vs store refs, `"*"`, reconcile actions), **manifest** byte-level contract (sorted keys, no trailing newline, `\uXXXX` escaping, unknown field preservation), **reconcile/prune** semantics, **root and argument validation**, **atomic rename/unlink** (including `interceptUnlink` and `assertAtomicRenameOf`), **resilience** (`onUnavailable`, timeouts, no prune on store outage, no retries), **verify-then-write**, **orphaned temp sweep** (narrow pattern, corrupt manifest suppresses sweep), and the **write-side telemetry** allowlist (Materialized, Revoked, Integrity; no paths or bodies). > > Shared harness additions: `RecordingEmitter`, `_setStore` / `_setEmitterForTesting`, telemetry signal constants, and a **test hygiene** check that `fsOps` mocks are restored between tests. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1c2c602. 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 2 potential issues.
❌ 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 2d5712d. Configure here.
|
|
||
| const deadline = performance.now() + timeout * 1000; | ||
| const rootPath = await resolveRoot(root); | ||
| const { manifest, error: manifestError } = await loadManifest(rootPath); |
There was a problem hiding this comment.
Linux root not fd-pinned
High Severity
After resolveRoot, writeSkills re-opens the managed root and every skill directory by absolute path. On Linux, O_NOFOLLOW only protects the last component, so a swapped root redirects later writes and deletes (SEC-8985). atomicWriteIn re-opens the root the same way for the manifest, and the capability probe checks for *at() instead of /proc/self/fd.
Additional Locations (2)
Triggered by learned rule: Pin skills root via /proc/self/fd; never re-open by path
Reviewed by Cursor Bugbot for commit 2d5712d. Configure here.
| reason: 'wrong_version', | ||
| }; | ||
| } | ||
| return { skill, reason: 'ok' }; |
There was a problem hiding this comment.
Store key not equality-checked
Medium Severity
resolveFromStore withholds a mismatched version but never compares the returned object's key to the one that was requested. The store is untrusted, so a lookup for one skill can yield a different verified skill, and writeSkills then materializes that other key.
Triggered by learned rule: Agent Skills types: fail-closed, opaque bytes, Python-identical
Reviewed by Cursor Bugbot for commit 2d5712d. Configure here.
XieX
marked this pull request as draft
September 18, 2026 20:32
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
1 participant
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.
This is the feature branch that all PRs will get merged into for one final review before merging to main in preparation for inclusion in a release.
Note
Overview
Adds the Agent Skills surface to
@launchdarkly/ai-server: AI configs can reference versioned skills, the SDK can resolve them from a configurableSkillStore, verify content (SHA-256, size cap, UTF-8, key grammar), and materializeSKILL.mdunder a managed root with a shared.launchdarkly-skills.jsonmanifest and reconcile reporting.Public API is wired through
index.ts(skillRefs,getSkill/getSkills/allSkills,getSkillResult,writeSkills, value types, and constants).initClientnow acceptsskillStore(applied on every call; cleared inshutdown), andparseAiConfigfails closed on malformedskillsentries (new tests inschema.test.ts).Implementation splits shared logic in
skills-core.ts(global store/emitter, integrity signals,ld.skills.integrity_failureconsole record,resolveFromStore) from symlink-aware I/O insafe-fs.ts(O_NOFOLLOW, exclusive temps, atomic rename, pinned-directory identity checks; documents Node’s residual TOCTOU without*at()). Disk reconcile behavior, manifest rules, pruning, clobber protection, and the security matrix are covered by large new Vitest suites (skills.test.ts,skills-fs.test.ts,safe-fs.test.ts).Reviewed by Cursor Bugbot for commit 2d5712d. Bugbot is set up for automated code reviews on this repo. Configure here.