4a5449233d
Seven bugs surfaced via two audit rounds — parallel reviewer agents
plus a real-server end-to-end demo. Each fix arrives with a
regression test.
1. doc upload --recursive --json corrupted the envelope stream.
Per-file FAIL/OK plain lines printed unconditionally to stdout,
then a Success envelope, then on partial failure a typed error
that the root handler turned into a SECOND Failure envelope —
three outputs where one was expected. Fix: gate the plain lines
behind !opts.JSONOut, and add cmdutil.Error.Silent so the JSON-
path partial-failure preserves its typed exit code without
triggering PrintErrorEnvelope's default Failure-envelope write.
2. auth refresh / AuthRetryTransport misclassified HTTP failures as
network.error. RefreshAndPersist wrapped every refresher error
with CodeNetworkError, but the SDK emits "HTTP error 401: ..."
for a rejected refresh token — which should surface as
auth.token_expired. Switched to WrapHTTP for proper status-
derived classification. Affects both `auth refresh` and the
transport's refresh closure.
3. doc download accepted ".." as a server-suggested filename. The
rejection list covered "" / "." / filepath.Separator but not
bare ".." — filepath.Base("..") is "..", which slipped through
to os.Create and produced a confusing local.file_io wrap. Added
to the rejection set.
4. search chunks / docs / kb / sessions had no lower bound on
--limit. `-L 0` / `-L -1` was forwarded to the server with
undefined behavior. Added a 1..1000 bound at the RunE boundary
across all four (matching doc list / session list page-size
bounds). Internal callers in tests can still pass Limit==0 for
the "no client-side cap" runChunks path — the bound only applies
at the user-input layer.
5. cli/AGENTS.md ADR-3 verb-canon summary listed only v0.2 verbs as
"gh-canonical" and missed v0.3 additions (edit, pin, unpin,
download — all gh-canonical) plus locally-introduced ones
(empty, refresh, add, remove, link). Rewritten as an explicit
gh-canonical / locally-introduced split.
6. kb pin returned 404. Server registers /knowledge-bases/{id}/pin
as PUT (router.go:292); SDK was using POST. gin's router silently
404s on method-mismatch (treats it as path-not-found, not 405),
so the CLI classified the response as resource.not_found and
masked the real failure mode. Switched the SDK to http.MethodPut.
The asymmetry that hid this past round 1: kb unpin on a freshly-
created KB hits the no-op branch in cmd/kb/pin.go that skips the
SDK call entirely, so unpin "worked" without ever exercising the
broken path. Only the real-server demo, where kb pin actually
fires, surfaced it.
7. kb edit clobbered current Name when only --description was
passed. EditOptions used *string to distinguish "unset" from
"set to empty", but sdk.UpdateKnowledgeBaseRequest declares both
fields as plain string (no omitempty), so the JSON body always
carried `"name": ""`. Server requires Name → 400. Fix: runEdit
does fetch-then-update — GetKnowledgeBase first, build the PUT
body with current values, then overlay user-set fields. Same
TOCTOU window as kb pin / unpin.
Audit-flagged items intentionally NOT changed:
- kb pin / unpin check-then-toggle TOCTOU: documented; the clean
fix would be a server-side setter and belongs in a separate API
change.
- AuthRetryTransport singleflight test gap for one concurrency
scenario; v0.4 polish.
- cli/README.md:50 "once v0.2 ships" and CHANGELOG.md:8
"10 top-level commands": v0.2-PR artifacts, not v0.3-introduced.
- kb edit / kb pin are v0.3-new commands, so neither bug needs a
cli/CHANGELOG.md Fixed entry — the v0.3 release ships them
working as the Added bullets advertise.
196 lines
6.2 KiB
Go
196 lines
6.2 KiB
Go
package doc
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"os"
|
|
"path/filepath"
|
|
"sort"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/Tencent/WeKnora/cli/internal/cmdutil"
|
|
"github.com/Tencent/WeKnora/cli/internal/iostreams"
|
|
sdk "github.com/Tencent/WeKnora/client"
|
|
)
|
|
|
|
// scriptedUploadSvc records every CreateKnowledgeFromFile call and returns
|
|
// per-path scripted results.
|
|
type scriptedUploadSvc struct {
|
|
results map[string]struct {
|
|
k *sdk.Knowledge
|
|
err error
|
|
}
|
|
called []string
|
|
}
|
|
|
|
func (s *scriptedUploadSvc) CreateKnowledgeFromFile(
|
|
_ context.Context,
|
|
_, filePath string,
|
|
_ map[string]string,
|
|
_ *bool,
|
|
_, _ string,
|
|
) (*sdk.Knowledge, error) {
|
|
s.called = append(s.called, filepath.Base(filePath))
|
|
r, ok := s.results[filepath.Base(filePath)]
|
|
if !ok {
|
|
return &sdk.Knowledge{ID: "doc_" + filepath.Base(filePath), FileName: filepath.Base(filePath)}, nil
|
|
}
|
|
return r.k, r.err
|
|
}
|
|
|
|
func mkTree(t *testing.T, base string, names ...string) {
|
|
t.Helper()
|
|
for _, n := range names {
|
|
full := filepath.Join(base, n)
|
|
require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755))
|
|
require.NoError(t, os.WriteFile(full, []byte("x"), 0o644))
|
|
}
|
|
}
|
|
|
|
func TestUploadRecursive_WalksAllFiles(t *testing.T) {
|
|
out, _ := iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "a.pdf", "b.pdf", "sub/c.pdf")
|
|
|
|
svc := &scriptedUploadSvc{}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*"}
|
|
require.NoError(t, runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir))
|
|
|
|
sort.Strings(svc.called)
|
|
assert.Equal(t, []string{"a.pdf", "b.pdf", "c.pdf"}, svc.called)
|
|
got := out.String()
|
|
for _, w := range []string{"a.pdf", "b.pdf", "c.pdf", "Uploaded 3"} {
|
|
assert.Contains(t, got, w)
|
|
}
|
|
}
|
|
|
|
func TestUploadRecursive_GlobFilter(t *testing.T) {
|
|
_, _ = iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "doc.pdf", "ignore.txt", "sub/keep.pdf", "sub/also-ignore.md")
|
|
|
|
svc := &scriptedUploadSvc{}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*.pdf"}
|
|
require.NoError(t, runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir))
|
|
|
|
sort.Strings(svc.called)
|
|
assert.Equal(t, []string{"doc.pdf", "keep.pdf"}, svc.called)
|
|
}
|
|
|
|
func TestUploadRecursive_PartialFailure_Exits1(t *testing.T) {
|
|
out, _ := iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "ok.pdf", "bad.pdf")
|
|
|
|
svc := &scriptedUploadSvc{results: map[string]struct {
|
|
k *sdk.Knowledge
|
|
err error
|
|
}{
|
|
"bad.pdf": {err: errors.New("HTTP error 500: internal")},
|
|
}}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*"}
|
|
err := runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir)
|
|
require.Error(t, err)
|
|
|
|
var typed *cmdutil.Error
|
|
require.ErrorAs(t, err, &typed)
|
|
// CodeServerError preserves the 500 classification of the underlying
|
|
// SDK error — the recursive wrapper just aggregates.
|
|
assert.Equal(t, cmdutil.CodeServerError, typed.Code)
|
|
|
|
got := out.String()
|
|
assert.Contains(t, got, "OK") // ok.pdf still succeeded
|
|
assert.Contains(t, got, "FAIL")
|
|
assert.Contains(t, got, "Uploaded 1")
|
|
assert.Contains(t, got, "Failed 1")
|
|
}
|
|
|
|
func TestUploadRecursive_NoMatches(t *testing.T) {
|
|
out, _ := iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "only.txt")
|
|
|
|
svc := &scriptedUploadSvc{}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*.pdf"}
|
|
require.NoError(t, runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir))
|
|
assert.Len(t, svc.called, 0)
|
|
assert.Contains(t, strings.ToLower(out.String()), "no files matched")
|
|
}
|
|
|
|
func TestUploadRecursive_NotADirectory(t *testing.T) {
|
|
_, _ = iostreams.SetForTest(t)
|
|
path := writeTempFile(t, "single.pdf")
|
|
svc := &scriptedUploadSvc{}
|
|
err := runUploadRecursive(context.Background(), &UploadOptions{Recursive: true, Glob: "*"}, svc, "kb_xxx", path)
|
|
require.Error(t, err)
|
|
var typed *cmdutil.Error
|
|
require.ErrorAs(t, err, &typed)
|
|
assert.Equal(t, cmdutil.CodeInputInvalidArgument, typed.Code)
|
|
assert.Contains(t, typed.Message, "directory")
|
|
}
|
|
|
|
func TestUploadRecursive_RejectsNameFlag(t *testing.T) {
|
|
_, _ = iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "a.pdf")
|
|
svc := &scriptedUploadSvc{}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*", Name: "single-name.pdf"}
|
|
err := runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir)
|
|
require.Error(t, err)
|
|
var typed *cmdutil.Error
|
|
require.ErrorAs(t, err, &typed)
|
|
assert.Equal(t, cmdutil.CodeInputInvalidArgument, typed.Code)
|
|
assert.Contains(t, typed.Message, "--name")
|
|
}
|
|
|
|
func TestUploadRecursive_JSON_Envelope(t *testing.T) {
|
|
out, _ := iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "ok.pdf", "bad.pdf")
|
|
|
|
svc := &scriptedUploadSvc{results: map[string]struct {
|
|
k *sdk.Knowledge
|
|
err error
|
|
}{
|
|
"bad.pdf": {err: errors.New("HTTP error 500: internal")},
|
|
}}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*", JSONOut: true}
|
|
err := runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir)
|
|
require.Error(t, err) // partial failure → typed error
|
|
|
|
body := out.String()
|
|
assert.Contains(t, body, `"kb_id":"kb_xxx"`)
|
|
assert.Contains(t, body, `"uploaded":`)
|
|
assert.Contains(t, body, `"failed":`)
|
|
assert.Contains(t, body, `ok.pdf`)
|
|
assert.Contains(t, body, `bad.pdf`)
|
|
|
|
// --json must emit exactly ONE envelope. Per-file "FAIL"/"OK" progress
|
|
// lines belong on the human path; the typed error is Silent so the root
|
|
// handler doesn't write a second Failure envelope on top of ours.
|
|
assert.NotContains(t, body, "FAIL ", "per-file plain lines must not appear under --json")
|
|
assert.NotContains(t, body, "OK ", "per-file plain lines must not appear under --json")
|
|
assert.Equal(t, 1, strings.Count(body, `"ok":`), "exactly one envelope on stdout")
|
|
|
|
var typed *cmdutil.Error
|
|
require.ErrorAs(t, err, &typed)
|
|
assert.True(t, typed.Silent, "JSON-path partial failure must be Silent")
|
|
assert.Equal(t, cmdutil.CodeServerError, typed.Code)
|
|
}
|
|
|
|
func TestUploadRecursive_DryRun(t *testing.T) {
|
|
out, _ := iostreams.SetForTest(t)
|
|
dir := t.TempDir()
|
|
mkTree(t, dir, "a.pdf", "b.pdf")
|
|
svc := &scriptedUploadSvc{}
|
|
opts := &UploadOptions{Recursive: true, Glob: "*", DryRun: true}
|
|
require.NoError(t, runUploadRecursive(context.Background(), opts, svc, "kb_xxx", dir))
|
|
assert.Len(t, svc.called, 0, "dry-run must not call SDK")
|
|
got := out.String()
|
|
assert.Contains(t, got, "would upload 2")
|
|
}
|