Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions codex-rs/core/src/context/world_state/agents_md.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::PreviousSectionState;
use super::WorldStateSection;
use crate::agents_md::LoadedAgentsMd;
use crate::context::ContextualUserFragment;
Expand Down Expand Up @@ -44,27 +45,35 @@ impl WorldStateSection for AgentsMdState {
}
}

fn matches_legacy_fragment(role: &str, text: &str) -> bool {
role == "user" && UserInstructions::matches_text(text)
}

fn render_diff(
&self,
previous: Option<&Self::Snapshot>,
previous: PreviousSectionState<'_, Self::Snapshot>,
) -> Option<Box<dyn ContextualUserFragment>> {
let current = self.snapshot();
if previous == Some(&current) {
if matches!(previous, PreviousSectionState::Known(previous) if previous == &current) {
return None;
}

let previous_instructions = previous.and_then(|state| state.text.as_ref());
let instructions = match (&self.instructions, previous_instructions) {
(Some(instructions), Some(_)) => UserInstructions {
let previous_may_contain_instructions = match previous {
PreviousSectionState::Known(previous) => previous.text.is_some(),
PreviousSectionState::Unknown => true,
PreviousSectionState::Absent => false,
};
let instructions = match (&self.instructions, previous_may_contain_instructions) {
(Some(instructions), true) => UserInstructions {
directory: instructions.directory.clone(),
text: format!("{REPLACEMENT_NOTICE}\n\n{}", instructions.text),
},
(Some(instructions), None) => instructions.clone(),
(None, Some(_)) => UserInstructions {
(Some(instructions), false) => instructions.clone(),
(None, true) => UserInstructions {
directory: None,
text: REMOVAL_NOTICE.to_string(),
},
(None, None) => return None,
(None, false) => return None,
};
Some(Box::new(instructions))
}
Expand Down
28 changes: 28 additions & 0 deletions codex-rs/core/src/context/world_state/agents_md_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,34 @@ fn changed_and_removed_state_supersedes_previous_instructions() {
);
}

#[test]
fn unknown_previous_state_is_explicitly_superseded() {
let loaded = LoadedAgentsMd::from_text_for_testing("current instructions");
let current = AgentsMdState::new(Some(&loaded));
assert_eq!(
vec![user_message(
"# AGENTS.md instructions\n\n<INSTRUCTIONS>\nThese AGENTS.md instructions replace all previously provided AGENTS.md instructions.\n\ncurrent instructions\n</INSTRUCTIONS>",
)],
render_fragments(vec![
WorldStateSection::render_diff(&current, PreviousSectionState::Unknown)
.expect("unknown state should be replaced"),
]),
);

assert_eq!(
vec![user_message(
"# AGENTS.md instructions\n\n<INSTRUCTIONS>\nThe previously provided AGENTS.md instructions no longer apply.\n</INSTRUCTIONS>",
)],
render_fragments(vec![
WorldStateSection::render_diff(
&AgentsMdState::default(),
PreviousSectionState::Unknown,
)
.expect("unknown state should be removed"),
]),
);
}

fn render_fragments(fragments: Vec<Box<dyn ContextualUserFragment>>) -> Vec<ResponseItem> {
fragments
.into_iter()
Expand Down
8 changes: 6 additions & 2 deletions codex-rs/core/src/context/world_state/environment.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::PreviousSectionState;
use super::WorldStateSection;
use crate::context::ContextualUserFragment;
use crate::context::environment_context::FileSystemContext;
Expand Down Expand Up @@ -95,11 +96,14 @@ impl WorldStateSection for EnvironmentsState {

fn render_diff(
&self,
previous: Option<&Self::Snapshot>,
previous: PreviousSectionState<'_, Self::Snapshot>,
) -> Option<Box<dyn ContextualUserFragment>> {
let current = self.snapshot();
let empty = EnvironmentsSnapshot::default();
let previous = previous.unwrap_or(&empty);
let previous = match previous {
PreviousSectionState::Known(previous) => previous,
PreviousSectionState::Absent | PreviousSectionState::Unknown => &empty,
};
let turn_context_values_changed = current.current_date != previous.current_date
|| current.timezone != previous.timezone
|| current.network != previous.network
Expand Down
7 changes: 5 additions & 2 deletions codex-rs/core/src/context/world_state/environment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,10 @@ fn single_environment_diff_ignores_unknown_shell() -> Result<()> {

assert_eq!(
None,
render_fragment(WorldStateSection::render_diff(&current, Some(&previous)))
render_fragment(WorldStateSection::render_diff(
&current,
PreviousSectionState::Known(&previous),
))
);
Ok(())
}
Expand Down Expand Up @@ -248,7 +251,7 @@ fn removed_legacy_environment_renders_unavailable() -> Result<()> {
)),
render_fragment(WorldStateSection::render_diff(
&EnvironmentsState::default(),
Some(&previous),
PreviousSectionState::Known(&previous),
)),
);
Ok(())
Expand Down
121 changes: 103 additions & 18 deletions codex-rs/core/src/context/world_state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ mod agents_md;
mod environment;

use crate::context::ContextualUserFragment;
use codex_protocol::models::ContentItem;
use codex_protocol::models::ResponseItem;
use indexmap::IndexMap;
use serde::Serialize;
use serde::de::DeserializeOwned;
Expand All @@ -16,7 +18,12 @@ pub(crate) use environment::EnvironmentsState;
trait ErasedWorldStateSection: Send + Sync {
fn snapshot(&self) -> Option<Value>;

fn render_diff(&self, previous: Option<&Value>) -> Option<Box<dyn ContextualUserFragment>>;
fn matches_legacy_fragment(&self, role: &str, text: &str) -> bool;

fn render_diff(
&self,
previous: PreviousSectionState<'_, Value>,
) -> Option<Box<dyn ContextualUserFragment>>;
}

impl<S: WorldStateSection> ErasedWorldStateSection for S {
Expand All @@ -43,38 +50,71 @@ impl<S: WorldStateSection> ErasedWorldStateSection for S {
Some(snapshot)
}

fn render_diff(&self, previous: Option<&Value>) -> Option<Box<dyn ContextualUserFragment>> {
let previous = previous.and_then(|previous| {
serde_json::from_value::<S::Snapshot>(previous.clone())
.inspect_err(|err| {
tracing::warn!(
section_id = S::ID,
%err,
"failed to restore world-state section snapshot"
);
})
.ok()
});
WorldStateSection::render_diff(self, previous.as_ref())
fn matches_legacy_fragment(&self, role: &str, text: &str) -> bool {
S::matches_legacy_fragment(role, text)
}

fn render_diff(
&self,
previous: PreviousSectionState<'_, Value>,
) -> Option<Box<dyn ContextualUserFragment>> {
let typed_snapshot;
let previous = match previous {
PreviousSectionState::Known(previous) => {
match serde_json::from_value::<S::Snapshot>(previous.clone()) {
Ok(previous) => {
typed_snapshot = previous;
PreviousSectionState::Known(&typed_snapshot)
}
Err(err) => {
tracing::warn!(
section_id = S::ID,
%err,
"failed to restore world-state section snapshot"
);
PreviousSectionState::Unknown
}
}
}
PreviousSectionState::Absent => PreviousSectionState::Absent,
PreviousSectionState::Unknown => PreviousSectionState::Unknown,
};
WorldStateSection::render_diff(self, previous)
}
}

/// What is known about a section's previously model-visible state.
pub(crate) enum PreviousSectionState<'a, T> {
/// No persisted snapshot or matching fragment exists in retained history.
Absent,
/// Retained history contains the section, but its typed snapshot is unavailable.
Unknown,
/// The exact persisted snapshot is available.
Known(&'a T),
}

/// A typed portion of the state visible to the model.
///
/// Implementations own how their current state is rendered relative to an
/// earlier snapshot of the same section. `ID` is persisted in rollouts and
/// must remain stable. `Snapshot` should contain only the comparison data
/// needed to decide what the model must be told next, and must not serialize
/// to null because merge-patch nulls represent deletion.
/// to null because merge-patch nulls represent deletion. Sections migrated
/// from older context can recognize their previous fragments through
/// `matches_legacy_fragment`.
pub(crate) trait WorldStateSection: Send + Sync + 'static {
const ID: &'static str;
type Snapshot: DeserializeOwned + Serialize;

fn snapshot(&self) -> Self::Snapshot;

fn matches_legacy_fragment(_role: &str, _text: &str) -> bool {
false
}

fn render_diff(
&self,
previous: Option<&Self::Snapshot>,
previous: PreviousSectionState<'_, Self::Snapshot>,
) -> Option<Box<dyn ContextualUserFragment>>;
}

Expand Down Expand Up @@ -143,21 +183,66 @@ impl WorldState {
}
}

/// Renders every section as new, without any known previous state.
pub(crate) fn render_full(&self) -> Vec<Box<dyn ContextualUserFragment>> {
self.render_diff(&WorldStateSnapshot::default())
self.render_with(|_, _| PreviousSectionState::Absent)
Comment thread
sayan-oai marked this conversation as resolved.
}

/// Renders each section against the exact persisted snapshot when available.
pub(crate) fn render_diff(
&self,
previous: &WorldStateSnapshot,
) -> Vec<Box<dyn ContextualUserFragment>> {
self.render_with(|id, _| match previous.sections.get(id) {
Some(previous) => PreviousSectionState::Known(previous),
None => PreviousSectionState::Absent,
})
}

/// Falls back to retained model history when no exact persisted snapshot is available.
pub(crate) fn render_history_diff(
&self,
previous: Option<&WorldStateSnapshot>,
items: &[ResponseItem],
) -> Vec<Box<dyn ContextualUserFragment>> {
self.render_with(|id, section| {
if let Some(previous) = previous.and_then(|previous| previous.sections.get(id)) {
PreviousSectionState::Known(previous)
} else if has_legacy_fragment(items, section) {
PreviousSectionState::Unknown
Comment thread
sayan-oai marked this conversation as resolved.
} else {
PreviousSectionState::Absent
}
})
}

fn render_with<'a>(
&self,
mut previous: impl FnMut(&str, &dyn ErasedWorldStateSection) -> PreviousSectionState<'a, Value>,
) -> Vec<Box<dyn ContextualUserFragment>> {
self.sections
.iter()
.filter_map(|(id, section)| section.render_diff(previous.sections.get(*id)))
.filter_map(|(id, section)| section.render_diff(previous(id, section.as_ref())))
.collect()
}
}

fn has_legacy_fragment(items: &[ResponseItem], section: &dyn ErasedWorldStateSection) -> bool {
items.iter().any(|item| {
matches!(
item,
ResponseItem::Message { role, content, .. }
if content.iter().any(|content| {
matches!(
content,
ContentItem::InputText { text }
if section.matches_legacy_fragment(role, text)
)
})
)
})
}

fn remove_null_object_fields(value: &mut Value) {
// RFC 7386 reserves object-valued nulls for deletion, but arrays are replaced whole.
match value {
Expand Down
37 changes: 32 additions & 5 deletions codex-rs/core/src/context/world_state/world_state_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,15 @@ impl WorldStateSection for TestSection {

fn render_diff(
&self,
previous: Option<&Self::Snapshot>,
previous: PreviousSectionState<'_, Self::Snapshot>,
) -> Option<Box<dyn ContextualUserFragment>> {
let previous = previous?;
(self.value != previous.value)
.then(|| Box::new(TestFragment(self.value.clone())) as Box<dyn ContextualUserFragment>)
match previous {
PreviousSectionState::Known(previous) if self.value != previous.value => {
Some(Box::new(TestFragment(self.value.clone())))
}
PreviousSectionState::Unknown => Some(Box::new(TestFragment("unknown".to_string()))),
PreviousSectionState::Absent | PreviousSectionState::Known(_) => None,
}
}
}

Expand Down Expand Up @@ -59,7 +63,7 @@ impl WorldStateSection for DuplicateTestSection {

fn render_diff(
&self,
_previous: Option<&Self::Snapshot>,
_previous: PreviousSectionState<'_, Self::Snapshot>,
) -> Option<Box<dyn ContextualUserFragment>> {
None
}
Expand Down Expand Up @@ -106,6 +110,29 @@ fn render_diff_restores_the_typed_section_snapshot() {
);
}

#[test]
fn unreadable_section_snapshot_is_treated_as_unknown() {
let mut current = WorldState::default();
current.add_section(TestSection {
value: "current".to_string(),
optional: None,
array: Vec::new(),
});
let previous = WorldStateSnapshot {
sections: BTreeMap::from([("test".to_string(), json!({"invalid": true}))]),
};

let rendered = current.render_diff(&previous);

assert_eq!(
vec!["unknown"],
rendered
.into_iter()
.map(|fragment| fragment.body())
.collect::<Vec<_>>()
);
}

#[test]
#[should_panic(expected = "duplicate world-state section ID: test")]
fn duplicate_section_ids_are_rejected() {
Expand Down
20 changes: 7 additions & 13 deletions codex-rs/core/src/context_manager/history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,14 @@ impl ContextManager {
world_state: &WorldState,
) -> (Vec<Box<dyn ContextualUserFragment>>, Option<WorldStateItem>) {
let snapshot = world_state.snapshot();
let (fragments, rollout_item) = self.world_state_baseline.as_ref().map_or_else(
|| {
(
world_state.render_full(),
Some(WorldStateItem::full(snapshot.clone().into_value())),
)
},
let fragments =
world_state.render_history_diff(self.world_state_baseline.as_ref(), &self.items);
let rollout_item = self.world_state_baseline.as_ref().map_or_else(
|| Some(WorldStateItem::full(snapshot.clone().into_value())),
|previous| {
(
world_state.render_diff(previous),
snapshot
.merge_patch_from(previous)
.map(WorldStateItem::patch),
)
snapshot
.merge_patch_from(previous)
.map(WorldStateItem::patch)
},
);
self.world_state_baseline = Some(snapshot);
Expand Down
Loading
Loading