From 2163ab1aa3cd74c74abe7cf7f832a29b581bee4a Mon Sep 17 00:00:00 2001 From: Joe-Kneeland <104656007+Joe-Kneeland@users.noreply.github.com> Date: Tue, 18 Aug 2026 08:51:53 -0600 Subject: [PATCH 1/8] fix(llm): forward provider_params.timeout to the OpenAI-compatible embedding client (#1024) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(llm): forward provider_params.timeout to the OpenAI-compatible embedding client #832 and #903 added a configurable request timeout for the LLM registry and the Gemini embedding client respectively, but the OpenAI-compatible embedding client (src/embedding_client.py) was never wired up. It constructed AsyncOpenAI with no timeout at all, so a stalled socket against a slow or contended OpenAI-compatible backend (e.g. a self-hosted embedding model under load) wedges the deriver worker's event loop indefinitely — the exact failure #785/#903 describe, just via a code path #903 didn't cover. EmbeddingModelConfig now carries provider_params through from resolve_embedding_model_config, mirroring how resolve_model_config already does it for ModelConfig, and the OpenAI branch of _EmbeddingClient.__init__ extracts `timeout` via the existing request_timeout_from_extra_params helper. Unset stays unset — no existing behavior changes. Reproduced and verified against a real self-hosted deployment (local Ollama backend under load): before this fix, a single stuck embedding call blocked all deriver queue processing for 20+ minutes with no error logged, twice in one session. * fix(embedding): use first-class timeout on embedding model config provider_params is the LLM per-request escape hatch; embedding timeouts are client-construction knobs and belong next to max_batch_size. Wire the field for OpenAI and Gemini, omit the OpenAI kwarg when unset so the SDK default stays, and keep Gemini's 10-minute floor when unset. * test(embedding): live coverage for first-class embedding timeout Exercise EmbeddingModelConfig.timeout on one representative OpenAI and Gemini model: configured timeout lands on the SDK client, and a near-zero timeout aborts before the provider answers. --------- Co-authored-by: Aakash Kattelu --- config.toml.example | 2 + docs/v3/contributing/configuration.mdx | 8 ++ src/config.py | 19 +++ src/embedding_client.py | 22 +-- tests/live_llm/README.md | 2 +- tests/live_llm/conftest.py | 12 +- tests/live_llm/test_live_embeddings.py | 84 +++++++++++ tests/llm/test_embedding_client.py | 185 ++++++++++++++++++++++++- 8 files changed, 317 insertions(+), 17 deletions(-) diff --git a/config.toml.example b/config.toml.example index c5517999..60d9092a 100644 --- a/config.toml.example +++ b/config.toml.example @@ -77,6 +77,8 @@ model = "text-embedding-3-small" # Optional provider request input cap. Useful for OpenAI-compatible embedding # APIs with smaller limits, such as DashScope text-embedding-v4. # max_batch_size = 10 +# Optional client HTTP timeout in seconds (OpenAI + Gemini). +# timeout = 90.0 # Optional module-level endpoint overrides # [embedding.model_config.overrides] diff --git a/docs/v3/contributing/configuration.mdx b/docs/v3/contributing/configuration.mdx index c02a4ebb..3c210e71 100644 --- a/docs/v3/contributing/configuration.mdx +++ b/docs/v3/contributing/configuration.mdx @@ -267,6 +267,7 @@ EMBEDDING_MAX_TOKENS_PER_REQUEST=300000 EMBEDDING_MODEL_CONFIG__TRANSPORT=openai # openai, gemini EMBEDDING_MODEL_CONFIG__MODEL=text-embedding-3-small EMBEDDING_MODEL_CONFIG__MAX_BATCH_SIZE=10 # optional per-request input cap +EMBEDDING_MODEL_CONFIG__TIMEOUT=90.0 # optional client HTTP timeout (seconds) # Optional endpoint overrides EMBEDDING_MODEL_CONFIG__OVERRIDES__BASE_URL=http://localhost:8000/v1 @@ -279,6 +280,13 @@ document a per-request limit. Set it when an OpenAI-compatible embedding provider accepts fewer inputs per request, such as DashScope `text-embedding-v4` with a limit of 10. +`EMBEDDING_MODEL_CONFIG__TIMEOUT` is an optional client HTTP timeout in +seconds. OpenAI-compatible transports receive it as the SDK `timeout` kwarg +(omitted when unset, so the SDK default applies). Gemini converts it to +milliseconds on `http_options.timeout`, and keeps its existing 10-minute +default when unset. The value is validated at config load the same way as +LLM `provider_params.timeout` (positive, finite number of seconds). + Forwarding `dimensions=` to OpenAI-compatible providers is controlled by `EMBEDDING_MODEL_CONFIG__DIMENSIONS_MODE`: - `auto` (default): forwards `dimensions=` when **the operator has explicitly set `EMBEDDING_VECTOR_DIMENSIONS`** — provenance, not value — and the configured model is not on the known-rejecting list (currently `text-embedding-ada-002`). Explicit `EMBEDDING_VECTOR_DIMENSIONS=1536` *does* trigger the forward; this is how `text-embedding-3-large` truncation to 1536 is expressed. Deployments that leave the setting unset get their existing behavior (`dimensions=` is not forwarded). diff --git a/src/config.py b/src/config.py index d8100694..092cb6ee 100644 --- a/src/config.py +++ b/src/config.py @@ -394,6 +394,15 @@ class ConfiguredEmbeddingModelSettings(BaseModel): dimensions_mode: EmbeddingDimensionsMode = "auto" encoding_format_mode: EmbeddingEncodingFormatMode = "auto" max_batch_size: Annotated[int, Field(gt=0)] | None = None + # Client HTTP timeout in seconds. OpenAI receives seconds; Gemini converts to ms. + timeout: float | None = None + + @field_validator("timeout", mode="before") + @classmethod + def _validate_timeout(cls, v: Any) -> float | None: + if v is None: + return None + return coerce_provider_timeout(v) @model_validator(mode="before") @classmethod @@ -431,6 +440,15 @@ class EmbeddingModelConfig(BaseModel): api_key: str | None = None base_url: str | None = None max_batch_size: Annotated[int, Field(gt=0)] | None = None + # Client HTTP timeout in seconds. OpenAI receives seconds; Gemini converts to ms. + timeout: float | None = None + + @field_validator("timeout", mode="before") + @classmethod + def _validate_timeout(cls, v: Any) -> float | None: + if v is None: + return None + return coerce_provider_timeout(v) @model_validator(mode="before") @classmethod @@ -556,6 +574,7 @@ def resolve_embedding_model_config( api_key=api_key, base_url=configured.overrides.base_url, max_batch_size=configured.max_batch_size, + timeout=configured.timeout, ) diff --git a/src/embedding_client.py b/src/embedding_client.py index 08a4ff04..3cee4484 100644 --- a/src/embedding_client.py +++ b/src/embedding_client.py @@ -195,13 +195,13 @@ class _EmbeddingClient: from google import genai from google.genai import types as genai_types - # 10-minute HTTP timeout, in lockstep with the LLM registry's Gemini - # client (`src/llm/registry.py:_build_gemini_http_options`). Without - # this, a stalled Gemini embedding socket wedges the deriver worker - # exactly the way #785 describes for the LLM client. + # Default 10-minute HTTP timeout matches the LLM registry Gemini client. + timeout_ms = ( + int(config.timeout * 1000) if config.timeout is not None else 600_000 + ) http_options = genai_types.HttpOptions( base_url=config.base_url, - timeout=600_000, + timeout=timeout_ms, ) self.client: genai.Client | AsyncOpenAI = genai.Client( api_key=config.api_key, @@ -216,10 +216,14 @@ class _EmbeddingClient: raise ValueError("OpenAI API key is required") from openai import AsyncOpenAI - self.client = AsyncOpenAI( - api_key=config.api_key, - base_url=config.base_url, - ) + # Omit timeout when unset so the OpenAI SDK keeps its own default. + client_kwargs: dict[str, Any] = { + "api_key": config.api_key, + "base_url": config.base_url, + } + if config.timeout is not None: + client_kwargs["timeout"] = config.timeout + self.client = AsyncOpenAI(**client_kwargs) self.max_embedding_tokens = max_input_tokens self.max_batch_size = config.max_batch_size or 2048 diff --git a/tests/live_llm/README.md b/tests/live_llm/README.md index 6da7dfb4..442049bc 100644 --- a/tests/live_llm/README.md +++ b/tests/live_llm/README.md @@ -68,5 +68,5 @@ Coverage by provider: - OpenAI transport → OpenRouter non-reasoning models (e.g. `inception/mercury-2`): non-chat / diffusion architectures must stay on `max_tokens`, no `reasoning_effort`, tool-calling parameter-schema compatibility is the canary for exotic OR-served providers - Gemini 2.5/3.0 classes: structured outputs, cached-content reuse, thought signatures, multi-turn tool replay - Gemini 3.1 class: thinking and tool replay coverage by default; structured-output/caching coverage should only be added once Google documents support for that path -- Embeddings (`test_live_embeddings.py`): single embed, batched embed, batch-vs-single alignment, and chunk-to-id mapping for every configured embedding model. `gemini-embedding-2*` is the reason this exists — those models collapse a list of bare strings into one document (#745), and only a live call catches it +- Embeddings (`test_live_embeddings.py`): single embed, batched embed, batch-vs-single alignment, and chunk-to-id mapping for every configured embedding model. `gemini-embedding-2*` is the reason this exists — those models collapse a list of bare strings into one document (#745), and only a live call catches it. Also covers first-class `EmbeddingModelConfig.timeout` plumbing (one representative model per transport): configured timeout lands on the SDK client, and a near-zero timeout aborts before the provider answers - OpenAI-compatible embedding providers (e.g. OpenRouter's `google/gemini-embedding-001`): the #932 surface. Those providers reject a base64 embedding request outright (HTTP 400) or answer HTTP 200 with empty data, so the whole matrix fails without `encoding_format="float"`. Real OpenAI accepts base64 happily, so only a third-party provider catches it. Note that OpenRouter load-balances across upstreams, so the base64 failure is per-attempt rather than guaranteed: a retry can land on an endpoint that accepts it. `test_live_openai_float_encoding_matches_base64` covers the other side, that the float switch must not move vectors on real OpenAI diff --git a/tests/live_llm/conftest.py b/tests/live_llm/conftest.py index eb7b8dbb..9c58bb87 100644 --- a/tests/live_llm/conftest.py +++ b/tests/live_llm/conftest.py @@ -66,14 +66,23 @@ def require_embedding_key(spec: LiveEmbeddingSpec) -> str: return key +_EMBEDDING_CONFIG_OVERRIDE_KEYS = frozenset({"timeout", "max_batch_size"}) + + def make_embedding_client( spec: LiveEmbeddingSpec, **overrides: Any ) -> _EmbeddingClient: """Build a live embedding client for one matrix entry. Bypasses the `EmbeddingClient` singleton so each spec gets its own client - without mutating global settings. + without mutating global settings. `timeout` and `max_batch_size` land on + `EmbeddingModelConfig`; remaining kwargs go to `_EmbeddingClient`. """ + config_overrides = { + key: overrides.pop(key) + for key in _EMBEDDING_CONFIG_OVERRIDE_KEYS + if key in overrides + } kwargs: dict[str, Any] = { "vector_dimensions": spec.dimensions, "max_input_tokens": 2048, @@ -90,6 +99,7 @@ def make_embedding_client( model=spec.model, api_key=require_embedding_key(spec), base_url=spec.base_url, + **config_overrides, ), **kwargs, ) diff --git a/tests/live_llm/test_live_embeddings.py b/tests/live_llm/test_live_embeddings.py index c8af34b8..6ef2b048 100644 --- a/tests/live_llm/test_live_embeddings.py +++ b/tests/live_llm/test_live_embeddings.py @@ -1,10 +1,15 @@ from __future__ import annotations +import time from typing import Any, cast +import httpx +import openai import pytest from openai import AsyncOpenAI +from src.config import EmbeddingTransport + from .conftest import cosine_similarity, make_embedding_client from .embedding_matrix import LiveEmbeddingSpec, get_live_embedding_specs @@ -25,6 +30,52 @@ OPENAI_NATIVE_SPECS = tuple( spec for spec in ALL_SPECS if spec.family == "openai_embedding" ) +GENEROUS_TIMEOUT_SECONDS = 120.0 +TIGHT_TIMEOUT_SECONDS = 0.01 +# Well under the client defaults; generous enough to absorb SDK retries. +TIGHT_TIMEOUT_WALL_CLOCK_LIMIT_SECONDS = 30 + +EMBEDDING_TIMEOUT_EXCEPTIONS: dict[ + EmbeddingTransport, tuple[type[BaseException], ...] +] = { + "openai": (openai.APITimeoutError,), + # google-genai raises httpx or aiohttp timeouts depending on its transport; + # aiohttp surfaces as asyncio.TimeoutError (== builtins.TimeoutError). + "gemini": (httpx.TimeoutException, TimeoutError), +} + +TRANSPORT_MARKS = { + "openai": pytest.mark.requires_openai, + "gemini": pytest.mark.requires_gemini, +} + + +def representative_embedding_specs() -> list[Any]: + """One spec per transport — timeout plumbing is client-level, not model-level.""" + params: list[Any] = [] + for transport in ("openai", "gemini"): + specs = get_live_embedding_specs(transport=transport) + if not specs: + continue + # Prefer the native family over openai-compatible proxies. + family = f"{transport}_embedding" + native = next((s for s in specs if s.family == family), specs[0]) + params.append( + pytest.param(native, marks=TRANSPORT_MARKS[transport], id=native.id) + ) + return params + + +def assert_embedding_timeout_on_client( + client: Any, transport: EmbeddingTransport, timeout_seconds: float +) -> None: + if transport == "gemini": + http_options = client.client._api_client._http_options + assert http_options.timeout == int(timeout_seconds * 1000) + return + openai_client = cast(AsyncOpenAI, client.client) + assert openai_client.timeout == timeout_seconds + @pytest.mark.asyncio @pytest.mark.parametrize("spec", ALL_SPECS, ids=lambda spec: spec.id) @@ -160,3 +211,36 @@ async def test_live_gemini_batch_embed_survives_batch_split( assert len(embeddings) == len(BATCH_TEXTS) assert len({tuple(embedding) for embedding in embeddings}) == len(BATCH_TEXTS) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("spec", representative_embedding_specs()) +async def test_live_embedding_timeout_reaches_the_client( + spec: LiveEmbeddingSpec, +) -> None: + """Configured embedding timeout lands on the provider SDK client.""" + client = make_embedding_client(spec, timeout=GENEROUS_TIMEOUT_SECONDS) + + embedding = await client.embed(BATCH_TEXTS[0]) + + assert len(embedding) == spec.dimensions + assert_embedding_timeout_on_client(client, spec.transport, GENEROUS_TIMEOUT_SECONDS) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("spec", representative_embedding_specs()) +async def test_live_tight_embedding_timeout_aborts_request( + spec: LiveEmbeddingSpec, +) -> None: + """A near-zero embedding timeout aborts before the provider can answer.""" + client = make_embedding_client(spec, timeout=TIGHT_TIMEOUT_SECONDS) + + started = time.monotonic() + with pytest.raises(EMBEDDING_TIMEOUT_EXCEPTIONS[spec.transport]): + await client.embed(BATCH_TEXTS[0]) + elapsed = time.monotonic() - started + + assert elapsed < TIGHT_TIMEOUT_WALL_CLOCK_LIMIT_SECONDS, ( + f"tight embedding timeout took {elapsed:.1f}s — client timeout " + f"likely not applied" + ) diff --git a/tests/llm/test_embedding_client.py b/tests/llm/test_embedding_client.py index b50a66f0..9df80be1 100644 --- a/tests/llm/test_embedding_client.py +++ b/tests/llm/test_embedding_client.py @@ -65,7 +65,13 @@ async def test_openai_embedding_client_uses_configured_model_and_dimensions( fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 8) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.api_key: str | None = api_key self.base_url: str | None = base_url self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings @@ -104,7 +110,13 @@ async def test_openai_embedding_client_rejects_dimension_mismatch( fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 7) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) @@ -220,6 +232,118 @@ async def test_gemini_embedding_client_keeps_timeout_without_base_url( assert gemini_client.http_options.timeout == 600_000 +@pytest.mark.asyncio +async def test_openai_embedding_client_forwards_timeout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Configured embedding timeout reaches the OpenAI-compatible client.""" + + class FakeOpenAIClient: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: + self.api_key: str | None = api_key + self.base_url: str | None = base_url + self.timeout: float | None = timeout + self.embeddings: FakeOpenAIEmbeddingsAPI = FakeOpenAIEmbeddingsAPI( + [0.1] * 8 + ) + + monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) + + client = _EmbeddingClient( + EmbeddingModelConfig( + transport="openai", + model="text-embedding-3-small", + api_key="test-key", + timeout=45, + ), + vector_dimensions=8, + max_input_tokens=8192, + max_tokens_per_request=300_000, + send_dimensions=False, + ) + + openai_client = cast(Any, client.client) + assert openai_client.timeout == 45.0 + + +@pytest.mark.asyncio +async def test_openai_embedding_client_omits_timeout_when_unset( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Unset timeout omits the kwarg so the OpenAI SDK keeps its default.""" + + missing = object() + + class FakeOpenAIClient: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: object = missing, + ) -> None: + self.api_key: str | None = api_key + self.base_url: str | None = base_url + self.timeout: object = timeout + self.embeddings: FakeOpenAIEmbeddingsAPI = FakeOpenAIEmbeddingsAPI( + [0.1] * 8 + ) + + monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) + + client = _EmbeddingClient( + EmbeddingModelConfig( + transport="openai", + model="text-embedding-3-small", + api_key="test-key", + ), + vector_dimensions=8, + max_input_tokens=8192, + max_tokens_per_request=300_000, + send_dimensions=False, + ) + + openai_client = cast(Any, client.client) + assert openai_client.timeout is missing + + +@pytest.mark.asyncio +async def test_gemini_embedding_client_forwards_timeout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Configured embedding timeout reaches Gemini as milliseconds.""" + + class FakeGeminiClient: + def __init__(self, *, api_key: str | None, http_options: Any) -> None: + self.api_key: str | None = api_key + self.http_options: Any = http_options + self.aio: Any = SimpleNamespace(models=SimpleNamespace()) + + monkeypatch.setattr("google.genai.Client", FakeGeminiClient) + + client = _EmbeddingClient( + EmbeddingModelConfig( + transport="gemini", + model="gemini-embedding-001", + api_key="gemini-key", + timeout=45, + ), + vector_dimensions=8, + max_input_tokens=4096, + max_tokens_per_request=300_000, + send_dimensions=False, + ) + + gemini_client = cast(Any, client.client) + assert gemini_client.http_options.timeout == 45_000 + + def _build_openai_client( monkeypatch: pytest.MonkeyPatch, *, @@ -233,7 +357,13 @@ def _build_openai_client( fake_embeddings = FakeOpenAIEmbeddingsAPI(embedding) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.api_key: str | None = api_key self.base_url: str | None = base_url self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings @@ -707,7 +837,13 @@ async def test_simple_batch_embed_respects_token_budget_per_request( fake_embeddings = FakeOpenAIEmbeddingsAPI([0.5] * 4) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) @@ -745,7 +881,13 @@ async def test_simple_batch_embed_rejects_oversized_input( fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 4) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) @@ -775,7 +917,13 @@ def test_prepare_chunks_returns_ordered_chunks( fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 4) class FakeOpenAIClient: - def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + def __init__( + self, + *, + api_key: str | None, + base_url: str | None, + timeout: float | None = None, + ) -> None: self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) @@ -818,6 +966,31 @@ def test_embedding_model_config_parses_max_batch_size_from_env( assert resolved.max_batch_size == 10 +def test_embedding_model_config_parses_timeout_from_env( + monkeypatch: pytest.MonkeyPatch, +) -> None: + s = _build_embedding_settings( + {"EMBEDDING_MODEL_CONFIG__TIMEOUT": "90.0"}, + monkeypatch, + ) + + assert s.MODEL_CONFIG.timeout == 90.0 + + resolved = resolve_embedding_model_config(s.MODEL_CONFIG) + assert resolved.timeout == 90.0 + + +def test_embedding_model_config_rejects_invalid_timeout() -> None: + with pytest.raises( + ValueError, match=r"provider_params\.timeout must be a positive number" + ): + EmbeddingModelConfig( + transport="openai", + model="text-embedding-3-small", + timeout=-1, + ) + + @pytest.mark.asyncio async def test_gemini_process_batch_wraps_contents_as_content_part( monkeypatch: pytest.MonkeyPatch, From c2d8cf3a724a1584e5748c0a127aac8e68a4f2a5 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:48:29 -0400 Subject: [PATCH 2/8] Scopes SDK Changes (#1030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: scopes SDK surface and session allowlist on session context Exposes the Scopes v1 facade in both SDKs, which until now was reachable only by hand-rolled HTTP, and closes the Phase 1 gap where the session allowlist never landed on the context route. SDKs (DEV-2001, folds in DEV-1996) New Scope class in both SDKs — addSessions / removeSession / sessions / status — plus honcho.scope() and honcho.scopes() entry points, a `scopes` option on session creation, and `scope` + `sessions` read options on chat, chatStream, representation, and session.context. `scope` on workspace search. Python covers sync and .aio equally. `sessions` is sugar, not a new wire field: on the recall endpoints it goes out as the constrained `filters: {session_id: [...]}` body, never as a key of its own. Kept separate from the `filters` parameter on the list/search methods on purpose — that one is the full filter DSL, whereas the recall endpoints accept a single key and 422 on anything else, so one name for two grammars would be a trap. Server (DEV-2357) `GET /sessions/{id}/context` accepts a `sessions` allowlist confining the target's representation. Two deliberate choices worth review: - Sent as a repeated query parameter rather than the `filters` body the issue specced. The route is a GET and `session_id` is the only supported key, so a JSON blob in a query string buys nothing. - The peer card is omitted under an allowlist. Cards key on (workspace, observer, observed) with no session dimension, so they cannot be narrowed; returning one would leak exactly what the allowlist exists to exclude. Same reasoning as ALLOWLIST_SAFE_LEVELS. `scope` needs no carve-out — it swaps the observer to the scope peer, so the card read is the scope's own. `extract_session_allowlist` now delegates to a shared `normalize_session_allowlist`, so the cap, id charset, and must_include rule have one implementation across both entry points. Existing error messages are unchanged. Also in here - ConclusionScope renamed to ConclusionsView in both SDKs. "Scope" now means a named set of sessions, which that class is not — it is a view over one observer/observed pair. ConclusionScope kept as a deprecated alias; the package-level import path only. - The TS HTTP client comma-joined array query params, so any list-valued parameter arrived as one malformed entry. Fixed at buildURL rather than the call site. Not in this PR: the "How Scopes Work" docs guide and the Groudon dashboard tab (both DEV-2001), and CHANGELOG entries for Phases 2a-2c, which are still merged-but-unrecorded. Verified: ruff, basedpyright, tsc --noEmit, biome all clean. New unit tests cover the SDK option translation and the Scope client, but the context route's own behavior — the 422s, the 401 membership gate, the dropped peer card — has no test yet; the analogous chat/representation cases in tests/test_session_allowlist.py are the place for it. Refs DEV-2001, DEV-1996, DEV-2357 Co-Authored-By: Claude Opus 5 (1M context) Exposes the Scopes v1 facade in both SDKs, which until now was reachable only by hand-rolled HTTP, and closes the Phase 1 gap where the session allowlist never landed on the context route. SDKs (DEV-2001, folds in DEV-1996) New Scope class in both SDKs — addSessions / removeSession / sessions / status — plus honcho.scope() and honcho.scopes() entry points, a `scopes` option on session creation, and `scope` + `sessions` read options on chat, chatStream, representation, and session.context. `scope` on workspace search. Python covers sync and .aio equally. `sessions` is sugar, not a new wire field: on the recall endpoints it goes out as the constrained `filters: {session_id: [...]}` body, never as a key of its own. Kept separate from the `filters` parameter on the list/search methods on purpose — that one is the full filter DSL, whereas the recall endpoints accept a single key and 422 on anything else, so one name for two grammars would be a trap. Server (DEV-2357) `GET /sessions/{id}/context` accepts a `sessions` allowlist confining the target's representation. Two deliberate choices worth review: - Sent as a repeated query parameter rather than the `filters` body the issue specced. The route is a GET and `session_id` is the only supported key, so a JSON blob in a query string buys nothing. - The peer card is omitted under an allowlist. Cards key on (workspace, observer, observed) with no session dimension, so they cannot be narrowed; returning one would leak exactly what the allowlist exists to exclude. Same reasoning as ALLOWLIST_SAFE_LEVELS. `scope` needs no carve-out — it swaps the observer to the scope peer, so the card read is the scope's own. `extract_session_allowlist` now delegates to a shared `normalize_session_allowlist`, so the cap, id charset, and must_include rule have one implementation across both entry points. Existing error messages are unchanged. Also in here - ConclusionScope renamed to ConclusionsView in both SDKs. "Scope" now means a named set of sessions, which that class is not — it is a view over one observer/observed pair. ConclusionScope kept as a deprecated alias; the package-level import path only. - The TS HTTP client comma-joined array query params, so any list-valued parameter arrived as one malformed entry. Fixed at buildURL rather than the call site * fix(scopes): close peer-card leak under limit_to_session, harden SDK inputs Addresses review findings on the scopes work. All four were verified by reproducing them, not by reading. Peer card no longer leaks under any allowlist The card was dropped when `sessions` was set but returned when `limit_to_session=true` produced the identical allowlist, so a control meant to fail closed was defeated by swapping one query parameter. It is now gated on the effective allowlist, computed once and shared by the representation call and the card read — the duplicated inline conditional is what let the two drift apart. `POST /peers/{id}/chat` still injects an unscoped card under an allowlist (src/dialectic/chat.py fetches it on peer_card.use alone, with no reference to session_allowlist). Left alone deliberately: that is a behavior change to the shipped dialectic and Scope validation messages survive the option union ScopeOptionSchema is a union, and Zod collapses a failing union into one `invalid_union` / "Invalid input" issue, burying the branch errors. Every invalid scope on chat/representation reported "Invalid input" and told the caller nothing — including the reserved-prefix case the check order exists to surface. The rules are now a plain function applied after the union resolves, so the specific message reaches the caller for bad charset, reserved prefix, empty and over-cap lists alike. Empty scope no longer fails open `session.context({scope: ''})` and `honcho.search(q, {scope: ''})` used truthiness checks, so an invalid scope was dropped and the call returned *unscoped* results. Both now test against undefined so the value reaches the schema. Session IDs validated before reaching a URL path `scope.removeSession('valid-session?typo')` addressed `valid-session` with a stray query string: the wrong session removed, and reconciliation run against it. Both SDKs now validate the charset first. Python's `add_sessions` was unvalidated too — harmless in a JSON body, but leaving one path checked and its sibling unchecked is how this recurs. Also - Corrected the `limit_to_session` description: it claimed "only used if search_query is provided", but the allowlist reaches _query_documents_recent unconditionally. - Corrected the documented 1,000-session cap on `sessions`, which is unreachable via repeated query params — the request line exceeds h11's 16 KB and nginx's 8 KB defaults at a few hundred entries, giving an opaque 414/431 instead of a 422. - Removed a dead route builder. Tests: 7 new TypeScript cases and 2 new Python classes covering all four findings. The TypeScript unit suite passes 146/146. The context route itself is still unexercised — the card gate and the 401 membership check remain verified by reading only. Refs DEV-2001, DEV-1996, DEV-2357, DEV-2201 Co-Authored-By: Claude Opus 5 (1M context) * fix(sdk): align the conclusions-view error message across both SDKs The ConclusionScope -> ConclusionsView rename updated the identifier but not the prose inside the thrown message, which the rename pattern (\bConclusionScope\b) does not match. TypeScript ended up throwing "managed by this conclusions view" while Python still threw "managed by this conclusion scope" — the same error, different text per SDK. Three server-backed conclusions.test.ts cases assert that message by regex and failed under `pytest -k typescript`. The four equivalent Python assertions were passing, because they matched Python's unchanged string — so fixing only the TypeScript tests would have made the suite green with the divergence still in place. Brings Python's message, comments and docstring in line with TypeScript, and updates the assertions in both suites. `grep -ri 'conclusion scope'` is now empty. Verified: 64 passed across tests/sdk_typescript/, tests/sdk/test_conclusions.py and tests/sdk/test_scope_options.py — the last of which had never been run. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- sdks/python/src/honcho/__init__.py | 33 +- sdks/python/src/honcho/aio.py | 341 +++++++++++++-- sdks/python/src/honcho/api_types.py | 39 ++ sdks/python/src/honcho/base.py | 17 + sdks/python/src/honcho/client.py | 124 +++++- sdks/python/src/honcho/conclusions.py | 40 +- sdks/python/src/honcho/http/routes.py | 25 ++ sdks/python/src/honcho/peer.py | 75 +++- sdks/python/src/honcho/scope.py | 233 +++++++++++ sdks/python/src/honcho/session.py | 29 +- sdks/python/src/honcho/utils/__init__.py | 16 + sdks/python/src/honcho/utils/resolve.py | 8 +- sdks/python/src/honcho/utils/scopes.py | 296 +++++++++++++ sdks/typescript/__tests__/conclusions.test.ts | 24 +- sdks/typescript/__tests__/http-client.test.ts | 33 ++ sdks/typescript/__tests__/peer.test.ts | 4 +- sdks/typescript/__tests__/scope.unit.test.ts | 389 ++++++++++++++++++ sdks/typescript/src/client.ts | 145 +++++++ sdks/typescript/src/conclusions.ts | 34 +- sdks/typescript/src/http/client.ts | 36 +- sdks/typescript/src/index.ts | 16 +- sdks/typescript/src/peer.ts | 68 ++- sdks/typescript/src/scope.ts | 278 +++++++++++++ sdks/typescript/src/session.ts | 32 +- sdks/typescript/src/types/api.ts | 22 + sdks/typescript/src/validation.ts | 246 +++++++++++ src/deriver/queue_manager.py | 3 +- src/routers/sessions.py | 99 ++++- src/utils/filter.py | 42 +- tests/sdk/test_conclusions.py | 18 +- tests/sdk/test_scope_options.py | 236 +++++++++++ 31 files changed, 2841 insertions(+), 160 deletions(-) create mode 100644 sdks/python/src/honcho/scope.py create mode 100644 sdks/python/src/honcho/utils/scopes.py create mode 100644 sdks/typescript/__tests__/scope.unit.test.ts create mode 100644 sdks/typescript/src/scope.ts create mode 100644 tests/sdk/test_scope_options.py diff --git a/sdks/python/src/honcho/__init__.py b/sdks/python/src/honcho/__init__.py index 239f35ef..a07bf35f 100644 --- a/sdks/python/src/honcho/__init__.py +++ b/sdks/python/src/honcho/__init__.py @@ -41,11 +41,16 @@ from importlib.metadata import PackageNotFoundError, version from pathlib import Path import re -from .aio import ConclusionScopeAio, HonchoAio, PeerAio, SessionAio -from .api_types import MessageCreateParams -from .base import PeerBase, SessionBase +from .aio import ConclusionsViewAio, HonchoAio, PeerAio, ScopeAio, SessionAio +from .api_types import ( + MessageCreateParams, + ScopeBackfillJob, + ScopeResponse, + ScopeStatusResponse, +) +from .base import PeerBase, ScopeBase, SessionBase from .client import Honcho -from .conclusions import Conclusion, ConclusionScope +from .conclusions import Conclusion, ConclusionsView from .http.exceptions import ( APIError, AuthenticationError, @@ -63,6 +68,7 @@ from .http.exceptions import ( from .message import Message from .pagination import AsyncPage, SyncPage from .peer import Peer +from .scope import Scope from .session import Session from .session_context import SessionContext, SessionSummaries, Summary from .types import ( @@ -70,6 +76,12 @@ from .types import ( DialecticStreamResponse, ) +# Deprecated aliases. "Scope" now means a named set of sessions (see `Scope`), +# which these are not — they are views over one observer/observed pair. Kept for +# one more minor version. +ConclusionScope = ConclusionsView +ConclusionScopeAio = ConclusionsViewAio + def _detect_version() -> str: try: @@ -95,23 +107,32 @@ __all__ = [ "Honcho", # Domain classes "Conclusion", - "ConclusionScope", + "ConclusionsView", "Message", "MessageCreateParams", "Peer", + "Scope", "Session", # Aio views (for type hints) - "ConclusionScopeAio", + "ConclusionsViewAio", "HonchoAio", "PeerAio", + "ScopeAio", "SessionAio", # Base classes "PeerBase", + "ScopeBase", "SessionBase", # Response types + "ScopeBackfillJob", + "ScopeResponse", + "ScopeStatusResponse", "SessionContext", "SessionSummaries", "Summary", + # Deprecated aliases + "ConclusionScope", + "ConclusionScopeAio", # Pagination "AsyncPage", "SyncPage", diff --git a/sdks/python/src/honcho/aio.py b/sdks/python/src/honcho/aio.py index 51797c72..29c0f445 100644 --- a/sdks/python/src/honcho/aio.py +++ b/sdks/python/src/honcho/aio.py @@ -3,7 +3,7 @@ This module provides async accessor classes that wrap the main SDK classes and provide async versions of all operations. Access via the `.aio` property -on Honcho, Peer, Session, and ConclusionScope instances. +on Honcho, Peer, Session, and ConclusionsView instances. Example: ```python @@ -24,7 +24,7 @@ from __future__ import annotations import json import logging import warnings -from collections.abc import AsyncGenerator +from collections.abc import AsyncGenerator, Sequence from datetime import datetime from typing import TYPE_CHECKING, Any, ClassVar, Literal, overload @@ -40,15 +40,18 @@ from .api_types import ( PeerResponse, QueueStatusResponse, RepresentationResponse, + ScopeBackfillJob, + ScopeResponse, + ScopeStatusResponse, SessionConfiguration, SessionPeerConfig, SessionResponse, WorkspaceConfiguration, WorkspaceResponse, ) -from .base import PeerBase, SessionBase +from .base import PeerBase, ScopeBase, SessionBase from .conclusions import ( - _SCOPE_RESERVED, + _VIEW_RESERVED, Conclusion, _reject_reserved_filter_keys, ) @@ -64,14 +67,20 @@ from .utils import ( parse_sse_astream, prepare_file_for_upload, resolve_id, + resolve_scope_membership, + resolve_scope_session, + scope_context_fields, + scope_recall_fields, + validate_scope_id, ) if TYPE_CHECKING: from .client import Honcho - from .conclusions import ConclusionScope + from .conclusions import ConclusionsView from .conclusions import ConclusionCreateParams from .peer import Peer, TResponseFormat, serialize_response_format +from .scope import Scope from .session import Session logger = logging.getLogger(__name__) @@ -79,8 +88,9 @@ logger = logging.getLogger(__name__) __all__ = [ "HonchoAio", "PeerAio", + "ScopeAio", "SessionAio", - "ConclusionScopeAio", + "ConclusionsViewAio", ] @@ -267,6 +277,7 @@ class HonchoAio(AsyncMetadataConfigMixin): | list[tuple[PeerBase | str, SessionPeerConfig]] | list[PeerBase | str | tuple[PeerBase | str, SessionPeerConfig]] | None = None, + scopes: Sequence[str | ScopeBase] | None = None, ) -> Session: """ Get or create a session with the given ID asynchronously. @@ -278,6 +289,11 @@ class HonchoAio(AsyncMetadataConfigMixin): peers: Optional peers to attach to the session at creation. Accepts the same shape as Session.add_peers (peer ID string, Peer object, list of either, or tuples with SessionPeerConfig). + scopes: Optional scopes this session should join, as IDs or Scope + objects. Each scope is created if it does not exist yet. Attaching + at creation avoids the asynchronous backfill a later + ``scope.add_sessions()`` triggers, since there is no history to + copy. Returns: A Session object with cached values from the API response. @@ -290,6 +306,8 @@ class HonchoAio(AsyncMetadataConfigMixin): body["configuration"] = configuration.model_dump(exclude_none=True) if peers is not None: body["peers"] = normalize_peers_to_dict(peers) + if scopes is not None: + body["scopes"] = [validate_scope_id(resolve_id(scope)) for scope in scopes] data = await self._honcho._async_http_client.post( routes.sessions(self._honcho.workspace_id), body=body @@ -358,6 +376,88 @@ class HonchoAio(AsyncMetadataConfigMixin): return AsyncPage(data, SessionResponse, transform, fetch_next) + async def scope( + self, + id: str, # noqa: A002 + *, + metadata: dict[str, object] | None = None, + ) -> Scope: + """ + Get or create a scope with the given ID asynchronously. + + A scope is a named set of sessions that acts as a visibility boundary: + recall performed through the scope sees only what happened in its sessions, + while the underlying peer keeps its single unified representation of + everything. + + Args: + id: Unprefixed scope name, unique within the workspace. + metadata: Optional metadata dictionary to associate with this scope. + + Returns: + A Scope object for managing membership. + + Raises: + ValueError: If the scope ID is invalid. + """ + validate_scope_id(id) + await self._honcho._ensure_workspace_async() + body: dict[str, Any] = {"id": id} + if metadata is not None: + body["metadata"] = metadata + + data = await self._honcho._async_http_client.post( + routes.scopes(self._honcho.workspace_id), body=body + ) + scope_data = ScopeResponse.model_validate(data) + return Scope( + id, + self._honcho, + metadata=scope_data.metadata, + created_at=scope_data.created_at, + ) + + async def scopes( + self, + *, + page: int = 1, + size: int = 50, + reverse: bool = False, + ) -> AsyncPage[ScopeResponse, Scope]: + """ + Get all scopes in the current workspace asynchronously. + + Args: + page: Page number (1-indexed). Default: 1. + size: Number of items per page. Default: 50. + reverse: If True, reverses the default ordering. Default: False. + """ + await self._honcho._ensure_workspace_async() + + async def fetch(next_page: int) -> dict[str, Any]: + query: dict[str, Any] = {"page": next_page, "size": size} + if reverse: + query["reverse"] = "true" + return await self._honcho._async_http_client.post( + routes.scopes_list(self._honcho.workspace_id), query=query + ) + + def transform(scope: ScopeResponse) -> Scope: + """Convert a scope API response into a Scope SDK object.""" + return Scope( + scope.id, + self._honcho, + metadata=scope.metadata, + created_at=scope.created_at, + ) + + async def fetch_next(next_page: int) -> AsyncPage[ScopeResponse, Scope]: + return AsyncPage( + await fetch(next_page), ScopeResponse, transform, fetch_next + ) + + return AsyncPage(await fetch(page), ScopeResponse, transform, fetch_next) + async def workspaces( self, filters: dict[str, object] | None = None, @@ -409,12 +509,27 @@ class HonchoAio(AsyncMetadataConfigMixin): limit: int = Field( default=10, ge=1, le=100, description="Number of results to return" ), + *, + scope: str | ScopeBase | None = None, ) -> list[Message]: - """Search for messages in the current workspace asynchronously.""" + """Search for messages in the current workspace asynchronously. + + Args: + query: The search query to use + filters: Filters to scope the search. + limit: Number of results to return (1-100, default: 10) + scope: Optional scope (ID or Scope object) restricting the search to + that scope's member sessions. Mutually exclusive with a + ``session_id`` filter. A scope with no member sessions matches + nothing rather than everything. + """ await self._honcho._ensure_workspace_async() + body: dict[str, Any] = {"query": query, "filters": filters, "limit": limit} + if scope is not None: + body["scope"] = validate_scope_id(resolve_id(scope)) data = await self._honcho._async_http_client.post( routes.workspace_search(self._honcho.workspace_id), - body={"query": query, "filters": filters, "limit": limit}, + body=body, ) return [ Message.from_api_response(MessageResponse.model_validate(item)) @@ -584,6 +699,8 @@ class PeerAio(AsyncMetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[TResponseFormat], @@ -596,6 +713,8 @@ class PeerAio(AsyncMetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: dict[str, Any] | None = None, @@ -608,6 +727,8 @@ class PeerAio(AsyncMetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[BaseModel] | dict[str, Any] | None = None, @@ -623,6 +744,11 @@ class PeerAio(AsyncMetadataConfigMixin): resolved_session_id = resolve_id(session) body: dict[str, Any] = {"query": query, "stream": False} + body.update( + scope_recall_fields( + scope=scope, sessions=sessions, session_id=resolved_session_id + ) + ) if target_id: body["target"] = target_id if resolved_session_id: @@ -651,6 +777,8 @@ class PeerAio(AsyncMetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[BaseModel] | dict[str, Any] | None = None, @@ -666,6 +794,11 @@ class PeerAio(AsyncMetadataConfigMixin): resolved_session_id = resolve_id(session) body: dict[str, Any] = {"query": query, "stream": True} + body.update( + scope_recall_fields( + scope=scope, sessions=sessions, session_id=resolved_session_id + ) + ) if target_id: body["target"] = target_id if resolved_session_id: @@ -826,13 +959,22 @@ class PeerAio(AsyncMetadataConfigMixin): search_max_distance: float | None = Field(None, ge=0.0, le=1.0), include_most_frequent: bool | None = None, max_conclusions: int | None = Field(None, ge=1, le=100), + *, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, ) -> str: - """Get a subset of the representation of the peer asynchronously.""" + """Get a subset of the representation of the peer asynchronously. + + See Peer.representation for parameter details, including the depth caveat + on ``sessions``. + """ await self._peer._honcho._ensure_workspace_async() session_id = resolve_id(session) target_id = resolve_id(target) - body: dict[str, Any] = {} + body: dict[str, Any] = scope_recall_fields( + scope=scope, sessions=sessions, session_id=session_id + ) if session_id: body["session_id"] = session_id if target_id: @@ -1192,6 +1334,14 @@ class SessionAio(AsyncMetadataConfigMixin): None, description="A peer ID to get context from the perspective of.", ), + scope: str | ScopeBase | None = Field( + None, + description="A scope to use as the perspective source instead of a peer.", + ), + sessions: Sequence[str | SessionBase] | None = Field( + None, + description="An allowlist of sessions confining `peer_target`'s representation to that set. This session must be one of them.", + ), limit_to_session: bool = Field( False, description="Whether to limit the representation to this session only.", @@ -1219,7 +1369,11 @@ class SessionAio(AsyncMetadataConfigMixin): description="Maximum number of conclusions to include in the representation.", ), ) -> SessionContext: - """Get optimized context for this session asynchronously.""" + """Get optimized context for this session asynchronously. + + See Session.context for parameter details, including the depth caveat on + ``sessions``. + """ await self._session._honcho._ensure_workspace_async() if peer_target is None and peer_perspective is not None: raise ValueError( @@ -1238,6 +1392,13 @@ class SessionAio(AsyncMetadataConfigMixin): query: dict[str, Any] = { "summary": summary, "limit_to_session": limit_to_session, + **scope_context_fields( + scope=scope, + sessions=sessions, + peer_target=peer_target, + peer_perspective=peer_perspective, + limit_to_session=limit_to_session, + ), } if tokens is not None: query["tokens"] = tokens @@ -1488,19 +1649,19 @@ class SessionAio(AsyncMetadataConfigMixin): return Message.from_api_response(MessageResponse.model_validate(data)) -class ConclusionScopeAio: +class ConclusionsViewAio: """ - Async view of a ConclusionScope. + Async view of a ConclusionsView. - Access via `scope.aio`. Provides async versions of all ConclusionScope methods. - Shares state with the parent ConclusionScope instance. + Access via `view.aio`. Provides async versions of all ConclusionsView methods. + Shares state with the parent ConclusionsView instance. """ - __slots__: ClassVar[tuple[str, ...]] = ("_scope",) - _scope: "ConclusionScope" + __slots__: ClassVar[tuple[str, ...]] = ("_view",) + _view: "ConclusionsView" - def __init__(self, scope: "ConclusionScope") -> None: - self._scope = scope + def __init__(self, view: "ConclusionsView") -> None: + self._view = view async def list( self, @@ -1520,13 +1681,13 @@ class ConclusionScopeAio: https://honcho.dev/docs/v3/documentation/features/advanced/using-filters """ _reject_reserved_filter_keys( - filters, _SCOPE_RESERVED + ("session", "session_id") + filters, _VIEW_RESERVED + ("session", "session_id") ) - await self._scope._honcho._ensure_workspace_async() + await self._view._honcho._ensure_workspace_async() resolved_session_id = resolve_id(session) filters = { - "observer_id": self._scope.observer, - "observed_id": self._scope.observed, + "observer_id": self._view.observer, + "observed_id": self._view.observed, **({"session_id": resolved_session_id} if resolved_session_id else {}), **(filters or {}), } @@ -1534,8 +1695,8 @@ class ConclusionScopeAio: query: dict[str, Any] = {"page": page, "size": size} if reverse: query["reverse"] = "true" - data = await self._scope._honcho._async_http_client.post( - routes.conclusions_list(self._scope.workspace_id), + data = await self._view._honcho._async_http_client.post( + routes.conclusions_list(self._view.workspace_id), body={"filters": filters}, query=query, ) @@ -1549,8 +1710,8 @@ class ConclusionScopeAio: next_query: dict[str, Any] = {"page": next_page, "size": size} if reverse: next_query["reverse"] = "true" - next_data = await self._scope._honcho._async_http_client.post( - routes.conclusions_list(self._scope.workspace_id), + next_data = await self._view._honcho._async_http_client.post( + routes.conclusions_list(self._view.workspace_id), body={"filters": filters}, query=next_query, ) @@ -1575,11 +1736,11 @@ class ConclusionScopeAio: filters: Optional dictionary of additional filter criteria, merged with this scope's observer/observed (e.g. ``{"level": "deductive"}``). """ - _reject_reserved_filter_keys(filters, _SCOPE_RESERVED) - await self._scope._honcho._ensure_workspace_async() + _reject_reserved_filter_keys(filters, _VIEW_RESERVED) + await self._view._honcho._ensure_workspace_async() filters = { - "observer_id": self._scope.observer, - "observed_id": self._scope.observed, + "observer_id": self._view.observer, + "observed_id": self._view.observed, **(filters or {}), } @@ -1591,8 +1752,8 @@ class ConclusionScopeAio: if distance is not None: body["distance"] = distance - data = await self._scope._honcho._async_http_client.post( - routes.conclusions_query(self._scope.workspace_id), + data = await self._view._honcho._async_http_client.post( + routes.conclusions_query(self._view.workspace_id), body=body, ) return [ @@ -1602,9 +1763,9 @@ class ConclusionScopeAio: async def delete(self, conclusion_id: str) -> None: """Delete a conclusion by ID asynchronously.""" - await self._scope._honcho._ensure_workspace_async() - await self._scope._honcho._async_http_client.delete( - routes.conclusion(self._scope.workspace_id, conclusion_id) + await self._view._honcho._ensure_workspace_async() + await self._view._honcho._async_http_client.delete( + routes.conclusion(self._view.workspace_id, conclusion_id) ) async def create( @@ -1612,15 +1773,15 @@ class ConclusionScopeAio: conclusions: list[ConclusionCreateParams | dict[str, Any]], ) -> list[Conclusion]: """Create conclusions in this scope asynchronously.""" - await self._scope._honcho._ensure_workspace_async() + await self._view._honcho._ensure_workspace_async() def build_conclusion_payload( item: ConclusionCreateParams | dict[str, Any], ) -> dict[str, Any]: """Build a single conclusion create payload.""" payload: dict[str, Any] = { - "observer_id": self._scope.observer, - "observed_id": self._scope.observed, + "observer_id": self._view.observer, + "observed_id": self._view.observed, } if isinstance(item, ConclusionCreateParams): payload["content"] = item.content @@ -1636,8 +1797,8 @@ class ConclusionScopeAio: conclusion_params = [build_conclusion_payload(c) for c in conclusions] - data = await self._scope._honcho._async_http_client.post( - routes.conclusions(self._scope.workspace_id), + data = await self._view._honcho._async_http_client.post( + routes.conclusions(self._view.workspace_id), body={"conclusions": conclusion_params}, ) return [ @@ -1654,8 +1815,8 @@ class ConclusionScopeAio: max_conclusions: int | None = None, ) -> str: """Get the computed representation for this scope asynchronously.""" - await self._scope._honcho._ensure_workspace_async() - body: dict[str, Any] = {"target": self._scope.observed} + await self._view._honcho._ensure_workspace_async() + body: dict[str, Any] = {"target": self._view.observed} if search_query is not None: body["search_query"] = search_query if search_top_k is not None: @@ -1667,9 +1828,99 @@ class ConclusionScopeAio: if max_conclusions is not None: body["max_conclusions"] = max_conclusions - data = await self._scope._honcho._async_http_client.post( - routes.peer_representation(self._scope.workspace_id, self._scope.observer), + data = await self._view._honcho._async_http_client.post( + routes.peer_representation(self._view.workspace_id, self._view.observer), body=body, ) response = RepresentationResponse.model_validate(data) return response.representation + + +class ScopeAio: + """ + Async view of a Scope. + + Access via `scope.aio`. Provides async versions of all Scope methods. + Shares state with the parent Scope instance. + """ + + __slots__: ClassVar[tuple[str, ...]] = ("_scope",) + _scope: "Scope" + + def __init__(self, scope: "Scope") -> None: + """Create an async view backed by a sync Scope.""" + self._scope = scope + + @validate_call(config=ConfigDict(arbitrary_types_allowed=True)) + async def add_sessions(self, sessions: Sequence[str | SessionBase]) -> None: + """Add sessions to this scope asynchronously. + + See Scope.add_sessions for details, including the asynchronous backfill + that sessions with existing messages trigger. + """ + session_ids = resolve_scope_membership(sessions) + await self._scope._honcho._ensure_workspace_async() + await self._scope._honcho._async_http_client.post( + routes.scope_sessions(self._scope.workspace_id, self._scope.id), + body={"session_ids": session_ids}, + ) + + @validate_call(config=ConfigDict(arbitrary_types_allowed=True)) + async def remove_session(self, session: str | SessionBase) -> None: + """Remove a session from this scope asynchronously. + + See Scope.remove_session for details on the asynchronous reconciliation. + """ + await self._scope._honcho._ensure_workspace_async() + await self._scope._honcho._async_http_client.delete( + routes.scope_session( + self._scope.workspace_id, self._scope.id, resolve_scope_session(session) + ) + ) + + async def sessions( + self, + page: int = 1, + size: int = 50, + *, + reverse: bool = False, + ) -> AsyncPage[SessionResponse, Session]: + """Get the sessions that are members of this scope asynchronously.""" + await self._scope._honcho._ensure_workspace_async() + + async def fetch(next_page: int) -> dict[str, Any]: + query: dict[str, Any] = {"page": next_page, "size": size} + if reverse: + query["reverse"] = "true" + return await self._scope._honcho._async_http_client.post( + routes.scope_sessions_list(self._scope.workspace_id, self._scope.id), + query=query, + ) + + def transform(response: SessionResponse) -> Session: + return Session( + response.id, + self._scope._honcho, + metadata=response.metadata, + configuration=response.configuration, + created_at=response.created_at, + is_active=response.is_active, + ) + + async def fetch_next(next_page: int) -> AsyncPage[SessionResponse, Session]: + return AsyncPage( + await fetch(next_page), SessionResponse, transform, fetch_next + ) + + return AsyncPage(await fetch(page), SessionResponse, transform, fetch_next) + + async def status(self) -> dict[str, ScopeBackfillJob]: + """Get the backfill/reconciliation progress for this scope asynchronously. + + See Scope.status for details. + """ + await self._scope._honcho._ensure_workspace_async() + data = await self._scope._honcho._async_http_client.get( + routes.scope_status(self._scope.workspace_id, self._scope.id) + ) + return ScopeStatusResponse.model_validate(data).backfill_status diff --git a/sdks/python/src/honcho/api_types.py b/sdks/python/src/honcho/api_types.py index f626c5a6..23692f2f 100644 --- a/sdks/python/src/honcho/api_types.py +++ b/sdks/python/src/honcho/api_types.py @@ -276,6 +276,7 @@ class SessionCreateParams(BaseModel): metadata: dict[str, Any] | None = None peers: dict[str, SessionPeerConfig] | None = None configuration: SessionConfiguration | None = None + scopes: list[str] | None = None class SessionUpdateParams(BaseModel): @@ -295,6 +296,44 @@ class SessionListParams(BaseModel): filters: dict[str, Any] | None = None +# ============================================================================== +# Scope Types +# ============================================================================== + + +class ScopeResponse(BaseModel): + """Scope API response.""" + + model_config = ConfigDict(populate_by_name=True) # pyright: ignore[reportUnannotatedClassAttribute] + + id: str + metadata: dict[str, Any] = Field(default_factory=dict) + created_at: datetime.datetime + + +class ScopeBackfillJob(BaseModel): + """Backfill job state for one session in a scope. + + ``docs_copied`` is present only once the backfill for that session completes. + """ + + model_config = ConfigDict(extra="ignore") # pyright: ignore[reportUnannotatedClassAttribute] + + state: Literal["pending", "completed", "failed"] + updated_at: datetime.datetime + docs_copied: int | None = None + + +class ScopeStatusResponse(BaseModel): + """Scope backfill/reconciliation status API response. + + ``backfill_status`` is keyed by session ID and only contains sessions that + have had a backfill enqueued. + """ + + backfill_status: dict[str, ScopeBackfillJob] = Field(default_factory=dict) + + # ============================================================================== # Summary Types # ============================================================================== diff --git a/sdks/python/src/honcho/base.py b/sdks/python/src/honcho/base.py index a27a64ad..fb80b121 100644 --- a/sdks/python/src/honcho/base.py +++ b/sdks/python/src/honcho/base.py @@ -43,3 +43,20 @@ class SessionBase(BaseModel): workspace_id: str = Field( ..., min_length=1, description="Workspace ID for scoping operations" ) + + +class ScopeBase(BaseModel): + """Base class for Scope objects (sync and async variants). + + Use this type in method signatures to accept either a scope ID string or any + Scope object. + + Attributes: + id: Unprefixed scope name, unique within the workspace + workspace_id: Workspace ID for scoping operations + """ + + id: str = Field(..., min_length=1, description="Unprefixed name of this scope") + workspace_id: str = Field( + ..., min_length=1, description="Workspace ID for scoping operations" + ) diff --git a/sdks/python/src/honcho/client.py b/sdks/python/src/honcho/client.py index 06a2887c..1527792e 100644 --- a/sdks/python/src/honcho/client.py +++ b/sdks/python/src/honcho/client.py @@ -4,7 +4,7 @@ from __future__ import annotations import logging import os -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from typing import Any, Literal import httpx @@ -16,20 +16,22 @@ from .api_types import ( PeerConfig, PeerResponse, QueueStatusResponse, + ScopeResponse, SessionConfiguration, SessionPeerConfig, SessionResponse, WorkspaceConfiguration, WorkspaceResponse, ) -from .base import PeerBase, SessionBase +from .base import PeerBase, ScopeBase, SessionBase from .http import AsyncHonchoHTTPClient, HonchoHTTPClient, routes from .message import Message from .mixins import MetadataConfigMixin from .pagination import SyncPage from .peer import Peer +from .scope import Scope from .session import Session -from .utils import normalize_peers_to_dict, resolve_id +from .utils import normalize_peers_to_dict, resolve_id, validate_scope_id logger = logging.getLogger(__name__) @@ -419,6 +421,10 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul None, description="Optional peers to attach to the session at creation. Accepts the same shape as Session.add_peers.", ), + scopes: Sequence[str | ScopeBase] | None = Field( + None, + description="Optional scopes this session should join. Each scope is created if it does not exist yet.", + ), ) -> Session: """ Get or create a session with the given ID. @@ -433,6 +439,11 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul peers: Optional peers to attach to the session at creation. Accepts the same shape as Session.add_peers (peer ID string, Peer object, list of either, or tuples with SessionPeerConfig). + scopes: Optional scopes this session should join, as IDs or Scope + objects. Each scope is created if it does not exist yet. Attaching + at creation avoids the asynchronous backfill a later + ``scope.add_sessions()`` triggers, since there is no history to + copy. Returns: A Session object with cached metadata, configuration, created_at, and is_active. @@ -445,6 +456,8 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul body["configuration"] = configuration.model_dump(exclude_none=True) if peers is not None: body["peers"] = normalize_peers_to_dict(peers) + if scopes is not None: + body["scopes"] = [validate_scope_id(resolve_id(scope)) for scope in scopes] data = self._http.post(routes.sessions(self.workspace_id), body=body) session_data = SessionResponse.model_validate(data) @@ -514,6 +527,97 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul return SyncPage(data, SessionResponse, transform, fetch_next) + @validate_call + def scope( + self, + id: str = Field( # noqa: A002 + ..., min_length=1, description="Unprefixed name for the scope" + ), + *, + metadata: dict[str, object] | None = Field( + None, + description="Optional metadata dictionary to associate with this scope.", + ), + ) -> Scope: + """ + Get or create a scope with the given ID. + + A scope is a named set of sessions that acts as a visibility boundary: + recall performed through the scope sees only what happened in its sessions, + while the underlying peer keeps its single unified representation of + everything. + + Args: + id: Unprefixed scope name, unique within the workspace. + metadata: Optional metadata dictionary to associate with this scope. + + Returns: + A Scope object for managing membership. + + Raises: + ValueError: If the scope ID is invalid. + + Example: + ```python + therapy = honcho.scope("therapy") + therapy.add_sessions([session_1, session_2]) + ``` + """ + validate_scope_id(id) + self._ensure_workspace() + body: dict[str, Any] = {"id": id} + if metadata is not None: + body["metadata"] = metadata + + data = self._http.post(routes.scopes(self.workspace_id), body=body) + scope_data = ScopeResponse.model_validate(data) + return Scope( + id, + self, + metadata=scope_data.metadata, + created_at=scope_data.created_at, + ) + + def scopes( + self, + *, + page: int = 1, + size: int = 50, + reverse: bool = False, + ) -> SyncPage[ScopeResponse, Scope]: + """ + Get all scopes in the current workspace. + + Args: + page: Page number (1-indexed). Default: 1. + size: Number of items per page. Default: 50. + reverse: If True, reverses the default ordering. Default: False. + + Returns: + A SyncPage of Scope objects representing all scopes in the workspace. + """ + self._ensure_workspace() + + def fetch(next_page: int) -> dict[str, Any]: + query: dict[str, Any] = {"page": next_page, "size": size} + if reverse: + query["reverse"] = "true" + return self._http.post(routes.scopes_list(self.workspace_id), query=query) + + def transform(scope: ScopeResponse) -> Scope: + """Convert a scope API response into a Scope SDK object.""" + return Scope( + scope.id, + self, + metadata=scope.metadata, + created_at=scope.created_at, + ) + + def fetch_next(next_page: int) -> SyncPage[ScopeResponse, Scope]: + return SyncPage(fetch(next_page), ScopeResponse, transform, fetch_next) + + return SyncPage(fetch(page), ScopeResponse, transform, fetch_next) + def workspaces( self, filters: dict[str, object] | None = None, @@ -592,6 +696,11 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul limit: int = Field( default=10, ge=1, le=100, description="Number of results to return" ), + *, + scope: str | ScopeBase | None = Field( + None, + description="Optional scope restricting the search to its member sessions", + ), ) -> list[Message]: """ Search for messages in the current workspace. @@ -602,15 +711,22 @@ class Honcho(BaseModel, MetadataConfigMixin): # pyright: ignore[reportUnsafeMul query: The search query to use filters: Filters to scope the search. See [search filters documentation](https://honcho.dev/docs/v3/documentation/core-concepts/features/using-filters). limit: Number of results to return (1-100, default: 10) + scope: Optional scope (ID or Scope object) restricting the search to + that scope's member sessions. Mutually exclusive with a + ``session_id`` filter. A scope with no member sessions matches + nothing rather than everything. Returns: A list of Message objects representing the search results. Returns an empty list if no messages are found. """ self._ensure_workspace() + body: dict[str, Any] = {"query": query, "filters": filters, "limit": limit} + if scope is not None: + body["scope"] = validate_scope_id(resolve_id(scope)) data = self._http.post( routes.workspace_search(self.workspace_id), - body={"query": query, "filters": filters, "limit": limit}, + body=body, ) return [ Message.from_api_response(MessageResponse.model_validate(item)) diff --git a/sdks/python/src/honcho/conclusions.py b/sdks/python/src/honcho/conclusions.py index 708cc3ed..c9f6a6a9 100644 --- a/sdks/python/src/honcho/conclusions.py +++ b/sdks/python/src/honcho/conclusions.py @@ -15,28 +15,28 @@ from .pagination import SyncPage from .utils import resolve_id if TYPE_CHECKING: - from .aio import ConclusionScopeAio + from .aio import ConclusionsViewAio from .client import Honcho __all__ = [ "Conclusion", - "ConclusionScope", + "ConclusionsView", "ConclusionCreateParams", ] -# Filter keys that define a conclusion scope (the observer/observed peer pair). -# They are set from the scope itself, so a caller must not pass them in `filters`. -_SCOPE_RESERVED = ("observer", "observed", "observer_id", "observed_id") +# Filter keys that define a conclusions view (the observer/observed peer pair). +# They are set from the view itself, so a caller must not pass them in `filters`. +_VIEW_RESERVED = ("observer", "observed", "observer_id", "observed_id") def _reject_reserved_filter_keys( filters: dict[str, Any] | None, reserved: tuple[str, ...] ) -> None: - """Raise if ``filters`` contains keys managed by the conclusion scope. + """Raise if ``filters`` contains keys managed by the conclusions view. The observer/observed peer pair (and, on ``list``, the session) is fixed by - the scope, so letting a user filter override it would silently return data - from a different scope than requested. Fail loud instead. + the view, so letting a user filter override it would silently return data + from a different pair than requested. Fail loud instead. """ if not filters: return @@ -48,7 +48,7 @@ def _reject_reserved_filter_keys( if "session" in reserved or "session_id" in reserved: guidance += "; use the session= parameter to filter by session" raise ValueError( - f"Filter key(s) {clash} are managed by this conclusion scope and " + f"Filter key(s) {clash} are managed by this conclusions view and " + f"cannot be passed in filters. {guidance}." ) @@ -126,7 +126,7 @@ class Conclusion: return self.content -class ConclusionScope: +class ConclusionsView: """ Scoped access to conclusions for a specific observer/observed relationship. @@ -165,7 +165,7 @@ class ConclusionScope: observed: str, ): """ - Initialize a ConclusionScope. + Initialize a ConclusionsView. Args: honcho: The Honcho client instance @@ -179,12 +179,12 @@ class ConclusionScope: self.observed = observed @property - def aio(self) -> "ConclusionScopeAio": + def aio(self) -> "ConclusionsViewAio": """ - Access async versions of all ConclusionScope methods. + Access async versions of all ConclusionsView methods. - Returns a ConclusionScopeAio view that provides async versions of all methods - while sharing state with this ConclusionScope instance. + Returns a ConclusionsViewAio view that provides async versions of all methods + while sharing state with this ConclusionsView instance. Example: ```python @@ -194,9 +194,9 @@ class ConclusionScope: ``` """ # Import here to avoid circular import (aio.py imports from this module) - from .aio import ConclusionScopeAio + from .aio import ConclusionsViewAio - return ConclusionScopeAio(self) + return ConclusionsViewAio(self) def list( self, @@ -226,7 +226,7 @@ class ConclusionScope: Paginated response containing Conclusion objects """ _reject_reserved_filter_keys( - filters, _SCOPE_RESERVED + ("session", "session_id") + filters, _VIEW_RESERVED + ("session", "session_id") ) self._honcho._ensure_workspace() resolved_session_id = resolve_id(session) @@ -288,7 +288,7 @@ class ConclusionScope: Returns: List of matching Conclusion objects """ - _reject_reserved_filter_keys(filters, _SCOPE_RESERVED) + _reject_reserved_filter_keys(filters, _VIEW_RESERVED) self._honcho._ensure_workspace() filters = { "observer_id": self.observer, @@ -443,6 +443,6 @@ class ConclusionScope: def __repr__(self) -> str: return ( - f"ConclusionScope(workspace_id={self.workspace_id!r}, " + f"ConclusionsView(workspace_id={self.workspace_id!r}, " f"observer={self.observer!r}, observed={self.observed!r})" ) diff --git a/sdks/python/src/honcho/http/routes.py b/sdks/python/src/honcho/http/routes.py index 3fdbd677..8f9ee2fb 100644 --- a/sdks/python/src/honcho/http/routes.py +++ b/sdks/python/src/honcho/http/routes.py @@ -102,6 +102,31 @@ def session_peer_config(workspace_id: str, session_id: str, peer_id: str) -> str return f"/{API_VERSION}/workspaces/{workspace_id}/sessions/{session_id}/peers/{peer_id}/config" +# Scope routes +def scopes(workspace_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes" + + +def scopes_list(workspace_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes/list" + + +def scope_sessions(workspace_id: str, scope_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes/{scope_id}/sessions" + + +def scope_sessions_list(workspace_id: str, scope_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes/{scope_id}/sessions/list" + + +def scope_session(workspace_id: str, scope_id: str, session_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes/{scope_id}/sessions/{session_id}" + + +def scope_status(workspace_id: str, scope_id: str) -> str: + return f"/{API_VERSION}/workspaces/{workspace_id}/scopes/{scope_id}/status" + + # Message routes def messages(workspace_id: str, session_id: str) -> str: return f"/{API_VERSION}/workspaces/{workspace_id}/sessions/{session_id}/messages" diff --git a/sdks/python/src/honcho/peer.py b/sdks/python/src/honcho/peer.py index e004db6a..38edf269 100644 --- a/sdks/python/src/honcho/peer.py +++ b/sdks/python/src/honcho/peer.py @@ -6,7 +6,7 @@ from __future__ import annotations import datetime import logging import warnings -from collections.abc import Generator +from collections.abc import Generator, Sequence from typing import TYPE_CHECKING, Any, Literal, TypeVar, overload from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, validate_call @@ -22,14 +22,14 @@ from .api_types import ( SessionConfiguration, SessionResponse, ) -from .base import PeerBase, SessionBase -from .conclusions import ConclusionScope +from .base import PeerBase, ScopeBase, SessionBase +from .conclusions import ConclusionsView from .http import routes from .message import Message from .mixins import MetadataConfigMixin from .pagination import SyncPage from .types import DialecticStreamResponse -from .utils import parse_datetime, parse_sse_stream, resolve_id +from .utils import parse_datetime, parse_sse_stream, resolve_id, scope_recall_fields if TYPE_CHECKING: from .aio import PeerAio @@ -241,6 +241,8 @@ class Peer(PeerBase, MetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[TResponseFormat], @@ -253,6 +255,8 @@ class Peer(PeerBase, MetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: dict[str, Any] | None = None, @@ -265,6 +269,8 @@ class Peer(PeerBase, MetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[BaseModel] | dict[str, Any] | None = None, @@ -285,6 +291,19 @@ class Peer(PeerBase, MetadataConfigMixin): session: Optional session to scope the query to. If provided, only information from that session is considered. Can be a session ID string or a Session object. + scope: Optional scope(s) to confine the query to. A single scope answers + from that scope's own view of the target, including the + higher-order conclusions reasoned within it. A sequence of scopes + restricts recall to the union of their member sessions, which — + like ``sessions`` — yields only directly-stated conclusions. + Mutually exclusive with ``session`` and ``sessions``, and requires + a workspace-level key. + sessions: Optional allowlist of sessions to confine the query to, for + one-off questions spanning a handful of sessions. Recall is + limited to conclusions stated directly in those sessions: + conclusions produced by reasoning across sessions are excluded, + because their provenance cannot be proven to sit inside the + allowlist. Reach for a named ``scope`` when you need that depth. reasoning_level: Optional reasoning level for the query: "minimal", "low", "medium", "high", or "max". Defaults to "low" if not provided. response_format: Optional structure for the answer. Pass a Pydantic @@ -296,12 +315,20 @@ class Peer(PeerBase, MetadataConfigMixin): Response string containing the answer (a JSON string when a schema dict was given), a parsed model instance when a Pydantic model class was given, or None if no relevant information. + + Raises: + ValueError: If ``scope`` is combined with ``session`` or ``sessions``. """ self._honcho._ensure_workspace() target_id = resolve_id(target) resolved_session_id = resolve_id(session) body: dict[str, Any] = {"query": query, "stream": False} + body.update( + scope_recall_fields( + scope=scope, sessions=sessions, session_id=resolved_session_id + ) + ) if target_id: body["target"] = target_id if resolved_session_id: @@ -330,6 +357,8 @@ class Peer(PeerBase, MetadataConfigMixin): *, target: str | PeerBase | None = None, session: str | SessionBase | None = None, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, reasoning_level: Literal["minimal", "low", "medium", "high", "max"] | None = None, response_format: type[BaseModel] | dict[str, Any] | None = None, @@ -350,6 +379,9 @@ class Peer(PeerBase, MetadataConfigMixin): session: Optional session to scope the query to. If provided, only information from that session is considered. Can be a session ID string or a Session object. + scope: Optional scope(s) to confine the query to. See :meth:`chat`. + sessions: Optional allowlist of sessions to confine the query to. See + :meth:`chat` for the depth caveat. reasoning_level: Optional reasoning level for the query: "minimal", "low", "medium", "high", or "max". Defaults to "low" if not provided. response_format: Optional structure for the answer: a Pydantic model @@ -360,12 +392,20 @@ class Peer(PeerBase, MetadataConfigMixin): Returns: DialecticStreamResponse object that can be iterated over and provides final response + + Raises: + ValueError: If ``scope`` is combined with ``session`` or ``sessions``. """ self._honcho._ensure_workspace() target_id = resolve_id(target) resolved_session_id = resolve_id(session) body: dict[str, Any] = {"query": query, "stream": True} + body.update( + scope_recall_fields( + scope=scope, sessions=sessions, session_id=resolved_session_id + ) + ) if target_id: body["target"] = target_id if resolved_session_id: @@ -633,6 +673,9 @@ class Peer(PeerBase, MetadataConfigMixin): search_max_distance: float | None = Field(None, ge=0.0, le=1.0), include_most_frequent: bool | None = None, max_conclusions: int | None = Field(None, ge=1, le=100), + *, + scope: str | ScopeBase | Sequence[str | ScopeBase] | None = None, + sessions: Sequence[str | SessionBase] | None = None, ) -> str: """ Get a subset of the representation of the peer. @@ -646,10 +689,18 @@ class Peer(PeerBase, MetadataConfigMixin): search_max_distance: Maximum semantic distance for search results (0.0-1.0) include_most_frequent: Whether to include the most frequent conclusions max_conclusions: Maximum number of conclusions to include + scope: Optional scope(s) confining the representation. See + :meth:`chat`. Mutually exclusive with ``session`` and ``sessions``. + sessions: Optional allowlist of sessions confining the representation to + directly-stated conclusions from those sessions. See + :meth:`chat` for the depth caveat. Returns: A Representation string + Raises: + ValueError: If ``scope`` is combined with ``session`` or ``sessions``. + Example: ```python # Get global representation @@ -671,7 +722,9 @@ class Peer(PeerBase, MetadataConfigMixin): session_id = resolve_id(session) target_id = resolve_id(target) - body: dict[str, Any] = {} + body: dict[str, Any] = scope_recall_fields( + scope=scope, sessions=sessions, session_id=session_id + ) if session_id: body["session_id"] = session_id if target_id: @@ -764,7 +817,7 @@ class Peer(PeerBase, MetadataConfigMixin): return PeerContextResponse.model_validate(data) @property - def conclusions(self) -> ConclusionScope: + def conclusions(self) -> ConclusionsView: """ Access this peer's self-conclusions (where observer == observed == self). @@ -772,7 +825,7 @@ class Peer(PeerBase, MetadataConfigMixin): has made about themselves. Use this for self-conclusion scenarios. Returns: - A ConclusionScope scoped to this peer's self-conclusions + A ConclusionsView scoped to this peer's self-conclusions Example: ```python @@ -786,9 +839,9 @@ class Peer(PeerBase, MetadataConfigMixin): peer.conclusions.delete("obs-123") ``` """ - return ConclusionScope(self._honcho, self.workspace_id, self.id, self.id) + return ConclusionsView(self._honcho, self.workspace_id, self.id, self.id) - def conclusions_of(self, target: str | PeerBase) -> ConclusionScope: + def conclusions_of(self, target: str | PeerBase) -> ConclusionsView: """ Access conclusions this peer has made about another peer. @@ -799,7 +852,7 @@ class Peer(PeerBase, MetadataConfigMixin): target: The target peer (either a Peer object or peer ID string) Returns: - A ConclusionScope scoped to this peer's conclusions of the target + A ConclusionsView scoped to this peer's conclusions of the target Example: ```python @@ -817,7 +870,7 @@ class Peer(PeerBase, MetadataConfigMixin): ``` """ target_id = target.id if isinstance(target, PeerBase) else target - return ConclusionScope(self._honcho, self.workspace_id, self.id, target_id) + return ConclusionsView(self._honcho, self.workspace_id, self.id, target_id) def __repr__(self) -> str: """ diff --git a/sdks/python/src/honcho/scope.py b/sdks/python/src/honcho/scope.py new file mode 100644 index 00000000..14a5752b --- /dev/null +++ b/sdks/python/src/honcho/scope.py @@ -0,0 +1,233 @@ +# pyright: reportPrivateUsage=false +"""Sync Scope class for Honcho SDK.""" + +from __future__ import annotations + +import logging +from collections.abc import Sequence +from datetime import datetime +from typing import TYPE_CHECKING, Any + +from pydantic import ConfigDict, PrivateAttr, validate_call + +from .api_types import ScopeBackfillJob, ScopeStatusResponse, SessionResponse +from .base import ScopeBase, SessionBase +from .http import routes +from .pagination import SyncPage +from .session import Session +from .utils import resolve_scope_membership, resolve_scope_session + +if TYPE_CHECKING: + from .aio import ScopeAio + from .client import Honcho + +logger = logging.getLogger(__name__) + +__all__ = ["Scope"] + + +class Scope(ScopeBase): + """ + Represents a scope in Honcho. + + A scope is a named set of sessions that acts as a visibility boundary. Recall + performed through a scope sees only what happened in that scope's sessions, + while the underlying peer keeps its single unified representation across + everything it has ever participated in. + + Membership changes are applied asynchronously: adding a session that already + has messages copies its existing conclusions into the scope, and removing one + reconciles them back out. Poll :meth:`status` to watch that settle. + + Attributes: + id: Unprefixed scope name, unique within the workspace + workspace_id: Workspace ID for scoping operations + metadata: Cached metadata for this scope. May be stale if not recently + fetched. + created_at: When this scope was created, if known + + Example: + ```python + therapy = honcho.scope("therapy") + therapy.add_sessions([session_1, session_2]) + + # Ask a question answered only from the therapy sessions + answer = user.chat("What is stressing them out?", scope="therapy") + ``` + """ + + _metadata: dict[str, Any] | None = PrivateAttr(default=None) + _created_at: datetime | None = PrivateAttr(default=None) + _honcho: "Honcho" = PrivateAttr() + + @property + def metadata(self) -> dict[str, Any] | None: + """Cached metadata for this scope. May be stale if not recently fetched.""" + return self._metadata + + @property + def created_at(self) -> datetime | None: + """When this scope was created. Only available if fetched from the API.""" + return self._created_at + + def __init__( + self, + scope_id: str, + honcho: "Honcho", + *, + metadata: dict[str, Any] | None = None, + created_at: datetime | None = None, + ) -> None: + """ + Initialize a new Scope. + + **Do not call this directly — use** ``honcho.scope()``. + + Args: + scope_id: Unprefixed scope name, unique within the workspace + honcho: Honcho client instance + metadata: Cached metadata, if already fetched + created_at: Creation timestamp, if already fetched + """ + super().__init__( + id=scope_id, + workspace_id=honcho.workspace_id, + ) + self._honcho = honcho + self._metadata = metadata + self._created_at = created_at + + @property + def aio(self) -> "ScopeAio": + """ + Access async versions of all Scope methods. + + Returns a ScopeAio view that provides async versions of all methods while + sharing state with this Scope instance. + + Example: + ```python + await scope.aio.add_sessions(["session-1"]) + status = await scope.aio.status() + ``` + """ + # Import here to avoid circular import (aio.py imports this module) + from .aio import ScopeAio + + return ScopeAio(self) + + @validate_call(config=ConfigDict(arbitrary_types_allowed=True)) + def add_sessions(self, sessions: Sequence[str | SessionBase]) -> None: + """ + Add sessions to this scope. + + Every named session must already exist. Adding a session that is already + a member is a no-op. + + Sessions that already hold messages are backfilled into the scope + asynchronously, so recall through this scope may not reflect their history + immediately — poll :meth:`status` to watch that complete. + + Args: + sessions: Sessions to add, as ID strings or Session objects. At most + 100 per call, matching the server's limit; split larger membership + changes into separate calls so a failure names the batch that + failed. + + Raises: + ValueError: If no sessions are given, or more than 100. + """ + session_ids = resolve_scope_membership(sessions) + self._honcho._ensure_workspace() + self._honcho._http.post( + routes.scope_sessions(self.workspace_id, self.id), + body={"session_ids": session_ids}, + ) + + @validate_call(config=ConfigDict(arbitrary_types_allowed=True)) + def remove_session(self, session: str | SessionBase) -> None: + """ + Remove a session from this scope. + + Conclusions copied or derived while the session was a member are + reconciled out asynchronously, and the scope's peer card is rebuilt from + whatever evidence remains. Poll :meth:`status` to watch that settle. + + Args: + session: Session to remove, as an ID string or a Session object + """ + self._honcho._ensure_workspace() + self._honcho._http.delete( + routes.scope_session( + self.workspace_id, self.id, resolve_scope_session(session) + ) + ) + + def sessions( + self, + page: int = 1, + size: int = 50, + *, + reverse: bool = False, + ) -> SyncPage[SessionResponse, Session]: + """ + Get the sessions that are members of this scope. + + Ordered by how long each session has been a member — longest-standing + first, or most recently added first when ``reverse`` is True. + + Args: + page: Page number (1-indexed) + size: Number of results per page + reverse: If True, reverses the default ordering. Default: False. + + Returns: + Paginated response containing Session objects + """ + self._honcho._ensure_workspace() + + def fetch(next_page: int) -> dict[str, Any]: + query: dict[str, Any] = {"page": next_page, "size": size} + if reverse: + query["reverse"] = "true" + return self._honcho._http.post( + routes.scope_sessions_list(self.workspace_id, self.id), + query=query, + ) + + def transform(response: SessionResponse) -> Session: + return Session( + response.id, + self._honcho, + metadata=response.metadata, + configuration=response.configuration, + created_at=response.created_at, + is_active=response.is_active, + ) + + def fetch_next(next_page: int) -> SyncPage[SessionResponse, Session]: + return SyncPage(fetch(next_page), SessionResponse, transform, fetch_next) + + return SyncPage(fetch(page), SessionResponse, transform, fetch_next) + + def status(self) -> dict[str, ScopeBackfillJob]: + """ + Get the backfill/reconciliation progress for this scope. + + Use this after a membership change to tell "the scope knows nothing about + that session yet" apart from "the scope has caught up and there is + genuinely nothing to recall". + + Returns: + Per-session backfill state, keyed by session ID. Only sessions that + have had a backfill enqueued appear; an empty dict means none have. + """ + self._honcho._ensure_workspace() + data = self._honcho._http.get(routes.scope_status(self.workspace_id, self.id)) + return ScopeStatusResponse.model_validate(data).backfill_status + + def __repr__(self) -> str: + return f"Scope(id={self.id!r}, workspace_id={self.workspace_id!r})" + + def __str__(self) -> str: + return self.id diff --git a/sdks/python/src/honcho/session.py b/sdks/python/src/honcho/session.py index 3b1b0f14..2f1838c9 100644 --- a/sdks/python/src/honcho/session.py +++ b/sdks/python/src/honcho/session.py @@ -5,6 +5,7 @@ from __future__ import annotations import json import logging +from collections.abc import Sequence from datetime import datetime from typing import TYPE_CHECKING, Any @@ -20,7 +21,7 @@ from .api_types import ( SessionPeerConfig, SessionResponse, ) -from .base import PeerBase, SessionBase +from .base import PeerBase, ScopeBase, SessionBase from .http import routes from .message import Message from .mixins import MetadataConfigMixin @@ -32,6 +33,7 @@ from .utils import ( normalize_peers_to_dict, prepare_file_for_upload, resolve_id, + scope_context_fields, ) if TYPE_CHECKING: @@ -574,6 +576,14 @@ class Session(SessionBase, MetadataConfigMixin): None, description="A peer ID to get context *from the perspective of*. If given, response will attempt to include representation and card from the perspective of `peer_perspective`. Must be provided with `peer_target`.", ), + scope: str | ScopeBase | None = Field( + None, + description="A scope to use as the perspective source instead of a peer: `peer_target`'s representation and card are read from what that scope observed. Must be provided with `peer_target`; mutually exclusive with `peer_perspective`. Requires a workspace-level key.", + ), + sessions: Sequence[str | SessionBase] | None = Field( + None, + description="An allowlist of sessions confining `peer_target`'s representation to that set. This session must be one of them. Mutually exclusive with `scope` and `limit_to_session`.", + ), limit_to_session: bool = Field( False, description="Whether to limit the representation to this session only. If True, only conclusions from this session will be included.", @@ -616,6 +626,11 @@ class Session(SessionBase, MetadataConfigMixin): peer_target: A peer ID to get context for. search_query: A query string for semantic search. peer_perspective: A peer ID to get context from the perspective of. + scope: A scope to read `peer_target`'s representation and card from. + sessions: An allowlist of sessions confining `peer_target`'s + representation. Recall is limited to conclusions stated directly in + those sessions, and the peer card is omitted, since neither derived + conclusions nor cards carry provable per-session provenance. limit_to_session: Whether to limit the representation to this session only. search_top_k: Number of semantically relevant facts to return. search_max_distance: Maximum semantic distance for search results. @@ -627,6 +642,11 @@ class Session(SessionBase, MetadataConfigMixin): summary, if available, that maximizes conversational context while respecting the token limit + Raises: + ValueError: If `peer_target` is missing when required, or if `scope`, + `sessions`, `peer_perspective`, and `limit_to_session` are combined + in ways the server rejects. + Note: Token counting is performed using tiktoken. For models using different tokenizers, you may need to adjust the token limit accordingly. @@ -650,6 +670,13 @@ class Session(SessionBase, MetadataConfigMixin): query: dict[str, Any] = { "summary": summary, "limit_to_session": limit_to_session, + **scope_context_fields( + scope=scope, + sessions=sessions, + peer_target=peer_target, + peer_perspective=peer_perspective, + limit_to_session=limit_to_session, + ), } if tokens is not None: query["tokens"] = tokens diff --git a/sdks/python/src/honcho/utils/__init__.py b/sdks/python/src/honcho/utils/__init__.py index aac9221b..90810763 100644 --- a/sdks/python/src/honcho/utils/__init__.py +++ b/sdks/python/src/honcho/utils/__init__.py @@ -6,6 +6,15 @@ from .datetime import datetime_to_iso, parse_datetime from .file_upload import normalize_file_input, prepare_file_for_upload from .peers import normalize_peers_to_dict from .resolve import resolve_id +from .scopes import ( + resolve_scope_membership, + resolve_scope_option, + resolve_scope_session, + resolve_session_allowlist, + scope_context_fields, + scope_recall_fields, + validate_scope_id, +) from .sse import SSEStreamParser, parse_sse_astream, parse_sse_chunk, parse_sse_stream __all__ = [ @@ -19,4 +28,11 @@ __all__ = [ "parse_sse_stream", "prepare_file_for_upload", "resolve_id", + "resolve_scope_membership", + "resolve_scope_option", + "resolve_scope_session", + "resolve_session_allowlist", + "scope_context_fields", + "scope_recall_fields", + "validate_scope_id", ] diff --git a/sdks/python/src/honcho/utils/resolve.py b/sdks/python/src/honcho/utils/resolve.py index db70fc01..30eaf811 100644 --- a/sdks/python/src/honcho/utils/resolve.py +++ b/sdks/python/src/honcho/utils/resolve.py @@ -5,7 +5,7 @@ from __future__ import annotations from typing import TYPE_CHECKING, overload if TYPE_CHECKING: - from ..base import PeerBase, SessionBase + from ..base import PeerBase, ScopeBase, SessionBase @overload @@ -17,12 +17,12 @@ def resolve_id(obj: str) -> str: ... @overload -def resolve_id(obj: "PeerBase | SessionBase") -> str: ... +def resolve_id(obj: "PeerBase | SessionBase | ScopeBase") -> str: ... -def resolve_id(obj: "str | PeerBase | SessionBase | None") -> str | None: +def resolve_id(obj: "str | PeerBase | SessionBase | ScopeBase | None") -> str | None: """ - Resolve an ID from a string, PeerBase, SessionBase, or None. + Resolve an ID from a string, PeerBase, SessionBase, ScopeBase, or None. This utility function extracts the ID from an object that may be: - A string (returned as-is) diff --git a/sdks/python/src/honcho/utils/scopes.py b/sdks/python/src/honcho/utils/scopes.py new file mode 100644 index 00000000..4a0f6ef8 --- /dev/null +++ b/sdks/python/src/honcho/utils/scopes.py @@ -0,0 +1,296 @@ +"""Scope and session-allowlist option handling for the Honcho Python SDK. + +The ``scope`` and ``sessions`` options appear on several read surfaces (chat, +representation, session context, search). Their validation and their wire +translation live here so those surfaces cannot drift apart — the server enforces +the same exclusions with a 422, and this raises before the round trip. +""" + +from __future__ import annotations + +import re +from collections.abc import Sequence +from typing import TYPE_CHECKING, Any + +from .resolve import resolve_id + +if TYPE_CHECKING: + from ..base import ScopeBase, SessionBase + +__all__ = [ + "MAX_SCOPES_PER_OPTION", + "MAX_SESSIONS_PER_ADD", + "MAX_SESSION_ALLOWLIST_ENTRIES", + "resolve_scope_membership", + "resolve_scope_option", + "resolve_scope_session", + "resolve_session_allowlist", + "scope_context_fields", + "scope_recall_fields", + "validate_scope_id", +] + +# Scope IDs are stored server-side as peer names with this prefix prepended, so +# they must leave room for it within the 512-character peer name limit. +_SCOPE_PEER_PREFIX = "scope." +MAX_SCOPE_ID_LENGTH = 512 - len(_SCOPE_PEER_PREFIX) + +MAX_SCOPES_PER_OPTION = 100 +MAX_SESSION_ALLOWLIST_ENTRIES = 1000 +# The server accepts at most this many sessions per membership call. +MAX_SESSIONS_PER_ADD = 100 + +_RESOURCE_NAME_PATTERN = r"^[a-zA-Z0-9_-]+$" + + +def validate_scope_id(value: str) -> str: + """Validate an unprefixed scope ID. + + Args: + value: The scope ID as the caller supplied it. + + Returns: + The validated scope ID, unchanged. + + Raises: + ValueError: If the ID is empty, too long, carries the reserved prefix, + or contains characters outside the resource-name charset. + """ + if not 1 <= len(value) <= MAX_SCOPE_ID_LENGTH: + raise ValueError( + f"Scope ID must be between 1 and {MAX_SCOPE_ID_LENGTH} characters" + ) + # Checked before the charset: the reserved prefix contains '.', which is + # itself outside the charset, so a charset-first check would report the + # charset instead of the real mistake for a double-prefixed ID. + if value.startswith(_SCOPE_PEER_PREFIX): + raise ValueError( + f"Scope ID must not start with the reserved prefix '{_SCOPE_PEER_PREFIX}' (scope IDs are unprefixed)" + ) + if not re.fullmatch(_RESOURCE_NAME_PATTERN, value): + raise ValueError(f"Scope ID must match pattern {_RESOURCE_NAME_PATTERN}") + return value + + +def resolve_scope_option( + scope: "str | ScopeBase | Sequence[str | ScopeBase]", +) -> str | list[str]: + """Resolve the ``scope`` read option to its wire value. + + A single scope stays a string; a sequence becomes a list of IDs. The two + shapes mean different things to the server — one scope reads that scope's own + view, a list restricts recall to the union of their member sessions — so the + distinction is preserved rather than normalized away. + + Args: + scope: One scope (ID or ``Scope``) or a sequence of them. + + Returns: + A single validated scope ID, or a list of them. + + Raises: + ValueError: On an empty sequence, an over-cap sequence, or an invalid ID. + """ + # ``str`` is itself a Sequence, so both single-scope forms — an ID and a + # ``Scope`` — are taken first; whatever remains is the list form. + if isinstance(scope, str) or not isinstance(scope, Sequence): + return validate_scope_id(resolve_id(scope)) + + ids = [validate_scope_id(resolve_id(entry)) for entry in scope] + if not ids: + # An empty list would resolve to an empty allowlist server-side and + # silently recall nothing, which is never the intent. + raise ValueError("scope must name at least one scope") + if len(ids) > MAX_SCOPES_PER_OPTION: + raise ValueError(f"scope can name at most {MAX_SCOPES_PER_OPTION} scopes") + return ids + + +def resolve_session_allowlist( + sessions: "Sequence[str | SessionBase]", +) -> list[str]: + """Resolve the ``sessions`` allowlist option to a list of session IDs. + + Args: + sessions: Sessions to allow, as IDs or ``Session`` objects. + + Returns: + The session IDs, in the order given. + + Raises: + ValueError: On an empty list or one over the server's cap. + """ + ids = [resolve_id(entry) for entry in sessions] + if not ids: + # The server treats an empty allowlist as fail-closed (recalls nothing), + # so an empty list here is a caller mistake rather than a query. + raise ValueError("sessions must name at least one session") + if len(ids) > MAX_SESSION_ALLOWLIST_ENTRIES: + raise ValueError( + f"sessions can name at most {MAX_SESSION_ALLOWLIST_ENTRIES} sessions" + ) + return ids + + +def _validate_session_id(value: str) -> str: + """Validate a session ID against the charset the server accepts. + + Args: + value: The session ID as the caller supplied it. + + Returns: + The validated session ID, unchanged. + + Raises: + ValueError: If the ID is empty or contains characters outside the + resource-name charset. + """ + if not value: + raise ValueError("Session ID must be a non-empty string") + if not re.fullmatch(_RESOURCE_NAME_PATTERN, value): + raise ValueError(f"Session ID must match pattern {_RESOURCE_NAME_PATTERN}") + return value + + +def resolve_scope_session(session: "str | SessionBase") -> str: + """Resolve and validate a single session ID for a scope membership change. + + Validated rather than passed through because this ID is interpolated into a + request *path*: an unvalidated value silently changes which resource the + request addresses. ``valid-session?typo`` would target ``valid-session`` + with a stray query string, removing the wrong session from the scope and + triggering reconciliation against it. + + Args: + session: The session, as an ID or a ``Session`` object. + + Returns: + The validated session ID. + + Raises: + ValueError: If the ID is empty or malformed. + """ + return _validate_session_id(resolve_id(session)) + + +def resolve_scope_membership( + sessions: "Sequence[str | SessionBase]", +) -> list[str]: + """Resolve a scope membership change to a list of session IDs. + + Capped at the server's per-call limit rather than silently chunking, so a + rejected batch is the batch the caller passed. + + Args: + sessions: Sessions to add, as IDs or ``Session`` objects. + + Returns: + The session IDs, in the order given. + + Raises: + ValueError: On an empty list, one over the server's per-call cap, or a + malformed session ID. + """ + ids = [_validate_session_id(resolve_id(session)) for session in sessions] + if not ids: + raise ValueError("At least one session must be given") + if len(ids) > MAX_SESSIONS_PER_ADD: + raise ValueError( + f"At most {MAX_SESSIONS_PER_ADD} sessions can be added per call" + ) + return ids + + +def scope_context_fields( + *, + scope: "str | ScopeBase | None", + sessions: "Sequence[str | SessionBase] | None", + peer_target: str | None, + peer_perspective: str | None, + limit_to_session: bool, +) -> dict[str, Any]: + """Build the query fields for ``scope``/``sessions`` on the context route. + + Unlike the recall endpoints, session context takes these as query parameters + — ``sessions`` is sent as a repeated parameter, not as a ``filters`` body. + + Only a single scope is accepted: a scope is the *perspective source* for the + target's representation and card, which is one observer, so a list has no + meaning here. + + Args: + scope: The ``scope`` option, if given. + sessions: The ``sessions`` allowlist option, if given. + peer_target: The observed peer. Required by either option, since both only + reach the representation and there is none without a target. + peer_perspective: The observing peer, if given — a scope replaces it. + limit_to_session: Whether recall is already pinned to this session alone. + + Returns: + The fields to merge into the query. Empty when neither option is set. + + Raises: + ValueError: If either option is combined with something it contradicts, or + used without ``peer_target``. + """ + # A scope already determines what the context can see, and limit_to_session + # already pins recall to this session alone, so combining them with a + # perspective or an allowlist is a contradiction rather than a narrowing. + # Raised here so the caller does not pay a round trip for a 422. + if sessions is not None: + if scope is not None: + raise ValueError("`sessions` and `scope` are mutually exclusive") + if limit_to_session: + raise ValueError("`sessions` and `limit_to_session` are mutually exclusive") + if peer_target is None: + raise ValueError( + "You must provide a `peer_target` when `sessions` is provided" + ) + return {"sessions": resolve_session_allowlist(sessions)} + + if scope is None: + return {} + + if peer_perspective is not None: + raise ValueError("`scope` and `peer_perspective` are mutually exclusive") + if peer_target is None: + raise ValueError("You must provide a `peer_target` when `scope` is provided") + return {"scope": validate_scope_id(resolve_id(scope))} + + +def scope_recall_fields( + *, + scope: "str | ScopeBase | Sequence[str | ScopeBase] | None", + sessions: "Sequence[str | SessionBase] | None", + session_id: str | None = None, +) -> dict[str, Any]: + """Build the request-body fields for the ``scope``/``sessions`` options. + + ``sessions`` is sugar: it goes on the wire as the constrained + ``filters: {"session_id": [...]}`` body the recall endpoints accept, never as + a field of its own, which the server would reject as an unknown key. + + Args: + scope: The ``scope`` option, if given. + sessions: The ``sessions`` allowlist option, if given. + session_id: A single session already set on the request, if any — a scope + already determines what can be seen, so the two conflict. + + Returns: + The fields to merge into the request body. Empty when neither option is + set. + + Raises: + ValueError: If ``scope`` is combined with ``sessions`` or ``session_id``, + or if either option is itself invalid. + """ + if scope is None: + if sessions is None: + return {} + return {"filters": {"session_id": resolve_session_allowlist(sessions)}} + + if sessions is not None: + raise ValueError("`scope` and `sessions` are mutually exclusive") + if session_id is not None: + raise ValueError("`scope` and `session` are mutually exclusive") + return {"scope": resolve_scope_option(scope)} diff --git a/sdks/typescript/__tests__/conclusions.test.ts b/sdks/typescript/__tests__/conclusions.test.ts index 7716d3ac..7beadc34 100644 --- a/sdks/typescript/__tests__/conclusions.test.ts +++ b/sdks/typescript/__tests__/conclusions.test.ts @@ -1,7 +1,7 @@ /** * Conclusions Tests * - * Tests for Conclusion operations via ConclusionScope. + * Tests for Conclusion operations via ConclusionsView. * * Endpoints covered: * - POST /v3/workspaces/:workspaceId/conclusions (create conclusions) @@ -11,7 +11,7 @@ */ import { describe, test, expect, beforeAll, afterAll } from 'bun:test' -import { Honcho, Conclusion, ConclusionScope } from '../src' +import { Honcho, Conclusion, ConclusionsView } from '../src' import { createTestClient, requireServer } from './setup' import { assertConclusionShape } from './helpers' @@ -31,16 +31,16 @@ describe('Conclusions', () => { }) // =========================================================================== - // ConclusionScope Access + // ConclusionsView Access // =========================================================================== - describe('ConclusionScope access', () => { + describe('ConclusionsView access', () => { test('peer.conclusions returns self-scope', async () => { const peer = await client.peer('self-scope-peer') const scope = peer.conclusions - expect(scope).toBeInstanceOf(ConclusionScope) + expect(scope).toBeInstanceOf(ConclusionsView) expect(scope.observer).toBe(peer.id) expect(scope.observed).toBe(peer.id) expect(scope.workspaceId).toBe(client.workspaceId) @@ -289,7 +289,7 @@ describe('Conclusions', () => { for (const key of ['observer', 'observed', 'observer_id', 'observed_id']) { await expect( peer.conclusions.list({ filters: { [key]: 'someone-else' } }) - ).rejects.toThrow(/managed by this conclusion scope/) + ).rejects.toThrow(/managed by this conclusions view/) } }) @@ -298,10 +298,10 @@ describe('Conclusions', () => { await expect( peer.conclusions.list({ filters: { session_id: 'sess' } }) - ).rejects.toThrow(/managed by this conclusion scope/) + ).rejects.toThrow(/managed by this conclusions view/) await expect( peer.conclusions.list({ filters: { session: 'sess' } }) - ).rejects.toThrow(/managed by this conclusion scope/) + ).rejects.toThrow(/managed by this conclusions view/) }) test('query rejects observer/observed scope keys in filters', async () => { @@ -310,7 +310,7 @@ describe('Conclusions', () => { for (const key of ['observer', 'observed', 'observer_id', 'observed_id']) { await expect( peer.conclusions.query('q', 10, undefined, { [key]: 'someone-else' }) - ).rejects.toThrow(/managed by this conclusion scope/) + ).rejects.toThrow(/managed by this conclusions view/) } }) @@ -464,16 +464,16 @@ describe('Conclusions', () => { }) // =========================================================================== - // ConclusionScope toString + // ConclusionsView toString // =========================================================================== - describe('ConclusionScope toString', () => { + describe('ConclusionsView toString', () => { test('returns readable format', async () => { const peer = await client.peer('scope-tostring-peer') const str = peer.conclusions.toString() - expect(str).toContain('ConclusionScope') + expect(str).toContain('ConclusionsView') expect(str).toContain(peer.id) expect(str).toContain(client.workspaceId) }) diff --git a/sdks/typescript/__tests__/http-client.test.ts b/sdks/typescript/__tests__/http-client.test.ts index 06d80230..878869b9 100644 --- a/sdks/typescript/__tests__/http-client.test.ts +++ b/sdks/typescript/__tests__/http-client.test.ts @@ -237,6 +237,39 @@ describe('URL building', () => { expect(url.searchParams.get('present')).toBe('value') expect(url.searchParams.has('missing')).toBe(false) }) + + test('sends array query parameters as repeated params, not comma-joined', async () => { + let capturedURL = '' + globalThis.fetch = async (url) => { + capturedURL = url.toString() + return mockResponse({ ok: true }) + } + + await client.get('/v1/test', { + query: { sessions: ['session-a', 'session-b'] }, + }) + + const url = new URL(capturedURL) + // The API reads list-valued params as ?k=a&k=b. A comma-joined single value + // would arrive as one malformed entry. + expect(url.searchParams.getAll('sessions')).toEqual([ + 'session-a', + 'session-b', + ]) + }) + + test('an empty array query parameter contributes nothing', async () => { + let capturedURL = '' + globalThis.fetch = async (url) => { + capturedURL = url.toString() + return mockResponse({ ok: true }) + } + + await client.get('/v1/test', { query: { sessions: [] } }) + + const url = new URL(capturedURL) + expect(url.searchParams.has('sessions')).toBe(false) + }) }) // ============================================================================= diff --git a/sdks/typescript/__tests__/peer.test.ts b/sdks/typescript/__tests__/peer.test.ts index fca85910..cde6a0ad 100644 --- a/sdks/typescript/__tests__/peer.test.ts +++ b/sdks/typescript/__tests__/peer.test.ts @@ -671,7 +671,7 @@ describe('Peer', () => { // =========================================================================== describe('Conclusion scope access', () => { - test('conclusions property returns ConclusionScope for self', async () => { + test('conclusions property returns ConclusionsView for self', async () => { const peer = await client.peer('self-conclusions-peer') const scope = peer.conclusions @@ -681,7 +681,7 @@ describe('Peer', () => { expect(scope.workspaceId).toBe(client.workspaceId) }) - test('conclusionsOf returns ConclusionScope for target', async () => { + test('conclusionsOf returns ConclusionsView for target', async () => { const observer = await client.peer('obs-conclusions-peer') const target = await client.peer('target-conclusions-peer') diff --git a/sdks/typescript/__tests__/scope.unit.test.ts b/sdks/typescript/__tests__/scope.unit.test.ts new file mode 100644 index 00000000..b4b9ebab --- /dev/null +++ b/sdks/typescript/__tests__/scope.unit.test.ts @@ -0,0 +1,389 @@ +import { describe, expect, test } from 'bun:test' +import { ZodError } from 'zod' +import type { HonchoHTTPClient } from '../src/http/client' +import { Peer } from '../src/peer' +import { Scope } from '../src/scope' +import { Session } from '../src/session' +import type { ScopeStatusResponse } from '../src/types/api' + +/** + * Capture the body of the single request a call makes, so the wire shape the + * server actually receives is asserted rather than the SDK's own options. + */ +function capturingHttp(response: unknown): { + http: HonchoHTTPClient + body: () => Record | undefined + query: () => Record | undefined + path: () => string | undefined +} { + let capturedBody: Record | undefined + let capturedQuery: Record | undefined + let capturedPath: string | undefined + const http = { + post: async ( + path: string, + options?: { body?: Record } + ) => { + capturedPath = path + capturedBody = options?.body + return response + }, + get: async ( + path: string, + options?: { query?: Record } + ) => { + capturedPath = path + capturedQuery = options?.query + return response + }, + delete: async (path: string) => { + capturedPath = path + return undefined + }, + } as unknown as HonchoHTTPClient + return { + http, + body: () => capturedBody, + query: () => capturedQuery, + path: () => capturedPath, + } +} + +describe('sessions allowlist sugar', () => { + test('chat sends `sessions` as a session_id filter, not a bare field', async () => { + const { http, body } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await peer.chat('what happened?', { + sessions: ['session-a', new Session('session-b', 'workspace-1', http)], + }) + + expect(body()).toMatchObject({ + filters: { session_id: ['session-a', 'session-b'] }, + }) + // The sugar must not leak through as its own wire field — the server would + // reject an unknown key. + expect(body()).not.toHaveProperty('sessions') + }) + + test('representation sends `sessions` as a session_id filter', async () => { + const { http, body } = capturingHttp({ representation: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await peer.representation({ sessions: ['session-a'] }) + + expect(body()).toMatchObject({ filters: { session_id: ['session-a'] } }) + }) + + test('an empty allowlist is rejected rather than silently recalling nothing', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await expect(peer.chat('q', { sessions: [] })).rejects.toBeInstanceOf( + ZodError + ) + }) +}) + +describe('scope read option', () => { + test('a single scope passes through as `scope`', async () => { + const { http, body } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await peer.chat('q', { scope: 'therapy' }) + + expect(body()).toMatchObject({ scope: 'therapy' }) + }) + + test('a Scope object resolves to its id', async () => { + const { http, body } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await peer.chat('q', { scope: new Scope('therapy', 'workspace-1', http) }) + + expect(body()).toMatchObject({ scope: 'therapy' }) + }) + + test('a list of scopes passes through as a list', async () => { + const { http, body } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await peer.chat('q', { scope: ['therapy', 'work'] }) + + expect(body()).toMatchObject({ scope: ['therapy', 'work'] }) + }) + + test('scope and sessions are rejected together', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await expect( + peer.chat('q', { scope: 'therapy', sessions: ['session-a'] }) + ).rejects.toBeInstanceOf(ZodError) + }) + + test('scope and a single session are rejected together', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await expect( + peer.chat('q', { scope: 'therapy', session: 'session-a' }) + ).rejects.toBeInstanceOf(ZodError) + }) +}) + +describe('scope id validation', () => { + test('a prefixed name reports the reserved prefix, not the charset', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + // 'scope.therapy' fails both rules; the prefix message is the useful one, + // so it must be the only one raised. + const error = await peer + .chat('q', { scope: 'scope.therapy' }) + .then(() => undefined) + .catch((err: unknown) => err as ZodError) + + expect(error).toBeInstanceOf(ZodError) + const messages = (error as ZodError).issues.map((issue) => issue.message) + expect(messages.some((m) => m.includes('reserved prefix'))).toBe(true) + expect(messages.some((m) => m.includes('may only contain'))).toBe(false) + }) + + test('a name with illegal characters is rejected', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + await expect(peer.chat('q', { scope: 'my scope' })).rejects.toBeInstanceOf( + ZodError + ) + }) + + test('the specific message survives the scope option union', async () => { + // ScopeOptionSchema is a union. Zod collapses a failing union into a single + // `invalid_union` / "Invalid input" issue and buries the branch errors, so + // the rules are applied after the union resolves. Without that, every bad + // scope reports "Invalid input" and the caller learns nothing. + for (const [input, expected] of [ + ['scope.therapy', 'reserved prefix'], + ['my scope', 'may only contain'], + ['', 'non-empty'], + ['a'.repeat(507), 'at most 506'], + ] as const) { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + const error = (await peer + .chat('q', { scope: input }) + .then(() => undefined) + .catch((err: unknown) => err)) as ZodError + + expect(error).toBeInstanceOf(ZodError) + const messages = error.issues.map((i) => i.message).join(' | ') + expect(messages).toContain(expected) + expect(messages).not.toContain('Invalid input') + } + }) + + test('list-form messages also survive the union', async () => { + const { http } = capturingHttp({ content: 'ok' }) + const peer = new Peer('alice', 'workspace-1', http) + + for (const [input, expected] of [ + [[], 'at least one scope'], + [['ok', 'scope.bad'], 'reserved prefix'], + [Array.from({ length: 101 }, (_, i) => `s${i}`), 'at most 100 scopes'], + ] as const) { + const error = (await peer + .chat('q', { scope: input as string[] }) + .then(() => undefined) + .catch((err: unknown) => err)) as ZodError + + expect(error.issues.map((i) => i.message).join(' | ')).toContain(expected) + } + }) +}) + +describe('empty-string options fail closed', () => { + test("session.context rejects scope: '' instead of returning unscoped context", async () => { + const { http, query } = capturingHttp({ + id: 'session-a', + messages: [], + summary: null, + peer_representation: null, + peer_card: null, + }) + const session = new Session('session-a', 'workspace-1', http) + + // A truthiness check here would drop the option and silently return the + // unscoped context — the opposite of what an invalid scope should do. + await expect( + session.context({ peerTarget: 'user', scope: '' }) + ).rejects.toBeInstanceOf(ZodError) + expect(query()).toBeUndefined() + }) +}) + +describe('scope membership ids are validated before reaching a URL', () => { + test('removeSession rejects an id that would alter the request path', async () => { + const { http, path } = capturingHttp(undefined) + const scope = new Scope('therapy', 'workspace-1', http) + + // `valid-session?typo` would address `valid-session` with a stray query + // string, removing the wrong session and reconciling against it. + await expect( + scope.removeSession('valid-session?typo') + ).rejects.toBeInstanceOf(ZodError) + expect(path()).toBeUndefined() + }) + + test('addSessions rejects the same shape', async () => { + const { http, body } = capturingHttp(undefined) + const scope = new Scope('therapy', 'workspace-1', http) + + await expect( + scope.addSessions(['ok-session', 'valid-session?typo']) + ).rejects.toBeInstanceOf(ZodError) + expect(body()).toBeUndefined() + }) +}) + +describe('session.context scoping', () => { + const contextResponse = { + id: 'session-a', + messages: [], + summary: null, + peer_representation: null, + peer_card: null, + } + + test('scope and sessions are sent as their own query params', async () => { + const { http, query } = capturingHttp(contextResponse) + const session = new Session('session-a', 'workspace-1', http) + + await session.context({ + peerTarget: 'user', + sessions: ['session-a', 'session-b'], + }) + + // An array here relies on the HTTP client emitting repeated params; see + // http-client.test.ts. + expect(query()).toMatchObject({ sessions: ['session-a', 'session-b'] }) + }) + + test('sessions without peerTarget is rejected, not silently ignored', async () => { + const { http } = capturingHttp(contextResponse) + const session = new Session('session-a', 'workspace-1', http) + + await expect( + session.context({ sessions: ['session-a'] }) + ).rejects.toBeInstanceOf(ZodError) + }) + + test('sessions and scope are rejected together', async () => { + const { http } = capturingHttp(contextResponse) + const session = new Session('session-a', 'workspace-1', http) + + await expect( + session.context({ + peerTarget: 'user', + scope: 'therapy', + sessions: ['session-a'], + }) + ).rejects.toBeInstanceOf(ZodError) + }) + + test('sessions and limitToSession are rejected together', async () => { + const { http } = capturingHttp(contextResponse) + const session = new Session('session-a', 'workspace-1', http) + + await expect( + session.context({ + peerTarget: 'user', + limitToSession: true, + sessions: ['session-a'], + }) + ).rejects.toBeInstanceOf(ZodError) + }) + + test('scope and peerPerspective are rejected together', async () => { + const { http } = capturingHttp(contextResponse) + const session = new Session('session-a', 'workspace-1', http) + + await expect( + session.context({ + peerTarget: 'user', + peerPerspective: 'assistant', + scope: 'therapy', + }) + ).rejects.toBeInstanceOf(ZodError) + }) +}) + +describe('Scope membership and status', () => { + test('addSessions posts session_ids and resolves Session objects', async () => { + const { http, body, path } = capturingHttp(undefined) + const scope = new Scope('therapy', 'workspace-1', http) + + await scope.addSessions([ + 'session-a', + new Session('session-b', 'workspace-1', http), + ]) + + expect(path()).toBe('/v3/workspaces/workspace-1/scopes/therapy/sessions') + expect(body()).toEqual({ session_ids: ['session-a', 'session-b'] }) + }) + + test('addSessions rejects a batch over the server limit instead of chunking', async () => { + const { http } = capturingHttp(undefined) + const scope = new Scope('therapy', 'workspace-1', http) + + const tooMany = Array.from({ length: 101 }, (_, i) => `session-${i}`) + + await expect(scope.addSessions(tooMany)).rejects.toBeInstanceOf(ZodError) + }) + + test('removeSession targets the session subpath', async () => { + const { http, path } = capturingHttp(undefined) + const scope = new Scope('therapy', 'workspace-1', http) + + await scope.removeSession(new Session('session-b', 'workspace-1', http)) + + expect(path()).toBe( + '/v3/workspaces/workspace-1/scopes/therapy/sessions/session-b' + ) + }) + + test('status maps snake_case job fields to camelCase', async () => { + const response: ScopeStatusResponse = { + backfill_status: { + 'session-a': { + state: 'completed', + updated_at: '2024-01-01T00:00:00Z', + docs_copied: 12, + }, + 'session-b': { state: 'pending', updated_at: '2024-01-02T00:00:00Z' }, + }, + } + const { http } = capturingHttp(response) + const scope = new Scope('therapy', 'workspace-1', http) + + const status = await scope.status() + + expect(status.backfillStatus['session-a']).toEqual({ + state: 'completed', + updatedAt: '2024-01-01T00:00:00Z', + docsCopied: 12, + }) + expect(status.backfillStatus['session-b']?.docsCopied).toBeUndefined() + }) + + test('status on a scope with no backfill is an empty map, not a throw', async () => { + // The server omits the key entirely when nothing was ever enqueued. + const { http } = capturingHttp({} as ScopeStatusResponse) + const scope = new Scope('therapy', 'workspace-1', http) + + const status = await scope.status() + + expect(status.backfillStatus).toEqual({}) + }) +}) diff --git a/sdks/typescript/src/client.ts b/sdks/typescript/src/client.ts index db8e237a..2b66b9c2 100644 --- a/sdks/typescript/src/client.ts +++ b/sdks/typescript/src/client.ts @@ -3,6 +3,7 @@ import { HonchoHTTPClient } from './http/client' import { Message } from './message' import { Page } from './pagination' import { Peer } from './peer' +import { Scope } from './scope' import { Session } from './session' import type { MessageResponse, @@ -11,6 +12,7 @@ import type { QueueStatus, QueueStatusParams, QueueStatusResponse, + ScopeResponse, SessionResponse, WorkspaceResponse, } from './types/api' @@ -32,12 +34,14 @@ import { peerConfigFromApi, peerConfigToApi, type QueueStatusOptions, + ScopeIdSchema, SearchQuerySchema, type SessionConfig, SessionConfigSchema, SessionIdSchema, type SessionMetadata, SessionMetadataSchema, + SessionScopesSchema, sessionConfigFromApi, sessionConfigToApi, type WorkspaceConfig, @@ -254,6 +258,7 @@ export class Honcho { params: { query: string filters?: Record + scope?: string limit?: number } ): Promise { @@ -314,6 +319,39 @@ export class Honcho { ) } + private async _getOrCreateScope( + workspaceId: string, + params: { + id: string + metadata?: Record + } + ): Promise { + return this._http.post( + `/${API_VERSION}/workspaces/${workspaceId}/scopes`, + { body: params } + ) + } + + private async _listScopes( + workspaceId: string, + params?: { + page?: number + size?: number + reverse?: boolean + } + ): Promise> { + return this._http.post>( + `/${API_VERSION}/workspaces/${workspaceId}/scopes/list`, + { + query: { + page: params?.page, + size: params?.size, + reverse: params?.reverse ? 'true' : undefined, + }, + } + ) + } + private async _listSessions( workspaceId: string, params?: { @@ -346,6 +384,7 @@ export class Honcho { string, { observe_me?: boolean | null; observe_others?: boolean | null } > + scopes?: string[] } ): Promise { return this._http.post( @@ -356,6 +395,7 @@ export class Honcho { metadata: params.metadata, configuration: sessionConfigToApi(params.configuration), peers: params.peers, + scopes: params.scopes, }, } ) @@ -504,6 +544,10 @@ export class Honcho { * @param options.peers - Optional peers to attach to the session at creation. * Accepts the same shape as `session.addPeers()` (peer ID strings, * Peer objects, arrays of either, or a record with per-peer config). + * @param options.scopes - Optional scopes this session should join. Each scope is + * created if it does not exist yet. Attaching at creation avoids the + * asynchronous backfill that a later `scope.addSessions()` triggers, + * since there is no history to copy. * @returns Promise resolving to a Session object that can be used to add peers, * send messages, and manage conversation context * @throws Error if the session ID is empty or invalid @@ -514,6 +558,7 @@ export class Honcho { metadata?: SessionMetadata configuration?: SessionConfig peers?: PeerAddition + scopes?: (string | Scope)[] } ): Promise { await this._ensureWorkspace() @@ -528,12 +573,17 @@ export class Honcho { options?.peers !== undefined ? PeerAdditionToApiSchema.parse(options.peers) : undefined + const validatedScopes = + options?.scopes !== undefined + ? SessionScopesSchema.parse(options.scopes.map(resolveId)) + : undefined const sessionData = await this._getOrCreateSession(this.workspaceId, { id: validatedId, configuration: validatedConfiguration, metadata: validatedMetadata, peers: validatedPeers, + scopes: validatedScopes, }) return new Session( validatedId, @@ -547,6 +597,90 @@ export class Honcho { ) } + /** + * Get or create a scope with the given ID. + * + * A scope is a named set of sessions that acts as a visibility boundary: recall + * performed through the scope sees only what happened in its sessions, while the + * underlying peer keeps its single unified representation of everything. + * + * @param id - Unprefixed scope name, unique within the workspace + * @param options.metadata - Optional metadata to associate with this scope + * @returns Promise resolving to a Scope object for managing membership + * @throws Error if the scope ID is empty or invalid, or if a peer already occupies + * the scope's reserved internal name + * + * @example + * ```typescript + * const therapy = await honcho.scope('therapy') + * await therapy.addSessions([session1, session2]) + * ``` + */ + async scope( + id: string, + options?: { + metadata?: Record + } + ): Promise { + await this._ensureWorkspace() + const validatedId = ScopeIdSchema.parse(id) + + const scopeData = await this._getOrCreateScope(this.workspaceId, { + id: validatedId, + metadata: options?.metadata, + }) + return new Scope( + validatedId, + this.workspaceId, + this._http, + scopeData.metadata ?? undefined, + () => this._ensureWorkspace(), + scopeData.created_at + ) + } + + /** + * Get all scopes in the current workspace. + * + * @param options - Pagination options: `page`, `size`, and `reverse` + * @returns Promise resolving to a Page of Scope objects. Returns an empty page if + * no scopes exist + */ + async scopes(options?: { + page?: number + size?: number + reverse?: boolean + }): Promise> { + await this._ensureWorkspace() + const reverse = options?.reverse + const scopesPage = await this._listScopes(this.workspaceId, { + page: options?.page, + size: options?.size, + reverse, + }) + + const fetchNextPage = async ( + page: number, + size: number + ): Promise> => { + return this._listScopes(this.workspaceId, { page, size, reverse }) + } + + return new Page( + scopesPage, + (scope) => + new Scope( + scope.id, + this.workspaceId, + this._http, + scope.metadata ?? undefined, + () => this._ensureWorkspace(), + scope.created_at + ), + fetchNextPage + ) + } + /** * Get all sessions in the current workspace. * @@ -775,6 +909,9 @@ export class Honcho { * * @param query - The search query to use * @param filters - Optional filters to scope the search. See [search filters documentation](https://honcho.dev/docs/v3/documentation/core-concepts/features/using-filters). + * @param options.scope - Optional scope to restrict the search to that scope's member + * sessions. Mutually exclusive with a `session_id` filter. A scope + * with no member sessions matches nothing rather than everything. * @param limit - Number of results to return (1-100, default: 10). * @returns Promise resolving to an array of Message objects representing the search results. * Returns an empty array if no messages are found. @@ -784,6 +921,7 @@ export class Honcho { query: string, options?: { filters?: Filters + scope?: string | Scope limit?: number } ): Promise { @@ -792,12 +930,19 @@ export class Honcho { const validatedFilters = options?.filters ? FilterSchema.parse(options.filters) : undefined + // Checked against undefined, not truthiness: `scope: ''` is invalid, and + // dropping it silently would diverge from the Python SDK, which rejects it. + const validatedScope = + options?.scope !== undefined + ? ScopeIdSchema.parse(resolveId(options.scope)) + : undefined const validatedLimit = options?.limit ? LimitSchema.parse(options.limit) : undefined const response = await this._searchWorkspace(this.workspaceId, { query: validatedQuery, filters: validatedFilters, + scope: validatedScope, limit: validatedLimit, }) return response.map(Message.fromApiResponse) diff --git a/sdks/typescript/src/conclusions.ts b/sdks/typescript/src/conclusions.ts index c9add4e2..44f5e7e1 100644 --- a/sdks/typescript/src/conclusions.ts +++ b/sdks/typescript/src/conclusions.ts @@ -12,10 +12,10 @@ import type { import { normalizeSearchQuery, RepresentationOptionsSchema } from './validation' /** - * Filter keys that define a conclusion scope (the observer/observed peer pair). - * They are set from the scope itself, so a caller must not pass them in `filters`. + * Filter keys that define a conclusions view (the observer/observed peer pair). + * They are set from the view itself, so a caller must not pass them in `filters`. */ -const SCOPE_RESERVED_KEYS = [ +const VIEW_RESERVED_KEYS = [ 'observer', 'observed', 'observer_id', @@ -23,11 +23,11 @@ const SCOPE_RESERVED_KEYS = [ ] /** - * Throw if `filters` contains keys managed by the conclusion scope. + * Throw if `filters` contains keys managed by the conclusions view. * * The observer/observed peer pair (and, on `list`, the session) is fixed by the - * scope, so letting a user filter override it would silently return data from a - * different scope than requested. Fail loud instead. + * view, so letting a user filter override it would silently return data from a + * different pair than requested. Fail loud instead. */ function rejectReservedFilterKeys( filters: Record | undefined, @@ -42,7 +42,7 @@ function rejectReservedFilterKeys( guidance += '; use the session option to filter by session' } throw new Error( - `Filter key(s) ${clash.join(', ')} are managed by this conclusion scope ` + + `Filter key(s) ${clash.join(', ')} are managed by this conclusions view ` + `and cannot be passed in filters. ${guidance}.` ) } @@ -120,7 +120,7 @@ export class Conclusion { /** * Scoped access to conclusions for a specific observer/observed relationship. */ -export class ConclusionScope { +export class ConclusionsView { private _http: HonchoHTTPClient private _ensureWorkspace: () => Promise readonly workspaceId: string @@ -223,14 +223,14 @@ export class ConclusionScope { // =========================================================================== /** - * List conclusions in this scope. + * List conclusions in this view. * * @param options - Optional configuration for the list request * @param options.page - Page number (1-indexed, default: 1) * @param options.size - Number of items per page (default: 50) * @param options.session - Optional session (ID string or Session object) to filter by * @param options.filters - Optional additional filter criteria, merged with - * this scope's observer/observed (and session, if given). Supports the same + * this view's observer/observed (and session, if given). Supports the same * operators as other list endpoints — e.g. `{ level: 'explicit' }` to get * only conclusions extracted directly from messages (i.e. not derived during * dreaming). See @@ -245,7 +245,7 @@ export class ConclusionScope { reverse?: boolean }): Promise> { rejectReservedFilterKeys(options?.filters, [ - ...SCOPE_RESERVED_KEYS, + ...VIEW_RESERVED_KEYS, 'session', 'session_id', ]) @@ -284,13 +284,13 @@ export class ConclusionScope { } /** - * Semantic search for conclusions in this scope. + * Semantic search for conclusions in this view. * * @param query - The search query string * @param topK - Maximum number of results to return (default: 10) * @param distance - Maximum cosine distance threshold (0.0-1.0) * @param filters - Optional additional filter criteria, merged with this - * scope's observer/observed. Supports the same operators as the list + * view's observer/observed. Supports the same operators as the list * endpoint — e.g. `{ level: 'deductive' }` to search only conclusions * derived during dreaming. See * https://honcho.dev/docs/v3/documentation/features/advanced/using-filters @@ -301,7 +301,7 @@ export class ConclusionScope { distance?: number, filters?: Record ): Promise { - rejectReservedFilterKeys(filters, SCOPE_RESERVED_KEYS) + rejectReservedFilterKeys(filters, VIEW_RESERVED_KEYS) const response = await this._query({ query, top_k: topK, @@ -324,7 +324,7 @@ export class ConclusionScope { } /** - * Create conclusions in this scope. + * Create conclusions in this view. */ async create( conclusions: ConclusionCreateParams | ConclusionCreateParams[] @@ -351,7 +351,7 @@ export class ConclusionScope { } /** - * Get the computed representation for this scope. + * Get the computed representation for this view. */ async representation(options?: RepresentationOptions): Promise { const searchQuery = normalizeSearchQuery(options?.searchQuery) @@ -375,6 +375,6 @@ export class ConclusionScope { } toString(): string { - return `ConclusionScope(workspaceId='${this.workspaceId}', observer='${this.observer}', observed='${this.observed}')` + return `ConclusionsView(workspaceId='${this.workspaceId}', observer='${this.observer}', observed='${this.observed}')` } } diff --git a/sdks/typescript/src/http/client.ts b/sdks/typescript/src/http/client.ts index e22480d2..fd5e6379 100644 --- a/sdks/typescript/src/http/client.ts +++ b/sdks/typescript/src/http/client.ts @@ -6,18 +6,27 @@ import { TimeoutError, } from './errors' +/** + * Query parameters for a request. An array value is sent as repeated + * parameters (`?k=a&k=b`), which is how the API reads list-valued parameters. + */ +export type QueryParams = Record< + string, + string | number | boolean | readonly (string | number | boolean)[] | undefined +> + export interface HonchoHTTPClientConfig { baseURL: string apiKey?: string timeout?: number maxRetries?: number defaultHeaders?: Record - defaultQuery?: Record + defaultQuery?: QueryParams } export interface RequestOptions { body?: unknown - query?: Record + query?: QueryParams headers?: Record timeout?: number signal?: AbortSignal @@ -37,7 +46,7 @@ export class HonchoHTTPClient { readonly timeout: number readonly maxRetries: number readonly defaultHeaders: Record - readonly defaultQuery?: Record + readonly defaultQuery?: QueryParams constructor(config: HonchoHTTPClientConfig) { // Remove trailing slash from baseURL @@ -273,21 +282,28 @@ export class HonchoHTTPClient { return JSON.parse(text) as T } - private buildURL( - path: string, - query?: Record - ): string { + private buildURL(path: string, query?: QueryParams): string { const url = new URL(path, this.baseURL) - const mergedQuery: Record = { + const mergedQuery: QueryParams = { ...(this.defaultQuery ?? {}), ...(query ?? {}), } for (const [key, value] of Object.entries(mergedQuery)) { - if (value !== undefined) { - url.searchParams.set(key, String(value)) + if (value === undefined) { + continue } + if (Array.isArray(value)) { + // Repeated params, not a comma-joined value: the API reads list-valued + // query parameters as `?k=a&k=b`, and String([a, b]) would arrive as a + // single malformed entry. + for (const entry of value) { + url.searchParams.append(key, String(entry)) + } + continue + } + url.searchParams.set(key, String(value)) } return url.toString() diff --git a/sdks/typescript/src/index.ts b/sdks/typescript/src/index.ts index 90bff9f2..24ba57b4 100644 --- a/sdks/typescript/src/index.ts +++ b/sdks/typescript/src/index.ts @@ -6,7 +6,13 @@ export { Honcho } from './client' export { Conclusion, type ConclusionCreateParams, - ConclusionScope, + /** + * @deprecated Renamed to `ConclusionsView`. "Scope" now means a named set of + * sessions (see `Scope`), which this class is not — it is a view over one + * observer/observed pair. Kept as an alias for one more minor version. + */ + ConclusionsView as ConclusionScope, + ConclusionsView, } from './conclusions' // HTTP infrastructure export { @@ -30,6 +36,11 @@ export { export { Message, type MessageInput } from './message' export { Page } from './pagination' export { Peer, PeerContext } from './peer' +export { + Scope, + type ScopeBackfillState, + type ScopeStatus, +} from './scope' export { Session } from './session' export { SessionContext, @@ -50,6 +61,9 @@ export type { QueueStatus, QueueStatusResponse, RepresentationOptions, + ScopeBackfillJob, + ScopeResponse, + ScopeStatusResponse, SessionContextResponse, SessionQueueStatus, SessionResponse, diff --git a/sdks/typescript/src/peer.ts b/sdks/typescript/src/peer.ts index adb67cb2..9dab1658 100644 --- a/sdks/typescript/src/peer.ts +++ b/sdks/typescript/src/peer.ts @@ -1,6 +1,6 @@ import { ZodType, z } from 'zod' import { API_VERSION } from './api-version' -import { ConclusionScope } from './conclusions' +import { ConclusionsView } from './conclusions' import type { HonchoHTTPClient } from './http/client' import { createDialecticStream, @@ -8,6 +8,9 @@ import { } from './http/streaming' import { Message, type MessageInput } from './message' import { Page } from './pagination' +// Type-only: scope.ts imports Session, which imports Peer. Importing the type +// keeps that cycle out of the emitted JS. +import type { Scope } from './scope' import { Session } from './session' import type { MessageResponse, @@ -40,6 +43,7 @@ import { peerConfigToApi, RepresentationOptionsSchema, SearchQuerySchema, + scopeRecallFields, sessionConfigFromApi, } from './validation' @@ -251,6 +255,8 @@ export class Peer { stream?: boolean target?: string session_id?: string + scope?: string | string[] + filters?: Record reasoning_level?: string response_format?: Record }): Promise { @@ -265,6 +271,8 @@ export class Peer { query: string target?: string session_id?: string + scope?: string | string[] + filters?: Record reasoning_level?: string response_format?: Record }): Promise { @@ -295,6 +303,8 @@ export class Peer { private async _getRepresentation(params: { session_id?: string + scope?: string | string[] + filters?: Record target?: string search_query?: string search_top_k?: number @@ -365,6 +375,18 @@ export class Peer { * @param options.session - Optional session to scope the query to. If provided, only * information from that session is considered. Can be a session * ID string or a Session object. + * @param options.scope - Optional scope(s) to confine the query to. A single scope answers + * from that scope's own view of the target, including the higher-order + * conclusions reasoned within it. A list of scopes restricts recall to + * the union of their member sessions, which — like `sessions` — yields + * only directly-stated conclusions. Mutually exclusive with `session` + * and `sessions`, and requires a workspace-level key. + * @param options.sessions - Optional allowlist of sessions to confine the query to, for + * one-off questions that span a handful of sessions. Recall is + * limited to conclusions stated directly in those sessions: + * conclusions produced by reasoning across sessions are excluded, + * because their provenance cannot be proven to sit inside the + * allowlist. Reach for a named `scope` when you need that depth. * @param options.reasoningLevel - Optional reasoning level for the query: "minimal", "low", "medium", * "high", or "max". Defaults to "low" if not provided. * @returns Promise resolving to the response string, or null if no relevant information @@ -379,6 +401,16 @@ export class Peer { * target: otherPeer, * reasoningLevel: 'high' * }) + * + * // Answer only from a named scope + * const response = await peer.chat('What is stressing them out?', { + * scope: 'therapy', + * }) + * + * // Answer only from an ad-hoc set of sessions + * const response = await peer.chat('What did we decide?', { + * sessions: [session1, session2], + * }) * ``` */ async chat( @@ -386,6 +418,8 @@ export class Peer { options: { target?: string | Peer session?: string | Session + scope?: string | Scope | (string | Scope)[] + sessions?: (string | Session)[] reasoningLevel?: string responseFormat: ZodType } @@ -395,6 +429,8 @@ export class Peer { options?: { target?: string | Peer session?: string | Session + scope?: string | Scope | (string | Scope)[] + sessions?: (string | Session)[] reasoningLevel?: string responseFormat?: Record } @@ -404,6 +440,8 @@ export class Peer { options?: { target?: string | Peer session?: string | Session + scope?: string | Scope | (string | Scope)[] + sessions?: (string | Session)[] reasoningLevel?: string responseFormat?: ZodType | Record } @@ -423,6 +461,8 @@ export class Peer { query, target: targetId, session: resolvedSessionId, + scope: options?.scope, + sessions: options?.sessions, reasoningLevel: options?.reasoningLevel, responseFormat: options?.responseFormat, }) @@ -437,6 +477,7 @@ export class Peer { stream: false, target: chatParams.target, session_id: chatParams.session, + ...scopeRecallFields(chatParams), reasoning_level: chatParams.reasoningLevel, response_format: Peer.toResponseFormatSchema(options?.responseFormat), }) @@ -465,6 +506,9 @@ export class Peer { * @param options.session - Optional session to scope the query to. If provided, only * information from that session is considered. Can be a session * ID string or a Session object. + * @param options.scope - Optional scope(s) to confine the query to. See {@link Peer.chat}. + * @param options.sessions - Optional allowlist of sessions to confine the query to. + * See {@link Peer.chat} for the depth caveat. * @param options.reasoningLevel - Optional reasoning level for the query: "minimal", "low", "medium", * "high", or "max". Defaults to "low" if not provided. * @returns Promise resolving to a DialecticStreamResponse that can be iterated over @@ -489,6 +533,8 @@ export class Peer { options?: { target?: string | Peer session?: string | Session + scope?: string | Scope | (string | Scope)[] + sessions?: (string | Session)[] reasoningLevel?: string responseFormat?: ZodType | Record } @@ -508,6 +554,8 @@ export class Peer { query, target: targetId, session: resolvedSessionId, + scope: options?.scope, + sessions: options?.sessions, reasoningLevel: options?.reasoningLevel, responseFormat: options?.responseFormat, }) @@ -516,6 +564,7 @@ export class Peer { query: chatParams.query, target: chatParams.target, session_id: chatParams.session, + ...scopeRecallFields(chatParams), reasoning_level: chatParams.reasoningLevel, response_format: Peer.toResponseFormatSchema(options?.responseFormat), }) @@ -846,6 +895,8 @@ export class Peer { */ async representation(options?: { session?: string | Session + scope?: string | Scope | (string | Scope)[] + sessions?: (string | Session)[] target?: string | Peer searchQuery?: string | Message searchTopK?: number @@ -856,6 +907,8 @@ export class Peer { const searchQuery = normalizeSearchQuery(options?.searchQuery) const getRepresentationParams = PeerGetRepresentationParamsSchema.parse({ session: options?.session, + scope: options?.scope, + sessions: options?.sessions, target: options?.target, options: { searchQuery, @@ -878,6 +931,7 @@ export class Peer { const response = await this._getRepresentation({ session_id: sessionId, + ...scopeRecallFields(getRepresentationParams), target: targetId, search_query: searchQuery, search_top_k: getRepresentationParams.options?.searchTopK, @@ -964,7 +1018,7 @@ export class Peer { * This property provides a convenient way to access conclusions that this peer * has made about themselves. Use this for self-conclusion scenarios. * - * @returns A ConclusionScope scoped to this peer's self-conclusions + * @returns A ConclusionsView scoped to this peer's self-conclusions * * @example * ```typescript @@ -978,8 +1032,8 @@ export class Peer { * await peer.conclusions.delete('obs-123') * ``` */ - get conclusions(): ConclusionScope { - return new ConclusionScope( + get conclusions(): ConclusionsView { + return new ConclusionsView( this._http, this.workspaceId, this.id, @@ -995,7 +1049,7 @@ export class Peer { * observer and the target is the observed peer. * * @param target - The target peer (either a Peer object or peer ID string) - * @returns A ConclusionScope scoped to this peer's conclusions of the target + * @returns A ConclusionsView scoped to this peer's conclusions of the target * * @example * ```typescript @@ -1012,9 +1066,9 @@ export class Peer { * const rep = await bobConclusions.representation() * ``` */ - conclusionsOf(target: string | Peer): ConclusionScope { + conclusionsOf(target: string | Peer): ConclusionsView { const targetId = typeof target === 'string' ? target : target.id - return new ConclusionScope( + return new ConclusionsView( this._http, this.workspaceId, this.id, diff --git a/sdks/typescript/src/scope.ts b/sdks/typescript/src/scope.ts new file mode 100644 index 00000000..5a1114fe --- /dev/null +++ b/sdks/typescript/src/scope.ts @@ -0,0 +1,278 @@ +import { API_VERSION } from './api-version' +import type { HonchoHTTPClient } from './http/client' +import { Page } from './pagination' +import { Session } from './session' +import type { + PageResponse, + ScopeStatusResponse, + SessionResponse, +} from './types/api' +import { resolveId } from './utils' +import { + ScopeSessionsSchema, + SessionIdSchema, + sessionConfigFromApi, +} from './validation' + +/** + * Backfill job state for one session in a scope. + */ +export interface ScopeBackfillState { + state: 'pending' | 'completed' | 'failed' + updatedAt: string + /** + * Number of documents copied into the scope. Present only once the backfill + * for this session completes. + */ + docsCopied?: number +} + +/** + * Backfill/reconciliation progress for a scope, keyed by session ID. + * + * Only sessions that have had a backfill enqueued appear. A scope whose + * sessions were all empty when added has an empty `backfillStatus`. + */ +export interface ScopeStatus { + backfillStatus: Record +} + +/** + * Represents a scope in the Honcho system. + * + * A scope is a named set of sessions that acts as a visibility boundary. Recall + * performed through a scope sees only what happened in that scope's sessions, + * while the underlying peer keeps its single unified representation across + * everything it has ever participated in. + * + * Membership changes are applied asynchronously: adding a session that already + * has messages copies its existing conclusions into the scope, and removing one + * reconciles them back out. Poll {@link Scope.status} to watch that settle. + * + * @example + * ```typescript + * const therapy = await honcho.scope('therapy') + * await therapy.addSessions([session1, session2]) + * + * // Ask a question answered only from the therapy sessions + * const answer = await user.chat('What is stressing them out?', { + * scope: 'therapy', + * }) + * ``` + */ +export class Scope { + /** + * Unique identifier for this scope, without the server-side `scope.` prefix. + */ + readonly id: string + /** + * Workspace ID for scoping operations. + */ + readonly workspaceId: string + private _http: HonchoHTTPClient + private _metadata?: Record + private _createdAt?: string + private _ensureWorkspace: () => Promise + + /** + * Cached metadata for this scope. May be stale if the scope was not recently + * fetched from the API. + */ + get metadata(): Record | undefined { + return this._metadata + } + + /** + * Timestamp when this scope was created. Only available if fetched from the API. + */ + get createdAt(): string | undefined { + return this._createdAt + } + + /** + * Initialize a new Scope. **Do not call this directly, use the client.scope() method instead.** + * + * @param id - Unprefixed scope name, unique within the workspace + * @param workspaceId - Workspace ID for scoping operations + * @param http - Reference to the HTTP client instance + * @param metadata - Optional metadata to initialize the cached value + * @param ensureWorkspace - Callback that guarantees the workspace exists + * @param createdAt - Creation timestamp, if already fetched + */ + constructor( + id: string, + workspaceId: string, + http: HonchoHTTPClient, + metadata?: Record, + ensureWorkspace: () => Promise = async () => undefined, + createdAt?: string + ) { + this.id = id + this.workspaceId = workspaceId + this._http = http + this._metadata = metadata + this._ensureWorkspace = ensureWorkspace + this._createdAt = createdAt + } + + // =========================================================================== + // Private API Methods + // =========================================================================== + + private get _basePath(): string { + return `/${API_VERSION}/workspaces/${this.workspaceId}/scopes/${this.id}` + } + + private async _addSessions(sessionIds: string[]): Promise { + await this._ensureWorkspace() + await this._http.post(`${this._basePath}/sessions`, { + body: { session_ids: sessionIds }, + }) + } + + private async _removeSession(sessionId: string): Promise { + await this._ensureWorkspace() + await this._http.delete(`${this._basePath}/sessions/${sessionId}`) + } + + private async _listSessions(params?: { + page?: number + size?: number + reverse?: boolean + }): Promise> { + await this._ensureWorkspace() + return this._http.post>( + `${this._basePath}/sessions/list`, + { + query: { + page: params?.page, + size: params?.size, + reverse: params?.reverse ? 'true' : undefined, + }, + } + ) + } + + private async _getStatus(): Promise { + await this._ensureWorkspace() + return this._http.get(`${this._basePath}/status`) + } + + // =========================================================================== + // Public API Methods + // =========================================================================== + + /** + * Add sessions to this scope. + * + * Every named session must already exist. Adding a session that is already a + * member is a no-op. + * + * Sessions that already hold messages are backfilled into the scope + * asynchronously, so recall through this scope may not reflect their history + * immediately — poll {@link Scope.status} to watch that complete. + * + * @param sessions - Sessions to add, as ID strings or Session objects. At most + * 100 per call, matching the server's limit; split larger + * membership changes into separate calls so a failure names + * the batch that failed. + */ + async addSessions(sessions: (string | Session)[]): Promise { + const sessionIds = ScopeSessionsSchema.parse(sessions.map(resolveId)) + await this._addSessions(sessionIds) + } + + /** + * Remove a session from this scope. + * + * Conclusions copied or derived while the session was a member are + * reconciled out asynchronously, and the scope's peer card is rebuilt from + * whatever evidence remains. Poll {@link Scope.status} to watch that settle. + * + * @param session - Session to remove, as an ID string or a Session object + * @throws If the session ID is malformed + */ + async removeSession(session: string | Session): Promise { + // Validated because this ID is interpolated into a request *path*: an + // unvalidated value silently changes which resource the request addresses. + // `valid-session?typo` would target `valid-session` with a stray query + // string, removing the wrong session and reconciling against it. + await this._removeSession(SessionIdSchema.parse(resolveId(session))) + } + + /** + * Get the sessions that are members of this scope. + * + * Ordered by how long each session has been a member — longest-standing + * first, or most recently added first when `reverse` is true. + * + * @param options - Pagination options: `page`, `size`, and `reverse` + * @returns Promise resolving to a paginated list of member Sessions + */ + async sessions(options?: { + page?: number + size?: number + reverse?: boolean + }): Promise> { + const reverse = options?.reverse + const sessionsPage = await this._listSessions({ + page: options?.page, + size: options?.size, + reverse, + }) + + const fetchNextPage = async ( + page: number, + size: number + ): Promise> => { + return this._listSessions({ page, size, reverse }) + } + + return new Page( + sessionsPage, + (session) => + new Session( + session.id, + this.workspaceId, + this._http, + session.metadata ?? undefined, + sessionConfigFromApi(session.configuration) ?? undefined, + () => this._ensureWorkspace(), + session.created_at, + session.is_active + ), + fetchNextPage + ) + } + + /** + * Get the backfill/reconciliation progress for this scope. + * + * Use this after a membership change to tell "the scope knows nothing about + * that session yet" apart from "the scope has caught up and there is genuinely + * nothing to recall". + * + * @returns Promise resolving to per-session backfill state + */ + async status(): Promise { + const response = await this._getStatus() + return { + backfillStatus: Object.fromEntries( + Object.entries(response.backfill_status ?? {}).map( + ([sessionId, job]) => [ + sessionId, + { + state: job.state, + updatedAt: job.updated_at, + docsCopied: job.docs_copied, + }, + ] + ) + ), + } + } + + toString(): string { + return `Scope(id='${this.id}', workspaceId='${this.workspaceId}')` + } +} diff --git a/sdks/typescript/src/session.ts b/sdks/typescript/src/session.ts index 0f9cf4aa..ed6140a6 100644 --- a/sdks/typescript/src/session.ts +++ b/sdks/typescript/src/session.ts @@ -3,6 +3,9 @@ import type { HonchoHTTPClient } from './http/client' import { Message } from './message' import { Page } from './pagination' import { Peer } from './peer' +// Type-only: scope.ts imports this module. Importing the type keeps that cycle +// out of the emitted JS. +import type { Scope } from './scope' import { SessionContext, SessionSummaries } from './session_context' import type { MessageResponse, @@ -17,7 +20,7 @@ import type { SessionResponse, SessionSummariesResponse, } from './types/api' -import { transformQueueStatus } from './utils' +import { resolveId, transformQueueStatus } from './utils' import { ContextParamsSchema, FileUploadSchema, @@ -220,6 +223,8 @@ export class Session { search_query?: string peer_target?: string peer_perspective?: string + scope?: string + sessions?: string[] limit_to_session?: boolean search_top_k?: number search_max_distance?: number @@ -752,6 +757,17 @@ export class Session { * @param options.tokens - Target token count for the context window * @param options.peerTarget - The peer to get representation for * @param options.peerPerspective - The peer whose perspective to use for representation + * @param options.scope - A scope to use as the perspective source instead of a peer: the + * target's representation and card are read from what that scope + * observed. Requires `peerTarget`, is mutually exclusive with + * `peerPerspective`, and requires a workspace-level key. + * @param options.sessions - Allowlist of sessions confining the target's representation + * to that set. This session must be one of them. Recall is + * limited to conclusions stated directly in those sessions, and + * the peer card is omitted, since neither derived conclusions + * nor cards carry provable per-session provenance. Mutually + * exclusive with `scope` and `limitToSession`; requires + * `peerTarget`. * @param options.limitToSession - Whether to limit representation to this session only * @param options.representationOptions - Options for representation retrieval (searchQuery, searchTopK, etc.) * @returns Promise resolving to a SessionContext with messages, summary, and representation @@ -764,6 +780,12 @@ export class Session { * peerTarget: user * }) * + * // Build the context from what a scope observed + * const ctx = await session.context({ + * peerTarget: user, + * scope: 'therapy', + * }) + * * // Convert to OpenAI format * const messages = ctx.toOpenAI(assistant) * ``` @@ -773,6 +795,8 @@ export class Session { tokens?: number peerTarget?: string | Peer peerPerspective?: string | Peer + scope?: string | Scope + sessions?: (string | Session)[] limitToSession?: boolean representationOptions?: RepresentationOptions }): Promise { @@ -795,6 +819,10 @@ export class Session { tokens: opts.tokens, peerTarget: peerTargetId, peerPerspective: peerPerspectiveId, + // Checked against undefined, not truthiness: `scope: ''` must reach the + // schema and be rejected, not be dropped into an unscoped context. + scope: opts.scope !== undefined ? resolveId(opts.scope) : undefined, + sessions: opts.sessions, limitToSession: opts.limitToSession, representationOptions: opts.representationOptions ? { @@ -810,6 +838,8 @@ export class Session { search_query: searchQuery, peer_target: contextParams.peerTarget, peer_perspective: contextParams.peerPerspective, + scope: contextParams.scope, + sessions: contextParams.sessions, limit_to_session: contextParams.limitToSession, search_top_k: contextParams.representationOptions?.searchTopK, search_max_distance: diff --git a/sdks/typescript/src/types/api.ts b/sdks/typescript/src/types/api.ts index dbe4378d..10ed6037 100644 --- a/sdks/typescript/src/types/api.ts +++ b/sdks/typescript/src/types/api.ts @@ -133,6 +133,28 @@ export interface SessionCreateParams { metadata?: Record configuration?: SessionConfigApi peers?: Record + scopes?: string[] +} + +export interface ScopeResponse { + id: string + metadata: Record + created_at: string +} + +/** + * Per-session backfill job state for a scope. + * + * `docs_copied` is present only once a backfill completes. + */ +export interface ScopeBackfillJob { + state: 'pending' | 'completed' | 'failed' + updated_at: string + docs_copied?: number +} + +export interface ScopeStatusResponse { + backfill_status: Record } export interface SessionUpdateParams { diff --git a/sdks/typescript/src/validation.ts b/sdks/typescript/src/validation.ts index 48255833..832690e2 100644 --- a/sdks/typescript/src/validation.ts +++ b/sdks/typescript/src/validation.ts @@ -159,6 +159,149 @@ export const SessionIdSchema = z */ const SessionIdObjectSchema = z.object({ id: SessionIdSchema }) +/** + * Reserved peer-name prefix the server uses to store a scope. + */ +const SCOPE_PEER_PREFIX = 'scope.' + +/** + * Scope IDs are stored as peer names with the reserved prefix prepended, so + * they must leave room for it within the 512-character peer name limit. + */ +const SCOPE_ID_MAX_LENGTH = 512 - SCOPE_PEER_PREFIX.length + +/** + * The scope ID rules, as a plain function rather than only a schema. + * + * Zod reports a failing union as a single `invalid_union` / "Invalid input" + * issue and buries the branch errors, so a schema alone cannot carry these + * messages out of `ScopeOptionSchema`. Keeping the rules callable lets both the + * bare schema and the union surface the same specific message. + * + * @returns The problems found, or an empty array when the ID is valid. + */ +function scopeIdIssues(value: string): string[] { + if (value.length < 1) { + return ['Scope ID must be a non-empty string'] + } + if (value.length > SCOPE_ID_MAX_LENGTH) { + return [`Scope ID can be at most ${SCOPE_ID_MAX_LENGTH} characters`] + } + // Checked before the charset: the reserved prefix contains '.', which is + // itself outside the charset, so a charset-first check would report the + // charset instead of the real mistake for a double-prefixed name. + if (value.startsWith(SCOPE_PEER_PREFIX)) { + return [ + `Scope ID must not start with the reserved prefix '${SCOPE_PEER_PREFIX}' (scope IDs are unprefixed)`, + ] + } + if (!/^[a-zA-Z0-9_-]+$/.test(value)) { + return [ + 'Scope ID may only contain letters, numbers, underscores, and hyphens', + ] + } + return [] +} + +/** + * Add every scope ID problem in `values` as a top-level issue. + */ +function addScopeIdIssues(values: string[], ctx: z.RefinementCtx): void { + for (const value of values) { + for (const message of scopeIdIssues(value)) { + ctx.addIssue({ code: z.ZodIssueCode.custom, message }) + } + } +} + +/** + * Schema for scope ID validation. + * + * Scope IDs are unprefixed — the `scope.` prefix is a server-side storage + * detail and never appears on the wire. + */ +export const ScopeIdSchema = z.string().superRefine((val, ctx) => { + addScopeIdIssues([val], ctx) +}) + +/** + * Shape-only branch for the `scope` option: an ID string, or an object carrying + * one (so a `Scope` instance is accepted). The ID itself is validated after the + * union resolves — see `ScopeOptionSchema`. + */ +const ScopeIdLikeSchema = z.union([z.string(), z.object({ id: z.string() })]) + +/** + * Schema for the `scope` read option: one scope, or a bounded list of them. + * + * A single scope reads that scope's own view. A list restricts recall to the + * union of the scopes' member sessions. An empty list is rejected rather than + * resolved to an empty allowlist, which would silently recall nothing. + * + * The union discriminates shape only; IDs and list bounds are checked after the + * transform so their messages are not swallowed as `invalid_union`. + */ +export const ScopeOptionSchema = z + .union([ScopeIdLikeSchema, z.array(ScopeIdLikeSchema)]) + .transform((val) => + Array.isArray(val) + ? val.map((entry) => (typeof entry === 'string' ? entry : entry.id)) + : typeof val === 'string' + ? val + : val.id + ) + .superRefine((resolved, ctx) => { + if (Array.isArray(resolved)) { + if (resolved.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'scope must name at least one scope', + }) + } + if (resolved.length > 100) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'scope can name at most 100 scopes', + }) + } + } + addScopeIdIssues(Array.isArray(resolved) ? resolved : [resolved], ctx) + }) + +/** + * Schema for a scope membership change: the sessions to add to a scope. + * + * Capped at 100 to match the server rather than silently chunking, so a + * rejected batch is the batch the caller passed. + */ +export const ScopeSessionsSchema = z + .array(SessionIdSchema) + .min(1, 'At least one session must be given') + .max(100, 'At most 100 sessions can be added per call') + +/** + * Schema for the `scopes` option on session creation: the scopes a new session + * should join. + */ +export const SessionScopesSchema = z + .array(ScopeIdSchema) + .min(1, 'scopes must name at least one scope') + .max(100, 'scopes can name at most 100 scopes') + +/** + * Schema for the `sessions` allowlist option — sugar for the wire-level + * `filters: { session_id: [...] }`. + * + * Capped at 1,000 entries to match the server. An empty list is rejected: the + * server treats an empty allowlist as fail-closed (recalls nothing), which is + * never what a caller passing `sessions: []` intends. + */ +export const SessionAllowlistSchema = z + .array(z.union([SessionIdSchema, SessionIdObjectSchema])) + .min(1, 'sessions must name at least one session') + .max(1000, 'sessions can name at most 1000 sessions') + .transform((vals) => vals.map((v) => (typeof v === 'string' ? v : v.id))) + /** * Schema for session peer configuration. */ @@ -291,6 +434,58 @@ export function normalizeListOptions( return { filters: input as Filters } as T } +/** + * Translate validated `scope` / `sessions` options into their wire fields. + * + * `sessions` is sugar: it goes out as the constrained + * `filters: { session_id: [...] }` body the recall endpoints accept, never as a + * field of its own — the server rejects unknown keys with a 422. Shared by chat, + * chatStream, and representation so the three cannot drift apart. + * + * Purely a translation; the schemas have already rejected the invalid + * combinations by the time this runs. + */ +export function scopeRecallFields(options: { + scope?: string | string[] + sessions?: string[] +}): { scope?: string | string[]; filters?: Record } { + return { + scope: options.scope, + filters: options.sessions ? { session_id: options.sessions } : undefined, + } +} + +/** + * Add issues for the `scope` exclusions the server enforces with a 422. + * + * A scope already determines what a query can see, so combining it with a + * session allowlist or a single session is a contradiction rather than a + * narrowing. Shared by the chat, representation, and context schemas so the + * three surfaces cannot drift apart. + */ +function scopeExclusivityIssues( + data: { scope?: unknown; sessions?: unknown; session?: unknown }, + ctx: z.RefinementCtx +): void { + if (data.scope === undefined) { + return + } + if (data.sessions !== undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'scope and sessions are mutually exclusive', + path: ['sessions'], + }) + } + if (data.session !== undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'scope and session are mutually exclusive', + path: ['session'], + }) + } +} + /** * Schema for chat query parameters. */ @@ -309,6 +504,8 @@ export const ChatQuerySchema = z .transform((val) => val ? (typeof val === 'string' ? val : val.id) : undefined ), + scope: ScopeOptionSchema.optional(), + sessions: SessionAllowlistSchema.optional(), reasoningLevel: z .enum(['minimal', 'low', 'medium', 'high', 'max']) .optional(), @@ -319,6 +516,7 @@ export const ChatQuerySchema = z .optional(), }) .strict() + .superRefine(scopeExclusivityIssues) /** * Schema for representation options. @@ -356,11 +554,40 @@ export const ContextParamsSchema = z tokens: z.int('Token limit must be an integer').optional(), peerTarget: PeerIdSchema.optional(), peerPerspective: PeerIdSchema.optional(), + // Only a single scope is accepted here: the context route uses a scope as + // the *perspective source* for the target's representation and card, which + // is one observer. A list of scopes has no meaning for that. + scope: ScopeIdSchema.optional(), + sessions: SessionAllowlistSchema.optional(), limitToSession: z.boolean().optional(), representationOptions: RepresentationOptionsSchema.optional(), }) .strict() .superRefine((data, ctx) => { + if (data.sessions && !data.peerTarget) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'peerTarget is required when sessions is provided', + path: ['sessions'], + }) + } + + if (data.sessions && data.scope) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'sessions and scope are mutually exclusive', + path: ['sessions'], + }) + } + + if (data.sessions && data.limitToSession) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'sessions and limitToSession are mutually exclusive', + path: ['sessions'], + }) + } + if (data.representationOptions?.searchQuery && !data.peerTarget) { ctx.addIssue({ code: z.ZodIssueCode.custom, @@ -376,6 +603,22 @@ export const ContextParamsSchema = z path: ['peerPerspective'], }) } + + if (data.scope && !data.peerTarget) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'peerTarget is required when scope is provided', + path: ['scope'], + }) + } + + if (data.scope && data.peerPerspective) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'scope and peerPerspective are mutually exclusive', + path: ['scope'], + }) + } }) /** @@ -437,10 +680,13 @@ export const GetRepresentationParamsSchema = z export const PeerGetRepresentationParamsSchema = z .object({ session: z.union([SessionIdSchema, SessionIdObjectSchema]).optional(), + scope: ScopeOptionSchema.optional(), + sessions: SessionAllowlistSchema.optional(), target: z.union([PeerIdSchema, PeerIdObjectSchema]).optional(), options: RepresentationOptionsSchema.optional(), }) .strict() + .superRefine(scopeExclusivityIssues) /** * Schema for peer card target parameter. diff --git a/src/deriver/queue_manager.py b/src/deriver/queue_manager.py index e4c8124b..1c19c131 100644 --- a/src/deriver/queue_manager.py +++ b/src/deriver/queue_manager.py @@ -478,8 +478,7 @@ class QueueManager: @staticmethod def _is_tenant_work(work_unit_keys: Iterable[str]) -> bool: - """True if any claimed work unit is real tenant work, not housekeeping. - """ + """True if any claimed work unit is real tenant work, not housekeeping.""" for key in work_unit_keys: try: if parse_work_unit_key(key).task_type != "reconciler": diff --git a/src/routers/sessions.py b/src/routers/sessions.py index 10b769c4..f0d5b056 100644 --- a/src/routers/sessions.py +++ b/src/routers/sessions.py @@ -11,6 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from src import config, crud, schemas from src.cache.client import safe_cache_delete +from src.crud.message import get_peer_session_names from src.crud.session import session_cache_key from src.dependencies import db, read_db from src.deriver.enqueue import enqueue_deletion @@ -23,6 +24,7 @@ from src.exceptions import ( from src.security import JWTParams, require_auth from src.telemetry.events import EmbeddingCallPurpose, GetContextEvent, emit from src.utils import summarizer +from src.utils.filter import normalize_session_allowlist from src.utils.representation import Representation from src.utils.search import search from src.utils.tokens import estimate_tokens @@ -714,9 +716,27 @@ async def get_session_context( None, description="An (unprefixed) scope name to use as the perspective source: the representation and peer card of `peer_target` are read from the scope's observations instead of the global (or `peer_perspective`) view. Must be provided with `peer_target`; mutually exclusive with `peer_perspective`. Requires a workspace- or admin-level key.", ), + sessions: list[str] | None = Query( + None, + description=( + "Optional allowlist of session IDs confining the representation of " + "`peer_target` to those sessions. This session must be one of them. " + "Recall is restricted to conclusions stated directly in the allowed " + "sessions — conclusions synthesized across sessions are excluded, " + "since their provenance cannot be proven to sit inside the allowlist " + "— and the peer card is omitted for the same reason. Mutually " + "exclusive with `scope` and `limit_to_session`. A peer-scoped key " + "must be an active member of every session named. The 1,000-session " + "cap shared with the recall endpoints applies but is not reachable " + "here: these are repeated query parameters, so a long list exceeds " + "the request-line limit of the server or any proxy in front of it " + "(a 414/431, not a 422) at a few hundred entries. Use a named " + "`scope` for large or reusable session sets." + ), + ), limit_to_session: bool = Query( default=False, - description="Only used if `search_query` is provided. Whether to limit the representation to the session (as opposed to everything known about the target peer)", + description="Whether to limit the representation to the session (as opposed to everything known about the target peer). Narrows recall the same way `sessions` does, so the same restrictions apply: explicit-only conclusions, and the peer card is omitted because it carries no per-session provenance.", ), search_top_k: int | None = Query( None, @@ -801,6 +821,47 @@ async def get_session_context( "`scope` requires a workspace- or admin-level key" ) + # The session allowlist confines the representation to a set of sessions this + # one belongs to. `scope` already determines what can be seen and + # `limit_to_session` already pins the set to this session alone, so both are + # contradictions rather than further narrowings — refused rather than given a + # silent precedence order. + session_allowlist: list[str] | None = None + if sessions is not None: + if scope is not None: + raise ValidationException("`sessions` and `scope` are mutually exclusive") + if limit_to_session: + raise ValidationException( + "`sessions` and `limit_to_session` are mutually exclusive" + ) + if not peer_target: + # The allowlist only reaches the representation, and there is no + # representation without a target. Refused rather than accepted and + # silently ignored, which would read as a scoped context. + raise ValidationException( + "peer_target must be provided if sessions is provided" + ) + # `must_include` keeps the allowlist from contradicting the route's own + # session: this session's messages and summary are always part of the + # response, so an allowlist excluding it would describe a context that + # cannot be assembled. + session_allowlist = normalize_session_allowlist( + sessions, field="sessions", must_include=session_id + ) + # A peer-scoped key may only name sessions its peer belongs to. Mirrors + # the chat route's gate (see routers/peers.py), including `active_only`, + # so both answer the same question for a peer that has left a session. + # Reuses the handler's session rather than opening its own: this is a + # DB-only read and the handler already holds a connection. + if jwt_params.p is not None: + member_sessions = set( + await get_peer_session_names( + db, workspace_id, jwt_params.p, active_only=True + ) + ) + if not set(session_allowlist) <= member_sessions: + raise AuthenticationException("JWT not permissioned for this resource") + if not peer_target: # No representation or card needed summary, messages = await _get_session_context_task( @@ -865,6 +926,17 @@ async def get_session_context( ): embedding = await embedding_client.embed(search_query) + # The allowlist recall must respect, whichever way the caller expressed it. + # `sessions` and `limit_to_session` are mutually exclusive (422 above), so at + # most one of these is set. `session_allowlist` is never an empty list here — + # `must_include=session_id` guarantees at least this session — so the + # None-check is the only distinction that matters. + effective_allowlist = ( + session_allowlist + if session_allowlist is not None + else ([session_id] if limit_to_session else None) + ) + # Sequential calls on shared DB session representation = await _get_working_representation_task( db, @@ -872,15 +944,34 @@ async def get_session_context( search_query, observer=observer, observed=observed, - session_allowlist=[session_id] if limit_to_session else None, + session_allowlist=effective_allowlist, search_top_k=search_top_k, search_max_distance=search_max_distance, include_most_derived=include_most_frequent, max_observations=max_conclusions, embedding=embedding, ) - card = await _get_peer_card_task( - db, workspace_id, observer=observer, observed=observed + # A peer card is keyed by (workspace, observer, observed) with no session + # dimension (crud/peer_card.py), so it is synthesized from everything the + # observer has ever seen and cannot be narrowed to an allowlist. Returning it + # would leak exactly what the allowlist exists to exclude, so it is dropped — + # the same fail-closed reasoning that limits allowlisted conclusion recall to + # ALLOWLIST_SAFE_LEVELS. + # + # Gated on the *effective* allowlist, not on `sessions` alone: + # `limit_to_session=true` narrows recall identically, so carving out only the + # newer parameter would leave a control that one parameter swap defeats. + # `scope` needs no carve-out at all — it swaps the observer to the scope peer + # above, so the card read below is the scope's own. + # + # POST /peers/{id}/chat still injects an unscoped card under an allowlist + # (src/dialectic/chat.py) — tracked in DEV-2201, not fixed here. + card = ( + None + if effective_allowlist is not None + else await _get_peer_card_task( + db, workspace_id, observer=observer, observed=observed + ) ) short_summary, long_summary = await _get_both_summaries_task( db, workspace_id, session_id diff --git a/src/utils/filter.py b/src/utils/filter.py index 1155d408..22d648af 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -279,16 +279,50 @@ def extract_session_allowlist( 'filters.session_id must be a session id, a list of session ids, or {"in": [...]}' ) + return normalize_session_allowlist( + entries, field="filters.session_id", must_include=must_include + ) + + +def normalize_session_allowlist( + entries: Sequence[Any], + *, + field: str, + must_include: str | None = None, +) -> list[str]: + """Validate and de-duplicate a session allowlist. + + Shared by every route-level entry point that accepts one — the ``filters`` + body on the recall endpoints and the ``sessions`` query parameter on session + context — so the cap, the id charset, and the ``must_include`` rule cannot + drift apart between them. Only the parameter *name* in error messages + differs, which is what ``field`` supplies. + + Args: + entries: Raw allowlist entries as the caller supplied them. + field: Caller-facing parameter name, used in error messages. + must_include: A session id that must appear in the allowlist — used by + routes that also carry a session of their own, so the two can't + contradict each other. + + Returns: + The allowlist, de-duplicated, in first-seen order. An empty input yields + an empty list so downstream consumers fail closed. + + Raises: + FilterError: On an over-cap list, a malformed session id, or a + ``must_include`` session missing from the allowlist. + """ if len(entries) > MAX_SESSION_ALLOWLIST_ENTRIES: raise FilterError( - f"filters.session_id supports at most {MAX_SESSION_ALLOWLIST_ENTRIES} sessions per request" + f"{field} supports at most {MAX_SESSION_ALLOWLIST_ENTRIES} sessions per request" ) allowlist: list[str] = [] seen: set[str] = set() for entry in entries: if not isinstance(entry, str) or not entry: - raise FilterError("filters.session_id entries must be non-empty strings") + raise FilterError(f"{field} entries must be non-empty strings") # Only names a session could actually have. The allowlist reaches # queries three ways — direct `IN`, the filter DSL, and a Python # membership test — and they don't agree on a value like "*", which the @@ -298,14 +332,14 @@ def extract_session_allowlist( # {"in": [...]}) which never included wildcards. if not re.fullmatch(RESOURCE_NAME_PATTERN, entry): raise FilterError( - f"Invalid session id in filters.session_id: {entry!r}. Session ids match {RESOURCE_NAME_PATTERN}" + f"Invalid session id in {field}: {entry!r}. Session ids match {RESOURCE_NAME_PATTERN}" ) if entry not in seen: seen.add(entry) allowlist.append(entry) if must_include is not None and must_include not in seen: - raise FilterError("session_id must be included in filters.session_id") + raise FilterError(f"session_id must be included in {field}") return allowlist diff --git a/tests/sdk/test_conclusions.py b/tests/sdk/test_conclusions.py index 9c2cdbc0..314717dc 100644 --- a/tests/sdk/test_conclusions.py +++ b/tests/sdk/test_conclusions.py @@ -6,7 +6,7 @@ from sdks.python.src.honcho.client import Honcho from sdks.python.src.honcho.conclusions import ( Conclusion, ConclusionCreateParams, - ConclusionScope, + ConclusionsView, ) @@ -34,7 +34,7 @@ async def test_observation_create_single( # Get observation scope for observer -> target obs_scope = observer.conclusions_of(target) - assert isinstance(obs_scope, ConclusionScope) + assert isinstance(obs_scope, ConclusionsView) # Create a single observation created = await obs_scope.aio.create( @@ -68,7 +68,7 @@ async def test_observation_create_single( # Get observation scope for observer -> target obs_scope = observer.conclusions_of(target) - assert isinstance(obs_scope, ConclusionScope) + assert isinstance(obs_scope, ConclusionsView) # Create a single observation created = obs_scope.create( @@ -422,7 +422,7 @@ async def test_self_observation_create( # Get self-observation scope obs_scope = peer.conclusions - assert isinstance(obs_scope, ConclusionScope) + assert isinstance(obs_scope, ConclusionsView) assert obs_scope.observer == peer.id assert obs_scope.observed == peer.id @@ -443,7 +443,7 @@ async def test_self_observation_create( # Get self-observation scope obs_scope = peer.conclusions - assert isinstance(obs_scope, ConclusionScope) + assert isinstance(obs_scope, ConclusionsView) assert obs_scope.observer == peer.id assert obs_scope.observed == peer.id @@ -796,7 +796,7 @@ async def test_list_rejects_reserved_scope_filter_keys( target = await honcho_client.aio.peer(id="test-obs-reserved-list-target") obs_scope = observer.conclusions_of(target) for key in reserved: - with pytest.raises(ValueError, match="managed by this conclusion scope"): + with pytest.raises(ValueError, match="managed by this conclusions view"): await obs_scope.aio.list(filters={key: "someone-else"}) # A non-reserved filter (level) is allowed through. await obs_scope.aio.list(filters={"level": "explicit"}) @@ -805,7 +805,7 @@ async def test_list_rejects_reserved_scope_filter_keys( target = honcho_client.peer(id="test-obs-reserved-list-target") obs_scope = observer.conclusions_of(target) for key in reserved: - with pytest.raises(ValueError, match="managed by this conclusion scope"): + with pytest.raises(ValueError, match="managed by this conclusions view"): obs_scope.list(filters={key: "someone-else"}) obs_scope.list(filters={"level": "explicit"}) @@ -827,7 +827,7 @@ async def test_query_rejects_reserved_scope_filter_keys( target = await honcho_client.aio.peer(id="test-obs-reserved-query-target") obs_scope = observer.conclusions_of(target) for key in reserved: - with pytest.raises(ValueError, match="managed by this conclusion scope"): + with pytest.raises(ValueError, match="managed by this conclusions view"): await obs_scope.aio.query("q", filters={key: "someone-else"}) # session_id is a normal filter for query (no dedicated param) — allowed. await obs_scope.aio.query("q", filters={"session_id": "some-session"}) @@ -836,6 +836,6 @@ async def test_query_rejects_reserved_scope_filter_keys( target = honcho_client.peer(id="test-obs-reserved-query-target") obs_scope = observer.conclusions_of(target) for key in reserved: - with pytest.raises(ValueError, match="managed by this conclusion scope"): + with pytest.raises(ValueError, match="managed by this conclusions view"): obs_scope.query("q", filters={key: "someone-else"}) obs_scope.query("q", filters={"session_id": "some-session"}) diff --git a/tests/sdk/test_scope_options.py b/tests/sdk/test_scope_options.py new file mode 100644 index 00000000..fddfe51d --- /dev/null +++ b/tests/sdk/test_scope_options.py @@ -0,0 +1,236 @@ +"""Unit tests for the SDK's scope / session-allowlist option handling. + +Pure logic — no server, no database. These pin the wire translation the server +expects, so a rename or a shape change fails here rather than as a 422 at runtime. +""" + +import sys +from pathlib import Path + +import pytest + +# Add the SDK src to the path to allow imports +sdk_src_path = Path(__file__).parent.parent.parent / "sdks" / "python" / "src" +sys.path.insert(0, str(sdk_src_path)) + +from sdks.python.src.honcho.utils.scopes import ( # noqa: E402 + MAX_SCOPES_PER_OPTION, + MAX_SESSION_ALLOWLIST_ENTRIES, + MAX_SESSIONS_PER_ADD, + resolve_scope_membership, + resolve_scope_option, + resolve_scope_session, + scope_context_fields, + scope_recall_fields, + validate_scope_id, +) + + +def context_fields(**overrides: object) -> dict[str, object]: + """Call scope_context_fields with the neutral defaults filled in.""" + kwargs: dict[str, object] = { + "scope": None, + "sessions": None, + "peer_target": "user", + "peer_perspective": None, + "limit_to_session": False, + } + kwargs.update(overrides) + return scope_context_fields(**kwargs) # pyright: ignore[reportArgumentType] + + +class TestValidateScopeId: + def test_accepts_a_plain_name(self): + assert validate_scope_id("therapy") == "therapy" + + def test_rejects_the_reserved_prefix_by_name(self): + # 'scope.therapy' violates both the prefix rule and the charset. The + # prefix message is the actionable one, so it must be the one raised. + with pytest.raises(ValueError, match="reserved prefix"): + validate_scope_id("scope.therapy") + + def test_rejects_characters_outside_the_charset(self): + with pytest.raises(ValueError, match="must match pattern"): + validate_scope_id("my scope") + + def test_rejects_empty(self): + with pytest.raises(ValueError, match="between 1 and"): + validate_scope_id("") + + def test_rejects_a_name_that_leaves_no_room_for_the_prefix(self): + # 512 - len("scope.") is the ceiling: the server stores the name prefixed + # into a 512-character peer name. + with pytest.raises(ValueError, match="between 1 and"): + validate_scope_id("a" * 507) + + +class TestResolveScopeOption: + def test_a_single_scope_stays_a_string(self): + # The shapes are not interchangeable to the server: one scope reads that + # scope's own view, a list restricts to the union of member sessions. + assert resolve_scope_option("therapy") == "therapy" + + def test_a_sequence_becomes_a_list(self): + assert resolve_scope_option(["therapy", "work"]) == ["therapy", "work"] + + def test_rejects_an_empty_sequence(self): + with pytest.raises(ValueError, match="at least one scope"): + resolve_scope_option([]) + + def test_rejects_an_over_cap_sequence(self): + with pytest.raises(ValueError, match="at most"): + resolve_scope_option([f"s{i}" for i in range(MAX_SCOPES_PER_OPTION + 1)]) + + +class TestScopeRecallFields: + def test_neither_option_contributes_nothing(self): + assert scope_recall_fields(scope=None, sessions=None) == {} + + def test_sessions_becomes_a_session_id_filter(self): + # `sessions` is sugar. It must never reach the wire as its own key — + # the server rejects unknown keys with a 422. + fields = scope_recall_fields(scope=None, sessions=["a", "b"]) + assert fields == {"filters": {"session_id": ["a", "b"]}} + assert "sessions" not in fields + + def test_scope_passes_through_under_its_own_key(self): + assert scope_recall_fields(scope="therapy", sessions=None) == { + "scope": "therapy" + } + + def test_scope_and_sessions_are_mutually_exclusive(self): + with pytest.raises(ValueError, match="mutually exclusive"): + scope_recall_fields(scope="therapy", sessions=["a"]) + + def test_scope_and_a_single_session_are_mutually_exclusive(self): + with pytest.raises(ValueError, match="mutually exclusive"): + scope_recall_fields(scope="therapy", sessions=None, session_id="a") + + def test_sessions_composes_with_a_single_session(self): + # Unlike `scope`, an allowlist may accompany a session_id — the server + # only requires that the session be inside the allowlist. + assert scope_recall_fields(scope=None, sessions=["a", "b"], session_id="a") == { + "filters": {"session_id": ["a", "b"]} + } + + def test_rejects_an_empty_allowlist(self): + # An empty allowlist is fail-closed server-side (recalls nothing), which + # is never what `sessions=[]` intends. + with pytest.raises(ValueError, match="at least one session"): + scope_recall_fields(scope=None, sessions=[]) + + def test_rejects_an_over_cap_allowlist(self): + with pytest.raises(ValueError, match="at most"): + scope_recall_fields( + scope=None, + sessions=[f"s{i}" for i in range(MAX_SESSION_ALLOWLIST_ENTRIES + 1)], + ) + + def test_resolves_objects_with_an_id(self): + class FakeSession: + id: str = "session-a" + + fields = scope_recall_fields(scope=None, sessions=[FakeSession()]) # pyright: ignore[reportArgumentType] + assert fields == {"filters": {"session_id": ["session-a"]}} + + +class TestScopeContextFields: + """The context route takes these as query params, not as a `filters` body.""" + + def test_neither_option_contributes_nothing(self): + assert context_fields() == {} + + def test_scope_passes_through(self): + assert context_fields(scope="therapy") == {"scope": "therapy"} + + def test_sessions_stays_a_plain_list(self): + # Not wrapped in `filters` — this route reads a repeated query parameter. + assert context_fields(sessions=["a", "b"]) == {"sessions": ["a", "b"]} + + def test_scope_and_peer_perspective_are_mutually_exclusive(self): + with pytest.raises(ValueError, match="mutually exclusive"): + context_fields(scope="therapy", peer_perspective="assistant") + + def test_scope_and_sessions_are_mutually_exclusive(self): + with pytest.raises(ValueError, match="mutually exclusive"): + context_fields(scope="therapy", sessions=["a"]) + + def test_sessions_and_limit_to_session_are_mutually_exclusive(self): + with pytest.raises(ValueError, match="mutually exclusive"): + context_fields(sessions=["a"], limit_to_session=True) + + @pytest.mark.parametrize("option", [{"scope": "therapy"}, {"sessions": ["a"]}]) + def test_either_option_requires_a_peer_target(self, option: dict[str, object]): + # Both only reach the representation, and there is none without a target. + # Refused rather than accepted and silently ignored. + with pytest.raises(ValueError, match="peer_target"): + context_fields(peer_target=None, **option) + + def test_limit_to_session_alone_is_untouched(self): + # The neutral case must not start emitting a scope/sessions key. + assert context_fields(limit_to_session=True) == {} + + +class TestResolveScopeMembership: + def test_resolves_ids_and_objects_in_order(self): + class FakeSession: + id: str = "session-b" + + assert resolve_scope_membership(["session-a", FakeSession()]) == [ # pyright: ignore[reportArgumentType] + "session-a", + "session-b", + ] + + def test_rejects_empty(self): + with pytest.raises(ValueError, match="At least one session"): + resolve_scope_membership([]) + + def test_rejects_over_the_per_call_cap_rather_than_chunking(self): + with pytest.raises(ValueError, match="At most"): + resolve_scope_membership([f"s{i}" for i in range(MAX_SESSIONS_PER_ADD + 1)]) + + def test_rejects_a_malformed_id(self): + with pytest.raises(ValueError, match="must match pattern"): + resolve_scope_membership(["ok-session", "valid-session?typo"]) + + +class TestResolveScopeSession: + """Guards the ID that gets interpolated into a scope membership URL path.""" + + def test_resolves_a_plain_id(self): + assert resolve_scope_session("session-a") == "session-a" + + def test_resolves_an_object(self): + class FakeSession: + id: str = "session-a" + + assert resolve_scope_session(FakeSession()) == "session-a" # pyright: ignore[reportArgumentType] + + @pytest.mark.parametrize( + "malformed", + [ + "valid-session?typo", # would address `valid-session` + a query string + "valid-session/../other", # would climb the path + "valid session", + "", + ], + ) + def test_rejects_ids_that_would_alter_the_request_path(self, malformed: str): + # This value lands in a DELETE path. An unvalidated id silently changes + # which session is removed, and removal triggers reconciliation against + # whatever it hits. + with pytest.raises(ValueError, match="Session ID"): + resolve_scope_session(malformed) + + +def test_deprecated_conclusion_scope_aliases_still_resolve(): + """The rename keeps working for callers on the old name.""" + from sdks.python.src.honcho import ( + ConclusionScope, + ConclusionScopeAio, + ConclusionsView, + ConclusionsViewAio, + ) + + assert ConclusionScope is ConclusionsView + assert ConclusionScopeAio is ConclusionsViewAio From a915d298f21017f34790448ae482cbd74a084e4e Mon Sep 17 00:00:00 2001 From: ZiYun Liu <95838698+DaShygoa@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:57:20 +0800 Subject: [PATCH 3/8] fix: force LF line endings for shell scripts (#1017) docker/entrypoint.sh runs under dash inside the container image. A Windows checkout with core.autocrlf=true rewrites it to CRLF, and dash aborts at startup because the carriage return becomes part of the -e flag argument. --- .gitattributes | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..9174f419 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,5 @@ +# Shell entrypoints are executed with sh/dash inside the container image. A +# Windows checkout with core.autocrlf=true rewrites them to CRLF, and dash +# then aborts with "set: Illegal option" because the carriage return becomes +# part of the "-e" flag argument (docker/entrypoint.sh). +*.sh text eol=lf From bd5fd4df62b5002b7aeff6e7f5a5237eb7157260 Mon Sep 17 00:00:00 2001 From: Serhii Zghama <20826225+serhiizghama@users.noreply.github.com> Date: Thu, 20 Aug 2026 00:57:49 +0700 Subject: [PATCH 4/8] fix: honor DERIVER_DEDUPLICATE in create_observations (#1018) * fix: honor DERIVER_DEDUPLICATE in create_observations The agent-tool path hardcoded deduplicate=True, so DERIVER_DEDUPLICATE=false could not disable dedup for observations created through this path. Pass settings.DERIVER.DEDUPLICATE, matching crud/representation.py. * test: cover deduplicate setting is forwarded in create_observations --- src/utils/agent_tools.py | 2 +- tests/utils/test_agent_tools.py | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/utils/agent_tools.py b/src/utils/agent_tools.py index 9f214009..7f094154 100644 --- a/src/utils/agent_tools.py +++ b/src/utils/agent_tools.py @@ -1010,7 +1010,7 @@ async def create_observations( workspace_name=workspace_name, observer=observer, observed=observed, - deduplicate=True, + deduplicate=settings.DERIVER.DEDUPLICATE, ) ).created_documents logger.info( diff --git a/tests/utils/test_agent_tools.py b/tests/utils/test_agent_tools.py index c13fb1e7..e53598fa 100644 --- a/tests/utils/test_agent_tools.py +++ b/tests/utils/test_agent_tools.py @@ -541,6 +541,62 @@ class TestCreateObservations: batch_embed.assert_not_awaited() create_documents.assert_not_awaited() + @pytest.mark.parametrize("deduplicate_setting", [True, False]) + async def test_create_observations_honors_deduplicate_setting( + self, + tool_test_data: Any, + monkeypatch: pytest.MonkeyPatch, + deduplicate_setting: bool, + ): + """create_observations forwards settings.DERIVER.DEDUPLICATE to create_documents. + + Guards against reintroducing a hardcoded deduplicate=True, which made + DERIVER_DEDUPLICATE=false unable to disable dedup on this path (#989). + """ + workspace, peer1, peer2, session, _, _ = tool_test_data + monkeypatch.setattr(settings.DERIVER, "DEDUPLICATE", deduplicate_setting) + + captured: dict[str, Any] = {} + + async def fake_batch_embed(texts: list[str]) -> list[list[float]]: + return [[0.1, 0.2, 0.3] for _ in texts] + + async def fake_create_documents( + _db: AsyncSession, + documents: list[Any], + workspace_name: str, + *, + observer: str, + observed: str, + deduplicate: bool = False, + ) -> crud.CreateDocumentsResult: + _ = (workspace_name, observer, observed) + captured["deduplicate"] = deduplicate + return crud.CreateDocumentsResult(created_documents=documents) + + monkeypatch.setattr( + "src.utils.agent_tools.embedding_client.simple_batch_embed", + fake_batch_embed, + ) + monkeypatch.setattr( + "src.utils.agent_tools.crud.create_documents", fake_create_documents + ) + + result = await create_observations( + observations=[ + schemas.ObservationInput(content="An observation", level="explicit"), + ], + observer=peer1.name, + observed=peer2.name, + session_name=session.name, + workspace_name=workspace.name, + message_ids=[], + message_created_at=str(datetime.now(timezone.utc)), + ) + + assert isinstance(result, ObservationsCreatedResult) + assert captured["deduplicate"] is deduplicate_setting + class TestNormalizeObservationId: """Unit tests for _normalize_observation_id.""" From 7bafee5de1b77a619f56c32ba59d9dcc0e115449 Mon Sep 17 00:00:00 2001 From: Aakash Kattelu Date: Wed, 19 Aug 2026 16:18:47 -0400 Subject: [PATCH 5/8] chore: add pr template and pre-pre skill (#1031) --- .github/pull_request_template.md | 13 +++++++ skills/pre-pr/SKILL.md | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 .github/pull_request_template.md create mode 100644 skills/pre-pr/SKILL.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 00000000..5463b6d0 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,13 @@ +## Description + + + +## Proofs + + + +## Checklist + +- [ ] This PR is correlated to an existing issue, and I understand it will be closed if that issue does not have the `maintainer-approved` label. + + diff --git a/skills/pre-pr/SKILL.md b/skills/pre-pr/SKILL.md new file mode 100644 index 00000000..0c1e1797 --- /dev/null +++ b/skills/pre-pr/SKILL.md @@ -0,0 +1,63 @@ +--- +name: pre-pr +description: Prepare a Honcho change for a pull request to plastic-labs/honcho. Invoke before opening a PR, when drafting a PR body, when asked if a branch is PR-ready, or when filling the pull request template. Checks the linked issue, required tests and docs, then writes Description / Proofs / Fixes. +--- + +# Pre-PR checklist + +Do this after the change works, before anyone opens the GitHub PR. Output is a filled template body — do not create the PR. + +The template lives at `.github/pull_request_template.md`. Do not add extra sections. + +## 1. Issue gate (hard stop) + +A PR without a maintainer-approved issue will be closed. + +```bash +gh issue view --repo plastic-labs/honcho --json number,title,labels,state +``` + +Stop if any of these fail: + +- no issue number, or the issue is not in `plastic-labs/honcho` +- issue is closed (unless this PR is explicitly reopening it) +- labels do not include `maintainer-approved` + +Say which check failed. Do not draft a PR body around it. + +## 2. Classify the diff + +```bash +git diff main...HEAD --stat +``` + +Pick one primary kind: bug, feature, docs. Then decide layers: + +| Surface touched | Required | +| --- | --- | +| `src/` (non-prompt) | unit tests under the matching `tests/` tree | +| deriver / dialectic / dreamer / LLM path | unit + consider live-llm (`tests/live_llm`) | +| queue, config hierarchy, multi-turn, SDK contract | unified (`uv run python -m tests.unified.run`) | +| `/v3` HTTP or deriver queue behavior | `/verify` skill (runtime, not just pytest) | +| public API, SDK exports, `config.toml` / settings, mintlify `docs/` | documentation in the matching file | + +Skip a layer only with a one-line reason (e.g. "docs-only", "comment-only"). "When appropriate" is not a skip. + +Invoke `/verify` when the runtime surface moved. Do not restate that skill here. + +Lint/type before claiming tests are green: `uv run ruff check src/` → `uv run basedpyright` → the pytest command for the layer. + +## 3. Proofs + +Collect evidence that belongs in the PR, not in the commit: + +- command + pass/fail for what you ran +- a log snippet, screenshot, or file path that shows the new behavior +- for bugs: the failing case before vs after, if you have it + +If `/verify` ran, the proofs *are* that session's output. Do not invent green runs. + +## 4. Write the body + +Fill the description, proofs, checklist portion of the pull request description template. +Make sure to link the related github issue, otherwise the PR will be auto-closed. From 47974892812cbe755a7970940f565e151767a20f Mon Sep 17 00:00:00 2001 From: Phil Date: Thu, 20 Aug 2026 10:24:49 -0400 Subject: [PATCH 6/8] telemetry: zero-initialize bounded-label metrics so an absent series means a broken scrape (#927) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * telemetry: materialize dropped-event counter children at 0 A labeled Prometheus counter exports no series until its first labels() call, so telemetry_events_dropped stayed invisible until an event was actually dropped — impossible to alert on or graph, and "no drops" was indistinguishable from "metric missing / scrape broken". Pre-create the (namespace, reason) children at 0 on emitter start, for each reason the emitter can emit, so the metric is always present. Co-Authored-By: Claude Opus 4.8 (1M context) * telemetry: generalize counter zero-init to all bounded-label counters Extends #927 (which zero-inited telemetry_events_dropped) to every counter whose label domain is bounded and known at startup, so metrics are present in Prometheus before their first event — a missing series then signals a broken scrape rather than "nothing happened yet". - add initialize_bounded_metrics(instance_type) on PrometheusMetrics; call it per-process from main.py (api) and deriver/__main__.py (deriver). - extract a shared _touch() helper; refactor initialize_telemetry_dropped_metrics onto it (that one stays per-emitter in start() — it's prefix-dependent). - explicit ALL_EVENT_TYPES / HIGH_VOLUME_EVENT_TYPES registry in telemetry.events, drift-guarded by tests that walk BaseEvent subclasses. - only VALID (task_type, token_type, component) tuples for deriver_tokens (the cartesian product would fabricate impossible always-0 series); only high-volume event types for sampled_out; high-cardinality labels (endpoint, workspace_name) left open. - gauges: zero-init embed_now_tasks_in_flight + telemetry_buffer_size; add a new message_embeddings_pending backlog gauge, set each reconciliation cycle and zero-inited at deriver startup (Rajat's pending/in-flight ask). - backfills the tests #927 shipped without. Co-Authored-By: Claude Opus 4.8 (1M context) * review: task-aware deriver combos + fail-soft gauge zero-init I1: _DERIVER_TOKEN_COMBOS was factored task-independently, materializing the impossible (ingestion, input, previous_summary) series — previous_summary is summary-only. Make combos task-aware (_DERIVER_TOKEN_COMBOS_BY_TASK) so no always-0 impossible series is fabricated, matching the PR's own goal. Tests tightened to assert the ingestion/previous_summary series is absent. I2: the three gauge .set(0) zero-inits were bare while the counter inits go through the fail-soft _touch. Add _set_gauge_zero() so a gauge init can't propagate an exception into process startup either. Co-Authored-By: Claude Opus 4.8 (1M context) * test(telemetry): isolate zero-init namespaces, add deriver-to-api guard Global-REGISTRY assertions used a fixed "test" namespace, which several other suites also pin, so another test's materialized children could satisfy a presence assertion or break an absence one. Each test now runs under a unique namespace resolved from settings at read time. Adds the inverse per-process isolation test: deriver-only init must not materialize API-only series (dialectic tokens, embed_now). Co-Authored-By: Claude * review: per-replica backlog gauge, drop duplicated constants and .meta refs Addresses Vineeth's review on #927. Blocking: - message_embeddings_pending is a DB-global count, so drive it from ReconcilerScheduler._scheduler_loop (runs on every replica, every interval) instead of run_vector_reconciliation_cycle (runs off the queue behind work-unit dedup, so one replica per cycle). Combined with the zero-init, the old placement made every replica that never won the work unit export a confident permanent 0. Help string now names the owner so dashboards don't reach for sum(). - guard initialize_telemetry_dropped_metrics on METRICS.ENABLED, matching its sibling initializer. - drop the duplicate REASONING_LEVELS; import the one in src/config. Non-blocking: - walk BaseSpecialist recursively via a shared utils.types.walk_subclasses (replaces the direct-children-only __subclasses__() and the test's private copy of the same helper). - derive the specialist assertion from the subclasses instead of hardcoding two names — the hardcoded pair kept passing after CardRefreshSpecialist landed, leaving it uncovered. - inline the zero-init rationale and the multi-instance bucket taxonomy; removes both pointers to a .meta design doc that is not in the repo. Tests: new tests/reconciler/test_pending_backlog_gauge.py pins both halves of the relocation (verified it fails when reverted). * review: fix inert test guard, stale comments, and the REASONING_LEVELS drift claim Second review pass on the branch. Findings, most severe first: - tests/reconciler/test_pending_backlog_gauge.py: the _try_enqueue_task stub was patched onto the class but declared without `self`, so calling it raised TypeError — which _scheduler_loop swallows. The guard was inert and the test passed for the wrong reason. Fixed the arity. - metrics.py still commented that the backlog gauge is "set live each reconciliation cycle". That is the exact claim the previous commit overturned; it now contradicted the help string, the bucket-3 docstring and sync_vectors.py. - metrics.py claimed REASONING_LEVELS is "derived from the config Literal so it never drifts", but config.py hand-listed it, so the earlier dedup had quietly traded away the guarantee the original get_args() call provided. Made it true instead: config.REASONING_LEVELS = list(get_args(...)), which keeps the dedup and restores the invariant. - dropped _set_gauge_zero: all three gauges it zeroed already have identical fail-soft setters, so it was a second way to do one thing. Using the setters also makes _handle_metric_error name the actual gauge. - record_pending_embeddings_backlog's docstring oversold the covering index as making the COUNT "negligible". The index makes cost proportional to the pending backlog, not to the table — which is worst precisely when the backlog matters. Stated honestly. - _scheduler_loop's docstring said it only enqueues; it also refreshes the gauge, at a cadence set by the shortest task interval. - comment reconciliation: stripped #927 / "the generalization" temporal anchoring, a CardRefreshSpecialist change-narration clause, and reviewer-directed phrasing from the test file; disambiguated the src/utils/summarizer.py path. - CLAUDE.md had no Prometheus section at all, so the new "add a BaseEvent subclass -> update ALL_EVENT_TYPES" obligation and the never-sum() rule for non-additive gauges were undiscoverable from the architecture doc. Verified: ruff + basedpyright clean (0 errors), tests/telemetry + reconciler + dialectic + llm 497 passed, full suite 1768 passed with only the 4 pre-existing test_document failures (OpenAI key required, reproduced on clean origin/main). Re-confirmed the relocation guard fails when reverted. * fix: silence the two basedpyright warnings inherited from main CI runs `uv run basedpyright` bare, and basedpyright exits non-zero on any warning — so these two have been failing the staticanalysis job on every branch cut from current main, not just this one: - src/vector_store/__init__.py:209 implicit string concatenation (#496) - tests/test_cache_redaction.py:5 private import (#869) Both predate this branch and are unrelated to the telemetry work; fixed here only because they block this PR from going green. Verified: clean origin/main also reports "0 errors, 2 warnings" and exits 1. basedpyright now 0 errors, 0 warnings, exit 0. * docs(telemetry): make the bucket-3 aggregation rule precise The multi-instance taxonomy said a service-scoped non-additive metric has "no aggregation correct once they disagree", then immediately mandated that every instance refresh on its own timer. Those undercut each other: staggered timers ALWAYS disagree slightly, so as written the rule reads as "ensure they don't", which is unachievable, and it leaves the reader unsure whether max() and avg() survived the fix. The actual rule is bounded disagreement plus a scale-preserving aggregator. Instances are N witnesses to one fact, not N parts of one whole, so sum() can never be correct (it scales with replica count) while max()/avg()/quantiles are correct precisely because the per-instance timer bounds the spread. Wording only; no behavior change. The gauge help string already said "max() or avg(), never sum()" — this makes the normative docstring agree with it. Surfaced walking Vineeth's comment 3668208059 for comprehension. * refactor(bench): import REASONING_LEVELS from config instead of re-listing Third copy of the constant, missed when ee781c0/694e07f deduped the other two. This one re-declared the ReasoningLevel Literal as well as the list, so the type alias could diverge from config's with nothing to catch it — and the list was hand-written, the variant that typechecks clean while missing a member. No import barrier justified it: this module already imports from src, as do seven of its siblings in tests/bench. Concrete effect of the drift was that a newly added sixth reasoning level would be rejected by the bench CLI's argparse choices=. src.config.REASONING_LEVELS is now the single definition repo-wide. * test(telemetry): pin the METRICS.ENABLED guard on the per-emitter initializer initialize_telemetry_dropped_metrics gained a METRICS.ENABLED guard in ee781c0, addressing Vineeth's asymmetry comment, but nothing asserted it — it had only the enabled half of the pair its sibling has. Deleting the guard left the suite green, so the fix closed the asymmetry in the guards and reproduced it one level up in the tests. Mirrors test_init_noop_when_metrics_disabled. Verified live rather than assumed: deleting the two guard lines turns this test red. Uses a unique namespace, without which the absence assertion would be satisfied by the enabled test's children rather than by the guard. * docs(telemetry): fold zero-init why-prose behind # region ai markers Comment/docstring-only pass over the changed files, per the groudon comment-marker standard: the terse human-facing "what" stays visible, and load-bearing "why" (the zero-init / absent-series-means-broken-scrape rationale, gotchas, receipts) folds into # region ai / # ai: blocks. Behavior-preserving: AST-identical modulo docstrings/comments vs the pre-pass merge; ruff, ruff format --check, and basedpyright all clean. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CLAUDE.md | 3 + src/config.py | 17 +- src/deriver/__main__.py | 7 + src/main.py | 6 + src/reconciler/scheduler.py | 16 +- src/reconciler/sync_vectors.py | 37 ++ src/telemetry/emitter.py | 15 + src/telemetry/events/__init__.py | 65 ++++ src/telemetry/prometheus/metrics.py | 193 +++++++++- src/utils/types.py | 16 +- tests/bench/runner_common.py | 7 +- .../reconciler/test_pending_backlog_gauge.py | 86 +++++ tests/telemetry/test_metric_zero_init.py | 357 ++++++++++++++++++ 13 files changed, 807 insertions(+), 18 deletions(-) create mode 100644 tests/reconciler/test_pending_backlog_gauge.py create mode 100644 tests/telemetry/test_metric_zero_init.py diff --git a/CLAUDE.md b/CLAUDE.md index c9862b59..6a83066d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,6 +187,9 @@ The Dreamer is an orchestrated multi-specialist system that runs during schedule - **LLM subsystem** (`src/llm/`): provider-agnostic `honcho_llm_call()`. Backends in `src/llm/backends/` (`anthropic.py`, `gemini.py`, `openai.py`). Includes prompt caching (`caching.py`), structured output (`structured_output.py`), tool loop (`tool_loop.py`), history adapters for cross-provider message formats, and a model registry. Per-retry provider selection is pinned via an `AttemptPlan` so stream-final retries don't bounce back to primary after the tool loop has settled on fallback. - **Per-agent model config**: each agent has its own `MODEL_CONFIG` in `src/config.py` with fallback chains (see `ConfiguredModelSettings`, `FallbackModelSettings`). - **Telemetry**: cloudevents in `src/telemetry/events/` cover API routes, dialectic, dream, deletion, reconciliation, representation, and per-call LLM accounting (`llm.py` — `LLMCallCompletedEvent` fires once per provider hit with full cost-attribution context). High-volume events are sampled deterministically per `run_id` via `TelemetrySettings.HIGH_VOLUME_SAMPLE_RATE`. +- **Prometheus metrics** (`src/telemetry/prometheus/`): every metric carries a `namespace` label and every recorder is fail-soft (a metrics error never propagates into a request or a worker loop). Counter children with a *bounded* label domain are zero-initialized per process at startup — `initialize_bounded_metrics(instance_type=...)`, called from the `src/main.py` lifespan (`api`) and `src/deriver/__main__.py` (`deriver`) — so an absent series means a broken scrape rather than "nothing happened". Two consequences worth knowing before touching telemetry: + - **Adding a `BaseEvent` subclass requires adding its `_event_type` to `ALL_EVENT_TYPES`** in `src/telemetry/events/__init__.py` (and to `HIGH_VOLUME_EVENT_TYPES` if `_volume_class == "high_volume"`). Enforced by the drift guards in `tests/telemetry/test_metric_zero_init.py`, which assert set-equality against the discovered subclasses. + - **A service-wide, non-additive gauge must be refreshed by every replica on its own timer**, and aggregated with `max()`/`avg()`, never `sum()`. `message_embeddings_pending` is the example: it reports a DB-global count, so it is driven from `ReconcilerScheduler._scheduler_loop` (runs on all replicas) rather than from the work-unit-deduped reconciliation cycle — otherwise, combined with the zero-init, every replica that never won the work unit would export a confident permanent `0`. ### Project Structure diff --git a/src/config.py b/src/config.py index 092cb6ee..993f9bfb 100644 --- a/src/config.py +++ b/src/config.py @@ -2,7 +2,7 @@ import logging import math import os from pathlib import Path -from typing import Annotated, Any, ClassVar, Literal, cast +from typing import Annotated, Any, ClassVar, Literal, cast, get_args from urllib.parse import urlparse import tomllib @@ -998,15 +998,14 @@ class PeerCardSettings(HonchoSettings): ENABLED: bool = True -# Reasoning levels for dialectic - defined here to avoid circular imports with schemas +# Reasoning levels for dialectic - defined here to avoid circular imports with schemas. +# region ai +# REASONING_LEVELS is derived from the Literal, not hand-listed: the annotation +# rejects an invalid member but not a MISSING one, so a hand-written copy could +# silently drop a level and still typecheck. +# endregion ReasoningLevel = Literal["minimal", "low", "medium", "high", "max"] -REASONING_LEVELS: list[ReasoningLevel] = [ - "minimal", - "low", - "medium", - "high", - "max", -] +REASONING_LEVELS: list[ReasoningLevel] = list(get_args(ReasoningLevel)) class DialecticLevelSettings(BaseModel): diff --git a/src/deriver/__main__.py b/src/deriver/__main__.py index c56ed6a0..ce3969b5 100644 --- a/src/deriver/__main__.py +++ b/src/deriver/__main__.py @@ -10,6 +10,7 @@ from src.db import engine, register_db_query_instrumentation from src.startup import validate_embedding_schema from src.telemetry import ( initialize_telemetry_async, + prometheus_metrics, register_db_pool_collector, shutdown_telemetry, ) @@ -25,6 +26,12 @@ def start_metrics_server() -> None: # Expose DB connection-pool stats for this deriver instance. register_db_pool_collector("deriver") register_db_query_instrumentation("deriver") + + # region ai + # Zero-init bounded-label counters so a missing series signals a broken scrape, + # not "no events" — see initialize_bounded_metrics. No-op if metrics off. + # endregion + prometheus_metrics.initialize_bounded_metrics(instance_type="deriver") logger.info("Prometheus metrics server started on port 9090") diff --git a/src/main.py b/src/main.py index 75eac455..6930d4c1 100644 --- a/src/main.py +++ b/src/main.py @@ -110,6 +110,12 @@ async def lifespan(_: FastAPI): register_db_pool_collector("api") register_db_query_instrumentation("api") + # region ai + # Zero-init bounded-label counters so a missing series signals a broken scrape, + # not "no events" — see initialize_bounded_metrics. No-op if metrics off. + # endregion + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + # Validate embedding schema before serving any traffic. Fails closed: if # the configured EMBEDDING_VECTOR_DIMENSIONS does not match the physical # pgvector columns, the process refuses to start rather than silently diff --git a/src/reconciler/scheduler.py b/src/reconciler/scheduler.py index e171c4fa..08941970 100644 --- a/src/reconciler/scheduler.py +++ b/src/reconciler/scheduler.py @@ -21,6 +21,7 @@ from src import models from src.config import settings from src.dependencies import tracked_db from src.models import QueueItem +from src.reconciler.sync_vectors import record_pending_embeddings_backlog logger = logging.getLogger(__name__) @@ -145,15 +146,26 @@ class ReconcilerScheduler: async def _scheduler_loop(self) -> None: """ - Main scheduler loop that enqueues tasks based on their intervals. + Main scheduler loop that enqueues tasks based on their intervals, and + refreshes the service-wide pending-embeddings backlog gauge each pass. Each task has its own interval and the loop checks all tasks on each - iteration, enqueueing any that are due. + iteration, enqueueing any that are due. The loop sleeps until the next + task is due, so the gauge's refresh cadence tracks the SHORTEST task + interval. """ try: while not self._shutdown_event.is_set(): now = datetime.now(timezone.utc) + # region ai + # Refresh on EVERY replica, not just whichever wins the sync_vectors + # work unit: the count is DB-global, so a replica that never ran a + # cycle would otherwise export a stale (or zero-initialized) value + # forever. Full rationale in record_pending_embeddings_backlog. + # endregion + await record_pending_embeddings_backlog() + # Check each task and enqueue if due for task_name, task in RECONCILER_TASKS.items(): next_run = self._next_run.get(task_name, now) diff --git a/src/reconciler/sync_vectors.py b/src/reconciler/sync_vectors.py index 68e9ac59..1ff94542 100644 --- a/src/reconciler/sync_vectors.py +++ b/src/reconciler/sync_vectors.py @@ -23,6 +23,7 @@ from src.config import settings from src.dependencies import tracked_db from src.embedding_client import embedding_client from src.exceptions import VectorStoreError +from src.telemetry import prometheus_metrics from src.telemetry.events import EmbeddingCallPurpose from src.utils.types import embedding_call_purpose from src.vector_store import VectorRecord, VectorStore, get_external_vector_store @@ -700,6 +701,42 @@ async def _cleanup_pgvector_batch( return True +async def record_pending_embeddings_backlog() -> None: + """Set the pending-embeddings backlog gauge to the current count of + MessageEmbedding rows awaiting a vector (sync_state='pending').""" + # region ai + # Called from ``ReconcilerScheduler._scheduler_loop``, deliberately NOT from + # ``run_vector_reconciliation_cycle``: the cycle runs off the queue behind + # work-unit dedup, so exactly one deriver replica executes it. Driving the gauge + # from there would leave every other replica exporting a stale value — or, since + # this metric is zero-initialized, a confident permanent 0 it never measured. The + # count is a property of the database, not the process, so every replica must + # refresh it on its own timer for ``max()``/``avg()`` to mean anything. + # + # Cost: one COUNT per replica per scheduler interval (~5 min by default). + # ``ix_message_embeddings_sync_state_last_sync_at`` keeps the scan proportional to + # the pending backlog, not the whole table — which is not the same as cheap: after + # an embedding outage the backlog is exactly what is large. Still a small duty + # cycle, and the cost shrinks as the reconciler drains. + # + # Best-effort: a metrics/DB hiccup here must never break the scheduler loop. + # endregion + if not settings.METRICS.ENABLED: + return + try: + async with tracked_db("reconciler_pending_count", read_only=True) as db: + count = await db.scalar( + select(func.count()) + .select_from(models.MessageEmbedding) + .where(models.MessageEmbedding.sync_state == "pending") + ) + prometheus_metrics.set_message_embeddings_pending(count=count or 0) + except Exception: + logger.warning( + "Failed to record pending-embeddings backlog gauge", exc_info=True + ) + + async def run_vector_reconciliation_cycle() -> ReconciliationMetrics: """ Run a complete reconciliation cycle. diff --git a/src/telemetry/emitter.py b/src/telemetry/emitter.py index 201d16aa..397ecd90 100644 --- a/src/telemetry/emitter.py +++ b/src/telemetry/emitter.py @@ -172,6 +172,21 @@ class TelemetryEmitter: ) self._running = True self._flush_task = asyncio.create_task(self._periodic_flush()) + + # region ai + # Pre-create the dropped-event counter children at 0: a labeled counter + # exports nothing until its first observation, so this makes the metric + # visible before any drop and lets us tell "no drops" from "metric missing". + # endregion + from src.telemetry.prometheus.metrics import prometheus_metrics + + prometheus_metrics.initialize_telemetry_dropped_metrics( + reasons=[ + f"{self.drop_reason_prefix}buffer_full", + f"{self.drop_reason_prefix}send_failed", + ] + ) + logger.info("Telemetry emitter started, endpoint: %s", self.endpoint) async def shutdown(self) -> None: diff --git a/src/telemetry/events/__init__.py b/src/telemetry/events/__init__.py index 000e70b4..f8950d0f 100644 --- a/src/telemetry/events/__init__.py +++ b/src/telemetry/events/__init__.py @@ -138,9 +138,74 @@ __all__ = [ # Lifecycle "initialize_telemetry_events", "shutdown_telemetry_events", + # Zero-init registry + "ALL_EVENT_TYPES", + "HIGH_VOLUME_EVENT_TYPES", ] +# Explicit registry of CloudEvents `type` values, used to zero-initialize the +# telemetry_events_emitted / telemetry_events_sampled_out counter children. +# region ai +# See metrics.py:initialize_bounded_metrics for why absent and zero are worth +# distinguishing. Explicit literal, not a set derived from BaseEvent subclasses: a +# derived set would follow whatever happens to be imported at init time, so a type +# could drop out of the registry with no code change. A hand-maintained list plus a +# drift-guard test fails loud at the right moment instead. +# +# When you add a BaseEvent subclass, add its `_event_type` here (and to +# HIGH_VOLUME_EVENT_TYPES if `_volume_class == "high_volume"`). The drift-guard test +# tests/telemetry/test_metric_zero_init.py fails until you do — it asserts this +# registry equals the set discovered by walking BaseEvent subclasses. +# endregion +ALL_EVENT_TYPES: tuple[str, ...] = ( + # api + "message.created", + "file.uploaded", + "context.retrieved", + # agent + "agent.iteration", + "agent.tool.conclusions.created", + "agent.tool.conclusions.deleted", + "agent.tool.peer_card.updated", + "agent.tool.summary.created", + "agent.tool.call.completed", + # deletion / dialectic / dream / representation + "deletion.completed", + "dialectic.completed", + "dream.run", + "dream.specialist", + "representation.completed", + # llm / embedding + "llm.call.completed", + "embedding.call.completed", + # reconciliation + "reconciliation.sync_vectors.completed", + "reconciliation.cleanup_stale_items.completed", + # trace stream + # region ai + # Only emitted when TELEMETRY.TRACE_PAYLOADS_ENABLED, but they flow through the + # same emit() path and increment the same counters, so they belong in the set. + # endregion + "llm.call.traced", + "embedding.call.traced", + "trace.content", +) + +# Subset of ALL_EVENT_TYPES whose `_volume_class == "high_volume"`. +# region ai +# Only these can ever be counted by ``telemetry_events_sampled_out``; ground_truth +# events skip the sampler entirely (pre-creating their sampled_out series would be a +# permanently-misleading 0). +# endregion +HIGH_VOLUME_EVENT_TYPES: tuple[str, ...] = ( + "agent.iteration", + "agent.tool.call.completed", + "llm.call.completed", + "embedding.call.completed", +) + + def emit(event: BaseEvent) -> None: """Queue an event for emission to the telemetry backend. diff --git a/src/telemetry/prometheus/metrics.py b/src/telemetry/prometheus/metrics.py index 01d7be4f..749591d9 100644 --- a/src/telemetry/prometheus/metrics.py +++ b/src/telemetry/prometheus/metrics.py @@ -20,7 +20,8 @@ from prometheus_client.core import GaugeMetricFamily from starlette.requests import Request from starlette.responses import Response -from src.config import settings +from src.config import REASONING_LEVELS, settings +from src.utils.types import walk_subclasses disable_created_metrics() @@ -66,6 +67,32 @@ class DialecticComponents(Enum): TOTAL = "total" +# Valid (token_type, component) pairs for deriver_tokens_processed, per task_type, +# used to zero-initialize counter children (see initialize_bounded_metrics). +# region ai +# NOT the cartesian product: input tokens only pair with input components, output +# only with OUTPUT_TOTAL, and PREVIOUS_SUMMARY occurs only for summary tasks +# (ingestion has no previous summary). Enumerating anything broader would fabricate +# impossible always-0 series (e.g. output/prompt, or ingestion/previous_summary). +# Explicit literal, drift-guarded by tests/telemetry/test_metric_zero_init.py. +# Sources: track_deriver_input_tokens (src/utils/tokens.py) + the OUTPUT_TOTAL sites +# in src/deriver/deriver.py and src/utils/summarizer.py. +# endregion +_DERIVER_TOKEN_COMBOS_BY_TASK: dict[str, tuple[tuple[str, str], ...]] = { + DeriverTaskTypes.INGESTION.value: ( + (TokenTypes.INPUT.value, DeriverComponents.PROMPT.value), + (TokenTypes.INPUT.value, DeriverComponents.MESSAGES.value), + (TokenTypes.OUTPUT.value, DeriverComponents.OUTPUT_TOTAL.value), + ), + DeriverTaskTypes.SUMMARY.value: ( + (TokenTypes.INPUT.value, DeriverComponents.PROMPT.value), + (TokenTypes.INPUT.value, DeriverComponents.MESSAGES.value), + (TokenTypes.INPUT.value, DeriverComponents.PREVIOUS_SUMMARY.value), + (TokenTypes.OUTPUT.value, DeriverComponents.OUTPUT_TOTAL.value), + ), +} + + api_requests_counter = NamespacedCounter( "api_requests", "Total API requests", @@ -155,6 +182,23 @@ telemetry_buffer_size_gauge = NamespacedGauge( ["namespace"], ) +# Embedding backlog: MessageEmbedding rows still awaiting a vector +# (sync_state='pending'). +# region ai +# Distinct from embed_now_tasks_in_flight (in-flight fast-path work in the API +# process) — this is the durable, DB-wide backlog the reconciler drains. Every +# deriver replica refreshes it on its own timer from +# ReconcilerScheduler._scheduler_loop, so replicas disagree by at most one interval. +# Service-wide, not per-process — hence the help string's "never sum()". +# endregion +message_embeddings_pending_gauge = NamespacedGauge( + "message_embeddings_pending", + "MessageEmbedding rows awaiting embedding (sync_state='pending'). " + + "Service-wide DB count, reported independently by every replica — " + + "aggregate with max() or avg(), never sum()", + ["namespace"], +) + # DB connection-pool health. The in-flight gauge counts statements actually # executing on the wire, so checked_out minus in_flight reveals connections held # but parked (the "idle in transaction during an external call" antipattern). @@ -325,12 +369,159 @@ class PrometheusMetrics: except Exception as e: self._handle_metric_error("record_telemetry_event_dropped", e) + def _touch(self, counter: NamespacedCounter, **labels: str) -> None: + """Pre-create a counter child series at 0 without incrementing it.""" + # region ai + # A labeled Prometheus counter exports no time series until its first + # ``labels(...)`` call, so pre-touching a child keeps it present at 0 — a + # missing series then signals a broken scrape rather than "no events". + # Fail-soft (like the recorders): a bad init must never crash startup. + # endregion + try: + counter.labels(**labels) + except Exception as e: + self._handle_metric_error("_touch", e) + + def initialize_telemetry_dropped_metrics(self, *, reasons: list[str]) -> None: + """Pre-create telemetry_events_dropped ``(namespace, reason)`` children at 0. + + Args: + reasons: The reason label values the calling emitter can produce. + """ + # region ai + # The metric stays invisible in Prometheus/Grafana until an event is actually + # dropped, so materializing the children at startup keeps it present at 0 — a + # missing series then means a broken scrape, not "no drops" (see _touch). + # + # Called per-emitter from ``TelemetryEmitter.start()`` rather than hoisted into + # the process-level ``initialize_bounded_metrics``: the trace emitter (whose + # reasons carry a ``trace_`` prefix) only exists when ``TRACE_PAYLOADS_ENABLED`` + # is set, so hoisting would fabricate ``trace_*`` series on deployments that run + # with tracing off. + # endregion + if not settings.METRICS.ENABLED: + return + + for reason in reasons: + self._touch(telemetry_events_dropped_counter, reason=reason) + + def initialize_bounded_metrics(self, *, instance_type: str) -> None: + """Pre-create bounded-label counter children at 0 for this process, so an + absent series means a broken scrape rather than "nothing happened". + + Args: + instance_type: "api" or "deriver" — selects the process-specific + counters. Event-type and buffer metrics are initialized in both. + """ + # region ai + # A Prometheus counter does not exist until its first increment, so a + # never-yet-incremented metric is indistinguishable from a broken scrape: + # you cannot graph or alert on a series that is absent. Materializing the + # children at 0 inverts that — a missing series now means something is wrong, + # and "no events" reads as a flat 0 instead of a gap. + # + # That only holds for label sets we can enumerate honestly, so a metric is + # initialized here only when its full label domain is bounded, enumerable at + # startup, and actually emitted by THIS process. High-cardinality labels + # (endpoint, workspace_name) and impossible label tuples are deliberately left + # absent — fabricating a permanently-0 series that no code path can ever + # increment is the same lie in the other direction. + # + # Multi-instance safety splits the metrics here into three buckets: + # + # 1. instance-scoped (``telemetry_buffer_size``, ``embed_now_tasks_in_flight``) + # — per-process by nature, so any aggregation is meaningful and zero-init is + # unambiguously right. + # 2. service-scoped additive (the token counters, ``telemetry_events_emitted``) + # — each instance holds a partial count and ``sum()`` reconstructs the whole, + # so multi-instance safe. + # 3. service-scoped non-additive — every instance reports the whole service's + # value, so the instances are N witnesses to one fact rather than N parts of + # one whole. ``sum()`` is therefore never correct here: it scales with the + # replica count. Scale-preserving aggregations (``max()``, ``avg()``, + # quantiles) ARE correct, but only while the witnesses disagree by a bounded + # amount — which requires every instance to refresh on its own timer (see + # ``message_embeddings_pending``, refreshed per replica from + # ``ReconcilerScheduler._scheduler_loop``). A bucket-3 metric that cannot + # meet that bar does not belong in the app at all — it belongs in an exporter + # that yields exactly one series. + # + # Prometheus stamps ``instance``/``job`` at scrape time, which is why buckets 1 + # and 2 need no special handling. ``telemetry_events_dropped`` is handled + # separately, per-emitter, in ``TelemetryEmitter.start()`` (prefix-dependent). + # endregion + if not settings.METRICS.ENABLED: + return + + # ai: lazy import avoids an import-time cycle (metrics is imported widely) + from src.telemetry.events import ALL_EVENT_TYPES, HIGH_VOLUME_EVENT_TYPES + + # region ai + # Common: both processes run a TelemetryEmitter, so both emit their own subset + # of event types. The domain is bounded/low-cardinality (~21 types), so init + # the full set in each process rather than maintain a fragile + # per-event-type -> process map. + # endregion + for event_type in ALL_EVENT_TYPES: + self._touch(telemetry_events_emitted_counter, type=event_type) + for event_type in HIGH_VOLUME_EVENT_TYPES: + self._touch(telemetry_events_sampled_out_counter, type=event_type) + self.set_telemetry_buffer_size(size=0) + + if instance_type == "api": + # dialectic tokens: token_type x component(total) x reasoning_level + for token_type in TokenTypes: + for level in REASONING_LEVELS: + self._touch( + dialectic_tokens_processed_counter, + token_type=token_type.value, + component=DialecticComponents.TOTAL.value, + reasoning_level=level, + ) + # ai: embed_now fast path runs as an API-process background task + self._touch(embed_now_tasks_shed_counter) + self.set_embed_now_tasks_in_flight(0) + + elif instance_type == "deriver": + # deriver tokens: only the valid (token_type, component) tuples per + # task_type (see _DERIVER_TOKEN_COMBOS_BY_TASK). + for task_type_value, combos in _DERIVER_TOKEN_COMBOS_BY_TASK.items(): + for token_type_value, component_value in combos: + self._touch( + deriver_tokens_processed_counter, + task_type=task_type_value, + token_type=token_type_value, + component=component_value, + ) + # dreamer tokens: specialist_name x token_type. + # region ai + # Names come from the concrete BaseSpecialist subclasses (walked recursively + # via walk_subclasses) so a new specialist can't silently miss init. + # endregion + from src.dreamer.specialists import BaseSpecialist + + for specialist in walk_subclasses(BaseSpecialist): + for token_type in TokenTypes: + self._touch( + dreamer_tokens_processed_counter, + specialist_name=specialist.name, + token_type=token_type.value, + ) + # ai: init at 0 so the gauge is visible before its first per-replica refresh + self.set_message_embeddings_pending(count=0) + def set_telemetry_buffer_size(self, *, size: int) -> None: try: telemetry_buffer_size_gauge.labels().set(size) except Exception as e: self._handle_metric_error("set_telemetry_buffer_size", e) + def set_message_embeddings_pending(self, *, count: int) -> None: + try: + message_embeddings_pending_gauge.labels().set(count) + except Exception as e: + self._handle_metric_error("set_message_embeddings_pending", e) + prometheus_metrics = PrometheusMetrics() diff --git a/src/utils/types.py b/src/utils/types.py index 24a13ae6..0bfada75 100644 --- a/src/utils/types.py +++ b/src/utils/types.py @@ -1,4 +1,4 @@ -from collections.abc import Awaitable, Callable, Generator +from collections.abc import Awaitable, Callable, Generator, Iterator from contextlib import contextmanager from contextvars import ContextVar from dataclasses import dataclass, field @@ -6,6 +6,20 @@ from typing import Any, Generic, Literal, TypeVar T = TypeVar("T") + +def walk_subclasses(cls: type[T]) -> Iterator[type[T]]: + """Yield every subclass of ``cls``, recursively.""" + # region ai + # ``type.__subclasses__()`` is direct-children-only, so a grandchild class is + # silently invisible to it. Any registry that enumerates subclasses to decide + # what to initialize or validate wants the transitive closure — otherwise + # subclassing a concrete class is enough to slip past the check. + # endregion + for subclass in cls.__subclasses__(): + yield subclass + yield from walk_subclasses(subclass) + + # Context variable for tracking current iteration in tool execution loop # This is used for telemetry to associate tool calls with their iteration _current_iteration: ContextVar[int] = ContextVar("current_iteration", default=0) diff --git a/tests/bench/runner_common.py b/tests/bench/runner_common.py index be262cb0..b150a3f8 100644 --- a/tests/bench/runner_common.py +++ b/tests/bench/runner_common.py @@ -17,21 +17,18 @@ from dataclasses import dataclass, field from datetime import datetime from logging import Logger from pathlib import Path -from typing import Any, Generic, Literal, TypeVar, cast +from typing import Any, Generic, TypeVar, cast from anthropic import AsyncAnthropic from honcho import Honcho from honcho.api_types import SessionConfiguration, SummaryConfiguration from openai import AsyncOpenAI +from src.config import REASONING_LEVELS, ReasoningLevel from src.telemetry.metrics_collector import MetricsCollector _logger = logging.getLogger(__name__) -# Valid reasoning levels for dialectic chat -ReasoningLevel = Literal["minimal", "low", "medium", "high", "max"] -REASONING_LEVELS: list[str] = ["minimal", "low", "medium", "high", "max"] - # Type variable for result types ResultT = TypeVar("ResultT") diff --git a/tests/reconciler/test_pending_backlog_gauge.py b/tests/reconciler/test_pending_backlog_gauge.py new file mode 100644 index 00000000..226a71e8 --- /dev/null +++ b/tests/reconciler/test_pending_backlog_gauge.py @@ -0,0 +1,86 @@ +"""The pending-embeddings backlog gauge must be refreshed per-replica. + +These tests pin both halves: the scheduler loop drives the refresh, and the +queue-driven reconciliation cycle does not. +""" +# region ai +# ``message_embeddings_pending`` reports a DB-global count, so it is the one gauge +# here whose value is service-wide rather than per-process. It is also zero- +# initialized at startup, which makes a missing refresh actively harmful: a replica +# that never measured the backlog would export a confident, permanently-healthy 0. +# So the count is driven from ``ReconcilerScheduler._scheduler_loop`` (runs on every +# replica, every interval), NOT from ``run_vector_reconciliation_cycle`` (runs off +# the queue behind work-unit dedup, so exactly one replica per cycle executes it). +# endregion + +import asyncio + +import pytest + +from src.reconciler import scheduler as scheduler_module +from src.reconciler import sync_vectors +from src.reconciler.scheduler import ReconcilerScheduler + + +@pytest.fixture(autouse=True) +def _reset_scheduler_singleton(): # pyright: ignore[reportUnusedFunction] + ReconcilerScheduler.reset_singleton() + yield + ReconcilerScheduler.reset_singleton() + + +async def test_scheduler_loop_refreshes_backlog_gauge( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Every scheduler iteration refreshes the gauge, on every replica. + + Patched at the scheduler's own reference so this asserts the call site, not + just that the function exists. + """ + calls = 0 + refreshed = asyncio.Event() + + async def _fake_refresh() -> None: + nonlocal calls + calls += 1 + refreshed.set() + + # region ai + # Patched onto the class, so it is invoked as a bound method — it needs the + # ``self`` parameter or the call raises TypeError, which ``_scheduler_loop`` would + # then swallow, leaving this guard silently inert. + # endregion + async def _never_enqueue(_self: object, _task: object) -> bool: + return False + + monkeypatch.setattr( + scheduler_module, "record_pending_embeddings_backlog", _fake_refresh + ) + monkeypatch.setattr( + ReconcilerScheduler, "_try_enqueue_task", _never_enqueue, raising=True + ) + + scheduler = ReconcilerScheduler() + await scheduler.start() + try: + await asyncio.wait_for(refreshed.wait(), timeout=5.0) + finally: + await scheduler.shutdown() + + assert calls >= 1, "scheduler loop never refreshed the backlog gauge" + + +def test_reconciliation_cycle_does_not_drive_the_gauge() -> None: + """The queue-driven cycle must not be the thing that sets the gauge.""" + # region ai + # If the refresh moves back into ``run_vector_reconciliation_cycle``, only the + # replica that wins the ``sync_vectors`` work unit would ever measure the backlog, + # and the zero-init would go back to lying on all the others. + # + # Structural guard: the cycle is a long DB-driven coroutine, so this inspects the + # global names it references rather than executing it. + # endregion + assert hasattr(sync_vectors, "record_pending_embeddings_backlog") + + referenced = sync_vectors.run_vector_reconciliation_cycle.__code__.co_names + assert "record_pending_embeddings_backlog" not in referenced diff --git a/tests/telemetry/test_metric_zero_init.py b/tests/telemetry/test_metric_zero_init.py new file mode 100644 index 00000000..e69f50df --- /dev/null +++ b/tests/telemetry/test_metric_zero_init.py @@ -0,0 +1,357 @@ +"""Tests for startup zero-initialization of bounded-label metrics. + +Asserts that: +- bounded-label counter children are materialized at 0 before any event, +- high-cardinality / impossible label combinations are deliberately NOT, +- per-process init doesn't materialize the other process's counters, +- the explicit registries stay in sync with the source of truth (drift guards). +""" +# region ai +# Reads use ``REGISTRY.get_sample_value`` (returns the value if a series exists, +# ``None`` if it does not) rather than ``counter.labels(...)``, because ``.labels`` +# would itself materialize the child and destroy the presence/absence signal. +# endregion + +from collections.abc import Iterator +from typing import cast +from uuid import uuid4 + +import pytest +from prometheus_client import REGISTRY + +from src.config import REASONING_LEVELS, settings +from src.dreamer.specialists import BaseSpecialist +from src.telemetry.events import ALL_EVENT_TYPES, HIGH_VOLUME_EVENT_TYPES +from src.telemetry.events.base import BaseEvent +from src.telemetry.prometheus.metrics import ( + _DERIVER_TOKEN_COMBOS_BY_TASK, # pyright: ignore[reportPrivateUsage] + DeriverComponents, + DeriverTaskTypes, + DialecticComponents, + TokenTypes, + prometheus_metrics, +) +from src.utils.types import walk_subclasses + + +def unique_ns(tag: str) -> str: + """A ``namespace`` label value no other test can have materialized under.""" + # region ai + # Every assertion here reads the process-global ``REGISTRY``, which keeps a child + # series for the rest of the session once anything materializes it. A shared + # namespace (several other suites pin ``"test"``) would let another test's children + # satisfy a presence assertion, or break an absence assertion, independently of + # what the initializer under test actually did. + # endregion + return f"test_metric_zero_init_{tag}_{uuid4().hex[:8]}" + + +@pytest.fixture +def metrics_enabled(monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: + """Enable metrics under a namespace unique to the requesting test.""" + ns = unique_ns("enabled") + monkeypatch.setattr("src.config.settings.METRICS.ENABLED", True) + monkeypatch.setattr("src.config.settings.METRICS.NAMESPACE", ns) + yield ns + + +def sample(name: str, **labels: str) -> float | None: + """Value of a series if it exists, else None. Never materializes it. + + Resolves the namespace from settings, so it always reads the unique one the + active test pinned. + """ + ns = cast(str, settings.METRICS.NAMESPACE) + return REGISTRY.get_sample_value(name, {"namespace": ns, **labels}) + + +# --------------------------------------------------------------------------- +# Drift guards (pure logic — no registry). Adding an event type / token component +# without updating the registry fails here, with a pointer to what to fix. +# --------------------------------------------------------------------------- + + +def test_all_event_types_registry_matches_subclasses(): + """ALL_EVENT_TYPES must equal every BaseEvent subclass's _event_type. + + If this fails you added/removed a BaseEvent subclass without updating + ALL_EVENT_TYPES in src/telemetry/events/__init__.py — its Prometheus counter + would not be zero-initialized. Update the registry. + """ + discovered = { + event_type + for cls in walk_subclasses(BaseEvent) + if (event_type := getattr(cls, "_event_type", None)) is not None + } + assert set(ALL_EVENT_TYPES) == discovered + assert len(ALL_EVENT_TYPES) == len(set(ALL_EVENT_TYPES)), "duplicate event types" + + +def test_high_volume_registry_matches_subclasses(): + """HIGH_VOLUME_EVENT_TYPES must equal the high_volume-classed subclasses.""" + discovered = { + event_type + for cls in walk_subclasses(BaseEvent) + if (event_type := getattr(cls, "_event_type", None)) is not None + and getattr(cls, "_volume_class", None) == "high_volume" + } + assert set(HIGH_VOLUME_EVENT_TYPES) == discovered + assert set(HIGH_VOLUME_EVENT_TYPES) <= set(ALL_EVENT_TYPES) + + +def test_deriver_token_combos_are_valid_and_complete(): + """Every combo uses real enum values; the union across tasks covers every + DeriverComponent; and no task enumerates an impossible pair. + + Fails if a DeriverComponent/DeriverTaskType is added without deciding which + task_type + token_type it pairs with in _DERIVER_TOKEN_COMBOS_BY_TASK. + """ + valid_token_types = {t.value for t in TokenTypes} + valid_components = {c.value for c in DeriverComponents} + valid_task_types = {t.value for t in DeriverTaskTypes} + + assert set(_DERIVER_TOKEN_COMBOS_BY_TASK) == valid_task_types + all_components: set[str] = set() + for task_type, combos in _DERIVER_TOKEN_COMBOS_BY_TASK.items(): + assert task_type in valid_task_types + for token_type, component in combos: + assert token_type in valid_token_types + assert component in valid_components + # each task enumerates fewer than its cartesian product (no impossible pairs) + assert len(combos) < len(valid_token_types) * len(valid_components) + all_components.update(comp for _, comp in combos) + + # every component is reachable via some task + assert all_components == valid_components + # previous_summary is summary-only: ingestion must NOT enumerate it + ingestion = _DERIVER_TOKEN_COMBOS_BY_TASK[DeriverTaskTypes.INGESTION.value] + assert ( + TokenTypes.INPUT.value, + DeriverComponents.PREVIOUS_SUMMARY.value, + ) not in ingestion + + +# --------------------------------------------------------------------------- +# API-process zero-init +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("metrics_enabled") +def test_api_init_materializes_event_type_children(): + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + for event_type in ALL_EVENT_TYPES: + assert sample("telemetry_events_emitted_total", type=event_type) is not None + for event_type in HIGH_VOLUME_EVENT_TYPES: + assert sample("telemetry_events_sampled_out_total", type=event_type) is not None + + +@pytest.mark.usefixtures("metrics_enabled") +def test_api_init_materializes_dialectic_and_embed(): + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + for token_type in TokenTypes: + for level in REASONING_LEVELS: + assert ( + sample( + "dialectic_tokens_processed_total", + token_type=token_type.value, + component=DialecticComponents.TOTAL.value, + reasoning_level=level, + ) + is not None + ) + assert sample("embed_now_tasks_shed_total") is not None + assert sample("embed_now_tasks_in_flight") == 0.0 # gauge, explicit .set(0) + + +@pytest.mark.usefixtures("metrics_enabled") +def test_sampled_out_excludes_ground_truth_event_types(): + """Ground-truth events can never be sampled out, so their sampled_out series + must NOT be pre-created (they'd be permanently misleading zeros).""" + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + ground_truth = set(ALL_EVENT_TYPES) - set(HIGH_VOLUME_EVENT_TYPES) + for event_type in ground_truth: + assert sample("telemetry_events_sampled_out_total", type=event_type) is None + + +# --------------------------------------------------------------------------- +# Deriver-process zero-init +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("metrics_enabled") +def test_deriver_init_materializes_token_and_backlog(): + prometheus_metrics.initialize_bounded_metrics(instance_type="deriver") + for task_type, combos in _DERIVER_TOKEN_COMBOS_BY_TASK.items(): + for token_type, component in combos: + assert ( + sample( + "deriver_tokens_processed_total", + task_type=task_type, + token_type=token_type, + component=component, + ) + is not None + ) + # region ai + # Specialist names are derived from the concrete BaseSpecialist subclasses here + # too, rather than hardcoded: a hardcoded list would keep passing when a new + # specialist is added (it only asserts presence), silently leaving it uncovered. + # endregion + specialist_names = { + name + for cls in walk_subclasses(BaseSpecialist) + if (name := getattr(cls, "name", None)) is not None + } + assert {"deduction", "induction", "card_refresh"} <= specialist_names + for specialist_name in specialist_names: + assert ( + sample( + "dreamer_tokens_processed_total", + specialist_name=specialist_name, + token_type=TokenTypes.INPUT.value, + ) + is not None + ), f"specialist {specialist_name!r} was not zero-initialized" + assert sample("message_embeddings_pending") == 0.0 # gauge zero-init + + +@pytest.mark.usefixtures("metrics_enabled") +def test_deriver_init_omits_impossible_token_combos(): + """The cartesian product includes combos that never occur (e.g. output tokens + with an input component). Those must not be materialized.""" + prometheus_metrics.initialize_bounded_metrics(instance_type="deriver") + # output tokens never pair with an input component + assert ( + sample( + "deriver_tokens_processed_total", + task_type=DeriverTaskTypes.INGESTION.value, + token_type=TokenTypes.OUTPUT.value, + component=DeriverComponents.PROMPT.value, + ) + is None + ) + # previous_summary is summary-only — ingestion must not materialize it + assert ( + sample( + "deriver_tokens_processed_total", + task_type=DeriverTaskTypes.INGESTION.value, + token_type=TokenTypes.INPUT.value, + component=DeriverComponents.PREVIOUS_SUMMARY.value, + ) + is None + ) + # base specialist is abstract and never emits — must not be materialized + assert ( + sample( + "dreamer_tokens_processed_total", + specialist_name="base", + token_type=TokenTypes.INPUT.value, + ) + is None + ) + + +# --------------------------------------------------------------------------- +# High-cardinality counters are left open, and per-process isolation holds +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("metrics_enabled") +def test_high_cardinality_counters_not_materialized(): + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + prometheus_metrics.initialize_bounded_metrics(instance_type="deriver") + # no endpoint/workspace_name series fabricated + assert ( + sample( + "api_requests_total", + method="GET", + endpoint="/v3/does-not-exist", + status_code="200", + ) + is None + ) + assert sample("messages_created_total", workspace_name="nope_ws") is None + + +@pytest.mark.usefixtures("metrics_enabled") +def test_api_init_does_not_touch_deriver_counters(): + """api-only init must not materialize or change a deriver-only counter. + + Delta-based (before == after) so it's robust to prior tests having + materialized the series. + """ + labels = dict( + task_type=DeriverTaskTypes.INGESTION.value, + token_type=TokenTypes.INPUT.value, + component=DeriverComponents.PROMPT.value, + ) + before = sample("deriver_tokens_processed_total", **labels) + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + after = sample("deriver_tokens_processed_total", **labels) + assert before == after + + +@pytest.mark.usefixtures("metrics_enabled") +def test_deriver_init_does_not_touch_api_counters(): + """The inverse: deriver-only init must not materialize an API-only counter. + + Without this, a deriver-startup regression could silently fabricate API + series (permanently-0 dialectic tokens on a process that never serves chat). + """ + labels = dict( + token_type=TokenTypes.INPUT.value, + component=DialecticComponents.TOTAL.value, + reasoning_level=REASONING_LEVELS[0], + ) + before = sample("dialectic_tokens_processed_total", **labels) + prometheus_metrics.initialize_bounded_metrics(instance_type="deriver") + after = sample("dialectic_tokens_processed_total", **labels) + assert before == after + # the API-process embed_now counters are equally off-limits + assert sample("embed_now_tasks_shed_total") is None + assert sample("embed_now_tasks_in_flight") is None + + +# --------------------------------------------------------------------------- +# telemetry_events_dropped: per-emitter child materialization +# --------------------------------------------------------------------------- + + +@pytest.mark.usefixtures("metrics_enabled") +def test_dropped_counter_children_materialized(): + prometheus_metrics.initialize_telemetry_dropped_metrics( + reasons=["buffer_full", "send_failed"] + ) + assert sample("telemetry_events_dropped_total", reason="buffer_full") is not None + assert sample("telemetry_events_dropped_total", reason="send_failed") is not None + + +def test_dropped_counter_init_noop_when_metrics_disabled( + monkeypatch: pytest.MonkeyPatch, +): + """The per-emitter initializer must no-op when metrics are disabled.""" + # region ai + # The enabled/disabled pair above and below this line exists for + # ``initialize_bounded_metrics`` (see ``test_init_noop_when_metrics_disabled``); + # without this test the sibling initializer had only the enabled half, so its + # ``METRICS.ENABLED`` guard could be deleted with the suite staying green. The + # unique namespace is what makes the absence assertion mean anything — the enabled + # test above materializes these same two reason values under a different one. + # endregion + monkeypatch.setattr("src.config.settings.METRICS.ENABLED", False) + monkeypatch.setattr( + "src.config.settings.METRICS.NAMESPACE", unique_ns("dropped_disabled") + ) + prometheus_metrics.initialize_telemetry_dropped_metrics( + reasons=["buffer_full", "send_failed"] + ) + assert sample("telemetry_events_dropped_total", reason="buffer_full") is None + assert sample("telemetry_events_dropped_total", reason="send_failed") is None + + +def test_init_noop_when_metrics_disabled(monkeypatch: pytest.MonkeyPatch): + """With metrics disabled, init must not fabricate series for a fresh label.""" + monkeypatch.setattr("src.config.settings.METRICS.ENABLED", False) + monkeypatch.setattr("src.config.settings.METRICS.NAMESPACE", unique_ns("disabled")) + prometheus_metrics.initialize_bounded_metrics(instance_type="api") + assert sample("telemetry_events_emitted_total", type="message.created") is None From 67f4dbf23ffba5bf8f7f5197ff8eba11f75993da Mon Sep 17 00:00:00 2001 From: Daniel Peng <97350516+original4422@users.noreply.github.com> Date: Thu, 20 Aug 2026 23:12:47 +0800 Subject: [PATCH 7/8] fix(llm): preserve reasoning content across tool turns (#1034) --- src/llm/history_adapters.py | 2 + src/llm/tool_loop.py | 3 + tests/llm/test_history_adapters.py | 48 ++++++++ tests/llm/test_tool_loop_reasoning_content.py | 104 ++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 tests/llm/test_tool_loop_reasoning_content.py diff --git a/src/llm/history_adapters.py b/src/llm/history_adapters.py index 02d2ea05..8e3b837e 100644 --- a/src/llm/history_adapters.py +++ b/src/llm/history_adapters.py @@ -121,6 +121,8 @@ class OpenAIHistoryAdapter: } if result.reasoning_details: message["reasoning_details"] = result.reasoning_details + elif result.thinking_content: + message["reasoning_content"] = result.thinking_content return message def format_tool_results( diff --git a/src/llm/tool_loop.py b/src/llm/tool_loop.py index 0feec1d8..783d4965 100644 --- a/src/llm/tool_loop.py +++ b/src/llm/tool_loop.py @@ -212,6 +212,7 @@ def format_assistant_tool_message( tool_calls: list[dict[str, Any]], thinking_blocks: list[dict[str, Any]] | None = None, reasoning_details: list[dict[str, Any]] | None = None, + thinking_content: str | None = None, ) -> dict[str, Any]: """Format an assistant message with tool calls in provider-native shape.""" from .backend import CompletionResult as BackendCompletionResult @@ -229,6 +230,7 @@ def format_assistant_tool_message( ) for tool_call in tool_calls ], + thinking_content=thinking_content, thinking_blocks=thinking_blocks or [], reasoning_details=reasoning_details or [], ) @@ -573,6 +575,7 @@ async def execute_tool_loop( response.tool_calls_made, response.thinking_blocks, response.reasoning_details, + response.thinking_content, ) conversation_messages.append(assistant_message) diff --git a/tests/llm/test_history_adapters.py b/tests/llm/test_history_adapters.py index 6881df6a..a992398d 100644 --- a/tests/llm/test_history_adapters.py +++ b/tests/llm/test_history_adapters.py @@ -1,3 +1,5 @@ +import pytest + from src.llm.backend import CompletionResult, ToolCallResult from src.llm.history_adapters import ( AnthropicHistoryAdapter, @@ -65,3 +67,49 @@ def test_openai_history_adapter_preserves_reasoning_details() -> None: assert message["role"] == "assistant" assert message["reasoning_details"] == [{"type": "reasoning", "content": "step 1"}] assert message["tool_calls"][0]["function"]["name"] == "search" + + +def test_openai_history_adapter_preserves_thinking_content() -> None: + adapter = OpenAIHistoryAdapter() + result = CompletionResult( + content="Calling a tool", + thinking_content="step 1", + tool_calls=[ + ToolCallResult(id="tool_1", name="search", input={"query": "honcho"}) + ], + ) + + message = adapter.format_assistant_tool_message(result) + + assert message["reasoning_content"] == "step 1" + assert "reasoning_details" not in message + + +def test_openai_history_adapter_prefers_reasoning_details() -> None: + adapter = OpenAIHistoryAdapter() + reasoning_details = [{"type": "reasoning", "content": "step 1"}] + result = CompletionResult( + content="Calling a tool", + thinking_content="duplicate step 1", + reasoning_details=reasoning_details, + ) + + message = adapter.format_assistant_tool_message(result) + + assert message["reasoning_details"] == reasoning_details + assert "reasoning_content" not in message + + +@pytest.mark.parametrize("thinking_content", [None, ""]) +def test_openai_history_adapter_omits_empty_thinking_content( + thinking_content: str | None, +) -> None: + adapter = OpenAIHistoryAdapter() + result = CompletionResult( + content="Calling a tool", + thinking_content=thinking_content, + ) + + message = adapter.format_assistant_tool_message(result) + + assert "reasoning_content" not in message diff --git a/tests/llm/test_tool_loop_reasoning_content.py b/tests/llm/test_tool_loop_reasoning_content.py new file mode 100644 index 00000000..cd4ec91b --- /dev/null +++ b/tests/llm/test_tool_loop_reasoning_content.py @@ -0,0 +1,104 @@ +from __future__ import annotations + +from copy import deepcopy +from typing import Any, cast +from unittest.mock import patch + +import pytest + +from src.config import ModelConfig +from src.llm import tool_loop +from src.llm.runtime import AttemptPlan +from src.llm.tool_loop import execute_tool_loop +from src.llm.types import HonchoLLMCallResponse, ProviderClient + + +def _make_plan() -> AttemptPlan: + return AttemptPlan( + provider="openai", + model="deepseek-v4-pro", + client=cast(ProviderClient, object()), + thinking_budget_tokens=None, + reasoning_effort=None, + selected_config=ModelConfig( + model="deepseek-v4-pro", + transport="openai", + ), + attempt=1, + retry_attempts=1, + is_fallback=False, + ) + + +@pytest.mark.asyncio +async def test_tool_loop_replays_reasoning_content_on_continuation() -> None: + calls: list[list[dict[str, Any]]] = [] + responses = iter( + [ + HonchoLLMCallResponse( + content="", + output_tokens=5, + finish_reasons=["tool_calls"], + tool_calls_made=[ + { + "id": "call_1", + "name": "search", + "input": {"query": "honcho"}, + } + ], + thinking_content="DeepSeek reasoning", + ), + HonchoLLMCallResponse( + content="done", + output_tokens=3, + finish_reasons=["stop"], + tool_calls_made=[], + ), + ] + ) + + async def fake_call(*_args: Any, **kwargs: Any) -> HonchoLLMCallResponse[Any]: + calls.append(deepcopy(kwargs["messages"])) + return next(responses) + + async def execute_search(_name: str, _input: dict[str, Any]) -> str: + return "result" + + with patch.object(tool_loop, "honcho_llm_call_inner", new=fake_call): + result = await execute_tool_loop( + prompt="hi", + max_tokens=64, + messages=[{"role": "user", "content": "hi"}], + tools=[ + { + "name": "search", + "description": "Search", + "input_schema": {"type": "object"}, + } + ], + tool_choice="auto", + tool_executor=execute_search, + max_tool_iterations=5, + response_model=None, + json_mode=False, + temperature=None, + stop_seqs=None, + verbosity=None, + enable_retry=False, + retry_attempts=1, + max_input_tokens=None, + get_attempt_plan=_make_plan, + before_retry_callback=lambda _retry_state: None, + stream_final=False, + telemetry=None, + ) + + assert isinstance(result, HonchoLLMCallResponse) + assert len(calls) == 2 + assert calls[1][1]["reasoning_content"] == "DeepSeek reasoning" + assert calls[1][1]["tool_calls"][0]["function"]["name"] == "search" + assert calls[1][2] == { + "role": "tool", + "tool_call_id": "call_1", + "content": "result", + } From ddbb90e36f2d148c7982f6ed85b09d31cabf5944 Mon Sep 17 00:00:00 2001 From: Aakash Kattelu Date: Thu, 20 Aug 2026 11:42:38 -0400 Subject: [PATCH 8/8] fix(embedding): truncate in batch embed and return results breakdown (#1019) * fix(deriver): truncate oversize observations so one cannot drop the batch simple_batch_embed raised ValueError when any input exceeded the per-input token cap, which failed the entire deriver save when a single observation was over-length. Add on_oversize="truncate": oversize inputs are embedded from a token-capped prefix (re-encoded until it fits, with a warning), preserving one vector per input. Default stays "raise" so existing callers are unchanged. RepresentationManager opts into truncate. Also add a live embedding test that fails on main (raise / missing kwarg) and passes once a mixed short+oversize batch survives. Refs #569 * fix(deriver): surface failure when all observer saves fail When every observer's save_representation failed (e.g. embedding retries exhausted under a sustained 429), the deriver logged the error and returned normally, so the queue marked the work unit processed with zero documents saved. Collect per-observer errors and, after telemetry is emitted, raise RepresentationSaveError when no observer succeeded. Partial failures stay processed (saved observers must not be discarded) and are recorded via an additive failed_observer_count on RepresentationCompletedEvent. Refs #728 * fix(embedding): guarantee truncation progress and truncate on re-embed The retry slice in _truncate_to_token_limit always recomputed the same keep count, so a slice whose re-encode grew past the cap could oscillate. Decrement keep after each unsuccessful retry. Document re-embed in the reconciler used the default on_oversize="raise", so one oversize document failed every other document in the batch. Co-Authored-By: Claude Opus 5 * chore: drop ticket ids and shrink comments to one sentence Comments and docstrings describe current behavior, not the PR that introduced them. Ticket numbers stay in the commit/PR. * chore: annotate RepresentationSaveError and assert truncate on re-embed * fix(embedding): truncate on conclusion create paths and document BPE loop Storage callers in create_observations (API + agent tools) now pass on_oversize="truncate" so a single oversize item cannot drop the batch. Docstring on _truncate_to_token_limit notes why decode/re-encode is load-bearing. --------- Co-authored-by: Claude Opus 5 --- src/crud/document.py | 4 +- src/crud/representation.py | 2 +- src/deriver/deriver.py | 20 ++- src/embedding_client.py | 72 ++++++++-- src/exceptions.py | 8 ++ src/reconciler/sync_vectors.py | 4 +- src/telemetry/events/representation.py | 4 + src/utils/agent_tools.py | 4 +- tests/conftest.py | 4 +- tests/crud/test_document.py | 40 ++++++ tests/crud/test_representation_manager.py | 63 ++++++++- tests/deriver/test_deriver_processing.py | 112 ++++++++++++++++ tests/deriver/test_vector_reconciliation.py | 5 +- tests/live_llm/README.md | 2 +- tests/live_llm/test_live_embeddings.py | 20 +++ tests/llm/test_embedding_client.py | 126 ++++++++++++++++++ .../test_representation_v2_fields.py | 2 + tests/utils/test_agent_tools.py | 66 ++++++++- 18 files changed, 530 insertions(+), 28 deletions(-) diff --git a/src/crud/document.py b/src/crud/document.py index ba80712d..0cec85c9 100644 --- a/src/crud/document.py +++ b/src/crud/document.py @@ -984,7 +984,9 @@ async def create_observations( # Generate embeddings in batch contents = [obs.content for obs in observations] try: - embeddings = await embedding_client.simple_batch_embed(contents) + embeddings = await embedding_client.simple_batch_embed( + contents, on_oversize="truncate" + ) except ValueError as e: raise ValidationException(str(e)) from e diff --git a/src/crud/representation.py b/src/crud/representation.py index 57e6e8e1..3b7070e8 100644 --- a/src/crud/representation.py +++ b/src/crud/representation.py @@ -107,7 +107,7 @@ class RepresentationManager: parent_category="representation", ): embeddings = await embedding_client.simple_batch_embed( - observation_texts + observation_texts, on_oversize="truncate" ) except ValueError as e: raise exceptions.ValidationException( diff --git a/src/deriver/deriver.py b/src/deriver/deriver.py index 2ad7d14f..f76c4d52 100644 --- a/src/deriver/deriver.py +++ b/src/deriver/deriver.py @@ -7,6 +7,7 @@ from src import crud from src.config import ConfiguredModelSettings, settings from src.crud.representation import RepresentationManager from src.dependencies import tracked_db +from src.exceptions import RepresentationSaveError from src.llm import honcho_llm_call from src.llm.types import LLMTelemetryContext from src.models import Message @@ -196,6 +197,7 @@ async def process_representation_tasks_batch( agg_representation_result = crud.CreateDocumentsResult() successful_observer_count = 0 + save_errors: list[tuple[str, Exception]] = [] if observations.is_empty() or not message_ids: logger.warning( "Deriver generated zero observations for messages %s:%s in %s/%s!", @@ -236,10 +238,11 @@ async def process_representation_tasks_batch( representation_result.semantic_dup_replaced_count ) successful_observer_count += 1 - except Exception as e: - logger.error( - "Failed to save representation for observer %s: %s", observer, e + except Exception as e: # noqa: BLE001 + logger.exception( + "Failed to save representation for observer %s", observer ) + save_errors.append((observer, e)) # Log metrics overall_duration = (time.perf_counter() - overall_start) * 1000 @@ -337,5 +340,16 @@ async def process_representation_tasks_batch( exact_dup_in_batch_count=agg_representation_result.exact_dup_in_batch_count, semantic_dup_rejected_count=agg_representation_result.semantic_dup_rejected_count, semantic_dup_replaced_count=agg_representation_result.semantic_dup_replaced_count, + failed_observer_count=len(save_errors), ) ) + + if save_errors and successful_observer_count == 0: + details = "; ".join( + f"{observer}: {exc.__class__.__name__}: {exc}" + for observer, exc in save_errors + ) + raise RepresentationSaveError( + f"save_representation failed for all {len(save_errors)} observer(s): " + + details + ) from save_errors[0][1] diff --git a/src/embedding_client.py b/src/embedding_client.py index 3cee4484..e55e09fd 100644 --- a/src/embedding_client.py +++ b/src/embedding_client.py @@ -321,39 +321,78 @@ class _EmbeddingClient: fn=_call_openai, ) - async def simple_batch_embed(self, texts: list[str]) -> list[list[float]]: + def _truncate_to_token_limit(self, text: str) -> tuple[str, int]: + """Return a prefix of `text` whose re-encoded token count fits the cap. + + Decode/re-encode after slicing: BPE boundaries can re-expand past the cap. """ - Batch-embed a list of text strings. Each input must already fit within - `max_embedding_tokens`; this method does not sub-chunk oversized inputs. + token_ids = self.encoding.encode(text) + keep = self.max_embedding_tokens + while len(token_ids) > self.max_embedding_tokens: + keep = min(keep, len(token_ids) - 1) + if keep < 1: + return "", 0 + text = self.encoding.decode(token_ids[:keep]) + token_ids = self.encoding.encode(text) + keep -= 1 + return text, len(token_ids) + + async def simple_batch_embed( + self, + texts: list[str], + *, + on_oversize: Literal["raise", "truncate"] = "raise", + ) -> list[list[float]]: + """ + Batch-embed a list of text strings. Does not sub-chunk oversized inputs. Internally goes through the same token-aware batching pipeline as `batch_embed()` so the per-request token cap is respected. Args: texts: List of text strings to embed + on_oversize: ``"raise"`` (default) errors; ``"truncate"`` embeds a + token-capped prefix. Returns: List of embedding vectors, one per input text (in order) Raises: - ValueError: If any text exceeds token limits + ValueError: If any text exceeds token limits and `on_oversize` is + ``"raise"`` """ if not texts: return [] - # Validate per-input token limit and collect token counts for batching + # Validate / cap per-input token limit and collect counts for batching + prepared_texts: list[str] = [] token_counts: list[int] = [] for idx, text in enumerate(texts): - tokens = len(self.encoding.encode(text)) - if tokens > self.max_embedding_tokens: - raise ValueError( - f"Text at index {idx} exceeds maximum token limit of {self.max_embedding_tokens} tokens (got {tokens} tokens)" - ) + token_ids = self.encoding.encode(text) + if len(token_ids) > self.max_embedding_tokens: + if on_oversize == "truncate": + original_count = len(token_ids) + text, tokens = self._truncate_to_token_limit(text) + logger.warning( + "truncated oversize embedding input at idx %d: %d->%d tokens", + idx, + original_count, + tokens, + ) + else: + raise ValueError( + f"Text at index {idx} exceeds maximum token limit of " + + f"{self.max_embedding_tokens} tokens (got {len(token_ids)} tokens)" + ) + else: + tokens = len(token_ids) + prepared_texts.append(text) token_counts.append(tokens) # Use positional indices as text_ids so we can reassemble in input order. text_chunks: dict[str, list[tuple[str, int]]] = { - str(i): [(text, token_counts[i])] for i, text in enumerate(texts) + str(i): [(prepared_texts[i], token_counts[i])] + for i in range(len(prepared_texts)) } batches = self._create_batches(text_chunks) @@ -695,9 +734,16 @@ class EmbeddingClient: """Embed a single query string.""" return await self._get_client().embed(query) - async def simple_batch_embed(self, texts: list[str]) -> list[list[float]]: + async def simple_batch_embed( + self, + texts: list[str], + *, + on_oversize: Literal["raise", "truncate"] = "raise", + ) -> list[list[float]]: """Batch embed a list of text strings (each must fit token limit).""" - return await self._get_client().simple_batch_embed(texts) + return await self._get_client().simple_batch_embed( + texts, on_oversize=on_oversize + ) def prepare_chunks(self, id_resource_dict: dict[str, str]) -> dict[str, list[str]]: """Chunk texts using the same rules as `batch_embed` (no network).""" diff --git a/src/exceptions.py b/src/exceptions.py index 129324b6..41d775fe 100644 --- a/src/exceptions.py +++ b/src/exceptions.py @@ -133,6 +133,14 @@ class VectorStoreError(HonchoException): detail = "Vector store operation failed" +@final +class RepresentationSaveError(HonchoException): + """Raised when every observer's representation save fails in a batch.""" + + status_code: int = 500 + detail: str = "Representation save failed for all observers" + + class LLMError(Exception): """Exception raised when an LLM call fails. diff --git a/src/reconciler/sync_vectors.py b/src/reconciler/sync_vectors.py index 1ff94542..1a8e99b5 100644 --- a/src/reconciler/sync_vectors.py +++ b/src/reconciler/sync_vectors.py @@ -302,7 +302,9 @@ async def _sync_documents( EmbeddingCallPurpose.VECTOR_SYNC.value, parent_category="reconciliation", ): - new_embeddings = await embedding_client.simple_batch_embed(contents) + new_embeddings = await embedding_client.simple_batch_embed( + contents, on_oversize="truncate" + ) if len(new_embeddings) != len(docs_needing_embed): logger.warning( diff --git a/src/telemetry/events/representation.py b/src/telemetry/events/representation.py index ad6dfca4..c4291092 100644 --- a/src/telemetry/events/representation.py +++ b/src/telemetry/events/representation.py @@ -154,6 +154,10 @@ class RepresentationCompletedEvent(BaseEvent): default=0, description="Number of observers this representation was saved against", ) + failed_observer_count: int = Field( + default=0, + description="Number of observers whose save_representation failed (partial or total)", + ) def get_resource_id(self) -> str: """Resource ID includes workspace, session, and latest message for uniqueness.""" diff --git a/src/utils/agent_tools.py b/src/utils/agent_tools.py index 7f094154..b8af9eee 100644 --- a/src/utils/agent_tools.py +++ b/src/utils/agent_tools.py @@ -930,7 +930,9 @@ async def create_observations( run_id=run_id, parent_category=parent_category, ): - embeddings = await embedding_client.simple_batch_embed(contents) + embeddings = await embedding_client.simple_batch_embed( + contents, on_oversize="truncate" + ) embeddings_by_index = dict( zip(range(len(normalized_observations)), embeddings, strict=True) ) diff --git a/tests/conftest.py b/tests/conftest.py index f99859b3..8a078652 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -604,7 +604,9 @@ def mock_openai_embeddings(request: pytest.FixtureRequest): mock_embed.side_effect = embed_side_effect - async def mock_simple_batch_embed_func(texts: list[str]) -> list[list[float]]: + async def mock_simple_batch_embed_func( + texts: list[str], **_kwargs: object + ) -> list[list[float]]: return [_content_to_embedding(text) for text in texts] mock_simple_batch_embed.side_effect = mock_simple_batch_embed_func diff --git a/tests/crud/test_document.py b/tests/crud/test_document.py index 5f03c80b..6686e688 100644 --- a/tests/crud/test_document.py +++ b/tests/crud/test_document.py @@ -1006,6 +1006,46 @@ class TestDocumentCRUD: assert documents[0].content in ["Observation 1", "Observation 2"] assert documents[1].content in ["Observation 1", "Observation 2"] + @pytest.mark.asyncio + async def test_create_observations_embeds_with_truncate_on_oversize( + self, + db_session: AsyncSession, + sample_data: tuple[models.Workspace, models.Peer], + ): + """API conclusion creates must opt into truncation on oversize content.""" + test_workspace, test_peer = sample_data + test_peer2, test_session, _ = await self._setup_test_data( + db_session, test_workspace, test_peer + ) + + with patch( + "src.crud.document.embedding_client.simple_batch_embed", + new=AsyncMock(return_value=[[0.1] * 1536, [0.2] * 1536]), + ) as mock_embed: + created = await crud.create_observations( + db_session, + observations=[ + schemas.ConclusionCreate( + content="short conclusion", + observer_id=test_peer.name, + observed_id=test_peer2.name, + session_id=test_session.name, + ), + schemas.ConclusionCreate( + content="another conclusion", + observer_id=test_peer.name, + observed_id=test_peer2.name, + session_id=test_session.name, + ), + ], + workspace_name=test_workspace.name, + ) + + assert len(created) == 2 + mock_embed.assert_awaited_once_with( + ["short conclusion", "another conclusion"], on_oversize="truncate" + ) + class TestSessionPurityInvariant: """Regression tests for the explicit-document session-purity invariant. diff --git a/tests/crud/test_representation_manager.py b/tests/crud/test_representation_manager.py index 6c5e094c..3c2f57e4 100644 --- a/tests/crud/test_representation_manager.py +++ b/tests/crud/test_representation_manager.py @@ -520,7 +520,9 @@ class TestRepresentationManagerSave: ) assert len(saved.created_documents) == 1 - mock_embed.assert_awaited_once_with(["useful observation"]) + mock_embed.assert_awaited_once_with( + ["useful observation"], on_oversize="truncate" + ) saved_observations = _saved_observations(mock_save) assert len(saved_observations) == 1 assert saved_observations[0].content == "useful observation" @@ -576,7 +578,9 @@ class TestRepresentationManagerSave: ) assert len(saved.created_documents) == 1 - mock_embed.assert_awaited_once_with(["inferred conclusion"]) + mock_embed.assert_awaited_once_with( + ["inferred conclusion"], on_oversize="truncate" + ) saved_observations = _saved_observations(mock_save) assert len(saved_observations) == 1 assert isinstance(saved_observations[0], DeductiveObservation) @@ -630,6 +634,61 @@ class TestRepresentationManagerSave: mock_embed.assert_not_awaited() mock_save.assert_not_awaited() + @pytest.mark.asyncio + async def test_save_representation_embeds_with_truncate_on_oversize(self): + """One oversize observation must not drop the rest of the batch.""" + manager = RepresentationManager( + "workspace", + observer="observer", + observed="observed", + ) + representation = Representation( + explicit=[ + ExplicitObservation( + content="short fact", + created_at=datetime.now(timezone.utc), + message_ids=[1], + session_name="session", + ) + ], + deductive=[ + DeductiveObservation( + conclusion="inferred fact", + premises=["premise"], + source_ids=["doc-a"], + created_at=datetime.now(timezone.utc), + message_ids=[1], + session_name="session", + ) + ], + ) + + with ( + patch("src.crud.representation.tracked_db", _fake_tracked_db), + patch( + "src.crud.representation.embedding_client.simple_batch_embed", + new=AsyncMock(return_value=[[0.1], [0.2]]), + ) as mock_embed, + patch.object( + manager, + "_save_representation_internal", + new=AsyncMock( + return_value=CreateDocumentsResult(created_documents=[MagicMock()]) + ), + ), + ): + await manager.save_representation( + representation, + message_ids=[1], + session_name="session", + message_created_at=datetime.now(timezone.utc), + message_level_configuration=_resolved_config(), + ) + + mock_embed.assert_awaited_once_with( + ["inferred fact", "short fact"], on_oversize="truncate" + ) + class TestVectorQueryTopKFloor: """Regression for HONCHO-19Q / HONCHO-4Q4. diff --git a/tests/deriver/test_deriver_processing.py b/tests/deriver/test_deriver_processing.py index 6785d159..29a983b5 100644 --- a/tests/deriver/test_deriver_processing.py +++ b/tests/deriver/test_deriver_processing.py @@ -7,7 +7,9 @@ import pytest from src import crud, models from src.config import settings +from src.crud.representation import RepresentationManager from src.deriver.deriver import process_representation_tasks_batch +from src.exceptions import RepresentationSaveError from src.llm import HonchoLLMCallResponse from src.utils.representation import ( ExplicitObservationBase, @@ -70,6 +72,116 @@ class TestDeriverProcessing: assert kwargs["model_config"].stop_sequences == expected_config.stop_sequences assert "llm_settings" not in kwargs + async def test_all_observer_saves_failing_surfaces_failure(self): + """When every observer's save_representation fails, the batch must raise.""" + message = Mock( + id=1, + public_id="msg_1", + session_name="session-1", + workspace_name="workspace-1", + peer_name="alice", + content="hello", + token_count=5, + created_at=datetime.now(timezone.utc), + ) + configuration = Mock() + configuration.reasoning.enabled = True + + mock_response = HonchoLLMCallResponse( + content=PromptRepresentation( + explicit=[ + ExplicitObservationBase(content="The user has a dog named Rover") + ] + ), + input_tokens=10, + output_tokens=5, + finish_reasons=["STOP"], + ) + + failing_save = AsyncMock(side_effect=RuntimeError("429 RESOURCE_EXHAUSTED")) + emitted: list[Any] = [] + with ( + patch( + "src.deriver.deriver.honcho_llm_call", + new_callable=AsyncMock, + return_value=mock_response, + ), + patch.object(RepresentationManager, "save_representation", failing_save), + patch("src.deriver.deriver.emit", side_effect=emitted.append), + pytest.raises(RepresentationSaveError, match="save_representation failed"), + ): + await process_representation_tasks_batch( + messages=[message], + message_level_configuration=configuration, + observers=["bob"], + observed="alice", + queue_item_message_ids=[1], + ) + + # Telemetry must fire *before* the raise so a total save failure is still + # visible to metrics. Guards against emit() being moved after the raise. + assert emitted, "expected telemetry to be emitted before the raised failure" + assert emitted[-1].observer_count == 0 + assert emitted[-1].failed_observer_count == 1 + + async def test_partial_observer_failure_is_processed_and_surfaced(self): + """When some observers save and one fails, the batch does NOT raise + (saved observers are kept) and the failure is visible via telemetry. + """ + message = Mock( + id=1, + public_id="msg_1", + session_name="session-1", + workspace_name="workspace-1", + peer_name="alice", + content="hello", + token_count=5, + created_at=datetime.now(timezone.utc), + ) + configuration = Mock() + configuration.reasoning.enabled = True + + mock_response = HonchoLLMCallResponse( + content=PromptRepresentation( + explicit=[ + ExplicitObservationBase(content="The user has a dog named Rover") + ] + ), + input_tokens=10, + output_tokens=5, + finish_reasons=["STOP"], + ) + + # bob succeeds, carol fails. + partial_save = AsyncMock( + side_effect=[ + crud.CreateDocumentsResult(), + RuntimeError("429 RESOURCE_EXHAUSTED"), + ] + ) + emitted: list[Any] = [] + with ( + patch( + "src.deriver.deriver.honcho_llm_call", + new_callable=AsyncMock, + return_value=mock_response, + ), + patch.object(RepresentationManager, "save_representation", partial_save), + patch("src.deriver.deriver.emit", side_effect=emitted.append), + ): + await process_representation_tasks_batch( + messages=[message], + message_level_configuration=configuration, + observers=["bob", "carol"], + observed="alice", + queue_item_message_ids=[1], + ) + + assert emitted, "expected a telemetry event to be emitted" + event = emitted[-1] + assert event.observer_count == 1 + assert event.failed_observer_count == 1 + async def test_process_representation_tasks_batch_passes_custom_instructions_into_prompt( self, ) -> None: diff --git a/tests/deriver/test_vector_reconciliation.py b/tests/deriver/test_vector_reconciliation.py index 5e2e4712..0ecd4d5c 100644 --- a/tests/deriver/test_vector_reconciliation.py +++ b/tests/deriver/test_vector_reconciliation.py @@ -551,9 +551,12 @@ class TestReEmbedding: # Mock embedding client to track batch calls batch_call_count = 0 - async def track_batch_embed(contents: list[str]) -> list[list[float]]: + async def track_batch_embed( + contents: list[str], *, on_oversize: str, **_kwargs: object + ) -> list[list[float]]: nonlocal batch_call_count batch_call_count += 1 + assert on_oversize == "truncate" return [[1.0] * 1536 for _ in contents] with patch("src.reconciler.sync_vectors.embedding_client") as mock_embed_client: diff --git a/tests/live_llm/README.md b/tests/live_llm/README.md index 442049bc..5cf39e4b 100644 --- a/tests/live_llm/README.md +++ b/tests/live_llm/README.md @@ -68,5 +68,5 @@ Coverage by provider: - OpenAI transport → OpenRouter non-reasoning models (e.g. `inception/mercury-2`): non-chat / diffusion architectures must stay on `max_tokens`, no `reasoning_effort`, tool-calling parameter-schema compatibility is the canary for exotic OR-served providers - Gemini 2.5/3.0 classes: structured outputs, cached-content reuse, thought signatures, multi-turn tool replay - Gemini 3.1 class: thinking and tool replay coverage by default; structured-output/caching coverage should only be added once Google documents support for that path -- Embeddings (`test_live_embeddings.py`): single embed, batched embed, batch-vs-single alignment, and chunk-to-id mapping for every configured embedding model. `gemini-embedding-2*` is the reason this exists — those models collapse a list of bare strings into one document (#745), and only a live call catches it. Also covers first-class `EmbeddingModelConfig.timeout` plumbing (one representative model per transport): configured timeout lands on the SDK client, and a near-zero timeout aborts before the provider answers +- Embeddings (`test_live_embeddings.py`): single embed, batched embed, batch-vs-single alignment, chunk-to-id mapping, and oversize-truncate survival (`on_oversize="truncate"`) for every configured embedding model. `gemini-embedding-2*` is the reason this exists — those models collapse a list of bare strings into one document (#745), and only a live call catches it. Also covers first-class `EmbeddingModelConfig.timeout` plumbing (one representative model per transport): configured timeout lands on the SDK client, and a near-zero timeout aborts before the provider answers - OpenAI-compatible embedding providers (e.g. OpenRouter's `google/gemini-embedding-001`): the #932 surface. Those providers reject a base64 embedding request outright (HTTP 400) or answer HTTP 200 with empty data, so the whole matrix fails without `encoding_format="float"`. Real OpenAI accepts base64 happily, so only a third-party provider catches it. Note that OpenRouter load-balances across upstreams, so the base64 failure is per-attempt rather than guaranteed: a retry can land on an endpoint that accepts it. `test_live_openai_float_encoding_matches_base64` covers the other side, that the float switch must not move vectors on real OpenAI diff --git a/tests/live_llm/test_live_embeddings.py b/tests/live_llm/test_live_embeddings.py index 6ef2b048..7c57a2e3 100644 --- a/tests/live_llm/test_live_embeddings.py +++ b/tests/live_llm/test_live_embeddings.py @@ -197,6 +197,26 @@ async def test_live_openai_float_encoding_matches_base64( ), f"{spec.id}: float encoding diverges from base64 (cosine={similarity:.8f})" +@pytest.mark.asyncio +@pytest.mark.parametrize("spec", ALL_SPECS, ids=lambda spec: spec.id) +async def test_live_batch_embed_truncates_oversize_instead_of_dropping_batch( + spec: LiveEmbeddingSpec, +) -> None: + """on_oversize='truncate' keeps one vector per input when an item exceeds the cap.""" + # Tiny cap so the oversize input stays cheap to tokenize and send. + client = make_embedding_client(spec, max_input_tokens=32) + oversize = " ".join(f"oversize-token-{index}" for index in range(200)) + assert len(client.encoding.encode(oversize)) > client.max_embedding_tokens + + texts = [BATCH_TEXTS[0], oversize, BATCH_TEXTS[1]] + embeddings = await client.simple_batch_embed(texts, on_oversize="truncate") + + assert len(embeddings) == len(texts) + assert all(len(embedding) == spec.dimensions for embedding in embeddings) + # A collapsed or dropped batch would reuse a vector or return fewer. + assert len({tuple(embedding) for embedding in embeddings}) == len(texts) + + @pytest.mark.asyncio @pytest.mark.parametrize("spec", GEMINI_SPECS, ids=lambda spec: spec.id) async def test_live_gemini_batch_embed_survives_batch_split( diff --git a/tests/llm/test_embedding_client.py b/tests/llm/test_embedding_client.py index 9df80be1..fc2ff411 100644 --- a/tests/llm/test_embedding_client.py +++ b/tests/llm/test_embedding_client.py @@ -13,6 +13,7 @@ from src.config import ( ) from src.embedding_client import ( BatchItem, + EmbeddingClient, _EmbeddingClient, # pyright: ignore[reportPrivateUsage] ) @@ -910,6 +911,131 @@ async def test_simple_batch_embed_rejects_oversized_input( await client.simple_batch_embed([too_long]) +@pytest.mark.asyncio +async def test_simple_batch_embed_truncates_oversize_when_requested( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """on_oversize='truncate' embeds a prefix instead of failing the batch.""" + fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 4) + + class FakeOpenAIClient: + def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings + + monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) + + client = _EmbeddingClient( + EmbeddingModelConfig( + transport="openai", + model="text-embedding-3-small", + api_key="test-key", + base_url=None, + ), + vector_dimensions=4, + max_input_tokens=10, + max_tokens_per_request=1000, + send_dimensions=False, + ) + + short = "hello" + too_long = ("word " * 50).strip() + assert len(client.encoding.encode(too_long)) > client.max_embedding_tokens + + out = await client.simple_batch_embed([short, too_long], on_oversize="truncate") + + assert len(out) == 2 + assert fake_embeddings.calls, "expected a provider call after truncation" + received = fake_embeddings.calls[0]["input"] + assert received[0] == short + truncated = received[1] + assert isinstance(truncated, str) + assert truncated != too_long + assert len(client.encoding.encode(truncated)) <= client.max_embedding_tokens + + +@pytest.mark.asyncio +async def test_simple_batch_embed_truncate_reencodes_until_under_cap( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """decode(ids[:n]) can re-encode past n; truncate must re-verify the count.""" + fake_embeddings = FakeOpenAIEmbeddingsAPI([0.1] * 4) + + class FakeOpenAIClient: + def __init__(self, *, api_key: str | None, base_url: str | None) -> None: + self.embeddings: FakeOpenAIEmbeddingsAPI = fake_embeddings + + monkeypatch.setattr("openai.AsyncOpenAI", FakeOpenAIClient) + + client = _EmbeddingClient( + EmbeddingModelConfig( + transport="openai", + model="text-embedding-3-small", + api_key="test-key", + base_url=None, + ), + vector_dimensions=4, + max_input_tokens=10, + max_tokens_per_request=1000, + send_dimensions=False, + ) + + encode_calls = {"n": 0} + + def encode(text: str) -> list[int]: + encode_calls["n"] += 1 + if text.startswith("LONG"): + # 1: original oversize; 2: still over after first slice; 3+: fits. + if encode_calls["n"] == 1: + return list(range(20)) + if encode_calls["n"] == 2: + return list(range(12)) + return list(range(8)) + return [1] + + def decode(ids: list[int]) -> str: + return "LONG" + "x" * len(ids) + + monkeypatch.setattr(client.encoding, "encode", encode) + monkeypatch.setattr(client.encoding, "decode", decode) + + out = await client.simple_batch_embed(["LONG-input"], on_oversize="truncate") + + assert len(out) == 1 + received = fake_embeddings.calls[0]["input"][0] + assert isinstance(received, str) + # The provider must see the post-loop text, which encodes to 8 (<= cap). + assert encode(received) == list(range(8)) + assert encode_calls["n"] >= 3 + + +@pytest.mark.asyncio +async def test_public_embedding_client_forwards_on_oversize( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The singleton wrapper must forward on_oversize to the inner client.""" + captured: dict[str, object] = {} + + class FakeInner: + async def simple_batch_embed( + self, + texts: list[str], + *, + on_oversize: str = "raise", + ) -> list[list[float]]: + captured["texts"] = texts + captured["on_oversize"] = on_oversize + return [[0.1]] + + wrapper = EmbeddingClient() + monkeypatch.setattr(wrapper, "_get_client", lambda: FakeInner()) + + out = await wrapper.simple_batch_embed(["hi"], on_oversize="truncate") + + assert out == [[0.1]] + assert captured["texts"] == ["hi"] + assert captured["on_oversize"] == "truncate" + + def test_prepare_chunks_returns_ordered_chunks( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/telemetry/test_representation_v2_fields.py b/tests/telemetry/test_representation_v2_fields.py index a41bed41..6f2251ea 100644 --- a/tests/telemetry/test_representation_v2_fields.py +++ b/tests/telemetry/test_representation_v2_fields.py @@ -51,6 +51,7 @@ class TestRepresentationV2AdditiveFields: assert event.exact_dup_existing_count == 0 assert event.semantic_dup_rejected_count == 0 assert event.semantic_dup_replaced_count == 0 + assert event.failed_observer_count == 0 def test_input_tokens_semantics_preserved(self): """The downstream metering key must remain 'queued-message tokens'. @@ -161,6 +162,7 @@ class TestRepresentationV2AdditiveFields: "exact_dup_existing_count", "semantic_dup_rejected_count", "semantic_dup_replaced_count", + "failed_observer_count", ): assert field in data, f"missing field: {field}" assert data["hit_batch_token_cap"] is True diff --git a/tests/utils/test_agent_tools.py b/tests/utils/test_agent_tools.py index e53598fa..ac45cabf 100644 --- a/tests/utils/test_agent_tools.py +++ b/tests/utils/test_agent_tools.py @@ -334,7 +334,9 @@ class TestCreateObservations: """If batch embedding fails but individual embeds succeed, all observations are created.""" workspace, peer1, peer2, session, _, _ = tool_test_data - async def fail_batch_embed(_texts: list[str]) -> list[list[float]]: + async def fail_batch_embed( + _texts: list[str], **_kwargs: object + ) -> list[list[float]]: raise RuntimeError("embedding provider timeout") async def succeed_single_embed(_content: str) -> list[float]: @@ -393,7 +395,9 @@ class TestCreateObservations: """If batch embedding fails and some individual embeds also fail, only successful ones are created.""" workspace, peer1, peer2, session, _, _ = tool_test_data - async def fail_batch_embed(_texts: list[str]) -> list[list[float]]: + async def fail_batch_embed( + _texts: list[str], **_kwargs: object + ) -> list[list[float]]: raise RuntimeError("embedding provider timeout") async def embed_per_observation(content: str) -> list[float]: @@ -458,7 +462,9 @@ class TestCreateObservations: workspace, peer1, peer2, session, _, _ = tool_test_data created_documents: list[Any] = [] - async def fake_batch_embed(texts: list[str]) -> list[list[float]]: + async def fake_batch_embed( + texts: list[str], **_kwargs: object + ) -> list[list[float]]: assert texts == ["trimmed observation"] return [[0.4, 0.5, 0.6]] @@ -504,6 +510,60 @@ class TestCreateObservations: assert len(created_documents) == 1 assert created_documents[0].content == "trimmed observation" + async def test_create_observations_embeds_with_truncate_on_oversize( + self, + tool_test_data: Any, + monkeypatch: pytest.MonkeyPatch, + ): + """Storage path must opt into truncation so one long obs cannot drop the batch.""" + workspace, peer1, peer2, session, _, _ = tool_test_data + captured: dict[str, object] = {} + + async def fake_batch_embed( + texts: list[str], *, on_oversize: str = "raise", **_kwargs: object + ) -> list[list[float]]: + captured["texts"] = texts + captured["on_oversize"] = on_oversize + return [[0.1] for _ in texts] + + async def fake_create_documents( + _db: AsyncSession, + documents: list[Any], + workspace_name: str, + *, + observer: str, + observed: str, + deduplicate: bool = False, + ) -> crud.CreateDocumentsResult: + _ = (workspace_name, observer, observed, deduplicate) + return crud.CreateDocumentsResult(created_documents=documents) + + monkeypatch.setattr( + "src.utils.agent_tools.embedding_client.simple_batch_embed", + fake_batch_embed, + ) + monkeypatch.setattr( + "src.utils.agent_tools.crud.create_documents", fake_create_documents + ) + + result = await create_observations( + observations=[ + schemas.ObservationInput(content="short fact", level="explicit"), + schemas.ObservationInput(content="long fact", level="explicit"), + ], + observer=peer1.name, + observed=peer2.name, + session_name=session.name, + workspace_name=workspace.name, + message_ids=[], + message_created_at=str(datetime.now(timezone.utc)), + ) + + assert isinstance(result, ObservationsCreatedResult) + assert result.created_count == 2 + assert captured["on_oversize"] == "truncate" + assert captured["texts"] == ["short fact", "long fact"] + async def test_create_observations_skips_all_blank_content( self, tool_test_data: Any,