Skip to content

Conversation

@DOsinga
Copy link
Collaborator

@DOsinga DOsinga commented Aug 8, 2025

fixing: #3824

Developerayo and others added 11 commits August 4, 2025 13:21
Move hardcoded prompts from router_tool_selector.rs and permission_judge.rs
into reusable .md template files using the MiniJinja template system.

Changes:
- Add router_tool_selector.md template with {{tools}} and {{query}} variables
- Add permission_judge.md template for read-only operation detection
- Update router_tool_selector.rs to use render_global_file()
- Update permission_judge.rs to use render_global_file() with fallback
- Add proper error handling for template rendering failures
- All existing tests continue to pass

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
.placeholder("https://api.example.com/v1/messages")
.validate(|input: &String| {
if !input.starts_with("http://") && !input.starts_with("https://") {
Err("Inputed URL must start with either http:// or https://")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Err("Inputed URL must start with either http:// or https://")
Err("URL must start with either http:// or https://")

verify_secret_key(&headers, &state)?;

let key = name_to_key(&name);
let key = goose::config::extensions::name_to_key(&name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the import change?

.clone()
.unwrap_or_else(|| {
format!(
"Custom {} provider",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Custom {} provider",
"{} (custom)",

.to_uppercase()
.replace(" ", "_")
.replace("-", "_")
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some duplication between here and the CLI, maybe that could be reduced?

/// custom providers
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum ProviderEngine {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we call this "format" elsewhere

.map_err(|e| anyhow::anyhow!("Failed to parse {}: {}", path.display(), e))?;

configs.push(config);
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty else block

}
}

/// load custom providers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// load custom providers

}

/// load custom providers
pub fn load_custom_providers(dir: &Path) -> Result<Vec<CustomProviderConfig>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also duplicates a bit I think from routes/config_management

REGISTRY.read().unwrap().all_metadata()
}

pub fn refresh_custom_providers() -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any path here that actually returns an error?

@jamadeo
Copy link
Collaborator

jamadeo commented Aug 14, 2025

closing in favor of #3824

@jamadeo jamadeo closed this Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants