mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Use internal model context fragments for goal steering (#24918)
## Why Goal steering is one form of runtime-owned model context, but the old `<goal_context>` wrapper made the contextual-fragment hiding path goal-specific. Using a source-labeled internal context fragment gives core and extensions a shared shape for hidden model steering while keeping those prompts out of visible turn history. The change also keeps legacy `<goal_context>` messages recognized as hidden contextual input so existing stored history does not start rendering old goal-steering prompts as user-visible turn items. ## What Changed - Replaces `GoalContext` with `InternalModelContextFragment` plus a validated `InternalContextSource`. - Renders goal steering as `<codex_internal_context source="goal">...</codex_internal_context>`. - Updates core goal steering and `ext/goal` steering to inject the new internal-context fragment. - Updates contextual-fragment, event-mapping, goal, and session tests for the new wrapper. ## Test Coverage - Adds coverage for detecting the new internal model context fragment. - Preserves coverage for hiding legacy `<goal_context>` fragments. - Verifies invalid internal context sources are rejected and arbitrary context tags are not hidden. - Updates goal steering/session assertions to expect the new `source="goal"` wrapper.
This commit is contained in:
@@ -6,7 +6,7 @@ use super::AdditionalContextUserFragment;
|
||||
use super::EnvironmentContext;
|
||||
use super::FragmentRegistration;
|
||||
use super::FragmentRegistrationProxy;
|
||||
use super::GoalContext;
|
||||
use super::InternalModelContextFragment;
|
||||
use super::LegacyApplyPatchExecCommandWarning;
|
||||
use super::LegacyModelMismatchWarning;
|
||||
use super::LegacyUnifiedExecProcessLimitWarning;
|
||||
@@ -30,8 +30,9 @@ static TURN_ABORTED_REGISTRATION: FragmentRegistrationProxy<TurnAborted> =
|
||||
FragmentRegistrationProxy::new();
|
||||
static SUBAGENT_NOTIFICATION_REGISTRATION: FragmentRegistrationProxy<SubagentNotification> =
|
||||
FragmentRegistrationProxy::new();
|
||||
static GOAL_CONTEXT_REGISTRATION: FragmentRegistrationProxy<GoalContext> =
|
||||
FragmentRegistrationProxy::new();
|
||||
static INTERNAL_MODEL_CONTEXT_REGISTRATION: FragmentRegistrationProxy<
|
||||
InternalModelContextFragment,
|
||||
> = FragmentRegistrationProxy::new();
|
||||
static LEGACY_UNIFIED_EXEC_PROCESS_LIMIT_WARNING_REGISTRATION: FragmentRegistrationProxy<
|
||||
LegacyUnifiedExecProcessLimitWarning,
|
||||
> = FragmentRegistrationProxy::new();
|
||||
@@ -50,7 +51,7 @@ static CONTEXTUAL_USER_FRAGMENTS: &[&dyn FragmentRegistration] = &[
|
||||
&USER_SHELL_COMMAND_REGISTRATION,
|
||||
&TURN_ABORTED_REGISTRATION,
|
||||
&SUBAGENT_NOTIFICATION_REGISTRATION,
|
||||
&GOAL_CONTEXT_REGISTRATION,
|
||||
&INTERNAL_MODEL_CONTEXT_REGISTRATION,
|
||||
&LEGACY_UNIFIED_EXEC_PROCESS_LIMIT_WARNING_REGISTRATION,
|
||||
&LEGACY_APPLY_PATCH_EXEC_COMMAND_WARNING_REGISTRATION,
|
||||
&LEGACY_MODEL_MISMATCH_WARNING_REGISTRATION,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::*;
|
||||
use crate::context::ContextualUserFragment;
|
||||
use crate::context::GoalContext;
|
||||
use crate::context::InternalContextSource;
|
||||
use crate::context::InternalModelContextFragment;
|
||||
use crate::context::SubagentNotification;
|
||||
use codex_protocol::items::HookPromptFragment;
|
||||
use codex_protocol::items::build_hook_prompt_message;
|
||||
@@ -30,23 +31,55 @@ fn detects_subagent_notification_fragment_case_insensitively() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_goal_context_fragment() {
|
||||
let text = GoalContext::new("Continue working toward the active thread goal.").render();
|
||||
fn detects_internal_model_context_fragment() {
|
||||
let text = InternalModelContextFragment::new(
|
||||
InternalContextSource::from_static("goal"),
|
||||
"Continue working toward the active thread goal.",
|
||||
)
|
||||
.render();
|
||||
|
||||
assert_eq!(
|
||||
text,
|
||||
"<codex_internal_context source=\"goal\">\nContinue working toward the active thread goal.\n</codex_internal_context>"
|
||||
);
|
||||
assert!(is_contextual_user_fragment(&ContentItem::InputText {
|
||||
text
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_legacy_goal_context_fragment() {
|
||||
assert!(is_contextual_user_fragment(&ContentItem::InputText {
|
||||
text: "<goal_context>\nContinue working toward the active thread goal.\n</goal_context>"
|
||||
.to_string(),
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn does_not_hide_arbitrary_context_tags() {
|
||||
assert!(!is_contextual_user_fragment(&ContentItem::InputText {
|
||||
text: "<project_context>\nbody\n</project_context>".to_string(),
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_invalid_internal_model_context_source() {
|
||||
assert!(!is_contextual_user_fragment(&ContentItem::InputText {
|
||||
text: "<codex_internal_context source=\"Goal\">\nbody\n</codex_internal_context>"
|
||||
.to_string(),
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn contextual_user_fragment_is_dyn_compatible() {
|
||||
let fragment: Box<dyn ContextualUserFragment> = Box::new(GoalContext::new(
|
||||
let fragment: Box<dyn ContextualUserFragment> = Box::new(InternalModelContextFragment::new(
|
||||
InternalContextSource::from_static("goal"),
|
||||
"Continue working toward the active thread goal.",
|
||||
));
|
||||
|
||||
assert_eq!(
|
||||
fragment.render(),
|
||||
"<goal_context>\nContinue working toward the active thread goal.\n</goal_context>"
|
||||
"<codex_internal_context source=\"goal\">\nContinue working toward the active thread goal.\n</codex_internal_context>"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -1,36 +0,0 @@
|
||||
//! Hidden user-context fragment for runtime-owned goal steering prompts.
|
||||
|
||||
use super::ContextualUserFragment;
|
||||
|
||||
/// Hidden runtime-owned goal steering context injected into model input.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub struct GoalContext {
|
||||
prompt: String,
|
||||
}
|
||||
|
||||
impl GoalContext {
|
||||
/// Creates goal context around an already-rendered steering prompt.
|
||||
pub fn new(prompt: impl Into<String>) -> Self {
|
||||
Self {
|
||||
prompt: prompt.into(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for GoalContext {
|
||||
fn role() -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
fn markers(&self) -> (&'static str, &'static str) {
|
||||
Self::type_markers()
|
||||
}
|
||||
|
||||
fn type_markers() -> (&'static str, &'static str) {
|
||||
("<goal_context>", "</goal_context>")
|
||||
}
|
||||
|
||||
fn body(&self) -> String {
|
||||
format!("\n{}\n", self.prompt)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,129 @@
|
||||
//! Hidden user-context fragment for extension-owned model steering.
|
||||
|
||||
use super::ContextualUserFragment;
|
||||
use std::error::Error;
|
||||
use std::fmt;
|
||||
|
||||
const CONTEXT_START_MARKER: &str = "<codex_internal_context";
|
||||
const CONTEXT_END_MARKER: &str = "</codex_internal_context>";
|
||||
const LEGACY_GOAL_CONTEXT_START_MARKER: &str = "<goal_context>";
|
||||
const LEGACY_GOAL_CONTEXT_END_MARKER: &str = "</goal_context>";
|
||||
const SOURCE_ATTR_START: &str = " source=\"";
|
||||
const SOURCE_ATTR_END: &str = "\">";
|
||||
|
||||
/// Source label for hidden internal model context.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct InternalContextSource(String);
|
||||
|
||||
impl InternalContextSource {
|
||||
/// Creates a source label for an internal model-context fragment.
|
||||
///
|
||||
/// Sources are intentionally constrained so the value can be embedded in the
|
||||
/// wrapper without escaping and still remain easy to audit in stored history.
|
||||
pub fn new(source: impl Into<String>) -> Result<Self, InvalidInternalContextSource> {
|
||||
let source = source.into();
|
||||
if is_valid_source(&source) {
|
||||
Ok(Self(source))
|
||||
} else {
|
||||
Err(InvalidInternalContextSource { source })
|
||||
}
|
||||
}
|
||||
|
||||
/// Creates a source label from a trusted static string.
|
||||
pub fn from_static(source: &'static str) -> Self {
|
||||
Self::new(source)
|
||||
.unwrap_or_else(|_| panic!("invalid static internal context source: {source}"))
|
||||
}
|
||||
|
||||
pub fn as_str(&self) -> &str {
|
||||
&self.0
|
||||
}
|
||||
}
|
||||
|
||||
/// Error returned when an internal model-context source is invalid.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct InvalidInternalContextSource {
|
||||
source: String,
|
||||
}
|
||||
|
||||
impl fmt::Display for InvalidInternalContextSource {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
let source = &self.source;
|
||||
write!(
|
||||
f,
|
||||
"invalid internal model context source {source:?}; expected [a-z][a-z0-9_]*"
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
impl Error for InvalidInternalContextSource {}
|
||||
|
||||
/// Hidden runtime-owned context injected into model input.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct InternalModelContextFragment {
|
||||
source: InternalContextSource,
|
||||
body: String,
|
||||
}
|
||||
|
||||
impl InternalModelContextFragment {
|
||||
/// Creates hidden model context with an extension-owned source label.
|
||||
pub fn new(source: InternalContextSource, body: impl Into<String>) -> Self {
|
||||
Self {
|
||||
source,
|
||||
body: body.into(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl ContextualUserFragment for InternalModelContextFragment {
|
||||
fn role() -> &'static str {
|
||||
"user"
|
||||
}
|
||||
|
||||
fn markers(&self) -> (&'static str, &'static str) {
|
||||
Self::type_markers()
|
||||
}
|
||||
|
||||
fn type_markers() -> (&'static str, &'static str) {
|
||||
(CONTEXT_START_MARKER, CONTEXT_END_MARKER)
|
||||
}
|
||||
|
||||
fn matches_text(text: &str) -> bool {
|
||||
let trimmed = text.trim();
|
||||
if matches_legacy_goal_context(trimmed) {
|
||||
return true;
|
||||
}
|
||||
|
||||
let Some(rest) = trimmed.strip_prefix(CONTEXT_START_MARKER) else {
|
||||
return false;
|
||||
};
|
||||
let Some(rest) = rest.strip_prefix(SOURCE_ATTR_START) else {
|
||||
return false;
|
||||
};
|
||||
let Some((source, body_and_close)) = rest.split_once(SOURCE_ATTR_END) else {
|
||||
return false;
|
||||
};
|
||||
|
||||
is_valid_source(source) && body_and_close.ends_with(CONTEXT_END_MARKER)
|
||||
}
|
||||
|
||||
fn body(&self) -> String {
|
||||
let source = self.source.as_str();
|
||||
let body = &self.body;
|
||||
format!(" source=\"{source}\">\n{body}\n")
|
||||
}
|
||||
}
|
||||
|
||||
fn matches_legacy_goal_context(text: &str) -> bool {
|
||||
text.starts_with(LEGACY_GOAL_CONTEXT_START_MARKER)
|
||||
&& text.ends_with(LEGACY_GOAL_CONTEXT_END_MARKER)
|
||||
}
|
||||
|
||||
fn is_valid_source(source: &str) -> bool {
|
||||
let mut chars = source.chars();
|
||||
let Some(first) = chars.next() else {
|
||||
return false;
|
||||
};
|
||||
first.is_ascii_lowercase()
|
||||
&& chars.all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_')
|
||||
}
|
||||
@@ -9,10 +9,10 @@ mod contextual_user_message;
|
||||
mod environment_context;
|
||||
mod fragment;
|
||||
mod fragments;
|
||||
mod goal_context;
|
||||
mod guardian_followup_review_reminder;
|
||||
mod hook_additional_context;
|
||||
mod image_generation_instructions;
|
||||
mod internal_model_context;
|
||||
mod legacy_apply_patch_exec_command_warning;
|
||||
mod legacy_model_mismatch_warning;
|
||||
mod legacy_unified_exec_process_limit_warning;
|
||||
@@ -43,10 +43,12 @@ pub(crate) use fragment::FragmentRegistration;
|
||||
pub(crate) use fragment::FragmentRegistrationProxy;
|
||||
pub(crate) use fragments::AdditionalContextDeveloperFragment;
|
||||
pub(crate) use fragments::AdditionalContextUserFragment;
|
||||
pub use goal_context::GoalContext;
|
||||
pub(crate) use guardian_followup_review_reminder::GuardianFollowupReviewReminder;
|
||||
pub(crate) use hook_additional_context::HookAdditionalContext;
|
||||
pub(crate) use image_generation_instructions::ImageGenerationInstructions;
|
||||
pub use internal_model_context::InternalContextSource;
|
||||
pub use internal_model_context::InternalModelContextFragment;
|
||||
pub use internal_model_context::InvalidInternalContextSource;
|
||||
pub(crate) use legacy_apply_patch_exec_command_warning::LegacyApplyPatchExecCommandWarning;
|
||||
pub(crate) use legacy_model_mismatch_warning::LegacyModelMismatchWarning;
|
||||
pub(crate) use legacy_unified_exec_process_limit_warning::LegacyUnifiedExecProcessLimitWarning;
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::parse_turn_item;
|
||||
use crate::context::ContextualUserFragment;
|
||||
use crate::context::GoalContext;
|
||||
use crate::context::InternalContextSource;
|
||||
use crate::context::InternalModelContextFragment;
|
||||
use codex_protocol::items::AgentMessageContent;
|
||||
use codex_protocol::items::HookPromptFragment;
|
||||
use codex_protocol::items::TurnItem;
|
||||
@@ -317,12 +318,16 @@ fn parses_hook_prompt_and_hides_other_contextual_fragments() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn goal_context_does_not_parse_as_visible_turn_item() {
|
||||
fn internal_model_context_does_not_parse_as_visible_turn_item() {
|
||||
let item = ResponseItem::Message {
|
||||
id: Some("msg-1".to_string()),
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: GoalContext::new("Continue working toward the active thread goal.").render(),
|
||||
text: InternalModelContextFragment::new(
|
||||
InternalContextSource::from_static("goal"),
|
||||
"Continue working toward the active thread goal.",
|
||||
)
|
||||
.render(),
|
||||
}],
|
||||
phase: None,
|
||||
};
|
||||
|
||||
@@ -6,7 +6,8 @@
|
||||
|
||||
use crate::StateDbHandle;
|
||||
use crate::context::ContextualUserFragment;
|
||||
use crate::context::GoalContext;
|
||||
use crate::context::InternalContextSource;
|
||||
use crate::context::InternalModelContextFragment;
|
||||
use crate::session::TurnInput;
|
||||
use crate::session::session::Session;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
@@ -1597,7 +1598,10 @@ fn budget_limit_steering_item(goal: &ThreadGoal) -> ResponseItem {
|
||||
}
|
||||
|
||||
fn goal_context_input_item(prompt: String) -> ResponseItem {
|
||||
ResponseItem::from(GoalContext::new(prompt).into_response_input_item())
|
||||
ContextualUserFragment::into(InternalModelContextFragment::new(
|
||||
InternalContextSource::from_static("goal"),
|
||||
prompt,
|
||||
))
|
||||
}
|
||||
|
||||
pub(crate) fn protocol_goal_from_state(goal: codex_state::ThreadGoal) -> ThreadGoal {
|
||||
@@ -1799,7 +1803,7 @@ mod tests {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "<goal_context>\nContinue working.\n</goal_context>".to_string(),
|
||||
text: "<codex_internal_context source=\"goal\">\nContinue working.\n</codex_internal_context>".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
}
|
||||
|
||||
@@ -8710,24 +8710,13 @@ async fn active_goal_continuation_runs_again_after_no_tool_turn() -> anyhow::Res
|
||||
})
|
||||
.await??;
|
||||
|
||||
let continuation_request = responses
|
||||
let goal_context_text = responses
|
||||
.requests()
|
||||
.into_iter()
|
||||
.find(|request| request.body_contains_text("<goal_context>"))
|
||||
.expect("expected a goal continuation request");
|
||||
let body = continuation_request.body_json();
|
||||
let goal_context_message = body["input"]
|
||||
.as_array()
|
||||
.expect("input should be an array")
|
||||
.iter()
|
||||
.find(|item| item.to_string().contains("<goal_context>"))
|
||||
.flat_map(|request| request.message_input_texts("user"))
|
||||
.find(|text| text.contains("<codex_internal_context source=\"goal\">"))
|
||||
.expect("goal context message should be present");
|
||||
assert_eq!(goal_context_message["role"].as_str(), Some("user"));
|
||||
assert!(
|
||||
goal_context_message
|
||||
.to_string()
|
||||
.contains("Continue working toward the active thread goal.")
|
||||
);
|
||||
assert!(goal_context_text.contains("Continue working toward the active thread goal."));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -8997,8 +8986,8 @@ async fn budget_limited_accounting_steers_active_turn_without_aborting() -> anyh
|
||||
let [ContentItem::InputText { text }] = content.as_slice() else {
|
||||
panic!("expected one text span in budget-limit steering message, got {content:#?}");
|
||||
};
|
||||
assert!(text.starts_with("<goal_context>"));
|
||||
assert!(text.trim_end().ends_with("</goal_context>"));
|
||||
assert!(text.starts_with("<codex_internal_context source=\"goal\">"));
|
||||
assert!(text.trim_end().ends_with("</codex_internal_context>"));
|
||||
assert!(text.contains("budget_limited"));
|
||||
assert!(text.to_lowercase().contains("wrap up this turn soon"));
|
||||
assert!(sess.active_turn.lock().await.is_some());
|
||||
@@ -9220,8 +9209,8 @@ async fn external_objective_change_steers_active_turn() -> anyhow::Result<()> {
|
||||
&& content.iter().any(|content| matches!(
|
||||
content,
|
||||
ContentItem::InputText { text }
|
||||
if text.starts_with("<goal_context>")
|
||||
&& text.trim_end().ends_with("</goal_context>")
|
||||
if text.starts_with("<codex_internal_context source=\"goal\">")
|
||||
&& text.trim_end().ends_with("</codex_internal_context>")
|
||||
&& text.contains("The active thread goal objective was edited")
|
||||
&& text.contains("Write a concise benchmark summary")
|
||||
))
|
||||
|
||||
@@ -1,14 +1,20 @@
|
||||
use codex_core::context::ContextualUserFragment;
|
||||
use codex_core::context::GoalContext;
|
||||
use codex_core::context::InternalContextSource;
|
||||
use codex_core::context::InternalModelContextFragment;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::protocol::ThreadGoal;
|
||||
|
||||
pub(crate) fn budget_limit_steering_item(goal: &ThreadGoal) -> ResponseInputItem {
|
||||
GoalContext::new(budget_limit_prompt(goal)).into_response_input_item()
|
||||
goal_context_input_item(budget_limit_prompt(goal))
|
||||
}
|
||||
|
||||
pub(crate) fn objective_updated_steering_item(goal: &ThreadGoal) -> ResponseInputItem {
|
||||
GoalContext::new(objective_updated_prompt(goal)).into_response_input_item()
|
||||
goal_context_input_item(objective_updated_prompt(goal))
|
||||
}
|
||||
|
||||
fn goal_context_input_item(prompt: String) -> ResponseInputItem {
|
||||
InternalModelContextFragment::new(InternalContextSource::from_static("goal"), prompt)
|
||||
.into_response_input_item()
|
||||
}
|
||||
|
||||
fn budget_limit_prompt(goal: &ThreadGoal) -> String {
|
||||
|
||||
Reference in New Issue
Block a user