mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
skills: resolve per-turn catalogs from turn input context (#26106)
## Why The skills extension needs the resolved turn environments to build a real per-turn `SkillListQuery`. The previous `TurnLifecycleContributor` hook only had a turn id, so it could only seed a placeholder query and never carry the executor authorities that executor-scoped skill routing will need. Moving catalog resolution onto `TurnInputContributor` puts the skills extension on the same turn-preparation path that already has the environment ids and working directories for the submitted turn, while keeping the actual prompt injection work for follow-up changes. ## What changed - switch `ext/skills` from `TurnLifecycleContributor` to `TurnInputContributor` - build `executor_authorities` from `TurnInputContext.environments` and pass them through `SkillListQuery` - keep storing the resolved catalog in `SkillsTurnState`, but drop the placeholder query helper that no longer matches the real data flow - update the extension TODOs to reflect that per-turn catalog resolution now happens in the turn-input contributor, and that prompt/context injection still needs to move later ## Testing - Not run locally.
This commit is contained in:
Generated
+1
@@ -2863,6 +2863,7 @@ name = "codex-extension-api"
|
||||
version = "0.0.0"
|
||||
dependencies = [
|
||||
"async-trait",
|
||||
"codex-context-fragments",
|
||||
"codex-protocol",
|
||||
"codex-tools",
|
||||
]
|
||||
|
||||
@@ -19,7 +19,7 @@ impl AdditionalContextUserFragment {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AdditionalContextUserFragment {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
@@ -65,7 +65,7 @@ impl AdditionalContextDeveloperFragment {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AdditionalContextDeveloperFragment {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -44,9 +44,7 @@ impl<T: ContextualUserFragment> FragmentRegistration for FragmentRegistrationPro
|
||||
/// in which case the default helpers render only the body and never match
|
||||
/// arbitrary text.
|
||||
pub trait ContextualUserFragment {
|
||||
fn role() -> &'static str
|
||||
where
|
||||
Self: Sized;
|
||||
fn role(&self) -> &'static str;
|
||||
|
||||
fn markers(&self) -> (&'static str, &'static str);
|
||||
|
||||
@@ -80,7 +78,18 @@ pub trait ContextualUserFragment {
|
||||
{
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: Self::role().to_string(),
|
||||
role: self.role().to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: self.render(),
|
||||
}],
|
||||
phase: None,
|
||||
}
|
||||
}
|
||||
|
||||
fn into_boxed_response_item(self: Box<Self>) -> ResponseItem {
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: self.role().to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: self.render(),
|
||||
}],
|
||||
@@ -93,7 +102,7 @@ pub trait ContextualUserFragment {
|
||||
Self: Sized,
|
||||
{
|
||||
ResponseInputItem::Message {
|
||||
role: Self::role().to_string(),
|
||||
role: self.role().to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: self.render(),
|
||||
}],
|
||||
|
||||
@@ -20,7 +20,7 @@ impl From<&SkillInjection> for SkillInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for SkillInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ impl ApprovedCommandPrefixSaved {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for ApprovedCommandPrefixSaved {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -18,7 +18,7 @@ impl AppsInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AppsInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -22,7 +22,7 @@ impl AvailablePluginsInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AvailablePluginsInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -21,7 +21,7 @@ impl From<AvailableSkills> for AvailableSkillsInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for AvailableSkillsInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -22,7 +22,7 @@ impl CollaborationModeInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for CollaborationModeInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -523,7 +523,7 @@ fn workspace_roots_from_turn_context_item(
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for EnvironmentContext {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ use super::ContextualUserFragment;
|
||||
pub(crate) struct GuardianFollowupReviewReminder;
|
||||
|
||||
impl ContextualUserFragment for GuardianFollowupReviewReminder {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@ impl HookAdditionalContext {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for HookAdditionalContext {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ impl ImageGenerationInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for ImageGenerationInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -76,7 +76,7 @@ impl InternalModelContextFragment {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for InternalModelContextFragment {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ use super::ContextualUserFragment;
|
||||
pub(crate) struct LegacyApplyPatchExecCommandWarning;
|
||||
|
||||
impl ContextualUserFragment for LegacyApplyPatchExecCommandWarning {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ use super::ContextualUserFragment;
|
||||
pub(crate) struct LegacyModelMismatchWarning;
|
||||
|
||||
impl ContextualUserFragment for LegacyModelMismatchWarning {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ use super::ContextualUserFragment;
|
||||
pub(crate) struct LegacyUnifiedExecProcessLimitWarning;
|
||||
|
||||
impl ContextualUserFragment for LegacyUnifiedExecProcessLimitWarning {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ impl ModelSwitchInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for ModelSwitchInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -18,7 +18,7 @@ impl NetworkRuleSaved {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for NetworkRuleSaved {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@ impl PersonalitySpecInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for PersonalitySpecInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@ impl PluginInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for PluginInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ impl RealtimeEndInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for RealtimeEndInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ use codex_protocol::protocol::REALTIME_CONVERSATION_OPEN_TAG;
|
||||
pub(crate) struct RealtimeStartInstructions;
|
||||
|
||||
impl ContextualUserFragment for RealtimeStartInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -16,7 +16,7 @@ impl RealtimeStartWithInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for RealtimeStartWithInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
@@ -18,7 +18,7 @@ impl SubagentNotification {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for SubagentNotification {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ impl TurnAborted {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for TurnAborted {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ pub(crate) struct UserInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for UserInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -27,7 +27,7 @@ impl UserShellCommand {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for UserShellCommand {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
|
||||
@@ -628,7 +628,7 @@ async fn build_extension_turn_input_items(
|
||||
|
||||
let mut items = Vec::new();
|
||||
for contributor in contributors {
|
||||
let contributed_items = contributor
|
||||
let contributed_fragments = contributor
|
||||
.contribute(
|
||||
input.clone(),
|
||||
&sess.services.session_extension_data,
|
||||
@@ -638,7 +638,11 @@ async fn build_extension_turn_input_items(
|
||||
.or_cancel(cancellation_token)
|
||||
.await
|
||||
.ok()?;
|
||||
items.extend(contributed_items);
|
||||
items.extend(
|
||||
contributed_fragments
|
||||
.into_iter()
|
||||
.map(ContextualUserFragment::into_boxed_response_item),
|
||||
);
|
||||
}
|
||||
|
||||
Some(items)
|
||||
|
||||
@@ -15,5 +15,6 @@ workspace = true
|
||||
|
||||
[dependencies]
|
||||
async-trait = { workspace = true }
|
||||
codex-context-fragments = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
codex-tools = { workspace = true }
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
use std::future::Future;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_context_fragments::ContextualUserFragment;
|
||||
use codex_protocol::items::TurnItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::TokenUsageInfo;
|
||||
use codex_tools::ToolCall;
|
||||
@@ -98,14 +98,14 @@ pub trait TurnLifecycleContributor: Send + Sync {
|
||||
/// host, not in this input.
|
||||
#[async_trait::async_trait]
|
||||
pub trait TurnInputContributor: Send + Sync {
|
||||
/// Returns additional model input items for one submitted turn.
|
||||
/// Returns additional contextual fragments for one submitted turn.
|
||||
async fn contribute(
|
||||
&self,
|
||||
input: TurnInputContext,
|
||||
session_store: &ExtensionData,
|
||||
thread_store: &ExtensionData,
|
||||
turn_store: &ExtensionData,
|
||||
) -> Vec<ResponseItem>;
|
||||
) -> Vec<Box<dyn ContextualUserFragment + Send>>;
|
||||
}
|
||||
|
||||
/// Contributor for host-owned configuration changes.
|
||||
|
||||
@@ -10,6 +10,7 @@ pub use capabilities::NoopExtensionEventSink;
|
||||
pub use capabilities::NoopResponseItemInjector;
|
||||
pub use capabilities::ResponseItemInjectionFuture;
|
||||
pub use capabilities::ResponseItemInjector;
|
||||
pub use codex_context_fragments::ContextualUserFragment;
|
||||
pub use codex_protocol::models::ResponseItem;
|
||||
pub use codex_tools::ConversationHistory;
|
||||
pub use codex_tools::ExtensionTurnItem;
|
||||
|
||||
@@ -5,14 +5,17 @@ use std::sync::Arc;
|
||||
use codex_core::config::Config;
|
||||
use codex_extension_api::ConfigContributor;
|
||||
use codex_extension_api::ContextContributor;
|
||||
use codex_extension_api::ContextualUserFragment;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_extension_api::PromptFragment;
|
||||
use codex_extension_api::ThreadLifecycleContributor;
|
||||
use codex_extension_api::ThreadStartInput;
|
||||
use codex_extension_api::TurnLifecycleContributor;
|
||||
use codex_extension_api::TurnStartInput;
|
||||
use codex_extension_api::TurnInputContext;
|
||||
use codex_extension_api::TurnInputContributor;
|
||||
|
||||
use crate::catalog::SkillAuthority;
|
||||
use crate::catalog::SkillSourceKind;
|
||||
use crate::provider::SkillListQuery;
|
||||
use crate::providers::SkillProviders;
|
||||
use crate::state::SkillsExtensionConfig;
|
||||
@@ -29,8 +32,8 @@ impl ThreadLifecycleContributor<Config> for SkillsExtension {
|
||||
// TODO(skills-extension): this is only the thread-level config snapshot.
|
||||
// Skills are loaded per turn today because cwd, plugin roots, config
|
||||
// layers, and the primary environment filesystem can change between
|
||||
// turns. The real migration needs a turn-preparation hook before model
|
||||
// input construction, not just thread startup.
|
||||
// turns. The TurnInputContributor below owns per-turn catalog
|
||||
// resolution.
|
||||
input
|
||||
.thread_store
|
||||
.insert(SkillsExtensionConfig::from_config(input.config));
|
||||
@@ -66,42 +69,50 @@ impl ContextContributor for SkillsExtension {
|
||||
}
|
||||
|
||||
// TODO(skills-extension): render the available-skills developer
|
||||
// block from the merged per-turn SkillCatalog. This should
|
||||
// preserve the existing bounded metadata budget, root aliasing,
|
||||
// warning behavior, and telemetry side effects.
|
||||
// block only if the final model needs thread-level guidance that
|
||||
// does not depend on the per-turn SkillCatalog.
|
||||
//
|
||||
// TODO(skills-extension): avoid using raw PromptFragment strings
|
||||
// for final skills context if the extension API grows typed
|
||||
// contextual fragments. Existing skill blocks are typed so resume
|
||||
// and history filtering can recognize them reliably.
|
||||
//
|
||||
// TODO(skills-extension): ContextContributor currently cannot see
|
||||
// the turn_store, so it cannot read the per-turn catalog seeded by
|
||||
// the turn provider path below. This is the main extension-api gap
|
||||
// to close before skills can move out of codex-core.
|
||||
Vec::new()
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl TurnLifecycleContributor for SkillsExtension {
|
||||
async fn on_turn_start(&self, input: TurnStartInput<'_>) {
|
||||
// TODO(skills-extension): replace this lifecycle callback with a real
|
||||
// turn-input contributor in codex-extension-api. This placeholder only
|
||||
// demonstrates where provider aggregation belongs; it cannot resolve
|
||||
// real skills because this hook does not receive cwd, executor
|
||||
// selections, effective plugins/materialized plugin skill roots,
|
||||
// connector slug counts, user input, cancellation, analytics, or a
|
||||
// response-item output channel.
|
||||
let query = SkillListQuery::placeholder_for_turn(input.turn_id);
|
||||
impl TurnInputContributor for SkillsExtension {
|
||||
async fn contribute(
|
||||
&self,
|
||||
input: TurnInputContext,
|
||||
_session_store: &ExtensionData,
|
||||
_thread_store: &ExtensionData,
|
||||
turn_store: &ExtensionData,
|
||||
) -> Vec<Box<dyn ContextualUserFragment + Send>> {
|
||||
let executor_authorities = input
|
||||
.environments
|
||||
.iter()
|
||||
.map(|environment| {
|
||||
SkillAuthority::new(
|
||||
SkillSourceKind::Executor,
|
||||
environment.environment_id.clone(),
|
||||
)
|
||||
})
|
||||
.collect();
|
||||
let query = SkillListQuery {
|
||||
turn_id: input.turn_id,
|
||||
executor_authorities,
|
||||
include_host_skills: true,
|
||||
include_remote_skills: true,
|
||||
};
|
||||
let catalog = self
|
||||
.providers
|
||||
.list_for_turn(query)
|
||||
.await
|
||||
.unwrap_or_default();
|
||||
|
||||
input.turn_store.insert(SkillsTurnState {
|
||||
turn_store.insert(SkillsTurnState {
|
||||
catalog,
|
||||
entrypoints_injected: false,
|
||||
});
|
||||
@@ -115,7 +126,9 @@ impl TurnLifecycleContributor for SkillsExtension {
|
||||
//
|
||||
// TODO(skills-extension): move explicit $skill mention resolution,
|
||||
// SKILL.md reads, skill body injection, and MCP dependency prompting
|
||||
// out of codex-core's turn assembly once that hook exists.
|
||||
// out of codex-core's turn assembly once provider implementations
|
||||
// are ready to preserve behavior.
|
||||
Vec::new()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -136,5 +149,5 @@ pub fn install(registry: &mut ExtensionRegistryBuilder<Config>) {
|
||||
registry.thread_lifecycle_contributor(extension.clone());
|
||||
registry.config_contributor(extension.clone());
|
||||
registry.prompt_contributor(extension.clone());
|
||||
registry.turn_lifecycle_contributor(extension);
|
||||
registry.turn_input_contributor(extension);
|
||||
}
|
||||
|
||||
@@ -16,17 +16,6 @@ pub struct SkillListQuery {
|
||||
pub include_remote_skills: bool,
|
||||
}
|
||||
|
||||
impl SkillListQuery {
|
||||
pub(crate) fn placeholder_for_turn(turn_id: &str) -> Self {
|
||||
Self {
|
||||
turn_id: turn_id.to_string(),
|
||||
executor_authorities: Vec::new(),
|
||||
include_host_skills: true,
|
||||
include_remote_skills: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub struct SkillReadRequest {
|
||||
pub authority: SkillAuthority,
|
||||
|
||||
@@ -143,7 +143,7 @@ impl PermissionsInstructions {
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for PermissionsInstructions {
|
||||
fn role() -> &'static str {
|
||||
fn role(&self) -> &'static str {
|
||||
"developer"
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user