Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `<recommended_plugins>` suggestions stop nagging: a plugin id is now
injected at most once per engine lifetime, and a plugin whose name a
loaded skill already covers is never suggested — the local skill owns
the domain, so the nudge was noise. Dismissals still apply, and the
fragment stays append-only on the user turn (#6274).
- A canceled automation run now settles with a transcript receipt that names
the cancellation (by request, cancel timeout, or shutdown) instead of
vanishing from the live band silently. The receipt wears attention ink and
Expand Down
5 changes: 5 additions & 0 deletions crates/tui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `<recommended_plugins>` suggestions stop nagging: a plugin id is now
injected at most once per engine lifetime, and a plugin whose name a
loaded skill already covers is never suggested — the local skill owns
the domain, so the nudge was noise. Dismissals still apply, and the
fragment stays append-only on the user turn (#6274).
- A canceled automation run now settles with a transcript receipt that names
the cancellation (by request, cancel timeout, or shutdown) instead of
vanishing from the live band silently. The receipt wears attention ink and
Expand Down
54 changes: 47 additions & 7 deletions crates/tui/src/core/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,12 @@ pub struct Engine {
mcp_event_generation: u64,
/// Workspace-scoped immutable plugin catalogue and authority receipts.
plugin_registry: Arc<crate::plugins::PluginRegistry>,
/// Keeps the append-only `<recommended_plugins>` fragment once-per-
/// Engine-lifetime per plugin id, and suppresses plugins whose name a
/// catalogue skill already covers (#6274). The skill-name snapshot is
/// taken at construction from the same catalogue the system prompt
/// indexes (see the gate's known-limitations note).
recommended_plugin_gate: StdMutex<crate::plugins::recommend::RecommendedPluginGate>,
api_provider: ApiProvider,
/// Exact configured route key. Named custom providers share the `Custom`
/// enum, so the enum alone cannot prove that the active client is current.
Expand Down Expand Up @@ -1674,6 +1680,28 @@ impl Engine {
// `run_turn` restarts it per turn; this initial value only matters
// for hosts that inspect the engine before the first turn.
let turn_wall_clock_budget = config.turn_wall_clock;
// Skill-name snapshot for the plugin-suggestion gate (#6274): the
// SAME catalogue the system prompt indexes (prompts.rs skills block —
// workspace roots + configured skills_dir + plugin-sourced skills),
// so suppression sees everything the session actually has.
let gate_skill_names: std::collections::BTreeSet<String> =
crate::skills::discover_for_workspace_and_dir_with_mode_and_plugins(
&config.workspace,
&config.skills_dir,
crate::skills::SkillDiscoveryMode::from_codewhale_only(
config.skills_scan_codewhale_only,
),
Some(plugin_registry.as_ref()),
)
.list()
.iter()
.flat_map(|skill| {
std::iter::once(skill.name.clone()).chain(skill.aliases.iter().cloned())
})
.map(|name| name.trim().to_ascii_lowercase())
.filter(|name| !name.is_empty())
.collect();

let engine = Engine {
config,
api_config: api_config.clone(),
Expand All @@ -1698,6 +1726,11 @@ impl Engine {
mcp_boot_generation: None,
mcp_event_generation: 0,
plugin_registry,
recommended_plugin_gate: StdMutex::new(
crate::plugins::recommend::RecommendedPluginGate::with_skill_names(
gate_skill_names,
),
),
api_provider,
api_provider_identity,
api_provider_id,
Expand Down Expand Up @@ -3708,13 +3741,20 @@ impl Engine {
cache_control: None,
}];
}
let recommended_plugins = crate::plugins::recommend::recommended_plugins_user_fragment(
&text,
self.plugin_registry.as_ref(),
&crate::plugins::recommend::load_marketplace_candidates(
self.plugin_registry.state_path(),
),
);
let recommended_plugins = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Engine gate mutex is held across filesystem reads and the whole match

The new block takes self.recommended_plugin_gate.lock() and then evaluates crate::plugins::recommend::load_marketplace_candidates(self.plugin_registry.state_path()) (a marketplace-state file read) and the entire recommended_plugins_user_fragment call (which itself does Settings::load_read_only() plus plugin matching) while the guard is live; the guard is only needed for gate.admits. On a host that composes user turns on more than one thread for the same Engine (the engine is long-lived and the lock is the only synchronisation on shown), the second composer blocks on the first composer's disk I/O under this std mutex. No correctness change — the once-per-engine set stays consistent — but the lock scope is wider than the state it protects.

let mut recommended_plugin_gate = self
.recommended_plugin_gate
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
crate::plugins::recommend::recommended_plugins_user_fragment(
&text,
self.plugin_registry.as_ref(),
&crate::plugins::recommend::load_marketplace_candidates(
self.plugin_registry.state_path(),
),
&mut recommended_plugin_gate,
)
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow the guarded region: load the marketplace candidates before taking the lock (and ideally let the fragment take the already-loaded slice plus a pre-loaded dismissal set), so the gate mutex only covers the admits decision rather than a state-file read plus settings load plus matching. This keeps the once-per-Engine semantics identical while removing disk I/O from the critical section. Anything that isolates only admits needs a small API change to recommended_plugins_user_fragment (it currently borrows the gate mutably for the whole body), so no literal replacement is given here.

let expanded = crate::image_attach::expand_attachment_blocks(&text);
let mut content = Vec::with_capacity(3 + expanded.blocks.len());
content.push(ContentBlock::Text {
Expand Down
122 changes: 118 additions & 4 deletions crates/tui/src/plugins/recommend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,59 @@ pub fn match_plugin_for_draft_among(
Some(matched)
}

/// Per-Engine gate for the append-only `<recommended_plugins>` fragment.
///
/// A plugin id is suggested at most once per Engine lifetime, and a plugin
/// whose name (or alias) matches a skill in the session's catalogue is never
/// suggested — the skill already covers the domain, so the nudge is noise
/// (#6274). Dismissals continue to be honored through `Settings`.
///
/// Known limitation: the skill-name set is snapshotted once at Engine
/// construction (from the same catalogue the system prompt indexes), so a
/// skill installed mid-session does not suppress its plugin twin until the
/// next Engine starts.
#[derive(Debug, Default)]
pub struct RecommendedPluginGate {
shown: BTreeSet<String>,
skill_names: BTreeSet<String>,
}

impl RecommendedPluginGate {
/// Engine constructor input and test seam: suppress exactly these
/// skill names and aliases (case is normalized here, so callers may
/// pass them in any form).
#[must_use]
pub fn with_skill_names(skill_names: BTreeSet<String>) -> Self {
Self {
shown: BTreeSet::new(),
skill_names: skill_names
.into_iter()
.map(|name| name.trim().to_ascii_lowercase())
.filter(|name| !name.is_empty())
.collect(),
}
}

/// True when this plugin may be suggested now: not covered by a
/// catalogue skill and not already suggested in this Engine's lifetime.
/// First admission records the plugin id.
fn admits(&mut self, id: &str, name: &str) -> bool {
let name_key = name.trim().to_ascii_lowercase();
if !name_key.is_empty() && self.skill_names.contains(&name_key) {
return false;
}
self.shown.insert(id.to_string())
}
}

/// Append-only user-turn fragment. Never part of the pinned system prefix.
/// Bounded, omitted when nothing matches.
#[must_use]
pub fn recommended_plugins_user_fragment(
draft: &str,
registry: &PluginRegistry,
marketplace: &[MarketplaceCandidate],
gate: &mut RecommendedPluginGate,
) -> Option<String> {
// Called once when composing a user turn, never from the render loop.
// Read the shared preference so headless and long-lived Engines also
Expand All @@ -277,6 +323,11 @@ pub fn recommended_plugins_user_fragment(
marketplace,
&settings.dismissed_plugin_suggestions,
)?;
// Once per Engine lifetime per plugin id, and never when a local skill
// already covers the plugin's domain (#6274).
if !gate.admits(&matched.id, &matched.name) {
return None;
}
let mut listed = vec![matched];
listed.truncate(MAX_RECOMMENDED_PLUGINS);
let body = listed
Expand Down Expand Up @@ -650,14 +701,77 @@ mod tests {
let registry = crate::plugins::PluginDiscoveryContext::capture_pre_dotenv()
.registry_for_workspace(root.path());

let fragment =
recommended_plugins_user_fragment("add supabase auth to login", &registry, &[])
.expect("idle plugin should produce a fragment");
let fragment = recommended_plugins_user_fragment(
"add supabase auth to login",
&registry,
&[],
&mut RecommendedPluginGate::default(),
)
.expect("idle plugin should produce a fragment");
assert!(fragment.starts_with("<recommended_plugins>"));
assert!(fragment.contains("- supabase ("));
assert!(fragment.contains("</recommended_plugins>"));
assert!(
recommended_plugins_user_fragment("fix the failing test", &registry, &[]).is_none()
recommended_plugins_user_fragment(
"fix the failing test",
&registry,
&[],
&mut RecommendedPluginGate::default(),
)
.is_none()
);
}

#[test]
fn recommended_plugins_fragment_suggests_a_plugin_once_per_gate() {
let _lock = lock_test_env();
let root = TempDir::new().unwrap();
let _home = EnvVarGuard::set("CODEWHALE_HOME", root.path().join("home"));
write_keyword_bundle(root.path(), "supabase", "Hosted Postgres", &["supabase"]);
let registry = crate::plugins::PluginDiscoveryContext::capture_pre_dotenv()
.registry_for_workspace(root.path());

let mut gate = RecommendedPluginGate::default();
let first = recommended_plugins_user_fragment(
"add supabase auth to login",
&registry,
&[],
&mut gate,
)
.expect("first matching turn suggests the plugin");
assert!(first.contains("- supabase ("));
assert!(
recommended_plugins_user_fragment(
"add supabase auth to the signup flow",
&registry,
&[],
&mut gate,
)
.is_none(),
"a plugin id is suggested at most once per Engine lifetime (#6274)"
);
}

#[test]
fn recommended_plugins_fragment_suppressed_by_local_skill_name() {
let _lock = lock_test_env();
let root = TempDir::new().unwrap();
let _home = EnvVarGuard::set("CODEWHALE_HOME", root.path().join("home"));
write_keyword_bundle(root.path(), "supabase", "Hosted Postgres", &["supabase"]);
let registry = crate::plugins::PluginDiscoveryContext::capture_pre_dotenv()
.registry_for_workspace(root.path());

let skills: BTreeSet<String> = ["Supabase".to_string()].into_iter().collect();
let mut gate = RecommendedPluginGate::with_skill_names(skills);
assert!(
recommended_plugins_user_fragment(
"add supabase auth to login",
&registry,
&[],
&mut gate,
)
.is_none(),
"a loaded local skill covering the plugin name must suppress the suggestion (#6274)"
);
}

Expand Down
1 change: 1 addition & 0 deletions crates/tui/src/tui/plugin_suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ mod tests {
&app.input,
restarted.plugin_registry.as_ref(),
&[],
&mut crate::plugins::recommend::RecommendedPluginGate::default(),
)
.is_none()
);
Expand Down
3 changes: 2 additions & 1 deletion web/lib/changelog.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const CHANGELOG: ChangelogRelease[] = [
{
"heading": "Fixed",
"items": [
"<recommended_plugins> suggestions stop nagging: a plugin id is now injected at most once per engine lifetime, and a plugin whose name a loaded skill already covers is never suggested — the local skill owns the domain, so the nudge was noise. Dismissals still apply, and the fragment stays append-only on the user turn (#6274).",
"A canceled automation run now settles with a transcript receipt that names the cancellation (by request, cancel timeout, or shutdown) instead of vanishing from the live band silently. The receipt wears attention ink and never lights the failure demand; the run record keeps the cancellation reason as its error detail. (#6162)",
"A failed workflow run no longer settles silently: its terminal failure raises a sticky error toast naming the cause (dispatch, schema, or script errors), alongside the existing panel state (#5528).",
"MCP OAuth re-login now forces the provider's consent screen: logout only clears the local token, so without a prompt the provider silently re-granted the same account/workspace and a re-login could never change it. /mcp logout and codewhale mcp logout also say plainly that they clear local credentials only (#6040).",
Expand All @@ -73,7 +74,7 @@ export const CHANGELOG: ChangelogRelease[] = [
"/mcp no longer freezes the console while a turn is running: the panel opens immediately from the last known MCP snapshot with a receipt naming the wait, and live-pool mutations say their refresh is deferred instead of parking the UI event loop behind the running turn (#6159).",
"MCP OAuth login no longer fails with \"Authorization server response missing required issuer\" against servers that implement RFC 9207, such as Cloudflare's mcp.cloudflare.com. The local callback listener now keeps the iss parameter from the redirect and hands it to the token exchange so the callback binds to the discovered issuer; servers that do not send iss keep working unchanged. (#6157)"
],
"itemCount": 10
"itemCount": 11
}
]
},
Expand Down
Loading