feature: reuse skill-scoped managed network proxies for skill scripts#14416
feature: reuse skill-scoped managed network proxies for skill scripts#14416
Conversation
ea68e43 to
9640836
Compare
9640836 to
1d511cf
Compare
19236d6 to
2da4e05
Compare
4925e68 to
d0d7ad8
Compare
345691e to
9d82ebc
Compare
| ) -> Self { | ||
| let mut spec = self.clone(); | ||
| if let Some(allowed_domains) = managed_network_override.allowed_domains.as_ref() { | ||
| spec.config.network.allowed_domains = allowed_domains.clone(); |
There was a problem hiding this comment.
This is why we really need this to be a map rather than a list.
| use std::sync::Arc; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub(crate) struct SkillNetworkProxyKey(String); |
There was a problem hiding this comment.
Why not just use a regular struct for this and rely on #[derive(Eq, Hash)]? Though you will need a constructor like shared_skill_proxy_key() that ensures the fields are sorted when this is constructed.
Or if you want to distill it down to a single value, why not use sha2::Digest instead of an arbitrarily long String?
| } | ||
|
|
||
| struct CoreShellCommandExecutor { | ||
| session: Option<Arc<crate::codex::Session>>, |
| async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions() { | ||
| let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir()).unwrap(); | ||
| let executor = CoreShellCommandExecutor { | ||
| session: None, |
There was a problem hiding this comment.
If it's only None in tests, then ideally we would use a dummy Session or something so the business logic doesn't have to deal with the None case.
| let Some(session) = self.session.as_ref() else { | ||
| return Ok(self.network.clone()); | ||
| }; | ||
| let Some(skill) = | ||
| find_skill_for_program(session.as_ref(), self.sandbox_policy_cwd.as_path(), program) | ||
| .await | ||
| else { | ||
| return Ok(self.network.clone()); | ||
| }; | ||
| if let Some(network) = session.get_or_start_skill_network_proxy(&skill).await? { |
There was a problem hiding this comment.
I think you can collapse these ifs such that Ok(self.network.clone()) only appears once?
|
|
||
| #[derive(Default)] | ||
| pub(crate) struct SkillNetworkProxyCache { | ||
| #[cfg_attr(not(unix), allow(dead_code))] |
There was a problem hiding this comment.
Why shouldn't this be available on Windows? I thought what we have is cross-platform?
There was a problem hiding this comment.
this is not not available on windows, it's not called because only unix_escalation path hits it
| return Ok(Arc::clone(proxy)); | ||
| } | ||
|
|
||
| let proxy = Arc::new(start().await?); |
There was a problem hiding this comment.
Can we find a way to represent this so that we do not have to await while holding the mutex for proxies?
297dea1 to
7b29237
Compare
339faab to
0eb91ed
Compare
0eb91ed to
a39c2d6
Compare
41655df to
ac4c459
Compare
ac4c459 to
4fbccee
Compare
|
Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
Summary
This follows up the managed-network override work for skills by wiring those overrides into shell execution. Before this change, skill scripts still reused the session-level managed network proxy, so per-skill allow/deny domain overrides were not applied through a dedicated proxy.
NetworkProxySpechelpers to applymanaged_network_overridesettings and derive a normalized cache key so equivalent specs reuse the same proxySkillNetworkProxyCachein session services and thread it through session and subagent constructionRelated:
Testing
tested with a skill that executes
fetch_example.shwhich fetches from www.example.com.case 1
with the following skill permission:
result:
That's because the program uses the allowed_domain of the default network proxy and
www.example.comis not in the allowed_domain.case 2
with the following permission:
result:
fetched the website correctly.