Skip to content

Activate selected executor skills atomically#29960

Open
jif-oai wants to merge 1 commit into
jif/cover-selected-capability-lifecyclefrom
jif/activate-selected-capability-skills
Open

Activate selected executor skills atomically#29960
jif-oai wants to merge 1 commit into
jif/cover-selected-capability-lifecyclefrom
jif/activate-selected-capability-skills

Conversation

@jif-oai

@jif-oai jif-oai commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • freeze selected executor skill declarations per resolved root and publish them with selected capability generations
  • keep provider-owned read routing while binding built-in skill reads to the exact executor filesystem
  • cover atomic publication plus fork and cold-resume lifecycle behavior

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

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: 1531dc319e

ℹ️ 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".

Comment on lines +103 to +104
for selected_root in selected_capabilities.ready() {
let selected_root_id = selected_root.selected_root().id.clone();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Surface failed selected executor roots

When a selected executor root reaches Failed (for example, an unavailable environment or a non-directory selected path), this code only iterates selected_capabilities.ready() and never adds the failure message to the skill catalog. Because on_thread_start now suppresses the legacy Vec<SelectedCapabilityRoot> path whenever a selected-skill snapshot exists, the previous ExecutorSkillProvider::list warning path is bypassed, so the configured executor skills silently disappear from both prompt context and warning events instead of telling the user what failed.

Useful? React with 👍 / 👎.

workspace = true

[dependencies]
codex-core-plugins = { workspace = true }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Refresh the Bazel lockfile for this dependency

Adding codex-core-plugins changes the crate dependency graph, but the diff only updates Cargo.toml/Cargo.lock and leaves MODULE.bazel.lock unchanged. The repo's Bazel lock check is expected to reject dependency drift, so this change should include the output of just bazel-lock-update with the same commit.

AGENTS.md reference: AGENTS.md:L37-L39

Useful? React with 👍 / 👎.

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.

1 participant