feat: add toml parsing to superposition_core #796
Conversation
knutties
force-pushed
the
superposition-toml
branch
from
b6b022d to
828b9c3
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces TOML parsing and evaluation capabilities across the superposition platform via new core modules, multiple language bindings (Haskell, Java, JavaScript, Python), HTTP content negotiation for TOML responses, and comprehensive test coverage spanning all client languages. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Application
participant API as API Handler
participant Core as superposition_core
participant Parser as TOML Parser
participant Serializer as TOML Serializer
Note over Client,Serializer: TOML Parse & Response Flow
Client->>API: GET /config with Accept: application/toml
API->>API: determine_response_format(request)
alt TOML Requested
API->>Core: eval_config(toml_content, dimensions)
Core->>Parser: parse_toml_config(toml_string)
Parser->>Parser: Validate dimensions, contexts, overrides
Parser->>Parser: Compute priorities via position bit-shifts
Parser-->>Core: Config{ contexts, overrides, default_configs, dimensions }
Core->>Core: Apply overrides with merge strategy
Core-->>API: Evaluated Config
API->>Serializer: serialize_to_toml(config)
Serializer->>Serializer: Render [default-config], [dimensions], [context.*] sections
Serializer-->>API: TOML string (deterministic order, Blake3 hash)
API->>Client: HTTP 200, Content-Type: application/toml, TOML body
else JSON Requested
API->>Core: eval_config(...)
Core-->>API: Config
API->>Client: HTTP 200, Content-Type: application/json, JSON body
end
Note over Client,Serializer: Error Handling Path
Client->>API: POST /config with invalid TOML
API->>Parser: parse_toml_config(invalid_toml)
Parser->>Parser: Detect syntax/validation error (TomlError)
Parser-->>API: Err(TomlError::TomlSyntaxError)
API->>Client: HTTP 400/500 with error details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: This diff spans 50+ files with heterogeneous changes across multiple subsystems: new core modules (toml_parser, validations), type system refactors (ExtendedMap wrapping), FFI/bindings in four languages with platform-specific logic, API content negotiation, validation infrastructure consolidation, and systematic accessor pattern updates throughout codebase. While individual cohorts are relatively straightforward, the breadth, cross-cutting nature, and multiple integration points (build, tests, bindings) require careful verification of consistency across all affected layers. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
superposition-toml
branch
from
a4593a6 to
0618356
Compare
clients/python/bindings/superposition_bindings/superposition_client.py
Outdated
Show resolved
Hide resolved
clients/python/bindings/superposition_bindings/superposition_types.py
Outdated
Show resolved
Hide resolved
knutties
force-pushed
the
superposition-toml
branch
from
0618356 to
e1aa10c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/context_aware_config/src/api/context/validations.rs (1)
136-230: Fix type mismatch in jsonlogic path ofvalidate_context_jsonschemaIn the
#[cfg(feature = "jsonlogic")]branch,dimension_schemais rebound from&Valueto a compiledJSONSchema, and that same identifier is later passed intovalidate_value_against_schema, whose second parameter is&Value. This will not compile when thejsonlogicfeature is enabled and also conflates the raw schema with its compiled form.Use a separate binding (e.g.
compiled_schema) for the compiled validator and keepdimension_schemaas the JSON value passed tovalidate_value_against_schema. For example:Proposed fix
@@ - #[cfg(feature = "jsonlogic")] - { - let dimension_schema = JSONSchema::options() - .with_draft(Draft::Draft7) - .compile(dimension_schema) + #[cfg(feature = "jsonlogic")] + { + let compiled_schema = JSONSchema::options() + .with_draft(Draft::Draft7) + .compile(dimension_schema) .map_err(|e| { @@ - match dimension_value { - Value::Array(val_arr) if object_key == "in" => { + match dimension_value { + Value::Array(val_arr) if object_key == "in" => { @@ - val_arr.iter().for_each(|x| { - dimension_schema - .validate(x) + val_arr.iter().for_each(|x| { + compiled_schema + .validate(x) @@ - if verrors.is_empty() { - Ok(()) + if verrors.is_empty() { + Ok(()) } else { // Check if the array as a whole validates, even with individual errors - match dimension_schema.validate(dimension_value) { + match compiled_schema.validate(dimension_value) { @@ - _ => { - // Use shared validation for non-array cases - validate_value_against_schema(dimension_value, dimension_schema).map_err( - |errors| { + _ => { + // Use shared validation for non-array cases against the original schema value + validate_value_against_schema(dimension_value, dimension_schema).map_err( + |errors| {This preserves the intended special handling for
inover arrays while keeping the shared validator API correctly typed for non-array cases.
♻️ Duplicate comments (2)
clients/python/bindings/superposition_bindings/superposition_types.py (1)
1-1975: Auto-generated file should not be manually modified.As noted in a previous review, this file is auto-generated by uniffi. If changes are needed, they should be made to the source schema/IDL, and this file should be regenerated via the build process rather than being committed directly.
Based on learnings, auto-generated binding files are typically regenerated during the build and shouldn't be manually edited or tracked in version control unless there's a specific reason for the project.
Do you want me to verify the build process for regenerating uniffi bindings, or is this file intentionally committed?
clients/python/bindings/superposition_bindings/superposition_client.py (1)
1-5: Acknowledge: Auto-generated file.As noted in the past review comments, this is an auto-generated file from uniffi. The changes follow the established patterns for other FFI functions in this file. Future PRs should consider excluding this from manual review or regenerating it as part of the build process.
🧹 Nitpick comments (32)
clients/haskell/superposition-bindings/superposition-bindings.cabal (1)
43-44: Consider adding version bounds for test dependencies.While
aesonandbytestringare appropriate for handling JSON responses from the FFI layer, consider adding version bounds to improve build reproducibility and avoid future compatibility issues.Example:
build-depends: base ^>=4.18.2.0, HUnit, async, - aeson, - bytestring, + aeson ^>=2.2, + bytestring ^>=0.11, superposition-bindingsNote: This applies to other dependencies (
HUnit,async) as well, but that's outside the scope of this PR.clients/java/bindings/build.gradle.kts (2)
27-35: Consider validating the library path existence.The fallback path computation navigates three parent directories, which assumes a fixed project structure. If the directory layout changes or the library hasn't been built yet, tests will fail with potentially unclear error messages.
Consider adding validation to provide clearer feedback:
🔎 Optional enhancement for better error reporting
tasks.test { // Use environment variable if set (for CI/Make), otherwise compute relative path val libPath = System.getenv("SUPERPOSITION_LIB_PATH") ?: project.rootDir.parentFile.parentFile.parentFile.resolve("target/release").absolutePath + + // Validate library path exists + val libDir = file(libPath) + if (!libDir.exists()) { + logger.warn("Native library path does not exist: $libPath. Tests may fail if native library is required.") + } + systemProperty("java.library.path", libPath) systemProperty("jna.library.path", libPath) environment("LD_LIBRARY_PATH", libPath) environment("DYLD_LIBRARY_PATH", libPath) }
23-24: Consider migrating to JUnit 5 and update Gson to the latest version.JUnit 4 is legacy; JUnit 5 (Jupiter) offers better APIs, improved extension mechanisms, and is the current standard. For new test code, this is a worthwhile migration.
Gson 2.10.1 is outdated; the current version is 2.13.2. While 2.10.1 has no known vulnerabilities (including CVE-2022-25647, which affected versions < 2.8.9), upgrading to 2.13.2 is recommended to benefit from bug fixes and stay current with the upstream project.
examples/superposition_toml_example/Cargo.toml (1)
9-9: Consider using workspace dependency or a more specific version constraint for serde_json.The version constraint "1.0" is imprecise and could resolve to different minor/patch versions. The workspace root specifies
serde_json = { version = "1.0.140" }, so for consistency you could either:
- Use the workspace dependency:
serde_json = { workspace = true }- Specify a more precise version:
serde_json = "1.0.140"🔎 Proposed fix
-serde_json = "1.0" +serde_json = { workspace = true }crates/superposition_core/Cargo.toml (2)
14-14: Use more specific version constraint for blake3.The version constraint "1.5" is imprecise. Consider using "1.5.*" or a specific patch version like "1.5.0" for better reproducibility.
🔎 Proposed fix
-blake3 = "1.5" +blake3 = "1.5.0"
36-36: Use workspace dependency for toml to ensure consistency.The workspace root defines
toml = { version = "0.8.8", features = ["preserve_order"] }inworkspace.dependencies, but this crate usestoml = "0.8"without the workspace reference. Update to:-toml = "0.8" +toml = { workspace = true }This aligns the dependency specification with workspace conventions. The
preserve_orderfeature is inherited from the workspace definition, even though the crate's current TOML parsing logic (intoml_parser.rs) does not explicitly depend on key ordering.clients/java/bindings/README_TOML_TESTS.md (1)
105-126: Add language specifier to the fenced code block.The fenced code block showing expected output should specify a language identifier for proper syntax highlighting and rendering.
🔎 Proposed fix
-``` +```text ====================================================================== TEST: Parse TOML Configuration ======================================================================makefile (1)
408-451: Consider adding error handling and validation.The new
bindings-testtarget orchestrates multi-language TOML binding tests effectively. However, consider these improvements:
- Library existence check: Before copying libraries, verify the source file exists
- Test failure propagation: Individual test failures might not halt the overall target execution
- Platform detection edge cases: The macOS architecture detection is limited to arm64/x86_64
🔎 Suggested improvements
Add validation before library copying:
@# Copy library to bindings directory for Python tests with platform-specific name + @if [ ! -f "$(CARGO_TARGET_DIR)/release/libsuperposition_core.$(LIB_EXTENSION)" ]; then \ + echo "Error: Library not found. Run 'cargo build --package superposition_core --lib --release' first."; \ + exit 1; \ + fi @if [ "$$(uname)" = "Darwin" ]; then \Consider using
set -eat the beginning of the test sections to ensure failures propagate:bindings-test: uniffi-bindings + @set -e @echo ""clients/python/bindings/README_TOML_TESTS.md (1)
106-116: Add language specifier to the fenced code block.The fenced code block showing expected test output should specify a language identifier for proper syntax highlighting and rendering.
🔎 Proposed fix
-``` +```text ====================================================================== TEST SUMMARY ======================================================================clients/javascript/bindings/README_TOML_TESTS.md (1)
132-142: Add language specifier to the fenced code block for consistency.For consistency with markdown best practices and proper rendering, the expected output code block should specify a language identifier.
🔎 Proposed fix
-``` +```text ====================================================================== TEST SUMMARY ======================================================================clients/haskell/superposition-bindings/test/Main.hs (1)
73-82: Consider validating parsed structure.The test only checks that JSON parsing succeeds but doesn't assert on the actual parsed content. Consider adding assertions on key fields (e.g., checking for expected dimension names or default config keys) to catch structural issues.
Example enhancement
parseTomlValid :: IO () parseTomlValid = do result <- FFI.parseTomlConfig exampleToml case result of Right jsonStr -> do let parsed = decode (BSL.pack jsonStr) :: Maybe Value case parsed of Nothing -> HUnit.assertFailure $ "Failed to parse result JSON: " ++ jsonStr - Just _ -> HUnit.assertBool "Valid TOML should parse successfully" True + Just val -> do + -- Example: verify dimensions or default_configs exist in the parsed result + HUnit.assertBool "Valid TOML should parse successfully" True Left e -> HUnit.assertFailure $ "Failed to parse valid TOML: " ++ edocs/plans/2026-01-02-toml-response-format.md (1)
1-906: Consider archiving or relocating this implementation plan.This document appears to be an AI-assisted implementation plan with detailed step-by-step instructions and commit templates. Since the implementation is complete (as evidenced by the PR), consider:
- Moving it to an
archived-plans/directory- Converting relevant parts into design documentation
- Removing implementation-specific details and keeping only the architectural decisions
Implementation plans with specific commands and commit messages are less valuable as permanent documentation compared to design rationale and architecture decisions.
crates/context_aware_config/src/api/default_config/handlers.rs (2)
96-106: Consider preserving all validation errors for better diagnostics.The shared validation approach is good, but using only the first error (
.first()) may obscure multiple validation failures. When a schema has multiple issues, showing all errors would provide better developer feedback.🔎 Consider joining all errors
validate_value_against_schema(&default_config.value, &schema).map_err(|errors| { log::info!( "Validation for value with given JSON schema failed: {:?}", errors ); validation_error!( "Schema validation failed: {}", - errors.first().unwrap_or(&String::new()) + errors.join("; ") ) })?;
202-207: Same consideration for update endpoint validation errors.Similar to the create endpoint, consider joining all validation errors here as well for complete diagnostics.
🔎 Consider joining all errors
validate_value_against_schema(&value, &schema).map_err(|errors| { validation_error!( "Schema validation failed: {}", - errors.first().unwrap_or(&String::new()) + errors.join("; ") ) })?;crates/superposition_core/tests/test_filter_debug.rs (2)
6-44: Consider adding assertions or marking as ignored debug test.This test only prints debug output without any assertions to verify behavior. Consider either:
- Adding assertions to verify the parsed config structure, filter results, and serialization round-trip
- Adding
#[ignore]attribute if this is intended as a manual debug testExample assertions to add
println!("\n=== Serialized output ==="); let serialized = serialize_to_toml(&filtered_config).unwrap(); println!("{}", serialized); + + // Verify basic structure + assert_eq!(config.contexts.len(), 2, "Should have 2 contexts"); + assert!(config.default_configs.contains_key("timeout"), "Should have timeout in defaults"); + + // Verify filtering behavior + assert_eq!(filtered_config.contexts.len(), 0, "Empty dimensions should filter out all contexts"); + + // Verify round-trip serialization + let reparsed: Config = parse_toml_config(&serialized).unwrap(); + assert_eq!(reparsed.default_configs, filtered_config.default_configs); }
22-22: Replace unwrap() with proper error propagation.Using
unwrap()in tests can make it harder to diagnose failures. Consider using?with aResultreturn type or.expect()with descriptive messages.Proposed refactor
#[test] -fn test_filter_by_dimensions_debug() { +fn test_filter_by_dimensions_debug() -> Result<(), Box<dyn std::error::Error>> { - let config: Config = parse_toml_config(toml).unwrap(); + let config: Config = parse_toml_config(toml)?; // ... rest of test ... - let serialized = serialize_to_toml(&filtered_config).unwrap(); + let serialized = serialize_to_toml(&filtered_config)?; println!("{}", serialized); + Ok(()) }Also applies to: 42-42
examples/superposition_toml_example/src/main.rs (1)
47-55: Consider extracting the default_configs clone.The
(*config.default_configs).clone()is repeated in eacheval_configcall. For better efficiency and maintainability, consider extracting it once before the examples.Proposed refactor
// STEP 2: Use the parsed Config with eval_config for evaluation println!("\n--- Step 2: Evaluating Configuration with Different Dimensions ---"); println!("\nNow we'll use the parsed Config struct with eval_config() to resolve"); println!("configurations based on different input dimensions.\n"); + + // Clone default configs once for all evaluations + let default_configs = (*config.default_configs).clone(); // Example 1: Basic bike ride println!("--- Example 1: Bike ride (no specific city) ---"); let mut dims1 = Map::new(); dims1.insert( "vehicle_type".to_string(), Value::String("bike".to_string()), ); let result1 = eval_config( - (*config.default_configs).clone(), + default_configs.clone(), &config.contexts, &config.overrides, &config.dimensions, &dims1, MergeStrategy::MERGE, None, )?;Also applies to: 74-82, 102-110, 131-139
crates/superposition_core/src/ffi_legacy.rs (1)
506-529: Consider simplifying JSON structure.The current implementation serializes contexts, overrides, and dimensions to JSON strings, then embeds those strings into another JSON object. This creates double-encoded JSON (e.g.,
"contexts_json": "{...}").Consider directly including the parsed values in the result JSON:
🔎 Proposed refactor
- // Serialize contexts, overrides, and dimensions to JSON strings - let contexts_json = match serde_json::to_string(&parsed.contexts) { - Ok(s) => s, - Err(e) => { - copy_string(ebuf, format!("Failed to serialize contexts: {}", e)); - return ptr::null_mut(); - } - }; - - let overrides_json = match serde_json::to_string(&parsed.overrides) { - Ok(s) => s, - Err(e) => { - copy_string(ebuf, format!("Failed to serialize overrides: {}", e)); - return ptr::null_mut(); - } - }; - - let dimensions_json = match serde_json::to_string(&parsed.dimensions) { - Ok(s) => s, - Err(e) => { - copy_string(ebuf, format!("Failed to serialize dimensions: {}", e)); - return ptr::null_mut(); - } - }; - - // Create result with default_config as Map and others as JSON strings + // Create result with all components as proper JSON structures let result = serde_json::json!({ "default_config": &*parsed.default_configs, - "contexts_json": contexts_json, - "overrides_json": overrides_json, - "dimensions_json": dimensions_json, + "contexts": parsed.contexts, + "overrides": parsed.overrides, + "dimensions": parsed.dimensions, });crates/context_aware_config/src/api/dimension/validations.rs (1)
270-279: Minor: Misplaced comment formatting.The comment
// Use shared validation...is awkwardly placed after the closing brace of theifblock due to the trailing}. This appears to be a formatting issue.🔎 Suggested fix
if cohort_based_on.is_empty() { return Err(validation_error!( "Please specify a valid dimension that this cohort can derive from. Refer our API docs for examples", )); - } // Use shared validation from superposition_core for cohort schema structure - // - validate_cohort_schema_structure(cohort_schema).map_err(|errors| { + } + + // Use shared validation from superposition_core for cohort schema structure + validate_cohort_schema_structure(cohort_schema).map_err(|errors| {clients/javascript/bindings/test-toml.ts (2)
137-169: Consider making error handling tests contribute to the pass/fail count.The
testErrorHandlingfunction doesn't return a boolean, so its results aren't counted in the final summary. If any error case unexpectedly succeeds, it logs a failure message but doesn't affect the exit code.This is consistent with the Python test file, so may be intentional for this initial implementation.
🔎 Optional: Track error handling test results
-function testErrorHandling(): void { +function testErrorHandling(): boolean { printSectionHeader('TEST 3: Error Handling'); const resolver = new NativeResolver(); + let allPassed = true; // ... existing test cases ... invalidTomlCases.forEach((testCase, i) => { // ... existing code ... try { resolver.parseTomlConfig(testCase.toml); console.log('✗ Expected error but parsing succeeded!'); + allPassed = false; } catch (error: any) { console.log(`✓ Correctly caught error: ${error.constructor.name}`); console.log(` Message: ${error.message.substring(0, 100)}`); } }); + + return allPassed; }
77-79: Potential runtime error when overrides has fewer than 3 entries.The slice
Object.entries(overrides).slice(0, 3)is safe, but the string truncation withsubstring(0, 100)followed by...could be misleading if the string is shorter than 100 characters.🔎 Minor improvement for output formatting
Object.entries(overrides).slice(0, 3).forEach(([id, data]) => { - console.log(` ${id}: ${JSON.stringify(data).substring(0, 100)}...`); + const dataStr = JSON.stringify(data); + console.log(` ${id}: ${dataStr.length > 100 ? dataStr.substring(0, 100) + '...' : dataStr}`); });clients/javascript/bindings/native-resolver.ts (1)
347-350: Redundant buffer initialization.
Buffer.alloc(ERROR_BUFFER_SIZE)already initializes all bytes to zero, soerrorBuffer.fill(0)is unnecessary.🔎 Remove redundant fill
// Allocate error buffer (matching the Rust implementation) const ERROR_BUFFER_SIZE = 2048; const errorBuffer = Buffer.alloc(ERROR_BUFFER_SIZE); - errorBuffer.fill(0);clients/python/bindings/test_toml_functions.py (3)
117-119: Remove extraneous f-string prefixes.Lines 118-119 use f-strings without any placeholders. This was flagged by static analysis.
🔎 Remove unnecessary f-prefixes
- print(f"\n✓ Successfully parsed external TOML file!") - print(f"\nParsed configuration summary:") + print("\n✓ Successfully parsed external TOML file!") + print("\nParsed configuration summary:")
157-162: Remove unused variable assignment.The
resultvariable is assigned but never used. The static analysis correctly flagged this.🔎 Remove unused variable
try: - result = ffi_parse_toml_config(case['toml']) + ffi_parse_toml_config(case['toml']) print(f"✗ Expected error but parsing succeeded!")
159-159: Remove extraneous f-string prefix.Line 159 uses an f-string without any placeholders.
🔎 Remove unnecessary f-prefix
- print(f"✗ Expected error but parsing succeeded!") + print("✗ Expected error but parsing succeeded!")design-docs/2026-01-02-toml-response-format-design.md (1)
71-99: Tighten markdown formatting to satisfy linters and improve readabilitySeveral fenced code blocks lack an explicit language, some bare URLs are used in the References section, and a few emphasized lines (e.g., “TOML Response Format for get_config API”, “TOML Response:”) are styled as headings instead of using
#/##. Adding languages to code fences, wrapping URLs in markdown links, and converting emphasis-only headings to real headings will quiet markdownlint and make the doc easier to consume.Also applies to: 429-453, 463-471, 1155-1163
clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.kt (1)
771-783:ffiParseTomlConfigwiring looks correct; fix KDoc error typeThe new checksum entry, JNA binding, and
ffiParseTomlConfigwrapper follow the existing UniFFI patterns and correctly lift aConfigviaFfiConverterTypeConfig. One small nit: the KDoc says the error case returnsErr(OperationError), but the wrapper actually throwsOperationExceptionviaOperationException’s error handler, in line with the rest of this file. Consider updating the comment to referenceOperationExceptionso the documented error type matches the Kotlin API.Also applies to: 826-835, 1753-1782
crates/context_aware_config/src/api/context/validations.rs (1)
35-48: Consider surfacing all validation errors using shared formatter
validate_override_with_default_configscurrently logs the fullerrorsvector but only returns the first error in thevalidation_error!message. Sincesuperposition_core::validationsexposesformat_validation_errors, you could reuse it here to show all validation failures for a key in a single, consistent string, improving debuggability when multiple constraints fail on the same value.design-docs/2025-12-21-toml-parsing-ffi-design.md (1)
31-61: Clarify historical vs current API in the design doc and tidy markdownThis doc now mixes the original design (e.g.,
TomlParseErrorenum andeval_toml_configpublic API) with later “Implementation Updates (2026-01-02)” that rename the error type and removeeval_toml_config. To avoid confusion for future readers, consider explicitly marking the earlier Public API / FFI sections as historical or updating them to match the final implementation (TomlError, parse-only public API).While touching the doc, you may also want to address the markdownlint hints: add languages to a few unlabeled fenced code blocks, avoid hard tabs in Makefile snippets, and convert bare URLs in the References section into markdown links.
Also applies to: 375-437, 1750-1807
crates/superposition_core/src/toml_parser.rs (3)
637-658: Strengthen string escaping invalue_to_tomlfor full TOML compatibility
value_to_tomlcurrently only escapes backslashes and double quotes in strings. Other control characters (newlines, carriage returns, tabs, etc.) will be emitted verbatim inside a basic string, which can result in invalid TOML or at least TOML that’s harder to round-trip reliably if configs ever contain multi-line or control-character values.A more robust approach would be to delegate escaping to the
tomlcrate for the string case, e.g. by constructing atoml::Value::Stringand using itsto_string()result, or by explicitly handling the standard TOML escape sequences (\n,\t,\r, etc.) in addition to\"and\\. This would makeserialize_to_tomlsafer for arbitrary string values without changing the overall layout of the generated TOML.
166-176: Guard against extreme or invalidpositionvalues incompute_priority
compute_prioritycalculates1 << dim_info.positionfor each participating dimension. Given thatpositionis ani32parsed directly from TOML, extremely large or negative positions could cause shifts that overflow or hit the sign bit oni32. In practice you probably only use small positive positions, but you may want to either validate thatpositionis within a sane range (e.g., 0–30) when parsing dimensions, or switch the accumulator/shift to a wider unsigned type to avoid surprising behaviour for bad inputs.
178-248: Be aware of implicit numeric/bool coercion inparse_context_expression
parse_context_expressionattempts to parse each right-hand side asi64, thenf64, thenbool, and only falls back toString. That’s reasonable, but it means a context likeport=8080will be treated as an integer (and must have an integer dimension schema), while a schema that intentionally expects a string"8080"would require quoting in TOML (port="8080"). Similarly, values liketrue/falsemap to booleans.If this coercion semantics is intentional, it would be good to document it in the TOML format docs and examples so users know when to quote values to force string-type matching. Otherwise, consider tightening or making this coercion configurable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockclients/javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.gitignoreCargo.tomlclients/haskell/superposition-bindings/lib/FFI/Superposition.hsclients/haskell/superposition-bindings/superposition-bindings.cabalclients/haskell/superposition-bindings/test/Main.hsclients/java/bindings/README_TOML_TESTS.mdclients/java/bindings/build.gradle.ktsclients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.ktclients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.ktclients/java/bindings/src/test/kotlin/TomlFunctionsTest.ktclients/javascript/bindings/.gitignoreclients/javascript/bindings/README_TOML_TESTS.mdclients/javascript/bindings/native-resolver.tsclients/javascript/bindings/test-ffi.tsclients/javascript/bindings/test-toml.tsclients/python/bindings/README_TOML_TESTS.mdclients/python/bindings/superposition_bindings/__init__.pyclients/python/bindings/superposition_bindings/superposition_client.pyclients/python/bindings/superposition_bindings/superposition_types.pyclients/python/bindings/test_toml_functions.pycrates/cac_client/src/eval.rscrates/cac_client/src/lib.rscrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/validations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/validations.rscrates/context_aware_config/src/helpers.rscrates/frontend/src/pages/home.rscrates/superposition_core/Cargo.tomlcrates/superposition_core/src/config.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/ffi_legacy.rscrates/superposition_core/src/lib.rscrates/superposition_core/src/toml_parser.rscrates/superposition_core/src/validations.rscrates/superposition_core/tests/test_filter_debug.rscrates/superposition_provider/src/client.rscrates/superposition_provider/src/utils.rscrates/superposition_types/src/config.rscrates/superposition_types/src/config/tests.rscrates/superposition_types/src/overridden.rsdesign-docs/2025-12-21-toml-parsing-ffi-design.mddesign-docs/2026-01-02-toml-response-format-design.mddocs/api/toml-response-format.mddocs/plans/2026-01-02-toml-response-format.mdexamples/superposition_toml_example/Cargo.tomlexamples/superposition_toml_example/README.mdexamples/superposition_toml_example/example.tomlexamples/superposition_toml_example/src/main.rsmakefilenix/rust.nixuniffi/patches/python.patch
🧰 Additional context used
🧬 Code graph analysis (15)
crates/context_aware_config/src/api/default_config/handlers.rs (2)
crates/service_utils/src/helpers.rs (1)
parse_config_tags(364-389)crates/superposition_core/src/validations.rs (3)
validate_value_against_schema(18-30)schema(85-93)errors(82-82)
crates/superposition_core/src/ffi_legacy.rs (1)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)
crates/superposition_core/src/ffi.rs (1)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)
crates/superposition_types/src/config.rs (1)
clients/python/bindings/superposition_bindings/superposition_types.py (2)
Config(929-952)Context(979-1006)
clients/python/bindings/test_toml_functions.py (2)
clients/python/bindings/superposition_bindings/superposition_client.py (1)
ffi_parse_toml_config(1633-1660)crates/superposition_core/src/ffi.rs (1)
ffi_parse_toml_config(181-184)
crates/superposition_core/src/validations.rs (1)
crates/context_aware_config/src/api/dimension/validations.rs (3)
schema(144-152)e(115-115)e(392-392)
crates/context_aware_config/src/api/context/validations.rs (2)
crates/superposition_core/src/validations.rs (3)
validate_value_against_schema(18-30)schema(85-93)errors(82-82)crates/service_utils/src/helpers.rs (1)
validation_err_to_str(186-311)
examples/superposition_toml_example/src/main.rs (1)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)
crates/frontend/src/pages/home.rs (1)
crates/superposition_types/src/config.rs (1)
into_iter(94-96)
crates/superposition_core/src/toml_parser.rs (1)
crates/superposition_core/src/validations.rs (5)
errors(82-82)validate_value_against_schema(18-30)schema(85-93)validate_schema(43-55)validate_cohort_schema_structure(77-145)
crates/superposition_core/tests/test_filter_debug.rs (3)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)crates/superposition_core/src/toml_parser.rs (1)
serialize_to_toml(697-779)crates/superposition_types/src/config.rs (1)
get_key(323-325)
crates/context_aware_config/src/api/config/handlers.rs (1)
crates/superposition_core/src/toml_parser.rs (1)
serialize_to_toml(697-779)
clients/python/bindings/superposition_bindings/superposition_client.py (4)
clients/python/bindings/superposition_bindings/superposition_types.py (40)
Config(929-952)_UniffiConverterTypeConfig(954-976)check_lower(381-389)check_lower(393-399)check_lower(862-865)check_lower(919-921)check_lower(965-969)check_lower(1020-1025)check_lower(1077-1082)check_lower(1134-1139)check_lower(1249-1258)check_lower(1307-1318)check_lower(1357-1362)check_lower(1395-1400)check_lower(1433-1438)check_lower(1471-1476)check_lower(1491-1493)check_lower(1518-1520)lift(372-373)lift(405-407)lift(882-884)lift(1784-1785)lift(1806-1807)lift(1828-1829)lift(1850-1851)lift(1872-1873)lift(1894-1895)lift(1916-1917)lift(1938-1939)lower(376-377)lower(410-413)lower(887-890)lower(1792-1793)lower(1814-1815)lower(1836-1837)lower(1858-1859)lower(1880-1881)lower(1902-1903)lower(1924-1925)lower(1946-1947)clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.kt (10)
uniffi_superposition_core_fn_func_ffi_parse_toml_config(833-834)lift(179-239)lift(181-181)lift(246-249)lift(247-247)lift(293-295)lower(184-184)lower(248-248)lower(1064-1066)lower(1107-1114)crates/superposition_core/src/ffi.rs (1)
ffi_parse_toml_config(181-184)clients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.kt (1)
lower(148-148)
clients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.kt (1)
clients/python/bindings/superposition_bindings/superposition_types.py (1)
Config(929-952)
clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.kt (1)
clients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.kt (1)
uniffiRustCallWithError(266-271)
🪛 LanguageTool
design-docs/2026-01-02-toml-response-format-design.md
[style] ~578-~578: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... - Consider pagination or streaming for very large configs - Not a problem for initial imp...
(EN_WEAK_ADJECTIVE)
[style] ~1128-~1128: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...5. Streaming Serialization - For very large configs (> 10MB) - Stream TOML outpu...
(EN_WEAK_ADJECTIVE)
docs/plans/2026-01-02-toml-response-format.md
[grammar] ~676-~676: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: TOML formatted response with Content-Type: ap...
(QB_NEW_EN_HYPHEN)
[grammar] ~685-~685: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: JSON formatted response (existing behavior) ...
(QB_NEW_EN_HYPHEN)
[grammar] ~694-~694: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: JSON formatted response **Step 6: Create doc...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
clients/java/bindings/README_TOML_TESTS.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
design-docs/2026-01-02-toml-response-format-design.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
429-429: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
441-441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1156-1156: Bare URL used
(MD034, no-bare-urls)
1157-1157: Bare URL used
(MD034, no-bare-urls)
1158-1158: Bare URL used
(MD034, no-bare-urls)
1163-1163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
clients/python/bindings/README_TOML_TESTS.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/plans/2026-01-02-toml-response-format.md
19-19: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
45-45: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
63-63: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
82-82: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
87-87: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
105-105: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
148-148: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
153-153: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
181-181: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
186-186: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
216-216: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
221-221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
249-249: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
254-254: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
307-307: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
312-312: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
381-381: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
386-386: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
394-394: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
399-399: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
418-418: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
451-451: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
464-464: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
492-492: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
530-530: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
536-536: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
541-541: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
561-561: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
584-584: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
608-608: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
635-635: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
640-640: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
659-659: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
664-664: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
669-669: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
678-678: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
687-687: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
696-696: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
763-763: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
784-784: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
791-791: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
826-826: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
844-844: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
849-849: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
854-854: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
859-859: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
864-864: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
869-869: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
877-877: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
design-docs/2025-12-21-toml-parsing-ffi-design.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1293-1293: Bare URL used
(MD034, no-bare-urls)
1294-1294: Bare URL used
(MD034, no-bare-urls)
1295-1295: Bare URL used
(MD034, no-bare-urls)
1296-1296: Bare URL used
(MD034, no-bare-urls)
1674-1674: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1675-1675: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1676-1676: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1677-1677: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1678-1678: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1679-1679: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1680-1680: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1681-1681: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1682-1682: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1683-1683: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1684-1684: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1685-1685: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1686-1686: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1687-1687: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1688-1688: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1689-1689: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1690-1690: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1691-1691: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1692-1692: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1693-1693: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1694-1694: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1695-1695: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1696-1696: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1697-1697: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1698-1698: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1699-1699: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1700-1700: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1701-1701: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1702-1702: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1703-1703: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1704-1704: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1705-1705: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1706-1706: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1707-1707: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1708-1708: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1709-1709: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1710-1710: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1711-1711: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1712-1712: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1713-1713: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1714-1714: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1715-1715: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1716-1716: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1842-1842: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
1860-1860: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.10)
clients/python/bindings/test_toml_functions.py
91-91: Consider moving this statement to an else block
(TRY300)
93-93: Do not catch blind exception: Exception
(BLE001)
118-118: f-string without any placeholders
Remove extraneous f prefix
(F541)
119-119: f-string without any placeholders
Remove extraneous f prefix
(F541)
125-125: Consider moving this statement to an else block
(TRY300)
127-127: Do not catch blind exception: Exception
(BLE001)
158-158: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
159-159: f-string without any placeholders
Remove extraneous f prefix
(F541)
160-160: Do not catch blind exception: Exception
(BLE001)
clients/python/bindings/superposition_bindings/superposition_types.py
1585-1585: Avoid specifying long messages outside the exception class
(TRY003)
1694-1694: Avoid specifying long messages outside the exception class
(TRY003)
1701-1701: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
1760-1760: Avoid specifying long messages outside the exception class
(TRY003)
1767-1767: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
clients/python/bindings/superposition_bindings/superposition_client.py
505-505: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Provider Tests (kotlin)
- GitHub Check: Testing
| parseTomlConfig :: String -> IO (Either String String) | ||
| parseTomlConfig tomlContent = do | ||
| ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation | ||
| tomlStr <- newCString tomlContent | ||
| res <- parse_toml_config tomlStr ebuf | ||
| err <- peekCAString ebuf | ||
| let peekMaybe p | p /= nullPtr = Just <$> peekCAString p | ||
| | otherwise = pure Nothing | ||
| result <- peekMaybe res | ||
| free tomlStr | ||
| free ebuf | ||
| pure $ case (result, err) of | ||
| (Just cfg, []) -> Right cfg | ||
| (Nothing, []) -> Left "null pointer returned" | ||
| _ -> Left err |
There was a problem hiding this comment.
Memory leak: missing free for result pointer.
The result pointer res returned by parse_toml_config is allocated by the Rust FFI (via string_to_c_str at line 548 in ffi_legacy.rs) and must be freed by the caller. Currently, only tomlStr and ebuf are freed, but res is not.
🔎 Proposed fix
+foreign import capi "superposition_core.h core_free_string"
+ free_string :: CString -> IO ()
+
parseTomlConfig :: String -> IO (Either String String)
parseTomlConfig tomlContent = do
ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation
tomlStr <- newCString tomlContent
res <- parse_toml_config tomlStr ebuf
err <- peekCAString ebuf
let peekMaybe p | p /= nullPtr = Just <$> peekCAString p
| otherwise = pure Nothing
result <- peekMaybe res
free tomlStr
free ebuf
+ when (res /= nullPtr) (free_string res)
pure $ case (result, err) of
(Just cfg, []) -> Right cfg
(Nothing, []) -> Left "null pointer returned"
_ -> Left err🤖 Prompt for AI Agents
In clients/haskell/superposition-bindings/lib/FFI/Superposition.hs around lines
118-132, the pointer res returned by parse_toml_config is allocated by the Rust
FFI and must be freed; update the function to, after reading result via
peekCAString (i.e. after using peekMaybe res), check if res /= nullPtr and call
free res to release the FFI-allocated memory (do this before returning), and
ensure the frees for tomlStr and ebuf remain in place so all allocated pointers
(res, tomlStr, ebuf) are freed on all code paths.
There was a problem hiding this comment.
Create a new ForeignPtr that has a finalizer, and then use withForeignPtr
https://hackage-content.haskell.org/package/base-4.22.0.0/docs/Foreign-ForeignPtr.html#v:withForeignPtr
There was a problem hiding this comment.
@Datron, excellent suggestion! Using ForeignPtr with a finalizer is indeed more idiomatic and safer than manual memory management. Here's the revised implementation:
-- Add to imports at top of file
import Foreign.ForeignPtr (ForeignPtr, newForeignPtr, withForeignPtr)
-- Foreign import for the finalizer
foreign import capi "superposition_core.h core_free_string"
free_string :: CString -> IO ()
-- Updated parseTomlConfig implementation
parseTomlConfig :: String -> IO (Either String String)
parseTomlConfig tomlContent = do
ebuf <- callocBytes 2048
tomlStr <- newCString tomlContent
res <- parse_toml_config tomlStr ebuf
err <- peekCAString ebuf
result <- if res /= nullPtr
then do
-- Create ForeignPtr with finalizer for automatic cleanup
resFPtr <- newForeignPtr free_string res
withForeignPtr resFPtr $ \ptr -> Just <$> peekCAString ptr
else pure Nothing
free tomlStr
free ebuf
pure $ case (result, err) of
(Just cfg, []) -> Right cfg
(Nothing, []) -> Left "null pointer returned"
_ -> Left errThis approach:
- Creates a
ForeignPtrwithfree_stringas the finalizer - Uses
withForeignPtrto safely access the pointer duringpeekCAString - Automatically frees the FFI-allocated memory when the
ForeignPtrgoes out of scope - Eliminates the risk of forgetting to free on different code paths
| default-language: GHC2021 | ||
| default-extensions: CApiFFI | ||
| extra-libraries: superposition_core | ||
| include-dirs: ../../../target/include |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for README files and build documentation in haskell clients directory
find clients/haskell -type f -name "README*" -o -name "*.md" | head -20Repository: juspay/superposition
Length of output: 397
🏁 Script executed:
#!/bin/bash
# Check for Makefile, build scripts in haskell clients directory
find clients/haskell -type f \( -name "Makefile" -o -name "build.sh" -o -name "build.py" -o -name "*.sh" \) | head -20Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for any documentation about Rust in the haskell clients directory
find clients/haskell -type f \( -name "*.md" -o -name "*.txt" -o -name "README*" \) -exec cat {} \; 2>/dev/null | head -50Repository: juspay/superposition
Length of output: 1682
🏁 Script executed:
#!/bin/bash
# Examine the cabal file itself and nearby structure
cat -n clients/haskell/superposition-bindings/superposition-bindings.cabal | head -60Repository: juspay/superposition
Length of output: 1528
🏁 Script executed:
#!/bin/bash
# Check the directory structure around the Haskell bindings
ls -la clients/haskell/
ls -la clients/haskell/superposition-bindings/Repository: juspay/superposition
Length of output: 1271
🏁 Script executed:
#!/bin/bash
# Check cabal.project file
cat clients/haskell/cabal.projectRepository: juspay/superposition
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check default.nix file for Rust build handling
cat clients/haskell/default.nixRepository: juspay/superposition
Length of output: 1216
🏁 Script executed:
#!/bin/bash
# Look for GitHub Actions or CI/CD workflows
find . -type d -name ".github" -o -name ".gitlab-ci.yml" 2>/dev/null | head -5Repository: juspay/superposition
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check if there's a top-level README or build documentation
cat README.md 2>/dev/null | head -100Repository: juspay/superposition
Length of output: 7289
🏁 Script executed:
#!/bin/bash
# Check for any setup.hs that might handle the build order
cat clients/haskell/superposition-bindings/Setup.hsRepository: juspay/superposition
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Look for any Nix flake or build files at root
ls -la | grep -E "flake|nix|Makefile|setup"Repository: juspay/superposition
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Search for GitHub Actions workflows that build Haskell code
find .github/workflows -name "*.yaml" -o -name "*.yml" 2>/dev/null | head -10Repository: juspay/superposition
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Check for any workflow that mentions haskell or cabal
find .github -type f \( -name "*.yaml" -o -name "*.yml" \) -exec grep -l "haskell\|cabal\|superposition-bindings" {} \;Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look at flake.nix to understand the complete build system
cat flake.nix | head -100Repository: juspay/superposition
Length of output: 2633
🏁 Script executed:
#!/bin/bash
# Check the CI workflow files to see how Haskell is built
cat .github/workflows/ci_check_pr.yamlRepository: juspay/superposition
Length of output: 15520
🏁 Script executed:
#!/bin/bash
# Check the release workflow to see how it handles Haskell builds
cat .github/workflows/release.yaml | head -150Repository: juspay/superposition
Length of output: 5746
🏁 Script executed:
#!/bin/bash
# Check nix workflow
cat .github/workflows/nix.yamlRepository: juspay/superposition
Length of output: 449
🏁 Script executed:
#!/bin/bash
# Look for any documentation about building in clients/haskell
find clients/haskell -name "*.md" -exec cat {} \;Repository: juspay/superposition
Length of output: 4822
The hardcoded include path requires clarification for non-Nix builds.
The include-dirs: ../../../target/include on line 29 points to a relative path outside the package. While the Nix build system (used in CI and dev shells) properly manages the superposition_core package dependency and provides the correct C_INCLUDE_PATH environment variable, the hardcoded relative path is fragile and unclear.
Consider:
- Verifying whether this path is necessary when building within Nix (
nix develop) or if the environment variableC_INCLUDE_PATHfromdefault.nixis sufficient - If the path is needed for non-Nix builds, documenting the Rust compilation prerequisite and directory structure assumption
- Alternatively, using a
Setup.hsscript or environment variable to avoid hardcoding the path
🤖 Prompt for AI Agents
In clients/haskell/superposition-bindings/superposition-bindings.cabal around
line 29, the hardcoded include-dirs: ../../../target/include is fragile for
non-Nix builds; remove or conditionalize the hardcoded relative path and instead
rely on the C_INCLUDE_PATH provided by the Nix environment when building under
Nix, and for non-Nix builds either document the required Rust build output
location and prerequisite steps in the package README or implement a Setup.hs
that reads an environment variable (e.g., SUPERPOSITION_CORE_INCLUDE) to set
include-dirs at configure time so the path is not hardcoded.
| pub unsafe extern "C" fn core_parse_toml_config( | ||
| toml_content: *const c_char, | ||
| ebuf: *mut c_char, | ||
| ) -> *mut c_char { | ||
| // Null pointer check | ||
| if toml_content.is_null() { | ||
| copy_string(ebuf, "toml_content is null"); | ||
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| // Convert C string to Rust string | ||
| let toml_str = match c_str_to_string(toml_content) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| copy_string(ebuf, format!("Invalid UTF-8 in toml_content: {}", e)); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Parse TOML | ||
| let parsed = match crate::parse_toml_config(&toml_str) { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| copy_string(ebuf, e.to_string()); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Serialize contexts, overrides, and dimensions to JSON strings | ||
| let contexts_json = match serde_json::to_string(&parsed.contexts) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| copy_string(ebuf, format!("Failed to serialize contexts: {}", e)); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| let overrides_json = match serde_json::to_string(&parsed.overrides) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| copy_string(ebuf, format!("Failed to serialize overrides: {}", e)); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| let dimensions_json = match serde_json::to_string(&parsed.dimensions) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| copy_string(ebuf, format!("Failed to serialize dimensions: {}", e)); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Create result with default_config as Map and others as JSON strings | ||
| let result = serde_json::json!({ | ||
| "default_config": &*parsed.default_configs, | ||
| "contexts_json": contexts_json, | ||
| "overrides_json": overrides_json, | ||
| "dimensions_json": dimensions_json, | ||
| }); | ||
|
|
||
| let result_str = match serde_json::to_string(&result) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| copy_string(ebuf, format!("JSON serialization error: {}", e)); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Convert to C string | ||
| string_to_c_str(result_str) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the copy_string function definition and check for REVIEW comments
echo "=== Looking for copy_string function definition ==="
rg -n "fn copy_string|REVIEW" crates/superposition_core/src/ -B2 -A5 | head -100
echo ""
echo "=== Searching for error buffer related documentation ==="
rg -n "2048|256|error.*buffer|TRUNCATE|truncate" crates/superposition_core/src/ffi_legacy.rs
echo ""
echo "=== Listing all files in crates directory to find language bindings ==="
find crates -type f \( -name "*.hs" -o -name "*.py" -o -name "*.js" -o -name "*.ts" \) | head -20Repository: juspay/superposition
Length of output: 1500
🏁 Script executed:
#!/bin/bash
# Find language bindings that interact with FFI functions
echo "=== Searching for calls to core_parse_toml_config in bindings ==="
rg -r -n "core_parse_toml_config|ebuf|callocBytes|malloc.*2048|malloc.*256" --type=js --type=ts --type=py
echo ""
echo "=== Check if there are other language binding directories ==="
find . -name "bindings" -o -name "ffi" -o -name "*-bindings*" -type d 2>/dev/null | head -10
echo ""
echo "=== Look for Go/Rust FFI bindings ==="
find . -type f \( -name "*.go" -o -name "*.so" -o -name "*.a" \) 2>/dev/null | grep -E "(binding|ffi)" | head -10
echo ""
echo "=== Check the documentation of core_parse_toml_config more thoroughly ==="
rg -B10 -A5 "pub unsafe extern.*core_parse_toml_config" crates/superposition_core/src/ffi_legacy.rsRepository: juspay/superposition
Length of output: 2102
🏁 Script executed:
#!/bin/bash
# Get the native-resolver.ts file content
echo "=== JavaScript binding (native-resolver.ts) ==="
cat clients/javascript/bindings/native-resolver.ts | head -200
echo ""
echo "=== Python binding files ==="
find clients/python/bindings -type f -name "*.py" | head -5
rg -n "256|2048|buffer|alloc" clients/python/bindings/ -A2
echo ""
echo "=== Haskell binding files ==="
find clients/haskell/superposition-bindings -type f | head -10Repository: juspay/superposition
Length of output: 30360
🏁 Script executed:
#!/bin/bash
# Check Haskell FFI binding for buffer allocation
echo "=== Haskell FFI binding ==="
cat clients/haskell/superposition-bindings/lib/FFI/Superposition.hs | head -150
echo ""
echo "=== Search for any comments about buffer size choices ==="
rg -n "256|2048" clients/ --type=hs --type=ts -B2 -A2
echo ""
echo "=== Check if there are more recent FFI function definitions in ffi_legacy.rs ==="
rg -n "core_parse_toml_config\|ebuf\|2048\|256" crates/superposition_core/src/ffi_legacy.rs | head -30Repository: juspay/superposition
Length of output: 10412
Fix buffer overflow vulnerability and correct error buffer size inconsistencies across language bindings.
The copy_string function (line 40) does not truncate—it copies the full string unbounded using ptr::copy_nonoverlapping(src, to, from.len() + 1) without bounds checking. This creates a buffer overflow risk when error messages exceed the allocated buffer size.
Additionally, error buffer sizes are inconsistent across bindings:
- JavaScript: allocates 256 bytes for most calls (lines 147, 211, 284), but 2048 bytes for
parseTomlConfig(line 348) - Haskell: allocates 256 bytes for
getResolvedConfig(line 79), but 2048 bytes forparseTomlConfig(line 120) - Rust documentation: specifies 2048 bytes minimum
Either add bounds checking to copy_string, or enforce consistent buffer sizes across all language bindings and align them with the Rust-side buffer capacity.
🤖 Prompt for AI Agents
In crates/superposition_core/src/ffi_legacy.rs around lines 478 to 549, the FFI
function uses copy_string to write error messages into an external buffer but
copy_string performs an unbounded copy causing buffer overflows; modify
copy_string to accept a destination length parameter and copy at most dest_len -
1 bytes then write a null terminator (truncating longer messages), update all
internal calls in this file to pass the provided ebuf length (or change the
extern API to accept an ebuf_len parameter), and simultaneously standardize all
language bindings to allocate a consistent minimum error buffer (align with Rust
docs: 2048 bytes) or update their calls to pass the actual buffer size so
truncation is safe across JS, Haskell and other bindings.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
uniffi/patches/python.patch (1)
27-43: Address platform support regression and architecture string inconsistency.The
import platformis already present in both files, so that concern is not an issue.However, two real problems remain:
Platform support regression: The old code supported macOS (any architecture), Windows (any architecture), and all ELF platforms (Linux, BSD, Solaris/illumos). The new
triple_maponly covers 4 specific combinations:("darwin", "arm64"),("darwin", "x86_64"),("linux", "x86_64"), and("win32", "x86_64"). This breaks support for ARM Linux (Raspberry Pi, ARM servers), non-x86_64 architectures, BSD systems, and Solaris/illumos users. Document whether this platform restriction is intentional or provide prebuilt binaries for broader architecture support.Windows platform inconsistency: On older Python versions on Windows,
platform.machine()returns"AMD64"instead of"x86_64". Thetriple_maponly has a key for("win32", "x86_64"), which will cause aRuntimeErroron affected systems. Normalize architecture strings (e.g., map"AMD64"→"x86_64") before lookup to ensure compatibility across Python versions.crates/context_aware_config/src/api/context/validations.rs (1)
136-230: Fix type mismatch in jsonlogic branch ofvalidate_context_jsonschema(compiled schema vs.&Value)In the
#[cfg(feature = "jsonlogic")]block ofvalidate_context_jsonschema, you currently do:
let dimension_schema = JSONSchema::options().with_draft(Draft::Draft7).compile(dimension_schema)?;- Later, in the non‑array
_arm, callvalidate_value_against_schema(dimension_value, dimension_schema).
validate_value_against_schemaexpects&serde_json::Valueas the second argument, but heredimension_schemahas been rebound to a compiledJSONSchema. This will not compile when thejsonlogicfeature is enabled and also doesn’t match the intended API.A minimal fix is to keep the original schema
&Valueseparate from the compiledJSONSchemaand:
- Use the compiled schema only in the
"in"/array path.- Use
validate_value_against_schemawith the original JSON schema for the non‑array path.For example:
Proposed fix to keep compiled and raw schemas separate
pub fn validate_context_jsonschema( #[cfg(feature = "jsonlogic")] object_key: &str, dimension_value: &Value, dimension_schema: &Value, ) -> result::Result<()> { @@ - // For jsonlogic, we need special handling for array validation + // For jsonlogic, we need special handling for array validation #[cfg(feature = "jsonlogic")] { - let dimension_schema = JSONSchema::options() + // Compile once for the array/"in" case + let compiled_schema = JSONSchema::options() .with_draft(Draft::Draft7) - .compile(dimension_schema) + .compile(dimension_schema) .map_err(|e| { @@ - match dimension_value { + match dimension_value { Value::Array(val_arr) if object_key == "in" => { let mut verrors = Vec::new(); val_arr.iter().for_each(|x| { - dimension_schema + compiled_schema .validate(x) @@ if verrors.is_empty() { Ok(()) } else { // Check if the array as a whole validates, even with individual errors - match dimension_schema.validate(dimension_value) { + match compiled_schema.validate(dimension_value) { @@ } _ => { - // Use shared validation for non-array cases - validate_value_against_schema(dimension_value, dimension_schema).map_err( + // Use shared validation for non-array cases against the raw JSON schema + validate_value_against_schema(dimension_value, dimension_schema).map_err( |errors| { log::error!(This preserves the special handling for
"in"arrays while reusing the shared validator for simpler cases and fixes the type mismatch.
♻️ Duplicate comments (1)
clients/python/bindings/superposition_bindings/superposition_types.py (1)
1-1975: This file should not be modified manually - it is auto-generated by uniffi.As noted in the previous review, changes to this file should be removed since it is auto-generated by uniffi (see comment on lines 3-4: "This file was autogenerated by some hot garbage in the
unifficrate"). The file should be regenerated from the Rust source rather than checked in with manual modifications.If these changes are the result of running
uniffi-bindgen, ensure the generated file is either:
- Excluded from version control and regenerated at build time, or
- Committed as-is after regeneration without manual edits
🧹 Nitpick comments (25)
uniffi/patches/python.patch (1)
79-95: Duplicate function—ensure fixes apply to both files.The
_uniffi_load_indirect()function is duplicated in bothsuperposition_client.pyandsuperposition_types.pywith identical logic. Ensure that any fixes for the import verification, platform coverage, and architecture string issues flagged in the first occurrence are applied consistently to both files.Additionally, consider normalizing emoji usage in error messages. While the ❌ emoji adds visual emphasis, it may not render consistently across all terminal environments and is unconventional in library error messages.
clients/haskell/superposition-bindings/test/Main.hs (1)
73-82: Strengthen test assertions.The test only validates that the result is valid JSON but doesn't verify the structure or content. Line 81's assertion (
HUnit.assertBool "Valid TOML should parse successfully" True) is redundant—if execution reaches that line, the test has already succeeded.Consider validating key fields in the parsed JSON (e.g., presence of
default_config,contexts_json,overrides_json,dimensions_json) to ensure the parser produces the expected output structure.🔎 Proposed enhancement
parseTomlValid :: IO () parseTomlValid = do result <- FFI.parseTomlConfig exampleToml case result of Right jsonStr -> do let parsed = decode (BSL.pack jsonStr) :: Maybe Value case parsed of Nothing -> HUnit.assertFailure $ "Failed to parse result JSON: " ++ jsonStr - Just _ -> HUnit.assertBool "Valid TOML should parse successfully" True + Just val -> do + -- Verify expected top-level keys exist in the parsed result + case val of + Object obj -> do + HUnit.assertBool "Should contain default_config" ("default_config" `member` obj) + HUnit.assertBool "Should contain dimensions_json" ("dimensions_json" `member` obj) + _ -> HUnit.assertFailure "Expected top-level object" Left e -> HUnit.assertFailure $ "Failed to parse valid TOML: " ++ eNote: You'll need to add
import Data.Aeson (decode, Value, Object)andimport Data.HashMap.Strict (member)or similar.crates/superposition_core/Cargo.toml (1)
14-14: Consider adding blake3 to workspace dependencies.The blake3 dependency is pinned directly here, while most other dependencies use
{ workspace = true }. For consistency and centralized version management, consider adding blake3 to the[workspace.dependencies]section in the root Cargo.toml.crates/frontend/src/pages/home.rs (1)
502-516: Consider avoiding double-cloning of default_configs.The current implementation clones
default_configsat line 502, then clones the map again at line 516 within theForcomponent'seachclosure. While this works correctly and is not critical in a frontend rendering context, you could potentially optimize by usingArc<Map<String, Value>>or restructuring to avoid the second clone.However, given that this is within a Leptos reactive component and the map is typically small, the current approach is acceptable.
💡 Potential optimization
If
default_configs_mapis large or this rendering happens frequently, consider:let default_configs_map = Arc::new((*config.default_configs).clone()); // ... each=move || { let map = Arc::clone(&default_configs_map); map.iter().map(|(k, v)| (k.clone(), v.clone())) }This reduces cloning the entire map on each reactive update.
clients/javascript/bindings/.gitignore (1)
1-3: Consider committingpackage-lock.jsonfor reproducible builds.Ignoring
package-lock.jsonis unusual for a project that will be built by others. The lockfile ensures reproducible dependency resolution across different environments and CI builds.If this is intentional (e.g., the bindings directory is always built fresh and not published), this can remain, but typically lockfiles should be committed.
🔎 Suggested fix
node_modules/ -package-lock.json *.logcrates/superposition_core/tests/test_filter_debug.rs (1)
6-44: Consider adding assertions or marking as ignored.This test provides useful diagnostic output but lacks assertions, so it will always pass regardless of behavior. Consider one of these improvements:
- Add assertions to verify expected behavior (e.g., filtered context count, override keys preservation)
- Mark with
#[ignore]if it's intended as a manual diagnostic tool- Move to an example if it's primarily for documentation/exploration
🔎 Example with assertions
#[test] fn test_filter_by_dimensions_debug() { let toml = r#" [default-config] timeout = { value = 30, schema = { type = "integer" } } [dimensions] dimension = { position = 1, schema = { type = "string" } } [context."dimension=d1"] timeout = 60 [context."dimension=d2"] timeout = 90 "#; let config: Config = parse_toml_config(toml).unwrap(); - println!("\n=== Before filter ==="); - println!("Contexts count: {}", config.contexts.len()); - for ctx in &config.contexts { - println!(" - Context id: {}, override_key: {}", ctx.id, ctx.override_with_keys.get_key()); - } - println!("Overrides keys: {:?}", config.overrides.keys().collect::<Vec<_>>()); + assert_eq!(config.contexts.len(), 2, "Should have 2 contexts before filter"); // Simulate what API does - filter by empty dimension data let empty_dimensions: Map<String, serde_json::Value> = Map::new(); let filtered_config = config.filter_by_dimensions(&empty_dimensions); - println!("\n=== After filter (empty dimensions) ==="); - println!("Contexts count: {}", filtered_config.contexts.len()); - for ctx in &filtered_config.contexts { - println!(" - Context id: {}, override_key: {}", ctx.id, ctx.override_with_keys.get_key()); - } - println!("Overrides keys: {:?}", filtered_config.overrides.keys().collect::<Vec<_>>()); + assert_eq!(filtered_config.contexts.len(), 0, "Empty dimensions should filter out all contexts"); - println!("\n=== Serialized output ==="); let serialized = serialize_to_toml(&filtered_config).unwrap(); - println!("{}", serialized); + assert!(serialized.contains("[default-config]"), "Serialization should preserve default config"); }clients/java/bindings/README_TOML_TESTS.md (1)
132-132: Optional: Add language identifier to fenced code block.The fenced code block for merge strategies should specify a language for proper syntax highlighting.
Based on coding guidelines from static analysis.
🔎 Suggested fix
+```text - `"merge"` (default): Merges override values with default configuration - `"replace"`: Replaces entire configuration with override values +```examples/superposition_toml_example/src/main.rs (1)
47-55: Consider extracting repeated eval_config pattern into a helper function.The pattern of cloning
(*config.default_configs)and callingeval_configwith the same arguments is repeated four times (lines 47-55, 74-82, 102-110, 131-139). While this is acceptable for an example, extracting a helper function would improve readability and reduce duplication.🔎 Proposed refactor
+fn evaluate_scenario( + config: &superposition_core::ParsedTomlConfig, + dims: &Map<String, Value>, +) -> Result<Map<String, Value>, Box<dyn std::error::Error>> { + eval_config( + (*config.default_configs).clone(), + &config.contexts, + &config.overrides, + &config.dimensions, + dims, + MergeStrategy::MERGE, + None, + ) +} + fn main() -> Result<(), Box<dyn std::error::Error>> { // ... existing code ... - let result1 = eval_config( - (*config.default_configs).clone(), - &config.contexts, - &config.overrides, - &config.dimensions, - &dims1, - MergeStrategy::MERGE, - None, - )?; + let result1 = evaluate_scenario(&config, &dims1)?;Apply similar changes to the other three eval_config calls.
docs/api/toml-response-format.md (2)
116-129: Highlight the schema loss limitation more prominently.The note about schema details not being preserved during TOML serialization is buried at the end. This is a significant limitation—enum values, minimum/maximum constraints, patterns, and other JSON Schema features are lost. Consider adding a warning or limitations section earlier in the document, perhaps right after the overview or in the requesting section, so users are aware of this trade-off before adopting TOML responses.
101-114: Consider documenting Accept header mismatch behavior.The error handling section describes TOML serialization failures but doesn't document what happens when TOML serialization fails for a client that explicitly requested TOML via
Accept: application/toml. The response returns JSON (line 109), which means the Content-Type will mismatch the Accept header. While pragmatic, this behavior should be explicitly documented to set correct client expectations.crates/context_aware_config/src/api/config/handlers.rs (1)
68-85: Consider implementing proper HTTP content negotiation with quality values.The current implementation doesn't handle Accept header quality values (e.g.,
Accept: application/json;q=0.9, application/toml;q=1.0), which means client preferences aren't fully respected. While the current simple approach works for basic cases, a more robust solution would parse quality values and select the highest-priority acceptable format.Libraries like
actix-web'saccept-headercrate or manual parsing of q-values would provide more correct HTTP semantics. This is particularly important if multiple formats might be requested with different priorities.Do you want me to provide an implementation that handles q-values, or is the simplified approach sufficient for your use case?
clients/javascript/bindings/test-toml.ts (1)
102-102: Fragile path construction for external file.The path construction uses multiple
'..'segments to navigate up the directory tree, which can be fragile if the directory structure changes or tests are run from different locations.Consider a more robust approach
- const exampleFile = path.join(__dirname, '..', '..', '..', '..', 'examples', 'superposition-toml-example', 'example.toml'); + // Try to find the repository root by looking for a marker file + const findRepoRoot = (startDir: string): string | null => { + let currentDir = startDir; + while (currentDir !== path.dirname(currentDir)) { + if (fs.existsSync(path.join(currentDir, 'Cargo.toml'))) { + return currentDir; + } + currentDir = path.dirname(currentDir); + } + return null; + }; + + const repoRoot = findRepoRoot(__dirname); + if (!repoRoot) { + console.log('⚠ Could not find repository root'); + return null; + } + const exampleFile = path.join(repoRoot, 'examples', 'superposition-toml-example', 'example.toml');clients/python/bindings/test_toml_functions.py (1)
134-163: Strengthen error‑handling test to actually fail on unexpected success and avoid broadExceptionRight now
test_error_handlingonly prints a message when invalid TOML unexpectedly parses and doesn’t influence the overall exit code, and it also catches a bareExceptionand assigns an unusedresult. That makes the negative‑case tests non‑actionable if this script is ever wired into CI.Consider tightening it like this:
- Assert‑fail when parsing succeeds for an invalid case.
- Catch the specific error type you expect from
ffi_parse_toml_config(e.g.OperationError), or at leastExceptionplus an assertion that the type is as expected.- Drop the unused
resultbinding.Proposed adjustment
@@ for i, case in enumerate(invalid_toml_cases, 1): print(f"\nTest {i}: {case['name']}") print("-" * 50) try: - result = ffi_parse_toml_config(case['toml']) - print(f"✗ Expected error but parsing succeeded!") - except Exception as e: + ffi_parse_toml_config(case["toml"]) + print("✗ Expected error but parsing succeeded!") + # Make the test fail if this ever happens + return False + except Exception as e: # consider narrowing this to the concrete error type print(f"✓ Correctly caught error: {type(e).__name__}") print(f" Message: {str(e)[:100]}") + + return TrueYou could then have
main()treat this like the other tests and include it in the pass/fail summary.design-docs/2026-01-02-toml-response-format-design.md (1)
71-99: Optional: add languages to code fences and wrap bare URLs to satisfy markdown lintersThe design content itself looks solid and in line with the implementation, but markdownlint is flagging a few purely-formatting issues:
- Fenced blocks like the ASCII architecture diagram, raw HTTP response examples, and some JSON/TOML snippets lack an explicit language tag.
- The reference links at the end use bare URLs.
If you care about a clean docs lint run, you can:
- Add appropriate language tags, e.g.
```textfor diagrams/HTTP,```toml, or```httpwhere relevant.- Wrap URLs in markdown links (
[TOML Specification](https://toml.io/), etc.).Also applies to: 429-452, 463-471, 1154-1158
crates/superposition_core/src/validations.rs (1)
18-55: Shared JSON Schema validation utilities look correct; meta‑schemas could be cached later if neededThe new helpers (
validate_value_against_schema,validate_schema,validate_cohort_schema_structure,get_meta_schema,get_cohort_meta_schema, andformat_validation_errors) are wired correctly:
- All validations use Draft7 consistently.
- Error paths return
Vec<String>with clear messages.- Cohort schema checks enforce the documented invariants (
enumcontains"otherwise", non‑emptydefinitions, 1:1 mapping between enum options and definitions aside from"otherwise").Given these functions may be called in hot paths, you might eventually want to cache the small compiled meta‑schemas from
get_meta_schemaandget_cohort_meta_schemabehind aOnceLock/lazy_static!to avoid recompiling them, but that’s a micro‑optimization and not required for correctness.Also applies to: 77-145, 155-215
docs/plans/2026-01-02-toml-response-format.md (1)
1-10: Optional: remove tool‑specific note and tighten wording for long‑term docs hygieneThe implementation plan is clear, but the line:
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.is very tool‑specific and may be confusing noise for future maintainers reading the docs.
You may want to:
- Drop that note (and similar assistant‑targeted wording) from the committed plan.
- Optionally adopt the minor grammar/markdown suggestions from your linters (e.g., “TOML‑formatted response” / “JSON‑formatted response”).
These are documentation‑only cleanups and won’t affect behavior.
crates/superposition_core/src/toml_parser.rs (5)
250-259: Address clippy type_complexity warning with a named struct.The return type
Result<(Map<String, Value>, Map<String, Value>), TomlError>is flagged by clippy as overly complex. A named struct improves readability and self-documentation.🔎 Proposed refactor
+/// Result of parsing the default-config section +struct DefaultConfigResult { + values: Map<String, Value>, + schemas: Map<String, Value>, +} + /// Parse the default-config section -/// Returns (values, schemas) where schemas are stored for validating overrides -fn parse_default_config(table: &toml::Table) -> Result<(Map<String, Value>, Map<String, Value>), TomlError> { +fn parse_default_config(table: &toml::Table) -> Result<DefaultConfigResult, TomlError> { // ... existing logic ... - Ok((values, schemas)) + Ok(DefaultConfigResult { values, schemas }) }Then update the caller in
parse():- let (default_config, schemas) = parse_default_config(&toml_table)?; + let DefaultConfigResult { values: default_config, schemas } = parse_default_config(&toml_table)?;
342-347: Consider validating position range before truncation.The cast from
i64toi32will silently truncate if the value exceedsi32::MAX. While unlikely in practice, defensive validation would prevent subtle bugs.🔎 Proposed fix
let position = table["position"].as_integer().ok_or_else(|| { TomlError::ConversionError(format!( "dimensions.{}.position must be an integer", key )) - })? as i32; + })?; + + if position < 0 || position > i32::MAX as i64 { + return Err(TomlError::ConversionError(format!( + "dimensions.{}.position must be between 0 and {}", + key, i32::MAX + ))); + } + let position = position as i32;
768-769: Remove leftover debug comment.The comment "DIAGNOSTIC: Print what we're looking for vs what's available" appears to be a debugging artifact that should be removed before merging.
🔎 Proposed fix
- // DIAGNOSTIC: Print what we're looking for vs what's available let override_key = context.override_with_keys.get_key();
700-726: Schema inference loses original constraints during serialization.The schema inference (lines 704-717) only preserves the basic type, losing constraints like
minimum,maximum,enum,pattern, etc. This means a round-trip (parse → serialize → parse) will result in less restrictive validation.Consider documenting this limitation in the function's docstring, or storing original schemas in the
Configstruct to enable lossless round-tripping.🔎 Proposed documentation update
/// Serialize Config structure to TOML format /// /// Converts a Config object back to TOML string format matching the input specification. -/// The output can be parsed by `parse()` to recreate an equivalent Config. +/// The output can be parsed by `parse()` to recreate a functionally equivalent Config. +/// +/// **Note:** Schema constraints (minimum, maximum, enum, pattern, etc.) are not preserved +/// during serialization. Only the basic type is inferred from the value. Round-tripped +/// configurations will have less restrictive validation than the original.
888-895: Add test coverage for strings with control characters.The special character tests cover quotes and backslashes but not newlines, tabs, or carriage returns. Adding such tests would help validate the string escaping fix suggested above.
🔎 Proposed additional test
#[test] fn test_value_to_toml_control_chars() { let val = Value::String("line1\nline2".to_string()); assert_eq!(value_to_toml(&val), r#""line1\nline2""#); let val2 = Value::String("col1\tcol2".to_string()); assert_eq!(value_to_toml(&val2), r#""col1\tcol2""#); let val3 = Value::String("text\r\n".to_string()); assert_eq!(value_to_toml(&val3), r#""text\r\n""#); }design-docs/2025-12-21-toml-parsing-ffi-design.md (4)
3-6: Update author attribution.The author is listed as "Claude Sonnet 4.5" which is an AI assistant. For official project documentation, consider attributing to the human author or team who reviewed and approved the design, or remove the author line if that's the project convention.
37-61: Add language specifier to fenced code blocks.The ASCII diagrams don't have a language specifier. While these are intentional plaintext diagrams, adding
textorplaintextsatisfies the markdown linter.🔎 Proposed fix
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ External Languages (C, Kotlin, Swift, etc.) │
1291-1296: Convert bare URLs to markdown links.The reference URLs should use markdown link syntax for consistency and accessibility.
🔎 Proposed fix
## References -- **TOML Specification:** https://toml.io/ -- **uniffi Documentation:** https://mozilla.github.io/uniffi-rs/ -- **BLAKE3 Hashing:** https://github.com/BLAKE3-team/BLAKE3 -- **Reference Implementation:** https://github.com/juspay/superposition/tree/cac-toml/crates/superposition_toml +- **TOML Specification:** <https://toml.io/> +- **uniffi Documentation:** <https://mozilla.github.io/uniffi-rs/> +- **BLAKE3 Hashing:** <https://github.com/BLAKE3-team/BLAKE3> +- **Reference Implementation:** <https://github.com/juspay/superposition/tree/cac-toml/crates/superposition_toml>
1858-1860: Consider using a heading instead of emphasis for the closing.The document ends with bold text "End of Design Document". Using a proper heading (
## End of Design Document) would be more semantically correct, though this is a minor stylistic preference.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockclients/javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.gitignoreCargo.tomlclients/haskell/superposition-bindings/lib/FFI/Superposition.hsclients/haskell/superposition-bindings/superposition-bindings.cabalclients/haskell/superposition-bindings/test/Main.hsclients/java/bindings/README_TOML_TESTS.mdclients/java/bindings/build.gradle.ktsclients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.ktclients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.ktclients/java/bindings/src/test/kotlin/TomlFunctionsTest.ktclients/javascript/bindings/.gitignoreclients/javascript/bindings/README_TOML_TESTS.mdclients/javascript/bindings/native-resolver.tsclients/javascript/bindings/test-ffi.tsclients/javascript/bindings/test-toml.tsclients/python/bindings/README_TOML_TESTS.mdclients/python/bindings/superposition_bindings/__init__.pyclients/python/bindings/superposition_bindings/superposition_client.pyclients/python/bindings/superposition_bindings/superposition_types.pyclients/python/bindings/test_toml_functions.pycrates/cac_client/src/eval.rscrates/cac_client/src/lib.rscrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/validations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/validations.rscrates/context_aware_config/src/helpers.rscrates/frontend/src/pages/home.rscrates/superposition_core/Cargo.tomlcrates/superposition_core/src/config.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/ffi_legacy.rscrates/superposition_core/src/lib.rscrates/superposition_core/src/toml_parser.rscrates/superposition_core/src/validations.rscrates/superposition_core/tests/test_filter_debug.rscrates/superposition_provider/src/client.rscrates/superposition_provider/src/utils.rscrates/superposition_types/src/config.rscrates/superposition_types/src/config/tests.rscrates/superposition_types/src/overridden.rsdesign-docs/2025-12-21-toml-parsing-ffi-design.mddesign-docs/2026-01-02-toml-response-format-design.mddocs/api/toml-response-format.mddocs/plans/2026-01-02-toml-response-format.mdexamples/superposition_toml_example/Cargo.tomlexamples/superposition_toml_example/README.mdexamples/superposition_toml_example/example.tomlexamples/superposition_toml_example/src/main.rsmakefilenix/rust.nixuniffi/patches/python.patch
🧰 Additional context used
🧬 Code graph analysis (12)
examples/superposition_toml_example/src/main.rs (1)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)
crates/superposition_core/src/validations.rs (1)
crates/context_aware_config/src/api/dimension/validations.rs (4)
schema(144-152)e(115-115)e(392-392)test_get_meta_schema(383-403)
crates/superposition_core/src/ffi_legacy.rs (1)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)
crates/frontend/src/pages/home.rs (1)
crates/superposition_types/src/config.rs (1)
into_iter(94-96)
crates/context_aware_config/src/api/default_config/handlers.rs (2)
crates/service_utils/src/helpers.rs (1)
parse_config_tags(364-389)crates/superposition_core/src/validations.rs (3)
validate_value_against_schema(18-30)schema(85-93)errors(82-82)
crates/context_aware_config/src/api/config/handlers.rs (1)
crates/superposition_core/src/toml_parser.rs (1)
serialize_to_toml(697-779)
clients/python/bindings/superposition_bindings/superposition_client.py (2)
clients/python/bindings/superposition_bindings/superposition_types.py (29)
Config(929-952)_UniffiConverterTypeConfig(954-976)check_lower(381-389)check_lower(393-399)check_lower(862-865)check_lower(919-921)check_lower(965-969)check_lower(1020-1025)check_lower(1077-1082)check_lower(1134-1139)check_lower(1249-1258)check_lower(1307-1318)check_lower(1357-1362)check_lower(1395-1400)check_lower(1433-1438)check_lower(1471-1476)check_lower(1491-1493)check_lower(1518-1520)lower(376-377)lower(410-413)lower(887-890)lower(1792-1793)lower(1814-1815)lower(1836-1837)lower(1858-1859)lower(1880-1881)lower(1902-1903)lower(1924-1925)lower(1946-1947)crates/superposition_core/src/ffi.rs (1)
ffi_parse_toml_config(181-184)
clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.kt (1)
clients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.kt (1)
uniffiRustCallWithError(266-271)
clients/python/bindings/test_toml_functions.py (2)
clients/python/bindings/superposition_bindings/superposition_client.py (1)
ffi_parse_toml_config(1633-1660)crates/superposition_core/src/ffi.rs (1)
ffi_parse_toml_config(181-184)
crates/context_aware_config/src/api/context/validations.rs (2)
crates/superposition_core/src/validations.rs (3)
validate_value_against_schema(18-30)schema(85-93)errors(82-82)crates/service_utils/src/helpers.rs (1)
validation_err_to_str(186-311)
clients/java/bindings/src/main/kotlin/uniffi/superposition_types/superposition_types.kt (1)
clients/python/bindings/superposition_bindings/superposition_types.py (1)
Config(929-952)
crates/superposition_core/tests/test_filter_debug.rs (3)
crates/superposition_core/src/lib.rs (1)
parse_toml_config(70-72)crates/superposition_core/src/toml_parser.rs (1)
serialize_to_toml(697-779)crates/superposition_types/src/config.rs (1)
get_key(323-325)
🪛 GitHub Actions: CI Checks on PRs
crates/superposition_core/src/toml_parser.rs
[error] 256-256: clippy: very complex type used. Consider factoring parts into type definitions. (TomlParser type_complexity)
[error] 651-651: clippy: redundant closure. Replace map(|v| value_to_toml(v)) with value_to_toml(v). (redundant_closure)
🪛 LanguageTool
design-docs/2026-01-02-toml-response-format-design.md
[style] ~578-~578: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... - Consider pagination or streaming for very large configs - Not a problem for initial imp...
(EN_WEAK_ADJECTIVE)
[style] ~1128-~1128: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...5. Streaming Serialization - For very large configs (> 10MB) - Stream TOML outpu...
(EN_WEAK_ADJECTIVE)
docs/plans/2026-01-02-toml-response-format.md
[grammar] ~676-~676: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: TOML formatted response with Content-Type: ap...
(QB_NEW_EN_HYPHEN)
[grammar] ~685-~685: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: JSON formatted response (existing behavior) ...
(QB_NEW_EN_HYPHEN)
[grammar] ~694-~694: Use a hyphen to join words.
Context: ...ocalhost:8080/config ``` Expected: JSON formatted response **Step 6: Create doc...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
clients/java/bindings/README_TOML_TESTS.md
6-6: Blank line inside blockquote
(MD028, no-blanks-blockquote)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
design-docs/2026-01-02-toml-response-format-design.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
429-429: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
441-441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1156-1156: Bare URL used
(MD034, no-bare-urls)
1157-1157: Bare URL used
(MD034, no-bare-urls)
1158-1158: Bare URL used
(MD034, no-bare-urls)
1163-1163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
clients/python/bindings/README_TOML_TESTS.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
design-docs/2025-12-21-toml-parsing-ffi-design.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1293-1293: Bare URL used
(MD034, no-bare-urls)
1294-1294: Bare URL used
(MD034, no-bare-urls)
1295-1295: Bare URL used
(MD034, no-bare-urls)
1296-1296: Bare URL used
(MD034, no-bare-urls)
1674-1674: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1675-1675: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1676-1676: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1677-1677: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1678-1678: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1679-1679: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1680-1680: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1681-1681: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1682-1682: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1683-1683: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1684-1684: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1685-1685: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1686-1686: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1687-1687: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1688-1688: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1689-1689: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1690-1690: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1691-1691: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1692-1692: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1693-1693: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1694-1694: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1695-1695: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1696-1696: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1697-1697: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1698-1698: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1699-1699: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1700-1700: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1701-1701: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1702-1702: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1703-1703: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1704-1704: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1705-1705: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1706-1706: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1707-1707: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1708-1708: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1709-1709: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1710-1710: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1711-1711: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1712-1712: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1713-1713: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1714-1714: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1715-1715: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1716-1716: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1842-1842: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
1860-1860: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.10)
clients/python/bindings/superposition_bindings/superposition_client.py
505-505: Avoid specifying long messages outside the exception class
(TRY003)
clients/python/bindings/superposition_bindings/superposition_types.py
1585-1585: Avoid specifying long messages outside the exception class
(TRY003)
1694-1694: Avoid specifying long messages outside the exception class
(TRY003)
1701-1701: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
1760-1760: Avoid specifying long messages outside the exception class
(TRY003)
1767-1767: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
clients/python/bindings/test_toml_functions.py
91-91: Consider moving this statement to an else block
(TRY300)
93-93: Do not catch blind exception: Exception
(BLE001)
118-118: f-string without any placeholders
Remove extraneous f prefix
(F541)
119-119: f-string without any placeholders
Remove extraneous f prefix
(F541)
125-125: Consider moving this statement to an else block
(TRY300)
127-127: Do not catch blind exception: Exception
(BLE001)
158-158: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
159-159: f-string without any placeholders
Remove extraneous f prefix
(F541)
160-160: Do not catch blind exception: Exception
(BLE001)
| parseTomlConfig :: String -> IO (Either String String) | ||
| parseTomlConfig tomlContent = do | ||
| ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation | ||
| tomlStr <- newCString tomlContent | ||
| res <- parse_toml_config tomlStr ebuf | ||
| err <- peekCAString ebuf | ||
| let peekMaybe p | p /= nullPtr = Just <$> peekCAString p | ||
| | otherwise = pure Nothing | ||
| result <- peekMaybe res | ||
| free tomlStr | ||
| free ebuf | ||
| pure $ case (result, err) of | ||
| (Just cfg, []) -> Right cfg | ||
| (Nothing, []) -> Left "null pointer returned" | ||
| _ -> Left err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the Haskell FFI file
find . -name "Superposition.hs" -type fRepository: juspay/superposition
Length of output: 129
🏁 Script executed:
# Once we have the file, let's examine the full context around parseTomlConfig and getResolvedConfig
cat -n clients/haskell/superposition-bindings/lib/FFI/Superposition.hs | head -150Repository: juspay/superposition
Length of output: 4951
🏁 Script executed:
# Search for memory management patterns in the Rust FFI layer
find . -path "*/ffi*" -name "*.rs" -type f | head -20Repository: juspay/superposition
Length of output: 148
🏁 Script executed:
# Look for parse_toml_config definition and how it manages memory
rg "parse_toml_config|core_parse_toml_config" --type rust -A 10Repository: juspay/superposition
Length of output: 7874
🏁 Script executed:
# Check for CString/buffer memory management patterns
rg "callocBytes|newCString|free.*Ptr|peekCAString" --type haskell -B 2 -A 2Repository: juspay/superposition
Length of output: 13443
🏁 Script executed:
cat -n crates/superposition_core/src/ffi_legacy.rs | grep -A 40 "pub unsafe extern \"C\" fn core_parse_toml_config"Repository: juspay/superposition
Length of output: 1619
🏁 Script executed:
# Also check for memory allocation patterns in the FFI layer
rg "Box::into_raw|CString|Box::leak" crates/superposition_core/src/ffi_legacy.rs -B 2 -A 2Repository: juspay/superposition
Length of output: 589
🏁 Script executed:
# Look at the header file to understand the FFI contract
find . -name "superposition_core.h" -o -name "*.h" | head -10Repository: juspay/superposition
Length of output: 127
🏁 Script executed:
# Check if core_free_string is being used anywhere in Haskell bindings
rg "free_string|core_free" clients/haskell/ -B 2 -A 2Repository: juspay/superposition
Length of output: 5836
🏁 Script executed:
# Look at the full core_parse_toml_config implementation to see what it returns
cat -n crates/superposition_core/src/ffi_legacy.rs | sed -n '478,560p'Repository: juspay/superposition
Length of output: 2730
🏁 Script executed:
# Check if there's a core_free_string FFI binding defined
rg "core_free_string" crates/superposition_core/src/ -B 2 -A 5Repository: juspay/superposition
Length of output: 1779
Fix critical memory leak and refactor duplicate code.
The result pointer from the Rust FFI functions is caller-owned and must be freed. In ffi_legacy.rs, core_parse_toml_config returns string_to_c_str(result_str), which uses CString::into_raw(), and the function documentation explicitly states: "Caller must free the returned string using core_free_string()". The same applies to core_get_resolved_config. Neither parseTomlConfig nor getResolvedConfig currently frees this pointer—this is a critical memory leak.
Required fixes:
- Import and bind
core_free_stringfrom the FFI layer (define:foreign import capi "superposition_core.h core_free_string" c_free_string :: CString -> IO ()) - Wrap result pointers using
newForeignPtr(pattern already used correctly inhs-exp-clientandhs-cac-client) - Extract
peekMaybeto module level to eliminate duplication (currently defined in both functions with identical implementations) - Unify cleanup patterns: use
traverse_ freeNonNullconsistently withparseTomlConfigfor maintainability
🤖 Prompt for AI Agents
In clients/haskell/superposition-bindings/lib/FFI/Superposition.hs around lines
118 to 132, the function parseTomlConfig does not free the caller-owned result
pointer returned from Rust (causing a critical leak) and duplicates the
peekMaybe helper; fix by importing and binding core_free_string from the FFI
(foreign import capi "superposition_core.h core_free_string" c_free_string ::
CString -> IO ()), then when you obtain the result pointer wrap it with
newForeignPtr c_free_string so the returned CString is freed by the finalizer
(or explicitly call c_free_string on non-null results), move the peekMaybe
function to module scope to remove duplication, and unify cleanup using
traverse_ freeNonNull (or similar) to free tomlStr, ebuf and the result pointer
consistently.
| /// Determine response format from Accept header | ||
| fn determine_response_format(req: &HttpRequest) -> ResponseFormat { | ||
| use actix_web::http::header; | ||
|
|
||
| let accept_header = req | ||
| .headers() | ||
| .get(header::ACCEPT) | ||
| .and_then(|h| h.to_str().ok()) | ||
| .unwrap_or("*/*"); | ||
|
|
||
| if accept_header.contains("application/toml") { | ||
| ResponseFormat::Toml | ||
| } else if accept_header.contains("application/json") { | ||
| ResponseFormat::Json | ||
| } else { | ||
| ResponseFormat::Json // Default to JSON for backwards compatibility | ||
| } | ||
| } |
There was a problem hiding this comment.
Use precise media type matching instead of substring matching.
Lines 78 and 80 use contains() to check for media types, which will incorrectly match partial strings. For example:
"application/toml-extended"would match line 78"my-application/toml"would match line 78"text/application/json"would match line 80
This could lead to incorrect response format selection when clients send non-standard or extended media types.
🔎 Proposed fix using exact media type matching
fn determine_response_format(req: &HttpRequest) -> ResponseFormat {
use actix_web::http::header;
let accept_header = req
.headers()
.get(header::ACCEPT)
.and_then(|h| h.to_str().ok())
.unwrap_or("*/*");
- if accept_header.contains("application/toml") {
+ // Split by comma to handle multiple media types
+ let media_types: Vec<&str> = accept_header.split(',')
+ .map(|s| s.trim().split(';').next().unwrap_or("").trim())
+ .collect();
+
+ if media_types.iter().any(|&mt| mt == "application/toml") {
ResponseFormat::Toml
- } else if accept_header.contains("application/json") {
+ } else if media_types.iter().any(|&mt| mt == "application/json" || mt == "*/*") {
ResponseFormat::Json
} else {
ResponseFormat::Json // Default to JSON for backwards compatibility
}
}📝 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.
| /// Determine response format from Accept header | |
| fn determine_response_format(req: &HttpRequest) -> ResponseFormat { | |
| use actix_web::http::header; | |
| let accept_header = req | |
| .headers() | |
| .get(header::ACCEPT) | |
| .and_then(|h| h.to_str().ok()) | |
| .unwrap_or("*/*"); | |
| if accept_header.contains("application/toml") { | |
| ResponseFormat::Toml | |
| } else if accept_header.contains("application/json") { | |
| ResponseFormat::Json | |
| } else { | |
| ResponseFormat::Json // Default to JSON for backwards compatibility | |
| } | |
| } | |
| /// Determine response format from Accept header | |
| fn determine_response_format(req: &HttpRequest) -> ResponseFormat { | |
| use actix_web::http::header; | |
| let accept_header = req | |
| .headers() | |
| .get(header::ACCEPT) | |
| .and_then(|h| h.to_str().ok()) | |
| .unwrap_or("*/*"); | |
| // Split by comma to handle multiple media types | |
| let media_types: Vec<&str> = accept_header.split(',') | |
| .map(|s| s.trim().split(';').next().unwrap_or("").trim()) | |
| .collect(); | |
| if media_types.iter().any(|&mt| mt == "application/toml") { | |
| ResponseFormat::Toml | |
| } else if media_types.iter().any(|&mt| mt == "application/json" || mt == "*/*") { | |
| ResponseFormat::Json | |
| } else { | |
| ResponseFormat::Json // Default to JSON for backwards compatibility | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/context_aware_config/src/api/config/handlers.rs around lines 68 to 85,
the code uses substring matching on the Accept header which can misclassify
media types; instead parse the Accept header into discrete media types and match
exact type/subtype pairs. Split the header on commas, parse each media type with
the mime crate (or mime::Mime::from_str), ignore parameters (q, charset) and
check mime.type_() == "application" && mime.subtype() == "toml" for TOML or
subtype == "json" for JSON (handle wildcards like "*/*" or "application/*" as
fallbacks); return the first exact match found, otherwise fall back to JSON.
Ensure you handle header::ACCEPT missing or invalid values by defaulting to
JSON.
| /// Convert JSON to deterministic sorted string for consistent hashing | ||
| fn json_to_sorted_string(v: &Value) -> String { | ||
| match v { | ||
| Value::Null => "null".to_string(), | ||
| Value::Bool(b) => b.to_string(), | ||
| Value::Number(n) => n.to_string(), | ||
| Value::String(s) => format!("\"{}\"", s), | ||
| Value::Array(arr) => { | ||
| let items: Vec<String> = arr.iter().map(json_to_sorted_string).collect(); | ||
| format!("[{}]", items.join(",")) | ||
| } | ||
| Value::Object(map) => { | ||
| let items: Vec<String> = map | ||
| .iter() | ||
| .sorted_by_key(|(k, _)| *k) | ||
| .map(|(k, v)| format!("\"{}\":{}", k, json_to_sorted_string(v))) | ||
| .collect(); | ||
| format!("{{{}}}", items.join(",")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
String values are not escaped, causing potential hash inconsistencies.
On line 144, strings are formatted without escaping special characters. If a string contains quotes, backslashes, or newlines, the output will be malformed, potentially causing hash collisions or inconsistent hashes across equivalent values.
🔎 Proposed fix
fn json_to_sorted_string(v: &Value) -> String {
match v {
Value::Null => "null".to_string(),
Value::Bool(b) => b.to_string(),
Value::Number(n) => n.to_string(),
- Value::String(s) => format!("\"{}\"", s),
+ Value::String(s) => format!("\"{}\"", s.replace('\\', "\\\\").replace('"', "\\\"").replace('\n', "\\n").replace('\r', "\\r").replace('\t', "\\t")),
Value::Array(arr) => {
let items: Vec<String> = arr.iter().map(json_to_sorted_string).collect();
format!("[{}]", items.join(","))
}📝 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.
| /// Convert JSON to deterministic sorted string for consistent hashing | |
| fn json_to_sorted_string(v: &Value) -> String { | |
| match v { | |
| Value::Null => "null".to_string(), | |
| Value::Bool(b) => b.to_string(), | |
| Value::Number(n) => n.to_string(), | |
| Value::String(s) => format!("\"{}\"", s), | |
| Value::Array(arr) => { | |
| let items: Vec<String> = arr.iter().map(json_to_sorted_string).collect(); | |
| format!("[{}]", items.join(",")) | |
| } | |
| Value::Object(map) => { | |
| let items: Vec<String> = map | |
| .iter() | |
| .sorted_by_key(|(k, _)| *k) | |
| .map(|(k, v)| format!("\"{}\":{}", k, json_to_sorted_string(v))) | |
| .collect(); | |
| format!("{{{}}}", items.join(",")) | |
| } | |
| } | |
| } | |
| /// Convert JSON to deterministic sorted string for consistent hashing | |
| fn json_to_sorted_string(v: &Value) -> String { | |
| match v { | |
| Value::Null => "null".to_string(), | |
| Value::Bool(b) => b.to_string(), | |
| Value::Number(n) => n.to_string(), | |
| Value::String(s) => format!("\"{}\"", s.replace('\\', "\\\\").replace('"', "\\\"").replace('\n', "\\n").replace('\r', "\\r").replace('\t', "\\t")), | |
| Value::Array(arr) => { | |
| let items: Vec<String> = arr.iter().map(json_to_sorted_string).collect(); | |
| format!("[{}]", items.join(",")) | |
| } | |
| Value::Object(map) => { | |
| let items: Vec<String> = map | |
| .iter() | |
| .sorted_by_key(|(k, _)| *k) | |
| .map(|(k, v)| format!("\"{}\":{}", k, json_to_sorted_string(v))) | |
| .collect(); | |
| format!("{{{}}}", items.join(",")) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
The JSON in our case is going to be an Object which is Map<String, Value>. Can you get consistent hashing just by sorting on Map keys?
There was a problem hiding this comment.
@Datron, while your top-level structure is always a Map<String, Value>, sorting just the top-level keys isn't sufficient for consistent hashing. Here's why:
The values in your Map can contain:
- Nested objects – their keys also need deterministic ordering
- Arrays containing objects – those objects need key sorting too
- Strings with special characters – need escaping at any nesting level
Example where it breaks without proper handling:
// Two semantically identical configs:
{
"feature": "{\"enabled\":true}", // String containing JSON
"nested": {"b": 2, "a": 1} // Nested object
}
{
"feature": "{\"enabled\":true}", // Same string (but with quote inside)
"nested": {"a": 1, "b": 2} // Same object, different key order
}Without the recursive approach:
- The nested object
{"b": 2, "a": 1}vs{"a": 1, "b": 2}would hash differently - A string like
"value with \"quotes\""would produce malformed output without escaping
The current implementation needs both:
- ✅ Recursive key sorting (already present) – handles nested objects
- ❌ String escaping (missing) – prevents malformed representations
The original suggestion to add string escaping is still valid and necessary for hash consistency, even though your root is always an Object.
| /// Compute priority based on dimension positions (bit-shift calculation) | ||
| fn compute_priority( | ||
| context_map: &Map<String, Value>, | ||
| dimensions: &HashMap<String, DimensionInfo>, | ||
| ) -> i32 { | ||
| context_map | ||
| .keys() | ||
| .filter_map(|key| dimensions.get(key)) | ||
| .map(|dim_info| 1 << dim_info.position) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/superposition_core/src && find . -name "toml_parser.rs" -type fRepository: juspay/superposition
Length of output: 80
🏁 Script executed:
grep -n "struct DimensionInfo" crates/superposition_core/src/toml_parser.rs | head -20Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
grep -n "position" crates/superposition_core/src/toml_parser.rs | head -30Repository: juspay/superposition
Length of output: 1745
🏁 Script executed:
sed -n '1,200p' crates/superposition_core/src/toml_parser.rs | head -100Repository: juspay/superposition
Length of output: 3438
🏁 Script executed:
grep -n "position" crates/superposition_core/src/toml_parser.rs | grep -E "(validate|check|bound|limit|max|min)" | head -20Repository: juspay/superposition
Length of output: 267
🏁 Script executed:
# Check for position validation logic
sed -n '340,370p' crates/superposition_core/src/toml_parser.rsRepository: juspay/superposition
Length of output: 1059
🏁 Script executed:
# Look for DimensionInfo in superposition_types
find . -name "*.rs" -type f -exec grep -l "struct DimensionInfo" {} \;Repository: juspay/superposition
Length of output: 162
🏁 Script executed:
# Check the DimensionInfo struct definition
grep -A 10 "struct DimensionInfo" crates/superposition_types/src/lib.rs 2>/dev/null || grep -r "struct DimensionInfo" --include="*.rs" | head -5Repository: juspay/superposition
Length of output: 301
🏁 Script executed:
# Check for any documentation or comments about position constraints
grep -B 3 -A 3 "position.*i32" crates/superposition_core/src/toml_parser.rs | head -20Repository: juspay/superposition
Length of output: 525
🏁 Script executed:
sed -n '1,50p' crates/superposition_types/src/config.rs | grep -A 20 "struct DimensionInfo"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for actual usage patterns and practical limits
grep -n "position" crates/superposition_core/src/toml_parser.rs | grep -E "(test|example|dimension)" | head -30Repository: juspay/superposition
Length of output: 782
🏁 Script executed:
# Check test cases to understand practical dimension counts
grep -B 5 -A 5 "position = " crates/superposition_core/src/toml_parser.rs | tail -50Repository: juspay/superposition
Length of output: 1296
🏁 Script executed:
# Search for any maximum dimension or position constants
rg "MAX.*DIMENSION|MAX.*POSITION" crates/ --type rsRepository: juspay/superposition
Length of output: 90
🏁 Script executed:
# Check how compute_priority is used and if there are any real constraints
rg "compute_priority" crates/ --type rs -B 3 -A 3Repository: juspay/superposition
Length of output: 90
🏁 Script executed:
grep -r "compute_priority" crates/ --include="*.rs" -B 3 -A 3Repository: juspay/superposition
Length of output: 2058
🏁 Script executed:
# Look at the full DimensionInfo struct definition
sed -n '1,100p' crates/superposition_types/src/config.rs | grep -A 10 "struct DimensionInfo"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check for any practical examples with multiple dimensions
grep -r "position = [0-9]" crates/ --include="*.rs" | sed 's/.*position = /position = /' | sort | uniq -c | sort -rn | head -20Repository: juspay/superposition
Length of output: 1331
🏁 Script executed:
# Search for any documentation about dimension limits
grep -r "dimension" crates/superposition_core/README.md crates/README.md 2>/dev/null | head -10Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there are integration tests with many dimensions
grep -B 20 "position = " crates/superposition_core/src/toml_parser.rs | grep -E "fn test_|#\[test\]" | head -10Repository: juspay/superposition
Length of output: 336
🏁 Script executed:
# Check the actual data flow - how are positions assigned?
sed -n '310,380p' crates/superposition_core/src/toml_parser.rsRepository: juspay/superposition
Length of output: 2459
🏁 Script executed:
# Look for architectural documentation or design decisions about dimensions
find . -name "*.md" -type f | xargs grep -l "dimension" 2>/dev/null | head -5Repository: juspay/superposition
Length of output: 252
🏁 Script executed:
# Check if there are constraints on how many dimensions can exist
grep -r "dimension" crates/superposition_core/README.md crates/README.md 2>/dev/null || echo "No READMEs found"Repository: juspay/superposition
Length of output: 80
🏁 Script executed:
# Look at the actual cac_toml compute_priority to compare approaches
sed -n '1,50p' crates/cac_toml/src/lib.rs | grep -A 15 "fn compute_priority"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there are any tests that actually validate dimension behavior with multiple positions
sed -n '1200,1400p' crates/superposition_core/src/toml_parser.rsRepository: juspay/superposition
Length of output: 5956
🏁 Script executed:
# Check if there's any documentation about position constraints
find . -name "*.md" -type f -exec grep -l "position\|dimension" {} \; | head -10Repository: juspay/superposition
Length of output: 2505
🏁 Script executed:
# Look for any config examples in documentation
find . -name "*.toml" -o -name "*.md" | xargs grep -l "position" 2>/dev/null | head -5Repository: juspay/superposition
Length of output: 197
🏁 Script executed:
# Check the actual implementation of compute_priority more carefully
sed -n '166,180p' crates/superposition_core/src/toml_parser.rsRepository: juspay/superposition
Length of output: 517
🏁 Script executed:
# See if position is ever validated or documented anywhere else
grep -r "position.*31\|position.*30\|MAX_POSITION\|maximum.*position" crates/ --include="*.rs" --include="*.md"Repository: juspay/superposition
Length of output: 1188
🏁 Script executed:
# Check if i32 was a deliberate choice or if there's i64 version
grep -r "compute_priority" crates/ --include="*.rs" -A 10 | grep -E "(i32|i64|u32|u64)"Repository: juspay/superposition
Length of output: 231
Add validation for dimension position bounds to prevent integer overflow.
The bit-shift operation 1 << dim_info.position will overflow if position >= 31 (for i32). Although position values are currently unconstrained during parsing and all existing examples use positions 1-2, adding explicit validation is prudent:
Suggested fix
+const MAX_DIMENSION_POSITION: i32 = 30;
+
fn compute_priority(
context_map: &Map<String, Value>,
dimensions: &HashMap<String, DimensionInfo>,
) -> i32 {
context_map
.keys()
.filter_map(|key| dimensions.get(key))
+ .filter(|dim_info| dim_info.position <= MAX_DIMENSION_POSITION)
.map(|dim_info| 1 << dim_info.position)
.sum()
}Alternatively, consider switching to i64 or u64 to safely support a larger range of positions, consistent with the similar compute_priority implementation in cac_toml.
📝 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.
| /// Compute priority based on dimension positions (bit-shift calculation) | |
| fn compute_priority( | |
| context_map: &Map<String, Value>, | |
| dimensions: &HashMap<String, DimensionInfo>, | |
| ) -> i32 { | |
| context_map | |
| .keys() | |
| .filter_map(|key| dimensions.get(key)) | |
| .map(|dim_info| 1 << dim_info.position) | |
| .sum() | |
| } | |
| /// Compute priority based on dimension positions (bit-shift calculation) | |
| const MAX_DIMENSION_POSITION: i32 = 30; | |
| fn compute_priority( | |
| context_map: &Map<String, Value>, | |
| dimensions: &HashMap<String, DimensionInfo>, | |
| ) -> i32 { | |
| context_map | |
| .keys() | |
| .filter_map(|key| dimensions.get(key)) | |
| .filter(|dim_info| dim_info.position <= MAX_DIMENSION_POSITION) | |
| .map(|dim_info| 1 << dim_info.position) | |
| .sum() | |
| } |
knutties
force-pushed
the
superposition-toml
branch
from
fb4d832 to
7883aeb
Compare
Add comprehensive design document for implementing TOML response format support in the get_config API endpoint through HTTP content negotiation. Key features: - Accept header-based content negotiation - Round-trip compatible TOML serialization - Backwards compatible with existing JSON clients - Support for both GET and POST requests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…riants - Rename error enum for broader scope (parse + serialize) - Add SerializationError and InvalidContextCondition variants - Update Display implementation for new variants - Add Serialize/Deserialize derives to error enum - Update lib.rs exports to use TomlError and ParsedTomlConfig alias 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add value_to_toml for converting JSON values to TOML strings - Add condition_to_string for context expressions - Add value_to_string_simple for simple value formatting - Include comprehensive test coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implements the serialize_to_toml function that converts a Config structure back to TOML format. The function: - Serializes [default-config] section with inferred schema types - Serializes [dimensions] section sorted by position - Serializes [context.*] sections with condition strings and override values Includes round-trip test to verify serialization produces valid TOML that can be parsed back to an equivalent Config. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add ResponseFormat enum for JSON/TOML formats - Implement determine_response_format function for Accept header parsing - Modify get_config handler to support content negotiation - Add superposition_core dependency to context_aware_config - Fix error handling to use AppError::UnexpectedError 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added 4 new tests to the serialization_tests module: - test_toml_round_trip_empty_config: Tests parsing and serializing configs with empty default-config - test_value_to_toml_special_chars: Tests proper escaping of quotes and backslashes in strings - test_toml_round_trip_all_value_types: Tests round-trip serialization for string, integer, float, and boolean types - test_value_to_toml_nested: Tests serialization of nested JSON objects 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add docs/api/toml-response-format.md with usage examples - Update design-docs/2026-01-02-toml-response-format-design.md - Change status to "Implemented" - Add implementation notes with commit references - Document key implementation details and test coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use IntoIterator trait to iterate over Overrides instead of trying to access private .0 field. This ensures context sections include their override values in the TOML output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update both test modules (serialization_tests and tests) to only run when the jsonlogic feature is disabled using #[cfg(all(test, not(feature = "jsonlogic")))]. This prevents test conflicts when the jsonlogic feature is enabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ion_core Moves the validation_err_to_str function from service_utils to superposition_core to consolidate all JSON schema validation logic and make superposition_core independent. This removes the service_utils dependency from superposition_core. Updated imports in context_aware_config to use the new location. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…from_index everywhere Removes the redundant calculate_priority_from_index function and updates toml_parser.rs to use calculate_weight_from_index with i32 conversion via ToPrimitive trait. This consolidates weight/priority calculation into a single function that can be used across all crates. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds URL-encoding (percent-encoding) for dimension keys and values in context strings to handle special characters like '=' and ';' that have special meaning in the TOML context syntax. Changes: - Added percent-encoding dependency - Updated parse_context_expression to URL-decode keys and values - Updated condition_to_string to URL-encode keys and values - Added needs_quoting helper to properly quote dimension names with special characters during serialization - Added tests for URL-encoding round-trip with special characters This allows dimensions and values to contain characters that were previously reserved for parsing (e.g., a dimension named "key=with=equals" with value "value;with;semicolon"). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
knutties
force-pushed
the
superposition-toml
branch
from
8f00cb0 to
27c885c
Compare
Apply needs_quoting to default-config keys and override keys inside contexts to properly handle special characters like '=', ';', '.', whitespace, etc. during TOML serialization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Object type values in default-config and context overrides are now
serialized as triple-quoted JSON strings for better readability and
unambiguous parsing. This fixes issues with nested inline tables in
TOML.
Changes:
- Updated value_to_toml to serialize Object values as triple-quoted JSON
- Added schema_to_toml for schema serialization (uses inline TOML format)
- Added parse_toml_string_as_json to parse triple-quoted JSON strings
- Updated parse_default_config to detect object type and parse as JSON
- Updated parse_contexts to detect object type in overrides and parse as JSON
- Added test_object_value_round_trip to verify round-trip functionality
Example output:
[default-config]
config = { value = '''
{
"host": "localhost",
"port": 8080
}
''', schema = { type = "object" } }
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
knutties
force-pushed
the
superposition-toml
branch
from
aeeace5 to
ae75143
Compare
| *.dylib | ||
| *.so | ||
| *.dll |
There was a problem hiding this comment.
For testing, should we link to libraries in target dir?
| -- - contexts_json: JSON string containing array of context objects | ||
| -- - overrides_json: JSON string containing object mapping hashes to override configurations | ||
| -- - dimensions_json: JSON string containing object mapping dimension names to dimension info | ||
| parseTomlConfig :: String -> IO (Either String String) |
There was a problem hiding this comment.
If we're returning an object, can we parse it to Aeson.Value and return instead of String - without this the user would need to parse the config
| ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation | ||
| tomlStr <- newCString tomlContent | ||
| res <- parse_toml_config tomlStr ebuf | ||
| err <- peekCAString ebuf |
There was a problem hiding this comment.
Use peekCString which supports utf8 (for emojis)
| parseTomlConfig :: String -> IO (Either String String) | ||
| parseTomlConfig tomlContent = do | ||
| ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation | ||
| tomlStr <- newCString tomlContent | ||
| res <- parse_toml_config tomlStr ebuf | ||
| err <- peekCAString ebuf | ||
| let peekMaybe p | p /= nullPtr = Just <$> peekCAString p | ||
| | otherwise = pure Nothing | ||
| result <- peekMaybe res | ||
| free tomlStr | ||
| free ebuf | ||
| pure $ case (result, err) of | ||
| (Just cfg, []) -> Right cfg | ||
| (Nothing, []) -> Left "null pointer returned" | ||
| _ -> Left err |
There was a problem hiding this comment.
Create a new ForeignPtr that has a finalizer, and then use withForeignPtr
https://hackage-content.haskell.org/package/base-4.22.0.0/docs/Foreign-ForeignPtr.html#v:withForeignPtr
|
|
||
| /** | ||
| * Parse TOML configuration string | ||
| * | ||
| * # Arguments | ||
| * * `toml_content` - TOML string with configuration | ||
| * | ||
| * # Returns | ||
| * * `Ok(Config)` - Parsed configuration with all components | ||
| * * `Err(OperationError)` - Detailed error message | ||
| * | ||
| * # Example TOML | ||
| * ```toml | ||
| * [default-config] | ||
| * timeout = { value = 30, schema = { type = "integer" } } | ||
| * | ||
| * [dimensions] | ||
| * os = { position = 1, schema = { type = "string" } } | ||
| * | ||
| * [context."os=linux"] | ||
| * timeout = 60 | ||
| * ``` | ||
| */ | ||
| @Throws(OperationException::class) fun `ffiParseTomlConfig`(`tomlContent`: kotlin.String): Config { | ||
| return FfiConverterTypeConfig.lift( | ||
| uniffiRustCallWithError(OperationException) { _status -> | ||
| UniffiLib.INSTANCE.uniffi_superposition_core_fn_func_ffi_parse_toml_config( | ||
| FfiConverterString.lower(`tomlContent`),_status) |
There was a problem hiding this comment.
@knutties this should be generated by uniFFI, no need to manually code this
There was a problem hiding this comment.
This is a guess, based on the code comments
| fn parse_toml_string_as_json(toml_value: toml::Value) -> Value { | ||
| match toml_value { | ||
| toml::Value::String(s) => { | ||
| // Try to parse as JSON | ||
| serde_json::from_str(&s).unwrap_or(Value::String(s)) | ||
| } | ||
| _ => toml_value_to_serde_value(toml_value), | ||
| } | ||
| } |
There was a problem hiding this comment.
Can be replaced with:
serde_json::to_string_pretty(&toml_value)
| /// Convert JSON to deterministic sorted string for consistent hashing | ||
| fn json_to_sorted_string(v: &Value) -> String { | ||
| match v { | ||
| Value::Null => "null".to_string(), | ||
| Value::Bool(b) => b.to_string(), | ||
| Value::Number(n) => n.to_string(), | ||
| Value::String(s) => format!("\"{}\"", s), | ||
| Value::Array(arr) => { | ||
| let items: Vec<String> = arr.iter().map(json_to_sorted_string).collect(); | ||
| format!("[{}]", items.join(",")) | ||
| } | ||
| Value::Object(map) => { | ||
| let items: Vec<String> = map | ||
| .iter() | ||
| .sorted_by_key(|(k, _)| *k) | ||
| .map(|(k, v)| format!("\"{}\":{}", k, json_to_sorted_string(v))) | ||
| .collect(); | ||
| format!("{{{}}}", items.join(",")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The JSON in our case is going to be an Object which is Map<String, Value>. Can you get consistent hashing just by sorting on Map keys?
| continue; | ||
| } | ||
|
|
||
| let parts: Vec<&str> = pair.splitn(2, '=').collect(); |
There was a problem hiding this comment.
Since this is always key and value in superposition v2, should it be a tuple here?
|
|
||
| // Second pass: parse dimension types and validate schemas based on type | ||
| for (key, value) in section { | ||
| let table = value.as_table().unwrap(); |
There was a problem hiding this comment.
Conversion error missing here
| fn value_to_toml(value: &Value) -> String { | ||
| match value { | ||
| Value::String(s) => { | ||
| let escaped = s | ||
| .replace('\\', "\\\\") | ||
| .replace('"', "\\\"") | ||
| .replace('\n', "\\n") | ||
| .replace('\r', "\\r") | ||
| .replace('\t', "\\t"); | ||
| format!("\"{}\"", escaped) | ||
| } | ||
| Value::Number(n) => n.to_string(), | ||
| Value::Bool(b) => b.to_string(), | ||
| Value::Array(arr) => { | ||
| let items: Vec<String> = arr.iter().map(value_to_toml).collect(); | ||
| format!("[{}]", items.join(", ")) | ||
| } | ||
| Value::Object(obj) => { | ||
| // Serialize object as JSON in triple-quoted string for readability | ||
| let json_str = | ||
| serde_json::to_string(obj).unwrap_or_else(|_| "{}".to_string()); | ||
| // Pretty-print JSON for better readability | ||
| let pretty_json = | ||
| serde_json::to_string_pretty(obj).unwrap_or_else(|_| json_str.clone()); | ||
| format!("'''\n{}'''", pretty_json) | ||
| } | ||
| Value::Null => "null".to_string(), | ||
| } | ||
| } | ||
|
|
||
| /// Convert serde_json::Value to TOML representation string for schemas | ||
| /// Schemas use inline TOML tables for objects (not triple-quoted JSON) | ||
| fn schema_to_toml(value: &Value) -> String { | ||
| match value { | ||
| Value::String(s) => { | ||
| let escaped = s | ||
| .replace('\\', "\\\\") | ||
| .replace('"', "\\\"") | ||
| .replace('\n', "\\n") | ||
| .replace('\r', "\\r") | ||
| .replace('\t', "\\t"); | ||
| format!("\"{}\"", escaped) | ||
| } | ||
| Value::Number(n) => n.to_string(), | ||
| Value::Bool(b) => b.to_string(), |
There was a problem hiding this comment.
You don't need these functions, use the serde converters directly
| -@pkill -f $(CARGO_TARGET_DIR)/debug/superposition | ||
|
|
||
| # Target to run all TOML bindings tests | ||
| bindings-test: uniffi-bindings |
There was a problem hiding this comment.
| bindings-test: uniffi-bindings | |
| toml-bindings-test: uniffi-bindings |
| superposition_derives = { workspace = true } | ||
| superposition_core = { workspace = true } |
There was a problem hiding this comment.
| superposition_derives = { workspace = true } | |
| superposition_core = { workspace = true } | |
| superposition_core = { workspace = true } | |
| superposition_derives = { workspace = true } |
| let mut result_config = (*config.default_configs).clone(); | ||
| merge_overrides_on_default_config(&mut result_config, overrides_map, &merge_strategy); | ||
|
|
||
| Ok(result_config) |
There was a problem hiding this comment.
we can avoid cloning in all of the places, we need to implement a function like this for ExtendedMap
impl ExtendedMap {
pub fn into_inner(self) -> Map<String, Value> {
self.0
}
}optionally we should add this function as well
impl ExtendedMap {
pub fn inner(&self) -> &Map<String, Value> {
&self.0
}
}and then the current code can become something like this
| let mut result_config = (*config.default_configs).clone(); | |
| merge_overrides_on_default_config(&mut result_config, overrides_map, &merge_strategy); | |
| Ok(result_config) | |
| let mut result_config = config.default_configs; | |
| merge_overrides_on_default_config(&mut result_config, overrides_map, &merge_strategy); | |
| Ok(result_config.into_inner()) |
we need to carry out a similar change/refactor in other places as well for the other custom types, to avoid unnecessay clones
| let mut result_config = (*config.default_configs).clone(); | ||
| merge_overrides_on_default_config(&mut result_config, overrides_map, &merge_strategy); |
There was a problem hiding this comment.
same would be applicable here as well, revert this section to original code, and just while returning,
Ok(result_config.into_inner())| contexts: filtered_context, | ||
| overrides: filtered_overrides, | ||
| default_configs: filtered_default_config, | ||
| default_configs: ExtendedMap(filtered_default_config), |
There was a problem hiding this comment.
instead of changing the type over here, I think we should do it in filter_default_by_prefix fn itself
changing that fucntion to something like:
pub fn filter_default_by_prefix(&self, prefix_list: &HashSet<String>) -> ExtendedMap {
filter_config_keys_by_prefix(&self.default_configs, prefix_list).into()
}| assert_eq!( | ||
| filter_config_keys_by_prefix(&config.default_configs, &prefix_list), | ||
| get_prefix_filtered_config1().default_configs | ||
| (*get_prefix_filtered_config1().default_configs).clone() |
There was a problem hiding this comment.
these can also take leverage of into_inner
| result.insert( | ||
| "default_configs".to_string(), | ||
| Value::Object(config.default_configs.clone()), | ||
| Value::Object((*config.default_configs).clone()), |
There was a problem hiding this comment.
this function is very weird, all the input config's fields are entirely cloned inside over here, there is no point in borrowing the ipnut, it should have taken the entire ownership itself
| pub use ffi_legacy::{ | ||
| core_free_string, core_get_resolved_config, core_get_resolved_config_with_reasoning, | ||
| }; | ||
| pub use superposition_types::Config as ParsedTomlConfig; |
| @@ -0,0 +1,1860 @@ | |||
| # TOML Parsing FFI Interface Design | |||
There was a problem hiding this comment.
what is this doc intended for ?
is this supposed to be published to github pages ?
because I dont think this folder is currently being published, correct me if I am wrong here
| buildInputs = | ||
| [ | ||
| pkgs.openssl | ||
| pkgs.postgresql_15 |
There was a problem hiding this comment.
this should not have psql dependencies, no ?
There was a problem hiding this comment.
can we revert the formatting changes in the file, not sure if it will impact its application
There was a problem hiding this comment.
I think this file needs to rebased correctly, this has been deleted
| _ => dimension_schema.validate(dimension_value).map_err(|e| { | ||
| let verrors = e.collect::<Vec<ValidationError>>(); | ||
| // Use shared validation from superposition_core for simple cases | ||
| #[cfg(not(feature = "jsonlogic"))] |
There was a problem hiding this comment.
this is repeating the code unnecessarily, I dont think any change in this function was required
| "required": ["type", "enum", "definitions"] | ||
| }); | ||
|
|
||
| JSONSchema::options() |
There was a problem hiding this comment.
can we use the newly created compile_schema fn here
| "required": ["type"], | ||
| }); | ||
|
|
||
| JSONSchema::options() |
There was a problem hiding this comment.
can we use the newly created compile_schema fn here
can we do the same for the other places as well, currently, compile_schema seems to be used in one place only
|
|
||
| let enum_options = validate_cohort_jsonschema(cohort_schema)?; | ||
| log::error!( | ||
| "##### checking validate_cohort_schema_structure {}", |
There was a problem hiding this comment.
should we better format these logs, if we at all want to keep them (both this one and the following one) ?
| /// Compile a JSON schema for validation | ||
| /// | ||
| /// # Arguments | ||
| /// * `schema` - The JSON schema to compile | ||
| /// | ||
| /// # Returns | ||
| /// * `Ok(JSONSchema)` - Compiled schema ready for validation | ||
| /// * `Err(String)` - Compilation error message | ||
| pub fn compile_schema(schema: &Value) -> Result<JSONSchema, String> { | ||
| JSONSchema::options() | ||
| .with_draft(Draft::Draft7) | ||
| .compile(schema) | ||
| .map_err(|e| e.to_string()) | ||
| } | ||
|
|
||
| /// Validate a value against a pre-compiled JSON schema | ||
| /// | ||
| /// # Arguments | ||
| /// * `value` - The value to validate | ||
| /// * `compiled_schema` - The pre-compiled JSONSchema | ||
| /// | ||
| /// # Returns | ||
| /// * `Ok(())` if validation succeeds | ||
| /// * `Err(Vec<String>)` containing validation error messages | ||
| pub fn validate_against_compiled_schema( | ||
| value: &Value, | ||
| compiled_schema: &JSONSchema, | ||
| ) -> Result<(), Vec<String>> { | ||
| compiled_schema | ||
| .validate(value) | ||
| .map_err(|errors| errors.map(|e| e.to_string()).collect()) | ||
| } | ||
|
|
||
| /// Validate a value against a raw JSON schema (compiles and validates) | ||
| /// | ||
| /// This is a convenience function that combines compilation and validation. | ||
| /// Use this when you don't need to distinguish between compilation and validation errors. | ||
| /// | ||
| /// # Arguments | ||
| /// * `value` - The value to validate | ||
| /// * `schema` - The JSON schema to validate against | ||
| /// | ||
| /// # Returns | ||
| /// * `Ok(())` if validation succeeds | ||
| /// * `Err(Vec<String>)` containing all error messages (compilation + validation) | ||
| pub fn validate_against_schema(value: &Value, schema: &Value) -> Result<(), Vec<String>> { | ||
| let compiled_schema = compile_schema(schema).map_err(|e| vec![e])?; | ||
| validate_against_compiled_schema(value, &compiled_schema) | ||
| } | ||
|
|
||
| /// Validate that a JSON schema is well-formed | ||
| /// | ||
| /// This function checks that a schema can be compiled and passes basic | ||
| /// structural validation against a meta-schema. | ||
| /// | ||
| /// # Arguments | ||
| /// * `schema` - The JSON schema to validate | ||
| /// | ||
| /// # Returns | ||
| /// * `Ok(())` if the schema is valid | ||
| /// * `Err(Vec<String>)` containing validation error messages | ||
| pub fn validate_schema(schema: &Value) -> Result<(), Vec<String>> { | ||
| // Use the new compile function | ||
| compile_schema(schema).map_err(|e| vec![e])?; | ||
|
|
||
| // Then validate against the meta-schema | ||
| let meta_schema = get_meta_schema(); | ||
| meta_schema | ||
| .validate(schema) | ||
| .map_err(|errors| errors.map(|e| e.to_string()).collect()) | ||
| } | ||
|
|
There was a problem hiding this comment.
are we sure, we need these functions ?
not able to verify there usage properly
| /// # Returns | ||
| /// * `Ok(())` if the schema structure is valid | ||
| /// * `Err(Vec<String>)` containing validation error messages | ||
| pub fn validate_cohort_schema_structure(schema: &Value) -> Result<(), Vec<String>> { |
There was a problem hiding this comment.
why do we need this manual check of all the fields ?
JSONSchema validation was already doing the exact same thing
ayushjain17
left a comment
ayushjain17
left a comment
There was a problem hiding this comment.
toml parsing core logic and client side change review pending
There was a problem hiding this comment.
there are direct unwrap usage in the this file - 4 instance other than usage in tests (test ones are fine)
There was a problem hiding this comment.
an intermediate type would have helped a lot, serde would have handled 90% of the work, we would have needed effort only for 10%
also, the top level fields do not have the key names consistent with the get config's fields
then something like this could have been done:
let value: Config = toml::from_str(FILE).unwrap();
println!("{}", serde_json::to_string_pretty(&value).unwrap());There was a problem hiding this comment.
the intermediate Config could be something like:
// for DimensionInfo - existing one can be reused
struct DefaultConfigInfo {
schema: ExtendedMap, // or Map<String, Value>
value: serde_json::Value,
}
// the key for contexts map would have to be pre-processed => (key, value) -> sort -> stringify with custom logic using `=` and `;`
struct TomlConfig {
contexts: HashMap<String, ExtendedMap>, // or HashMap<String, Map<String, Value>>,
default_config: HashMap<String, DefaultConfigInfo>,
dimensions: HashMap<String, DimensionInfo>,
}
| // Create result with default_config as Map and others as JSON strings | ||
| let result = serde_json::json!({ | ||
| "default_config": &*parsed.default_configs, | ||
| "contexts_json": parsed.contexts, | ||
| "overrides_json": parsed.overrides, | ||
| "dimensions_json": parsed.dimensions, | ||
| }); |
There was a problem hiding this comment.
I dont think we are doing what this comment is suggesting
| // Create result with default_config as Map and others as JSON strings | ||
| let result = serde_json::json!({ | ||
| "default_config": &*parsed.default_configs, | ||
| "contexts_json": parsed.contexts, | ||
| "overrides_json": parsed.overrides, | ||
| "dimensions_json": parsed.dimensions, | ||
| }); | ||
|
|
||
| let result_str = match serde_json::to_string(&result) { |
There was a problem hiding this comment.
we could have simply done this
| // Create result with default_config as Map and others as JSON strings | |
| let result = serde_json::json!({ | |
| "default_config": &*parsed.default_configs, | |
| "contexts_json": parsed.contexts, | |
| "overrides_json": parsed.overrides, | |
| "dimensions_json": parsed.dimensions, | |
| }); | |
| let result_str = match serde_json::to_string(&result) { | |
| let result_str = match serde_json::to_string(&config) { |
| /// Detailed error type for TOML parsing and serialization | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub enum TomlError { | ||
| FileReadError(String), |
There was a problem hiding this comment.
an intermediate type would have helped a lot, serde would have handled 90% of the work, we would have needed effort only for 10%
also, the top level fields do not have the key names consistent with the get config's fields
then something like this could have been done:
let value: Config = toml::from_str(FILE).unwrap();
println!("{}", serde_json::to_string_pretty(&value).unwrap());
| let schema = match value { | ||
| Value::String(_) => r#"{ type = "string" }"#, | ||
| Value::Number(n) => { | ||
| if n.is_i64() { | ||
| r#"{ type = "integer" }"# | ||
| } else { | ||
| r#"{ type = "number" }"# | ||
| } | ||
| } | ||
| Value::Bool(_) => r#"{ type = "boolean" }"#, | ||
| Value::Array(_) => r#"{ type = "array" }"#, | ||
| Value::Object(_) => r#"{ type = "object" }"#, | ||
| Value::Null => r#"{ type = "null" }"#, | ||
| }; |
There was a problem hiding this comment.
our input to this function should have been something else
inferring types like this would lead to loss of all the info someone would have saved in their default config's schema
There was a problem hiding this comment.
the intermediate Config could be something like:
// for DimensionInfo - existing one can be reused
struct DefaultConfigInfo {
schema: ExtendedMap, // or Map<String, Value>
value: serde_json::Value,
}
// the key for contexts map would have to be pre-processed => (key, value) -> sort -> stringify with custom logic using `=` and `;`
struct TomlConfig {
contexts: HashMap<String, ExtendedMap>, // or HashMap<String, Map<String, Value>>,
default_config: HashMap<String, DefaultConfigInfo>,
dimensions: HashMap<String, DimensionInfo>,
}
| || c == '\'' | ||
| || c == '.' | ||
| }) | ||
| } |
There was a problem hiding this comment.
TOML spec mentions that bare keys can only contain:
ASCII letters: A-Z, a-z
ASCII digits: 0-9
ASCII underscore: _
ASCII hyphen: -
Reference: https://toml.io/en/v1.1.0 , keys section.
Example of unicode edge cases :
-
"café" - actually it should return false but the code will return true.
-
"key＝value" - imagine using a full width = , not the ascii one
Fullwidth ＝ (U+FF1D)
this will also return false but it should return true.
So, instead we should return true if ANY character is NOT in the allowed set for bare keys
fn needs_quoting(s: &str) -> bool {
s.chars().any(|c| {
!c.is_ascii_alphanumeric() && c != '_' && c != '-'
})
}
| } | ||
| _ => toml_value_to_serde_value(toml_value), | ||
| } | ||
| } |
There was a problem hiding this comment.
Not logging parse failures?
| match toml_value { | ||
| toml::Value::String(s) => { | ||
| // Try to parse as JSON | ||
| serde_json::from_str(&s).unwrap_or(Value::String(s)) |
There was a problem hiding this comment.
serde_json::from_str(&s).map_err(|e| {
TomlError::ConversionError(format!("Invalid JSON in triple-quoted string: {}", e))
})
| @@ -0,0 +1,1850 @@ | |||
| use std::collections::HashMap; | |||
There was a problem hiding this comment.
Why two #[cfg(all(test, not(feature = "jsonlogic")))] mod serialization_tests blocks. Can we merge them to avoid confusion?
| @@ -0,0 +1,1850 @@ | |||
| use std::collections::HashMap; | |||
There was a problem hiding this comment.
value_to_toml() and schema_to_toml() share ~90% of their logic. The only difference is object handling (triple-quoted JSON vs inline tables). Can we consider extracting common logic?
| err <- peekCAString ebuf | ||
| traverse_ freeNonNull [dc, ctx, ovrs, qry, mergeS, pfltr, exp, ebuf] | ||
| pure $ case (res, err) of | ||
| (Just cfg, []) -> Right cfg |
There was a problem hiding this comment.
The traverse_ freeNonNull list is missing di (dimensionInfo), any specific reason ?
Isn't this memory leak ?
Out of scope of this PR but curious.
| -- - dimensions_json: JSON string containing object mapping dimension names to dimension info | ||
| parseTomlConfig :: String -> IO (Either String String) | ||
| parseTomlConfig tomlContent = do | ||
| ebuf <- callocBytes 2048 -- Error buffer size matches Rust implementation |
There was a problem hiding this comment.
The error buffer is hardcoded to 2048 bytes with no validation. If the Rust code writes beyond this, it's undefined behavior , 2048 is a magic number , possible to define a constant for this ?
| CString -> | ||
| -- | error-buffer | ||
| CString -> | ||
| -- | resolved config json |
There was a problem hiding this comment.
typo at line 23 , it should be dimension_info_json
|
|
||
| module FFI.Superposition (getResolvedConfig, ResolveConfigParams (..), defaultResolveParams, MergeStrategy (..)) where | ||
| module FFI.Superposition (getResolvedConfig, ResolveConfigParams (..), defaultResolveParams, MergeStrategy (..), parseTomlConfig) where | ||
|
|
There was a problem hiding this comment.
importing prelude is not needed.
| tomlStr <- newCString tomlContent | ||
| res <- parse_toml_config tomlStr ebuf | ||
| err <- peekCAString ebuf | ||
| let peekMaybe p | p /= nullPtr = Just <$> peekCAString p |
There was a problem hiding this comment.
parseTomlConfig checks res for null after peeking, but getResolvedConfig does it correctly. Can we reorder the operations in parseTomlConfig?
| if let Err(e) = jschema.validate(instance) { | ||
| let verrors = e.collect::<Vec<ValidationError>>(); | ||
|
|
||
| let jschema = jsonschema::JSONSchema::options() |
There was a problem hiding this comment.
Can we precompile schemas outside the loop ? It is doing a lot of wasteful recompilation.
Precompilation can really help if let's say we have 10 unique schemas for 1000 overrides. It is like caching.
There was a problem hiding this comment.
Pre compiling requires Lazy static, I am not sure if Lazy static is by default thread safe or it would have to be wrapped in Arc Mutex
Problem
There is no easy human readable/write-able format of Superposition configuration data for simple applications (without a dedicated store) to use.
Solution
This PR adds the support to generate and parse Superposition configuration data written in as toml files. It introduces a specific convention over the TOML format to be able to support this.
More details in the design doc in the PR.
Environment variable changes
NA
Pre-deployment activity
NA
Post-deployment activity
NA
API changes
application/tomlSummary by CodeRabbit
New Features
Accept: application/tomlheader)Documentation
✏️ Tip: You can customize this high-level summary in your review settings.