refactor(api): unify list endpoints on {data} envelope#372
Merged
Conversation
…e 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.
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.
…rnings
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.
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.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR unifies list-style HTTP responses around a { "data": [...] } envelope across the server, SDKs, CLI behavior, frontend types, and architecture docs.
Changes:
- Updates backend knowledge/API-key list models, routes, and tests to return
{data}withoutcount. - Updates Go/Python SDK remote query decoding and CLI query warning output for remote failures.
- Documents list/page HTTP API response conventions.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
server/frontend/src/types.ts |
Renames API-key list type and removes count. |
server/frontend/src/api.ts |
Uses the renamed frontend API-key list type. |
server/backend/tests/test_sqlite_store_e2e.py |
Updates query assertions for {data}. |
server/backend/tests/test_review.py |
Updates review/query tests for {data}. |
server/backend/tests/test_auth.py |
Updates API-key list tests and adds envelope contract coverage. |
server/backend/tests/test_app.py |
Updates knowledge query tests and adds envelope contract coverage. |
server/backend/src/cq_server/services/api_keys.py |
Returns ApiKeyList(data=...). |
server/backend/src/cq_server/models/users.py |
Renames API-key list model and removes count. |
server/backend/src/cq_server/models/knowledge.py |
Adds KnowledgeUnitList envelope model. |
server/backend/src/cq_server/api/routes/users.py |
Updates API-key list route return type. |
server/backend/src/cq_server/api/routes/knowledge.py |
Wraps knowledge query results in KnowledgeUnitList. |
sdk/python/tests/test_client.py |
Updates remote query fixtures and adds invalid-envelope warning tests. |
sdk/python/src/cq/client.py |
Decodes remote query {data} envelope. |
sdk/go/remote.go |
Changes remote query to return results plus errors and decode {data}. |
sdk/go/remote_test.go |
Updates remote query tests for envelope and error behavior. |
sdk/go/client.go |
Surfaces remote query errors as query warnings. |
sdk/go/client_test.go |
Updates query fixtures and adds warning coverage. |
docs/architecture.md |
Adds HTTP API list/page conventions. |
cli/go.sum |
Removes published SDK checksums due local replace. |
cli/go.mod |
Enables local SDK replace directive. |
cli/cmd/query.go |
Prints query warnings to stderr. |
cli/cmd/query_test.go |
Adds CLI stderr warning test. |
cli/cmd/drain_test.go |
Refactors fake remote setup usage. |
cli/cmd/cmd_test.go |
Adds shared fake remote test helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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}.
…tion 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{data: [...]}envelope, because both SDKs silently swallowed the JSON decode failure as graceful degradation. The same shape of bug also masked transport errors and non-200 statuses.GET /api/v1/knowledge,GET /api/v1/users/me/api-keys) now share a singleFooList = {data: [...]}envelope; the redundantcountfield is gone. The Go and Python SDKs decode the envelope and surface remote failures asQueryResult.Warnings; the CLI'squerysubcommand prints those warnings to stderr.docs/architecture.md(new section 6: HTTP API Conventions) so future list endpoints either pickFooList(unpaginated) orFooPage(cursor-paginated) without inventing a third shape.Commits
refactor(server): rename ApiKeysPublic to ApiKeyList— pure rename, no wire change. FrontendApiKeysListrenamed in lockstep.refactor(server): switch list endpoints to {data} envelope, drop count— both/api/v1/knowledgeand/api/v1/users/me/api-keysemit{data}only. Two new contract tests pin the shape so a straycountor other top-level field can't regress.fix(sdk-go): decode {data} envelope and surface remote failures as warnings— silentreturn nilreplaced with explicit error return;Client.Queryappends remote errors toQueryResult.Warnings; CLIquerycommand prints them to stderr; newwithFakeRemote(t, handler)test helper incli/cmd/cmd_test.go(drain_test.go's three inline equivalents migrate to it).cli/go.moduncomments the local-SDK replace directive for the duration of the branch.fix(sdk-python): decode {data} envelope from /api/v1/knowledge— explicit envelope-shape validation; warning text now names the actual problem instead of bubbling a Pydantic ValidationError up generically.docs: add HTTP API Conventions section to architecture.md—FooList/FooPageconvention, thedataroot-key rule, the "no count/total/offset" rule, snake_case wire fields, per-language class naming.Test plan
make test(full suite green at HEAD: 279 server backend + 335 Python SDK + Go SDK + CLI + 11 frontend tests, with-race -count=20stress run on the Go SDK).make lint(all lint targets clean at HEAD).test_query_response_uses_envelope_shape), Go SDK envelope decode (TestRemoteQueryRejectsBareArrayResponse), Python SDK envelope decode (test_remote_query_warns_on_bare_array_response), and CLI stderr surfacing (TestQueryPrintsRemoteWarningsToStderrasserts stderr contains both"warning:"and"decoding").cq query --addr=... --domain=ci --api-key=...returns the expected KUs (the case from the original bug report).Follow-ups (not in this PR)
/oauth/providersto the{data}envelope upstream; the CLI's adapter atcli/internal/auth/http_client.gocan then drop its bespokeprovidersfield name.FooPagedecoder. Today neither has one — by design, no abstraction ahead of need.cli/go.modto a published SDK version once the SDK release goes out (replace directive is uncommented in this PR for the local build).