-
Notifications
You must be signed in to change notification settings - Fork 7.6k
[skills] Auto install MCP dependencies when running skils with dependency specs. #9982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…/auto_skill_deps_install
codex-rs/core/src/mcp/mod.rs
Outdated
| .any(|entry| entry == MCP_DEPENDENCY_OPTION_INSTALL) | ||
| }); | ||
|
|
||
| sess.record_mcp_dependency_prompted(missing.keys().cloned()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save the mcps that are prompted so that we don't show the approval prompt for them again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should always show the prompt if it is still not set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add that later, for the first version I'm trying to play safe and try not to annoy the users with repeating prompts
codex-rs/core/src/mcp/mod.rs
Outdated
| ) | ||
| .await; | ||
|
|
||
| if let Err(err) = perform_oauth_login( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically trigger OAuth if available.
| && seen.insert(name.clone()) | ||
| && let Some(skill) = skills.iter().find(|s| s.name == *name && s.path == *path) | ||
| && !disabled_paths.contains(&skill.path) | ||
| match input { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extending the detection from CLI-specific logic, to actually matching $skill-name patterns.
| pub(crate) history: ContextManager, | ||
| pub(crate) latest_rate_limits: Option<RateLimitSnapshot>, | ||
| pub(crate) server_reasoning_included: bool, | ||
| pub(crate) mcp_dependency_prompted: HashSet<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep track of which mcps have been prompted to be installed.
| Arc::clone(&self.services.user_shell) | ||
| } | ||
|
|
||
| async fn refresh_mcp_servers_inner( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted from below.
…/auto_skill_deps_install
| let mut seen: HashSet<String> = HashSet::new(); | ||
|
|
||
| for input in inputs { | ||
| if let UserInput::Skill { name, path } = input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually realized it would still be helpful for the CLI to use this, once we have connectors we need to include it's path which the CLI doesn't support display.
Auto install MCP dependencies when running skils with dependency specs.