diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index dffd867e..47bd928e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -60,7 +60,17 @@ tail -f ~/.pgrx/17.log ## Critical Patterns ### Orchestrations Must Be Deterministic -Files in `src/orchestrations/` must be 100% deterministic—no I/O, no `Utc::now()`, no random numbers. All side effects go through activities. +Files in `src/orchestrations/` must be 100% deterministic. This requirement also applies to every helper they call, even when that helper lives outside `src/orchestrations/`. + +For the same orchestration input and recorded history, code must produce the same ordered durable operations, operation inputs, `continue_as_new` input, and return value on every replay. + +- Do not perform I/O directly. Put side effects in activities. +- Do not use wall-clock time, random values, environment/process state, or mutable global state. Use duroxide context APIs such as `ctx.utc_now()` when a value must be recorded in history. +- Do not let `HashMap`/`HashSet` iteration order affect control flow, mutation order, generated strings, durable operation order, or outputs. Exact-key lookup is safe; observable traversal must use a naturally ordered source, sort explicitly, or use `BTreeMap`/`BTreeSet`. +- Do not serialize unordered maps directly into activity, sub-orchestration, `continue_as_new`, or orchestration output payloads. Canonicalize map keys first; use `serialize_string_map` or `string_map_to_json` for string maps. +- Do not select a "first" or "last" item from an unordered collection. Define the ordering explicitly. +- When changing an orchestration or a transitive helper, consider replay of histories created by the previous binary. Treat changes to durable operation names, order, inputs, timers, branching, and returned output as in-flight compatibility changes. +- Add determinism regression tests for order-sensitive logic. Construct logically identical maps in different insertion orders and assert byte-identical outputs; for substitution/transformation code, include values containing placeholder-like text to catch accidental rescanning. ### Activity Naming Convention Each activity has a co-located `NAME` constant for IDE navigation: diff --git a/src/types.rs b/src/types.rs index 4b90a082..4f6f4221 100644 --- a/src/types.rs +++ b/src/types.rs @@ -845,6 +845,44 @@ fn substitute_results( Ok(out) } +/// Substitute `{name}` placeholders in a single left-to-right pass over the template. +/// Inserted values are appended as opaque text and are never scanned for more placeholders. +fn substitute_braced_variables( + template: &str, + vars: &std::collections::HashMap, + sys_vars: &SystemVars, +) -> String { + let mut out = String::with_capacity(template.len()); + let mut remaining = template; + + while let Some(open) = remaining.find('{') { + out.push_str(&remaining[..open]); + let after_open = &remaining[open + 1..]; + let Some(close) = after_open.find('}') else { + out.push_str(&remaining[open..]); + return out; + }; + + let name = &after_open[..close]; + let replacement = match name { + "sys_instance_id" => Some(sys_vars.instance_id.as_str()), + "sys_label" => Some(sys_vars.label.as_deref().unwrap_or("")), + _ if parse_identifier(name).len() == name.len() => vars.get(name).map(String::as_str), + _ => None, + }; + + if let Some(value) = replacement { + out.push_str(value); + } else { + out.push_str(&remaining[open..open + close + 2]); + } + remaining = &after_open[close + 1..]; + } + + out.push_str(remaining); + out +} + /// Substitute all variable types in a query: /// - {name} for user vars (from FunctionInput.vars) - values are inserted as-is /// - {sys_instance_id}, {sys_label} for system vars - inserted as-is @@ -862,24 +900,13 @@ pub fn substitute_all_with_options( sys_vars: &SystemVars, quote_results_for_sql: bool, ) -> Result { - let mut result = query.to_string(); - - // 1. Substitute system vars: {sys_*} (inserted as-is) - result = result.replace("{sys_instance_id}", &sys_vars.instance_id); - result = result.replace("{sys_label}", sys_vars.label.as_deref().unwrap_or("")); - // SECURITY: Raw substitution of user vars is by design — variables are // intended for SQL fragments (table names, expressions), not just values. // The user controls both the variable content and the query template, and // SQL executes under their own role via connect_as_user(). // See docs/spec-security-model.md §4.3, T10. - // 2. Substitute user vars: {name} (inserted as-is, no quoting) - for (name, value) in vars { - let pattern = format!("{{{name}}}"); - if result.contains(&pattern) { - result = result.replace(&pattern, value); - } - } + // 1/2. Substitute system and user vars in one pass (inserted as-is, no quoting). + let result = substitute_braced_variables(query, vars, sys_vars); // 3. Substitute results: $name with dot-notation, null-safe, and row-set support substitute_results(&result, results, quote_results_for_sql) @@ -1755,6 +1782,48 @@ mod tests { } } + #[test] + fn user_variable_substitution_does_not_rescan_inserted_values() { + let vars = make_results(&[("a", "{b}"), ("b", "value")]); + + let out = substitute_all_raw("{a} / {b}", &empty_vars(), &vars, &sys_vars()).unwrap(); + + assert_eq!(out, "{b} / value"); + } + + #[test] + fn user_variable_substitution_is_independent_of_map_insertion_order() { + let forward = make_results(&[("a", "{b}"), ("b", "value")]); + let reverse = make_results(&[("b", "value"), ("a", "{b}")]); + + let forward_out = + substitute_all_raw("{a} / {b}", &empty_vars(), &forward, &sys_vars()).unwrap(); + let reverse_out = + substitute_all_raw("{a} / {b}", &empty_vars(), &reverse, &sys_vars()).unwrap(); + + assert_eq!(forward_out, reverse_out); + assert_eq!(forward_out, "{b} / value"); + } + + #[test] + fn braced_substitution_preserves_unknowns_and_resolves_system_vars_once() { + let vars = make_results(&[("name", "{sys_instance_id}")]); + let system = SystemVars { + instance_id: "instance-42".to_string(), + label: Some("billing".to_string()), + }; + + let out = substitute_all_raw( + "{sys_instance_id}/{sys_label}/{name}/{unknown}", + &empty_vars(), + &vars, + &system, + ) + .unwrap(); + + assert_eq!(out, "instance-42/billing/{sys_instance_id}/{unknown}"); + } + #[test] fn test_dot_notation_string() { let results =