Repository navigation
feat(client)!: Agent Skills — take FDv2SkillStore time options in seconds - #121
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>
…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>
Migration note:
|
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
left a comment
There was a problem hiding this comment.
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 failinitialBackoffSeconds: 6 failmaxBackoffSeconds: 6 failpollIntervalSeconds: 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.
- A retired
...Mskey is ignored with no log. I probed it from JS:{ mode: 'poll', pollIntervalMs: 5000 }logs nothing and polls every 30 s. The worse case ismaxBackoffMs: 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 forinitClient("Such a key is ignored, but never silently", :1116). Inline. - A value over 2^31−1 ms (about 24.8 days) overflows
setTimeout, and in poll mode that makes the store spin.pollIntervalSeconds: 3e6passes validation, and Node fires the timer after 1 ms with aTimeoutOverflowWarning. I probed it: 523 polls in 600 ms. Backoff (initialBackoffSeconds=maxBackoffSeconds=3e6) reconnects until a jittered delay fits.waitForSkills({ timeoutSeconds: 3e6 })resolvesfalseafter 1 ms. This isn't new:pollIntervalMs: 3e9at4f2375fagives 516 polls. But this PR adds the one place every value becomes milliseconds, so clamping there fixes all four at once. Inline. - Nit: the
TypeErrormessage 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 callsstore.waitForSkills(10_000), which now rejects with aTypeError. The options table at :442-460 listspollIntervalMs,initialBackoffMs/maxBackoffMsandreadTimeoutMs. 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/maxBackoffMson #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; |
There was a problem hiding this comment.
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.
| * a zero delay. | ||
| */ | ||
| export function secondsToMs(seconds: number, minimum = 0): number { | ||
| return Math.max(minimum, Math.round(seconds * 1000)); |
There was a problem hiding this comment.
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.
| return Promise.reject( | ||
| new TypeError( | ||
| `waitForSkills takes an options object, { timeoutSeconds }, got ${ | ||
| typeof options === 'number' ? `the number ${options}` : String(options) |
There was a problem hiding this comment.
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.
…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>
|
Pushed Conflicts. The base had moved Review
Client tests: 1342 passed, 10 skipped. |
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. |
Stacked on #120. This moves every
FDv2SkillStoretime option to seconds, for consistency with the Python SDK, the base Node SDK (pollInterval,timeout,waitForInitialization({ timeoutSeconds })) andwriteSkills' existingtimeout.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:
pollIntervalMspollIntervalSeconds30readTimeoutMsreadTimeoutSeconds10poll /300streaminitialBackoffMsinitialBackoffSeconds1maxBackoffMsmaxBackoffSeconds30waitForSkills(timeoutMs)waitForSkills({ timeoutSeconds })10DEFAULT_POLL_TIMEOUT_MS,DEFAULT_STREAM_READ_TIMEOUT_MSDEFAULT_POLL_TIMEOUT_SECONDS,DEFAULT_STREAM_READ_TIMEOUT_SECONDSwaitForSkillstakes an options object because a positional argument can't be renamed:waitForSkills(10_000)would silently wait almost three hours. A bare number,nullor a string rejects with aTypeErrorthat names the new form....Mskeys are not read. TypeScript rejects them in an object literal; from JavaScript they are ignored and the default applies.watchSkills'debounceMsstays in milliseconds, along withSKILLS_DEFAULT_DEBOUNCE_MS. It is now the one millisecond option in the skills API.NaNandInfinityare rejected, and the checks run in seconds.secondsToMs, at thesetTimeoutor read deadline they set. It rounds rather than truncates:1.005 * 1000is1004.9999999999999, and the base SDK'sMath.truncwould give 1004. A positive option is floored at 1 ms, so a sub-millisecond value never becomes a zero delay.Tests
waitForSkillsrejecting a bare number,nulland a string, the 10-second default, a timeout of zero, positiveInfinity, and thesecondsToMstable.pollIntervalSeconds: 1.005sleeps 1005 ms. SwappingMath.roundforMath.truncturns both precision tests red.yarn build,yarn test(client: 1318 passed, 10 skipped),yarn typecheck,yarn code:checkandapi:checkall pass.etc/ai-server-experimental.api.mdis updated.Spec text that now needs updating (ai-sdks-monorepo
TESTING.md; not edited here)initial_backoff/initialBackoffMsandmax_backoff/maxBackoffMsraise in the constructor…". These should becomeinitialBackoffSeconds/maxBackoffSeconds.waitForSkills' shape. Python usespoll_interval,read_timeout,initial_backoff,max_backoffandwait_for_skills(timeout), while TypeScript now uses the...Secondsnames andwaitForSkills({ 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 fordebounceMs("a baredebouncenumber in a language without keyword arguments does not say what unit it is in").🤖 Generated with Claude Code
Note
Overview
Breaking change (experimental):
FDv2SkillStoretiming is expressed in seconds and aligned with the base Node/Python SDKs andwriteSkills'timeout. Millisecond option names are renamed, not reinterpreted (pollIntervalSeconds,readTimeoutSeconds,initialBackoffSeconds,maxBackoffSeconds; defaults stay 30s / 10s poll / 300s stream / 1s / 30s).waitForSkillsnow takes{ timeoutSeconds }(default 10) instead of a positional millisecond argument. A bare number,null, or string throwsTypeErrorso old calls likewaitForSkills(10_000)cannot silently wait ~3 hours. Legacy...Msconstructor keys are ignored with a one-time warning.Implementation adds
secondsToMs(round, 1 ms floor for positive delays, cap at2^31−1forsetTimeout) used only at timer/deadline sites.watchSkillsdebounceMsis unchanged — the only ms-based skills API surface.Docs (
README,agents.md), public API report, tests, and an error hint inwriteSkillswhen 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.