test(sdk): run the Python client against a real server

.github/CONTRIBUTING.md

@@ -97,6 +97,21 @@ async def test_something(app_client):     # AsyncClient on that state
 returns what it installed. Never install state from a test body that the next test
 depends on.
 
+### The Python SDK's live tests
+
+`sdk/python/tests/test_live_client.py` runs against a real server and skips when
+none is reachable, so the package still tests on a laptop. It exists because every
+other test in that package mocks the transport, and that is how `forget` on the
+async client shipped without the archiving PATCH it needs: the mock answered the
+DELETE and never modelled the protocol's `archived -> deleted` precondition.
+
+```bash
+AMP_TEST_URL=http://127.0.0.1:8765 pytest python/tests/test_live_client.py
+```
+
+CI sets `AMP_REQUIRE_LIVE_SERVER=1`, which turns the skip into a failure, so the
+job cannot pass without having exercised the client against the server it starts.
+
 ### Storage backends
 
 `tests/test_adapter_contract.py` runs the same behaviour tests against every

.github/workflows/ci.yml

@@ -89,9 +89,41 @@ jobs:
       - name: Install SDK package
         working-directory: sdk
         run: pip install -e ".[dev,langchain]"
-      - name: Run SDK tests
+
+      # The client tests that matter most are the ones against a real server:
+      # every mock-based test in this package passed while `forget` on the async
+      # client could not work at all.
+      - name: Install the reference server
+        if: matrix.python-version != '3.10'
+        working-directory: server
+        run: pip install -e .
+
+      - name: Start the reference server
+        if: matrix.python-version != '3.10'
+        working-directory: server
+        run: |
+          uvicorn amp_server.main:app --host 127.0.0.1 --port 8765 &
+          for i in $(seq 1 30); do
+            curl -sf http://127.0.0.1:8765/amp/v1/health && break
+            sleep 1
+          done
+          curl -sf http://127.0.0.1:8765/amp/v1/health
+
+      - name: Run SDK tests (unit)
+        working-directory: sdk
+        run: pytest -v python/tests --ignore=python/tests/test_live_client.py
+
+      # Not run on 3.10: the reference server's own floor is 3.11, so that leg has
+      # nothing to talk to. The condition is explicit rather than a skip, because a
+      # skip would read as a pass.
+      - name: Run the SDK against a real server
+        if: matrix.python-version != '3.10'
         working-directory: sdk
-        run: pytest -v python/tests
+        env:
+          # A skip here would report green while the client went unexercised - the
+          # same reason the storage job fails when its Postgres is missing.
+          AMP_REQUIRE_LIVE_SERVER: "1"
+        run: pytest -v python/tests/test_live_client.py
       - name: Security scan (pip-audit)
         working-directory: sdk
         continue-on-error: true

CHANGELOG.md

@@ -262,6 +262,18 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   conformance suite gained a vector so any implementation is held to the same
   shape (39 vectors now).
 
+- **The Python SDK is tested against a real server** (`sdk/python/tests/test_live_client.py`,
+  10 cases). Every other test in that package mocks the transport, which is how
+  `forget` on the async client shipped without the archiving PATCH the protocol
+  requires: the mock answered the DELETE and never modelled the
+  `archived -> deleted` precondition. The new file covers the round trips, the
+  access rule as seen from a client, the error codes as the SDK parses them
+  (including `VALIDATION_ERROR` from the previous fix), and both clients' `forget`.
+  It skips when no server is reachable so the package still tests on a laptop;
+  CI runs it with `AMP_REQUIRE_LIVE_SERVER=1`, which turns that skip into a
+  failure, and does not run it on the 3.10 leg because the reference server's own
+  floor is 3.11.
+
 ### Changed
 - **Every endpoint returns one error shape.** `PATCH /memories/{id}` answered a
   conflict with `{"detail": ...}` while `DELETE` answered with

sdk/python/tests/test_live_client.py

@@ -0,0 +1,204 @@
+"""The client against a real server, not a mock.
+
+Every other test in this package mocks the transport, and that is exactly why two
+bugs lived in this client:
+
+- the async `forget` never archived before deleting. The protocol only permits
+  `archived -> deleted`, so it was refused with 409 for every cell a caller would
+  actually want to forget. Its test mocked the DELETE and never modelled the
+  precondition, so it passed while the method could not work.
+- the contract test asserted the client echoed `created_at` on its archiving PATCH,
+  which was a workaround for a server-side requirement that no longer exists.
+
+A mock answers whatever the test tells it to. Only a real server can disagree with
+it, so these run against one when it is reachable and skip when it is not - the
+suite still runs on a laptop with nothing started. `AMP_REQUIRE_LIVE_SERVER=1`
+turns that skip into a failure, the way the storage job does with
+`AMP_REQUIRE_POSTGRES`, because a suite that skips itself reports green and the
+point of the CI job is to prove the client works against a real server.
+"""
+
+from __future__ import annotations
+
+import os
+import uuid
+
+import pytest
+
+from amp_client import AMPClient, AsyncAMPClient
+from amp_client.exceptions import AMPError
+
+DEFAULT_URL = "http://127.0.0.1:8765"
+REQUIRE_SERVER = os.environ.get("AMP_REQUIRE_LIVE_SERVER") not in (
+    None,
+    "",
+    "0",
+    "false",
+)
+
+
+def owner_id() -> str:
+    """A fresh owner per test, so a re-run does not read the previous one's data."""
+    return f"user-live-{uuid.uuid4().hex[:10]}"
+
+
+@pytest.fixture(scope="module")
+def server_url() -> str:
+    url = os.environ.get("AMP_TEST_URL", DEFAULT_URL)
+    if AMPClient(url, agent_id="live-probe").health():
+        return url
+    if REQUIRE_SERVER:
+        pytest.fail(
+            f"AMP_REQUIRE_LIVE_SERVER is set but no server answered at {url}: "
+            "the client would not have been exercised against a real one"
+        )
+    pytest.skip(f"no AMP server reachable at {url}; set AMP_TEST_URL to run these")
+
+
+def client(server_url: str, agent_id: str = "live-agent") -> AMPClient:
+    return AMPClient(server_url, agent_id=agent_id)
+
+
+def async_client(server_url: str, agent_id: str = "live-agent") -> AsyncAMPClient:
+    return AsyncAMPClient(server_url, agent_id=agent_id)
+
+
+# ---------------------------------------------------------------------------
+# Round trips
+# ---------------------------------------------------------------------------
+
+
+def test_remember_then_get_memory_returns_the_cell(server_url):
+    owner = owner_id()
+    stored = client(server_url).remember("User prefers email.", owner_id=owner)
+
+    fetched = client(server_url).get_memory(stored["id"])
+
+    assert fetched["content"]["text"] == "User prefers email."
+    assert fetched["identity"]["owner_id"] == owner
+
+
+def test_reading_bumps_the_access_count(server_url):
+    """GET is documented as resetting the decay clock, and it is the only way to."""
+    owner = owner_id()
+    stored = client(server_url).remember("Read me twice.", owner_id=owner)
+
+    first = client(server_url).get_memory(stored["id"])
+    second = client(server_url).get_memory(stored["id"])
+
+    assert second["scoring"]["access_count"] == first["scoring"]["access_count"] + 1
+
+
+def test_recall_finds_what_remember_stored(server_url):
+    owner = owner_id()
+    client(server_url).remember("The invoice number is INV-4471.", owner_id=owner)
+
+    hits = client(server_url).recall("invoice number", owner_id=owner)
+
+    assert any("INV-4471" in cell["content"]["text"] for cell in hits)
+
+
+def test_list_memories_pages_with_offset(server_url):
+    owner = owner_id()
+    api = client(server_url)
+    for index in range(3):
+        api.remember(f"paged memory {index}", owner_id=owner)
+
+    first = api.list_memories(owner_id=owner, limit=2)
+    second = api.list_memories(owner_id=owner, limit=2, offset=2)
+
+    assert len(first) == 2
+    assert {cell["id"] for cell in first} & {cell["id"] for cell in second} == set()
+
+
+# ---------------------------------------------------------------------------
+# The rules, seen from a client
+# ---------------------------------------------------------------------------
+
+
+def test_forget_archives_then_deletes_against_a_real_server(server_url):
+    """The order the protocol requires, proven where it is enforced.
+
+    This is the case a mock never modelled: DELETE is only permitted from
+    `archived`, so a client that skips the PATCH is refused with 409 for any cell
+    a caller would actually want to forget.
+    """
+    owner = owner_id()
+    api = client(server_url)
+    stored = api.remember("Delete me properly.", owner_id=owner)
+
+    assert api.forget(stored["id"]) is True
+
+    # Gone: a deleted cell answers 403 to a by-id read, per spec §8.4.
+    with pytest.raises(AMPError) as raised:
+        api.get_memory(stored["id"])
+    assert raised.value.status_code == 403
+
+    assert api.recall("Delete me properly", owner_id=owner) == []
+
+
+def test_an_agent_the_memory_was_not_shared_with_gets_nothing(server_url):
+    owner = owner_id()
+    client(server_url, "live-owner").remember(
+        "Only billing may read this.", owner_id=owner, readable_by=["live-billing*"]
+    )
+
+    allowed = client(server_url, "live-billing-v1").recall(
+        "only billing", owner_id=owner
+    )
+    refused = client(server_url, "live-marketing").recall(
+        "only billing", owner_id=owner
+    )
+
+    assert len(allowed) == 1
+    assert refused == []
+
+
+def test_an_unknown_cell_surfaces_the_protocol_error_code(server_url):
+    """The SDK parses the protocol envelope, so the code has to arrive intact.
+
+    Spec §8.4: the answer is 403 and not 404, whichever the truth is, so a caller
+    cannot probe for cells it may not read.
+    """
+    with pytest.raises(AMPError) as raised:
+        client(server_url).get_memory("mem_00000000000000000000000000")
+
+    assert raised.value.status_code == 403
+    assert "ACCESS_DENIED" in str(raised.value)
+
+
+def test_a_rejected_body_surfaces_the_validation_code(server_url):
+    """The one error shape holds for a bad request too, and the client reads it."""
+    with pytest.raises(AMPError) as raised:
+        client(server_url).recall("anything", owner_id="user-live", limit=100000)
+
+    assert raised.value.status_code == 422
+    assert "VALIDATION_ERROR" in str(raised.value)
+
+
+# ---------------------------------------------------------------------------
+# The async client, which is the one that was broken
+# ---------------------------------------------------------------------------
+
+
+async def test_async_forget_archives_then_deletes(server_url):
+    """The regression this file exists for: async `forget` used to skip the archive."""
+    owner = owner_id()
+    async with async_client(server_url) as api:
+        stored = await api.remember("Async delete me.", owner_id=owner)
+
+        assert await api.forget(stored["id"]) is True
+
+        with pytest.raises(AMPError):
+            await api.get_memory(stored["id"])
+
+
+async def test_async_round_trip(server_url):
+    owner = owner_id()
+    async with async_client(server_url) as api:
+        stored = await api.remember("Async memory.", owner_id=owner)
+        fetched = await api.get_memory(stored["id"])
+        hits = await api.recall("async memory", owner_id=owner)
+
+    assert fetched["id"] == stored["id"]
+    assert any(cell["id"] == stored["id"] for cell in hits)