diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index cdf782e3..5a42f698 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -133,16 +133,28 @@ func (s *PGSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, us // ListAccessible returns skills accessible to a given agent+user combination. // Access logic: public → all, private → owner only, internal → check grants. // System skills (is_system=true) are always visible regardless of tenant. +// +// To preserve visibility across the ACTOR-vs-SCOPE split (#915), the query +// matches owner_id and user grant rows against BOTH the caller-supplied +// userID (scope identity, legacy rows) and the actor identity from ctx +// (individual sender, new rows). In DM contexts these are identical and +// the OR clause collapses; in group chats they diverge and the OR ensures +// a publisher can still see their own private skill. func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) { + actorID := store.ActorIDFromContext(ctx) + if actorID == "" { + actorID = userID + } // tenant filter: system skills bypass it, tenant-owned skills are filtered - tc, tcArgs, _, err := scopeClause(ctx, 3) + // (bumped from $3 to $4 to accommodate the new actorID bind at $3). + tc, tcArgs, _, err := scopeClause(ctx, 4) if err != nil { return nil, err } tenantCond := "" if tc != "" { - // tc is " AND tenant_id = $3"; we need it as an OR condition inside the WHERE - tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 3) + // tc is " AND tenant_id = $4"; we need it as an OR condition inside the WHERE + tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 4) _ = tc // tcArgs carries the value } // LEFT JOIN skill_tenant_configs to exclude per-tenant disabled skills. @@ -150,20 +162,20 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us stcJoin := "" stcFilter := "" if len(tcArgs) > 0 { - stcJoin = fmt.Sprintf(" LEFT JOIN skill_tenant_configs stc ON s.id = stc.skill_id AND stc.tenant_id = $%d", 3) + stcJoin = fmt.Sprintf(" LEFT JOIN skill_tenant_configs stc ON s.id = stc.skill_id AND stc.tenant_id = $%d", 4) stcFilter = " AND (stc.enabled IS NULL OR stc.enabled = true)" } rows, err := s.db.QueryContext(ctx, `SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1 - LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND sug.user_id = $2`+stcJoin+` + LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = $2 OR sug.user_id = $3)`+stcJoin+` WHERE s.status = 'active'`+tenantCond+stcFilter+` AND ( s.is_system = true OR s.visibility = 'public' - OR (s.visibility = 'private' AND s.owner_id = $2) + OR (s.visibility = 'private' AND (s.owner_id = $2 OR s.owner_id = $3)) OR (s.visibility = 'internal' AND (sag.id IS NOT NULL OR sug.id IS NOT NULL)) ) - ORDER BY s.name`, append([]any{agentID, userID}, tcArgs...)...) + ORDER BY s.name`, append([]any{agentID, userID, actorID}, tcArgs...)...) if err != nil { return nil, err } diff --git a/internal/store/sqlitestore/skills_grants.go b/internal/store/sqlitestore/skills_grants.go index 60e5de02..344189c8 100644 --- a/internal/store/sqlitestore/skills_grants.go +++ b/internal/store/sqlitestore/skills_grants.go @@ -133,7 +133,12 @@ func (s *SQLiteSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID } // ListAccessible returns skills accessible to a given agent+user combination. +// See PGSkillStore.ListAccessible for the ACTOR-vs-SCOPE dual-match rationale. func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) { + actorID := store.ActorIDFromContext(ctx) + if actorID == "" { + actorID = userID + } tClause, tArgs, err := scopeClauseAlias(ctx, "s") if err != nil { return nil, err @@ -147,8 +152,8 @@ func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID stcFilter = " AND (stc.enabled IS NULL OR stc.enabled = 1)" } - // Build args: agentID, userID, [tenantID for tenant cond, tenantID for stc join] - queryArgs := []any{agentID, userID} + // Positional args: agentID, userID, actorID, [tenantID x2 if tenant-scoped], userID, actorID (private-owner clause) + queryArgs := []any{agentID, userID, actorID} if len(tArgs) > 0 { queryArgs = append(queryArgs, tArgs...) // tenant cond queryArgs = append(queryArgs, tArgs...) // stc join @@ -159,15 +164,15 @@ func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID rows, err := s.db.QueryContext(ctx, `SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ? - LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND sug.user_id = ?`+stcJoin+` + LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = ? OR sug.user_id = ?)`+stcJoin+` WHERE s.status = 'active'`+tenantCond+stcFilter+` AND ( s.is_system = 1 OR s.visibility = 'public' - OR (s.visibility = 'private' AND s.owner_id = ?) + OR (s.visibility = 'private' AND (s.owner_id = ? OR s.owner_id = ?)) OR (s.visibility = 'internal' AND (sag.id IS NOT NULL OR sug.id IS NOT NULL)) ) ORDER BY s.name`, - append(queryArgs, userID)..., + append(queryArgs, userID, actorID)..., ) if err != nil { return nil, err diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index 0131231a..7ea13e1e 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -32,6 +32,25 @@ func NewSkillManageTool(skills store.SkillManageStore, baseDir, dataDir string, return &SkillManageTool{skills: skills, base: baseDir, dataDir: dataDir, loader: loader} } +// isOwnerOfSkill returns true if the caller owns the skill identified by slug. +// Matches owner_id against three identities for backward compatibility (#915): +// - ActorIDFromContext: current helper, merge-aware in DM, sender in groups +// - UserIDFromContext: legacy rows pre-#915 (group-scope) and merged tenant id +// - SenderIDFromContext: raw channel sender (covers the pre-merge-aware ActorID window) +// +// If the slug does not exist, returns true (caller sees "skill not found" error +// from the downstream resolver — preserves the existing error surface). +func isOwnerOfSkill(ctx context.Context, skills store.SkillManageStore, slug string) bool { + ownerID, found := skills.GetSkillOwnerIDBySlug(ctx, slug) + if !found { + return true + } + actorID := store.ActorIDFromContext(ctx) + userID := store.UserIDFromContext(ctx) + senderID := store.SenderIDFromContext(ctx) + return ownerID == actorID || ownerID == userID || ownerID == senderID +} + // tenantSkillsDir returns the skills-store directory scoped to the calling agent's tenant. func (t *SkillManageTool) tenantSkillsDir(ctx context.Context) string { tid := store.TenantIDFromContext(ctx) @@ -235,13 +254,15 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) } // Ownership check: only the skill owner can patch. - // Accept actor (new post-#915 format) or user-scope (legacy pre-#915 rows - // where owner_id was set to the group principal). Legacy fallback lets - // any group member manage skills created before the ACTOR migration; - // they can re-publish under their individual identity to tighten ownership. - actorID := store.ActorIDFromContext(ctx) - legacyID := store.UserIDFromContext(ctx) - if ownerID, found := t.skills.GetSkillOwnerIDBySlug(ctx, slug); found && ownerID != actorID && ownerID != legacyID { + // Accept any of three identities the same human maps to: + // - actor (current merge-aware helper — preferred for new rows) + // - userID (legacy pre-#915 rows where owner_id was group principal + // or the merged tenant identity) + // - senderID (legacy rows from the pre-merge-aware ActorID window + // where DM owners got the raw channel sender) + // A DM user merged to "viettx" with Telegram ID "386246614" matches all + // three of their skills regardless of when they were created. + if !isOwnerOfSkill(ctx, t.skills, slug) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } @@ -330,10 +351,8 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any } // Ownership check: only the skill owner can delete. - // Same legacy fallback pattern as patch-flow above (#915). - deleteActorID := store.ActorIDFromContext(ctx) - deleteLegacyID := store.UserIDFromContext(ctx) - if ownerID, found := t.skills.GetSkillOwnerIDBySlug(ctx, slug); found && ownerID != deleteActorID && ownerID != deleteLegacyID { + // Same three-identity match as the patch flow above (#915). + if !isOwnerOfSkill(ctx, t.skills, slug) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) }