From d7bb1af12af9cfb9721b218388f17caea7682cf7 Mon Sep 17 00:00:00 2001 From: nullkey Date: Thu, 14 May 2026 19:06:38 +0800 Subject: [PATCH 1/3] fix(create): exit 5 on InitTask insufficient_credits (0.5.1) InitTask's insufficient_credits handler returned a plain fmt.Errorf which cobra mapped to its default exit 1, while the same condition surfacing later in the stream path explicitly os.Exit(5). The documented contract (AGENTS.md, README) is 5 = business failure; agents branching on exit codes had to know about two paths. Caught during real-backend smoke for 0.5.0 (depleted credits during testing made the inconsistency observable). Match the stream-side pattern verbatim: print the i18n message to stderr, os.Exit(5). Add an integration test that mocks /v1/tasks/init returning envelope code 100001 and asserts the binary exits 5. --- CHANGELOG.md | 12 +++++ cmd/create.go | 4 +- package.json | 2 +- skills/vibeknow-core/SKILL.md | 2 +- skills/vibeknow-create/SKILL.md | 2 +- skills/vibeknow-doc/SKILL.md | 2 +- tests/integration/create_credits_test.go | 60 ++++++++++++++++++++++++ 7 files changed, 79 insertions(+), 5 deletions(-) create mode 100644 tests/integration/create_credits_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d1b403..956c7d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## 0.5.1 — 2026-05-14 + +### Fixed + +- `vk create` now exits **5** (business failure) when the backend rejects + `POST /v1/tasks/init` with `insufficient_credits` (envelope code 100001), + matching the stream-side path's existing behavior. Previously this case + exited 1 (cobra's generic error code), inconsistent with the documented + exit-code contract and with the same condition surfacing later in the + pipeline. Caught while running real-backend smoke tests during 0.5.0 + validation. + ## 0.5.0 — 2026-05-14 ### New diff --git a/cmd/create.go b/cmd/create.go index 340a46c..4dc016b 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -149,7 +149,9 @@ var createCmd = &cobra.Command{ task, err := fc.InitTask(ctx, initParams) if err != nil { if errs.HasCode(err, "insufficient_credits") { - return fmt.Errorf("%s", i18n.T("credits.insufficient")) + // Mirror the stream-side path's exit code: business failure → 5. + fmt.Fprintln(os.Stderr, i18n.T("credits.insufficient")) + os.Exit(5) } if errs.HasCode(err, "script_invalid") { // Backend's localized message already lives on the error. diff --git a/package.json b/package.json index 4b9624b..3791223 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "vibeknow-cli", - "version": "0.5.0", + "version": "0.5.1", "description": "VibeKnow CLI — turn docs / URLs into videos", "license": "MIT", "bin": { diff --git a/skills/vibeknow-core/SKILL.md b/skills/vibeknow-core/SKILL.md index 587f706..7f70ca7 100644 --- a/skills/vibeknow-core/SKILL.md +++ b/skills/vibeknow-core/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-core -version: 0.5.0 +version: 0.5.1 description: "vibeknow CLI setup, authentication, profile management, and diagnostics. Use when: first-time setup, auth errors, switching environments, diagnosing connection issues." metadata: requires: diff --git a/skills/vibeknow-create/SKILL.md b/skills/vibeknow-create/SKILL.md index 5b1451d..e7a943c 100644 --- a/skills/vibeknow-create/SKILL.md +++ b/skills/vibeknow-create/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-create -version: 0.5.0 +version: 0.5.1 description: "Generate videos from documents/URLs/files, track video task progress, download results, list voice templates. Use when: user wants to create a video, check task status, download video, or browse voices." metadata: requires: diff --git a/skills/vibeknow-doc/SKILL.md b/skills/vibeknow-doc/SKILL.md index b8ee7e3..e3217ce 100644 --- a/skills/vibeknow-doc/SKILL.md +++ b/skills/vibeknow-doc/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-doc -version: 0.5.0 +version: 0.5.1 description: "Upload documents to vectoria and check processing status. Use when: user wants to upload a document, check if a document is ready, or get a doc_id for use with vibeknow create." metadata: requires: diff --git a/tests/integration/create_credits_test.go b/tests/integration/create_credits_test.go new file mode 100644 index 0000000..80e4068 --- /dev/null +++ b/tests/integration/create_credits_test.go @@ -0,0 +1,60 @@ +package integration + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "strings" + "testing" +) + +// TestCreate_InsufficientCreditsOnInit_Exits5 covers the bug fixed in 0.5.1: +// when the backend rejects InitTask with envelope code 100001 (insufficient +// credits), the CLI must exit 5 (business failure) to match the stream-side +// path's behavior, not exit 1 from cobra's default error handler. +func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { + if testing.Short() { + t.Skip("integration test") + } + + mux := http.NewServeMux() + mux.HandleFunc("/v1/tasks/init", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusPaymentRequired) // 402, matches backend + _ = json.NewEncoder(w).Encode(map[string]any{ + "code": 100001, + "message": "insufficient credits", + }) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + bin := build(t) + configHome := buildVideoProfile(t, srv.URL) + + cmd := exec.Command(bin, "create", "--from", "doc_abc12345") + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + cmd.Env = append(os.Environ(), + "VIBEKNOW_TOKEN=fake-token", + "VIBEKNOW_CONFIG_HOME="+configHome, + ) + + err := cmd.Run() + code := 0 + if ee, ok := err.(*exec.ExitError); ok { + code = ee.ExitCode() + } else if err != nil { + t.Fatalf("run: %v\nstderr: %s", err, stderr.String()) + } + + if code != 5 { + t.Fatalf("exit code = %d, want 5 (business failure)\nstderr: %s", code, stderr.String()) + } + if !strings.Contains(stderr.String(), "insufficient credits") { + t.Fatalf("stderr missing insufficient-credits message:\n%s", stderr.String()) + } +} From 671a84b8225c721aec679323abb6dc3454497002 Mon Sep 17 00:00:00 2001 From: nullkey Date: Thu, 14 May 2026 21:07:05 +0800 Subject: [PATCH 2/3] fix(create): clean up orphan vectoria kb on pipeline failure (0.5.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this change, every failed `vk create` invocation left a freshly-created vectoria knowledgebase orphaned. Real-backend smoke during 0.5.0 alone accumulated 6+ such kbs from a handful of test runs; the user's tenant had accrued 424 kbs in total. Three failure boundaries now clean up the kb the CLI just created: 1. uploadFile / uploadURL — kb cleanup deferred if the subsequent doc upload or doc-status poll fails inside the helper. The kb leak window is closed before the function returns ("","",err). 2. RunE — kb cleanup deferred from the moment kbID is known until InitTask succeeds. Covers script_invalid (returns clerr.Validation → defer runs), generic InitTask errors (returns err → defer runs), and the `--mode script` no-doc reject path. 3. insufficient_credits handler — os.Exit(5) skips defers, so the same cleanup is invoked inline before os.Exit. The comment in create.go documents this asymmetry. Past InitTask success, the backend task owns the kb's lifecycle and the CLI must not delete it (even on task.failed, stream interrupted, --async detach, etc.) because the backend may still be reading from it. The `initTaskClaimed` flag fences this. Adds client/vectoria.Client.DeleteKB wrapping the backend's existing DELETE /v1/knowledgebases/{id} endpoint. Best-effort: errors are swallowed so cleanup hygiene doesn't mask the real failure. Extends the 0.5.1 integration test to drive the upload path and assert DELETE /v1/knowledgebases/{id} fires after the 100001 rejection. --- CHANGELOG.md | 22 ++++++ client/vectoria/knowledgebase.go | 11 +++ client/vectoria/knowledgebase_test.go | 20 ++++++ cmd/create.go | 52 ++++++++++++++ package.json | 2 +- skills/vibeknow-core/SKILL.md | 2 +- skills/vibeknow-create/SKILL.md | 2 +- skills/vibeknow-doc/SKILL.md | 2 +- tests/integration/create_credits_test.go | 90 +++++++++++++++++++++++- 9 files changed, 196 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 956c7d5..5f85b73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ # Changelog +## 0.5.2 — 2026-05-14 + +### Fixed + +- `vk create` now cleans up the temporary vectoria knowledgebase it + created when the pipeline fails before the backend task takes + ownership (`InitTask` rejection: `insufficient_credits`, + `script_invalid`, network errors, or any earlier upload/poll + failure). Previously every failed `vk create` invocation left an + orphan kb in vectoria forever — testing during 0.5.0 validation + alone accumulated 6+ such orphans, and the user's tenant had + accumulated 424 kbs total. The cleanup is best-effort: errors are + swallowed so they don't mask the real failure the user is about to + see. Once `InitTask` succeeds, the backend task owns the kb's + lifecycle and the CLI no longer interferes. + +### New + +- `vectoria.Client.DeleteKB(ctx, kbID)` — exposes the existing + vectoria `DELETE /v1/knowledgebases/{id}` endpoint, used by the + cleanup above. Available to external callers of the client. + ## 0.5.1 — 2026-05-14 ### Fixed diff --git a/client/vectoria/knowledgebase.go b/client/vectoria/knowledgebase.go index f68ab32..671596b 100644 --- a/client/vectoria/knowledgebase.go +++ b/client/vectoria/knowledgebase.go @@ -56,3 +56,14 @@ func (c *Client) DeleteDoc(ctx context.Context, kbID, docID string) error { } return nil } + +// DeleteKB removes a knowledgebase and all its documents. +// Used by `vk create` to clean up orphan kbs when upload-then-pipeline fails +// before the backend task takes ownership of the kb. +func (c *Client) DeleteKB(ctx context.Context, kbID string) error { + path := fmt.Sprintf("/v1/knowledgebases/%s", kbID) + if err := c.http.Do(ctx, "DELETE", path, nil, nil); err != nil { + return fmt.Errorf("delete knowledgebase: %w", err) + } + return nil +} diff --git a/client/vectoria/knowledgebase_test.go b/client/vectoria/knowledgebase_test.go index 94b4860..f6bf7d0 100644 --- a/client/vectoria/knowledgebase_test.go +++ b/client/vectoria/knowledgebase_test.go @@ -128,6 +128,26 @@ func TestDeleteDoc(t *testing.T) { } } +func TestDeleteKB(t *testing.T) { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + if r.Method != "DELETE" { + t.Fatalf("unexpected method %s", r.Method) + } + w.WriteHeader(204) + })) + defer srv.Close() + + c := vectoria.New(srv.URL, staticToken("test-jwt")) + if err := c.DeleteKB(context.Background(), "kb_1"); err != nil { + t.Fatalf("DeleteKB: %v", err) + } + if gotPath != "/v1/knowledgebases/kb_1" { + t.Fatalf("path = %q, want /v1/knowledgebases/kb_1", gotPath) + } +} + func TestUploadDoc_FileContent(t *testing.T) { var gotContent string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/create.go b/cmd/create.go index 4dc016b..153dd5f 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -101,6 +101,20 @@ var createCmd = &cobra.Command{ return clerr.Validation(i18n.T("create.err.script_needs_doc")) } + // Deferred kb cleanup: if we created a kb here (kbID != "") and the + // backend never claims it (InitTask never returns OK), drop it. + // Cleared once InitTask succeeds — past that point the backend task + // owns the kb's lifecycle and we must not delete it. + // Inline cleanup before os.Exit(5) below is also required because + // os.Exit skips defers. + var initTaskClaimed bool + defer func() { + if kbID == "" || initTaskClaimed { + return + } + cleanupOrphanKB(kbID) + }() + // Step 2: optimize prompt (skip if user provided --prompt). _, url, tp, err := cmdutil.Default().Service("figlens") if err != nil { @@ -150,6 +164,10 @@ var createCmd = &cobra.Command{ if err != nil { if errs.HasCode(err, "insufficient_credits") { // Mirror the stream-side path's exit code: business failure → 5. + // os.Exit skips defers — clean up the orphan kb inline first. + if kbID != "" { + cleanupOrphanKB(kbID) + } fmt.Fprintln(os.Stderr, i18n.T("credits.insufficient")) os.Exit(5) } @@ -164,6 +182,11 @@ var createCmd = &cobra.Command{ } return err } + // InitTask returned OK — backend task now owns the kb. Past this + // point we must not delete the kb on any error path (task.failed, + // stream interrupted, --async detach, etc.), because the backend + // may still be reading from it. + initTaskClaimed = true // Step 4: async or sync. if flagCreateAsync { @@ -390,6 +413,14 @@ func uploadFile(ctx context.Context, filePath string) (string, string, error) { if err != nil { return "", "", err } + // Best-effort cleanup if any subsequent step in this function fails. + // Cleared just before the successful return. + cleanup := func() { _ = vc.DeleteKB(context.Background(), kbID) } + defer func() { + if cleanup != nil { + cleanup() + } + }() f, err := os.Open(filePath) if err != nil { @@ -407,6 +438,7 @@ func uploadFile(ctx context.Context, filePath string) (string, string, error) { if err != nil { return "", "", err } + cleanup = nil // ownership transfers to caller from here return kbID, docID, nil } @@ -423,6 +455,12 @@ func uploadURL(ctx context.Context, url string) (string, string, error) { if err != nil { return "", "", err } + cleanup := func() { _ = vc.DeleteKB(context.Background(), kbID) } + defer func() { + if cleanup != nil { + cleanup() + } + }() fmt.Fprintln(os.Stderr, i18n.T("create.uploading_url", url)) doc, err := vc.UploadURL(ctx, kbID, url) @@ -434,9 +472,23 @@ func uploadURL(ctx context.Context, url string) (string, string, error) { if err != nil { return "", "", err } + cleanup = nil return kbID, docID, nil } +// cleanupOrphanKB best-effort deletes a knowledgebase the CLI created on +// behalf of `vk create` when the backend never claimed it (e.g., InitTask +// failed before the task could take ownership). Errors are swallowed — +// orphan cleanup is hygiene, not correctness, and we don't want it to +// mask the real failure the user is about to see. +func cleanupOrphanKB(kbID string) { + vc, err := cliauth.NewVectoriaClient() + if err != nil { + return + } + _ = vc.DeleteKB(context.Background(), kbID) +} + // pollDocReady polls until the document is completed or fails. func pollDocReady(ctx context.Context, vc *vectoria.Client, kbID, docID string) (string, error) { fmt.Fprintln(os.Stderr, i18n.T("create.doc_polling", docID)) diff --git a/package.json b/package.json index 3791223..987fc49 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "vibeknow-cli", - "version": "0.5.1", + "version": "0.5.2", "description": "VibeKnow CLI — turn docs / URLs into videos", "license": "MIT", "bin": { diff --git a/skills/vibeknow-core/SKILL.md b/skills/vibeknow-core/SKILL.md index 7f70ca7..985a870 100644 --- a/skills/vibeknow-core/SKILL.md +++ b/skills/vibeknow-core/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-core -version: 0.5.1 +version: 0.5.2 description: "vibeknow CLI setup, authentication, profile management, and diagnostics. Use when: first-time setup, auth errors, switching environments, diagnosing connection issues." metadata: requires: diff --git a/skills/vibeknow-create/SKILL.md b/skills/vibeknow-create/SKILL.md index e7a943c..b2d990c 100644 --- a/skills/vibeknow-create/SKILL.md +++ b/skills/vibeknow-create/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-create -version: 0.5.1 +version: 0.5.2 description: "Generate videos from documents/URLs/files, track video task progress, download results, list voice templates. Use when: user wants to create a video, check task status, download video, or browse voices." metadata: requires: diff --git a/skills/vibeknow-doc/SKILL.md b/skills/vibeknow-doc/SKILL.md index e3217ce..c65d915 100644 --- a/skills/vibeknow-doc/SKILL.md +++ b/skills/vibeknow-doc/SKILL.md @@ -1,6 +1,6 @@ --- name: vibeknow-doc -version: 0.5.1 +version: 0.5.2 description: "Upload documents to vectoria and check processing status. Use when: user wants to upload a document, check if a document is ready, or get a doc_id for use with vibeknow create." metadata: requires: diff --git a/tests/integration/create_credits_test.go b/tests/integration/create_credits_test.go index 80e4068..36c1d72 100644 --- a/tests/integration/create_credits_test.go +++ b/tests/integration/create_credits_test.go @@ -2,27 +2,93 @@ package integration import ( "encoding/json" + "fmt" "net/http" "net/http/httptest" "os" "os/exec" + "path/filepath" "strings" + "sync" "testing" ) +// buildProfileFigVect writes a temp profile pointing both figlens and vectoria +// at mockURL. buildVideoProfile (video_flow_test.go) only wires figlens, which +// is insufficient when a test exercises the upload path. +func buildProfileFigVect(t *testing.T, mockURL string) string { + t.Helper() + configDir := filepath.Join(t.TempDir(), "vibeknow") + if err := os.MkdirAll(configDir, 0755); err != nil { + t.Fatalf("mkdir config: %v", err) + } + profileYAML := fmt.Sprintf(`schema_version: "2" +current: test +profiles: + - name: test + endpoints: + figlens: %s + vectoria: %s + credential_ref: test + trust: dev + is_production: false +`, mockURL, mockURL) + if err := os.WriteFile(filepath.Join(configDir, "profiles.yaml"), []byte(profileYAML), 0644); err != nil { + t.Fatalf("write profiles.yaml: %v", err) + } + return configDir +} + // TestCreate_InsufficientCreditsOnInit_Exits5 covers the bug fixed in 0.5.1: // when the backend rejects InitTask with envelope code 100001 (insufficient // credits), the CLI must exit 5 (business failure) to match the stream-side // path's behavior, not exit 1 from cobra's default error handler. +// +// It also covers the orphan-kb cleanup fixed in 0.5.2: when InitTask fails +// after the CLI uploaded a doc and created a kb, the CLI must DELETE the +// just-created kb before exiting so it doesn't leak. func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { if testing.Short() { t.Skip("integration test") } + var mu sync.Mutex + var kbCreated, kbDeleted bool + var deletedID, createdID string + mux := http.NewServeMux() + mux.HandleFunc("/v1/knowledgebases", func(w http.ResponseWriter, r *http.Request) { + // vectoria CreateKB + mu.Lock() + kbCreated = true + createdID = "kb_insufcredits_test" + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"id": createdID}) + }) + mux.HandleFunc("/v1/knowledgebases/", func(w http.ResponseWriter, r *http.Request) { + // catches both /v1/knowledgebases//documents/file (upload) AND /v1/knowledgebases/ (delete) + switch r.Method { + case "POST": + // doc upload + _ = r.ParseMultipartForm(32 << 20) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"id": "doc_x", "status": "completed"}) + case "GET": + // doc status poll + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"id": "doc_x", "status": "completed"}) + case "DELETE": + mu.Lock() + kbDeleted = true + deletedID = strings.TrimPrefix(r.URL.Path, "/v1/knowledgebases/") + mu.Unlock() + w.WriteHeader(204) + } + }) mux.HandleFunc("/v1/tasks/init", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusPaymentRequired) // 402, matches backend + w.WriteHeader(http.StatusPaymentRequired) _ = json.NewEncoder(w).Encode(map[string]any{ "code": 100001, "message": "insufficient credits", @@ -31,10 +97,16 @@ func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { srv := httptest.NewServer(mux) defer srv.Close() + // Write a tiny local file so the CLI takes the upload path (which creates a kb). + tmpFile := t.TempDir() + "/test.txt" + if err := os.WriteFile(tmpFile, []byte("hello world"), 0644); err != nil { + t.Fatalf("write tmp file: %v", err) + } + bin := build(t) - configHome := buildVideoProfile(t, srv.URL) + configHome := buildProfileFigVect(t, srv.URL) - cmd := exec.Command(bin, "create", "--from", "doc_abc12345") + cmd := exec.Command(bin, "create", "--from", tmpFile) var stdout, stderr strings.Builder cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -57,4 +129,16 @@ func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { if !strings.Contains(stderr.String(), "insufficient credits") { t.Fatalf("stderr missing insufficient-credits message:\n%s", stderr.String()) } + + mu.Lock() + defer mu.Unlock() + if !kbCreated { + t.Fatalf("expected kb to be created by CLI but no POST /v1/knowledgebases received") + } + if !kbDeleted { + t.Fatalf("expected orphan kb cleanup: DELETE /v1/knowledgebases/ was never called.\nstderr:%s", stderr.String()) + } + if deletedID != createdID { + t.Fatalf("deleted kb = %q, want %q", deletedID, createdID) + } } From 463192ba73b716c561140ce23db6769ad57cd008 Mon Sep 17 00:00:00 2001 From: nullkey Date: Thu, 14 May 2026 21:35:11 +0800 Subject: [PATCH 3/3] =?UTF-8?q?refactor:=20review=20pass=20=E2=80=94=20tim?= =?UTF-8?q?eouts,=20dedup=20test=20helpers,=20simpler=20defer=20fence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply findings from /simplify review on the orphan-kb cleanup: - cmd/create.go: add 5s context timeout to every DeleteKB cleanup callsite. Previously context.Background() had no deadline; a hung backend during cleanup would silently stall the user, especially on the synchronous inline path before os.Exit(5). - cmd/create.go: replace the standalone `initTaskClaimed bool` flag with `task != nil` (declaring `task` earlier so the defer can reference it). Same semantics, one less state variable to reason about. Trim the over-narrated comments around the defer. - tests/integration/video_flow_test.go: introduce `buildProfile(t, endpoints map[string]string)` as the core helper; `buildVideoProfile` becomes a one-line wrapper. Removes 90%-duplicate YAML/file-writing logic that buildProfileFigVect (now deleted) had reinvented. - tests/integration/create_credits_test.go: use buildProfile directly, drop buildProfileFigVect. Tighten DELETE-path assertion to full path equality (loud failure if upload/poll ever mis-routes), add inline comments on the exact-vs-prefix mux pattern. No behavior change; tests still green. --- cmd/create.go | 50 +++++++++++++----------- tests/integration/create_credits_test.go | 47 ++++++---------------- tests/integration/video_flow_test.go | 34 +++++++++++----- 3 files changed, 66 insertions(+), 65 deletions(-) diff --git a/cmd/create.go b/cmd/create.go index 153dd5f..0ea6723 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -101,15 +101,12 @@ var createCmd = &cobra.Command{ return clerr.Validation(i18n.T("create.err.script_needs_doc")) } - // Deferred kb cleanup: if we created a kb here (kbID != "") and the - // backend never claims it (InitTask never returns OK), drop it. - // Cleared once InitTask succeeds — past that point the backend task - // owns the kb's lifecycle and we must not delete it. - // Inline cleanup before os.Exit(5) below is also required because - // os.Exit skips defers. - var initTaskClaimed bool + // Orphan kb cleanup: drop the kb if InitTask never returns OK + // (task stays nil). os.Exit(5) below skips defers, so an inline + // call is also needed there. + var task *figlens.Task defer func() { - if kbID == "" || initTaskClaimed { + if kbID == "" || task != nil { return } cleanupOrphanKB(kbID) @@ -160,7 +157,7 @@ var createCmd = &cobra.Command{ initParams.KnowledgeID = kbID initParams.DocID = docID } - task, err := fc.InitTask(ctx, initParams) + task, err = fc.InitTask(ctx, initParams) if err != nil { if errs.HasCode(err, "insufficient_credits") { // Mirror the stream-side path's exit code: business failure → 5. @@ -182,11 +179,9 @@ var createCmd = &cobra.Command{ } return err } - // InitTask returned OK — backend task now owns the kb. Past this - // point we must not delete the kb on any error path (task.failed, - // stream interrupted, --async detach, etc.), because the backend - // may still be reading from it. - initTaskClaimed = true + // Past this point, `task != nil` and the backend task owns the kb; + // the deferred cleanup above will skip on any later error path + // (task.failed, stream interrupted, --async detach). // Step 4: async or sync. if flagCreateAsync { @@ -415,7 +410,13 @@ func uploadFile(ctx context.Context, filePath string) (string, string, error) { } // Best-effort cleanup if any subsequent step in this function fails. // Cleared just before the successful return. - cleanup := func() { _ = vc.DeleteKB(context.Background(), kbID) } + cleanup := func() { + // Fresh context with timeout: parent ctx may already be cancelled, + // and a hung backend must not hold the user hostage. + c, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _ = vc.DeleteKB(c, kbID) + } defer func() { if cleanup != nil { cleanup() @@ -455,7 +456,13 @@ func uploadURL(ctx context.Context, url string) (string, string, error) { if err != nil { return "", "", err } - cleanup := func() { _ = vc.DeleteKB(context.Background(), kbID) } + cleanup := func() { + // Fresh context with timeout: parent ctx may already be cancelled, + // and a hung backend must not hold the user hostage. + c, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _ = vc.DeleteKB(c, kbID) + } defer func() { if cleanup != nil { cleanup() @@ -476,17 +483,16 @@ func uploadURL(ctx context.Context, url string) (string, string, error) { return kbID, docID, nil } -// cleanupOrphanKB best-effort deletes a knowledgebase the CLI created on -// behalf of `vk create` when the backend never claimed it (e.g., InitTask -// failed before the task could take ownership). Errors are swallowed — -// orphan cleanup is hygiene, not correctness, and we don't want it to -// mask the real failure the user is about to see. +// cleanupOrphanKB best-effort deletes a kb the CLI created when the +// backend never claimed it. Errors are swallowed: hygiene, not correctness. func cleanupOrphanKB(kbID string) { vc, err := cliauth.NewVectoriaClient() if err != nil { return } - _ = vc.DeleteKB(context.Background(), kbID) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _ = vc.DeleteKB(ctx, kbID) } // pollDocReady polls until the document is completed or fails. diff --git a/tests/integration/create_credits_test.go b/tests/integration/create_credits_test.go index 36c1d72..c8af797 100644 --- a/tests/integration/create_credits_test.go +++ b/tests/integration/create_credits_test.go @@ -2,43 +2,15 @@ package integration import ( "encoding/json" - "fmt" "net/http" "net/http/httptest" "os" "os/exec" - "path/filepath" "strings" "sync" "testing" ) -// buildProfileFigVect writes a temp profile pointing both figlens and vectoria -// at mockURL. buildVideoProfile (video_flow_test.go) only wires figlens, which -// is insufficient when a test exercises the upload path. -func buildProfileFigVect(t *testing.T, mockURL string) string { - t.Helper() - configDir := filepath.Join(t.TempDir(), "vibeknow") - if err := os.MkdirAll(configDir, 0755); err != nil { - t.Fatalf("mkdir config: %v", err) - } - profileYAML := fmt.Sprintf(`schema_version: "2" -current: test -profiles: - - name: test - endpoints: - figlens: %s - vectoria: %s - credential_ref: test - trust: dev - is_production: false -`, mockURL, mockURL) - if err := os.WriteFile(filepath.Join(configDir, "profiles.yaml"), []byte(profileYAML), 0644); err != nil { - t.Fatalf("write profiles.yaml: %v", err) - } - return configDir -} - // TestCreate_InsufficientCreditsOnInit_Exits5 covers the bug fixed in 0.5.1: // when the backend rejects InitTask with envelope code 100001 (insufficient // credits), the CLI must exit 5 (business failure) to match the stream-side @@ -56,29 +28,33 @@ func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { var kbCreated, kbDeleted bool var deletedID, createdID string + const fixedKBID = "kb_insufcredits_test" mux := http.NewServeMux() + // Exact match: vectoria CreateKB (POST /v1/knowledgebases). mux.HandleFunc("/v1/knowledgebases", func(w http.ResponseWriter, r *http.Request) { - // vectoria CreateKB mu.Lock() kbCreated = true - createdID = "kb_insufcredits_test" + createdID = fixedKBID mu.Unlock() w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(map[string]string{"id": createdID}) }) + // Prefix match: doc upload (POST), doc status (GET), kb delete (DELETE). mux.HandleFunc("/v1/knowledgebases/", func(w http.ResponseWriter, r *http.Request) { - // catches both /v1/knowledgebases//documents/file (upload) AND /v1/knowledgebases/ (delete) switch r.Method { case "POST": - // doc upload _ = r.ParseMultipartForm(32 << 20) w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(map[string]string{"id": "doc_x", "status": "completed"}) case "GET": - // doc status poll w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(map[string]string{"id": "doc_x", "status": "completed"}) case "DELETE": + // Assert the full path to fail loudly if upload or poll ever + // accidentally routes here. + if r.URL.Path != "/v1/knowledgebases/"+fixedKBID { + t.Errorf("unexpected DELETE path %q", r.URL.Path) + } mu.Lock() kbDeleted = true deletedID = strings.TrimPrefix(r.URL.Path, "/v1/knowledgebases/") @@ -104,7 +80,10 @@ func TestCreate_InsufficientCreditsOnInit_Exits5(t *testing.T) { } bin := build(t) - configHome := buildProfileFigVect(t, srv.URL) + configHome := buildProfile(t, map[string]string{ + "figlens": srv.URL, + "vectoria": srv.URL, + }) cmd := exec.Command(bin, "create", "--from", tmpFile) var stdout, stderr strings.Builder diff --git a/tests/integration/video_flow_test.go b/tests/integration/video_flow_test.go index bb5072f..638ff66 100644 --- a/tests/integration/video_flow_test.go +++ b/tests/integration/video_flow_test.go @@ -9,38 +9,54 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strings" "sync/atomic" "testing" ) -// buildVideoProfile writes a minimal profiles.yaml that points only at figlens -// (no vectoria needed for pure video commands) and returns the config-home -// path to pass to the binary via VIBEKNOW_CONFIG_HOME. Using that var (rather -// than XDG_CONFIG_HOME) keeps the test cross-platform — on Windows the CLI -// resolves its config dir from %AppData% and ignores XDG_CONFIG_HOME entirely. -func buildVideoProfile(t *testing.T, figlensURL string) string { +// buildProfile writes a minimal profiles.yaml wiring the given endpoint +// name→URL pairs and returns the config-home path to pass to the binary +// via VIBEKNOW_CONFIG_HOME. Using that var (rather than XDG_CONFIG_HOME) +// keeps tests cross-platform — on Windows the CLI resolves its config +// dir from %AppData% and ignores XDG_CONFIG_HOME entirely. +func buildProfile(t *testing.T, endpoints map[string]string) string { t.Helper() configDir := filepath.Join(t.TempDir(), "vibeknow") if err := os.MkdirAll(configDir, 0755); err != nil { t.Fatalf("mkdir config: %v", err) } + // Sort keys for stable output (helps debugging). + names := make([]string, 0, len(endpoints)) + for k := range endpoints { + names = append(names, k) + } + sort.Strings(names) + var endpointsBlock strings.Builder + for _, name := range names { + fmt.Fprintf(&endpointsBlock, " %s: %s\n", name, endpoints[name]) + } profileYAML := fmt.Sprintf(`schema_version: "2" current: test profiles: - name: test endpoints: - figlens: %s - credential_ref: test +%s credential_ref: test trust: dev is_production: false -`, figlensURL) +`, endpointsBlock.String()) if err := os.WriteFile(filepath.Join(configDir, "profiles.yaml"), []byte(profileYAML), 0644); err != nil { t.Fatalf("write profiles.yaml: %v", err) } return configDir } +// buildVideoProfile is a convenience wrapper around buildProfile for tests +// that only need a figlens endpoint. Existing callers stay unchanged. +func buildVideoProfile(t *testing.T, figlensURL string) string { + return buildProfile(t, map[string]string{"figlens": figlensURL}) +} + // runVideoCmd runs the binary with the given args, capturing stdout and stderr // separately. Returns stdout, combined output, and exit code. func runVideoCmd(t *testing.T, bin, configHome string, args ...string) (string, string, int) {