mirror of
https://github.com/Tencent/WeKnora.git
synced 2026-10-31 03:47:56 +08:00
chore(cli): remove unused v0.0 scaffolding
Foundation PR-1 reserved several internal packages and helpers as
scaffolding for follow-up PRs that ended up taking different routes.
Audit confirms zero production references; this commit removes them so
the cli/ tree reflects what's actually shipped.
Removed (148 LOC):
cli/internal/safepaths/ — `Validate` / `WithinRoot` /
three sentinel errors. Reserved
for `weknora doc upload`'s path
scrubbing; that command landed
in PR-10 using its own
`validateUploadPath` (os.Stat +
regular-file check) — sufficient
for the actual threat model
(local CLI invocations).
cli/internal/cmdutil/json_flags.go — `AddJSONFlags` helper +
unused --jq / --template flag
registration. Reserved for PR-3
"lipgloss tables / jq evaluator"
which never materialized; every
command directly registers
BoolVar(&JSONOut, "json", ...)
since v0.0 ship time.
cmdutil.NewTableExporter — empty alias for jsonExporter,
reserved for the same PR-3
renderer. Removed; jsonExporter
stays under NewJSONExporter.
cmdutil.Options marker interface — empty interface{} reserved as a
convention; no command embeds
or asserts against it.
Stale comments fixed:
- cmd/root.go: package comment updated kb (list+get) → kb
(list+view+create+delete) and noted the `get` cobra alias.
- cmd/root.go: dropped --no-version-check forward-reference (no such flag).
- cmd/root.go: removed "(PR-7)" attribution from NewRootCmd doc comment.
- cmd/kb/kb.go: same package-comment update.
- cmd/chat/chat.go: replaced "PR-7" mention in --help example with a
generic placeholder so cobra-rendered help is review-clean.
- cmd/search/search.go: removed "Lipgloss tables arrive in PR-3"
forward-reference; the inline indent helper is the shipped form.
- internal/agent/annotations.go: ShouldUseAgentMode → DetectAIAgent
(removed in PR-12).
AGENTS.md "Known limitations" section added:
Documents that chat / search / doc upload currently surface server-side
precondition misses (LLM / vector store / storage engine not configured)
as `network.error` with `context deadline exceeded`. A planned future
release will introduce a `precondition.*` typed error namespace
(server returns HTTP 412 before opening the SSE / streaming response).
This documents the limitation honestly for reviewers and integrators
rather than claiming a behavior we don't yet have.
Tests: 27 cli packages pass (safepaths_test was the 28th — gone with the
package). go vet clean.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
+3
-1
@@ -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 (
|
||||
|
||||
+1
-4
@@ -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")
|
||||
|
||||
@@ -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)")
|
||||
|
||||
@@ -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 ""
|
||||
|
||||
@@ -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{} }
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 <expr> embedded jq filter (PR-3 wires the real evaluator)
|
||||
// --template <tmpl> 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()
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
@@ -6,11 +6,6 @@ import (
|
||||
"github.com/spf13/cobra"
|
||||
)
|
||||
|
||||
// Options is the marker interface every command's <Verb>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 —
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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))
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user