agentmux_srv/backend/storage/
skills.rs

1// Copyright 2025-2026, AgentMux Corp.
2// SPDX-License-Identifier: Apache-2.0
3
4//! Agent skills — reusable capability records attached to an agent
5//! definition.
6//!
7//! Extracted from `store.rs` in Phase R.4 of the storage
8//! modularization plan
9//! (`docs/specs/SPEC_STORE_MODULARIZATION_2026_05_27.md`). The
10//! method surface is unchanged — `Store::agent_skill_*` still
11//! lives on `Store` via this `impl` block; callers stay on
12//! `storage::store::AgentSkill` thanks to the re-export.
13
14use rusqlite::params;
15use serde::{Deserialize, Serialize};
16
17use super::error::StoreError;
18use super::managed::{ManagedResource, Owner};
19use super::store::Store;
20
21/// A reusable skill/capability attached to a agent definition.
22#[derive(Debug, Clone, Serialize, Deserialize, ts_rs::TS)]
23#[ts(export, export_to = "../../../frontend/types/rpc/")]
24pub struct AgentSkill {
25    pub id: String,
26    pub agent_id: String,
27    pub name: String,
28    pub trigger: String,
29    /// Free-form rather than an enum: rows written by Memory → Skills and
30    /// by older builds both land here, so a closed set would fail to read
31    /// them back. `createagentskill` defaults it to "prompt".
32    pub skill_type: String,
33    pub description: String,
34    pub content: String,
35    #[ts(type = "number")]
36    pub created_at: i64,
37}
38
39impl Store {
40    /// List all skills for an agent, ordered by created_at ascending.
41    /// LOCAL channel's skills only — NO cross-channel fallback. Used by the
42    /// def-registry mirror (which always operates on a local agent) and by
43    /// `agent_skill_list`.
44    pub(super) fn agent_skill_list_local(
45        &self,
46        agent_id: &str,
47    ) -> Result<Vec<AgentSkill>, StoreError> {
48        let conn = self.conn.lock().unwrap();
49        let mut stmt = conn.prepare(
50            "SELECT id, agent_id, name, trigger, skill_type, description, content, created_at
51             FROM db_agent_skills WHERE agent_id=?1 ORDER BY created_at ASC",
52        )?;
53        let rows = stmt.query_map(params![agent_id], |row| {
54            Ok(AgentSkill {
55                id: row.get(0)?,
56                agent_id: row.get(1)?,
57                name: row.get(2)?,
58                trigger: row.get(3)?,
59                skill_type: row.get(4)?,
60                description: row.get(5)?,
61                content: row.get(6)?,
62                created_at: row.get(7)?,
63            })
64        })?;
65        let mut skills = Vec::new();
66        for row in rows {
67            skills.push(row?);
68        }
69        Ok(skills)
70    }
71
72    pub fn agent_skill_list(&self, agent_id: &str) -> Result<Vec<AgentSkill>, StoreError> {
73        let local = self.agent_skill_list_local(agent_id)?;
74        if !local.is_empty() {
75            return Ok(local);
76        }
77        // Fall back to the global record ONLY for a cross-channel agent
78        // (absent from local SQLite); a locally-known agent with genuinely no
79        // skills must return empty, not resurrect them. (reagent P1 on #1385.)
80        if self.agent_def_exists_local(agent_id)? {
81            return Ok(local);
82        }
83        if let Some(reg) = self.shared_def_registry() {
84            if let Ok(Some(rec)) = reg.get(agent_id) {
85                return Ok(rec
86                    .data
87                    .skills
88                    .iter()
89                    .map(|s| AgentSkill {
90                        id: s.id.clone(),
91                        agent_id: agent_id.to_string(),
92                        name: s.name.clone(),
93                        trigger: s.trigger.clone(),
94                        skill_type: s.skill_type.clone(),
95                        description: s.description.clone(),
96                        content: s.content.clone(),
97                        created_at: rec.data.created_at,
98                    })
99                    .collect());
100            }
101        }
102        Ok(local)
103    }
104
105    /// Get a single skill by id.
106    pub fn agent_skill_get(&self, id: &str) -> Result<Option<AgentSkill>, StoreError> {
107        let conn = self.conn.lock().unwrap();
108        let mut stmt = conn.prepare(
109            "SELECT id, agent_id, name, trigger, skill_type, description, content, created_at
110             FROM db_agent_skills WHERE id=?1",
111        )?;
112        let result = stmt.query_row(params![id], |row| {
113            Ok(AgentSkill {
114                id: row.get(0)?,
115                agent_id: row.get(1)?,
116                name: row.get(2)?,
117                trigger: row.get(3)?,
118                skill_type: row.get(4)?,
119                description: row.get(5)?,
120                content: row.get(6)?,
121                created_at: row.get(7)?,
122            })
123        });
124        match result {
125            Ok(skill) => Ok(Some(skill)),
126            Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None),
127            Err(e) => Err(StoreError::Sqlite(e)),
128        }
129    }
130
131    /// Insert a new skill.
132    pub fn agent_skill_insert(&self, skill: &AgentSkill) -> Result<(), StoreError> {
133        {
134            let conn = self.conn.lock().unwrap();
135            conn.execute(
136                "INSERT INTO db_agent_skills (id, agent_id, name, trigger, skill_type, description, content, created_at)
137                 VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)",
138                params![
139                    skill.id,
140                    skill.agent_id,
141                    skill.name,
142                    skill.trigger,
143                    skill.skill_type,
144                    skill.description,
145                    skill.content,
146                    skill.created_at
147                ],
148            )?;
149        }
150        // Re-mirror the owning definition — only refreshes an EXISTING
151        // mirror; never creates one for a bare template-launch row (reagent
152        // P1 on #3092; see registry_def_refresh_if_mirrored's doc comment).
153        // No-op for seeded templates too. (P0.2b.)
154        self.registry_def_refresh_if_mirrored(&skill.agent_id);
155        Ok(())
156    }
157
158    /// Update an existing skill (all fields except id, agent_id, created_at).
159    pub fn agent_skill_update(&self, skill: &AgentSkill) -> Result<bool, StoreError> {
160        let rows = {
161            let conn = self.conn.lock().unwrap();
162            conn.execute(
163                "UPDATE db_agent_skills SET name=?1, trigger=?2, skill_type=?3, description=?4, content=?5
164                 WHERE id=?6",
165                params![
166                    skill.name,
167                    skill.trigger,
168                    skill.skill_type,
169                    skill.description,
170                    skill.content,
171                    skill.id
172                ],
173            )?
174        };
175        // conn dropped — re-mirror the definition (no-op for seeded, and
176        // for a bare template-launch row — see registry_def_refresh_if_mirrored).
177        // (P0.2b.)
178        if rows > 0 {
179            self.registry_def_refresh_if_mirrored(&skill.agent_id);
180        }
181        Ok(rows > 0)
182    }
183
184    /// Delete a legacy skill by id. Returns true if a row was deleted.
185    pub fn agent_skill_delete(&self, id: &str) -> Result<bool, StoreError> {
186        let (rows, agent_id) = {
187            let conn = self.conn.lock().unwrap();
188            // Capture the owning agent_id before the delete so we can
189            // re-mirror its definition afterwards.
190            let agent_id: Option<String> = match conn.query_row(
191                "SELECT agent_id FROM db_agent_skills WHERE id=?1",
192                params![id],
193                |row| row.get(0),
194            ) {
195                Ok(v) => Some(v),
196                Err(rusqlite::Error::QueryReturnedNoRows) => None,
197                Err(e) => return Err(StoreError::Sqlite(e)),
198            };
199            let rows = conn.execute("DELETE FROM db_agent_skills WHERE id=?1", params![id])?;
200            (rows, agent_id)
201        };
202        // conn dropped — re-mirror the definition so the global record drops
203        // the removed skill (no-op for seeded templates, and for a bare
204        // template-launch row — see registry_def_refresh_if_mirrored). (P0.2b.)
205        if rows > 0 {
206            if let Some(aid) = agent_id {
207                self.registry_def_refresh_if_mirrored(&aid);
208            }
209        }
210        Ok(rows > 0)
211    }
212}
213
214// ---------------------------------------------------------------------------
215// v1 standalone Skill primitive
216// ---------------------------------------------------------------------------
217
218/// Standalone skill primitive (v1 composable model).
219/// Not bound to a specific agent at rest — agents reference via db_agent_skills_ref.
220#[derive(Debug, Clone, Serialize, Deserialize, ts_rs::TS)]
221#[ts(export, export_to = "../../../frontend/types/rpc/")]
222pub struct Skill {
223    pub id: String,
224    pub name: String,
225    pub trigger: String,
226    pub skill_type: String,
227    pub description: String,
228    pub content: String,
229    pub is_global: bool,
230    #[ts(type = "number")]
231    pub created_at: i64,
232    #[ts(type = "number")]
233    pub updated_at: i64,
234}
235
236impl ManagedResource for Skill {
237    const TABLE: &'static str = "db_skills";
238    const REF_COL: &'static str = "skill_id";
239    const AGENT_REF_TABLE: &'static str = "db_agent_skills_ref";
240    const BUNDLE_REF_TABLE: &'static str = "db_bundle_skills_ref";
241    const COLUMNS: &'static [&'static str] = &[
242        "id", "name", "trigger", "skill_type", "description", "content", "is_global", "created_at", "updated_at",
243    ];
244    const UPDATE_COLUMNS: &'static [&'static str] =
245        &["name", "trigger", "skill_type", "description", "content", "updated_at"];
246    const NAME_NOUN: &'static str = "skill";
247    const ARTICLE_NOUN: &'static str = "a skill";
248    const KIND: &'static str = "skill";
249
250    fn from_row(row: &rusqlite::Row<'_>) -> rusqlite::Result<Self> {
251        Ok(Skill {
252            id: row.get(0)?,
253            name: row.get(1)?,
254            trigger: row.get(2)?,
255            skill_type: row.get(3)?,
256            description: row.get(4)?,
257            content: row.get(5)?,
258            is_global: row.get::<_, i64>(6)? != 0,
259            created_at: row.get(7)?,
260            updated_at: row.get(8)?,
261        })
262    }
263    fn values(&self) -> Vec<rusqlite::types::Value> {
264        use rusqlite::types::Value;
265        vec![
266            Value::Text(self.id.clone()),
267            Value::Text(self.name.clone()),
268            Value::Text(self.trigger.clone()),
269            Value::Text(self.skill_type.clone()),
270            Value::Text(self.description.clone()),
271            Value::Text(self.content.clone()),
272            Value::Integer(if self.is_global { 1 } else { 0 }),
273            Value::Integer(self.created_at),
274            Value::Integer(self.updated_at),
275        ]
276    }
277    fn id(&self) -> &str {
278        &self.id
279    }
280    fn name(&self) -> &str {
281        &self.name
282    }
283    fn is_global(&self) -> bool {
284        self.is_global
285    }
286    fn updated_at(&self) -> i64 {
287        self.updated_at
288    }
289}
290
291/// `skill_list`'s response shape: the skill plus whether the requesting agent
292/// specifically holds a `db_agent_skills_ref` row for it. Mirrors
293/// `McpServerListItem` — see its doc comment for why `is_global` alone isn't
294/// enough to render bind/unbind as a stateful toggle (tracked in #1960).
295#[derive(Debug, Clone, Serialize, ts_rs::TS)]
296#[ts(export, export_to = "../../../frontend/types/rpc/")]
297pub struct SkillListItem {
298    #[serde(flatten)]
299    pub skill: Skill,
300    pub bound_to_agent: bool,
301}
302
303/// `skill_list_global`'s response shape: the skill plus how many agents
304/// currently hold a `db_agent_skills_ref` to it — Memory → Skills'
305/// "used by N agents" count (gap #2 of #1960).
306#[derive(Debug, Clone, Serialize, ts_rs::TS)]
307#[ts(export, export_to = "../../../frontend/types/rpc/")]
308pub struct SkillCatalogItem {
309    #[serde(flatten)]
310    pub skill: Skill,
311    #[ts(type = "number")]
312    pub bound_count: i64,
313}
314
315/// `bundle_skill_list`'s response shape — mirrors `SkillListItem`, but
316/// "bound" means a `db_bundle_skills_ref` row for this bundle, not an
317/// agent's `db_agent_skills_ref` row. Composable model v2,
318/// docs/specs/SPEC_BUNDLE_AS_CONTAINER_V2_2026_08_17.md.
319#[derive(Debug, Clone, Serialize, ts_rs::TS)]
320#[ts(export, export_to = "../../../frontend/types/rpc/")]
321pub struct SkillBundleListItem {
322    #[serde(flatten)]
323    pub skill: Skill,
324    pub bound_to_bundle: bool,
325}
326
327impl Store {
328    /// List all skills visible to an agent: own (referenced) + global, each
329    /// annotated with whether this specific agent holds the bind ref.
330    /// `catalog` is where `db_skills` is authoritative — `identity_store` in
331    /// production (Phase 2 of SPEC_DURABLE_BINDINGS_2026_09_10.md §5.4).
332    pub fn skill_list(&self, catalog: &Store, agent_id: &str) -> Result<Vec<SkillListItem>, StoreError> {
333        Ok(self
334            .managed_list::<Skill>(catalog, Owner::Agent, agent_id)?
335            .into_iter()
336            .map(|(skill, bound_to_agent)| SkillListItem { skill, bound_to_agent })
337            .collect())
338    }
339
340    /// Effective skills for an agent at launch/config-materialization time:
341    /// this agent's own ref-bound skills (via `skill_list`) take over
342    /// *entirely* when present (globals still included), otherwise fall back
343    /// to the legacy `db_agent_skills` table with globals layered on top. The
344    /// fallback decision is gated on *own* refs only — not the
345    /// global-inclusive list — so adding a global skill never discards a
346    /// legacy-only agent's skills.
347    ///
348    /// Single source of truth for two call sites that must stay consistent:
349    /// `write_agent_config_files` (the authoritative Rust materialization
350    /// path) and the `listagentskills` RPC handler, which the frontend's
351    /// pre-launch `ListAgentSkillsCommand` call depends on for its own
352    /// `buildConfigFiles` mirror. Before this was extracted, `listagentskills`
353    /// returned only legacy skills — silently hiding every standalone/Memory →
354    /// Skills skill (including any Agent-Skills-format one, see
355    /// SKILL_TYPE_AGENT_SKILL) from the actual "click Launch" flow, since
356    /// that RPC is window-scoped (no `check_s1`) and can't call the
357    /// agent-scoped `skill.list` RPC the way an already-running, authenticated
358    /// agent connection could (reagent P0 on PR #2322 — the launch UI is not
359    /// an authenticated agent connection, so it was never able to reach the
360    /// standalone Skill primitive via the RPC layer at all).
361    pub fn effective_skills(&self, catalog: &Store, agent_id: &str) -> Vec<AgentSkill> {
362        let legacy_skills = self.agent_skill_list(agent_id).unwrap_or_default();
363        let mut visible_skills: Vec<Skill> = self
364            .skill_list(catalog, agent_id)
365            .unwrap_or_default()
366            .into_iter()
367            .map(|item| item.skill)
368            .collect();
369        // reagentx P1 on PR #2639: `has_own_skill_refs` must be decided
370        // BEFORE unioning in the bundle's referenced skills below — it
371        // reflects only the agent's OWN direct binds. Mandatory ABF means
372        // every agent has a bundle, so if a bundle's private skill alone
373        // could flip this flag, a legacy-only agent bound to any bundle
374        // that happens to reference one private skill would silently lose
375        // every `db_agent_skills` entry, with no action taken on the agent
376        // itself. The "own refs are authoritative" path is meant strictly
377        // for the agent's own binds (see `own_refs_take_over_entirely_and_discard_legacy_skills`).
378        let has_own_skill_refs = visible_skills.iter().any(|s| !s.is_global);
379        // Composable model v2 (docs/specs/SPEC_BUNDLE_AS_CONTAINER_V2_2026_08_17.md,
380        // GH issue #2024 item 3): union in the agent's bound bundle's own
381        // referenced skills too — without this, db_bundle_skills_ref would
382        // be exactly as inert at launch as the bundle's old inline `skills`
383        // JSON column always was. Deduped by id since a global skill is
384        // already visible via both `skill_list` above and
385        // `bundle_skill_list` below. Happens AFTER the has_own decision —
386        // bundle-referenced skills still appear in the final list, they
387        // just never trigger the "discard legacy" path on their own.
388        self.managed_union_bundle_refs(catalog, agent_id, &mut visible_skills);
389        if has_own_skill_refs {
390            crate::backend::agent_config::skills_to_agent_skills(&visible_skills, agent_id)
391        } else {
392            let mut merged = legacy_skills;
393            merged.extend(crate::backend::agent_config::skills_to_agent_skills(&visible_skills, agent_id));
394            merged
395        }
396    }
397
398    /// List every GLOBAL skill — Memory → Skills. Unlike
399    /// `skill_list`, this takes no `agent_id` and never includes an agent's
400    /// private skills; it backs the window-scoped `skill.catalog.*` MuxAPI
401    /// (no `check_s1`, so there is no agent context to scope by). Each row
402    /// carries `bound_count` — how many agents currently hold a
403    /// `db_agent_skills_ref` to it — per
404    /// SPEC_V1_MCP_SKILLS_PRIMITIVES_2026_06_30.md §8 ("used by N agents"),
405    /// tracked as gap #2 of #1960.
406    pub fn skill_list_global(&self, catalog: &Store) -> Result<Vec<SkillCatalogItem>, StoreError> {
407        Ok(self
408            .managed_list_global::<Skill>(catalog)?
409            .into_iter()
410            .map(|(skill, bound_count)| SkillCatalogItem { skill, bound_count })
411            .collect())
412    }
413
414    /// Catalog-tier sibling of `skill_list` (above) — same `bound_to_agent`
415    /// shape, but deliberately GLOBAL ROWS ONLY, unlike `skill_list`'s UNION
416    /// with `agent_id`'s own private skills. Backs `skill.catalog.list_for_agent`,
417    /// which — like every other `skill.catalog.*` command — has no
418    /// `check_s1`, so `agent_id` here is caller-supplied and unverified.
419    /// Returning private skill rows (whose `content`/`description`/`trigger`
420    /// can carry sensitive agent-authored material) for an arbitrary
421    /// caller-chosen `agent_id` would let any window connection read any
422    /// agent's private skills. Global rows carry nothing per-agent-secret —
423    /// they're already fully visible via `skill_list_global` (Memory →
424    /// Skills) — so exposing them alongside a caller-chosen agent's bind
425    /// status is safe. reagentx P0 on PR #2329.
426    pub fn skill_list_global_for_agent(&self, catalog: &Store, agent_id: &str) -> Result<Vec<SkillListItem>, StoreError> {
427        Ok(self
428            .managed_list_global_for_agent::<Skill>(catalog, agent_id)?
429            .into_iter()
430            .map(|(skill, bound_to_agent)| SkillListItem { skill, bound_to_agent })
431            .collect())
432    }
433
434    /// Get a standalone skill by id.
435    pub fn skill_get(&self, id: &str) -> Result<Option<Skill>, StoreError> {
436        self.managed_get::<Skill>(id)
437    }
438
439    /// Delete a standalone skill and purge its ref rows (both agent- and
440    /// bundle-level). Returns true if deleted.
441    pub fn skill_delete(&self, catalog: &Store, id: &str) -> Result<bool, StoreError> {
442        self.managed_delete::<Skill>(catalog, id)
443    }
444
445    /// Bind a skill to an agent (insert ref row). Idempotent — binding an
446    /// already-bound pair is a silent no-op success.
447    ///
448    /// Errors if `agent_id` isn't a LOCAL agent:
449    /// `db_agent_skills_ref.agent_id` has an ON-enforced FK to
450    /// `db_agents(id)` (store.rs; was `db_agent_definitions(id)` before
451    /// Phase 3c, #3088), but Memory's agent
452    /// picker (`ListAgentDefinitionsCommand` → `agent_def_list()`) also
453    /// lists cross-channel agents that only exist in another channel's
454    /// local database. Binding one of those would otherwise have the FK
455    /// silently swallow the `INSERT OR IGNORE` — indistinguishable, by
456    /// affected-row-count alone, from the equally-silent "already bound"
457    /// case — reporting success while creating nothing. reagentx P1, PR
458    /// #2315.
459    pub fn skill_bind(&self, catalog: &Store, agent_id: &str, skill_id: &str) -> Result<(), StoreError> {
460        self.managed_bind_agent::<Skill>(catalog, agent_id, skill_id)
461    }
462
463    /// Unbind a skill from an agent. Returns true if a row was removed.
464    pub fn skill_unbind(&self, agent_id: &str, skill_id: &str) -> Result<bool, StoreError> {
465        self.managed_unbind::<Skill>(Owner::Agent, agent_id, skill_id)
466    }
467
468    /// Atomically upsert a skill enforcing per-agent name uniqueness, and (when
469    /// `bind_new`) bind it to the agent — all in one transaction so concurrent
470    /// `skill.upsert` calls for the same name can't both pass a separate check
471    /// and insert duplicates (the check+write is not split across lock releases).
472    /// Returns `StoreError::Other` ("… already bound to this agent") if another
473    /// skill visible to the agent (bound or global) already uses the name.
474    pub fn skill_upsert_unique(
475        &self,
476        catalog: &Store,
477        agent_id: &str,
478        skill: &Skill,
479        bind_new: bool,
480    ) -> Result<(), StoreError> {
481        self.managed_upsert_unique(catalog, Owner::Agent, agent_id, skill, bind_new, None)
482    }
483
484    /// Atomically upsert a GLOBAL skill enforcing catalog-wide name
485    /// uniqueness (no `agent_id` — unlike `skill_upsert_unique`, this checks
486    /// for a duplicate name among *every* global row, not just those visible
487    /// to one agent). Same defense-in-depth as
488    /// `McpServer::mcp_server_upsert_unique_global` (reagent P1 on #1948) —
489    /// two same-named global skills would at minimum produce a confusing
490    /// duplicate bullet in the assembled CLAUDE.md skills index.
491    /// `skill.is_global` must already be `true`; caller's job.
492    pub fn skill_upsert_unique_global(&self, skill: &Skill) -> Result<(), StoreError> {
493        self.managed_upsert_unique_global(skill)
494    }
495
496    // ── Migration-only raw access (Phase 2 of SPEC_DURABLE_BINDINGS_2026_09_10.md) ──
497    // Bypasses the ordinary uniqueness/ownership validation on purpose: the
498    // carry-across migration has already made its own dedup decision (which
499    // survives, which gets a fresh id) before calling these, and re-running
500    // that validation here would both be redundant and, for a private row,
501    // actively wrong — `skill_upsert_unique_global` hardcodes `is_global = 1`,
502    // which a private row must not get. Not for use outside a migration.
503
504    /// Every row in this store's `db_skills`, global and private alike, with
505    /// no accessibility filtering. Ordinary app code never needs this — it is
506    /// always scoped by agent/bundle accessibility — which is exactly why no
507    /// such method existed before the carry-across migration needed one.
508    pub(crate) fn skill_list_all_raw(&self) -> Result<Vec<Skill>, StoreError> {
509        let conn = self.conn.lock().unwrap();
510        let mut stmt = conn.prepare(
511            "SELECT id, name, trigger, skill_type, description, content, is_global, created_at, updated_at
512             FROM db_skills",
513        )?;
514        let rows = stmt.query_map([], |row| {
515            Ok(Skill {
516                id: row.get(0)?,
517                name: row.get(1)?,
518                trigger: row.get(2)?,
519                skill_type: row.get(3)?,
520                description: row.get(4)?,
521                content: row.get(5)?,
522                is_global: row.get::<_, i64>(6)? != 0,
523                created_at: row.get(7)?,
524                updated_at: row.get(8)?,
525            })
526        })?;
527        let mut out = Vec::new();
528        for r in rows {
529            out.push(r?);
530        }
531        Ok(out)
532    }
533
534    /// Insert `skill` verbatim — including its `is_global` flag, which
535    /// `skill_upsert_unique_global` would otherwise force to `true`. `INSERT
536    /// OR IGNORE`: idempotent against a re-run migrating the same row twice
537    /// (by id), never overwrites an existing row.
538    /// Returns 1 if a row was actually inserted, 0 if `OR IGNORE` found one
539    /// already there under this id — the caller's signal for whether this
540    /// was a genuinely new carry or an idempotent re-run no-op.
541    pub(crate) fn skill_insert_raw(&self, skill: &Skill) -> Result<usize, StoreError> {
542        let conn = self.conn.lock().unwrap();
543        let rows = conn.execute(
544            "INSERT OR IGNORE INTO db_skills
545                (id, name, trigger, skill_type, description, content, is_global, created_at, updated_at)
546             VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)",
547            params![
548                skill.id,
549                skill.name,
550                skill.trigger,
551                skill.skill_type,
552                skill.description,
553                skill.content,
554                skill.is_global as i64,
555                skill.created_at,
556                skill.updated_at,
557            ],
558        )?;
559        Ok(rows)
560    }
561
562    /// The first existing GLOBAL row matching `name` alone, for the
563    /// migration's first-wins dedup among global skills that aren't a
564    /// recognized starter (those converge on `starter_skill_id` instead —
565    /// see `skill_seed::starter_skill_trigger_for_name`).
566    ///
567    /// Name-only, not `(name, skill_type)` — matching `idx_ids_skills_global_name`
568    /// (`IDENTITY_STORE_SCHEMA_VERSION` v8, `SPEC_DURABLE_BINDINGS_2026_09_10.md`
569    /// §5.4, Codex P1 on PR #3183): a `(name, skill_type)`-scoped re-query
570    /// would miss the actual winner whenever two channels' carries collide on
571    /// name but not type — the insert that failed did so because of the
572    /// name-only constraint, so the re-query must match the constraint that
573    /// actually fired, or it finds nothing, falls through to the
574    /// unrelated-PK-collision branch, and mints a fresh id whose insert
575    /// SILENTLY no-ops against the same name constraint (`INSERT OR IGNORE`),
576    /// leaving a local ref rewritten to an id the identity store never
577    /// actually holds — the skill vanishes the moment reads redirect there.
578    pub(crate) fn skill_find_global_by_name(&self, name: &str) -> Result<Option<Skill>, StoreError> {
579        let conn = self.conn.lock().unwrap();
580        let mut stmt = conn.prepare(
581            "SELECT id, name, trigger, skill_type, description, content, is_global, created_at, updated_at
582             FROM db_skills
583             WHERE is_global = 1 AND name = ?1
584             LIMIT 1",
585        )?;
586        let result = stmt.query_row(params![name], |row| {
587            Ok(Skill {
588                id: row.get(0)?,
589                name: row.get(1)?,
590                trigger: row.get(2)?,
591                skill_type: row.get(3)?,
592                description: row.get(4)?,
593                content: row.get(5)?,
594                is_global: row.get::<_, i64>(6)? != 0,
595                created_at: row.get(7)?,
596                updated_at: row.get(8)?,
597            })
598        });
599        match result {
600            Ok(s) => Ok(Some(s)),
601            Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None),
602            Err(e) => Err(e.into()),
603        }
604    }
605
606    /// Repoint every ref row (agent- and bundle-level) naming `old_id` to
607    /// `new_id` instead, on THIS store only. Called by the carry-across
608    /// migration after it decides a skill's row must live under a different
609    /// id in the identity store than it does locally — without this, the ref
610    /// row would name an id that no longer resolves anywhere once catalog
611    /// reads redirect to the identity store (Phase 2's whole point).
612    ///
613    /// INSERT-survivor-then-DELETE-old, not a blind `UPDATE`: the PK
614    /// `(agent_id, skill_id)`/`(bundle_id, skill_id)` collides whenever this
615    /// exact owner already independently held a ref to `new_id` too — not
616    /// vanishingly unlikely once two DIFFERENT catalog rows (not just two
617    /// copies of the same one) can converge onto one survivor, which is
618    /// exactly what `m0033_narrow_skill_global_uniqueness_index`'s dedup
619    /// does (an owner can easily have bound BOTH of two same-name,
620    /// different-`skill_type` skills before the collision was discovered).
621    /// A plain `UPDATE OR IGNORE` left that case's old-id row untouched
622    /// (documented at the time as an acceptable no-op — ReAgent P1, PR
623    /// #3181, round 2) — but a caller that then deletes the OLD catalog row
624    /// (as `m0033` does, right after this rewrite) turns that "no-op" into a
625    /// permanently dangling ref pointing at an id nothing resolves anymore,
626    /// even within the current channel (Codex P2, PR #3183). Insert-then-
627    /// delete instead: `INSERT OR IGNORE` the survivor binding for every
628    /// owner who held `old_id` (already-bound owners no-op harmlessly, same
629    /// as before), then unconditionally `DELETE` every `old_id` row — so the
630    /// owner ends up bound to the survivor either way, never left pointing
631    /// at a name that's about to stop existing. Returns the number of
632    /// `old_id` ref rows resolved (0 if none named `old_id`, distinct from
633    /// "the call itself no-oped").
634    pub(crate) fn skill_rewrite_ref_id(&self, old_id: &str, new_id: &str) -> Result<usize, StoreError> {
635        if old_id == new_id {
636            return Ok(0);
637        }
638        let conn = self.conn.lock().unwrap();
639        conn.execute(
640            "INSERT OR IGNORE INTO db_agent_skills_ref (agent_id, skill_id)
641                SELECT agent_id, ?2 FROM db_agent_skills_ref WHERE skill_id = ?1",
642            params![old_id, new_id],
643        )?;
644        let agent = conn.execute("DELETE FROM db_agent_skills_ref WHERE skill_id = ?1", params![old_id])?;
645        conn.execute(
646            "INSERT OR IGNORE INTO db_bundle_skills_ref (bundle_id, skill_id)
647                SELECT bundle_id, ?2 FROM db_bundle_skills_ref WHERE skill_id = ?1",
648            params![old_id, new_id],
649        )?;
650        let bundle = conn.execute("DELETE FROM db_bundle_skills_ref WHERE skill_id = ?1", params![old_id])?;
651        Ok(agent + bundle)
652    }
653
654    /// Return true if the given skill is accessible to the agent (global or bound).
655    /// Used for read and delete access checks.
656    pub fn skill_is_accessible_to(&self, catalog: &Store, agent_id: &str, skill_id: &str) -> Result<bool, StoreError> {
657        self.managed_is_accessible_to::<Skill>(catalog, Owner::Agent, agent_id, skill_id)
658    }
659
660    /// Return true if the agent has a direct ref binding to this skill (for delete/mutation).
661    /// Global skills are excluded — use the is_global guard separately.
662    pub fn skill_is_bound_to(&self, agent_id: &str, skill_id: &str) -> Result<bool, StoreError> {
663        self.managed_is_bound_to::<Skill>(Owner::Agent, agent_id, skill_id)
664    }
665
666    // ── Bundle-level references (composable model v2) ──────────────────
667    // docs/specs/SPEC_BUNDLE_AS_CONTAINER_V2_2026_08_17.md, GH issue #2024
668    // item 3. Mirror the agent-level methods above exactly, keyed by
669    // bundle_id via db_bundle_skills_ref instead of agent_id/db_agent_skills_ref.
670
671    /// Bundle-level sibling of `skill_list` — this bundle's own (referenced)
672    /// and global skills, each annotated with whether this specific bundle
673    /// holds the `db_bundle_skills_ref` row.
674    pub fn bundle_skill_list(&self, catalog: &Store, bundle_id: &str) -> Result<Vec<SkillBundleListItem>, StoreError> {
675        Ok(self
676            .managed_list::<Skill>(catalog, Owner::Bundle, bundle_id)?
677            .into_iter()
678            .map(|(skill, bound_to_bundle)| SkillBundleListItem { skill, bound_to_bundle })
679            .collect())
680    }
681
682    /// Bind a skill to a bundle (insert ref row). Idempotent.
683    ///
684    /// `id_store` (NOT `self`) is where bundle existence is checked: bundles
685    /// are written through the shared store, and `self`'s own `db_bundles`
686    /// copy is essentially always empty in production, so checking there made
687    /// "bundle not found" fire for every real bundle (PR #2639). `catalog` is where
688    /// `db_skills` is authoritative — see `managed_bind_bundle`'s doc
689    /// comment for why it now ALSO checks skill existence there (Part A of
690    /// SPEC_DURABLE_BINDINGS_2026_09_10.md §5.4 dropped this table's
691    /// `skill_id` FK).
692    pub fn bundle_skill_bind(&self, catalog: &Store, id_store: &Store, bundle_id: &str, skill_id: &str) -> Result<(), StoreError> {
693        self.managed_bind_bundle::<Skill>(catalog, id_store, bundle_id, skill_id)
694    }
695
696    /// Atomically create a NEW, PRIVATE (never global) skill scoped and
697    /// bound directly to a bundle, enforcing bundle-scoped name uniqueness
698    /// — the bundle-level analog of `skill_upsert_unique`. Binding can only
699    /// reference global skills (binding another entity's private one would
700    /// hand this bundle read access to it), and a global skill already
701    /// reaches every agent, so this is how a bundle gets a skill of its own
702    /// (PR #2639).
703    pub fn bundle_skill_upsert_unique(
704        &self,
705        catalog: &Store,
706        id_store: &Store,
707        bundle_id: &str,
708        skill: &Skill,
709        bind_new: bool,
710    ) -> Result<(), StoreError> {
711        self.managed_upsert_unique_for_bundle(catalog, id_store, bundle_id, skill, bind_new)
712    }
713
714    /// Unbind a skill from a bundle. Returns true if a row was removed.
715    pub fn bundle_skill_unbind(&self, bundle_id: &str, skill_id: &str) -> Result<bool, StoreError> {
716        self.managed_unbind::<Skill>(Owner::Bundle, bundle_id, skill_id)
717    }
718
719    /// Return true if the given skill is accessible to the bundle (global or
720    /// bundle-bound). Mirrors `skill_is_accessible_to`.
721    pub fn bundle_skill_is_accessible_to(&self, catalog: &Store, bundle_id: &str, skill_id: &str) -> Result<bool, StoreError> {
722        self.managed_is_accessible_to::<Skill>(catalog, Owner::Bundle, bundle_id, skill_id)
723    }
724
725    /// Return true if the bundle has a direct ref binding to this skill
726    /// (global excluded). Mirrors `skill_is_bound_to` — used for
727    /// edit/delete-ownership checks, as opposed to
728    /// `bundle_skill_is_accessible_to`'s broader read-access check.
729    pub fn bundle_skill_is_bound_to(&self, bundle_id: &str, skill_id: &str) -> Result<bool, StoreError> {
730        self.managed_is_bound_to::<Skill>(Owner::Bundle, bundle_id, skill_id)
731    }
732}
733
734#[cfg(test)]
735mod effective_skills_tests {
736    use super::*;
737    use crate::backend::storage::store::AgentDefinition;
738
739    fn make_store() -> Store {
740        Store::open_in_memory().unwrap()
741    }
742
743    fn insert_agent(store: &Store, id: &str) {
744        let mut def = AgentDefinition {
745            conversation_visibility: crate::backend::storage::agents::default_conversation_visibility(),
746            id: id.to_string(),
747            slug: String::new(),
748            name: "Test Agent".to_string(),
749            icon: String::new(),
750            provider: "claude".to_string(),
751            description: String::new(),
752            working_directory: String::new(),
753            shell: String::new(),
754            provider_flags: String::new(),
755            auto_start: 0,
756            restart_on_crash: 0,
757            idle_timeout_minutes: 0,
758            created_at: 1_700_000_000_000,
759            agent_type: "host".to_string(),
760            environment: String::new(),
761            agent_bus_id: String::new(),
762            is_seeded: 0,
763            accounts: String::new(),
764            parent_id: String::new(),
765            branch_label: String::new(),
766            updated_at: 1_700_000_000_000,
767            user_hidden: 0,
768            container_image: String::new(),
769            container_volumes: "[]".to_string(),
770            container_name: String::new(),
771            use_ambient_login: 0,
772            auto_continue_enabled: 0,
773            model_vendor_base_url: String::new(),
774            memory_id: String::new(),
775        };
776        store.agent_def_insert(&mut def).unwrap();
777    }
778
779    fn global_skill(id: &str, name: &str) -> Skill {
780        Skill {
781            id: id.to_string(),
782            name: name.to_string(),
783            trigger: String::new(),
784            skill_type: "prompt".to_string(),
785            description: format!("{name} description"),
786            content: format!("{name} content"),
787            is_global: true,
788            created_at: 1_700_000_000_000,
789            updated_at: 1_700_000_000_000,
790        }
791    }
792
793    #[test]
794    fn falls_back_to_legacy_plus_globals_when_agent_has_no_own_refs() {
795        let store = make_store();
796        insert_agent(&store, "agent-1");
797
798        store.agent_skill_insert(&AgentSkill {
799            id: "legacy-1".to_string(),
800            agent_id: "agent-1".to_string(),
801            name: "Legacy Skill".to_string(),
802            trigger: "legacy".to_string(),
803            skill_type: "prompt".to_string(),
804            description: "legacy description".to_string(),
805            content: "legacy content".to_string(),
806            created_at: 1_700_000_000_000,
807        }).unwrap();
808
809        store.skill_upsert_unique_global(&global_skill("global-1", "Global Skill")).unwrap();
810        // Not bound to agent-1 -- has_own_skill_refs must stay false.
811
812        let effective = store.effective_skills(&store, "agent-1");
813        let names: Vec<&str> = effective.iter().map(|s| s.name.as_str()).collect();
814        assert_eq!(names.len(), 2, "expected legacy + global, got: {names:?}");
815        assert!(names.contains(&"Legacy Skill"));
816        assert!(names.contains(&"Global Skill"));
817    }
818
819    #[test]
820    fn own_refs_take_over_entirely_and_discard_legacy_skills() {
821        let store = make_store();
822        insert_agent(&store, "agent-1");
823
824        store.agent_skill_insert(&AgentSkill {
825            id: "legacy-1".to_string(),
826            agent_id: "agent-1".to_string(),
827            name: "Legacy Skill".to_string(),
828            trigger: "legacy".to_string(),
829            skill_type: "prompt".to_string(),
830            description: "legacy description".to_string(),
831            content: "legacy content".to_string(),
832            created_at: 1_700_000_000_000,
833        }).unwrap();
834
835        store.skill_upsert_unique_global(&global_skill("global-1", "Global Skill")).unwrap();
836
837        // Bind a NEW (non-global) skill to agent-1 -- this must flip
838        // has_own_skill_refs to true and discard the legacy skill entirely.
839        let own_skill = Skill {
840            id: "own-1".to_string(),
841            name: "Own Skill".to_string(),
842            trigger: String::new(),
843            skill_type: "agent-skill".to_string(),
844            description: "own description".to_string(),
845            content: "own content".to_string(),
846            is_global: false,
847            created_at: 1_700_000_000_000,
848            updated_at: 1_700_000_000_000,
849        };
850        store.skill_upsert_unique(&store, "agent-1", &own_skill, true).unwrap();
851
852        let effective = store.effective_skills(&store, "agent-1");
853        let names: Vec<&str> = effective.iter().map(|s| s.name.as_str()).collect();
854        assert_eq!(names.len(), 2, "expected own + global, legacy discarded, got: {names:?}");
855        assert!(names.contains(&"Own Skill"));
856        assert!(names.contains(&"Global Skill"));
857        assert!(!names.contains(&"Legacy Skill"), "legacy skill must be discarded once own refs exist");
858    }
859
860    #[test]
861    fn agent_skill_format_survives_the_merge_with_correct_skill_type() {
862        // Regression for PR #2322: an agent-skill-format skill materialized
863        // via effective_skills must retain skill_type == "agent-skill" so
864        // build_config_files still branches it to SKILL.md, not a slash
865        // command.
866        let store = make_store();
867        insert_agent(&store, "agent-1");
868
869        let own_skill = Skill {
870            id: "own-1".to_string(),
871            name: "Deploy Checklist".to_string(),
872            trigger: String::new(),
873            skill_type: "agent-skill".to_string(),
874            description: "checklist".to_string(),
875            content: "1. test\n2. deploy".to_string(),
876            is_global: false,
877            created_at: 1_700_000_000_000,
878            updated_at: 1_700_000_000_000,
879        };
880        store.skill_upsert_unique(&store, "agent-1", &own_skill, true).unwrap();
881
882        let effective = store.effective_skills(&store, "agent-1");
883        assert_eq!(effective.len(), 1);
884        assert_eq!(effective[0].skill_type, "agent-skill");
885    }
886
887    fn insert_agent_with_bundle(store: &Store, id: &str, memory_id: &str) {
888        let mut def = AgentDefinition {
889            conversation_visibility: crate::backend::storage::agents::default_conversation_visibility(),
890            id: id.to_string(),
891            slug: String::new(),
892            name: "Test Agent".to_string(),
893            icon: String::new(),
894            provider: "claude".to_string(),
895            description: String::new(),
896            working_directory: String::new(),
897            shell: String::new(),
898            provider_flags: String::new(),
899            auto_start: 0,
900            restart_on_crash: 0,
901            idle_timeout_minutes: 0,
902            created_at: 1_700_000_000_000,
903            agent_type: "host".to_string(),
904            environment: String::new(),
905            agent_bus_id: String::new(),
906            is_seeded: 0,
907            accounts: String::new(),
908            parent_id: String::new(),
909            branch_label: String::new(),
910            updated_at: 1_700_000_000_000,
911            user_hidden: 0,
912            container_image: String::new(),
913            container_volumes: "[]".to_string(),
914            container_name: String::new(),
915            use_ambient_login: 0,
916            auto_continue_enabled: 0,
917            model_vendor_base_url: String::new(),
918            memory_id: memory_id.to_string(),
919        };
920        store.agent_def_insert(&mut def).unwrap();
921    }
922
923    fn insert_test_bundle(store: &Store, id: &str) {
924        store
925            .bundle_upsert(&crate::backend::storage::bundles::Bundle {
926                id: id.to_string(),
927                name: format!("Bundle {id}"),
928                description: String::new(),
929                is_blank: false,
930                is_global: false,
931                provider: "claude".to_string(),
932                model: "anthropic".to_string(),
933                instructions: String::new(),
934                instructions_by_provider: "{}".to_string(),
935                context_files: "[]".to_string(),
936                mcp_servers: "[]".to_string(),
937                skills: "[]".to_string(),
938                sort_order: 0,
939                created_at: 1_700_000_000_000,
940                updated_at: 1_700_000_000_000,
941                is_system: false,
942            })
943            .unwrap();
944    }
945
946    /// Composable model v2 (docs/specs/SPEC_BUNDLE_AS_CONTAINER_V2_2026_08_17.md,
947    /// GH issue #2024 item 3): a skill referenced by the agent's OWN bound
948    /// bundle (not the agent itself) must show up in effective_skills —
949    /// otherwise the new bundle-level ref tables are inert at launch, same
950    /// as the bug this whole feature exists to fix (see
951    /// `write_agent_config_files`'s doc comment on why this must be a
952    /// single source of truth shared with the RPC handler).
953    #[test]
954    fn effective_skills_includes_the_bound_bundles_referenced_skills() {
955        let store = make_store();
956        insert_test_bundle(&store, "bundle-1");
957        insert_agent_with_bundle(&store, "agent-1", "bundle-1");
958
959        let bundle_skill = Skill {
960            id: "bundle-skill-1".to_string(),
961            name: "Bundle Skill".to_string(),
962            trigger: String::new(),
963            skill_type: "prompt".to_string(),
964            description: String::new(),
965            content: "content".to_string(),
966            is_global: false,
967            created_at: 1_700_000_000_000,
968            updated_at: 1_700_000_000_000,
969        };
970        // Upserted under an unrelated agent context, then bound to the
971        // BUNDLE (not to agent-1 directly) — simulates a skill added via
972        // the bundle editor, not the agent's own Stash.
973        store.skill_upsert_unique(&store, "some-other-context", &bundle_skill, false).unwrap();
974        store.bundle_skill_bind(&store, &store, "bundle-1", "bundle-skill-1").unwrap();
975
976        let effective = store.effective_skills(&store, "agent-1");
977        let names: Vec<&str> = effective.iter().map(|s| s.name.as_str()).collect();
978        assert!(
979            names.contains(&"Bundle Skill"),
980            "a skill referenced only by the agent's bound bundle must still be effective: {names:?}"
981        );
982    }
983
984    /// End-to-end proof of the reagentx P1 fix, mirrored from
985    /// mcp_servers.rs's identical test: a skill created via
986    /// `bundle_skill_upsert_unique` reaches a spawned agent's effective
987    /// skill list.
988    #[test]
989    fn effective_skills_includes_a_bundle_owned_private_skill_created_via_upsert_unique() {
990        let store = make_store();
991        insert_test_bundle(&store, "bundle-1");
992        insert_agent_with_bundle(&store, "agent-1", "bundle-1");
993
994        let bundle_skill = Skill {
995            id: "bundle-own-skill".to_string(),
996            name: "Bundle-Owned Skill".to_string(),
997            trigger: String::new(),
998            skill_type: "prompt".to_string(),
999            description: String::new(),
1000            content: "content".to_string(),
1001            is_global: true, // ignored — upsert_unique forces is_global=false
1002            created_at: 1_700_000_000_000,
1003            updated_at: 1_700_000_000_000,
1004        };
1005        store.bundle_skill_upsert_unique(&store, &store, "bundle-1", &bundle_skill, true).unwrap();
1006
1007        let effective = store.effective_skills(&store, "agent-1");
1008        let names: Vec<&str> = effective.iter().map(|s| s.name.as_str()).collect();
1009        assert!(
1010            names.contains(&"Bundle-Owned Skill"),
1011            "a bundle-owned private skill created via bundle_skill_upsert_unique must reach the agent's effective skills: {names:?}"
1012        );
1013    }
1014
1015    /// reagentx P1 on PR #2639: `has_own_skill_refs` must reflect ONLY the
1016    /// agent's own direct binds, never a bundle-referenced skill — mandatory
1017    /// ABF means every agent has a bundle, so if a bundle's private skill
1018    /// alone could flip this flag, a legacy-only agent bound to ANY bundle
1019    /// that happens to reference one private skill would silently lose all
1020    /// of its `db_agent_skills` entries the moment someone edits that
1021    /// bundle, with no action taken on the agent itself. The bundle's
1022    /// referenced skill must still appear in the result (it's not excluded)
1023    /// — it just must not trigger the "own refs are authoritative, discard
1024    /// legacy" path on its own.
1025    #[test]
1026    fn a_bundle_referenced_private_skill_does_not_discard_the_agents_own_legacy_skills() {
1027        let store = make_store();
1028        insert_test_bundle(&store, "bundle-1");
1029        insert_agent_with_bundle(&store, "agent-1", "bundle-1");
1030
1031        store.agent_skill_insert(&AgentSkill {
1032            id: "legacy-1".to_string(),
1033            agent_id: "agent-1".to_string(),
1034            name: "Legacy Skill".to_string(),
1035            trigger: "legacy".to_string(),
1036            skill_type: "prompt".to_string(),
1037            description: "legacy description".to_string(),
1038            content: "legacy content".to_string(),
1039            created_at: 1_700_000_000_000,
1040        }).unwrap();
1041
1042        let bundle_private_skill = Skill {
1043            id: "bundle-private-1".to_string(),
1044            name: "Bundle Private Skill".to_string(),
1045            trigger: String::new(),
1046            skill_type: "prompt".to_string(),
1047            description: String::new(),
1048            content: "content".to_string(),
1049            is_global: false,
1050            created_at: 1_700_000_000_000,
1051            updated_at: 1_700_000_000_000,
1052        };
1053        store.skill_upsert_unique(&store, "some-other-context", &bundle_private_skill, false).unwrap();
1054        store.bundle_skill_bind(&store, &store, "bundle-1", "bundle-private-1").unwrap();
1055        // agent-1 itself has NO own db_skills ref — only its bundle does.
1056
1057        let effective = store.effective_skills(&store, "agent-1");
1058        let names: Vec<&str> = effective.iter().map(|s| s.name.as_str()).collect();
1059        assert!(
1060            names.contains(&"Legacy Skill"),
1061            "a bundle-referenced private skill must not silently discard the agent's own legacy skills: {names:?}"
1062        );
1063        assert!(
1064            names.contains(&"Bundle Private Skill"),
1065            "the bundle-referenced skill must still be included, just not treated as agent-authoritative: {names:?}"
1066        );
1067    }
1068
1069    #[test]
1070    fn effective_skills_does_not_duplicate_a_global_skill_visible_via_both_agent_and_bundle() {
1071        let store = make_store();
1072        insert_test_bundle(&store, "bundle-1");
1073        insert_agent_with_bundle(&store, "agent-1", "bundle-1");
1074        store.skill_upsert_unique_global(&global_skill("global-1", "Global Skill")).unwrap();
1075
1076        let effective = store.effective_skills(&store, "agent-1");
1077        let matches: Vec<_> = effective.iter().filter(|s| s.name == "Global Skill").collect();
1078        assert_eq!(matches.len(), 1, "a global skill visible via both the agent and its bundle must not be duplicated: {effective:?}");
1079    }
1080}
1081
1082#[cfg(test)]
1083mod bundle_ref_tests {
1084    use super::*;
1085    use crate::backend::storage::bundles::Bundle;
1086
1087    fn make_store() -> Store {
1088        Store::open_in_memory().unwrap()
1089    }
1090
1091    fn insert_bundle(store: &Store, id: &str) {
1092        store
1093            .bundle_upsert(&Bundle {
1094                id: id.to_string(),
1095                name: format!("Bundle {id}"),
1096                description: String::new(),
1097                is_blank: false,
1098                is_global: false,
1099                provider: "claude".to_string(),
1100                model: "anthropic".to_string(),
1101                instructions: String::new(),
1102                instructions_by_provider: "{}".to_string(),
1103                context_files: "[]".to_string(),
1104                mcp_servers: "[]".to_string(),
1105                skills: "[]".to_string(),
1106                sort_order: 0,
1107                created_at: 1_700_000_000_000,
1108                updated_at: 1_700_000_000_000,
1109                is_system: false,
1110            })
1111            .unwrap();
1112    }
1113
1114    fn skill(id: &str, name: &str, is_global: bool) -> Skill {
1115        Skill {
1116            id: id.to_string(),
1117            name: name.to_string(),
1118            trigger: String::new(),
1119            skill_type: "prompt".to_string(),
1120            description: String::new(),
1121            content: "content".to_string(),
1122            is_global,
1123            created_at: 1_700_000_000_000,
1124            updated_at: 1_700_000_000_000,
1125        }
1126    }
1127
1128    #[test]
1129    fn bind_makes_a_private_skill_visible_in_bundle_skill_list() {
1130        let store = make_store();
1131        insert_bundle(&store, "bundle-1");
1132        store.skill_upsert_unique_global(&skill("skill-global", "Global", true)).unwrap();
1133        store
1134            .skill_upsert_unique(&store, "some-other-agent-context", &skill("skill-private", "Private", false), false)
1135            .unwrap_or(());
1136
1137        let before = store.bundle_skill_list(&store, "bundle-1").unwrap();
1138        assert_eq!(before.len(), 1, "only the global skill should be visible before any bind: {before:?}");
1139
1140        store.bundle_skill_bind(&store, &store, "bundle-1", "skill-private").unwrap();
1141        let after = store.bundle_skill_list(&store, "bundle-1").unwrap();
1142        assert_eq!(after.len(), 2, "private skill must now be visible after binding: {after:?}");
1143        let private_item = after.iter().find(|i| i.skill.id == "skill-private").expect("private skill present");
1144        assert!(private_item.bound_to_bundle);
1145    }
1146
1147    #[test]
1148    fn bind_is_not_visible_to_a_different_bundle() {
1149        let store = make_store();
1150        insert_bundle(&store, "bundle-1");
1151        insert_bundle(&store, "bundle-2");
1152        store
1153            .skill_upsert_unique(&store, "ctx", &skill("skill-private", "Private", false), false)
1154            .unwrap_or(());
1155        store.bundle_skill_bind(&store, &store, "bundle-1", "skill-private").unwrap();
1156
1157        let bundle2_list = store.bundle_skill_list(&store, "bundle-2").unwrap();
1158        assert!(
1159            bundle2_list.is_empty(),
1160            "a private skill bound to bundle-1 must not leak into bundle-2's list: {bundle2_list:?}"
1161        );
1162    }
1163
1164    #[test]
1165    fn bind_errors_when_the_bundle_does_not_exist() {
1166        let store = make_store();
1167        store.skill_upsert_unique_global(&skill("skill-1", "S", true)).unwrap();
1168        let result = store.bundle_skill_bind(&store, &store, "no-such-bundle", "skill-1");
1169        assert!(result.is_err(), "binding to a nonexistent bundle must error, not silently no-op");
1170    }
1171
1172    #[test]
1173    fn unbind_removes_the_ref_and_is_idempotent() {
1174        let store = make_store();
1175        insert_bundle(&store, "bundle-1");
1176        store
1177            .skill_upsert_unique(&store, "ctx", &skill("skill-private", "Private", false), false)
1178            .unwrap_or(());
1179        store.bundle_skill_bind(&store, &store, "bundle-1", "skill-private").unwrap();
1180
1181        let removed = store.bundle_skill_unbind("bundle-1", "skill-private").unwrap();
1182        assert!(removed);
1183        assert!(store.bundle_skill_list(&store, "bundle-1").unwrap().is_empty());
1184
1185        let removed_again = store.bundle_skill_unbind("bundle-1", "skill-private").unwrap();
1186        assert!(!removed_again, "unbinding an already-unbound pair returns false, not an error");
1187    }
1188
1189    #[test]
1190    fn deleting_a_skill_purges_its_bundle_ref_too() {
1191        let store = make_store();
1192        insert_bundle(&store, "bundle-1");
1193        store
1194            .skill_upsert_unique(&store, "ctx", &skill("skill-private", "Private", false), false)
1195            .unwrap_or(());
1196        store.bundle_skill_bind(&store, &store, "bundle-1", "skill-private").unwrap();
1197        assert_eq!(store.bundle_skill_list(&store, "bundle-1").unwrap().len(), 1);
1198
1199        store.skill_delete(&store, "skill-private").unwrap();
1200        assert!(
1201            store.bundle_skill_list(&store, "bundle-1").unwrap().is_empty(),
1202            "the bundle ref row must be purged when the underlying skill is deleted"
1203        );
1204    }
1205
1206    /// reagentx P0 review on PR #2639: bundle existence must be checked
1207    /// against `id_store`, not `self` — see the identical test in
1208    /// mcp_servers.rs for the full reasoning, mirrored here for skills.
1209    #[test]
1210    fn bind_checks_bundle_existence_in_id_store_not_self() {
1211        let mstore = make_store();
1212        let id_store = make_store();
1213        insert_bundle(&id_store, "bundle-1");
1214        mstore.skill_upsert_unique_global(&skill("skill-1", "S", true)).unwrap();
1215
1216        let result = mstore.bundle_skill_bind(&mstore, &id_store, "bundle-1", "skill-1");
1217        assert!(result.is_ok(), "must check bundle existence against id_store, not self: {result:?}");
1218    }
1219
1220    #[test]
1221    fn bind_fails_when_bundle_exists_only_in_self_not_id_store() {
1222        let mstore = make_store();
1223        let id_store = make_store();
1224        insert_bundle(&mstore, "bundle-1");
1225        mstore.skill_upsert_unique_global(&skill("skill-1", "S", true)).unwrap();
1226
1227        let result = mstore.bundle_skill_bind(&mstore, &id_store, "bundle-1", "skill-1");
1228        assert!(
1229            result.is_err(),
1230            "a bundle only present in self's non-authoritative copy must not satisfy the id_store check: {result:?}"
1231        );
1232    }
1233
1234    /// reagentx P1 review on PR #2639: the missing "give this bundle its
1235    /// own skill" path — see the identical test in mcp_servers.rs.
1236    #[test]
1237    fn upsert_unique_creates_a_new_private_skill_bound_to_the_bundle() {
1238        let store = make_store();
1239        insert_bundle(&store, "bundle-1");
1240
1241        store
1242            .bundle_skill_upsert_unique(&store, &store, "bundle-1", &skill("new-skill", "Bundle-Only Skill", true), true)
1243            .unwrap();
1244
1245        let list = store.bundle_skill_list(&store, "bundle-1").unwrap();
1246        let item = list.iter().find(|i| i.skill.id == "new-skill").expect("newly created skill present");
1247        assert!(item.bound_to_bundle);
1248        assert!(!item.skill.is_global, "upsert_unique must force is_global=false regardless of the input struct");
1249    }
1250
1251    #[test]
1252    fn upsert_unique_rejects_a_duplicate_name_already_bound_to_the_bundle() {
1253        let store = make_store();
1254        insert_bundle(&store, "bundle-1");
1255        store.bundle_skill_upsert_unique(&store, &store, "bundle-1", &skill("skill-a", "Dup Name", true), true).unwrap();
1256
1257        let result = store.bundle_skill_upsert_unique(&store, &store, "bundle-1", &skill("skill-b", "Dup Name", true), true);
1258        assert!(result.is_err(), "a second skill with the same name bound to the same bundle must be rejected");
1259    }
1260
1261    #[test]
1262    fn is_accessible_to_reflects_global_and_bound_state() {
1263        let store = make_store();
1264        insert_bundle(&store, "bundle-1");
1265        store.skill_upsert_unique_global(&skill("skill-global", "Global", true)).unwrap();
1266        store
1267            .skill_upsert_unique(&store, "ctx", &skill("skill-private", "Private", false), false)
1268            .unwrap_or(());
1269
1270        assert!(store.bundle_skill_is_accessible_to(&store, "bundle-1", "skill-global").unwrap());
1271        assert!(!store.bundle_skill_is_accessible_to(&store, "bundle-1", "skill-private").unwrap());
1272        store.bundle_skill_bind(&store, &store, "bundle-1", "skill-private").unwrap();
1273        assert!(store.bundle_skill_is_accessible_to(&store, "bundle-1", "skill-private").unwrap());
1274    }
1275}
1276
1277#[cfg(test)]
1278mod rewrite_ref_id_and_delete_ordering_tests {
1279    use super::*;
1280    use crate::backend::storage::bundles::Bundle;
1281
1282    fn make_store() -> Store {
1283        Store::open_in_memory().unwrap()
1284    }
1285
1286    fn skill(id: &str, name: &str, is_global: bool) -> Skill {
1287        Skill {
1288            id: id.to_string(),
1289            name: name.to_string(),
1290            trigger: String::new(),
1291            skill_type: "prompt".to_string(),
1292            description: String::new(),
1293            content: "content".to_string(),
1294            is_global,
1295            created_at: 1_700_000_000_000,
1296            updated_at: 1_700_000_000_000,
1297        }
1298    }
1299
1300    fn insert_agent(store: &Store, id: &str) {
1301        store
1302            .conn()
1303            .lock()
1304            .unwrap()
1305            .execute("INSERT INTO db_agents (id, name, provider) VALUES (?1, 'A', 'claude')", params![id])
1306            .unwrap();
1307    }
1308
1309    fn insert_bundle(store: &Store, id: &str) {
1310        store
1311            .bundle_upsert(&Bundle {
1312                id: id.to_string(),
1313                name: format!("Bundle {id}"),
1314                description: String::new(),
1315                is_blank: false,
1316                is_global: false,
1317                provider: "claude".to_string(),
1318                model: "anthropic".to_string(),
1319                instructions: String::new(),
1320                instructions_by_provider: "{}".to_string(),
1321                context_files: "[]".to_string(),
1322                mcp_servers: "[]".to_string(),
1323                skills: "[]".to_string(),
1324                sort_order: 0,
1325                created_at: 1_700_000_000_000,
1326                updated_at: 1_700_000_000_000,
1327                is_system: false,
1328            })
1329            .unwrap();
1330    }
1331
1332    fn raw_bind_agent(store: &Store, agent_id: &str, skill_id: &str) {
1333        store
1334            .conn()
1335            .lock()
1336            .unwrap()
1337            .execute(
1338                "INSERT INTO db_agent_skills_ref (agent_id, skill_id) VALUES (?1, ?2)",
1339                params![agent_id, skill_id],
1340            )
1341            .unwrap();
1342    }
1343
1344    fn raw_bind_bundle(store: &Store, bundle_id: &str, skill_id: &str) {
1345        store
1346            .conn()
1347            .lock()
1348            .unwrap()
1349            .execute(
1350                "INSERT INTO db_bundle_skills_ref (bundle_id, skill_id) VALUES (?1, ?2)",
1351                params![bundle_id, skill_id],
1352            )
1353            .unwrap();
1354    }
1355
1356    /// Codex P2, PR #3183: when an owner already holds a ref to BOTH the
1357    /// loser id and the survivor id (e.g. an agent independently bound two
1358    /// same-name-different-type skills before `m0033` discovered the
1359    /// collision), the old `UPDATE OR IGNORE` left the loser's ref row
1360    /// untouched — a caller that then deletes the loser catalog row (as
1361    /// `m0033` does) turns that into a permanently dangling ref. The fixed
1362    /// insert-then-delete version must leave the owner bound to ONLY the
1363    /// survivor.
1364    #[test]
1365    fn rewrite_resolves_a_ref_row_even_when_the_owner_already_holds_the_survivor_too() {
1366        let store = make_store();
1367        insert_agent(&store, "agent-1");
1368        raw_bind_agent(&store, "agent-1", "loser-id");
1369        raw_bind_agent(&store, "agent-1", "survivor-id");
1370
1371        let rewritten = store.skill_rewrite_ref_id("loser-id", "survivor-id").unwrap();
1372        assert_eq!(rewritten, 1, "the loser ref row must be resolved (deleted, since the survivor was already bound)");
1373
1374        assert!(store.skill_is_bound_to("agent-1", "survivor-id").unwrap());
1375        assert!(
1376            !store.skill_is_bound_to("agent-1", "loser-id").unwrap(),
1377            "no ref may still name the loser id after the rewrite — that id is about to be deleted from the catalog"
1378        );
1379    }
1380
1381    /// Same scenario, bundle-level ref table.
1382    #[test]
1383    fn rewrite_resolves_a_bundle_ref_row_even_when_the_bundle_already_holds_the_survivor_too() {
1384        let store = make_store();
1385        insert_bundle(&store, "bundle-1");
1386        raw_bind_bundle(&store, "bundle-1", "loser-id");
1387        raw_bind_bundle(&store, "bundle-1", "survivor-id");
1388
1389        let rewritten = store.skill_rewrite_ref_id("loser-id", "survivor-id").unwrap();
1390        assert_eq!(rewritten, 1);
1391
1392        assert!(store.bundle_skill_is_bound_to("bundle-1", "survivor-id").unwrap());
1393        assert!(!store.bundle_skill_is_bound_to("bundle-1", "loser-id").unwrap());
1394    }
1395
1396    /// The ordinary case (no pre-existing survivor ref) must still behave
1397    /// like a plain rename.
1398    #[test]
1399    fn rewrite_behaves_like_a_plain_rename_when_the_owner_has_no_prior_survivor_ref() {
1400        let store = make_store();
1401        insert_agent(&store, "agent-1");
1402        raw_bind_agent(&store, "agent-1", "old-id");
1403
1404        let rewritten = store.skill_rewrite_ref_id("old-id", "new-id").unwrap();
1405        assert_eq!(rewritten, 1);
1406        assert!(store.skill_is_bound_to("agent-1", "new-id").unwrap());
1407        assert!(!store.skill_is_bound_to("agent-1", "old-id").unwrap());
1408    }
1409
1410    /// Codex P1, PR #3183: `managed_delete` (via `skill_delete`) must delete
1411    /// the catalog row BEFORE purging refs, not after — so a catalog-delete
1412    /// failure never leaves a private row's refs already gone (which would
1413    /// make it permanently inaccessible and un-retryable, since the ordinary
1414    /// delete RPC's own pre-check depends on the very ref this would have
1415    /// already destroyed). Forces a real SQL error the same way
1416    /// `managed_upsert_unique_compensates_by_deleting_the_catalog_row_when_the_bind_fails`
1417    /// does (`managed.rs`'s own test) — dropping the catalog table, since
1418    /// `DELETE ... WHERE id = ?` on a missing row is not itself an error.
1419    #[test]
1420    fn skill_delete_leaves_refs_intact_when_the_catalog_delete_itself_fails() {
1421        let store = make_store();
1422        insert_agent(&store, "agent-1");
1423        store
1424            .skill_upsert_unique(&store, "agent-1", &skill("private-1", "Deploy", false), true)
1425            .unwrap();
1426        assert!(store.skill_is_bound_to("agent-1", "private-1").unwrap());
1427
1428        store.conn().lock().unwrap().execute_batch("DROP TABLE db_skills;").unwrap();
1429
1430        let result = store.skill_delete(&store, "private-1");
1431        assert!(result.is_err(), "the catalog delete must fail now that its table is gone");
1432
1433        // Re-create db_skills (empty) so the ref-table read below works —
1434        // the point being verified is that the ref row was NEVER purged in
1435        // the first place, i.e. the failure above happened before any ref
1436        // mutation, not that the schema self-heals.
1437        store
1438            .conn()
1439            .lock()
1440            .unwrap()
1441            .execute_batch(
1442                "CREATE TABLE db_skills (
1443                    id TEXT PRIMARY KEY, name TEXT NOT NULL, trigger TEXT NOT NULL DEFAULT '',
1444                    skill_type TEXT NOT NULL DEFAULT 'prompt', description TEXT NOT NULL DEFAULT '',
1445                    content TEXT NOT NULL DEFAULT '', is_global INTEGER NOT NULL DEFAULT 0,
1446                    created_at INTEGER NOT NULL DEFAULT 0, updated_at INTEGER NOT NULL DEFAULT 0
1447                );",
1448            )
1449            .unwrap();
1450        assert!(
1451            store.skill_is_bound_to("agent-1", "private-1").unwrap(),
1452            "the ref row must still exist — a failed catalog delete must never strand a row with its bindings already purged"
1453        );
1454    }
1455}
agentmux_srv/backend/storage/skills.rs

agentmux_srv/backend/storage/
skills.rs
