mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-18 10:14:49 +00:00
fix(agents): predefined agents visible in web UI for channel members (#34)
* fix: SeedUserFiles uses agent-level USER.md as seed for predefined agents For predefined agents, the wizard writes a populated USER.md (owner name, language, notes) via agents.files.set into agent_context_files (agent-level). On the first chat message, SeedUserFiles was seeding a blank embedded template into user_context_files (per-user level). LoadContextFiles for predefined agents lets per-user content override agent-level content, so the blank per-user USER.md shadowed the wizard-written content. The owner profile was stored in the DB but never served to the agent, triggering unnecessary onboarding. Fix: before falling back to the embedded template, check whether agent_context_files already has a non-empty USER.md for the agent. If it does, use that content as the per-user seed. The check is conditional (predefined agent + USER.md not yet seeded per-user) so it adds at most one extra DB read per new user. The existing "don't overwrite" guard (hasFile check) is unchanged — personalized per-user content written through conversation is still never overwritten. * test: add unit tests for SeedUserFiles agent-level USER.md seeding Covers: - Predefined agent: wizard-written USER.md at agent level is used as per-user seed (primary regression test for the bug fix) - Predefined agent: falls back to blank embedded template when no agent-level USER.md exists - Predefined agent: existing per-user content is never overwritten - Open agent: embedded template path is unaffected - Predefined agent: BOOTSTRAP.md uses BOOTSTRAP_PREDEFINED.md template - Idempotency: second SeedUserFiles call seeds nothing * fix(bootstrap): SeedUserFiles uses agent-level USER.md as seed for predefined agents Use map[string]string for agent-level files (more general, extensible) and clean continue-based loop flow, matching the investigation spec in plans/goclaw-pr/goclaw-pr-seed-user-files.md exactly. Previous implementation used a single string and if/else structure — functionally correct but diverged from the spec in variable type, load condition, and loop exit pattern. * fix(agents): predefined agents visible in web UI for channel members Two bugs combined to make wizard-installed predefined agents invisible in the web UI for all real users. Bug 1 — agents.create WS hardcoded owner_id = "system": - Add `owner_ids []string` to WS params struct (omitempty, backward compat) - Resolve ownerID: use first entry if provided, fall back to "system" - Allows external provisioning tools to set a real user as owner Bug 2 — ListAccessible SQL excluded system-owned predefined agents: - Extend WHERE clause with 4th OR: predefined agents are visible when the requesting user appears in any enabled channel's allow_from list - Uses EXISTS + jsonb_array_elements_text(ci.config->'allow_from') - No schema migration needed — allow_from already populated by wizard Also adds 4 unit tests for Bug 1 (agents_create_owner_test.go): - UsesProvidedOwnerID, FallsBackToSystem_WhenAbsent, FallsBackToSystem_WhenEmpty, MultipleOwnerIDs_UsesFirst
This commit is contained in:
therichardngai-code authored and GitHub committed
1 parent
419935b9f9
commit
3cbdd8d778
5 files changed
+568
-9
No files matched your search
@@ -91,6 +91,12 @@ var userSeedFilesPredefined = []string{
|
||||
// For "open" agents: all 7 files (including BOOTSTRAP.md).
|
||||
// For "predefined" agents: USER.md + BOOTSTRAP.md (user-focused onboarding template).
|
||||
// Only writes files that don't already exist — safe to call multiple times.
|
||||
//
|
||||
// For predefined agents seeding USER.md: if the agent already has a populated
|
||||
// USER.md in agent_context_files (e.g. written by the wizard or management dashboard),
|
||||
// that content is used as the per-user seed instead of the blank embedded template.
|
||||
// This ensures wizard-configured owner profiles are preserved on first chat.
|
||||
//
|
||||
// Returns the list of file names that were seeded.
|
||||
func SeedUserFiles(ctx context.Context, agentStore store.AgentStore, agentID uuid.UUID, userID, agentType string) ([]string, error) {
|
||||
files := userSeedFilesOpen
|
||||
@@ -98,7 +104,7 @@ func SeedUserFiles(ctx context.Context, agentStore store.AgentStore, agentID uui
|
||||
files = userSeedFilesPredefined
|
||||
}
|
||||
|
||||
// Check existing files to avoid overwriting personalized content
|
||||
// Check existing per-user files to avoid overwriting personalized content
|
||||
existing, err := agentStore.GetUserContextFiles(ctx, agentID, userID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -110,10 +116,40 @@ func SeedUserFiles(ctx context.Context, agentStore store.AgentStore, agentID uui
|
||||
}
|
||||
}
|
||||
|
||||
// For predefined agents: load agent-level files once to use as seed fallback.
|
||||
// USER.md at agent-level may contain a pre-configured owner profile (e.g. set by
|
||||
// the wizard or management dashboard). Use it as the per-user seed instead of the
|
||||
// blank embedded template so the agent starts with the correct owner context.
|
||||
var agentLevelFiles map[string]string
|
||||
if agentType == store.AgentTypePredefined {
|
||||
agentFiles, err := agentStore.GetAgentContextFiles(ctx, agentID)
|
||||
if err == nil && len(agentFiles) > 0 {
|
||||
agentLevelFiles = make(map[string]string, len(agentFiles))
|
||||
for _, f := range agentFiles {
|
||||
if f.Content != "" {
|
||||
agentLevelFiles[f.FileName] = f.Content
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var seeded []string
|
||||
for _, name := range files {
|
||||
if hasFile[name] {
|
||||
continue // already has content, don't overwrite
|
||||
continue // already has personalized content, don't overwrite
|
||||
}
|
||||
|
||||
// For predefined agents seeding USER.md: prefer agent-level content as seed.
|
||||
// This propagates wizard/dashboard-configured owner profile to the first user.
|
||||
if agentType == store.AgentTypePredefined && name == UserFile {
|
||||
if agentContent, ok := agentLevelFiles[name]; ok {
|
||||
if err := agentStore.SetUserContextFile(ctx, agentID, userID, name, agentContent); err != nil {
|
||||
return seeded, err
|
||||
}
|
||||
seeded = append(seeded, name)
|
||||
continue
|
||||
}
|
||||
// No agent-level USER.md → fall through to blank embedded template
|
||||
}
|
||||
|
||||
// Predefined agents use a user-focused bootstrap template
|
||||
|
||||
@@ -0,0 +1,279 @@
|
||||
package bootstrap
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||
)
|
||||
|
||||
// ---- minimal AgentStore stub for seed tests ----
|
||||
|
||||
type seedStubStore struct {
|
||||
// agent-level files (simulates agent_context_files)
|
||||
agentFiles map[string]string // fileName → content
|
||||
// per-user files (simulates user_context_files)
|
||||
userFiles map[string]string // fileName → content (shared across all users for simplicity)
|
||||
// captures what was written per-user: fileName → content
|
||||
seededUserFiles map[string]string
|
||||
}
|
||||
|
||||
func newSeedStub() *seedStubStore {
|
||||
return &seedStubStore{
|
||||
agentFiles: make(map[string]string),
|
||||
userFiles: make(map[string]string),
|
||||
seededUserFiles: make(map[string]string),
|
||||
}
|
||||
}
|
||||
|
||||
func (s *seedStubStore) GetAgentContextFiles(_ context.Context, _ uuid.UUID) ([]store.AgentContextFileData, error) {
|
||||
var out []store.AgentContextFileData
|
||||
for name, content := range s.agentFiles {
|
||||
out = append(out, store.AgentContextFileData{FileName: name, Content: content})
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
func (s *seedStubStore) SetAgentContextFile(_ context.Context, _ uuid.UUID, name, content string) error {
|
||||
s.agentFiles[name] = content
|
||||
return nil
|
||||
}
|
||||
func (s *seedStubStore) GetUserContextFiles(_ context.Context, _ uuid.UUID, _ string) ([]store.UserContextFileData, error) {
|
||||
var out []store.UserContextFileData
|
||||
for name, content := range s.userFiles {
|
||||
out = append(out, store.UserContextFileData{FileName: name, Content: content})
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
func (s *seedStubStore) SetUserContextFile(_ context.Context, _ uuid.UUID, _, name, content string) error {
|
||||
s.seededUserFiles[name] = content
|
||||
return nil
|
||||
}
|
||||
func (s *seedStubStore) DeleteUserContextFile(_ context.Context, _ uuid.UUID, _, _ string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// Remaining interface methods — not exercised.
|
||||
func (s *seedStubStore) Create(_ context.Context, _ *store.AgentData) error { return nil }
|
||||
func (s *seedStubStore) GetByKey(_ context.Context, _ string) (*store.AgentData, error) { return nil, nil }
|
||||
func (s *seedStubStore) GetByID(_ context.Context, _ uuid.UUID) (*store.AgentData, error) { return nil, nil }
|
||||
func (s *seedStubStore) Update(_ context.Context, _ uuid.UUID, _ map[string]any) error { return nil }
|
||||
func (s *seedStubStore) Delete(_ context.Context, _ uuid.UUID) error { return nil }
|
||||
func (s *seedStubStore) List(_ context.Context, _ string) ([]store.AgentData, error) { return nil, nil }
|
||||
func (s *seedStubStore) ShareAgent(_ context.Context, _ uuid.UUID, _, _, _ string) error { return nil }
|
||||
func (s *seedStubStore) RevokeShare(_ context.Context, _ uuid.UUID, _ string) error { return nil }
|
||||
func (s *seedStubStore) ListShares(_ context.Context, _ uuid.UUID) ([]store.AgentShareData, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *seedStubStore) CanAccess(_ context.Context, _ uuid.UUID, _ string) (bool, string, error) {
|
||||
return true, "admin", nil
|
||||
}
|
||||
func (s *seedStubStore) ListAccessible(_ context.Context, _ string) ([]store.AgentData, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *seedStubStore) GetUserOverride(_ context.Context, _ uuid.UUID, _ string) (*store.UserAgentOverrideData, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *seedStubStore) SetUserOverride(_ context.Context, _ *store.UserAgentOverrideData) error {
|
||||
return nil
|
||||
}
|
||||
func (s *seedStubStore) GetOrCreateUserProfile(_ context.Context, _ uuid.UUID, _, _ string) (bool, error) {
|
||||
return false, nil
|
||||
}
|
||||
func (s *seedStubStore) IsGroupFileWriter(_ context.Context, _ uuid.UUID, _, _ string) (bool, error) {
|
||||
return false, nil
|
||||
}
|
||||
func (s *seedStubStore) AddGroupFileWriter(_ context.Context, _ uuid.UUID, _, _, _, _ string) error {
|
||||
return nil
|
||||
}
|
||||
func (s *seedStubStore) RemoveGroupFileWriter(_ context.Context, _ uuid.UUID, _, _ string) error {
|
||||
return nil
|
||||
}
|
||||
func (s *seedStubStore) ListGroupFileWriters(_ context.Context, _ uuid.UUID, _ string) ([]store.GroupFileWriterData, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
// ---- Tests ----
|
||||
|
||||
// TestSeedUserFiles_PredefinedAgent_UsesAgentLevelUserMD is the primary regression test.
|
||||
// When a predefined agent has wizard-written USER.md in agent_context_files, SeedUserFiles
|
||||
// must seed that content into user_context_files — NOT the blank embedded template.
|
||||
func TestSeedUserFiles_PredefinedAgent_UsesAgentLevelUserMD(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
wizardContent := "# User Profile\nOwner: Alice\nLanguage: English\nNotes: Prefers concise answers"
|
||||
|
||||
// Simulate wizard writing USER.md at agent level via agents.files.set
|
||||
as.agentFiles[UserFile] = wizardContent
|
||||
|
||||
seeded, err := SeedUserFiles(context.Background(), as, agentID, "user-alice", store.AgentTypePredefined)
|
||||
if err != nil {
|
||||
t.Fatalf("SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
|
||||
// USER.md must be in the seeded list
|
||||
foundUserMD := false
|
||||
for _, f := range seeded {
|
||||
if f == UserFile {
|
||||
foundUserMD = true
|
||||
}
|
||||
}
|
||||
if !foundUserMD {
|
||||
t.Errorf("USER.md not in seeded files list: %v", seeded)
|
||||
}
|
||||
|
||||
// The seeded USER.md must contain wizard content, not the blank template
|
||||
got, ok := as.seededUserFiles[UserFile]
|
||||
if !ok {
|
||||
t.Fatal("USER.md was not written to user_context_files")
|
||||
}
|
||||
if got != wizardContent {
|
||||
t.Errorf("seeded USER.md content mismatch:\n want: %q\n got: %q", wizardContent, got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSeedUserFiles_PredefinedAgent_FallsBackToTemplateWhenNoAgentLevelUserMD verifies
|
||||
// that when there is no wizard-written USER.md at agent level, the blank template is used.
|
||||
func TestSeedUserFiles_PredefinedAgent_FallsBackToTemplateWhenNoAgentLevelUserMD(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
// No agent-level USER.md — wizard did not write one
|
||||
|
||||
seeded, err := SeedUserFiles(context.Background(), as, agentID, "user-bob", store.AgentTypePredefined)
|
||||
if err != nil {
|
||||
t.Fatalf("SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
|
||||
// USER.md should still be seeded (from embedded template)
|
||||
foundUserMD := false
|
||||
for _, f := range seeded {
|
||||
if f == UserFile {
|
||||
foundUserMD = true
|
||||
}
|
||||
}
|
||||
if !foundUserMD {
|
||||
t.Errorf("USER.md should be seeded from template when no agent-level file exists: %v", seeded)
|
||||
}
|
||||
|
||||
// Content should be the embedded template (non-empty — the template file exists)
|
||||
got, ok := as.seededUserFiles[UserFile]
|
||||
if !ok {
|
||||
t.Fatal("USER.md was not written to user_context_files")
|
||||
}
|
||||
if got == "" {
|
||||
t.Error("seeded USER.md should not be empty (expected embedded template content)")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSeedUserFiles_PredefinedAgent_DoesNotOverwriteExistingPerUserContent verifies
|
||||
// that personalized per-user USER.md written via conversation is never overwritten.
|
||||
func TestSeedUserFiles_PredefinedAgent_DoesNotOverwriteExistingPerUserContent(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
personalContent := "# User Profile\nMy customized personal content"
|
||||
|
||||
// Pre-populate per-user USER.md (simulates user who already chatted and personalized)
|
||||
as.userFiles[UserFile] = personalContent
|
||||
// Also set wizard content at agent level
|
||||
as.agentFiles[UserFile] = "wizard content that should NOT override personal content"
|
||||
|
||||
seeded, err := SeedUserFiles(context.Background(), as, agentID, "user-charlie", store.AgentTypePredefined)
|
||||
if err != nil {
|
||||
t.Fatalf("SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
|
||||
// USER.md must NOT be in the seeded list (existing content, should skip)
|
||||
for _, f := range seeded {
|
||||
if f == UserFile {
|
||||
t.Error("USER.md should NOT be re-seeded when per-user content already exists")
|
||||
}
|
||||
}
|
||||
|
||||
// SetUserContextFile must not have been called for USER.md
|
||||
if _, wrote := as.seededUserFiles[UserFile]; wrote {
|
||||
t.Error("SetUserContextFile should not be called when per-user USER.md already has content")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSeedUserFiles_OpenAgent_UsesEmbeddedTemplate verifies that open agents
|
||||
// are completely unaffected — they still receive embedded templates per-user.
|
||||
func TestSeedUserFiles_OpenAgent_UsesEmbeddedTemplate(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
// Open agents should never check agent_context_files for USER.md
|
||||
|
||||
seeded, err := SeedUserFiles(context.Background(), as, agentID, "user-dave", store.AgentTypeOpen)
|
||||
if err != nil {
|
||||
t.Fatalf("SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
|
||||
// Open agents seed the full set: AGENTS.md, SOUL.md, IDENTITY.md, USER.md, BOOTSTRAP.md
|
||||
expectedFiles := map[string]bool{
|
||||
AgentsFile: true, SoulFile: true, IdentityFile: true, UserFile: true, BootstrapFile: true,
|
||||
}
|
||||
for _, f := range seeded {
|
||||
delete(expectedFiles, f)
|
||||
}
|
||||
if len(expectedFiles) > 0 {
|
||||
t.Errorf("open agent: missing seeded files: %v", expectedFiles)
|
||||
}
|
||||
|
||||
// USER.md must have been written using embedded template (non-empty)
|
||||
got, ok := as.seededUserFiles[UserFile]
|
||||
if !ok {
|
||||
t.Fatal("open agent: USER.md was not written to user_context_files")
|
||||
}
|
||||
if got == "" {
|
||||
t.Error("open agent: seeded USER.md should not be empty")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSeedUserFiles_PredefinedAgent_BootstrapUsesCorrectTemplate verifies that
|
||||
// BOOTSTRAP.md is still seeded from BOOTSTRAP_PREDEFINED.md for predefined agents.
|
||||
func TestSeedUserFiles_PredefinedAgent_BootstrapUsesCorrectTemplate(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
|
||||
_, err := SeedUserFiles(context.Background(), as, agentID, "user-eve", store.AgentTypePredefined)
|
||||
if err != nil {
|
||||
t.Fatalf("SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
|
||||
// BOOTSTRAP.md must have been seeded
|
||||
got, ok := as.seededUserFiles[BootstrapFile]
|
||||
if !ok {
|
||||
t.Fatal("predefined agent: BOOTSTRAP.md was not seeded")
|
||||
}
|
||||
if got == "" {
|
||||
t.Error("predefined agent: BOOTSTRAP.md should have content from BOOTSTRAP_PREDEFINED.md template")
|
||||
}
|
||||
}
|
||||
|
||||
// TestSeedUserFiles_IdempotentOnSecondCall verifies that calling SeedUserFiles
|
||||
// a second time for the same user does not re-seed already-present files.
|
||||
func TestSeedUserFiles_IdempotentOnSecondCall(t *testing.T) {
|
||||
as := newSeedStub()
|
||||
agentID := uuid.New()
|
||||
|
||||
// First call — seeds files
|
||||
SeedUserFiles(context.Background(), as, agentID, "user-frank", store.AgentTypePredefined)
|
||||
|
||||
// Simulate what the first call wrote (move seededUserFiles → userFiles)
|
||||
for k, v := range as.seededUserFiles {
|
||||
as.userFiles[k] = v
|
||||
}
|
||||
as.seededUserFiles = make(map[string]string)
|
||||
|
||||
// Second call — must seed nothing (all files already exist)
|
||||
seeded, err := SeedUserFiles(context.Background(), as, agentID, "user-frank", store.AgentTypePredefined)
|
||||
if err != nil {
|
||||
t.Fatalf("second SeedUserFiles returned error: %v", err)
|
||||
}
|
||||
if len(seeded) != 0 {
|
||||
t.Errorf("second call should seed nothing, but seeded: %v", seeded)
|
||||
}
|
||||
if len(as.seededUserFiles) != 0 {
|
||||
t.Errorf("second call should not write any files, but wrote: %v", as.seededUserFiles)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user