Skip to content

fix(config): Record a change to compaction rules and labels #1133

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.

Sign up for GitHub

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

Open
JeanMertz wants to merge 3 commits into from
Open

fix(config): Record a change to compaction rules and labels #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b00509
fix(config): Record a change to the compaction rules
JeanMertz Sep 7, 2026
684c4be
fix(config): Let a workspace label rule reach an old conversation
JeanMertz Sep 7, 2026
52498cb
test(llm): Drop the label wrapper from conversation snapshots
JeanMertz Sep 7, 2026
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
6 changes: 4 additions & 2 deletions crates/jp_config/src/conversation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::{
internal::merge::{map_with_strategy, vec_with_strategy},
partial::{ToPartial, partial_opt},
types::{
map::{MergeableMap, map_to_mergeable_partial},
map::{MergeableMap, map_to_partial_per_key},
vec::{MergeableVec, MergedVec, vec_to_mergeable_partial},
},
validate::Validator,
Expand Down Expand Up @@ -201,7 +201,9 @@ impl ToPartial for ConversationConfig {
tools: self.tools.to_partial(),
compaction: self.compaction.to_partial(),
attachments: vec_to_mergeable_partial(&self.attachments),
labels: map_to_mergeable_partial(self.labels.iter()),
// Per key rather than `replace`: a label rule the workspace config
// gained after this conversation was created still reaches it.
labels: map_to_partial_per_key(self.labels.iter()),
inquiry: self.inquiry.to_partial(),
start_local: partial_opt(&self.start_local, defaults.start_local),
default_id: self.default_id.clone(),
Expand Down
27 changes: 10 additions & 17 deletions crates/jp_config/src/conversation/compaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,16 @@ impl AssignKeyValue for PartialCompactionConfig {
impl PartialConfigDelta for PartialCompactionConfig {
fn delta(&self, next: Self) -> Self {
Self {
// Not `delta_mergeable_vec`, which would say `replace` for any
// difference: this field's partial is a bare `MergeableVec`, so an
// empty one is both "the user said nothing about rules" and "the
// user asked for no rules". A sparse partial (a `--model` override,
// say) carries the empty one, and replacing with it would record
// zero rules the user never asked for, making a later
// `jp conversation compact` a no-op.
//
// Telling the two apart needs the field's partial to be an
// `Option<MergeableVec<_>>`, as every converted list field has,
// where `None` is absent and `Some([])` is a deliberate empty.
rules: {
next.rules
.into_iter()
.filter(|v| !self.rules.contains(v))
.collect::<Vec<_>>()
.into()
// An empty `rules` says nothing about rules rather than asking for
// none: `fill_from` substitutes the built-in defaults for it, so
// that is what the config language already means by an empty list.
// Every sparse partial reaching this delta carries one, and
// replacing with it would record zero rules the user never asked
// for, making a later `jp conversation compact` do nothing.
rules: if next.rules.is_empty() {
MergeableVec::default()
} else {
crate::delta::delta_mergeable_vec(&self.rules, next.rules)
},
}
}
Expand Down
29 changes: 1 addition & 28 deletions crates/jp_config/src/delta_law_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,31 +79,6 @@ fn assert_law(before: &[&str], after: &[&str]) {
);
}

/// Fields whose clear is known not to survive a fold, and why.
///
/// `conversation.compaction.rules` keeps its rules in a bare `MergeableVec`, so
/// an empty one cannot say whether the user asked for no rules or said nothing
/// about them.
/// A delta that replaced on any difference would record zero rules from any
/// sparse partial that reached it, which is how it was found: routing it
/// through [`delta_mergeable_vec`] wrote `replace` with an empty list into 37
/// snapshots and appended an event that should not exist.
/// Reaching it needs the partial to be an `Option<MergeableVec<_>>`, as every
/// converted list field has.
///
/// `model.parameters.other` is the catch-all arm of its own key-value dispatch,
/// so `parameters.other` names a key *inside* the map rather than the map
/// itself, and clearing removes an entry that was never there.
/// Reaching the whole field needs a path vocabulary that can say "this map"
/// where the map is also the fallback.
const CLEAR_NOT_RECORDED: &[&str] = &[
"conversation.compaction.rules",
"assistant.model.parameters.other",
"style.reasoning.summary_model.parameters.other",
"conversation.inquiry.assistant.model.parameters.other",
"conversation.title.generate.model.parameters.other",
];

/// Set `path` to whichever of a few generic values it accepts.
///
/// A field has to hold something before clearing it proves anything, and there
Expand Down Expand Up @@ -229,9 +204,7 @@ fn clearing_any_field_survives_a_fold() {
}
folded.merge(&(), delta).expect("folding cannot fail");

if crate::util::build(folded).ok().as_ref() != Some(&expected)
&& !CLEAR_NOT_RECORDED.contains(&path.as_str())
{
if crate::util::build(folded).ok().as_ref() != Some(&expected) {
lost.push(path);
}
}
Expand Down
37 changes: 37 additions & 0 deletions crates/jp_config/src/lib_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,43 @@ fn a_server_added_to_the_workspace_reaches_an_existing_conversation() {
);
}

/// A label rule only the workspace config declares reaches an existing
/// conversation, the same way a server or a tool does.
#[test]
fn a_label_added_to_the_workspace_reaches_an_existing_conversation() {
use schematic::PartialConfig as _;

use crate::conversation::label::LabelConfig;

// The conversation was created knowing only `topic`.
let mut conversation = AppConfig::new_test();
conversation
.conversation
.labels
.insert("topic".to_owned(), LabelConfig::Static("rust".to_owned()));

// The workspace config has since gained `area`.
let mut files = PartialAppConfig::new_test();
files.conversation.labels.insert(
"area".to_owned(),
LabelConfig::Static("config".to_owned()).to_partial(),
);

files
.merge(&(), conversation.to_partial())
.expect("merging cannot fail");
let resolved = crate::util::build(files).expect("valid config");

assert!(
resolved.conversation.labels.contains_key("area"),
"a label only the files declare survives the conversation layer"
);
assert!(
resolved.conversation.labels.contains_key("topic"),
"the conversation's own label survives too"
);
}

/// A union that names an expanded form contributes both the shorthand path and
/// the expanded keys; a union of distinct values contributes only its path.
///
Expand Down
18 changes: 0 additions & 18 deletions crates/jp_config/src/types/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,24 +164,6 @@ impl<T> FillDefaults for MergeableMap<T> {
}
}

/// Convert a resolved map to a `MergeableMap<T::Partial>` with replace
/// strategy.
///
/// Used by `ToPartial` impls for fields whose finalized type is a plain map but
/// whose partial is a [`MergeableMap`].
pub fn map_to_mergeable_partial<'a, T: ToPartial + 'a>(
entries: impl IntoIterator<Item = (&'a String, &'a T)>,
) -> MergeableMap<T::Partial> {
MergeableMap::Merged(MergedMap {
value: entries
.into_iter()
.map(|(k, v)| (k.clone(), v.to_partial()))
.collect(),
strategy: Some(MergedMapStrategy::Replace),
discard_when_merged: false,
})
}

/// Convert a resolved map to a `MergeableMap<T::Partial>` that merges per key.
///
/// Used by `ToPartial` impls for a map that should still take an entry a later
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_chat_completion_stream__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions ...m/tests/fixtures/anthropic/test_fable_5_forced_tool_soft_forces__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_image_attachment__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions ...es/jp_llm/tests/fixtures/anthropic/test_multi_turn_conversation__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions ...jp_llm/tests/fixtures/anthropic/test_opus_4_6_adaptive_thinking__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_max_effort__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_redacted_thinking__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_request_chaining__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_structured_output__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_tool_call_auto__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_tool_call_function__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_tool_call_reasoning__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions ...m/tests/fixtures/anthropic/test_tool_call_required_no_reasoning__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions ..._llm/tests/fixtures/anthropic/test_tool_call_required_reasoning__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/anthropic/test_tool_call_stream__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/cerebras/test_chat_completion_stream__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/cerebras/test_multi_turn_conversation__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/cerebras/test_structured_output__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
5 changes: 0 additions & 5 deletions crates/jp_llm/tests/fixtures/cerebras/test_tool_call_auto__conversation_stream.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ expression: v
"strategy": "replace",
"discard_when_merged": false
},
"labels": {
"value": {},
"strategy": "replace",
"discard_when_merged": false
},
"start_local": false
},
"style": {
Expand Down
Loading
Loading