fix(config): Apply shared extends files only once - #887
Open
JeanMertz wants to merge 7 commits into
Open
Conversation
A config that reaches the same file through two `extends` branches (a diamond, e.g. `a -> b -> d` and `a -> c -> d`) now applies `d` once instead of once per branch. Previously each branch loaded the file independently, so an `append`/`prepend` merge strategy in the shared file duplicated its contribution to the final value. The `extends` graph is now resolved fully before any file is loaded, then reduced to one entry per file, keeping each file's last position so declared `extends` order (and precedence) is preserved. Merged strings gain the same `dedup` opt-out already available on merged vecs: an `append` or `prepend` whose value is already present as a whole `separator`-delimited block is skipped, so re-supplying the same `--cfg` or hitting the same file through an `extends` diamond does not duplicate prompt text. Dedup now defaults to `true` for both merged vecs and merged strings (previously vecs defaulted to no dedup); set `dedup = false` to keep duplicates. The flag stays sticky across merges, matching the existing vec behavior. `schematic_macros` now treats an invalid `#[setting]`/`#[schema]` attribute as a hard error instead of silently discarding the field's other keys, and adds `deserialize_with` support so a partial field can use a custom deserializer while staying optional. The `default` argument is parsed with a dedicated helper that distinguishes a bare `default` (fall back to `Default::default()`) from `default = <expr>`. `AppConfig::inherit` and `config_load_paths` drop their now-redundant `#[setting(optional)]`, and `parameters.other` stays a nested `[parameters.other]` table rather than being flattened, since stored conversation configs already use a literal `other` key that flattening would otherwise capture as a provider parameter. Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A config that reaches the same file through two
extendsbranches (a diamond, e.g.a -> b -> danda -> c -> d) now appliesdonce instead of once per branch. Previously each branch loaded the file independently, so anappend/prependmerge strategy in the shared file duplicated its contribution to the final value. Theextendsgraph is now resolved fully before any file is loaded, then reduced to one entry per file, keeping each file's last position so declaredextendsorder (and precedence) is preserved.Merged strings gain the same
dedupopt-out already available on merged vecs: anappendorprependwhose value is already present as a wholeseparator-delimited block is skipped, so re-supplying the same--cfgor hitting the same file through anextendsdiamond does not duplicate prompt text. Dedup now defaults totruefor both merged vecs and merged strings (previously vecs defaulted to no dedup); setdedup = falseto keep duplicates. The flag is sticky across merges and survives a plain-string replacement. Strategies that combine two sources (append,prepend) deduplicate by default; areplacedoes not, since its contents are a single source's own data — free-formJsonValuearrays such as tool options and template values are merged through the same path with an implicitreplaceand must survive untouched.assistant.system_promptaccepts dotted overrides for its merge metadata (--cfg assistant.system_prompt.dedup=false,.strategy,.separator,.value,.discard_when_merged), which previously failed withUnknownKey. A plain-string prompt supplied by a lower layer is promoted to the merged form withreplacepinned, preserving the scalar's meaning, and a metadata-only override no longer suppresses the built-in prompt (it gap-fills the default's value instead of resolving to an empty string).dedup = "inherit"is accepted on every input channel and is a no-op for key-value assignments: since those mutate the accumulated partial, writing "no opinion" would erase a lower layer's explicit choice. Returning to the default takes an explicitdedup=true.schematic_macrosnow treats an invalid#[setting]/#[schema]attribute as a hard error instead of silently discarding the field's other keys, and addsdeserialize_withsupport so a partial field can use a custom deserializer while staying optional. Thedefaultargument is parsed with a dedicated helper that distinguishes a baredefault(fall back toDefault::default(), matching what the generated code already emitted) fromdefault = <expr>.The hard error surfaced four fields whose keys had been silently dropped, all of which now behave as declared:
AppConfig::inheritdrops its#[setting(optional)], which never did anything.AppConfig::config_load_pathsdrops the same key, which was aborting the parse of themerge/transformkeys beside it. Load paths now accumulate across config layers instead of the highest layer replacing the rest.--cfg <name>resolution takes the first directory in the list that holds a matching file, so a lower layer's directory can now win a name collision it previously lost.StdioConfig::argumentsandvariables(MCP stdio servers) now append across layers, asmerge = append_vecalways declared.ParametersConfig::stop_wordsnow appends across layers for the same reason.ParametersConfig::otherkeeps itsmergestrategy but stays a nested[parameters.other]table rather than being flattened: stored conversation configs carry a literalotherkey, which a flattened map would capture as a provider parameter namedother. Flattening needs a compat migration first, tracked separately.Also fixed:
string_with_strategytreated an unstatedstrategyas "no strategy" and dropped the value entirely when both sides had one, so--cfg 'assistant.system_prompt:{"value":"x"}'wiped an existing prompt. An unstated strategy now resolves toappend, matchingMergedStringStrategy's default and the vec merge's handling.