Skip to content

fix(core): make ${_param.…} substitution single-pass and order-independent - #3101

Draft
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-tkzcup-param-subst-single-pass
Draft

fix(core): make ${_param.…} substitution single-pass and order-independent#3101
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-tkzcup-param-subst-single-pass

Conversation

@phil-opp

@phil-opp phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Issue

substitute_params_in_node expands ${_param.<key>} references in a module node's args via substitute_params_in_str (libraries/core/src/descriptor/expand.rs):

fn substitute_params_in_str(s: &str, params: &BTreeMap<String, String>) -> String {
    let mut result = s.to_string();
    for (key, value) in params {
        let pattern = format!("${{_param.{key}}}");
        result = result.replace(&pattern, value);   // runs over the PREVIOUS result
    }
    result
}

Because each replace runs over the output of the previous iteration, the result is order-dependent (on BTreeMap key order) and transitively recursive: a parameter whose value contains another ${_param.…} token — or a literal one the user wanted to preserve — gets expanded again.

Concretely, with a = "${_param.b}" and b = "x": a sorts before b, so ${_param.a} becomes ${_param.b} and then x. Swap the names and it does not expand. A user value that legitimately contains the literal text ${_param.something} is silently corrupted.

Fix

Replace the chained-replace loop with a single left-to-right scan that substitutes each ${_param.<key>} token exactly once and continues after the substituted region, so substituted values are never re-scanned. This makes substitution simultaneous and order-independent. Unknown keys are left verbatim (matching the old "only replace keys present in the map" behavior), and a dangling ${_param. with no closing brace is emitted literally with guaranteed loop progress.

No change for the ordinary case (--speed ${_param.speed}--speed 2.0).

Validation

  • Added unit tests: basic/repeated/unknown-key/dangling-prefix, and an order-independent + non-transitive case.
  • Existing expand_params_in_args / expand_params_in_env tests still pass.
  • cargo +1.97.1 fmt -p dora-core -- --check — clean.
  • cargo +1.97.1 clippy -p dora-core -- -D warnings — clean.
  • cargo +1.97.1 test -p dora-core substitute_params / expand_params — all pass.

⚠️ This is a machine-generated pull request authored by Claude (Claude Code). A human should review before merging.

🤖 Generated with Claude Code


Generated by Claude Code

…pendent

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.<key>}` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj
@trunk-io

trunk-io Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude Code — fully automated review, not vetted by a human.

No issues found.

The rewritten substitute_params_in_str is correct: it scans once left-to-right, advances rest past the closing brace so substituted values are never re-scanned, leaves unknown keys verbatim (matching the old "only replace keys present in the map" behavior), and always makes progress on a dangling ${_param. prefix, so the loop terminates. Slicing is UTF-8-safe since PREFIX and } are ASCII. The added tests cover the meaningful cases (repeated keys, unknown key, dangling prefix, and the order-independence / non-transitivity regression).

One behavioral note, not a blocker: a value that itself contains a ${_param.…} token is now inserted verbatim rather than transitively expanded. That is the intended fix, and since the old expansion was order-dependent it's unlikely anything relied on it — worth calling out only in case nested-param expansion was ever meant to be supported.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants