From 466ef577f7694bb49b42d203be313172703aab36 Mon Sep 17 00:00:00 2001 From: Justin Truong Date: Mon, 20 Jul 2026 14:18:35 +0700 Subject: [PATCH] fix(vault): resolve vault_read paths under tenant workspace root vault_read is a singleton wired once at boot with the global workspace root, then joins doc.Path (stored relative to the TENANT root) onto it. For master tenant both roots coincide, so the bug stayed invisible; every other tenant reads ENOENT because its files live under tenants//. vault_search is DB-only and keeps returning the doc, so the symptom is "search finds it, read fails" rather than an obvious missing file. Resolve the tenant-scoped root per request via TenantLayer, which is a no-op for master and preserves existing single-tenant behaviour. Passing the tenant root into resolvePath also narrows the boundary check, so it now doubles as cross-tenant isolation. Runs originating from channels or cron may not carry the tenant slug in context; without it TenantLayer falls back to a UUID-named directory that does not match the slug-named one the write path creates. Add a nil-safe TenantStore lookup for that case only. --- cmd/gateway_vault_wiring.go | 4 + internal/tools/vault_read.go | 41 +++++++-- internal/tools/vault_read_test.go | 135 +++++++++++++++++++++++++++++- 3 files changed, 170 insertions(+), 10 deletions(-) diff --git a/cmd/gateway_vault_wiring.go b/cmd/gateway_vault_wiring.go index 9957fd74..9c07bfa9 100644 --- a/cmd/gateway_vault_wiring.go +++ b/cmd/gateway_vault_wiring.go @@ -36,6 +36,10 @@ func wireVault(stores *store.Stores, toolsReg *tools.Registry, workspace string, if stores.Episodic != nil { vaultReadTool.SetEpisodicStore(stores.Episodic) } + // Tenant slug fallback for runs whose context carries no slug (channels, cron). + if stores.Tenants != nil { + vaultReadTool.SetTenantStore(stores.Tenants) + } toolsReg.Register(vaultReadTool) // Build VaultSearchService: fan-out across vault + episodic + KG. diff --git a/internal/tools/vault_read.go b/internal/tools/vault_read.go index e00c3c05..befbacd0 100644 --- a/internal/tools/vault_read.go +++ b/internal/tools/vault_read.go @@ -22,6 +22,7 @@ type VaultReadTool struct { vaultStore store.VaultStore kgStore store.KnowledgeGraphStore episodicStore store.EpisodicStore + tenantStore store.TenantStore workspace string } @@ -74,9 +75,15 @@ func (t *VaultReadTool) SetKGStore(kg store.KnowledgeGraphStore) { t.kgStore = k // lookup when vault lookup misses. nil is safe. func (t *VaultReadTool) SetEpisodicStore(es store.EpisodicStore) { t.episodicStore = es } -// SetWorkspace injects the tenant workspace root (wired at boot). +// SetWorkspace injects the global workspace root (wired at boot). This is the +// base for every tenant — the tenant-scoped root is resolved per request in +// tenantWorkspace, since one tool instance serves all tenants. func (t *VaultReadTool) SetWorkspace(ws string) { t.workspace = ws } +// SetTenantStore injects an optional TenantStore used to resolve the tenant +// slug when the request context does not carry one. nil is safe. +func (t *VaultReadTool) SetTenantStore(ts store.TenantStore) { t.tenantStore = ts } + func (t *VaultReadTool) Name() string { return "vault_read" } func (t *VaultReadTool) Description() string { @@ -145,8 +152,8 @@ func (t *VaultReadTool) Execute(ctx context.Context, args map[string]any) *Resul return ErrorResult(fmt.Sprintf("vault_read does not support %s files — use read_image/read_audio/read_video/read_document", ext)) } - // Resolve path under tenant workspace root. - fullPath, err := t.resolvePath(doc.Path) + // Resolve path under the tenant workspace root. + fullPath, err := t.resolvePath(t.tenantWorkspace(ctx, tenantID), doc.Path) if err != nil { return ErrorResult(err.Error()) } @@ -344,13 +351,33 @@ func (t *VaultReadTool) allowed(ctx context.Context, doc *store.VaultDocument) b } } -// resolvePath joins the tenant workspace with the doc's relative path and -// enforces that the fully-resolved path remains strictly under the workspace. +// tenantWorkspace resolves the tenant-scoped workspace root for this request. +// Vault paths are stored relative to that root (e.g. "agents//note.md"), +// so joining them onto the global root silently misses every non-master tenant +// whose files live under /tenants//. Master tenant is a no-op +// inside TenantLayer and keeps using the base root. +func (t *VaultReadTool) tenantWorkspace(ctx context.Context, tenantID uuid.UUID) string { + slug := store.TenantSlugFromContext(ctx) + // Channel- and cron-originated runs may not carry the slug in context. + // Without it TenantLayer falls back to a UUID-named directory, which does + // not match the slug-named directory the write path creates on disk. + if slug == "" && tenantID != store.MasterTenantID && t.tenantStore != nil { + if tenant, err := t.tenantStore.GetTenant(ctx, tenantID); err == nil && tenant != nil { + slug = tenant.Slug + } + } + return ResolveWorkspace(t.workspace, TenantLayer(tenantID, slug)) +} + +// resolvePath joins the given workspace root with the doc's relative path and +// enforces that the fully-resolved path remains strictly under that root. +// Passing the tenant root (not the global one) also makes the boundary check +// double as cross-tenant isolation. // Symlinks are resolved via EvalSymlinks for defence-in-depth; if the file is // missing EvalSymlinks will fail and we fall back to the cleaned join (the // subsequent os.Open will surface the not-found error naturally). -func (t *VaultReadTool) resolvePath(relPath string) (string, error) { - wsClean := filepath.Clean(t.workspace) +func (t *VaultReadTool) resolvePath(root, relPath string) (string, error) { + wsClean := filepath.Clean(root) joined := filepath.Join(wsClean, filepath.FromSlash(relPath)) // Resolve symlinks where possible; on error (e.g. file missing) keep the diff --git a/internal/tools/vault_read_test.go b/internal/tools/vault_read_test.go index f80f3757..adeb8182 100644 --- a/internal/tools/vault_read_test.go +++ b/internal/tools/vault_read_test.go @@ -11,6 +11,7 @@ import ( "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/store" ) @@ -61,17 +62,30 @@ func (f *fakeVaultStore) GetDocumentsByIDs(ctx context.Context, tenantID string, // newVaultReadTestTool builds a VaultReadTool with a temp workspace and a // fake store pre-seeded with docs. +// +// The tool is given the GLOBAL workspace root, mirroring boot wiring. The +// returned path is the TENANT-scoped root of the first seeded doc — the +// directory the tool resolves per request, and therefore where test content +// must be written. Tests set no slug on the context, so the tenant directory +// is UUID-named (see config.TenantWorkspace). func newVaultReadTestTool(t *testing.T, docs ...*store.VaultDocument) (*VaultReadTool, string) { t.Helper() - ws := t.TempDir() + base := t.TempDir() fake := &fakeVaultStore{byID: make(map[string]*store.VaultDocument)} for _, d := range docs { fake.byID[d.TenantID+":"+d.ID] = d } tool := NewVaultReadTool() tool.SetVaultStore(fake) - tool.SetWorkspace(ws) - return tool, ws + tool.SetWorkspace(base) + + contentRoot := base + if len(docs) > 0 { + if tid, err := uuid.Parse(docs[0].TenantID); err == nil { + contentRoot = config.TenantWorkspace(base, tid, "") + } + } + return tool, contentRoot } // writeFile creates a file under workspace with the given relative path. @@ -917,3 +931,118 @@ func TestVaultRead_TrulyMissingReturnsNotFound(t *testing.T) { t.Fatalf("expected document not found, got: %s", res.ForLLM) } } + +// --- 18. Regression: non-master tenant content lives under tenants//. +// The tool holds the GLOBAL root, so a global-root join misses the file and +// every non-master tenant reads ENOENT while vault_search (DB-only) still +// returns the doc. Mirrors the production failure on tenant "alpha". --- +func TestVaultRead_NonMasterTenant_ResolvesUnderTenantRoot(t *testing.T) { + tenantID := uuid.New() + agentID := uuid.New() + docID := uuid.New() + doc := &store.VaultDocument{ + ID: docID.String(), TenantID: tenantID.String(), + Scope: "shared", Path: "agents/ho-tro/Refund Flow.md", + Title: "Refund Flow", DocType: "note", + } + base := t.TempDir() + fake := &fakeVaultStore{byID: map[string]*store.VaultDocument{ + tenantID.String() + ":" + docID.String(): doc, + }} + tool := NewVaultReadTool() + tool.SetVaultStore(fake) + tool.SetWorkspace(base) + + // Written by the tenant-aware write path; slug comes from context. + tenantRoot := config.TenantWorkspace(base, tenantID, "alpha") + writeFile(t, tenantRoot, "agents/ho-tro/Refund Flow.md", "refund body") + + ctx := store.WithTenantSlug(makeCtx(tenantID, agentID), "alpha") + res := tool.Execute(ctx, map[string]any{"doc_id": docID.String()}) + if res.IsError { + t.Fatalf("tenant doc must resolve under tenants//, got: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "refund body") { + t.Fatalf("content missing: %s", res.ForLLM) + } +} + +// --- 19. Master tenant keeps using the base root (no tenants/ segment), so +// the fix above must not regress single-tenant and master installs. --- +func TestVaultRead_MasterTenant_UsesBaseRoot(t *testing.T) { + agentID := uuid.New() + docID := uuid.New() + doc := &store.VaultDocument{ + ID: docID.String(), TenantID: store.MasterTenantID.String(), + Scope: "shared", Path: "agents/ho-tro/note.md", + Title: "Note", DocType: "note", + } + base := t.TempDir() + fake := &fakeVaultStore{byID: map[string]*store.VaultDocument{ + store.MasterTenantID.String() + ":" + docID.String(): doc, + }} + tool := NewVaultReadTool() + tool.SetVaultStore(fake) + tool.SetWorkspace(base) + + // No tenants/ segment for master. + writeFile(t, base, "agents/ho-tro/note.md", "master body") + + res := tool.Execute(makeCtx(store.MasterTenantID, agentID), + map[string]any{"doc_id": docID.String()}) + if res.IsError { + t.Fatalf("master doc must resolve at base root, got: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "master body") { + t.Fatalf("content missing: %s", res.ForLLM) + } +} + +// --- 20. Slug absent from context (channel/cron runs) → fall back to the +// TenantStore lookup, since a UUID-named directory would miss the slug-named +// one the write path created. --- +func TestVaultRead_SlugMissingFromContext_FallsBackToTenantStore(t *testing.T) { + tenantID := uuid.New() + agentID := uuid.New() + docID := uuid.New() + doc := &store.VaultDocument{ + ID: docID.String(), TenantID: tenantID.String(), + Scope: "shared", Path: "agents/ho-tro/note.md", + Title: "Note", DocType: "note", + } + base := t.TempDir() + fake := &fakeVaultStore{byID: map[string]*store.VaultDocument{ + tenantID.String() + ":" + docID.String(): doc, + }} + tool := NewVaultReadTool() + tool.SetVaultStore(fake) + tool.SetWorkspace(base) + tool.SetTenantStore(&fakeTenantStoreRead{slug: "alpha", id: tenantID}) + + writeFile(t, config.TenantWorkspace(base, tenantID, "alpha"), + "agents/ho-tro/note.md", "cron body") + + // Context deliberately carries no slug. + res := tool.Execute(makeCtx(tenantID, agentID), + map[string]any{"doc_id": docID.String()}) + if res.IsError { + t.Fatalf("slug fallback must resolve the doc, got: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "cron body") { + t.Fatalf("content missing: %s", res.ForLLM) + } +} + +// fakeTenantStoreRead resolves a single tenant slug for the fallback path. +type fakeTenantStoreRead struct { + store.TenantStore + id uuid.UUID + slug string +} + +func (f *fakeTenantStoreRead) GetTenant(_ context.Context, id uuid.UUID) (*store.TenantData, error) { + if id != f.id { + return nil, os.ErrNotExist + } + return &store.TenantData{ID: f.id, Slug: f.slug}, nil +}