diff --git a/src/harness/build.rs b/src/harness/build.rs index e4315aaae..5d94e778c 100644 --- a/src/harness/build.rs +++ b/src/harness/build.rs @@ -660,6 +660,14 @@ pub fn build_agent( // the `read_run_output` companion reads back, so a clipped preview // is reachable within the turn. Orchestrator-only, like the tools. deps.run_outputs.clone(), + // Issue #619: who is minting, and how wide they are. `add_agent` + // bounds the teammate it mints by this agent's own scope — #661 + // clamped to the *company* grant, which still lets a narrowly + // scoped agent mint a teammate holding everything the company + // holds — and names this agent in the mint log. + manifest_agent.id.clone(), + manifest_agent.tools.clone(), + grants.to_vec(), )); } diff --git a/src/harness/mod.rs b/src/harness/mod.rs index 45746a832..4c74ee7ac 100644 --- a/src/harness/mod.rs +++ b/src/harness/mod.rs @@ -2884,7 +2884,7 @@ description = "Builds the product." async fn a_tool_added_teammate_colliding_with_a_manifest_id_still_joins_the_roster() { use openhuman_core::openhuman::tools::Tool; - use crate::harness::orchestrator::AddAgentTool; + use crate::harness::orchestrator::unscoped_add_agent; /// A `CompanyStore` that actually holds the record, unlike /// `RecordingStore` — `add_agent` has to load what it saves. @@ -2914,7 +2914,7 @@ description = "Builds the product." let fx = fixture(); let company = CompanyId::new("acme"); let store = Arc::new(SeededStore(StdMutex::new(record()))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(serde_json::json!({ "name": "Engineer", "role": "Platform" })) diff --git a/src/harness/orchestrator.rs b/src/harness/orchestrator.rs index 6ccc6e453..3671c80ce 100644 --- a/src/harness/orchestrator.rs +++ b/src/harness/orchestrator.rs @@ -124,6 +124,7 @@ pub const QUERY_COMPANY_TOOL: &str = "query_company"; // The `spawn_task` / `delegate_to_desk` names are the brain-agnostic canonical // constants (issue #176) — re-exported here so the harness path and the hosted // path share one definition and cannot drift. +use crate::runtime::builder::agent_effective_grants; use crate::runtime::delegation_tools; pub use crate::runtime::delegation_tools::{DELEGATE_TO_DESK_TOOL, SPAWN_TASK_TOOL}; /// The `run_workflow` tool name (issue #67). @@ -1752,16 +1753,73 @@ pub fn delegation_tools( pub struct AddAgentTool { company: CompanyId, store: Arc, + /// The id of the agent this tool is wired onto — the minter. Named in the + /// mint log so an operator can see who added a teammate, and with what. + minter: String, + /// The minter's own `tools` line, verbatim. Empty means the minter itself + /// holds the company's standard grant, in which case so does the teammate. + minter_tools: Vec, + /// The minter's **effective** grant — its line already narrowed by the + /// company `allow`. The ceiling an explicit `tools` argument is clamped to. + minter_grants: Vec, } impl AddAgentTool { /// Builds the tool over the company id and its store handle - /// ([`HarnessDeps::store`](crate::harness::HarnessDeps::store)). - pub fn new(company: CompanyId, store: Arc) -> Self { - Self { company, store } + /// ([`HarnessDeps::store`](crate::harness::HarnessDeps::store)), plus the + /// minting agent's identity and tool scope (issue #619). + /// + /// # Why the minter's scope is a constructor argument + /// + /// #661 gave a minted teammate a `tools` list clamped to the **company** + /// grant. That leaves the defect #619 was filed about intact: omitting + /// `tools` still yields the company's *whole* grant, so an agent scoped to + /// a corner of the company can mint a teammate holding everything the + /// company holds — and `add_agent` is [`Reach::Nothing`](crate::policy) + /// and sits in `INTRINSIC_TOOLS`, so it is always present and never asks. + /// + /// The ceiling is therefore the **minter**, not the company: a minted + /// teammate is never wider than the agent that minted it. + pub fn new( + company: CompanyId, + store: Arc, + minter: String, + minter_tools: Vec, + minter_grants: Vec, + ) -> Self { + Self { + company, + store, + minter, + minter_tools, + minter_grants, + } } } +/// An `add_agent` tool wired onto an **unscoped** minter: an agent whose own +/// `tools` line is empty and which therefore holds the whole company grant. +/// +/// This is the pre-#619 shape of every minter, so a test written before the +/// minter ceiling existed still describes the same company through it. A test +/// that cares about narrowing constructs the tool directly with a scoped +/// minter instead. +#[cfg(test)] +pub(crate) fn unscoped_add_agent(company: CompanyId, store: Arc) -> AddAgentTool { + AddAgentTool::new( + company, + store, + "ceo".to_string(), + // No line of its own — the minter inherits the company grant… + Vec::new(), + // …which for these fixtures is the catch-all, so the minter ceiling is + // wide open and a test about *other* behaviour is not accidentally a + // test about the #619 clamp. A test that cares about the clamp uses + // `scoped_add_agent`. + vec!["*".to_string()], + ) +} + #[async_trait] impl Tool for AddAgentTool { fn name(&self) -> &str { @@ -1818,12 +1876,11 @@ impl Tool for AddAgentTool { // Issue #661 / L5: an optional per-teammate tool grant. The globs are // INTERSECTED with the company's `[tools].allow` at roster-build time // (`agent_effective_grants`), so this can only narrow the new teammate - // below the company grant — never widen or escalate it. Omitted, `null`, - // or empty means the standard company-wide grant, exactly like a manifest - // agent with no `tools` line. A non-string item is a clean argument - // error, the same shape as a missing `name`/`role`. - let tools = match args.get("tools") { - None | Some(Value::Null) => Vec::new(), + // below the company grant — never widen or escalate it. A non-string + // item is a clean argument error, the same shape as a missing + // `name`/`role`. + let requested: Option> = match args.get("tools") { + None | Some(Value::Null) => None, Some(Value::Array(items)) => { let mut globs = Vec::with_capacity(items.len()); for item in items { @@ -1835,10 +1892,44 @@ impl Tool for AddAgentTool { globs.push(glob.to_string()); } } - globs + Some(globs) } Some(_) => return Err(anyhow::anyhow!("`tools` must be an array of strings")), }; + // Issue #619: the company grant is the wrong ceiling. Clamp to the + // MINTER's own scope, resolved before the store is touched so a refused + // scope never leaves a half-written roster. + let tools = match requested { + // Nothing asked for: copy the minter's own line. Copying the *line* + // rather than its resolved grant is deliberate — an unscoped minter + // mints an unscoped teammate that keeps tracking `[tools].allow`, + // instead of freezing today's allow-list into the record as an + // explicit scope a later company-wide narrowing would not reach. + None => self.minter_tools.clone(), + // An explicitly empty list is the same request as none at all — + // "give them what you have" — not "grant everything". + Some(globs) if globs.is_empty() => self.minter_tools.clone(), + Some(globs) => { + // Narrow against what the minter actually holds. An empty result + // means nothing asked for was within reach, and storing that + // would read back as "inherit the whole company grant" — the + // exact inversion #619 exists to remove, reached through the + // most deliberate narrowing an agent can ask for. + let narrowed = agent_effective_grants(&self.minter_grants, &globs); + if narrowed.is_empty() { + return Ok(ToolResult::error(format!( + "None of the requested tools ({}) are within your own tool grant ({}), so \"{name}\" was not added. Ask for a subset of what you hold, or omit `tools` to give them the same grant you have.", + globs.join(", "), + if self.minter_grants.is_empty() { + "nothing".to_string() + } else { + self.minter_grants.join(", ") + }, + ))); + } + narrowed + } + }; // Serialize per-company writes so the orchestrator's add_agent and the // console `POST .../team` route can never clobber each other's @@ -1882,18 +1973,47 @@ impl Tool for AddAgentTool { name: name.clone(), role: role.clone(), description, - tools, + tools: tools.clone(), }; record.overlay_agents.push(agent); self.store.save(&record).await?; + // Issue #619: the mint is observable — the minter, the teammate, and + // the grant it was given. This was the condition attached to sanctioning + // the narrowing at all: `add_agent` is `Reach::Nothing` and never asks, + // so this log is the only place the decision is visible. A narrowing + // that happens silently is the defect being fixed, one layer down. + // + // An **inherited** grant is the line an operator most needs to see, + // because that is the teammate holding everything its minter holds. + tracing::info!( + company = %self.company, + minter = %self.minter, + teammate = %id, + teammate_name = %name, + scope = %if tools.is_empty() { + "inherited: the minter's own standard grant".to_string() + } else { + tools.join(", ") + }, + "[add_agent] minted an overlay teammate" + ); + // The id is in the result because the orchestrator has to be able to // address the teammate it just created — delegating to it, or putting it // on a desk, takes the id, not the display name. The console gets the // same answer from `TeamMemberDto.id`; before this the agent-facing half // had no way to learn it at all. + // The scope is in the result for the same reason it is in the log: the + // minting agent should see what it handed over, and "the same tools you + // hold" is a materially different answer from a named list. + let scope = if tools.is_empty() { + "They hold the same tools you do.".to_string() + } else { + format!("Their tools are scoped to: {}.", tools.join(", ")) + }; Ok(ToolResult::success(format!( - "Added {name} (id `{id}`) as {role} to the team. They'll be reachable as a teammate starting next turn." + "Added {name} (id `{id}`) as {role} to the team. {scope} They'll be reachable as a teammate starting next turn." ))) } } @@ -1931,6 +2051,9 @@ pub fn orchestrator_tools( store: Arc, workflow_refs: WorkflowRefQueue, run_outputs: RunOutputCache, + minter: String, + minter_tools: Vec, + minter_grants: Vec, ) -> Vec> { let mut tools: Vec> = vec![Box::new(QueryCompanyTool::new( company.clone(), @@ -1967,7 +2090,13 @@ pub fn orchestrator_tools( events, workflow_refs, ))); - tools.push(Box::new(AddAgentTool::new(company, store))); + tools.push(Box::new(AddAgentTool::new( + company, + store, + minter, + minter_tools, + minter_grants, + ))); tools } @@ -4868,7 +4997,7 @@ name = "Morning" async fn add_agent_tool_persists_an_overlay_teammate() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(json!({ @@ -4901,6 +5030,129 @@ name = "Morning" ); } + /// A minter scoped to part of the company grant, for the #619 tests below. + /// `minter_tools` is the line it declares; `minter_grants` is that line + /// already narrowed by the company `allow` — what `build_agent` hands the + /// tool. + fn scoped_add_agent(company: CompanyId, store: Arc) -> AddAgentTool { + AddAgentTool::new( + company, + store, + "ceo".to_string(), + vec!["workspace".to_string()], + vec!["workspace".to_string()], + ) + } + + /// Issue #619: a teammate minted by a **scoped** agent inherits that + /// agent's line, not the company's whole grant. + /// + /// #661 clamped an explicit `tools` argument to the company grant, which + /// leaves this open: omitting `tools` still yields the company's *entire* + /// grant, so a narrowly scoped agent could mint a teammate holding + /// everything the company holds. `add_agent` is `Reach::Nothing` and never + /// asks, so nothing else in the path would catch it. + #[tokio::test] + async fn a_minted_teammate_is_bounded_by_its_minter_not_the_company() { + let company = CompanyId::new("acme"); + let store = Arc::new(MemStore::seeded(seeded_record(&company))); + let tool = scoped_add_agent(company.clone(), store.clone()); + + let result = tool + .execute(json!({ "name": "Jamie", "role": "Growth Lead" })) + .await + .expect("execute"); + assert!(!result.is_error, "got {:?}", result.text()); + + let record = store.load(&company).await.unwrap().expect("persisted"); + assert_eq!( + record.overlay_agents[0].tools, + vec!["workspace".to_string()], + "the minted teammate must be bounded by the agent that minted it, \ + not by the company" + ); + } + + /// An **unscoped** minter still mints an unscoped teammate — the pre-#619 + /// behaviour, kept deliberately. Copying the minter's *line* rather than + /// its resolved grant is what keeps the teammate tracking `[tools].allow` + /// instead of freezing today's copy of it into the record. + #[tokio::test] + async fn an_unscoped_minter_mints_an_unscoped_teammate() { + let company = CompanyId::new("acme"); + let store = Arc::new(MemStore::seeded(seeded_record(&company))); + let tool = unscoped_add_agent(company.clone(), store.clone()); + + let result = tool + .execute(json!({ "name": "Jamie", "role": "Growth Lead" })) + .await + .expect("execute"); + assert!(!result.is_error, "got {:?}", result.text()); + + let record = store.load(&company).await.unwrap().expect("persisted"); + assert!( + record.overlay_agents[0].tools.is_empty(), + "an empty line means the company's standard grant (#264), and a \ + minter holding that grant hands on exactly it" + ); + } + + /// An explicit `tools` request is narrowed against what the **minter** + /// holds, so the tool cannot hand out a grant its caller does not have. + #[tokio::test] + async fn an_explicit_scope_is_narrowed_to_what_the_minter_holds() { + let company = CompanyId::new("acme"); + let store = Arc::new(MemStore::seeded(seeded_record(&company))); + let tool = scoped_add_agent(company.clone(), store.clone()); + + let result = tool + .execute(json!({ + "name": "Jamie", + "role": "Growth Lead", + "tools": ["workspace", "composio"] + })) + .await + .expect("execute"); + assert!(!result.is_error, "got {:?}", result.text()); + + let record = store.load(&company).await.unwrap().expect("persisted"); + assert_eq!( + record.overlay_agents[0].tools, + vec!["workspace".to_string()], + "`composio` is outside the minter's own grant and must be dropped" + ); + } + + /// A request that narrows to **nothing** is a refusal, not a stored empty + /// list. + /// + /// This is the sharp edge: an empty `tools` list means "inherit the + /// company's standard grant". Storing the empty result of a narrowing + /// would turn the most deliberate narrowing an agent can ask for into the + /// widest grant in the company — the exact inversion #619 exists to remove. + #[tokio::test] + async fn a_scope_entirely_outside_the_minters_grant_is_refused() { + let company = CompanyId::new("acme"); + let store = Arc::new(MemStore::seeded(seeded_record(&company))); + let tool = scoped_add_agent(company.clone(), store.clone()); + + let result = tool + .execute(json!({ + "name": "Jamie", + "role": "Growth Lead", + "tools": ["composio"] + })) + .await + .expect("execute"); + assert!(result.is_error, "got {:?}", result.text()); + + let record = store.load(&company).await.unwrap().expect("persisted"); + assert!( + record.overlay_agents.is_empty(), + "and no teammate was written at all, scoped or otherwise" + ); + } + /// Issue #661 / L5: `add_agent` carries a per-teammate tool grant onto the /// overlay record, trimming and dropping blank globs. The grant is narrowed /// against `[tools].allow` later (at roster build); persistence keeps the @@ -4909,7 +5161,7 @@ name = "Morning" async fn add_agent_tool_persists_a_tool_grant() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(json!({ @@ -4935,7 +5187,7 @@ name = "Morning" async fn add_agent_tool_empty_tools_is_the_standard_grant() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(json!({ "name": "Ravi", "role": "Researcher", "tools": [] })) @@ -4954,7 +5206,7 @@ name = "Morning" async fn add_agent_tool_rejects_a_non_string_tool() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); assert!( tool.execute(json!({ "name": "Ravi", "role": "Researcher", "tools": [123] })) @@ -4984,7 +5236,7 @@ name = "Morning" async fn add_agent_tool_mints_a_readable_id_and_reports_it() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(json!({ "name": "Dana Designer", "role": "Designer" })) @@ -5009,7 +5261,7 @@ name = "Morning" async fn add_agent_tool_still_refuses_a_duplicate_display_name() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); for _ in 0..1 { let first = tool @@ -5051,7 +5303,7 @@ name = "Morning" ) .expect("valid manifest"); let store = Arc::new(MemStore::seeded(record)); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); let result = tool .execute(json!({ "name": "Backend Engineer", "role": "Platform" })) @@ -5067,7 +5319,7 @@ name = "Morning" async fn add_agent_tool_requires_name_and_role() { let company = CompanyId::new("acme"); let store = Arc::new(MemStore::seeded(seeded_record(&company))); - let tool = AddAgentTool::new(company.clone(), store.clone()); + let tool = unscoped_add_agent(company.clone(), store.clone()); assert!( tool.execute(json!({ "role": "Growth Lead" })) @@ -5090,7 +5342,7 @@ name = "Morning" async fn add_agent_tool_reports_company_not_found() { let company = CompanyId::new("ghost"); let store: Arc = Arc::new(MemStore::default()); - let tool = AddAgentTool::new(company, store); + let tool = unscoped_add_agent(company, store); let err = tool .execute(json!({ "name": "Jamie", "role": "Growth Lead" })) @@ -5214,6 +5466,9 @@ name = "Morning" Arc::new(MemStore::default()), WorkflowRefQueue::default(), RunOutputCache::default(), + "ceo".to_string(), + Vec::new(), + vec!["fs:*".to_string()], ); let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); // Six before #186; `assign_task` + `review_task` made eight; #418's diff --git a/src/server/ops/mcp.rs b/src/server/ops/mcp.rs index a4a38a1af..7a47793ed 100644 --- a/src/server/ops/mcp.rs +++ b/src/server/ops/mcp.rs @@ -266,11 +266,12 @@ fn dto_from_decl( /// roster walk beside this one is exactly how the two consoles would come to /// disagree with each other and with the harness. The roster is exactly what the harness builds in /// `build_roster`: the manifest agents (each with its own `tools` narrowed by -/// the company `allow`), plus the promoted overlay teammates. An overlay -/// teammate has no manifest `tools` row, so it inherits the full company `allow` -/// — the standard grant `overlay_agent_to_manifest` gives it — and an overlay id -/// already claimed by a manifest agent is skipped, both mirroring the harness so -/// console reachability equals what an agent is actually granted. +/// the company `allow`), plus the promoted overlay teammates — each narrowed by +/// **its own** `tools` line the same way (issue #661), which for the common +/// empty line is still the full company `allow`, the standard grant +/// `overlay_agent_to_manifest` gives it. An overlay id already claimed by a +/// manifest agent is skipped, both mirroring the harness so console +/// reachability equals what an agent is actually granted. pub(super) fn roster_grants(record: &CompanyRecord) -> Vec<(String, Vec)> { let allow = &record.manifest.tools.allow; let mut grants: Vec<(String, Vec)> = record @@ -294,9 +295,21 @@ pub(super) fn roster_grants(record: &CompanyRecord) -> Vec<(String, Vec) if manifest_ids.contains(overlay.id.as_str()) { continue; } - // No manifest tools row → the company's standard grant (empty `tools` - // ⇒ inherit `allow`), matching `overlay_agent_to_manifest`. - grants.push((overlay.id.clone(), agent_effective_grants(allow, &[]))); + // The overlay teammate's **own** tools line, read through the same + // function and with the same empty-means-inherit rule as the manifest + // half above — matching `overlay_agent_to_manifest` (issue #740). + // + // This read was hard-coded empty until #661 gave `OverlayAgent` a tools + // list. The comment that stood here ("no manifest tools row → the + // company's standard grant") described a fact that expired with that + // change, which is why it read as a decision rather than a stale + // assumption: a scoped teammate reported as reaching every enabled + // server, and the console asserted a connection the harness does not + // grant. + grants.push(( + overlay.id.clone(), + agent_effective_grants(allow, &overlay.tools), + )); } grants } @@ -865,3 +878,101 @@ async fn discover_tools( let _ = (company, name); crate::server::ops::not_wired("mcp tool discovery") } + +#[cfg(test)] +mod tests { + use super::*; + use crate::company::CompanyManifest; + use crate::ports::types::{CompanyId, OverlayAgent}; + + /// A company allowing two MCP families, with one manifest agent that lists + /// none (so it inherits both). + fn record(overlay_agents: Vec) -> CompanyRecord { + let manifest: CompanyManifest = toml::from_str( + r#" +[company] +name = "Acme" + +[tools] +allow = ["mcp:notion", "mcp:linear"] + +[[agent]] +id = "ceo" +role = "Chief Executive" +"#, + ) + .expect("manifest parses"); + CompanyRecord { + id: CompanyId::new("acme"), + manifest, + ledger: Vec::new(), + lifecycle: "running".to_string(), + overlay_agents, + overlay_desk_members: Vec::new(), + overlay_desk_order: Vec::new(), + overlay_desks: Vec::new(), + overlay_workflows: Vec::new(), + overlay_budgets: Vec::new(), + overlay_policy: None, + disabled_workflows: Vec::new(), + template_provenance: None, + } + } + + fn teammate(id: &str, tools: Vec<&str>) -> OverlayAgent { + OverlayAgent { + id: id.to_string(), + name: id.to_string(), + role: "Growth".to_string(), + description: None, + tools: tools.into_iter().map(str::to_string).collect(), + } + } + + /// Issue #740: a scoped overlay teammate must not read back here as + /// reaching everything. + /// + /// `roster_grants` is what every MCP server row's `reachableBy` is computed + /// from. #661 gave `OverlayAgent` a tools list and taught two of the three + /// readers to honour it; this one still passed an empty grant, so a + /// teammate scoped to one server reported as reaching all of them — the + /// console asserting a connection the harness does not grant. + #[test] + fn a_scoped_overlay_teammate_does_not_read_back_as_reaching_everything() { + let scoped = record(vec![teammate("jamie", vec!["mcp:notion"])]); + let grants = roster_grants(&scoped); + let jamie = grants + .iter() + .find(|(id, _)| id == "jamie") + .expect("the overlay teammate is on the roster"); + assert_eq!( + jamie.1, + vec!["mcp:notion".to_string()], + "a scoped teammate reaches only what it was scoped to" + ); + + // The manifest agent lists nothing and still inherits everything, so + // the narrowing above is the teammate's own and not a company change. + let ceo = grants + .iter() + .find(|(id, _)| id == "ceo") + .expect("on roster"); + assert_eq!( + ceo.1, + vec!["mcp:notion".to_string(), "mcp:linear".to_string()] + ); + } + + /// The empty-means-inherit rule (#264) is untouched: a teammate written + /// before #661, and every teammate created without a scope, still reads + /// back holding the company's whole grant. + #[test] + fn an_unscoped_overlay_teammate_still_inherits_the_company_grant() { + let grants = roster_grants(&record(vec![teammate("jamie", Vec::new())])); + let jamie = grants.iter().find(|(id, _)| id == "jamie").expect("roster"); + assert_eq!( + jamie.1, + vec!["mcp:notion".to_string(), "mcp:linear".to_string()] + ); + } +} diff --git a/src/server/ops/team_agent.rs b/src/server/ops/team_agent.rs index 55d079e85..a3dbff359 100644 --- a/src/server/ops/team_agent.rs +++ b/src/server/ops/team_agent.rs @@ -62,7 +62,8 @@ //! save. use axum::Json; -use axum::extract::Path; +use axum::extract::{Path, State}; +use axum::http::HeaderMap; use axum::response::{IntoResponse, Response}; use axum::routing::{self, MethodRouter}; use serde::{Deserialize, Serialize}; @@ -76,6 +77,7 @@ use crate::server::error::ApiError; use crate::server::ops::ScopedCompany; use crate::server::ops::language; use crate::server::ops::team::{AgentPath, daily_spend_samples, double_option}; +use crate::server::users::admin::require_admin; /// The `{scope}/team/{agent_id}` fragment: read one agent, edit one agent. /// @@ -100,7 +102,17 @@ pub(super) enum AgentSource { /// The fields a `PATCH` accepts for an overlay teammate. Sent to the console so /// it renders the same rule the host enforces. -const OVERLAY_EDITABLE: [&str; 3] = ["name", "role", "description"]; +const OVERLAY_EDITABLE: [&str; 4] = ["name", "role", "description", "tools"]; + +/// The subset a **non-admin** member may `PATCH` (issue #619). +/// +/// `tools` is admin-only because an empty list means "the company's standard +/// grant", which makes a `tools` edit a potential *widening* — see +/// [`edit_agent`]. The list is actor-dependent for the reason the module note +/// gives: a console renders a field read-only exactly when the host says it is, +/// so offering `tools` to a member who would meet a `403` on save is precisely +/// the drift `editable` exists to remove. +const OVERLAY_EDITABLE_MEMBER: [&str; 3] = ["name", "role", "description"]; /// One agent, in full — everything #264 lists as unreachable. #[derive(Debug, Serialize)] @@ -289,33 +301,88 @@ pub(super) struct EditAgent { role: Option, #[serde(default, deserialize_with = "double_option")] description: Option>, + /// The teammate's tool scope (issue #619). Absent leaves it alone; an + /// **empty array** is the deliberate way back to the company's standard + /// grant, which is why this is a plain `Option` and not a double option — + /// `[]` already spells "clear it" without needing `null` to mean something + /// different from omission. + /// + /// #661 made a teammate scopable at *creation* (`POST …/team` and + /// `add_agent`). This is the half that was missing: narrowing one that + /// already exists, without deleting and recreating it — which would orphan + /// its workspace folder, budget row, desk memberships and inbox. + #[serde(default)] + tools: Option>, } /// `GET {scope}/team/{agent_id}` — one agent, read. async fn agent_detail( company: ScopedCompany, + State(state): State, + headers: HeaderMap, Path(AgentPath { agent_id }): Path, ) -> Result, ApiError> { + // Only to decide what `editable` may claim — the read itself is open to any + // member, unchanged. A principal this cannot resolve reads as not-admin, + // which is fail-closed in the right direction: it under-claims what the + // caller may edit rather than over-claiming it. + let is_admin = is_admin_actor(&headers, &state, &company).await; let record = company .runtime .store() .load(company.id()) .await? .ok_or_else(|| OpenCompanyError::CompanyNotFound(company.id().to_string()))?; - detail(&company, &record, &agent_id).await + detail(&company, &record, &agent_id, is_admin).await } /// `PATCH {scope}/team/{agent_id}` — edit an overlay teammate. /// /// Refuses a manifest teammate with a `409` naming where the edit belongs, and -/// an unknown id with a `404`. Open to any signed-in member, matching `POST -/// …/team`: defining a teammate was never admin-only, so correcting one it -/// defined is not either. Setting a *budget* still is, on its own route. +/// an unknown id with a `404`. `name`, `role` and `description` are open to any +/// signed-in member, matching `POST …/team`: defining a teammate was never +/// admin-only, so correcting one it defined is not either. +/// +/// # Why `tools` is the exception (issue #619) +/// +/// That reasoning covers what a teammate *is*. It does not cover what a +/// teammate may *do*, and a tool grant is the second thing — the +/// [`AdminScopedCompany`](super::AdminScopedCompany) axis: a write that settles +/// something *on behalf of* the company rather than one a member makes for +/// themselves. +/// +/// The sharp edge is that **an empty `tools` list means "inherit the company's +/// standard grant"** — the widest grant the company has. So `{"tools": []}` is +/// not a small edit, it is a *widening*, and left member-open it would let any +/// signed-in member hand a deliberately-scoped teammate the company's whole +/// grant back. That is the exact inversion this field was added to prevent, and +/// `add_agent` already refuses its own version of it (a narrowing that lands +/// empty is a hard error there, never a stored empty list). +/// +/// So the admin check is **conditional on the field being present**, in the +/// same shape and for the same reason as the cap on +/// [`add_member`](super::team): a member who edits a name or a role keeps +/// working exactly as before, and adding this field must not quietly take an +/// existing capability away from members. +/// +/// Narrow-only-for-members was considered and rejected: it makes the scope a +/// one-way ratchet, so a teammate scoped too tightly could never be loosened by +/// anyone, and the only way back would be delete-and-recreate — which orphans +/// the workspace folder, budget row, desk memberships and inbox this route +/// exists to preserve. async fn edit_agent( company: ScopedCompany, + State(state): State, + headers: HeaderMap, Path(AgentPath { agent_id }): Path, Json(body): Json, ) -> Result, Response> { + // Authority before the write lock: a refused edit must not hold the lock, + // and must not have looked at the record either. + if body.tools.is_some() { + require_admin(&headers, &state, &company.runtime).await?; + } + // Serialize with every other write to `overlay_agents`, so a console edit // and a concurrent `add_agent` cannot clobber one another's roster. let write_lock = company_write_lock(company.id()); @@ -348,6 +415,11 @@ async fn edit_agent( let name = trimmed_field(body.name.as_deref(), "name").map_err(|e| e.into_response())?; let role = trimmed_field(body.role.as_deref(), "role").map_err(|e| e.into_response())?; + let tools = body + .tools + .map(|globs| trimmed_globs(&globs)) + .transpose() + .map_err(|e| e.into_response())?; { let agent = record @@ -369,6 +441,15 @@ async fn edit_agent( .map(|text| text.trim().to_string()) .filter(|text| !text.is_empty()); } + // Issue #619: stored verbatim, exactly like a manifest `[[agent]].tools` + // line. The company `allow` ceiling is applied at *read* time by + // `agent_effective_grants`, so a glob the company does not cover is + // surfaced as asked-for-but-not-granted rather than silently dropped + // here — and this route can only ever narrow a teammate within a grant + // the company already made. + if let Some(tools) = tools { + agent.tools = tools; + } } company @@ -378,7 +459,11 @@ async fn edit_agent( .await .map_err(|e| ApiError(e).into_response())?; - detail(&company, &record, &agent_id) + // The caller either passed `require_admin` above or sent no `tools`, so + // re-resolve rather than assume: an admin editing only a name must still + // read back `tools` as editable. + let is_admin = is_admin_actor(&headers, &state, &company).await; + detail(&company, &record, &agent_id, is_admin) .await .map_err(|e| e.into_response()) } @@ -408,12 +493,54 @@ fn trimmed_field(value: Option<&str>, field: &str) -> Result, Api Ok(Some(trimmed.to_string())) } +/// Trims a submitted tool-scope list, refusing a blank entry and dropping +/// duplicates (issue #619). +/// +/// A blank glob is a `400` rather than a stored empty string for a sharper +/// reason than tidiness: `""` matches nothing an operator meant, so it would +/// read as a scope that grants nothing while looking like a scope that was set. +/// Duplicates are dropped rather than refused — a repeated glob is harmless and +/// the resolved grant list is de-duplicated downstream anyway. +/// +/// Same `ApiError`-not-`Response` return shape as [`trimmed_field`], for the +/// reason given there. +fn trimmed_globs(globs: &[String]) -> Result, ApiError> { + let mut seen = std::collections::HashSet::new(); + let mut out = Vec::with_capacity(globs.len()); + for glob in globs { + let trimmed = glob.trim(); + if trimmed.is_empty() { + return Err(ApiError(OpenCompanyError::InvalidRequest( + "a tool grant can't be empty. Send an empty list to give this teammate the company's standard grant.".to_string(), + ))); + } + if seen.insert(trimmed.to_string()) { + out.push(trimmed.to_string()); + } + } + Ok(out) +} + +/// Whether the signed-in caller may administer this company — the question +/// [`OVERLAY_EDITABLE`] keys off, asked without refusing. +/// +/// [`require_admin`] is the enforcement path and returns a `Response` on +/// failure, which is right for a write and wrong for a read that must still +/// succeed for a member. This answers the same question through the same +/// `may_administer` predicate, so the two cannot drift. +async fn is_admin_actor(headers: &HeaderMap, state: &AppState, company: &ScopedCompany) -> bool { + crate::server::users::routes::current_user(headers, state, company.id()) + .await + .is_some_and(|user| user.may_administer()) +} + /// Builds one agent's detail from the loaded record, or 404s when the id names /// nobody on the roster. async fn detail( company: &ScopedCompany, record: &CompanyRecord, agent_id: &str, + is_admin: bool, ) -> Result, ApiError> { let manifest_agent = record.manifest.agents.iter().find(|a| a.id == agent_id); let overlay_agent = record.overlay_agents.iter().find(|a| a.id == agent_id); @@ -466,9 +593,10 @@ async fn detail( role, description, source, - editable: match source { - AgentSource::Overlay => OVERLAY_EDITABLE.to_vec(), - AgentSource::Manifest => Vec::new(), + editable: match (source, is_admin) { + (AgentSource::Overlay, true) => OVERLAY_EDITABLE.to_vec(), + (AgentSource::Overlay, false) => OVERLAY_EDITABLE_MEMBER.to_vec(), + (AgentSource::Manifest, _) => Vec::new(), }, tier: declared_tier(record, agent_id), is_orchestrator: is_orchestrator(record, agent_id), @@ -706,6 +834,39 @@ members = ["writer", "ceo"] .await } + /// Drives the route as a specific principal. The harness signs every other + /// request in as an admin, which is exactly why this exists: an + /// authority check verified only as an admin passes identically against no + /// check at all. + async fn send_as( + state: &AppState, + method: &str, + uri: &str, + body: Option, + cookie: String, + ) -> (StatusCode, Value) { + let builder = Request::builder() + .method(method) + .uri(uri) + .header("cookie", cookie); + let request = match &body { + Some(value) => builder + .header("content-type", "application/json") + .body(Body::from(serde_json::to_vec(value).unwrap())) + .unwrap(), + None => builder.body(Body::empty()).unwrap(), + }; + let response = router(state.clone()).oneshot(request).await.unwrap(); + let status = response.status(); + let bytes = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + let value = if bytes.is_empty() { + Value::Null + } else { + serde_json::from_slice(&bytes).unwrap_or(Value::Null) + }; + (status, value) + } + /// Adds a teammate through the console's own route and returns its id. async fn add_overlay(state: &AppState, name: &str, role: &str) -> String { let (status, created) = send( @@ -1122,11 +1283,225 @@ members = ["writer", "ceo"] let (_, agent) = get_agent(&state, &jamie).await; assert_eq!( strings(&agent["editable"]), - vec!["name", "role", "description"], + vec!["name", "role", "description", "tools"], "{agent}" ); } + /// Issue #619: a teammate can be narrowed **after** it exists, not only at + /// creation. + /// + /// #661 made the scope writable on `POST …/team` and through `add_agent`. + /// This is the half that was missing — without it, correcting a teammate's + /// grant means deleting and recreating it, which orphans its workspace + /// folder, budget row, desk memberships and inbox. + /// + /// The three levels are asserted separately on purpose: `requested` proves + /// the scope was stored, `effective` proves it reached the function the + /// harness builds the agent with, and the untouched company `allow` proves + /// the narrowing is per-teammate rather than a company-wide edit. + #[tokio::test] + async fn an_overlay_teammate_can_be_scoped_after_creation() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + let jamie = add_overlay(&state, "Jamie", "Growth").await; + + let (_, before) = get_agent(&state, &jamie).await; + assert!( + strings(&before["tools"]["requested"]).is_empty(), + "unscoped to begin with: {before}" + ); + assert_eq!( + strings(&before["tools"]["effective"]), + vec!["workspace", "workspace.*", "composio"], + "which resolves to everything the company allows: {before}" + ); + + let (status, scoped) = patch_agent(&state, &jamie, json!({"tools": ["workspace"]})).await; + assert_eq!(status, StatusCode::OK, "{scoped}"); + assert_eq!( + strings(&scoped["tools"]["requested"]), + vec!["workspace"], + "{scoped}" + ); + assert_eq!( + strings(&scoped["tools"]["effective"]), + vec!["workspace"], + "and it is narrower than the company grant, which is the point: {scoped}" + ); + assert_eq!( + strings(&scoped["tools"]["companyAllow"]), + vec!["workspace", "workspace.*", "composio"], + "the company ceiling is untouched — this scoped one teammate: {scoped}" + ); + + // Read back through a fresh request, so this is the stored record and + // not the handler's own answer. + let (_, reread) = get_agent(&state, &jamie).await; + assert_eq!( + strings(&reread["tools"]["requested"]), + vec!["workspace"], + "{reread}" + ); + + // An empty list is the deliberate way back to the standard grant, and + // must read as "inherits everything" rather than "holds nothing". + let (status, cleared) = patch_agent(&state, &jamie, json!({"tools": []})).await; + assert_eq!(status, StatusCode::OK, "{cleared}"); + assert!( + strings(&cleared["tools"]["requested"]).is_empty(), + "{cleared}" + ); + assert_eq!( + strings(&cleared["tools"]["effective"]), + vec!["workspace", "workspace.*", "composio"], + "{cleared}" + ); + } + + /// **The review finding (#745).** A member must not be able to widen a + /// teammate's scope — and because an empty list means "the company's + /// standard grant", `{"tools": []}` is the widest possible widening. + /// + /// This is #619's own defect reachable through the route added to fix it: + /// `add_agent` refuses a narrowing that lands empty precisely because an + /// empty list inherits everything, and leaving `edit_agent` member-open + /// would have let any signed-in member undo any scoping with one call. + /// + /// The two-account shape is the point: the harness signs every other + /// request in as an admin, so a check verified only as an admin passes + /// identically against no check at all. + #[tokio::test] + async fn a_member_cannot_widen_a_teammates_scope() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + crate::server::test_support::seed_fixed_member(&state, "acme").await; + let jamie = add_overlay(&state, "Jamie", "Growth").await; + + // Scoped by an admin. + let (status, _) = patch_agent(&state, &jamie, json!({"tools": ["workspace"]})).await; + assert_eq!(status, StatusCode::OK); + + let uri = format!("/api/v1/company/team/{jamie}"); + let member = || crate::server::test_support::member_cookie("acme"); + + // The widening a member must not be able to perform. + let (status, refusal) = + send_as(&state, "PATCH", &uri, Some(json!({"tools": []})), member()).await; + assert_eq!( + status, + StatusCode::FORBIDDEN, + "an empty list is the company's whole grant: {refusal}" + ); + + // …and neither may a member set a different scope at all. + let (status, _) = send_as( + &state, + "PATCH", + &uri, + Some(json!({"tools": ["composio"]})), + member(), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN); + + // Nothing was written by either attempt. + let (_, unchanged) = get_agent(&state, &jamie).await; + assert_eq!( + strings(&unchanged["tools"]["requested"]), + vec!["workspace"], + "the scope an admin set must survive both refusals: {unchanged}" + ); + } + + /// The conditional check must not take an existing capability away: a + /// member editing a name or a role keeps working exactly as before, which + /// is the same rule `POST …/team` applies to its budget cap. + #[tokio::test] + async fn a_member_may_still_edit_a_teammates_name_and_role() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + crate::server::test_support::seed_fixed_member(&state, "acme").await; + let jamie = add_overlay(&state, "Jamie", "Growth").await; + + let (status, edited) = send_as( + &state, + "PATCH", + &format!("/api/v1/company/team/{jamie}"), + Some(json!({"name": "Jamie R", "role": "Head of Growth"})), + crate::server::test_support::member_cookie("acme"), + ) + .await; + assert_eq!(status, StatusCode::OK, "{edited}"); + assert_eq!(edited["name"], "Jamie R", "{edited}"); + assert_eq!(edited["role"], "Head of Growth", "{edited}"); + } + + /// `editable` is the host stating the rule so the console does not + /// re-derive it. It therefore has to answer per **actor**, or a member is + /// offered a `tools` field whose save is a `403` — the drift this list + /// exists to remove. + #[tokio::test] + async fn editable_names_tools_only_for_an_admin() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + crate::server::test_support::seed_fixed_member(&state, "acme").await; + let jamie = add_overlay(&state, "Jamie", "Growth").await; + + let (_, as_admin) = get_agent(&state, &jamie).await; + assert_eq!( + strings(&as_admin["editable"]), + vec!["name", "role", "description", "tools"], + "{as_admin}" + ); + + let (_, as_member) = send_as( + &state, + "GET", + &format!("/api/v1/company/team/{jamie}"), + None, + crate::server::test_support::member_cookie("acme"), + ) + .await; + assert_eq!( + strings(&as_member["editable"]), + vec!["name", "role", "description"], + "a member is not offered a field they cannot save: {as_member}" + ); + } + + /// A blank glob is refused rather than stored: `""` matches nothing an + /// operator meant, so it would read as a scope that grants nothing while + /// looking like a scope that was set. + #[tokio::test] + async fn a_blank_tool_glob_is_refused() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + let jamie = add_overlay(&state, "Jamie", "Growth").await; + + let (status, refusal) = + patch_agent(&state, &jamie, json!({"tools": ["workspace", " "]})).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{refusal}"); + + let (_, unchanged) = get_agent(&state, &jamie).await; + assert!( + strings(&unchanged["tools"]["requested"]).is_empty(), + "and nothing was written: {unchanged}" + ); + } + + /// A manifest teammate's tool line lives in the version-controlled + /// blueprint, and #619 did not move it: the overlay half became editable, + /// the manifest half stayed a `409`. + #[tokio::test] + async fn a_manifest_teammates_tools_are_still_not_editable_here() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), ROSTER).await; + + let (status, refusal) = patch_agent(&state, "ceo", json!({"tools": ["workspace"]})).await; + assert_eq!(status, StatusCode::CONFLICT, "{refusal}"); + } + /// A blank name would render a card with no way back to it, so it is a /// refusal rather than a stored blank. Whitespace is trimmed, not accepted. #[tokio::test]