From bb592a59a63f1910e7f3406544b58e1ac5e63f07 Mon Sep 17 00:00:00 2001 From: nullkey Date: Fri, 8 May 2026 16:42:17 +0800 Subject: [PATCH] feat(cli): contract test suite + dependabot (PR-8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cli/acceptance/contract/: envelope_test.go — 16 envelope golden cases (9 commands × {success/error variants}; 3 cases dropped with rationale: doctor.success non-offline has unstable timing detail; auth_login.* needs stdin/keyring scaffold deferred to v0.2; context_use.error needs leaf-local --json deferred to follow-up) errorcodes_test.go — single-direction AST scan of cli/cmd/ extracting first arg of cmdutil.NewError / cmdutil.Wrapf calls; ClassifyHTTPError dynamic-classify bridged via cmdutil.ClassifyHTTPErrorOutputs() per spec §4.3. testdata/envelopes/ — 16 JSON golden files helpers_test.go (PR-6 scaffold) extended: runCmd now wires cobra Out/Err sinks (version uses c.OutOrStdout) AND replicates cmd.Execute()'s error-envelope path so error-case goldens are populated. Without this, every error scenario's golden was 0 bytes. cli/cmd/root.go: mapCobraError → MapCobraError, wantsJSONOutput → WantsJSONOutput (exported so the contract test helper can replicate Execute()'s envelope-printing path without calling Execute() itself). root_test.go updated to use new exported names. .github/dependabot.yml (新增):gomod /cli + github-actions weekly,gh-style ignore semver-major to avoid noise. Open-source dependency safety,independent of release cadence. v0.1 不发布到任何分发平台 (release infra 推迟到发布窗口 milestone)。 --- .gitattributes | 8 +- .github/dependabot.yml | 22 ++ cli/acceptance/contract/envelope_test.go | 372 ++++++++++++++++++ cli/acceptance/contract/errorcodes_test.go | 191 +++++++++ cli/acceptance/contract/helpers_test.go | 35 +- ...uth_status.error_auth_unauthenticated.json | 1 + .../envelopes/auth_status.success.json | 1 + .../envelopes/context_use.success.json | 1 + .../envelopes/doctor.error_network.json | 1 + .../envelopes/doctor.success_offline.json | 1 + .../kb_get.error_resource_not_found.json | 1 + .../testdata/envelopes/kb_get.success.json | 1 + .../kb_list.error_auth_forbidden.json | 1 + .../testdata/envelopes/kb_list.success.json | 1 + .../envelopes/kb_list.success_empty.json | 1 + .../envelopes/search.error_input_invalid.json | 1 + .../search.error_resource_not_found.json | 1 + .../testdata/envelopes/search.success.json | 1 + .../testdata/envelopes/version.success.json | 1 + .../whoami.error_auth_unauthenticated.json | 1 + .../testdata/envelopes/whoami.success.json | 1 + cli/cmd/auth/auth_test.go | 5 +- cli/cmd/auth/login.go | 7 +- cli/cmd/context/use.go | 31 +- cli/cmd/context/use_test.go | 31 +- cli/cmd/doctor/doctor.go | 210 ++++++---- cli/cmd/doctor/doctor_test.go | 65 ++- cli/cmd/kb/list.go | 7 + cli/cmd/root.go | 56 ++- cli/cmd/root_test.go | 41 +- cli/internal/cmdutil/exit.go | 58 ++- cli/internal/cmdutil/factory.go | 23 +- cli/internal/cmdutil/factory_test.go | 44 +++ cli/internal/compat/cache.go | 29 +- cli/internal/config/config.go | 35 +- cli/internal/secrets/secrets.go | 11 +- cli/internal/xdg/xdg.go | 86 ++++ cli/internal/xdg/xdg_test.go | 59 +++ 38 files changed, 1245 insertions(+), 197 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 cli/acceptance/contract/envelope_test.go create mode 100644 cli/acceptance/contract/errorcodes_test.go create mode 100644 cli/acceptance/testdata/envelopes/auth_status.error_auth_unauthenticated.json create mode 100644 cli/acceptance/testdata/envelopes/auth_status.success.json create mode 100644 cli/acceptance/testdata/envelopes/context_use.success.json create mode 100644 cli/acceptance/testdata/envelopes/doctor.error_network.json create mode 100644 cli/acceptance/testdata/envelopes/doctor.success_offline.json create mode 100644 cli/acceptance/testdata/envelopes/kb_get.error_resource_not_found.json create mode 100644 cli/acceptance/testdata/envelopes/kb_get.success.json create mode 100644 cli/acceptance/testdata/envelopes/kb_list.error_auth_forbidden.json create mode 100644 cli/acceptance/testdata/envelopes/kb_list.success.json create mode 100644 cli/acceptance/testdata/envelopes/kb_list.success_empty.json create mode 100644 cli/acceptance/testdata/envelopes/search.error_input_invalid.json create mode 100644 cli/acceptance/testdata/envelopes/search.error_resource_not_found.json create mode 100644 cli/acceptance/testdata/envelopes/search.success.json create mode 100644 cli/acceptance/testdata/envelopes/version.success.json create mode 100644 cli/acceptance/testdata/envelopes/whoami.error_auth_unauthenticated.json create mode 100644 cli/acceptance/testdata/envelopes/whoami.success.json create mode 100644 cli/internal/xdg/xdg.go create mode 100644 cli/internal/xdg/xdg_test.go diff --git a/.gitattributes b/.gitattributes index 526c8a38..671cc242 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,7 @@ -*.sh text eol=lf \ No newline at end of file +*.sh text eol=lf + +# Force LF on Go source + JSON golden fixtures so Windows checkouts +# (with default core.autocrlf=true) don't trip byte-equal comparisons in +# acceptance/contract envelope tests. +*.go text eol=lf +cli/acceptance/testdata/**/*.json text eol=lf diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..edecf2f0 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,22 @@ +# Dependabot configuration for the WeKnora repository. +# +# Mirrors gh CLI's pattern (https://github.com/cli/cli/blob/trunk/.github/dependabot.yml): +# scoped to the cli/ subdirectory Go module + GitHub Actions, weekly schedule, +# ignore semver-major to avoid noise. Major upgrades are routed through manual +# review. +# +# Spec ref: docs/superpowers/specs/2026-05-08-weknora-cli-v0.1-design.md §3.2 +version: 2 +updates: + - package-ecosystem: gomod + directory: "/cli" + schedule: + interval: weekly + ignore: + - dependency-name: "*" + update-types: ["version-update:semver-major"] + + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: weekly diff --git a/cli/acceptance/contract/envelope_test.go b/cli/acceptance/contract/envelope_test.go new file mode 100644 index 00000000..189f2286 --- /dev/null +++ b/cli/acceptance/contract/envelope_test.go @@ -0,0 +1,372 @@ +// cli/acceptance/contract/envelope_test.go +// +// Envelope contract test (PR-8 Task 18). Drives root cobra in-process for +// each scenario, captures stdout, and compares against a JSON golden file +// in cli/acceptance/testdata/envelopes/. +// +// Spec §4.1 lists 19 envelope scenarios. Implemented count: 16. +// +// Cases dropped (with reason): +// - doctor.success — non-offline path emits unstable +// timing ("reachable in 2ms"). +// Unit tests in cli/cmd/doctor +// cover the all-pass shape; +// doctor.success_offline is the +// deterministic sibling kept here. +// - auth_login.success — requires stdin pipe +// (--with-token) + keyring-aware +// Secrets store; helpers_test +// (PR-6) does not yet expose a +// stdin hook. Deferred to v0.2 e2e. +// - auth_login.error_auth_unauthenticated — same setup as above; deferred +// together. +// - context_use.error_local_context_not_found — `context use` has no --json +// flag in v0.1, so error path +// renders plain stderr. Pinning +// its envelope shape needs either +// a --json flag added to the leaf +// or a global --json. Deferred +// until that lands; the success +// case is golden-pinned (writes +// envelope unconditionally). +// +// All cases use leaf-positioned --json (e.g. `version --json`) instead of the +// `--json version` form sketched in the spec. v0.0–v0.1 implements --json as a +// per-leaf flag, not a global persistent flag — root-level --json is detected +// only as an error-envelope fallback (see argsRequestJSON in cmd/root.go). +package contract_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/Tencent/WeKnora/cli/internal/config" + sdk "github.com/Tencent/WeKnora/client" +) + +// envelopeCase declares one row in the contract matrix. Optional fields: +// server — mock /api/v1/* endpoints; nil means no network needed. +// preConfig — seed config.yaml under the per-test XDG_CONFIG_HOME (set by +// newTestFactory); use for cases like context use that read +// local state without an SDK round-trip. +// wantErr — true means the run is expected to exit non-zero. +type envelopeCase struct { + name string + args []string + server http.HandlerFunc + preConfig func(t *testing.T) + wantErr bool +} + +// envelopeCases enumerates every contract scenario whose envelope is golden- +// pinned. Order is illustrative (matches spec §4.1 mostly), not load-bearing. +var envelopeCases = []envelopeCase{ + // 1. version.success — pure local; no client touched. + { + name: "version.success", + args: []string{"version", "--json"}, + }, + + // 2-3. whoami — single SDK call to /api/v1/auth/me. + { + name: "whoami.success", + args: []string{"whoami", "--json"}, + server: whoamiOK, + }, + { + name: "whoami.error_auth_unauthenticated", + args: []string{"whoami", "--json"}, + server: always401, + wantErr: true, + }, + + // 4. doctor.success_offline — only credential_storage runs; the three + // network checks are skipped. Stable details + summary. + { + name: "doctor.success_offline", + args: []string{"doctor", "--offline", "--json"}, + server: doctorReachable, // ensures buildServices succeeds even if probed + }, + + // 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. + { + name: "doctor.error_network", + args: []string{"doctor", "--json"}, + server: alwaysServerError, + }, + + // 6-9. kb list / get — SDK paths /api/v1/knowledge-bases[/] + { + name: "kb_list.success", + args: []string{"kb", "list", "--json"}, + server: kbListTwo, + }, + { + name: "kb_list.success_empty", + args: []string{"kb", "list", "--json"}, + server: kbListEmpty, + }, + { + name: "kb_list.error_auth_forbidden", + args: []string{"kb", "list", "--json"}, + server: always403, + wantErr: true, + }, + { + name: "kb_get.success", + args: []string{"kb", "get", "kb1", "--json"}, + server: kbGetOne, + }, + { + name: "kb_get.error_resource_not_found", + args: []string{"kb", "get", "missing", "--json"}, + server: always404, + wantErr: true, + }, + + // 10-11. context use — pure local I/O against config.yaml. + { + name: "context_use.success", + args: []string{"context", "use", "production"}, + preConfig: func(t *testing.T) { + cfg := &config.Config{ + CurrentContext: "staging", + Contexts: map[string]config.Context{ + "staging": {Host: "https://staging.example.com"}, + "production": {Host: "https://prod.example.com"}, + }, + } + if err := config.Save(cfg); err != nil { + t.Fatalf("seed config: %v", err) + } + }, + }, + // (context_use.error_local_context_not_found dropped — see file header.) + + // 11-12. auth status — SDK /api/v1/auth/me, plus config inspection. + { + name: "auth_status.success", + args: []string{"auth", "status", "--json"}, + server: whoamiOK, + }, + { + name: "auth_status.error_auth_unauthenticated", + args: []string{"auth", "status", "--json"}, + server: always401, + wantErr: true, + }, + + // 14-16. search — top-level command, positional query, --kb required. + { + name: "search.success", + args: []string{"search", "query", "--kb=kb1", "--top-k=3", "--json"}, + server: searchTwoResults, + }, + { + name: "search.error_resource_not_found", + args: []string{"search", "query", "--kb=missing", "--json"}, + server: always404, + wantErr: true, + }, + { + // Mutex flag violation. Validation runs before f.Client(), so the + // 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"}, + wantErr: true, + }, +} + +// TestEnvelopeGolden is the matrix-runner. Cases are sequential (the +// iostreams singleton swap inside helpers.runCmd is package-global; t.Parallel +// 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 + if tc.server != nil { + ts = httptest.NewServer(tc.server) + defer ts.Close() + mockClient = sdk.NewClient(ts.URL) + } + f := newTestFactory(t, ts, mockClient) + if tc.preConfig != nil { + tc.preConfig(t) + } + stdout, stderr, exit := runCmd(t, f, tc.args...) + if tc.wantErr && exit == 0 { + t.Errorf("expected non-zero exit, got 0; stdout=%q stderr=%q", stdout, stderr) + } + if !tc.wantErr && exit != 0 { + t.Errorf("unexpected non-zero exit %d; stdout=%q stderr=%q", exit, stdout, stderr) + } + path := filepath.Join("..", "testdata", "envelopes", tc.name+".json") + assertGolden(t, []byte(stdout), path) + }) + } +} + +// --------------------------------------------------------------------------- +// HTTP fixtures +// +// Handlers are intentionally permissive on path matching (HasSuffix) so they +// work whether the SDK adds the /api/v1 prefix or not. The SDK pins the +// /api/v1 prefix today; the suffix match keeps the fixtures resilient to +// future route renames as long as the leaf path stays stable. + +// fixedTime is the deterministic timestamp embedded in KnowledgeBase fixtures. +// time.Time marshals to RFC3339; using a fixed value keeps the golden stable. +var fixedTime = time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) + +func whoamiOK(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/auth/me") { + w.WriteHeader(http.StatusNotFound) + return + } + resp := sdk.CurrentUserResponse{Success: true} + resp.Data.User = &sdk.AuthUser{ID: "usr_abc", Email: "user@example.com", TenantID: 42} + resp.Data.Tenant = &sdk.AuthTenant{ID: 42, Name: "Acme"} + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} + +func always401(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"unauthenticated"}`)) +} + +func always403(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"forbidden"}`)) +} + +func always404(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"not found"}`)) +} + +func alwaysServerError(w http.ResponseWriter, _ *http.Request) { + // 5xx triggers PingBaseURL failure path and SDK transport error. + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`internal error`)) +} + +// doctorReachable serves /health 200 (so PingBaseURL would succeed if it +// were called). doctor.success_offline still skips ping, so this handler +// is a no-op for that case but keeps buildServices on a happy path. +func doctorReachable(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) +} + +func kbListTwo(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/knowledge-bases") { + w.WriteHeader(http.StatusNotFound) + return + } + resp := sdk.KnowledgeBaseListResponse{ + Success: true, + Data: []sdk.KnowledgeBase{ + { + ID: "kb1", + Name: "Onboarding Docs", + TenantID: 42, + EmbeddingModelID: "text-embedding-3", + CreatedAt: fixedTime, + UpdatedAt: fixedTime, + KnowledgeCount: 5, + ChunkCount: 128, + }, + { + ID: "kb2", + Name: "API Reference", + TenantID: 42, + EmbeddingModelID: "text-embedding-3", + CreatedAt: fixedTime, + UpdatedAt: fixedTime, + KnowledgeCount: 12, + ChunkCount: 340, + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} + +func kbListEmpty(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/knowledge-bases") { + w.WriteHeader(http.StatusNotFound) + return + } + resp := sdk.KnowledgeBaseListResponse{Success: true, Data: []sdk.KnowledgeBase{}} + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} + +func kbGetOne(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/knowledge-bases/kb1") { + w.WriteHeader(http.StatusNotFound) + return + } + resp := sdk.KnowledgeBaseResponse{ + Success: true, + Data: sdk.KnowledgeBase{ + ID: "kb1", + Name: "Onboarding Docs", + Description: "Internal onboarding handbook", + TenantID: 42, + EmbeddingModelID: "text-embedding-3", + CreatedAt: fixedTime, + UpdatedAt: fixedTime, + KnowledgeCount: 5, + ChunkCount: 128, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} + +func searchTwoResults(w http.ResponseWriter, r *http.Request) { + if !strings.Contains(r.URL.Path, "/knowledge-bases/kb1/hybrid-search") { + w.WriteHeader(http.StatusNotFound) + return + } + resp := sdk.HybridSearchResponse{ + Success: true, + Data: []*sdk.SearchResult{ + { + ID: "chunk-1", + Content: "first chunk content", + KnowledgeID: "doc-1", + ChunkIndex: 0, + KnowledgeTitle: "Doc 1", + Score: 0.92, + MatchType: sdk.MatchTypeVector, + }, + { + ID: "chunk-2", + Content: "second chunk content", + KnowledgeID: "doc-2", + ChunkIndex: 1, + KnowledgeTitle: "Doc 2", + Score: 0.81, + MatchType: sdk.MatchTypeKeyword, + }, + }, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} diff --git a/cli/acceptance/contract/errorcodes_test.go b/cli/acceptance/contract/errorcodes_test.go new file mode 100644 index 00000000..f1332950 --- /dev/null +++ b/cli/acceptance/contract/errorcodes_test.go @@ -0,0 +1,191 @@ +// cli/acceptance/contract/errorcodes_test.go +package contract_test + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Tencent/WeKnora/cli/internal/cmdutil" +) + +// TestAllReferencedCodesAreRegistered scans cli/cmd/ for every literal use of +// cmdutil.NewError(codeXxx, ...) and cmdutil.Wrapf(codeXxx, ...) and verifies +// that codeXxx is registered in cmdutil.AllCodes(). +// +// ClassifyHTTPError is dynamic: callers pass cmdutil.ClassifyHTTPError(err) +// as the first arg. We bridge that via cmdutil.ClassifyHTTPErrorOutputs(), +// which returns every code that the switch can return — those are added to +// the registered set so the AST scanner doesn't false-positive on them. +// +// Limitations (documented in spec §4.3): +// - Only literal cmdutil.CodeXxx idents are detected; codes assigned to +// a local variable then passed are NOT scanned (rare pattern). +// - cmdutil.ClassifyHTTPError(...) call expressions are skipped — bridge +// covers them. +// - v0.x does not enforce a baseline diff (spec ADR-6b); v0.9 will. +func TestAllReferencedCodesAreRegistered(t *testing.T) { + registered := make(map[cmdutil.ErrorCode]struct{}) + for _, c := range cmdutil.AllCodes() { + registered[c] = struct{}{} + } + // Bridge: ClassifyHTTPError returns these dynamically; treat them as + // "registered" for the purposes of the AST scan since callers pass the + // function call (not a literal ident) as arg0. + for _, c := range cmdutil.ClassifyHTTPErrorOutputs() { + registered[c] = struct{}{} + } + + referenced := scanCmdDir(t, "../../cmd") + + for code, locs := range referenced { + if _, ok := registered[code]; !ok { + for _, loc := range locs { + t.Errorf("%s: error code %q referenced but not registered in cmdutil.AllCodes()", loc, code) + } + } + } +} + +// scanCmdDir walks cli/cmd/** for *.go files (excluding tests) and collects +// every literal cmdutil.CodeXxx ident passed as the first arg to +// cmdutil.NewError / cmdutil.Wrapf. +// +// Returns map keyed by ErrorCode value (the const's underlying string), with +// a slice of source positions for nicer error messages. +func scanCmdDir(t *testing.T, dir string) map[cmdutil.ErrorCode][]token.Position { + t.Helper() + out := make(map[cmdutil.ErrorCode][]token.Position) + fset := token.NewFileSet() + walkAndScan(t, fset, dir, out) + return out +} + +func walkAndScan(t *testing.T, fset *token.FileSet, root string, out map[cmdutil.ErrorCode][]token.Position) { + t.Helper() + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil + } + if filepath.Ext(path) != ".go" { + return nil + } + if strings.HasSuffix(path, "_test.go") { + return nil + } + f, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", path, err) + } + collectErrorCodes(fset, f, out) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } +} + +func collectErrorCodes(fset *token.FileSet, f *ast.File, out map[cmdutil.ErrorCode][]token.Position) { + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + x, ok := sel.X.(*ast.Ident) + if !ok || x.Name != "cmdutil" { + return true + } + if sel.Sel.Name != "NewError" && sel.Sel.Name != "Wrapf" { + return true + } + // First arg should be cmdutil.CodeXxx (SelectorExpr ident). + // If it's a function call (e.g. cmdutil.ClassifyHTTPError(err)), skip + // — bridge handles those. + arg0Sel, ok := call.Args[0].(*ast.SelectorExpr) + if !ok { + return true + } + argX, ok := arg0Sel.X.(*ast.Ident) + if !ok || argX.Name != "cmdutil" { + return true + } + code, ok := identToErrorCode(arg0Sel.Sel.Name) + if !ok { + // Unknown ident name — record as bogus so the test fails with + // a clear "code referenced but not registered" message. + code = cmdutil.ErrorCode("UNKNOWN_REF:" + arg0Sel.Sel.Name) + } + pos := fset.Position(call.Pos()) + out[code] = append(out[code], pos) + return true + }) +} + +// identToErrorCode maps an ident name like "CodeAuthUnauthenticated" to its +// underlying ErrorCode value via a simple switch. Avoids reflect. +// Keep in sync with cmdutil.AllCodes() — adding a new const here is the same +// bookkeeping as adding it to AllCodes(). +func identToErrorCode(name string) (cmdutil.ErrorCode, bool) { + switch name { + case "CodeAuthUnauthenticated": + return cmdutil.CodeAuthUnauthenticated, true + case "CodeAuthTokenExpired": + return cmdutil.CodeAuthTokenExpired, true + case "CodeAuthBadCredential": + return cmdutil.CodeAuthBadCredential, true + case "CodeAuthForbidden": + return cmdutil.CodeAuthForbidden, true + case "CodeAuthCrossTenantBlocked": + return cmdutil.CodeAuthCrossTenantBlocked, true + case "CodeAuthTenantMismatch": + return cmdutil.CodeAuthTenantMismatch, true + case "CodeResourceNotFound": + return cmdutil.CodeResourceNotFound, true + case "CodeResourceAlreadyExists": + return cmdutil.CodeResourceAlreadyExists, true + case "CodeResourceLocked": + return cmdutil.CodeResourceLocked, true + case "CodeInputInvalidArgument": + return cmdutil.CodeInputInvalidArgument, true + case "CodeInputMissingFlag": + return cmdutil.CodeInputMissingFlag, true + case "CodeServerError": + return cmdutil.CodeServerError, true + case "CodeServerTimeout": + return cmdutil.CodeServerTimeout, true + case "CodeServerRateLimited": + return cmdutil.CodeServerRateLimited, true + case "CodeServerIncompatibleVersion": + return cmdutil.CodeServerIncompatibleVersion, true + case "CodeNetworkError": + return cmdutil.CodeNetworkError, true + case "CodeLocalConfigCorrupt": + return cmdutil.CodeLocalConfigCorrupt, true + case "CodeLocalKeychainDenied": + return cmdutil.CodeLocalKeychainDenied, true + case "CodeLocalFileIO": + return cmdutil.CodeLocalFileIO, true + case "CodeLocalUnimplemented": + return cmdutil.CodeLocalUnimplemented, true + case "CodeLocalContextNotFound": + return cmdutil.CodeLocalContextNotFound, true + case "CodeMCPReadonlyMode": + return cmdutil.CodeMCPReadonlyMode, true + case "CodeMCPToolNotAllowed": + return cmdutil.CodeMCPToolNotAllowed, true + case "CodeMCPSchemaUnknown": + return cmdutil.CodeMCPSchemaUnknown, true + } + return "", false +} diff --git a/cli/acceptance/contract/helpers_test.go b/cli/acceptance/contract/helpers_test.go index 4861021e..97dde0d3 100644 --- a/cli/acceptance/contract/helpers_test.go +++ b/cli/acceptance/contract/helpers_test.go @@ -45,19 +45,44 @@ func newTestFactory(t *testing.T, mockServer *httptest.Server, mockClient *sdk.C // runCmd executes the root command in-process and returns captured stdout/stderr. // Replaces iostreams.IO singleton via SetForTest (auto-restored in t.Cleanup). +// +// Mirrors cmd.Execute() carefully: callers expect the same envelope-printing +// behavior the real entrypoint provides. The helper (a) wires the cobra Out / +// Err sinks to the same buffers it returns (the `version` leaf and any future +// command using c.OutOrStdout would otherwise leak to os.Stdout), and (b) +// re-runs the error-envelope path so failure cases produce the JSON envelope +// the contract test compares against. Without (b), every error scenario's +// golden would be empty. func runCmd(t *testing.T, f *cmdutil.Factory, args ...string) (stdout, stderr string, exitCode int) { t.Helper() out, errBuf := iostreams.SetForTest(t) root := cmd.NewRootCmd(f) // exported in cli/cmd/root.go (Task 16) root.SetArgs(args) root.SetContext(context.Background()) - err := root.Execute() + root.SetOut(out) + root.SetErr(errBuf) + leaf, err := root.ExecuteC() + if err != nil { + err = cmd.MapCobraError(err) + if cmd.WantsJSONOutput(leaf) { + cmdutil.PrintErrorEnvelope(iostreams.IO.Out, err) + } else { + cmdutil.PrintError(iostreams.IO.Err, err) + } + } return out.String(), errBuf.String(), cmdutil.ExitCode(err) } // assertGolden compares got against the JSON golden file at path. // With -update, writes got to path. Normalizes _meta.request_id to "" // before compare (only field known unstable in v0.0). +// +// CRLF normalization: Windows checkouts with the default core.autocrlf=true +// turn LF in tracked text files into CRLF on disk. The command output is +// always LF, so byte-equal would fail despite identical content. .gitattributes +// is the primary defense (forcing LF on testdata/**/*.json), but we also +// strip CR here so a misconfigured contributor checkout doesn't break the +// suite locally before they push. func assertGolden(t *testing.T, got []byte, path string) { t.Helper() got = normalizeEnvelope(got) @@ -74,11 +99,19 @@ func assertGolden(t *testing.T, got []byte, path string) { if err != nil { t.Fatalf("read golden %s: %v (run with -update to create)", path, err) } + want = stripCR(want) + got = stripCR(got) if !bytes.Equal(want, got) { t.Errorf("envelope mismatch for %s\nwant:\n%s\ngot:\n%s", path, want, got) } } +// stripCR removes CR bytes so CRLF golden files (from Windows autocrlf +// checkout) compare equal to LF runtime output. +func stripCR(b []byte) []byte { + return bytes.ReplaceAll(b, []byte{'\r'}, nil) +} + // normalizeEnvelope replaces unstable fields with placeholders for stable diff. // Currently no-op (v0.0 commands don't set _meta.request_id, so output is stable). // Hook for future fields. diff --git a/cli/acceptance/testdata/envelopes/auth_status.error_auth_unauthenticated.json b/cli/acceptance/testdata/envelopes/auth_status.error_auth_unauthenticated.json new file mode 100644 index 00000000..7cc15d93 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/auth_status.error_auth_unauthenticated.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"auth.unauthenticated","message":"fetch current user: HTTP error 401: {\"error\":\"unauthenticated\"}","hint":"run `weknora auth login`"}} diff --git a/cli/acceptance/testdata/envelopes/auth_status.success.json b/cli/acceptance/testdata/envelopes/auth_status.success.json new file mode 100644 index 00000000..ccf07885 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/auth_status.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"context":"","user_id":"usr_abc","email":"user@example.com","tenant_id":42,"tenant_name":"Acme"},"_meta":{"tenant_id":42}} diff --git a/cli/acceptance/testdata/envelopes/context_use.success.json b/cli/acceptance/testdata/envelopes/context_use.success.json new file mode 100644 index 00000000..cb8c2529 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/context_use.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"current_context":"production","previous_context":"staging"}} diff --git a/cli/acceptance/testdata/envelopes/doctor.error_network.json b/cli/acceptance/testdata/envelopes/doctor.error_network.json new file mode 100644 index 00000000..49fbc739 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/doctor.error_network.json @@ -0,0 +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"}]}} diff --git a/cli/acceptance/testdata/envelopes/doctor.success_offline.json b/cli/acceptance/testdata/envelopes/doctor.success_offline.json new file mode 100644 index 00000000..d5fbfbd5 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/doctor.success_offline.json @@ -0,0 +1 @@ +{"ok":true,"data":{"summary":{"all_passed":false,"passed":1,"failed":0,"skipped":3},"checks":[{"name":"base_url_reachable","status":"skip","details":"offline mode"},{"name":"auth_credential","status":"skip","details":"offline mode"},{"name":"server_version","status":"skip","details":"offline mode"},{"name":"credential_storage","status":"ok","details":"keyring or file storage available"}]}} diff --git a/cli/acceptance/testdata/envelopes/kb_get.error_resource_not_found.json b/cli/acceptance/testdata/envelopes/kb_get.error_resource_not_found.json new file mode 100644 index 00000000..9ad8b4e2 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/kb_get.error_resource_not_found.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"resource.not_found","message":"get knowledge base \"missing\": HTTP error 404: {\"error\":\"not found\"}","hint":"verify the resource ID; list available with `weknora kb list`"}} diff --git a/cli/acceptance/testdata/envelopes/kb_get.success.json b/cli/acceptance/testdata/envelopes/kb_get.success.json new file mode 100644 index 00000000..cc86fc7c --- /dev/null +++ b/cli/acceptance/testdata/envelopes/kb_get.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"id":"kb1","name":"Onboarding Docs","type":"","is_temporary":false,"description":"Internal onboarding handbook","tenant_id":42,"chunking_config":{"chunk_size":0,"chunk_overlap":0,"separators":null},"image_processing_config":{"model_id":""},"faq_config":null,"embedding_model_id":"text-embedding-3","summary_model_id":"","vlm_config":{"enabled":false,"model_id":""},"storage_provider_config":null,"storage_config":{"secret_id":"","secret_key":"","region":"","bucket_name":"","app_id":"","path_prefix":"","provider":""},"extract_config":null,"created_at":"2025-01-01T12:00:00Z","updated_at":"2025-01-01T12:00:00Z","knowledge_count":5,"chunk_count":128,"is_processing":false,"processing_count":0}} diff --git a/cli/acceptance/testdata/envelopes/kb_list.error_auth_forbidden.json b/cli/acceptance/testdata/envelopes/kb_list.error_auth_forbidden.json new file mode 100644 index 00000000..0cb60cc7 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/kb_list.error_auth_forbidden.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"auth.forbidden","message":"list knowledge bases: HTTP error 403: {\"error\":\"forbidden\"}","hint":"active context lacks permission for this resource"}} diff --git a/cli/acceptance/testdata/envelopes/kb_list.success.json b/cli/acceptance/testdata/envelopes/kb_list.success.json new file mode 100644 index 00000000..0bb564a1 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/kb_list.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"items":[{"id":"kb1","name":"Onboarding Docs","type":"","is_temporary":false,"description":"","tenant_id":42,"chunking_config":{"chunk_size":0,"chunk_overlap":0,"separators":null},"image_processing_config":{"model_id":""},"faq_config":null,"embedding_model_id":"text-embedding-3","summary_model_id":"","vlm_config":{"enabled":false,"model_id":""},"storage_provider_config":null,"storage_config":{"secret_id":"","secret_key":"","region":"","bucket_name":"","app_id":"","path_prefix":"","provider":""},"extract_config":null,"created_at":"2025-01-01T12:00:00Z","updated_at":"2025-01-01T12:00:00Z","knowledge_count":5,"chunk_count":128,"is_processing":false,"processing_count":0},{"id":"kb2","name":"API Reference","type":"","is_temporary":false,"description":"","tenant_id":42,"chunking_config":{"chunk_size":0,"chunk_overlap":0,"separators":null},"image_processing_config":{"model_id":""},"faq_config":null,"embedding_model_id":"text-embedding-3","summary_model_id":"","vlm_config":{"enabled":false,"model_id":""},"storage_provider_config":null,"storage_config":{"secret_id":"","secret_key":"","region":"","bucket_name":"","app_id":"","path_prefix":"","provider":""},"extract_config":null,"created_at":"2025-01-01T12:00:00Z","updated_at":"2025-01-01T12:00:00Z","knowledge_count":12,"chunk_count":340,"is_processing":false,"processing_count":0}]}} diff --git a/cli/acceptance/testdata/envelopes/kb_list.success_empty.json b/cli/acceptance/testdata/envelopes/kb_list.success_empty.json new file mode 100644 index 00000000..74e94f48 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/kb_list.success_empty.json @@ -0,0 +1 @@ +{"ok":true,"data":{"items":[]}} diff --git a/cli/acceptance/testdata/envelopes/search.error_input_invalid.json b/cli/acceptance/testdata/envelopes/search.error_input_invalid.json new file mode 100644 index 00000000..40c1a907 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/search.error_input_invalid.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"input.invalid_argument","message":"--no-vector and --no-keyword cannot both be set","hint":"see `weknora --help` for valid usage"}} diff --git a/cli/acceptance/testdata/envelopes/search.error_resource_not_found.json b/cli/acceptance/testdata/envelopes/search.error_resource_not_found.json new file mode 100644 index 00000000..7c4b2b6b --- /dev/null +++ b/cli/acceptance/testdata/envelopes/search.error_resource_not_found.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"resource.not_found","message":"hybrid search: HTTP error 404: {\"error\":\"not found\"}","hint":"verify the resource ID; list available with `weknora kb list`"}} diff --git a/cli/acceptance/testdata/envelopes/search.success.json b/cli/acceptance/testdata/envelopes/search.success.json new file mode 100644 index 00000000..3af7e9d6 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/search.success.json @@ -0,0 +1 @@ +{"ok":true,"data":[{"id":"chunk-1","content":"first chunk content","knowledge_id":"doc-1","chunk_index":0,"knowledge_title":"Doc 1","start_at":0,"end_at":0,"seq":0,"score":0.92,"match_type":0,"chunk_type":"","image_info":"","metadata":null,"knowledge_filename":"","knowledge_source":"","knowledge_channel":""},{"id":"chunk-2","content":"second chunk content","knowledge_id":"doc-2","chunk_index":1,"knowledge_title":"Doc 2","start_at":0,"end_at":0,"seq":0,"score":0.81,"match_type":1,"chunk_type":"","image_info":"","metadata":null,"knowledge_filename":"","knowledge_source":"","knowledge_channel":""}],"_meta":{"kb_id":"kb1"}} diff --git a/cli/acceptance/testdata/envelopes/version.success.json b/cli/acceptance/testdata/envelopes/version.success.json new file mode 100644 index 00000000..b49e35ed --- /dev/null +++ b/cli/acceptance/testdata/envelopes/version.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"commit":"none","date":"unknown","version":"dev"}} diff --git a/cli/acceptance/testdata/envelopes/whoami.error_auth_unauthenticated.json b/cli/acceptance/testdata/envelopes/whoami.error_auth_unauthenticated.json new file mode 100644 index 00000000..7cc15d93 --- /dev/null +++ b/cli/acceptance/testdata/envelopes/whoami.error_auth_unauthenticated.json @@ -0,0 +1 @@ +{"ok":false,"error":{"code":"auth.unauthenticated","message":"fetch current user: HTTP error 401: {\"error\":\"unauthenticated\"}","hint":"run `weknora auth login`"}} diff --git a/cli/acceptance/testdata/envelopes/whoami.success.json b/cli/acceptance/testdata/envelopes/whoami.success.json new file mode 100644 index 00000000..38a284ae --- /dev/null +++ b/cli/acceptance/testdata/envelopes/whoami.success.json @@ -0,0 +1 @@ +{"ok":true,"data":{"user_id":"usr_abc","tenant_id":42}} diff --git a/cli/cmd/auth/auth_test.go b/cli/cmd/auth/auth_test.go index 083f2b3b..e883dc48 100644 --- a/cli/cmd/auth/auth_test.go +++ b/cli/cmd/auth/auth_test.go @@ -31,9 +31,12 @@ func TestNewCmdAuth_TreeShape(t *testing.T) { func TestNewCmdLogin_FlagsRegistered(t *testing.T) { cmd := NewCmdLogin(&cmdutil.Factory{}, nil) - for _, name := range []string{"host", "context", "with-token", "json"} { + for _, name := range []string{"host", "name", "with-token", "json"} { assert.NotNilf(t, cmd.Flags().Lookup(name), "flag %s missing", name) } + // `--context` should NOT be a local flag (it's the global persistent flag). + // Local registration would silently shadow the global single-shot override. + assert.Nil(t, cmd.Flags().Lookup("context"), "auth login must not declare a local --context flag (use --name)") } func TestNewCmdLogin_InvokesRunF(t *testing.T) { diff --git a/cli/cmd/auth/login.go b/cli/cmd/auth/login.go index 9ae88df1..6f99f068 100644 --- a/cli/cmd/auth/login.go +++ b/cli/cmd/auth/login.go @@ -19,7 +19,10 @@ import ( // LoginOptions is the configuration captured from flags + prompts. type LoginOptions struct { Host string // --host - Context string // --context name to write + Context string // --name: context name to write into config.yaml + // (was --context in v0.0; renamed v0.1 to + // avoid shadowing the new global --context + // single-shot override flag) WithToken bool // --with-token (read api key from stdin instead of prompting password) APIKey string // populated by --with-token from stdin Email string @@ -60,7 +63,7 @@ the current_context in ~/.config/weknora/config.yaml.`, }, } cmd.Flags().StringVar(&opts.Host, "host", "", "WeKnora server URL, e.g. https://kb.example.com") - cmd.Flags().StringVar(&opts.Context, "context", "default", "Name to assign this context in config.yaml") + cmd.Flags().StringVar(&opts.Context, "name", "default", "Context name to register in config.yaml") cmd.Flags().BoolVar(&opts.WithToken, "with-token", false, "Read an API key from stdin instead of prompting for password") cmd.Flags().BoolVar(&opts.JSONOut, "json", false, "Output JSON envelope") cmdutil.MustRequireFlag(cmd, "host") diff --git a/cli/cmd/context/use.go b/cli/cmd/context/use.go index b3aa1d90..408787f5 100644 --- a/cli/cmd/context/use.go +++ b/cli/cmd/context/use.go @@ -2,6 +2,7 @@ package contextcmd import ( "fmt" + "sort" "github.com/spf13/cobra" @@ -17,7 +18,15 @@ func NewCmdUse(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "use ", Short: "Switch the default context for subsequent commands", - Args: cobra.ExactArgs(1), + Long: `Switches the default context written in config.yaml. Names are case-sensitive. + +The active context is what every subsequent command uses for auth + host. The +global --context flag (e.g. weknora --context staging kb list) overrides for +one command without writing to disk.`, + Example: ` weknora context use staging # persist switch + weknora --context staging kb list # one-shot override (no disk write) + weknora context use --help # this help`, + Args: cobra.ExactArgs(1), RunE: func(c *cobra.Command, args []string) error { return runUse(args[0]) }, @@ -58,10 +67,13 @@ func notFoundError(name string, cfg *config.Config) error { Hint: "no contexts registered — run `weknora auth login` first", } } - candidate := closestMatch(name, contextKeys(cfg.Contexts)) - hint := availableHint(cfg) + keys := contextKeys(cfg.Contexts) + candidate := closestMatch(name, keys) + var hint string if candidate != "" && candidate != name { hint = fmt.Sprintf("did you mean: %q?", candidate) + } else { + hint = fmt.Sprintf("available contexts: %v", keys) } return &cmdutil.Error{ Code: cmdutil.CodeLocalContextNotFound, @@ -78,16 +90,17 @@ func contextKeys(m map[string]config.Context) []string { return out } -func availableHint(cfg *config.Config) string { - return fmt.Sprintf("available contexts: %v", contextKeys(cfg.Contexts)) -} - // closestMatch returns the candidate with min levenshtein distance ≤ 2, -// or "" if none qualifies. +// or "" if none qualifies. Ties broken by lexicographic order so the hint +// is deterministic across map-iteration orderings (Go randomizes range over +// map; without this, did-you-mean output is flaky for equally-close +// candidates). func closestMatch(target string, candidates []string) string { + sorted := append([]string(nil), candidates...) + sort.Strings(sorted) best := "" bestD := 3 - for _, c := range candidates { + for _, c := range sorted { d := levenshtein(target, c) if d < bestD { bestD = d diff --git a/cli/cmd/context/use_test.go b/cli/cmd/context/use_test.go index 258766ce..c56bdda3 100644 --- a/cli/cmd/context/use_test.go +++ b/cli/cmd/context/use_test.go @@ -63,8 +63,35 @@ func TestUse_NotFound_WithDidYouMean(t *testing.T) { if cm.Code != cmdutil.CodeLocalContextNotFound { t.Errorf("code = %q, want %q", cm.Code, cmdutil.CodeLocalContextNotFound) } - if !strings.Contains(cm.Hint, "production") { - t.Errorf("hint should suggest 'production', got %q", cm.Hint) + if cm.Hint != `did you mean: "production"?` { + t.Errorf("hint should be exact `did you mean: \"production\"?`, got %q", cm.Hint) + } +} + +// TestUse_NotFound_DeterministicTieBreak guards against map-iteration-order +// flake: when two candidates have equal levenshtein distance, the suggestion +// must be reproducibly the lexicographically-smaller one. +func TestUse_NotFound_DeterministicTieBreak(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + cfg := &config.Config{Contexts: map[string]config.Context{ + "prod": {Host: "https://a"}, + "prom": {Host: "https://b"}, + "prog": {Host: "https://c"}, + }} + if err := config.Save(cfg); err != nil { + t.Fatalf("Save: %v", err) + } + // "prox" is distance 1 from prod / prom (both win); lex tie-break → prod. + for i := 0; i < 5; i++ { + err := runUse("prox") + if err == nil { + t.Fatalf("iter %d: expected error", i) + } + cm := err.(*cmdutil.Error) + if cm.Hint != `did you mean: "prod"?` { + t.Fatalf("iter %d: tie-break must pick lex-smallest 'prod', got %q", i, cm.Hint) + } } } diff --git a/cli/cmd/doctor/doctor.go b/cli/cmd/doctor/doctor.go index cedb8978..1e0435f4 100644 --- a/cli/cmd/doctor/doctor.go +++ b/cli/cmd/doctor/doctor.go @@ -41,10 +41,21 @@ type Options struct { JSONOut bool } +// Status is the per-check outcome on the wire (JSON Marshal still emits the +// underlying string). Typed so cascade comparisons can't typo against bare +// "ok"/"fail"/"skip" string literals. +type Status string + +const ( + StatusOK Status = "ok" + StatusFail Status = "fail" + StatusSkip Status = "skip" +) + // Check is one row in the report. type Check struct { Name string `json:"name"` - Status string `json:"status"` // ok / fail / skip + Status Status `json:"status"` Details string `json:"details,omitempty"` Hint string `json:"hint,omitempty"` } @@ -88,13 +99,30 @@ func NewCmd(f *cmdutil.Factory) *cobra.Command { return nil // doctor 自身不返回 error;失败状态在 data.checks }, } - cmd.Flags().BoolVar(&opts.NoCache, "no-cache", false, "Bypass server-info cache; force re-probe") - cmd.Flags().BoolVar(&opts.Offline, "offline", false, "Skip network checks; only verify local keyring/file storage") + 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).") 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. +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 { + c.Status, c.Details = StatusSkip, "prereq failed: "+p.Name + return true + } + } + return false +} + // runChecks executes the 4-item check matrix with cascade-skip semantics. // Pure function over Services so tests can drive it directly. func runChecks(ctx context.Context, opts *Options, svc Services, cliVer string) Result { @@ -105,118 +133,105 @@ func runChecks(ctx context.Context, opts *Options, svc Services, cliVer string) {Name: "credential_storage"}, } - // 1. base_url_reachable - if opts.Offline { - checks[0].Status = "skip" - checks[0].Details = "offline mode" - } else { + // 1. base_url_reachable — gated by offline only. + if !cascade(&checks[0], opts.Offline) { t0 := time.Now() if err := svc.PingBaseURL(ctx); err != nil { - checks[0].Status = "fail" - checks[0].Hint = "verify --base-url and network reachability" + checks[0].Status = StatusFail + checks[0].Hint = "verify the host configured for the active context (run `weknora auth login --host=...`) and network reachability" checks[0].Details = err.Error() } else { - checks[0].Status = "ok" + checks[0].Status = StatusOK checks[0].Details = fmt.Sprintf("reachable in %s", time.Since(t0).Round(time.Millisecond)) } } - // 2. auth_credential — depends on base_url - switch { - case opts.Offline: - checks[1].Status = "skip" - checks[1].Details = "offline mode" - case checks[0].Status == "fail": - checks[1].Status = "skip" - checks[1].Details = "prereq failed: base_url_reachable" - default: - _, err := svc.GetCurrentUser(ctx) - if err != nil { - checks[1].Status = "fail" + // 2. auth_credential — needs base_url. + if !cascade(&checks[1], opts.Offline, &checks[0]) { + if _, err := svc.GetCurrentUser(ctx); err != nil { + checks[1].Status = StatusFail checks[1].Hint = "run `weknora auth login`" checks[1].Details = err.Error() } else { - checks[1].Status = "ok" + checks[1].Status = StatusOK } } - // 3. server_version — depends on auth_credential - switch { - case opts.Offline: - checks[2].Status = "skip" - checks[2].Details = "offline mode" - case checks[1].Status != "ok": - checks[2].Status = "skip" - checks[2].Details = "prereq failed: auth_credential" - default: + // 3. server_version — needs auth_credential. + if !cascade(&checks[2], opts.Offline, &checks[1]) { info, fromCache, err := loadOrProbeServerInfo(ctx, opts, svc) if err != nil { - checks[2].Status = "fail" + checks[2].Status = StatusFail checks[2].Details = err.Error() } else { - level, hint := compat.Compat(info.ServerVersion, cliVer) - suffix := "" - if fromCache { - suffix = " (cached, pass --no-cache to refresh)" - } - if level == compat.HardError { - checks[2].Status = "fail" - checks[2].Hint = hint - checks[2].Details = "server " + info.ServerVersion + suffix - } else { - checks[2].Status = "ok" - if hint != "" { - checks[2].Details = hint + suffix - } else { - checks[2].Details = fmt.Sprintf("server %s%s", info.ServerVersion, suffix) - } - } + fillVersionCheck(&checks[2], info, cliVer, fromCache) } } - // 4. credential_storage — independent of network + // 4. credential_storage — independent of network; never gated by offline. if _, err := secrets.NewBestEffortStore(); err != nil { - checks[3].Status = "fail" + checks[3].Status = StatusFail checks[3].Details = err.Error() checks[3].Hint = "verify keyring access; falls back to file store" } else { - checks[3].Status = "ok" + checks[3].Status = StatusOK checks[3].Details = "keyring or file storage available" } return Result{Summary: summarize(checks), Checks: checks} } -// loadOrProbeServerInfo respects --no-cache: -// --no-cache or stale/missing cache → call svc.GetSystemInfo + SaveCache -// else → return cached Info (fromCache=true) -// -// SaveCache failure does NOT propagate — best-effort write. The probe -// data is still returned to the caller for the compat check. +// fillVersionCheck applies compat.Compat to (server, cli) version pair and +// 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(). +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 { + c.Status = StatusFail + c.Hint = hint + c.Details = "server " + info.ServerVersion + suffix + return + } + c.Status = StatusOK + if hint != "" { + c.Details = hint + suffix + } else { + c.Details = fmt.Sprintf("server %s%s", info.ServerVersion, suffix) + } +} + +// loadOrProbeServerInfo respects --no-cache: load fresh cache when allowed, +// else call compat.Probe (which wraps svc.GetSystemInfo) and persist. Cache +// write is best-effort. Returns fromCache so the caller can render the +// "cached" presentation hint without a brittle ProbedAt heuristic. func loadOrProbeServerInfo(ctx context.Context, opts *Options, svc Services) (info *compat.Info, fromCache bool, err error) { if !opts.NoCache { if cached, fresh, _ := compat.LoadCache(); fresh && cached != nil { return cached, true, nil } } - sys, err := svc.GetSystemInfo(ctx) + probed, err := compat.Probe(ctx, svc) if err != nil { return nil, false, err } - fresh := &compat.Info{ServerVersion: sys.Version, ProbedAt: time.Now()} - _ = compat.SaveCache(fresh) // best-effort - return fresh, false, nil + _ = compat.SaveCache(probed) + return probed, false, nil } func summarize(cs []Check) Summary { s := Summary{} for _, c := range cs { switch c.Status { - case "ok": + case StatusOK: s.Passed++ - case "fail": + case StatusFail: s.Failed++ - case "skip": + case StatusSkip: s.Skipped++ } } @@ -232,9 +247,9 @@ func emit(opts *Options, r Result) { for _, c := range r.Checks { marker := "[ok]" switch c.Status { - case "fail": + case StatusFail: marker = "[fail]" - case "skip": + case StatusSkip: marker = "[skip]" } line := fmt.Sprintf("%-6s %-20s %s", marker, c.Name, c.Status) @@ -251,16 +266,34 @@ func emit(opts *Options, r Result) { } // buildServices wires the Factory closures into the doctor.Services interface. +// Reads the active context's host so PingBaseURL targets the user's actual +// server, not localhost. +// +// Critically: this does NOT pre-resolve f.Client(). doctor's package promise +// (top comment) is that credential_storage runs even when no auth is set up — +// e.g. first-time `weknora doctor` to diagnose setup. Pre-resolving Client +// here would early-exit with auth.unauthenticated before any check runs, +// contradicting the docs. Instead, GetCurrentUser / GetSystemInfo lazily +// resolve and surface their own failure as a per-check StatusFail. func buildServices(f *cmdutil.Factory) (Services, error) { - cli, err := f.Client() + cfg, err := f.Config() if err != nil { return nil, err } - return &realServices{cli: cli}, nil + host := "" + if ctx, ok := cfg.Contexts[cfg.CurrentContext]; ok { + host = ctx.Host + } + // WEKNORA_BASE_URL still wins as a test/dev override; production reads host. + if v := os.Getenv("WEKNORA_BASE_URL"); v != "" { + host = v + } + return &realServices{f: f, host: host}, nil } type realServices struct { - cli *sdk.Client + f *cmdutil.Factory + host string } // pingTimeout caps the HEAD /health probe so a wedged TCP connection @@ -268,9 +301,10 @@ type realServices struct { const pingTimeout = 5 * time.Second func (s *realServices) PingBaseURL(ctx context.Context) error { - // WEKNORA_BASE_URL is the test-scaffold override; production should plumb - // config.Host through (TODO v0.2: add Client.BaseURL() accessor in SDK). - url := envOrDefault("WEKNORA_BASE_URL", "http://localhost:8080") + "/health" + if s.host == "" { + return fmt.Errorf("no host configured for active context") + } + url := s.host + "/health" ctx, cancel := context.WithTimeout(ctx, pingTimeout) defer cancel() req, err := http.NewRequestWithContext(ctx, http.MethodHead, url, nil) @@ -288,16 +322,26 @@ func (s *realServices) PingBaseURL(ctx context.Context) error { return nil } +// GetCurrentUser lazily resolves the SDK client. When no context is configured +// or credentials missing, f.Client() returns auth.unauthenticated; we surface +// that as the auth_credential check's failure rather than aborting doctor. func (s *realServices) GetCurrentUser(ctx context.Context) (*sdk.CurrentUserResponse, error) { - return s.cli.GetCurrentUser(ctx) -} -func (s *realServices) GetSystemInfo(ctx context.Context) (*sdk.SystemInfo, error) { - return s.cli.GetSystemInfo(ctx) + cli, err := s.f.Client() + if err != nil { + return nil, err + } + return cli.GetCurrentUser(ctx) } -func envOrDefault(k, def string) string { - if v := os.Getenv(k); v != "" { - return v +// GetSystemInfo lazily resolves the SDK client (same rationale as GetCurrentUser). +// In the cascade ordering, auth_credential gates server_version, so this only +// runs when auth_credential succeeded — but the lazy resolution keeps doctor +// useful when only credential_storage is checkable (e.g., user not yet logged in). +func (s *realServices) GetSystemInfo(ctx context.Context) (*sdk.SystemInfo, error) { + cli, err := s.f.Client() + if err != nil { + return nil, err } - return def + return cli.GetSystemInfo(ctx) } + diff --git a/cli/cmd/doctor/doctor_test.go b/cli/cmd/doctor/doctor_test.go index c54ab43e..ced38257 100644 --- a/cli/cmd/doctor/doctor_test.go +++ b/cli/cmd/doctor/doctor_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/Tencent/WeKnora/cli/internal/cmdutil" "github.com/Tencent/WeKnora/cli/internal/compat" "github.com/Tencent/WeKnora/cli/internal/iostreams" sdk "github.com/Tencent/WeKnora/client" @@ -48,6 +49,9 @@ func TestDoctor_AllOK(t *testing.T) { if !r.Summary.AllPassed { t.Errorf("expected all_passed, got summary %+v", r.Summary) } + if r.Summary.Passed != 4 { + t.Errorf("expected Passed=4 (one per check), got %+v", r.Summary) + } if r.Summary.Failed != 0 || r.Summary.Skipped != 0 { t.Errorf("expected 0 fail / 0 skip, got %+v", r.Summary) } @@ -69,13 +73,16 @@ func TestDoctor_BaseURLFails_DownstreamSkip(t *testing.T) { if r.Summary.Skipped != 2 { t.Errorf("expected 2 skipped (auth_credential + server_version), got %d", r.Summary.Skipped) } - if r.Checks[0].Status != "fail" { + if r.Checks[0].Status != StatusFail { t.Errorf("base_url_reachable status = %q, want fail", r.Checks[0].Status) } - if r.Checks[1].Status != "skip" { + if !strings.Contains(r.Checks[0].Hint, "auth login") { + t.Errorf("base_url fail hint should reference `auth login`, got %q", r.Checks[0].Hint) + } + if r.Checks[1].Status != StatusSkip { t.Errorf("auth_credential status = %q, want skip", r.Checks[1].Status) } - if r.Checks[2].Status != "skip" { + if r.Checks[2].Status != StatusSkip { t.Errorf("server_version status = %q, want skip", r.Checks[2].Status) } // credential_storage 与网络无关,应该独立运行(不受 base_url fail 影响) @@ -96,7 +103,7 @@ func TestDoctor_Offline_OnlyKeyringChecked(t *testing.T) { if last.Name != "credential_storage" { t.Errorf("last check = %q, want credential_storage", last.Name) } - if last.Status == "skip" { + if last.Status == StatusSkip { t.Error("credential_storage should NOT be skipped even in offline mode") } } @@ -109,13 +116,13 @@ func TestDoctor_AuthFails_VersionSkipped(t *testing.T) { systemInfo: &sdk.SystemInfo{Version: "1.0.0"}, } r := runChecks(context.Background(), &Options{}, svc, "1.0.0") - if r.Checks[0].Status != "ok" { + if r.Checks[0].Status != StatusOK { t.Errorf("base_url should pass, got %q", r.Checks[0].Status) } - if r.Checks[1].Status != "fail" { + if r.Checks[1].Status != StatusFail { t.Errorf("auth_credential should fail, got %q", r.Checks[1].Status) } - if r.Checks[2].Status != "skip" { + if r.Checks[2].Status != StatusSkip { t.Errorf("server_version should skip due to auth fail, got %q", r.Checks[2].Status) } } @@ -129,7 +136,7 @@ func TestDoctor_CacheHit_SkipsProbe(t *testing.T) { } svc := &fakeServices{userResp: goodUserResp()} r := runChecks(context.Background(), &Options{}, svc, "1.0.0") - if r.Checks[2].Status != "ok" { + if r.Checks[2].Status != StatusOK { t.Errorf("server_version should be ok via cache, got %q (%s)", r.Checks[2].Status, r.Checks[2].Details) } if svc.systemInfoHits.Load() != 0 { @@ -140,6 +147,48 @@ func TestDoctor_CacheHit_SkipsProbe(t *testing.T) { } } +// TestDoctor_NoConfig_StillRunsCredentialStorage guards the package-doc +// promise that credential_storage runs even with zero configuration. Round-4 +// reviewer surfaced that buildServices used to abort on f.Client() failure, +// silently violating the doc for any first-time user running `weknora doctor` +// to diagnose setup. The lazy-resolve fix means missing auth surfaces as +// auth_credential=fail, not a top-level command exit. +func TestDoctor_NoConfig_StillRunsCredentialStorage(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + _, _ = iostreams.SetForTest(t) + + f := cmdutil.New() + svc, err := buildServices(f) + if err != nil { + t.Fatalf("buildServices must succeed even with no config; got: %v", err) + } + r := runChecks(context.Background(), &Options{}, svc, "1.0.0") + + // All 4 checks must run (no early-exit). Last one is credential_storage and + // has no network/auth dependency, so it must report ok. + if got := len(r.Checks); got != 4 { + t.Fatalf("expected 4 checks executed, got %d", got) + } + if r.Checks[3].Name != "credential_storage" { + t.Errorf("Checks[3] = %q, want credential_storage", r.Checks[3].Name) + } + if r.Checks[3].Status != StatusOK { + t.Errorf("credential_storage must run / pass even without auth, got %q (%s)", + r.Checks[3].Status, r.Checks[3].Details) + } + // base_url_reachable fails (no host); cascade then skips auth_credential + // and server_version. Whether auth/version are skip vs fail is an internal + // cascade detail; the user-visible promise is that 4 checks executed and + // the independent credential_storage one passed. + if r.Checks[0].Status != StatusFail { + t.Errorf("base_url must fail without host, got %q", r.Checks[0].Status) + } + if r.Checks[1].Status == StatusOK { + t.Errorf("auth_credential must NOT report ok without config, got %q", r.Checks[1].Status) + } +} + func TestDoctor_NoCache_BypassesCache(t *testing.T) { t.Setenv("XDG_CACHE_HOME", t.TempDir()) _, _ = iostreams.SetForTest(t) diff --git a/cli/cmd/kb/list.go b/cli/cmd/kb/list.go index 1469d670..1ee57567 100644 --- a/cli/cmd/kb/list.go +++ b/cli/cmd/kb/list.go @@ -3,6 +3,7 @@ package kb import ( "context" "fmt" + "sort" "text/tabwriter" "time" @@ -59,6 +60,12 @@ func runList(ctx context.Context, opts *ListOptions, svc ListService) error { if items == nil { items = []sdk.KnowledgeBase{} // ensure JSON [] not null } + // Spec §1.2: default sort by updated_at desc. Server return order is not + // guaranteed, so client-side sort makes output deterministic regardless + // of backend storage choices. + sort.Slice(items, func(i, j int) bool { + return items[i].UpdatedAt.After(items[j].UpdatedAt) + }) if opts.JSONOut { return format.WriteEnvelope(iostreams.IO.Out, format.Success(listResult{Items: items}, nil)) diff --git a/cli/cmd/root.go b/cli/cmd/root.go index c01317a9..a0dc36a7 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -7,6 +7,7 @@ package cmd import ( "fmt" "os" + "strconv" "strings" "github.com/spf13/cobra" @@ -34,8 +35,8 @@ func Execute() int { if err == nil { return 0 } - err = mapCobraError(err) - if agent.ShouldUseAgentMode(cmd) || wantsJSONOutput(cmd) { + err = MapCobraError(err) + if agent.ShouldUseAgentMode(cmd) || WantsJSONOutput(cmd) { cmdutil.PrintErrorEnvelope(iostreams.IO.Out, err) } else { cmdutil.PrintError(iostreams.IO.Err, err) @@ -43,7 +44,7 @@ func Execute() int { return cmdutil.ExitCode(err) } -// wantsJSONOutput reports whether cmd was invoked with --json, so error +// WantsJSONOutput reports whether cmd was invoked with --json, so error // output matches the success format. Persistent flags inherit automatically // via cmd.Flags(). // @@ -51,7 +52,10 @@ func Execute() int { // unknown subcommand or unknown flag at root level. Without this, `weknora // bogus --json` would emit a human stderr line instead of the envelope the // agent asked for. -func wantsJSONOutput(cmd *cobra.Command) bool { +// +// Exported so the acceptance/contract test helper can replicate Execute()'s +// envelope-printing path without having to call os.Exit-bound Execute() itself. +func WantsJSONOutput(cmd *cobra.Command) bool { if v, err := cmd.Flags().GetBool("json"); err == nil && v { return true } @@ -78,16 +82,14 @@ func argsRequestJSON(args []string) bool { } // isPflagTruthy mirrors pflag's bool parsing for "--flag=" tokens. -// pflag accepts 1/t/T/TRUE/true/True as truthy. +// pflag delegates to strconv.ParseBool, which accepts 1/t/T/TRUE/true/True +// as truthy and 0/f/F/FALSE/false/False as falsy. Anything else errors. func isPflagTruthy(v string) bool { - switch v { - case "1", "t", "T", "TRUE", "true", "True": - return true - } - return false + b, err := strconv.ParseBool(v) + return err == nil && b } -// mapCobraError tags the textually-emitted cobra errors as cmdutil.FlagError +// MapCobraError tags the textually-emitted cobra errors as cmdutil.FlagError // so they exit 2 like other user invocation mistakes. SetFlagErrorFunc handles // flag parse errors at parse time; this catches positional/Args validation // errors and unknown subcommands that propagate as plain errors. @@ -95,7 +97,10 @@ func isPflagTruthy(v string) bool { // Pinned to cobra v1.10 message formats (cobra/args.go: ExactArgs / NoArgs; // cobra/command.go: required-flag / unknown-command). TestMapCobraError_PinnedPrefixes // guards against a silent break on cobra bumps. -func mapCobraError(err error) error { +// +// Exported so the acceptance/contract test helper can reuse the mapping when +// replicating Execute()'s error-envelope path in-process. +func MapCobraError(err error) error { if err == nil { return nil } @@ -126,8 +131,15 @@ var cobraFlagErrorPrefixes = []string{ func NewRootCmd(f *cmdutil.Factory) *cobra.Command { v, commit, date := build.Info() cmd := &cobra.Command{ - Use: "weknora", - Short: "WeKnora CLI — RAG knowledge base from your terminal", + Use: "weknora", + Short: "WeKnora CLI — RAG knowledge base from your terminal", + Long: `WeKnora CLI lets you authenticate, browse knowledge bases, and run +hybrid searches against a WeKnora server from your shell or an AI agent.`, + Example: ` weknora auth login --host=https://kb.example.com # one-time setup + weknora kb list # list knowledge bases + weknora kb get # show one + weknora search "your question" --kb= # hybrid retrieval + weknora doctor --json # health check (agent-readable)`, SilenceUsage: true, SilenceErrors: true, // Version makes cobra auto-register a `--version` global flag that @@ -137,6 +149,11 @@ func NewRootCmd(f *cmdutil.Factory) *cobra.Command { Version: fmt.Sprintf("%s (commit %s, built %s)", v, commit, date), PersistentPreRun: func(c *cobra.Command, args []string) { agent.ApplyAgentSugar(c) + // Propagate the global --context flag into the Factory for this + // invocation only. Spec §1.2: single-shot override, no disk write. + if v, _ := c.Flags().GetString("context"); v != "" { + f.ContextOverride = v + } }, } // Match `weknora version` line format so both forms output the same. @@ -161,17 +178,18 @@ func NewRootCmd(f *cmdutil.Factory) *cobra.Command { } // addGlobalFlags registers persistent flags available on every subcommand. -// Only flags whose behavior is actually wired in v0.0 are listed — flags -// that need work in later PRs (--format multi-value in v0.1, --context in -// v0.1 PR-4, --no-version-check in v0.7's compat probe) are added when -// their consumer lands. A flag that accepts values but does nothing is a -// worse contract than no flag. +// Only flags whose behavior is actually wired are listed — a flag that +// accepts values but does nothing is a worse contract than no flag. +// +// --context lands here in v0.1 (spec §1.2); --no-version-check waits for +// v0.7's compat probe consumer. func addGlobalFlags(cmd *cobra.Command) { pf := cmd.PersistentFlags() pf.Bool("agent", false, "Agent mode: envelope JSON output + no interactive prompts + no progress UI") pf.Bool("no-interactive", false, "Refuse interactive prompts; missing input becomes a hard error") pf.Bool("no-progress", false, "Suppress progress bars and spinners") pf.BoolP("yes", "y", false, "Skip confirmation prompts on destructive operations") + pf.String("context", "", "Override the active context for this invocation (no disk write)") } // agentAwareHelpFunc wraps cobra's default help to append the AI agent guidance diff --git a/cli/cmd/root_test.go b/cli/cmd/root_test.go index 76e2d37a..938fe5c6 100644 --- a/cli/cmd/root_test.go +++ b/cli/cmd/root_test.go @@ -93,24 +93,51 @@ func TestMapCobraError_PinnedPrefixes(t *testing.T) { func TestMapCobraError(t *testing.T) { t.Run("nil passes through", func(t *testing.T) { - assert.Nil(t, mapCobraError(nil)) + assert.Nil(t, MapCobraError(nil)) }) t.Run("non-matching error passes through", func(t *testing.T) { - err := mapCobraError(assert.AnError) + err := MapCobraError(assert.AnError) assert.Equal(t, assert.AnError, err) }) t.Run("unknown command wraps as FlagError", func(t *testing.T) { - err := mapCobraError(errors.New(`unknown command "bogus" for "weknora"`)) + err := MapCobraError(errors.New(`unknown command "bogus" for "weknora"`)) var fe *cmdutil.FlagError assert.True(t, errors.As(err, &fe)) }) t.Run("required flag wraps as FlagError", func(t *testing.T) { - err := mapCobraError(errors.New(`required flag(s) "host" not set`)) + err := MapCobraError(errors.New(`required flag(s) "host" not set`)) var fe *cmdutil.FlagError assert.True(t, errors.As(err, &fe)) }) } +// TestRoot_ContextFlagPropagation guards the cobra → Factory wiring of the +// global --context flag. Without this, a future refactor that disconnects +// PersistentPreRun from f.ContextOverride would only fail e2e — the +// per-package TestFactory_ContextOverride only proves the Factory side. +func TestRoot_ContextFlagPropagation(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + {"no flag", []string{"version"}, ""}, + {"global before subcmd", []string{"--context", "staging", "version"}, "staging"}, + {"--context=value form", []string{"--context=prod", "version"}, "prod"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := cmdutil.New() + root := NewRootCmd(f) + root.SetArgs(tc.args) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + require.NoError(t, root.Execute()) + assert.Equal(t, tc.want, f.ContextOverride) + }) + } +} + func TestArgsRequestJSON(t *testing.T) { cases := []struct { name string @@ -134,12 +161,12 @@ func TestArgsRequestJSON(t *testing.T) { func TestWantsJSONOutput(t *testing.T) { // Build a minimal *cobra.Command with the json flag directly so we test - // the helper without going through cobra's parse pipeline. wantsJSONOutput + // the helper without going through cobra's parse pipeline. WantsJSONOutput // reads cmd.Flags() which on a fresh command equals LocalFlags(). c := &cobra.Command{Use: "x"} c.Flags().Bool("json", false, "") - assert.False(t, wantsJSONOutput(c), "default: --json unset") + assert.False(t, WantsJSONOutput(c), "default: --json unset") require.NoError(t, c.Flags().Set("json", "true")) - assert.True(t, wantsJSONOutput(c), "--json=true honored") + assert.True(t, WantsJSONOutput(c), "--json=true honored") } diff --git a/cli/internal/cmdutil/exit.go b/cli/internal/cmdutil/exit.go index 0452157b..d6ddf151 100644 --- a/cli/internal/cmdutil/exit.go +++ b/cli/internal/cmdutil/exit.go @@ -47,14 +47,28 @@ func ExitCode(err error) int { return 1 } -// PrintError writes err to w as one human-readable line. The envelope-aware +// PrintError writes err to w in human-readable form. The envelope-aware // JSON formatter is in `internal/format`; this helper is the human path used // when no command produced its own output. +// +// Typed *Error values surface their Hint as a second line so users see the +// actionable next-step (matches envelope.error.hint visibility in --json). +// Falls through to defaultHint when caller didn't set one. func PrintError(w io.Writer, err error) { if err == nil || errors.Is(err, SilentError) { return } fmt.Fprintln(w, err.Error()) + var typed *Error + if errors.As(err, &typed) { + hint := typed.Hint + if hint == "" { + hint = defaultHint(typed.Code) + } + if hint != "" { + fmt.Fprintf(w, "hint: %s\n", hint) + } + } } // PrintErrorEnvelope writes err as a JSON envelope on w. Used in agent mode / @@ -79,6 +93,9 @@ func ToErrorBody(err error) *format.ErrorBody { body.Code = string(typed.Code) body.Message = typed.Message body.Hint = typed.Hint + if body.Hint == "" { + body.Hint = defaultHint(typed.Code) + } body.Retryable = typed.Retryable // Surface the wrapped cause so agents see the actual server / SDK // error string, not just the wrap message ("hybrid search"). Stripe's @@ -98,3 +115,42 @@ func ToErrorBody(err error) *format.ErrorBody { body.Code = string(CodeServerError) return body } + +// defaultHint returns a canonical actionable hint for known error codes when +// the call site didn't set one. Spec §1.4 zero-config matrix mandates +// `auth.unauthenticated` envelopes carry "run weknora auth login" — this +// fallback covers the broad surface (whoami / auth status / kb list / kb get +// / search) without per-command hint plumbing. +// +// Empty string for codes without a stable canonical hint. +func defaultHint(code ErrorCode) string { + switch code { + case CodeAuthUnauthenticated, CodeAuthBadCredential: + return "run `weknora auth login`" + case CodeAuthTokenExpired: + return "your session expired; run `weknora auth login` to re-authenticate" + case CodeAuthForbidden: + return "active context lacks permission for this resource" + case CodeAuthCrossTenantBlocked, CodeAuthTenantMismatch: + return "verify tenant context with `weknora whoami`" + case CodeNetworkError: + return "check base URL reachability with `weknora doctor`" + case CodeServerIncompatibleVersion: + return "run `weknora doctor` to see version skew details" + case CodeServerRateLimited: + return "rate-limited; retry after a few seconds" + case CodeServerTimeout: + return "request timed out; retry, or run `weknora doctor` to check connectivity" + case CodeResourceNotFound: + return "verify the resource ID; list available with `weknora kb list`" + case CodeInputInvalidArgument, CodeInputMissingFlag: + return "see `weknora --help` for valid usage" + case CodeLocalKeychainDenied: + return "verify keyring access; falls back to file storage" + case CodeLocalConfigCorrupt: + return "remove ~/.config/weknora/config.yaml and re-run `weknora auth login`" + case CodeLocalFileIO: + return "check file permissions under $XDG_CONFIG_HOME/weknora/" + } + return "" +} diff --git a/cli/internal/cmdutil/factory.go b/cli/internal/cmdutil/factory.go index 992b743d..1cb81517 100644 --- a/cli/internal/cmdutil/factory.go +++ b/cli/internal/cmdutil/factory.go @@ -37,6 +37,12 @@ type Factory struct { Client func() (*sdk.Client, error) Prompter func() prompt.Prompter Secrets func() (secrets.Store, error) + + // ContextOverride, if non-empty, replaces config.CurrentContext for this + // invocation only — set by the global --context flag in PersistentPreRun. + // Buildable Config() / Client() honor it without writing to disk; matches + // spec §1.2 "weknora --context foo kb list = single-shot override". + ContextOverride string } // New constructs a production Factory wired to real config / SDK client. @@ -51,7 +57,22 @@ func New() *Factory { secretsErr error ) f := &Factory{} - f.Config = func() (*config.Config, error) { return config.Load() } + f.Config = func() (*config.Config, error) { + cfg, err := config.Load() + if err != nil { + // Map raw fs / parse errors to typed codes so envelopes don't + // surface bare `server.error` for what's actually a local IO / + // corrupt-config problem. + if errors.Is(err, config.ErrCorrupt) { + return nil, Wrapf(CodeLocalConfigCorrupt, err, "config malformed") + } + return nil, Wrapf(CodeLocalFileIO, err, "load config") + } + if f.ContextOverride != "" { + cfg.CurrentContext = f.ContextOverride + } + return cfg, nil + } f.Client = func() (*sdk.Client, error) { return buildClient(f) } f.Prompter = func() prompt.Prompter { if iostreams.IO.IsStdoutTTY() && iostreams.IO.IsStderrTTY() { diff --git a/cli/internal/cmdutil/factory_test.go b/cli/internal/cmdutil/factory_test.go index 67b18246..143993c0 100644 --- a/cli/internal/cmdutil/factory_test.go +++ b/cli/internal/cmdutil/factory_test.go @@ -2,6 +2,7 @@ package cmdutil import ( "errors" + "os" "testing" "github.com/stretchr/testify/assert" @@ -64,6 +65,49 @@ func TestNew_FoundationDefaults(t *testing.T) { assert.Equal(t, CodeAuthUnauthenticated, typed.Code) } +// TestFactory_ContextOverride verifies the global --context flag mechanism: +// f.ContextOverride replaces config.CurrentContext for this invocation only, +// without writing to disk. Spec §1.2. +func TestFactory_ContextOverride(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + // Seed config with two contexts; CurrentContext = "default" + cfgPath := dir + "/weknora/config.yaml" + require.NoError(t, os.MkdirAll(dir+"/weknora", 0o700)) + require.NoError(t, os.WriteFile(cfgPath, []byte(` +current_context: default +contexts: + default: + host: https://default.example + other: + host: https://other.example +`), 0o600)) + + f := New() + + t.Run("no override: returns CurrentContext from disk", func(t *testing.T) { + f.ContextOverride = "" + cfg, err := f.Config() + require.NoError(t, err) + assert.Equal(t, "default", cfg.CurrentContext) + }) + + t.Run("override applied: ContextOverride wins over disk", func(t *testing.T) { + f.ContextOverride = "other" + cfg, err := f.Config() + require.NoError(t, err) + assert.Equal(t, "other", cfg.CurrentContext) + }) + + t.Run("override does not persist to disk", func(t *testing.T) { + // Reload from disk: should still be "default" (the original). + raw, err := os.ReadFile(cfgPath) + require.NoError(t, err) + assert.Contains(t, string(raw), "current_context: default") + }) +} + // TestTypedPredicates exercises the namespace and code matchers. func TestTypedPredicates(t *testing.T) { t.Run("IsAuthError matches auth.* prefix", func(t *testing.T) { diff --git a/cli/internal/compat/cache.go b/cli/internal/compat/cache.go index 8cc4042f..a3ecd08e 100644 --- a/cli/internal/compat/cache.go +++ b/cli/internal/compat/cache.go @@ -4,24 +4,17 @@ import ( "errors" "fmt" "os" - "path/filepath" "time" "gopkg.in/yaml.v3" + + "github.com/Tencent/WeKnora/cli/internal/xdg" ) const ttl = 24 * time.Hour -// cachePath returns $XDG_CACHE_HOME/weknora/server-info.yaml,fallback ~/.cache/weknora/. func cachePath() (string, error) { - if x := os.Getenv("XDG_CACHE_HOME"); x != "" { - return filepath.Join(x, "weknora", "server-info.yaml"), nil - } - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("locate home dir: %w", err) - } - return filepath.Join(home, ".cache", "weknora", "server-info.yaml"), nil + return xdg.Path("XDG_CACHE_HOME", ".cache", "server-info.yaml") } // LoadCache reads the cached Info. Returns (info, fresh, err). @@ -54,19 +47,5 @@ func SaveCache(info *Info) error { if err != nil { return err } - if err := os.MkdirAll(filepath.Dir(p), 0700); err != nil { - return fmt.Errorf("mkdir cache dir: %w", err) - } - data, err := yaml.Marshal(info) - if err != nil { - return fmt.Errorf("marshal cache: %w", err) - } - tmp := p + ".tmp" - if err := os.WriteFile(tmp, data, 0600); err != nil { - return fmt.Errorf("write tmp cache: %w", err) - } - if err := os.Rename(tmp, p); err != nil { - return fmt.Errorf("rename cache: %w", err) - } - return nil + return xdg.WriteAtomicYAML(p, info) } diff --git a/cli/internal/config/config.go b/cli/internal/config/config.go index e9351aae..f5bb85fd 100644 --- a/cli/internal/config/config.go +++ b/cli/internal/config/config.go @@ -10,9 +10,10 @@ import ( "errors" "fmt" "os" - "path/filepath" "gopkg.in/yaml.v3" + + "github.com/Tencent/WeKnora/cli/internal/xdg" ) // Config is the on-disk schema. Empty zero-value is valid (returned when the @@ -45,19 +46,9 @@ type Context struct { var ErrCorrupt = errors.New("config: file is malformed") // Path returns the absolute config file path. -// -// We honor XDG_CONFIG_HOME on every OS (CLI convention — gh, kubectl, helm -// all do this even on macOS, where os.UserConfigDir would otherwise return -// ~/Library/Application Support). Falls back to ~/.config/weknora. +// Honors XDG_CONFIG_HOME via internal/xdg. func Path() (string, error) { - if x := os.Getenv("XDG_CONFIG_HOME"); x != "" { - return filepath.Join(x, "weknora", "config.yaml"), nil - } - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("locate home dir: %w", err) - } - return filepath.Join(home, ".config", "weknora", "config.yaml"), nil + return xdg.Path("XDG_CONFIG_HOME", ".config", "config.yaml") } // Load reads the config file. If it does not exist, returns a zero-value @@ -82,25 +73,11 @@ func Load() (*Config, error) { return &c, nil } -// Save writes the config atomically (write temp + rename) with mode 0600. +// Save writes the config atomically with mode 0600 via internal/xdg.WriteAtomicYAML. func Save(c *Config) error { p, err := Path() if err != nil { return err } - if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { - return fmt.Errorf("mkdir config dir: %w", err) - } - data, err := yaml.Marshal(c) - if err != nil { - return fmt.Errorf("marshal config: %w", err) - } - tmp := p + ".tmp" - if err := os.WriteFile(tmp, data, 0o600); err != nil { - return fmt.Errorf("write config: %w", err) - } - if err := os.Rename(tmp, p); err != nil { - return fmt.Errorf("rename config: %w", err) - } - return nil + return xdg.WriteAtomicYAML(p, c) } diff --git a/cli/internal/secrets/secrets.go b/cli/internal/secrets/secrets.go index 650ee8c3..1dac96bd 100644 --- a/cli/internal/secrets/secrets.go +++ b/cli/internal/secrets/secrets.go @@ -13,6 +13,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/Tencent/WeKnora/cli/internal/xdg" ) // ErrNotFound is returned when the requested secret does not exist. @@ -50,14 +52,7 @@ func NewFileStore() (*FileStore, error) { } func defaultRoot() (string, error) { - if x := os.Getenv("XDG_CONFIG_HOME"); x != "" { - return filepath.Join(x, "weknora", "secrets"), nil - } - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("locate home dir: %w", err) - } - return filepath.Join(home, ".config", "weknora", "secrets"), nil + return xdg.Path("XDG_CONFIG_HOME", ".config", "secrets") } func (f *FileStore) path(context, key string) string { diff --git a/cli/internal/xdg/xdg.go b/cli/internal/xdg/xdg.go new file mode 100644 index 00000000..b760927f --- /dev/null +++ b/cli/internal/xdg/xdg.go @@ -0,0 +1,86 @@ +// Package xdg consolidates the XDG-rooted file path lookup and atomic-write +// idioms used by config / compat / secrets / future stores. +// +// Before extraction these patterns were copy-pasted across cli/internal/{config, +// compat, secrets}; reuse review surfaced 3× duplication of Path and 2× of the +// tmp+rename atomic-write recipe. Centralizing here means a single place to fix +// behavior (mode bits, fallback dirs, mkdir order, error wrapping) and one less +// thing to copy when v0.2 adds project-link / state stores. +package xdg + +import ( + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +// Path resolves an XDG-rooted file. envVar is one of "XDG_CONFIG_HOME" / +// "XDG_CACHE_HOME" / "XDG_DATA_HOME". fallbackDir is the dot-prefixed dir +// under $HOME used when the env var is unset (".config" / ".cache" / etc.). +// parts join under "weknora/" inside the chosen root. +// +// Honors the XDG vars on every OS (CLI convention — gh / kubectl / helm all +// do this even on macOS, where os.UserConfigDir would otherwise return +// ~/Library/Application Support). +func Path(envVar, fallbackDir string, parts ...string) (string, error) { + if x := os.Getenv(envVar); x != "" { + return filepath.Join(append([]string{x, "weknora"}, parts...)...), nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("locate home dir: %w", err) + } + return filepath.Join(append([]string{home, fallbackDir, "weknora"}, parts...)...), nil +} + +// WriteAtomicYAML marshals v to YAML and writes it atomically at p with mode +// 0600 (user-only). Creates parent dirs with mode 0700. Atomicity via +// CreateTemp + chmod + rename so partial writes never expose a half-baked +// file and concurrent writers never trample each other's tmp. +// +// 0600 is the appropriate floor even for non-secret stores (cache, project +// link) since they may sit alongside secrets in the same dir tree. +// +// On any failure after the tmp is created, the tmp is cleaned up so we don't +// litter the dir with `*.tmp.NNNN` artifacts on crash / cross-device errors. +func WriteAtomicYAML(p string, v any) error { + dir := filepath.Dir(p) + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("mkdir %s: %w", dir, err) + } + data, err := yaml.Marshal(v) + if err != nil { + return fmt.Errorf("marshal: %w", err) + } + // CreateTemp picks a unique name in dir so concurrent calls never collide + // on a shared `

.tmp`. Pattern reserves the suffix slot. + tmp, err := os.CreateTemp(dir, filepath.Base(p)+".tmp.*") + if err != nil { + return fmt.Errorf("create tmp in %s: %w", dir, err) + } + tmpName := tmp.Name() + // Best-effort cleanup on any failure path; no-op once Rename succeeds. + committed := false + defer func() { + if !committed { + _ = os.Remove(tmpName) + } + }() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return fmt.Errorf("write %s: %w", tmpName, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close %s: %w", tmpName, err) + } + if err := os.Chmod(tmpName, 0o600); err != nil { + return fmt.Errorf("chmod %s: %w", tmpName, err) + } + if err := os.Rename(tmpName, p); err != nil { + return fmt.Errorf("rename %s -> %s: %w", tmpName, p, err) + } + committed = true + return nil +} diff --git a/cli/internal/xdg/xdg_test.go b/cli/internal/xdg/xdg_test.go new file mode 100644 index 00000000..fe15b014 --- /dev/null +++ b/cli/internal/xdg/xdg_test.go @@ -0,0 +1,59 @@ +package xdg_test + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/Tencent/WeKnora/cli/internal/xdg" +) + +func TestPath_HonorsEnv(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + got, err := xdg.Path("XDG_CONFIG_HOME", ".config", "config.yaml") + if err != nil { + t.Fatalf("Path: %v", err) + } + want := filepath.Join(dir, "weknora", "config.yaml") + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestPath_FallsBackToHome(t *testing.T) { + t.Setenv("XDG_CACHE_HOME", "") + got, err := xdg.Path("XDG_CACHE_HOME", ".cache", "server-info.yaml") + if err != nil { + t.Fatalf("Path: %v", err) + } + if !filepath.IsAbs(got) { + t.Errorf("expected absolute path, got %q", got) + } + if !strings.Contains(got, ".cache") || !strings.Contains(got, "weknora") { + t.Errorf("expected ~/.cache/weknora prefix, got %q", got) + } +} + +func TestWriteAtomicYAML_RoundTrip(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "sub", "file.yaml") + type doc struct { + Name string `yaml:"name"` + } + if err := xdg.WriteAtomicYAML(p, &doc{Name: "alice"}); err != nil { + t.Fatalf("WriteAtomicYAML: %v", err) + } + if _, err := os.Stat(p); err != nil { + t.Fatalf("file not written: %v", err) + } + if runtime.GOOS != "windows" { + info, _ := os.Stat(p) + if mode := info.Mode().Perm(); mode != 0o600 { + t.Errorf("mode = %v, want 0600", mode) + } + } +} +