refactor: config format framework with json support#907
Merged
ayushjain17 merged 2 commits intomainfrom Mar 17, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a unified configuration format abstraction supporting both JSON and TOML parsing. It refactors the previous dedicated TOML module into a modular Changes
Sequence DiagramsequenceDiagram
participant Client as Language Client
participant FFI as FFI Layer
participant Dispatcher as Format Dispatcher
participant Parser as ConfigFormat (Trait)
participant Validator as Validator
participant Converter as Type Converter
participant Output as Config/DetailedConfig
Client->>FFI: JSON/TOML String
FFI->>Dispatcher: Route to JsonFormat/TomlFormat
Dispatcher->>Parser: parse_into_detailed()
Parser->>Validator: validate default_configs
Validator->>Validator: validate dimensions & build dependency graph
Validator->>Validator: detect duplicate positions
Validator->>Parser: validation result
Parser->>Converter: try_into_detailed() transformation
Converter->>Converter: build contexts, sort by priority
Converter->>Output: DetailedConfig
Output->>FFI: DetailedConfig (JSON serialized for legacy FFI)
FFI->>Client: Config or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
knutties
force-pushed
the
other-config-formats
branch
2 times, most recently
from
e99b6d2 to
b7d3b2a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt (1)
101-131: MaketestTomlJsonEquivalenceuse equivalent fixtures.The TOML sample only defines 2 dimensions and 2 overrides, while
EXAMPLE_JSONhas 3 dimensions and 5 overrides. With only the Line 129defaultConfigs.sizeassertion, this test can stay green even when the two formats diverge materially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt` around lines 101 - 131, The TOML fixture in testTomlJsonEquivalence (variables tomlConfig and EXAMPLE_JSON used with ffiParseTomlConfig and ffiParseJsonConfig) is not equivalent to the JSON fixture so the single assertion on defaultConfigs.size can miss divergences; update the test to either (A) replace tomlConfig with a TOML string that exactly mirrors EXAMPLE_JSON (same dimensions, overrides, and default configs) or (B) strengthen assertions to compare the full parsed structures (compare dimension keys and schemas, override counts and contents, and all defaultConfig entries) rather than only defaultConfigs.size so the test fails when TOML and JSON diverge.crates/superposition_core/tests/format_integration.rs (2)
185-228: Debug println should be removed or converted to test output.Line 211-218 has a
println!for debugging context priorities. Consider removing it or usingeprintln!/dbg!which only shows on test failure with--nocapture.♻️ Suggested fix
- // Print actual priorities for debugging - println!( - "Context priorities: {:?}", - config - .contexts - .iter() - .map(|c| c.priority) - .collect::<Vec<_>>() - ); + // Debug output visible only on test failure with --nocapture + dbg!(config + .contexts + .iter() + .map(|c| c.priority) + .collect::<Vec<_>>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/tests/format_integration.rs` around lines 185 - 228, Remove the debug println in the test_priority_calculation_json function: replace or remove the block that prints config.contexts priorities (the println! that maps c.priority from config.contexts) — either delete it entirely or switch to using dbg! / eprintln! so output only appears on failure (or when running tests with --nocapture); ensure the rest of the assertions referencing config and config.contexts remain unchanged.
266-292: Third instance of duplicated schema inference logic.This is the same
ConfigtoDetailedConfigconversion pattern used injson_tests.rsandtoml_tests.rs. Consider creating a shared test utility (e.g., in atest_utilsmodule or atests/common.rsfile) that provides this helper, reducing maintenance burden across three files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/tests/format_integration.rs` around lines 266 - 292, The test duplicates the same schema-inference logic when converting config.default_configs into a BTreeMap<String, DefaultConfigInfo> (the match over serde_json::Value variants producing schema JSON), which appears in this file and in json_tests.rs and toml_tests.rs; extract that logic into a shared test helper (e.g., tests::test_utils::infer_default_configs or a common function infer_schema_for_value) that accepts &BTreeMap<String, Value> or a (&str, &Value) pair and returns DefaultConfigInfo (or the BTreeMap), then replace the inline map+match block in this test (the code constructing DefaultConfigInfo { value: ..., schema }) with a call to the shared helper so all three tests reuse the same implementation and avoid duplication.examples/superposition_config_file_examples/src/main.rs (1)
70-246: Consider extracting shared evaluation logic.
run_toml_exampleandrun_json_exampleshare nearly identical evaluation scenarios (lines 102-154 and 191-243). While some duplication is acceptable in example code for clarity, you could extract the evaluation scenarios into a helper function that accepts the parsedConfigto reduce repetition:♻️ Optional refactor suggestion
fn run_scenarios( config: &superposition_core::Config, default_configs: &Map<String, Value>, format_name: &str, ) -> Result<(), Box<dyn std::error::Error>> { println!("\n--- Evaluating {} Configuration ---", format_name); // Scenario definitions could be shared let scenarios = [ ("Bike ride (no specific city)", vec![("vehicle_type", "bike")]), ("Cab ride in Bangalore", vec![("city", "Bangalore"), ("vehicle_type", "cab")]), // ... etc ]; for (desc, dims) in scenarios { let mut map = Map::new(); for (k, v) in dims { map.insert(k.to_string(), Value::String(v.to_string())); } evaluate_and_print(config, default_configs, desc, &map)?; } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/superposition_config_file_examples/src/main.rs` around lines 70 - 246, The two functions run_toml_example and run_json_example duplicate the same evaluation scenarios; extract the shared scenario setup and iteration into a helper (e.g., run_scenarios or evaluate_scenarios) that accepts &superposition_core::Config, &Map<String, Value>, and a label (format_name) and runs evaluate_and_print for each scenario; replace the duplicated blocks in run_toml_example and run_json_example with calls to that helper to centralize scenario definitions and reduce repetition while keeping parsing/display logic in the original functions.crates/superposition_core/src/format/tests/json_tests.rs (1)
11-52: Duplicated helper function across test modules.The
config_to_detailedhelper is identical to the one intoml_tests.rs. Consider extracting this to a shared test utilities module (e.g.,tests/common.rsor within thetestsmodule itself) to reduce duplication and ease maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/tests/json_tests.rs` around lines 11 - 52, The helper function config_to_detailed (which constructs DetailedConfig from Config using DefaultConfigInfo and DefaultConfigsWithSchema) is duplicated in multiple test files; extract this function into a shared test utilities module (e.g., create a tests/common.rs or a module under crate::tests::common) as a single reusable function with appropriate visibility (pub(crate) or pub(super)) and move any dependent type imports (Config, DetailedConfig, DefaultConfigInfo, DefaultConfigsWithSchema) into that module, then replace the duplicate implementations in json_tests.rs and toml_tests.rs with a use/import of the shared config_to_detailed helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/superposition_core/src/format/error.rs`:
- Around line 50-60: The Display message for the InvalidCohortDimensionPosition
variant is inverted: in the Display impl (format::error.rs) for
InvalidCohortDimensionPosition (used by validate_cohort_dimension_position) you
should change the wording so it tells the user that the cohort dimension must
come before the base dimension (i.e. cohort position should be less than the
base dimension position) instead of saying it "should be greater than"; update
the write! message accordingly to reflect the correct direction.
In `@crates/superposition_core/src/format/json_impl.rs`:
- Around line 97-121: The mapping currently swallows errors by using
unwrap_or_default() for overrides and condition serialization and substituting
empty maps; instead make the closure return Result<JsonContext,
SerializationError> and propagate failures so callers see serialization errors.
Replace detailed_config.overrides.get(override_key).cloned().unwrap_or_default()
with a presence check (e.g., .get(override_key).cloned().ok_or_else(||
SerializationError::MissingOverride(override_key.clone()))?), replace
serde_json::to_value(&ctx.condition).unwrap_or_default() with
serde_json::to_value(&ctx.condition).map_err(|e|
SerializationError::ConditionSerialization { source: e, context_key:
override_key.clone() })?, and if the Value is not an Object return an
appropriate SerializationError (e.g., InvalidConditionType). Then collect with
collect::<Result<Vec<JsonContext>, _>>()? and propagate the error out of the
surrounding function so invalid/missing context data surface as
SerializationError rather than producing {} silently.
- Around line 13-19: JsonConfig currently uses the internal DimensionInfo (which
includes dependency_graph and will be serialized), so replace the wire type with
a JSON-specific DTO: create a DimensionInfoJson struct containing only schema,
position, and dimension_type (mirroring DimensionInfoToml), update JsonConfig to
use Vec/HashMap of DimensionInfoJson instead of DimensionInfo, and add
conversion functions between DimensionInfo <-> DimensionInfoJson (like the TOML
conversion logic) so serialization no longer leaks dependency_graph; ensure
serde attributes match existing JSON field names and keep dependency_graph out
of serialized output.
In `@crates/superposition_core/src/format/toml_impl.rs`:
- Around line 39-53: The TOML emission paths in the TryFrom<DimensionInfo> for
DimensionInfoToml (fn try_from) and the corresponding block around lines 85-95
use conversion_error (ConversionError) but must return a serialization error for
the TOML emit path; replace the conversion_error(...) calls with the
serialization_error(...) variant (preserving the same "TOML" context and the
error text like "Schema contains incompatible values: {}") and ensure the Result
uses the existing FormatError alias that can represent SerializationError so the
failure surfaces as a SerializationError rather than a ConversionError.
In `@crates/superposition_core/src/lib.rs`:
- Around line 37-38: The current pub use alias (pub use format::FormatError as
TomlError) breaks callers expecting the old TomlError enum shape (e.g.,
TomlError::TomlSyntaxError or tuple variants); replace the alias with a
deprecated wrapper enum named TomlError that defines the original legacy
variants and implements From<format::FormatError> (and Into/From conversions as
needed) to map each FormatError variant into the corresponding TomlError variant
so existing pattern-matching and tuple-style variants continue to work while
converting from the new FormatError.
In `@crates/superposition_core/src/toml.rs`:
- Around line 1-4: The crate no longer re-exports ContextToml but
DetailedConfigToml.overrides still references it; re-export ContextToml from the
legacy module so callers using the old path compile. Add ContextToml to the
public re-exports alongside parse_toml_config, serialize_to_toml,
DetailedConfigToml, and DimensionInfoToml (i.e., re-export
crate::format::toml_impl::ContextToml) so DetailedConfigToml.overrides and
external callers find the type.
In `@crates/superposition_core/src/validations.rs`:
- Around line 675-721: The validation errors for override lookups currently
produce paths like "context[...]" which is wrong; update the error key/context
formatting inside the validate_overrides error mapping so it references the
overrides array key instead. In the closure passed to validate_overrides (around
validate_overrides(override_vals, default_configs) -> map_err), replace
occurrences that build FormatError::InvalidOverrideKey.context and
FormatError::ValidationError.key (currently using format!("context[{}]" ...) or
format!("context[{}].{}", index, ...)) to use the override key from override_key
(use its string from context.override_with_keys.get_key()) and produce paths
like "overrides[<override_key>]" and "overrides[<override_key>].<field>" and
similarly use format!("overrides[{}]", override_key) for the default case so the
reported paths match the input names expected by
ffi_parse_json_config/ffi_parse_toml_config.
In `@docs/plans/2025-02-15-unified-config-format.md`:
- Around line 1568-1708: The embedded markdown sample uses an outer
"```markdown" fence that contains inner triple-backtick fences, causing
rendering/lint failures; update the outer fence to a longer fence (e.g.,
"````markdown") or indent/escape the inner fences so they are not treated as
block delimiters—specifically edit the outer code block marker around the JSON
sample (the "```markdown" block that wraps the JSON and examples) to use a
quadruple/backtick fence or adjust the inner "```json" / "```rust" / "```kotlin"
fences so they don't terminate the outer block.
---
Nitpick comments:
In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt`:
- Around line 101-131: The TOML fixture in testTomlJsonEquivalence (variables
tomlConfig and EXAMPLE_JSON used with ffiParseTomlConfig and ffiParseJsonConfig)
is not equivalent to the JSON fixture so the single assertion on
defaultConfigs.size can miss divergences; update the test to either (A) replace
tomlConfig with a TOML string that exactly mirrors EXAMPLE_JSON (same
dimensions, overrides, and default configs) or (B) strengthen assertions to
compare the full parsed structures (compare dimension keys and schemas, override
counts and contents, and all defaultConfig entries) rather than only
defaultConfigs.size so the test fails when TOML and JSON diverge.
In `@crates/superposition_core/src/format/tests/json_tests.rs`:
- Around line 11-52: The helper function config_to_detailed (which constructs
DetailedConfig from Config using DefaultConfigInfo and DefaultConfigsWithSchema)
is duplicated in multiple test files; extract this function into a shared test
utilities module (e.g., create a tests/common.rs or a module under
crate::tests::common) as a single reusable function with appropriate visibility
(pub(crate) or pub(super)) and move any dependent type imports (Config,
DetailedConfig, DefaultConfigInfo, DefaultConfigsWithSchema) into that module,
then replace the duplicate implementations in json_tests.rs and toml_tests.rs
with a use/import of the shared config_to_detailed helper.
In `@crates/superposition_core/tests/format_integration.rs`:
- Around line 185-228: Remove the debug println in the
test_priority_calculation_json function: replace or remove the block that prints
config.contexts priorities (the println! that maps c.priority from
config.contexts) — either delete it entirely or switch to using dbg! / eprintln!
so output only appears on failure (or when running tests with --nocapture);
ensure the rest of the assertions referencing config and config.contexts remain
unchanged.
- Around line 266-292: The test duplicates the same schema-inference logic when
converting config.default_configs into a BTreeMap<String, DefaultConfigInfo>
(the match over serde_json::Value variants producing schema JSON), which appears
in this file and in json_tests.rs and toml_tests.rs; extract that logic into a
shared test helper (e.g., tests::test_utils::infer_default_configs or a common
function infer_schema_for_value) that accepts &BTreeMap<String, Value> or a
(&str, &Value) pair and returns DefaultConfigInfo (or the BTreeMap), then
replace the inline map+match block in this test (the code constructing
DefaultConfigInfo { value: ..., schema }) with a call to the shared helper so
all three tests reuse the same implementation and avoid duplication.
In `@examples/superposition_config_file_examples/src/main.rs`:
- Around line 70-246: The two functions run_toml_example and run_json_example
duplicate the same evaluation scenarios; extract the shared scenario setup and
iteration into a helper (e.g., run_scenarios or evaluate_scenarios) that accepts
&superposition_core::Config, &Map<String, Value>, and a label (format_name) and
runs evaluate_and_print for each scenario; replace the duplicated blocks in
run_toml_example and run_json_example with calls to that helper to centralize
scenario definitions and reduce repetition while keeping parsing/display logic
in the original functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 739ed750-4a25-4077-9391-de01eb81fab9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlclients/java/bindings/src/test/kotlin/JsonFunctionsTest.ktclients/javascript/bindings/README_TOML_TESTS.mdclients/javascript/bindings/test-toml.tsclients/python/bindings/README_TOML_TESTS.mdclients/python/bindings/test_toml_functions.pycrates/superposition_core/src/ffi.rscrates/superposition_core/src/format.rscrates/superposition_core/src/format/error.rscrates/superposition_core/src/format/json_impl.rscrates/superposition_core/src/format/tests.rscrates/superposition_core/src/format/tests/json_tests.rscrates/superposition_core/src/format/tests/toml_tests.rscrates/superposition_core/src/format/toml_helpers.rscrates/superposition_core/src/format/toml_impl.rscrates/superposition_core/src/lib.rscrates/superposition_core/src/toml.rscrates/superposition_core/src/toml/test.rscrates/superposition_core/src/validations.rscrates/superposition_core/tests/format_integration.rscrates/superposition_types/src/config.rsdocs/json-config-format.mddocs/plans/2025-02-15-unified-config-format.mdexamples/config.jsonexamples/superposition_config_file_examples/Cargo.tomlexamples/superposition_config_file_examples/README.mdexamples/superposition_config_file_examples/example.jsonexamples/superposition_config_file_examples/example.tomlexamples/superposition_config_file_examples/src/main.rsexamples/superposition_toml_example/Cargo.tomlexamples/superposition_toml_example/README.mdexamples/superposition_toml_example/src/main.rsnix/rust.nix
💤 Files with no reviewable changes (4)
- crates/superposition_core/src/toml/test.rs
- examples/superposition_toml_example/Cargo.toml
- examples/superposition_toml_example/README.md
- examples/superposition_toml_example/src/main.rs
Comment on lines
+50
to
+60
| Self::InvalidCohortDimensionPosition { | ||
| dimension, | ||
| dimension_position, | ||
| cohort_dimension, | ||
| cohort_dimension_position, | ||
| } => { | ||
| write!( | ||
| f, | ||
| "Validation error: Dimension {} position {} should be greater than cohort dimension {} position {}", | ||
| dimension, dimension_position, cohort_dimension, cohort_dimension_position | ||
| ) |
Contributor
There was a problem hiding this comment.
Fix the direction in the cohort-position message.
validate_cohort_dimension_position rejects configs where the cohort dimension is placed after its base dimension, but this message says the cohort dimension should be greater. That points users toward the wrong fix once the error is surfaced as text.
🛠️ Suggested wording
- "Validation error: Dimension {} position {} should be greater than cohort dimension {} position {}",
+ "Validation error: Cohort dimension {} position {} should be less than or equal to dimension {} position {}",
dimension, dimension_position, cohort_dimension, cohort_dimension_position📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Self::InvalidCohortDimensionPosition { | |
| dimension, | |
| dimension_position, | |
| cohort_dimension, | |
| cohort_dimension_position, | |
| } => { | |
| write!( | |
| f, | |
| "Validation error: Dimension {} position {} should be greater than cohort dimension {} position {}", | |
| dimension, dimension_position, cohort_dimension, cohort_dimension_position | |
| ) | |
| Self::InvalidCohortDimensionPosition { | |
| dimension, | |
| dimension_position, | |
| cohort_dimension, | |
| cohort_dimension_position, | |
| } => { | |
| write!( | |
| f, | |
| "Validation error: Cohort dimension {} position {} should be less than or equal to dimension {} position {}", | |
| dimension, dimension_position, cohort_dimension, cohort_dimension_position | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format/error.rs` around lines 50 - 60, The
Display message for the InvalidCohortDimensionPosition variant is inverted: in
the Display impl (format::error.rs) for InvalidCohortDimensionPosition (used by
validate_cohort_dimension_position) you should change the wording so it tells
the user that the cohort dimension must come before the base dimension (i.e.
cohort position should be less than the base dimension position) instead of
saying it "should be greater than"; update the write! message accordingly to
reflect the correct direction.
Collaborator
Author
There was a problem hiding this comment.
The error is meaningful and matches the current main implementation.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the cohort dimension position validation logic
rg -n "cohort" --type rust -C 5 crates/superposition_core/src/validations.rs | head -100
# Also search broadly for the validation function
rg -n "InvalidCohortDimensionPosition\|cohort_dimension_position\|validate_cohort" --type rust -C 5
Length of output: 3797
🏁 Script executed:
#!/bin/bash
# Get the full validate_cohort_dimension_position function body
rg -n "validate_cohort_dimension_position\|InvalidCohortDimensionPosition" --type rust -C 10
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all Rust files and search for cohort position validation
fd -e rs | xargs grep -n "InvalidCohortDimensionPosition\|validate_cohort_dimension_position\|granular_dimension\|coarse_dimension" 2>/dev/null | head -60
# Also check the validations.rs file directly around that area
sed -n '338,400p' crates/superposition_core/src/validations.rs
Length of output: 3405
🏁 Script executed:
#!/bin/bash
sed -n '600,660p' crates/superposition_core/src/validations.rs
Length of output: 2770
@knutties, thanks for the response. I took a closer look at the actual validation logic to be precise here. Here's what the code does in an error scenario:
Say city_cohort is at position 5 and city is at position 4:
// call site (dim = "city_cohort", dim_info.position = 5)
// cohort_dim = "city", cohort_dimension_info.position = 4
validate_cohort_dimension_position(cohort_dimension_info, dim_info)
// → granular_dimension_info = city (pos=4)
// → coarse_dimension_info = city_cohort (pos=5)
// → errors when 4 < 5 ✓ (error fires)
FormatError::InvalidCohortDimensionPosition {
dimension: "city_cohort", // dim_info
dimension_position: 5,
cohort_dimension: "city", // cohort_dimension_info
cohort_dimension_position: 4,
}The Display message the user sees is:
"Dimension city_cohort position 5 should be greater than cohort dimension city position 4"
But city_cohort is already at 5 which is greater than 4 — that's precisely what triggered the error. The message points the user toward the wrong fix. The correct guidance is:
"Dimension city_cohort position 5 should be less than cohort dimension city position 4"
(as confirmed by the example.json in the PR itself, where city_cohort = position 1 and city = position 4.)
If the same wording exists on main, it would be an existing bug that's also worth fixing. That said, if you'd prefer to track this separately and not block the PR, I'm happy to open a follow-up issue. Would you like me to do that?
🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
Comment on lines
+97
to
+121
| let contexts: Vec<JsonContext> = detailed_config | ||
| .contexts | ||
| .iter() | ||
| .map(|ctx| { | ||
| let override_key = ctx.override_with_keys.get_key(); | ||
| let overrides = detailed_config | ||
| .overrides | ||
| .get(override_key) | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
|
|
||
| // Serialize condition to JSON value, then convert to Map | ||
| let condition_value = | ||
| serde_json::to_value(&ctx.condition).unwrap_or_default(); | ||
| let context_map = match condition_value { | ||
| serde_json::Value::Object(map) => map, | ||
| _ => serde_json::Map::new(), | ||
| }; | ||
|
|
||
| JsonContext { | ||
| context: context_map, | ||
| overrides: overrides.into(), | ||
| } | ||
| }) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
Don't turn missing or invalid context data into {}.
This serializer currently drops missing override hashes and condition serialization failures via unwrap_or_default() / empty-map fallback, then still returns Ok(...). That silently emits a broken config instead of surfacing SerializationError.
🐛 Proposed fix
- let contexts: Vec<JsonContext> = detailed_config
+ let contexts: Vec<JsonContext> = detailed_config
.contexts
.iter()
- .map(|ctx| {
+ .map(|ctx| -> Result<JsonContext, FormatError> {
let override_key = ctx.override_with_keys.get_key();
let overrides = detailed_config
.overrides
.get(override_key)
.cloned()
- .unwrap_or_default();
+ .ok_or_else(|| {
+ crate::format::serialization_error(
+ "JSON",
+ format!(
+ "Missing overrides for context '{}'",
+ ctx.id
+ ),
+ )
+ })?;
// Serialize condition to JSON value, then convert to Map
- let condition_value =
- serde_json::to_value(&ctx.condition).unwrap_or_default();
+ let condition_value =
+ serde_json::to_value(&ctx.condition).map_err(|e| {
+ crate::format::serialization_error("JSON", e.to_string())
+ })?;
let context_map = match condition_value {
serde_json::Value::Object(map) => map,
- _ => serde_json::Map::new(),
+ _ => {
+ return Err(crate::format::serialization_error(
+ "JSON",
+ "Condition must serialize to a JSON object",
+ ));
+ }
};
- JsonContext {
+ Ok(JsonContext {
context: context_map,
overrides: overrides.into(),
- }
+ })
})
- .collect();
+ .collect::<Result<_, _>>()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format/json_impl.rs` around lines 97 - 121, The
mapping currently swallows errors by using unwrap_or_default() for overrides and
condition serialization and substituting empty maps; instead make the closure
return Result<JsonContext, SerializationError> and propagate failures so callers
see serialization errors. Replace
detailed_config.overrides.get(override_key).cloned().unwrap_or_default() with a
presence check (e.g., .get(override_key).cloned().ok_or_else(||
SerializationError::MissingOverride(override_key.clone()))?), replace
serde_json::to_value(&ctx.condition).unwrap_or_default() with
serde_json::to_value(&ctx.condition).map_err(|e|
SerializationError::ConditionSerialization { source: e, context_key:
override_key.clone() })?, and if the Value is not an Object return an
appropriate SerializationError (e.g., InvalidConditionType). Then collect with
collect::<Result<Vec<JsonContext>, _>>()? and propagate the error out of the
surrounding function so invalid/missing context data surface as
SerializationError rather than producing {} silently.
Comment on lines
+39
to
+53
| impl TryFrom<DimensionInfo> for DimensionInfoToml { | ||
| type Error = FormatError; | ||
| fn try_from(d: DimensionInfo) -> Result<Self, Self::Error> { | ||
| let schema = toml::Table::try_from(d.schema.into_inner()).map_err(|e| { | ||
| conversion_error( | ||
| "TOML", | ||
| format!("Schema contains incompatible values: {}", e), | ||
| ) | ||
| })?; | ||
| Ok(Self { | ||
| position: d.position, | ||
| schema, | ||
| dimension_type: d.dimension_type.to_string(), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Use SerializationError on the TOML emit path.
These branches only run while producing TOML output, but they currently return ConversionError. That makes the new error contract misleading, and the missing-override case collapses into a generic conversion failure.
♻️ Proposed fix
impl TryFrom<DimensionInfo> for DimensionInfoToml {
type Error = FormatError;
fn try_from(d: DimensionInfo) -> Result<Self, Self::Error> {
let schema = toml::Table::try_from(d.schema.into_inner()).map_err(|e| {
- conversion_error(
+ serialization_error(
"TOML",
format!("Schema contains incompatible values: {}", e),
)
})?;
Ok(Self {
@@
impl TryFrom<(Context, &HashMap<String, Overrides>)> for ContextToml {
type Error = FormatError;
fn try_from(
(context, overrides): (Context, &HashMap<String, Overrides>),
) -> Result<Self, Self::Error> {
let context_toml: toml::Table =
toml::Table::try_from(context.condition.deref().clone())
- .map_err(|e| conversion_error("TOML", e.to_string()))?;
- let overrides_toml: toml::Table =
- toml::Table::try_from(overrides.get(context.override_with_keys.get_key()))
- .map_err(|e| conversion_error("TOML", e.to_string()))?;
+ .map_err(|e| serialization_error("TOML", e.to_string()))?;
+ let override_value = overrides
+ .get(context.override_with_keys.get_key())
+ .ok_or_else(|| {
+ serialization_error(
+ "TOML",
+ format!("Missing overrides for context '{}'", context.id),
+ )
+ })?;
+ let overrides_toml: toml::Table =
+ toml::Table::try_from(override_value)
+ .map_err(|e| serialization_error("TOML", e.to_string()))?;
Ok(Self {
context: context_toml,
overrides: overrides_toml,
})Also applies to: 85-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format/toml_impl.rs` around lines 39 - 53, The
TOML emission paths in the TryFrom<DimensionInfo> for DimensionInfoToml (fn
try_from) and the corresponding block around lines 85-95 use conversion_error
(ConversionError) but must return a serialization error for the TOML emit path;
replace the conversion_error(...) calls with the serialization_error(...)
variant (preserving the same "TOML" context and the error text like "Schema
contains incompatible values: {}") and ensure the Result uses the existing
FormatError alias that can represent SerializationError so the failure surfaces
as a SerializationError rather than a ConversionError.
crates/superposition_core/src/lib.rs
Outdated
Comment on lines
+37
to
+38
| // Re-export TOML-specific error type for backward compatibility | ||
| pub use format::FormatError as TomlError; |
Contributor
There was a problem hiding this comment.
TomlError is no longer backward compatible.
This preserves the name only. Callers matching legacy TomlError::TomlSyntaxError or tuple-style variants will still break because FormatError has a different enum shape. If this release needs compatibility, keep a deprecated wrapper enum and convert from FormatError instead of aliasing it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/lib.rs` around lines 37 - 38, The current pub
use alias (pub use format::FormatError as TomlError) breaks callers expecting
the old TomlError enum shape (e.g., TomlError::TomlSyntaxError or tuple
variants); replace the alias with a deprecated wrapper enum named TomlError that
defines the original legacy variants and implements From<format::FormatError>
(and Into/From conversions as needed) to map each FormatError variant into the
corresponding TomlError variant so existing pattern-matching and tuple-style
variants continue to work while converting from the new FormatError.
Comment on lines
+675
to
+721
| validate_context(condition, dimensions).map_err(|errors| { | ||
| let first_error = &errors[0]; | ||
| match first_error { | ||
| ContextValidationError::UndeclaredDimension { dimension } => { | ||
| FormatError::UndeclaredDimension { | ||
| dimension: dimension.clone(), | ||
| context: format!("[{}]", index), | ||
| } | ||
| } | ||
| ContextValidationError::ValidationError { key, errors } => { | ||
| FormatError::ValidationError { | ||
| key: format!("context[{}]._context_.{}", index, key), | ||
| errors: format_validation_errors(errors), | ||
| } | ||
| } | ||
| _ => FormatError::ValidationError { | ||
| key: format!("context[{}]._context_", index), | ||
| errors: format!("{} validation errors", errors.len()), | ||
| }, | ||
| } | ||
| })?; | ||
| } | ||
|
|
||
| for (index, context) in detailed.contexts.iter().enumerate() { | ||
| let override_key = context.override_with_keys.get_key(); | ||
| if let Some(override_vals) = detailed.overrides.get(override_key) { | ||
| validate_overrides(override_vals, default_configs).map_err(|errors| { | ||
| let first_error = &errors[0]; | ||
| match first_error { | ||
| ContextValidationError::InvalidOverrideKey { key } => { | ||
| FormatError::InvalidOverrideKey { | ||
| key: key.clone(), | ||
| context: format!("[{}]", index), | ||
| } | ||
| } | ||
| ContextValidationError::ValidationError { key, errors } => { | ||
| FormatError::ValidationError { | ||
| key: format!("context[{}].{}", index, key), | ||
| errors: format_validation_errors(errors), | ||
| } | ||
| } | ||
| _ => FormatError::ValidationError { | ||
| key: format!("context[{}]", index), | ||
| errors: format!("{} validation errors", errors.len()), | ||
| }, | ||
| } | ||
| })?; |
Contributor
There was a problem hiding this comment.
Report overrides[...] in validation errors.
Both parsers expose this array as overrides, but these keys use context[...]. Since ffi_parse_json_config and ffi_parse_toml_config bubble up FormatError::to_string(), callers will be pointed at a path that doesn't exist in their input.
🛠️ Suggested fix
- context: format!("[{}]", index),
+ context: format!("overrides[{}]", index),
@@
- key: format!("context[{}]._context_.{}", index, key),
+ key: format!("overrides[{}]._context_.{}", index, key),
@@
- key: format!("context[{}]._context_", index),
+ key: format!("overrides[{}]._context_", index),
@@
- context: format!("[{}]", index),
+ context: format!("overrides[{}]", index),
@@
- key: format!("context[{}].{}", index, key),
+ key: format!("overrides[{}].{}", index, key),
@@
- key: format!("context[{}]", index),
+ key: format!("overrides[{}]", index),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/validations.rs` around lines 675 - 721, The
validation errors for override lookups currently produce paths like
"context[...]" which is wrong; update the error key/context formatting inside
the validate_overrides error mapping so it references the overrides array key
instead. In the closure passed to validate_overrides (around
validate_overrides(override_vals, default_configs) -> map_err), replace
occurrences that build FormatError::InvalidOverrideKey.context and
FormatError::ValidationError.key (currently using format!("context[{}]" ...) or
format!("context[{}].{}", index, ...)) to use the override key from override_key
(use its string from context.override_with_keys.get_key()) and produce paths
like "overrides[<override_key>]" and "overrides[<override_key>].<field>" and
similarly use format!("overrides[{}]", override_key) for the default case so the
reported paths match the input names expected by
ffi_parse_json_config/ffi_parse_toml_config.
Comment on lines
+1568
to
+1708
| ```markdown | ||
| # JSON Configuration Format | ||
|
|
||
| Superposition now supports JSON as an alternative to TOML for configuration files. | ||
|
|
||
| ## Format Overview | ||
|
|
||
| The JSON format uses the same structure as the internal data model: | ||
|
|
||
| ```json | ||
| { | ||
| "default-configs": { | ||
| "<config-key>": { | ||
| "value": <any>, | ||
| "schema": <json-schema> | ||
| } | ||
| }, | ||
| "dimensions": { | ||
| "<dimension-name>": { | ||
| "position": <integer>, | ||
| "schema": <json-schema>, | ||
| "type": "REGULAR|LOCAL_COHORT:<dim>|REMOTE_COHORT:<dim>" | ||
| } | ||
| }, | ||
| "contexts": [ | ||
| { | ||
| "_context_": { "<dimension>": <value> }, | ||
| "<config-key>": <override-value> | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| ## Example | ||
|
|
||
| ```json | ||
| { | ||
| "default-configs": { | ||
| "timeout": { | ||
| "value": 30, | ||
| "schema": { "type": "integer" } | ||
| }, | ||
| "database": { | ||
| "value": { "host": "localhost", "port": 5432 }, | ||
| "schema": { "type": "object" } | ||
| } | ||
| }, | ||
| "dimensions": { | ||
| "environment": { | ||
| "position": 1, | ||
| "schema": { "type": "string", "enum": ["dev", "staging", "prod"] } | ||
| }, | ||
| "region": { | ||
| "position": 2, | ||
| "schema": { "type": "string" } | ||
| } | ||
| }, | ||
| "contexts": [ | ||
| { | ||
| "_context_": { "environment": "prod" }, | ||
| "timeout": 60 | ||
| }, | ||
| { | ||
| "_context_": { "environment": "prod", "region": "us-east" }, | ||
| "timeout": 90, | ||
| "database": { "host": "prod.db.example.com", "port": 5432 } | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| ## Comparison with TOML | ||
|
|
||
| ### Advantages of JSON: | ||
| - Native support for all JSON types (objects, arrays, null) | ||
| - No need for special quoting rules | ||
| - Widely supported by tools and editors | ||
| - Easier to generate programmatically | ||
|
|
||
| ### Advantages of TOML: | ||
| - More human-readable for simple configs | ||
| - Supports comments | ||
| - Better for version control diffs | ||
| - Datetime type support | ||
|
|
||
| ## API Usage | ||
|
|
||
| ### Rust | ||
| ```rust | ||
| use superposition_core::{parse_json_config, parse_toml_config}; | ||
|
|
||
| // Parse JSON | ||
| let config = parse_json_config(json_string)?; | ||
|
|
||
| // Parse TOML | ||
| let config = parse_toml_config(toml_string)?; | ||
| ``` | ||
|
|
||
| ### Java/Kotlin (via FFI) | ||
| ```kotlin | ||
| import uniffi.superposition_client.* | ||
|
|
||
| // Parse JSON | ||
| val config = ffiParseJsonConfig(jsonString) | ||
|
|
||
| // Parse TOML | ||
| val config = ffiParseTomlConfig(tomlString) | ||
| ``` | ||
|
|
||
| ## Validation | ||
|
|
||
| Both formats use the same validation logic: | ||
|
|
||
| 1. **Default configs** - Values are validated against their schemas | ||
| 2. **Dimensions** - Schemas are validated, cohort references checked | ||
| 3. **Contexts** - All dimensions used must be declared | ||
| 4. **Overrides** - Keys must exist in default-configs | ||
| 5. **Positions** - No duplicate dimension positions allowed | ||
|
|
||
| ## Migration from TOML | ||
|
|
||
| To convert TOML to JSON: | ||
|
|
||
| 1. Parse the TOML file using existing tools | ||
| 2. Serialize to JSON (tools like `yq` or custom scripts) | ||
| 3. Validate with Superposition | ||
|
|
||
| Example using Python: | ||
| ```python | ||
| import toml | ||
| import json | ||
|
|
||
| with open('config.toml') as f: | ||
| data = toml.load(f) | ||
|
|
||
| with open('config.json', 'w') as f: | ||
| json.dump(data, f, indent=2) | ||
| ``` | ||
|
|
||
| Note: Manual review may be needed for complex TOML structures. | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix the nested code fence in the embedded markdown sample.
The outer ```markdown block contains inner triple-backtick fences, so the sample renders incorrectly and is already tripping markdownlint on Line 1708. Use a longer outer fence or indent the inner fences.
🛠️ Minimal fix
-```markdown
+````markdown
...
-```
+````🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1708-1708: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2025-02-15-unified-config-format.md` around lines 1568 - 1708, The
embedded markdown sample uses an outer "```markdown" fence that contains inner
triple-backtick fences, causing rendering/lint failures; update the outer fence
to a longer fence (e.g., "````markdown") or indent/escape the inner fences so
they are not treated as block delimiters—specifically edit the outer code block
marker around the JSON sample (the "```markdown" block that wraps the JSON and
examples) to use a quadruple/backtick fence or adjust the inner "```json" /
"```rust" / "```kotlin" fences so they don't terminate the outer block.
sauraww
force-pushed
the
other-config-formats
branch
from
b7d3b2a to
c0c5bec
Compare
Collaborator
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
docs/plans/2025-02-15-unified-config-format.md (1)
1568-1708:⚠️ Potential issue | 🟡 MinorUse a longer outer fence for the embedded markdown sample.
The outer
```markdownblock still contains inner triple-backtick fences, so the sample closes early and Line 1708 is parsed as a standalone fence instead of the end of the example.🛠️ Minimal fix
-```markdown +````markdown ... -``` +````🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2025-02-15-unified-config-format.md` around lines 1568 - 1708, The outer example block starts with ```markdown but contains inner triple-backtick fences which prematurely close it; update the outer fence to a longer delimiter (e.g., ````markdown) and close it with the matching four-backtick sequence so the inner ``` blocks remain intact—edit the outer fence that begins the JSON Configuration Format example and its closing fence at the end of the sample accordingly.crates/superposition_core/src/format/json_impl.rs (2)
12-18:⚠️ Potential issue | 🟠 MajorDon't use internal
DimensionInfoas the JSON wire type.
serialize_to_jsonfeeds validatedDimensionInfovalues back through serde, so internal enrichment state likedependency_graphcan leak into emitted JSON even though it is not part of the public config format. Mirror the TOML path with a JSON-specific DTO that only carries public fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/json_impl.rs` around lines 12 - 18, JsonConfig currently exposes the internal DimensionInfo type which can leak enriched fields (e.g., dependency_graph) into emitted JSON; create a JSON-specific DTO (e.g., JsonDimensionInfo) that only contains the public fields that mirror the TOML DTO, replace the dimensions field in JsonConfig from HashMap<String, DimensionInfo> to HashMap<String, JsonDimensionInfo>, add conversion logic in serialize_to_json (or the helper used there) to map internal DimensionInfo -> JsonDimensionInfo (omitting internal/enriched fields) and use that DTO for serde output, and likewise ensure any contexts/JsonContext mapping only includes public fields.
83-107:⚠️ Potential issue | 🟠 MajorSurface reconstruction failures instead of defaulting to empty objects.
Both fallbacks here turn malformed
DetailedConfiginput into{}and still returnOk(...). If a context points at a missing override entry, or if the condition does not serialize to an object, callers get corrupted JSON instead of aSerializationError.🛠️ Minimal fix
- let contexts: Vec<JsonContext> = detailed_config + let contexts: Vec<JsonContext> = detailed_config .contexts .iter() - .map(|ctx| { + .map(|ctx| -> Result<JsonContext, FormatError> { let override_key = ctx.override_with_keys.get_key(); let overrides = detailed_config .overrides .get(override_key) .cloned() - .unwrap_or_default(); + .ok_or_else(|| { + crate::format::serialization_error( + "JSON", + format!("Missing overrides for context '{}'", override_key), + ) + })?; - let condition_value = - serde_json::to_value(&ctx.condition).unwrap_or_default(); + let condition_value = serde_json::to_value(&ctx.condition) + .map_err(|e| crate::format::serialization_error("JSON", e.to_string()))?; let context_map = match condition_value { serde_json::Value::Object(map) => map, - _ => serde_json::Map::new(), + _ => { + return Err(crate::format::serialization_error( + "JSON", + "Condition must serialize to a JSON object", + )); + } }; - JsonContext { + Ok(JsonContext { context: context_map, overrides: overrides.into(), - } + }) }) - .collect(); + .collect::<Result<_, _>>()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/json_impl.rs` around lines 83 - 107, The current mapping silently defaults missing overrides and non-object condition serializations to empty values; update the iterator in the code that builds Vec<JsonContext> (the closure over detailed_config.contexts and the JsonContext construction) to return a Result and surface errors instead of defaulting: when looking up overrides via detailed_config.overrides.get(override_key) treat None as an error and propagate a SerializationError (or map to the function's existing error type), and after serde_json::to_value(&ctx.condition) check that the Value is an Object and if not return a SerializationError rather than using serde_json::Map::new(); adjust the surrounding function to collect Results (e.g., collect::<Result<Vec<_>, _>>()) and propagate the error up.crates/superposition_core/src/validations.rs (1)
675-721:⚠️ Potential issue | 🟡 MinorReport validation errors under
overrides[...]paths.These messages bubble out of both parsers, but
context[...]and bare[index]are not valid paths in either input format. Point them at theoverridesarray so callers can find the failing entry.🛠️ Minimal fix
- context: format!("[{}]", index), + context: format!("overrides[{}]", index), @@ - key: format!("context[{}]._context_.{}", index, key), + key: format!("overrides[{}]._context_.{}", index, key), @@ - key: format!("context[{}]._context_", index), + key: format!("overrides[{}]._context_", index), @@ - context: format!("[{}]", index), + context: format!("overrides[{}]", index), @@ - key: format!("context[{}].{}", index, key), + key: format!("overrides[{}].{}", index, key), @@ - key: format!("context[{}]", index), + key: format!("overrides[{}]", index),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/validations.rs` around lines 675 - 721, The current error paths for override validation point to "context[<index>]" or bare indices, which are invalid; change them to report under the overrides map using the override key. In the validate_overrides error mapping (the block using override_key, detailed.overrides, and validate_overrides), replace any FormatError path that uses format!("context[{}]", index) or format!("context[{}].{}", index, key) or format!("context[{}]", index) with overrides-based paths using the override_key (e.g., format!("overrides[{}]", override_key) and format!("overrides[{}].{}", override_key, key)) so InvalidOverrideKey, ValidationError, and fallback errors point to the correct overrides[...] location.crates/superposition_core/src/format/toml_impl.rs (1)
85-101:⚠️ Potential issue | 🟡 MinorPotential panic if override key is missing.
Line 94 calls
overrides.get(context.override_with_keys.get_key())and passes theOptiondirectly totoml::Table::try_from. If the key doesn't exist in the map, this could produce unexpected behavior or a confusing error message.🛠️ Proposed fix
let context_toml: toml::Table = toml::Table::try_from(context.condition.deref().clone()) - .map_err(|e| conversion_error("TOML", e.to_string()))?; - let overrides_toml: toml::Table = - toml::Table::try_from(overrides.get(context.override_with_keys.get_key())) - .map_err(|e| conversion_error("TOML", e.to_string()))?; + .map_err(|e| serialization_error("TOML", e.to_string()))?; + let override_key = context.override_with_keys.get_key(); + let override_value = overrides.get(override_key).ok_or_else(|| { + serialization_error( + "TOML", + format!("Missing overrides for context '{}'", context.id), + ) + })?; + let overrides_toml: toml::Table = toml::Table::try_from(override_value) + .map_err(|e| serialization_error("TOML", e.to_string()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/toml_impl.rs` around lines 85 - 101, The conversion impl for TryFrom<(Context, &HashMap<String, Overrides>)> for ContextToml can panic or produce unclear errors because it passes an Option from overrides.get(context.override_with_keys.get_key()) directly into toml::Table::try_from; update the TryFrom block (ContextToml) to explicitly handle the missing key by checking overrides.get(context.override_with_keys.get_key()) first, return a clear FormatError (via conversion_error or similar) when the key is None, and only call toml::Table::try_from on the Some(&Overrides) value so the error message clearly indicates a missing override key instead of delegating to toml conversion errors.
🧹 Nitpick comments (4)
examples/superposition_config_file_examples/Cargo.toml (1)
1-9: Consider aligning edition with workspace.The example crate uses
edition = "2021"while the workspace definespackage.edition = "2024"in the rootCargo.toml. Consider usingedition.workspace = trueto inherit the workspace edition for consistency, unless there's a specific reason to use an older edition.♻️ Suggested change
[package] name = "superposition_config_file_examples" description = "Examples demonstrating TOML and JSON parsing with superposition_core" version = "0.1.0" -edition = "2021" +edition.workspace = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/superposition_config_file_examples/Cargo.toml` around lines 1 - 9, The crate's Cargo.toml sets edition = "2021" which diverges from the workspace edition; change the Cargo manifest to inherit the workspace edition by replacing the explicit edition with edition.workspace = true (i.e., update the edition field in the package table of the example crate) so the example uses the workspace's package.edition value.crates/superposition_core/src/format/tests/json_tests.rs (2)
10-52: This round-trip test is lossy before serialization.
config_to_detailed()rebuilds schemas from values, so metadata likeminimum,enum, and nested object properties is discarded beforeserialize_to_json()runs. The assertions then only compare counts for dimensions/contexts and skip overrides entirely. That can let serializer regressions pass unnoticed; use a fixedDetailedConfigfixture and compare the full reparsed structure.Also applies to: 71-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/tests/json_tests.rs` around lines 10 - 52, The test helper config_to_detailed is losing schema metadata by reconstructing schemas from values (dropping minimum/enum/object properties), so replace the round-trip approach with a fixed DetailedConfig test fixture (populate DetailedConfig, DefaultConfigInfo and DefaultConfigsWithSchema explicitly with expected schema metadata) and update the tests that call config_to_detailed (including the other occurrence around lines 71-91) to use that fixture; then assert the full reparsed structure from serialize_to_json()/deserialize (not just counts) and include overrides in the comparisons to catch serializer regressions.
93-146: Prefer matchingFormatErrorvariants over string checks.These tests already import
FormatError, so they can assertSyntaxError,UndeclaredDimension, andInvalidOverrideKeydirectly instead of relying on"JSON"or"not found"substrings. That will stay stable if display text changes.Example of the stricter assertion style
- assert!(result.unwrap_err().to_string().contains("not found")); + match result.unwrap_err() { + FormatError::InvalidOverrideKey { key, .. } => { + assert_eq!(key, "invalid_key"); + } + other => panic!("expected InvalidOverrideKey, got {:?}", other), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/tests/json_tests.rs` around lines 93 - 146, Update the three tests (test_json_invalid_syntax, test_json_undeclared_dimension, test_json_invalid_override_key) to assert against specific FormatError variants instead of string substrings: call parse_json_config and pattern-match the Result to Err(FormatError::SyntaxError(_)) for test_json_invalid_syntax, Err(FormatError::UndeclaredDimension(_)) for test_json_undeclared_dimension, and Err(FormatError::InvalidOverrideKey(_)) for test_json_invalid_override_key (using parse_json_config, Config, and FormatError from the diff), ensuring the tests use direct variant matching or matches!(...) rather than contains("JSON") or contains("not found").crates/superposition_core/src/format/error.rs (1)
103-103: Consider adding#[non_exhaustive]to the error enum.Since
FormatErroris a public API, adding#[non_exhaustive]would allow future variants to be added without breaking downstream code that pattern-matches on it.♻️ Proposed change
/// Unified error type for all configuration formats +#[non_exhaustive] #[derive(Debug, Clone, Serialize, Deserialize)] pub enum FormatError {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/error.rs` at line 103, The public error enum FormatError should be annotated with #[non_exhaustive] to allow adding variants in the future without breaking downstream pattern matches; locate the enum declaration for FormatError in this file and add the #[non_exhaustive] attribute above it, then run cargo test/build to ensure no internal code relies on exhaustive matching (update any local matches to use a wildcard arm if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt`:
- Around line 101-130: The test testTomlJsonEquivalence is not validating full
equivalence because tomlConfig differs from EXAMPLE_JSON and the test only
asserts defaultConfigs.size; update the test to either (A) replace tomlConfig
with a TOML fixture that exactly mirrors EXAMPLE_JSON (so
ffiParseTomlConfig(tomlConfig) should produce the same structure as
ffiParseJsonConfig(EXAMPLE_JSON)), or (B) add stronger assertions after parsing
to compare the parsed structures from ffiParseTomlConfig and ffiParseJsonConfig
(e.g., compare sizes of defaultConfigs, dimensions.size, overrides.size and key
fields or perform a deep equality check on the parsed config objects) so
testTomlJsonEquivalence actually verifies TOML/JSON equivalence.
In `@docs/json-config-format.md`:
- Around line 105-117: The example uses parse_json_config(...) to produce config
but then calls serialize_to_json(detailed_config) which is undefined and a
mismatched type; fix by either passing the parsed value (call
serialize_to_json(config)) or explicitly convert config to the DetailedConfig
shape before serializing (show the conversion step and then call
serialize_to_json(detailed_config)). Update the snippet to use the actual symbol
names parse_json_config, config, DetailedConfig (if conversion is needed), and
serialize_to_json so the round-trip compiles.
- Around line 213-223: Update the documentation paragraph in
json-config-format.md to remove or correct the stale claim that JSON uses the
same shared validation function; replace the reference to
validate_detailed_config() with the current validation path/function used by the
JSON implementation (or state that validation is implemented differently), and
fix the broken example link by changing [Example
Configuration](examples/config.json) to the correct relative path from docs
(e.g., ../examples/config.json or the actual location). Also adjust the bullet
that mentions sharing validation logic so it accurately reflects whether
ConfigFormat or the JSON parser (ConfigFormat impl) reuses TOML validation or
not.
In `@examples/superposition_config_file_examples/README.md`:
- Around line 26-77: Update the README snippets so they match the shipped
config: either add the vehicle_type dimension to the JSON `dimensions` block
(mirroring the TOML `vehicle_type` definition) before using `"vehicle_type"` in
`_context_`, or remove `"vehicle_type"` from the `_context_` override in the
JSON example; also reconcile the Chennai example text with the actual shipped
override (`city_cohort = south`) that affects `per_km_rate` by updating the
"Chennai ride uses default values" sentence to reflect that Chennai is covered
by the `city_cohort` override (or modify the shipped `example.json` to not
include the `city_cohort` override if you intend Chennai to use defaults).
Ensure references to `per_km_rate`, `_context_`, `dimensions`, and `city_cohort`
are consistent between README snippets and the example files.
---
Duplicate comments:
In `@crates/superposition_core/src/format/json_impl.rs`:
- Around line 12-18: JsonConfig currently exposes the internal DimensionInfo
type which can leak enriched fields (e.g., dependency_graph) into emitted JSON;
create a JSON-specific DTO (e.g., JsonDimensionInfo) that only contains the
public fields that mirror the TOML DTO, replace the dimensions field in
JsonConfig from HashMap<String, DimensionInfo> to HashMap<String,
JsonDimensionInfo>, add conversion logic in serialize_to_json (or the helper
used there) to map internal DimensionInfo -> JsonDimensionInfo (omitting
internal/enriched fields) and use that DTO for serde output, and likewise ensure
any contexts/JsonContext mapping only includes public fields.
- Around line 83-107: The current mapping silently defaults missing overrides
and non-object condition serializations to empty values; update the iterator in
the code that builds Vec<JsonContext> (the closure over detailed_config.contexts
and the JsonContext construction) to return a Result and surface errors instead
of defaulting: when looking up overrides via
detailed_config.overrides.get(override_key) treat None as an error and propagate
a SerializationError (or map to the function's existing error type), and after
serde_json::to_value(&ctx.condition) check that the Value is an Object and if
not return a SerializationError rather than using serde_json::Map::new(); adjust
the surrounding function to collect Results (e.g., collect::<Result<Vec<_>,
_>>()) and propagate the error up.
In `@crates/superposition_core/src/format/toml_impl.rs`:
- Around line 85-101: The conversion impl for TryFrom<(Context, &HashMap<String,
Overrides>)> for ContextToml can panic or produce unclear errors because it
passes an Option from overrides.get(context.override_with_keys.get_key())
directly into toml::Table::try_from; update the TryFrom block (ContextToml) to
explicitly handle the missing key by checking
overrides.get(context.override_with_keys.get_key()) first, return a clear
FormatError (via conversion_error or similar) when the key is None, and only
call toml::Table::try_from on the Some(&Overrides) value so the error message
clearly indicates a missing override key instead of delegating to toml
conversion errors.
In `@crates/superposition_core/src/validations.rs`:
- Around line 675-721: The current error paths for override validation point to
"context[<index>]" or bare indices, which are invalid; change them to report
under the overrides map using the override key. In the validate_overrides error
mapping (the block using override_key, detailed.overrides, and
validate_overrides), replace any FormatError path that uses
format!("context[{}]", index) or format!("context[{}].{}", index, key) or
format!("context[{}]", index) with overrides-based paths using the override_key
(e.g., format!("overrides[{}]", override_key) and format!("overrides[{}].{}",
override_key, key)) so InvalidOverrideKey, ValidationError, and fallback errors
point to the correct overrides[...] location.
In `@docs/plans/2025-02-15-unified-config-format.md`:
- Around line 1568-1708: The outer example block starts with ```markdown but
contains inner triple-backtick fences which prematurely close it; update the
outer fence to a longer delimiter (e.g., ````markdown) and close it with the
matching four-backtick sequence so the inner ``` blocks remain intact—edit the
outer fence that begins the JSON Configuration Format example and its closing
fence at the end of the sample accordingly.
---
Nitpick comments:
In `@crates/superposition_core/src/format/error.rs`:
- Line 103: The public error enum FormatError should be annotated with
#[non_exhaustive] to allow adding variants in the future without breaking
downstream pattern matches; locate the enum declaration for FormatError in this
file and add the #[non_exhaustive] attribute above it, then run cargo test/build
to ensure no internal code relies on exhaustive matching (update any local
matches to use a wildcard arm if needed).
In `@crates/superposition_core/src/format/tests/json_tests.rs`:
- Around line 10-52: The test helper config_to_detailed is losing schema
metadata by reconstructing schemas from values (dropping minimum/enum/object
properties), so replace the round-trip approach with a fixed DetailedConfig test
fixture (populate DetailedConfig, DefaultConfigInfo and DefaultConfigsWithSchema
explicitly with expected schema metadata) and update the tests that call
config_to_detailed (including the other occurrence around lines 71-91) to use
that fixture; then assert the full reparsed structure from
serialize_to_json()/deserialize (not just counts) and include overrides in the
comparisons to catch serializer regressions.
- Around line 93-146: Update the three tests (test_json_invalid_syntax,
test_json_undeclared_dimension, test_json_invalid_override_key) to assert
against specific FormatError variants instead of string substrings: call
parse_json_config and pattern-match the Result to
Err(FormatError::SyntaxError(_)) for test_json_invalid_syntax,
Err(FormatError::UndeclaredDimension(_)) for test_json_undeclared_dimension, and
Err(FormatError::InvalidOverrideKey(_)) for test_json_invalid_override_key
(using parse_json_config, Config, and FormatError from the diff), ensuring the
tests use direct variant matching or matches!(...) rather than contains("JSON")
or contains("not found").
In `@examples/superposition_config_file_examples/Cargo.toml`:
- Around line 1-9: The crate's Cargo.toml sets edition = "2021" which diverges
from the workspace edition; change the Cargo manifest to inherit the workspace
edition by replacing the explicit edition with edition.workspace = true (i.e.,
update the edition field in the package table of the example crate) so the
example uses the workspace's package.edition value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91cfec23-581c-43c2-83c3-cbcd4f93e850
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlclients/java/bindings/src/test/kotlin/JsonFunctionsTest.ktclients/javascript/bindings/README_TOML_TESTS.mdclients/javascript/bindings/test-toml.tsclients/python/bindings/README_TOML_TESTS.mdclients/python/bindings/test_toml_functions.pycrates/superposition_core/src/ffi.rscrates/superposition_core/src/format.rscrates/superposition_core/src/format/error.rscrates/superposition_core/src/format/json_impl.rscrates/superposition_core/src/format/tests.rscrates/superposition_core/src/format/tests/json_tests.rscrates/superposition_core/src/format/tests/toml_tests.rscrates/superposition_core/src/format/toml_helpers.rscrates/superposition_core/src/format/toml_impl.rscrates/superposition_core/src/lib.rscrates/superposition_core/src/toml.rscrates/superposition_core/src/toml/test.rscrates/superposition_core/src/validations.rscrates/superposition_core/tests/format_integration.rscrates/superposition_types/src/config.rsdocs/json-config-format.mddocs/plans/2025-02-15-unified-config-format.mdexamples/config.jsonexamples/superposition_config_file_examples/Cargo.tomlexamples/superposition_config_file_examples/README.mdexamples/superposition_config_file_examples/example.jsonexamples/superposition_config_file_examples/example.tomlexamples/superposition_config_file_examples/src/main.rsexamples/superposition_toml_example/Cargo.tomlexamples/superposition_toml_example/README.mdexamples/superposition_toml_example/src/main.rsnix/rust.nix
💤 Files with no reviewable changes (4)
- examples/superposition_toml_example/src/main.rs
- examples/superposition_toml_example/README.md
- examples/superposition_toml_example/Cargo.toml
- crates/superposition_core/src/toml/test.rs
Comment on lines
+101
to
+130
| fun testTomlJsonEquivalence() { | ||
| println("\n" + "=".repeat(70)) | ||
| println(" TEST: TOML/JSON Equivalence") | ||
| println("=".repeat(70)) | ||
|
|
||
| // Same configuration in TOML format | ||
| val tomlConfig = """ | ||
| [default-configs] | ||
| per_km_rate = { value = 20.0, schema = { type = "number" } } | ||
| surge_factor = { value = 0.0, schema = { type = "number" } } | ||
|
|
||
| [dimensions] | ||
| city = { position = 1, schema = { type = "string", enum = ["Bangalore", "Delhi"] } } | ||
| vehicle_type = { position = 2, schema = { type = "string", enum = ["auto", "cab", "bike"] } } | ||
|
|
||
| [[overrides]] | ||
| _context_ = { vehicle_type = "cab" } | ||
| per_km_rate = 25.0 | ||
|
|
||
| [[overrides]] | ||
| _context_ = { vehicle_type = "bike" } | ||
| per_km_rate = 15.0 | ||
| """ | ||
|
|
||
| val tomlResult = ffiParseTomlConfig(tomlConfig) | ||
| val jsonResult = ffiParseJsonConfig(EXAMPLE_JSON) | ||
|
|
||
| // Both should parse successfully with same structure | ||
| assertEquals(tomlResult.defaultConfigs.size, jsonResult.defaultConfigs.size) | ||
| println("\n✓ Both TOML and JSON produce equivalent configs") |
Contributor
There was a problem hiding this comment.
This test is not exercising TOML/JSON equivalence.
The TOML fixture here has 2 dimensions and 2 overrides, while EXAMPLE_JSON has 3 dimensions and 5 overrides. Since the assertion only checks defaultConfigs.size, this can pass without proving the two formats produce equivalent configs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt` around lines 101
- 130, The test testTomlJsonEquivalence is not validating full equivalence
because tomlConfig differs from EXAMPLE_JSON and the test only asserts
defaultConfigs.size; update the test to either (A) replace tomlConfig with a
TOML fixture that exactly mirrors EXAMPLE_JSON (so
ffiParseTomlConfig(tomlConfig) should produce the same structure as
ffiParseJsonConfig(EXAMPLE_JSON)), or (B) add stronger assertions after parsing
to compare the parsed structures from ffiParseTomlConfig and ffiParseJsonConfig
(e.g., compare sizes of defaultConfigs, dimensions.size, overrides.size and key
fields or perform a deep equality check on the parsed config objects) so
testTomlJsonEquivalence actually verifies TOML/JSON equivalence.
docs/json-config-format.md
Outdated
Comment on lines
+105
to
+117
| ```rust | ||
| use superposition_core::{parse_json_config, serialize_to_json}; | ||
|
|
||
| // Parse JSON configuration | ||
| let config = parse_json_config(json_string)?; | ||
|
|
||
| // Access parsed configuration | ||
| println!("Default configs: {:?}", config.default_configs); | ||
| println!("Dimensions: {:?}", config.dimensions); | ||
| println!("Contexts: {:?}", config.contexts); | ||
|
|
||
| // Serialize back to JSON | ||
| let json_output = serialize_to_json(detailed_config)?; |
Contributor
There was a problem hiding this comment.
The Rust round-trip snippet does not compile as written.
parse_json_config() returns config, but the example later calls serialize_to_json(detailed_config) with an undefined variable and a different type shape. Please either show the DetailedConfig conversion explicitly or use the API that actually round-trips the parsed value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/json-config-format.md` around lines 105 - 117, The example uses
parse_json_config(...) to produce config but then calls
serialize_to_json(detailed_config) which is undefined and a mismatched type; fix
by either passing the parsed value (call serialize_to_json(config)) or
explicitly convert config to the DetailedConfig shape before serializing (show
the conversion step and then call serialize_to_json(detailed_config)). Update
the snippet to use the actual symbol names parse_json_config, config,
DetailedConfig (if conversion is needed), and serialize_to_json so the
round-trip compiles.
docs/json-config-format.md
Outdated
Comment on lines
+213
to
+223
| The JSON implementation: | ||
| - Uses `serde_json` for parsing/serialization | ||
| - Implements the `ConfigFormat` trait for consistency with TOML | ||
| - Shares validation logic via `validate_detailed_config()` | ||
| - Sorts contexts by priority after parsing (same as TOML) | ||
| - Supports all superposition features (cohorts, overrides, etc.) | ||
|
|
||
| ## See Also | ||
|
|
||
| - [TOML Format Documentation](toml-config-format.md) | ||
| - [Example Configuration](examples/config.json) |
Contributor
There was a problem hiding this comment.
The implementation note and example link are already stale.
validate_detailed_config() no longer matches the shared validation path described in this PR, and [Example Configuration](examples/config.json) is the wrong relative path from docs/, so the link will resolve incorrectly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/json-config-format.md` around lines 213 - 223, Update the documentation
paragraph in json-config-format.md to remove or correct the stale claim that
JSON uses the same shared validation function; replace the reference to
validate_detailed_config() with the current validation path/function used by the
JSON implementation (or state that validation is implemented differently), and
fix the broken example link by changing [Example
Configuration](examples/config.json) to the correct relative path from docs
(e.g., ../examples/config.json or the actual location). Also adjust the bullet
that mentions sharing validation logic so it accurately reflects whether
ConfigFormat or the JSON parser (ConfigFormat impl) reuses TOML validation or
not.
7448b18 to
ce1a031
Compare
ayushjain17
requested changes
Mar 16, 2026
ayushjain17
reviewed
Mar 16, 2026
| } | ||
|
|
||
| #[test] | ||
| fn test_toml_round_trip_simple() { |
Collaborator
There was a problem hiding this comment.
in the previous deleted test.rs file, the test coverage seemed much wider
sauraww
force-pushed
the
other-config-formats
branch
3 times, most recently
from
542f69d to
c983ad0
Compare
Datron
approved these changes
Mar 17, 2026
examples/config.json
Outdated
| @@ -0,0 +1,57 @@ | |||
| { | |||
Collaborator
There was a problem hiding this comment.
this file is outside the example folder
sauraww
force-pushed
the
other-config-formats
branch
3 times, most recently
from
cbf2393 to
93d7c16
Compare
Collaborator
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
sauraww
force-pushed
the
other-config-formats
branch
from
93d7c16 to
8d5437c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/superposition_types/src/config.rs (1)
360-366:⚠️ Potential issue | 🟠 MajorAdd
#[serde(default)]todependency_graphfield to maintain backward compatibility with cached payloads.
DimensionInfonow includes a requireddependency_graphfield without a serde default. Any previously cachedConfigvalue in Redis that omits this field will fail deserialization whenread_through_cache::<Config>()is called, causing request failures during rollout. The field should have#[serde(default)]to safely deserialize old cache entries with an empty dependency graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/config.rs` around lines 360 - 366, The dependency_graph field on the DimensionInfo struct is required but lacks a serde default, causing older cached Config payloads to fail deserialization; add the #[serde(default)] attribute to the dependency_graph field in DimensionInfo and ensure DependencyGraph implements Default (or provide a serde default function) so read_through_cache::<Config>() can deserialize older cache entries into an empty/zeroed dependency graph.crates/superposition_core/src/ffi_legacy.rs (1)
516-574:⚠️ Potential issue | 🟠 MajorAdd
core_parse_json_configto the public C header.
core_parse_json_configis exported fromcrates/superposition_core/src/lib.rsand defined incrates/superposition_core/src/ffi_legacy.rs, but the public C headerheaders/libcac_client.hdoes not declare it. C consumers cannot use this function without the header declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/ffi_legacy.rs` around lines 516 - 574, Missing C header declaration for the exported FFI function core_parse_json_config prevents C consumers from linking to it; add a corresponding prototype to the public header (headers/libcac_client.h) matching the exported symbol (extern "C" signature): declare core_parse_json_config with C types for the parameters (const char* json_content, char* ebuf) and returning char* (or NULL on error), ensure it is inside the header guards and extern "C" block so the C++ consumers get C linkage, and keep the prototype consistent with the Rust definition in ffi_legacy.rs to avoid mismatches.
♻️ Duplicate comments (4)
examples/superposition_config_file_examples/README.md (1)
32-32:⚠️ Potential issue | 🟡 MinorScenario description conflicts with the shipped example config.
Line 32 says Chennai uses defaults, but the example config applies a
city_cohort = southoverride that changesper_km_rate. Please align the scenario text with actual behavior.📝 Suggested README fix
-5. **Chennai ride** - Uses default values (city not in overrides) +5. **Chennai ride** - Uses `city_cohort = south` override (`per_km_rate = 100.0`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/superposition_config_file_examples/README.md` at line 32, Update the scenario description for "Chennai ride" to match the shipped example config by noting that Chennai is covered by the override (city_cohort = south) and therefore does not use the default per_km_rate; explicitly state the applied override and the resulting per_km_rate value so the README text aligns with the example config.clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt (1)
101-130:⚠️ Potential issue | 🟡 MinorThis still is not proving TOML/JSON equivalence.
The TOML fixture here omits
hour_of_dayand three overrides fromEXAMPLE_JSON, and the assertion only comparesdefaultConfigs.size. This can pass even when the parsed configs differ materially, so it won't catch cross-format regressions. Either mirrorEXAMPLE_JSONin TOML or compare the parsed structures much more deeply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt` around lines 101 - 130, The test testTomlJsonEquivalence only compares defaultConfigs.size and uses a TOML fixture that omits hour_of_day and three overrides from EXAMPLE_JSON, so it doesn't prove equivalence; fix by either making tomlConfig match EXAMPLE_JSON exactly (include hour_of_day and the three override blocks) or perform a deep comparison of the parsed outputs from ffiParseTomlConfig and ffiParseJsonConfig (compare defaultConfigs contents, dimensions entries and their schemas/positions, overrides count and each override's _context_ and values) to ensure structural parity rather than a single-size assertion.crates/superposition_core/src/format/toml.rs (1)
38-46:⚠️ Potential issue | 🟡 MinorReport TOML emit failures as serialization errors.
These branches only run while producing TOML, but they still emit
ConversionError. The context branch also collapses a missing override entry into a generictry_fromfailure. ReturningSerializationErrorand checkingget(...)explicitly makes the new error contract accurate and easier to debug.💡 Proposed fix
impl TryFrom<DimensionInfo> for DimensionInfoToml { type Error = FormatError; fn try_from(d: DimensionInfo) -> Result<Self, Self::Error> { let schema = toml::Table::try_from(d.schema.into_inner()).map_err(|e| { - TomlFormat::conversion_error(format!( + TomlFormat::serialization_error(format!( "Schema contains incompatible values: {}", e )) })?; @@ impl TryFrom<(Context, &HashMap<String, Overrides>)> for ContextToml { type Error = FormatError; fn try_from( (context, overrides): (Context, &HashMap<String, Overrides>), ) -> Result<Self, Self::Error> { let context_toml: toml::Table = toml::Table::try_from(context.condition.deref().clone()) - .map_err(|e| TomlFormat::conversion_error(e.to_string()))?; - let overrides_toml: toml::Table = - toml::Table::try_from(overrides.get(context.override_with_keys.get_key())) - .map_err(|e| TomlFormat::conversion_error(e.to_string()))?; + .map_err(|e| TomlFormat::serialization_error(e.to_string()))?; + let override_value = overrides + .get(context.override_with_keys.get_key()) + .ok_or_else(|| { + TomlFormat::serialization_error(format!( + "Missing overrides for key '{}'", + context.override_with_keys.get_key() + )) + })?; + let overrides_toml: toml::Table = + toml::Table::try_from(override_value) + .map_err(|e| TomlFormat::serialization_error(e.to_string()))?; Ok(Self { context: context_toml, overrides: overrides_toml, })Also applies to: 84-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/format/toml.rs` around lines 38 - 46, The TOML conversion currently maps TOML emit/lookup failures to TomlFormat::conversion_error (ConversionError) inside the TryFrom<DimensionInfo> for DimensionInfoToml implementation; change these error returns to the appropriate SerializationError via the FormatError type (use TomlFormat::serialization_error or the SerializationError variant) when producing TOML and when an expected override key is missing, and replace the collapsed access with an explicit table.get("override") check (and return a SerializationError if absent or of the wrong type). Apply the same change to the analogous branch around the other block (lines ~84-95) so both schema conversion and override lookup emit SerializationError with clear context instead of ConversionError.crates/superposition_core/src/lib.rs (1)
7-29:⚠️ Potential issue | 🟠 MajorThis drops the old top-level TOML API.
Callers importing
superposition_core::{parse_toml_config, serialize_to_toml, TomlError}will stop compiling against this crate surface. If backward compatibility is still part of this migration, please keep deprecated crate-root shims for at least one release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/src/lib.rs` around lines 7 - 29, The crate root currently removed the old top-level TOML API; restore backward-compatible, deprecated shims by re-exporting the legacy symbols (e.g., parse_toml_config, serialize_to_toml, TomlError) from the toml format module so callers using superposition_core::{parse_toml_config, serialize_to_toml, TomlError} continue to compile; add the re-exports alongside the existing format exports (reference TomlFormat, FormatError) and mark them deprecated so they can be removed in a future release.
🧹 Nitpick comments (5)
clients/javascript/bindings/native-resolver.ts (1)
388-445: Factor the TOML/JSON native parse wrappers into one helper.
parseJsonConfig()is a near copy ofparseTomlConfig()with only the native symbol and error label changed. Pulling the shared buffer/decode/free/JSON.parseflow into a private helper will keep both formats in sync and make the next format cheaper to add.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/javascript/bindings/native-resolver.ts` around lines 388 - 445, Extract the shared native-call/response handling from parseJsonConfig and parseTomlConfig into a private helper (e.g., parseNativeConfig or _callNativeParser) that accepts the native symbol to call (like "core_parse_json_config" or "core_parse_toml_config") and an error label; the helper should allocate the ERROR_BUFFER_SIZE Buffer, invoke this.lib[fnSymbol](input, errorBuffer), check for a falsy result and read errorBuffer (null-terminated) to throw an Error with the label, decode non-string results via this.lib.decode(..., "string"), free non-string native strings with this.lib.core_free_string(...), then return the parsed JS object (JSON.parse for JSON path) or raw string for higher-level parsing; update parseJsonConfig to call this helper and perform the final JSON.parse and parsing error handling, and do the analogous change in parseTomlConfig so both use the same flow.clients/python/bindings/test_json_functions.py (2)
119-120: Remove extraneousfprefix from strings without placeholders.Static analysis correctly identifies that these f-strings have no placeholders.
🧹 Fix f-string warnings
- print(f"\n✓ Successfully parsed external JSON file!") - print(f"\nParsed configuration summary:") + print("\n✓ Successfully parsed external JSON file!") + print("\nParsed configuration summary:")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/bindings/test_json_functions.py` around lines 119 - 120, The two print statements in test_json_functions.py use f-strings with no placeholders (e.g., the calls printing "✓ Successfully parsed external JSON file!" and "Parsed configuration summary:"); remove the unnecessary f prefix and convert them to regular strings (use print("...") instead of print(f"...")) so static analysis warnings are resolved and behavior remains unchanged.
158-158: Remove extraneousfprefix.🧹 Fix f-string warning
- print(f"✗ Expected error but parsing succeeded!") + print("✗ Expected error but parsing succeeded!")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/bindings/test_json_functions.py` at line 158, The print call uses an unnecessary f-string prefix: change the statement print(f"✗ Expected error but parsing succeeded!") to a plain string print("✗ Expected error but parsing succeeded!") (locate the print call in the test_json_functions.py test that emits the "Expected error but parsing succeeded!" message and remove the leading 'f').crates/superposition_core/tests/format_integration.rs (2)
232-233: Remove redundant imports.
JsonFormatandConfigFormatare already imported at line 1 viasuperposition_core::{ConfigFormat, JsonFormat, TomlFormat}.🧹 Remove duplicate imports
#[test] fn test_json_round_trip_preserve_config() { - use superposition_core::format::json::JsonFormat; - use superposition_core::format::ConfigFormat; - let json = r#"{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/tests/format_integration.rs` around lines 232 - 233, Remove the duplicate imports by deleting the redundant lines that re-import JsonFormat and ConfigFormat; keep the existing glob import that already brings in ConfigFormat, JsonFormat, and TomlFormat. Specifically, remove the separate use statements referencing JsonFormat and ConfigFormat so only the initial superposition_core::{ConfigFormat, JsonFormat, TomlFormat} import remains (look for symbols JsonFormat and ConfigFormat in the tests/format_integration.rs imports).
112-123: The "not found" alternative in the error check is unreachable.Based on the
FormatError::UndeclaredDimensionDisplay implementation (which emits "Parsing error: Undeclared dimension '...'"), the error message will always contain "undeclared" but never "not found". The|| .contains("not found")branch is dead code.This doesn't break the test, but it could mask regressions if the error message changes unexpectedly.
🔧 Suggested simplification
assert!( toml_err.to_lowercase().contains("undeclared") - || toml_err.to_lowercase().contains("not found"), + || toml_err.to_lowercase().contains("unknown"), "TOML error should mention unknown dimension: {}", toml_err ); assert!( json_err.to_lowercase().contains("undeclared") - || json_err.to_lowercase().contains("not found"), + || json_err.to_lowercase().contains("unknown"), "JSON error should mention unknown dimension: {}", json_err );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_core/tests/format_integration.rs` around lines 112 - 123, The test assertions check for "undeclared" or "not found" in toml_err and json_err, but FormatError::UndeclaredDimension never emits "not found", so remove the unreachable alternative and assert only for the actual message; update the two asserts referencing toml_err and json_err to check that to_lowercase().contains("undeclared") (or otherwise match the Display output of FormatError::UndeclaredDimension) so the test accurately reflects the error text emitted by the Display impl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/javascript/bindings/README_TOML_TESTS.md`:
- Around line 114-116: Update the README_TOML_TESTS.md to reflect the
post-refactor JS contract: replace references claiming parseTomlConfig() returns
default_config plus `*_json` strings with the new structured return shape
(default_configs, contexts, overrides, dimensions) produced by
native-resolver.ts, remove the note about running an eval flow and the `3/3`
summary since clients/javascript/bindings/test-toml.ts no longer executes eval,
and ensure the “Parse External File” example references the correct example path
and output format to match the current native-resolver.ts output.
In `@crates/superposition_core/src/format.rs`:
- Around line 94-99: The DuplicatePosition error message is nondeterministic
because position_to_dimensions is populated from a HashMap iteration of
dimensions; make the output deterministic by sorting the dimension names before
producing the error: when building position_to_dimensions (and again in the
similar block at the other occurrence), ensure you collect dimension keys into a
Vec<String>, sort that Vec (e.g., .sort()), and then use the sorted Vec when
constructing FormatError::DuplicatePosition or when joining for display so the
same invalid config yields a stable message; refer to position_to_dimensions,
dimensions, and FormatError::DuplicatePosition to locate the changes.
- Around line 107-145: The validation error mappings in the cohort and
RemoteCohort branches (the closures that map validation errors to
FormatError::ValidationError after calling
validations::validate_cohort_schema_structure and validations::validate_schema)
must emit the original source-document paths rather than internal context names;
update the map_err closures to produce errors whose key and the entries returned
by validations::format_validation_errors are prefixed (or rewritten) with the
external path (use the dim variable / dim_info source path as the prefix) so
callers see e.g. overrides[...] or dimensions[...] schema locations. Concretely,
either call a validations helper that accepts a path prefix or post-process the
vector from validations::format_validation_errors(&errors) to rewrite internal
contexts to format!("{}.{}", dim, internal_key) before constructing
FormatError::ValidationError for both the validate_cohort_schema_structure and
validate_schema mappings.
In `@crates/superposition_core/src/format/json.rs`:
- Around line 124-147: The current mapping over detailed_config.contexts
silently swallows errors by using unwrap_or_default when fetching overrides and
serializing ctx.condition; change the closure passed to .map(|ctx| { ... }) so
it returns Result<JsonContext, FormatError> instead of JsonContext, replace
.cloned().unwrap_or_default() with a checked lookup that returns an
Err(FormatError::MissingOverride(...)) if the override key from
ctx.override_with_keys.get_key() is absent, replace
serde_json::to_value(&ctx.condition).unwrap_or_default() with
serde_json::to_value(&ctx.condition).map_err(|e|
FormatError::Serialization{source: e, context: ctx.clone()?})? (or an
appropriate FormatError variant), and when converting the Value to a Map return
an Err(FormatError::InvalidConditionType(...)) if it is not an Object; finally
collect the iterator with .collect::<Result<Vec<_>, _>>()? (mirroring the
dimensions handling) so failures propagate instead of producing empty maps.
In `@crates/superposition_core/src/format/tests/json.rs`:
- Around line 12-48: The tests use the helper function config_to_detailed which
rebuilds only a coarse "type"-only schema and hides schema metadata lost by
parse_config; replace uses of config_to_detailed with calling
parse_into_detailed on the serialized output (or adjust the test to parse via
parse_into_detailed) so the round-trip asserts validate the full DetailedConfig
produced by JsonFormat::serialize/parse_into_detailed, and remove or stop using
config_to_detailed in those serialize/parse tests to ensure constraints like
minimum/maximum/enum/object-shape and integer-vs-number are preserved.
---
Outside diff comments:
In `@crates/superposition_core/src/ffi_legacy.rs`:
- Around line 516-574: Missing C header declaration for the exported FFI
function core_parse_json_config prevents C consumers from linking to it; add a
corresponding prototype to the public header (headers/libcac_client.h) matching
the exported symbol (extern "C" signature): declare core_parse_json_config with
C types for the parameters (const char* json_content, char* ebuf) and returning
char* (or NULL on error), ensure it is inside the header guards and extern "C"
block so the C++ consumers get C linkage, and keep the prototype consistent with
the Rust definition in ffi_legacy.rs to avoid mismatches.
In `@crates/superposition_types/src/config.rs`:
- Around line 360-366: The dependency_graph field on the DimensionInfo struct is
required but lacks a serde default, causing older cached Config payloads to fail
deserialization; add the #[serde(default)] attribute to the dependency_graph
field in DimensionInfo and ensure DependencyGraph implements Default (or provide
a serde default function) so read_through_cache::<Config>() can deserialize
older cache entries into an empty/zeroed dependency graph.
---
Duplicate comments:
In `@clients/java/bindings/src/test/kotlin/JsonFunctionsTest.kt`:
- Around line 101-130: The test testTomlJsonEquivalence only compares
defaultConfigs.size and uses a TOML fixture that omits hour_of_day and three
overrides from EXAMPLE_JSON, so it doesn't prove equivalence; fix by either
making tomlConfig match EXAMPLE_JSON exactly (include hour_of_day and the three
override blocks) or perform a deep comparison of the parsed outputs from
ffiParseTomlConfig and ffiParseJsonConfig (compare defaultConfigs contents,
dimensions entries and their schemas/positions, overrides count and each
override's _context_ and values) to ensure structural parity rather than a
single-size assertion.
In `@crates/superposition_core/src/format/toml.rs`:
- Around line 38-46: The TOML conversion currently maps TOML emit/lookup
failures to TomlFormat::conversion_error (ConversionError) inside the
TryFrom<DimensionInfo> for DimensionInfoToml implementation; change these error
returns to the appropriate SerializationError via the FormatError type (use
TomlFormat::serialization_error or the SerializationError variant) when
producing TOML and when an expected override key is missing, and replace the
collapsed access with an explicit table.get("override") check (and return a
SerializationError if absent or of the wrong type). Apply the same change to the
analogous branch around the other block (lines ~84-95) so both schema conversion
and override lookup emit SerializationError with clear context instead of
ConversionError.
In `@crates/superposition_core/src/lib.rs`:
- Around line 7-29: The crate root currently removed the old top-level TOML API;
restore backward-compatible, deprecated shims by re-exporting the legacy symbols
(e.g., parse_toml_config, serialize_to_toml, TomlError) from the toml format
module so callers using superposition_core::{parse_toml_config,
serialize_to_toml, TomlError} continue to compile; add the re-exports alongside
the existing format exports (reference TomlFormat, FormatError) and mark them
deprecated so they can be removed in a future release.
In `@examples/superposition_config_file_examples/README.md`:
- Line 32: Update the scenario description for "Chennai ride" to match the
shipped example config by noting that Chennai is covered by the override
(city_cohort = south) and therefore does not use the default per_km_rate;
explicitly state the applied override and the resulting per_km_rate value so the
README text aligns with the example config.
---
Nitpick comments:
In `@clients/javascript/bindings/native-resolver.ts`:
- Around line 388-445: Extract the shared native-call/response handling from
parseJsonConfig and parseTomlConfig into a private helper (e.g.,
parseNativeConfig or _callNativeParser) that accepts the native symbol to call
(like "core_parse_json_config" or "core_parse_toml_config") and an error label;
the helper should allocate the ERROR_BUFFER_SIZE Buffer, invoke
this.lib[fnSymbol](input, errorBuffer), check for a falsy result and read
errorBuffer (null-terminated) to throw an Error with the label, decode
non-string results via this.lib.decode(..., "string"), free non-string native
strings with this.lib.core_free_string(...), then return the parsed JS object
(JSON.parse for JSON path) or raw string for higher-level parsing; update
parseJsonConfig to call this helper and perform the final JSON.parse and parsing
error handling, and do the analogous change in parseTomlConfig so both use the
same flow.
In `@clients/python/bindings/test_json_functions.py`:
- Around line 119-120: The two print statements in test_json_functions.py use
f-strings with no placeholders (e.g., the calls printing "✓ Successfully parsed
external JSON file!" and "Parsed configuration summary:"); remove the
unnecessary f prefix and convert them to regular strings (use print("...")
instead of print(f"...")) so static analysis warnings are resolved and behavior
remains unchanged.
- Line 158: The print call uses an unnecessary f-string prefix: change the
statement print(f"✗ Expected error but parsing succeeded!") to a plain string
print("✗ Expected error but parsing succeeded!") (locate the print call in the
test_json_functions.py test that emits the "Expected error but parsing
succeeded!" message and remove the leading 'f').
In `@crates/superposition_core/tests/format_integration.rs`:
- Around line 232-233: Remove the duplicate imports by deleting the redundant
lines that re-import JsonFormat and ConfigFormat; keep the existing glob import
that already brings in ConfigFormat, JsonFormat, and TomlFormat. Specifically,
remove the separate use statements referencing JsonFormat and ConfigFormat so
only the initial superposition_core::{ConfigFormat, JsonFormat, TomlFormat}
import remains (look for symbols JsonFormat and ConfigFormat in the
tests/format_integration.rs imports).
- Around line 112-123: The test assertions check for "undeclared" or "not found"
in toml_err and json_err, but FormatError::UndeclaredDimension never emits "not
found", so remove the unreachable alternative and assert only for the actual
message; update the two asserts referencing toml_err and json_err to check that
to_lowercase().contains("undeclared") (or otherwise match the Display output of
FormatError::UndeclaredDimension) so the test accurately reflects the error text
emitted by the Display impl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95014435-9057-47ec-92f6-47cca226cd09
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
Cargo.tomlclients/haskell/superposition-bindings/lib/FFI/Superposition.hsclients/haskell/superposition-bindings/test/Main.hsclients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.ktclients/java/bindings/src/test/kotlin/JsonFunctionsTest.ktclients/javascript/bindings/README_TOML_TESTS.mdclients/javascript/bindings/native-resolver.tsclients/javascript/bindings/test-json.tsclients/javascript/bindings/test-toml.tsclients/python/bindings/README_TOML_TESTS.mdclients/python/bindings/superposition_bindings/superposition_client.pyclients/python/bindings/test_json_functions.pyclients/python/bindings/test_toml_functions.pycrates/context_aware_config/src/api/config/handlers.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/ffi_legacy.rscrates/superposition_core/src/format.rscrates/superposition_core/src/format/error.rscrates/superposition_core/src/format/json.rscrates/superposition_core/src/format/tests.rscrates/superposition_core/src/format/tests/json.rscrates/superposition_core/src/format/tests/toml.rscrates/superposition_core/src/format/toml.rscrates/superposition_core/src/format/toml/helpers.rscrates/superposition_core/src/lib.rscrates/superposition_core/src/toml.rscrates/superposition_core/tests/format_integration.rscrates/superposition_core/tests/test_filter_debug.rscrates/superposition_types/src/config.rsdocs/plans/2026-01-02-toml-response-format.mdexamples/superposition_config_file_examples/Cargo.tomlexamples/superposition_config_file_examples/README.mdexamples/superposition_config_file_examples/example.jsonexamples/superposition_config_file_examples/example.tomlexamples/superposition_config_file_examples/src/main.rsexamples/superposition_toml_example/Cargo.tomlexamples/superposition_toml_example/README.mdexamples/superposition_toml_example/src/main.rsmakefilenix/rust.nix
💤 Files with no reviewable changes (5)
- examples/superposition_toml_example/src/main.rs
- examples/superposition_toml_example/README.md
- examples/superposition_toml_example/Cargo.toml
- docs/plans/2026-01-02-toml-response-format.md
- crates/superposition_core/src/toml.rs
Comment on lines
114
to
+116
| ### 3. Parse External File | ||
| - Demonstrates reading TOML from filesystem | ||
| - Parses `examples/superposition_toml_example/example.toml` | ||
| - Parses `examples/superposition_config_file_examples/example.toml` |
Contributor
There was a problem hiding this comment.
This README is still describing the pre-refactor JS contract.
While the example path is updated, the same document still says parseTomlConfig() returns default_config plus *_json strings and that the suite runs an eval test with a 3/3 summary. clients/javascript/bindings/native-resolver.ts now returns structured default_configs/contexts/overrides/dimensions, and clients/javascript/bindings/test-toml.ts no longer runs the eval flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/javascript/bindings/README_TOML_TESTS.md` around lines 114 - 116,
Update the README_TOML_TESTS.md to reflect the post-refactor JS contract:
replace references claiming parseTomlConfig() returns default_config plus
`*_json` strings with the new structured return shape (default_configs,
contexts, overrides, dimensions) produced by native-resolver.ts, remove the note
about running an eval flow and the `3/3` summary since
clients/javascript/bindings/test-toml.ts no longer executes eval, and ensure the
“Parse External File” example references the correct example path and output
format to match the current native-resolver.ts output.
Comment on lines
+94
to
+99
| let mut position_to_dimensions: HashMap<i32, Vec<String>> = HashMap::new(); | ||
| for (dim, dim_info) in dimensions.clone().into_iter() { | ||
| position_to_dimensions | ||
| .entry(dim_info.position) | ||
| .or_default() | ||
| .push(dim.clone()); |
Contributor
There was a problem hiding this comment.
Make DuplicatePosition deterministic.
position_to_dimensions is filled from a HashMap, so the dimensions list in FormatError::DuplicatePosition inherits hash iteration order. Because the display path joins that list directly, the same invalid config can produce different messages across runs or between formats.
🧭 Suggested fix
- for (position, dimensions) in position_to_dimensions {
- if dimensions.len() > 1 {
+ for (position, mut dimensions) in position_to_dimensions {
+ if dimensions.len() > 1 {
+ dimensions.sort();
return Err(FormatError::DuplicatePosition {
position,
dimensions,
});
}
}Also applies to: 179-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format.rs` around lines 94 - 99, The
DuplicatePosition error message is nondeterministic because
position_to_dimensions is populated from a HashMap iteration of dimensions; make
the output deterministic by sorting the dimension names before producing the
error: when building position_to_dimensions (and again in the similar block at
the other occurrence), ensure you collect dimension keys into a Vec<String>,
sort that Vec (e.g., .sort()), and then use the sorted Vec when constructing
FormatError::DuplicatePosition or when joining for display so the same invalid
config yields a stable message; refer to position_to_dimensions, dimensions, and
FormatError::DuplicatePosition to locate the changes.
Comment on lines
+107
to
+145
| validations::validate_cohort_schema_structure(&Value::from( | ||
| &dim_info.schema, | ||
| )) | ||
| .map_err(|errors| { | ||
| FormatError::ValidationError { | ||
| key: format!("{}.schema", dim), | ||
| errors: validations::format_validation_errors(&errors), | ||
| } | ||
| })?; | ||
|
|
||
| let cohort_dimension_info = | ||
| dimensions.get(cohort_dim).ok_or_else(|| { | ||
| FormatError::InvalidDimension(cohort_dim.clone()) | ||
| })?; | ||
|
|
||
| validations::validate_cohort_dimension_position( | ||
| cohort_dimension_info, | ||
| &dim_info, | ||
| ) | ||
| .map_err(|_| { | ||
| FormatError::InvalidCohortDimensionPosition { | ||
| dimension: dim.clone(), | ||
| dimension_position: dim_info.position, | ||
| cohort_dimension: cohort_dim.to_string(), | ||
| cohort_dimension_position: cohort_dimension_info.position, | ||
| } | ||
| })?; | ||
|
|
||
| create_connections_with_dependents(cohort_dim, &dim, &mut dimensions); | ||
| } | ||
| DimensionType::RemoteCohort(ref cohort_dim) => { | ||
| if !dimensions.contains_key(cohort_dim) { | ||
| return Err(FormatError::InvalidDimension(cohort_dim.clone())); | ||
| } | ||
|
|
||
| validations::validate_schema(&Value::from(&dim_info.schema)) | ||
| .map_err(|errors| FormatError::ValidationError { | ||
| key: format!("{}.schema", dim), | ||
| errors: validations::format_validation_errors(&errors), |
Contributor
There was a problem hiding this comment.
Emit source-document paths in these errors.
The diagnostics here currently mix real input paths (default-configs...) with internal ones (context[...], bare os.schema). That makes it harder for callers to map a parse failure back to overrides[...] or dimensions... in the JSON/TOML file.
Also applies to: 192-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format.rs` around lines 107 - 145, The
validation error mappings in the cohort and RemoteCohort branches (the closures
that map validation errors to FormatError::ValidationError after calling
validations::validate_cohort_schema_structure and validations::validate_schema)
must emit the original source-document paths rather than internal context names;
update the map_err closures to produce errors whose key and the entries returned
by validations::format_validation_errors are prefixed (or rewritten) with the
external path (use the dim variable / dim_info source path as the prefix) so
callers see e.g. overrides[...] or dimensions[...] schema locations. Concretely,
either call a validations helper that accepts a path prefix or post-process the
vector from validations::format_validation_errors(&errors) to rewrite internal
contexts to format!("{}.{}", dim, internal_key) before constructing
FormatError::ValidationError for both the validate_cohort_schema_structure and
validate_schema mappings.
Comment on lines
+124
to
+147
| let overrides = detailed_config | ||
| .contexts | ||
| .into_iter() | ||
| .map(|ctx| { | ||
| let override_key = ctx.override_with_keys.get_key(); | ||
| let overrides = detailed_config | ||
| .overrides | ||
| .get(override_key) | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
|
|
||
| let condition_value = | ||
| serde_json::to_value(&ctx.condition).unwrap_or_default(); | ||
| let context_map = match condition_value { | ||
| Value::Object(map) => map, | ||
| _ => Map::new(), | ||
| }; | ||
|
|
||
| JsonContext { | ||
| context: context_map, | ||
| overrides: overrides.into(), | ||
| } | ||
| }) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -160 crates/superposition_core/src/format/json.rs | tail -50Repository: juspay/superposition
Length of output: 1705
🏁 Script executed:
sed -n '100,160p' crates/superposition_core/src/format/json.rsRepository: juspay/superposition
Length of output: 2081
🏁 Script executed:
# Search for the parent function and its return type
rg "fn.*serialize|impl.*TryFrom" crates/superposition_core/src/format/json.rs -A 3Repository: juspay/superposition
Length of output: 1006
🏁 Script executed:
# Check if FormatError and serialization_error exist
rg "FormatError|serialization_error" crates/superposition_core/src/format/json.rsRepository: juspay/superposition
Length of output: 564
Fail fast when serializing broken contexts instead of silently defaulting to empty maps.
The closures at lines 128-147 currently mask errors by calling unwrap_or_default() on missing overrides (line 129) and failed condition serialization (line 135), then fallback to empty maps. A missing override_with_keys reference or serialization failure would silently produce invalid JSON configs with altered behavior. Convert the closure to return Result<JsonContext, FormatError> and collect with collect::<Result<Vec<_>, _>>()? to propagate errors, following the pattern already established for dimensions in the same function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format/json.rs` around lines 124 - 147, The
current mapping over detailed_config.contexts silently swallows errors by using
unwrap_or_default when fetching overrides and serializing ctx.condition; change
the closure passed to .map(|ctx| { ... }) so it returns Result<JsonContext,
FormatError> instead of JsonContext, replace .cloned().unwrap_or_default() with
a checked lookup that returns an Err(FormatError::MissingOverride(...)) if the
override key from ctx.override_with_keys.get_key() is absent, replace
serde_json::to_value(&ctx.condition).unwrap_or_default() with
serde_json::to_value(&ctx.condition).map_err(|e|
FormatError::Serialization{source: e, context: ctx.clone()?})? (or an
appropriate FormatError variant), and when converting the Value to a Map return
an Err(FormatError::InvalidConditionType(...)) if it is not an Object; finally
collect the iterator with .collect::<Result<Vec<_>, _>>()? (mirroring the
dimensions handling) so failures propagate instead of producing empty maps.
Comment on lines
+12
to
+48
| /// Helper to create a DetailedConfig from Config for testing | ||
| fn config_to_detailed(config: &Config) -> DetailedConfig { | ||
| let default_configs: BTreeMap<String, DefaultConfigInfo> = config | ||
| .default_configs | ||
| .iter() | ||
| .map(|(key, value)| { | ||
| let schema = match value { | ||
| Value::String(_) => json!({ "type": "string" }), | ||
| Value::Number(n) => { | ||
| if n.is_i64() { | ||
| json!({ "type": "integer" }) | ||
| } else { | ||
| json!({ "type": "number" }) | ||
| } | ||
| } | ||
| Value::Bool(_) => json!({ "type": "boolean" }), | ||
| Value::Array(_) => json!({ "type": "array" }), | ||
| Value::Object(_) => json!({ "type": "object" }), | ||
| Value::Null => json!({ "type": "null" }), | ||
| }; | ||
| ( | ||
| key.clone(), | ||
| DefaultConfigInfo { | ||
| value: value.clone(), | ||
| schema, | ||
| }, | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| DetailedConfig { | ||
| contexts: config.contexts.clone(), | ||
| overrides: config.overrides.clone(), | ||
| default_configs: DefaultConfigsWithSchema::from(default_configs), | ||
| dimensions: config.dimensions.clone(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
config_to_detailed weakens the serializer round-trip tests.
parse_config drops default-config schema metadata, and this helper rebuilds only a coarse "type" schema from the current value. That means the serialize/parse tests can still pass after losing constraints like minimum, maximum, enum, object shape, or integer-vs-number details; use parse_into_detailed anywhere the goal is to validate JsonFormat::serialize.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_core/src/format/tests/json.rs` around lines 12 - 48, The
tests use the helper function config_to_detailed which rebuilds only a coarse
"type"-only schema and hides schema metadata lost by parse_config; replace uses
of config_to_detailed with calling parse_into_detailed on the serialized output
(or adjust the test to parse via parse_into_detailed) so the round-trip asserts
validate the full DetailedConfig produced by
JsonFormat::serialize/parse_into_detailed, and remove or stop using
config_to_detailed in those serialize/parse tests to ensure constraints like
minimum/maximum/enum/object-shape and integer-vs-number are preserved.
8d5437c to
e0e1dd4
Compare
sauraww
force-pushed
the
other-config-formats
branch
2 times, most recently
from
e5f265e to
6944963
Compare
ayushjain17
force-pushed
the
other-config-formats
branch
from
11c41f1 to
fbaebc5
Compare
ayushjain17
force-pushed
the
other-config-formats
branch
from
fbaebc5 to
6e2676e
Compare
ayushjain17
approved these changes
Mar 17, 2026
This was referenced Mar 24, 2026
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
4 participants
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.
Problem
Currently superposition supports TOML as the only support format. This PR makes adding support for additional configuration file formats standardized and easier.
Solution
Summary by CodeRabbit
Release Notes
New Features
Documentation