mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-07 10:36:05 -10:00
fix(approvals): keep Full Access for destructive MCP tools; doctor and dismissal edge cases
Review of the 0.10.1 addendum-remainder commits (CW-11, U7, policy rule 9). CW-11 (b90590369): - The destructiveHint branch set auto_approve=false under Full Access. Every host then got an approval request, and `codewhale exec` answers from its own --auto flag, so exec with approval_policy = "full-access" in config (and no --auto) denied every destructive-declared MCP call (all Computer Use input tools declare it). The TUI and runtime auto-approved it anyway, and a session grant still covered it. So the promised "keeps its prompt" never held anywhere, and one host broke. Full Access again covers these tools (#3866). destructiveHint still withholds the reviewed-plugin read-only relaxation and labels the card. docs/MODES.md now says exactly that. - Preparation let a bounded worker (tool_authority) run a plugin-declared read-only MCP tool, but tool_execution refuses every non-built-in MCP tool for such a worker. So an Auto call passed preparation and failed at execution. Preparation now applies the execution gate's rule. U7 (05ff7c753,d2b1430cc): first_run_ready accepts ProviderModel NeedsAction (a failed key still reaches the wizard's ready screen), so doctor printed "Ready: setup is complete." for a finished setup whose key was never confirmed. The provider check now runs first. Policy rule 9 (735fb509a): the saved dismissal list is matched and listed case-insensitively. A hand-edited mixed-case entry could not be reset by name and was listed twice. No-Issue: 0.10.1 addendum review (C5/CW-11, U7, PLG-12) Checks (targeted, local; the shared tree also held a peer's deny.toml edit): - cargo test -p codewhale-tui --lib -- mcp_annotation_hints only_a_reviewed_plugin mcp_write_preparation a_fresh_home_is_not_ready finished_setup_with_an_unconfirmed_key plugin_dismissals fleet_run_check agent_run: 17 passed, 0 failed - cargo test -p codewhale-tui --lib -- fleet::manager mcp::tests tool_preparation commands::groups::plugins runtime_api::tests::agent_run doctor: 449 passed, 0 failed, 1 ignored - cargo test -p codewhale-tui --lib -- core::engine::tests::mcp computer_use approval_cache: 33 passed, 0 failed - cargo test -p codewhale-command-contract: 59 passed, 0 failed - rustfmt --check on the touched files: clean Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
1 parent
7b16039278
commit
669c32d53f
4 files changed
+94
-23
No files matched your search
@@ -4202,9 +4202,14 @@ impl CommandPluginContext for PluginAdapter<'_> {
|
||||
fn suggestion_dismissals(
|
||||
&self,
|
||||
) -> Result<codewhale_command_contract::facets::PluginSuggestionDismissals, String> {
|
||||
let persisted = crate::settings::Settings::load()
|
||||
// Stored lowercase by the CTA, but a hand-edited settings file may not
|
||||
// be; fold here so the list matches what suggestions actually skip.
|
||||
let persisted: std::collections::BTreeSet<String> = crate::settings::Settings::load()
|
||||
.map_err(|err| format!("could not read saved plugin dismissals: {err}"))?
|
||||
.dismissed_plugin_suggestions;
|
||||
.dismissed_plugin_suggestions
|
||||
.iter()
|
||||
.map(|name| name.to_ascii_lowercase())
|
||||
.collect();
|
||||
let app = self.host.app.borrow();
|
||||
let session = app
|
||||
.plugin_cta
|
||||
@@ -4222,15 +4227,15 @@ impl CommandPluginContext for PluginAdapter<'_> {
|
||||
}
|
||||
|
||||
fn reset_suggestion_dismissals(&mut self, name: Option<&str>) -> Result<Vec<String>, String> {
|
||||
let target = name.map(str::to_ascii_lowercase);
|
||||
let matches = |candidate: &String| target.as_ref().is_none_or(|target| candidate == target);
|
||||
let matches =
|
||||
|candidate: &String| name.is_none_or(|target| candidate.eq_ignore_ascii_case(target));
|
||||
let mut cleared = std::collections::BTreeSet::new();
|
||||
crate::settings::Settings::transact_opt(|settings| {
|
||||
let before = settings.dismissed_plugin_suggestions.len();
|
||||
settings.dismissed_plugin_suggestions.retain(|candidate| {
|
||||
let reset = matches(candidate);
|
||||
if reset {
|
||||
cleared.insert(candidate.clone());
|
||||
cleared.insert(candidate.to_ascii_lowercase());
|
||||
}
|
||||
!reset
|
||||
});
|
||||
|
||||
@@ -38,13 +38,18 @@ pub(super) fn prepare_tool_call(
|
||||
) -> Result<PreparedToolPolicy, ToolError> {
|
||||
if McpPool::is_mcp_tool(name) {
|
||||
// CW-11: a reviewed plugin's `readOnlyHint` makes its tool run like
|
||||
// the built-in resource reads; a declared `destructiveHint` keeps the
|
||||
// prompt even when the session auto-approves tools.
|
||||
let hint = crate::mcp::mcp_tool_approval_hint(name);
|
||||
// the built-in resource reads. A declared `destructiveHint` only
|
||||
// withholds that relaxation and labels the card: Full Access still
|
||||
// covers it (#3866), because a host that answers approvals from its
|
||||
// own flag (`exec` with a Full Access `approval_policy`) would
|
||||
// otherwise deny a call its posture already allows.
|
||||
let read_only = mcp_tool_is_read_only(name)
|
||||
|| hint == Some(crate::mcp::McpToolApprovalHint::TrustedReadOnly);
|
||||
let destructive = hint == Some(crate::mcp::McpToolApprovalHint::Destructive);
|
||||
if !read_only
|
||||
|| crate::mcp::mcp_tool_approval_hint(name)
|
||||
== Some(crate::mcp::McpToolApprovalHint::TrustedReadOnly);
|
||||
// A bounded worker keeps the execution gate's rule (built-in resource
|
||||
// reads only), so preparation never admits a call that
|
||||
// `tool_execution` then refuses.
|
||||
if !mcp_tool_is_read_only(name)
|
||||
&& let Some(authority) =
|
||||
registry.and_then(|registry| registry.context().tool_authority.as_ref())
|
||||
{
|
||||
@@ -113,7 +118,7 @@ pub(super) fn prepare_tool_call(
|
||||
},
|
||||
resources: vec![ResourceClaim::GlobalExclusive],
|
||||
},
|
||||
auto_approve: session_auto_approve && !destructive,
|
||||
auto_approve: session_auto_approve,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -535,15 +540,44 @@ mod tests {
|
||||
assert_eq!(prepared.call.approval, ApprovalRequirement::Auto);
|
||||
assert!(prepared.call.read_only);
|
||||
|
||||
// A bounded worker keeps the execution gate's rule: only the built-in
|
||||
// resource reads, so preparation never admits a call execution refuses.
|
||||
let workspace = tempfile::tempdir().expect("tempdir");
|
||||
let context = crate::tools::ToolContext::new(workspace.path().to_path_buf())
|
||||
.with_tool_authority(crate::tools::spec::ToolAuthorityEnvelope {
|
||||
schema_version: 1,
|
||||
owner: "cw11-worker".to_string(),
|
||||
authority: crate::tools::spec::ToolMutationAuthority::ScopedWrite,
|
||||
network_access: None,
|
||||
shell: crate::tools::spec::ToolShellAuthority::None,
|
||||
verification: crate::tools::spec::ToolVerificationAuthority::None,
|
||||
writable_roots: Vec::new(),
|
||||
writable_files: vec!["src/named.rs".to_string()],
|
||||
coordination_contracts: Vec::new(),
|
||||
})
|
||||
.expect("valid envelope");
|
||||
let registry = crate::tools::ToolRegistry::new(context);
|
||||
let refused = prepare_tool_call(read_only, json!({}), Some(®istry), false)
|
||||
.expect_err("a bounded worker cannot run a plugin-declared read");
|
||||
assert!(refused.to_string().contains("cw11-worker"), "{refused}");
|
||||
|
||||
let destructive = "mcp_cw11test_drop_table";
|
||||
set_mcp_tool_approval_hint_for_test(destructive, Some(McpToolApprovalHint::Destructive));
|
||||
let prepared = prepare_tool_call(destructive, json!({}), None, true)
|
||||
let prepared = prepare_tool_call(destructive, json!({}), None, false)
|
||||
.expect("prepare destructive MCP tool");
|
||||
assert_eq!(prepared.call.approval, ApprovalRequirement::Suggest);
|
||||
assert!(!prepared.call.read_only);
|
||||
assert!(
|
||||
!prepared.auto_approve,
|
||||
"session auto-approve must not cover a destructive tool"
|
||||
prepared.call.description.contains("destructive"),
|
||||
"{}",
|
||||
prepared.call.description
|
||||
);
|
||||
// Full Access covers it like any other promptable tool (#3866): a
|
||||
// host answering from its own flag must not deny what the posture
|
||||
// allows.
|
||||
let prepared = prepare_tool_call(destructive, json!({}), None, true)
|
||||
.expect("prepare destructive MCP tool under Full Access");
|
||||
assert!(prepared.auto_approve);
|
||||
|
||||
set_mcp_tool_approval_hint_for_test(read_only, None);
|
||||
set_mcp_tool_approval_hint_for_test(destructive, None);
|
||||
|
||||
+36
-6
@@ -5455,17 +5455,19 @@ async fn run_doctor(
|
||||
/// is the setup lane's own verdict; doctor never probes credential values to
|
||||
/// decide it.
|
||||
fn doctor_verdict(state: &codewhale_config::SetupState) -> &'static str {
|
||||
if state.first_run_ready() {
|
||||
return "Ready: setup is complete.";
|
||||
}
|
||||
// NeedsAction means a route is named but no credential is confirmed for
|
||||
// it, which is still "no provider set up" from where the user sits.
|
||||
// `first_run_ready` accepts NeedsAction (a failed key still reaches the
|
||||
// wizard's ready screen), so check the provider first: finished setup
|
||||
// with an unconfirmed key is not "Ready".
|
||||
let provider_verified = state.status(codewhale_config::SetupStep::ProviderModel)
|
||||
== codewhale_config::StepStatus::Verified;
|
||||
if provider_verified {
|
||||
"Not ready: first-run setup is unfinished → run `codewhale setup`."
|
||||
} else {
|
||||
if !provider_verified {
|
||||
"Not ready: no model provider set up → run /provider in Codewhale, or `codewhale setup`."
|
||||
} else if state.first_run_ready() {
|
||||
"Ready: setup is complete."
|
||||
} else {
|
||||
"Not ready: first-run setup is unfinished → run `codewhale setup`."
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5477,6 +5479,34 @@ mod doctor_verdict_tests {
|
||||
assert!(verdict.starts_with("Not ready"), "{verdict}");
|
||||
assert!(verdict.contains("/provider"), "{verdict}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn finished_setup_with_an_unconfirmed_key_is_not_ready() {
|
||||
use codewhale_config::{
|
||||
ConstitutionChoice, RuntimePostureSource, SetupState, SetupStep, StepEntry, StepStatus,
|
||||
};
|
||||
let mut state = SetupState::default();
|
||||
state.set_step(
|
||||
SetupStep::Language,
|
||||
StepEntry::new(StepStatus::Verified, true, "0.10.1"),
|
||||
);
|
||||
state.set_step(
|
||||
SetupStep::ProviderModel,
|
||||
StepEntry::new(StepStatus::NeedsAction, true, "0.10.1"),
|
||||
);
|
||||
state.runtime_posture_source = RuntimePostureSource::Confirmed;
|
||||
state.constitution_choice = ConstitutionChoice::Bundled;
|
||||
assert!(state.first_run_ready(), "fixture must be wizard-ready");
|
||||
let verdict = super::doctor_verdict(&state);
|
||||
assert!(verdict.starts_with("Not ready"), "{verdict}");
|
||||
assert!(verdict.contains("/provider"), "{verdict}");
|
||||
|
||||
state.set_step(
|
||||
SetupStep::ProviderModel,
|
||||
StepEntry::new(StepStatus::Verified, true, "0.10.1"),
|
||||
);
|
||||
assert_eq!(super::doctor_verdict(&state), "Ready: setup is complete.");
|
||||
}
|
||||
}
|
||||
|
||||
const DOCTOR_LEGACY_STATE_ITEMS: &[&str] = &[
|
||||
|
||||
+4
-2
@@ -317,8 +317,10 @@ Access does not bypass hard policy holds.
|
||||
A tool's own MCP annotations count only as far as their source is trusted. A
|
||||
tool from a reviewed, enabled plugin that declares `readOnlyHint: true` runs
|
||||
like the built-in read helpers, with no prompt; the same claim from any other
|
||||
server is ignored. A tool that declares `destructiveHint: true` keeps its
|
||||
prompt even when you have approved tools for the rest of the session.
|
||||
server is ignored. A tool that declares `destructiveHint: true` never gets
|
||||
that relaxation, even from a reviewed plugin, and its approval card says the
|
||||
server marked it destructive. Full Access still runs it without a prompt, like
|
||||
any other tool that would ask.
|
||||
|
||||
See `MCP.md`.
|
||||
|
||||
|
||||
Reference in new issue
Block a user