From a476e1c9271abea9716db1b9c73aa1733ef61720 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 23:41:58 +0000 Subject: [PATCH] =?UTF-8?q?fix(core):=20make=20`${=5Fparam.=E2=80=A6}`=20s?= =?UTF-8?q?ubstitution=20single-pass=20and=20order-independent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit substitute_params_in_str looped over the params map calling String::replace on the accumulating result. That made expansion depend on BTreeMap key ordering and let a parameter value that itself contained a `${_param.…}` token (or a literal one the user wanted to keep) be expanded transitively — e.g. with a="${_param.b}", b="x", the arg `${_param.a}` expanded all the way to `x` only because `a` sorts before `b`. Replace it with a single left-to-right scan that substitutes each `${_param.}` token once and never re-examines substituted text, so substitution is simultaneous and order-independent. Unknown keys are left verbatim, matching the previous behavior of only replacing keys present in the map. Adds unit tests for the basic, unknown-key, dangling-prefix, and non-transitive/order-independent cases. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj --- libraries/core/src/descriptor/expand.rs | 89 +++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 4 deletions(-) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 14b5f29eaa..124fe9d467 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -650,12 +650,49 @@ fn substitute_params_in_node(node: &mut Node, params: &BTreeMap) } } +/// Replace every `${_param.}` token with its parameter value in a single +/// left-to-right pass. +/// +/// A previous implementation looped over the params calling `String::replace` +/// on the accumulating result, which had two problems: the outcome depended on +/// `BTreeMap` key ordering, and a parameter *value* that itself contained a +/// `${_param.…}` token (or a literal one a user wanted to keep) was expanded +/// transitively. Scanning once and never re-examining substituted text makes +/// substitution simultaneous and order-independent. Unknown keys are left +/// verbatim, matching the old behavior of only replacing keys present in +/// `params`. fn substitute_params_in_str(s: &str, params: &BTreeMap) -> String { - let mut result = s.to_string(); - for (key, value) in params { - let pattern = format!("${{_param.{key}}}"); - result = result.replace(&pattern, value); + const PREFIX: &str = "${_param."; + let mut result = String::with_capacity(s.len()); + let mut rest = s; + while let Some(start) = rest.find(PREFIX) { + result.push_str(&rest[..start]); + let after_prefix = &rest[start + PREFIX.len()..]; + match after_prefix.find('}') { + Some(end) => { + let key = &after_prefix[..end]; + match params.get(key) { + Some(value) => result.push_str(value), + // Unknown key: emit the token unchanged rather than dropping it. + None => { + result.push_str(PREFIX); + result.push_str(key); + result.push('}'); + } + } + // Continue *after* the closing brace so a substituted value is + // never re-scanned for further tokens. + rest = &after_prefix[end + 1..]; + } + // No closing brace: emit the prefix literally and continue past it + // (guarantees progress, so the loop always terminates). + None => { + result.push_str(PREFIX); + rest = after_prefix; + } + } } + result.push_str(rest); result } @@ -1639,6 +1676,50 @@ nodes: assert_eq!(proc.args.as_deref(), Some("--speed 2.0 --verbose")); } + #[test] + fn substitute_params_basic_and_unknown() { + let params = BTreeMap::from([ + ("speed".to_string(), "2.0".to_string()), + ("name".to_string(), "robot".to_string()), + ]); + assert_eq!( + substitute_params_in_str("--speed ${_param.speed} --name ${_param.name}", ¶ms), + "--speed 2.0 --name robot" + ); + // Multiple occurrences of the same key are all replaced. + assert_eq!( + substitute_params_in_str("${_param.speed}/${_param.speed}", ¶ms), + "2.0/2.0" + ); + // An unknown key is left verbatim, not dropped. + assert_eq!( + substitute_params_in_str("${_param.missing}", ¶ms), + "${_param.missing}" + ); + // A dangling prefix without a closing brace is emitted literally. + assert_eq!( + substitute_params_in_str("prefix ${_param.speed", ¶ms), + "prefix ${_param.speed" + ); + } + + #[test] + fn substitute_params_is_order_independent_and_non_transitive() { + // A parameter value that itself looks like a `${_param.…}` token must be + // inserted verbatim, never re-expanded — and the result must not depend + // on `BTreeMap` key ordering. With the old chained-`replace` approach, + // `a`'s value `${_param.b}` was expanded to `x` because `a` sorts first. + let params = BTreeMap::from([ + ("a".to_string(), "${_param.b}".to_string()), + ("b".to_string(), "x".to_string()), + ]); + assert_eq!( + substitute_params_in_str("${_param.a}", ¶ms), + "${_param.b}" + ); + assert_eq!(substitute_params_in_str("${_param.b}", ¶ms), "x"); + } + // ---- Feature 5: module-level build ---- #[test]