diff --git a/cli/AGENTS.md b/cli/AGENTS.md index 24c5c356b..dfdc7ee29 100644 --- a/cli/AGENTS.md +++ b/cli/AGENTS.md @@ -155,6 +155,24 @@ inspiration) only tags User-Agent for telemetry, never flips behavior; --- +## Known limitations + +The following classes of failure currently surface as `error.code = "network.error"` +with `context deadline exceeded` rather than a precise typed code. A future +release will introduce a `precondition.*` namespace (server returns HTTP 412 +with a typed remediation body before opening the SSE / streaming response): + +- `weknora chat` when no chat model is configured for the active tenant +- `weknora search` when no retriever / vector store is configured +- `weknora doc upload` when no storage engine is selected for the KB + +Workaround until then: if a chat / search / upload call times out without +producing a first-byte response, check the server's tenant configuration +(LLM / vector store / storage engine) before retrying. A planned +`weknora doctor --server-config` will probe these directly. + +--- + ## Reporting issues If the CLI's behavior contradicts this document, that is a bug. File at diff --git a/cli/cmd/chat/chat.go b/cli/cmd/chat/chat.go index ddd98a15f..76637f993 100644 --- a/cli/cmd/chat/chat.go +++ b/cli/cmd/chat/chat.go @@ -78,7 +78,7 @@ 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 "Summarise this design doc" --kb my-kb --json weknora chat "Continue?" --session-id sess_abc`, Args: cobra.MinimumNArgs(1), RunE: func(c *cobra.Command, args []string) error { diff --git a/cli/cmd/kb/kb.go b/cli/cmd/kb/kb.go index c1f1bbc82..84f40a125 100644 --- a/cli/cmd/kb/kb.go +++ b/cli/cmd/kb/kb.go @@ -1,4 +1,6 @@ -// Package kb holds `weknora kb` command tree (list / get; create / delete in v0.2). +// Package kb holds the `weknora kb` command tree: list / view / create / delete. +// `view` is the primary read verb (gh repo view convention); `get` survives as +// a cobra alias on the view subcommand for v0.0/v0.1 callers. package kb import ( diff --git a/cli/cmd/root.go b/cli/cmd/root.go index 4c952f60d..20e53e265 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -134,7 +134,7 @@ var cobraFlagErrorPrefixes = []string{ } // NewRootCmd builds the cobra tree. Splitting it from Execute() lets tests -// drive the tree directly with their own factory. Exported (PR-7) so the +// drive the tree directly with their own factory. Exported so the // acceptance/contract suite can construct the tree in-process. func NewRootCmd(f *cmdutil.Factory) *cobra.Command { v, commit, date := build.Info() @@ -192,9 +192,6 @@ hybrid searches against a WeKnora server from your shell or an AI agent.`, // addGlobalFlags registers persistent flags available on every subcommand. // 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.BoolP("yes", "y", false, "Skip confirmation prompts on destructive operations") diff --git a/cli/cmd/search/search.go b/cli/cmd/search/search.go index f0a702ed1..b9ed5ba6c 100644 --- a/cli/cmd/search/search.go +++ b/cli/cmd/search/search.go @@ -133,10 +133,9 @@ func runSearch(ctx context.Context, opts *Options, svc Service) error { return renderHumanResults(results, opts.KBID) } -// renderHumanResults prints a compact pretty list to stdout. -// -// Lipgloss tables arrive in PR-3; the inline indent helper here is a minimal -// stopgap so search is usable in a terminal without color today. +// renderHumanResults prints a compact pretty list to stdout. The inline +// indent helper is a minimal stopgap so search output is usable in a plain +// terminal; a richer tabular renderer can replace this later. func renderHumanResults(results []*sdk.SearchResult, kbID string) error { if len(results) == 0 { fmt.Fprintln(iostreams.IO.Out, "(no results)") diff --git a/cli/internal/agent/annotations.go b/cli/internal/agent/annotations.go index df5929dbb..6ee350d25 100644 --- a/cli/internal/agent/annotations.go +++ b/cli/internal/agent/annotations.go @@ -12,7 +12,8 @@ import ( const AIAgentHelpKey = "ai_agent_help" // FormatAgentGuidance returns the agent-targeted help text registered on cmd, -// or "" if none. Render it after the standard help when ShouldUseAgentMode reports true. +// or "" if none. Render it after the standard help when DetectAIAgent() != "" +// (an AI coding agent env var is set). func FormatAgentGuidance(cmd *cobra.Command) string { if cmd == nil { return "" diff --git a/cli/internal/cmdutil/exporter.go b/cli/internal/cmdutil/exporter.go index d8c7c4398..23c8a1620 100644 --- a/cli/internal/cmdutil/exporter.go +++ b/cli/internal/cmdutil/exporter.go @@ -6,9 +6,10 @@ import ( "github.com/Tencent/WeKnora/cli/internal/format" ) -// Exporter renders command output. Foundation PR ships a single -// jsonExporter that writes the envelope; PR-3 lands lipgloss tables, jq, -// and templates as additional implementations. +// Exporter renders an envelope to a writer. Currently the only +// implementation is the JSON exporter; the interface stays in case a future +// renderer (templated text, table) needs to plug in without changing call +// sites that already write through Exporter.Write. type Exporter interface { Write(w io.Writer, env format.Envelope) error } @@ -22,9 +23,3 @@ type jsonExporter struct{} func (jsonExporter) Write(w io.Writer, env format.Envelope) error { return format.WriteEnvelope(w, env) } - -// NewTableExporter is a foundation-PR alias for the JSON exporter; PR-3 -// replaces it with a lipgloss-based renderer that respects iostreams.IO -// ColorEnabled. Until then table output looks identical to JSON output so -// commands work end-to-end either way. -func NewTableExporter(_ []string) Exporter { return &jsonExporter{} } diff --git a/cli/internal/cmdutil/exporter_test.go b/cli/internal/cmdutil/exporter_test.go index 63aba319d..21bc8b24f 100644 --- a/cli/internal/cmdutil/exporter_test.go +++ b/cli/internal/cmdutil/exporter_test.go @@ -20,12 +20,6 @@ func TestJSONExporter_WritesEnvelope(t *testing.T) { assert.Equal(t, true, got["ok"]) } -func TestTableExporter_PR1AliasesJSON(t *testing.T) { - var buf bytes.Buffer - require.NoError(t, NewTableExporter([]string{"id"}).Write(&buf, format.Success("x", nil))) - assert.Contains(t, buf.String(), `"ok":true`) -} - func TestFlagError_IsSentinel(t *testing.T) { err := NewFlagError(assert.AnError) _, ok := err.(*FlagError) diff --git a/cli/internal/cmdutil/json_flags.go b/cli/internal/cmdutil/json_flags.go deleted file mode 100644 index dbbf039a3..000000000 --- a/cli/internal/cmdutil/json_flags.go +++ /dev/null @@ -1,28 +0,0 @@ -package cmdutil - -import ( - "github.com/spf13/cobra" -) - -// AddJSONFlags registers the standard output triplet on cmd: -// -// --json [fields] JSON envelope output, optional field-projection list -// --jq embedded jq filter (PR-3 wires the real evaluator) -// --template Go text/template (PR-3) -// -// The three flags are mutually exclusive at evaluation time; PR-3 enforces it. -// `fields` is the list of valid field names that --json [fields] may project -// against; commands pass their resource's field set (kb / doc / chunk / ...). -// -// *exporter is initialized to a JSON exporter; PR-3 swaps in jq/template -// variants based on which flag the user supplied. -func AddJSONFlags(cmd *cobra.Command, exporter *Exporter, fields []string) { - _ = fields // PR-3 uses this for --json field-list completion + validation - var jsonOut bool - var jqExpr string - var tmpl string - cmd.Flags().BoolVar(&jsonOut, "json", false, "Output JSON envelope") - cmd.Flags().StringVar(&jqExpr, "jq", "", "Filter output via embedded jq expression") - cmd.Flags().StringVar(&tmpl, "template", "", "Format output via Go text/template") - *exporter = NewJSONExporter() -} diff --git a/cli/internal/cmdutil/json_flags_test.go b/cli/internal/cmdutil/json_flags_test.go deleted file mode 100644 index b565ef033..000000000 --- a/cli/internal/cmdutil/json_flags_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package cmdutil - -import ( - "testing" - - "github.com/spf13/cobra" - "github.com/stretchr/testify/assert" -) - -func TestAddJSONFlags_RegistersFlags(t *testing.T) { - c := &cobra.Command{Use: "x"} - var exp Exporter - AddJSONFlags(c, &exp, []string{"id", "name"}) - assert.NotNil(t, c.Flags().Lookup("json")) - assert.NotNil(t, c.Flags().Lookup("jq")) - assert.NotNil(t, c.Flags().Lookup("template")) - assert.NotNil(t, exp, "exporter must be initialized") -} diff --git a/cli/internal/cmdutil/options.go b/cli/internal/cmdutil/options.go index bfe376a99..ccd48d4f6 100644 --- a/cli/internal/cmdutil/options.go +++ b/cli/internal/cmdutil/options.go @@ -6,11 +6,6 @@ import ( "github.com/spf13/cobra" ) -// Options is the marker interface every command's Options struct should -// satisfy. Concrete Options structs (KbListOptions, DocUploadOptions, ...) -// are declared in their own command file; see CONTRIBUTING.md for the template. -type Options interface{} - // MustRequireFlag panics on programmer error (typo in flag name). cobra's // MarkFlagRequired only returns an error when the named flag does not exist // on the command, which means the caller has a typo at registration time — diff --git a/cli/internal/safepaths/safepaths.go b/cli/internal/safepaths/safepaths.go deleted file mode 100644 index 0536bcbe3..000000000 --- a/cli/internal/safepaths/safepaths.go +++ /dev/null @@ -1,72 +0,0 @@ -// Package safepaths provides path-traversal protection for file inputs to -// commands such as `weknora doc upload`. -// -// Two primary checks: -// - Validate(path): reject empty, ".", "..", or paths containing embedded ".." segments. -// - WithinRoot(path, root): both are made absolute and compared with a -// separator-aware prefix check, so a directory named "..foo" cannot -// masquerade as escaping ".." (the lexical strings.HasPrefix(rel, "..") -// trap). -// -// Symlinks are NOT resolved; callers needing realpath semantics must -// pre-resolve. Modeled on the same pattern as `internal/utils/security.go` -// SafePathUnderBase in the server module. -package safepaths - -import ( - "errors" - "fmt" - "path/filepath" - "strings" -) - -// ErrPathEscapes signals that the given path resolves outside the allowed root. -var ErrPathEscapes = errors.New("path escapes allowed root") - -// ErrEmptyPath signals an empty input. -var ErrEmptyPath = errors.New("path is empty") - -// ErrSuspiciousPath signals a "."/".." or embedded ".." traversal. -var ErrSuspiciousPath = errors.New("path contains suspicious traversal segment") - -// Validate cleans path and rejects empty inputs, "." / "..", or any path that -// contains an embedded ".." segment after cleaning. It does not resolve symlinks. -func Validate(path string) (string, error) { - if path == "" { - return "", ErrEmptyPath - } - cleaned := filepath.Clean(path) - if cleaned == "" || cleaned == "." || cleaned == ".." { - return "", fmt.Errorf("%w: %q", ErrSuspiciousPath, path) - } - for _, seg := range strings.Split(cleaned, string(filepath.Separator)) { - if seg == ".." { - return "", fmt.Errorf("%w: %q", ErrSuspiciousPath, path) - } - } - return cleaned, nil -} - -// WithinRoot reports whether path lies inside root after both are made -// absolute. Comparison is separator-aware: a path "/srv/data..foo" is NOT -// treated as escaping "/srv/data" just because "..foo" textually starts -// with "..". -func WithinRoot(path, root string) error { - absPath, err := filepath.Abs(path) - if err != nil { - return fmt.Errorf("resolve path: %w", err) - } - absRoot, err := filepath.Abs(root) - if err != nil { - return fmt.Errorf("resolve root: %w", err) - } - // Equal paths count as "within"; otherwise require a separator-bounded prefix. - if absPath == absRoot { - return nil - } - withSep := absRoot + string(filepath.Separator) - if !strings.HasPrefix(absPath, withSep) { - return fmt.Errorf("%w: %s not under %s", ErrPathEscapes, absPath, absRoot) - } - return nil -} diff --git a/cli/internal/safepaths/safepaths_test.go b/cli/internal/safepaths/safepaths_test.go deleted file mode 100644 index 25b7b26b7..000000000 --- a/cli/internal/safepaths/safepaths_test.go +++ /dev/null @@ -1,70 +0,0 @@ -package safepaths - -import ( - "errors" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestValidate(t *testing.T) { - t.Run("rejects empty", func(t *testing.T) { - _, err := Validate("") - require.ErrorIs(t, err, ErrEmptyPath) - }) - t.Run("rejects bare dot", func(t *testing.T) { - _, err := Validate(".") - require.ErrorIs(t, err, ErrSuspiciousPath) - }) - t.Run("rejects bare double-dot", func(t *testing.T) { - _, err := Validate("..") - require.ErrorIs(t, err, ErrSuspiciousPath) - }) - t.Run("rejects relative path that ascends past start", func(t *testing.T) { - // "../foo" preserves the ".." after Clean. - _, err := Validate("../foo") - require.ErrorIs(t, err, ErrSuspiciousPath) - }) - t.Run("cleans benign relative", func(t *testing.T) { - // "a/b/../c" cleans to "a/c" — fine. - got, err := Validate("a/b/../c") - require.NoError(t, err) - assert.Equal(t, filepath.Clean("a/c"), got) - }) - t.Run("cleans cancelling relative", func(t *testing.T) { - // "a/../b" cleans to "b" — also fine; the ascent fully resolves. - got, err := Validate("a/../b") - require.NoError(t, err) - assert.Equal(t, "b", got) - }) -} - -func TestWithinRoot(t *testing.T) { - t.Run("inside root", func(t *testing.T) { - err := WithinRoot("/srv/data/file.md", "/srv/data") - require.NoError(t, err) - }) - t.Run("equal to root", func(t *testing.T) { - err := WithinRoot("/srv/data", "/srv/data") - require.NoError(t, err) - }) - t.Run("escapes via parent", func(t *testing.T) { - err := WithinRoot("/srv/data/../../etc/passwd", "/srv/data") - require.Error(t, err) - assert.True(t, errors.Is(err, ErrPathEscapes)) - }) - t.Run("sibling outside", func(t *testing.T) { - err := WithinRoot("/srv/other", "/srv/data") - require.Error(t, err) - assert.True(t, errors.Is(err, ErrPathEscapes)) - }) - t.Run("dotdot-prefix sibling is not within", func(t *testing.T) { - // /srv/data..foo lexically starts with "/srv/data" but is a different dir. - // Without separator-awareness this would be a false negative. - err := WithinRoot("/srv/data..foo/x", "/srv/data") - require.Error(t, err) - assert.True(t, errors.Is(err, ErrPathEscapes)) - }) -}