diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index bb80d071..ee812635 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -61,6 +61,7 @@ use std::{ }; use camino::{Utf8Path, Utf8PathBuf}; +use chrono::{DateTime, Utc}; use clap::{ArgAction, builder::TypedValueParser as _}; use indexmap::IndexMap; use jp_attachment::Attachment; @@ -85,6 +86,7 @@ use jp_config::{ use jp_conversation::{ Conversation, ConversationEvent, ConversationId, ConversationStream, event::{ChatRequest, ChatResponse}, + stream::{ApplyDelta, ResetDelta}, thread::{Thread, ThreadBuilder}, }; use jp_inquire::prompt::TerminalPromptBackend; @@ -123,6 +125,7 @@ use crate::{ conversation::fork, lock::{LockRequest, acquire_lock}, }, + config_pipeline::{ConfigReset, ConfigResetEvents}, ctx::IntoPartialAppConfig, editor, error::{Error, Result}, @@ -361,7 +364,7 @@ impl Query { // 2. picker "start new": `start_new` is set, create a fresh conversation. // 3. --fork/--id/session: resolve an existing conversation, lock it. // 4. Lock contention: user picks "new" or "fork" from the prompt. - let lock = self.acquire_lock(ctx, handle, start_new).await?; + let (lock, fresh) = self.acquire_lock(ctx, handle, start_new).await?; // Create symlinks and seed approvals for any `--mount` flags before the // turn runs, so tools can reach the mounted paths. @@ -381,7 +384,29 @@ impl Query { warn!(%error, "Failed to record activation."); } - if let Some(delta) = get_config_delta_from_cli(&cfg, &lock)? { + // Persist config state changes into the conversation stream. + // + // A fresh conversation needs neither branch: its base config was + // written from this invocation's resolved config at creation time + // (that also absorbs any `--cfg` reset keyword, per [RFD 038]). + // + // A conversation carrying earlier config state — continuing or forked + // — records a `--cfg` reset keyword as its stream events, appended + // directly: between the `Reset` and whichever `Apply` restores the + // required fields the stream does not resolve to a valid config, so + // the empty-diff suppression path in `add_config_delta` cannot run. + // + // Without a reset keyword, any divergence between the stream's config + // and this invocation's resolved config is appended as a single + // suppression-checked `Apply` diff, as before. + // + // [RFD 038]: https://jp.computer/rfd/038 + if let Some(reset_events) = ctx.config_reset.take() { + if !fresh { + lock.as_mut() + .update_events(|events| persist_config_reset(events, reset_events, now)); + } + } else if let Some(delta) = get_config_delta_from_cli(&cfg, &lock)? { lock.as_mut() .update_events(|events| events.add_config_delta(delta)); } @@ -964,15 +989,22 @@ impl Query { Ok(()) } + /// Resolve the target conversation and return its exclusive lock. + /// + /// The second element is `true` when the conversation was freshly created + /// by this call: its base config is this invocation's resolved config, so + /// no config state predates it. + /// Forks return `false` — a fork copies the source's base config and + /// events, and therefore carries config state from before this invocation. async fn acquire_lock( &self, ctx: &mut Ctx, handle: Option, start_new: bool, - ) -> Result { + ) -> Result<(ConversationLock, bool)> { // Handle --new: create a fresh conversation. if self.is_new() { - return self.create_new_conversation(ctx); + return Ok((self.create_new_conversation(ctx)?, true)); } // Handle the picker's "start a new conversation" choice. It carries no @@ -982,14 +1014,14 @@ impl Query { if !self.allows_new_from_picker() { return Err(Error::NewConflictsWithTarget); } - return self.create_new_conversation(ctx); + return Ok((self.create_new_conversation(ctx)?, true)); } let handle = handle.ok_or(Error::NoConversationTarget)?; // Handle --fork: fork the conversation before locking. if let Some(fork_turns) = &self.fork { - return fork_conversation(ctx, &handle, *fork_turns); + return Ok((fork_conversation(ctx, &handle, *fork_turns)?, false)); } let req = LockRequest::from_ctx(handle, ctx) @@ -997,9 +1029,11 @@ impl Query { .allow_fork(true); match acquire_lock(req).await? { - LockOutcome::Acquired(lock) => Ok(lock), - LockOutcome::NewConversation => self.create_new_conversation(ctx), - LockOutcome::ForkConversation(handle) => fork_conversation(ctx, &handle, None), + LockOutcome::Acquired(lock) => Ok((lock, false)), + LockOutcome::NewConversation => Ok((self.create_new_conversation(ctx)?, true)), + LockOutcome::ForkConversation(handle) => { + Ok((fork_conversation(ctx, &handle, None)?, false)) + } } } } @@ -1236,6 +1270,34 @@ fn apply_title_override(lock: &ConversationLock, title: Option<&str>, no_title: } } +/// Append a `--cfg` reset keyword's events to a conversation stream. +/// +/// Persists the reset-then-layer sequence from [RFD 038]: a [`ResetDelta`] +/// marking the reset point, then the workspace partial for `WORKSPACE` resets, +/// then whatever state this invocation layered on top of the reset point. +/// Empty layers are skipped by [`ConversationStream::add_config_reset`], which +/// also documents why the sequence bypasses diff-suppression. +/// +/// [RFD 038]: https://jp.computer/rfd/038 +fn persist_config_reset( + events: &mut ConversationStream, + reset: ConfigResetEvents, + timestamp: DateTime, +) { + let mut layers = Vec::with_capacity(2); + + if let ConfigReset::Workspace(delta) = reset.reset { + layers.push(ApplyDelta { timestamp, delta }); + } + + layers.push(ApplyDelta { + timestamp, + delta: reset.post, + }); + + events.add_config_reset(ResetDelta { timestamp }, layers); +} + fn get_config_delta_from_cli( cfg: &AppConfig, lock: &ConversationLock, diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index 5287d65e..dff54ea9 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -111,7 +111,7 @@ fn build_query_config( query: &Query, handle: Option<&ConversationHandle>, ) -> AppConfig { - let pipeline = ConfigPipeline::new(base, cfg_args, Some(workspace), None).unwrap(); + let pipeline = ConfigPipeline::new(cfg_args, Some(workspace), None, || Ok(base)).unwrap(); let conversation_partial = handle.map(|handle| { query @@ -754,6 +754,141 @@ fn query_cfg_sourced_compaction_persists_as_config_delta() { ); } +/// The `config_delta` events of a stream's serialized `events.json`. +fn serialized_config_deltas(events: &ConversationStream) -> Vec { + let (_base, serialized) = events.clone().to_parts().unwrap(); + serialized + .into_iter() + .filter(|event| event.get("type").and_then(Value::as_str) == Some("config_delta")) + .collect() +} + +#[test] +fn cfg_reset_none_appends_reset_then_post_apply() { + // `jp q --cfg=NONE --cfg=` on a continuing conversation ([RFD 038]): + // the stream records `[Reset, Apply(post)]`, where `post` restores the + // required fields on top of program defaults. + let base_config = Arc::new(config_with_model(ProviderId::Anthropic, "base-model")); + let conversation_id = make_id(3000); + + let mut workspace = Workspace::new("/tmp/test"); + workspace.create_conversation_with_id( + conversation_id, + Conversation::default(), + Arc::clone(&base_config), + ); + let handle = workspace.acquire_conversation(&conversation_id).unwrap(); + let lock = workspace.test_lock(handle); + + let post = config_with_model(ProviderId::Openai, "fresh-model").to_partial(); + let reset = ConfigResetEvents { + reset: ConfigReset::Defaults, + post: Box::new(post), + }; + + lock.as_mut() + .update_events(|events| persist_config_reset(events, reset, DateTime::::UNIX_EPOCH)); + + let events = lock.events().clone(); + + // The stream resolves to the post-reset state, not the pre-reset base. + let merged = events.config().unwrap(); + let model_id = merged.assistant.model.id.resolved(); + assert_eq!(model_id.provider, ProviderId::Openai); + assert_eq!(model_id.name.as_ref(), "fresh-model"); + + // Wire shape: a `Reset` marker followed by a plain `Apply` (no `op`). + let deltas = serialized_config_deltas(&events); + assert_eq!(deltas.len(), 2, "expected [Reset, Apply], got {deltas:?}"); + assert_eq!(deltas[0].get("op").and_then(Value::as_str), Some("reset")); + assert!( + deltas[1].get("op").is_none(), + "`Apply` writes no `op` field" + ); +} + +#[test] +fn cfg_reset_workspace_appends_reset_then_workspace_apply() { + // `jp q --cfg=WORKSPACE` on a continuing conversation ([RFD 038]): the + // stream records `[Reset, Apply(workspace)]`, re-adopting the workspace's + // resolved config as of this invocation. No further `Apply` is written + // when nothing is layered on top. + let base_config = Arc::new(config_with_model(ProviderId::Anthropic, "base-model")); + let conversation_id = make_id(3001); + + let mut workspace = Workspace::new("/tmp/test"); + workspace.create_conversation_with_id( + conversation_id, + Conversation::default(), + Arc::clone(&base_config), + ); + let handle = workspace.acquire_conversation(&conversation_id).unwrap(); + let lock = workspace.test_lock(handle); + + let workspace_partial = config_with_model(ProviderId::Openai, "ws-model").to_partial(); + let reset = ConfigResetEvents { + reset: ConfigReset::Workspace(Box::new(workspace_partial)), + post: Box::new(PartialAppConfig::default()), + }; + + lock.as_mut() + .update_events(|events| persist_config_reset(events, reset, DateTime::::UNIX_EPOCH)); + + let events = lock.events().clone(); + + let merged = events.config().unwrap(); + let model_id = merged.assistant.model.id.resolved(); + assert_eq!(model_id.provider, ProviderId::Openai); + assert_eq!(model_id.name.as_ref(), "ws-model"); + + let deltas = serialized_config_deltas(&events); + assert_eq!( + deltas.len(), + 2, + "empty post-reset state must not write a third event: {deltas:?}" + ); + assert_eq!(deltas[0].get("op").and_then(Value::as_str), Some("reset")); + assert!(deltas[1].get("op").is_none()); +} + +#[test] +fn cfg_reset_none_without_post_leaves_unresolvable_config() { + // A bare `jp q --cfg=NONE` on a continuing conversation records only the + // `Reset`. Program defaults lack required fields (`assistant.model.id`), + // so resolving the stream's config fails until a later `Apply` restores + // them — the intended escape-hatch semantics from [RFD 038]. + let base_config = Arc::new(config_with_model(ProviderId::Anthropic, "base-model")); + let conversation_id = make_id(3002); + + let mut workspace = Workspace::new("/tmp/test"); + workspace.create_conversation_with_id( + conversation_id, + Conversation::default(), + Arc::clone(&base_config), + ); + let handle = workspace.acquire_conversation(&conversation_id).unwrap(); + let lock = workspace.test_lock(handle); + + let reset = ConfigResetEvents { + reset: ConfigReset::Defaults, + post: Box::new(PartialAppConfig::default()), + }; + + lock.as_mut() + .update_events(|events| persist_config_reset(events, reset, DateTime::::UNIX_EPOCH)); + + let events = lock.events().clone(); + + let deltas = serialized_config_deltas(&events); + assert_eq!(deltas.len(), 1, "expected only the Reset: {deltas:?}"); + assert_eq!(deltas[0].get("op").and_then(Value::as_str), Some("reset")); + + assert!( + events.config().is_err(), + "program defaults alone must fail validation" + ); +} + #[test] fn inline_compact_dsl_is_not_written_into_query_config() { // The inline `-k SPEC` plan is applied as overlay events at query time, not diff --git a/crates/jp_cli/src/config_pipeline.rs b/crates/jp_cli/src/config_pipeline.rs index 2dcbf421..7afbb316 100644 --- a/crates/jp_cli/src/config_pipeline.rs +++ b/crates/jp_cli/src/config_pipeline.rs @@ -11,21 +11,121 @@ //! caller after each build — they're not part of the pipeline because they //! depend on the specific command struct. +use std::path::Path; + use camino::Utf8PathBuf; use jp_config::{ PartialAppConfig, assignment::{AssignKeyValue as _, KvAssignment}, fs::{load_partial, user_global_config_dir}, - util::{find_file_in_load_path, load_partial_at_path}, + loader::PartialLoaderConfig, + util::{find_file_in_load_path, load_loader_directives, load_partial_at_path}, }; use jp_storage::backend::FsStorageBackend; use jp_workspace::Workspace; use relative_path::RelativePath; use tracing::{debug, error}; -use super::KeyValueOrPath; +use super::{CfgKeyword, KeyValueOrPath}; use crate::error::{Error, Result}; +/// A config reset point encountered in the `--cfg` directive stream. +/// +/// Reset points share one mechanism: discard the accumulated config state, then +/// layer a known state on top ([RFD 038]). +/// +/// [RFD 038]: https://jp.computer/rfd/038 +#[derive(Debug, Clone)] +pub(crate) enum ConfigReset { + /// `--cfg=NONE`: reset to program defaults. + Defaults, + + /// `--cfg=WORKSPACE`: reset to the workspace's fully-resolved config. + /// + /// Carries the workspace partial as it resolved at invocation time, so the + /// reset is value-stable once persisted. + Workspace(Box), +} + +impl ConfigReset { + /// The state this reset point returns the accumulated config to. + /// + /// Program defaults live in the empty partial (they are injected when a + /// partial is finalized into an `AppConfig`), so `NONE` resets to the empty + /// partial and `WORKSPACE` to the workspace partial. + pub fn state(&self) -> PartialAppConfig { + match self { + Self::Defaults => PartialAppConfig::default(), + Self::Workspace(workspace) => (**workspace).clone(), + } + } +} + +/// The reset a continuing conversation must persist into its event stream. +/// +/// Assembled by `resolve_config` when the `--cfg` list contains a reset +/// keyword; consumed by the query command, which appends the corresponding +/// `ConfigDelta` events ([RFD 038]): +/// +/// 1. `Reset` (both keywords), +/// 2. `Apply(workspace partial)` (`WORKSPACE` only), +/// 3. `Apply(post)` (whatever the invocation layered on top of the reset point, +/// if anything). +/// +/// The `post` partial is a partial-level diff from the reset point's state to +/// the invocation's final partial, so it captures post-keyword `--cfg` +/// directives and command CLI overrides without pinning program defaults. +/// It is computed directly instead of routing through the empty-diff +/// suppression path, because that path resolves the stream's current config — +/// which is not a valid configuration between a `Reset` and whichever `Apply` +/// restores the required fields. +/// +/// [RFD 038]: https://jp.computer/rfd/038 +#[derive(Debug, Clone)] +pub(crate) struct ConfigResetEvents { + /// The reset point itself. + pub reset: ConfigReset, + + /// State layered on top of the reset point by this invocation. + pub post: Box, +} + +/// Presence of reserved `--cfg` keywords, detected by [`scan_cfg_keywords`]. +#[derive(Debug, Clone, Copy, Default)] +struct CfgKeywords { + /// An exact `NONE` value appears in the `--cfg` list. + pub none: bool, + + /// An exact `WORKSPACE` value appears in the `--cfg` list. + pub workspace: bool, +} + +/// Pre-scan the `--cfg` list for reset keywords. +/// +/// This runs in [`ConfigPipeline::new`] before any directive is processed: +/// `NONE` gates implicit config loading, and the `NONE`/`WORKSPACE` combination +/// is rejected independent of position — `NONE` skips the implicit-loading +/// step that `WORKSPACE` expands to, so combining them is internally +/// inconsistent. +fn scan_cfg_keywords(overrides: &[KeyValueOrPath]) -> Result { + let mut keywords = CfgKeywords::default(); + for field in overrides { + match field { + KeyValueOrPath::Keyword(CfgKeyword::None) => keywords.none = true, + KeyValueOrPath::Keyword(CfgKeyword::Workspace) => keywords.workspace = true, + _ => {} + } + } + + if keywords.none && keywords.workspace { + return Err(Error::CliConfig( + "--cfg=NONE and --cfg=WORKSPACE are mutually exclusive.".into(), + )); + } + + Ok(keywords) +} + /// A `--cfg` argument with file contents already resolved from disk. /// /// File I/O happens once during [`resolve_cfg_args`], and the results are @@ -35,8 +135,52 @@ enum ResolvedCfgArg { /// A key=value assignment (e.g. `--cfg conversation.default_id=last`). KeyValue(KvAssignment), - /// One or more partials loaded from a config file path. - Partials(Vec), + /// One or more config entries loaded from a config file path. + /// + /// A single `--cfg` argument can resolve to multiple entries across search + /// roots; they apply in root precedence order. + Partials(Vec), + + /// A reset keyword (`NONE` or `WORKSPACE`). + Reset(ConfigReset), +} + +/// A config entry resolved from an explicit `--cfg` file argument. +#[derive(Debug, Clone)] +struct CfgEntry { + /// The entry declares `loader.reset = "none"` in its own `[loader]` + /// section. + /// + /// The declaration is read shallowly from the entry file itself — + /// `[loader]` in a file reached through `extends` is ignored ([RFD 038]). + /// + /// [RFD 038]: https://jp.computer/rfd/038 + reset: bool, + + /// The entry's partial, resolved including its `extends` tree. + partial: PartialAppConfig, +} + +/// Load a `--cfg` file entry from `path`. +/// +/// Combines the fully-resolved partial (including the `extends` tree) with the +/// loader directives read shallowly from the entry file's own `[loader]` +/// section. +fn load_cfg_entry>(path: P) -> Result> { + let path = path.as_ref(); + let Some(mut partial) = load_partial_at_path(path)? else { + return Ok(None); + }; + + let reset = load_loader_directives(path)?.reset.is_some(); + + // Loader metadata is load-time-only ([RFD 038]): its effect is captured + // in `reset`, and the section itself (which here may also carry values + // merged in from `extends`-reached files) must not travel past the + // pipeline into resolved or persisted state. + partial.loader = PartialLoaderConfig::default(); + + Ok(Some(CfgEntry { reset, partial })) } /// Config sources loaded once from disk, reusable for multiple builds. @@ -56,13 +200,38 @@ impl ConfigPipeline { /// Build a pipeline from the workspace and CLI `--cfg` overrides. /// /// This is the only place where config files and `--cfg` file args are read - /// from disk. + /// from disk, and it owns the implicit-loading decision ([RFD 038]): the + /// `--cfg` list is pre-scanned for reset keywords (rejecting the mutually + /// exclusive `NONE`/`WORKSPACE` combination), and `load_base` — providing + /// the files + inheritance + env layer — is invoked only when no `NONE` + /// keyword is present. + /// The gate is decided before any config file I/O, which is what keeps + /// `NONE` usable as an escape hatch when implicit config is broken. + /// + /// [RFD 038]: https://jp.computer/rfd/038 pub fn new( - base: PartialAppConfig, overrides: &[KeyValueOrPath], workspace: Option<&Workspace>, fs: Option<&FsStorageBackend>, + load_base: impl FnOnce() -> Result, ) -> Result { + let keywords = scan_cfg_keywords(overrides)?; + + // `NONE` gate: skip implicit loading entirely and start from program + // defaults (the empty partial; defaults are injected when the partial + // is finalized into an `AppConfig`). + let mut base = if keywords.none { + PartialAppConfig::default() + } else { + load_base()? + }; + + // `[loader]` steers how an explicit `--cfg` entry is loaded; in + // implicitly-loaded config it has no effect, and it must not linger + // in the base state that `WORKSPACE` resets capture and new + // conversations persist ([RFD 038]). + base.loader = PartialLoaderConfig::default(); + let cfg_args = resolve_cfg_args(overrides, &base, workspace, fs)?; Ok(Self { base, cfg_args }) } @@ -86,6 +255,25 @@ impl ConfigPipeline { partial = apply_cfg_args(partial, &self.cfg_args)?; Ok(partial) } + + /// The effective reset point of this invocation, if any. + /// + /// A reset discards everything accumulated before it, so only the last + /// reset point in the directive stream is effective. + /// + /// Reset points come from the `NONE`/`WORKSPACE` keywords and from file + /// entries declaring `loader.reset = "none"` — the latter is the + /// entry-local equivalent of `--cfg=NONE` immediately before the entry, so + /// it maps to a reset to program defaults ([RFD 038]). + pub fn config_reset(&self) -> Option { + self.cfg_args.iter().rev().find_map(|arg| match arg { + ResolvedCfgArg::Reset(reset) => Some(reset.clone()), + ResolvedCfgArg::Partials(entries) if entries.iter().any(|e| e.reset) => { + Some(ConfigReset::Defaults) + } + _ => None, + }) + } } /// Resolve `--cfg` arguments into their in-memory representations. @@ -101,15 +289,31 @@ fn resolve_cfg_args( let home = std::env::home_dir().and_then(|p| Utf8PathBuf::from_path_buf(p).ok()); let mut resolved = Vec::with_capacity(overrides.len()); - for field in overrides { + // A `NONE` keyword positionally discards everything before it, so the + // directive loop skips processing pre-`NONE` values entirely: their + // file-load and merge effects are not executed ([RFD 038]). + // This keeps `NONE` usable as an escape hatch even when an earlier `--cfg` + // value references a broken or missing file. + // + // `WORKSPACE` gets no such exemption: pre-`WORKSPACE` values are processed + // normally (a missing file still errors), and their contribution is + // discarded by the reset during the merge. + // + // [RFD 038]: https://jp.computer/rfd/038 + let skip_until = overrides + .iter() + .rposition(|f| matches!(f, KeyValueOrPath::Keyword(CfgKeyword::None))) + .unwrap_or_default(); + + for field in &overrides[skip_until..] { match field { KeyValueOrPath::Path(path) if path.exists() => { - let mut partials = Vec::new(); - if let Some(p) = load_partial_at_path(path)? { - partials.push(p); + let mut entries = Vec::new(); + if let Some(entry) = load_cfg_entry(path.as_std_path())? { + entries.push(entry); } - if !partials.is_empty() { - resolved.push(ResolvedCfgArg::Partials(partials)); + if !entries.is_empty() { + resolved.push(ResolvedCfgArg::Partials(entries)); } } KeyValueOrPath::Path(path) => { @@ -132,7 +336,7 @@ fn resolve_cfg_args( roots.push(path); } - let mut matches: Vec = Vec::new(); + let mut matches: Vec = Vec::new(); let mut searched: Vec = Vec::new(); // Search each root independently. Within a single root, the @@ -167,8 +371,8 @@ fn resolve_cfg_args( ); if let Some(file) = find_file_in_load_path(path, load_path) { - if let Some(p) = load_partial_at_path(file)? { - matches.push(p); + if let Some(entry) = load_cfg_entry(&file)? { + matches.push(entry); } break; // first match within this root @@ -188,6 +392,17 @@ fn resolve_cfg_args( KeyValueOrPath::KeyValue(kv) => { resolved.push(ResolvedCfgArg::KeyValue(kv.clone())); } + KeyValueOrPath::Keyword(CfgKeyword::None) => { + resolved.push(ResolvedCfgArg::Reset(ConfigReset::Defaults)); + } + KeyValueOrPath::Keyword(CfgKeyword::Workspace) => { + // `base` is the workspace's fully-resolved config: when + // `WORKSPACE` appears, the `NONE` gate is off (the keywords + // are mutually exclusive), so implicit loading ran. + resolved.push(ResolvedCfgArg::Reset(ConfigReset::Workspace(Box::new( + base.clone(), + )))); + } } } @@ -207,14 +422,35 @@ fn apply_cfg_args( .assign(kv.clone()) .map_err(|e| Error::CliConfig(e.to_string()))?; } - ResolvedCfgArg::Partials(partials) => { - for p in partials { - partial = load_partial(partial, p.clone())?; + ResolvedCfgArg::Partials(entries) => { + for entry in entries { + if entry.reset { + // Entry-local reset ([RFD 038]): equivalent to + // `--cfg=NONE` immediately before this entry. + // Discards the accumulated state — including earlier + // entries resolved from the same argument. + partial = PartialAppConfig::default(); + } + partial = load_partial(partial, entry.partial.clone())?; } } + ResolvedCfgArg::Reset(reset) => { + // Reset-then-layer: discard the accumulated state (including + // the base and per-conversation layers), restart from the + // reset point's state, and let subsequent args layer on top. + partial = reset.state(); + } } } + // Defensive cleanup: loader metadata must not leak into resolved or + // persisted state ([RFD 038]). No route writes it here today — + // assignments reject `loader.*` as an unknown key (see + // `loader_assignment_is_rejected`), and file entries strip their own + // `[loader]` section at load time in `load_cfg_entry` — so this strip + // only guards future routes that forget to. + partial.loader = PartialLoaderConfig::default(); + Ok(partial) } @@ -234,6 +470,14 @@ pub(crate) fn build_partial_from_cfg_args( )); } + // Reset keywords describe a state transition in a conversation's config + // stream; they have no meaning as a value to persist. + if args.iter().any(|a| matches!(a, KeyValueOrPath::Keyword(_))) { + return Err(Error::CliConfig( + "--cfg reset keywords (NONE, WORKSPACE) are not supported here.".into(), + )); + } + let resolved = resolve_cfg_args(args, base, workspace, fs)?; apply_cfg_args(PartialAppConfig::empty(), &resolved) } diff --git a/crates/jp_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index cd0dbf5d..bacc0c3d 100644 --- a/crates/jp_cli/src/config_pipeline_tests.rs +++ b/crates/jp_cli/src/config_pipeline_tests.rs @@ -1,6 +1,12 @@ -use jp_config::{PartialAppConfig, assignment::KvAssignment, conversation::DefaultConversationId}; +use assert_matches::assert_matches; +use camino::Utf8Path; +use jp_config::{ + PartialAppConfig, PartialConfig as _, assignment::KvAssignment, + conversation::DefaultConversationId, +}; use super::*; +use crate::CfgKeyword; fn empty_pipeline() -> ConfigPipeline { ConfigPipeline { @@ -53,3 +59,258 @@ fn conversation_layer_overrides_base() { let partial = pipeline.partial_with_conversation(conv).unwrap(); assert_eq!(partial.conversation.start_local, Some(true)); } + +/// Write `content` to `name` inside `root` and return the file's path. +fn write_config(root: &Utf8Path, name: &str, content: &str) -> KeyValueOrPath { + let path = root.join(name); + std::fs::write(&path, content).unwrap(); + KeyValueOrPath::Path(path) +} + +#[test] +fn entry_loader_reset_discards_accumulated_state() { + let tmp = camino_tempfile::tempdir().unwrap(); + let entry = write_config(tmp.path(), "committer.toml", indoc::indoc! {r#" + [loader] + reset = "none" + + [conversation] + start_local = true + "#}); + + let mut base = PartialAppConfig::empty(); + base.conversation.title.generate.auto = Some(false); + + let pipeline = ConfigPipeline::new(&[entry], None, None, || Ok(base)).unwrap(); + let partial = pipeline.partial_without_conversation().unwrap(); + + // The entry's own contribution survives the reset… + assert_eq!(partial.conversation.start_local, Some(true)); + // …while the accumulated base state is discarded. + assert_eq!(partial.conversation.title.generate.auto, None); + + // The reset is reported as a reset point to program defaults, so + // continuing conversations persist `[Reset, Apply(post)]` ([RFD 038]). + assert_matches!(pipeline.config_reset(), Some(ConfigReset::Defaults)); +} + +#[test] +fn entry_without_loader_reset_layers_on_top() { + let tmp = camino_tempfile::tempdir().unwrap(); + let entry = write_config(tmp.path(), "dev.toml", "conversation.start_local = true"); + + let mut base = PartialAppConfig::empty(); + base.conversation.title.generate.auto = Some(false); + + let pipeline = ConfigPipeline::new(&[entry], None, None, || Ok(base)).unwrap(); + let partial = pipeline.partial_without_conversation().unwrap(); + + assert_eq!(partial.conversation.start_local, Some(true)); + assert_eq!(partial.conversation.title.generate.auto, Some(false)); + assert_matches!(pipeline.config_reset(), None); +} + +#[test] +fn extends_reached_loader_reset_is_ignored() { + // `loader.reset` is honored only on the explicit entry itself: a + // transitive reset would let an included fragment discard its parent + // entry's accumulated config ([RFD 038]). + let tmp = camino_tempfile::tempdir().unwrap(); + write_config(tmp.path(), "fragment.toml", indoc::indoc! {r#" + [loader] + reset = "none" + + [conversation] + start_local = true + "#}); + let entry = write_config(tmp.path(), "entry.toml", "extends = [\"fragment.toml\"]"); + + let mut base = PartialAppConfig::empty(); + base.conversation.title.generate.auto = Some(false); + + let pipeline = ConfigPipeline::new(&[entry], None, None, || Ok(base)).unwrap(); + let partial = pipeline.partial_without_conversation().unwrap(); + + // The fragment's values apply, but its reset directive does not. + assert_eq!(partial.conversation.start_local, Some(true)); + assert_eq!(partial.conversation.title.generate.auto, Some(false)); + assert_matches!(pipeline.config_reset(), None); + + // The fragment's `[loader]` section does not leak into resolved state. + assert_eq!(partial.loader.reset, None); +} + +#[test] +fn last_reset_point_wins() { + let tmp = camino_tempfile::tempdir().unwrap(); + let entry = write_config(tmp.path(), "committer.toml", indoc::indoc! {r#" + [loader] + reset = "none" + "#}); + + // Entry-local reset followed by `WORKSPACE`: the keyword is effective. + let pipeline = ConfigPipeline::new( + &[ + entry.clone(), + KeyValueOrPath::Keyword(CfgKeyword::Workspace), + ], + None, + None, + || Ok(PartialAppConfig::empty()), + ) + .unwrap(); + assert_matches!(pipeline.config_reset(), Some(ConfigReset::Workspace(_))); + + // `WORKSPACE` followed by an entry-local reset: the entry is effective. + let pipeline = ConfigPipeline::new( + &[KeyValueOrPath::Keyword(CfgKeyword::Workspace), entry], + None, + None, + || Ok(PartialAppConfig::empty()), + ) + .unwrap(); + assert_matches!(pipeline.config_reset(), Some(ConfigReset::Defaults)); +} + +#[test] +fn later_root_reset_discards_earlier_entries_of_same_argument() { + // When one `--cfg` argument resolves to multiple entries across search + // roots, a `loader.reset = "none"` on a later entry resets state at that + // point, discarding earlier entries from the same argument ([RFD 038]). + let mut first = PartialAppConfig::empty(); + first.conversation.start_local = Some(true); + + let mut second = PartialAppConfig::empty(); + second.conversation.title.generate.auto = Some(false); + + let mut base = PartialAppConfig::empty(); + base.conversation.default_id = Some(DefaultConversationId::LastActivated); + + let pipeline = ConfigPipeline { + base, + cfg_args: vec![ResolvedCfgArg::Partials(vec![ + CfgEntry { + reset: false, + partial: first, + }, + CfgEntry { + reset: true, + partial: second, + }, + ])], + }; + + let partial = pipeline.partial_without_conversation().unwrap(); + + // The resetting entry's own contribution survives… + assert_eq!(partial.conversation.title.generate.auto, Some(false)); + // …while the earlier entry from the same argument and the base state are + // discarded. + assert_eq!(partial.conversation.start_local, None); + assert_eq!(partial.conversation.default_id, None); + + assert_matches!(pipeline.config_reset(), Some(ConfigReset::Defaults)); +} + +#[test] +fn scan_detects_keywords_and_rejects_the_combination() { + let keywords = scan_cfg_keywords(&[KeyValueOrPath::Keyword(CfgKeyword::None)]).unwrap(); + assert!(keywords.none); + assert!(!keywords.workspace); + + let err = scan_cfg_keywords(&[ + KeyValueOrPath::Keyword(CfgKeyword::None), + KeyValueOrPath::Keyword(CfgKeyword::Workspace), + ]) + .unwrap_err(); + assert!(err.to_string().contains("mutually exclusive"), "{err}"); +} + +#[test] +fn none_keyword_gates_base_loading_inside_the_pipeline() { + // The pipeline owns the implicit-loading decision ([RFD 038]): under + // `--cfg=NONE`, the base loader is never invoked, so broken implicit + // config cannot prevent the pipeline from being built. + let pipeline = ConfigPipeline::new( + &[KeyValueOrPath::Keyword(CfgKeyword::None)], + None, + None, + || panic!("implicit config loading must be skipped under --cfg=NONE"), + ) + .unwrap(); + + // The reset point is program defaults, and nothing leaks into the state. + assert_matches!(pipeline.config_reset(), Some(ConfigReset::Defaults)); + let partial = pipeline.partial_without_conversation().unwrap(); + assert!(partial.is_empty()); +} + +#[test] +fn keyword_mutual_exclusion_is_rejected_before_base_loading() { + // The `NONE`/`WORKSPACE` combination is rejected by the pre-scan, before + // the pipeline touches any config source. + let result = ConfigPipeline::new( + &[ + KeyValueOrPath::Keyword(CfgKeyword::None), + KeyValueOrPath::Keyword(CfgKeyword::Workspace), + ], + None, + None, + || panic!("base loading must not run when the keyword scan fails"), + ); + let err = result + .err() + .expect("the keyword combination must be rejected"); + assert!(err.to_string().contains("mutually exclusive"), "{err}"); +} + +#[test] +fn base_loader_runs_without_the_none_gate() { + // Without `NONE`, the pipeline invokes the loader and layers `--cfg` + // directives on top of its result. + let mut base = PartialAppConfig::empty(); + base.conversation.start_local = Some(true); + + let pipeline = ConfigPipeline::new(&[], None, None, || Ok(base)).unwrap(); + let partial = pipeline.partial_without_conversation().unwrap(); + assert_eq!(partial.conversation.start_local, Some(true)); +} + +#[test] +fn loader_reset_does_not_trigger_the_none_gate() { + // `loader.reset = "none"` is positional only: the pre-pipeline gate that + // skips implicit config loading responds to the `NONE` keyword alone, so + // broken implicit config still requires `NONE` / `--no-cfg` ([RFD 038]). + // The gate runs before any file is read, so a resetting entry cannot + // influence it. + let tmp = camino_tempfile::tempdir().unwrap(); + let entry = write_config(tmp.path(), "committer.toml", indoc::indoc! {r#" + [loader] + reset = "none" + "#}); + + let keywords = scan_cfg_keywords(&[entry]).unwrap(); + assert!(!keywords.none); + assert!(!keywords.workspace); +} + +#[test] +fn loader_assignment_is_rejected() { + // `loader` is load-time metadata, not application config: it is not an + // assignable key, so `--cfg loader.reset=none` fails instead of leaking + // loader state into the resolved partial ([RFD 038]). Only a file entry's + // own `[loader]` section is honored, at load time. + let pipeline = ConfigPipeline::new( + &[KeyValueOrPath::KeyValue( + "loader.reset=none".parse::().unwrap(), + )], + None, + None, + || Ok(PartialAppConfig::empty()), + ) + .unwrap(); + + let err = pipeline.partial_without_conversation().unwrap_err(); + assert!(err.to_string().contains("unknown key"), "{err}"); + assert_matches!(pipeline.config_reset(), None); +} diff --git a/crates/jp_cli/src/ctx.rs b/crates/jp_cli/src/ctx.rs index c197a1fe..f66749a4 100644 --- a/crates/jp_cli/src/ctx.rs +++ b/crates/jp_cli/src/ctx.rs @@ -19,7 +19,7 @@ use tokio::{ task::JoinSet, }; -use crate::{Globals, Result, signals::SignalRouter}; +use crate::{Globals, Result, config_pipeline::ConfigResetEvents, signals::SignalRouter}; /// Context for the CLI application pub(crate) struct Ctx { @@ -55,6 +55,15 @@ pub(crate) struct Ctx { /// root shutdown token. pub(crate) signals: SignalRouter, + /// A `--cfg` reset keyword's persistence payload ([RFD 038]). + /// + /// `Some` when the invocation's `--cfg` list contained `NONE` or + /// `WORKSPACE`; the query command appends the corresponding events to a + /// continuing conversation's stream. + /// + /// [RFD 038]: https://jp.computer/rfd/038 + pub(crate) config_reset: Option, + runtime: Runtime, #[cfg(test)] @@ -116,6 +125,7 @@ impl Ctx { mcp_client, task_handler: TaskHandler::default(), signals: SignalRouter::new(&runtime, escalation_cooldown), + config_reset: None, runtime, #[cfg(test)] diff --git a/crates/jp_cli/src/lib.rs b/crates/jp_cli/src/lib.rs index 2b7521da..0344eaed 100644 --- a/crates/jp_cli/src/lib.rs +++ b/crates/jp_cli/src/lib.rs @@ -61,7 +61,7 @@ use crate::{ plugin::dispatch::{describe_plugin, discover_plugins}, target::resolve_request, }, - config_pipeline::ConfigPipeline, + config_pipeline::{ConfigPipeline, ConfigReset, ConfigResetEvents}, timer::{LineTimer, spawn_line_timer}, }; @@ -114,6 +114,13 @@ struct Globals { )] config: Vec, + /// Shorthand for `--cfg=NONE`: skip implicit config loading and start from + /// program defaults. + /// + /// Subsequent `--cfg` values layer on top of the defaults. + #[arg(long = "no-cfg", global = true, default_value_t = false)] + no_config: bool, + /// Increase verbosity of logging. /// /// Can be specified multiple times to increase verbosity. @@ -210,10 +217,21 @@ pub(crate) enum LogFormat { Json, } +/// A reserved UPPERCASE `--cfg` keyword naming a config reset point. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum CfgKeyword { + /// `NONE`: reset to program defaults, and skip implicit config loading for + /// the whole invocation. + None, + /// `WORKSPACE`: reset to the workspace's resolved config. + Workspace, +} + #[derive(Debug, Clone)] pub(crate) enum KeyValueOrPath { KeyValue(KvAssignment), Path(Utf8PathBuf), + Keyword(CfgKeyword), } impl FromStr for KeyValueOrPath { @@ -225,6 +243,17 @@ impl FromStr for KeyValueOrPath { return Ok(Self::Path(Utf8PathBuf::from(s.trim()))); } + // Reserved UPPERCASE keywords are matched exactly, before any other + // resolution. + // A file literally named `NONE` or `WORKSPACE` is reachable through + // the `@` prefix above or a path-style prefix such as `./NONE`. + if s == "NONE" { + return Ok(Self::Keyword(CfgKeyword::None)); + } + if s == "WORKSPACE" { + return Ok(Self::Keyword(CfgKeyword::Workspace)); + } + // A JSON object is treated as a root-level config assignment that // merges each top-level key individually. if s.starts_with('{') { @@ -412,11 +441,19 @@ fn run_inner(cli: Cli, format: OutputFormat) -> Result<()> { // individual conversations, this is done lazily as needed. workspace.load_conversation_index(); - let base = load_base_partial(fs_backend.as_deref())?; - let (config, handles, start_new) = resolve_config( + // `--no-cfg` is shorthand for a leading `--cfg=NONE`, applied to config + // resolution only. `Globals.config` stays as the user typed it: commands + // re-consume the raw `--cfg` args (e.g. `config set` persists them), and + // must not see a synthetic reset keyword they'd have to reject + // ([RFD 038]). + // + // [RFD 038]: https://jp.computer/rfd/038 + let cfg_overrides = effective_cfg_overrides(&cli.globals); + + let (config, handles, start_new, config_reset) = resolve_config( &cli.command, - base, - &cli.globals.config, + || load_base_partial(fs_backend.as_deref()), + &cfg_overrides, &mut workspace, session.as_ref(), fs_backend.as_deref(), @@ -432,6 +469,7 @@ fn run_inner(cli: Cli, format: OutputFormat) -> Result<()> { session, printer, ); + ctx.config_reset = config_reset; let rt = ctx.handle().clone(); // Run the requested command, racing it against the shutdown token. @@ -665,24 +703,36 @@ fn parse_error(error: cmd::Error, format: OutputFormat) -> (u8, String) { /// Resolve the final [`AppConfig`] and conversation handles. /// -/// Takes a pre-loaded base partial (from config files + env) and runs the full -/// config pipeline: +/// Takes a loader for the base partial (the `files + env` layer) and runs the +/// full config pipeline: /// -/// 1. Extract `default_id` for conversation resolution (loading-time only). -/// 2. Resolve conversation handles from the command's load request. -/// 3. Merge per-conversation config layer. -/// 4. Apply CLI flag overrides via [`IntoPartialAppConfig`]. -/// 5. Consume `default_id` so it doesn't leak into the runtime config. -/// 6. Build the final [`AppConfig`]. +/// 1. Build the [`ConfigPipeline`], which invokes `load_base` unless a +/// `--cfg=NONE` keyword skips implicit loading ([RFD 038]). +/// 2. Extract `default_id` for conversation resolution (loading-time only). +/// 3. Resolve conversation handles from the command's load request. +/// 4. Merge per-conversation config layer. +/// 5. Apply CLI flag overrides via [`IntoPartialAppConfig`]. +/// 6. Consume `default_id` so it doesn't leak into the runtime config. +/// 7. Build the final [`AppConfig`]. +/// +/// [RFD 038]: https://jp.computer/rfd/038 pub(crate) fn resolve_config( command: &Commands, - base: PartialAppConfig, + load_base: impl FnOnce() -> Result, cfg_overrides: &[KeyValueOrPath], workspace: &mut Workspace, session: Option<&jp_workspace::session::Session>, fs: Option<&FsStorageBackend>, -) -> Result<(AppConfig, Vec, bool)> { - let pipeline = ConfigPipeline::new(base, cfg_overrides, Some(workspace), fs)?; +) -> Result<( + AppConfig, + Vec, + bool, + Option, +)> { + let pipeline = ConfigPipeline::new(cfg_overrides, Some(workspace), fs, load_base)?; + + // The effective reset point of this invocation, if any ([RFD 038]). + let config_reset = pipeline.config_reset(); // Extract default_id — a loading-time concern consumed here, not // propagated to the runtime config. @@ -703,20 +753,27 @@ pub(crate) fn resolve_config( let handles = outcome.handles; // Phase 2: per-conversation layer. + // + // Skipped when this invocation contains a reset point: the reset discards + // everything accumulated before it — including this layer — and resolving + // the stream's current config can itself fail, which must not block the + // reset (recovering from broken conversation config is a reset use case, + // [RFD 038]). let config_handle = request.config_conversation.and_then(|idx| handles.get(idx)); - if let Some(handle) = config_handle - && let Err(error) = workspace.eager_load_conversation(handle) - { - tracing::warn!(error = ?error, "Failed to eager-load conversation."); - } + let conversation_partial = match config_handle { + Some(handle) if config_reset.is_none() => { + if let Err(error) = workspace.eager_load_conversation(handle) { + tracing::warn!(error = ?error, "Failed to eager-load conversation."); + } - let conversation_partial = config_handle - .map(|handle| { - command - .apply_conversation_config(workspace, PartialAppConfig::default(), None, handle) - .map_err(|error| Error::CliConfig(error.to_string())) - }) - .transpose()?; + Some( + command + .apply_conversation_config(workspace, PartialAppConfig::default(), None, handle) + .map_err(|error| Error::CliConfig(error.to_string()))?, + ) + } + _ => None, + }; let mut partial = match conversation_partial { Some(conversation_config) => pipeline.partial_with_conversation(conversation_config)?, @@ -731,8 +788,53 @@ pub(crate) fn resolve_config( // Consume default_id so it doesn't appear in the runtime config. partial.conversation.default_id.take(); + // Capture this invocation's final partial for the reset persistence + // payload, before `build` consumes it. + let post_partial = config_reset.as_ref().map(|_| partial.clone()); + let config = build(partial)?; - Ok((config, handles, outcome.start_new)) + + // Assemble the reset point for conversation persistence ([RFD 038]): a + // continuing conversation records the reset, and whatever this invocation + // layered on top of it, into its event stream. + // + // Both layers resolve model aliases against the final config's flattened + // alias map (built by `build` above) before capture: partials stored as + // conversation config deltas must contain resolved model IDs (see + // [`PartialAppConfig::resolve_model_aliases`]), because the stream's own + // config resolution never resolves aliases. + let config_reset = config_reset.map(|mut reset| { + let aliases = &config.providers.llm.aliases; + if let ConfigReset::Workspace(workspace) = &mut reset { + workspace.resolve_model_aliases(aliases); + } + + let mut post = post_partial.expect("captured when a reset point is present"); + post.resolve_model_aliases(aliases); + + ConfigResetEvents { + post: Box::new(reset.state().delta(post)), + reset, + } + }); + + Ok((config, handles, outcome.start_new, config_reset)) +} + +/// The `--cfg` directive list used for config resolution. +/// +/// Prepends the `NONE` keyword when `--no-cfg` is set, without mutating +/// [`Globals::config`]: the raw `--cfg` args are re-consumed by commands (e.g. +/// `config set` persisting them), which reject reset keywords ([RFD 038]). +/// +/// [RFD 038]: https://jp.computer/rfd/038 +fn effective_cfg_overrides(globals: &Globals) -> Vec { + let mut overrides = Vec::with_capacity(globals.config.len() + 1); + if globals.no_config { + overrides.push(KeyValueOrPath::Keyword(CfgKeyword::None)); + } + overrides.extend(globals.config.iter().cloned()); + overrides } /// Load the base partial config from files and environment variables. diff --git a/crates/jp_cli/src/lib_tests.rs b/crates/jp_cli/src/lib_tests.rs index 7a5f9626..4030696c 100644 --- a/crates/jp_cli/src/lib_tests.rs +++ b/crates/jp_cli/src/lib_tests.rs @@ -60,7 +60,7 @@ fn build_cfg( overrides: &[KeyValueOrPath], workspace: Option<&Workspace>, ) -> Result { - let pipeline = config_pipeline::ConfigPipeline::new(base, overrides, workspace, None)?; + let pipeline = config_pipeline::ConfigPipeline::new(overrides, workspace, None, || Ok(base))?; pipeline.partial_without_conversation() } @@ -565,9 +565,9 @@ fn resolve_config_consumes_default_id() { base.conversation.default_id = Some(DefaultConversationId::LastActivated); let cli = Cli::try_parse_from(["jp", "conversation", "ls"]).unwrap(); - let (config, _handles, _start_new) = resolve_config( + let (config, _handles, _start_new, _config_reset) = resolve_config( &cli.command, - base, + || Ok(base), &cli.globals.config, &mut workspace, None, @@ -581,3 +581,200 @@ fn resolve_config_consumes_default_id() { config.conversation.default_id, ); } + +fn kv(s: &str) -> KeyValueOrPath { + KeyValueOrPath::KeyValue(s.parse().unwrap()) +} + +/// `--no-cfg` expands to a leading `NONE` keyword for config resolution only; +/// the raw `--cfg` args stay as typed, so commands that re-consume them (e.g. +/// `config set`, which rejects reset keywords) don't see a synthetic keyword +/// ([RFD 038]). +#[test] +fn no_cfg_shorthand_does_not_leak_into_raw_cfg_args() { + let cli = Cli::try_parse_from(["jp", "--no-cfg", "conversation", "ls"]).unwrap(); + + let overrides = effective_cfg_overrides(&cli.globals); + assert!( + matches!(overrides.as_slice(), [KeyValueOrPath::Keyword( + CfgKeyword::None + )]), + "expected a single synthetic NONE keyword, got: {overrides:?}", + ); + + // The raw args are untouched — `config set` and friends never see the + // synthetic keyword. + assert!(cli.globals.config.is_empty(), "{:?}", cli.globals.config); + + // Without `--no-cfg`, the list passes through unchanged. + let cli = Cli::try_parse_from(["jp", "--cfg", "user.name=x", "conversation", "ls"]).unwrap(); + let overrides = effective_cfg_overrides(&cli.globals); + assert_eq!(overrides.len(), 1); + assert!(matches!(&overrides[0], KeyValueOrPath::KeyValue(_))); +} + +/// A `--cfg` reset point must not resolve the targeted conversation's config: +/// the reset discards that layer, and resolving it can fail outright — +/// recovering a conversation with broken config is a reset use case ([RFD +/// 038]). +#[test] +fn resolve_config_reset_skips_broken_conversation_config() { + use jp_conversation::stream::ResetDelta; + + let tmp = tempdir().unwrap(); + let mut workspace = Workspace::new(tmp.path()); + workspace.load_conversation_index(); + + let base_config = Arc::new(config_with_model(ProviderId::Anthropic, "base-model")); + let conversation_id = make_id(4000); + workspace.create_conversation_with_id( + conversation_id, + Conversation::default(), + Arc::clone(&base_config), + ); + + // Break the conversation's config resolution: a bare `Reset` with no + // restoring `Apply` leaves the stream at program defaults, which lack + // required fields, so `events.config()` fails. + { + let handle = workspace.acquire_conversation(&conversation_id).unwrap(); + let lock = workspace.test_lock(handle); + lock.as_mut().update_events(|events| { + events.add_config_delta(ResetDelta { + timestamp: chrono::DateTime::::UNIX_EPOCH, + }); + }); + } + + let id = conversation_id.to_string(); + let cli = Cli::try_parse_from(["jp", "query", "--id", &id, "hello"]).unwrap(); + + // Without a reset point, the conversation layer is resolved and fails. + let result = resolve_config( + &cli.command, + || Ok(base_config.to_partial()), + &[], + &mut workspace, + None, + None, + ); + assert!(result.is_err(), "broken conversation config must propagate"); + + // With `--cfg=NONE` (+ the required fields), the conversation layer is + // skipped and resolution succeeds — the escape hatch works. + let (config, _handles, _start_new, config_reset) = resolve_config( + &cli.command, + || Ok(base_config.to_partial()), + &[ + KeyValueOrPath::Keyword(CfgKeyword::None), + kv("assistant.model.id=openai/fresh-model"), + kv("conversation.tools.*.run=ask"), + ], + &mut workspace, + None, + None, + ) + .expect("--cfg=NONE must recover a broken conversation config"); + + assert_eq!( + config.assistant.model.id.resolved().name.as_ref(), + "fresh-model" + ); + assert!(config_reset.is_some()); +} + +/// Reset layers are persisted into conversation streams, so they must contain +/// resolved model IDs ([`PartialAppConfig::resolve_model_aliases`]): the +/// stream's own config resolution never resolves aliases ([RFD 038]). +#[test] +fn resolve_config_reset_workspace_layer_contains_resolved_model_ids() { + use jp_config::model::id::PartialModelIdOrAliasConfig; + + let tmp = tempdir().unwrap(); + let mut workspace = Workspace::new(tmp.path()); + workspace.load_conversation_index(); + + // The workspace config defines an alias and references it. + let mut base = AppConfig::new_test().to_partial(); + base.providers.llm.aliases.insert( + "fast".to_owned(), + PartialModelIdOrAliasConfig::Id(PartialModelIdConfig { + provider: Some(ProviderId::Openai), + name: "gpt-4".parse().ok(), + }), + ); + base.assistant.model.id = PartialModelIdOrAliasConfig::Alias("fast".to_owned()); + + let cli = Cli::try_parse_from(["jp", "conversation", "ls"]).unwrap(); + let (_config, _handles, _start_new, config_reset) = resolve_config( + &cli.command, + || Ok(base), + &[KeyValueOrPath::Keyword(CfgKeyword::Workspace)], + &mut workspace, + None, + None, + ) + .unwrap(); + + let reset_events = config_reset.expect("WORKSPACE keyword produces a reset"); + let ConfigReset::Workspace(layer) = &reset_events.reset else { + panic!("expected a WORKSPACE reset, got: {:?}", reset_events.reset); + }; + + match &layer.assistant.model.id { + PartialModelIdOrAliasConfig::Id(id) => { + assert_eq!(id.provider, Some(ProviderId::Openai)); + } + PartialModelIdOrAliasConfig::Alias(alias) => { + panic!("alias `{alias}` persisted unresolved in the workspace layer"); + } + } + + // The post layer is the diff between two resolved states; it must not + // carry an alias either. + assert!( + !matches!( + reset_events.post.assistant.model.id, + PartialModelIdOrAliasConfig::Alias(_) + ), + "alias persisted unresolved in the post layer", + ); +} + +/// Post-reset `--cfg` directives referencing an alias must persist the resolved +/// model ID, not the alias ([RFD 038]). +#[test] +fn resolve_config_reset_post_layer_contains_resolved_model_ids() { + use jp_config::model::id::PartialModelIdOrAliasConfig; + + let tmp = tempdir().unwrap(); + let mut workspace = Workspace::new(tmp.path()); + workspace.load_conversation_index(); + + let cli = Cli::try_parse_from(["jp", "conversation", "ls"]).unwrap(); + let (_config, _handles, _start_new, config_reset) = resolve_config( + &cli.command, + || unreachable!("NONE skips implicit loading"), + &[ + KeyValueOrPath::Keyword(CfgKeyword::None), + kv("providers.llm.aliases.fast=openai/gpt-4"), + kv("assistant.model.id=fast"), + kv("conversation.tools.*.run=ask"), + ], + &mut workspace, + None, + None, + ) + .unwrap(); + + let reset_events = config_reset.expect("NONE keyword produces a reset"); + match &reset_events.post.assistant.model.id { + PartialModelIdOrAliasConfig::Id(id) => { + assert_eq!(id.provider, Some(ProviderId::Openai)); + assert_eq!(id.name.as_ref().unwrap().to_string(), "gpt-4"); + } + PartialModelIdOrAliasConfig::Alias(alias) => { + panic!("alias `{alias}` persisted unresolved in the post layer"); + } + } +} diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index cbb489e1..87600969 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -42,6 +42,7 @@ pub(crate) mod fill; pub mod fs; pub(crate) mod internal; pub mod interrupt; +pub mod loader; pub mod model; mod partial; pub mod plugins; @@ -74,6 +75,7 @@ use crate::{ delta::{PartialConfigDelta, delta_opt_vec}, editor::{EditorConfig, PartialEditorConfig}, interrupt::{InterruptConfig, PartialInterruptConfig}, + loader::{LoaderConfig, PartialLoaderConfig}, partial::partial_opt, plugins::{PartialPluginsConfig, PluginsConfig}, providers::{PartialProviderConfig, ProviderConfig}, @@ -124,6 +126,16 @@ pub struct AppConfig { #[setting(default = vec!["config.d/**/*".into()], merge = schematic::merge::preserve)] pub extends: Vec, + /// Loader directives for the config file declaring them. + /// + /// Interpreted while the declaring file is loaded, never part of the + /// resolved runtime configuration: the section is ignored when the file is + /// reached through `extends`, and is never persisted ([RFD 038]). + /// + /// [RFD 038]: https://jp.computer/rfd/038 + #[setting(nested)] + pub loader: LoaderConfig, + /// Assistant configuration. /// /// The assistant is the component that takes user input, and uses an LLM to @@ -238,6 +250,11 @@ impl PartialConfigDelta for PartialAppConfig { // `inherit` value of `true`. inherit: None, + // Loader metadata is interpreted while the declaring file is + // loaded ([RFD 038]): only its *effect* outlives loading, never + // the field itself. + loader: PartialLoaderConfig::default(), + config_load_paths: delta_opt_vec( self.config_load_paths.as_ref(), next.config_load_paths, @@ -262,6 +279,7 @@ impl FillDefaults for PartialAppConfig { inherit: self.inherit.or(defaults.inherit), config_load_paths: self.config_load_paths.or(defaults.config_load_paths), extends: self.extends.or(defaults.extends), + loader: self.loader.fill_from(defaults.loader), assistant: self.assistant.fill_from(defaults.assistant), conversation: self.conversation.fill_from(defaults.conversation), style: self.style.fill_from(defaults.style), @@ -283,6 +301,7 @@ impl ToPartial for AppConfig { inherit: partial_opt(&self.inherit, defaults.inherit), config_load_paths: partial_opt(&self.config_load_paths, defaults.config_load_paths), extends: partial_opt(&self.extends, defaults.extends), + loader: self.loader.to_partial(), assistant: self.assistant.to_partial(), conversation: self.conversation.to_partial(), style: self.style.to_partial(), diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 8a4abccb..3ccd3957 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -28,7 +28,11 @@ fn test_app_config_fields() { #[test] fn test_ensure_no_missing_assignments() { // Some fields cannot be assigned via CLI. - let skip_fields = ["extends"]; + // + // `loader.reset` is load-time metadata steering how the declaring file is + // loaded ([RFD 038]): it counts only in a file's own `[loader]` section + // and never becomes part of the resolved application config. + let skip_fields = ["extends", "loader.reset"]; for field in AppConfig::fields() { if skip_fields.contains(&field.as_str()) { diff --git a/crates/jp_config/src/loader.rs b/crates/jp_config/src/loader.rs new file mode 100644 index 00000000..513243b3 --- /dev/null +++ b/crates/jp_config/src/loader.rs @@ -0,0 +1,67 @@ +//! Loader directives for configuration entries. + +use schematic::{Config, ConfigEnum}; +use serde::{Deserialize, Serialize}; + +use crate::{ + fill::FillDefaults, + partial::{ToPartial, partial_opts}, +}; + +/// Loader directives for the config file declaring them. +/// +/// These settings steer *how* the declaring file is loaded instead of +/// contributing to the resolved configuration. +/// The section counts only when read from the declaring file itself, via +/// [`load_loader_directives`]: a file reached through another file's `extends` +/// has its `[loader]` section ignored, and the section never becomes part of +/// resolved or persisted configuration ([RFD 038]). +/// *When* an entry's directives are honored is the loading host's policy, not +/// defined here. +/// +/// [RFD 038]: https://jp.computer/rfd/038 +/// [`load_loader_directives`]: crate::util::load_loader_directives +#[derive(Debug, Clone, PartialEq, Config)] +#[config(rename_all = "snake_case")] +pub struct LoaderConfig { + /// Reset accumulated config state before applying the declaring entry. + /// + /// `reset = "none"` discards whatever configuration state accumulated + /// before this entry in the load order; the entry then applies on top of + /// program defaults. + /// + /// The reset is positional: it does not prevent earlier configuration from + /// being *loaded* — the declaring entry must itself be resolved through + /// the normal loading sequence — it only discards that configuration's + /// contribution to the accumulated state. + pub reset: Option, +} + +/// The state a `loader.reset` declaration resets accumulated config to. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize, ConfigEnum)] +#[serde(rename_all = "snake_case")] +pub enum LoaderReset { + /// Reset to program defaults, discarding all accumulated config state. + #[default] + None, +} + +impl FillDefaults for PartialLoaderConfig { + fn fill_from(self, defaults: Self) -> Self { + Self { + reset: self.reset.or(defaults.reset), + } + } +} + +impl ToPartial for LoaderConfig { + fn to_partial(&self) -> Self::Partial { + Self::Partial { + reset: partial_opts(self.reset.as_ref(), None), + } + } +} + +#[cfg(test)] +#[path = "loader_tests.rs"] +mod tests; diff --git a/crates/jp_config/src/loader_tests.rs b/crates/jp_config/src/loader_tests.rs new file mode 100644 index 00000000..05a97f23 --- /dev/null +++ b/crates/jp_config/src/loader_tests.rs @@ -0,0 +1,49 @@ +use serde_json::json; +use test_log::test; + +use super::*; +use crate::PartialAppConfig; + +#[test] +fn test_loader_reset_deserialize() { + let partial: PartialLoaderConfig = serde_json::from_value(json!({ "reset": "none" })).unwrap(); + assert_eq!(partial.reset, Some(LoaderReset::None)); + + let partial: PartialLoaderConfig = serde_json::from_value(json!({})).unwrap(); + assert_eq!(partial.reset, None); +} + +#[test] +fn test_loader_reset_rejects_unknown_target() { + // Only `"none"` is defined by [RFD 038]; unknown targets fail loudly + // instead of being misread, keeping the field open for future variants. + let result = serde_json::from_value::(json!({ "reset": "workspace" })); + assert!(result.is_err()); +} + +#[test] +fn test_fill_from_prefers_own_value() { + let own = PartialLoaderConfig { + reset: Some(LoaderReset::None), + }; + let filled = own.fill_from(PartialLoaderConfig { reset: None }); + assert_eq!(filled.reset, Some(LoaderReset::None)); + + let filled = PartialLoaderConfig { reset: None }.fill_from(PartialLoaderConfig { + reset: Some(LoaderReset::None), + }); + assert_eq!(filled.reset, Some(LoaderReset::None)); +} + +#[test] +fn test_app_config_delta_strips_loader() { + // Loader metadata is interpreted at load time and never travels through + // partial deltas ([RFD 038]): only its *effect* outlives loading, never + // the field itself. + let prev = PartialAppConfig::empty(); + let mut next = PartialAppConfig::empty(); + next.loader.reset = Some(LoaderReset::None); + + let delta = prev.delta(next); + assert_eq!(delta.loader.reset, None); +} diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap index f123dbc3..95621335 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap @@ -62,6 +62,7 @@ expression: "AppConfig::fields()" "plugins.auto_install", "plugins.command", "plugins.shutdown_timeout_secs", + "loader.reset", "interrupt.escalation_cooldown_secs", "interrupt.tool_call.action", "interrupt.tool_call.compose_in_editor", diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap index 3d86a796..951499d9 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap @@ -6,6 +6,9 @@ PartialAppConfig { inherit: None, config_load_paths: None, extends: None, + loader: PartialLoaderConfig { + reset: None, + }, assistant: PartialAssistantConfig { name: None, system_prompt: None, diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap index d3b2b008..7d5512dd 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap @@ -14,6 +14,9 @@ Ok( ), ], ), + loader: PartialLoaderConfig { + reset: None, + }, assistant: PartialAssistantConfig { name: None, system_prompt: Some( diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap index 7a5dae78..e0ef8ca5 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap @@ -6,6 +6,9 @@ PartialAppConfig { inherit: None, config_load_paths: None, extends: None, + loader: PartialLoaderConfig { + reset: None, + }, assistant: PartialAssistantConfig { name: None, system_prompt: None, diff --git a/crates/jp_config/src/util.rs b/crates/jp_config/src/util.rs index 75593ca1..ade51e04 100644 --- a/crates/jp_config/src/util.rs +++ b/crates/jp_config/src/util.rs @@ -13,7 +13,7 @@ use schematic::{ConfigLoader, MergeError, MergeResult, PartialConfig, TransformR use tracing::{debug, error, info, trace, warn}; use crate::{ - AppConfig, BoxedError, PartialAppConfig, error::Error, + AppConfig, BoxedError, PartialAppConfig, error::Error, loader::PartialLoaderConfig, types::extending_path::ExtendingRelativePath, }; @@ -202,6 +202,25 @@ fn load_partial_at_path_with_max_depth>( loader.load_partial(&()).map(Some).map_err(Into::into) } +/// Read the `[loader]` section from the config file at `path`, without +/// resolving its `extends` tree. +/// +/// Loader directives steer how the declaring entry itself is loaded, so only +/// the file's own section counts: a `[loader]` section in a file reached +/// through `extends` must not affect the entry ([RFD 038]). +/// +/// # Errors +/// +/// Can error if the file cannot be read or parsed. +/// +/// [RFD 038]: https://jp.computer/rfd/038 +pub fn load_loader_directives>(path: P) -> Result { + Ok(ConfigLoader::::new() + .file(path.as_ref())? + .load_partial(&())? + .loader) +} + /// Load a partial configuration from a file at `path`, walking upwards until /// either the filesystem root or `root` is reached. /// diff --git a/crates/jp_config/src/util_tests.rs b/crates/jp_config/src/util_tests.rs index 195964ce..97bf7234 100644 --- a/crates/jp_config/src/util_tests.rs +++ b/crates/jp_config/src/util_tests.rs @@ -736,6 +736,63 @@ fn test_load_partial_at_path_diamond_is_not_a_cycle() { assert!(partial.is_some()); } +#[test] +fn test_load_loader_directives_reads_own_section() { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config( + &root.join("entry.toml"), + indoc::indoc!( + r#" + [loader] + reset = "none" + "# + ), + ); + + let loader = load_loader_directives(root.join("entry.toml")).unwrap(); + assert_eq!(loader.reset, Some(crate::loader::LoaderReset::None)); + + write_config(&root.join("plain.toml"), "assistant.system_prompt = \"p\""); + let loader = load_loader_directives(root.join("plain.toml")).unwrap(); + assert_eq!(loader.reset, None); +} + +#[test] +fn test_load_loader_directives_ignores_extends() { + // The read is shallow: `[loader]` in a file reached through `extends` + // must not affect the declaring entry ([RFD 038]). + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config( + &root.join("entry.toml"), + indoc::indoc!( + r#" + extends = ["fragment.toml"] + "# + ), + ); + write_config( + &root.join("fragment.toml"), + indoc::indoc!( + r#" + [loader] + reset = "none" + "# + ), + ); + + let loader = load_loader_directives(root.join("entry.toml")).unwrap(); + assert_eq!(loader.reset, None); + + // The full load, by contrast, merges the fragment's section into the + // resolved partial; the pipeline strips it after reading directives. + let partial = load_partial_at_path(root.join("entry.toml")) + .unwrap() + .unwrap(); + assert_eq!(partial.loader.reset, Some(crate::loader::LoaderReset::None)); +} + #[test] fn test_vec_dedup_preserves_order() { let result = vec_dedup(vec![3, 1, 2, 1, 3, 4], &()).unwrap(); diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index e1c72c7d..727b78dc 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use chrono::{DateTime, Utc}; -use jp_config::{AppConfig, PartialAppConfig, PartialConfig as _}; +use jp_config::{AppConfig, ConfigError, PartialAppConfig, PartialConfig as _}; use serde::{Deserialize, Serialize, Serializer}; use serde_json::{Map, Value}; use tracing::{error, warn}; @@ -36,11 +36,15 @@ enum InternalEvent { /// When this event is emitted, all subsequent events in the stream are /// bound to the new configuration. /// - /// This is a *delta* event, meaning that it is merged on top of all other - /// `ConfigDelta` events in the stream. + /// An [`Apply`] delta is merged on top of all previous `ConfigDelta` events + /// in the stream; a [`Reset`] discards the accumulated state, restarting + /// from program defaults. /// /// Any non-config events before the first `ConfigDelta` event are /// considered to have the default configuration. + /// + /// [`Apply`]: ConfigDelta::Apply + /// [`Reset`]: ConfigDelta::Reset ConfigDelta(ConfigDelta), /// An event in the conversation stream. Event(Box), @@ -154,82 +158,156 @@ impl InternalEvent { } /// A configuration delta. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -pub struct ConfigDelta { - /// The timestamp of the event. - #[serde( - serialize_with = "crate::serialize_dt", - deserialize_with = "crate::deserialize_dt" - )] - pub timestamp: DateTime, +#[derive(Debug, Clone, PartialEq)] +pub enum ConfigDelta { + /// Merge a partial configuration on top of the accumulated config state. + Apply(ApplyDelta), - /// The configuration delta. - pub delta: Box, + /// Discard the accumulated config state. + /// + /// Config resolution restarts from program defaults; subsequent [`Apply`] + /// events layer on top. + /// + /// [`Apply`]: Self::Apply + Reset(ResetDelta), } impl ConfigDelta { - /// Get the [`PartialAppConfig`] delta. + /// The timestamp of the event, regardless of variant. #[must_use] - pub fn into_inner(self) -> Box { - self.delta + pub const fn timestamp(&self) -> DateTime { + match self { + Self::Apply(delta) => delta.timestamp, + Self::Reset(delta) => delta.timestamp, + } } } -impl std::ops::Deref for ConfigDelta { - type Target = PartialAppConfig; +// Hand-rolled so `Apply` keeps the legacy flat shape (no `op` field) and +// `Reset` carries `"op": "reset"`. The variant discriminator must live inside +// the event body: the outer `InternalEvent` envelope already claims the +// top-level `type` key. +impl Serialize for ConfigDelta { + fn serialize(&self, serializer: S) -> Result { + match self { + Self::Apply(delta) => delta.serialize(serializer), + Self::Reset(delta) => { + #[derive(Serialize)] + struct Tagged<'a> { + op: &'static str, + #[serde(flatten)] + inner: &'a ResetDelta, + } - fn deref(&self) -> &Self::Target { - &self.delta + Tagged { + op: "reset", + inner: delta, + } + .serialize(serializer) + } + } } } -impl std::ops::DerefMut for ConfigDelta { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.delta - } +/// A configuration delta that merges on top of the accumulated config state. +#[derive(Debug, Clone, Serialize, PartialEq)] +pub struct ApplyDelta { + /// The timestamp of the event. + #[serde(serialize_with = "crate::serialize_dt")] + pub timestamp: DateTime, + + /// The configuration delta. + pub delta: Box, +} + +/// A configuration delta that discards the accumulated config state, resetting +/// it to program defaults. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub struct ResetDelta { + /// The timestamp of the event. + #[serde(serialize_with = "crate::serialize_dt")] + pub timestamp: DateTime, } -impl AsRef for ConfigDelta { - fn as_ref(&self) -> &PartialAppConfig { - &self.delta +impl From for ConfigDelta { + fn from(delta: ApplyDelta) -> Self { + Self::Apply(delta) } } -impl From for PartialAppConfig { - fn from(delta: ConfigDelta) -> Self { - *delta.delta +impl From for ConfigDelta { + fn from(delta: ResetDelta) -> Self { + Self::Reset(delta) } } impl From for ConfigDelta { fn from(config: PartialAppConfig) -> Self { - Self { + Self::Apply(ApplyDelta { timestamp: Utc::now(), delta: Box::new(config), - } + }) } } /// Deserialize a [`ConfigDelta`] from a raw JSON value, tolerating schema -/// changes. +/// changes within the `delta` subtree. /// +/// The `op` field selects the variant: absent (which covers every event written +/// before the reset variant existed) or `"apply"` decodes as +/// [`ConfigDelta::Apply`]; `"reset"` decodes as [`ConfigDelta::Reset`]. /// Delegates to [`deserialize_partial_config`] for the `delta` subtree and /// extracts the timestamp separately. -pub(crate) fn deserialize_config_delta(value: &Value) -> ConfigDelta { - let delta = value - .get("delta") - .cloned() - .map_or_else(PartialAppConfig::empty, deserialize_partial_config); - +/// +/// # Errors +/// +/// Returns an error for any other `op` value: an op added by a newer version +/// must fail loudly here instead of being misread as an apply and corrupting +/// config resolution. +pub(crate) fn deserialize_config_delta(value: &Value) -> Result { let timestamp = value .get("timestamp") .and_then(Value::as_str) .and_then(|s| crate::parse_dt(s).ok()) .unwrap_or_else(Utc::now); - ConfigDelta { + if let Some(op) = value.get("op") { + if op == "reset" { + return Ok(ConfigDelta::Reset(ResetDelta { timestamp })); + } + + if op != "apply" { + return Err(format!("unknown config delta `op`: {op}")); + } + } + + let delta = value + .get("delta") + .cloned() + .map_or_else(PartialAppConfig::empty, deserialize_partial_config); + + Ok(ConfigDelta::Apply(ApplyDelta { timestamp, delta: Box::new(delta), + })) +} + +/// Fold a single [`ConfigDelta`] into an accumulated partial config state. +/// +/// [`Apply`] merges the delta on top of `state`. +/// [`Reset`] discards `state`, restarting from the empty partial +/// (`PartialAppConfig::default()`); program defaults are injected when the +/// partial is finalized into an [`AppConfig`]. +/// +/// [`Apply`]: ConfigDelta::Apply +/// [`Reset`]: ConfigDelta::Reset +fn fold_config_delta(state: &mut PartialAppConfig, delta: ConfigDelta) -> Result<(), ConfigError> { + match delta { + ConfigDelta::Apply(apply) => state.merge(&(), *apply.delta), + ConfigDelta::Reset(_) => { + *state = PartialAppConfig::default(); + Ok(()) + } } } @@ -318,6 +396,10 @@ impl ConversationStream { /// in the stream from first to last, including any delta's that come /// *after* the last conversation event. /// + /// A [`Reset`] delta discards everything accumulated before it, including + /// the base configuration's contribution; resolution restarts from program + /// defaults. + /// /// If you need the configuration state of the last event in the stream, use /// [`ConversationStream::last`], which returns a /// [`ConversationEventWithConfig`]. containing the `config` field for that @@ -326,6 +408,8 @@ impl ConversationStream { /// # Errors /// /// Returns an error if the merged configuration is invalid. + /// + /// [`Reset`]: ConfigDelta::Reset pub fn config(&self) -> Result { let mut partial = self.base_config.to_partial(); let iter = self.events.iter().filter_map(|event| match event { @@ -336,7 +420,7 @@ impl ConversationStream { }); for delta in iter { - partial.merge(&(), delta.into())?; + fold_config_delta(&mut partial, delta)?; } AppConfig::from_partial_with_defaults(partial).map_err(Into::into) @@ -380,25 +464,70 @@ impl ConversationStream { /// Add a config delta to the stream. /// - /// This is a no-op if the delta is empty. + /// An [`Apply`] delta is reduced to its diff against the stream's current + /// config state; if the diff is empty, nothing is appended. + /// A [`Reset`] is always appended: it carries no diff to suppress, and its + /// presence in the stream is the point. + /// + /// [`Apply`]: ConfigDelta::Apply + /// [`Reset`]: ConfigDelta::Reset pub fn add_config_delta(&mut self, delta: impl Into) { - let ConfigDelta { delta, timestamp } = delta.into(); - let delta = match self.config() { - Ok(config) => config.to_partial().delta(*delta), - Err(error) => { - error!(%error, "Unable to get valid config from conversation stream."); - return; + let delta = match delta.into() { + ConfigDelta::Apply(ApplyDelta { delta, timestamp }) => { + let delta = match self.config() { + Ok(config) => config.to_partial().delta(*delta), + Err(error) => { + error!(%error, "Unable to get valid config from conversation stream."); + return; + } + }; + + if delta.is_empty() { + return; + } + + ConfigDelta::Apply(ApplyDelta { + delta: Box::new(delta), + timestamp, + }) } + reset @ ConfigDelta::Reset(_) => reset, }; - if delta.is_empty() { - return; - } + self.events.push(InternalEvent::ConfigDelta(delta)); + } + + /// Append a config reset point followed by the state layered on top of it. + /// + /// Writes the reset-then-layer sequence as-is: the [`Reset`] is always + /// appended, then each non-empty layer as an [`Apply`]. + /// No diff-suppression runs — between the `Reset` and whichever layer + /// restores the required fields, the stream may not resolve to a valid + /// configuration, so the suppression path in [`Self::add_config_delta`] + /// (which resolves the stream's current config) cannot run. + /// + /// This is the only way to append an `Apply` without diff-suppression: + /// tying the verbatim writes to a preceding `Reset` keeps the + /// reset-then-layer invariant enforced at the API level. + /// + /// [`Apply`]: ConfigDelta::Apply + /// [`Reset`]: ConfigDelta::Reset + pub fn add_config_reset( + &mut self, + reset: ResetDelta, + layers: impl IntoIterator, + ) { + self.events + .push(InternalEvent::ConfigDelta(ConfigDelta::Reset(reset))); + + for layer in layers { + if layer.delta.is_empty() { + continue; + } - self.events.push(InternalEvent::ConfigDelta(ConfigDelta { - delta: Box::new(delta), - timestamp, - })); + self.events + .push(InternalEvent::ConfigDelta(ConfigDelta::Apply(layer))); + } } /// Add a config delta to the stream. @@ -1212,7 +1341,7 @@ impl Extend for ConversationStream { let config_delta = tail.delta(config.clone()); if !config_delta.is_empty() { - self.add_config_delta(ConfigDelta { + self.add_config_delta(ApplyDelta { delta: Box::new(config_delta), timestamp: event.timestamp, }); @@ -1263,7 +1392,7 @@ impl Iterator for IntoIter { match event { InternalEvent::ConfigDelta(delta) => { - if let Err(error) = self.current_config.merge(&(), delta.into()) { + if let Err(error) = fold_config_delta(&mut self.current_config, delta) { error!(%error, "Failed to merge config delta."); } } @@ -1302,7 +1431,7 @@ impl DoubleEndedIterator for IntoIter { // config. for internal_event in self.inner_iter.as_slice() { if let InternalEvent::ConfigDelta(delta) = internal_event - && let Err(error) = config.merge(&(), delta.clone().into()) + && let Err(error) = fold_config_delta(&mut config, delta.clone()) { error!(%error, "Failed to merge config delta."); } @@ -1343,7 +1472,7 @@ impl<'a> Iterator for Iter<'a> { match event { InternalEvent::ConfigDelta(delta) => { - if let Err(error) = self.front_config.merge(&(), delta.clone().into()) { + if let Err(error) = fold_config_delta(&mut self.front_config, delta.clone()) { error!(%error, "Failed to merge config delta."); } } @@ -1374,7 +1503,7 @@ impl DoubleEndedIterator for Iter<'_> { let mut config = self.stream.base_config.to_partial(); for internal_event in &self.stream.events[..self.back] { if let InternalEvent::ConfigDelta(delta) = internal_event - && let Err(error) = config.merge(&(), delta.clone().into()) + && let Err(error) = fold_config_delta(&mut config, delta.clone()) { error!(%error, "Failed to merge config delta."); } @@ -1403,7 +1532,7 @@ impl<'a> Iterator for IterMut<'a> { for event in self.iter.by_ref() { match event { InternalEvent::ConfigDelta(delta) => { - if let Err(error) = self.front_config.merge(&(), delta.clone().into()) { + if let Err(error) = fold_config_delta(&mut self.front_config, delta.clone()) { error!(%error, "Failed to merge config delta."); } } @@ -1726,7 +1855,9 @@ impl<'de> Deserialize<'de> for InternalEvent { .unwrap_or_default(); if tag == "config_delta" { - return Ok(Self::ConfigDelta(deserialize_config_delta(&value))); + return deserialize_config_delta(&value) + .map(Self::ConfigDelta) + .map_err(serde::de::Error::custom); } if tag == "compaction" { diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 0d6e0877..57912fd5 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -898,12 +898,269 @@ fn test_roundtrip_delta_strip_unknown_field_preserves_rest() { json["delta"]["style"]["code"]["removed_field"] = serde_json::json!("stale"); let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); - let InternalEvent::ConfigDelta(result) = deserialized else { - panic!("expected ConfigDelta"); + let InternalEvent::ConfigDelta(ConfigDelta::Apply(result)) = deserialized else { + panic!("expected Apply config delta"); }; assert_eq!(result.delta.style.code.color, Some(false)); } +#[test] +fn test_internal_event_config_delta_reset_roundtrip() { + let reset = ConfigDelta::Reset(ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + }); + + let event = InternalEvent::ConfigDelta(reset.clone()); + let json = serde_json::to_value(&event).unwrap(); + + assert_eq!( + json, + serde_json::json!({ + "type": "config_delta", + "op": "reset", + "timestamp": "2020-01-01 00:00:00.0", + }) + ); + + let deserialized: InternalEvent = serde_json::from_value(json).unwrap(); + assert_eq!(deserialized, InternalEvent::ConfigDelta(reset)); +} + +#[test] +fn test_internal_event_config_delta_apply_shape_has_no_op_field() { + let mut partial = jp_config::PartialAppConfig::empty(); + partial.style.code.color = Some(false); + + let event = InternalEvent::ConfigDelta(ConfigDelta::Apply(ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + delta: Box::new(partial), + })); + + let json = serde_json::to_value(&event).unwrap(); + let mut keys: Vec<_> = json.as_object().unwrap().keys().cloned().collect(); + keys.sort(); + + assert_eq!(keys, ["delta", "timestamp", "type"]); + assert_eq!(json["type"], "config_delta"); + assert_eq!(json["timestamp"], "2020-01-01 00:00:00.0"); + assert_eq!(json["delta"]["style"]["code"]["color"], false); +} + +#[test] +fn test_legacy_config_delta_without_op_decodes_as_apply() { + // Every config delta written before the reset variant existed lacks an + // `op` field; those must keep decoding as `Apply`. + let raw = serde_json::json!({ + "type": "config_delta", + "timestamp": "2025-01-01 00:00:00.0", + "delta": { "style": { "code": { "color": false } } } + }); + + let internal: InternalEvent = serde_json::from_value(raw).unwrap(); + let InternalEvent::ConfigDelta(ConfigDelta::Apply(apply)) = internal else { + panic!("expected Apply config delta"); + }; + assert_eq!(apply.delta.style.code.color, Some(false)); +} + +#[test] +fn test_config_delta_with_explicit_apply_op_decodes_as_apply() { + let raw = serde_json::json!({ + "type": "config_delta", + "op": "apply", + "timestamp": "2025-01-01 00:00:00.0", + "delta": { "style": { "code": { "color": false } } } + }); + + let internal: InternalEvent = serde_json::from_value(raw).unwrap(); + let InternalEvent::ConfigDelta(ConfigDelta::Apply(apply)) = internal else { + panic!("expected Apply config delta"); + }; + assert_eq!(apply.delta.style.code.color, Some(false)); +} + +#[test] +fn test_config_delta_with_unknown_op_fails_deserialization() { + // An op added by a newer jp must fail loudly instead of being misread as + // an apply. + let raw = serde_json::json!({ + "type": "config_delta", + "op": "unset", + "timestamp": "2025-01-01 00:00:00.0", + }); + assert!(serde_json::from_value::(raw).is_err()); + + // Non-string values are rejected too. + let raw = serde_json::json!({ + "type": "config_delta", + "op": 42, + "timestamp": "2025-01-01 00:00:00.0", + }); + assert!(serde_json::from_value::(raw).is_err()); +} + +#[test] +fn test_add_config_delta_reset_always_appends() { + let mut stream = ConversationStream::new_test(); + let delta_count = |s: &ConversationStream| { + s.events + .iter() + .filter(|e| matches!(e, InternalEvent::ConfigDelta(_))) + .count() + }; + + // An `Apply` whose diff against the current config is empty is suppressed. + stream.add_config_delta(jp_config::PartialAppConfig::empty()); + assert_eq!(delta_count(&stream), 0); + + // A `Reset` always lands, even though it carries no diff. + stream.add_config_delta(ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + }); + assert_eq!(delta_count(&stream), 1); +} + +#[test] +fn test_add_config_reset_appends_reset_then_nonempty_layers() { + let mut stream = ConversationStream::new_test(); + + let mut layer = jp_config::PartialAppConfig::empty(); + layer.style.code.color = Some(false); + + stream.add_config_reset( + ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + }, + [ + // Empty layers are skipped: they would resolve to a no-op `Apply`. + ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + delta: Box::new(jp_config::PartialAppConfig::empty()), + }, + ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + delta: Box::new(layer), + }, + ], + ); + + // The reset lands first, followed by the single non-empty layer — + // verbatim, without diff-suppression (the delta is written as given, not + // reduced against the stream's current config). + let deltas: Vec<_> = stream + .events + .iter() + .filter_map(|e| match e { + InternalEvent::ConfigDelta(delta) => Some(delta), + _ => None, + }) + .collect(); + + assert_eq!(deltas.len(), 2, "expected [Reset, Apply], got {deltas:?}"); + assert!(matches!(deltas[0], ConfigDelta::Reset(_))); + let ConfigDelta::Apply(apply) = deltas[1] else { + panic!("expected Apply config delta"); + }; + assert_eq!(apply.delta.style.code.color, Some(false)); +} + +#[test] +fn test_config_fold_reset_discards_accumulated_state() { + use jp_config::model::id::{Name, PartialModelIdConfig, ProviderId}; + + let mut stream = ConversationStream::new_test(); + + // Diverge from the program default (`style.code.color` defaults to + // `true`). The test base config also carries + // `conversation.title.generate.auto = false` (default `true`). + let mut dev = jp_config::PartialAppConfig::empty(); + dev.style.code.color = Some(false); + + // The post-reset state starts from program defaults, which lack the + // required model id and tool run mode, so `fresh` must supply both for + // the stream config to be valid. + let mut fresh = jp_config::PartialAppConfig::empty(); + fresh.conversation.tools.defaults.run = Some(RunMode::Ask); + fresh.assistant.model.id = PartialModelIdConfig { + provider: Some(ProviderId::Anthropic), + name: Some(Name("fresh".to_owned())), + } + .into(); + + for delta in [ + ConfigDelta::Apply(ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + delta: Box::new(dev), + }), + ConfigDelta::Reset(ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 1).unwrap(), + }), + ConfigDelta::Apply(ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), + delta: Box::new(fresh), + }), + ] { + stream.events.push(InternalEvent::ConfigDelta(delta)); + } + + let config = stream.config().unwrap(); + + // dev's change is behind the reset; the program default is restored. + assert!(config.style.code.color); + // The base config's contribution is discarded too. + assert!(config.conversation.title.generate.auto); + // fresh applies on top of program defaults. + assert_eq!(config.assistant.model.id.resolved().name.0, "fresh"); +} + +#[test] +fn test_iter_config_reflects_reset() { + let mut dev = jp_config::PartialAppConfig::empty(); + dev.style.code.color = Some(false); + + let mut fresh = jp_config::PartialAppConfig::empty(); + fresh.user.name = Some("fresh".to_owned()); + + let mut stream = ConversationStream::new_test(); + stream + .events + .push(InternalEvent::ConfigDelta(ConfigDelta::Apply(ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), + delta: Box::new(dev), + }))); + stream.push(ConversationEvent::new( + ChatRequest::from("before reset"), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 1).unwrap(), + )); + stream + .events + .push(InternalEvent::ConfigDelta(ConfigDelta::Reset(ResetDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), + }))); + stream + .events + .push(InternalEvent::ConfigDelta(ConfigDelta::Apply(ApplyDelta { + timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), + delta: Box::new(fresh), + }))); + stream.push(ConversationEvent::new( + ChatRequest::from("after reset"), + Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 4).unwrap(), + )); + + let events: Vec<_> = stream.iter().collect(); + assert_eq!(events.len(), 2); + + // Before the reset: dev's change plus the base config's fields. + assert_eq!(events[0].config.style.code.color, Some(false)); + + // After the reset: only fresh's fields remain; both dev's change and the + // base config's contribution are gone. + assert_eq!(events[1].config.style.code.color, None); + assert_eq!(events[1].config.user.name, Some("fresh".to_owned())); + assert!(events[1].config.conversation.title.generate.auto.is_none()); +} + // --- deserialize_config_delta tests --- #[test] @@ -915,9 +1172,12 @@ fn test_deserialize_config_delta_extracts_timestamp_and_delta() { } }); - let delta = deserialize_config_delta(&value); - assert_eq!(delta.timestamp.to_string(), "2025-01-01 00:00:00 UTC"); - assert_eq!(delta.delta.style.code.color, Some(false)); + let delta = deserialize_config_delta(&value).unwrap(); + assert_eq!(delta.timestamp().to_string(), "2025-01-01 00:00:00 UTC"); + let ConfigDelta::Apply(apply) = delta else { + panic!("expected Apply config delta"); + }; + assert_eq!(apply.delta.style.code.color, Some(false)); } #[test] @@ -927,9 +1187,12 @@ fn test_deserialize_config_delta_preserves_timestamp_on_bad_delta() { "delta": "not an object at all" }); - let delta = deserialize_config_delta(&value); - assert_eq!(delta.timestamp.to_string(), "2024-12-25 18:30:00 UTC"); - assert!(delta.delta.is_empty()); + let delta = deserialize_config_delta(&value).unwrap(); + assert_eq!(delta.timestamp().to_string(), "2024-12-25 18:30:00 UTC"); + let ConfigDelta::Apply(apply) = delta else { + panic!("expected Apply config delta"); + }; + assert!(apply.delta.is_empty()); } // --- from_parts / to_parts stream-level compat tests --- @@ -1609,7 +1872,7 @@ fn extend_into_empty_preserves_observed_iter_and_serialized_shape() { // Build a source stream with two turns and a config delta before each. let mut source = ConversationStream::new_test(); - source.add_config_delta(ConfigDelta { + source.add_config_delta(ApplyDelta { delta: Box::new(partial1), timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), }); @@ -1625,7 +1888,7 @@ fn extend_into_empty_preserves_observed_iter_and_serialized_shape() { ChatResponse::message("A1"), Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 2).unwrap(), )); - source.add_config_delta(ConfigDelta { + source.add_config_delta(ApplyDelta { delta: Box::new(partial2), timestamp: Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 3).unwrap(), }); diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 18f1688b..3cb7c9e0 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -196,7 +196,7 @@ "summary": "Adds scripting ergonomics: shared config structs, `conversation new` command, `--no-activate` flag, and `--root-id` ancestry constraint." }, "051-sub-agent-workflows.md": { - "hash": "dfda328626635792c345489714f59ff6d07b4e010c9da37e0b5e028713d689e6", + "hash": "75cefb8d7ca2579b9f1371ac230a9205635ed53543e827cb1d38030f9647095b", "summary": "Guide for building sub-agent workflows using local tools, config overlays, and conversation trees to delegate scoped tasks cheaply." }, "052-workspace-data-store-sanitization.md": { @@ -288,11 +288,11 @@ "summary": "Adds conversation archiving to JP, moving inactive conversations to a hidden partition while preserving them." }, "038-config-reset-keywords.md": { - "hash": "9e3ec573727ac4dd94589dd82904fd0b6946eecd2da45bc354d92c71ce698984", + "hash": "0492ca03e690477c1a5c63a9beaa1f138c7cc964f4d3c3ccaf968dbed9dd3a73", "summary": "UPPERCASE keywords NONE and WORKSPACE reset config state; loader.reset entry setting provides local resets; ConfigDelta becomes enum to persist resets." }, "079-config-sources-and-load-order.md": { - "hash": "5e5c78898d1ba2ac5de5ef86ebad0da886c8ca608beaa6fd202e75ba6db670fc", + "hash": "2ba304ffd0f14f3f1830aaaac43820461da856920d708e9b0192815a47aee687", "summary": "JP loads config from four implicit sources plus environment variables, with extends directives and inherit flags controlling precedence." }, "074-eager-loading-with-command-declared-data-requirements.md": { diff --git a/docs/rfd/038-config-reset-keywords.md b/docs/rfd/038-config-reset-keywords.md index caaf7fb8..a0995b07 100644 --- a/docs/rfd/038-config-reset-keywords.md +++ b/docs/rfd/038-config-reset-keywords.md @@ -1,10 +1,9 @@ # RFD 038: Config Reset Keywords -- **Status**: Accepted +- **Status**: Implemented - **Category**: Design - **Authors**: Jean Mertz - **Date**: 2026-03-08 -- **Required by**: [RFD 051], [RFD 070], [RFD 079] ## Summary @@ -88,8 +87,9 @@ and additionally triggers a pre-pipeline gate: if `NONE` appears anywhere in the entirely. Only explicit `--cfg` values apply on top of program defaults. -Required fields without defaults (for example `assistant.model.id`) must be -supplied by subsequent explicit `--cfg` values. +Required fields without defaults (for example `assistant.model.id` and +`conversation.tools.defaults.run`) must be supplied by subsequent explicit +`--cfg` values. Otherwise validation fails with a clear error indicating which fields are missing. @@ -384,7 +384,8 @@ own `type` tag and break deserialization (the existing Wire shapes: ```json -// Apply (unchanged from today; no `op` field). +// Apply (unchanged from today; no `op` field is written, though an explicit +// "apply" is accepted on read). {"type": "config_delta", "timestamp": "...", "delta": { /* ... */ }} // Reset (new). @@ -392,10 +393,13 @@ Wire shapes: ``` The `Apply` form is identical to today's on-disk shape. -Legacy events (no `op` field) decode as `Apply` — no migration needed. -`deserialize_config_delta` in `crates/jp_conversation/src/stream.rs` is updated -to look for `op == "reset"` first and fall through to the legacy `Apply` parse -otherwise. +`deserialize_config_delta` in `crates/jp_conversation/src/stream.rs` matches the +`op` field strictly: absent (which covers all legacy events, so no migration is +needed) or `"apply"` decodes as `Apply`; `"reset"` decodes as `Reset`; any other +value is a deserialization error. +Rejecting unknown ops keeps the field open for future variants: an op written by +a newer version fails loudly on older versions instead of being misread as an +`Apply`. Fold semantics: @@ -553,6 +557,13 @@ not at base resolution time. The current validation flow should already handle this (validation happens on the final merged config), but it needs verification. +Phase 1 adds a related constraint for continuing conversations: +`add_config_delta` suppresses empty `Apply` diffs by resolving the stream's +current config, and that resolution fails between a `Reset` and whichever +`Apply` restores the required fields. +Phase 2 must append post-reset `Apply` events directly (or defer diffing) rather +than routing them through the suppression path. + ### `NONE` detection ordering Because `NONE`'s pre-scan gate affects implicit config loading, it must be @@ -601,19 +612,20 @@ Promote `ConfigDelta` from a struct to an enum with `Apply` and `Reset` variants The outer `InternalEvent` wrapper continues to add `"type": "config_delta"` unchanged. - Update the hand-rolled `deserialize_config_delta` in - `crates/jp_conversation/src/stream.rs` to dispatch on `op == "reset"`. - Events without an `op` field (including all legacy events) decode as `Apply`. + `crates/jp_conversation/src/stream.rs` to dispatch on the `op` field: absent + (all legacy events) or `"apply"` decodes as `Apply`, `"reset"` decodes as + `Reset`, and any other value is a deserialization error. - Update `add_config_delta` in the same file: the existing destructure and empty-diff suppression apply only to `Apply`. `add_config_delta(Reset)` always appends — `Reset` events have no diff to suppress and their presence is the whole point. -- Update the stream fold (in `config()`, `Iter`, `IterMut`, `IntoIter`, and the - `apply_config_delta` helper introduced by [RFD 070] Phase 1) to match on the - variant: - - `Apply`: existing merge + unset + claims logic. - - `Reset`: replace accumulated state with `PartialAppConfig::default()`; clear - any in-progress claims state. -- Update [RFD 070]'s walk-back algorithm to terminate at `Reset` events. +- Centralize the stream fold in a `fold_config_delta` helper shared by + `config()`, `Iter`, `IterMut`, and `IntoIter`, matching on the variant: + - `Apply`: existing merge logic. + - `Reset`: replace accumulated state with `PartialAppConfig::default()`. +- [RFD 070] hooks into the same helper when it lands (it requires this RFD): + `Apply` gains unset and claims handling, `Reset` clears per-invocation claims + state, and its walk-back algorithm terminates at `Reset` events. - Add a common `timestamp()` accessor on `ConfigDelta` so call sites that only need the timestamp don't need to match on the variant. @@ -623,12 +635,14 @@ Tests: serialize/deserialize (not just `deserialize_config_delta` in isolation), asserting the on-disk shape is `{"type":"config_delta","op":"reset",...}`. - `ConfigDelta::Apply` on-disk shape is unchanged from today. -- Backward compat: legacy events without `op` decode as `Apply`. +- Backward compat: legacy events without `op` decode as `Apply`, and an explicit + `op == "apply"` is accepted. +- Unknown `op` values fail deserialization. - `add_config_delta(ConfigDelta::Reset(_))` always appends, even on a stream where adding an empty `Apply` would be suppressed. - Fold: stream `[Apply(dev), Reset, Apply(fresh)]` resolves to defaults + fresh. - `-C dev` walk-back after `[Apply(dev), Reset]` reports no matching claims - (`Reset` terminated the walk). + (`Reset` terminated the walk; implemented as part of [RFD 070]'s claims work). Can be merged independently. @@ -729,7 +743,6 @@ Conversation-ID resolution is out of scope for this RFD (see partial-merge primitives. [RFD 008]: 008-ordered-tool-directives.md -[RFD 051]: 051-sub-agent-workflows.md [RFD 054]: 054-split-conversation-config-and-events.md [RFD 070]: 070-negative-config-deltas.md [RFD 079]: 079-config-sources-and-load-order.md diff --git a/docs/rfd/051-sub-agent-workflows.md b/docs/rfd/051-sub-agent-workflows.md index d6434828..41d575b1 100644 --- a/docs/rfd/051-sub-agent-workflows.md +++ b/docs/rfd/051-sub-agent-workflows.md @@ -4,7 +4,7 @@ - **Category**: Guide - **Authors**: Jean Mertz - **Date**: 2026-03-08 -- **Requires**: [RFD 038], [RFD 039], [RFD 040], [RFD 049], [RFD 050] +- **Requires**: [RFD 039], [RFD 040], [RFD 049], [RFD 050] ## Summary diff --git a/docs/rfd/079-config-sources-and-load-order.md b/docs/rfd/079-config-sources-and-load-order.md index 4c43a754..811031fb 100644 --- a/docs/rfd/079-config-sources-and-load-order.md +++ b/docs/rfd/079-config-sources-and-load-order.md @@ -4,7 +4,6 @@ - **Category**: Guide - **Authors**: Jean Mertz - **Date**: 2026-04-20 -- **Requires**: [RFD 038] - **Required by**: [RFD 080] ## Summary