From ff6e71de154401eef7e429dd8342a4d6c6342f60 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 19:37:42 +0100 Subject: [PATCH 1/7] refactor(server): rename ApiKeysPublic to ApiKeyList for list response envelopes Aligns the API key list response model with the conventional *List collection-envelope naming we'll also use for the knowledge list endpoint. Pure rename: wire format ({data, count}) is unchanged, so existing clients keep working. The frontend ApiKeysList interface is renamed in lockstep for consistency. --- server/backend/src/cq_server/api/routes/users.py | 4 ++-- server/backend/src/cq_server/models/users.py | 4 ++-- server/backend/src/cq_server/services/api_keys.py | 6 +++--- server/frontend/src/api.ts | 4 ++-- server/frontend/src/types.ts | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/server/backend/src/cq_server/api/routes/users.py b/server/backend/src/cq_server/api/routes/users.py index c42867e3..32a065e3 100644 --- a/server/backend/src/cq_server/api/routes/users.py +++ b/server/backend/src/cq_server/api/routes/users.py @@ -10,7 +10,7 @@ UserNotFoundError, ) from ...models.users import ( - ApiKeysPublic, + ApiKeyList, CreateApiKeyRequest, CreateApiKeyResponse, MeResponse, @@ -73,7 +73,7 @@ async def create_api_key_route( async def list_api_keys_route( username: CurrentUserDep, api_keys: APIKeyServiceDep, -) -> ApiKeysPublic: +) -> ApiKeyList: """Return the authenticated user's API keys. Never returns plaintext. Revoked keys are included with ``is_active: false`` so users can audit diff --git a/server/backend/src/cq_server/models/users.py b/server/backend/src/cq_server/models/users.py index d8b31a3b..bfd3fe0a 100644 --- a/server/backend/src/cq_server/models/users.py +++ b/server/backend/src/cq_server/models/users.py @@ -19,8 +19,8 @@ class ApiKeyPublic(BaseModel): is_active: bool -class ApiKeysPublic(BaseModel): - """Collection wrapper for API key listings. +class ApiKeyList(BaseModel): + """Collection envelope for API key listings. The envelope shape leaves room for pagination metadata (e.g. a ``next_cursor`` field) without breaking existing clients. diff --git a/server/backend/src/cq_server/services/api_keys.py b/server/backend/src/cq_server/services/api_keys.py index 8d6e081f..4ff2a8f2 100644 --- a/server/backend/src/cq_server/services/api_keys.py +++ b/server/backend/src/cq_server/services/api_keys.py @@ -19,8 +19,8 @@ UserNotFoundError, ) from ..models.users import ( + ApiKeyList, ApiKeyPublic, - ApiKeysPublic, CreateApiKeyResponse, Message, ) @@ -106,11 +106,11 @@ async def create( public = _to_public(row) return CreateApiKeyResponse(**public.model_dump(), token=plaintext) - async def list_for_user(self, username: str) -> ApiKeysPublic: + async def list_for_user(self, username: str) -> ApiKeyList: """Return every API key owned by ``username`` (including revoked rows).""" user_id = await self._require_user_id(username) data = [_to_public(row) for row in await self._api_keys.list_for_user(user_id)] - return ApiKeysPublic(data=data, count=len(data)) + return ApiKeyList(data=data, count=len(data)) async def revoke(self, *, username: str, key_id: str) -> Message: """Revoke ``key_id`` if it belongs to ``username``. Idempotent. diff --git a/server/frontend/src/api.ts b/server/frontend/src/api.ts index 2df1d26b..01812965 100644 --- a/server/frontend/src/api.ts +++ b/server/frontend/src/api.ts @@ -1,5 +1,5 @@ import type { - ApiKeysList, + ApiKeyList, CreatedApiKey, MessageResponse, ReviewDecisionResponse, @@ -113,7 +113,7 @@ export const api = { return request(`/review/units${query ? `?${query}` : ""}`) }, - listApiKeys: () => request("/users/me/api-keys"), + listApiKeys: () => request("/users/me/api-keys"), createApiKey: (name: string, ttl: string, labels: string[] = []) => request("/users/me/api-keys", { diff --git a/server/frontend/src/types.ts b/server/frontend/src/types.ts index d978ce7a..888e1a84 100644 --- a/server/frontend/src/types.ts +++ b/server/frontend/src/types.ts @@ -101,7 +101,7 @@ export interface CreatedApiKey extends ApiKeyPublic { token: string } -export interface ApiKeysList { +export interface ApiKeyList { data: ApiKeyPublic[] count: number } From a47105cfdf704d04a7b4159c418d59da3d41da38 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 19:50:34 +0100 Subject: [PATCH 2/7] refactor(server): switch list endpoints to {data} envelope, drop count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two list endpoints — GET /api/v1/knowledge and GET /api/v1/users/me/api-keys — now share one envelope shape: an object with a single "data" key holding the list. The previous shapes (bare JSON array on knowledge, {data, count} on api-keys) are gone. Rationale: an envelope leaves room to grow into pagination metadata (next_cursor, has_more, true total) without breaking clients. A bare array forecloses that. Dropping the "count" field is part of the same discipline — it equalled len(data) at every call site, so it carried no information, but committing to count == len(data) would have collided with the natural meaning of a future "count" field (total-matches-before-limit). Removing it now keeps the name free. Server changes: - Add KnowledgeUnitList model in models/knowledge.py. - Knowledge GET route returns KnowledgeUnitList; the response_model_exclude path for hiding "created_by" on each item is updated to {"data": {"__all__": {"created_by"}}}. - ApiKeyList drops its "count" field; APIKeyService.list_for_user stops setting it. - Frontend ApiKeyList interface drops count (unread by any consumer). Tests: - All knowledge-query assertions updated to .json()["data"]. - All api-keys list assertions stop checking count. - Two new contract tests pin the envelope shape: test_query_response_uses_envelope_shape (knowledge) test_list_response_uses_envelope_shape (api-keys) Both assert set(body.keys()) == {"data"} so an accidental reintroduction of count or any other top-level field fails fast. Wire-format break: the bundled Go and Python SDKs still decode the knowledge response as a bare array and will return no results against this server until they are updated in the following commits. --- .../src/cq_server/api/routes/knowledge.py | 9 +++-- .../backend/src/cq_server/models/knowledge.py | 12 +++++- server/backend/src/cq_server/models/users.py | 1 - .../src/cq_server/services/api_keys.py | 2 +- server/backend/tests/test_app.py | 38 +++++++++++-------- server/backend/tests/test_auth.py | 17 ++++++++- server/backend/tests/test_review.py | 4 +- server/backend/tests/test_sqlite_store_e2e.py | 2 +- server/frontend/src/types.ts | 1 - 9 files changed, 58 insertions(+), 28 deletions(-) diff --git a/server/backend/src/cq_server/api/routes/knowledge.py b/server/backend/src/cq_server/api/routes/knowledge.py index ab707c6c..e817d474 100644 --- a/server/backend/src/cq_server/api/routes/knowledge.py +++ b/server/backend/src/cq_server/api/routes/knowledge.py @@ -6,7 +6,7 @@ from fastapi import APIRouter, Query, status from ...exceptions import InvalidDomainError, KnowledgeUnitNotFoundError, ServiceError -from ...models.knowledge import FlagRequest, ProposeRequest, StatsResponse +from ...models.knowledge import FlagRequest, KnowledgeUnitList, ProposeRequest, StatsResponse from ..deps import APIKeyAuthDep, KnowledgeServiceDep router = APIRouter(prefix="/knowledge", tags=["knowledge"]) @@ -20,7 +20,7 @@ def knowledge_exception_mappings() -> dict[type[ServiceError], int]: } -@router.get("", response_model_exclude={"__all__": {"created_by"}}) +@router.get("", response_model_exclude={"data": {"__all__": {"created_by"}}}) async def query_units( domains: Annotated[list[str], Query()], knowledge: KnowledgeServiceDep, @@ -28,20 +28,21 @@ async def query_units( frameworks: Annotated[list[str] | None, Query()] = None, pattern: Annotated[str | None, Query()] = None, limit: Annotated[int, Query(gt=0)] = 5, -) -> list[KnowledgeUnit]: +) -> KnowledgeUnitList: """Search knowledge units by domain tags with relevance ranking. ``created_by`` is excluded from query responses to avoid leaking personal identifiers through the public read path until user-level attribution opt-in semantics are implemented. """ - return await knowledge.query( + units = await knowledge.query( domains=domains, languages=languages, frameworks=frameworks, pattern=pattern or "", limit=limit, ) + return KnowledgeUnitList(data=units) @router.post("", status_code=201) diff --git a/server/backend/src/cq_server/models/knowledge.py b/server/backend/src/cq_server/models/knowledge.py index 62503ef0..2ed2fcaf 100644 --- a/server/backend/src/cq_server/models/knowledge.py +++ b/server/backend/src/cq_server/models/knowledge.py @@ -1,6 +1,6 @@ """Pydantic schemas for /knowledge routes.""" -from cq.models import Context, FlagReason, Insight +from cq.models import Context, FlagReason, Insight, KnowledgeUnit from pydantic import BaseModel, Field @@ -10,6 +10,16 @@ class FlagRequest(BaseModel): reason: FlagReason +class KnowledgeUnitList(BaseModel): + """Collection envelope for knowledge unit listings. + + The envelope shape leaves room for pagination metadata (e.g. a + ``next_cursor`` field) without breaking existing clients. + """ + + data: list[KnowledgeUnit] + + class ProposeRequest(BaseModel): """Request body for proposing a new knowledge unit.""" diff --git a/server/backend/src/cq_server/models/users.py b/server/backend/src/cq_server/models/users.py index bfd3fe0a..ad7d511c 100644 --- a/server/backend/src/cq_server/models/users.py +++ b/server/backend/src/cq_server/models/users.py @@ -27,7 +27,6 @@ class ApiKeyList(BaseModel): """ data: list[ApiKeyPublic] - count: int class CreateApiKeyRequest(BaseModel): diff --git a/server/backend/src/cq_server/services/api_keys.py b/server/backend/src/cq_server/services/api_keys.py index 4ff2a8f2..f918a595 100644 --- a/server/backend/src/cq_server/services/api_keys.py +++ b/server/backend/src/cq_server/services/api_keys.py @@ -110,7 +110,7 @@ async def list_for_user(self, username: str) -> ApiKeyList: """Return every API key owned by ``username`` (including revoked rows).""" user_id = await self._require_user_id(username) data = [_to_public(row) for row in await self._api_keys.list_for_user(user_id)] - return ApiKeyList(data=data, count=len(data)) + return ApiKeyList(data=data) async def revoke(self, *, username: str, key_id: str) -> Message: """Revoke ``key_id`` if it belongs to ``username``. Idempotent. diff --git a/server/backend/tests/test_app.py b/server/backend/tests/test_app.py index 9b9404ce..9fae8366 100644 --- a/server/backend/tests/test_app.py +++ b/server/backend/tests/test_app.py @@ -137,7 +137,7 @@ def test_query_returns_matching_units(self, client: TestClient) -> None: self._insert_unit(client, domains=["databases"]) resp = client.get("/api/v1/knowledge", params={"domains": ["databases"]}) assert resp.status_code == 200 - results = resp.json() + results = resp.json()["data"] assert len(results) == 1 assert results[0]["domains"] == ["databases"] @@ -145,7 +145,15 @@ def test_query_returns_empty_for_no_match(self, client: TestClient) -> None: self._insert_unit(client, domains=["databases"]) resp = client.get("/api/v1/knowledge", params={"domains": ["networking"]}) assert resp.status_code == 200 - assert resp.json() == [] + assert resp.json() == {"data": []} + + def test_query_response_uses_envelope_shape(self, client: TestClient) -> None: + self._insert_unit(client, domains=["envelope-shape"]) + resp = client.get("/api/v1/knowledge", params={"domains": ["envelope-shape"]}) + assert resp.status_code == 200 + body = resp.json() + assert set(body.keys()) == {"data"} + assert isinstance(body["data"], list) def test_query_boosts_matching_language(self, client: TestClient) -> None: self._insert_unit( @@ -160,7 +168,7 @@ def test_query_boosts_matching_language(self, client: TestClient) -> None: ) resp = client.get("/api/v1/knowledge", params={"domains": ["web"], "languages": ["python"]}) assert resp.status_code == 200 - results = resp.json() + results = resp.json()["data"] assert len(results) == 2 assert "python" in results[0]["context"]["languages"] @@ -185,7 +193,7 @@ def test_query_boosts_any_matching_language(self, client: TestClient) -> None: params={"domains": ["web"], "languages": ["python", "go"]}, ) assert resp.status_code == 200 - results = resp.json() + results = resp.json()["data"] assert len(results) == 3 top_langs = {results[0]["context"]["languages"][0], results[1]["context"]["languages"][0]} assert top_langs == {"python", "go"} @@ -195,7 +203,7 @@ def test_query_respects_limit(self, client: TestClient) -> None: self._insert_unit(client, domains=["api"]) resp = client.get("/api/v1/knowledge", params={"domains": ["api"], "limit": 2}) assert resp.status_code == 200 - assert len(resp.json()) == 2 + assert len(resp.json()["data"]) == 2 def test_query_rejects_zero_limit(self, client: TestClient) -> None: resp = client.get("/api/v1/knowledge", params={"domains": ["api"], "limit": 0}) @@ -214,7 +222,7 @@ def test_query_boosts_matching_pattern(self, client: TestClient) -> None: ) resp = client.get("/api/v1/knowledge", params={"domains": ["api"], "pattern": "api-client"}) assert resp.status_code == 200 - results = resp.json() + results = resp.json()["data"] assert len(results) == 2 assert results[0]["context"]["pattern"] == "api-client" @@ -323,7 +331,7 @@ def test_full_review_lifecycle(self, client: TestClient) -> None: # KU is not queryable yet (pending). query_resp = client.get("/api/v1/knowledge", params={"domains": ["e2e-test"]}) - assert len(query_resp.json()) == 0 + assert len(query_resp.json()["data"]) == 0 # KU appears in review queue. queue_resp = client.get("/api/v1/review/queue", headers=headers) @@ -336,7 +344,7 @@ def test_full_review_lifecycle(self, client: TestClient) -> None: # KU is now queryable. query_resp = client.get("/api/v1/knowledge", params={"domains": ["e2e-test"]}) - assert len(query_resp.json()) == 1 + assert len(query_resp.json()["data"]) == 1 # Queue is empty. queue_resp = client.get("/api/v1/review/queue", headers=headers) @@ -374,22 +382,22 @@ def test_propose_confirm_flag_lifecycle(self, client: TestClient) -> None: "/api/v1/knowledge", params={"domains": ["api", "payments"], "languages": ["python"]}, ) - assert len(resp.json()) == 1 - assert resp.json()[0]["evidence"]["confidence"] == 0.5 + assert len(resp.json()["data"]) == 1 + assert resp.json()["data"][0]["evidence"]["confidence"] == 0.5 # Confirm boosts confidence. resp = client.post(f"/api/v1/knowledge/{unit_id}/confirmations") assert resp.status_code == 201 resp = client.get("/api/v1/knowledge", params={"domains": ["api", "payments"]}) - assert resp.json()[0]["evidence"]["confidence"] == pytest.approx(0.6) + assert resp.json()["data"][0]["evidence"]["confidence"] == pytest.approx(0.6) # Flag reduces confidence. resp = client.post(f"/api/v1/knowledge/{unit_id}/flags", json={"reason": "stale"}) assert resp.status_code == 201 resp = client.get("/api/v1/knowledge", params={"domains": ["api", "payments"]}) - result = resp.json()[0] + result = resp.json()["data"][0] assert result["evidence"]["confidence"] == pytest.approx(0.45) assert len(result["flags"]) == 1 @@ -527,9 +535,9 @@ def test_query_omits_created_by_from_response(self, enforced_client: TestClient) query = enforced_client.get("/api/v1/knowledge", params={"domains": ["privacy"]}) assert query.status_code == 200 - body = query.json() - assert len(body) == 1 - assert "created_by" not in body[0] + results = query.json()["data"] + assert len(results) == 1 + assert "created_by" not in results[0] def test_stats_stays_open_under_enforcement(self, enforced_client: TestClient) -> None: resp = enforced_client.get("/api/v1/knowledge/stats") diff --git a/server/backend/tests/test_auth.py b/server/backend/tests/test_auth.py index a1cd0c52..bf220b4d 100644 --- a/server/backend/tests/test_auth.py +++ b/server/backend/tests/test_auth.py @@ -217,11 +217,25 @@ def test_list_hides_plaintext(self, api_key_client: TestClient) -> None: resp = api_key_client.get("/api/v1/users/me/api-keys", headers={"Authorization": f"Bearer {token}"}) assert resp.status_code == 200 body = resp.json() - assert body["count"] == 1 assert len(body["data"]) == 1 assert "token" not in body["data"][0] assert body["data"][0]["name"] == "laptop" + def test_list_response_uses_envelope_shape(self, api_key_client: TestClient) -> None: + token = _login(api_key_client) + api_key_client.post( + "/api/v1/users/me/api-keys", + headers={"Authorization": f"Bearer {token}"}, + json={"name": "shape", "ttl": "30d"}, + ) + resp = api_key_client.get( + "/api/v1/users/me/api-keys", + headers={"Authorization": f"Bearer {token}"}, + ) + body = resp.json() + assert set(body.keys()) == {"data"} + assert isinstance(body["data"], list) + def test_list_requires_jwt(self, api_key_client: TestClient) -> None: resp = api_key_client.get("/api/v1/users/me/api-keys") assert resp.status_code == 401 @@ -241,7 +255,6 @@ def test_list_scoped_to_caller(self, api_key_client: TestClient) -> None: ) resp = api_key_client.get("/api/v1/users/me/api-keys", headers={"Authorization": f"Bearer {token_a}"}) body = resp.json() - assert body["count"] == 1 assert [k["name"] for k in body["data"]] == ["alice-key"] diff --git a/server/backend/tests/test_review.py b/server/backend/tests/test_review.py index 550de070..ae7da8d4 100644 --- a/server/backend/tests/test_review.py +++ b/server/backend/tests/test_review.py @@ -104,7 +104,7 @@ def test_approved_unit_appears_in_query(self, client: TestClient) -> None: unit = _propose(client, domains=["searchable"]) client.post(f"/api/v1/review/{unit['id']}/approve", headers=_auth_header(token)) resp = client.get("/api/v1/knowledge", params={"domains": ["searchable"]}) - assert len(resp.json()) == 1 + assert len(resp.json()["data"]) == 1 class TestReject: @@ -121,7 +121,7 @@ def test_rejected_unit_not_in_query(self, client: TestClient) -> None: unit = _propose(client, domains=["hidden"]) client.post(f"/api/v1/review/{unit['id']}/reject", headers=_auth_header(token)) resp = client.get("/api/v1/knowledge", params={"domains": ["hidden"]}) - assert len(resp.json()) == 0 + assert len(resp.json()["data"]) == 0 class TestListUnits: diff --git a/server/backend/tests/test_sqlite_store_e2e.py b/server/backend/tests/test_sqlite_store_e2e.py index 5391854d..d82305e6 100644 --- a/server/backend/tests/test_sqlite_store_e2e.py +++ b/server/backend/tests/test_sqlite_store_e2e.py @@ -42,7 +42,7 @@ def test_e2e_propose_via_store_query_via_api(tmp_path: Path, monkeypatch: pytest resp = client.get("/api/v1/knowledge", params={"domains": "smoke"}) assert resp.status_code == 200 - ids = [r["id"] for r in resp.json()] + ids = [r["id"] for r in resp.json()["data"]] assert unit.id in ids assert db.exists() diff --git a/server/frontend/src/types.ts b/server/frontend/src/types.ts index 888e1a84..6f942297 100644 --- a/server/frontend/src/types.ts +++ b/server/frontend/src/types.ts @@ -103,7 +103,6 @@ export interface CreatedApiKey extends ApiKeyPublic { export interface ApiKeyList { data: ApiKeyPublic[] - count: number } export interface MessageResponse { From 8290f78cd860ce9e2b7355752063ccd461d40271 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 21:37:47 +0100 Subject: [PATCH 3/7] fix(sdk-go): decode {data} envelope and surface remote failures as warnings The Go SDK decoded GET /api/v1/knowledge responses as a bare []KnowledgeUnit and swallowed decode failures with a silent return nil; against a server emitting the new {data: [...]} envelope the CLI showed "No matching knowledge units found." with no signal that the remote response had been rejected as malformed. The same shape of bug also masked transport errors and non-200 statuses. SDK changes: - sdk/go/remote.go: remoteClient.query now returns ([]KnowledgeUnit, error). The error is non-nil for URL build, transport, non-200 status, or decode failures; the unit slice is nil whenever err is non-nil. Transport and HTTP failures wrap errUnreachable. Decode uses an inline anonymous struct for the {data: [...]} envelope; there's one call site and the shape is right next to the code that uses it. - sdk/go/client.go: Client.Query captures the remote error and appends it to QueryResult.Warnings. Source stays SourceRemote so callers can tell the remote path was attempted; only the contributed units are missing. CLI changes: - cli/cmd/query.go: after a successful Query call, write each warning as "warning: ..." to cmd.ErrOrStderr(). Stderr is safe for the interactive CLI; the MCP server reads warnings off the QueryResult struct rather than from stdio and is unaffected. - cli/cmd/cmd_test.go: extract withFakeRemote(t, handler) helper that spins up an httptest.Server, points flagAddr at it, and registers cleanup. drain_test.go's three inline equivalents migrate to the helper. - cli/go.mod: uncomment the local-SDK replace directive so CLI tests build against the SDK changes in this branch. Pin back to a published SDK version after release. Tests: - sdk/go/remote_test.go: happy-path tests send the envelope; new TestRemoteQueryRejectsBareArrayResponse pins the regression for the original bug; TestRemoteQueryRejectsMalformedBody and TestRemoteQueryTransportError cover the other error paths; TestRemoteQueryServerError now asserts errUnreachable. - sdk/go/client_test.go: TestQuerySourceRemoteWhenRemoteFails now asserts QueryResult.Warnings is populated; TestQueryWarnsOnRemoteDecodeFailure is new and locks in the decode-error path. - cli/cmd/query_test.go: TestQueryPrintsRemoteWarningsToStderr spins up a fake remote returning a bare array and asserts stderr contains both "warning:" and "decoding" so a regression that suppresses just the decode-error path can't pass with some other unrelated warning. --- cli/cmd/cmd_test.go | 13 +++++++ cli/cmd/drain_test.go | 82 +++++++++++++++++++------------------------ cli/cmd/query.go | 4 +++ cli/cmd/query_test.go | 27 ++++++++++++++ cli/go.mod | 2 +- cli/go.sum | 2 -- sdk/go/client.go | 9 +++-- sdk/go/client_test.go | 24 +++++++++++-- sdk/go/remote.go | 27 +++++++------- sdk/go/remote_test.go | 68 ++++++++++++++++++++++++++++------- 10 files changed, 179 insertions(+), 79 deletions(-) diff --git a/cli/cmd/cmd_test.go b/cli/cmd/cmd_test.go index 80d1b0e7..39dad7df 100644 --- a/cli/cmd/cmd_test.go +++ b/cli/cmd/cmd_test.go @@ -1,6 +1,8 @@ package cmd import ( + "net/http" + "net/http/httptest" "path/filepath" "testing" ) @@ -29,3 +31,14 @@ func setFlag(t *testing.T, target *string, value string) { t.Cleanup(func() { *target = prev }) } + +// withFakeRemote starts a fake remote API on a test server, points the +// CLI's --addr flag at it, and registers cleanup. Callers that need an +// API key on the wire should setFlag(&flagAPIKey, ...) separately. +func withFakeRemote(t *testing.T, handler http.Handler) { + t.Helper() + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + setFlag(t, &flagAddr, srv.URL) +} diff --git a/cli/cmd/drain_test.go b/cli/cmd/drain_test.go index 5ed5d36c..75fa73bb 100644 --- a/cli/cmd/drain_test.go +++ b/cli/cmd/drain_test.go @@ -3,7 +3,6 @@ package cmd import ( "bytes" "net/http" - "net/http/httptest" "testing" "github.com/stretchr/testify/require" @@ -62,8 +61,19 @@ func TestDrainDryRunJSONFormat(t *testing.T) { func TestDrainPushesUnits(t *testing.T) { testSetup(t) + // Propose locally first (no remote configured). + propose := NewProposeCmd() + propose.SetArgs([]string{ + "--summary", "push-me", + "--detail", "d", + "--action", "a", + "--domain", "test", + }) + require.NoError(t, propose.Execute()) + + // Point at remote, then drain. var pushCount int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + withFakeRemote(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/v1/knowledge" && r.Method == "POST" { pushCount++ w.Header().Set("Content-Type", "application/json") @@ -75,20 +85,6 @@ func TestDrainPushesUnits(t *testing.T) { w.WriteHeader(http.StatusServiceUnavailable) })) - defer srv.Close() - - // Propose locally first (no remote configured). - propose := NewProposeCmd() - propose.SetArgs([]string{ - "--summary", "push-me", - "--detail", "d", - "--action", "a", - "--domain", "test", - }) - require.NoError(t, propose.Execute()) - - // Point at remote, then drain. - setFlag(t, &flagAddr, srv.URL) drain := NewDrainCmd() var buf bytes.Buffer @@ -101,19 +97,6 @@ func TestDrainPushesUnits(t *testing.T) { func TestDrainJSONFormat(t *testing.T) { testSetup(t) - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/v1/knowledge" && r.Method == "POST" { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":"ku_00000000000000000000000000000001","version":1,"domains":["test"],"insight":{"summary":"s","detail":"d","action":"a"},"context":{"languages":[],"frameworks":[],"pattern":""},"evidence":{"confidence":0.5,"confirmations":1},"tier":"local","flags":[]}`)) - - return - } - - w.WriteHeader(http.StatusServiceUnavailable) - })) - defer srv.Close() - // Propose locally first (no remote configured). propose := NewProposeCmd() propose.SetArgs([]string{ @@ -125,7 +108,17 @@ func TestDrainJSONFormat(t *testing.T) { require.NoError(t, propose.Execute()) // Point at remote, then drain. - setFlag(t, &flagAddr, srv.URL) + withFakeRemote(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/knowledge" && r.Method == "POST" { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"ku_00000000000000000000000000000001","version":1,"domains":["test"],"insight":{"summary":"s","detail":"d","action":"a"},"context":{"languages":[],"frameworks":[],"pattern":""},"evidence":{"confidence":0.5,"confirmations":1},"tier":"local","flags":[]}`)) + + return + } + + w.WriteHeader(http.StatusServiceUnavailable) + })) drain := NewDrainCmd() var buf bytes.Buffer @@ -161,21 +154,6 @@ func TestDrainAddrFromEnv(t *testing.T) { func TestDrainAddrFlagOverridesEnv(t *testing.T) { testSetup(t) - var pushCount int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/v1/knowledge" && r.Method == "POST" { - pushCount++ - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":"ku_00000000000000000000000000000001","version":1,"domains":["test"],"insight":{"summary":"s","detail":"d","action":"a"},"context":{"languages":[],"frameworks":[],"pattern":""},"evidence":{"confidence":0.5,"confirmations":1},"tier":"local","flags":[]}`)) - - return - } - - w.WriteHeader(http.StatusServiceUnavailable) - })) - defer srv.Close() - // Propose locally first (no remote configured). propose := NewProposeCmd() propose.SetArgs([]string{ @@ -188,7 +166,19 @@ func TestDrainAddrFlagOverridesEnv(t *testing.T) { // Env says one thing, but the flag (simulating --addr) says another. t.Setenv(envVarAddr, "http://env-addr:8742") - setFlag(t, &flagAddr, srv.URL) + var pushCount int + withFakeRemote(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/knowledge" && r.Method == "POST" { + pushCount++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"ku_00000000000000000000000000000001","version":1,"domains":["test"],"insight":{"summary":"s","detail":"d","action":"a"},"context":{"languages":[],"frameworks":[],"pattern":""},"evidence":{"confidence":0.5,"confirmations":1},"tier":"local","flags":[]}`)) + + return + } + + w.WriteHeader(http.StatusServiceUnavailable) + })) drain := NewDrainCmd() var buf bytes.Buffer diff --git a/cli/cmd/query.go b/cli/cmd/query.go index 9d3405af..69975cf6 100644 --- a/cli/cmd/query.go +++ b/cli/cmd/query.go @@ -50,6 +50,10 @@ func NewQueryCmd() *cobra.Command { return err } + for _, w := range qr.Warnings { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: %s\n", w) + } + if format == "json" { enc := json.NewEncoder(cmd.OutOrStdout()) enc.SetIndent("", jsonIndent) diff --git a/cli/cmd/query_test.go b/cli/cmd/query_test.go index bdcd080e..0f5142ba 100644 --- a/cli/cmd/query_test.go +++ b/cli/cmd/query_test.go @@ -3,6 +3,7 @@ package cmd import ( "bytes" "encoding/json" + "net/http" "testing" "github.com/stretchr/testify/require" @@ -127,6 +128,32 @@ func TestQueryUnsupportedFormat(t *testing.T) { require.Error(t, query.Execute()) } +func TestQueryPrintsRemoteWarningsToStderr(t *testing.T) { + testSetup(t) + + // Fake remote that returns a bare JSON array instead of the {data: [...]} + // envelope. The SDK should fail to decode and surface a warning, which + // the CLI must print to stderr. + withFakeRemote(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("[]")) + })) + + query := NewQueryCmd() + var out, errBuf bytes.Buffer + query.SetOut(&out) + query.SetErr(&errBuf) + query.SetArgs([]string{"--domain", "api"}) + require.NoError(t, query.Execute()) + + // The warning must be specifically about the decode failure; a generic + // "warning:" line could come from any future warning source and would + // mask a regression where the decode error stops surfacing. + stderr := errBuf.String() + require.Contains(t, stderr, "warning:") + require.Contains(t, stderr, "decoding") +} + func TestQueryPatternFlag(t *testing.T) { testSetup(t) diff --git a/cli/go.mod b/cli/go.mod index 47e49e3e..797d3311 100644 --- a/cli/go.mod +++ b/cli/go.mod @@ -36,4 +36,4 @@ require ( // Monorepo: the SDK is consumed locally. Re-pin to a published version // when the SDK is released alongside the CLI. -//replace github.com/mozilla-ai/cq/sdk/go => ../sdk/go +replace github.com/mozilla-ai/cq/sdk/go => ../sdk/go diff --git a/cli/go.sum b/cli/go.sum index 30b3396e..c704fe2d 100644 --- a/cli/go.sum +++ b/cli/go.sum @@ -31,8 +31,6 @@ github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWE github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mozilla-ai/cq/schema v0.0.1 h1:jPoZAYbU7/b3R3CEghmwuG0A3NrqczWsMkIWjXeFI94= github.com/mozilla-ai/cq/schema v0.0.1/go.mod h1:trKUR0o8hGYkQ26XAb3HFVHcQOct7lP/cS8beS5v2eE= -github.com/mozilla-ai/cq/sdk/go v0.7.0 h1:JJO2tjqEHSv0zrSQwUwYhjE1XJIS9s82J4nElsI4Glg= -github.com/mozilla-ai/cq/sdk/go v0.7.0/go.mod h1:0lsVnbeeJS76z9Q2AYMBndYPhuYDtZdovLAAvmZDA74= github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOFAw7w= github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= diff --git a/sdk/go/client.go b/sdk/go/client.go index 19316542..fd1dfd98 100644 --- a/sdk/go/client.go +++ b/sdk/go/client.go @@ -378,12 +378,17 @@ func (c *Client) Query(ctx context.Context, params QueryParams) (QueryResult, er normalised := params normalised.Limit = limit - remoteResults := c.remote.query(ctx, normalised) + remoteResults, remoteErr := c.remote.query(ctx, normalised) + + warnings := storeResult.Warnings + if remoteErr != nil { + warnings = append(warnings, remoteErr) + } return QueryResult{ Units: mergeResults(localResults, remoteResults, limit), Source: SourceRemote, - Warnings: storeResult.Warnings, + Warnings: warnings, }, nil } diff --git a/sdk/go/client_test.go b/sdk/go/client_test.go index 744a3688..d5397589 100644 --- a/sdk/go/client_test.go +++ b/sdk/go/client_test.go @@ -432,7 +432,9 @@ func TestQueryMergesLocalAndRemote(t *testing.T) { return } w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000003")}) + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": []map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000003")}, + }) })) ctx := context.Background() @@ -470,7 +472,9 @@ func TestQuerySourceRemoteWhenOnlyRemoteReturnsResults(t *testing.T) { c := newTestClientWithRemote(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000005")}) + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": []map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000005")}, + }) })) qr, err := c.Query(context.Background(), QueryParams{Domains: []string{"api"}}) @@ -489,6 +493,22 @@ func TestQuerySourceRemoteWhenRemoteFails(t *testing.T) { require.NoError(t, err) require.Empty(t, qr.Units) require.Equal(t, SourceRemote, qr.Source) + require.NotEmpty(t, qr.Warnings, "remote failure should surface as a warning") +} + +func TestQueryWarnsOnRemoteDecodeFailure(t *testing.T) { + + c := newTestClientWithRemote(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Bare array; the SDK now expects the {data: [...]} envelope. + _, _ = w.Write([]byte(`[{"id":"ku_00000000000000000000000000000099"}]`)) + })) + + qr, err := c.Query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.NoError(t, err) + require.Empty(t, qr.Units) + require.NotEmpty(t, qr.Warnings) + require.Contains(t, qr.Warnings[0].Error(), "decoding") } func TestConfirmLocalUnit(t *testing.T) { diff --git a/sdk/go/remote.go b/sdk/go/remote.go index ef9b0800..fcdb2a43 100644 --- a/sdk/go/remote.go +++ b/sdk/go/remote.go @@ -200,9 +200,9 @@ func (r *remoteClient) propose(ctx context.Context, ku KnowledgeUnit) (Knowledge return result, nil } -// query fetches knowledge units from the remote API. -// Returns nil on transport or HTTP errors for graceful degradation. -func (r *remoteClient) query(ctx context.Context, params QueryParams) []KnowledgeUnit { +// query fetches knowledge units from the remote API matching params. +// Returns errUnreachable on transport/non-200 HTTP failure. +func (r *remoteClient) query(ctx context.Context, params QueryParams) ([]KnowledgeUnit, error) { qv := url.Values{} for _, d := range params.Domains { qv.Add("domains", d) @@ -226,32 +226,33 @@ func (r *remoteClient) query(ctx context.Context, params QueryParams) []Knowledg base, err := r.url("/api/v1/knowledge") if err != nil { - return nil + return nil, fmt.Errorf("%w: %w", errUnreachable, err) } resp, err := r.do(ctx, http.MethodGet, base+"?"+qv.Encode(), nil) if err != nil { - return nil + return nil, fmt.Errorf("%w: %w", errUnreachable, err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return nil + return nil, fmt.Errorf("%w: HTTP %d", errUnreachable, resp.StatusCode) } - var units []KnowledgeUnit - if err := json.NewDecoder(resp.Body).Decode(&units); err != nil { - // Query degrades gracefully; log-worthy but not a hard error. - return nil + var env struct { + Data []KnowledgeUnit `json:"data"` + } + if err := json.NewDecoder(resp.Body).Decode(&env); err != nil { + return nil, fmt.Errorf("decoding remote knowledge response: %w", err) } - return units + return env.Data, nil } // remoteStatsResponse holds the server's /api/v1/knowledge/stats response. type remoteStatsResponse struct { - TotalUnits int `json:"total_units"` - Tiers map[Tier]int `json:"tiers"` + TotalUnits int `json:"total_units"` + Tiers map[Tier]int `json:"tiers"` Domains map[string]int `json:"domains"` } diff --git a/sdk/go/remote_test.go b/sdk/go/remote_test.go index 285af7d8..f5555d0b 100644 --- a/sdk/go/remote_test.go +++ b/sdk/go/remote_test.go @@ -20,12 +20,15 @@ func TestRemoteQuery(t *testing.T) { require.Equal(t, []string{"api", "testing"}, r.URL.Query()["domains"]) w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000002")}) + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": []map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000002")}, + }) })) defer srv.Close() rc := newRemoteClient(srv.URL, "", 5*time.Second) - units := rc.query(context.Background(), QueryParams{Domains: []string{"api", "testing"}}) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api", "testing"}}) + require.NoError(t, err) require.Len(t, units, 1) require.Equal(t, "ku_00000000000000000000000000000002", units[0].ID) require.Equal(t, "S", units[0].Insight.Summary) @@ -36,12 +39,13 @@ func TestRemoteQueryWithAuth(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { require.Equal(t, "Bearer test-token", r.Header.Get("Authorization")) w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{}) + _ = json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{}}) })) defer srv.Close() rc := newRemoteClient(srv.URL, "test-token", 5*time.Second) - units := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.NoError(t, err) require.Empty(t, units) } @@ -53,8 +57,46 @@ func TestRemoteQueryServerError(t *testing.T) { defer srv.Close() rc := newRemoteClient(srv.URL, "", 5*time.Second) - units := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.Nil(t, units) + require.ErrorIs(t, err, errUnreachable) +} + +func TestRemoteQueryRejectsBareArrayResponse(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]map[string]any{testRemoteKUJSON("ku_00000000000000000000000000000002")}) + })) + defer srv.Close() + + rc := newRemoteClient(srv.URL, "", 5*time.Second) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) require.Nil(t, units) + require.Error(t, err) + require.Contains(t, err.Error(), "decoding") +} + +func TestRemoteQueryRejectsMalformedBody(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("not json")) + })) + defer srv.Close() + + rc := newRemoteClient(srv.URL, "", 5*time.Second) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.Nil(t, units) + require.Error(t, err) +} + +func TestRemoteQueryTransportError(t *testing.T) { + t.Parallel() + rc := newRemoteClient("http://127.0.0.1:1", "", 1*time.Second) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.Nil(t, units) + require.ErrorIs(t, err, errUnreachable) } func TestRemotePropose(t *testing.T) { @@ -185,12 +227,12 @@ func TestRemoteQuerySendsPluralParamNames(t *testing.T) { require.Equal(t, []string{"grpc"}, r.URL.Query()["frameworks"]) w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{}) + _ = json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{}}) })) defer srv.Close() rc := newRemoteClient(srv.URL, "", 5*time.Second) - rc.query(context.Background(), QueryParams{ + _, _ = rc.query(context.Background(), QueryParams{ Domains: []string{"api"}, Languages: []string{"go"}, Frameworks: []string{"grpc"}, @@ -205,12 +247,12 @@ func TestRemoteQuerySendsPluralParamNames(t *testing.T) { require.Equal(t, []string{"grpc", "http"}, r.URL.Query()["frameworks"]) w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode([]map[string]any{}) + _ = json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{}}) })) defer srv.Close() rc := newRemoteClient(srv.URL, "", 5*time.Second) - rc.query(context.Background(), QueryParams{ + _, _ = rc.query(context.Background(), QueryParams{ Domains: []string{"api", "testing"}, Languages: []string{"python", "go"}, Frameworks: []string{"grpc", "http"}, @@ -271,12 +313,12 @@ func TestRemoteQueryAddsPatternToURL(t *testing.T) { capturedURL = r.URL.String() mu.Unlock() w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`[]`)) + _, _ = w.Write([]byte(`{"data":[]}`)) })) t.Cleanup(srv.Close) rc := newRemoteClient(srv.URL, "test-key", 5*time.Second) - rc.query(context.Background(), QueryParams{ + _, _ = rc.query(context.Background(), QueryParams{ Domains: []string{"api"}, Pattern: "api-client", }) @@ -298,12 +340,12 @@ func TestRemoteQueryOmitsEmptyPattern(t *testing.T) { capturedURL = r.URL.String() mu.Unlock() w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`[]`)) + _, _ = w.Write([]byte(`{"data":[]}`)) })) t.Cleanup(srv.Close) rc := newRemoteClient(srv.URL, "test-key", 5*time.Second) - rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + _, _ = rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) mu.Lock() defer mu.Unlock() From 38f083cc7963a4fa1d72ed7756cbd62bf68d29eb Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 22:02:01 +0100 Subject: [PATCH 4/7] fix(sdk-python): decode {data} envelope from /api/v1/knowledge The Python SDK's _remote_query iterated resp.json() directly as if it were a list of knowledge units. Against the new {data: [...]} envelope the iteration yields the dict's keys ("data") and KnowledgeUnit.model_validate("data") raises ValidationError; the surrounding try/except surfaces a "Remote query failed: ..." warning but the warning text doesn't tell the reader that the wire shape is wrong, only that validation failed somewhere. After this change the SDK validates the envelope shape explicitly and the warning names the actual problem. SDK changes (sdk/python/src/cq/client.py): - _remote_query inspects the parsed body before iterating. If the body isn't a dict, or doesn't have a "data" key, raise ValueError with a message that names the expected envelope shape. ValueError is already in the except tuple in Client.query, so the warning flow is unchanged; only the message becomes useful. - KnowledgeUnit.model_validate is now called on body["data"] rather than on the raw resp.json(); items inside the envelope go through the same per-item validation as before. Tests (sdk/python/tests/test_client.py): - Existing remote-query mocks (test_remote_query_merges_with_local, _sends_plural_*, _includes_pattern_*, _omits_pattern_*) updated to send {"data": [...]} envelopes. - New test_remote_query_warns_on_bare_array_response pins the regression for the original bug: a pre-envelope bare array response must surface as a warning, not silently degrade. - New test_remote_query_warns_on_missing_data_key covers the other obvious shape regression: a JSON object without a "data" key must also warn. --- sdk/python/src/cq/client.py | 5 ++++- sdk/python/tests/test_client.py | 35 +++++++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/sdk/python/src/cq/client.py b/sdk/python/src/cq/client.py index 70ef843d..2a43b37c 100644 --- a/sdk/python/src/cq/client.py +++ b/sdk/python/src/cq/client.py @@ -419,7 +419,10 @@ def _remote_query( params["pattern"] = pattern resp = self._http.get("/api/v1/knowledge", params=params) resp.raise_for_status() - return [KnowledgeUnit.model_validate(item) for item in resp.json()] + body = resp.json() + if not isinstance(body, dict) or "data" not in body: + raise ValueError("expected {data: [...]} envelope from /api/v1/knowledge") + return [KnowledgeUnit.model_validate(item) for item in body["data"]] def _remote_propose(self, unit: KnowledgeUnit) -> KnowledgeUnit: """Push a unit to the remote API. diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 8273986f..9a1ea3af 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -319,7 +319,7 @@ def test_remote_query_merges_with_local(self, tmp_path: Path, httpx_mock): } httpx_mock.add_response( url=httpx.URL("http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5"}), - json=[remote_unit], + json={"data": [remote_unit]}, ) # Insert a local unit directly (propose with remote skips local store). @@ -357,7 +357,7 @@ def test_remote_query_sends_plural_language_and_framework_params(self, tmp_path: "http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5", "languages": ["python"], "frameworks": ["django"]}, ), - json=[remote_unit], + json={"data": [remote_unit]}, ) c = Client(addr="http://test-remote", local_db_path=tmp_path / "test.db") @@ -380,7 +380,7 @@ def test_remote_query_includes_pattern_param_when_non_empty(self, tmp_path: Path "http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5", "pattern": "api-client"}, ), - json=[remote_unit], + json={"data": [remote_unit]}, ) c = Client(addr="http://test-remote", local_db_path=tmp_path / "test.db") @@ -400,7 +400,7 @@ def test_remote_query_omits_pattern_param_when_empty(self, tmp_path: Path, httpx } httpx_mock.add_response( url=httpx.URL("http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5"}), - json=[remote_unit], + json={"data": [remote_unit]}, ) c = Client(addr="http://test-remote", local_db_path=tmp_path / "test.db") @@ -409,6 +409,33 @@ def test_remote_query_omits_pattern_param_when_empty(self, tmp_path: Path, httpx assert len(result.units) == 1 c.close() + def test_remote_query_warns_on_bare_array_response(self, tmp_path: Path, httpx_mock): + """A server returning a bare JSON array (pre-envelope shape) must surface as a warning, + not silently degrade to empty results.""" + httpx_mock.add_response( + url=httpx.URL("http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5"}), + json=[{"id": "ku_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa99"}], + ) + + c = Client(addr="http://test-remote", local_db_path=tmp_path / "test.db") + result = c.query(["api"]) + assert result.units == [] + assert any("envelope" in w or "data" in w for w in result.warnings) + c.close() + + def test_remote_query_warns_on_missing_data_key(self, tmp_path: Path, httpx_mock): + """A server returning a JSON object without a ``data`` key must surface as a warning.""" + httpx_mock.add_response( + url=httpx.URL("http://test-remote/api/v1/knowledge", params={"domains": ["api"], "limit": "5"}), + json={"results": []}, + ) + + c = Client(addr="http://test-remote", local_db_path=tmp_path / "test.db") + result = c.query(["api"]) + assert result.units == [] + assert result.warnings, "missing data key should produce a warning" + c.close() + def test_propose_returns_server_response_when_remote_accepts(self, tmp_path: Path, httpx_mock): """When remote accepts, propose() returns the server-created unit.""" server_unit = { From a1965b3402e60c1b87c7b80c939a7ab0152832f2 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 22:06:31 +0100 Subject: [PATCH 5/7] docs: add HTTP API Conventions section to architecture.md Adds section 6 of architecture.md documenting the two list-response envelope shapes (FooList / FooPage), the rules they share (data is always the root key, no count/total/offset, snake_case wire fields, per-language class naming), and why cursor pagination is preferred over offset for mutable filtered streams. The "Today" sub-section lists the current List endpoints; no Page endpoints exist yet, so the first one to add cursor pagination has the contract written down up-front rather than inventing a third shape. --- docs/architecture.md | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/architecture.md b/docs/architecture.md index 044ebee7..0a8e0527 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -458,3 +458,51 @@ The ecosystem convergence on MCP and Agent Skills means cq does not need to conv Non-Claude-Code hosts (Cursor, Windsurf, OpenCode) are installed via a host-agnostic Python installer at `scripts/install/`. It is a stdlib-only uv-managed project whose CLI (`python -m cq_install install --target `) resolves a per-host target directory, writes the host-specific MCP config, and installs the shared skill commons to `~/.agents/skills/cq/` (or a project-scoped equivalent). Adding a new host is a single file under `scripts/install/src/cq_install/hosts/`: the primitive library in `common.py` (merge-not-replace JSON, hook entry, manifest-tracked file copies, markdown blocks) handles the shared mechanics. Claude Code remains on its own native marketplace via a thin wrapper host that shells out to `claude plugin marketplace`. > **Domain scope:** The initial implementation targets coding agents — the domain where agent tooling is most mature and adoption is fastest. The underlying mechanism (structured knowledge sharing via MCP with tiered trust) generalizes to arbitrary domains: DevOps, security, data engineering, and beyond. + +--- + +## 6. HTTP API Conventions + +Reference for endpoint authors and SDK implementers. The HTTP surface follows two response-shape conventions for list-returning endpoints and a small set of cross-cutting rules. + +### List vs Page + +Every list-returning endpoint emits one of two shapes. The model name suffix tells the caller which one to expect. + +**`FooList`** — unpaginated. The response is the whole set or a server-clamped top-N. Callers treat it as "what you got, full stop." + +```json +{ + "data": [Foo, Foo, ...] +} +``` + +**`FooPage`** — cursor-paginated. The response is a slice of an ordered stream, with an opaque token to fetch the next slice. + +```json +{ + "data": [Foo, Foo, ...], + "next_cursor": "opaque-token-or-null" +} +``` + +Callers paginate by passing `next_cursor` back as a query parameter on the next request. `next_cursor: null` (or omitted) means end of stream. The cursor is server-encoded and opaque to clients; clients must not parse or construct it. + +### Why cursor over offset + +Cursor (a.k.a. keyset) pagination anchors to a position in the sort order rather than a numeric offset. Concurrent inserts or deletes in the filtered set do not cause skips or duplicates between page fetches. This matters most for mutable filtered streams like review queues or recent-activity feeds; for stable browse sets either shape works, but `Page` is the more honest contract once a list is large enough that any client might want to paginate it. Offset pagination is not used. + +### Rules + +- **`data` is always the root key for the items**, in both `List` and `Page`. Consumers always start with the same first level of JSON regardless of which shape they hit. There is no third shape. +- **Suffix discipline.** A model name ending in `List` means unpaginated; ending in `Page` means cursor-paginated. The suffix is the contract; do not invent new ones. +- **No `count`, no `total`, no `offset` in either envelope.** A `count` field that equals `len(data)` carries no information and pre-commits the field name to a meaning that conflicts with a future "total-matches-before-limit" value. If a caller genuinely needs the total count of matches, expose it as a separate endpoint or a dedicated `/count` sub-resource rather than smuggling it into the list response. +- **JSON wire fields stay snake_case** in every language. Class and type names follow each language's native idiom; the wire shape does not. +- **Class naming across languages.** Python (Pydantic models) and TypeScript use `ApiKeyList`, `KnowledgeUnitList`, etc., with camelCase initialisms — this keeps wire-facing types symmetric across the two languages. Go uses `APIKeyList`, `KnowledgeUnitList` with all-caps initialisms per standard Go convention. All three names serialize to and from the same JSON. + +### Today + +- `GET /api/v1/knowledge` returns `KnowledgeUnitList`. +- `GET /api/v1/users/me/api-keys` returns `ApiKeyList`. + +No cursor-paginated endpoints exist yet; the first one to land will follow the `FooPage` shape above. From a294477440dd6c623acc9ff71f55684bce517868 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 22:24:52 +0100 Subject: [PATCH 6/7] fix(sdk-go): reject envelope responses with missing or null data field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A response like {"results": []} or {"data": null} previously decoded successfully into the local struct, leaving the slice nil, and the SDK returned no remote results with no warning — the same silent-failure shape this PR set out to eliminate, just at one layer deeper. The Python SDK already rejects this case explicitly; the Go SDK now does too. The decoder uses a pointer-to-slice for the `data` field so an absent key (env.Data == nil) is distinguishable from an empty array (env.Data != nil, *env.Data == []). When env.Data is nil after a successful decode, the function returns an error naming the missing/null data field. QueryResult.Warnings surfaces that error to the caller exactly like the existing decode-failure path. Two new regression tests pin the behavior: TestRemoteQueryRejectsMissingDataKey covers {"results": []}; TestRemoteQueryRejectsNullDataField covers {"data": null}. --- sdk/go/remote.go | 10 ++++++++-- sdk/go/remote_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/sdk/go/remote.go b/sdk/go/remote.go index fcdb2a43..4907fb8b 100644 --- a/sdk/go/remote.go +++ b/sdk/go/remote.go @@ -239,14 +239,20 @@ func (r *remoteClient) query(ctx context.Context, params QueryParams) ([]Knowled return nil, fmt.Errorf("%w: HTTP %d", errUnreachable, resp.StatusCode) } + // Pointer field distinguishes an absent or null "data" key (env.Data == nil) + // from an empty array (env.Data != nil, *env.Data == []). var env struct { - Data []KnowledgeUnit `json:"data"` + Data *[]KnowledgeUnit `json:"data"` } if err := json.NewDecoder(resp.Body).Decode(&env); err != nil { return nil, fmt.Errorf("decoding remote knowledge response: %w", err) } - return env.Data, nil + if env.Data == nil { + return nil, fmt.Errorf("decoding remote knowledge response: missing or null data field") + } + + return *env.Data, nil } // remoteStatsResponse holds the server's /api/v1/knowledge/stats response. diff --git a/sdk/go/remote_test.go b/sdk/go/remote_test.go index f5555d0b..41248e89 100644 --- a/sdk/go/remote_test.go +++ b/sdk/go/remote_test.go @@ -77,6 +77,36 @@ func TestRemoteQueryRejectsBareArrayResponse(t *testing.T) { require.Contains(t, err.Error(), "decoding") } +func TestRemoteQueryRejectsMissingDataKey(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"results": []}`)) + })) + defer srv.Close() + + rc := newRemoteClient(srv.URL, "", 5*time.Second) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.Nil(t, units) + require.Error(t, err) + require.Contains(t, err.Error(), "data") +} + +func TestRemoteQueryRejectsNullDataField(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data": null}`)) + })) + defer srv.Close() + + rc := newRemoteClient(srv.URL, "", 5*time.Second) + units, err := rc.query(context.Background(), QueryParams{Domains: []string{"api"}}) + require.Nil(t, units) + require.Error(t, err) + require.Contains(t, err.Error(), "data") +} + func TestRemoteQueryRejectsMalformedBody(t *testing.T) { t.Parallel() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { From dcf152fa30731143fade3949555394f7b271ed17 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 15 May 2026 22:24:59 +0100 Subject: [PATCH 7/7] docs(server): correct List model docstrings to match List/Page convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KnowledgeUnitList and ApiKeyList docstrings said the envelope "leaves room for pagination metadata (e.g. a next_cursor field)" — wording that survived from before the List/Page convention was settled. Per docs/architecture.md §6, List is the unpaginated shape and pagination metadata belongs on a separate Page model. Replaces the misleading rationale with a one-line NOTE that names the shape and points to the convention doc. --- server/backend/src/cq_server/models/knowledge.py | 6 +++--- server/backend/src/cq_server/models/users.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/backend/src/cq_server/models/knowledge.py b/server/backend/src/cq_server/models/knowledge.py index 2ed2fcaf..fe6a4890 100644 --- a/server/backend/src/cq_server/models/knowledge.py +++ b/server/backend/src/cq_server/models/knowledge.py @@ -11,10 +11,10 @@ class FlagRequest(BaseModel): class KnowledgeUnitList(BaseModel): - """Collection envelope for knowledge unit listings. + """Unpaginated collection envelope for knowledge unit listings. - The envelope shape leaves room for pagination metadata (e.g. a - ``next_cursor`` field) without breaking existing clients. + NOTE: List is the unpaginated shape (``{data: [...]}``); cursor-paginated + endpoints use a separate ``Page`` model. See docs/architecture.md §6. """ data: list[KnowledgeUnit] diff --git a/server/backend/src/cq_server/models/users.py b/server/backend/src/cq_server/models/users.py index ad7d511c..19c27d43 100644 --- a/server/backend/src/cq_server/models/users.py +++ b/server/backend/src/cq_server/models/users.py @@ -20,10 +20,10 @@ class ApiKeyPublic(BaseModel): class ApiKeyList(BaseModel): - """Collection envelope for API key listings. + """Unpaginated collection envelope for API key listings. - The envelope shape leaves room for pagination metadata (e.g. a - ``next_cursor`` field) without breaking existing clients. + NOTE: List is the unpaginated shape (``{data: [...]}``); cursor-paginated + endpoints use a separate ``Page`` model. See docs/architecture.md §6. """ data: list[ApiKeyPublic]