From d2355b8ac2faa957697f854de2ef923e0ecbe642 Mon Sep 17 00:00:00 2001 From: ahmeda-cominty Date: Tue, 28 Jul 2026 13:09:36 +0200 Subject: [PATCH 1/5] Add full memory support to the Python SDK --- AGENTS.md | 2 +- CHANGELOG.md | 12 +- README.md | 34 +++- examples/09_memory.py | 66 +++++++ examples/README.md | 3 +- src/cominty_sdk/__init__.py | 10 ++ src/cominty_sdk/client.py | 4 +- src/cominty_sdk/models/__init__.py | 7 +- src/cominty_sdk/models/memory.py | 71 ++++++++ src/cominty_sdk/resources/__init__.py | 3 +- src/cominty_sdk/resources/memory.py | 116 ++++++++++++ tests/integration/test_smoke.py | 37 +++- tests/unit/test_memory.py | 250 ++++++++++++++++++++++++++ 13 files changed, 607 insertions(+), 8 deletions(-) create mode 100644 examples/09_memory.py create mode 100644 src/cominty_sdk/models/memory.py create mode 100644 src/cominty_sdk/resources/memory.py create mode 100644 tests/unit/test_memory.py diff --git a/AGENTS.md b/AGENTS.md index 9f630f5..bd27ec3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -448,4 +448,4 @@ One-time PyPI setup (must match the workflow exactly, or PyPI rejects the token) Note: `release.yml` builds + `twine check`s but does **not** run the test suite — tests run in `ci.yml` on push/PR to `main`/`dev`. Only merge to `main` through green CI so a Release -never ships untested code. +never ships untested code. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 25725c2..9e15737 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- `client.memory` — full async CRUD for per-user memory files: `list()`, + `create()`, `get()`, `update()`, `delete()` (`GET/POST /memory`, + `GET/PUT/DELETE /memory/file`). New models `MemoryFileCreate`, + `MemoryFileUpdate`, `MemoryFileOut`, `MemoryFileSummaryOut`. `update()` is a + partial update — pass only the fields you want to change, distinguishing an + omitted field (left untouched) from an explicit `None` (cleared) — and + guards against concurrent writes via an opaque `version` token, raising + `ConflictError` (409) on a stale value. See `examples/09_memory.py`. + ### Changed - `__version__` is now resolved at runtime from installed package metadata (`importlib.metadata.version("cominty-sdk")`) instead of the removed @@ -43,4 +53,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [Unreleased]: https://github.com/cominty/python-sdk/compare/v0.1.1...HEAD [0.1.1]: https://github.com/cominty/python-sdk/compare/v0.1.0...v0.1.1 -[0.1.0]: https://github.com/cominty/python-sdk/releases/tag/v0.1.0 +[0.1.0]: https://github.com/cominty/python-sdk/releases/tag/v0.1.0 \ No newline at end of file diff --git a/README.md b/README.md index 3a249ce..201cff1 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,38 @@ await client.threads.update(thread_id, name="Renamed", starred=True) await client.threads.archive(thread_id) ``` +### Memory files + +`client.memory` stores per-user files an agent can read back later — scoped to +the client's `user_id` automatically. + +```python +# Create a file +file = await client.memory.create( + path="preferences/tone.md", purpose="writing style", content="Keep it casual." +) + +# List files (summaries — no content) +for f in await client.memory.list(): + print(f.path, f.purpose, f.version) + +# Read one file's content +file = await client.memory.get("preferences/tone.md") + +# Partial update — only the fields you pass change. `version` guards against +# overwriting a concurrent change: pass back the value from your last read, +# and a stale one raises ConflictError (409). +file = await client.memory.update( + "preferences/tone.md", version=file.version, content="Keep it upbeat." +) + +# Delete +await client.memory.delete("preferences/tone.md") +``` + +`version` is an opaque token — never parse or compare it, just round-trip +whatever the API last gave you. + ## Examples Runnable scripts for each scenario live in [`examples/`](examples/): @@ -294,4 +326,4 @@ A local rehearsal to TestPyPI is available via `uv run invoke publish-test`. ## License -MIT +MIT \ No newline at end of file diff --git a/examples/09_memory.py b/examples/09_memory.py new file mode 100644 index 0000000..8cb3754 --- /dev/null +++ b/examples/09_memory.py @@ -0,0 +1,66 @@ +"""Create, list, read, update, and delete a memory file. + + python examples/09_memory.py + +Demonstrates the full memory resource lifecycle. ``update`` is partial: only +the fields you pass are changed, and ``version`` (an opaque token from the +previous read) guards against overwriting a concurrent change — a stale +``version`` raises ``ConflictError``. The file created here is always deleted +before the script exits, even on error. +""" + +from __future__ import annotations + +import asyncio +from uuid import uuid4 + +import _pretty as pretty +from _shared import make_client + +from cominty_sdk import ConflictError + + +async def main() -> None: + async with make_client() as client: + path = f"sdk-examples/{uuid4()}.md" + + # create() -> the new file, with its initial version token. + created = await client.memory.create( + path=path, + purpose="scratch note for the memory example", + content="Remember to buy milk.", + ) + pretty.console.print(f" [bold]create[/] path={created.path!r}") + + try: + # list() -> lightweight summaries (no content) for every file. + summaries = await client.memory.list() + pretty.console.print(f" [bold]list[/] {len(summaries)} file(s)") + + # get() -> the full file, including content. + fetched = await client.memory.get(path) + pretty.console.print(f" [bold]get[/] content={fetched.content!r}") + + # update() is partial: only content changes here, purpose is untouched. + # version must match the file's current version or this raises + # ConflictError (409) — the API's optimistic-concurrency guard. + updated = await client.memory.update( + path, version=fetched.version, content="Buy oat milk instead." + ) + pretty.console.print(f" [bold]update[/] content={updated.content!r}") + + # Reusing the now-stale version demonstrates the 409 guard. + try: + await client.memory.update( + path, version=fetched.version, content="stale write" + ) + except ConflictError: + pretty.console.print(" [bold]conflict[/] [yellow]stale version rejected[/]") + finally: + # Always clean up the file this example created. + await client.memory.delete(path) + pretty.console.print(" [bold]delete[/] [green]done[/]") + + +if __name__ == "__main__": + asyncio.run(main()) \ No newline at end of file diff --git a/examples/README.md b/examples/README.md index 9219da5..e2955b3 100644 --- a/examples/README.md +++ b/examples/README.md @@ -34,5 +34,6 @@ python examples/01_stream_events.py | [`06_manage_thread.py`](06_manage_thread.py) | Get, rename/star, and archive a thread | | [`07_custom_agent.py`](07_custom_agent.py) | Call a custom managed agent (needs `COMINTY_CUSTOM_AGENT_ID`) | | [`08_mcp_linear.py`](08_mcp_linear.py) | Custom agent pulls live context from the Linear MCP server | +| [`09_memory.py`](09_memory.py) | Create, list, read, update, and delete a memory file | -> Shared client setup lives in [`_shared.py`](_shared.py). +> Shared client setup lives in [`_shared.py`](_shared.py). \ No newline at end of file diff --git a/src/cominty_sdk/__init__.py b/src/cominty_sdk/__init__.py index 313c369..ac3c328 100644 --- a/src/cominty_sdk/__init__.py +++ b/src/cominty_sdk/__init__.py @@ -38,6 +38,12 @@ ThreadSummary, UpdateThreadParams, ) +from .models.memory import ( + MemoryFileCreate, + MemoryFileOut, + MemoryFileSummaryOut, + MemoryFileUpdate, +) from .streaming import AssistantRun, StartedChat try: @@ -81,4 +87,8 @@ "Thread", "ThreadSummary", "UpdateThreadParams", + "MemoryFileCreate", + "MemoryFileOut", + "MemoryFileSummaryOut", + "MemoryFileUpdate", ] diff --git a/src/cominty_sdk/client.py b/src/cominty_sdk/client.py index 566e5dd..07329c1 100644 --- a/src/cominty_sdk/client.py +++ b/src/cominty_sdk/client.py @@ -10,6 +10,7 @@ from ._transport import AsyncTransport from .models.chat import validate_user_id from .resources.chat import ChatResource +from .resources.memory import MemoryResource from .resources.threads import ThreadsResource __all__ = ["AsyncCominty"] @@ -53,6 +54,7 @@ def __init__( self._transport = AsyncTransport(self._config) self.chat = ChatResource(self._transport, user_id=self._config.user_id) self.threads = ThreadsResource(self._transport, user_id=self._config.user_id) + self.memory = MemoryResource(self._transport, user_id=self._config.user_id) @property def user_id(self) -> str: @@ -76,4 +78,4 @@ async def __aexit__( await self.close() async def close(self) -> None: - await self._transport.aclose() + await self._transport.aclose() \ No newline at end of file diff --git a/src/cominty_sdk/models/__init__.py b/src/cominty_sdk/models/__init__.py index 805426a..273efb1 100644 --- a/src/cominty_sdk/models/__init__.py +++ b/src/cominty_sdk/models/__init__.py @@ -18,6 +18,7 @@ Thread, ThreadSummary, ) +from .memory import MemoryFileCreate, MemoryFileOut, MemoryFileSummaryOut, MemoryFileUpdate __all__ = [ "Agent", @@ -28,10 +29,14 @@ "Message", "MessageRole", "MessageStatus", + "MemoryFileCreate", + "MemoryFileOut", + "MemoryFileSummaryOut", + "MemoryFileUpdate", "Question", "ShareLink", "StartChatOptions", "StartChatParams", "Thread", "ThreadSummary", -] +] \ No newline at end of file diff --git a/src/cominty_sdk/models/memory.py b/src/cominty_sdk/models/memory.py new file mode 100644 index 0000000..1e7344b --- /dev/null +++ b/src/cominty_sdk/models/memory.py @@ -0,0 +1,71 @@ +from __future__ import annotations + +from datetime import datetime + +from pydantic import BaseModel, ConfigDict, model_validator + +from .chat import UserId + +__all__ = [ + "MemoryFileCreate", + "MemoryFileUpdate", + "MemoryFileOut", + "MemoryFileSummaryOut", +] + + +class MemoryFileCreate(BaseModel): + model_config = ConfigDict(strict=True, extra="forbid") + + path: str + purpose: str + content: str + user_id: UserId + """Unlike the other memory endpoints, ``POST /memory`` takes ``user_id`` in + the request body rather than as a query parameter — confirmed empirically, + the OpenAPI contract doesn't declare it as a parameter here at all.""" + + +class MemoryFileUpdate(BaseModel): + """Partial update body for ``PUT /memory/file``. + + Built by the resource from only the arguments the caller actually passed, + then dumped with ``exclude_unset=True`` — this is what lets an explicit + ``None`` (clear the field) round-trip differently from an omitted argument + (leave the field untouched), which plain ``exclude_none`` cannot do. + """ + + model_config = ConfigDict(strict=True, extra="forbid") + + content: str | None = None + purpose: str | None = None + + @model_validator(mode="after") + def _require_at_least_one_field(self) -> MemoryFileUpdate: + if not self.model_fields_set: + raise ValueError("at least one of `content` or `purpose` must be provided") + return self + + +class MemoryFileOut(BaseModel): + model_config = ConfigDict(extra="ignore") + + path: str + purpose: str + content: str + created_at: datetime + updated_at: datetime + version: str + """Opaque concurrency token (currently identical to ``updated_at``) — pass + it back unchanged to :meth:`~.resources.memory.MemoryResource.update`. + Never parse, compare, or otherwise interpret its contents.""" + + +class MemoryFileSummaryOut(BaseModel): + model_config = ConfigDict(extra="ignore") + + path: str + purpose: str + created_at: datetime + updated_at: datetime + version: str \ No newline at end of file diff --git a/src/cominty_sdk/resources/__init__.py b/src/cominty_sdk/resources/__init__.py index ab7f6b8..5a473ec 100644 --- a/src/cominty_sdk/resources/__init__.py +++ b/src/cominty_sdk/resources/__init__.py @@ -3,6 +3,7 @@ from __future__ import annotations from .chat import ChatResource +from .memory import MemoryResource from .threads import ThreadsResource -__all__ = ["ChatResource", "ThreadsResource"] +__all__ = ["ChatResource", "ThreadsResource", "MemoryResource"] \ No newline at end of file diff --git a/src/cominty_sdk/resources/memory.py b/src/cominty_sdk/resources/memory.py new file mode 100644 index 0000000..8fa2f02 --- /dev/null +++ b/src/cominty_sdk/resources/memory.py @@ -0,0 +1,116 @@ +"""The memory resource: list, create, read, update, and delete memory files.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Union + +from pydantic import ValidationError + +from ..exceptions import InvalidParams +from ..models.memory import ( + MemoryFileCreate, + MemoryFileOut, + MemoryFileSummaryOut, + MemoryFileUpdate, +) + +if TYPE_CHECKING: + from .._transport import AsyncTransport + +__all__ = ["MemoryResource"] + + +class _Unset: + """Sentinel default for :meth:`MemoryResource.update`'s optional fields. + + Lets the method tell "argument not passed" (leave untouched) apart from + "argument passed as ``None``" (clear the field) — a plain ``None`` default + can't make that distinction. + """ + + def __repr__(self) -> str: + return "UNSET" + + +_UNSET = _Unset() +_OptionalField = Union[str, None, _Unset] + + +class MemoryResource: + def __init__(self, transport: AsyncTransport, *, user_id: str) -> None: + self._transport = transport + self._user_id = user_id + + async def list(self) -> list[MemoryFileSummaryOut]: + """List the current user's memory files (``GET /memory``).""" + raw = await self._transport.request( + "GET", "/memory", params={"user_id": self._user_id} + ) + return [MemoryFileSummaryOut.model_validate(item) for item in raw] + + async def create(self, *, path: str, purpose: str, content: str) -> MemoryFileOut: + """Create a memory file (``POST /memory``, 201 Created). + + Unlike every other memory endpoint, ``user_id`` is injected into the + request body here rather than sent as a query param. + """ + try: + params = MemoryFileCreate( + path=path, purpose=purpose, content=content, user_id=self._user_id + ) + except ValidationError as exc: + raise InvalidParams.from_validation_error(exc, context="memory.create") from None + raw = await self._transport.request( + "POST", "/memory", json_body=params.model_dump(mode="json") + ) + return MemoryFileOut.model_validate(raw) + + async def get(self, path: str) -> MemoryFileOut: + """Fetch a single memory file (``GET /memory/file``).""" + raw = await self._transport.request( + "GET", "/memory/file", params={"path": path, "user_id": self._user_id} + ) + return MemoryFileOut.model_validate(raw) + + async def update( + self, + path: str, + *, + version: str, + content: _OptionalField = _UNSET, + purpose: _OptionalField = _UNSET, + ) -> MemoryFileOut: + """Update a memory file's content and/or purpose (``PUT /memory/file``). + + ``version`` is the opaque token from a previously fetched + :class:`~.models.memory.MemoryFileOut` — round-tripped unchanged as a + query param. Raises :class:`~.exceptions.ConflictError` (409) if it no + longer matches the file's current version. + + Only the fields you pass are sent: an omitted ``content``/``purpose`` + leaves that field untouched server-side, while an explicit ``None`` + clears it — the two are not equivalent. Omitting both raises + :class:`~.exceptions.InvalidParams` before any request is sent, since + that call would be a no-op. + """ + fields: dict[str, object] = {} + if not isinstance(content, _Unset): + fields["content"] = content + if not isinstance(purpose, _Unset): + fields["purpose"] = purpose + try: + body_model = MemoryFileUpdate.model_validate(fields) + except ValidationError as exc: + raise InvalidParams.from_validation_error(exc, context="memory.update") from None + body = body_model.model_dump(mode="json", exclude_unset=True) + params = {"path": path, "version": version, "user_id": self._user_id} + raw = await self._transport.request( + "PUT", "/memory/file", params=params, json_body=body + ) + return MemoryFileOut.model_validate(raw) + + async def delete(self, path: str) -> None: + """Delete a memory file (``DELETE /memory/file``, 204 No Content).""" + await self._transport.request( + "DELETE", "/memory/file", params={"path": path, "user_id": self._user_id} + ) \ No newline at end of file diff --git a/tests/integration/test_smoke.py b/tests/integration/test_smoke.py index b9f4b52..7b27e73 100644 --- a/tests/integration/test_smoke.py +++ b/tests/integration/test_smoke.py @@ -9,10 +9,11 @@ from __future__ import annotations import os +from uuid import uuid4 import pytest -from cominty_sdk import AsyncCominty +from cominty_sdk import AsyncCominty, ConflictError, NotFoundError pytestmark = pytest.mark.integration @@ -52,3 +53,37 @@ async def test_start_and_get_reply(creds: tuple[str, str], agent_id: str) -> Non reply = await run.result() assert reply.content assert str(reply.thread_id) == str(run.thread.id) + + +@pytest.mark.asyncio +async def test_memory_lifecycle(creds: tuple[str, str]) -> None: + api_key, user_id = creds + async with AsyncCominty(api_token=api_key, user_id=user_id) as client: + path = f"sdk-integration-tests/{uuid4()}.md" + created = await client.memory.create( + path=path, purpose="integration test", content="buy milk" + ) + try: + assert created.path == path + assert created.content == "buy milk" + + summaries = await client.memory.list() + assert any(f.path == path for f in summaries) + + fetched = await client.memory.get(path) + assert fetched.content == "buy milk" + + updated = await client.memory.update( + path, version=fetched.version, content="buy oat milk" + ) + assert updated.content == "buy oat milk" + + with pytest.raises(ConflictError): + await client.memory.update( + path, version=fetched.version, content="stale write" + ) + finally: + await client.memory.delete(path) + + with pytest.raises(NotFoundError): + await client.memory.get(path) diff --git a/tests/unit/test_memory.py b/tests/unit/test_memory.py new file mode 100644 index 0000000..0d7c445 --- /dev/null +++ b/tests/unit/test_memory.py @@ -0,0 +1,250 @@ +"""Unit tests for the memory resource: list, create, get, update, delete. + +user_id is sourced from the client (set once at construction). Every memory +endpoint takes it as a query param except POST /memory, which takes it in the +request body instead — confirmed against the validated OpenAPI contract. +""" + +from __future__ import annotations + +import json + +import httpx +import pytest +import respx + +from cominty_sdk import ( + AsyncCominty, + ConflictError, + InvalidParams, + MemoryFileOut, + MemoryFileSummaryOut, +) + +USER_ID = "user_31HPTBuBvX20xlQNAbvxjOxPbKB" + + +def _file( + path: str = "notes/todo.md", + *, + purpose: str = "scratch notes", + content: str = "buy milk", + version: str = "v1", +) -> dict[str, object]: + return { + "path": path, + "purpose": purpose, + "content": content, + "created_at": "2026-06-28T10:00:00Z", + "updated_at": "2026-06-28T10:00:00Z", + "version": version, + } + + +def _summary( + path: str = "notes/todo.md", *, purpose: str = "scratch notes", version: str = "v1" +) -> dict[str, object]: + return { + "path": path, + "purpose": purpose, + "created_at": "2026-06-28T10:00:00Z", + "updated_at": "2026-06-28T10:00:00Z", + "version": version, + } + + +# --------------------------------------------------------------------------- # +# list +# --------------------------------------------------------------------------- # +async def test_list_scopes_to_client_user_id( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.get("/memory").mock( + return_value=httpx.Response(200, json=[_summary("a"), _summary("b")]) + ) + + files = await client.memory.list() + + assert [f.path for f in files] == ["a", "b"] + assert all(isinstance(f, MemoryFileSummaryOut) for f in files) + params = route.calls.last.request.url.params + assert params["user_id"] == USER_ID + assert route.calls.last.request.method == "GET" + + +# --------------------------------------------------------------------------- # +# create +# --------------------------------------------------------------------------- # +async def test_create_sends_user_id_in_body_not_query( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.post("/memory").mock( + return_value=httpx.Response(201, json=_file()) + ) + + result = await client.memory.create( + path="notes/todo.md", purpose="scratch notes", content="buy milk" + ) + + request = route.calls.last.request + assert request.method == "POST" + # user_id belongs in the body here — every other memory endpoint puts it + # in the query string instead. + assert "user_id" not in request.url.params + body = json.loads(request.content) + assert body == { + "path": "notes/todo.md", + "purpose": "scratch notes", + "content": "buy milk", + "user_id": USER_ID, + } + assert isinstance(result, MemoryFileOut) + assert result.path == "notes/todo.md" + + +async def test_create_conflict_raises_conflict_error( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + mock_api.post("/memory").mock( + return_value=httpx.Response( + 409, json={"detail": "A memory file already exists at 'notes/todo.md'."} + ) + ) + + with pytest.raises(ConflictError): + await client.memory.create( + path="notes/todo.md", purpose="scratch notes", content="buy milk" + ) + + +# --------------------------------------------------------------------------- # +# get +# --------------------------------------------------------------------------- # +async def test_get_sends_path_and_user_id_as_query( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.get("/memory/file").mock( + return_value=httpx.Response(200, json=_file()) + ) + + result = await client.memory.get("notes/todo.md") + + params = route.calls.last.request.url.params + assert params["path"] == "notes/todo.md" + assert params["user_id"] == USER_ID + assert isinstance(result, MemoryFileOut) + assert result.content == "buy milk" + + +# --------------------------------------------------------------------------- # +# update +# --------------------------------------------------------------------------- # +async def test_update_omitted_field_is_excluded_from_body( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.put("/memory/file").mock( + return_value=httpx.Response(200, json=_file(content="new content", version="v2")) + ) + + result = await client.memory.update("notes/todo.md", version="v1", content="new content") + + request = route.calls.last.request + # purpose was never passed -> excluded entirely, not sent as null. + assert json.loads(request.content) == {"content": "new content"} + params = request.url.params + assert params["path"] == "notes/todo.md" + assert params["version"] == "v1" + assert params["user_id"] == USER_ID + assert isinstance(result, MemoryFileOut) + assert result.version == "v2" + + +async def test_update_explicit_none_clears_field( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.put("/memory/file").mock(return_value=httpx.Response(200, json=_file())) + + await client.memory.update("notes/todo.md", version="v1", purpose=None) + + # An explicit None round-trips as a JSON null, distinct from being omitted. + assert json.loads(route.calls.last.request.content) == {"purpose": None} + + +async def test_update_no_fields_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.update("notes/todo.md", version="v1") + + # Rejected client-side before any request is sent — a no-op PUT would just + # waste a round trip and silently mask a caller bug. + assert mock_api.calls.call_count == 0 + + +async def test_update_version_round_trips_unchanged( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.put("/memory/file").mock(return_value=httpx.Response(200, json=_file())) + + opaque_version = "W/\"2026-06-28T10:00:00Z-xyz\"" + await client.memory.update("notes/todo.md", version=opaque_version, content="x") + + assert route.calls.last.request.url.params["version"] == opaque_version + + +async def test_update_conflict_raises_conflict_error( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + mock_api.put("/memory/file").mock( + return_value=httpx.Response(409, json={"detail": "version mismatch"}) + ) + + with pytest.raises(ConflictError): + await client.memory.update("notes/todo.md", version="stale", content="x") + + +# --------------------------------------------------------------------------- # +# delete +# --------------------------------------------------------------------------- # +async def test_delete_sends_query_and_returns_none( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + route = mock_api.delete("/memory/file").mock(return_value=httpx.Response(204)) + + result = await client.memory.delete("notes/todo.md") + + assert result is None + request = route.calls.last.request + assert request.method == "DELETE" + assert request.url.params["path"] == "notes/todo.md" + assert request.url.params["user_id"] == USER_ID + + +# --------------------------------------------------------------------------- # +# lifecycle +# --------------------------------------------------------------------------- # +async def test_full_lifecycle(client: AsyncCominty, mock_api: respx.MockRouter) -> None: + mock_api.post("/memory").mock(return_value=httpx.Response(201, json=_file(version="v1"))) + mock_api.get("/memory").mock(return_value=httpx.Response(200, json=[_summary()])) + mock_api.get("/memory/file").mock(return_value=httpx.Response(200, json=_file(version="v1"))) + mock_api.put("/memory/file").mock( + return_value=httpx.Response(200, json=_file(content="updated", version="v2")) + ) + mock_api.delete("/memory/file").mock(return_value=httpx.Response(204)) + + created = await client.memory.create( + path="notes/todo.md", purpose="scratch notes", content="buy milk" + ) + listed = await client.memory.list() + fetched = await client.memory.get(created.path) + updated = await client.memory.update( + fetched.path, version=fetched.version, content="updated" + ) + deleted = await client.memory.delete(updated.path) + + assert created.path == "notes/todo.md" + assert listed[0].path == "notes/todo.md" + assert fetched.version == "v1" + assert updated.content == "updated" + assert updated.version == "v2" + assert deleted is None \ No newline at end of file From 7499616f1cf6c60e36aa574eaef3d667eac89e31 Mon Sep 17 00:00:00 2001 From: ahmeda-cominty Date: Tue, 28 Jul 2026 14:17:16 +0200 Subject: [PATCH 2/5] docs: trim redundant docstring commentary in memory module --- src/cominty_sdk/models/memory.py | 14 +++++--------- src/cominty_sdk/resources/memory.py | 22 ++++++---------------- tests/unit/test_memory.py | 5 ++--- 3 files changed, 13 insertions(+), 28 deletions(-) diff --git a/src/cominty_sdk/models/memory.py b/src/cominty_sdk/models/memory.py index 1e7344b..ea40ac7 100644 --- a/src/cominty_sdk/models/memory.py +++ b/src/cominty_sdk/models/memory.py @@ -22,17 +22,14 @@ class MemoryFileCreate(BaseModel): content: str user_id: UserId """Unlike the other memory endpoints, ``POST /memory`` takes ``user_id`` in - the request body rather than as a query parameter — confirmed empirically, - the OpenAPI contract doesn't declare it as a parameter here at all.""" + the request body rather than as a query parameter.""" class MemoryFileUpdate(BaseModel): """Partial update body for ``PUT /memory/file``. - Built by the resource from only the arguments the caller actually passed, - then dumped with ``exclude_unset=True`` — this is what lets an explicit - ``None`` (clear the field) round-trip differently from an omitted argument - (leave the field untouched), which plain ``exclude_none`` cannot do. + Dumped with ``exclude_unset=True`` so an explicit ``None`` (clear the + field) round-trips differently from an omitted argument (leave untouched). """ model_config = ConfigDict(strict=True, extra="forbid") @@ -56,9 +53,8 @@ class MemoryFileOut(BaseModel): created_at: datetime updated_at: datetime version: str - """Opaque concurrency token (currently identical to ``updated_at``) — pass - it back unchanged to :meth:`~.resources.memory.MemoryResource.update`. - Never parse, compare, or otherwise interpret its contents.""" + """Opaque concurrency token — pass it back unchanged to + :meth:`~.resources.memory.MemoryResource.update`.""" class MemoryFileSummaryOut(BaseModel): diff --git a/src/cominty_sdk/resources/memory.py b/src/cominty_sdk/resources/memory.py index 8fa2f02..97b86b8 100644 --- a/src/cominty_sdk/resources/memory.py +++ b/src/cominty_sdk/resources/memory.py @@ -21,12 +21,8 @@ class _Unset: - """Sentinel default for :meth:`MemoryResource.update`'s optional fields. - - Lets the method tell "argument not passed" (leave untouched) apart from - "argument passed as ``None``" (clear the field) — a plain ``None`` default - can't make that distinction. - """ + """Sentinel default distinguishing "not passed" from "passed as ``None``" + for :meth:`MemoryResource.update`'s optional fields.""" def __repr__(self) -> str: return "UNSET" @@ -82,16 +78,10 @@ async def update( ) -> MemoryFileOut: """Update a memory file's content and/or purpose (``PUT /memory/file``). - ``version`` is the opaque token from a previously fetched - :class:`~.models.memory.MemoryFileOut` — round-tripped unchanged as a - query param. Raises :class:`~.exceptions.ConflictError` (409) if it no - longer matches the file's current version. - - Only the fields you pass are sent: an omitted ``content``/``purpose`` - leaves that field untouched server-side, while an explicit ``None`` - clears it — the two are not equivalent. Omitting both raises - :class:`~.exceptions.InvalidParams` before any request is sent, since - that call would be a no-op. + Partial: only the fields you pass are sent, and an explicit ``None`` + clears a field rather than leaving it untouched. ``version`` is the + opaque token from a previous read; a stale one raises + :class:`~.exceptions.ConflictError` (409). """ fields: dict[str, object] = {} if not isinstance(content, _Unset): diff --git a/tests/unit/test_memory.py b/tests/unit/test_memory.py index 0d7c445..c5c1314 100644 --- a/tests/unit/test_memory.py +++ b/tests/unit/test_memory.py @@ -2,7 +2,7 @@ user_id is sourced from the client (set once at construction). Every memory endpoint takes it as a query param except POST /memory, which takes it in the -request body instead — confirmed against the validated OpenAPI contract. +request body instead. """ from __future__ import annotations @@ -176,8 +176,7 @@ async def test_update_no_fields_raises_invalid_params( with pytest.raises(InvalidParams): await client.memory.update("notes/todo.md", version="v1") - # Rejected client-side before any request is sent — a no-op PUT would just - # waste a round trip and silently mask a caller bug. + # Rejected client-side before any request is sent. assert mock_api.calls.call_count == 0 From 67c42ebfb8142844145a5eda9a2e96364c9acc8d Mon Sep 17 00:00:00 2001 From: ahmeda-cominty Date: Fri, 31 Jul 2026 16:12:13 +0200 Subject: [PATCH 3/5] fix: reject null updates and over-deep memory paths locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both looked like they'd work but the live API silently no-ops on them (200, nothing actually changes) instead of erroring — now caught client-side with InvalidParams so callers don't get a false success. --- CHANGELOG.md | 16 ++++-- README.md | 12 +++++ src/cominty_sdk/models/memory.py | 59 +++++++++++++++++++-- src/cominty_sdk/resources/memory.py | 42 +++++++++++++-- tests/unit/test_memory.py | 79 ++++++++++++++++++++++++++--- 5 files changed, 188 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e15737..1f85d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `create()`, `get()`, `update()`, `delete()` (`GET/POST /memory`, `GET/PUT/DELETE /memory/file`). New models `MemoryFileCreate`, `MemoryFileUpdate`, `MemoryFileOut`, `MemoryFileSummaryOut`. `update()` is a - partial update — pass only the fields you want to change, distinguishing an - omitted field (left untouched) from an explicit `None` (cleared) — and - guards against concurrent writes via an opaque `version` token, raising - `ConflictError` (409) on a stale value. See `examples/09_memory.py`. + partial update — pass only the fields you want to change; the API does not + support clearing `content`/`purpose` once set (a `null` is silently ignored + server-side), so passing `content=None`/`purpose=None` raises + `InvalidParams` locally instead of sending a request that looks like it + succeeded but did nothing. `path` may have at most one folder segment + (`"folder/file.md"`, not `"a/b/file.md"`) — checked locally, also raising + `InvalidParams`, since the API only enforces this after a round trip. + `content` may be an empty string (no minimum length). Guards against + concurrent writes via an opaque `version` token, raising `ConflictError` + (409) on a stale value — a malformed `version` raises `APIError` (422) + instead. `delete()` is not idempotent: deleting an already-deleted path + raises `NotFoundError` (404). See `examples/09_memory.py`. ### Changed - `__version__` is now resolved at runtime from installed package metadata diff --git a/README.md b/README.md index 201cff1..eabeb98 100644 --- a/README.md +++ b/README.md @@ -208,6 +208,18 @@ await client.memory.delete("preferences/tone.md") `version` is an opaque token — never parse or compare it, just round-trip whatever the API last gave you. +There's currently no way to clear `content` or `purpose` once set — the API +ignores an explicit `null` (leaves the existing value untouched), so +`memory.update(..., content=None)` raises `InvalidParams` locally rather than +sending a request that looks like it succeeded but did nothing. + +A few other things worth knowing: +- `path` may have at most one folder segment — `"preferences/tone.md"` is + fine, `"a/b/tone.md"` isn't (raises `InvalidParams` locally). +- `content` may be an empty string; there's no minimum length. +- `memory.delete()` is not idempotent — deleting an already-deleted path + raises `NotFoundError`, not a repeated success. + ## Examples Runnable scripts for each scenario live in [`examples/`](examples/): diff --git a/src/cominty_sdk/models/memory.py b/src/cominty_sdk/models/memory.py index ea40ac7..007962e 100644 --- a/src/cominty_sdk/models/memory.py +++ b/src/cominty_sdk/models/memory.py @@ -1,8 +1,10 @@ from __future__ import annotations from datetime import datetime +from typing import Annotated -from pydantic import BaseModel, ConfigDict, model_validator +from pydantic import AfterValidator, BaseModel, ConfigDict, model_validator +from typing_extensions import TypeAlias from .chat import UserId @@ -11,13 +13,46 @@ "MemoryFileUpdate", "MemoryFileOut", "MemoryFileSummaryOut", + "MemoryPath", + "MemoryPathParam", + "validate_memory_path", ] +# Not in the OpenAPI spec — found by manually exercising the live API: a path +# with more than one folder segment (e.g. "a/b/file.md") is rejected with a +# 422 "Maximum folder depth is 1". Checked locally so a too-deep path fails +# before a request, not after a round trip. +_MAX_PATH_DEPTH = 1 + + +def validate_memory_path(value: str) -> str: + """Return ``value`` if it's within the API's folder-depth limit, else raise.""" + depth = value.count("/") + if depth > _MAX_PATH_DEPTH: + raise ValueError( + f"path {value!r} has {depth} folder levels; the API allows at most " + f"{_MAX_PATH_DEPTH} (e.g. 'folder/file.md' is fine, 'a/b/file.md' isn't)" + ) + return value + + +MemoryPath: TypeAlias = Annotated[str, AfterValidator(validate_memory_path)] +"""A memory file path, folder-depth-checked before any request is sent.""" + + +class MemoryPathParam(BaseModel): + """Validates a bare ``path`` argument (``get``/``update``/``delete``, + which don't otherwise go through a request-body model).""" + + model_config = ConfigDict(strict=True) + + path: MemoryPath + class MemoryFileCreate(BaseModel): model_config = ConfigDict(strict=True, extra="forbid") - path: str + path: MemoryPath purpose: str content: str user_id: UserId @@ -28,8 +63,12 @@ class MemoryFileCreate(BaseModel): class MemoryFileUpdate(BaseModel): """Partial update body for ``PUT /memory/file``. - Dumped with ``exclude_unset=True`` so an explicit ``None`` (clear the - field) round-trips differently from an omitted argument (leave untouched). + Dumped with ``exclude_unset=True`` so only explicitly-passed fields are + sent. The API does not currently support clearing ``content``/``purpose`` + once set — a ``null`` is silently ignored server-side (200, value + unchanged) rather than clearing the field. To avoid that confusing + silent-no-op, this model rejects an explicit ``None`` locally instead of + forwarding it. """ model_config = ConfigDict(strict=True, extra="forbid") @@ -43,6 +82,18 @@ def _require_at_least_one_field(self) -> MemoryFileUpdate: raise ValueError("at least one of `content` or `purpose` must be provided") return self + @model_validator(mode="after") + def _reject_explicit_none(self) -> MemoryFileUpdate: + nulled = sorted(name for name in self.model_fields_set if getattr(self, name) is None) + if nulled: + fields = " and ".join(nulled) + raise ValueError( + f"{fields} cannot be set to None: the API does not support " + "clearing a field once set (it's currently a silent no-op) — " + "omit the argument instead of passing None" + ) + return self + class MemoryFileOut(BaseModel): model_config = ConfigDict(extra="ignore") diff --git a/src/cominty_sdk/resources/memory.py b/src/cominty_sdk/resources/memory.py index 97b86b8..a997a4a 100644 --- a/src/cominty_sdk/resources/memory.py +++ b/src/cominty_sdk/resources/memory.py @@ -12,6 +12,7 @@ MemoryFileOut, MemoryFileSummaryOut, MemoryFileUpdate, + MemoryPathParam, ) if TYPE_CHECKING: @@ -37,6 +38,13 @@ def __init__(self, transport: AsyncTransport, *, user_id: str) -> None: self._transport = transport self._user_id = user_id + @staticmethod + def _validate_path(path: str, *, context: str) -> str: + try: + return MemoryPathParam(path=path).path + except ValidationError as exc: + raise InvalidParams.from_validation_error(exc, context=context) from None + async def list(self) -> list[MemoryFileSummaryOut]: """List the current user's memory files (``GET /memory``).""" raw = await self._transport.request( @@ -48,7 +56,13 @@ async def create(self, *, path: str, purpose: str, content: str) -> MemoryFileOu """Create a memory file (``POST /memory``, 201 Created). Unlike every other memory endpoint, ``user_id`` is injected into the - request body here rather than sent as a query param. + request body here rather than sent as a query param. ``path`` may have + at most one folder segment (``"folder/file.md"``, not + ``"a/b/file.md"``); a deeper path raises + :class:`~.exceptions.InvalidParams` locally. ``content`` may be an + empty string — the API doesn't enforce a minimum length. Creating at a + ``path`` that already exists raises + :class:`~.exceptions.ConflictError` (409). """ try: params = MemoryFileCreate( @@ -63,6 +77,7 @@ async def create(self, *, path: str, purpose: str, content: str) -> MemoryFileOu async def get(self, path: str) -> MemoryFileOut: """Fetch a single memory file (``GET /memory/file``).""" + path = self._validate_path(path, context="memory.get") raw = await self._transport.request( "GET", "/memory/file", params={"path": path, "user_id": self._user_id} ) @@ -78,11 +93,22 @@ async def update( ) -> MemoryFileOut: """Update a memory file's content and/or purpose (``PUT /memory/file``). - Partial: only the fields you pass are sent, and an explicit ``None`` - clears a field rather than leaving it untouched. ``version`` is the - opaque token from a previous read; a stale one raises + Partial: only the fields you pass are sent. ``version`` is the opaque + token from a previous read; a stale one raises :class:`~.exceptions.ConflictError` (409). + + The API does not currently support clearing ``content``/``purpose`` + once set — passing ``content=None`` or ``purpose=None`` raises + :class:`~.exceptions.InvalidParams` locally rather than silently + sending a ``null`` the server would ignore. Omit the argument to + leave a field untouched. + + ``version`` must be a real version token from a previous read, not an + arbitrary string — a well-formed but stale one raises + :class:`~.exceptions.ConflictError` (409), a malformed one raises + :class:`~.exceptions.APIError` (422). """ + path = self._validate_path(path, context="memory.update") fields: dict[str, object] = {} if not isinstance(content, _Unset): fields["content"] = content @@ -100,7 +126,13 @@ async def update( return MemoryFileOut.model_validate(raw) async def delete(self, path: str) -> None: - """Delete a memory file (``DELETE /memory/file``, 204 No Content).""" + """Delete a memory file (``DELETE /memory/file``, 204 No Content). + + Not idempotent: deleting an already-deleted (or never-existing) path + raises :class:`~.exceptions.NotFoundError` (404) rather than + succeeding again. + """ + path = self._validate_path(path, context="memory.delete") await self._transport.request( "DELETE", "/memory/file", params={"path": path, "user_id": self._user_id} ) \ No newline at end of file diff --git a/tests/unit/test_memory.py b/tests/unit/test_memory.py index c5c1314..74a38be 100644 --- a/tests/unit/test_memory.py +++ b/tests/unit/test_memory.py @@ -117,6 +117,62 @@ async def test_create_conflict_raises_conflict_error( ) +async def test_create_empty_content_is_allowed( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + # The API has no minimum-length constraint on content. + mock_api.post("/memory").mock(return_value=httpx.Response(201, json=_file(content=""))) + + result = await client.memory.create(path="notes/todo.md", purpose="scratch notes", content="") + + assert result.content == "" + + +# --------------------------------------------------------------------------- # +# path folder-depth limit (create/get/update/delete) +# --------------------------------------------------------------------------- # +# Not in the OpenAPI spec — the live API rejects more than one folder segment +# with a 422 ("Maximum folder depth is 1"). Checked locally in all 4 methods +# that take a path, so it fails before a request, not after a round trip. +TOO_DEEP_PATH = "a/b/c.md" + + +async def test_create_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.create(path=TOO_DEEP_PATH, purpose="x", content="y") + + assert mock_api.calls.call_count == 0 + + +async def test_get_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.get(TOO_DEEP_PATH) + + assert mock_api.calls.call_count == 0 + + +async def test_update_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.update(TOO_DEEP_PATH, version="v1", content="x") + + assert mock_api.calls.call_count == 0 + + +async def test_delete_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.delete(TOO_DEEP_PATH) + + assert mock_api.calls.call_count == 0 + + # --------------------------------------------------------------------------- # # get # --------------------------------------------------------------------------- # @@ -159,15 +215,24 @@ async def test_update_omitted_field_is_excluded_from_body( assert result.version == "v2" -async def test_update_explicit_none_clears_field( - client: AsyncCominty, mock_api: respx.MockRouter +@pytest.mark.parametrize( + "kwargs", + [ + {"purpose": None}, + {"content": None}, + {"content": "new content", "purpose": None}, + ], +) +async def test_update_explicit_none_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter, kwargs: dict[str, object] ) -> None: - route = mock_api.put("/memory/file").mock(return_value=httpx.Response(200, json=_file())) - - await client.memory.update("notes/todo.md", version="v1", purpose=None) + # The API silently ignores an explicit null (200, value unchanged) instead + # of clearing the field, so the SDK rejects it client-side rather than + # sending a request that looks like it succeeded but did nothing. + with pytest.raises(InvalidParams): + await client.memory.update("notes/todo.md", version="v1", **kwargs) - # An explicit None round-trips as a JSON null, distinct from being omitted. - assert json.loads(route.calls.last.request.content) == {"purpose": None} + assert mock_api.calls.call_count == 0 async def test_update_no_fields_raises_invalid_params( From c644aeafbe1eeda311bffd9ba025cd61d3b9c80c Mon Sep 17 00:00:00 2001 From: ahmeda-cominty Date: Mon, 3 Aug 2026 15:46:35 +0200 Subject: [PATCH 4/5] docs: reorganize memory tests and drop remaining redundant comments --- src/cominty_sdk/models/memory.py | 7 ++-- tests/unit/test_memory.py | 71 ++++++++++++++------------------ 2 files changed, 34 insertions(+), 44 deletions(-) diff --git a/src/cominty_sdk/models/memory.py b/src/cominty_sdk/models/memory.py index 007962e..1f90006 100644 --- a/src/cominty_sdk/models/memory.py +++ b/src/cominty_sdk/models/memory.py @@ -18,10 +18,9 @@ "validate_memory_path", ] -# Not in the OpenAPI spec — found by manually exercising the live API: a path -# with more than one folder segment (e.g. "a/b/file.md") is rejected with a -# 422 "Maximum folder depth is 1". Checked locally so a too-deep path fails -# before a request, not after a round trip. + +# A path with more than one folder segment (e.g. "a/b/file.md") is rejected with a +# 422 "Maximum folder depth is 1". _MAX_PATH_DEPTH = 1 diff --git a/tests/unit/test_memory.py b/tests/unit/test_memory.py index 74a38be..da351fc 100644 --- a/tests/unit/test_memory.py +++ b/tests/unit/test_memory.py @@ -23,6 +23,10 @@ USER_ID = "user_31HPTBuBvX20xlQNAbvxjOxPbKB" +# The live API rejects a path with more than one folder segment (422 "Maximum +# folder depth is 1"). +TOO_DEEP_PATH = "a/b/c.md" + def _file( path: str = "notes/todo.md", @@ -88,8 +92,6 @@ async def test_create_sends_user_id_in_body_not_query( request = route.calls.last.request assert request.method == "POST" - # user_id belongs in the body here — every other memory endpoint puts it - # in the query string instead. assert "user_id" not in request.url.params body = json.loads(request.content) assert body == { @@ -128,15 +130,6 @@ async def test_create_empty_content_is_allowed( assert result.content == "" -# --------------------------------------------------------------------------- # -# path folder-depth limit (create/get/update/delete) -# --------------------------------------------------------------------------- # -# Not in the OpenAPI spec — the live API rejects more than one folder segment -# with a 422 ("Maximum folder depth is 1"). Checked locally in all 4 methods -# that take a path, so it fails before a request, not after a round trip. -TOO_DEEP_PATH = "a/b/c.md" - - async def test_create_path_too_deep_raises_invalid_params( client: AsyncCominty, mock_api: respx.MockRouter ) -> None: @@ -146,33 +139,6 @@ async def test_create_path_too_deep_raises_invalid_params( assert mock_api.calls.call_count == 0 -async def test_get_path_too_deep_raises_invalid_params( - client: AsyncCominty, mock_api: respx.MockRouter -) -> None: - with pytest.raises(InvalidParams): - await client.memory.get(TOO_DEEP_PATH) - - assert mock_api.calls.call_count == 0 - - -async def test_update_path_too_deep_raises_invalid_params( - client: AsyncCominty, mock_api: respx.MockRouter -) -> None: - with pytest.raises(InvalidParams): - await client.memory.update(TOO_DEEP_PATH, version="v1", content="x") - - assert mock_api.calls.call_count == 0 - - -async def test_delete_path_too_deep_raises_invalid_params( - client: AsyncCominty, mock_api: respx.MockRouter -) -> None: - with pytest.raises(InvalidParams): - await client.memory.delete(TOO_DEEP_PATH) - - assert mock_api.calls.call_count == 0 - - # --------------------------------------------------------------------------- # # get # --------------------------------------------------------------------------- # @@ -192,6 +158,15 @@ async def test_get_sends_path_and_user_id_as_query( assert result.content == "buy milk" +async def test_get_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.get(TOO_DEEP_PATH) + + assert mock_api.calls.call_count == 0 + + # --------------------------------------------------------------------------- # # update # --------------------------------------------------------------------------- # @@ -205,7 +180,6 @@ async def test_update_omitted_field_is_excluded_from_body( result = await client.memory.update("notes/todo.md", version="v1", content="new content") request = route.calls.last.request - # purpose was never passed -> excluded entirely, not sent as null. assert json.loads(request.content) == {"content": "new content"} params = request.url.params assert params["path"] == "notes/todo.md" @@ -241,7 +215,6 @@ async def test_update_no_fields_raises_invalid_params( with pytest.raises(InvalidParams): await client.memory.update("notes/todo.md", version="v1") - # Rejected client-side before any request is sent. assert mock_api.calls.call_count == 0 @@ -267,6 +240,15 @@ async def test_update_conflict_raises_conflict_error( await client.memory.update("notes/todo.md", version="stale", content="x") +async def test_update_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.update(TOO_DEEP_PATH, version="v1", content="x") + + assert mock_api.calls.call_count == 0 + + # --------------------------------------------------------------------------- # # delete # --------------------------------------------------------------------------- # @@ -284,6 +266,15 @@ async def test_delete_sends_query_and_returns_none( assert request.url.params["user_id"] == USER_ID +async def test_delete_path_too_deep_raises_invalid_params( + client: AsyncCominty, mock_api: respx.MockRouter +) -> None: + with pytest.raises(InvalidParams): + await client.memory.delete(TOO_DEEP_PATH) + + assert mock_api.calls.call_count == 0 + + # --------------------------------------------------------------------------- # # lifecycle # --------------------------------------------------------------------------- # From 09b6ce98616c26bb370c0ed19fbfc729ebee733d Mon Sep 17 00:00:00 2001 From: ahmeda-cominty Date: Wed, 5 Aug 2026 09:44:53 +0200 Subject: [PATCH 5/5] feat: add task to bump, validate, commit, and tag releases --- AGENTS.md | 36 +++++++--- CHANGELOG.md | 5 ++ README.md | 26 +++++-- pyproject.toml | 2 + tasks.py | 153 ++++++++++++++++++++++++++++++++++++++-- tests/test_release.py | 159 ++++++++++++++++++++++++++++++++++++++++++ uv.lock | 2 + 7 files changed, 365 insertions(+), 18 deletions(-) create mode 100644 tests/test_release.py diff --git a/AGENTS.md b/AGENTS.md index bd27ec3..9527002 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -400,6 +400,7 @@ uv run invoke clean # remove dist/ build/ *.egg-info (no stale artifa uv run invoke build # clean, then `uv build` -> sdist + wheel in dist/ uv run invoke check # build, `twine check dist/*`, and print sdist + wheel contents uv run invoke publish-test # check, then upload to TestPyPI (rehearsal — never PyPI) +uv run invoke release --patch # bump + validate + commit + tag (see §12.3) — modifies Git state ``` `check` is the gate to run before cutting a release. It confirms: @@ -414,14 +415,33 @@ uv run invoke publish-test # check, then upload to TestPyPI (rehearsal — n ### 12.3 Cutting a release (steps) -1. **Bump the version** in `pyproject.toml`'s `[project] version` (the only place — see §12.1). -2. **Verify locally**: `uv run invoke check` (and `uv run invoke publish-test` for a dry run). -3. **Commit + tag**: `git commit -am "Release X.Y.Z"` then `git tag vX.Y.Z`; push both. - The tag `vX.Y.Z` must equal `pyproject.toml`'s version — the build derives the version from - that file, **not** the tag. -4. **Publish via a GitHub Release** — create/publish a Release for tag `vX.Y.Z` - (`gh release create vX.Y.Z` or the GitHub UI). Publishing the Release is what triggers - the `release` workflow; a plain tag push does not. +> **`uv run invoke release` modifies repository files and CREATES A GIT COMMIT AND TAG.** +> It never pushes and never creates the GitHub Release — that's step 2 below, always manual. + +1. **Bump, validate, commit, and tag** — pick exactly one mode: + + ```bash + uv run invoke release --patch # X.Y.Z -> X.Y.(Z+1) + uv run invoke release --minor # X.Y.Z -> X.(Y+1).0 + uv run invoke release --major # X.Y.Z -> (X+1).0.0 + uv run invoke release --version X.Y.Z # explicit target instead of incrementing + ``` + + The task (`tasks.py`): rejects zero or more than one of the four flags; requires a clean + working tree, a target version strictly greater than the current one, and no pre-existing + `vX.Y.Z` tag — all checked **before** touching any file. It then bumps `pyproject.toml`'s + `[project] version` (the only place — see §12.1), regenerates `uv.lock` via `uv lock`, and + runs the same lint/type-check/test/build gate as `uv run invoke check`. If any of that + fails, the file changes are rolled back and nothing is committed. On success it creates one + commit (`chore(release): version X.Y.Z`) and one **annotated** tag (`vX.Y.Z`), and prints + the exact next commands — it does not run them for you. +2. **Push, then publish via a GitHub Release** — `git push origin HEAD && git push origin + vX.Y.Z`, then create/publish a Release for that tag (`gh release create vX.Y.Z + --generate-notes` or the GitHub UI). Publishing the Release is what triggers the `release` + workflow; a plain tag push does not. + +`tests/test_release.py` covers the task itself (patch/minor/major/explicit-version success, +plus every validation failure) against disposable temp Git repos — never this repository. `uv run invoke publish` (direct upload to real PyPI) exists as a manual fallback only. The **preferred** path is the GitHub Release → CI flow below, so no PyPI token lives on a laptop. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f85d4f..8f93dd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (409) on a stale value — a malformed `version` raises `APIError` (422) instead. `delete()` is not idempotent: deleting an already-deleted path raises `NotFoundError` (404). See `examples/09_memory.py`. +- `uv run invoke release --patch|--minor|--major|--version X.Y.Z` — a dev-only + task that bumps `pyproject.toml`, regenerates `uv.lock`, runs the lint/ + type-check/test/build gate, then creates one release commit and one + annotated `vX.Y.Z` tag. Never pushes or creates the GitHub Release; prints + the exact next commands instead. See `AGENTS.md` §12.3. ### Changed - `__version__` is now resolved at runtime from installed package metadata diff --git a/README.md b/README.md index eabeb98..31aaeee 100644 --- a/README.md +++ b/README.md @@ -324,14 +324,28 @@ See [AGENTS.md](AGENTS.md) for coding conventions (typing, versioning, models). Publishing to PyPI uses **Trusted Publishing (OIDC)** — no tokens stored in GitHub — and is triggered by publishing a **GitHub Release** (`.github/workflows/release.yml`). The published version comes from -`pyproject.toml`, so the tag is cosmetic; keep them in sync. +`pyproject.toml`. + +**`uv run invoke release` modifies repository files and CREATES A GIT COMMIT +AND TAG.** It bumps `pyproject.toml`, regenerates `uv.lock`, runs the lint / +type-check / test / build gate, then commits and tags — it never pushes and +never creates the GitHub Release itself. ```bash -# 1. bump the version in pyproject.toml -# 2. commit on main and push -# 3. create the release — this tags and triggers the publish -gh release create v0.4.0 --title "v0.4.0" --generate-notes -# pre-release rehearsal: gh release create v0.4.0rc1 --prerelease --generate-notes +# 1. bump, validate, commit, and tag locally — pick exactly one +uv run invoke release --patch # X.Y.Z -> X.Y.(Z+1) +uv run invoke release --minor # X.Y.Z -> X.(Y+1).0 +uv run invoke release --major # X.Y.Z -> (X+1).0.0 +uv run invoke release --version X.Y.Z # set an explicit version + +# 2. push the commit and the tag it just created +git push origin HEAD +git push origin vX.Y.Z + +# 3. create the release — this triggers the publish workflow +gh release create vX.Y.Z --title vX.Y.Z --generate-notes +# pre-release rehearsal (skips `invoke release`): tag and push manually, +# then gh release create vX.Y.Zrc1 --prerelease --generate-notes ``` A local rehearsal to TestPyPI is available via `uv run invoke publish-test`. diff --git a/pyproject.toml b/pyproject.toml index 9b18562..bcf2c92 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -48,6 +48,7 @@ dev = [ "invoke>=2.2", "twine>=5", "rich>=13", # examples/ pretty terminal output (not a runtime dependency) + "packaging>=23", # tasks.py release: PEP 440 / SemVer version parsing ] [build-system] @@ -64,6 +65,7 @@ only-include = ["src/cominty_sdk", "README.md", "pyproject.toml"] asyncio_mode = "auto" asyncio_default_fixture_loop_scope = "function" testpaths = ["tests"] +pythonpath = ["."] # so tests/test_release.py can `import tasks` (repo-root script) markers = [ "integration: opt-in tests requiring COMINTY_API_KEY", ] diff --git a/tasks.py b/tasks.py index 9ffc1bf..0ff5e32 100644 --- a/tasks.py +++ b/tasks.py @@ -1,14 +1,19 @@ """Developer tasks. Run with `uv run invoke ` (e.g. `uv run invoke publish-test`). -List tasks: uv run invoke --list -Build + validate: uv run invoke check +List tasks: uv run invoke --list +Build + validate: uv run invoke check Dry-run to TestPyPI: uv run invoke publish-test -Real release: done via CI on GitHub Release (see .github/workflows/release.yml) +Bump + tag release: uv run invoke release --patch|--minor|--major|--version X.Y.Z +Real release: done via CI on GitHub Release (see .github/workflows/release.yml) """ from __future__ import annotations -from invoke import task +import re +from pathlib import Path + +from invoke import Exit, task +from packaging.version import InvalidVersion, Version TESTPYPI_URL = "https://test.pypi.org/legacy/" @@ -64,3 +69,143 @@ def publish(c, token=None): if token: cmd += f" --token {token}" c.run(cmd, echo=True) + + +# --------------------------------------------------------------------------- # +# release +# --------------------------------------------------------------------------- # +PYPROJECT_PATH = Path("pyproject.toml") +UV_LOCK_PATH = Path("uv.lock") + +_VERSION_LINE_RE = re.compile(r'(?m)^(version = ")([^"]+)(")') +_XYZ_RE = re.compile(r"\d+\.\d+\.\d+") + + +def _read_version(text: str) -> str: + match = _VERSION_LINE_RE.search(text) + if match is None: + raise Exit('could not find `version = "..."` in pyproject.toml') + current = match.group(2) + if not _XYZ_RE.fullmatch(current): + raise Exit( + f"pyproject.toml's version {current!r} isn't X.Y.Z — fix it manually first" + ) + return current + + +def _write_version(text: str, new_version: str) -> str: + return _VERSION_LINE_RE.sub(rf"\g<1>{new_version}\g<3>", text, count=1) + + +def _bump(current: str, part: str) -> str: + major, minor, patch = (int(x) for x in current.split(".")) + if part == "major": + return f"{major + 1}.0.0" + if part == "minor": + return f"{major}.{minor + 1}.0" + return f"{major}.{minor}.{patch + 1}" + + +def _validate_explicit_version(value: str) -> None: + if not _XYZ_RE.fullmatch(value): + raise Exit(f"--version must be X.Y.Z (three dot-separated integers), got {value!r}") + try: + Version(value) + except InvalidVersion as exc: + raise Exit(f"{value!r} is not a valid PEP 440 version: {exc}") from exc + + +def _git_is_dirty(c) -> bool: + result = c.run("git status --porcelain", hide=True, warn=True, in_stream=False) + return bool(result.stdout.strip()) + + +def _tag_exists(c, tag: str) -> bool: + result = c.run( + f"git rev-parse -q --verify refs/tags/{tag}", hide=True, warn=True, in_stream=False + ) + return result.ok + + +def _run_validation(c) -> None: + """The pre-commit gate: lint, type-check, tests, and a packaging dry run.""" + c.run("uv run ruff check .", echo=True, in_stream=False) + c.run("uv run pyright", echo=True, in_stream=False) + c.run("uv run pytest", echo=True, in_stream=False) + c.run("uv run invoke check", echo=True, in_stream=False) + + +@task( + help={ + "patch": "Bump the patch version: X.Y.Z -> X.Y.(Z+1)", + "minor": "Bump the minor version: X.Y.Z -> X.(Y+1).0", + "major": "Bump the major version: X.Y.Z -> (X+1).0.0", + "version": "Set an explicit target version X.Y.Z instead of incrementing", + } +) +def release(c, patch=False, minor=False, major=False, version=None): + """Bump the version and cut a release commit + tag. + + WARNING: this modifies repository files and CREATES A GIT COMMIT AND TAG. + It never pushes and never creates a GitHub Release — run the commands it + prints at the end to do that yourself. + + Exactly one of --patch/--minor/--major/--version is required. + """ + modes = {"patch": patch, "minor": minor, "major": major, "version": version is not None} + selected = [name for name, on in modes.items() if on] + if len(selected) != 1: + raise Exit( + "exactly one of --patch/--minor/--major/--version is required " + f"(got {selected or 'none'})" + ) + + if not PYPROJECT_PATH.exists(): + raise Exit("pyproject.toml not found — run this from the repo root") + + original_pyproject = PYPROJECT_PATH.read_text() + current = _read_version(original_pyproject) + + if version is not None: + _validate_explicit_version(version) + target = version + else: + target = _bump(current, selected[0]) + + if Version(target) <= Version(current): + raise Exit(f"target version {target} is not greater than the current version {current}") + + tag = f"v{target}" + + if _git_is_dirty(c): + raise Exit("git working tree is dirty — commit or stash changes before releasing") + if _tag_exists(c, tag): + raise Exit(f"tag {tag} already exists") + + original_uv_lock = UV_LOCK_PATH.read_text() if UV_LOCK_PATH.exists() else None + + def _rollback() -> None: + PYPROJECT_PATH.write_text(original_pyproject) + if original_uv_lock is not None: + UV_LOCK_PATH.write_text(original_uv_lock) + elif UV_LOCK_PATH.exists(): + UV_LOCK_PATH.unlink() + + PYPROJECT_PATH.write_text(_write_version(original_pyproject, target)) + try: + c.run("uv lock", echo=True, in_stream=False) + _run_validation(c) + except Exception: + _rollback() + raise + + c.run(f"git add {PYPROJECT_PATH} {UV_LOCK_PATH}", echo=True, in_stream=False) + c.run(f'git commit -m "chore(release): version {target}"', echo=True, in_stream=False) + c.run(f'git tag -a {tag} -m "{tag}"', echo=True, in_stream=False) + + print( + f"\nCreated commit and tag {tag} locally. Nothing was pushed. Next steps:\n\n" + f" git push origin HEAD\n" + f" git push origin {tag}\n" + f" gh release create {tag} --title {tag} --generate-notes\n" + ) diff --git a/tests/test_release.py b/tests/test_release.py new file mode 100644 index 0000000..3b9e436 --- /dev/null +++ b/tests/test_release.py @@ -0,0 +1,159 @@ +"""Tests for the `release` invoke task in tasks.py. + +Everything runs inside a disposable temp Git repo (see the `repo` fixture) — +never against this actual repository. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest +import tasks +from invoke import Context, Exit + +_MINIMAL_PYPROJECT = """\ +[project] +name = "scratch-pkg" +version = "{version}" +requires-python = ">=3.9" +dependencies = [] + +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" +""" + + +def _git(*args: str) -> None: + subprocess.run(["git", *args], check=True, capture_output=True) + + +def _git_out(*args: str) -> str: + result = subprocess.run(["git", *args], check=True, capture_output=True, text=True) + return result.stdout.strip() + + +def _is_clean() -> bool: + return _git_out("status", "--porcelain") == "" + + +def _pyproject_version() -> str: + return tasks._read_version(Path("pyproject.toml").read_text()) + + +@pytest.fixture +def repo(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.chdir(tmp_path) + _git("init", "-q") + _git("config", "user.email", "test@example.com") + _git("config", "user.name", "Test") + (tmp_path / "pyproject.toml").write_text(_MINIMAL_PYPROJECT.format(version="0.1.0")) + subprocess.run(["uv", "lock"], check=True, capture_output=True) + _git("add", "-A") + _git("commit", "-q", "-m", "initial") + # The SDK's own lint/type-check/test/build gate has nothing to check + # against a scratch project — stub it out so tests only exercise the + # version/Git logic. + monkeypatch.setattr(tasks, "_run_validation", lambda c: None) + return tmp_path + + +@pytest.fixture +def c() -> Context: + return Context() + + +# --------------------------------------------------------------------------- # +# success +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize( + "kwargs, expected", + [ + ({"patch": True}, "0.1.1"), + ({"minor": True}, "0.2.0"), + ({"major": True}, "1.0.0"), + ({"version": "5.2.1"}, "5.2.1"), + ], +) +def test_release_success( + repo: Path, c: Context, capsys: pytest.CaptureFixture[str], kwargs: dict, expected: str +) -> None: + tasks.release(c, **kwargs) + + assert _pyproject_version() == expected + assert f'version = "{expected}"' in (repo / "uv.lock").read_text() + assert _git_out("log", "-1", "--format=%s") == f"chore(release): version {expected}" + assert _git_out("cat-file", "-t", f"v{expected}") == "tag" # annotated, not lightweight + assert _is_clean() + + out = capsys.readouterr().out + assert f"v{expected}" in out + assert "git push" in out + assert "gh release create" in out + + +# --------------------------------------------------------------------------- # +# failures — each must leave the repo untouched +# --------------------------------------------------------------------------- # +def test_release_no_mode_fails(repo: Path, c: Context) -> None: + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c) + assert _git_out("rev-parse", "HEAD") == before + assert _is_clean() + assert _pyproject_version() == "0.1.0" + + +@pytest.mark.parametrize( + "kwargs", + [ + {"patch": True, "minor": True}, + {"patch": True, "version": "9.9.9"}, + ], + ids=["two-flags", "flag-and-version"], +) +def test_release_multiple_modes_fails(repo: Path, c: Context, kwargs: dict) -> None: + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c, **kwargs) + assert _git_out("rev-parse", "HEAD") == before + assert _is_clean() + + +@pytest.mark.parametrize("bad", ["not-a-version", "1.2", "1.2.3.4", "v1.2.3"]) +def test_release_invalid_version_format_fails(repo: Path, c: Context, bad: str) -> None: + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c, version=bad) + assert _git_out("rev-parse", "HEAD") == before + assert _is_clean() + assert _pyproject_version() == "0.1.0" + + +def test_release_dirty_tree_fails(repo: Path, c: Context) -> None: + (repo / "README.md").write_text("uncommitted change") + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c, patch=True) + assert _git_out("rev-parse", "HEAD") == before + assert _pyproject_version() == "0.1.0" + + +@pytest.mark.parametrize("target", ["0.1.0", "0.0.9"], ids=["equal", "lower"]) +def test_release_non_increasing_version_fails(repo: Path, c: Context, target: str) -> None: + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c, version=target) + assert _git_out("rev-parse", "HEAD") == before + assert _is_clean() + + +def test_release_existing_tag_fails(repo: Path, c: Context) -> None: + _git("tag", "v0.1.1") + before = _git_out("rev-parse", "HEAD") + with pytest.raises(Exit): + tasks.release(c, patch=True) + assert _git_out("rev-parse", "HEAD") == before + assert _pyproject_version() == "0.1.0" diff --git a/uv.lock b/uv.lock index f42e5b0..8fd3f73 100644 --- a/uv.lock +++ b/uv.lock @@ -511,6 +511,7 @@ dev = [ { name = "invoke" }, { name = "jupyter" }, { name = "nbformat" }, + { name = "packaging" }, { name = "pyright" }, { name = "pytest", version = "8.4.2", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10'" }, { name = "pytest", version = "9.0.3", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.10'" }, @@ -536,6 +537,7 @@ dev = [ { name = "invoke", specifier = ">=2.2" }, { name = "jupyter", specifier = ">=1.0" }, { name = "nbformat", specifier = ">=5" }, + { name = "packaging", specifier = ">=23" }, { name = "pyright", specifier = ">=1.1" }, { name = "pytest", specifier = ">=8" }, { name = "pytest-asyncio", specifier = ">=0.24" },