Files
WeKnora/cli/cmd/doc/download.go
T
nullkey 4a5449233d fix(cli): plug v0.3 final review findings (json + auth + path + bounds + kb)
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.
2026-05-14 10:57:17 +08:00

155 lines
5.2 KiB
Go

package doc
import (
"context"
"errors"
"fmt"
"io"
"os"
"path/filepath"
"github.com/spf13/cobra"
"github.com/Tencent/WeKnora/cli/internal/agent"
"github.com/Tencent/WeKnora/cli/internal/cmdutil"
"github.com/Tencent/WeKnora/cli/internal/iostreams"
)
type DownloadOptions struct {
Output string // --output / -O: target path, "-" for stdout, "" for server-suggested filename
Clobber bool // --clobber: allow overwrite of an existing file
}
// DownloadService is the narrow SDK surface this command depends on. The
// CLI calls OpenKnowledgeFile so it can inspect the server-suggested
// filename and refuse-to-overwrite *before* streaming any bytes.
type DownloadService interface {
OpenKnowledgeFile(ctx context.Context, knowledgeID string) (string, io.ReadCloser, error)
}
// NewCmdDownload builds `weknora doc download <id>`. Positional id, output
// flag, `-` sentinel for stdout. Flags: `-O, --output <file>` for
// destination, `--clobber` for overwrite control.
func NewCmdDownload(f *cmdutil.Factory) *cobra.Command {
opts := &DownloadOptions{}
cmd := &cobra.Command{
Use: "download <id>",
Short: "Download a document by ID",
Long: `Streams the document bytes to disk (or stdout with --output -).
Default behavior (no --output): writes to the cwd under the filename the
server suggests via Content-Disposition. If the server doesn't suggest
one, the command errors and asks for --output FILE explicitly.
Existing files are NOT overwritten unless --clobber is passed.`,
Example: ` weknora doc download doc_abc # writes ./<server-name>
weknora doc download doc_abc -O report.pdf
weknora doc download doc_abc --output - # stream to stdout (binary safe)
weknora doc download doc_abc -O report.pdf --clobber`,
Args: cobra.ExactArgs(1),
RunE: func(c *cobra.Command, args []string) error {
cli, err := f.Client()
if err != nil {
return err
}
return runDownload(c.Context(), opts, cli, args[0])
},
}
cmd.Flags().StringVarP(&opts.Output, "output", "O", "", `Output path; "-" for stdout. Defaults to the server-suggested filename.`)
cmd.Flags().BoolVar(&opts.Clobber, "clobber", false, "Overwrite the output file if it already exists")
agent.SetAgentHelp(cmd, "Downloads a document. Agents that pipe the bytes should pass --output - (stdout). The exit code is 0 on success, 1 on resource.not_found, 5 on missing --output when the server didn't supply a filename.")
return cmd
}
func runDownload(ctx context.Context, opts *DownloadOptions, svc DownloadService, id string) error {
suggested, body, err := svc.OpenKnowledgeFile(ctx, id)
if err != nil {
return cmdutil.WrapHTTP(err, "download %s", id)
}
defer body.Close()
dest, err := resolveDownloadDest(opts, suggested)
if err != nil {
return err
}
if dest == "-" {
_, err := io.Copy(iostreams.IO.Out, body)
return err
}
if err := refuseIfExists(dest, opts.Clobber); err != nil {
return err
}
return streamToFile(body, dest)
}
// resolveDownloadDest returns the final destination ("-" for stdout, an
// absolute or relative path otherwise) after applying the --output flag
// and sanitizing the server-suggested name. A server that returns a path-
// like filename (..\, /etc/foo) is rejected — only the basename is
// accepted.
func resolveDownloadDest(opts *DownloadOptions, suggested string) (string, error) {
if opts.Output == "-" {
return "-", nil
}
if opts.Output != "" {
return opts.Output, nil
}
if suggested == "" {
return "", &cmdutil.Error{
Code: cmdutil.CodeInputMissingFlag,
Message: "server did not supply a filename and --output is unset",
Hint: "pass --output FILE (or --output - for stdout)",
}
}
base := filepath.Base(suggested)
if base == "" || base == "." || base == ".." || base == string(filepath.Separator) {
return "", &cmdutil.Error{
Code: cmdutil.CodeInputInvalidArgument,
Message: fmt.Sprintf("server returned an unusable filename %q", suggested),
Hint: "pass --output FILE explicitly",
}
}
return base, nil
}
// refuseIfExists returns CodeInputInvalidArgument when path is present on
// disk and clobber is false. Missing-file is success.
func refuseIfExists(path string, clobber bool) error {
if clobber {
return nil
}
_, err := os.Stat(path)
if errors.Is(err, os.ErrNotExist) {
return nil
}
if err != nil {
return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "stat %s", path)
}
return &cmdutil.Error{
Code: cmdutil.CodeInputInvalidArgument,
Message: fmt.Sprintf("%s already exists", path),
Hint: "pass --clobber to overwrite",
}
}
// streamToFile copies body into a newly-created file at path. On any
// streaming error the partial file is removed so callers don't see a
// truncated artifact at the user-visible path.
func streamToFile(body io.Reader, path string) error {
f, err := os.Create(path)
if err != nil {
return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "create %s", path)
}
if _, err := io.Copy(f, body); err != nil {
_ = f.Close()
_ = os.Remove(path)
return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "write %s", path)
}
if err := f.Close(); err != nil {
_ = os.Remove(path)
return cmdutil.Wrapf(cmdutil.CodeLocalFileIO, err, "close %s", path)
}
fmt.Fprintf(iostreams.IO.Err, "✓ Saved %s\n", path)
return nil
}