Skip to content

feat(client): Agent Skills — re-reconcile on delivery with watch_skills - #81

Merged
XieX merged 11 commits into
split/skills-review-closeoutfrom
xie/skills-watch
Sep 15, 2026
Merged

XieX merged 11 commits into
split/skills-review-closeoutfrom
xie/skills-watch

Conversation

@XieX

@XieX XieX commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Stacked on #66 (split/skills-review-closeout). First of three PRs split out of #69; the FDv2 protocol layer and the transport follow on top of this one.

write_skills is a one-shot reconcile, so a revocation takes effect at the next process restart. watch_skills runs that reconcile now and again whenever the configured store reports a change, so a revoked skill's SKILL.md leaves the disk within a debounce interval of the store learning about it, rather than at the next restart.

What's here

skills_watch.pywatch_skills / SkillWatcher. Wired to the SkillStore interface, not to any one transport: it needs a store that implements add_listener and nothing more. It refuses loudly when the store does not, because a watcher that silently never fires looks exactly like one whose skills never changed. The listener itself only sets an event; the reconcile runs on a single worker thread, debounced, so a burst of changes coalesces into one pass and a slow disk never stalls the store's delivery thread. A reconcile that raises is logged and the watcher continues. on_unavailable="keep" stays the default: an outage must not read as "everything was revoked".

remove_listener(kind, fn) joins add_listener as the optional second half of change notification, on the SkillStore contract and on InMemorySkillStore. SkillWatcher.close() needs it to detach; without it a store held every watcher ever created for the rest of its life. The watcher probes for it and skips detaching when a store does not implement it, so a customer's own store keeps working.

Tests

test_skills_watch.py drives the watcher through InMemorySkillStore, whose put notifies synchronously, and through small store doubles: initial reconcile, coalescing, refusal of a store without add_listener, detaching on close, and a store without remove_listener. Three tests on InMemorySkillStore.remove_listener join test_skills.py. The end-to-end case, a delete-object arriving over a live connection and pruning a file, lands with the transport PR where the fake endpoint lives.

🤖 Generated with Claude Code


Note

Overview
Adds production skill delivery and live disk reconciliation on top of the existing SkillStore / write_skills stack, without changing accessors or verification.

FDv2SkillStore connects to LaunchDarkly’s SDK FDv2 channel (/sdk/poll or /sdk/stream), holds raw wire objects behind the same SkillStore interface, commits updates at payload-transferred, maps wire keys key:version into stored identity (payload version is ignored), skips non-skill objects, refuses mobile/client credentials, and exposes StoreDiagnostics plus last-known-good behavior on outages.

watch_skills / SkillWatcher runs an initial write_skills, then debounced re-reconciles when the store fires change listeners—so revocations can prune managed files without a restart, with on_unavailable="keep" preserved.

remove_listener is added on InMemorySkillStore and documented on the optional listener seam so watchers can detach cleanly. Public exports and README/agents guidance cover FDv2 setup, beta caveats (TLS-only, no relay), and hashless_objects troubleshooting.

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

XieX and others added 2 commits September 10, 2026 16:02
write_skills is a one-shot reconcile, so a revocation takes effect at the
next process restart. watch_skills runs that reconcile now and again
whenever the configured store reports a change, so a revoked skill's
files leave the disk within a debounce interval of the store learning
about it. It is wired to the SkillStore interface, not to any one
transport: it needs a store that implements add_listener and nothing
more, and refuses loudly when the store does not, since a watcher that
silently never fires looks exactly like one whose skills never changed.

Above the interface, remove_listener joins add_listener as the optional
second half of change notification, on the SkillStore contract and on
InMemorySkillStore. SkillWatcher.close needs it to detach; without it a
store held every watcher ever created for the rest of its life. The
watcher probes for it, so a store without it keeps working.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…network

The half of the delivery transport that has no I/O: identifying a skill
object on the wire by kind inline-resource plus category skill, translating
it into the raw object shape the SkillStore interface defines, holding it
by (key, objectVersion), and applying a payload's events as one commit at
payload-transferred. The store that puts a connection underneath this
follows separately, so the three decisions that matter most can be
reviewed on their own:

- objectVersion is the skill's version; version is the payload's. The
  translation happens in one place and TestVersionTranslation asserts it
  in both directions, because confusing them fails silently.
- Changes commit at payload-transferred, not per object. A half-applied
  full transfer would briefly empty the store, which with pruning on is
  the difference between a reconcile and deleting a customer's files.
- A hashless object is held, not dropped, so verification withholds it
  with a reason code rather than the transport reporting it absent.

Flag and segment objects share the connection and are skipped and
counted, not rejected. Nothing here is exported yet; the store exports
it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread packages/client/src/launchdarkly_ai_server/skills_watch.py
XieX and others added 5 commits September 11, 2026 14:17
…oncile

watch_skills awaited write_skills and only then constructed SkillWatcher,
which is where the store listener attaches. The reconcile snapshots the
store as its first step and then spends the rest of its time on the
filesystem, so every write, fsync, prune and manifest rewrite in that
first pass ran with nothing listening. A change delivered in that window
was never seen, and since nothing re-reconciles on a timer, a revocation
that landed there waited for the next unrelated change — on a quiet root,
the next restart. Exactly the gap watch_skills exists to close.

The watcher now attaches its listener before the initial reconcile and
starts its worker after. notify only sets an event, so a change arriving
mid-reconcile is recorded and picked up by the worker's first pass, while
holding the thread back keeps write_skills's one-root-one-reconcile
contract: the worker cannot race the caller's own reconcile over the same
manifest. A reconcile that raises detaches the listener on the way out,
since the caller is handed an exception rather than a watcher to close.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The protocol reader took payloads[0]'s intentCode and applied it to the
skill object set, which is what the delivery protocol requires — one
payload per credential, read the first intent, tolerate the rest — but it
left the assumption behind that rule undocumented and unguarded. If the
one-payload guarantee ever widens, an xfer-full for another payload would
start an empty pending set and the next payload-transferred would publish
it: every skill reported revoked, and with pruning on, a customer's files
deleted.

The first payload is still the payload that is read. What is new is that
the reader now knows which payload skills actually arrive on — learnt from
the intent's id, or from the (p:<id>:<version>) selector, since no object
or transfer event carries a payload id of its own — and declines to apply
a transfer of any other, holding last known good, warning once, and
counting it in diagnostics.payloads_ignored. An intent describing more
than one payload warns once on its own, because that is the one case the
comparison cannot catch: another payload's transfer arriving before any
skill has been seen has nothing to be compared against.

Behaviour under one-payload delivery is unchanged, and a full transfer of
the skill payload still empties it — every skill deleted is a real state
the guard must not mask.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SDK-facing FDv2 channel now delivers skills the way streamer #4681 and
gonfalon #70638 spell them: object kinds are open strings, the agent-skill
payload is classified `generic`, and every generic object carries only `key`,
`kind`, `version` and `object`, exactly like a flag. A skill arrives under
kind `skill` with its own version folded into the key as `<key>:<version>`.
There is no `category` field and no `objectVersion` field; both came from an
earlier streamer draft that never shipped.

Identification is now the kind alone. The wire key is split in one place,
`_split_wire_key`, and both the put and the delete translation go through it.
A key that will not split cleanly is held rather than dropped — version-less,
or with the offending text as its version — so verification withholds it with
`invalid_version` under a key the caller recognises; only a key with nothing
before the delimiter is dropped, since there is no identity to hold it under.

`SDK_DATA_MODEL_VERSION` goes with it: the connection's `mv` parameter only
accepts flag model versions, and generic payloads ignore it. The transport
stops sending it in the following change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
FDv2SkillStore puts LaunchDarkly's SDK-facing FDv2 channel underneath the
protocol layer: GET /sdk/poll and GET /sdk/stream, authenticated with the
environment's server-side SDK key, streaming by default. It carries
basis across requests, sends If-None-Match and treats 304 as a current
answer, retries with capped jittered backoff, honours Retry-After only up
to max_backoff, gives up after a bounded run of consecutive failures
where a committed payload resets the count, and keeps serving last known
good through every failure. A mobile key or client-side environment ID
is refused in the constructor. Standard library only.

close interrupts the socket rather than only setting a flag, because the
delivery thread lives in a read no flag can reach; without that every
shutdown of a healthy stream waited out the full join timeout.

The no-store message now names FDv2SkillStore first, and watch_skills
points at it as the store with a delivery transport.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
_Requester.stream wrapped only the connect as recoverable, so a read
timeout, reset or truncated chunk in the body reached the delivery loop
as whatever the socket raised. The loop read that as a bug and gave up:
delivery stopped for the process lifetime, taking updates and
revocations with it, the first time a socket died. read_timeout exists
to bound a stream that has gone quiet so the loop can reconnect, and
tripping it did the opposite.

The body now carries the same promise the connect already did. Wrapping
the line source rather than the whole read keeps protocol reader errors
out of it: those are raised from the consumer's loop body, where they
still surface as the bugs they are.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
XieX and others added 2 commits September 14, 2026 14:36
…n promptly

Three faults in the delivery loop, all of which left the store reporting
itself healthy while doing less than it claimed.

**An up-to-date stream tripped the failure cap.** The consecutive-failure
count reset only at a commit, and an environment whose skills are not
changing answers every reconnect with `intentCode: "none"` and transfers
nothing. A stream only ever ends by being dropped, so each recycle of a
perfectly healthy idle connection counted as a failure — announced with a
`goodbye` or not — and `max_consecutive_failures + 1` of them stopped
delivery for the process lifetime, revocations included. The reset on
commit covered only the case where content had changed, which is the case
that was easy to test and not the case that runs in production.

`_TransferOutcome` now reports `up_to_date`, and a complete answer that
transfers nothing breaks the row of failures exactly as a commit does. An
intent this module does not recognise is still not an answer.

**`close` could not interrupt a poll.** The interrupt reached the streaming
connection only, so polling parked in its request with nothing to reach and
`close` returned when its join timed out — on a 300s-class request, long
after the process meant to exit. `_Requester` now tracks the response of a
poll in flight and offers `interrupt`, which `close` calls alongside the
stream's own. A request still inside its connect has no response to reach;
that one is bounded by `read_timeout`, and `start` no longer leaves the
store inert when a join times out around it. An interrupt we asked for is
no longer recorded as a delivery failure.

**`close` left a waiter parked.** `wait_for_skills` waited on the first
payload alone, so a shutdown racing a waiter added the waiter's whole
timeout to it. Delivery ending is now its own event: a waiter is released
by a payload, a give-up or a close, and reports whether a payload actually
arrived rather than merely that it was let go. That also settles what
`_give_up` had been quietly asserting — it set the first-payload flag to
unblock waiters, which made `wait_for_skills` answer `True` for a store
holding nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A stream only ever ends by being dropped, and LaunchDarkly — and any proxy
in between — recycles a long-lived one. Every reconnect therefore logged
"Skill delivery failed" at WARNING, for as long as the process ran. Until
the previous commit that noise was bounded, because an idle stream gave up
after eleven recycles and went quiet; now that delivery correctly survives
them, it would run forever and describe a healthy store as failing.

A connection that got a complete answer before it ended — a committed
payload, or an up-to-date intent — delivered everything it was asked for,
so its reconnect is now DEBUG and says so. A connection that ended without
answering is the case the warning exists for and still gets it: a connect
that never landed, or a transfer that died part-way through.

Filling a customer's logs with a fault they do not have is not merely
untidy; it teaches them that the level which means something can be
ignored.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stacked on the protocol-layer PR (`xie/skills-fdv2-protocol`). Third of
three PRs split out of #69, and the one that makes the feature real: the
network underneath the protocol reader, exported as `FDv2SkillStore`.

## Why this shape

Skill content arrives over `GET /sdk/poll` and `GET /sdk/stream`,
authenticated with the environment's server-side SDK key. These are the
SDK-facing endpoints the base SDK's FDv2 data source uses, and the
channel that payload signing will eventually cover. No private route is
involved, and no credential other than the environment's own SDK key
ships to a customer host. Standard library only, so the content path
adds no dependency to a package whose sole runtime dependency is
`opentelemetry-api`.

## What's here

**`FDv2SkillStore`.** Authenticates, streams (default) or polls, carries
`basis` across requests, sends `If-None-Match` and treats 304 as a
first-class current answer, and serves `get_object` / `all_objects` /
`add_listener` / `remove_listener` from what the protocol reader has
committed. Capped jittered backoff; `Retry-After` honoured but clamped
to `max_backoff` and rejected when non-finite; bounded
consecutive-failure retries, where a committed payload resets the count.
One network timeout, `read_timeout`, whose default follows the mode: 10s
for a whole poll, 300s between reads on a stream. A mobile key or
client-side environment ID raises from the constructor. Last known good
survives every failure; `diagnostics` and `failed` report the
degradation.

**`close` interrupts the socket.** The delivery thread parks in a read
no flag can reach, and closing a urllib response from another thread
does not unblock CPython's buffered reader, so `_interrupt_read` shuts
the socket down underneath it. Without that every shutdown of a
*healthy* stream blocked for the full join timeout.

**Above the interface**, two strings: `NO_STORE_MESSAGE` now names
`FDv2SkillStore` first, since it is the first thing a user sees on a
missing store and offering only the development store was wrong once a
production transport existed; and `watch_skills`' refusal message names
it as the store with a delivery transport.

## Bugs found and fixed while testing the loop

Five, all sharing one shape: the store stopped delivering while
continuing to report itself healthy.

- **The consecutive-failure counter never reset in stream mode.**
`_stream_once` always ends by raising, so a reset on return was
unreachable and `failures` grew for the whole process lifetime. Eleven
*fully successful* payload transfers were enough to trip
`max_consecutive_failures` and stop delivery for good, revocations
included. A commit now resets the count, in `_apply`.
- **A non-finite `Retry-After` killed the delivery thread.**
`float("inf")` parses, and `Event.wait(inf)` raises `OverflowError` from
inside the recoverable-error handler. Non-finite values are rejected and
every honoured delay is clamped to `max_backoff`.
- **`close()` during the initial connect waited out its full join
timeout.** `self._connection` was assigned after the connect returned,
so a `close()` in that window found nothing to interrupt. The stop flag
is re-checked immediately after the assignment.
- **`close()` blocked for the full join timeout on every healthy
stream.** See `_interrupt_read` above.
- **A stream interrupted by our own `close` was reported as a delivery
failure.**

Also: `connect_timeout` was accepted and never used, so a poll against a
black-holed host hung for 300s rather than 10. It is gone, with the
request timeout now chosen by mode, and `TestTimeouts` measures the
bound against a socket that accepts and never answers.

## Tests

> **Rebase note.** The previous push of this branch had silently
reverted the protocol PR's last commit (payload identity:
`payloads_ignored`, `_is_foreign_payload`, `TestPayloadIdentity`).
Rebasing onto the updated protocol branch restored it; the full suite
passes with it present.

`_FakeFDv2Endpoint` is an in-process `ThreadingHTTPServer` implementing
the wire contract, so request construction and header handling are
exercised over real sockets rather than mocked. Covers skill put/delete
over the wire, mixed payloads, 304, `basis` round-tripping,
reconnect/backoff in both modes, `Retry-After` including non-finite and
oversized values, bounded retries and the reset on commit, prompt
shutdown during connect and during a healthy stream, hashless envelopes
end to end through the accessors, server-side-only credentials,
timeouts, and `watch_skills` over the transport: a wire-level revocation
pruning a file without a restart.

Full suite 1629 passing, 11 skipped; `ruff`, `ruff format`, and `mypy`
clean.

## Open items, none in this PR's scope

- 🔴 **`contentHash` is not on the wire yet.** Against a real environment
today every skill resolves to nothing. This PR makes that loud (an error
per hashless object, a summary per wholly-hashless payload,
`diagnostics.hashless_objects`) rather than surviving it.
- 🔴 **Server-side skill delivery is not deployed.** The wire shape this
store reads — kind `skill`, key `<key>:<version>`, generic payload — is
what [streamer
#4681](launchdarkly/streamer#4681) and [gonfalon
#70638](launchdarkly/gonfalon#70638) emit; both
are still open. No account can receive skill objects until they ship and
the producer is enabled.
- 🟡 **FDv2 is opt-in per account.** A real environment returns 403
today; the store reports it as fatal and explains what to do.
- ~~🟡 **`mv` is a guess.**~~ Resolved: the request sends no `mv`. That
parameter selects the *flag* data model and the connection rejects any
value but the flag default, while the generic agent-skill payload is
served regardless of it. The `data_model_version` constructor argument
is gone with it.
- 🟡 **No payload signing** on this channel yet, so Beta is TLS-only.
- 🟡 **The connection also carries the environment's flags.** Skipped and
counted; a transport property, not fixable here.
- 🟡 **`ld-relay` does not speak the FDv2 endpoints**, so relay-only
deployments cannot receive skills in Beta.

**Nothing here has touched a real LaunchDarkly environment**, because it
cannot yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds **`FDv2SkillStore`**, a production `SkillStore` that pulls agent
skills over LaunchDarkly’s SDK FDv2 **`/sdk/poll`** and
**`/sdk/stream`** endpoints (stdlib HTTP, background delivery thread,
stream-by-default). It implements **`SkillStore`** (`get_object`,
listeners, etc.) on top of the existing protocol reader, plus
**`StoreDiagnostics`**, **`wait_for_skills`**, capped backoff with
**`Retry-After`**, and **`close`** that interrupts blocked socket reads
so shutdown is prompt.
> 
> **Public surface:** `FDv2SkillStore` and `StoreDiagnostics` are
exported from the package; README documents production setup with
`init_client` and `watch_skills`. Server-side SDK keys only;
mobile/client credentials are rejected. Outages keep last-known-good
content; accessors above the store are unchanged.
> 
> **Delivery-loop fixes** bundled here: reset consecutive-failure counts
on successful commits / up-to-date answers (so healthy stream recycling
does not stop delivery), safe handling of non-finite **`Retry-After`**,
and not treating intentional **`close`** interrupts as transport
failures. Removed unused **`connect_timeout`**; **`read_timeout`** is
the single knob with mode-specific defaults.
> 
> **Tests:** in-process fake FDv2 server exercises poll/stream,
basis/ETag/304, revocations, retries, timeouts, hashless payloads, and
**`watch_skills`** over the transport.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
88c225e. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
…network (#82)

Stacked on the `watch_skills` PR (`xie/skills-watch`). Second of three
PRs split out of #69; the transport that puts a connection underneath
this follows.

The half of the delivery transport that has no I/O: identifying a skill
object on the wire, translating it into the raw object shape the
`SkillStore` interface defines, holding it by `(key, objectVersion)`,
and applying a payload's events as one consistent commit. Splitting it
out lets the three decisions that matter most be reviewed without a
socket in the way.

## Three things worth reviewing closely

**1. The skill's version is in the object's `key`; `version` is the
payload's.** Each version of a skill is its own object on the wire,
identified as `<key>:<version>` (`pdf-extraction:3`), and that is the
only place the skill's version appears. The event's `version` is the
payload's, and confusing the two fails *silently*: the object verifies,
the hash matches, and the caller gets content under a version number
that means nothing. The wire key is split in exactly one place
(`_split_wire_key`), both the put and the delete translation go through
it, and `TestVersionTranslation` asserts it in both directions. A key
that will not split cleanly is *held* rather than dropped —
version-less, or with the offending text as its version — so
verification withholds it with `invalid_version` under a key the caller
recognises; only a key with nothing before the delimiter is dropped.

**2. Changes commit at `payload-transferred`, not per object.** A
payload version is the unit of consistency. A half-applied full transfer
would publish a state the server never described and would briefly empty
the store, which, with pruning on, is the difference between a reconcile
and deleting a customer's skill files. An interrupted transfer leaves
last known good intact, and listeners fire once per commit.

**3. A hashless object is held, not dropped.** Dropping it at the
transport would report `absent`, indistinguishable from "no such skill",
and would let a prune delete the last known-good copy on disk. Holding
it means verification withholds it with `missing_content_hash`, which is
diagnosable: an ERROR per `(key, version)`, deduped per reader rather
than per process so two stores never quieten each other, a summary per
wholly-hashless payload, and a `StoreDiagnostics.hashless_objects`
counter. There is deliberately no fallback that synthesises a hash from
the delivered content.

## Also here

- **Skills are `kind == "skill"`; everything else is ignored, not
rejected.** Object kinds on the SDK-facing channel are open strings and
the agent-skill payload is classified `generic`, so a skill arrives
under the kind its producer registered — the bare category name — with
no `category` or `objectVersion` field ([streamer
#4681](launchdarkly/streamer#4681), [gonfalon
#70638](launchdarkly/gonfalon#70638)). An
environment's assignment carries its flag payload alongside its
agent-skill payload, so flag and segment objects arrive as a matter of
course. Erroring on them would turn a normal payload into a permanent
reconnect loop.
- **`_SkillObjectSet`** holds several versions of one key, with lookup
semantics identical to `InMemorySkillStore` down to the fall-through to
a version-less entry. Its opaque snapshot keys are spelt
`<key>:<version>`, the same as the wire, and a test pins that round
trip. `TestInterfaceParity` asserts the two resolve identically.
- **`_require_server_side_credential`** refuses a mobile key or
client-side environment ID. Its tests arrive with the store constructor
that calls it.

Nothing here is exported yet; the store exports it. The module imports
nothing from the feature but the version validator.

## Tests

`test_skills_fdv2.py` drives `_ProtocolReader` directly: identification,
version translation, full and change transfers, interruption,
revocation, tombstones, mixed payloads, unknown kinds and events, error
and goodbye, and the hashless dedupe across readers. The wire builders
it introduces are shared with the transport PR's fake endpoint.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds **`skills_fdv2.py`**, a stdlib-only layer below `SkillStore` that
parses LaunchDarkly FDv2 events without sockets. It maps wire
`put-object`/`delete-object` into the raw store shape, keeps skills in
**`_SkillObjectSet`**, and applies updates atomically in
**`_ProtocolReader`** at **`payload-transferred`** (not per object).
> 
> The critical wire rule is **`key` = `skillKey:objectVersion`** while
the event’s **`version` is the payload revision** and is
dropped—confusing them would silently serve the wrong pinned version.
Non-`kind == "skill"` objects (flags, segments) are **ignored**, not
errors. **Foreign payload** full transfers are declined once skills’
payload id is known, so a flag `xfer-full` cannot wipe held skills (and
trigger prune). **Hashless** skills are retained for verification to
withhold with `missing_content_hash`, with **`StoreDiagnostics`** and
loud logging. **`_require_server_side_credential`** rejects
mobile/client credentials (for the upcoming networked store).
> 
> **`agents.md`** documents the transport contract; **`skills_core`**
clarifies `SKILL_OBJECT_KIND` vs wire kind. **`test_skills_fdv2.py`**
(~900 lines) exercises the reader, version translation, payload
identity, and parity with `InMemorySkillStore`. Nothing is exported or
wired from accessors yet—that’s the follow-on transport PR.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
efc4ca7. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
@XieX
XieX merged commit 09d7b60 into split/skills-review-closeout Sep 15, 2026
4 checks passed
@XieX
XieX deleted the xie/skills-watch branch September 15, 2026 14:34

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 96576aa. Configure here.

if outcome.committed:
self._publish_first_payload()
if outcome.changes:
self._notify(outcome.changes)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Empty full transfer skips listener notify

High Severity

An xfer-full commit that only removes skills never wakes listeners. _changes records puts and deletes, not objects dropped by replacing the set, and _notify is skipped when that list is empty. watch_skills therefore does not re-reconcile, so a revoked skill's SKILL.md stays on disk until the next unrelated change or process restart — the gap this feature exists to close.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 96576aa. Configure here.

committed=True,
changes=changes,
basis=state if isinstance(state, str) and state else None,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ignored payload still advances basis

Medium Severity

A transfer declined as a foreign payload still returns committed=True and the foreign state. _apply then stores that selector as _basis, so the next poll or stream advertises the other payload's cursor. Subsequent skill deltas can be skipped or applied against the wrong snapshot, even though the skill set was intentionally left unchanged.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 96576aa. Configure here.

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