Skip to content

feat(client)!: Agent Skills — take FDv2SkillStore time options in seconds - #121

Merged
XieX merged 13 commits into
xie/agent-skills-feature-ac9ac7from
xie/skills-time-options-seconds
Oct 9, 2026
Merged

XieX merged 13 commits into
xie/agent-skills-feature-ac9ac7from
xie/skills-time-options-seconds

Conversation

@XieX

@XieX XieX commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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


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.

Reviewed by Cursor Bugbot for commit 64da00d. Bugbot is set up for automated code reviews on this repo. Configure here.

XieX and others added 6 commits October 7, 2026 16:54
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>
…onds

Every FDv2SkillStore time option is now in seconds, like the Python SDK,
the base Node SDK, and writeSkills' timeout. Each option is renamed
rather than reinterpreted, so no existing name changes meaning:

- pollIntervalMs -> pollIntervalSeconds (default 30)
- readTimeoutMs -> readTimeoutSeconds (defaults 10 poll, 300 stream)
- initialBackoffMs -> initialBackoffSeconds (default 1)
- maxBackoffMs -> maxBackoffSeconds (default 30)
- waitForSkills(timeoutMs) -> waitForSkills({ timeoutSeconds }) (default 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, as the base SDK's
waitForInitialization({ timeoutSeconds }) does, because a positional
argument cannot be renamed: waitForSkills(10_000) would silently wait
almost three hours. A bare number rejects with a TypeError naming the
new form. The retired ...Ms option keys are not read.

Validation is unchanged: positive and finite (non-negative for the wait),
NaN and Infinity rejected, checked 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
floors a positive option at 1 ms. watchSkills' debounceMs stays in
milliseconds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@XieX

XieX commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Migration note: FDv2SkillStore time options are now in seconds (experimental)

Every time option on FDv2SkillStore is now in seconds. Divide your millisecond values by 1000 and rename the option:

// Before
const store = new FDv2SkillStore(sdkKey, {
  pollIntervalMs: 30_000,
  readTimeoutMs: 10_000,
  initialBackoffMs: 1_000,
  maxBackoffMs: 30_000,
}).start();
await store.waitForSkills(10_000);

// After
const store = new FDv2SkillStore(sdkKey, {
  pollIntervalSeconds: 30,
  readTimeoutSeconds: 10,
  initialBackoffSeconds: 1,
  maxBackoffSeconds: 30,
}).start();
await store.waitForSkills({ timeoutSeconds: 10 });
  • TypeScript flags the old option keys in an object literal, and flags waitForSkills(<number>).
  • JavaScript, or an options object TypeScript doesn't check: the old ...Ms keys are ignored, and the defaults (30 / 10 or 300 / 1 / 30 seconds) apply. Search for pollIntervalMs, readTimeoutMs, initialBackoffMs and maxBackoffMs.
  • waitForSkills(10_000) now rejects with a TypeError naming { timeoutSeconds }, so a missed call site fails loudly rather than waiting 1000× longer.
  • Fractional seconds work, for example { initialBackoffSeconds: 0.25 }. Values are rounded to the nearest millisecond, with a 1 ms floor for the options that must be positive.
  • Unchanged: watchSkills' debounceMs and SKILLS_DEFAULT_DEBOUNCE_MS stay in milliseconds, and writeSkills' timeout was already in seconds.
  • DEFAULT_POLL_TIMEOUT_MS and DEFAULT_STREAM_READ_TIMEOUT_MS are now DEFAULT_POLL_TIMEOUT_SECONDS (10) and DEFAULT_STREAM_READ_TIMEOUT_SECONDS (300). These are module-internal, and neither old nor new name was exported from @launchdarkly/ai-server/experimental.

Agent Skills is experimental, so this ships in a minor release with a changelog entry.

XieX and others added 6 commits October 8, 2026 12:07
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>
… HEAD

# Conflicts:
#	packages/client/etc/ai-server-experimental.api.md
…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 jeffdupont left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 627703ad. The later commits up to 9da14007 only merge in #120 (7978770b, fe8e5ec2), so this holds at the current head. The rename is clean, and every unit conversion goes through secondsToMs. I'd approve once the first two items below are settled. Both are small.

At 627703ad the client package has 1321 passed and 10 skipped. That's 3 more than the body's 1318, probably #120's tests from the merge. typecheck, api:check, code:check and build are clean, and CI is green. The head contains #120's head (4f2375fa) and main (993225f).

Each change, reverted, fails a test:

  • Math.round → Math.trunc: 2 tests fail.
  • Dropping the 1 ms floor: 1 fails.
  • Treating each option as milliseconds again at its use site:
    • readTimeoutSeconds: 2 fail
    • initialBackoffSeconds: 6 fail
    • maxBackoffSeconds: 6 fail
    • pollIntervalSeconds: 1 fails
  • Removing waitForSkills' type rejection: 3 fail.
  • A default of 10_000: 1 fails.

secondsToMs stays off the entry point (experimental.ts:27-28). Nothing stale is left in the repo: README :386 and agents.md use the new names, and the remaining ...Ms hits are local variables or migration text.

  1. A retired ...Ms key is ignored with no log. I probed it from JS: { mode: 'poll', pollIntervalMs: 5000 } logs nothing and polls every 30 s. The worse case is maxBackoffMs: 300_000, which quietly becomes a 30 s cap. TypeScript catches this in an object literal, but not for a JS caller, an options object built from config or JSON, or a spread from a wider type. Since this PR is where the rename happens, a one-time warning naming the new key for the four retired names would make the migration loud for everyone. TESTING.md already uses that rule for initClient ("Such a key is ignored, but never silently", :1116). Inline.
  2. A value over 2^31−1 ms (about 24.8 days) overflows setTimeout, and in poll mode that makes the store spin. pollIntervalSeconds: 3e6 passes validation, and Node fires the timer after 1 ms with a TimeoutOverflowWarning. I probed it: 523 polls in 600 ms. Backoff (initialBackoffSeconds = maxBackoffSeconds = 3e6) reconnects until a jittered delay fits. waitForSkills({ timeoutSeconds: 3e6 }) resolves false after 1 ms. This isn't new: pollIntervalMs: 3e9 at 4f2375fa gives 516 polls. But this PR adds the one place every value becomes milliseconds, so clamping there fixes all four at once. Inline.
  3. Nit: the TypeError message reads "got ." for [] (String([]) is empty) and "got 10" for '10'. Inline.

Follow-ups outside this repo:

  • Public docs (ld-docs-private, mintlify/sdk/features/agent-skills.mdx): :287 calls store.waitForSkills(10_000), which now rejects with a TypeError. The options table at :442-460 lists pollIntervalMs, initialBackoffMs / maxBackoffMs and readTimeoutMs. This could go in the docs follow-up already agreed on py #148, timed with this release.
  • #48: A.12's new row matches these names. But §3.25's backoff bullet (TESTING.md :1988), which your body flags, still says initialBackoffMs / maxBackoffMs on #48's head. I've raised it there.

FYI, not for this PR: the "seconds everywhere" consistency argument stops at the core API. waitForTelemetry(timeoutMs = 5000) (lifecycle.ts:182, etc/ai-server.api.md:661) is still milliseconds on the non-experimental surface. Worth a decision before 1.0 freezes it.

this.pollIntervalMs = options.pollIntervalMs ?? 30_000;
// Every time option is in seconds, and stays in seconds until `secondsToMs`
// at the timer or deadline it sets.
this.pollIntervalSeconds = options.pollIntervalSeconds ?? 30;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From JS, a retired key is dropped silently: { mode: 'poll', pollIntervalMs: 5000 } logs nothing and polls every 30 s (probed). maxBackoffMs: 300_000 becomes a 30 s cap the same way. Could you warn once here when any of pollIntervalMs, readTimeoutMs, initialBackoffMs, maxBackoffMs is present, naming the replacement and the unit ("pollIntervalMs is no longer read; use pollIntervalSeconds (seconds)")? It's a few lines, and it follows TESTING.md's "ignored, but never silently" rule for option keys (:1116). Throwing would also be defensible while this is experimental.

Comment thread packages/client/src/skills-fdv2.ts Outdated
* a zero delay.
*/
export function secondsToMs(seconds: number, minimum = 0): number {
return Math.max(minimum, Math.round(seconds * 1000));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing caps this at setTimeout's 2^31−1 ms (about 24.8 days). Node fires an overflowing timer after 1 ms. pollIntervalSeconds: 3e6 passes validation and polls 523 times in 600 ms (probed). The backoff options and waitForSkills({ timeoutSeconds: 3e6 }) overflow the same way. It's pre-existing (pollIntervalMs: 3e9 at 4f2375fa does the same), but this is now the one conversion point, so Math.min(2 ** 31 - 1, ...) here, or a constructor check that rejects it, would fix all four. One test with a huge pollIntervalSeconds and a fake timer would pin it.

Comment thread packages/client/src/skills-fdv2.ts Outdated
return Promise.reject(
new TypeError(
`waitForSkills takes an options object, { timeoutSeconds }, got ${
typeof options === 'number' ? `the number ${options}` : String(options)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: String([]) is empty, so waitForSkills([]) says "got ." and waitForSkills('10') says "got 10", which hides that it was a string. JSON.stringify(options), or typeof plus the value, would read better.

Base automatically changed from xie/skills-review-must-fix to xie/agent-skills-feature-ac9ac7 October 8, 2026 21:14
…ptions-seconds

Keep the seconds time options where the branches diverged. Warn once when a retired millisecond option is still passed, cap secondsToMs at setTimeout's limit, and show the rejected waitForSkills value with its type.

Co-authored-by: Cursor <cursoragent@cursor.com>
@andrewklatzke

Copy link
Copy Markdown
Contributor

Pushed 64da00d. Merged xie/agent-skills-feature-ac9ac7 and addressed the review on 9da1400.

Conflicts. The base had moved waitForSkills call sites back to a millisecond argument, and the api-extractor warning line had drifted. Kept the seconds API (secondsToMs(timeoutSeconds), waitForSkills({ timeoutSeconds })) and regenerated the forgotten-export line in etc/ai-server-experimental.api.md (454).

Review

  1. A retired millisecond key is no longer silent. If pollIntervalMs, readTimeoutMs, initialBackoffMs, or maxBackoffMs is present, one warning names each key and its ...Seconds replacement (pollIntervalMs is no longer read; use pollIntervalSeconds (seconds)). The keys are still not read.
  2. secondsToMs caps at 2^31 − 1, so a pollIntervalSeconds past ~24.8 days cannot overflow setTimeout into a 1ms poll loop. Backoff and waitForSkills go through the same function.
  3. waitForSkills's TypeError uses JSON.stringify for a string, null, or array, and typeof plus the value otherwise. [] reports got []; '10' reports got "10".

Client tests: 1342 passed, 10 skipped. api:check is clean.

@XieX
XieX requested a review from jeffdupont October 9, 2026 15:34
@XieX

XieX commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

FYI, not for this PR: the "seconds everywhere" consistency argument stops at the core API.

Yeah, this is a great point. Sometime between now and GA we may want to have a chat about the "JS should be in seconds because Python" decision, I'm feeling less confident about it now that I see so many inconsistencies within the TS SDK. Maybe it's OK for the TS SDK to be in ms as a convention of that community. Not for this PR though, agreed. Just a conversation we should have at some point.

@XieX
XieX merged commit 90ead9d into xie/agent-skills-feature-ac9ac7 Oct 9, 2026
10 checks passed
@XieX
XieX deleted the xie/skills-time-options-seconds branch October 9, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants