-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added support for live updates to skills #10478
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
Add a centralized FileWatcher in codex-core (using notify) that watches skill roots from the config layer stack (recursive) Send `SkillsChanged` events when relevant file system changes are detected On `SkillsChanged`: * Invalidate the skills cache immediately in ThreadManager * Emit EventMsg::SkillsUpdateAvailable to active sessions * Broadcast a new app-server notification: skills/list/updated Add SkillsListUpdatedNotification to the app-server protocol and gate broadcast until after initialize. This change does not inject new events into the event stream. That means the agent will not know about new skills, so it won't be able to implicitly invoke new skills. It also won't know about changes to existing skills, so if it has already read the contents of a modified skill, it will not honor the new behavior. I plan to address these limitations in a follow-on PR modeled after #9985. Injection of new skills (and AGENTS) was deemed to risky at this point, hence the need to split the feature into two stages. Testing: * In addition to automated tests, I did manual testing to confirm that newly-created skills, deleted skills, and renamed skills are reflected in the TUI skill picker menu. Also confirmed that modifications to behaviors for explicitly-invoked skills are honored.
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76c7486950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l_update_part1 # Conflicts: # codex-rs/core/src/codex.rs
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "title": "SkillsListUpdatedNotification", | ||
| "type": "object" | ||
| } No newline at end of file |
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.
Add new line?
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.
This is an auto-generated file.
codex-rs/core/src/project_doc.rs
Outdated
| /// directory (inclusive). Symlinks are allowed. When `project_doc_max_bytes` | ||
| /// is zero, returns an empty list. | ||
| pub fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<PathBuf>> { | ||
| let search_dirs = project_doc_search_dirs(config)?; |
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.
Irrelevant change?
| config: Arc<Config>, | ||
| initialized: bool, | ||
| experimental_api_enabled: Arc<AtomicBool>, | ||
| initialized_flag: Arc<AtomicBool>, |
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.
nit: maybe more descriptive name? requested_skills? send_skill_updated_events?
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.
ArcSwap might be a good primitive here but not required.
| err.into_inner().insert(cwd.to_path_buf(), outcome.clone()); | ||
| } | ||
| } | ||
| let mut cache = match self.cache_by_cwd.write() { |
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.
👍
| } | ||
|
|
||
| #[test] | ||
| fn throttles_and_coalesces_within_interval() { |
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 might want a few more file watcher specific tests in the future. Not blocking.
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.
add basic tests on util functions.
|
|
||
| // Watch for on-disk skill changes and send notifications to the client. | ||
| let initialized_flag = Arc::new(AtomicBool::new(false)); | ||
| let mut skills_updates_rx = thread_manager.subscribe_file_watcher(); |
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.
A little surprising we don't do subscription somewhere in core while the watcher lives in core.
Feels a bit like a leaking abstraction where we create the watcher in core but then expose it as is and have the external consumer manage it.
Have you considered adding a SkillsChanged core event instead?
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.
Ah, you did add the event. Should we use it here instead of resubscribing to watcher directly?
| use tokio::time::timeout; | ||
|
|
||
| fn sse_completed(id: &str) -> String { | ||
| load_sse_fixture_with_id("../fixtures/completed_template.json", id) |
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.
prefer the existing shared sse_completed helper
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 don't see a shared sse_completed helper?
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.
responses::ev_completed("resp-1"),
| // Some environments do not reliably surface file watcher events for | ||
| // skill changes. Clear the cache explicitly so we can still validate | ||
| // that the updated skill body is injected on the next turn. | ||
| test.thread_manager.skills_manager().clear_cache(); |
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.
Is this test too weak?
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 can’t find a way to make this behave consistently. I agree that if we swallow the error, this test becomes largely meaningless.
| let mut rx = test.thread_manager.subscribe_file_watcher(); | ||
| write_skill(test.codex_home_path(), "demo", "demo skill", skill_v2); | ||
|
|
||
| let changed_paths = timeout(Duration::from_secs(5), async move { |
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.
Should we assert EventMsg::SkillsUpdateAvailable event instead?
| .expect("request captured after skill update"); | ||
|
|
||
| assert!( | ||
| contains_skill_body(&last_request, skill_v2), |
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.
Surprised the skill is getting reinjected on next turn.
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.
The submit_skill_turn explicitly mentioned the skill again. Currently we do not dedupe pre-injection if you mention the same skills multiple times in a thread.
pakrym-oai
left a comment
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 think we should peek file watcher as code implementation detail and only expose/consume EventMsg::SkillsUpdateAvailable event.
codex-rs/core/src/file_watcher.rs
Outdated
| } | ||
|
|
||
| pub(crate) fn register_config(&self, config: &Config) { | ||
| self.register_skills_root(config.codex_home.join("skills")); |
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.
skill_roots_from_layer_stack_with_agents should cover this already?
# Conflicts: # codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json # codex-rs/app-server-protocol/schema/typescript/v2/index.ts
| } | ||
|
|
||
| // Start the watcher after SessionConfigured so it cannot emit earlier events. | ||
| sess.start_file_watcher_listener(); |
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.
So we will still emit EventMsg::SkillsUpdateAvailable (for each session) for app-server clients right?
446394d to
7ceda56
Compare
7ceda56 to
f870207
Compare
Add a centralized FileWatcher in codex-core (using notify) that watches skill roots from the config layer stack (recursive)
Send
SkillsChangedevents when relevant file system changes are detectedOn
SkillsChanged:* Broadcast a new app-server notification: SkillsListUpdatedNotificationThis change does not inject new items into the event stream. That means the agent will not know about new skills, so it won't be able to implicitly invoke new skills. It also won't know about changes to existing skills, so if it has already read the contents of a modified skill, it will not honor the new behavior.
This change also does not detect modifications to AGENTS.md.
I plan to address these limitations in a follow-on PR modeled after #9985. Injection of new skills and AGENTS was deemed to risky, hence the need to split the feature into two stages. The changes in this PR were designed to easily accommodate the second stage once we have some other foundational changes in place.
Testing: In addition to automated tests, I did manual testing to confirm that newly-created skills, deleted skills, and renamed skills are reflected in the TUI skill picker menu. Also confirmed that modifications to behaviors for explicitly-invoked skills are honored.