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/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. 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..4907fb8b 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,39 @@ 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 + // 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"` + } + if err := json.NewDecoder(resp.Body).Decode(&env); err != nil { + return nil, fmt.Errorf("decoding remote knowledge response: %w", err) + } + + if env.Data == nil { + return nil, fmt.Errorf("decoding remote knowledge response: missing or null data field") } - 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..41248e89 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,76 @@ 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 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) { + 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 +257,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 +277,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 +343,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 +370,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() 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 = { 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/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/knowledge.py b/server/backend/src/cq_server/models/knowledge.py index 62503ef0..fe6a4890 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): + """Unpaginated collection envelope for knowledge unit listings. + + NOTE: List is the unpaginated shape (``{data: [...]}``); cursor-paginated + endpoints use a separate ``Page`` model. See docs/architecture.md §6. + """ + + 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 d8b31a3b..19c27d43 100644 --- a/server/backend/src/cq_server/models/users.py +++ b/server/backend/src/cq_server/models/users.py @@ -19,15 +19,14 @@ class ApiKeyPublic(BaseModel): is_active: bool -class ApiKeysPublic(BaseModel): - """Collection wrapper for API key listings. +class ApiKeyList(BaseModel): + """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] - 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 8d6e081f..f918a595 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) 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/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..6f942297 100644 --- a/server/frontend/src/types.ts +++ b/server/frontend/src/types.ts @@ -101,9 +101,8 @@ export interface CreatedApiKey extends ApiKeyPublic { token: string } -export interface ApiKeysList { +export interface ApiKeyList { data: ApiKeyPublic[] - count: number } export interface MessageResponse {