From bdedb33ec8ddd2b629ad5ecaed353537b0343c39 Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 29 Sep 2026 12:07:48 -0400 Subject: [PATCH 1/4] fix(client): treat HTTP 422 as a fatal transport failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements ai-sdks-monorepo #29 (§3.25, A.12), which specifies the opposite of #23 and is expected to supersede it. 422 was classified as a third thing -- neither broken nor terminal -- and that shape is a retry loop with no bound and no budget: it never moved `_failures`, so it retried at `max_backoff` for the life of the process, invisible to both `failed` and `connection_failures`. The platform chose the status to be terminal rather than us inferring it. The streamer says so at both sites that produce it -- the narrow-assignment check and the status constant -- each noting the code exists so a misconfigured SDK stops instead of hammering the fleet. Retrying forever is the precise behavior it was picked to prevent. The stated cause was also wrong, which is what made the old handling look reasonable. The gate is `payloadvers.SkillDeliveryAllowed`: delivery enabled for the account, and a credential that is not view-scoped. Every way of failing it is permanent. Skill existence is not among the causes -- with the gate open and a non-view-scoped key, the assignment path creates the agent-skill payload row lazily, so an environment holding zero skills is assigned an empty payload that commits normally through `payload-transferred`. So the message is rewritten as well as the classification. It names the two causes a reader can act on, and drops the two false claims the old text made: that the condition is about whether any skill exists, and that a skill created later arrives without a restart. It is what a customer pastes into a support ticket, so it is asserted on substance. Routing 422 through `_give_up` needed no new code and buys the right accounting: `failed` and `last_error` are set and `connection_failures` is untouched, because that counter measures consecutive *recoverable* failures against the retry bound and a fatal never retries. It also makes `wait_for_skills` return `False` immediately, through the existing `_end_delivery` release -- the observable half of the classification, so a boot gated on skills stops paying its whole timeout. Both are asserted rather than implemented. `_NoSkillPayloadError` and `diagnostics.payload_unavailable` are deleted. Both absences are asserted by name, the way §3.22 asserts its unexported constants, since a reintroduction is otherwise visible only in a log line no test reads; the diagnostics field set is pinned whole alongside. Classification is asserted to answer every status in 300..599 with exactly one of the two classes, so the forbidden shape cannot return under a different name. The `?kinds=agent-skill` declaration and the kind constants are unchanged; they already conform. README and agents.md described the deleted field and the retry behaviour, so both are rewritten. Co-Authored-By: Claude Opus 5 --- packages/client/README.md | 22 +- packages/client/agents.md | 33 ++- .../src/launchdarkly_ai_server/skills_fdv2.py | 78 +----- packages/client/tests/test_skills_fdv2.py | 238 ++++++++++-------- 4 files changed, 186 insertions(+), 185 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index ca0ba47e..c2db6ecd 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -552,13 +552,19 @@ that as every skill having been revoked and delete the files it wrote on a previ against a store that has not received a payload reports the retrieval unavailable and leaves everything on disk alone. `report.ok` is `False` in that case, and the error names it. -**A project with no skills yet is a waiting state, not a failure.** Every request declares the -payload it wants (`kinds=agent-skill`), and LaunchDarkly answers HTTP 422 when the environment -has no such payload — which is the case until somebody creates the first skill in the project. -The store logs it once, keeps asking at its backoff cap, and picks up that first skill without -a restart. `failed` stays `None` throughout, the 422s are kept off `connection_failures` and -`last_error`, and `diagnostics.payload_unavailable` counts them: nonzero and rising beside an -empty store means "there is nothing to deliver", not "delivery is broken". +**Agent Skills has to be enabled for your account.** Every request declares the payload it +wants (`kinds=agent-skill`), and LaunchDarkly answers HTTP 422 when this connection will never +be assigned that payload: either Agent Skills is not enabled for the account, or the SDK key is +view-scoped, which cannot be assigned a skill payload. Neither is fixed by retrying, so delivery +stops — `failed` carries the reason and `last_error` is populated, while `connection_failures` +stays at zero, since nothing was retried. The store never initializes, so a reconcile takes the +readiness path above: `write_skills("*")` reports the retrieval unavailable and leaves every +file on disk alone. Enabling Agent Skills for the account reaches a process that already gave +up only when that process restarts. + +An environment that simply has no skills yet is **not** this case. It is assigned an empty +agent-skill payload, which commits normally and initializes the store, so a skill created later +arrives over the running connection with no restart. **Nothing above the store changes.** The accessors, verification, and `write_skills` see raw objects through the `SkillStore` interface and cannot tell which store produced them. @@ -641,7 +647,7 @@ Windows. | `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. | | `FDv2SkillStore(sdk_key, *, base_uri=…, stream_uri=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `is_initialized()`, `close()`, `diagnostics`, `failed`; also a context manager. `base_uri` and `stream_uri` are separate hosts, defaulting to LaunchDarkly's polling and streaming origins; `base_uri` alone covers both. `close()` is **final** — `start()` afterwards raises. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. | | `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change. Returns `(initial report, SkillWatcher)`; close the watcher when done. Revocation then takes effect within `debounce` of arriving rather than at the next restart. `debounce` is in **seconds** and must be non-negative and finite; `on_reconcile` is called with each *subsequent* report, the initial one being returned directly. One watcher per root. | -| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `payload_unavailable`, `last_error`. | +| `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `payloads_ignored`, `hashless_objects`, `connection_failures`, `last_error`. | Configure the store with `init_client(options={"skillStore": store})`. With none configured, the accessors raise `RuntimeError` explaining what to do and `write_skills` reports the diff --git a/packages/client/agents.md b/packages/client/agents.md index 886dcf17..a262d231 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -267,17 +267,28 @@ strings, held apart on purpose. No `mv`: that parameter selects the *flag* data delivery overrides whatever a request asks for with the payload's own default for any non-flagging payload, so sending it would state a preference that is ignored. -**HTTP 422 is not a failure.** It is the answer to that declaration when the credential is -assigned no agent-skill payload, which is every project in which no skill has ever been created -— LaunchDarkly creates that payload with the environment's first skill, never in advance. So -`_classify_status` maps it to `_NoSkillPayloadError`, and `_run` catches that **ahead of** -`_RecoverableTransportError`: counted under `diagnostics.payload_unavailable`, logged once per -store, retried at `max_backoff` indefinitely, and kept off `connection_failures`, `last_error`, -and `failed`. Neither of the two obvious classifications is right — as a recoverable failure it -spends `max_consecutive_failures` and then gives up permanently on an ordinary configuration; -as a fatal one, the skill created a minute later never arrives without a process restart. The -retry is at the *cap* rather than the initial backoff because `_failures` deliberately never -moves, so the exponential schedule would sit at the initial delay forever. +**HTTP 422 is fatal, and the platform decided that rather than this SDK inferring it.** +Delivery answers it when a connection's declared kinds exclude every payload it is assigned, and +it picked a non-400 4xx *because* LaunchDarkly SDKs treat those as terminal — the streamer +records the intent at both the narrow-assignment check and the status constant itself, each +saying the code exists so a misconfigured SDK stops instead of hammering the fleet. Retrying it +forever is the behaviour the status was chosen to prevent. So `_classify_status` returns a +`_FatalTransportError` and `_run` reaches `_give_up` through the same path as 401 and 404, which +is what gives the right accounting for free: `failed` and `last_error` are set, and +`connection_failures` is untouched, because that counter measures consecutive *recoverable* +failures against the retry bound and a fatal never retries. `wait_for_skills` returns `False` +immediately rather than at the timeout, via `_give_up` → `_end_delivery` → `_released`. + +The gate is `payloadvers.SkillDeliveryAllowed` in gonfalon: skill delivery enabled for the +account, **and** a credential that is not view-scoped. Every way of failing it is permanent — +the account's beta gate is off, the key is view-scoped, or the declared kind is not one delivery +recognises. **Skill existence is not among the causes**, which is what the error message must +not claim: with the gate open and a non-view-scoped key, the assignment path creates the +agent-skill payload row lazily, so an environment holding zero skills is assigned an empty +payload that commits normally through `payload-transferred`. The one cost is that enabling Agent +Skills for an account does not reach a process whose store already gave up: nothing reopens +delivery short of constructing a new store, since `close` is final and a later `start` raises. +That is preferred to retrying a permanent rejection for the life of the process. **The key travels over TLS only, and only to the base URI.** `_require_https_base_uri` refuses a plain `http://` base URI in the constructor — the SDK key would go out in cleartext — with a diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index c7c56759..f24b28c9 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -308,16 +308,6 @@ class StoreDiagnostics: """ connection_failures: int = 0 """Recoverable transport failures since the last successful transfer.""" - payload_unavailable: int = 0 - """ - Requests answered "no payload of the kind you asked for" (HTTP 422), which - is the answer until the first skill is created in this project. Cumulative, - and never reset. - - Deliberately not counted as a connection failure: nothing is wrong, there is - nothing to deliver yet. Nonzero and rising alongside an empty store means - this environment has no skills, not that delivery is broken. - """ last_error: str | None = None """The most recent transport error, if any. Human-readable; do not parse.""" @@ -1004,26 +994,6 @@ def __init__(self, message: str, retry_after: float | None = None) -> None: self.retry_after = retry_after -class _NoSkillPayloadError(_RecoverableTransportError): - """ - An HTTP 422: delivery has no payload of the kind this store declared. - - That is the answer for every project in which no skill has ever been - created, since the agent-skill payload row is created with the first one. - Neither of the two obvious classifications is right, which is why this is - its own class: - - - as a failure it would spend ``max_consecutive_failures`` and then give up - permanently -- "gave up after N consecutive failures" -- on a - configuration that is merely waiting for its first skill; - - as fatal, the skill created a minute later would never arrive, because - nothing reopens delivery short of a process restart. - - ``_run`` handles it ahead of ``_RecoverableTransportError``, which it - subclasses. - """ - - class _StaleRequestStateError(_RecoverableTransportError): """ An HTTP 400 for a request carrying client state — the ``basis`` selector, or @@ -1110,12 +1080,19 @@ def _classify_status(status: int, headers: Any) -> Exception: f"LaunchDarkly returned HTTP 400. {_REQUEST_ADVICE}" ) if status == 422: - return _NoSkillPayloadError( - "LaunchDarkly has no Agent Skills payload for this environment " - "(HTTP 422). That is the answer until the first skill is created in " - "this project; when one is, it reaches this store without a restart. " - "If this environment does have skills, check that this SDK key " - "belongs to it." + # Delivery refuses a connection whose declared kinds exclude every + # payload it is assigned, and chose a non-400 4xx precisely so SDKs stop + # instead of retrying. Its own branch rather than the generic list + # below: the two causes are specific and nothing about them is a + # malformed request, so ``_REQUEST_ADVICE`` would send the reader to the + # base URI, which is not where the problem is. + return _FatalTransportError( + "LaunchDarkly will not deliver Agent Skills on this connection " + "(HTTP 422). Either Agent Skills is not enabled for this account, " + "or this SDK key is view-scoped — a view-scoped key cannot be " + "assigned a skill payload. Neither is fixed by retrying, so " + "delivery has stopped; restart the process once Agent Skills is " + "enabled for the account." ) if status in (405, 406, 414, 501): return _FatalTransportError( @@ -1699,9 +1676,6 @@ def __init__( # A stream only ever ends by being dropped, so this is what separates # a recycled healthy connection from one that failed. self._attempt_answered = False - # Said once per store rather than once per attempt: the condition holds - # until somebody creates a skill, and delivery keeps asking throughout. - self._warned_no_skill_payload = False # -- lifecycle --------------------------------------------------------- @@ -1959,32 +1933,6 @@ def _run(self) -> None: except _FatalTransportError as exc: self._give_up(str(exc)) return - except _NoSkillPayloadError as exc: - # Ahead of the recoverable block below, none of which applies: - # nothing failed, so there is no count to advance and no - # ``last_error`` to leave on a store that is working fine. - if self._stop.is_set(): - return - with self._lock: - # A 422 refuses the connection before it opens, so this is - # only for a payload an earlier attempt left in flight. - self._reader._abandon_in_flight() - self._reader.diagnostics.payload_unavailable += 1 - say_it = not self._warned_no_skill_payload - self._warned_no_skill_payload = True - if say_it: - logger.warning( - "Skill delivery is idle: %s Retrying every %.1fs. This " - "is logged once.", - exc, - self._max_backoff, - ) - # At the cap rather than on the backoff schedule: ``_failures`` - # deliberately never moves, so the schedule would hold this at - # the *initial* delay forever. - if self._stop.wait(self._max_backoff): - return - continue except _RecoverableTransportError as exc: if self._stop.is_set(): # ``close`` interrupted the request on purpose. Counting it diff --git a/packages/client/tests/test_skills_fdv2.py b/packages/client/tests/test_skills_fdv2.py index f620989d..660e9210 100644 --- a/packages/client/tests/test_skills_fdv2.py +++ b/packages/client/tests/test_skills_fdv2.py @@ -51,12 +51,12 @@ FDV2_OBJECT_KIND, FDV2_PAYLOAD_KIND, MAX_RESPONSE_BYTES, + StoreDiagnostics, _backoff_delay, _classify_status, _FatalTransportError, _is_skill_event, _iter_sse, - _NoSkillPayloadError, _ProtocolReader, _RecoverableTransportError, _Requester, @@ -1783,19 +1783,6 @@ def stream_store(**kwargs: Any) -> FDv2SkillStore: ) -class _NoWaitStop(threading.Event): - """A ``_stop`` that answers every wait at once. - - Retrying at the backoff cap is the right production behaviour and the wrong - test fixture: a test that only cares what happens *after* the wait should - not sit through one. Kept out of ``poll_store`` so the waits stay real - everywhere they are part of what is being asserted. - """ - - def wait(self, timeout: float | None = None) -> bool: - return super().wait(0.001) - - class TestFailureHandling: def test_a_403_stops_delivery_and_explains_why( self, endpoint: Any, caplog: Any @@ -2009,123 +1996,172 @@ def test_the_exceptional_statuses_are_classified_apart(self) -> None: assert isinstance(_classify_status(status, None), _FatalTransportError) assert isinstance(_classify_status(503, None), _RecoverableTransportError) assert not isinstance(_classify_status(503, None), _StaleRequestStateError) - # 422 is the third class: recoverable enough to be retried, but its own - # type so the loop can keep it off the failure budget. - no_payload = _classify_status(422, None) - assert isinstance(no_payload, _NoSkillPayloadError) - assert isinstance(no_payload, _RecoverableTransportError) - assert not isinstance(no_payload, _FatalTransportError) - # The message explains the state rather than reciting the status: this - # is the line a user reads when their skills never show up. - assert "no Agent Skills payload" in str(no_payload) - - def test_a_422_never_stops_delivery_and_is_not_counted_as_a_failure( - self, endpoint: Any, caplog: Any - ) -> None: + # 422 is fatal, and the platform chose the status to be exactly that: + # a connection whose declared kinds match no payload it is assigned is + # refused, with a code LaunchDarkly SDKs stop on rather than retry. + refused = _classify_status(422, None) + assert isinstance(refused, _FatalTransportError) + assert not isinstance(refused, _RecoverableTransportError) + + def test_every_status_is_either_recoverable_or_fatal(self) -> None: + """ + There is no third class, and the shape matters more than the name. A + status classified as neither broken nor terminal is a retry loop with no + bound and no budget — retrying for the life of the process and invisible + to ``failed`` and ``connection_failures`` alike — which is exactly what + held 422 before it was classified as fatal. + """ + for status in range(300, 600): + classified = _classify_status(status, None) + fatal = isinstance(classified, _FatalTransportError) + recoverable = isinstance(classified, _RecoverableTransportError) + assert fatal ^ recoverable, ( + f"HTTP {status} classified as {type(classified).__name__}, " + "which is neither exactly recoverable nor exactly fatal" + ) + + def test_there_is_no_expected_recoverable_error_class(self) -> None: + """ + Asserted gone by name, because a reintroduction is otherwise visible + only in a log line no test reads. The class existed only to hold 422 and + goes away with that classification. + """ + assert not hasattr(skills_fdv2, "_NoSkillPayloadError") + + def test_diagnostics_does_not_count_an_unavailable_payload(self) -> None: + """ + ``StoreDiagnostics`` is public API from the moment it ships, so its field + list is the contract. A field counting "no payload of the kind you + declared" would count the 422 — and that status is fatal, so there is no + recurring event to accumulate and no running store to accumulate it on. + """ + assert "payload_unavailable" not in StoreDiagnostics.__dataclass_fields__ + assert not hasattr(StoreDiagnostics(), "payload_unavailable") + # The whole list, so a reintroduction under any other name fails too. + assert set(StoreDiagnostics.__dataclass_fields__) == { + "payloads_transferred", + "skill_objects_received", + "objects_ignored", + "objects_revoked", + "payloads_ignored", + "hashless_objects", + "connection_failures", + "last_error", + } + + def test_a_422_on_the_first_response_stops_delivery(self, endpoint: Any) -> None: """ - A 422 means the credential is assigned no agent-skill payload, which is - every project in which no skill has ever been created. Counting it as a - failure would spend the budget and report an ordinary configuration as - "gave up after N consecutive failures"; so it is counted, said once, and - retried for as long as the store is open. + A 422 means this connection will never be assigned a skill payload, not + that the environment has no skills yet: with delivery enabled and a key + that is not view-scoped, an environment holding zero skills is assigned + an empty payload that commits normally. Every cause of the 422 is + permanent, so the first one is enough to stop on. """ - endpoint.default_poll_status = 422 - store = poll_store(endpoint, max_consecutive_failures=1) - with caplog.at_level("WARNING"): - with store: - # On the diagnostic rather than the request count: the counter - # moves after the response is read, so the fourth request being - # logged does not mean the fourth 422 has been handled. - assert wait_until(lambda: store.diagnostics.payload_unavailable >= 4) - # Well past a bound of one, and still asking. - assert store.failed is None - assert store.diagnostics.payload_unavailable >= 4 - # None of it reads as a failure, because none of it is one. - assert store.diagnostics.connection_failures == 0 - assert store.diagnostics.last_error is None - # Said once, not once per attempt. - idle = [r for r in caplog.records if "Skill delivery is idle" in r.getMessage()] - assert len(idle) == 1 - assert "no Agent Skills payload" in idle[0].getMessage() - # And nothing was logged as a failure. - assert not any( - "Skill delivery failed" in r.getMessage() for r in caplog.records - ) + endpoint.queue_poll(status=422) + endpoint.queue_poll(full_payload(("put-object", put_skill()))) + # Well above one, to show the bound is not what stopped it. + with poll_store(endpoint, max_consecutive_failures=5) as store: + assert wait_until(lambda: store.failed is not None) + assert "422" in store.failed + # The queued payload is never asked for. + assert len(endpoint.requests) == 1 + assert store.is_initialized() is False - def test_a_422_waits_the_backoff_cap_rather_than_the_initial_delay( - self, endpoint: Any - ) -> None: + def test_the_422_message_names_both_of_its_real_causes(self, endpoint: Any) -> None: """ - The exponential schedule is a function of the failure count, which this - case deliberately never advances — so reusing it would hold a store - waiting for its first skill at the *initial* delay forever, polling as - fast as a fresh connection retries. + The message is a contract, because it is what a customer pastes into a + support ticket: it has to name the account-level enablement and the key's + scoping so the first person to read it can check both without reading + platform source. - Reaches into ``_stop`` because the delay is the thing under test: the - loop's wait is recorded and then not actually waited out, so the - assertion is on the interval asked for rather than on wall-clock timing. + Asserted on substance rather than prose, so the wording stays free to + improve while a message that drops either cause fails. """ - asked: list[float | None] = [] - - class RecordingStop(threading.Event): - def wait(self, timeout: float | None = None) -> bool: - asked.append(timeout) - return super().wait(0.001) - - endpoint.default_poll_status = 422 - store = poll_store(endpoint, initial_backoff=0.01, max_backoff=7.5) - store._stop = RecordingStop() - with store: - assert wait_until(lambda: len(endpoint.requests) >= 3) - assert asked, "the loop never waited" - assert asked[0] == 7.5 - assert 0.01 not in asked - - def test_a_skill_payload_arriving_after_a_422_is_picked_up( + endpoint.queue_poll(status=422) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + message = store.failed.lower() + assert "enabled" in message and "account" in message + assert "view-scoped" in message + # The two things it must not say. Both are what the earlier text said, + # and both are false: the condition is not about whether any skill + # exists, and no skill created later arrives without a restart. + assert "first skill" not in message + assert "without a restart" not in message + # It does name what actually clears the condition. + assert "restart" in message + + def test_a_fatal_422_is_not_counted_against_the_retry_bound( self, endpoint: Any ) -> None: """ - The reason this is not fatal. A skill created after the store started is - delivered to the store that was already running, with nothing restarted. + ``connection_failures`` measures consecutive *recoverable* failures + against the retry bound. A fatal never retries, so counting one would + put a number against a budget nothing will spend and make a store that + gave up on its first response indistinguishable from one that exhausted + its attempts. ``_give_up`` already accounts 401 and 404 this way, and + 422 reaching it means the accounting comes for free — so it is asserted + rather than implemented. """ - store = poll_store(endpoint, max_consecutive_failures=1) endpoint.queue_poll(status=422) + with poll_store(endpoint) as store: + assert wait_until(lambda: store.failed is not None) + assert store.diagnostics.connection_failures == 0 + assert store.diagnostics.last_error is not None + assert store.failed == store.diagnostics.last_error + + def test_a_fatal_422_releases_wait_for_skills_at_once(self, endpoint: Any) -> None: + """ + The observable difference from treating the status as recoverable, and + as much the point of the classification as the stopped retries are: a + boot gated on skills behind a ten-second wait would otherwise pay that + wait on every start, against a store that knew the answer on its first + response. + + The timeout is far longer than the suite would tolerate sitting through, + so the assertion is on the value *and* the elapsed time. + """ endpoint.queue_poll(status=422) - endpoint.queue_poll(full_payload(("put-object", put_skill()))) - store._stop = _NoWaitStop() - with store: - assert store.wait_for_skills(timeout=5) is True - assert store.get_object(SKILL_OBJECT_KIND, "pdf-extraction") is not None - # Both readings, in the order they happened. - assert store.diagnostics.payload_unavailable == 2 - assert store.diagnostics.payloads_transferred == 1 - assert store.failed is None + with poll_store(endpoint) as store: + started = time.monotonic() + assert store.wait_for_skills(timeout=30.0) is False + elapsed = time.monotonic() - started + assert elapsed < 5.0, ( + f"waited {elapsed:.1f}s for an answer delivery already had" + ) - async def test_a_422_leaves_the_store_uninitialised_and_prunes_nothing( + async def test_a_store_that_gave_up_on_a_422_prunes_nothing( self, endpoint: Any, tmp_path: Any ) -> None: """ - "LaunchDarkly has no skill payload for this environment" and "this + "LaunchDarkly will not deliver skills to this connection" and "this environment's every skill was revoked" are the two readings of an empty answer, and only the second may delete a customer's files. A 422 commits no payload, so the readiness probe stays false and the wildcard - reconcile withholds the prune. + reconcile withholds the prune for the whole run. + + The composition is what is under test rather than either half: the + readiness gate already exists, but "delivery gave up, therefore the + store is empty, therefore prune" is the inference an implementation + makes when it has to assemble this behaviour from two sections. It is + also the path a filesystem-agent deployment takes when Agent Skills is + not enabled for the account. """ root = tmp_path / "skills" stale = root / "left-behind" stale.mkdir(parents=True) (stale / "SKILL.md").write_text("not ours to delete", encoding="utf-8") - endpoint.default_poll_status = 422 - store = poll_store(endpoint, max_consecutive_failures=1) - store._stop = _NoWaitStop() + endpoint.queue_poll(status=422) + store = poll_store(endpoint) with store: - assert wait_until(lambda: store.diagnostics.payload_unavailable >= 3) + assert wait_until(lambda: store.failed is not None) assert store.is_initialized() is False assert store.wait_for_skills(timeout=0.1) is False await init_client(options={"skillStore": store}, client=object()) report = await write_skills("*", root) - assert (stale / "SKILL.md").exists() + assert report.ok is False + assert (stale / "SKILL.md").read_text(encoding="utf-8") == "not ours to delete" assert not any(a.action == "removed" for a in report.actions) def test_a_401_stops_delivery(self, endpoint: Any) -> None: From 1119479938ab8569af529c9fa74442618efb096e Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 29 Sep 2026 13:09:14 -0400 Subject: [PATCH 2/4] Update skills_fdv2.py --- .../client/src/launchdarkly_ai_server/skills_fdv2.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py index f24b28c9..5e9d18f7 100644 --- a/packages/client/src/launchdarkly_ai_server/skills_fdv2.py +++ b/packages/client/src/launchdarkly_ai_server/skills_fdv2.py @@ -1082,17 +1082,11 @@ def _classify_status(status: int, headers: Any) -> Exception: if status == 422: # Delivery refuses a connection whose declared kinds exclude every # payload it is assigned, and chose a non-400 4xx precisely so SDKs stop - # instead of retrying. Its own branch rather than the generic list - # below: the two causes are specific and nothing about them is a - # malformed request, so ``_REQUEST_ADVICE`` would send the reader to the - # base URI, which is not where the problem is. + # instead of retrying. return _FatalTransportError( "LaunchDarkly will not deliver Agent Skills on this connection " - "(HTTP 422). Either Agent Skills is not enabled for this account, " - "or this SDK key is view-scoped — a view-scoped key cannot be " - "assigned a skill payload. Neither is fixed by retrying, so " - "delivery has stopped; restart the process once Agent Skills is " - "enabled for the account." + "(HTTP 422). The usual cause is a view-scoped SDK key. Check your " + "SDK key or contact LaunchDarkly support. ) if status in (405, 406, 414, 501): return _FatalTransportError( From 50260aa35765f50031a78d426be551fe3d3b28be Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 29 Sep 2026 13:10:27 -0400 Subject: [PATCH 3/4] Update agents.md --- packages/client/agents.md | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/packages/client/agents.md b/packages/client/agents.md index a262d231..fb975658 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -267,28 +267,7 @@ strings, held apart on purpose. No `mv`: that parameter selects the *flag* data delivery overrides whatever a request asks for with the payload's own default for any non-flagging payload, so sending it would state a preference that is ignored. -**HTTP 422 is fatal, and the platform decided that rather than this SDK inferring it.** -Delivery answers it when a connection's declared kinds exclude every payload it is assigned, and -it picked a non-400 4xx *because* LaunchDarkly SDKs treat those as terminal — the streamer -records the intent at both the narrow-assignment check and the status constant itself, each -saying the code exists so a misconfigured SDK stops instead of hammering the fleet. Retrying it -forever is the behaviour the status was chosen to prevent. So `_classify_status` returns a -`_FatalTransportError` and `_run` reaches `_give_up` through the same path as 401 and 404, which -is what gives the right accounting for free: `failed` and `last_error` are set, and -`connection_failures` is untouched, because that counter measures consecutive *recoverable* -failures against the retry bound and a fatal never retries. `wait_for_skills` returns `False` -immediately rather than at the timeout, via `_give_up` → `_end_delivery` → `_released`. - -The gate is `payloadvers.SkillDeliveryAllowed` in gonfalon: skill delivery enabled for the -account, **and** a credential that is not view-scoped. Every way of failing it is permanent — -the account's beta gate is off, the key is view-scoped, or the declared kind is not one delivery -recognises. **Skill existence is not among the causes**, which is what the error message must -not claim: with the gate open and a non-view-scoped key, the assignment path creates the -agent-skill payload row lazily, so an environment holding zero skills is assigned an empty -payload that commits normally through `payload-transferred`. The one cost is that enabling Agent -Skills for an account does not reach a process whose store already gave up: nothing reopens -delivery short of constructing a new store, since `close` is final and a later `start` raises. -That is preferred to retrying a permanent rejection for the life of the process. +**HTTP 422 is fatal, and the platform decided that rather than the SDK inferring it.** Delivery answers 422 when a connection's declared kinds exclude every payload it is assigned, and it picked a non-400 4xx *because* LD SDKs treat those as terminal. So `classifyStatus` returns a `FatalTransportError` and the existing give-up path handles it: `failed` and `lastError` are set, `connectionFailures` is untouched (it measures consecutive *recoverable* failures against the retry bound, and a fatal never retries), and `waitForSkills` resolves `false` at once rather than at the timeout. **The key travels over TLS only, and only to the base URI.** `_require_https_base_uri` refuses a plain `http://` base URI in the constructor — the SDK key would go out in cleartext — with a From a359b3b9735626d18c5a09ff7eb98098782eb3cd Mon Sep 17 00:00:00 2001 From: Christie Williams Date: Tue, 29 Sep 2026 13:12:06 -0400 Subject: [PATCH 4/4] Update README.md --- packages/client/README.md | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index c2db6ecd..2898ea79 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -552,19 +552,20 @@ that as every skill having been revoked and delete the files it wrote on a previ against a store that has not received a payload reports the retrieval unavailable and leaves everything on disk alone. `report.ok` is `False` in that case, and the error names it. -**Agent Skills has to be enabled for your account.** Every request declares the payload it -wants (`kinds=agent-skill`), and LaunchDarkly answers HTTP 422 when this connection will never -be assigned that payload: either Agent Skills is not enabled for the account, or the SDK key is -view-scoped, which cannot be assigned a skill payload. Neither is fixed by retrying, so delivery -stops — `failed` carries the reason and `last_error` is populated, while `connection_failures` -stays at zero, since nothing was retried. The store never initializes, so a reconcile takes the -readiness path above: `write_skills("*")` reports the retrieval unavailable and leaves every -file on disk alone. Enabling Agent Skills for the account reaches a process that already gave -up only when that process restarts. - -An environment that simply has no skills yet is **not** this case. It is assigned an empty -agent-skill payload, which commits normally and initializes the store, so a skill created later -arrives over the running connection with no restart. +**A 422 means this connection will never be assigned a skill payload, and delivery stops.** +Every request declares the payload it wants (`kinds=agent-skill`), and LaunchDarkly answers +HTTP 422 when it will not serve one. The cause you can act on is a **view-scoped SDK key**: +a key restricted to a view cannot be assigned a skill payload, so check the key's scoping and +use one that is not view-scoped. The other cause is that Agent Skills delivery is not enabled +for your account, which is not a setting you control — contact LaunchDarkly support if the key +is not the problem. Retrying fixes neither, and LaunchDarkly chose the status so that SDKs stop +rather than hammer the fleet, so the store gives up: `failed` carries the reason, `lastError` is +populated, `connectionFailures` stays at zero (it counts consecutive *recoverable* failures against +the retry bound, which a fatal never spends), and `waitForSkills` resolves `false` immediately +instead of at your timeout. It is **not** the answer for an environment that merely has no skills +yet — with delivery enabled and a non-view-scoped key, an environment holding zero skills is served +an empty payload that commits normally. Because delivery has stopped for good, a process that booted +while the cause was in effect picks up skills only after a restart. **Nothing above the store changes.** The accessors, verification, and `write_skills` see raw objects through the `SkillStore` interface and cannot tell which store produced them.