Repository navigation
fix(skills): pre-release review must-fix findings - #120
Conversation
The waitForSkills timer was unreffed like the backoff and deadline timers. When LaunchDarkly is unreachable those are the only other handles, and a refused connection leaves no socket open, so node exited mid-await: exit 13 under ESM top-level await, and exit 0 in CommonJS without running the line after the await. 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. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
iterSse warned and skipped an event whose data was not JSON, and the payload-transferred that followed committed the transfer without it and advanced the basis. A corrupted delete-object therefore left a revoked skill served indefinitely, since the reconnect asked only for changes after the advanced basis; a corrupted put-object inside xfer-full revoked that skill by omission. The event now raises RecoverableTransportError. The connection ends, the in-flight payload is abandoned, and the reconnect carries the last committed basis. This matches js-sdk-common's PayloadStreamReader, which reports malformed data as InvalidData and stops the stream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A goodbye with catastrophe: true returned a fatal outcome, so delivery, revocations included, stopped until start() was called again. Neither js-sdk-common's protocol handler nor StreamingProcessorFDv2 reads the flag, and the streamer sets it on its own connection-error path. It is now a recoverable disconnect: counted in connectionFailures even after a completed exchange (never expected), recorded in lastError, and logged at error level whatever silent says. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
skillRefs returned [] for null or undefined, and inspectConfig answers config: null on any failure. The obvious pipeline writeSkills(skillRefs(info.config), root) then ran with its default prune: true and deleted every managed skill during an outage. A config that is not an object, null and undefined included, now throws TypeError, as a malformed skills field already does. An object with no skills field still returns []. The README's pipeline example and API table say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rimental stage The README promised that ld.skills.integrity_failure would not change outside a major release. Agent Skills is experimental, and experimental names may change in a minor release with a changelog entry under Experimental. The note now says that, and that the major-release rule applies once the feature is core. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jeffdupont
left a comment
There was a problem hiding this comment.
All four fixes match py #148, and the catastrophe and skillRefs changes look right. At fa44c1a9 I get 1306 passed and 10 skipped, and typecheck and code:check are clean. Two things should be fixed before this merges. Both are the same as on py #148, so the twins can change together. Details are inline.
- A
heart-beator unknown event with data that is not JSON now ends the connection. I ran it: the probe is below. - Nothing tests a non-JSON
put-objectinside anxfer-full. The PR body andagents.mdboth claim that case.
Smaller:
- The PR body is stale. It says "Python's
skill_refs(None)still returns[]". py #148 (77023bb8) makes it raise, so the two SDKs agree now. - The error class differs. JS throws
TypeErrorand Python raisesValueError, with the same rule otherwise. Could #48 record which each SDK raises, the way A.12 does for the malformed-skillserror? - A delete that is valid JSON but the wrong shape still commits the rest of its transfer. An example is a
delete-objectwith no usablekey, which:444warns about and ignores. So the revoked skill stays served. I checked this in Python (6 shapes) and only read the JS code, so treat the JS half as unverified.ldclientinterrupts on most of these shapes. This is worth a decision in #48 rather than a change here.
A heart-beat or an unknown event whose data is not JSON no longer ends the stream; iterSse yields it with null data, since unknown events are ignored by contract and js-sdk-common's PayloadStreamReader attaches handlers to the six known events only. The SSE framing tests that used heart-beat data now use put-object. Adds a store-level test for a truncated put-object inside an xfer-full, which would otherwise revoke that skill by omission. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@jeffdupont, thanks. Both fixes are in at
|
jeffdupont
left a comment
There was a problem hiding this comment.
Re-reviewed at 4f2375fa. Both fixes are in, and every claim in your replies checks out. One small test gap (below) should be closed before merge, and then I'd approve.
At 4f2375fa the client package has 1309 passed and 10 skipped. typecheck, code:check and api:check are clean, and CI is green. Trial merges onto xie/agent-skills-feature-ac9ac7 (7fc790a) and onto main (993225f) are clean.
- Parse only known events:
EVENTS_WITH_DATA(skills-fdv2.ts:83-90) is exactly the sixhandlecases that read data (:682-701). Data flows only fromstreamOncetoapplytohandle, sonulldata forheart-beatand unknown events is safe. When I restored parse-every-event, both new cases failed. - js-sdk-common:
PayloadStreamReaderin 2.25.1 attaches handlers to the same six events (dist/cjs/index.cjs:3325-3330) and reportsInvalidDataon a failedJSON.parse. So the parity claim in #48 holds. xfer-fulltest: if a non-JSONput-objectis skipped instead of dropping the connection, only the new test fails (1 failed, 1308 passed).- Framing tests moved to
put-object: they go throughframed(), which runs only the event parser, so the event name has no side effects. No coverage lost. - PR body and README: both up to date.
README.md:332saysskillRefsthrowsTypeErrorfornull/undefined.
Fix before merge: nothing tests that a goodbye reaches handle with its data parsed. I removed EVENT_GOODBYE from EVENTS_WITH_DATA, and all 1309 tests still pass. A goodbye would then arrive as null, catastrophe: true would never be read (:870-889), and a catastrophic goodbye would become an uncounted recycle. That undoes c71345e. Both catastrophe tests skip the stream parser: one calls handle directly (skills-fdv2.test.ts:855), and the store test at :2350 gets already-parsed events from UnchangingRequester. EVENT_ERROR has the same gap, but there only the logged reason is lost. Inline. py #148 has the same gap.
…ye through SSE Each of the six events the reader reads is asserted to arrive parsed, and a store-level test sends a catastrophic goodbye through iterSse after a commit. Dropping goodbye from EVENTS_WITH_DATA, which would read a catastrophe as an uncounted recycle, now fails both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pin the keyless-object rule Empty data on one of the six events the reader reads now ends the connection, as js-sdk-common reports a known event with no data as an error. Read as null, a delete-object was ignored and a payload-transferred committed with no selector. 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). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@jeffdupont, thanks. The goodbye gap is closed in At |
Fixes the must-fix-before-merge findings from the pre-release review of Agent Skills (#87, reviewed at `0306c69`). One commit per fix. Each code fix has a test that fails on `0306c69` and passes here. ## 1. A stream event whose data isn't JSON no longer commits the rest of its transfer `_iter_sse` used to log the event and skip it. The `payload-transferred` that followed then committed the transfer without the event and advanced `basis`. A corrupted `delete-object` left a revoked skill served indefinitely, because the reconnect asked only for changes after a basis that already covered the revocation. A corrupted `put-object` in an `xfer-full` revoked that skill by omission. It now raises `_RecoverableTransportError`, matching `ldclient`'s FDv2 stream, which interrupts on `JSONDecodeError`. The in-flight payload is dropped and the reconnect sends the last committed basis. The drop is logged at WARNING in `_iter_sse`, because `_run` logs a disconnect after a completed exchange at debug, and a revocation usually arrives after a commit on the same connection. Only the six events `handle` reads are parsed (`server-intent`, `put-object`, `delete-object`, `payload-transferred`, `goodbye`, `error`). A `heart-beat` or an unknown event is yielded with `None` data, so its data can't drop the connection. `ldclient` likewise calls `json.loads` only inside its known-event branches. Tests: `test_a_malformed_event_abandons_its_transfer_and_keeps_the_basis`, parametrized over two cases: - `delete-object-in-xfer-changes` commits `a:1` and `revoked:1`, then sends an `xfer-changes` containing a clean `put fresh:1` and a truncated `delete revoked:1`, then `payload-transferred`. At the reconnect, nothing from that transfer is committed: `revoked` is still held and `fresh` is absent. - `put-object-in-xfer-full` commits `a:1`, then sends an `xfer-full` with a clean `put b:1` and a truncated `put a:1`. At the reconnect, `a` is still held and `b` is absent. In both cases the reconnect carries `basis-1`, a clean retransmission then applies in full, and a WARNING names the event. `test_an_event_handle_does_not_read_is_not_parsed` covers `heart-beat` and `x-future-event` with `data: not json`. `test_an_event_handle_reads_is_parsed` pins the parse set (all six), and the store-level catastrophe test goes through `_iter_sse`. Empty data on a known event is not JSON either (`bf559fb`), as `json.loads("")` raises in `ldclient`. Read as `None`, a `delete-object` was ignored and a `payload-transferred` committed with no selector. The malformed-event test has a third case, `empty-delete-object-in-xfer-changes`, and `test_an_event_handle_reads_with_no_data_drops_the_connection` covers all six names. Two 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). ## 2. A `goodbye` with `catastrophe: true` no longer stops delivery It was raised as fatal, so delivery stopped until something called `start()`. Python `ldclient` 9.16 never reads `catastrophe`, and the Go SDK only logs it. It is now a recoverable disconnect, logged at ERROR. It is not marked `recycled`, so it counts toward `connection_failures` even after a completed exchange. `_TransferOutcome.fatal` had no other producer, so it is removed. Tests: `test_a_catastrophic_goodbye_is_fatal` is flipped to `test_a_catastrophic_goodbye_is_a_counted_disconnect`. New: `test_a_catastrophic_goodbye_reconnects_and_is_counted`. No docstring or README said the goodbye was fatal; only the code and that test did. ## 3. `skill_refs` raises on a config that is not a dict `skill_refs(None)` returned `[]`, and `None` is the config a failed `inspect_config` returns. So `write_skills(skill_refs(info["config"]), root)` pruned every managed skill during an outage. A non-dict config now raises `ValueError`, as a malformed `skills` field already does. A dict without a `skills` key still returns `[]`. The docstring, the README (quick-start comment, validation paragraph, API table), and `agents.md` are updated. Test: `test_a_config_that_is_not_a_dict_raises`, parametrized over `None`, a list, and a string. ## 4. Docs: the `ld.skills.integrity_failure` commitment The README said the event "will not be renamed". It now says that during the experimental stage the name, fields, and `reason_code` values may change in a minor release, with a changelog entry under **Experimental** (TESTING.md §0.1's wording, and js-ai-sdk's). `agents.md` is updated to match. Docs only, so there is no test. ## Checks `uv run pytest` (repo root): 2321 passed, 11 skipped. `make lint`, `make format-check`, and `make typecheck` are clean. Spec: launchdarkly/ai-sdks-monorepo#48. JS counterparts: launchdarkly/js-ai-sdk#120. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Addresses pre-release review must-fixes for **Agent Skills** in the Python client: safer reference discovery, FDv2 stream consistency, and doc alignment. > > **`skill_refs`** now raises `ValueError` when the config is not a dict (including `None` from a failed `inspect_config`), instead of returning `[]`. That stops `write_skills(skill_refs(...), root)` from pruning every managed skill during a config outage. A dict with no `skills` field still returns `[]`. > > **FDv2 streaming** treats non-JSON data on the six events the protocol reader consumes as a recoverable disconnect (`_RecoverableTransportError`), matching base `ldclient` behavior and abandoning the in-flight payload so a later `payload-transferred` cannot commit a partial transfer and advance `basis`. `heart-beat` and unknown events are not parsed. **`goodbye` with `catastrophe: true`** reconnects with backoff (ERROR log, counted failure) instead of stopping delivery fatally; `_TransferOutcome.fatal` is removed. > > **Docs** (`README.md`, `agents.md`) describe the `skill_refs` contract, stream/keyless-object behavior, and soften the `ld.skills.integrity_failure` stability wording for the experimental stage (minor-release changes with changelog). Tests cover malformed SSE, catastrophic goodbye, non-dict configs, and keyless put/delete experimental rules. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit bf559fb. 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>
93ded06
into
xie/agent-skills-feature-ac9ac7
…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>
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
await store.waitForSkills()was pending (ca79d2f). The wait'ssetTimeoutwas unreffed. When LaunchDarkly is unreachable, the backoff and deadline timers (also unreffed) are the only other handles, sonodeexited 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
tsxagainst the source: ESM top-level await, a CommonJS async function, and a never-started store. Each asserts exit 0 and that the wait resolvesfalseat the timeout.datais not JSON was dropped, and the transfer still committed (88e21c1). A corrupteddelete-objectkept a revoked skill served, because the reconnect sent the advancedbasis. A corruptedput-objectinsidexfer-fullrevoked that skill by omission.iterSsenow throwsRecoverableTransportError, so the in-flight payload is abandoned and the reconnect resumes from the last committed basis. This matches js-sdk-common'sPayloadStreamReader, which reportsInvalidData;StreamingProcessorFDv2then stops the source. Only the six events the reader acts on are parsed (server-intent,put-object,delete-object,payload-transferred,goodbye,error). Aheart-beator an unknown event is yielded withnulldata, so its data can't end the connection (4f2375f).Tests: commit
{a:1, b:1}, then sendxfer-changeswithput c:1, a corruptdelete-object a:1, andpayload-transferred. The test asserts that the reconnect carriesbasis-1, that listeners seec:1once (from the clean retransmission), and that the retransmission revokesa. A second test commitsa:1, then sends anxfer-fullwith a cleanput b:1and a corruptput a:1. It asserts the reconnect carriesbasis-1,ais never revoked, andbreaches listeners once.iterSseframing tests cover the corruptdelete-object, plus aheart-beatand an unknown event withdata: not json. Framing tests also pin the parse set (all six), and a store-level test streams a catastrophic goodbye throughiterSse(7978770). Empty data on a known event is not JSON either (fe8e5ec). Read asnull, adelete-objectwas ignored and apayload-transferredcommitted with no selector. Two protocol-reader tests pin the experimental-stage rule for a put or delete with no usablekey: it is warned and ignored, and its transfer commits (TESTING.md §3.25, ai-sdks-monorepo#48).goodbyewithcatastrophe: truewas fatal untilstart()(c71345e). Neither base JS package readscatastrophe, 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 (neverexpected), recorded inlastError, and logged at error level whateversilentsays.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,
failedstaysnull, and the goodbye is logged at error level.skillRefs(null)returned[](9466bd8).inspectConfiganswersconfig: nullon any failure, sowriteSkills(skillRefs(info.config), root)pruned every managed skill during an outage. A config that is not an object,nullandundefinedincluded, now throwsTypeError. An object with noskillsfield 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
inspectConfigwhoseconfigthrows.fa44c1a). The README promised thatld.skills.integrity_failurewould 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
skill_refs(None)now raisesValueError(JS throwsTypeError), so the two SDKs agree apart from the error class, which A.12 records.Checks run locally
yarn build,yarn test(client: 1325 passed, 10 skipped),yarn typecheck,yarn code:check, andyarn workspace @launchdarkly/ai-server api:check. The only API-report changes are source line numbers in a comment.🤖 Generated with Claude Code
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.
skillRefsno longer treats a failedinspectConfig(config: null) as “no skills” — non-object configs (includingnull/undefined) now throwTypeErrorsowriteSkills(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 withRecoverableTransportErrorwhen a known protocol event’sdatais missing or not JSON (instead of skipping it and still committingpayload-transferred). That abandons the in-flight transfer and reconnects from the last committedbasis, fixing cases where a baddelete-objectleft revoked skills served or a badput-objectinxfer-fullrevoked by omission. Only the six reader-handled events are JSON-parsed; heartbeats/unknown events staynulldata.Catastrophic
goodbyeis no longer fatal (delivery keeps retrying); it is always counted inconnectionFailures, setslastError, and logs at error level even whensilent.waitForSkillskeeps its timeout timer referenced (other store timers stay unreffed) so Node does not exit mid-awaitwhen delivery is down and nothing else holds the event loop.Docs clarify that
ld.skills.integrity_failuremay change in minor releases while Agent Skills is experimental.Reviewed by Cursor Bugbot for commit fe8e5ec. Bugbot is set up for automated code reviews on this repo. Configure here.