Repository navigation
Conversation
…Source
A watcher given a fixed reference list never re-reads the config, so a
config moving a skill from v2 to v3 leaves v2 on disk reported `error` and
never writes v3. A SkillSource is a requested set read on every reconcile,
plus the config keys whose changes trigger one.
- SkillSource / createSkillSource({ refs, configKeys }): frozen; a
non-function refs or a bare-string configKeys throws TypeError.
- configSkills(key, context): evaluates variation(key, context, null) and
classifies the value. Every failed evaluation throws, never `[]`; it never
initializes the client, and reports experimental.configSkills when called.
- writeSkills reads a source once, bounded by `timeout`. A throwing,
rejecting or malformed answer is an unavailable retrieval.
- watchSkills subscribes to `update:<key>` per config key before the initial
reconcile, shares the store's debounce, throws when no client can report
changes, and detaches with `off` on close.
- LDClientInterface gains optional on / off / isOffline.
All new names are on the experimental entry point only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jeffdupont
left a comment
There was a problem hiding this comment.
Review comment (not an approval). Checked against spec launchdarkly/ai-sdks-monorepo#37 (TESTING.md on xie/watch-skills-follow-config), python-ai-sdk#211, and the skills-to-core PRs (#159 here, py#210, spec #86). No review threads yet.
1. CI: two red checks
- Type check / "Check API reports" fails on
packages/claude-agents/: an API report is out of date(run 40001523799, job 120579668287). That is not the@launchdarkly/ai-serverreport this PR regenerated. The branch base isf7883ce, and main's CI is green atf2404d9. Fix: rebase on main, runyarn api:update, and commitpackages/claude-agents/etc/ai-claude-agents.api.mdif it still differs. I did not reproduce which declaration changed. - API compatibility flags
LDClientInterface(the new optionalon/off/isOffline). I agree these are compatible for the four server SDKs and for structural implementers. Fix: an owner appliesapi-compatible. Do not add!.
2. absent does not hold the prune: confirmed in both SDKs. Fix the spec, not the reconcile
- js main:
skills-fs.ts:413-415(:424on this branch) marks a run incomplete only for an unavailable retrieval. Anabsentreference stays requested aserror, and the dropped key is still pruned. Python main does the same (skills_fs.py_resolve_requests, "Anabsentreference does not set it"), and py#211 pins it withtest_an_absent_reference_does_not_hold_the_prune. - Spec #37's "Dropping a reference" bullet says an
absentsecond reference keeps the dropped skill. That contradicts §3.22's per-key bullet ("protected by its key, not by suppressing the prune … the narrowness is the point"). - Fix: before this leaves draft, get #37 to say "unavailable" in place of "
absent". Also get §3.22's "a reference that did not resolve" reworded to "a reference whose retrieval was unavailable". Your test atskills-source.test.ts("prunes it on the next complete reconcile, and not while another reference is unavailable") already matches that wording. py#211 lacks this unavailable-half test, and I've asked for it there.
3. Debounce parity: JS matches the spec, Python does not
JS restarts the timer on every notify(), so a mixed burst gives one reconcile and one read, as #37 asks. py#211 asserts two (its watcher clears the wake flag before the window). This needs no change here. I flagged it on py#211 so the two SDKs stop disagreeing.
4. Small parity items to settle in A.12
- Empty config key. Here it is
TypeError(types.tscreateSkillSource, andconfigSkills('')). Python raisesValueError. Fix: pick one class in both SDKs, or record the split in #37's A.12 error table. isOffline()that throws is treated as offline here (skills-watch.ts:218-223). Python lets the raw exception escape. I asked Python to match this behavior. Get #37 to state the rule so neither side drifts again.
5. Interaction with #159 (Agent Skills to core) and spec #86
#159 marks every skills name - in api-surface/ai-server.experimental.txt, moves the names to the root, and deletes the skills rows in changelog-sections. Spec #86 renames the skills usage IDs to client.*. Spec #86 lists seven IDs, and #37 lists eight under experimental.*, so the two spec PRs conflict too. If #159 merges first, this PR needs to:
- export
configSkills,createSkillSourceandSkillSourcefromsrc/index.ts, notsrc/experimental.ts, with the root list lines inapi-surface/ai-server.txt. Example:+ configSkills usage=client.configSkills. - change
reportUsage('experimental.configSkills')(skills-source.ts, first line ofconfigSkills) and thesdk-usage.test.tsrow toclient.configSkills; - add the three names to the root export lists that #159 extends:
integration-tests/commonjs-webpack/fixture/entrypoints.{cjs,mjs},types.ts, andpackages/ai-node/src/__tests__/reexports.test.ts; - move
skills.test.ts'sEXPERIMENTAL_SKILLS_*additions to the root lists; - retitle away from
feat(skills):. After #159 that scope no longer maps to Experimental, so use a core scope such asfeat(client):.
Spec #37 has to be rebased over #86 in the same way, or the next list sync will disagree with this PR.
Parity checked with no gap found
configSkills classification and null default; no initClient; usage reported on the call only, with skillRefsInternal used for the reads; a source read exactly once per run; an invalid entry fails the whole read; timeout: 0 never calls refs; a late rejection is handled (beforeDeadline races both promises); update:<key> listeners attached once per distinct key, before the initial reconcile; the listener only signals; no-signal raises before anything is attached; close() detaches with off or removeListener; '*' and lists never probe; version bump in both orders with no removed; a real-SDK test through TestData; the daemon-mode README statement.
Not checked: I did not run the suite locally, and I relied on CI and the diff.
…able references Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Why
A config-scoped skill watcher today is given a fixed list, usually
skillRefs(inspectConfig(...).config), so it never re-reads the config. When an AI Config movespdf-extractionfrom v2 to v3, v2 stays on disk reportederroron every run, and v3 is never written. When a config stops referencing a skill, the skill stays on disk.This adds a
SkillSource: a requested set that is read when it is needed, plus the AI Config keys whose changes should trigger a re-read.watchSkillssubscribes to the base client'supdate:<key>events for those keys.What changed
All new names are on
@launchdarkly/ai-server/experimentalonly (and so on@launchdarkly/ai-node/experimentalthrough itsexport *). None is on the package root.SkillSourceis a readonly interface{ refs, configKeys }.createSkillSource({ refs, configKeys })builds a frozen one with a frozen copy ofconfigKeys. A non-functionrefs, a bare-stringconfigKeys, or aconfigKeysholding anything but non-empty strings throwsTypeError.configSkills(key, context)returns the source for one AI Config. Each read evaluatesvariation(key, context, null)on the configured client and classifies the value:initClient),variationthrows, the default or any non-object, an object with no boolean_ldMeta.enabled, or an enabled config that fails to parse or has a malformedskills.[]: a disabled variation, or an enabled config with noskills.nullon purpose. The{ enabled: false }default thatinspectConfig/extractVariationuse would read as a disabled variation, and a client that is not ready yet would prune everything.experimental.configSkillsonce, when called, not on each read. It projects through a new non-reportingskillRefsInternal.writeSkills(source, root)readsrefsexactly once.SkillReferencevalues and valid key strings is an unavailable retrieval. That includesnull, a bare string or'*', or one malformed entry beside a valid one. Under'keep'the report gets one run-levelerror(empty key) and nothing is written or removed. Under'raise'it throws, and the message names the skill source.timeoutbounds the read.timeout: 0never callsrefs. A promise still pending at the deadline is abandoned and its eventual result discarded; a late rejection is not left unhandled.watchSkills(source, root)follows config changes:client.on('update:<key>', listener)once per distinct config key, before the initial reconcile. The listener only callswatcher.notify(), so store changes and config changes share one debounce, and each reconcile reads the source once.configKeys, it throws before attaching anything if there is no initialized client, the client has noon, orisOffline()is true. The message names both remedies: a base LaunchDarkly client, or an emptyconfigKeysplus your own listener callingwatcher.notify().close()detaches withoff(falling back toremoveListener). A client with neither still closes cleanly.'*'and fixed lists never subscribe, and need no listener API on the client.LDClientInterfacegains optionalon,offandisOffline, probed withtypeof … === 'function'. The four server SDKs still satisfy it (sdk-compat.typecheck.ts).SkillWatcher.notifywas already a public arrow property. Its docs now describe it as a public trigger rather than "the registered listener, for tests". The watcher's constructor signature only widensrequestto accept a source. Config detach handles live in a module-levelWeakMap, so the class gains no members.api-surface/ai-server.experimental.txtgets+ configSkills usage=experimental.configSkills,+ createSkillSourceand+ SkillSource. These are the lines the pending upstream list change adds, so the next sync is a no-op. The usage test now derives anexperimental.configSkillsrow from that line. Both API reports are regenerated.configSkills" (including the daemon-mode statement foruseLdd: true) and a "Which changes reach disk" table for'*'/ fixed list /SkillSource. The revocation paragraphs now claim pruning only for the forms that prune. No example feedsskillRefs(...)towatchSkills.agents.mdgains the new module in the file map and pitfalls 4e–4g.Tests
skills-source.test.ts(fake clients, 115 tests) covers:configSkillsclassification, thenulldefault argument, no lazy init, fresh evaluation per read, and usage/telemetry;writeSkillssource equivalence with the list form, read-once, every malformed answer under'keep'and'raise',nullafter a two-skill write,timeout: 0, and abandonment at the deadline;'*'or a list;notify();close()withoff, withremoveListener, with neither, and with a throwingoff;removed;absentreference, and while the store is unavailable;skills-source-sdk.test.tsdrives the real@launchdarkly/node-server-sdkthroughTestData(updateProcessor: td.getFactory(), the FDv1 path): v2 → config moves to v3 →error→ v3 arrives →updated. Another flag's change doesn't wake the watcher, andclose()leaves noupdate:<key>listener.Where the spec was unclear or wrong
absentreference does not hold the prune. The first draft of the spec said anabsentsecond reference in the same run keeps a dropped skill on disk, which matched neither SDK's reconcile. The spec now says what both SDKs do: anabsentreference is kept on disk under its own key and does not mark the run incomplete, and only an unavailable retrieval (a store that raised, one without its initial data, an exhausted timeout) suppresses the prune. The watcher tests assert the three cases it names: a clean drop removes; a drop beside anabsentreference still removes and keeps the absent skill's file; with the store unavailable nothing is removed, and the next available run removes it. The equivalence test also asserts thatabsentplus a dropped key givesremoved.experimental.*IDs. Here,configSkillsis the first skills helper that reports. Its non-reportingskillRefsInternalis in place for whenskillRefsstarts reporting.SkillWatcher.notify()already existed here as a public arrow property, so "add a publicnotify()" was a documentation change on this side.writeSkillsrecognizes a source by shape (a non-array object with arefsfunction).watchSkillsre-validates a hand-built source throughcreateSkillSource, so a bare-stringconfigKeysstill throwsTypeErrorthere.isOffline()that throws isn't specified. It is treated as offline, sowatchSkillsraises rather than subscribing to a client it cannot vouch for.configKeysaren't specified. They are kept inconfigKeysas given, and subscribed once each.LDClientInterface.scripts/api-breaking.mjscounts any changed declaration as breaking, including new optional members. They are compatible for every SDK client and for structural implementers, so this needs an owner'sapi-compatiblelabel rather than a!title.configSkills,createSkillSourceandSkillSourcealong with the rest.Test plan
yarn code:checkyarn buildyarn typecheck(includes the SDK-compat typecheck)yarn testyarn api:surface,yarn workspace @launchdarkly/ai-server api:check,yarn test:api-surface,yarn lint:pkgnode scripts/api-breaking.mjs pr: fails on the optionalLDClientInterfacemembers until an owner appliesapi-compatible(above)🤖 Generated with Claude Code
Note
Overview
Adds dynamic skill sets for Agent Skills so
watchSkillscan follow AI Config changes instead of only a fixedskillRefslist.SkillSource(createSkillSource,configSkills) suppliesrefsread on each reconcile and optionalconfigKeysthat trigger re-reads via the base client'supdate:<key>events.writeSkillsandwatchSkillsaccept a source; reads are bounded bytimeout, fail closed (never treat errors as[]), and leave disk unchanged on bad reads.configSkillsevaluatesvariation(key, context, null)per read (real LD evaluations) and reports usage once.LDClientInterfacegains optionalon/off/isOfflinefor config following.SkillWatcher.notify()is documented as a public debounced reconcile trigger;close()also detaches config listeners.Experimental exports and API surface lists are updated; README/agents.md add
configSkillsguidance and a "which changes reach disk" table. Large test suites cover source validation, watcher debouncing, and real Node SDK integration.Reviewed by Cursor Bugbot for commit 8afeead. Bugbot is set up for automated code reviews on this repo. Configure here.