feat(skills): privacy/visibility controls for agent-owned skills

[mrgoonie]

Closes #1009

Summary

• Add private/public visibility enum (validator + normalizer) in internal/skills/visibility.go. Default: private.
• Add IsSkillVisibleTo / FilterVisibleSkills authorization helper in internal/store/visibility_filter.go, using the same three-identity ownership check (actor/user/sender) introduced for Bug - Telegram group: write_file checks writer permission against group:telegram:<id> instead of sender user #915.
• Propagate owner_id into SkillInfo and all PG/SQLite SELECTs so the filter has the data it needs.
• Security fix: agent-injection path (FilterSkills with nil allowList) now hides other users' private skills — closes the leak across tenant members.
• publish_skill: accept visibility parameter (replaces hardcoded "private").
• skill_manage: visibility settable on create and editable via patch, including a content-less visibility-only patch path (no version bump).
• skills.list / skills.get RPC: admin bypass + visibility gate — non-admins only see system + public + own-private skills; private skills 404 for non-owners.
• skills.update RPC: validate + normalize visibility enum before persist (fail closed on unknown).

Test plan

• go build ./... (PG) green
• go build -tags sqliteonly ./... (desktop/lite) green
• go vet ./... clean
• Unit tests: TestValidateVisibility, TestNormalizeVisibility, TestIsSkillVisibleTo, TestFilterVisibleSkills pass
• Touched-package tests pass (internal/skills, internal/store/..., internal/tools, internal/gateway/...)
• Manual: agent A publishes private skill → agent B's skills.list does not include it → skills.get returns NotFound
• Manual: admin sees all skills regardless of visibility

Notes

• team visibility rejected in v1 (deferred until team-scoping semantics resolved) — surfaces as validator error.
• Empty/legacy visibility treated as public; private rows without owner_id also treated as public to avoid hiding legacy/system-like records.

[mrgoonie]

@claude review

[claude]

Claude finished @mrgoonie's task in 2m 40s —— View job

---

PR Review: feat(skills): privacy/visibility controls for agent-owned skills

Tasks:

• Gather context and understand the PR
• Read visibility core logic (internal/skills/visibility.go, internal/store/visibility_filter.go)
• Review store changes (PG + SQLite)
• Review gateway/RPC handler changes
• Review tool changes (publish_skill.go, skill_manage.go)
• Review tests
• Compile and summarize findings

---

Summary

Solid security feature overall — the core visibility model (IsSkillVisibleTo, FilterVisibleSkills), the three-identity ownership pattern, the nil-allowList security fix, and the validator/normalizer are all well-designed. One critical bug will silently break the ownership check in skills.update for all non-admin users. One medium issue with empty-visibility normalization can accidentally leak private skills.

---

🔴 Critical: skillOwnerGetter interface signature mismatch — ownership check never fires

File: internal/gateway/methods/skills.go:19-20

type skillOwnerGetter interface {
    GetSkillOwnerID(id uuid.UUID) (string, bool)   // ← missing ctx
}

Both PGSkillStore and SQLiteSkillStore implement:

func (s *PGSkillStore) GetSkillOwnerID(ctx context.Context, id uuid.UUID) (string, bool)

Because the interface doesn't include context.Context, the type assertion m.store.(skillOwnerGetter) at line 228 will always fail — no store ever satisfies this interface. The handler falls into the "store doesn't implement skillOwnerGetter" branch and returns ErrUnauthorized for every non-admin caller. In practice:

• Skill owners can never update their own skills via skills.update RPC.
• The ownership check code at line 233 is dead code — it is never reached.

Fix: add ctx to the interface and the call site.

type skillOwnerGetter interface {
    GetSkillOwnerID(ctx context.Context, id uuid.UUID) (string, bool)
}
// …
if ownerID, found := ownerGetter.GetSkillOwnerID(ctx, skillID); found && ownerID != client.UserID() {

Fix this →

---

🟡 Medium: Empty visibility string in updates bypasses normalization — silently becomes "public"

File: internal/gateway/methods/skills.go:214-223

if v, ok := params.Updates["visibility"]; ok {
    vs, _ := v.(string)
    if err := skills.ValidateVisibility(vs); err != nil { … }
    if vs != "" {
        params.Updates["visibility"] = skills.NormalizeVisibility(vs)
    }
    // vs=="" falls through — stores "" in DB, treated as "public" by IsSkillVisibleTo
}

A client that sends {"updates": {"visibility": ""}} will write "" to the database. IsSkillVisibleTo treats "" as public (legacy-data compat), so a private skill becomes discoverable without any explicit "set to public" intent. The fix is to reject an empty visibility in an update context, since ValidateVisibility accepts empty (for the "omit = use default" flow in create paths, not update paths):

if vs == "" {
    // empty is not a valid update value — omit the key or send "private"/"public"
    client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest,
        "visibility cannot be empty; use \"private\" or \"public\""))
    return
}

Fix this →

---

🟢 Low / Observations

owner_id not exposed in handleList / handleGet responses
handleList (lines 48-77) and handleGet (lines 135-152) expose visibility but not owner_id. Clients cannot tell whether they own a skill. The caller can infer ownership by comparing visibility == "private" to their own session identity, but that's fragile. Consider adding "owner_id" conditionally (only when the skill is private and the caller is the owner, to avoid leaking ownership metadata to non-owners).

is_system missing from handleGet response
handleList includes "is_system" (line 56) but handleGet does not (lines 135-152). Inconsistent API surface.

FilterSkills allowList path skips visibility check
In both pg/skills_content.go:139-146 and sqlitestore/skills_content.go:126-137, when allowList != nil, the visibility filter is not applied — an explicitly-granted skill is always injected regardless of visibility. This is intentional (grant = explicit permission) but undocumented. Worth a short comment in FilterSkills so future maintainers don't "fix" it inadvertently.

executePatch calls t.loader.BumpVersion() only
The store's UpdateSkill already calls s.BumpVersion() (verified in pg/skills_crud.go:42, sqlitestore/skills_crud.go line 244/261), so the list-cache is properly invalidated. The extra t.loader.BumpVersion() in skill_manage.go:299 is correct. No issue here — just confirming it's safe.

---

✅ What's well done

• Core visibility model is clean and well-scoped. Three-identity ownership check is consistent with Bug - Telegram group: write_file checks writer permission against group:telegram:<id> instead of sender user #915 and properly documented.
• Fail-closed on unknown enum: IsSkillVisibleTo returning false for unrecognized visibility values is the right default.
• Legacy data safety: treating empty or unowned private records as public prevents hidden-data regressions on upgrade.
• Visibility-only patch path (no version bump) is a nice optimization.
• Unit tests for all the new helpers are thorough and cover the key edge cases.
• publish_skill and skill_manage visibility parameter handling is consistent.
• Build-tag parity: both PG and SQLite paths updated symmetrically.

---

The interface mismatch in skillOwnerGetter is the blocker — it makes the ownership gate on skills.update entirely inoperative for non-admins. Everything else is minor or stylistic.