Remove locking from trusted skill collection (#41150)

## What changed

- Store per-turn trusted skill paths directly in a `BTreeSet` instead of a
  mutex-protected set.
- Mutably record paths and consume the collection when producing the final
  ordered path list.
- Strengthen the bounded-deduplication test to verify the exact collected paths.

GitOrigin-RevId: 69e87fc7783738d85ed204f18a3968d0837c3693
This commit is contained in:
jif authored and copyberry committed 2026-08-27 14:20:01 +00:00
1 parent e8b938b02e
commit df9f537a6e
3 files changed
+16 -21

No files matched your search

@@ -678,7 +678,7 @@ impl GuardianV2Extension {
None => None, None => None,
}; };
let root_snapshot = thread.guardian_root_snapshot().await; let root_snapshot = thread.guardian_root_snapshot().await;
let trusted_skills = TrustedSkillInvocations::default(); let mut trusted_skills = TrustedSkillInvocations::default();
for path in local_trusted_skill_paths.iter().chain( for path in local_trusted_skill_paths.iter().chain(
root_snapshot root_snapshot
.as_ref() .as_ref()
@@ -687,7 +687,7 @@ impl GuardianV2Extension {
) { ) {
trusted_skills.record(path.clone()); trusted_skills.record(path.clone());
} }
let trusted_skill_paths = trusted_skills.snapshot(); let trusted_skill_paths = trusted_skills.into_paths();
let root_authorization_version = root_snapshot let root_authorization_version = root_snapshot
.as_ref() .as_ref()
.map(|snapshot| snapshot.authorization_version); .map(|snapshot| snapshot.authorization_version);
@@ -3,7 +3,6 @@
use std::collections::BTreeSet; use std::collections::BTreeSet;
use std::path::Path; use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
use std::sync::Mutex;
use codex_core::config::Config; use codex_core::config::Config;
use codex_extension_api::ContentItemKind; use codex_extension_api::ContentItemKind;
@@ -104,14 +103,11 @@ impl TrustedSkillRoots {
/// Bounded, deduplicated user-owned skill paths observed in one turn. /// Bounded, deduplicated user-owned skill paths observed in one turn.
#[derive(Default)] #[derive(Default)]
pub(crate) struct TrustedSkillInvocations(Mutex<BTreeSet<String>>); pub(crate) struct TrustedSkillInvocations(BTreeSet<String>);
impl TrustedSkillInvocations { impl TrustedSkillInvocations {
pub(crate) fn record(&self, path: String) { pub(crate) fn record(&mut self, path: String) {
let mut skills = self let skills = &mut self.0;
.0
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
if skills.contains(&path) if skills.contains(&path)
|| skills.len() >= MAX_TRUSTED_SKILLS || skills.len() >= MAX_TRUSTED_SKILLS
|| skills || skills
@@ -126,13 +122,8 @@ impl TrustedSkillInvocations {
skills.insert(path); skills.insert(path);
} }
pub(crate) fn snapshot(&self) -> Vec<String> { pub(crate) fn into_paths(self) -> Vec<String> {
self.0 self.0.into_iter().collect()
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.iter()
.cloned()
.collect()
} }
} }
@@ -137,21 +137,25 @@ async fn rejects_skills_that_escape_user_roots_through_symlinks() -> Result<()>
#[test] #[test]
fn invoked_skill_paths_are_deduplicated_and_bounded() { fn invoked_skill_paths_are_deduplicated_and_bounded() {
let skills = TrustedSkillInvocations::default(); let mut skills = TrustedSkillInvocations::default();
for index in 0..MAX_TRUSTED_SKILLS.saturating_mul(2) { for index in 0..MAX_TRUSTED_SKILLS.saturating_mul(2) {
let path = format!("/home/user/.codex/skills/{index:03}/SKILL.md"); let path = format!("/home/user/.codex/skills/{index:03}/SKILL.md");
skills.record(path.clone()); skills.record(path.clone());
skills.record(path); skills.record(path);
} }
let snapshot = skills.snapshot(); assert_eq!(
assert_eq!(snapshot.len(), MAX_TRUSTED_SKILLS); skills.into_paths(),
(0..MAX_TRUSTED_SKILLS)
.map(|index| format!("/home/user/.codex/skills/{index:03}/SKILL.md"))
.collect::<Vec<_>>()
);
let bounded = TrustedSkillInvocations::default(); let mut bounded = TrustedSkillInvocations::default();
for index in 0..MAX_TRUSTED_SKILLS { for index in 0..MAX_TRUSTED_SKILLS {
bounded.record(format!("{index:03}{}", "x".repeat(500))); bounded.record(format!("{index:03}{}", "x".repeat(500)));
} }
let snapshot = bounded.snapshot(); let snapshot = bounded.into_paths();
assert!(snapshot.len() < MAX_TRUSTED_SKILLS); assert!(snapshot.len() < MAX_TRUSTED_SKILLS);
assert!(snapshot.iter().map(String::len).sum::<usize>() <= MAX_TRUSTED_SKILL_PATHS_BYTES); assert!(snapshot.iter().map(String::len).sum::<usize>() <= MAX_TRUSTED_SKILL_PATHS_BYTES);
} }