fix: reject context commands for context-owning providers (#11094)
Co-authored-by: Douwe M Osinga <douwe@sidewalklabs.com>
This commit is contained in:
@@ -37,7 +37,9 @@ use anyhow::{Context, Result};
|
||||
use completion::GooseCompleter;
|
||||
use goose::agents::extension::{Envs, ExtensionConfig, PLATFORM_EXTENSIONS};
|
||||
use goose::agents::types::RetryConfig;
|
||||
use goose::agents::{Agent, SessionConfig, COMPACT_TRIGGERS};
|
||||
use goose::agents::{
|
||||
context_management_unsupported_message, Agent, SessionConfig, COMPACT_TRIGGERS,
|
||||
};
|
||||
use goose::config::extensions::name_to_key;
|
||||
use goose::config::{Config, GooseMode};
|
||||
use input::InputResult;
|
||||
@@ -1047,6 +1049,15 @@ impl CliSession {
|
||||
}
|
||||
|
||||
async fn handle_clear(&mut self) -> Result<()> {
|
||||
let provider = self.agent.provider().await?;
|
||||
if provider.manages_own_context() {
|
||||
output::render_error(&context_management_unsupported_message(
|
||||
"clear",
|
||||
provider.get_name(),
|
||||
));
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if let Err(e) = self
|
||||
.agent
|
||||
.config
|
||||
@@ -1263,6 +1274,15 @@ impl CliSession {
|
||||
}
|
||||
|
||||
async fn handle_compact(&mut self) -> Result<()> {
|
||||
let provider = self.agent.provider().await?;
|
||||
if provider.manages_own_context() {
|
||||
output::render_error(&context_management_unsupported_message(
|
||||
"compact",
|
||||
provider.get_name(),
|
||||
));
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let prompt = "Are you sure you want to compact this conversation? This will condense the message history.";
|
||||
let should_summarize = match cliclack::confirm(prompt).initial_value(true).interact() {
|
||||
Ok(choice) => choice,
|
||||
|
||||
@@ -1633,19 +1633,24 @@ impl Agent {
|
||||
.unwrap_or_else(|_| {
|
||||
crate::context_mgmt::compute_tool_call_cutoff(context_limit, compaction_threshold)
|
||||
});
|
||||
let tool_pair_compaction_enabled = crate::context_mgmt::tool_pair_summarization_enabled()
|
||||
&& !provider.manages_own_context();
|
||||
let manages_own_context = provider.manages_own_context();
|
||||
let tool_pair_compaction_enabled =
|
||||
crate::context_mgmt::tool_pair_summarization_enabled() && !manages_own_context;
|
||||
|
||||
let operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
let mut operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
Arc::new(SteerOperation::new(steer_queue, self.hook_manager.clone())),
|
||||
Arc::new(MaxTurnsOperation::new(max_turns)),
|
||||
Arc::new(BangShellOperation::new()),
|
||||
Arc::new(CompactionOperation::new(
|
||||
];
|
||||
if !manages_own_context {
|
||||
operations.push(Arc::new(CompactionOperation::new(
|
||||
provider.clone(),
|
||||
model_config.clone(),
|
||||
context_limit,
|
||||
compaction_threshold,
|
||||
)),
|
||||
)));
|
||||
}
|
||||
let remaining_operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
Arc::new(ToolPairCompactionOperation::new(
|
||||
provider.clone(),
|
||||
model_config.clone(),
|
||||
@@ -1678,6 +1683,7 @@ impl Agent {
|
||||
)),
|
||||
Arc::new(ExitOnErrorOperation),
|
||||
];
|
||||
operations.extend(remaining_operations);
|
||||
let inference = Arc::new(InferenceRunner::new(
|
||||
provider,
|
||||
model_config,
|
||||
|
||||
@@ -89,6 +89,12 @@ pub fn list_commands() -> &'static [CommandDef] {
|
||||
COMMANDS
|
||||
}
|
||||
|
||||
pub fn context_management_unsupported_message(command: &str, provider: &str) -> String {
|
||||
format!(
|
||||
"/{command} is not available for provider '{provider}' because it manages its own conversation context"
|
||||
)
|
||||
}
|
||||
|
||||
pub fn is_known_slash_command(message_text: &str, working_dir: Option<&Path>) -> bool {
|
||||
let Some(parsed) = parse_slash_command(message_text) else {
|
||||
return false;
|
||||
@@ -165,6 +171,14 @@ impl Agent {
|
||||
}
|
||||
|
||||
async fn handle_compact_command(&self, session_id: &str) -> Result<Option<Message>> {
|
||||
let provider = self.provider().await?;
|
||||
if provider.manages_own_context() {
|
||||
return Err(anyhow!(context_management_unsupported_message(
|
||||
"compact",
|
||||
provider.get_name()
|
||||
)));
|
||||
}
|
||||
|
||||
let manager = self.config.session_manager.clone();
|
||||
let session = manager.get_session(session_id, true).await?;
|
||||
let conversation = session
|
||||
@@ -173,7 +187,7 @@ impl Agent {
|
||||
|
||||
let model_config = self.model_config_for_session(session_id).await?;
|
||||
let compaction = compact_messages(
|
||||
self.provider().await?.as_ref(),
|
||||
provider.as_ref(),
|
||||
&model_config,
|
||||
session_id,
|
||||
&conversation,
|
||||
@@ -199,6 +213,14 @@ impl Agent {
|
||||
async fn handle_clear_command(&self, session_id: &str) -> Result<Option<Message>> {
|
||||
use crate::conversation::Conversation;
|
||||
|
||||
let provider = self.provider().await?;
|
||||
if provider.manages_own_context() {
|
||||
return Err(anyhow!(context_management_unsupported_message(
|
||||
"clear",
|
||||
provider.get_name()
|
||||
)));
|
||||
}
|
||||
|
||||
let manager = self.config.session_manager.clone();
|
||||
manager
|
||||
.replace_conversation(session_id, &Conversation::default())
|
||||
|
||||
@@ -27,7 +27,7 @@ pub mod validate_extensions;
|
||||
|
||||
pub use agent::{Agent, AgentConfig, ExtensionLoadResult, GoosePlatform};
|
||||
pub use container::Container;
|
||||
pub use execute_commands::COMPACT_TRIGGERS;
|
||||
pub use execute_commands::{context_management_unsupported_message, COMPACT_TRIGGERS};
|
||||
pub use extension::{ExtensionConfig, ExtensionError};
|
||||
pub use extension_manager::ExtensionManager;
|
||||
pub use goose_agent::events::AgentEvent;
|
||||
|
||||
@@ -74,6 +74,7 @@ impl Operation<Session, GooseEffect> for SlashCommandOperation<'_> {
|
||||
applied @ OperationResult::Applied(_) => return Ok(applied),
|
||||
}
|
||||
}
|
||||
|
||||
not_applicable()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ use goose_providers::conversation::token_usage::{ProviderUsage, Usage as Provide
|
||||
use rmcp::model::{CallToolRequestParams, CallToolResult, ContentBlock};
|
||||
|
||||
use super::calculator_extension::{value, ADD};
|
||||
use super::dummy_api::ProviderFeatures;
|
||||
use super::pipeline::{self, test_pipeline, MessageKind::Agent};
|
||||
use crate::agents::state_machine;
|
||||
use crate::agents::state_machine::ops_compaction::MAX_CONTEXT_ERROR_COMPACTIONS;
|
||||
@@ -157,6 +158,35 @@ async fn a_failed_compact_command_reports_the_error_and_keeps_working() -> Resul
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn context_owning_provider_has_no_compaction_operation() -> Result<()> {
|
||||
let (pipeline, api) = pipeline::test_pipeline_with(ProviderFeatures {
|
||||
manages_own_context: true,
|
||||
..ProviderFeatures::default()
|
||||
})
|
||||
.await?;
|
||||
api.on("continue").reply("continued");
|
||||
pipeline
|
||||
.set_total_tokens((pipeline.context_limit() as f64 * 0.81) as i32)
|
||||
.await;
|
||||
|
||||
let continued = pipeline.run(["continue"]).await?;
|
||||
continued.assert_message(-1, Agent, "continued");
|
||||
assert_eq!(continued.history_replacements(), 0);
|
||||
assert_eq!(api.calls().len(), 1);
|
||||
|
||||
for command in ["clear", "compact"] {
|
||||
let input = format!("/{command}");
|
||||
api.on(&input).reply(format!("provider handled /{command}"));
|
||||
let handled = pipeline.run([input.as_str()]).await?;
|
||||
handled.assert_message(-1, Agent, &format!("provider handled /{command}"));
|
||||
assert_eq!(handled.history_replacements(), 0);
|
||||
}
|
||||
assert_eq!(api.calls().len(), 3);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn text_that_looks_like_a_context_error_does_not_compact() -> Result<()> {
|
||||
let (pipeline, api) = test_pipeline().await?;
|
||||
|
||||
@@ -12,6 +12,7 @@ pub(super) struct ProviderFeatures {
|
||||
pub(super) resolved_model: Option<&'static str>,
|
||||
pub(super) cache_read_tokens: Option<i32>,
|
||||
pub(super) cache_write_tokens: Option<i32>,
|
||||
pub(super) manages_own_context: bool,
|
||||
}
|
||||
|
||||
impl Default for ProviderFeatures {
|
||||
@@ -22,6 +23,7 @@ impl Default for ProviderFeatures {
|
||||
resolved_model: None,
|
||||
cache_read_tokens: None,
|
||||
cache_write_tokens: None,
|
||||
manages_own_context: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -114,24 +114,28 @@ impl TestPipeline {
|
||||
self.model_config.context_limit(),
|
||||
COMPACTION_THRESHOLD,
|
||||
);
|
||||
let operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
let mut operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
Arc::new(SteerOperation::new(
|
||||
self.steer_queue.clone(),
|
||||
self.hook_manager.clone(),
|
||||
)),
|
||||
Arc::new(MaxTurnsOperation::new(self.max_turns)),
|
||||
Arc::new(BangShellOperation::new()),
|
||||
Arc::new(CompactionOperation::new(
|
||||
];
|
||||
if !self.provider_features.manages_own_context {
|
||||
operations.push(Arc::new(CompactionOperation::new(
|
||||
provider.clone(),
|
||||
self.model_config.clone(),
|
||||
self.model_config.context_limit(),
|
||||
COMPACTION_THRESHOLD,
|
||||
)),
|
||||
)));
|
||||
}
|
||||
let remaining_operations: Vec<Arc<dyn Operation<Session, GooseEffect> + '_>> = vec![
|
||||
Arc::new(ToolPairCompactionOperation::new(
|
||||
provider.clone(),
|
||||
self.model_config.clone(),
|
||||
tool_call_cutoff,
|
||||
true,
|
||||
!self.provider_features.manages_own_context,
|
||||
)),
|
||||
Arc::new(ToolApprovalOperation::new(
|
||||
&self.goose_mode,
|
||||
@@ -159,6 +163,7 @@ impl TestPipeline {
|
||||
)),
|
||||
Arc::new(ExitOnErrorOperation),
|
||||
];
|
||||
operations.extend(remaining_operations);
|
||||
let inference = Arc::new(InferenceRunner::new(
|
||||
provider,
|
||||
self.model_config.clone(),
|
||||
|
||||
@@ -21,12 +21,21 @@ use tempfile::TempDir;
|
||||
struct MockCompactionProvider {
|
||||
/// Tracks whether compaction has occurred (for context limit recovery case)
|
||||
has_compacted: Arc<AtomicBool>,
|
||||
manages_own_context: bool,
|
||||
}
|
||||
|
||||
impl MockCompactionProvider {
|
||||
fn new() -> Self {
|
||||
Self {
|
||||
has_compacted: Arc::new(AtomicBool::new(false)),
|
||||
manages_own_context: false,
|
||||
}
|
||||
}
|
||||
|
||||
fn context_owning() -> Self {
|
||||
Self {
|
||||
has_compacted: Arc::new(AtomicBool::new(false)),
|
||||
manages_own_context: true,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -98,6 +107,10 @@ impl MockCompactionProvider {
|
||||
|
||||
#[async_trait]
|
||||
impl Provider for MockCompactionProvider {
|
||||
fn manages_own_context(&self) -> bool {
|
||||
self.manages_own_context
|
||||
}
|
||||
|
||||
async fn stream(
|
||||
&self,
|
||||
_model_config: &ModelConfig,
|
||||
@@ -240,6 +253,58 @@ async fn setup_test_session(
|
||||
Ok(session)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn context_owning_provider_rejects_clear_and_compact_without_changing_session() -> Result<()>
|
||||
{
|
||||
let temp_dir = TempDir::new()?;
|
||||
let agent = Agent::new();
|
||||
let messages = vec![
|
||||
Message::user().with_text("Remember this"),
|
||||
Message::assistant().with_text("I will"),
|
||||
];
|
||||
let session = setup_test_session(
|
||||
&agent,
|
||||
&temp_dir,
|
||||
"context-owning-provider",
|
||||
messages.clone(),
|
||||
)
|
||||
.await?;
|
||||
let before = agent
|
||||
.config
|
||||
.session_manager
|
||||
.get_session(&session.id, true)
|
||||
.await?;
|
||||
let conversation_before = before.conversation.unwrap();
|
||||
let usage_before = before.usage;
|
||||
let provider = Arc::new(MockCompactionProvider::context_owning());
|
||||
agent
|
||||
.update_provider(provider, ModelConfig::new("mock-model"), &session.id)
|
||||
.await?;
|
||||
|
||||
for command in ["clear", "compact"] {
|
||||
let error = agent
|
||||
.execute_command(&format!("/{command}"), &session.id)
|
||||
.await
|
||||
.unwrap_err();
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
format!(
|
||||
"/{command} is not available for provider 'mock-compaction' because it manages its own conversation context"
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
let unchanged = agent
|
||||
.config
|
||||
.session_manager
|
||||
.get_session(&session.id, true)
|
||||
.await?;
|
||||
assert_eq!(unchanged.conversation.unwrap(), conversation_before);
|
||||
assert_eq!(unchanged.usage, usage_before);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Helper: Assert conversation has been compacted with proper message visibility
|
||||
fn assert_conversation_compacted(conversation: &Conversation) {
|
||||
let messages = conversation.messages();
|
||||
|
||||
Reference in New Issue
Block a user