diff --git a/desktop/src-tauri/src/commands/personas/inbound.rs b/desktop/src-tauri/src/commands/personas/inbound.rs index cbb2314353..4842207e99 100644 --- a/desktop/src-tauri/src/commands/personas/inbound.rs +++ b/desktop/src-tauri/src/commands/personas/inbound.rs @@ -117,6 +117,12 @@ fn reconcile_inbound_persona_event_blocking( if let Some(managed_agent) = &inbound_managed_agent { validate_inbound_managed_agent_definition(managed_agent)?; } + let inbound_team = (kind == KIND_TEAM) + .then(|| team_content_from_event(&event)) + .transpose()?; + if let Some(team) = &inbound_team { + validate_inbound_team_definition(team)?; + } let d_tag = match &inbound_persona { Some(persona) => persona_d_tag(persona), None => event_d_tag(&event)?, @@ -168,7 +174,8 @@ fn reconcile_inbound_persona_event_blocking( } KIND_TEAM => { let mut teams = load_teams(&app)?; - apply_inbound_team(&mut teams, d_tag, team_content_from_event(&event)?); + // Parsed and validated above, before retention. + apply_inbound_team(&mut teams, d_tag, inbound_team.expect("team parsed above")); save_teams(&app, &teams)?; } KIND_MANAGED_AGENT => { @@ -198,6 +205,19 @@ fn validate_inbound_persona_definition(persona: &AgentDefinition) -> Result<(), .map_err(|error| format!("Inbound persona definition is unsafe: {error}")) } +/// Team `instructions` are runtime-layered into every member deployment, so an +/// inbound team carries executable text under the same review contract as a +/// persona. The wire type is a double option: absent means "publisher predates +/// always-publish, preserve local" and `null` means "explicitly cleared" -- +/// neither delivers text to validate. +fn validate_inbound_team_definition(team: &TeamEventContent) -> Result<(), String> { + crate::managed_agents::validate_team_definition_text( + &team.name, + team.instructions.clone().flatten().as_deref(), + ) + .map_err(|error| format!("Inbound team definition is unsafe: {error}")) +} + fn validate_inbound_managed_agent_definition( managed_agent: &ManagedAgentEventContent, ) -> Result<(), String> { diff --git a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs index e65973f149..6f2eab4884 100644 --- a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs @@ -733,3 +733,49 @@ fn inbound_definition_less_agent_accepts_visible_multiline_prompt() { assert!(validate_inbound_managed_agent_definition(&inbound).is_ok()); } + +// ── Inbound team definition gate ───────────────────────────────────────── +// +// Team `instructions` are runtime-layered into every member deployment, so an +// inbound kind:30176 carries executable text. It is now parsed and validated +// alongside persona and managed-agent content -- before retention -- so an +// unsafe team stays out of both the retention database and the local store. + +#[test] +fn inbound_team_with_concealed_instructions_is_rejected() { + let mut content = team_content("Release Team"); + content.instructions = Some(Some("Ship it.\u{200B}".to_string())); + + let error = validate_inbound_team_definition(&content).unwrap_err(); + assert!( + error.starts_with("Inbound team definition is unsafe"), + "{error}" + ); + assert!(error.contains("U+200B"), "{error}"); +} + +#[test] +fn inbound_team_with_a_concealed_name_is_rejected() { + let content = team_content("Release\u{202E} Team"); + assert!(validate_inbound_team_definition(&content).is_err()); +} + +#[test] +fn an_ordinary_inbound_team_is_accepted() { + assert!(validate_inbound_team_definition(&team_content("Release Team")).is_ok()); +} + +#[test] +fn inbound_team_omitting_instructions_carries_no_text_to_validate() { + // Absent means "publisher predates always-publish, preserve local" and + // `null` means "explicitly cleared". Neither delivers text, and neither may + // be rejected as if it had. + assert!( + validate_inbound_team_definition(&team_content_omitting_optional_fields("Release Team")) + .is_ok() + ); + + let mut cleared = team_content("Release Team"); + cleared.instructions = Some(None); + assert!(validate_inbound_team_definition(&cleared).is_ok()); +} diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 97cd11933d..0eb6056230 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -164,6 +164,17 @@ pub(crate) fn build_import_team( if name.is_empty() { return Err("Team snapshot name is empty.".to_string()); } + // Same review contract the agent-snapshot path applies to its definition + // text (`agent_snapshot.rs`). A snapshot is a file from outside this + // install, and a team's instructions are runtime-layered into every member + // deployment -- so this is the one place they are reviewed before they can + // execute. Phase 1 runs before key minting and before any store write, so a + // rejection here leaves nothing behind. + crate::managed_agents::validate_team_definition_text( + name, + snapshot.team.instructions.as_deref(), + ) + .map_err(|error| format!("Team snapshot is unsafe: {error}"))?; Ok(TeamRecord { id: Uuid::new_v4().to_string(), diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index c9a6d8812a..0621fef98b 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -761,3 +761,59 @@ mod egress_guard_boundary { assert!(err.contains("key-backup material"), "{err}"); } } + +// ── Snapshot text review contract ──────────────────────────────────────── +// +// A team snapshot is a file from outside this install and its `instructions` +// are runtime-layered into every member deployment, so import is the one place +// they are reviewed before they can execute. The agent-snapshot path already +// validates its definition text; this covers the team wrapper. + +fn snapshot_with_team_text(name: &str, instructions: Option<&str>) -> TeamSnapshot { + let mut snap = snapshot(vec![member("Alice")]); + snap.team.name = name.to_string(); + snap.team.instructions = instructions.map(str::to_string); + snap +} + +fn import_team(snap: &TeamSnapshot) -> Result { + build_import_team(snap, vec!["persona-1".to_string()], "now") +} + +#[test] +fn a_snapshot_with_concealed_instructions_is_refused() { + let snap = snapshot_with_team_text("Review Team", Some("Be thorough.\u{200B}")); + let error = import_team(&snap).unwrap_err(); + assert!(error.starts_with("Team snapshot is unsafe"), "{error}"); + assert!(error.contains("U+200B"), "{error}"); +} + +#[test] +fn a_snapshot_with_a_concealed_team_name_is_refused() { + let snap = snapshot_with_team_text("Review\u{202E} Team", Some("Be thorough.")); + assert!(import_team(&snap).is_err()); +} + +#[test] +fn an_ordinary_snapshot_still_imports() { + let snap = snapshot_with_team_text("Review Team", Some("Be thorough.\n\tCheck tests.")); + let team = import_team(&snap).unwrap(); + assert_eq!(team.name, "Review Team"); + assert_eq!( + team.instructions.as_deref(), + Some("Be thorough.\n\tCheck tests.") + ); +} + +#[test] +fn a_snapshot_carrying_no_instructions_still_imports() { + let team = import_team(&snapshot_with_team_text("Review Team", None)).unwrap(); + assert_eq!(team.instructions, None); +} + +#[test] +fn an_empty_team_name_is_still_refused_with_its_own_message() { + // The pre-existing emptiness check runs first and keeps its wording. + let error = import_team(&snapshot_with_team_text(" ", Some("Be thorough."))).unwrap_err(); + assert_eq!(error, "Team snapshot name is empty."); +} diff --git a/desktop/src-tauri/src/commands/teams.rs b/desktop/src-tauri/src/commands/teams.rs index 4377ddaa43..4387cf0137 100644 --- a/desktop/src-tauri/src/commands/teams.rs +++ b/desktop/src-tauri/src/commands/teams.rs @@ -5,7 +5,8 @@ use crate::{ app_state::AppState, managed_agents::{ delete_team_with_cascade, ensure_persona_ids_are_active, load_personas, load_teams, - save_teams, try_regenerate_nest, CreateTeamRequest, TeamRecord, UpdateTeamRequest, + save_teams, try_regenerate_nest, validate_team_definition_text, CreateTeamRequest, + TeamRecord, UpdateTeamRequest, }, util::now_iso, }; @@ -148,6 +149,9 @@ pub async fn create_team(input: CreateTeamRequest, app: AppHandle) -> Result Result Result<(), String> { - if display_name.trim().is_empty() { - return Err("Display name is required".to_string()); + validate_reviewed_text( + display_name, + "Display name", + system_prompt, + "Agent instructions", + ) +} + +/// Validate the human-reviewed text carried by a team. +/// +/// A team's `instructions` are runtime-layered into every member deployment, +/// so they are executable text under the same review contract as an agent +/// definition. A team carrying no instructions has no executable text and only +/// its name is checked. +pub(crate) fn validate_team_definition_text( + name: &str, + instructions: Option<&str>, +) -> Result<(), String> { + validate_reviewed_text( + name, + "Team name", + instructions.unwrap_or_default(), + "Team instructions", + ) +} + +/// Shared contract for reviewed-then-executed text. The labels differ per +/// surface so the error a person sees names the field they were editing; the +/// limits and the invisible-character rules are deliberately identical. +fn validate_reviewed_text( + name: &str, + name_label: &str, + instructions: &str, + instructions_label: &str, +) -> Result<(), String> { + if name.trim().is_empty() { + return Err(format!("{name_label} is required")); } - let display_name_chars = display_name.chars().count(); - if display_name_chars > MAX_DISPLAY_NAME_CHARS { + let name_chars = name.chars().count(); + if name_chars > MAX_DISPLAY_NAME_CHARS { return Err(format!( - "Display name is too long ({display_name_chars} characters, max {MAX_DISPLAY_NAME_CHARS})" + "{name_label} is too long ({name_chars} characters, max {MAX_DISPLAY_NAME_CHARS})" )); } - if system_prompt.len() > MAX_SYSTEM_PROMPT_BYTES { + if instructions.len() > MAX_SYSTEM_PROMPT_BYTES { return Err(format!( - "Agent instructions are too long ({} bytes, max {MAX_SYSTEM_PROMPT_BYTES})", - system_prompt.len() + "{instructions_label} are too long ({} bytes, max {MAX_SYSTEM_PROMPT_BYTES})", + instructions.len() )); } - validate_visible_text(display_name, "Display name", false)?; - validate_visible_text(system_prompt, "Agent instructions", true) + validate_visible_text(name, name_label, false)?; + validate_visible_text(instructions, instructions_label, true) } /// Validate the human-reviewed definition text carried by a managed agent. @@ -157,6 +192,76 @@ fn is_default_ignorable(character: char) -> bool { mod tests { use super::*; + // ── Team text: same contract, team-shaped labels ───────────────────────── + // + // Team `instructions` are runtime-layered into every member deployment, so + // they are executed exactly like an agent's `system_prompt`. Before this + // they were the one shared executable text with no review contract at all, + // which made the same hidden characters safer in a team than in the agent + // wrapped by it. + + #[test] + fn team_text_rejects_the_same_invisible_characters_as_an_agent() { + for character in [ + '\u{00AD}', + '\u{034F}', + '\u{200B}', + '\u{202E}', + '\u{2060}', + '\u{2066}', + '\u{3164}', + '\u{E007F}', + ] { + let name = format!("Release{character} Team"); + let instructions = format!("Ship the release.{character}"); + assert!(validate_team_definition_text(&name, Some("Ship it.")).is_err()); + assert!(validate_team_definition_text("Release Team", Some(&instructions)).is_err()); + } + } + + #[test] + fn team_text_accepts_ordinary_whitespace_and_emoji() { + assert!(validate_team_definition_text( + "Release Team 🚀", + Some("Ship the release.\n\tPost the ledger row 🚀") + ) + .is_ok()); + } + + #[test] + fn a_team_without_instructions_carries_no_executable_text() { + assert!(validate_team_definition_text("Release Team", None).is_ok()); + } + + #[test] + fn team_errors_name_the_team_field_the_person_was_editing() { + let name_error = validate_team_definition_text("", Some("Ship it.")).unwrap_err(); + assert!( + name_error.starts_with("Team name"), + "expected a team-shaped error, got {name_error}" + ); + + let long_instructions = "x".repeat(MAX_SYSTEM_PROMPT_BYTES + 1); + let instructions_error = + validate_team_definition_text("Release Team", Some(&long_instructions)).unwrap_err(); + assert!( + instructions_error.starts_with("Team instructions"), + "expected a team-shaped error, got {instructions_error}" + ); + } + + #[test] + fn agent_error_wording_is_unchanged_by_the_shared_contract() { + assert_eq!( + validate_agent_definition_text("", "Review code.").unwrap_err(), + "Display name is required" + ); + let long_prompt = "x".repeat(MAX_SYSTEM_PROMPT_BYTES + 1); + assert!(validate_agent_definition_text("Reviewer", &long_prompt) + .unwrap_err() + .starts_with("Agent instructions are too long")); + } + #[test] fn accepts_plain_multiline_instructions() { assert!(validate_agent_definition_text( diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index c6ccd3709c..489d976860 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -54,6 +54,7 @@ pub(crate) fn lock_path_mutex() -> std::sync::MutexGuard<'static, ()> { pub use backend::*; pub(crate) use definition_validation::{ validate_agent_definition_text, validate_managed_agent_definition_text, + validate_team_definition_text, }; pub use discovery::*; pub use env_vars::*;