fix(client): Agent Skills — transport, lifecycle and spec fixes from the TypeScript review - #93
Merged
Merged
Conversation
`FDv2SkillStore` built both `/sdk/poll` and `/sdk/stream` from a single `base_uri`, so the default configuration sent its streaming request to `sdk.launchdarkly.com`. LaunchDarkly serves streaming from a separate host, and `mode="stream"` is this store's default (TESTING.md §3.25), so this was the default path rather than an edge case: the first contact with a real environment would have gone to the wrong host. Both base server-side SDKs ship the two as distinct defaults, which is the evidence this follows: ldclient.config.Config base_uri='https://app.launchdarkly.com' stream_uri='https://stream.launchdarkly.com' @launchdarkly/js-server-sdk-common baseUri: 'https://sdk.launchdarkly.com' streamUri: 'https://stream.launchdarkly.com' `DEFAULT_BASE_URI` here already pointed at the FDv2 polling host (`sdk.launchdarkly.com`, not `ldclient`'s older `app.` value), so only the streaming host is new. Adds `DEFAULT_STREAM_URI` and a `stream_uri` keyword, mirroring `skills-fdv2.ts`: a `base_uri` given on its own still applies to both endpoints, so a relay or a private instance serving both from one host needs only the one option, and naming both overrides them independently. `_require_https_base_uri` becomes `_require_https_uri` and takes the option name, so a cleartext `stream_uri` is refused under its own name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two independent transport defects. **`Retry-After: 0` had no floor.** `_retry_after_seconds` returns `max(0.0, seconds)`, and the retry site capped the value but never floored it, so a server — or an intermediate proxy — answering `0` had the delivery loop reconnect as fast as it could schedule, spending the whole of `max_consecutive_failures` in milliseconds and hammering the endpoint on the way. It is bounded, so nothing runs away, but the retry budget is gone and `Retry-After` is honoured in the least useful direction. Floored at `initial_backoff`, the same floor our own backoff starts from, matching `skills-fdv2.ts`. A short `Retry-After` is therefore raised to `initial_backoff`, which is what made `test_a_retry_after_header_is_honoured` discriminate before: it asked for 0.25s against a 5s initial backoff. Rewritten to ask for *longer* than our own backoff instead, so honouring the header is still the only way the assertion can pass, and `test_a_retry_after_header_is_parsed_off_the_wire` likewise — it was passing only because `poll_store`'s 0.05s `max_backoff` capped the floored value, so it no longer told the two apart. **`objects_revoked` counted tombstones for keys the store never held.** The increment fired whenever a `delete-object` parsed, whether or not it took anything away. The counter is operator-facing and gets read exactly when somebody is working out whether a revocation landed (TESTING.md §3.25, "Counters are assertable facts"), so an inflated figure misleads at the worst moment. Gated on `_SkillObjectSet.delete` having returned something. `changes` still carries every tombstone, so listeners are unaffected. Note this is a divergence from TypeScript rather than a port of it: `skills-fdv2.ts`'s `deleteObject` increments unconditionally too, and its `keysFullyRevoked` applies to the full-transfer revoke-by-omission diff, which this side does not implement. The same fix is owed there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three §3.25 deviations, all in the transport's lifecycle and reconnection rules. Grouped because each one's existing test pinned the old answer and had to move in the same change. **`close()` is final** (§3.25 lifecycle). A subsequent `start()` now raises instead of resuming delivery. Finality is what gives `close()` a postcondition a caller can rely on — delivery has stopped — including when the join times out on a thread parked somewhere no interrupt reaches. A store that could be restarted from there leaves the caller unable to tell whether delivery stopped, and a restart that silently never delivered again is the failure this forecloses. To resume, build a new store. Giving up at the failure bound is *not* closing, so a store that gave up can still be restarted and the re-arm path is unchanged. `test_a_restarted_store_waits_again` therefore drives the give-up path now, and `test_a_close_that_timed_out_leaves_the_store_restartable` becomes `..._is_still_final` — it was asserting exactly the behaviour finality removes. **HTTP 404 is fatal** (§3.25 reconnection). A 404 on `/sdk/poll` or `/sdk/stream` means the endpoint does not exist for this credential or instance — a mistyped base URI, typically — and no reconnect produces one. It was recoverable, so it burned the whole retry budget first. **HTTP 400 is retried exactly once, then fatal** (§3.25 reconnection). A 400 is what a stale `basis` selector looks like: the request carried state the server no longer recognises, and a connection built from nothing fixes it. It was fatal, so that repair never ran. Implemented as `_StaleRequestStateError`, a `_RecoverableTransportError` subclass whose handling drops `basis` and `etag` and asks for a full transfer — and gives up when there was neither to drop, since a request carrying no client state was refused on its own terms. That bound is what keeps this from being "400 is recoverable", and it needs no counter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`add_listener` recorded a registration for any kind and then never notified it, on both shipped stores — `InMemorySkillStore`, whose `put` only accepts skill objects, and `FDv2SkillStore`, which only ever delivers `SKILL_OBJECT_KIND`. TESTING.md §3.21 now requires both to raise. The argument is §3.26's own, the one that already makes `watch_skills` refuse a store with no `add_listener` at all rather than degrading to a one-shot reconcile: a listener that silently never fires "looks exactly like a watcher whose skills never changed". Registering on a kind the store will never deliver is the same failure with the same signature, and a store that accepted the registration has promised something it cannot keep. `ValueError` rather than `TypeError`: `kind` is a `str`, which is the accepted type, carrying a value the store cannot serve — the same distinction A.12 records for `write_skills` versus `get_skills`. `remove_listener` for an unregistered kind stays a no-op, so a caller tearing down need not track what it attached. `test_put_does_not_notify_other_kind_listeners` pinned the old behaviour and is replaced by the raise assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…window
**`debounce=float("nan")` defeated the guard** (TESTING.md §3.26). `nan
< 0` is `False`, so it passed `if debounce < 0` and then collapsed the
coalescing window to nothing: `threading.Event().wait(nan)` returns
immediately, so every delivered object reconciled on its own with no
coalescing at all — the opposite of what the option is for. Now
`math.isfinite` is checked alongside the sign, matching how
`write_skills` already guards its `timeout`. `inf` is refused for the
same reason it is refused there: a window nothing ever leaves.
Also fills the §3.26 test gaps, which had no coverage either way:
negative and non-finite debounce, `on_reconcile` firing for subsequent
reports and not the initial one, a reconcile that raises not killing the
watcher, `prune` / `on_unavailable` pass-through, and an invalid root
raising out of `watch_skills` rather than into a worker thread's log.
**The coalescing test was one of the vacuous shapes §3.26 warns about**
— twelve puts inside a single synchronous listener pass, asserted with
an upper bound (`reconciles <= 2`). A burst that arrives before the
worker has woken collapses to one reconcile whether or not the debounce
exists, so that shape passes against an implementation with the
debouncing deleted. Replaced with six puts spread across one window, an
exact figure, and an assertion that the counter moved at all; verified
by mutation — removing the debounce sleep turns it red.
The exact figure is two, not one, and that is a real deviation from
§3.26's "exactly one reconcile per debounce window": this watcher clears
its wake flag *before* the window rather than after, so a change
arriving inside the window schedules a further pass instead of merging
into the reconcile that is about to run. TypeScript restarts its timer
on each notification and so does merge them. The behaviour is
deliberate and documented in `_run` (a redundant reconcile converges;
a missed one does not) and it avoids the starvation a restart-the-timer
debounce admits, so it is left alone here and raised separately rather
than changed under a test-coverage commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`parse_ai_config` gated on `if skills is not None`, so `skills: null` was accepted and read as absent. TESTING.md §3.5 now calls this case out specifically, and the reason is the reason it is worth a commit of its own: read as absent, it makes `skill_refs` return `[]`, and a `prune=True` reconcile then **deletes** previously-materialized skill files on the strength of a field the SDK could not parse. §3.21 establishes exactly this principle on the store path — reading a malformed thing as absent "would let a tampered object look like a deleted one … and would let prune delete the last known-good copy on disk" — and it applies identically here. Failing the whole parse is the louder and safer outcome, and it is what TypeScript already does. Gated on key presence instead, so `_parse_skills` sees the `None` and rejects it as a non-array, with `skills` in the message. There was no Python test either way; both halves are now pinned, including that an *absent* `skills` key is still valid, since rejecting `null` must not cost backward compatibility for configs that simply have no `skills`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`verified_bytes` encoded before checking the size cap, so content that
is both over the cap and carries a lone surrogate reported `not_utf8`.
TESTING.md §3.21 fixes the order as shape, then **size**, then
**encoding**, then hash, and that input is the one boundary where the
order is observable: without it, the code is whichever check the
implementation happens to reach first, and §3.21's "one code per failure
class" rule cannot be tested against it. Size first is also the cheap
check — running an encoding pass over a 10 MiB body before rejecting it
for being 10 MiB is a DoS foothold rather than a nicety.
Python cannot reorder these literally, because the size is a count of
encoded bytes and `str.encode("utf-8")` raises on an unpaired surrogate
rather than substituting. So the unencodable case now encodes a second
time with `errors="replace"`, purely to measure, and reports
`over_size_cap` or `not_utf8` in that order. Those replacement bytes are
never hashed and never returned — the `not_utf8` branch returns before
the hash comparison — which is what makes `errors="replace"` safe here
where `errors="surrogatepass"` is not safe anywhere: fabricated bytes
that reached the comparison could satisfy it. TypeScript arrives at the
same order for free, since `TextEncoder` substitutes U+FFFD silently
and its guard is an explicit round-trip check (A.12).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…README `agents.md`'s POSIX-only justification claimed the TypeScript SDK "could not match them at all — Node exposes no `*at()` family on *any* platform, so its racy floor is universal rather than Windows-only." The first half is still true; the second is not. TypeScript commit `0a15b10` added a `SUPPORTS_PROC_FD` probe and `/proc/self/fd/<fd>/<name>` addressing, which the Linux kernel resolves from the inode a descriptor holds rather than from the name it was opened under — closing the swap window there exactly as `*at()` does here. TypeScript's `lstat` floor now applies on macOS and Windows only, the same shape as this side's. The passage is load-bearing — it is why the Windows reparse-point checks are not implemented — so the rewrite says explicitly that the decision is unchanged and why: it never rested on TypeScript being equally exposed, it rests on there being no Windows CI runner to verify the checks against, which is still true of both repositories. README, for the behaviour changes in this branch: the two transport hosts and the `stream_uri` option, `close()` being final, `watch_skills`' `debounce` unit and bound and its `on_reconcile` contract, `add_listener` refusing a kind it will never deliver, and a non-array `skills` field failing the config parse rather than reading as absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4092de7. Configure here.
`_ProtocolReader` built a fresh empty `_pending` for `xfer-full`, so a skill the full transfer omitted was correctly dropped at `_payload_transferred` — but `_changes` only ever accumulated from `_put_object` and `_delete_object`, so nothing was published for the omitted skill. A full transfer carrying other puts still wakes a watcher through those puts, so the observable gap was the transfer carrying no puts at all: the environment's last skill revoked. The store emptied while `changes` stayed empty and `objects_revoked` stayed zero, `watch_skills` never reconciled, and the skill's `SKILL.md` survived on disk until some unrelated change — in exactly the case pruning exists for. Port TypeScript's `revocationsBetween` / `keysFullyRevoked`: diff the committed set against the incoming one before the swap, at `(key, version)` granularity so a key whose version moved yields both a put for the arrival and a tombstone for the departure, and count only keys that left the payload entirely toward `objects_revoked` — the same rule `_delete_object` follows in counting only a tombstone that took something away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An HTTP 400 for a request carrying a `basis` selector or an etag means that state may be one the server no longer accepts, so the transport drops it and asks for a full transfer once before treating the status as fatal. That one request was not guaranteed to go out: it was prepared and then measured against the consecutive-failure bound like any other retry, so a 400 arriving on a budget an outage had already spent gave up while holding the only request known to repair it, and delivery stopped for the process lifetime over state the store had just dropped. The repair is now exempt from the bound. It cannot unbound the loop: the repaired request carries no state, so a second 400 is fatal on its own, and any other failure after it meets a budget still over the bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tests for TESTING.md §3.21/§3.24 as just reconciled: a store answering under a different key records the `ld.skills.integrity_failure` log record with `reason_code: key_mismatch` and records no product signal. `key_mismatch` cannot join `REASON_CODE_CASES` — that table is uniformly driven through `all_skills`, and this code is decided at the retrieval boundary after `verify_raw_skill` has passed, so it is unreachable from a listing. It gets its own test and is unioned into the vocabulary assertion, with the reason recorded there so the next reader does not try to move it into the table. Adds the redaction guard, called directly since no store can drive it, and the listing-path case: an object listed under a disagreeing map key is filed under its own key with neither surface firing. That one passes already; it is a regression guard against a "fix" that would break every multi-version store. 3 failing: no record is emitted yet, the vocabulary is short a token, and `record_key_mismatch` does not exist. The no-signal half already passes — the implementation is additive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements the §3.21/§3.24 decision the previous commit's tests pin: a store answering under a different key now writes the `ld.skills.integrity_failure` log record with `reason_code: key_mismatch`, and still records no product signal. Adds `record_key_mismatch` beside `record_integrity_failure`, so the single-emission-site rule still holds by reading one module, and widens `IntegrityReasonCode` to nine tokens. The check stays where it was, at the retrieval boundary: `verify_raw_skill` is unary and this comparison is relational, so moving it inside would mean an optional expected-key parameter that silently disables the check when a caller omits it. The record carries both keys. `skill_key` keeps the meaning it has everywhere else — the key requested — and the key the store answered under goes in `served_key`, the one record-only field beyond the four, since it is what makes a broken adapter diagnosable and prose in `reason` is not parseable. No hash fields and no `version`: verification passed, so neither the hashes nor the version is what disqualified the answer, and reporting a served version beside a requested key would mix two frames in one record. Docs corrected in the same change: the README's `reason_code` table and record field table gain rows, `skill_key` is described as the *requested* key, and `AGENTS.md` records why `key_mismatch` cannot join `REASON_CODE_CASES` — that table is driven uniformly through `all_skills`, which cannot reach a code decided after verification passes. Verified the emitted JSON is byte-identical to the TypeScript SDK's for the same input, modulo `language`. 1875 tests passing, mypy and ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A transfer this layer declines as foreign kept its contents out of the store but still handed its selector up as the new basis, so the next poll or stream resumed from someone else's payload while every diagnostic read healthy. The check was also gated on a pending set, which a `none` intent never builds — so a `none` for another payload was not even recognised as foreign, and its selector was adopted outright. Ask the question unconditionally and drop the basis of a payload whose contents were thrown away, matching js-ai-sdk#67. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
``_poll_once`` held one ``_etag`` with no record of which request produced it, and adopted it before the body was applied. Two consequences: An ETag validates one representation of one resource, and the ``basis`` selector is part of the request that names it. Once a payload moved the basis on, the etag from the response before it was still offered, so a server validating it would be answering the question the store had stopped asking. Held as a pair now, and offered only while the pair holds — the base SDK keys its etag cache by request URL for the same reason. The cost is one unconditional request after each commit, which was never going to be a 304. A body that broke off partway — an ``error`` or ``goodbye`` after an announced transfer — left the payload it described unapplied, but its etag was already stored. The next 304 then reported that store as current and reset the failure count, where the 200 it replaced would have been retried and eventually given up on. Adopted after the body is applied in full. Ports js-ai-sdk#67. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch was two commits behind its base, which blocked CI: GitHub could not build a merge ref, so no workflow ran on the head. One conflict, in the stale-selector repair, where both sides added a line to the same block and both belong — the base's `repairing_state = True`, which keeps the one-shot repair out of the retry budget, and this branch's `self._etag_basis = None`, which drops the etag's pairing along with the etag. Resolved by keeping both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes row 18 (AV-1, Beta revocation propagation bound) of the security design review. The README already states the fast path: with the watcher running, a delete-object removes a revoked SKILL.md within the debounce interval on a stream, or within one poll interval. This adds the other half: a deployment that reconciles once at boot and never runs the watcher has a revocation bound of process lifetime, restart or an explicit re-run is the incident-response action, and content an agent has already read into a conversation is out of reach at this layer. The same pitfall is added to agents.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
donei003
approved these changes
Sep 24, 2026
knfreemLD
approved these changes
Sep 24, 2026
…94) Stacks on #93 -> #87 This should be the last of the python SDK fixes, ported over from the Typescript SDK work. ## The gap `_ProtocolReader._server_intent` builds a fresh empty `_pending` for `xfer-full`, so a skill the full transfer omits is correctly dropped from the committed set at `_payload_transferred`. But `_changes` only ever accumulated from `_put_object` and `_delete_object`, so nothing was published for an omitted skill. Verified against the reader directly before the fix: after a full transfer carrying a put for `pdf:3`, a second `xfer-full` carrying **no** objects emptied the store while `outcome.changes` was `[]` and `diagnostics.objects_revoked` stayed `0`. A full transfer that carries other puts still wakes a watcher through those puts, so the observable gap is the transfer carrying no puts at all — the environment's last skill revoked. `watch_skills` then never reconciles and the skill's `SKILL.md` stays on disk until some unrelated change, which contradicts TESTING.md §3.26 ("a revocation delivered over a live stream prunes the skill's files within `debounce` of arriving") in exactly the case pruning exists for. ## The fix Ports the shape TypeScript already has (`revocationsBetween` / `keysFullyRevoked` in `typescript/packages/client/src/skills-fdv2.ts`, called from `payloadTransferred` under `intent === INTENT_TRANSFER_FULL`): - `_identity_of`, `_revocations_between`, `_keys_fully_revoked` — module-level, placed where TypeScript has them, just above `_ProtocolReader`. - `_payload_transferred` diffs the committed set against the incoming one **before** the swap when the intent is `xfer-full`, extends `_changes` with the tombstones, and adds `_keys_fully_revoked(...)` to `objects_revoked`. - The diff is at `(key, version)` granularity to match `delete-object`, so a key whose version moved yields both a put for the arrival and a tombstone for the departure. The two counting rules stay consistent: **per key** for the omission diff (a version move counts zero), **per applied revocation** for `delete-object` (#93's change, where `objects_revoked` only increments when `_SkillObjectSet.delete` actually removed something). `_keys_fully_revoked`'s docstring names that as the shared rule. One doc touch: `add_listener` now says both ways of stating a revocation arrive as a tombstone — a `delete-object`, and a full transfer that stopped carrying the object. ## Tests Four protocol-reader cases: - a full transfer omitting a previously-held skill publishes a `{key, version}` tombstone and increments `objects_revoked`; - an omitted version-less object is reported with `version: None`, the way a `delete-object` naming no version spells it; - a version move publishes both ends and increments `objects_revoked` by zero; - an `xfer-changes` that mentions nothing revokes nothing — diffing a delta would drop every skill it simply had no reason to mention. Plus an end-to-end `test_a_full_transfer_that_omits_every_skill_prunes` over the fake FDv2 endpoint: queue a put, then `full_payload(state="basis-2")` with no objects, and assert the `SKILL.md` is pruned. These discriminate — with the source reverted, all four of the new cases fail. (The `xfer-changes` guard passes either way, as it should; it guards against applying the diff to the wrong intent.) The e2e case was run five times for flakiness. ## Gate From `python/`: `make test` (1872 passed, 11 skipped), `make typecheck` (clean, 51 source files), `make lint`, `make format-check` — all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code), edited by @XieX <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **FDv2 full transfers now surface revocations-by-omission** so `watch_skills` and pruning behave like explicit `delete-object` events: on `xfer-full`, the reader diffs committed vs pending at `(key, version)`, extends `changes` with tombstones, and counts true key removals in `objects_revoked` (version bumps emit both put and tombstone but do not count as revoked). > > **Polling/stream resume state is tightened** — declined or foreign payloads no longer advance `basis`; ETags are only sent when they match the current basis and are adopted only after a payload fully applies, avoiding false `304` “healthy” states. > > **Store key substitution is a first-class integrity outcome**: new `key_mismatch` `reason_code` with `served_key` on the `ld.skills.integrity_failure` log line only (no product signal), wired from `resolve_from_store` when verification passed but the object’s key ≠ the requested key. Docs and tests cover the nine-token vocabulary, listing vs pinned asymmetry, and boot-only `write_skills` revocation limits. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1ce4e53. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes surfaced by the TypeScript code review that needed fixing on the Python side. Includes bugs discovered with the Python implementation, and contract changes made during the review that needed to be ported over to Python.
6ac1577stream_uri, abovec69d880Retry-After: 0floored atinitial_backoff;objects_revokedcounts only revocations that removed something86fd04cclose()is final; HTTP 404 fatal; HTTP 400 retried exactly once from scratch, then fatal (§3.25)2f59b9aadd_listeneron a kind the store never delivers raises, on both shipped stores (§3.21)daeedecdebouncerejected; the §3.26 watcher test gaps filled and the coalescing test de-vacuumed (§3.26)4a204f8skills: nullfails the config parse instead of reading as absent (§3.5)030e8c3over_size_cap(§3.21)4092de7agents.md's stale cross-language parity claim, and the READMEBehaviour changes worth a second look
close()no longer resumes.start()on a closed store raises; constructa new one. Giving up at the failure bound is not closing, so that restart
path is unchanged. Three existing tests pinned the old answer and moved with
the change.
add_listener("flag", fn)now raisesValueErrorwhere it used to recorda listener that never fired.
skills: nullnow fails the whole config parse. Previously accepted.🤖 Generated with Claude Code, updated by @XieX
Note
Overview
Ports TypeScript review fixes into the Python Agent Skills stack: FDv2 delivery, store lifecycle, integrity telemetry, and config parsing, with matching README/
agents.mdupdates.FDv2SkillStore now treats polling and streaming as separate HTTPS origins (
base_uri/stream_uri, defaulting tosdk.launchdarkly.comvsstream.launchdarkly.com). Transport behavior is tightened: HTTP 404 is fatal; 400 with client state triggers one full reconnect with state cleared, then fatals; declined foreign payloads no longer advance the resumebasis; full transfers synthesize revocation tombstones when skills drop by omission;objects_revokedcounts only keys actually removed; poll ETags are tied to thebasisthey were issued for;Retry-After: 0is floored atinitial_backoff.close()is final —start()on a closed store raises (construct a new store to resume).Integrity and retrieval: new
key_mismatchreason when a store returns verified content under the wrong key — emits theld.skills.integrity_failurelog (withserved_key) but not the product integrity signal.verified_bytesuses a fixed order (size → encoding → hash) so over-cap surrogate content reportsover_size_cap.add_listeneron non-skill kinds raises on both shipped stores instead of registering a no-op listener.Config / watcher: explicit
skills: nullfails the whole AI config parse (avoids prune deleting materialized files).watch_skillsrejects non-finitedebounce; tests and docs cover debouncing,on_reconcile, and boot-only revocation limits.Reviewed by Cursor Bugbot for commit 6776390. Bugbot is set up for automated code reviews on this repo. Configure here.