From e46783e5c96eb8c857ffb01d9ddc97a678107884 Mon Sep 17 00:00:00 2001 From: cyberpapi Date: Wed, 7 Oct 2026 14:11:03 -0400 Subject: [PATCH] fix: server icons survive a disk cleaner, and an allow list keeps Plug's search tool Found icons are kept beside the chosen ones, and a cancelled search is not remembered as no icon. From an audit of the changes since 0.10.0: an allow list no longer hides Plug's own search tool, removing a server takes it off allow lists, a replayed renewal token ends the sign-in even when the save fails, and a task called by upstream name records its server. Duplicate helpers and two needless process spawns are gone. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 21 +++++++++ PlugApp/PlugApp/Core/AppIcons.swift | 22 ++++++--- .../PlugAppTests/SettingsSurfaceTests.swift | 17 +++++++ plug-core/src/downstream_oauth/mod.rs | 4 +- plug-core/src/events/access.rs | 2 +- plug-core/src/operator.rs | 30 +++++++++++++ plug-core/src/proxy/catalog.rs | 6 +-- plug-core/src/proxy/mod.rs | 45 +++++++++---------- plug-core/src/proxy/tasks.rs | 9 +++- plug-core/src/proxy/tests.rs | 26 +++++++++++ plug-core/src/session/stateful.rs | 13 ++++-- plug/src/client_host.rs | 3 +- plug/src/commands/clients.rs | 2 +- plug/src/daemon/menu_bar.rs | 22 +++++---- plug/src/daemon/mod.rs | 4 +- 15 files changed, 178 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c89af9b..cb0aefdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,27 @@ All notable changes to plug are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- Server icons no longer turn into a letter in a colored square after a disk + cleaner runs. Found icons are kept with Plug's own files, not in the + system cache a cleaner empties, and a search cut short while Plug starts + is no longer remembered as "this site has no icon" for a day. +- A client with an allow list keeps Plug's own search tool, so it can still + find the tools it is allowed when Plug serves tools by search. +- Removing a server takes it off every client's allow list, so a server + added later under the same name is not handed to those clients unasked. +- A sign-in ended because its renewal token was used twice stays ended even + when Plug could not save the change. +- An allowed tool Plug no longer lists is shown as `server/tool`, the form + `plug clients only --remove --tool` takes. +- A watch on a tool Plug does not list right now is hidden from a client + whose allow list does not name that tool. +- The large result of a task called by its server's own tool name is no + longer kept from the client that ran it. + ## [0.11.0] - 2026-10-07 ### Added diff --git a/PlugApp/PlugApp/Core/AppIcons.swift b/PlugApp/PlugApp/Core/AppIcons.swift index b1024ded..ffbfc580 100644 --- a/PlugApp/PlugApp/Core/AppIcons.swift +++ b/PlugApp/PlugApp/Core/AppIcons.swift @@ -440,10 +440,9 @@ final class IconStore { init(chosenDirectory: URL? = nil, cacheDirectory: URL? = nil) { self.chosenDirectory = chosenDirectory ?? PlugIPCClient.defaultSocketURL.deletingLastPathComponent().appending(path: "icons") - self.cacheDirectory = cacheDirectory - ?? FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask)[0] - .appending(path: Bundle.main.bundleIdentifier ?? "com.cyberpapiii.plug") - .appending(path: "icons") + // Beside the chosen icons, not under Caches: a disk cleaner empties + // Caches, and every icon was then looked for again the next day. + self.cacheDirectory = cacheDirectory ?? self.chosenDirectory.appending(path: "found") let files = (try? FileManager.default.contentsOfDirectory(atPath: self.chosenDirectory.path)) ?? [] for file in files where file.hasSuffix(".png") { chosen[String(file.dropLast(4))] = NSImage(contentsOf: self.chosenDirectory.appending(path: file)) @@ -573,7 +572,10 @@ final class IconStore { switch kept(key) { case let .found(image): advertised[source.name] = image case .missing: break - case .unknown: advertised[source.name] = keep(await SiteIcon.image(at: url), as: key) + case .unknown: + let data = await SiteIcon.image(at: url) + guard !stopped(asking: [key]) else { return } + advertised[source.name] = keep(data, as: key) } } } @@ -610,11 +612,21 @@ final class IconStore { for await (index, data) in group { answers[index] = data } return answers } + guard !stopped(asking: asking.map(\.key)) else { return } for (index, want) in asking.enumerated() { if let image = keep(answers[index], as: want.key) { sites[want.name] = image } } } + /// Whether this search was cancelled, as it is when the servers change + /// while it runs. A cancelled request comes back empty, which is not a + /// site without an icon: nothing is remembered and `keys` are asked again. + private func stopped(asking keys: [String]) -> Bool { + guard Task.isCancelled else { return false } + searched.subtract(keys) + return true + } + private enum Kept { case found(NSImage) /// Looked for not long ago, and not there. diff --git a/PlugApp/PlugAppTests/SettingsSurfaceTests.swift b/PlugApp/PlugAppTests/SettingsSurfaceTests.swift index 5bc5eb28..269ee44c 100644 --- a/PlugApp/PlugAppTests/SettingsSurfaceTests.swift +++ b/PlugApp/PlugAppTests/SettingsSurfaceTests.swift @@ -385,6 +385,23 @@ final class FoundIconTests: XCTestCase { XCTAssertNil(IconStore(chosenDirectory: directory, cacheDirectory: directory).image(forServer: "No Such Server")) } + /// The servers change while icons are being looked for at launch, which + /// cancels the search. That used to be remembered as "no icon" for a day. + @MainActor + func testACancelledIconSearchIsNotRememberedAsNoIcon() async throws { + let directory = FileManager.default.temporaryDirectory.appending(path: "plug-icons-\(UUID().uuidString)") + defer { try? FileManager.default.removeItem(at: directory) } + let found = directory.appending(path: "found") + let store = IconStore(chosenDirectory: directory, cacheDirectory: found) + let search = Task { @MainActor in + await store.load([IconSource(name: "nowhere", address: "https://icons.invalid/mcp")]) + } + search.cancel() + await search.value + let kept = (try? FileManager.default.contentsOfDirectory(atPath: found.path)) ?? [] + XCTAssertEqual(kept, [], "nothing is remembered about a search that was cut short") + } + func testTheStatusAnswerCarriesWhatAServerSaysAboutItself() throws { let json = """ {"type":"Status","servers":[ diff --git a/plug-core/src/downstream_oauth/mod.rs b/plug-core/src/downstream_oauth/mod.rs index b87c61ee..31c3f527 100644 --- a/plug-core/src/downstream_oauth/mod.rs +++ b/plug-core/src/downstream_oauth/mod.rs @@ -1646,7 +1646,9 @@ impl DownstreamOauthManager { } // Before the save, as for a removed client: if the save // fails, what is already open under this sign-in still ends. - if let Some((_, sign_in)) = self.family_ended.remove(&consumed.family_id) { + // The entry stays, so a token the failed save left behind + // opens nothing new either. + if let Some(sign_in) = self.family_ended.get(&consumed.family_id) { sign_in.ended.store(true, Ordering::SeqCst); } let mut next = guard.clone(); diff --git a/plug-core/src/events/access.rs b/plug-core/src/events/access.rs index 980affff..0ee89bb6 100644 --- a/plug-core/src/events/access.rs +++ b/plug-core/src/events/access.rs @@ -58,7 +58,7 @@ impl WatchAccess for RuntimeWatchAccess { fn may_use(&self, client_id: &str, watch: &WatchConfig) -> bool { self.engine.upgrade().is_some_and(|engine| { engine.tool_router().client_may_watch( - Some(&format!("oauth:{client_id}")), + Some(&crate::ipc::grant_client_key(client_id)), &watch.server, &watch.tool, ) diff --git a/plug-core/src/operator.rs b/plug-core/src/operator.rs index d2341f26..3a6d95cc 100644 --- a/plug-core/src/operator.rs +++ b/plug-core/src/operator.rs @@ -186,6 +186,14 @@ pub fn apply_operator_mutation_knowing( if config.servers.remove(&name).is_none() { anyhow::bail!("unknown server `{name}`"); } + // Off every allow list too, so a server added later under the + // same name is a new one and no client gets it unasked. + for settings in config.clients.values_mut() { + // A list this empties still holds: nothing, not everything. + settings.only_allowed = settings.has_allow_list(); + settings.allowed_servers.retain(|server| *server != name); + settings.allowed_tools.retain(|tool| tool.server != name); + } OperatorMutationResult::server(None) } OperatorMutation::AddAccount { server, account } => { @@ -1093,6 +1101,28 @@ API_KEY = "sk-live-123" assert!(!config.clients.contains_key("radar")); } + #[test] + fn removing_a_server_takes_it_off_every_allow_list() { + let path = fixture_path(); + std::fs::write( + &path, + "[servers.git]\ncommand = \"git-mcp\"\n\n[clients.radar]\nallowed_servers = [\"git\"]\n", + ) + .unwrap(); + let (config, _) = apply_operator_mutation_knowing( + &path, + OperatorMutation::RemoveServer { + name: "git".to_string(), + }, + &|_| None, + ) + .unwrap(); + // A server added later under the same name is not one it gets, and + // the emptied list still holds. + assert!(config.clients["radar"].allowed_servers.is_empty()); + assert!(config.clients["radar"].has_allow_list()); + } + #[test] fn a_tool_block_is_stored_by_server_and_tool_and_old_ones_are_brought_over() { use crate::config::ToolBlock; diff --git a/plug-core/src/proxy/catalog.rs b/plug-core/src/proxy/catalog.rs index 6af35b78..5b4b0ca8 100644 --- a/plug-core/src/proxy/catalog.rs +++ b/plug-core/src/proxy/catalog.rs @@ -846,7 +846,7 @@ impl super::ToolRouter { /// what belongs to a server it is kept from. pub fn list_resources_for_client(&self, client_key: Option<&str>) -> Arc> { let snapshot = self.cache.load(); - if !self.client_has_blocked_servers(client_key) { + if !self.client_is_kept_from_a_server(client_key) { return Arc::clone(&snapshot.resources_all); } Arc::new( @@ -891,7 +891,7 @@ impl super::ToolRouter { client_key: Option<&str>, ) -> Arc> { let snapshot = self.cache.load(); - if !self.client_has_blocked_servers(client_key) { + if !self.client_is_kept_from_a_server(client_key) { return Arc::clone(&snapshot.resource_templates_all); } Arc::new( @@ -935,7 +935,7 @@ impl super::ToolRouter { pub fn list_prompts_for_client(&self, client_key: Option<&str>) -> Arc> { let snapshot = self.cache.load(); - if !self.client_has_blocked_servers(client_key) { + if !self.client_is_kept_from_a_server(client_key) { return Arc::clone(&snapshot.prompts_all); } Arc::new( diff --git a/plug-core/src/proxy/mod.rs b/plug-core/src/proxy/mod.rs index a57f7d55..4311dfea 100644 --- a/plug-core/src/proxy/mod.rs +++ b/plug-core/src/proxy/mod.rs @@ -1193,7 +1193,8 @@ impl ToolRouter { } } - /// Whether the client behind `client_key` is kept from any server. + /// Whether the client behind `client_key` is kept from any server, so the + /// lists it is served need filtering at all. pub fn client_is_kept_from_a_server(&self, client_key: Option<&str>) -> bool { client_key.is_some_and(|key| { self.client_access @@ -1220,6 +1221,11 @@ impl ToolRouter { let Some((server_id, own_name)) = cache.resolve_route(tool_name) else { return true; }; + // Plug's own tools are how a client finds the rest. What they hand + // back is filtered for the client, so no list keeps them from it. + if server_id == "__plug_internal__" { + return true; + } !access.keeps_from( server_id, Some(own_name), @@ -1280,9 +1286,12 @@ impl ToolRouter { /// the tool's result, so a block on the tool covers it. pub fn client_may_watch(&self, client_key: Option<&str>, server: &str, tool: &str) -> bool { self.client_may_use_server(client_key, server) - && self - .watched_tool(server, tool) - .is_none_or(|(name, _)| self.client_may_use_tool(client_key, &name)) + && match self.watched_tool(server, tool) { + Some((name, _)) => self.client_may_use_tool(client_key, &name), + // Not listed now, so asked the way a kept result is: by + // server and the server's own name for the tool. + None => self.client_may_read_result_of(client_key, Some(server), Some(tool), ""), + } } /// Whether the client behind `client_key` may see `server_id` at all. @@ -1320,17 +1329,6 @@ impl ToolRouter { } } - /// Whether the client behind `client_key` is kept from any server, so the - /// lists it is served need filtering at all. - pub(crate) fn client_has_blocked_servers(&self, client_key: Option<&str>) -> bool { - client_key.is_some_and(|client_key| { - self.client_access - .load() - .get(client_key) - .is_some_and(ClientAccess::keeps_from_a_server) - }) - } - fn ensure_client_may_use_tool( &self, downstream: Option<&DownstreamCallContext>, @@ -2618,14 +2616,15 @@ impl ToolRouter { // A tool switched off stays off when a second account changes // its name, or when the account that changed it is removed. - if is_disabled_tool(&self.config.disabled_tools, &prefixed_name) - || crate::tool_naming::other_names( - &prefixed_name, - &c.server_name, - &self.config.prefix_delimiter, - ) - .iter() - .any(|name| is_disabled_tool(&self.config.disabled_tools, name)) + if !self.config.disabled_tools.is_empty() + && (is_disabled_tool(&self.config.disabled_tools, &prefixed_name) + || crate::tool_naming::other_names( + &prefixed_name, + &c.server_name, + &self.config.prefix_delimiter, + ) + .iter() + .any(|name| is_disabled_tool(&self.config.disabled_tools, name))) { continue; } diff --git a/plug-core/src/proxy/tasks.rs b/plug-core/src/proxy/tasks.rs index 7d64318b..b4d934ee 100644 --- a/plug-core/src/proxy/tasks.rs +++ b/plug-core/src/proxy/tasks.rs @@ -448,7 +448,14 @@ impl super::ToolRouter { // detaching the future, which kept running and kept holding its // server's `max_concurrent` semaphore permit.) No await is held // across this scope other than acquiring the lock itself. - let behind = self.tool_behind(tool_name); + // The way the call itself routes, so a tool called by its server's + // own name is recorded too. + let behind = self + .cache + .load() + .resolve_route(tool_name) + .filter(|(server, _)| server != "__plug_internal__") + .cloned(); let created = { let mut store = self.task_store.lock().await; let task = match task_quota_lease { diff --git a/plug-core/src/proxy/tests.rs b/plug-core/src/proxy/tests.rs index 08ead15b..3b363ea3 100644 --- a/plug-core/src/proxy/tests.rs +++ b/plug-core/src/proxy/tests.rs @@ -5108,6 +5108,32 @@ async fn the_meta_tools_do_not_reach_around_a_block() { ); } +#[test] +fn an_allow_list_leaves_a_client_the_tool_it_searches_with() { + let mut config = test_router_config(); + config.meta_tool_mode = true; + let router = router_with_client_blocks(config); + let mut clients = std::collections::BTreeMap::new(); + clients.insert( + "radar".to_string(), + crate::config::ClientSettings { + allowed_servers: vec!["slack".to_string()], + ..Default::default() + }, + ); + router.set_client_access(&clients); + + let names = + tool_names(&router.list_tools_for_client_session(ClientType::Unknown, None, Some("radar"))); + assert!( + names.contains(&"plug__search_tools".to_string()), + "{names:?}" + ); + assert!(router.client_may_use_tool(Some("radar"), "plug__search_tools")); + // What it finds is still only what is on the list. + assert!(!router.client_may_use_tool(Some("radar"), "git__commit")); +} + #[test] fn clients_hear_about_a_block_changing_and_about_nothing_else() { let router = ToolRouter::new(Arc::new(ServerManager::new()), test_router_config()); diff --git a/plug-core/src/session/stateful.rs b/plug-core/src/session/stateful.rs index 5e6a824b..b422c0c0 100644 --- a/plug-core/src/session/stateful.rs +++ b/plug-core/src/session/stateful.rs @@ -37,6 +37,13 @@ struct SessionState { broadcast_audience: crate::session::BroadcastAudience, } +impl SessionState { + /// Whether the sign-in this session was opened under no longer holds. + fn access_ended(&self) -> bool { + self.still_allowed.as_ref().is_some_and(|check| !check()) + } +} + impl StatefulSessionStore { /// Return a transport-aware snapshot of tracked HTTP sessions. pub fn list_sessions(&self) -> Vec { @@ -158,7 +165,7 @@ impl StatefulSessionStore { self.remove_if_expired(session_id); return Err(HttpError::SessionNotFound); } - if entry.still_allowed.as_ref().is_some_and(|check| !check()) { + if entry.access_ended() { drop(entry); self.end(session_id); return Err(HttpError::SessionNotFound); @@ -192,7 +199,7 @@ impl StatefulSessionStore { if let Some(mut entry) = self.sessions.get_mut(session_id) { if entry.last_activity.elapsed() > self.timeout { remove_session = true; - } else if entry.still_allowed.as_ref().is_some_and(|check| !check()) { + } else if entry.access_ended() { drop(entry); self.end(session_id); return SessionSendOutcome::SessionNotFound; @@ -540,7 +547,7 @@ impl SessionStore for StatefulSessionStore { expired.push(session_id); continue; } - if entry.still_allowed.as_ref().is_some_and(|check| !check()) { + if entry.access_ended() { ended.push(session_id); continue; } diff --git a/plug/src/client_host.rs b/plug/src/client_host.rs index 8bf23279..107e0143 100644 --- a/plug/src/client_host.rs +++ b/plug/src/client_host.rs @@ -37,7 +37,8 @@ pub(crate) fn current() -> Option { if is_interpreter(executable) { host.script = command_line(pid).as_deref().and_then(|line| { let script = script_of(line)?; - Some(if runs_a_module(line) { + // Only a script typed without its folder needs it. + Some(if runs_a_module(line) || script.starts_with('/') { script } else { anchored(&script, working_folder(pid).as_deref()) diff --git a/plug/src/commands/clients.rs b/plug/src/commands/clients.rs index 1fd648ad..25752663 100644 --- a/plug/src/commands/clients.rs +++ b/plug/src/commands/clients.rs @@ -169,7 +169,7 @@ pub(crate) enum ClientCommands { /// Take the named servers and tools off the list instead #[arg(long, conflicts_with = "off")] remove: bool, - /// End the list. The client keeps what it gets now, and gets + /// End the list. The client keeps the servers on it, and gets /// servers added later #[arg(long)] off: bool, diff --git a/plug/src/daemon/menu_bar.rs b/plug/src/daemon/menu_bar.rs index 435f89c0..590b9202 100644 --- a/plug/src/daemon/menu_bar.rs +++ b/plug/src/daemon/menu_bar.rs @@ -77,15 +77,21 @@ pub(super) fn spawn(cancel: CancellationToken) { _ = cancel.cancelled() => return, _ = ticker.tick() => {} } - let Ok(ps) = tokio::process::Command::new("/bin/ps") - .args(["-axo", "comm="]) - .output() - .await - else { - continue; + // Hidden by its owner: nothing to reopen, so nothing to look for. + let hidden = hidden_marker().exists(); + let running = if hidden { + false + } else { + let Ok(ps) = tokio::process::Command::new("/bin/ps") + .args(["-axo", "comm="]) + .output() + .await + else { + continue; + }; + app_is_running(&String::from_utf8_lossy(&ps.stdout), &app_exe) }; - let running = app_is_running(&String::from_utf8_lossy(&ps.stdout), &app_exe); - if !watch.look(running, hidden_marker().exists()) || cancel.is_cancelled() { + if !watch.look(running, hidden) || cancel.is_cancelled() { continue; } tracing::info!(app = %bundle.display(), "opening Plug.app again: it was closed while the daemon is serving"); diff --git a/plug/src/daemon/mod.rs b/plug/src/daemon/mod.rs index 535169cb..46b4349f 100644 --- a/plug/src/daemon/mod.rs +++ b/plug/src/daemon/mod.rs @@ -1657,7 +1657,9 @@ async fn dispatch_request(request: &IpcRequest, ctx: &mut ConnectionContext) -> ctx.engine .tool_router() .listed_name(&allowed.server, &allowed.tool) - .unwrap_or_else(|| allowed.to_string()) + // The form `plug clients only --remove` + // takes for a tool Plug no longer lists. + .unwrap_or_else(|| format!("{}/{}", allowed.server, allowed.tool)) }) .collect(), // By the name each tool is listed under now, which is