diff --git a/cmd/claw/cli/auto.go b/cmd/claw/cli/auto.go index ebf7f7fd..e92ff7d5 100644 --- a/cmd/claw/cli/auto.go +++ b/cmd/claw/cli/auto.go @@ -456,7 +456,14 @@ func inspectBBHWorkspace(ctx context.Context, workspaceDir string) (string, erro if err != nil { return "", err } - retrievalWS := sdkworkspace.Sub(sdkworkspace.Sub(ws, memoryRoot), "retrieval") + memoryWS, err := ws.Sub(memoryRoot) + if err != nil { + return "", fmt.Errorf("open memory workspace: %w", err) + } + retrievalWS, err := memoryWS.Sub("retrieval") + if err != nil { + return "", fmt.Errorf("open retrieval workspace: %w", err) + } inspector, err := bbh.NewInspector(retrievalWS) if err != nil { return "", err diff --git a/memory/retrieval/bbh/index.go b/memory/retrieval/bbh/index.go index 6fd45d0c..2e270b58 100644 --- a/memory/retrieval/bbh/index.go +++ b/memory/retrieval/bbh/index.go @@ -38,10 +38,6 @@ const ( hnswGraphExt = ".graph" ) -type localRoot interface { - Root() string -} - // Index implements retrieval.Index using a shared Badger doc store plus // per-namespace Bleve and HNSW indexes. type Index struct { @@ -71,22 +67,14 @@ type textDoc struct { Content string `json:"content"` } -// New constructs an Index rooted directly in a local workspace. -// -// BBH accepts the Workspace interface so callers can share the same -// constructor shape as other retrieval backends, but the current implementation -// requires a workspace that exposes a local Root() path because Bleve and -// Badger are path-backed embedded stores. Callers that need a common prefix -// should provide an already-prefixed workspace instead of configuring BBH. -func New(ws sdkworkspace.Workspace, opts ...Option) (*Index, error) { +// New constructs an Index rooted directly in a local workspace. Badger, Bleve, +// and HNSW are path-backed stores, so callers must provide an already-prefixed +// LocalWorkspace rather than a generic workspace implementation. +func New(ws *sdkworkspace.LocalWorkspace, opts ...Option) (*Index, error) { if ws == nil { return nil, errdefs.Validationf("retrieval/bbh: workspace is nil") } - lr, ok := ws.(localRoot) - if !ok { - return nil, errdefs.Validationf("retrieval/bbh: workspace must expose local Root()") - } - root := lr.Root() + root := ws.Root() cfg, err := resolveConfig(root, opts) if err != nil { return nil, err diff --git a/memory/retrieval/bbh/index_test.go b/memory/retrieval/bbh/index_test.go index 445499ae..5ea18c3d 100644 --- a/memory/retrieval/bbh/index_test.go +++ b/memory/retrieval/bbh/index_test.go @@ -26,9 +26,6 @@ func TestNewValidationAndClosedIndexErrors(t *testing.T) { if _, err := New(nil); err == nil { t.Fatal("nil workspace should fail") } - if _, err := New(sdkworkspace.NewMemWorkspace()); err == nil { - t.Fatal("workspace without Root should fail") - } if _, err := New(rootedWorkspace(t, filepath.Join(t.TempDir(), "missing-config")), WithConfigFilePath("missing.yaml")); err == nil { t.Fatal("bad config in New should fail") } @@ -806,22 +803,15 @@ func openInternalIndex(t *testing.T, dir string, opts ...Option) *Index { return idx } -func rootedWorkspace(t *testing.T, root string) sdkworkspace.Workspace { +func rootedWorkspace(t *testing.T, root string) *sdkworkspace.LocalWorkspace { t.Helper() - ws, err := sdkworkspace.NewLocalWorkspace(t.TempDir()) + ws, err := sdkworkspace.NewLocalWorkspace(root) if err != nil { t.Fatal(err) } - return localRootWorkspace{Workspace: ws, root: root} -} - -type localRootWorkspace struct { - sdkworkspace.Workspace - root string + return ws } -func (w localRootWorkspace) Root() string { return w.root } - func iterIDs(t *testing.T, idx *Index, ns, cursor string, batch int) []string { t.Helper() docs, next, err := idx.Iterate(context.Background(), ns, cursor, batch) diff --git a/memory/retrieval/bbh/inspector.go b/memory/retrieval/bbh/inspector.go index e751c95d..f3cbcb59 100644 --- a/memory/retrieval/bbh/inspector.go +++ b/memory/retrieval/bbh/inspector.go @@ -70,15 +70,11 @@ type NamespaceInspection struct { // NewInspector opens an offline, read-only inspector rooted at a BBH workspace. // It opens Badger with a read-only lock, so it cannot inspect a workspace while // a writable Index is already open on the same path. -func NewInspector(ws sdkworkspace.Workspace) (*Inspector, error) { +func NewInspector(ws *sdkworkspace.LocalWorkspace) (*Inspector, error) { if ws == nil { return nil, errdefs.Validationf("retrieval/bbh: workspace is nil") } - lr, ok := ws.(localRoot) - if !ok { - return nil, errdefs.Validationf("retrieval/bbh: workspace must expose local Root()") - } - root := lr.Root() + root := ws.Root() in := &Inspector{root: root} dbPath := filepath.Join(root, badgerDir) info, err := os.Stat(dbPath) diff --git a/memory/retrieval/bbh/inspector_test.go b/memory/retrieval/bbh/inspector_test.go index a39adfd0..bccdb530 100644 --- a/memory/retrieval/bbh/inspector_test.go +++ b/memory/retrieval/bbh/inspector_test.go @@ -8,7 +8,6 @@ import ( "time" "github.com/GizClaw/flowcraft/memory/retrieval" - sdkworkspace "github.com/GizClaw/flowcraft/sdk/workspace" "github.com/dgraph-io/badger/v4" ) @@ -16,9 +15,6 @@ func TestInspectorValidationAndEmptyWorkspace(t *testing.T) { if _, err := NewInspector(nil); err == nil { t.Fatal("nil workspace should fail") } - if _, err := NewInspector(sdkworkspace.NewMemWorkspace()); err == nil { - t.Fatal("workspace without Root should fail") - } dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, badgerDir), []byte("not a dir"), 0o644); err != nil { t.Fatal(err) @@ -28,7 +24,8 @@ func TestInspectorValidationAndEmptyWorkspace(t *testing.T) { } empty := t.TempDir() - in, err := NewInspector(rootedWorkspace(t, empty)) + emptyWS := rootedWorkspace(t, empty) + in, err := NewInspector(emptyWS) if err != nil { t.Fatalf("NewInspector empty: %v", err) } @@ -36,7 +33,7 @@ func TestInspectorValidationAndEmptyWorkspace(t *testing.T) { if err != nil { t.Fatalf("Inspect empty: %v", err) } - if got.Root != empty || got.BadgerExists || got.BleveExists || got.HNSWExists { + if got.Root != emptyWS.Root() || got.BadgerExists || got.BleveExists || got.HNSWExists { t.Fatalf("unexpected empty inspection: %+v", got) } if len(got.Namespaces) != 0 || got.TotalDocs != 0 { diff --git a/sdk/retrieval/bbh/index.go b/sdk/retrieval/bbh/index.go index 19daaf77..510dd41b 100644 --- a/sdk/retrieval/bbh/index.go +++ b/sdk/retrieval/bbh/index.go @@ -38,10 +38,6 @@ const ( hnswGraphExt = ".graph" ) -type localRoot interface { - Root() string -} - // Index implements retrieval.Index using a shared Badger doc store plus // per-namespace Bleve and HNSW indexes. type Index struct { @@ -73,22 +69,14 @@ type textDoc struct { Content string `json:"content"` } -// New constructs an Index rooted directly in a local workspace. -// -// BBH accepts the Workspace interface so callers can share the same -// constructor shape as other retrieval backends, but the current implementation -// requires a workspace that exposes a local Root() path because Bleve and -// Badger are path-backed embedded stores. Callers that need a common prefix -// should provide an already-prefixed workspace instead of configuring BBH. -func New(ws sdkworkspace.Workspace, opts ...Option) (*Index, error) { +// New constructs an Index rooted directly in a local workspace. Badger, Bleve, +// and HNSW are path-backed stores, so callers must provide an already-prefixed +// LocalWorkspace rather than a generic workspace implementation. +func New(ws *sdkworkspace.LocalWorkspace, opts ...Option) (*Index, error) { if ws == nil { return nil, errdefs.Validationf("retrieval/bbh: workspace is nil") } - lr, ok := ws.(localRoot) - if !ok { - return nil, errdefs.Validationf("retrieval/bbh: workspace must expose local Root()") - } - root := lr.Root() + root := ws.Root() cfg, err := resolveConfig(root, opts) if err != nil { return nil, err diff --git a/sdk/retrieval/bbh/index_test.go b/sdk/retrieval/bbh/index_test.go index e6750425..dad92471 100644 --- a/sdk/retrieval/bbh/index_test.go +++ b/sdk/retrieval/bbh/index_test.go @@ -26,9 +26,6 @@ func TestNewValidationAndClosedIndexErrors(t *testing.T) { if _, err := New(nil); err == nil { t.Fatal("nil workspace should fail") } - if _, err := New(sdkworkspace.NewMemWorkspace()); err == nil { - t.Fatal("workspace without Root should fail") - } if _, err := New(rootedWorkspace(t, filepath.Join(t.TempDir(), "missing-config")), WithConfigFilePath("missing.yaml")); err == nil { t.Fatal("bad config in New should fail") } @@ -807,22 +804,15 @@ func openInternalIndex(t *testing.T, dir string, opts ...Option) *Index { return idx } -func rootedWorkspace(t *testing.T, root string) sdkworkspace.Workspace { +func rootedWorkspace(t *testing.T, root string) *sdkworkspace.LocalWorkspace { t.Helper() - ws, err := sdkworkspace.NewLocalWorkspace(t.TempDir()) + ws, err := sdkworkspace.NewLocalWorkspace(root) if err != nil { t.Fatal(err) } - return localRootWorkspace{Workspace: ws, root: root} -} - -type localRootWorkspace struct { - sdkworkspace.Workspace - root string + return ws } -func (w localRootWorkspace) Root() string { return w.root } - func iterIDs(t *testing.T, idx *Index, ns, cursor string, batch int) []string { t.Helper() docs, next, err := idx.Iterate(context.Background(), ns, cursor, batch) diff --git a/sdk/workspace/local.go b/sdk/workspace/local.go index da9e4d47..f5228741 100644 --- a/sdk/workspace/local.go +++ b/sdk/workspace/local.go @@ -38,6 +38,36 @@ func NewLocalWorkspace(root string) (*LocalWorkspace, error) { // Root returns the absolute path of the workspace root. func (w *LocalWorkspace) Root() string { return w.root } +// Sub returns a local workspace rooted under prefix. +// +// The resolved child root must remain inside the current workspace, including +// after symlink resolution performed by NewLocalWorkspace. +func (w *LocalWorkspace) Sub(prefix string) (*LocalWorkspace, error) { + if w == nil { + return nil, errdefs.Validationf("workspace: local workspace is nil") + } + cleaned, err := cleanPath(prefix) + if err != nil { + return nil, fmt.Errorf("workspace local sub: invalid prefix %q: %w", prefix, err) + } + if cleaned == "" { + return w, nil + } + full, err := w.resolve(cleaned) + if err != nil { + return nil, fmt.Errorf("workspace local sub: resolve root %q: %w", cleaned, err) + } + local, err := NewLocalWorkspace(full) + if err != nil { + return nil, fmt.Errorf("workspace local sub: open root %q: %w", cleaned, err) + } + root := local.Root() + if root != w.Root() && !strings.HasPrefix(root, w.Root()+string(filepath.Separator)) { + return nil, fmt.Errorf("%w: %s (symlink escape)", ErrPathTraversal, cleaned) + } + return local, nil +} + // Capabilities reports LocalWorkspace's storage characteristics: // backed by the host filesystem, so Rename is atomic on the same // device, writes are read-after-write consistent, and durability diff --git a/sdk/workspace/local_test.go b/sdk/workspace/local_test.go index 01d22d95..39bf9e03 100644 --- a/sdk/workspace/local_test.go +++ b/sdk/workspace/local_test.go @@ -2,12 +2,59 @@ package workspace import ( "context" + "errors" "os" "path/filepath" "runtime" "testing" ) +func TestLocalWorkspace_Sub(t *testing.T) { + base, _ := newLocalWS(t) + child, err := base.Sub(filepath.Join("runtime-a", "memory")) + if err != nil { + t.Fatalf("Sub: %v", err) + } + want := filepath.Join(base.Root(), "runtime-a", "memory") + if child.Root() != want { + t.Fatalf("Root() = %q, want %q", child.Root(), want) + } + nested, err := child.Sub("retrieval") + if err != nil { + t.Fatalf("nested Sub: %v", err) + } + if want := filepath.Join(child.Root(), "retrieval"); nested.Root() != want { + t.Fatalf("nested Root() = %q, want %q", nested.Root(), want) + } + same, err := base.Sub(".") + if err != nil { + t.Fatalf("empty Sub: %v", err) + } + if same != base { + t.Fatal("empty Sub should return the receiver") + } +} + +func TestLocalWorkspace_SubRejectsTraversalAndSymlinkEscape(t *testing.T) { + base, _ := newLocalWS(t) + if _, err := base.Sub("../escape"); !errors.Is(err, ErrPathTraversal) { + t.Fatalf("traversal Sub error = %v, want ErrPathTraversal", err) + } + if runtime.GOOS == "windows" { + return + } + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(base.Root(), "escape")); err != nil { + t.Fatalf("Symlink: %v", err) + } + if _, err := base.Sub(filepath.Join("escape", "created")); !errors.Is(err, ErrPathTraversal) { + t.Fatalf("symlink Sub error = %v, want ErrPathTraversal", err) + } + if _, err := os.Stat(filepath.Join(outside, "created")); !os.IsNotExist(err) { + t.Fatalf("symlink Sub created an outside directory: %v", err) + } +} + func TestLocalWorkspace_ReadWrite(t *testing.T) { ws, ctx := newLocalWS(t) diff --git a/sdk/workspace/sub.go b/sdk/workspace/sub.go index af36e99a..24fc6b0d 100644 --- a/sdk/workspace/sub.go +++ b/sdk/workspace/sub.go @@ -5,7 +5,6 @@ import ( "fmt" "io/fs" "path/filepath" - "strings" "github.com/GizClaw/flowcraft/sdk/errdefs" ) @@ -45,15 +44,10 @@ func Sub(inner Workspace, prefix string) Workspace { sw := &subWorkspace{inner: inner, prefix: cleaned} switch typed := inner.(type) { case *LocalWorkspace: - local, err := NewLocalWorkspace(filepath.Join(typed.Root(), cleaned)) + local, err := typed.Sub(cleaned) if err != nil { return &subWorkspace{inner: inner, initErr: fmt.Errorf("workspace sub: open local root %q: %w", cleaned, err)} } - root := local.Root() - if root != typed.Root() && !strings.HasPrefix(root, typed.Root()+string(filepath.Separator)) { - err := fmt.Errorf("%w: %s (symlink escape)", ErrPathTraversal, cleaned) - return &subWorkspace{inner: inner, initErr: fmt.Errorf("workspace sub: open local root %q: %w", cleaned, err)} - } return local } return sw diff --git a/sdkx/claw/memory.go b/sdkx/claw/memory.go index cfd71d11..bc7b2853 100644 --- a/sdkx/claw/memory.go +++ b/sdkx/claw/memory.go @@ -154,7 +154,6 @@ func (c *Claw) buildMemory(ctx context.Context) (*memoryRuntime, error) { opts := []recall.Option{recall.WithGraphEnabled(memCfg.Recall.GraphEnabled)} memoryWS := sdkworkspace.Sub(c.ws, c.cfg.Workspace.MemoryRoot) metadataWS := sdkworkspace.Sub(memoryWS, "metadata") - retrievalWS := sdkworkspace.Sub(memoryWS, "retrieval") backend, err := recallworkspace.New(metadataWS) if err != nil { return nil, err @@ -175,6 +174,14 @@ func (c *Claw) buildMemory(ctx context.Context) (*memoryRuntime, error) { switch strings.TrimSpace(memCfg.Retrieval.Backend) { case "", "memory": case "bbh": + localMemoryWS, ok := memoryWS.(*sdkworkspace.LocalWorkspace) + if !ok { + return nil, fmt.Errorf("claw: retrieval backend bbh requires a local workspace") + } + retrievalWS, err := localMemoryWS.Sub("retrieval") + if err != nil { + return nil, fmt.Errorf("claw: create bbh retrieval workspace: %w", err) + } index, err := bbh.New(retrievalWS, bbh.WithConfig(memCfg.Retrieval.BBH)) if err != nil { return nil, err diff --git a/sdkx/claw/memory_test.go b/sdkx/claw/memory_test.go index 4fab74e7..89886d44 100644 --- a/sdkx/claw/memory_test.go +++ b/sdkx/claw/memory_test.go @@ -699,6 +699,22 @@ func newMemoryEnabledTestClaw(t *testing.T) *Claw { return newMemoryEnabledTestClawWith(t, nil) } +func TestMemoryBBHRequiresLocalWorkspace(t *testing.T) { + ws := workspace.NewMemWorkspace() + cfg := testConfigForLLM(t, staticLLM{reply: "ok"}) + cfg.Memory.Enabled = true + cfg.Memory.Retrieval.Backend = "bbh" + writeTestConfig(t, ws, cfg) + + _, err := New(ws) + if err == nil { + t.Fatal("New should reject bbh with a non-local workspace") + } + if !strings.Contains(err.Error(), "bbh requires a local workspace") { + t.Fatalf("New error = %v, want local workspace requirement", err) + } +} + func newMemoryEnabledTestClawWith(t *testing.T, mutate func(*Config)) *Claw { t.Helper() ws, err := workspace.NewLocalWorkspace(t.TempDir())