Skip to content

feat(skills): follow AI Config changes in watchSkills through a SkillSource - #162

Open
XieX wants to merge 3 commits into
mainfrom
xie/skills-config-source
Open

XieX wants to merge 3 commits into
mainfrom
xie/skills-config-source

Conversation

@XieX

@XieX XieX commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Draft: the cross-SDK contract this follows is still in review, so details may change.

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 moves pdf-extraction from v2 to v3, v2 stays on disk reported error on 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. watchSkills subscribes to the base client's update:<key> events for those keys.

What changed

All new names are on @launchdarkly/ai-server/experimental only (and so on @launchdarkly/ai-node/experimental through its export *). None is on the package root.

  • SkillSource is a readonly interface { refs, configKeys }. createSkillSource({ refs, configKeys }) builds a frozen one with a frozen copy of configKeys. A non-function refs, a bare-string configKeys, or a configKeys holding anything but non-empty strings throws TypeError.
  • configSkills(key, context) returns the source for one AI Config. Each read evaluates variation(key, context, null) on the configured client and classifies the value:
    • Failed read (throws): no initialized client (it never calls initClient), variation throws, the default or any non-object, an object with no boolean _ldMeta.enabled, or an enabled config that fails to parse or has a malformed skills.
    • []: a disabled variation, or an enabled config with no skills.
    • The default is null on purpose. The { enabled: false } default that inspectConfig / extractVariation use would read as a disabled variation, and a client that is not ready yet would prune everything.
    • It reports experimental.configSkills once, when called, not on each read. It projects through a new non-reporting skillRefsInternal.
  • writeSkills(source, root) reads refs exactly once.
    • A source that throws, rejects, or answers anything but an array of SkillReference values and valid key strings is an unavailable retrieval. That includes null, a bare string or '*', or one malformed entry beside a valid one. Under 'keep' the report gets one run-level error (empty key) and nothing is written or removed. Under 'raise' it throws, and the message names the skill source.
    • timeout bounds the read. timeout: 0 never calls refs. 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:
    • It calls client.on('update:<key>', listener) once per distinct config key, before the initial reconcile. The listener only calls watcher.notify(), so store changes and config changes share one debounce, and each reconcile reads the source once.
    • With a non-empty configKeys, it throws before attaching anything if there is no initialized client, the client has no on, or isOffline() is true. The message names both remedies: a base LaunchDarkly client, or an empty configKeys plus your own listener calling watcher.notify().
    • close() detaches with off (falling back to removeListener). A client with neither still closes cleanly.
    • '*' and fixed lists never subscribe, and need no listener API on the client.
  • LDClientInterface gains optional on, off and isOffline, probed with typeof … === 'function'. The four server SDKs still satisfy it (sdk-compat.typecheck.ts).
  • SkillWatcher.notify was 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 widens request to accept a source. Config detach handles live in a module-level WeakMap, so the class gains no members.
  • API lists: api-surface/ai-server.experimental.txt gets + configSkills usage=experimental.configSkills, + createSkillSource and + SkillSource. These are the lines the pending upstream list change adds, so the next sync is a no-op. The usage test now derives an experimental.configSkills row from that line. Both API reports are regenerated.
  • Docs: the README gains "Following an AI Config: configSkills" (including the daemon-mode statement for useLdd: 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 feeds skillRefs(...) to watchSkills. agents.md gains the new module in the file map and pitfalls 4e–4g.

Tests

  • skills-source.test.ts (fake clients, 115 tests) covers:
    • construction and freezing;
    • every row of the configSkills classification, the null default argument, no lazy init, fresh evaluation per read, and usage/telemetry;
    • writeSkills source equivalence with the list form, read-once, every malformed answer under 'keep' and 'raise', null after a two-skill write, timeout: 0, and abandonment at the deadline;
    • watcher behavior:
      • no subscription for '*' or a list;
      • one listener per key;
      • a read per store trigger and per config trigger;
      • an unrelated key doesn't wake the watcher;
      • the listener only signals;
      • registration before the initial reconcile;
      • each no-signal raise, with nothing left attached;
      • notify();
      • the shared debounce, spread over time;
      • close() with off, with removeListener, with neither, and with a throwing off;
      • version bumps in both arrival orders, ending in identical state with no removed;
      • dropped-reference pruning: clean, beside an absent reference, and while the store is unavailable;
      • failed reads at boot and later.
  • skills-source-sdk.test.ts drives the real @launchdarkly/node-server-sdk through TestData (updateProcessor: td.getFactory(), the FDv1 path): v2 → config moves to v3 → error → v3 arrives → updated. Another flag's change doesn't wake the watcher, and close() leaves no update:<key> listener.
  • Checked by mutation: subscribing to the wrong event name fails the real-SDK test, and removing the debounce fails the shared-debounce test.

Where the spec was unclear or wrong

  1. Resolved upstream: an absent reference does not hold the prune. The first draft of the spec said an absent second 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: an absent reference 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 an absent reference 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 that absent plus a dropped key gives removed.
  2. This SDK reported no skills usage IDs before this PR. The spec describes seven existing experimental.* IDs. Here, configSkills is the first skills helper that reports. Its non-reporting skillRefsInternal is in place for when skillRefs starts reporting.
  3. SkillWatcher.notify() already existed here as a public arrow property, so "add a public notify()" was a documentation change on this side.
  4. Recognizing a source "by type alone": an interface has no runtime type, so writeSkills recognizes a source by shape (a non-array object with a refs function). watchSkills re-validates a hand-built source through createSkillSource, so a bare-string configKeys still throws TypeError there.
  5. isOffline() that throws isn't specified. It is treated as offline, so watchSkills raises rather than subscribing to a client it cannot vouch for.
  6. Duplicate configKeys aren't specified. They are kept in configKeys as given, and subscribed once each.
  7. The breaking-change check flags LDClientInterface. scripts/api-breaking.mjs counts 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's api-compatible label rather than a ! title.
  8. Conflict to expect: open PR feat(skills)!: promote Agent Skills to core #159 moves every Agent Skills name to the package root. Whichever of the two merges second will need to move configSkills, createSkillSource and SkillSource along with the rest.

Test plan

  • yarn code:check
  • yarn build
  • yarn typecheck (includes the SDK-compat typecheck)
  • yarn test
  • yarn api:surface, yarn workspace @launchdarkly/ai-server api:check, yarn test:api-surface, yarn lint:pkg
  • node scripts/api-breaking.mjs pr: fails on the optional LDClientInterface members until an owner applies api-compatible (above)

🤖 Generated with Claude Code


Note

Overview
Adds dynamic skill sets for Agent Skills so watchSkills can follow AI Config changes instead of only a fixed skillRefs list.

SkillSource (createSkillSource, configSkills) supplies refs read on each reconcile and optional configKeys that trigger re-reads via the base client's update:<key> events. writeSkills and watchSkills accept a source; reads are bounded by timeout, fail closed (never treat errors as []), and leave disk unchanged on bad reads. configSkills evaluates variation(key, context, null) per read (real LD evaluations) and reports usage once.

LDClientInterface gains optional on / off / isOffline for 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 configSkills guidance 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.

…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 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.

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-server report this PR regenerated. The branch base is f7883ce, and main's CI is green at f2404d9. Fix: rebase on main, run yarn api:update, and commit packages/claude-agents/etc/ai-claude-agents.api.md if it still differs. I did not reproduce which declaration changed.
  • API compatibility flags LDClientInterface (the new optional on / off / isOffline). I agree these are compatible for the four server SDKs and for structural implementers. Fix: an owner applies api-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 (:424 on this branch) marks a run incomplete only for an unavailable retrieval. An absent reference stays requested as error, and the dropped key is still pruned. Python main does the same (skills_fs.py _resolve_requests, "An absent reference does not set it"), and py#211 pins it with test_an_absent_reference_does_not_hold_the_prune.
  • Spec #37's "Dropping a reference" bullet says an absent second 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 at skills-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.ts createSkillSource, and configSkills('')). Python raises ValueError. 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, createSkillSource and SkillSource from src/index.ts, not src/experimental.ts, with the root list lines in api-surface/ai-server.txt. Example: + configSkills usage=client.configSkills.
  • change reportUsage('experimental.configSkills') (skills-source.ts, first line of configSkills) and the sdk-usage.test.ts row to client.configSkills;
  • add the three names to the root export lists that #159 extends: integration-tests/commonjs-webpack/fixture/entrypoints.{cjs,mjs}, types.ts, and packages/ai-node/src/__tests__/reexports.test.ts;
  • move skills.test.ts's EXPERIMENTAL_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 as feat(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.

@XieX
XieX marked this pull request as ready for review October 23, 2026 20:32
@XieX
XieX requested a review from a team as a code owner October 23, 2026 20:32
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.

2 participants