Conversation
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>
XieX
force-pushed
the
xie/skills-08-review-closeout
branch
from
September 14, 2026 20:13
90b45a5 to
888a616
Compare
XieX
force-pushed
the
xie/skills-fdv2-transport
branch
from
September 14, 2026 20:13
4be5659 to
d0ccb0f
Compare
XieX
marked this pull request as ready for review
September 15, 2026 15:02
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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>
A reconnect whose basis is already current is answered with the `none` intent and no payload, then the connection is recycled with a goodbye. Nothing committed on such a connection, and only a commit cleared the consecutive-failure count, so every healthy recycle counted as a failure and delivery gave up for good after `maxConsecutiveFailures` of them — on an environment whose skills simply never changed. A `server-intent` that parses now clears the count, whether or not a payload follows, and a non-catastrophic goodbye reconnects without counting as a failure or appearing in the diagnostics. Dropped streams, read timeouts, HTTP errors, and `error` events count as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… gives up Giving up on delivery released first-payload waiters by marking a first payload, so `waitForSkills` resolved `true` after a fatal failure such as an unauthorized key. A caller gating boot on the return value went on to boot against a store that would never receive content. Release the waiters without marking a payload, so they resolve `false` promptly, and answer a later `waitForSkills` from `failed` rather than making it sit out the full timeout. A 304 before any payload still counts as a first payload, and `close()` still releases waiters with `false`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A block with no `event:` field left its `data:` lines in the buffer, so the next event parsed as the two payloads concatenated, failed `JSON.parse` and was discarded. Dispatch now resets the buffered fields whether or not the block becomes an event. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three defects in the delivery retry path, all in the same
retry/HTTP-classification region.
A `goodbye` is exempt from `maxConsecutiveFailures` because it is how a
long-lived stream is recycled. That exemption was unconditional, so a
server closing every connection with a non-catastrophic `goodbye`
without ever sending a `server-intent` reconnected without limit, with
nothing reaching `failed` or the diagnostics. Only a connection that
reached a working server was being served normally, so only that one is
exempt now; a `goodbye` on a connection that never got there delivered
nothing and is counted, logged and bounded like any other failure. A
genuine recycle sends `server-intent` first, which resets the count, so
an unchanging environment is unaffected.
`Number('')` is 0 and finite, so an empty `Retry-After` — a blank header
from a proxy is enough — was honoured as no delay at all, burning every
retry the bound allows in milliseconds and going permanently fatal. A
blank value is now absent rather than zero, and an honoured `Retry-After`
is floored at `initialBackoffMs` so a legal `Retry-After: 0` still waits.
The `maxBackoffMs` clamp on the other side is unchanged.
HTTP 400 was permanently fatal, but the request carries a `basis`
selector and an etag, which can go stale and leave delivery stopped for
the process lifetime. A 400 for a request carrying either now clears both
and asks for a full transfer once; a 400 for a request carrying neither
is the request itself being refused, and is still fatal with the same
base-URI and FDv2 advice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…accurately Three reporting defects in the FDv2 skill transport. What the store holds, what it serves, and what reaches listeners are all unchanged; only the counters and the log volume move. A full transfer that moves a key to a new version no longer counts as a revocation. `revocationsBetween` still diffs at `(key, version)`, so both halves of a version move reach listeners as a put and a tombstone, but `objectsRevoked` now counts only the keys that left the payload entirely — the counter operators alert on, so a routine version bump must not raise it. The wholly-hashless summary is deduped. It speaks when the condition becomes true and whenever the withheld objects change, and stays quiet for a store that has not moved: with `contentHash` absent from the wire, a polling connection answered with `xfer-changes` was logging the identical paragraph once per interval indefinitely. A store that recovers and relapses is reported again. The set backing both dedupes is now bounded, and is cleared outright once everything held verifies, so a long-running agent whose skills are versioned frequently no longer accumulates an entry per version for the life of the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ecycle
Three lifecycle edges on the skill stores' listener and wait surfaces, all
of which produced a store that looked live and was not.
`addListener` accepted a listener under any kind and only ever notified
'skill', so `addListener('flag', fn)` was retained and never called — the
same silent-watcher failure `watchSkills` already refuses loudly for a
store with no `addListener` at all. Both shipped stores now throw for a
kind they cannot notify. `removeListener` still tolerates any kind, so
`SkillWatcher.close` can keep detaching unconditionally.
`waitForSkills` pushed a resolver closure into `firstPayloadWaiters` and
left it there when the timeout fired, retaining one entry per timed-out
call for the store's lifetime. The timeout path now removes its own
waiter.
A wait started after `close()` saw no first payload and no failure, so it
built a fresh timer and ate the full duration — while the README promised
`false` for a closed store. `close` now records that it ran and
`waitForSkills` short-circuits on it, as it already did for delivery that
gave up. `failed` stays null: closing is not a failure. `start` refuses a
closed store rather than opening a second delivery loop on one the caller
shut down.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fault `FetchRequester` sent `GET /sdk/stream` to the polling host. LaunchDarkly serves streaming from `stream.launchdarkly.com` and polling from `sdk.launchdarkly.com`, matching the base server-side SDK's defaults, so the default streaming store would have connected to the wrong host on first contact with a real environment. Adds `DEFAULT_STREAM_URI` and a `streamUri` option. A `baseUri` given without `streamUri` is used for both endpoints, so a relay or private instance serving both from one host, and the test fake, keep working unchanged. The hook's biome checks were run by hand from the main checkout; this worktree has no node_modules for it to find. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… date The delivery fixes changed rules agents.md still stated the old way, on exactly the points it exists to pin down. The consecutive-failure rule was the wrong one: the counter resets on any sign of a working server, not only at a committed payload, because a reconnect whose basis is current is answered with the `none` intent and commits nothing. Records the `reachedServer` qualifier alongside it, since dropping either half brings back one of the two failures — an unchanging environment expiring, or a goodbye-only server reconnecting unbounded and unreported. Retry delays are now floored at `initialBackoffMs` as well as clamped to `maxBackoffMs`, and a blank `Retry-After` reads as "none given" rather than as zero. The hashless summary is per distinct withheld set, not per payload, and `_warnedHashless` is capped. Adds the contracts that were established without being written down: the split poll and stream origins, HTTP 400 being recoverable exactly once against a stale selector, `addListener` refusing a foreign kind in both stores, and `close` being final with `start` throwing afterwards. Separates `objectsRevoked`'s per-key counting from the per-tombstone `changes` it is computed from. The split-origin note is here because its absence is why the stream request went to the polling host: the fake endpoint serves both from one origin, so no test can catch that. Docs only; agents.md is not published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
XieX
added a commit
that referenced
this pull request
Sep 15, 2026
Ports two Agent Skills store-answer fixes from launchdarkly/python-ai-sdk (`split/skills-materialization`, launchdarkly/python-ai-sdk#53) into the Node client. ## What changed **A non-object listing is a broken store, not an empty one** (`packages/client/src/skills-core.ts`). `allRawObjects` used to collapse anything `store.allObjects()` returned that was not an object down to `{}` with no error. A store that served nothing usable was therefore indistinguishable from a store holding no skills, which the `'*'` reconcile read as "every skill was revoked" and pruned accordingly. It now returns `{ objects, error }`, logs an error naming the returned type (`the skill store listed skills as null rather than an object`), and puts that answer on the same footing as a store that threw. A throwing store is caught in the same place with the same wording (`storeThrew`), so `allSkills` and the reconcile's `resolveAll` no longer each re-derive the log line and the message. `allSkills` still returns `[]` on either failure; `writeSkills('*')` reports an incomplete run and prunes nothing. Mirrors Python's `list_raw_objects` returning `(objects, error)`. **An answer under a different key is withheld** (`resolveFromStore`). Identity is read off the object itself, so a store answering under a different key used to hand the caller a `Skill` carrying someone else's key. The reconcile then wrote that other key's path and, since prune keys off the request, deleted it in the same pass while reporting `ok`. After verification, `skill.key !== key` is now withheld with `skill '<key>' is not available: the store answered under key '<skill.key>'`, using the same shape as the existing version-mismatch branch. **Tests.** Accessor side from Python `5817d89` (aliased key withheld by `getSkill` / `getSkillResult`; non-object listing reported via `allRawObjects` with the type named) and reconcile side from `4c6d965` (a non-object listing leaves every managed file and manifest entry alone; an aliased answer writes nothing, is reported against the requested key, and never reaches the other key's file). All six fail against the unpatched source. Commits mirror Python `5817d89` and `4c6d965`. ## Notes for review - **Reason token for the key mismatch.** Python's `Resolution` has no typed outcome, so there is nothing to port there. Here it reuses `wrong_version`, per the existing branch's shape: the store held an answer, but not the one that was asked for. If a dedicated token is wanted it is a cross-language vocabulary change and belongs in its own PR. - **`e2b54fd` (an unheld version pin is a miss, not an integrity failure) is deliberately not ported.** Python's `InMemorySkillStore` files several versions per key plus a version-less slot, and that commit changes which one a pinned miss falls back to. Node's `InMemorySkillStore` holds exactly one object per key and ignores the version argument, letting `resolveFromStore` refuse a wrong-version answer afterwards. There is no fallback slot to fix, so the change has no direct equivalent; whether Node should grow multi-version semantics is a separate decision. - **#50 (`xie/skills-fdv2-transport`) sits above this branch and will need a rebase once this lands**, since `allRawObjects` changed its return type and `resolveAll` / `allSkills` changed accordingly. Not rebased here. ## Verification - `vitest run` in `packages/client`: 652 passed, 2 skipped - `tsc --noEmit`: clean - `biome check`: clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Hardens the Node client against **untrusted skill store** behavior (ported from the Python SDK): broken listings and key aliasing no longer look like “no skills” or succeed while corrupting disk state. > > **Non-object listings** — `allRawObjects` now returns `{ objects, error }` instead of silently `{}`. Throws and invalid list shapes (null, array, etc.) are logged with consistent wording via **`storeThrew`**, and **`writeSkills('*')`** treats a listing error as an **incomplete run** so **prune does not delete** managed files. **`allSkills`** still returns `[]` on failure but the error is available on the listing path. > > **Key aliasing** — After verification, **`resolveFromStore`** withholds answers where **`skill.key !==` the requested key**, with the same outcome shape as version mismatch (`wrong_version`). That blocks **`getSkill`** from returning the wrong identity and stops **`writeSkills`** from writing under one key and pruning under another in the same reconcile. > > **Tests** cover accessor and filesystem reconcile paths for both fixes. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ee5d469. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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 91f9a0b. Configure here.
| if (typeof raw.contentHash !== 'string') { | ||
| this.diagnostics.hashlessObjects += 1; | ||
| warnHashless(raw); | ||
| } |
There was a problem hiding this comment.
Hashless counter never tracks held set
Medium Severity
hashlessObjects increments on every hashless put-object, including uncommitted events and the same payload re-delivered on each poll. It never shrinks when those objects commit with a hash or leave the store. Callers are told a nonzero value means skills are being withheld, so after recovery — or under today's hashless-on-every-poll backend — the counter stays wrong while skillObjectsReceived is the field documented as cumulative.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 91f9a0b. Configure here.
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.
Stacked on #48 (
xie/skills-08-review-closeout), the tip of the Agent Skills stack. Review that first.The Node port of the Python SDK's FDv2 transport (launchdarkly/python-ai-sdk#69), behind the same
SkillStoreseam. Deliberately mechanical: same protocol boundary, same constants, same wire semantics, same diagnostics vocabulary. A change to one belongs in both.Almost nothing above the seam changed. Outside the two new modules and their tests, the diff is 4 export lines in
index.ts, two stale comments intypes.ts, and one real behavioural change inskills-fs.ts— a hardened key derivation inpendingForRaw(see below).Why this shape
The design's original Plan A — a bespoke poller against
GET /private/flagdlv/payloads/{payloadID}/latest/obj/skill/{key}— is dead. The security review found that route is a gonfalon private endpoint authenticated by Cognito machine-token OAuth scopes with no per-tenant authorization, and ruled out both relaxing its auth and shipping a machine credential to customer hosts (AZ-2). This usesGET /sdk/pollandGET /sdk/streamwith the environment's server-side SDK key instead — the review's own preferred resolution, and the channel payload signing will eventually cover.Design doc §2.3: https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/5215649997 · Backend ticket: AIC-2905
What's here
skills-fdv2.ts—FDv2SkillStore. Authenticates, streams (default) or polls, carries selector/basisacross requests, sendsIf-None-Matchand treats 304 as a first-class current answer, deserializesinline-resource/skillobjects, holds them by(key, objectVersion), and servesgetObject/allObjects/addListener. Capped jittered backoff,Retry-Afterhonoured, bounded retries. Platform globals only —fetch,AbortController,TextDecoder— so the content path adds no dependency.skills-watch.ts—watchSkills/SkillWatcher. The change-listener re-reconcile, pulled forward out of phase 4.Three things worth reviewing closely
1.
objectVersionis the skill's version;versionis the payload's. On the wire a skillput-objectcarries both, and confusing them fails silently — the object verifies, the hash matches, and the caller gets content under a version number that means nothing. Flags and segments carry onlyversionand omit bothcategoryandobjectVersion, which is exactly why the two look interchangeable. The translation happens in exactly one place (seamObjectFromPut) and theversion translationsuite asserts it in both directions, including end-to-end.2. Changes commit at
payload-transferred, not per object. A payload version is the unit of consistency. A half-applied full transfer would publish a state the server never described and would briefly empty the store — which, with pruning on, is the difference between a reconcile and deleting a customer's skill files. An interrupted transfer therefore leaves last known good intact, and listeners fire once per commit.3. One deliberate divergence from Python, and it needs a second opinion.
SkillObjectSet.snapshot()collapses to one object per key at its newest version.<root>/<key>/SKILL.mdis a single path, so a whole-store consumer must see one object per key or a'*'reconcile writes the same path twice andallSkillsreturns a list holding two versions of one skill. Python collapses innewest_by_key, above the seam; that helper has not been ported to this SDK yet, so the collapse happens in the adapter instead. End-to-end behaviour is identical andgetObjectstill resolves a pinned version out of the full set — which is the case the collapse must not break, and is tested. WhennewestByKeylands inskills-core.ts, move it and delete the note on the class.snapshot's keys turned out to be load-bearing in the same way, and for a less obvious reason:writeSkills('*')derives its prune keep-set from them, so a key it cannot parse as a skill key drops out of the keep-set and takes the copy already on disk with it. They are the bare skill key, never the wirekey:version— see the first review fix below.Security review deltas
delete-objectreaches a live stream in seconds, sowatchSkillsgets a revoked skill'sSKILL.mdoff disk within a debounce interval instead of at the next restart. Revocation by omission on a full transfer propagates the same way, which it did not before review fix 2 below.onUnavailable: 'keep'stays the default, as the review endorses: an outage must not read as "everything was revoked".The changes above the seam
Flagging these because they are above the seam. The two
types.tsones are comment-only with zero behavioural change:SkillStoredocblock described "the future real transport — a poller against the FDv2 delivery route". It is no longer future, and it streams by default.addListenerhad no consumer in this SDK.watchSkillsis now its consumer, and refuses loudly rather than degrading when a configured store lacks it.The
skills-fs.tsone is not comment-only, and is the one to look at:pendingForRawderived the skill key of an unverifiable object from the keyallObjectsserved it under. That is a pre-existing weakness in the seam contract — the seam never promised a store's map key spells a skill key — whichInMemorySkillStorehappened to satisfy and this PR's store is the first to violate. It now reads the object's ownkeyfirst and falls back to the map key, so no store's choice of map key can make an unverifiable object look revoked. Fixingsnapshotalone would have closed it forFDv2SkillStore; this closes it for any store.Shutdown behaviour
close()aborts theAbortSignalrather than only setting a flag — the delivery task spends its life awaiting a stream read, and a flag it never checks would leave a healthy stream running until the process exited. Every backoff timer and thewaitForSkillstimer areunrefed for the same reason: a background store must not be whynodestays up. Both are tested.Review fixes
Four defects found in review, all in
66a6369. Each was verified by reverting its fix and watching its test fail, so these are regression tests rather than assertions of current behaviour.1.
snapshotkeyedkey:version, which made a withheld skill look revoked. 🔴 The data-loss one.writeSkills('*')builds its prune keep-set from the keysallObjectsis keyed by, andpendingForRawcould not parsekey:versionas 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. ThecontentHashgap made this universal rather than an edge case: with no hash on the wire, every object fails verification, so a'*'reconcile against a real environment would have deleted everySKILL.mdit had previously written. Fixed on both sides —snapshotkeys by the bare skill key, andpendingForRawprefers the object's ownkey.2. A full transfer that dropped every skill notified nobody. A full transfer revokes by omission — whatever it did not carry is gone, and no
delete-objectsays so.payloadTransferrednow diffs the committed set against the pending one before the swap and reports the departures as tombstones. The partial case only ever self-healed by accident, when a surviving put happened to land inchanges; a wholly empty payload had nothing to piggyback on and left revoked files on disk until restart. Listeners that read versions now also see both halves of a version move, andobjectsRevokedcounts implicit revocations.3. A declined foreign payload still moved the delivery
basis. Throwing away a payload's contents while adopting its resume point asked the next poll or stream to resume from someone else's payload — skill updates could stop arriving while every diagnostic still read healthy. A declined payload no longer moves the basis.committed: truestays on that branch deliberately: the connection is healthy, sowaitForSkillsshould still release.4.
watchSkillsregistered its listener after the initial reconcile. A payload committing during that reconcile's filesystem I/O reached nobody. Note the naive fix trades this for a worse bug — the initial reconcile was not part of the watcher's chain, so simply movingaddListenerearlier lets a notify reconcile the same root concurrently, which loses manifest entries. The listener now goes on first and the initial reconcile runs through the chain (runInitial), which still propagates failures so a bad root throws out ofwatchSkills. A failed initial reconcile now also closes the watcher rather than leaving a listener on a root that never reconciled.Tests
158 tests in
skills-fdv2.test.ts, most of them againstFakeFDv2Endpoint, an in-processnode:httpserver implementing the wire contract — real sockets, so request construction and header handling are exercised rather than mocked. Covers skill put/delete,objectVersionvsversion, mixed payloads where flag and segment objects are skipped, unknown kinds ignored, 304,basisround-tripping, reconnect/backoff,Retry-After, bounded retries, the snapshot collapse and its keying, revocation by omission on a full transfer, a declined payload leaving the basis alone, a change committing during the initial reconcile, and a missing-contentHashenvelope producing withheld skills with the correct reason code rather than a crash — including one that asserts a hashless payload leaves the copy already on disk alone.Full suite 1337 passing across all packages (808 in
packages/client);tsc --noEmitandbiome checkclean.Blockers — none of these are in this PR's scope
contentHashis not on the wire. gonfalon AIC-2905 PR 6 is unmerged, so the envelope is still{contentType, content, name, description}. Verification withholds any skill without a hash, so against a real environment today every skill resolves to nothing. This PR makes that outcome loud rather than surviving it: an error per(key, version)namingmissing_content_hashand the ticket, one summary per wholly-hashless payload, and adiagnostics.hashlessObjectscounter. There is deliberately no fallback that skips verification — a hash the SDK computed from the content it was handed would certify the content against itself.inline-resourcebranch is unmerged and undeployed (kfreeman/AIC-2905/streamer-inline-resource-support). No account can receive skill objects at all until it ships.fdv2-protocol-controldefaults toforbid. A real environment returns 403 today. The store reports that as fatal and names the setting, but the path is only exercised against the fake.mvparameter is sent.mvselects the flag data model and the connection rejects any value but the flag default; the generic agent-skill payload is served regardless of it, and sendingmv=1gets the whole connection refused. The base SDK's own FDv2 data source does not send it either. This has been reasoned from the protocol rather than observed against a live server, so it is still worth confirming on first contact.ld-relaydoes not speak the FDv2 endpoints, so relay-only deployments cannot receive skills in Beta.Nothing here has touched a real LaunchDarkly environment, because it cannot yet. Everything is verified against the fake endpoint.
🤖 Generated with Claude Code
Note
Overview
Ships production skill delivery via
FDv2SkillStore, aSkillStoreimplementation that pulls agent skills over LaunchDarkly’s SDK-facing FDv2 channel (GET /sdk/poll/GET /sdk/stream, server-side SDK key). Docs no longer describe delivery as a follow-up; they document boot (start,waitForSkills,close), streaming vs polling, diagnostics, and beta caveats (403 without FDv2 opt-in, no relay, TLS-only signing).Adds
watchSkillsto run an initialwriteSkillsreconcile and debounced re-reconciles when the store commits skill changes, so revocations and updates can hit disk without a process restart.Tightens the store seam for watchers:
SkillStoregains optionalremoveListener;InMemorySkillStoreandFDv2SkillStorethrow onaddListenerfor kinds other than'skill'. The no-store error message now namesFDv2SkillStorebeforeInMemorySkillStore.Exports
FDv2SkillStore,watchSkills,SkillWatcher, URI defaults, and related types from the public barrel. Tests add a largeskills-fdv2.test.tssuite (fake HTTP endpoint, protocol reader, retries, hashless payloads, lifecycle) and extend in-memory store listener tests.Reviewed by Cursor Bugbot for commit 91f9a0b. Bugbot is set up for automated code reviews on this repo. Configure here.