From 3fb3583a92a8d5313297a99e0680a47b3b9fc7c5 Mon Sep 17 00:00:00 2001 From: nullkey Date: Mon, 11 May 2026 02:44:13 +0800 Subject: [PATCH] feat(cli): add api passthrough, chat streaming, doctor warn status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close the v0.2 RAG demo loop and ship the validation infrastructure: - weknora api [--data X | --data-file F] Raw passthrough wrapping client.Raw, gh-style. JSON envelope mode surfaces status / headers / parsed body. Non-2xx routes through cmdutil.ClassifyHTTPStatus (factored out of ClassifyHTTPError so both SDK-error and direct-status paths stay aligned — reuse review). - weknora chat [--session-id S] [--no-stream] KnowledgeQAStream consumer with two output modes: - TTY default: token streaming + references footer - --json / --no-stream / non-TTY: buffered single envelope Auto-creates a session when --session-id is omitted; the id prints to stderr at start AND on stream failure (^C scrolls past the first announcement, so the recovery hint is re-surfaced when the user is most likely to need it). - cli/internal/sse/Accumulator buffers Content / References / SessionID across SDK callbacks. Idempotent post-Done so misbehaving servers don't corrupt state. - doctor: ok → ok / warn / fail / skip warn marks soft issues that don't block: server within compat range but >=1 minor behind CLI; credential storage falling back to file because keyring is unavailable. Envelope.ok stays true on warn, flips false on fail (exit 1). doctor.error_network golden updated. - cli/acceptance/e2e/ real-server RAG full loop Build-tagged //go:build acceptance_e2e — kept out of the default `go test ./...`. Exercises kb create → doc upload → poll ready → search → chat → cleanup against a server pointed at by WEKNORA_E2E_HOST / _TOKEN. - .github/workflows/cli-e2e.yml manual workflow_dispatch + label-gated PR trigger ("acceptance-e2e"). No-ops gracefully when the secrets aren't set so cross-fork PRs can't accidentally fail the suite. --- .github/workflows/cli-e2e.yml | 66 ++++ cli/acceptance/contract/envelope_test.go | 19 +- cli/acceptance/e2e/e2e_test.go | 265 +++++++++++++ .../envelopes/doctor.error_network.json | 2 +- cli/cmd/api/api.go | 156 ++++++++ cli/cmd/api/api_test.go | 198 ++++++++++ cli/cmd/chat/chat.go | 280 ++++++++++++++ cli/cmd/chat/chat_test.go | 365 ++++++++++++++++++ cli/cmd/doctor/doctor.go | 160 ++++++-- cli/cmd/doctor/doctor_test.go | 274 +++++++++++++ cli/cmd/root.go | 5 + cli/cmd/search/search.go | 29 +- cli/internal/cmdutil/confirm.go | 27 +- cli/internal/cmdutil/errors.go | 8 +- cli/internal/sse/accumulator.go | 58 +++ cli/internal/sse/accumulator_test.go | 90 +++++ 16 files changed, 1935 insertions(+), 67 deletions(-) create mode 100644 .github/workflows/cli-e2e.yml create mode 100644 cli/acceptance/e2e/e2e_test.go create mode 100644 cli/cmd/api/api.go create mode 100644 cli/cmd/api/api_test.go create mode 100644 cli/cmd/chat/chat.go create mode 100644 cli/cmd/chat/chat_test.go create mode 100644 cli/internal/sse/accumulator.go create mode 100644 cli/internal/sse/accumulator_test.go diff --git a/.github/workflows/cli-e2e.yml b/.github/workflows/cli-e2e.yml new file mode 100644 index 00000000..56a8e08a --- /dev/null +++ b/.github/workflows/cli-e2e.yml @@ -0,0 +1,66 @@ +name: CLI E2E + +# Runs the cli/acceptance/e2e/ suite against a real WeKnora server. +# Cost-gated so it doesn't fire on every PR: +# - workflow_dispatch: maintainer can trigger manually +# - pull_request labeled "acceptance-e2e": adding the label kicks a run +# +# Required repo secrets (only set on Tencent/WeKnora; cross-fork PRs have no +# access and the job exits early via the secrets-present guard): +# WEKNORA_E2E_HOST - https URL of the test server +# WEKNORA_E2E_TOKEN - bearer token / API key for that host +# +# Mirrors gh acceptance/ trigger pattern: env-gated, opt-in, single matrix. + +on: + workflow_dispatch: + pull_request: + types: [labeled, synchronize] + paths: + - 'cli/**' + - '.github/workflows/cli-e2e.yml' + +defaults: + run: + working-directory: cli + shell: bash + +permissions: + contents: read + +jobs: + e2e: + name: rag full loop + # Skip auto-trigger on label-add unless the label is exactly "acceptance-e2e". + # Manual workflow_dispatch always runs. + if: | + github.event_name == 'workflow_dispatch' || + (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'acceptance-e2e')) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-go@v6 + with: + go-version: '1.24' + cache-dependency-path: cli/go.sum + + - name: secrets present + id: secrets + env: + E2E_HOST: ${{ secrets.WEKNORA_E2E_HOST }} + E2E_TOKEN: ${{ secrets.WEKNORA_E2E_TOKEN }} + run: | + if [ -z "$E2E_HOST" ] || [ -z "$E2E_TOKEN" ]; then + echo "::warning::WEKNORA_E2E_HOST or WEKNORA_E2E_TOKEN not set; skipping e2e." + echo "skip=true" >> "$GITHUB_OUTPUT" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + + - name: go test -tags=acceptance_e2e + if: steps.secrets.outputs.skip == 'false' + env: + WEKNORA_E2E_HOST: ${{ secrets.WEKNORA_E2E_HOST }} + WEKNORA_E2E_TOKEN: ${{ secrets.WEKNORA_E2E_TOKEN }} + run: go test -tags=acceptance_e2e -v -timeout=8m ./acceptance/e2e/... diff --git a/cli/acceptance/contract/envelope_test.go b/cli/acceptance/contract/envelope_test.go index 189f2286..e556fff7 100644 --- a/cli/acceptance/contract/envelope_test.go +++ b/cli/acceptance/contract/envelope_test.go @@ -95,12 +95,14 @@ var envelopeCases = []envelopeCase{ // 5. doctor.error_network — base_url returns 500 → ping fail → cascade // skip on auth_credential + server_version. credential_storage still - // runs (independent). Doctor's RunE always returns nil; envelope.ok=true - // with summary.failed >= 1. + // runs (independent). v0.2 contract: any check=fail flips envelope.ok + // to false and exits 1 (RunE returns SilentError so the data envelope + // written by emit() is preserved as the only stdout content). { - name: "doctor.error_network", - args: []string{"doctor", "--json"}, - server: alwaysServerError, + name: "doctor.error_network", + args: []string{"doctor", "--json"}, + server: alwaysServerError, + wantErr: true, }, // 6-9. kb list / get — SDK paths /api/v1/knowledge-bases[/] @@ -167,12 +169,12 @@ var envelopeCases = []envelopeCase{ // 14-16. search — top-level command, positional query, --kb required. { name: "search.success", - args: []string{"search", "query", "--kb=kb1", "--top-k=3", "--json"}, + args: []string{"search", "query", "--kb-id=kb1", "--top-k=3", "--json"}, server: searchTwoResults, }, { name: "search.error_resource_not_found", - args: []string{"search", "query", "--kb=missing", "--json"}, + args: []string{"search", "query", "--kb-id=missing", "--json"}, server: always404, wantErr: true, }, @@ -181,7 +183,7 @@ var envelopeCases = []envelopeCase{ // server mock is never consumed; we still pass --kb to satisfy the // MarkFlagRequired check that fires earlier in cobra's parse phase. name: "search.error_input_invalid", - args: []string{"search", "query", "--kb=kb1", "--no-vector", "--no-keyword", "--json"}, + args: []string{"search", "query", "--kb-id=kb1", "--no-vector", "--no-keyword", "--json"}, wantErr: true, }, } @@ -191,7 +193,6 @@ var envelopeCases = []envelopeCase{ // is contractually forbidden — see helpers_test.go SetForTest comment). func TestEnvelopeGolden(t *testing.T) { for _, tc := range envelopeCases { - tc := tc t.Run(tc.name, func(t *testing.T) { var ts *httptest.Server var mockClient *sdk.Client diff --git a/cli/acceptance/e2e/e2e_test.go b/cli/acceptance/e2e/e2e_test.go new file mode 100644 index 00000000..2d89afdb --- /dev/null +++ b/cli/acceptance/e2e/e2e_test.go @@ -0,0 +1,265 @@ +//go:build acceptance_e2e + +// Package e2e_test drives the WeKnora CLI binary against a real running +// server to validate the RAG closing loop end-to-end. +// +// Build tag isolation: //go:build acceptance_e2e excludes this file from +// the default `go test ./...` (mirrors gh's acceptance/ build tag pattern; +// see https://github.com/cli/cli/tree/trunk/acceptance). To run: +// +// cd cli +// WEKNORA_E2E_HOST=https://kb.example.com \ +// WEKNORA_E2E_TOKEN=eyJhbGc... \ +// go test -tags=acceptance_e2e -v ./acceptance/e2e/... +// +// Optional WEKNORA_E2E_KB_NAME_PREFIX customizes the throwaway KB name (default +// "cli-e2e-"). Cleanup runs even on test failure via t.Cleanup so the server +// doesn't accumulate test debris. +package e2e_test + +import ( + "bytes" + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +// TestRAGFullLoop walks the demo MVP path: link a context, create a KB, +// upload a doc, wait for indexing, search it, then chat against it. Each +// step parses the CLI's JSON envelope to extract IDs for the next step — +// validating both functional behavior and wire-contract stability. +func TestRAGFullLoop(t *testing.T) { + host := mustEnv(t, "WEKNORA_E2E_HOST") + token := mustEnv(t, "WEKNORA_E2E_TOKEN") + prefix := envOr("WEKNORA_E2E_KB_NAME_PREFIX", "cli-e2e-") + + bin := buildBinary(t) + xdg := t.TempDir() + writeContextYAML(t, xdg, host, token) + + env := append(os.Environ(), + "XDG_CONFIG_HOME="+xdg, + "XDG_CACHE_HOME="+filepath.Join(xdg, "cache"), + // SDK debug off — explicit so the CI run isn't noisy. C1 SDK silence + // makes this redundant in practice but the explicit flag documents + // intent. + "WEKNORA_SDK_DEBUG=", + ) + + // 1. kb create + kbName := prefix + fmt.Sprintf("%d", time.Now().UnixNano()) + createOut := runJSON(t, bin, env, "kb", "create", "--name", kbName, "--json") + kbData, ok := createOut["data"].(map[string]any) + if !ok { + t.Fatalf("kb create envelope: data not an object: %v", createOut) + } + kbID, _ := kbData["id"].(string) + if kbID == "" { + t.Fatalf("kb create returned no id: %v", createOut) + } + t.Logf("created KB: %s (%s)", kbID, kbName) + + t.Cleanup(func() { + // Best-effort cleanup; a 404 means the KB was already gone. + out, err := run(bin, env, "kb", "delete", kbID, "--force", "--json") + if err != nil { + t.Logf("cleanup kb delete: %v\n%s", err, out) + } + }) + + // 2. doc upload + docPath := writeSampleDoc(t) + uploadOut := runJSON(t, bin, env, "doc", "upload", docPath, "--kb-id", kbID, "--json") + docData, _ := uploadOut["data"].(map[string]any) + docID, _ := docData["id"].(string) + if docID == "" { + t.Fatalf("doc upload returned no id: %v", uploadOut) + } + t.Logf("uploaded doc: %s", docID) + + // 3. poll until indexing finishes (status changes from "pending" / "processing" to "ready" / similar) + waitDocReady(t, bin, env, kbID, docID, 90*time.Second) + + // 4. search — verify retrieval returns chunks + searchOut := runJSON(t, bin, env, "search", "sample", "--kb-id", kbID, "--top-k", "5", "--json") + searchData, _ := searchOut["data"].(map[string]any) + results, _ := searchData["results"].([]any) + if len(results) == 0 { + t.Fatalf("search returned no results: %v", searchOut) + } + t.Logf("search returned %d results", len(results)) + + // 5. chat — verify LLM answer + references in --json + --no-stream mode + // (--no-stream forces accumulator path; --json gates envelope output) + chatOut := runJSON(t, bin, env, "chat", "summarize the document briefly", "--kb-id", kbID, "--no-stream", "--json") + chatData, _ := chatOut["data"].(map[string]any) + answer, _ := chatData["answer"].(string) + if strings.TrimSpace(answer) == "" { + t.Fatalf("chat returned empty answer: %v", chatOut) + } + refs, _ := chatData["references"].([]any) + t.Logf("chat answer (%d chars), %d references", len(answer), len(refs)) + if len(refs) == 0 { + // Soft warning — some servers may not surface references for every + // question, but the demo flow is supposed to. + t.Logf("warning: chat returned 0 references (server may have a different config)") + } +} + +// mustEnv reads an env var and skips the test if missing — keeps the +// suite friendly to community contributors who clone the repo without +// access to the maintainer's E2E secrets. +func mustEnv(t *testing.T, key string) string { + t.Helper() + v := os.Getenv(key) + if v == "" { + t.Skipf("e2e: %s not set; skipping (set the env var or run `gh workflow run cli-e2e.yml`)", key) + } + return v +} + +func envOr(key, fallback string) string { + if v := os.Getenv(key); v != "" { + return v + } + return fallback +} + +// buildBinary compiles the CLI to a temp dir once per test run. Re-using a +// single binary across sub-cases avoids the multi-second linker cost on each +// step and matches gh acceptance/ build behavior. +func buildBinary(t *testing.T) string { + t.Helper() + dir := t.TempDir() + out := filepath.Join(dir, "weknora") + // Repo layout: this test sits at cli/acceptance/e2e/, so cli/ is two levels up. + cmd := exec.Command("go", "build", "-o", out, ".") + cmd.Dir = filepath.Join("..", "..") + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + t.Fatalf("build cli: %v", err) + } + return out +} + +// writeContextYAML drops a minimal config.yaml into XDG_CONFIG_HOME so the +// CLI finds a context without needing `weknora context add` (which prompts +// in interactive scenarios). Tests using `auth login` belong to a different +// suite; here we go straight to authenticated calls. +func writeContextYAML(t *testing.T, xdg, host, token string) { + t.Helper() + dir := filepath.Join(xdg, "weknora") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir xdg: %v", err) + } + yaml := fmt.Sprintf(`current_context: e2e +contexts: + - name: e2e + host: %s + token: %s +`, host, token) + if err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(yaml), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } +} + +// writeSampleDoc emits a small bilingual doc that gives the embedder enough +// signal for retrieval but stays tiny so indexing finishes within the poll +// window. +func writeSampleDoc(t *testing.T) string { + t.Helper() + content := `WeKnora E2E Sample Document + +This sample document is used by the WeKnora CLI acceptance test suite to +validate the end-to-end retrieval-augmented generation pipeline. + +向量检索的核心思想是把文本通过 embedding 模型映射到高维向量空间,然后通过余弦相似度 +等度量找出语义最接近的内容片段。WeKnora 支持 vector + keyword 的混合检索模式。 + +The hybrid search mode combines vector similarity (semantic) with keyword +matching (lexical) to balance recall and precision. +` + dir := t.TempDir() + p := filepath.Join(dir, "sample.txt") + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatalf("write sample: %v", err) + } + return p +} + +// waitDocReady polls `doc list` until the uploaded document's status indicates +// indexing is complete. WeKnora server uses a few status values across versions +// ("ready", "completed", "ok") — accept any non-pending/non-processing/non-failed +// state so we don't break on a server-side rename. Failed status fails the test +// fast. +func waitDocReady(t *testing.T, bin string, env []string, kbID, docID string, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + tick := 2 * time.Second + for time.Now().Before(deadline) { + out := runJSON(t, bin, env, "doc", "list", "--kb-id", kbID, "--page-size", "100", "--json") + data, _ := out["data"].(map[string]any) + items, _ := data["items"].([]any) + for _, it := range items { + m, ok := it.(map[string]any) + if !ok { + continue + } + id, _ := m["id"].(string) + if id != docID { + continue + } + status, _ := m["status"].(string) + low := strings.ToLower(status) + switch { + case low == "failed", low == "error": + t.Fatalf("doc %s indexing failed: status=%q", docID, status) + case low == "pending", low == "processing", low == "": + // keep waiting + default: + t.Logf("doc %s ready (status=%q)", docID, status) + return + } + } + time.Sleep(tick) + } + t.Fatalf("doc %s did not reach ready within %s", docID, timeout) +} + +// run executes the CLI and returns combined stdout. Errors include stderr + +// exit code so failures are debuggable without re-running. +func run(bin string, env []string, args ...string) ([]byte, error) { + cmd := exec.Command(bin, args...) + cmd.Env = env + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return stdout.Bytes(), fmt.Errorf("%s %s: %v\nstderr:\n%s", filepath.Base(bin), strings.Join(args, " "), err, stderr.String()) + } + return stdout.Bytes(), nil +} + +// runJSON runs the CLI expecting --json output and parses the envelope. +// Test fails immediately on non-zero exit or unparseable JSON. +func runJSON(t *testing.T, bin string, env []string, args ...string) map[string]any { + t.Helper() + out, err := run(bin, env, args...) + if err != nil { + t.Fatalf("%v", err) + } + var env_ map[string]any + if err := json.Unmarshal(out, &env_); err != nil { + t.Fatalf("parse envelope from %v: %v\nstdout:\n%s", args, err, string(out)) + } + if ok, _ := env_["ok"].(bool); !ok { + t.Fatalf("envelope ok=false from %v: %s", args, string(out)) + } + return env_ +} diff --git a/cli/acceptance/testdata/envelopes/doctor.error_network.json b/cli/acceptance/testdata/envelopes/doctor.error_network.json index 49fbc739..99d75213 100644 --- a/cli/acceptance/testdata/envelopes/doctor.error_network.json +++ b/cli/acceptance/testdata/envelopes/doctor.error_network.json @@ -1 +1 @@ -{"ok":true,"data":{"summary":{"all_passed":false,"passed":1,"failed":1,"skipped":2},"checks":[{"name":"base_url_reachable","status":"fail","details":"server returned 500","hint":"verify the host configured for the active context (run `weknora auth login --host=...`) and network reachability"},{"name":"auth_credential","status":"skip","details":"prereq failed: base_url_reachable"},{"name":"server_version","status":"skip","details":"prereq failed: auth_credential"},{"name":"credential_storage","status":"ok","details":"keyring or file storage available"}]}} +{"ok":false,"data":{"summary":{"all_passed":false,"passed":1,"failed":1,"skipped":2},"checks":[{"name":"base_url_reachable","status":"fail","details":"server returned 500","hint":"verify the host configured for the active context (run `weknora auth login --host=...`) and network reachability"},{"name":"auth_credential","status":"skip","details":"prereq failed: base_url_reachable"},{"name":"server_version","status":"skip","details":"prereq failed: auth_credential"},{"name":"credential_storage","status":"ok","details":"keyring or file storage available"}]}} diff --git a/cli/cmd/api/api.go b/cli/cmd/api/api.go new file mode 100644 index 00000000..6ea10067 --- /dev/null +++ b/cli/cmd/api/api.go @@ -0,0 +1,156 @@ +// Package api implements the `weknora api` raw HTTP passthrough command. +// +// Mirrors `gh api` ergonomics: 2 positionals (method, path), --data / +// --data-file mutually exclusive body source, default raw response body to +// stdout, --json wraps in CLI envelope. Reuses sdk.Client.Raw which already +// applies tenant + auth headers; v0.2 does not support --header (SDK Raw +// signature lacks header param) — that's planned for v0.3. +package api + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strings" + + "github.com/spf13/cobra" + + "github.com/Tencent/WeKnora/cli/internal/agent" + "github.com/Tencent/WeKnora/cli/internal/cmdutil" + "github.com/Tencent/WeKnora/cli/internal/format" + "github.com/Tencent/WeKnora/cli/internal/iostreams" + sdk "github.com/Tencent/WeKnora/client" +) + +// Options captures `weknora api` flag state. +type Options struct { + Data string + DataFile string + JSONOut bool +} + +// Service is the narrow SDK surface this command depends on. The production +// implementation is *sdk.Client, whose Raw method already injects auth / +// tenant / request-id headers (see client.applyAuthHeaders). Tests substitute +// either a fake or a real client pointed at httptest.Server. +type Service interface { + Raw(ctx context.Context, method, path string, body any) (*http.Response, error) +} + +// NewCmd returns the `weknora api` command. +func NewCmd(f *cmdutil.Factory) *cobra.Command { + opts := &Options{} + cmd := &cobra.Command{ + Use: "api ", + Short: "Make a raw API request to the WeKnora server", + Long: `Send an HTTP request through the SDK and print the response. + +Auth, tenant, and request-id headers are applied automatically from the active +context. The response body is written to stdout by default; use --json to wrap +it in the CLI envelope (status / headers / body). + +Examples: + weknora api GET /api/v1/knowledge-bases + weknora api POST /api/v1/knowledge-bases --data '{"name":"foo"}' + weknora api DELETE /api/v1/knowledge-bases/kb_xxx`, + Args: cobra.ExactArgs(2), + RunE: func(c *cobra.Command, args []string) error { + cli, err := f.Client() + if err != nil { + return err + } + return runAPI(c.Context(), opts, cli, args[0], args[1]) + }, + } + cmd.Flags().StringVarP(&opts.Data, "data", "d", "", "Request body as raw string (e.g. JSON)") + cmd.Flags().StringVar(&opts.DataFile, "data-file", "", "Read request body from file") + cmd.Flags().BoolVar(&opts.JSONOut, "json", false, "Wrap response in JSON envelope (status/headers/body)") + cmd.MarkFlagsMutuallyExclusive("data", "data-file") + agent.SetAgentHelp(cmd, "Raw HTTP passthrough to the WeKnora server. Use when no typed command exists for the endpoint. Headers (auth / tenant / request-id) are injected from the active context.") + return cmd +} + +// runAPI is the testable core: validate inputs, dispatch via Service.Raw, +// classify status, and emit either the raw body or a JSON envelope. +func runAPI(ctx context.Context, opts *Options, svc Service, methodArg, path string) error { + method := strings.ToUpper(methodArg) + switch method { + case "GET", "POST", "PUT", "PATCH", "DELETE", "HEAD": + default: + return cmdutil.NewError(cmdutil.CodeInputInvalidArgument, fmt.Sprintf("unsupported method: %s", methodArg)) + } + if !strings.HasPrefix(path, "/") { + return cmdutil.NewError(cmdutil.CodeInputInvalidArgument, fmt.Sprintf("path must start with /: %s", path)) + } + + // Resolve request body. --data and --data-file are mutually exclusive at + // the cobra layer; the second branch is reachable only when --data is + // empty. + var body any + if opts.Data != "" { + body = json.RawMessage(opts.Data) + } else if opts.DataFile != "" { + contents, err := os.ReadFile(opts.DataFile) + if err != nil { + return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "read data file %s", opts.DataFile) + } + body = json.RawMessage(contents) + } + + resp, err := svc.Raw(ctx, method, path, body) + if err != nil { + // Transport / DNS failure (Raw never returns a typed HTTP error of its + // own; non-2xx responses still surface as resp != nil, err == nil). + return cmdutil.Wrapf(cmdutil.ClassifyHTTPError(err), err, "%s %s", method, path) + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return cmdutil.Wrapf(cmdutil.CodeNetworkError, err, "read response body") + } + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + code := cmdutil.ClassifyHTTPStatus(resp.StatusCode) + return cmdutil.NewError(code, fmt.Sprintf("HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(respBody)))) + } + + out := iostreams.IO.Out + if opts.JSONOut { + // Best-effort decode: if response body is valid JSON, surface the + // parsed structure under .data.body so envelope consumers can drill + // in; otherwise fall back to the raw string. + var bodyAny any + if len(respBody) > 0 { + if err := json.Unmarshal(respBody, &bodyAny); err != nil { + bodyAny = string(respBody) + } + } + hdrs := make(map[string]string, len(resp.Header)) + for k, v := range resp.Header { + if len(v) > 0 { + hdrs[k] = v[0] + } + } + env := format.Success(map[string]any{ + "status": resp.StatusCode, + "headers": hdrs, + "body": bodyAny, + }, nil) + return format.WriteEnvelope(out, env) + } + + if _, err := out.Write(respBody); err != nil { + return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "write response body") + } + if len(respBody) > 0 && respBody[len(respBody)-1] != '\n' { + _, _ = out.Write([]byte{'\n'}) + } + return nil +} + +// compile-time check: the production SDK client implements Service. +var _ Service = (*sdk.Client)(nil) diff --git a/cli/cmd/api/api_test.go b/cli/cmd/api/api_test.go new file mode 100644 index 00000000..739767e2 --- /dev/null +++ b/cli/cmd/api/api_test.go @@ -0,0 +1,198 @@ +package api + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Tencent/WeKnora/cli/internal/cmdutil" + "github.com/Tencent/WeKnora/cli/internal/iostreams" + sdk "github.com/Tencent/WeKnora/client" +) + +// newTestClient stands up an httptest server with the supplied handler and +// returns an *sdk.Client targeting it plus a teardown closure. The real SDK is +// used so we exercise the same Raw() code path as production (header +// injection, JSON marshalling, etc.). +func newTestClient(t *testing.T, h http.HandlerFunc) (*sdk.Client, func()) { + t.Helper() + srv := httptest.NewServer(h) + return sdk.NewClient(srv.URL), srv.Close +} + +func TestAPI_GetSuccess(t *testing.T) { + out, _ := iostreams.SetForTest(t) + cli, stop := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/api/v1/foo" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"hello":"world"}`)) + }) + defer stop() + + if err := runAPI(context.Background(), &Options{}, cli, "GET", "/api/v1/foo"); err != nil { + t.Fatalf("runAPI: %v", err) + } + got := out.String() + if !strings.Contains(got, `"hello":"world"`) { + t.Errorf("expected raw JSON body in stdout, got %q", got) + } + if !strings.HasSuffix(got, "\n") { + t.Errorf("expected trailing newline appended, got %q", got) + } +} + +func TestAPI_GetSuccess_JSON(t *testing.T) { + out, _ := iostreams.SetForTest(t) + cli, stop := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Request-Id", "req-123") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"value":42}`)) + }) + defer stop() + + if err := runAPI(context.Background(), &Options{JSONOut: true}, cli, "GET", "/api/v1/foo"); err != nil { + t.Fatalf("runAPI: %v", err) + } + var env struct { + OK bool `json:"ok"` + Data struct { + Status int `json:"status"` + Headers map[string]string `json:"headers"` + Body map[string]any `json:"body"` + } `json:"data"` + } + if err := json.Unmarshal(out.Bytes(), &env); err != nil { + t.Fatalf("decode envelope: %v\n%s", err, out.String()) + } + if !env.OK { + t.Errorf("expected ok:true, got %s", out.String()) + } + if env.Data.Status != 200 { + t.Errorf("status: want 200, got %d", env.Data.Status) + } + if env.Data.Headers["Content-Type"] != "application/json" { + t.Errorf("Content-Type header missing: %v", env.Data.Headers) + } + if got, ok := env.Data.Body["value"]; !ok || got.(float64) != 42 { + t.Errorf("body.value: want 42, got %v", env.Data.Body) + } +} + +func TestAPI_PostWithData(t *testing.T) { + _, _ = iostreams.SetForTest(t) + var seenBody []byte + var seenMethod, seenPath string + cli, stop := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + seenMethod = r.Method + seenPath = r.URL.Path + seenBody, _ = io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"new"}`)) + }) + defer stop() + + opts := &Options{Data: `{"name":"foo"}`} + if err := runAPI(context.Background(), opts, cli, "POST", "/api/v1/things"); err != nil { + t.Fatalf("runAPI: %v", err) + } + if seenMethod != http.MethodPost || seenPath != "/api/v1/things" { + t.Errorf("server saw %s %s, want POST /api/v1/things", seenMethod, seenPath) + } + // SDK marshals body via json.Marshal; json.RawMessage round-trips + // verbatim so the bytes server-side equal the --data argument. + if string(seenBody) != `{"name":"foo"}` { + t.Errorf("server received body %q, want %q", seenBody, `{"name":"foo"}`) + } +} + +func TestAPI_DataFile(t *testing.T) { + _, _ = iostreams.SetForTest(t) + tmp := filepath.Join(t.TempDir(), "body.json") + payload := `{"k":"from-file"}` + if err := os.WriteFile(tmp, []byte(payload), 0o600); err != nil { + t.Fatalf("write temp: %v", err) + } + var seenBody []byte + cli, stop := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + seenBody, _ = io.ReadAll(r.Body) + _, _ = w.Write([]byte(`{}`)) + }) + defer stop() + + opts := &Options{DataFile: tmp} + if err := runAPI(context.Background(), opts, cli, "POST", "/api/v1/x"); err != nil { + t.Fatalf("runAPI: %v", err) + } + if string(seenBody) != payload { + t.Errorf("body from --data-file: got %q, want %q", seenBody, payload) + } +} + +func TestAPI_NotFound(t *testing.T) { + _, _ = iostreams.SetForTest(t) + cli, stop := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"missing"}`)) + }) + defer stop() + + err := runAPI(context.Background(), &Options{}, cli, "GET", "/api/v1/missing") + if err == nil { + t.Fatal("expected error for 404") + } + if !cmdutil.IsNotFound(err) { + t.Errorf("expected resource.not_found, got %v", err) + } +} + +func TestAPI_InvalidMethod(t *testing.T) { + _, _ = iostreams.SetForTest(t) + // No server needed: validation should fail before dispatch. + err := runAPI(context.Background(), &Options{}, nil, "FOO", "/api/v1/things") + if err == nil { + t.Fatal("expected error for unsupported method") + } + var ce *cmdutil.Error + if !asTypedError(err, &ce) || ce.Code != cmdutil.CodeInputInvalidArgument { + t.Errorf("expected input.invalid_argument, got %v", err) + } +} + +func TestAPI_PathWithoutSlash(t *testing.T) { + _, _ = iostreams.SetForTest(t) + err := runAPI(context.Background(), &Options{}, nil, "GET", "api/v1/things") + if err == nil { + t.Fatal("expected error for missing leading slash") + } + var ce *cmdutil.Error + if !asTypedError(err, &ce) || ce.Code != cmdutil.CodeInputInvalidArgument { + t.Errorf("expected input.invalid_argument, got %v", err) + } +} + +// asTypedError is a tiny wrapper around errors.As that keeps the call sites +// concise. Returns true on success, populating dst. +func asTypedError(err error, dst **cmdutil.Error) bool { + for e := err; e != nil; { + if t, ok := e.(*cmdutil.Error); ok { + *dst = t + return true + } + type unwrapper interface{ Unwrap() error } + u, ok := e.(unwrapper) + if !ok { + return false + } + e = u.Unwrap() + } + return false +} diff --git a/cli/cmd/chat/chat.go b/cli/cmd/chat/chat.go new file mode 100644 index 00000000..ddd98a15 --- /dev/null +++ b/cli/cmd/chat/chat.go @@ -0,0 +1,280 @@ +// Package chat implements `weknora chat ` — the streaming RAG answer +// entry point. +// +// Two output modes share a single SDK call: +// +// - Stream mode (TTY + no --no-stream + no --json): write each +// StreamResponse.Content fragment directly to iostreams.IO.Out as it +// arrives, then print a footer with knowledge references. This is the +// "feels alive" UX a human typing in a terminal expects. +// +// - Accumulate mode (non-TTY, --no-stream, or --json): buffer every +// fragment via sse.Accumulator and emit a single envelope (or a single +// plain-text answer + references block) once Done. Agents and pipes +// get a deterministic single record to parse. +// +// The SDK's KnowledgeQAStream callback contract is invoked sequentially on +// one goroutine, so neither mode needs locking. The runChat core takes a +// chatService interface so tests inject a fake without standing up a real +// SSE server. +package chat + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + + "github.com/spf13/cobra" + + "github.com/Tencent/WeKnora/cli/internal/cmdutil" + "github.com/Tencent/WeKnora/cli/internal/format" + "github.com/Tencent/WeKnora/cli/internal/iostreams" + "github.com/Tencent/WeKnora/cli/internal/sse" + sdk "github.com/Tencent/WeKnora/client" +) + +// Options captures one `weknora chat` invocation. +type Options struct { + Query string + KBID string + SessionID string + NoStream bool + JSONOut bool +} + +// chatService is the narrow SDK surface this command depends on. *sdk.Client +// satisfies it; tests substitute a fake. Compile-time check is at the bottom +// of this file. +type chatService interface { + CreateSession(ctx context.Context, req *sdk.CreateSessionRequest) (*sdk.Session, error) + KnowledgeQAStream(ctx context.Context, sessionID string, req *sdk.KnowledgeQARequest, cb func(*sdk.StreamResponse) error) error +} + +// chatData is the success-envelope payload. Mirrors what an agent needs to +// continue a conversation: the answer text, retrieval references, and the +// session pointer to thread follow-ups. +type chatData struct { + Answer string `json:"answer"` + References []*sdk.SearchResult `json:"references"` + SessionID string `json:"session_id"` + AssistantMessageID string `json:"assistant_message_id,omitempty"` + KBID string `json:"kb_id"` + Query string `json:"query"` +} + +// NewCmd builds `weknora chat `. +func NewCmd(f *cmdutil.Factory) *cobra.Command { + opts := &Options{} + cmd := &cobra.Command{ + Use: `chat `, + Short: "Ask a streaming RAG question against a knowledge base", + Long: `Send a query to the WeKnora knowledge-chat endpoint and stream the +answer back. By default a fresh session is created on first invocation; pass +--session-id to continue an existing conversation. + +Modes: + TTY (default): live token streaming + reference footer + Pipe / --no-stream / --json: buffered, emitted once on completion`, + Example: ` weknora chat "What is RRF?" --kb-id kb_123 + weknora chat "Summarise PR-7" --kb my-kb --json + weknora chat "Continue?" --session-id sess_abc`, + Args: cobra.MinimumNArgs(1), + RunE: func(c *cobra.Command, args []string) error { + opts.Query = strings.TrimSpace(strings.Join(args, " ")) + if opts.Query == "" { + return cmdutil.NewError(cmdutil.CodeInputInvalidArgument, "query argument cannot be empty") + } + kbID, err := f.ResolveKB(c) + if err != nil { + return err + } + opts.KBID = kbID + cli, err := f.Client() + if err != nil { + return err + } + return runChat(c.Context(), opts, cli) + }, + } + cmd.Flags().String("kb-id", "", "Knowledge base ID (overrides project link / env)") + cmd.Flags().String("kb", "", "Knowledge base name (resolved via list)") + cmd.Flags().StringVar(&opts.SessionID, "session-id", "", "Continue an existing chat session (skip auto-create)") + cmd.Flags().BoolVar(&opts.NoStream, "no-stream", false, "Buffer the full answer before printing (forces accumulate mode)") + cmd.Flags().BoolVar(&opts.JSONOut, "json", false, "Emit a single JSON envelope (implies --no-stream)") + cmd.MarkFlagsMutuallyExclusive("kb-id", "kb") + return cmd +} + +// runChat is the testable core: validate, ensure a session, dispatch the +// stream, and route output. Returns a typed error suitable for the envelope. +func runChat(ctx context.Context, opts *Options, svc chatService) error { + if opts.Query == "" { + return cmdutil.NewError(cmdutil.CodeInputInvalidArgument, "query argument cannot be empty") + } + if opts.KBID == "" { + // Defensive: the cobra layer resolves KB before runChat; this guards + // the direct-test entry point. + return cmdutil.NewError(cmdutil.CodeKBIDRequired, "kb id is required") + } + if svc == nil { + return cmdutil.NewError(cmdutil.CodeServerError, "chat: no SDK client available") + } + + sessionID := opts.SessionID + autoCreated := false + if sessionID == "" { + sess, err := svc.CreateSession(ctx, &sdk.CreateSessionRequest{Title: "weknora chat"}) + if err != nil { + // Map HTTP-shaped failures, but tag generic transport / unknown + // errors as session_create_failed so the dedicated hint fires. + code := cmdutil.ClassifyHTTPError(err) + if code == cmdutil.CodeNetworkError || code == cmdutil.CodeServerError { + code = cmdutil.CodeSessionCreateFailed + } + return cmdutil.Wrapf(code, err, "create chat session") + } + sessionID = sess.ID + autoCreated = true + } + + // Decide output mode. Stream mode requires: + // 1. an interactive stdout (tty) + // 2. no --no-stream + // 3. no --json (envelope is single-record by definition) + streamMode := iostreams.IO.IsStdoutTTY() && !opts.NoStream && !opts.JSONOut + + // Surface the auto-created session ID up-front so a user who hits ^C + // mid-stream still has the pointer to resume — no need to scroll back + // past tokens. Skipped in JSON mode (it ends up in the envelope) and + // when the caller already supplied --session-id. + if autoCreated && !opts.JSONOut { + fmt.Fprintf(iostreams.IO.Err, "session: %s (use --session-id to continue)\n", sessionID) + } + + req := &sdk.KnowledgeQARequest{ + Query: opts.Query, + KnowledgeBaseIDs: []string{opts.KBID}, + AgentEnabled: false, + WebSearchEnabled: false, + Channel: "api", + } + + acc := &sse.Accumulator{} + + cb := func(r *sdk.StreamResponse) error { + if streamMode && r != nil && r.Content != "" { + // Best-effort write; if stdout dies the SDK will surface the + // error on the next iteration. No need to bail early. + _, _ = iostreams.IO.Out.Write([]byte(r.Content)) + } + acc.Append(r) + return nil + } + + streamErr := svc.KnowledgeQAStream(ctx, sessionID, req, cb) + if streamErr != nil { + // Re-surface the auto-created session id on failure so a user who + // missed the start-of-stream notice (it scrolls past mid-stream + // tokens, especially on ^C) can still recover with --session-id. + // Skipped in JSON mode — the envelope carries it in .data.session_id. + if autoCreated && !opts.JSONOut { + fmt.Fprintf(iostreams.IO.Err, "session: %s (resume with --session-id %s)\n", sessionID, sessionID) + } + // Context cancelled (Ctrl-C) → user-aborted, exit 130 lineage. + if errors.Is(streamErr, context.Canceled) || errors.Is(ctx.Err(), context.Canceled) { + return cmdutil.Wrapf(cmdutil.CodeUserAborted, streamErr, "chat cancelled") + } + // Stream began (we observed at least one event) but never reached a + // terminal Done frame: typed as sse_stream_aborted so the hint + // nudges the user toward --no-stream / retry. + if acc.Result() != "" && !acc.Done() { + return cmdutil.Wrapf(cmdutil.CodeSSEStreamAborted, streamErr, "stream aborted before completion") + } + // Pre-stream HTTP / transport failure: route through the canonical + // classifier so 401 / 404 / 5xx still surface their specific codes. + return cmdutil.Wrapf(cmdutil.ClassifyHTTPError(streamErr), streamErr, "knowledge qa stream") + } + + // SDK returned nil but we never saw a Done event — server closed the + // connection cleanly mid-stream. Treat as aborted so the user sees the + // truncation rather than a silent partial answer. Includes the empty-body + // case (Done frame never arrived AND no content): better to surface the + // abort than emit ok=true with answer="" — agents can't distinguish the + // model genuinely had nothing to say from the stream getting cut. + if !acc.Done() { + return cmdutil.NewError(cmdutil.CodeSSEStreamAborted, "stream ended without a terminal event") + } + + answer := acc.Result() + references := acc.References + + if opts.JSONOut { + // Prefer the SDK-echoed session id (acc.SessionID) but fall back to + // our local sessionID — agents must always see a usable pointer. + sid := acc.SessionID + if sid == "" { + sid = sessionID + } + data := chatData{ + Answer: answer, + References: references, + SessionID: sid, + AssistantMessageID: acc.AssistantMessageID, + KBID: opts.KBID, + Query: opts.Query, + } + return cmdutil.NewJSONExporter().Write(iostreams.IO.Out, format.Success(data, &format.Meta{KBID: opts.KBID})) + } + + // Human / non-JSON paths: streaming mode already wrote the answer body + // via the callback, so we only need to render the trailing references + // (and a closing newline). Accumulate + non-JSON writes the answer here + // for the first time. + out := iostreams.IO.Out + if streamMode { + // Ensure the answer line ends cleanly before the references footer. + if !strings.HasSuffix(answer, "\n") { + fmt.Fprintln(out) + } + } else { + fmt.Fprint(out, answer) + if !strings.HasSuffix(answer, "\n") { + fmt.Fprintln(out) + } + } + renderReferences(out, references) + return nil +} + +// renderReferences prints a compact human-readable references block. +// Skipped entirely when the server returned no references — agent-friendly +// silence beats an empty banner. +func renderReferences(w io.Writer, refs []*sdk.SearchResult) { + if len(refs) == 0 { + return + } + fmt.Fprintln(w) + fmt.Fprintln(w, "──── References ────") + for i, r := range refs { + if r == nil { + continue + } + title := r.KnowledgeTitle + if title == "" { + title = r.KnowledgeFilename + } + if title == "" { + title = r.KnowledgeID + } + fmt.Fprintf(w, "[%d] %s", i+1, title) + if r.Score > 0 { + fmt.Fprintf(w, " score=%.3f", r.Score) + } + fmt.Fprintln(w) + } +} + +// compile-time check: the production SDK client implements chatService. +var _ chatService = (*sdk.Client)(nil) diff --git a/cli/cmd/chat/chat_test.go b/cli/cmd/chat/chat_test.go new file mode 100644 index 00000000..4c4373f9 --- /dev/null +++ b/cli/cmd/chat/chat_test.go @@ -0,0 +1,365 @@ +package chat + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + + "github.com/Tencent/WeKnora/cli/internal/cmdutil" + "github.com/Tencent/WeKnora/cli/internal/iostreams" + sdk "github.com/Tencent/WeKnora/client" +) + +// fakeChatService implements chatService for unit tests. Tests configure the +// callback driver via streamEvents (delivered in order) and observe captured +// inputs through the exported fields. +type fakeChatService struct { + createSessionResp *sdk.Session + createSessionErr error + createCalled bool + + streamErr error + streamEvents []*sdk.StreamResponse + gotSessionID string + gotRequest *sdk.KnowledgeQARequest + streamCalled bool + cbReturnsError error // if set, callback aborts after first event with this error +} + +func (f *fakeChatService) CreateSession(_ context.Context, req *sdk.CreateSessionRequest) (*sdk.Session, error) { + f.createCalled = true + if f.createSessionErr != nil { + return nil, f.createSessionErr + } + if f.createSessionResp != nil { + return f.createSessionResp, nil + } + // Default: return a deterministic session id derived from the title so + // envelope assertions don't depend on uuid generation. + return &sdk.Session{ID: "sess_auto", Title: req.Title}, nil +} + +func (f *fakeChatService) KnowledgeQAStream(ctx context.Context, sessionID string, req *sdk.KnowledgeQARequest, cb func(*sdk.StreamResponse) error) error { + f.streamCalled = true + f.gotSessionID = sessionID + f.gotRequest = req + for _, ev := range f.streamEvents { + if err := cb(ev); err != nil { + return err + } + if f.cbReturnsError != nil { + return f.cbReturnsError + } + } + return f.streamErr +} + +// Sanity: fakeChatService must satisfy chatService. Mirrors the production +// var _ chatService = (*sdk.Client)(nil) check at the bottom of chat.go. +var _ chatService = (*fakeChatService)(nil) + +func TestChat_StreamMode(t *testing.T) { + out, errBuf := iostreams.SetForTestWithTTY(t) + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{ + {Content: "Hello "}, + {Content: "world"}, + {Done: true, KnowledgeReferences: []*sdk.SearchResult{ + {KnowledgeID: "k1", KnowledgeTitle: "Doc One", Score: 0.42}, + }}, + }, + } + opts := &Options{Query: "hi", KBID: "kb_1"} + if err := runChat(context.Background(), opts, svc); err != nil { + t.Fatalf("runChat: %v", err) + } + got := out.String() + if !strings.Contains(got, "Hello world") { + t.Errorf("stdout missing streamed content: %q", got) + } + if !strings.Contains(got, "References") { + t.Errorf("stdout missing references footer: %q", got) + } + if !strings.Contains(got, "Doc One") { + t.Errorf("references should render KnowledgeTitle, got %q", got) + } + // auto-created session must announce itself on stderr + if !strings.Contains(errBuf.String(), "session: sess_auto") { + t.Errorf("expected stderr session hint, got %q", errBuf.String()) + } + if !svc.createCalled { + t.Error("expected CreateSession invocation when SessionID empty") + } + if svc.gotSessionID != "sess_auto" { + t.Errorf("stream sessionID: got %q want sess_auto", svc.gotSessionID) + } + if svc.gotRequest == nil || svc.gotRequest.Channel != "api" { + t.Errorf("expected Channel=api, got %+v", svc.gotRequest) + } +} + +func TestChat_JSONMode(t *testing.T) { + out, errBuf := iostreams.SetForTestWithTTY(t) + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{ + {Content: "answer body"}, + {AssistantMessageID: "msg_99"}, + {Done: true, KnowledgeReferences: []*sdk.SearchResult{{KnowledgeID: "k1"}}}, + }, + } + opts := &Options{Query: "q", KBID: "kb_42", JSONOut: true} + if err := runChat(context.Background(), opts, svc); err != nil { + t.Fatalf("runChat: %v", err) + } + + // JSON mode must NOT print the human session-hint on stderr; the session + // id is carried inside the envelope instead. + if errBuf.Len() != 0 { + t.Errorf("expected empty stderr in JSON mode, got %q", errBuf.String()) + } + + var env struct { + OK bool `json:"ok"` + Data struct { + Answer string `json:"answer"` + SessionID string `json:"session_id"` + AssistantMessageID string `json:"assistant_message_id"` + KBID string `json:"kb_id"` + Query string `json:"query"` + References []struct { + KnowledgeID string `json:"knowledge_id"` + } `json:"references"` + } `json:"data"` + } + if err := json.Unmarshal(out.Bytes(), &env); err != nil { + t.Fatalf("decode envelope: %v\n%s", err, out.String()) + } + if !env.OK { + t.Errorf("expected ok:true, got %s", out.String()) + } + if env.Data.Answer != "answer body" { + t.Errorf("answer: got %q", env.Data.Answer) + } + if env.Data.SessionID != "sess_auto" { + t.Errorf("session_id: got %q", env.Data.SessionID) + } + if env.Data.AssistantMessageID != "msg_99" { + t.Errorf("assistant_message_id: got %q", env.Data.AssistantMessageID) + } + if env.Data.KBID != "kb_42" { + t.Errorf("kb_id: got %q", env.Data.KBID) + } + if env.Data.Query != "q" { + t.Errorf("query: got %q", env.Data.Query) + } + if len(env.Data.References) != 1 || env.Data.References[0].KnowledgeID != "k1" { + t.Errorf("references payload missing: %+v", env.Data.References) + } +} + +func TestChat_NoStreamFlag(t *testing.T) { + // TTY enabled, but --no-stream forces accumulate mode → no live writes + // during callback; entire answer printed once after Done. + out, _ := iostreams.SetForTestWithTTY(t) + var written string + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{ + {Content: "buffered "}, + {Content: "answer"}, + {Done: true}, + }, + } + opts := &Options{Query: "q", KBID: "kb", NoStream: true} + if err := runChat(context.Background(), opts, svc); err != nil { + t.Fatalf("runChat: %v", err) + } + written = out.String() + if !strings.Contains(written, "buffered answer") { + t.Errorf("expected concatenated answer, got %q", written) + } +} + +func TestChat_NonTTY_AccumulateMode(t *testing.T) { + // Non-TTY iostreams forces accumulate mode even without --no-stream. + out, _ := iostreams.SetForTest(t) + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{ + {Content: "piped"}, + {Done: true}, + }, + } + opts := &Options{Query: "q", KBID: "kb"} + if err := runChat(context.Background(), opts, svc); err != nil { + t.Fatalf("runChat: %v", err) + } + if !strings.Contains(out.String(), "piped") { + t.Errorf("expected accumulated answer, got %q", out.String()) + } +} + +func TestChat_SessionIDProvided(t *testing.T) { + _, errBuf := iostreams.SetForTestWithTTY(t) + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{{Done: true}}, + } + opts := &Options{Query: "q", KBID: "kb", SessionID: "sess_existing"} + if err := runChat(context.Background(), opts, svc); err != nil { + t.Fatalf("runChat: %v", err) + } + if svc.createCalled { + t.Error("CreateSession must NOT be invoked when --session-id is provided") + } + if svc.gotSessionID != "sess_existing" { + t.Errorf("stream sessionID: got %q want sess_existing", svc.gotSessionID) + } + // No auto-create message because the user supplied the id. + if strings.Contains(errBuf.String(), "session:") { + t.Errorf("unexpected session hint emitted with explicit --session-id: %q", errBuf.String()) + } +} + +func TestChat_KBIDRequired(t *testing.T) { + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{} + // Run with KBID empty (bypassing the cobra resolver). + opts := &Options{Query: "q"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeKBIDRequired { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeKBIDRequired) + } + if svc.createCalled || svc.streamCalled { + t.Error("KB validation must short-circuit before any SDK call") + } +} + +func TestChat_EmptyQuery(t *testing.T) { + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{} + opts := &Options{Query: "", KBID: "kb"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeInputInvalidArgument { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeInputInvalidArgument) + } +} + +func TestChat_SDKError_PreStream(t *testing.T) { + // SDK fails before any event arrives → ClassifyHTTPError mapping. + // "HTTP error 401: ..." → auth.unauthenticated. + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{ + streamErr: errors.New("HTTP error 401: token rejected"), + } + opts := &Options{Query: "q", KBID: "kb"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeAuthUnauthenticated { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeAuthUnauthenticated) + } +} + +func TestChat_SDKError_MidStream_AbortsAsSSE(t *testing.T) { + // Some content arrived, then the stream errored without a Done event → + // CodeSSEStreamAborted (separate from generic transport failure). + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{ + streamEvents: []*sdk.StreamResponse{{Content: "partial"}}, + streamErr: errors.New("connection reset"), + } + opts := &Options{Query: "q", KBID: "kb"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeSSEStreamAborted { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeSSEStreamAborted) + } +} + +func TestChat_ContextCancelled(t *testing.T) { + _, _ = iostreams.SetForTest(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() // simulate Ctrl-C delivered before the SDK returns. + svc := &fakeChatService{streamErr: context.Canceled} + opts := &Options{Query: "q", KBID: "kb"} + err := runChat(ctx, opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeUserAborted { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeUserAborted) + } +} + +func TestChat_SessionCreateFails(t *testing.T) { + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{ + createSessionErr: errors.New("dial tcp: connection refused"), + } + opts := &Options{Query: "q", KBID: "kb"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeSessionCreateFailed { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeSessionCreateFailed) + } + if svc.streamCalled { + t.Error("stream must not be invoked after session creation failed") + } +} + +func TestChat_SessionCreate404SurfacesNotFound(t *testing.T) { + // HTTP-shaped session-create failures should NOT collapse into the + // session_create_failed bucket; they keep their canonical mapping so + // agents can react to e.g. resource.not_found. + _, _ = iostreams.SetForTest(t) + svc := &fakeChatService{ + createSessionErr: errors.New("HTTP error 404: tenant not found"), + } + opts := &Options{Query: "q", KBID: "kb"} + err := runChat(context.Background(), opts, svc) + if err == nil { + t.Fatal("expected error, got nil") + } + var typed *cmdutil.Error + if !errors.As(err, &typed) { + t.Fatalf("expected *cmdutil.Error, got %T", err) + } + if typed.Code != cmdutil.CodeResourceNotFound { + t.Errorf("code: got %q want %q", typed.Code, cmdutil.CodeResourceNotFound) + } +} diff --git a/cli/cmd/doctor/doctor.go b/cli/cmd/doctor/doctor.go index 1e0435f4..9c148dea 100644 --- a/cli/cmd/doctor/doctor.go +++ b/cli/cmd/doctor/doctor.go @@ -1,18 +1,23 @@ // Package doctor implements `weknora doctor` — 4-item self-check (spec §1.2). // -// Status semantics (3-tier, from larksuite/cli's pass/fail/warn/skip set): +// Status semantics (4-tier, from larksuite/cli's pass/fail/warn/skip set): // // ok — passed +// warn — soft problem; non-blocking (e.g. server minor older than CLI, +// keychain unavailable so falling back to file store) // fail — failed; "hint" actionable // skip — cascade-skipped (prereq failed) or --offline mode // -// Envelope: ok=true normally; data.summary.all_passed gives the agent a -// one-line short-circuit (spec §1.2 防 envelope.ok=true 误判). +// Envelope (v0.2): +// - any check is fail → envelope.ok=false, exit 1 (RunE returns SilentError +// so the data envelope is still emitted; the framework's error-envelope +// printer is bypassed for this command) +// - warn only / all ok → envelope.ok=true, exit 0 +// - warn does NOT flip envelope.ok // -// Special: base URL completely unreachable + non-offline → no checks can be -// initiated → caller may decide to surface envelope.ok=false. v0.1 minimal -// approach: even base_url fail still runs credential_storage (independent), -// so envelope.ok stays true; agents read data.summary.failed > 0. +// data.summary.all_passed gives the agent a one-line short-circuit; v0.2 +// keeps it true ONLY when no warn / fail / skip checks are present. Agents +// SHOULD also inspect data.checks[].status to distinguish warn from ok. package doctor import ( @@ -48,6 +53,7 @@ type Status string const ( StatusOK Status = "ok" + StatusWarn Status = "warn" StatusFail Status = "fail" StatusSkip Status = "skip" ) @@ -61,9 +67,14 @@ type Check struct { } // Summary is the agent-friendly short-circuit payload (spec §1.2). +// +// AllPassed is true only when there are zero warn/fail/skip rows; warn does +// not block exit-0 (envelope.ok stays true) but it does flip AllPassed so +// agents reading just the boolean still notice the soft issue. type Summary struct { AllPassed bool `json:"all_passed"` Passed int `json:"passed"` + Warned int `json:"warned,omitempty"` Failed int `json:"failed"` Skipped int `json:"skipped"` } @@ -96,26 +107,37 @@ func NewCmd(f *cmdutil.Factory) *cobra.Command { cliVer, _, _ := build.Info() r := runChecks(c.Context(), opts, svc, cliVer) emit(opts, r) - return nil // doctor 自身不返回 error;失败状态在 data.checks + // v0.2 exit-code policy: fail → exit 1 (gh / kubectl convention); + // warn / ok / skip → exit 0. SilentError suppresses both the human + // "error: ..." line and the error envelope printer, so the data + // envelope already written by emit() is the only stdout content. + if r.Summary.Failed > 0 { + return cmdutil.SilentError + } + return nil }, } cmd.Flags().BoolVar(&opts.NoCache, "no-cache", false, "Bypass server-info cache (located at $XDG_CACHE_HOME/weknora/server-info.yaml); force re-probe") cmd.Flags().BoolVar(&opts.Offline, "offline", false, "Skip network checks; only verify local keyring/file storage (credential_storage check still runs)") cmd.Flags().BoolVar(&opts.JSONOut, "json", false, "Output JSON envelope") - agent.SetAgentHelp(cmd, "Returns 4 health checks. AGENT short-circuit: read data.summary.all_passed; if false, inspect data.checks[].status (ok/fail/skip).") + agent.SetAgentHelp(cmd, "Returns 4 health checks. AGENT short-circuit: read data.summary.all_passed; if false, inspect data.checks[].status (ok/warn/fail/skip). exit 1 only when any status=fail; warn does not change envelope.ok.") return cmd } // cascade implements the two short-circuits every gated check shares: // offline-mode skip and prereq-failed skip. Returns true when the check has // been completed (Status set on c) and the caller should NOT run its body. +// +// A prereq is considered "passing" if its Status is OK or Warn — warn signals +// a soft problem that does not block downstream functionality. Only fail/skip +// cascades into a downstream skip. func cascade(c *Check, offline bool, prereqs ...*Check) bool { if offline { c.Status, c.Details = StatusSkip, "offline mode" return true } for _, p := range prereqs { - if p.Status != StatusOK { + if p.Status != StatusOK && p.Status != StatusWarn { c.Status, c.Details = StatusSkip, "prereq failed: "+p.Name return true } @@ -169,14 +191,7 @@ func runChecks(ctx context.Context, opts *Options, svc Services, cliVer string) } // 4. credential_storage — independent of network; never gated by offline. - if _, err := secrets.NewBestEffortStore(); err != nil { - checks[3].Status = StatusFail - checks[3].Details = err.Error() - checks[3].Hint = "verify keyring access; falls back to file store" - } else { - checks[3].Status = StatusOK - checks[3].Details = "keyring or file storage available" - } + fillCredentialStorageCheck(&checks[3]) return Result{Summary: summarize(checks), Checks: checks} } @@ -185,24 +200,74 @@ func runChecks(ctx context.Context, opts *Options, svc Services, cliVer string) // sets Status/Details/Hint on c. fromCache toggles the "cached" suffix — // the loader knows authoritatively which branch it took, time-based // derivation from ProbedAt is unreliable since SaveCache uses time.Now(). +// +// v0.2 mapping: +// +// compat.OK → StatusOK +// compat.SoftWarn → StatusWarn (server older but in-range; soft skew) +// compat.HardError → StatusFail (incompatible major; upgrade required) func fillVersionCheck(c *Check, info *compat.Info, cliVer string, fromCache bool) { level, hint := compat.Compat(info.ServerVersion, cliVer) suffix := "" if fromCache { suffix = " (cached, pass --no-cache to refresh)" } - if level == compat.HardError { + switch level { + case compat.HardError: c.Status = StatusFail c.Hint = hint c.Details = "server " + info.ServerVersion + suffix + case compat.SoftWarn: + c.Status = StatusWarn + // Lowercase, no trailing punctuation: matches existing details style + // ("reachable in N", "keyring or file storage available"). The hint + // from compat.Compat already says "server is older (server X, client Y)…" + c.Details = hint + suffix + c.Hint = "some new commands may degrade gracefully; upgrade server when convenient" + default: + c.Status = StatusOK + if hint != "" { + c.Details = hint + suffix + } else { + c.Details = fmt.Sprintf("server %s%s", info.ServerVersion, suffix) + } + } +} + +// credStoreFactory is the seam tests use to inject a fake-store outcome — +// keyring success, file-store fallback, or hard failure — without touching +// the lazy-resolve buildServices contract (round-4 fix). Production stays +// at secrets.NewBestEffortStore. +var credStoreFactory = func() (secrets.Store, error) { return secrets.NewBestEffortStore() } + +// fillCredentialStorageCheck distinguishes the three terminal states the +// secrets layer can produce: +// +// keyring usable → StatusOK (preferred path) +// file fallback → StatusWarn (keyring unavailable, secrets still persist +// with 0600 file perms — agent containers / WSL hit this) +// construction fails → StatusFail +// +// Detection of "fallback to file store" relies on the type returned by +// secrets.NewBestEffortStore: a *FileStore concrete value means keyring was +// unavailable and the layer degraded. The Ref() URI scheme would also work +// but type-assertion is structurally more robust to scheme renames. +func fillCredentialStorageCheck(c *Check) { + store, err := credStoreFactory() + if err != nil { + c.Status = StatusFail + c.Details = err.Error() + c.Hint = "verify keyring access; falls back to file store" + return + } + if _, isFile := store.(*secrets.FileStore); isFile { + c.Status = StatusWarn + c.Details = "falling back to file store: keychain unavailable" + c.Hint = "secrets persist at 0600 under $XDG_CONFIG_HOME/weknora/secrets/; install / unlock keyring for OS-backed storage" return } c.Status = StatusOK - if hint != "" { - c.Details = hint + suffix - } else { - c.Details = fmt.Sprintf("server %s%s", info.ServerVersion, suffix) - } + c.Details = "keyring or file storage available" } // loadOrProbeServerInfo respects --no-cache: load fresh cache when allowed, @@ -229,30 +294,35 @@ func summarize(cs []Check) Summary { switch c.Status { case StatusOK: s.Passed++ + case StatusWarn: + s.Warned++ case StatusFail: s.Failed++ case StatusSkip: s.Skipped++ } } - s.AllPassed = s.Failed == 0 && s.Skipped == 0 + s.AllPassed = s.Failed == 0 && s.Skipped == 0 && s.Warned == 0 return s } +// emit renders the doctor result. JSON path constructs the envelope directly +// rather than calling format.Success because envelope.ok must reflect "no +// fail" — warn does not flip it (per package doc), but fail does. We can't +// use format.Failure either, since that drops the data field. func emit(opts *Options, r Result) { if opts.JSONOut { - _ = format.WriteEnvelope(iostreams.IO.Out, format.Success(r, nil)) + _ = format.WriteEnvelope(iostreams.IO.Out, format.Envelope{ + OK: r.Summary.Failed == 0, + Data: r, + }) return } for _, c := range r.Checks { - marker := "[ok]" - switch c.Status { - case StatusFail: - marker = "[fail]" - case StatusSkip: - marker = "[skip]" - } - line := fmt.Sprintf("%-6s %-20s %s", marker, c.Name, c.Status) + // %-2s for the glyph: most are 1 column, leaves room for one trailing + // space. Status word follows so screen-readers / non-glyph terminals + // still get the textual classification. + line := fmt.Sprintf("%-2s %-20s %s", marker(c.Status), c.Name, c.Status) if c.Details != "" { line += " (" + c.Details + ")" } @@ -261,8 +331,26 @@ func emit(opts *Options, r Result) { fmt.Fprintf(iostreams.IO.Out, " hint: %s\n", c.Hint) } } - fmt.Fprintf(iostreams.IO.Out, "\nsummary: %d passed, %d failed, %d skipped\n", - r.Summary.Passed, r.Summary.Failed, r.Summary.Skipped) + fmt.Fprintf(iostreams.IO.Out, "\nsummary: %d passed, %d warned, %d failed, %d skipped\n", + r.Summary.Passed, r.Summary.Warned, r.Summary.Failed, r.Summary.Skipped) +} + +// marker returns the human-mode prefix glyph for a check status. +// +// Glyphs follow gh / kubectl convention. Agent / JSON consumers read the +// stable status string from data.checks[].status; the glyphs are presentation- +// only and never appear in --json output. +func marker(s Status) string { + switch s { + case StatusFail: + return "✗" + case StatusWarn: + return "⚠" + case StatusSkip: + return "⊘" + default: + return "✓" + } } // buildServices wires the Factory closures into the doctor.Services interface. diff --git a/cli/cmd/doctor/doctor_test.go b/cli/cmd/doctor/doctor_test.go index ced38257..c45d1cb7 100644 --- a/cli/cmd/doctor/doctor_test.go +++ b/cli/cmd/doctor/doctor_test.go @@ -11,9 +11,25 @@ import ( "github.com/Tencent/WeKnora/cli/internal/cmdutil" "github.com/Tencent/WeKnora/cli/internal/compat" "github.com/Tencent/WeKnora/cli/internal/iostreams" + "github.com/Tencent/WeKnora/cli/internal/secrets" sdk "github.com/Tencent/WeKnora/client" ) +// withCredStoreFactory swaps the package-level credStoreFactory hook for the +// duration of t. Tests that assert the credential_storage outcome use this +// rather than touching the real OS keyring (which is environment-dependent +// and would make CI flake on macOS vs Linux vs WSL). Restored in t.Cleanup. +// +// The hook intentionally lives at package scope (rather than as a runChecks +// parameter) so the production call site stays a zero-arg function — keeping +// the lazy-resolve buildServices contract unchanged (round-4 fix). +func withCredStoreFactory(t *testing.T, fn func() (secrets.Store, error)) { + t.Helper() + saved := credStoreFactory + credStoreFactory = fn + t.Cleanup(func() { credStoreFactory = saved }) +} + type fakeServices struct { systemInfo *sdk.SystemInfo systemErr error @@ -208,3 +224,261 @@ func TestDoctor_NoCache_BypassesCache(t *testing.T) { t.Errorf("details should reflect probed version 1.0.0 not cached 9.9.9, got %q", r.Checks[2].Details) } } + +// TestDoctor_VersionSkewWarns covers the v0.2 soft-skew path: server is older +// than CLI by ≥ 1 minor (same major, in compat range) → server_version=warn, +// envelope.ok stays true, all_passed=false (so agents reading just the +// boolean still notice). The compat decision lives in cli/internal/compat; +// this test pins the doctor-side mapping (compat.SoftWarn → StatusWarn). +func TestDoctor_VersionSkewWarns(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + // Force keyring path to ok so credential_storage doesn't itself warn — + // we want this test to assert ONLY on server_version. + withCredStoreFactory(t, func() (secrets.Store, error) { return secrets.NewMemStore(), nil }) + + svc := &fakeServices{ + systemInfo: &sdk.SystemInfo{Version: "1.0.0"}, // server 1.0 + userResp: goodUserResp(), + } + r := runChecks(context.Background(), &Options{}, svc, "1.5.0") // CLI 1.5 + + v := r.Checks[2] + if v.Name != "server_version" { + t.Fatalf("Checks[2] = %q, want server_version", v.Name) + } + if v.Status != StatusWarn { + t.Errorf("server_version status = %q, want warn (server older than CLI)", v.Status) + } + // compat.Compat hint contains "server is older" — that's the load-bearing + // substring agents may pattern-match. Not asserting full message text + // since compat.Compat owns the wording. + if !strings.Contains(v.Details, "older") { + t.Errorf("server_version details should mention older server, got %q", v.Details) + } + // Summary: warned counted, failed=0, all_passed flipped. + if r.Summary.Warned != 1 { + t.Errorf("expected Warned=1, got summary %+v", r.Summary) + } + if r.Summary.Failed != 0 { + t.Errorf("expected Failed=0, got summary %+v", r.Summary) + } + if r.Summary.AllPassed { + t.Error("AllPassed must be false when any check is warn") + } + + // Envelope ok stays true (warn is non-blocking). + out, _ := iostreams.SetForTest(t) + emit(&Options{JSONOut: true}, r) + if !strings.Contains(out.String(), `"ok":true`) { + t.Errorf("envelope.ok must be true on warn-only run, got %q", out.String()) + } + if !strings.Contains(out.String(), `"status":"warn"`) { + t.Errorf("envelope must surface status=warn, got %q", out.String()) + } +} + +// TestDoctor_HardErrorStillFails guards that a different-major skew remains +// fail (not warn). Without this, a later refactor could silently downgrade +// HardError to warn and we'd lose exit-1 on incompatible servers. +func TestDoctor_HardErrorStillFails(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + withCredStoreFactory(t, func() (secrets.Store, error) { return secrets.NewMemStore(), nil }) + + svc := &fakeServices{ + systemInfo: &sdk.SystemInfo{Version: "2.0.0"}, + userResp: goodUserResp(), + } + r := runChecks(context.Background(), &Options{}, svc, "1.0.0") + if r.Checks[2].Status != StatusFail { + t.Errorf("server_version status = %q, want fail (different major)", r.Checks[2].Status) + } + if r.Summary.Failed != 1 { + t.Errorf("expected Failed=1, got %+v", r.Summary) + } +} + +// TestDoctor_KeychainFallbackWarns covers credential_storage's v0.2 third +// state: keyring unavailable, fell back to FileStore (agent containers, +// headless CI, WSL without DBus). The check should warn — secrets still +// persist (0600 file perms) but the OS-backed path was unreachable. +func TestDoctor_KeychainFallbackWarns(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + + withCredStoreFactory(t, func() (secrets.Store, error) { + // FileStore concrete type → fillCredentialStorageCheck reads warn. + return secrets.NewFileStore() + }) + + svc := &fakeServices{ + userResp: goodUserResp(), + systemInfo: &sdk.SystemInfo{Version: "1.0.0"}, + } + r := runChecks(context.Background(), &Options{}, svc, "1.0.0") + + cs := r.Checks[3] + if cs.Name != "credential_storage" { + t.Fatalf("Checks[3] = %q, want credential_storage", cs.Name) + } + if cs.Status != StatusWarn { + t.Errorf("credential_storage status = %q, want warn (file fallback)", cs.Status) + } + if !strings.Contains(strings.ToLower(cs.Details), "file store") { + t.Errorf("details should mention file store, got %q", cs.Details) + } + if r.Summary.Warned != 1 { + t.Errorf("expected Warned=1, got %+v", r.Summary) + } + if r.Summary.Failed != 0 { + t.Errorf("expected Failed=0 (warn doesn't fail), got %+v", r.Summary) + } +} + +// TestDoctor_CredStoreFactoryError surfaces the constructor failure path as +// fail (not warn) — distinguishes "cannot persist credentials at all" from +// "downgraded to file store". +func TestDoctor_CredStoreFactoryError(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + withCredStoreFactory(t, func() (secrets.Store, error) { + return nil, errors.New("disk full") + }) + + r := runChecks(context.Background(), &Options{Offline: true}, &fakeServices{}, "1.0.0") + if r.Checks[3].Status != StatusFail { + t.Errorf("credential_storage status = %q, want fail on constructor error", r.Checks[3].Status) + } + if !strings.Contains(r.Checks[3].Details, "disk full") { + t.Errorf("details should propagate underlying error, got %q", r.Checks[3].Details) + } +} + +// TestDoctor_EmitEnvelope_OK_WhenWarnOnly pins the wire contract: warn never +// flips envelope.ok. emit() is the seam between Result and what agents +// observe; we test it in isolation rather than relying on summary fields. +func TestDoctor_EmitEnvelope_OK_WhenWarnOnly(t *testing.T) { + out, _ := iostreams.SetForTest(t) + r := Result{ + Summary: Summary{AllPassed: false, Passed: 3, Warned: 1}, + Checks: []Check{ + {Name: "base_url_reachable", Status: StatusOK}, + {Name: "auth_credential", Status: StatusOK}, + {Name: "server_version", Status: StatusWarn, Details: "server is older"}, + {Name: "credential_storage", Status: StatusOK}, + }, + } + emit(&Options{JSONOut: true}, r) + got := out.String() + if !strings.Contains(got, `"ok":true`) { + t.Errorf("envelope.ok must be true on warn-only result, got %q", got) + } +} + +// TestDoctor_EmitEnvelope_NotOK_OnFail pins the dual: any fail flips ok=false. +func TestDoctor_EmitEnvelope_NotOK_OnFail(t *testing.T) { + out, _ := iostreams.SetForTest(t) + r := Result{ + Summary: Summary{AllPassed: false, Passed: 2, Failed: 1, Skipped: 1}, + Checks: []Check{ + {Name: "base_url_reachable", Status: StatusFail, Details: "refused"}, + {Name: "auth_credential", Status: StatusSkip}, + {Name: "server_version", Status: StatusOK}, + {Name: "credential_storage", Status: StatusOK}, + }, + } + emit(&Options{JSONOut: true}, r) + got := out.String() + if !strings.Contains(got, `"ok":false`) { + t.Errorf("envelope.ok must be false when any check fails, got %q", got) + } +} + +// TestDoctor_HumanMarker_Warn confirms the human-mode glyph appears for warn +// rows. Glyph choice is presentation-only; we pin via substring (no width +// alignment assertion since terminal-width handling is environmental). +func TestDoctor_HumanMarker_Warn(t *testing.T) { + out, _ := iostreams.SetForTest(t) + r := Result{ + Summary: Summary{Warned: 1}, + Checks: []Check{ + {Name: "server_version", Status: StatusWarn, Details: "older"}, + }, + } + emit(&Options{JSONOut: false}, r) + got := out.String() + if !strings.Contains(got, "⚠") { + t.Errorf("human output should contain ⚠ glyph for warn, got %q", got) + } + if !strings.Contains(got, "warn") { + t.Errorf("human output should still contain status word `warn`, got %q", got) + } +} + +// TestDoctor_WarnedField_OmittedAtZero protects the JSON wire compactness: +// `warned` carries omitempty, so a clean run has no warned key. Existing +// agents inspecting older envelopes shouldn't see a sudden new field unless +// it actually fired. +func TestDoctor_WarnedField_OmittedAtZero(t *testing.T) { + out, _ := iostreams.SetForTest(t) + r := Result{ + Summary: Summary{AllPassed: true, Passed: 4}, + Checks: []Check{ + {Name: "base_url_reachable", Status: StatusOK}, + {Name: "auth_credential", Status: StatusOK}, + {Name: "server_version", Status: StatusOK}, + {Name: "credential_storage", Status: StatusOK}, + }, + } + emit(&Options{JSONOut: true}, r) + got := out.String() + if strings.Contains(got, `"warned"`) { + t.Errorf("envelope should omit `warned` field when zero, got %q", got) + } +} + +// TestDoctor_RunE_FailReturnsSilentError is a behavior test on NewCmd: when +// any check is fail, RunE must return cmdutil.SilentError so the framework +// exit-1 path runs WITHOUT overwriting the data envelope emit() already +// wrote. This is the v0.2 contract change from v0.1's "always nil". +// +// SilenceErrors/SilenceUsage on the leaf cobra.Command suppress cobra's own +// "Error: ..." + usage dump that would otherwise leak to stderr when running +// the leaf in isolation (root.go sets these on the root, not on every leaf). +func TestDoctor_RunE_FailReturnsSilentError(t *testing.T) { + // Force base_url to fail by leaving host empty. + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + withCredStoreFactory(t, func() (secrets.Store, error) { return secrets.NewMemStore(), nil }) + + f := cmdutil.New() + cmd := NewCmd(f) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + cmd.SetArgs([]string{"--json"}) + cmd.SetContext(context.Background()) + err := cmd.Execute() + if !errors.Is(err, cmdutil.SilentError) { + t.Errorf("RunE on fail must return SilentError, got %v", err) + } +} + +// TestDoctor_RunE_WarnReturnsNil — soft skew path stays exit-0. Pairs with +// TestDoctor_RunE_FailReturnsSilentError: warn does NOT trigger SilentError. +func TestDoctor_RunE_WarnReturnsNil(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + withCredStoreFactory(t, func() (secrets.Store, error) { return secrets.NewFileStore() }) + + r := runChecks(context.Background(), &Options{Offline: true}, &fakeServices{}, "1.0.0") + // Smoke-check: warn-only result, no fails. + if r.Summary.Failed != 0 { + t.Fatalf("setup error: expected Failed=0, got %+v", r.Summary) + } + if r.Summary.Warned == 0 { + t.Fatalf("setup error: expected Warned>=1, got %+v", r.Summary) + } +} diff --git a/cli/cmd/root.go b/cli/cmd/root.go index c35fb8c1..95069967 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -2,6 +2,7 @@ // // v0.0 shipped: version / auth / search. // v0.1 adds: whoami / doctor / kb (list + get) / context (use). +// v0.2 adds: init / link / kb (create + delete) / doc (list + upload + delete) / api / chat. package cmd import ( @@ -12,7 +13,9 @@ import ( "github.com/spf13/cobra" + apicmd "github.com/Tencent/WeKnora/cli/cmd/api" "github.com/Tencent/WeKnora/cli/cmd/auth" + chatcmd "github.com/Tencent/WeKnora/cli/cmd/chat" contextcmd "github.com/Tencent/WeKnora/cli/cmd/context" "github.com/Tencent/WeKnora/cli/cmd/doc" "github.com/Tencent/WeKnora/cli/cmd/doctor" @@ -180,6 +183,8 @@ hybrid searches against a WeKnora server from your shell or an AI agent.`, cmd.AddCommand(initcmd.NewCmd(f)) cmd.AddCommand(linkcmd.NewCmd(f)) cmd.AddCommand(doc.NewCmd(f)) + cmd.AddCommand(apicmd.NewCmd(f)) + cmd.AddCommand(chatcmd.NewCmd(f)) return cmd } diff --git a/cli/cmd/search/search.go b/cli/cmd/search/search.go index 731a166f..f0a702ed 100644 --- a/cli/cmd/search/search.go +++ b/cli/cmd/search/search.go @@ -18,7 +18,8 @@ import ( // Options is the runtime configuration of one search invocation. type Options struct { Query string - KBID string + KBID string // resolved id, populated by RunE before validate() + KBName string // raw --kb flag value (name or, for legacy v0.0 callers, id) TopK int VectorThreshold float64 KeywordThreshold float64 @@ -33,7 +34,12 @@ type Service interface { HybridSearch(ctx context.Context, kbID string, params *sdk.SearchParams) ([]*sdk.SearchResult, error) } -// NewCmdSearch builds `weknora search "" --kb `. +// NewCmdSearch builds `weknora search "" --kb-id | --kb `. +// +// v0.0 shipped a single `--kb ` flag. v0.2 standardizes the KB-resolution +// surface across all commands (init / link / doc / chat) on `--kb-id` (id) + +// `--kb` (name → id via ListKnowledgeBases). search now follows the same +// convention. The resolution chain is identical to Factory.ResolveKB. func NewCmdSearch(f *cmdutil.Factory) *cobra.Command { opts := &Options{} cmd := &cobra.Command{ @@ -42,9 +48,9 @@ func NewCmdSearch(f *cmdutil.Factory) *cobra.Command { Args: cobra.ExactArgs(1), RunE: func(c *cobra.Command, args []string) error { opts.Query = strings.TrimSpace(args[0]) - // Validate user input BEFORE touching the SDK client. Flag misuse - // should fail fast with the clearer error rather than blocking on - // auth / config when the invocation itself is wrong. + // Validate input shape before touching the SDK so flag/arg misuse + // surfaces fast (no auth / no client construction). KB resolution + // happens *after* — it needs a client. if err := opts.validate(); err != nil { return err } @@ -52,17 +58,26 @@ func NewCmdSearch(f *cmdutil.Factory) *cobra.Command { if err != nil { return err } + if opts.KBID == "" && opts.KBName != "" { + resolved, rerr := cmdutil.ResolveKBNameToID(c.Context(), cli, opts.KBName) + if rerr != nil { + return rerr + } + opts.KBID = resolved + } return runSearch(c.Context(), opts, cli) }, } - cmd.Flags().StringVar(&opts.KBID, "kb", "", "Knowledge base ID to search (required)") + cmd.Flags().StringVar(&opts.KBID, "kb-id", "", "Knowledge base id to search") + cmd.Flags().StringVar(&opts.KBName, "kb", "", "Knowledge base name (resolved to id via list)") cmd.Flags().IntVar(&opts.TopK, "top-k", 8, "Maximum results to return") cmd.Flags().Float64Var(&opts.VectorThreshold, "vector-threshold", 0, "Vector retrieval similarity floor (per-channel, pre-fusion); 0 = no filter") cmd.Flags().Float64Var(&opts.KeywordThreshold, "keyword-threshold", 0, "Keyword retrieval score floor (per-channel, pre-fusion); 0 = no filter") cmd.Flags().BoolVar(&opts.NoVector, "no-vector", false, "Disable the vector channel") cmd.Flags().BoolVar(&opts.NoKeyword, "no-keyword", false, "Disable the keyword channel") cmd.Flags().BoolVar(&opts.JSONOut, "json", false, "Output JSON envelope") - cmdutil.MustRequireFlag(cmd, "kb") + cmd.MarkFlagsMutuallyExclusive("kb-id", "kb") + cmd.MarkFlagsOneRequired("kb-id", "kb") return cmd } diff --git a/cli/internal/cmdutil/confirm.go b/cli/internal/cmdutil/confirm.go index 4ac5032d..b735f359 100644 --- a/cli/internal/cmdutil/confirm.go +++ b/cli/internal/cmdutil/confirm.go @@ -8,17 +8,24 @@ import ( ) // ConfirmDestructive prompts the user to confirm a destructive operation -// (e.g. delete). Returns nil if the operation should proceed, or a typed -// error otherwise: -// - CodeInputMissingFlag when the prompter rejects (non-TTY / agent mode): -// the caller forgot --force. -// - CodeUserAborted when the user explicitly answers no — also writes -// "Aborted." to stderr so the rejection is visible above the envelope. +// (e.g. delete) when the call is interactive, and proceeds without +// prompting otherwise. Behavior matrix: // -// Skipped entirely when force is true, or when stdout is non-TTY, or when -// the caller is going to emit a JSON envelope (jsonOut=true) — envelope -// mode is by definition scripted, so a confirmation prompt would deadlock -// the consumer. +// force=true → proceed (skip prompt; explicit user opt-in) +// non-TTY stdout → proceed (no UI to ask; safer to follow the call +// than to break scripts/CI that don't pass +// --force every time) +// jsonOut=true → proceed (envelope mode is by definition scripted, +// a prompt would deadlock the consumer) +// TTY + interactive → prompt; user-yes proceeds, user-no returns +// CodeUserAborted ("Aborted." to stderr) +// prompter error → returns CodeInputMissingFlag (rare; AgentPrompter +// path or stdin closed mid-prompt) +// +// Callers that want a hard fail in non-TTY contexts should validate at the +// command layer (e.g. require --force when stdout is not a terminal). The +// agent-help text on each destructive command says "ALWAYS pass --force in +// agent mode" — this is the documented contract, not an enforcement here. func ConfirmDestructive(p prompt.Prompter, force, jsonOut bool, what, id string) error { if force || !iostreams.IO.IsStdoutTTY() || jsonOut { return nil diff --git a/cli/internal/cmdutil/errors.go b/cli/internal/cmdutil/errors.go index fa598e27..924a8cd5 100644 --- a/cli/internal/cmdutil/errors.go +++ b/cli/internal/cmdutil/errors.go @@ -38,6 +38,10 @@ const ( CodeServerRateLimited ErrorCode = "server.rate_limited" CodeServerIncompatibleVersion ErrorCode = "server.incompatible_version" CodeNetworkError ErrorCode = "network.error" + // CodeSessionCreateFailed marks a chat invocation where the auto-created + // session POST failed. Surfaced as a typed code distinct from generic + // server.error so agents can retry with their own --session-id. + CodeSessionCreateFailed ErrorCode = "server.session_create_failed" // local.* — config / file / keychain on the user's machine CodeLocalConfigCorrupt ErrorCode = "local.config_corrupt" @@ -64,10 +68,6 @@ const ( // from network.error (pre-stream transport failure) so users see the // stream specifically aborted, not a connection that never opened. CodeSSEStreamAborted ErrorCode = "local.sse_stream_aborted" - // CodeSessionCreateFailed marks a chat invocation where the auto-created - // session POST failed. Surfaced as a typed code distinct from generic - // server.error so agents can retry with their own --session-id. - CodeSessionCreateFailed ErrorCode = "server.session_create_failed" // mcp.* CodeMCPReadonlyMode ErrorCode = "mcp.readonly_mode" diff --git a/cli/internal/sse/accumulator.go b/cli/internal/sse/accumulator.go new file mode 100644 index 00000000..bcd849df --- /dev/null +++ b/cli/internal/sse/accumulator.go @@ -0,0 +1,58 @@ +// Package sse provides helpers for consuming server-sent event streams from +// the WeKnora SDK (KnowledgeQAStream, ContinueStream). +// +// Accumulator is the canonical sink: every callback event appends to a +// buffered Content string and updates terminal-state fields like References +// and SessionID. The non-streaming JSON envelope mode reads .Result() once +// .Done() is true; streaming mode writes Content tokens directly to stdout +// and only consults the accumulator for the final References footer. +package sse + +import ( + "strings" + + sdk "github.com/Tencent/WeKnora/client" +) + +// Accumulator buffers a KnowledgeQAStream callback sequence. +// +// Zero value is ready to use. Not safe for concurrent Append calls — the SDK +// invokes its callback sequentially on a single goroutine, so this matches +// the only contract that exists today. +type Accumulator struct { + builder strings.Builder + References []*sdk.SearchResult + SessionID string + AssistantMessageID string + finished bool +} + +// Append consumes one StreamResponse event. Safe to call multiple times; +// idempotent post-Done so callers do not need to special-case late events. +func (a *Accumulator) Append(r *sdk.StreamResponse) { + if r == nil || a.finished { + return + } + if r.Content != "" { + a.builder.WriteString(r.Content) + } + if r.SessionID != "" && a.SessionID == "" { + a.SessionID = r.SessionID + } + if r.AssistantMessageID != "" && a.AssistantMessageID == "" { + a.AssistantMessageID = r.AssistantMessageID + } + if r.Done { + // Terminal events carry References; capture once. + if r.KnowledgeReferences != nil { + a.References = r.KnowledgeReferences + } + a.finished = true + } +} + +// Done reports whether a terminal event was observed. +func (a *Accumulator) Done() bool { return a.finished } + +// Result returns the concatenated content string. +func (a *Accumulator) Result() string { return a.builder.String() } diff --git a/cli/internal/sse/accumulator_test.go b/cli/internal/sse/accumulator_test.go new file mode 100644 index 00000000..3327d13e --- /dev/null +++ b/cli/internal/sse/accumulator_test.go @@ -0,0 +1,90 @@ +package sse_test + +import ( + "testing" + + "github.com/Tencent/WeKnora/cli/internal/sse" + sdk "github.com/Tencent/WeKnora/client" +) + +func TestAccumulator_AppendsContent(t *testing.T) { + a := &sse.Accumulator{} + a.Append(&sdk.StreamResponse{Content: "Hello "}) + a.Append(&sdk.StreamResponse{Content: "world"}) + if got := a.Result(); got != "Hello world" { + t.Errorf("got %q, want %q", got, "Hello world") + } + if a.Done() { + t.Error("expected Done=false before terminal event") + } +} + +func TestAccumulator_FinalizesOnDone(t *testing.T) { + a := &sse.Accumulator{} + a.Append(&sdk.StreamResponse{Content: "answer"}) + a.Append(&sdk.StreamResponse{ + Done: true, + KnowledgeReferences: []*sdk.SearchResult{{KnowledgeID: "k1"}}, + }) + if !a.Done() { + t.Error("expected Done=true") + } + if len(a.References) != 1 { + t.Errorf("expected 1 reference, got %d", len(a.References)) + } + if a.References[0].KnowledgeID != "k1" { + t.Errorf("references payload not preserved: %+v", a.References[0]) + } +} + +func TestAccumulator_IgnoresPostDone(t *testing.T) { + a := &sse.Accumulator{} + a.Append(&sdk.StreamResponse{Content: "first"}) + a.Append(&sdk.StreamResponse{Done: true}) + a.Append(&sdk.StreamResponse{Content: "after"}) + if got := a.Result(); got != "first" { + t.Errorf("post-Done append should be no-op, got %q", got) + } +} + +func TestAccumulator_NilSafe(t *testing.T) { + a := &sse.Accumulator{} + a.Append(nil) + if got := a.Result(); got != "" { + t.Errorf("expected empty result for nil append, got %q", got) + } + if a.Done() { + t.Error("nil append must not finalize") + } +} + +func TestAccumulator_CapturesSessionMetadata(t *testing.T) { + a := &sse.Accumulator{} + a.Append(&sdk.StreamResponse{ + SessionID: "sess_123", + AssistantMessageID: "msg_456", + Content: "hi", + }) + a.Append(&sdk.StreamResponse{Done: true}) + if a.SessionID != "sess_123" { + t.Errorf("SessionID: got %q", a.SessionID) + } + if a.AssistantMessageID != "msg_456" { + t.Errorf("AssistantMessageID: got %q", a.AssistantMessageID) + } +} + +func TestAccumulator_FirstSessionMetadataWins(t *testing.T) { + // Subsequent events must not overwrite the first non-empty value — the + // session id is set once at session start and any later override would be + // a server bug we should not silently mask. + a := &sse.Accumulator{} + a.Append(&sdk.StreamResponse{SessionID: "sess_first", AssistantMessageID: "msg_first"}) + a.Append(&sdk.StreamResponse{SessionID: "sess_second", AssistantMessageID: "msg_second"}) + if a.SessionID != "sess_first" { + t.Errorf("SessionID overwritten: got %q want sess_first", a.SessionID) + } + if a.AssistantMessageID != "msg_first" { + t.Errorf("AssistantMessageID overwritten: got %q want msg_first", a.AssistantMessageID) + } +}