diff --git a/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx b/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx index b9926131..3dfb040c 100644 --- a/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx +++ b/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx @@ -58,8 +58,11 @@ export function FeatureSetPanel({ featureSet, spaceId, onClose, onDelete, onUpda // Both FS types are member-driven now. const isConfigurable = true; - // The auto-seeded "Starter" FS is treated identically to a Custom one - // — the type tag is a UI hint, not a routing flag. + // The auto-seeded "Starter" FS has editable membership like a Custom one + // (change which tools it includes, or empty it). What's locked is its + // identity + lifecycle: it's the default fallback for unmapped folders, so + // its name is fixed (the backend ignores name changes on builtin rows) and + // it can't be deleted — the Delete action below is gated to Custom sets. const isStarter = isStarterFeatureSet(featureSet); const isCustom = featureSet.feature_set_type === 'custom'; @@ -257,7 +260,7 @@ export function FeatureSetPanel({ featureSet, spaceId, onClose, onDelete, onUpda
- Starter FeatureSet: auto-created with this Space. It's an ordinary FeatureSet — edit, rename, or delete it freely. No special routing role: Workspace bindings and per-client grants pick FeatureSets explicitly. + Starter FeatureSet: auto-created with this Space and used as the default for folders you haven't explicitly mapped (and rootless sessions). Edit which tools it includes (or empty it) to change what they get. Its name is fixed and it can't be deleted, since the fallback always needs a stable target.
diff --git a/apps/desktop/src/features/featuresets/FeatureSetsPage.tsx b/apps/desktop/src/features/featuresets/FeatureSetsPage.tsx index e0c3b1c2..4fbaeec1 100644 --- a/apps/desktop/src/features/featuresets/FeatureSetsPage.tsx +++ b/apps/desktop/src/features/featuresets/FeatureSetsPage.tsx @@ -199,9 +199,9 @@ export function FeatureSetsPage() { }) .sort((a, b) => { // Starter FS first (pinned to top — operator usually wants the - // auto-seeded one near the top so they can edit / delete it - // first), then Custom sets alphabetically. The 'default' key is - // kept so a stale row read pre-migration still sorts correctly. + // auto-seeded default set near the top so they can edit it first), + // then Custom sets alphabetically. The 'default' key is kept so a + // stale row read pre-migration still sorts correctly. const order: Record = { starter: 0, default: 0, @@ -375,7 +375,7 @@ export function FeatureSetsPage() { {isStarter && (
diff --git a/apps/desktop/src/features/workspaces/WorkspaceBindingSheet.tsx b/apps/desktop/src/features/workspaces/WorkspaceBindingSheet.tsx index 495de614..8a1d5dbb 100644 --- a/apps/desktop/src/features/workspaces/WorkspaceBindingSheet.tsx +++ b/apps/desktop/src/features/workspaces/WorkspaceBindingSheet.tsx @@ -1,15 +1,17 @@ /** * Workspace Binding Sheet * - * Fires when a connected client session resolves via source=Default for a - * workspace root that has no binding yet. The user picks a Space + a - * FeatureSet in that space, and we write a WorkspaceBinding locking both. + * Fires when a connected client session reports a workspace root that has no + * explicit binding yet — the folder is already working via the default + * Starter set, and this sheet offers to map it to something else. The user + * picks a Space + a FeatureSet, and we write a WorkspaceBinding locking both. * * • Space picker — defaults to the caller's current space, can be changed. - * • FS picker — always includes a "space default" option (follow - * whichever FS is active for the selected Space) plus - * every Default + Custom set in that space. - * • Dismiss — nothing written, ask again next session. + * • FS picker — pre-selects the Space's Starter (the active default), + * plus every Starter + Custom set in that space. + * • Modify — writes the binding for the picked Space + FS. + * • Close — nothing written; the folder keeps the default Starter + * set, and the sheet re-offers next session. * * Committing the binding emits `WorkspaceBindingChanged` on the backend, * which triggers `notifications/tools/list_changed` — the client re-fetches @@ -61,8 +63,8 @@ export function WorkspaceBindingSheet() { // Only dedupe the currently-open sheet against itself — if one is already // showing, swallow a second emit for the same session. We deliberately // don't dedupe across sessions / reconnects: the backend only emits when - // `source=Default` (i.e. no binding exists), and reconnecting a client - // is a normal signal that the user may want to configure the folder. + // the folder has no explicit binding (it's on the default Starter set), and + // reconnecting a client is a normal signal that the user may want to map it. // Persisting the dismissal in a ref would black-hole later attempts // until the next app restart, which is how this bug surfaced before. const currentSessionRef = useRef(null); @@ -74,7 +76,7 @@ export function WorkspaceBindingSheet() { (event) => { // Swallow only while a sheet is already showing — the user is // mid-decision, a second emit would stack a new sheet on top. Once - // the current sheet closes (Save or Not now), the next emit from + // the current sheet closes (Modify or Close), the next emit from // any fresh session on an unbound root opens the sheet again. if (currentSessionRef.current !== null) return; const p = event.payload; @@ -194,12 +196,13 @@ export function WorkspaceBindingSheet() { New workspace detected

- Which tools should this folder get? + This folder is using your Starter set

You just opened this folder in a connected app. It's already - using your default Starter tools — pick a Space and feature set to - give it a specific set instead, or keep the default. + configured with your default Starter tools. Pick a different Space + or feature set below to change what it gets, or close to keep the + Starter.

@@ -282,9 +285,10 @@ export function WorkspaceBindingSheet() { {error}
)} - {/* "Not now" auto-sizes to its label; the primary action takes - the rest of the row. Equal flex-1 columns wrapped the longer - "Remember for this folder" text onto two lines. */} + {/* "Close" keeps the default Starter set (nothing written); the + primary "Modify" applies the picked Space + feature set as an + explicit mapping. Labels deliberately avoid "Not now", which read + as "this folder is unmapped / has no tools" — it isn't. */}

diff --git a/apps/desktop/src/lib/api/featureSets.ts b/apps/desktop/src/lib/api/featureSets.ts index 64bf8e2b..94acfea0 100644 --- a/apps/desktop/src/lib/api/featureSets.ts +++ b/apps/desktop/src/lib/api/featureSets.ts @@ -7,11 +7,13 @@ import { invoke } from '@tauri-apps/api/core'; * - `custom`: user-defined. */ /** - * `starter` is the auto-seeded FS that comes with each Space. It has no - * special routing role under resolver v3 — bindings and per-client grants - * pick FeatureSets explicitly. The legacy `'default'` value is accepted on - * read because migration 013 rewrites stored rows lazily and a stale fetch - * could still surface it; new writes use `'starter'`. + * `starter` is the auto-seeded FS that comes with each Space. It's the + * default fallback the resolver routes unmapped folders / rootless sessions + * to, so it's builtin: its members are editable (change which tools it + * includes, or empty it), but it can't be renamed or deleted. The legacy + * `'default'` value is accepted on read because migration 013 rewrites stored + * rows lazily and a stale fetch could still surface it; new writes use + * `'starter'`. */ export type FeatureSetType = 'starter' | 'default' | 'custom'; diff --git a/crates/mcpmux-core/src/domain/feature_set.rs b/crates/mcpmux-core/src/domain/feature_set.rs index 53bd950c..1a7659aa 100644 --- a/crates/mcpmux-core/src/domain/feature_set.rs +++ b/crates/mcpmux-core/src/domain/feature_set.rs @@ -1,12 +1,15 @@ //! FeatureSet entity - permission bundles for tools/prompts/resources //! //! Each FeatureSet is scoped to a space and is one of two types: -//! - **Starter**: auto-created with the Space as a convenient starting -//! point. Has no special routing role under the resolver — bindings and -//! per-client grants pick FeatureSets explicitly. Pre-resolver-v3 this -//! was the "Default" type and acted as the implicit fallback; that -//! behaviour is gone, and the rename reflects the type's actual job -//! (a seed you can rename, edit, or delete freely). +//! - **Starter**: auto-created with the Space. It's the **default fallback** +//! for folders that aren't explicitly mapped (and for rootless/unknown +//! sessions) — the resolver routes them here instead of denying. Its +//! membership is editable (change which tools it includes, or empty it to +//! grant nothing by default), but its **identity is locked**: builtin, so +//! not renamable and not deletable since the fallback always needs a stable +//! target. (Pre-resolver-v3 it was the "Default" type and also acted as the +//! implicit fallback; that role is back after a stint where it was a no-op +//! seed.) //! - **Custom**: any other operator-defined FeatureSet. use chrono::{DateTime, Utc}; @@ -16,15 +19,18 @@ use uuid::Uuid; /// The type of a FeatureSet. /// /// `Starter` is auto-created once per Space; `Custom` covers everything -/// else. Routing-wise the two are interchangeable — the type tag is -/// purely a UI affordance ("this one came pre-seeded with the Space"). +/// else. The type tag carries routing weight: the Starter is the default +/// fallback the resolver routes unmapped/rootless sessions to, and it's +/// builtin (not deletable). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] #[derive(Default)] pub enum FeatureSetType { - /// Auto-created with the Space. Editable / deletable like any other - /// FS — no special routing semantics. Was historically called - /// `Default` (DB column value carried over via migration 013). + /// Auto-created with the Space and used as the **default fallback** for + /// unmapped folders / rootless sessions. Its members are editable, but its + /// identity is locked: builtin — not renamable and not deletable. Was + /// historically called `Default` (DB column value carried over via + /// migration 013). Starter, /// Any operator-defined FeatureSet. #[default] @@ -237,9 +243,10 @@ impl FeatureSet { id: format!("fs_default_{}", space_id), name: "Starter".to_string(), description: Some( - "Auto-created with this Space. Edit, rename, or delete freely \ - — bindings and per-client grants pick FeatureSets explicitly, \ - so this one has no special routing role." + "Auto-created with this Space — the default set for folders \ + you haven't explicitly mapped. Edit which tools it includes \ + to change what they get. Its name is fixed and it can't be \ + deleted." .to_string(), ), icon: Some("⭐".to_string()), diff --git a/crates/mcpmux-storage/src/database.rs b/crates/mcpmux-storage/src/database.rs index ca3b9271..e6336d5a 100644 --- a/crates/mcpmux-storage/src/database.rs +++ b/crates/mcpmux-storage/src/database.rs @@ -118,6 +118,11 @@ const MIGRATIONS: &[Migration] = &[ name: "purge_orphaned_feature_set_members", sql: include_str!("migrations/017_purge_orphaned_feature_set_members.sql"), }, + Migration { + version: 18, + name: "starter_is_default_fallback_copy", + sql: include_str!("migrations/018_starter_is_default_fallback_copy.sql"), + }, ]; /// SQLite database wrapper. diff --git a/crates/mcpmux-storage/src/migrations/018_starter_is_default_fallback_copy.sql b/crates/mcpmux-storage/src/migrations/018_starter_is_default_fallback_copy.sql new file mode 100644 index 00000000..5b5e6c45 --- /dev/null +++ b/crates/mcpmux-storage/src/migrations/018_starter_is_default_fallback_copy.sql @@ -0,0 +1,18 @@ +-- Migration 018: the Starter FS is the default fallback again (and can't be +-- deleted). +-- +-- After the "default FeatureSet for unmapped roots" change, an unmapped folder +-- (plus rootless / unknown sessions) falls back to the default Space's Starter +-- FS instead of being denied. That makes the Starter load-bearing: it's the +-- default toolset for anything not explicitly mapped, and it is no longer +-- deletable. Migrations 014/015 (and the seed paths) set a description that +-- now lies — "no special routing role; delete freely" — so rewrite it. +-- +-- Safety: only rewrite rows that STILL match the exact 014/015 seed text, so +-- an operator who customized the copy keeps their change. + +UPDATE feature_sets + SET description = 'Auto-created with this Space — the default set for folders you haven''t explicitly mapped. Edit which tools it includes to change what they get. Its name is fixed and it can''t be deleted.' + WHERE is_builtin = 1 + AND feature_set_type IN ('starter', 'default') + AND description = 'Auto-created with this Space. Edit, rename, or delete freely — bindings and per-client grants pick FeatureSets explicitly, so this one has no special routing role.'; diff --git a/crates/mcpmux-storage/src/repositories/feature_set_repository.rs b/crates/mcpmux-storage/src/repositories/feature_set_repository.rs index aaf2b436..580a0f54 100644 --- a/crates/mcpmux-storage/src/repositories/feature_set_repository.rs +++ b/crates/mcpmux-storage/src/repositories/feature_set_repository.rs @@ -230,9 +230,20 @@ impl FeatureSetRepository for SqliteFeatureSetRepository { let db = self.db.lock().await; let conn = db.connection(); + // Builtin FeatureSets (the auto-seeded Starter) are the default + // fallback for unmapped folders, so their identity is fixed: name, + // description, and icon are preserved here regardless of the incoming + // values — only the MEMBERS (replaced below) and `updated_at` are + // editable. The DB's own `is_builtin` flag governs (not the caller's + // struct), so the lock holds for every caller, including the + // member-set command that routes through update(). Custom sets update + // normally. let rows_affected = conn.execute( - "UPDATE feature_sets - SET name = ?2, description = ?3, icon = ?4, updated_at = ?5 + "UPDATE feature_sets + SET name = CASE WHEN is_builtin = 1 THEN name ELSE ?2 END, + description = CASE WHEN is_builtin = 1 THEN description ELSE ?3 END, + icon = CASE WHEN is_builtin = 1 THEN icon ELSE ?4 END, + updated_at = ?5 WHERE id = ?1 AND is_deleted = 0", params![ feature_set.id, diff --git a/crates/mcpmux-storage/src/repositories/space_repository.rs b/crates/mcpmux-storage/src/repositories/space_repository.rs index d17c9060..e89d90c0 100644 --- a/crates/mcpmux-storage/src/repositories/space_repository.rs +++ b/crates/mcpmux-storage/src/repositories/space_repository.rs @@ -109,11 +109,13 @@ impl SpaceRepository for SqliteSpaceRepository { // Auto-seed the builtin "Starter" FeatureSet for this Space — a // ready-to-use starting point. The id prefix `fs_default_` // is preserved for FK-stability across the rename (migration 013). - // No special routing role under resolver v3 — bindings and per- - // client grants pick FeatureSets explicitly. + // The Starter is the default fallback for folders that aren't + // explicitly mapped (and rootless/unknown sessions), so it's + // load-bearing and builtin: members are editable, but it can't be + // renamed or deleted. conn.execute( "INSERT OR IGNORE INTO feature_sets (id, name, description, icon, space_id, feature_set_type, is_builtin, created_at, updated_at) - VALUES (?1, 'Starter', 'Auto-created with this Space. Edit, rename, or delete freely — bindings and per-client grants pick FeatureSets explicitly, so this one has no special routing role.', '⭐', ?2, 'starter', 1, ?3, ?3)", + VALUES (?1, 'Starter', 'Auto-created with this Space — the default set for folders you haven''t explicitly mapped. Edit which tools it includes to change what they get. Its name is fixed and it can''t be deleted.', '⭐', ?2, 'starter', 1, ?3, ?3)", params![ format!("fs_default_{}", space_id), space_id, diff --git a/tests/rust/tests/database/feature_set.rs b/tests/rust/tests/database/feature_set.rs index c160e881..1750a8fe 100644 --- a/tests/rust/tests/database/feature_set.rs +++ b/tests/rust/tests/database/feature_set.rs @@ -3,7 +3,7 @@ //! Tests for feature set CRUD, builtin types (All, Default, ServerAll), //! and feature member composition. -use mcpmux_core::domain::{FeatureSetType, MemberMode}; +use mcpmux_core::domain::{FeatureSetMember, FeatureSetType, MemberMode}; use mcpmux_core::repository::{FeatureSetRepository, SpaceRepository}; use mcpmux_storage::{SqliteFeatureSetRepository, SqliteSpaceRepository}; use std::sync::Arc; @@ -112,6 +112,55 @@ async fn test_update_feature_set() { assert_eq!(loaded.description, Some("New description".to_string())); } +/// The builtin Starter's identity is locked: `update()` must ignore name / +/// description / icon changes on a builtin row (it's the default fallback, not +/// renamable) while still applying MEMBER changes — that's how the member +/// editor (which routes through `update()`) keeps working for the Starter. +#[tokio::test] +async fn test_update_locks_builtin_identity_but_allows_member_edits() { + let test_db = TestDatabase::new(); + let db = Arc::new(Mutex::new(test_db.db)); + let feature_repo = SqliteFeatureSetRepository::new(Arc::clone(&db)); + let space_repo = SqliteSpaceRepository::new(db); + + let space = fixtures::test_space("Test Space"); + SpaceRepository::create(&space_repo, &space).await.unwrap(); + + // Auto-seed the builtin Starter for this Space. + FeatureSetRepository::ensure_builtin_for_space(&feature_repo, &space.id.to_string()) + .await + .unwrap(); + let mut starter = + FeatureSetRepository::get_starter_for_space(&feature_repo, &space.id.to_string()) + .await + .unwrap() + .expect("starter seeded"); + let original_name = starter.name.clone(); + let original_desc = starter.description.clone(); + + // Attempt to rename + re-describe the builtin AND add a member at once. + starter.name = "Hacked Name".to_string(); + starter.description = Some("hacked description".to_string()); + starter.members = vec![FeatureSetMember::include_feature(&starter.id, "feat-x")]; + FeatureSetRepository::update(&feature_repo, &starter) + .await + .expect("update should succeed"); + + let loaded = FeatureSetRepository::get_with_members(&feature_repo, &starter.id) + .await + .unwrap() + .unwrap(); + // Identity preserved... + assert_eq!(loaded.name, original_name, "builtin name must not change"); + assert_eq!( + loaded.description, original_desc, + "builtin description must not change" + ); + // ...but the member edit went through. + assert_eq!(loaded.members.len(), 1, "members must stay editable"); + assert_eq!(loaded.members[0].member_id, "feat-x"); +} + #[tokio::test] async fn test_delete_feature_set() { let test_db = TestDatabase::new(); diff --git a/tests/rust/tests/database/migrations.rs b/tests/rust/tests/database/migrations.rs index 747a526a..e8ea4aac 100644 --- a/tests/rust/tests/database/migrations.rs +++ b/tests/rust/tests/database/migrations.rs @@ -95,3 +95,59 @@ fn test_017_purges_orphaned_feature_set_members() { "only the still-resolvable feature member should survive the purge" ); } + +/// Migration 018 rewrites the auto-seeded Starter FS's now-stale "no special +/// routing role; delete freely" description (the Starter is the default +/// fallback again and can't be deleted), but only on rows that STILL match the +/// exact 014/015 seed text — a Starter an operator re-described keeps its copy. +#[test] +fn test_018_rewrites_stale_starter_description_only() { + const STALE: &str = "Auto-created with this Space. Edit, rename, or delete freely — bindings and per-client grants pick FeatureSets explicitly, so this one has no special routing role."; + const FRESH: &str = "Auto-created with this Space — the default set for folders you haven't explicitly mapped. Edit which tools it includes to change what they get. Its name is fixed and it can't be deleted."; + + let test_db = TestDatabase::new(); + let conn = test_db.db.connection(); + + // Two builtin Starter rows: one still carrying the stale 014/015 copy + // (should be rewritten), one an operator customized (must be left alone). + conn.execute_batch( + "INSERT INTO spaces (id,name,icon,description,is_default,sort_order,created_at,updated_at) + VALUES ('s-stale','Stale','x','',0,0,datetime('now'),datetime('now')), + ('s-cust','Custom','x','',0,0,datetime('now'),datetime('now')); + INSERT INTO feature_sets + (id,name,description,icon,space_id,feature_set_type,server_id,is_builtin,is_deleted,created_at,updated_at) + VALUES + ('fs_default_s-stale','Starter', + 'Auto-created with this Space. Edit, rename, or delete freely — bindings and per-client grants pick FeatureSets explicitly, so this one has no special routing role.', + '⭐','s-stale','starter',NULL,1,0,datetime('now'),datetime('now')), + ('fs_default_s-cust','Starter','My own words','⭐','s-cust','starter',NULL,1,0,datetime('now'),datetime('now'));", + ) + .expect("seed failed"); + + // Re-apply migration 018 (idempotent) to exercise the rewrite. + conn.execute_batch(include_str!( + "../../../../crates/mcpmux-storage/src/migrations/018_starter_is_default_fallback_copy.sql" + )) + .expect("migration 018 failed"); + + let desc = |id: &str| -> String { + conn.query_row( + "SELECT description FROM feature_sets WHERE id = ?", + [id], + |r| r.get::<_, String>(0), + ) + .unwrap() + }; + + assert_eq!( + desc("fs_default_s-stale"), + FRESH, + "stale copy should be rewritten" + ); + assert_ne!(STALE, FRESH); // guard against the strings drifting equal + assert_eq!( + desc("fs_default_s-cust"), + "My own words", + "operator-customized copy must be preserved" + ); +}