mirror of
https://github.com/RightNow-AI/openfang.git
synced 2026-08-14 00:47:51 +00:00
- #875: Install script uses robust sed parsing instead of fragile cut for version detection - #872: Session endpoint returns full tool results (removed 2000-char truncation) - #867: agent_send/agent_spawn get 600s timeout (was 120s), regular tools keep 120s - #824: Doctor workspace skills count uses direct return value from load_workspace_skills - #833: Model switching respects provider via new find_model_for_provider() lookup - #766: Closed as resolved by combined heartbeat fixes (v0.5.3 + merged PRs) All tests passing. Live tested with daemon.
This commit is contained in:
jaberjaber23
@@ -579,9 +579,7 @@ pub async fn get_agent_session(
|
||||
msg.get_mut("tools").and_then(|v| v.as_array_mut())
|
||||
{
|
||||
if let Some(tool_obj) = tools_arr.get_mut(tool_idx) {
|
||||
let preview: String =
|
||||
result.chars().take(2000).collect();
|
||||
tool_obj["result"] = serde_json::Value::String(preview);
|
||||
tool_obj["result"] = serde_json::Value::String(result.clone());
|
||||
tool_obj["is_error"] =
|
||||
serde_json::Value::Bool(*is_error);
|
||||
}
|
||||
|
||||
@@ -2642,9 +2642,7 @@ decay_rate = 0.05
|
||||
// Check workspace skills if home dir available
|
||||
if skills_dir.exists() {
|
||||
match skill_reg.load_workspace_skills(&skills_dir) {
|
||||
Ok(_) => {
|
||||
let total = skill_reg.count();
|
||||
let ws_count = total.saturating_sub(bundled_count);
|
||||
Ok(ws_count) => {
|
||||
if ws_count > 0 {
|
||||
if !json {
|
||||
ui::check_ok(&format!("Workspace skills loaded: {ws_count}"));
|
||||
|
||||
@@ -2893,7 +2893,16 @@ impl OpenFangKernel {
|
||||
.model_catalog
|
||||
.read()
|
||||
.ok()
|
||||
.and_then(|catalog| catalog.find_model(model).cloned());
|
||||
.and_then(|catalog| {
|
||||
// When the caller specifies a provider, use provider-aware lookup
|
||||
// so we resolve the model on the correct provider — not a builtin
|
||||
// from a different provider that happens to share the same name (#833).
|
||||
if let Some(ep) = explicit_provider {
|
||||
catalog.find_model_for_provider(model, ep).cloned()
|
||||
} else {
|
||||
catalog.find_model(model).cloned()
|
||||
}
|
||||
});
|
||||
let provider = if let Some(ep) = explicit_provider {
|
||||
// User explicitly set the provider — use it as-is
|
||||
Some(ep.to_string())
|
||||
|
||||
@@ -44,6 +44,20 @@ const BASE_RETRY_DELAY_MS: u64 = 1000;
|
||||
/// Raised from 60s to 120s for browser automation and long-running builds.
|
||||
const TOOL_TIMEOUT_SECS: u64 = 120;
|
||||
|
||||
/// Timeout for inter-agent tool calls (seconds).
|
||||
/// Agent delegation (agent_send, agent_spawn) can involve a full agent loop on the
|
||||
/// target, so these need a significantly longer timeout than regular tools.
|
||||
const AGENT_TOOL_TIMEOUT_SECS: u64 = 600;
|
||||
|
||||
/// Returns the appropriate timeout duration for a given tool name.
|
||||
/// Inter-agent calls get a longer timeout since they may trigger full agent loops.
|
||||
fn tool_timeout_for(tool_name: &str) -> Duration {
|
||||
match tool_name {
|
||||
"agent_send" | "agent_spawn" => Duration::from_secs(AGENT_TOOL_TIMEOUT_SECS),
|
||||
_ => Duration::from_secs(TOOL_TIMEOUT_SECS),
|
||||
}
|
||||
}
|
||||
|
||||
/// Maximum consecutive MaxTokens continuations before returning partial response.
|
||||
/// Raised from 3 to 5 to allow longer-form generation.
|
||||
const MAX_CONTINUATIONS: u32 = 5;
|
||||
@@ -738,8 +752,10 @@ pub async fn run_agent_loop(
|
||||
let effective_exec_policy = manifest.exec_policy.as_ref();
|
||||
|
||||
// Timeout-wrapped execution
|
||||
let timeout = tool_timeout_for(&tool_call.name);
|
||||
let timeout_secs = timeout.as_secs();
|
||||
let result = match tokio::time::timeout(
|
||||
Duration::from_secs(TOOL_TIMEOUT_SECS),
|
||||
timeout,
|
||||
tool_runner::execute_tool(
|
||||
&tool_call.id,
|
||||
&tool_call.name,
|
||||
@@ -768,12 +784,12 @@ pub async fn run_agent_loop(
|
||||
{
|
||||
Ok(result) => result,
|
||||
Err(_) => {
|
||||
warn!(tool = %tool_call.name, "Tool execution timed out after {}s", TOOL_TIMEOUT_SECS);
|
||||
warn!(tool = %tool_call.name, "Tool execution timed out after {}s", timeout_secs);
|
||||
openfang_types::tool::ToolResult {
|
||||
tool_use_id: tool_call.id.clone(),
|
||||
content: format!(
|
||||
"Tool '{}' timed out after {}s.",
|
||||
tool_call.name, TOOL_TIMEOUT_SECS
|
||||
tool_call.name, timeout_secs
|
||||
),
|
||||
is_error: true,
|
||||
}
|
||||
@@ -1881,8 +1897,10 @@ pub async fn run_agent_loop_streaming(
|
||||
let effective_exec_policy = manifest.exec_policy.as_ref();
|
||||
|
||||
// Timeout-wrapped execution
|
||||
let timeout = tool_timeout_for(&tool_call.name);
|
||||
let timeout_secs = timeout.as_secs();
|
||||
let result = match tokio::time::timeout(
|
||||
Duration::from_secs(TOOL_TIMEOUT_SECS),
|
||||
timeout,
|
||||
tool_runner::execute_tool(
|
||||
&tool_call.id,
|
||||
&tool_call.name,
|
||||
@@ -1911,12 +1929,12 @@ pub async fn run_agent_loop_streaming(
|
||||
{
|
||||
Ok(result) => result,
|
||||
Err(_) => {
|
||||
warn!(tool = %tool_call.name, "Tool execution timed out after {}s (streaming)", TOOL_TIMEOUT_SECS);
|
||||
warn!(tool = %tool_call.name, "Tool execution timed out after {}s (streaming)", timeout_secs);
|
||||
openfang_types::tool::ToolResult {
|
||||
tool_use_id: tool_call.id.clone(),
|
||||
content: format!(
|
||||
"Tool '{}' timed out after {}s.",
|
||||
tool_call.name, TOOL_TIMEOUT_SECS
|
||||
tool_call.name, timeout_secs
|
||||
),
|
||||
is_error: true,
|
||||
}
|
||||
@@ -2980,6 +2998,15 @@ mod tests {
|
||||
#[test]
|
||||
fn test_tool_timeout_constant() {
|
||||
assert_eq!(TOOL_TIMEOUT_SECS, 120);
|
||||
assert_eq!(AGENT_TOOL_TIMEOUT_SECS, 600);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tool_timeout_for_agent_tools() {
|
||||
assert_eq!(tool_timeout_for("agent_send"), Duration::from_secs(600));
|
||||
assert_eq!(tool_timeout_for("agent_spawn"), Duration::from_secs(600));
|
||||
assert_eq!(tool_timeout_for("file_read"), Duration::from_secs(120));
|
||||
assert_eq!(tool_timeout_for("shell_exec"), Duration::from_secs(120));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -172,6 +172,72 @@ impl ModelCatalog {
|
||||
None
|
||||
}
|
||||
|
||||
/// Find a model by ID/alias, preferring entries from the given provider.
|
||||
///
|
||||
/// When `provider` is specified, this method first looks for a matching model
|
||||
/// that belongs to that provider. If no provider-scoped match is found, it
|
||||
/// falls back to the normal `find_model` resolution.
|
||||
///
|
||||
/// This prevents issue #833 where switching to model "kimi-2.5" with provider
|
||||
/// "model_studio" would incorrectly resolve to moonshot's builtin kimi-2.5
|
||||
/// because `find_model` does not consider provider affinity.
|
||||
pub fn find_model_for_provider(
|
||||
&self,
|
||||
id_or_alias: &str,
|
||||
provider: &str,
|
||||
) -> Option<&ModelCatalogEntry> {
|
||||
let lower = id_or_alias.to_lowercase();
|
||||
|
||||
// First pass: look for a match scoped to the requested provider.
|
||||
// Priority: exact-case ID > case-insensitive ID > display-name.
|
||||
let mut provider_ci: Option<&ModelCatalogEntry> = None;
|
||||
|
||||
for m in &self.models {
|
||||
if m.provider != provider {
|
||||
continue;
|
||||
}
|
||||
if m.id.to_lowercase() != lower {
|
||||
continue;
|
||||
}
|
||||
if m.id == id_or_alias {
|
||||
return Some(m); // Exact-case match on the right provider — best result
|
||||
}
|
||||
if provider_ci.is_none() {
|
||||
provider_ci = Some(m);
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(entry) = provider_ci {
|
||||
return Some(entry);
|
||||
}
|
||||
|
||||
// Display-name match scoped to provider
|
||||
if let Some(entry) = self
|
||||
.models
|
||||
.iter()
|
||||
.find(|m| m.provider == provider && m.display_name.to_lowercase() == lower)
|
||||
{
|
||||
return Some(entry);
|
||||
}
|
||||
|
||||
// Alias resolution scoped to provider: resolve the alias, then check if
|
||||
// the canonical model belongs to the requested provider.
|
||||
if let Some(canonical) = self.aliases.get(&lower) {
|
||||
if let Some(entry) = self
|
||||
.models
|
||||
.iter()
|
||||
.find(|m| m.id == *canonical && m.provider == provider)
|
||||
{
|
||||
return Some(entry);
|
||||
}
|
||||
}
|
||||
|
||||
// No provider-scoped match — fall back to normal resolution so callers
|
||||
// still get a result when the model genuinely doesn't exist on this provider
|
||||
// (e.g. user typo, or a model name that only exists elsewhere).
|
||||
self.find_model(id_or_alias)
|
||||
}
|
||||
|
||||
/// Resolve an alias to a canonical model ID, or None if not an alias.
|
||||
pub fn resolve_alias(&self, alias: &str) -> Option<&str> {
|
||||
self.aliases.get(&alias.to_lowercase()).map(|s| s.as_str())
|
||||
@@ -4365,4 +4431,96 @@ mod tests {
|
||||
assert_eq!(lower.tier, ModelTier::Local);
|
||||
assert_eq!(lower.provider, "ollama");
|
||||
}
|
||||
|
||||
/// Regression test for #833: find_model_for_provider should prefer the entry
|
||||
/// from the specified provider when multiple providers share the same model name.
|
||||
///
|
||||
/// Scenario: a custom provider "model_studio" has a model "kimi-k2.5", and the
|
||||
/// builtin "moonshot" provider also has "kimi-k2.5". When the user switches to
|
||||
/// "kimi-k2.5" with provider "model_studio", we must resolve to model_studio's
|
||||
/// entry, not moonshot's builtin.
|
||||
#[test]
|
||||
fn test_find_model_for_provider_prefers_specified_provider_833() {
|
||||
let mut catalog = ModelCatalog::new();
|
||||
|
||||
// Verify the builtin moonshot entry exists
|
||||
let builtin = catalog.find_model("kimi-k2.5").unwrap();
|
||||
assert_eq!(builtin.provider, "moonshot");
|
||||
|
||||
// Add a custom model with the same name on a different provider
|
||||
let added = catalog.add_custom_model(ModelCatalogEntry {
|
||||
id: "kimi-k2.5".into(),
|
||||
display_name: "Kimi K2.5 (Model Studio)".into(),
|
||||
provider: "model_studio".into(),
|
||||
tier: ModelTier::Balanced,
|
||||
context_window: 131_072,
|
||||
max_output_tokens: 8_192,
|
||||
input_cost_per_m: 0.0,
|
||||
output_cost_per_m: 0.0,
|
||||
supports_tools: true,
|
||||
supports_vision: false,
|
||||
supports_streaming: true,
|
||||
aliases: vec![],
|
||||
});
|
||||
assert!(added, "custom model should be added (different provider)");
|
||||
|
||||
// Plain find_model returns the custom entry (Custom tier wins over builtin)
|
||||
let plain = catalog.find_model("kimi-k2.5").unwrap();
|
||||
assert_eq!(plain.tier, ModelTier::Custom);
|
||||
|
||||
// find_model_for_provider with "model_studio" must return model_studio's entry
|
||||
let ms = catalog
|
||||
.find_model_for_provider("kimi-k2.5", "model_studio")
|
||||
.unwrap();
|
||||
assert_eq!(ms.provider, "model_studio");
|
||||
assert_eq!(ms.display_name, "Kimi K2.5 (Model Studio)");
|
||||
|
||||
// find_model_for_provider with "moonshot" must return moonshot's builtin
|
||||
let moonshot = catalog
|
||||
.find_model_for_provider("kimi-k2.5", "moonshot")
|
||||
.unwrap();
|
||||
assert_eq!(moonshot.provider, "moonshot");
|
||||
assert_eq!(moonshot.display_name, "Kimi K2.5");
|
||||
}
|
||||
|
||||
/// Verify find_model_for_provider falls back to normal resolution when the
|
||||
/// model doesn't exist on the requested provider.
|
||||
#[test]
|
||||
fn test_find_model_for_provider_fallback() {
|
||||
let catalog = ModelCatalog::new();
|
||||
|
||||
// "claude-sonnet-4-20250514" only exists on "anthropic"
|
||||
let entry = catalog
|
||||
.find_model_for_provider("claude-sonnet-4-20250514", "nonexistent_provider")
|
||||
.unwrap();
|
||||
assert_eq!(entry.provider, "anthropic");
|
||||
}
|
||||
|
||||
/// Verify find_model_for_provider is case-insensitive for the model name.
|
||||
#[test]
|
||||
fn test_find_model_for_provider_case_insensitive() {
|
||||
let mut catalog = ModelCatalog::new();
|
||||
|
||||
catalog.add_custom_model(ModelCatalogEntry {
|
||||
id: "My-Custom-LLM".into(),
|
||||
display_name: "My Custom LLM".into(),
|
||||
provider: "custom_provider".into(),
|
||||
tier: ModelTier::Balanced,
|
||||
context_window: 32_768,
|
||||
max_output_tokens: 4_096,
|
||||
input_cost_per_m: 0.0,
|
||||
output_cost_per_m: 0.0,
|
||||
supports_tools: true,
|
||||
supports_vision: false,
|
||||
supports_streaming: true,
|
||||
aliases: vec![],
|
||||
});
|
||||
|
||||
// Case-insensitive lookup with the correct provider
|
||||
let found = catalog
|
||||
.find_model_for_provider("my-custom-llm", "custom_provider")
|
||||
.unwrap();
|
||||
assert_eq!(found.provider, "custom_provider");
|
||||
assert_eq!(found.id, "My-Custom-LLM");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -732,4 +732,61 @@ input_schema = { type = "object" }
|
||||
"Global beta should remain unchanged"
|
||||
);
|
||||
}
|
||||
|
||||
/// #824: load_workspace_skills must return the count of workspace skills loaded,
|
||||
/// even when a workspace skill overrides a global skill with the same HashMap key.
|
||||
/// The old doctor code computed `total - bundled_count` which underreported when
|
||||
/// an override didn't increase total_loaded.
|
||||
#[test]
|
||||
fn test_workspace_override_returns_correct_count() {
|
||||
let global_dir = TempDir::new().unwrap();
|
||||
let ws_dir = TempDir::new().unwrap();
|
||||
|
||||
// One global skill named "shared"
|
||||
create_test_skill(global_dir.path(), "shared");
|
||||
|
||||
// Workspace skill with the SAME name — override
|
||||
let ws_shared = ws_dir.path().join("shared");
|
||||
std::fs::create_dir_all(&ws_shared).unwrap();
|
||||
std::fs::write(
|
||||
ws_shared.join("skill.toml"),
|
||||
r#"
|
||||
[skill]
|
||||
name = "shared"
|
||||
version = "2.0.0"
|
||||
description = "Workspace override"
|
||||
|
||||
[runtime]
|
||||
type = "python"
|
||||
entry = "main.py"
|
||||
|
||||
[[tools.provided]]
|
||||
name = "shared_tool"
|
||||
description = "Workspace tool"
|
||||
input_schema = { type = "object" }
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let mut registry = SkillRegistry::new(global_dir.path().to_path_buf());
|
||||
registry.load_all().unwrap();
|
||||
assert_eq!(registry.count(), 1, "One global skill loaded");
|
||||
|
||||
let ws_count = registry.load_workspace_skills(ws_dir.path()).unwrap();
|
||||
|
||||
// The return value must be 1, NOT 0.
|
||||
// Before the #824 fix, doctor computed total(1) - bundled(1) = 0.
|
||||
assert_eq!(
|
||||
ws_count, 1,
|
||||
"load_workspace_skills must report 1 even when overriding a global skill (#824)"
|
||||
);
|
||||
// Total registry count stays 1 because the override replaced, not added
|
||||
assert_eq!(registry.count(), 1);
|
||||
// But the skill is the workspace version
|
||||
assert_eq!(
|
||||
registry.get("shared").unwrap().manifest.skill.version,
|
||||
"2.0.0",
|
||||
"Workspace version should be active"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -52,7 +52,7 @@ install() {
|
||||
echo " Using specified version: $VERSION"
|
||||
else
|
||||
echo " Fetching latest release..."
|
||||
VERSION=$(curl -fsSL "https://api.github.com/repos/$REPO/releases/latest" | grep '"tag_name"' | head -1 | cut -d '"' -f 4)
|
||||
VERSION=$(curl -fsSL "https://api.github.com/repos/$REPO/releases/latest" | grep '"tag_name"' | sed 's/.*"tag_name": *"//' | sed 's/".*//')
|
||||
fi
|
||||
|
||||
if [ -z "$VERSION" ]; then
|
||||
|
||||
Reference in New Issue
Block a user