Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change adds TOML and JSON configuration import with create-only, upsert, replace, and dry-run modes. It adds context descriptions to configuration formats, backend transactions, frontend import UI, API contracts, tests, examples, and documentation. ChangesConfiguration import and context descriptions
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds full-workspace config import (TOML + JSON) across the API surface (Smithy + server handlers), core format parsing/serialization, and the admin frontend (including a new Import page), with supporting docs and integration tests.
Changes:
- Introduces
/config/{toml|json}/importendpoints withx-import-strategyandx-import-dry-run, returning an import summary. - Extends config file formats to include per-context
_description_and wires descriptions through parsing, export, and DB persistence. - Adds an admin UI flow to upload a file, preview changes, and apply imports (with confirmation on destructive changes), plus docs/examples/tests.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/lsp/vscode-extension/test.super.toml | Adds _description_ to override entries for editor/test SuperTOML coverage. |
| tests/src/config_import.test.ts | New Bun integration tests covering JSON/TOML import strategies, dry-run, validation, and round-trip. |
| smithy/models/main.smithy | Registers import operations on the Superposition Smithy service. |
| smithy/models/config.smithy | Adds ImportStrategy, import report shapes, and TOML/JSON import operations. |
| examples/superposition_toml_example/README.md | Documents _description_ in overrides examples. |
| examples/superposition_config_file_examples/example.toml | Adds _description_ to example overrides. |
| docs/docs/superposition-config-file/intro.md | Links new Import & Export documentation page. |
| docs/docs/superposition-config-file/import-export.md | New doc page describing import/export endpoints and headers. |
| docs/docs/superposition-config-file/format-specification.md | Updates format spec to include _description_ in overrides for TOML/JSON. |
| crates/superposition_types/src/lib.rs | Introduces shared MarkupFormat enum used across backend/frontend. |
| crates/superposition_types/src/config.rs | Extends DetailedConfig with context_descriptions. |
| crates/superposition_types/src/api/config.rs | Adds Rust API types for import strategy and import summaries. |
| crates/superposition_core/tests/test_filter_debug.rs | Updates test helpers/fixtures for _description_ and context_descriptions. |
| crates/superposition_core/tests/format_integration.rs | Updates integration fixtures for _description_ in TOML/JSON parsing paths. |
| crates/superposition_core/src/format/toml.rs | Requires/serializes _description_ for contexts in TOML format. |
| crates/superposition_core/src/format/tests/toml.rs | Expands TOML format tests for _description_ validation and round-trip. |
| crates/superposition_core/src/format/tests/json.rs | Updates JSON format tests to include _description_ and serialization error behavior. |
| crates/superposition_core/src/format/json.rs | Parses/serializes optional _description_ for JSON contexts; improves missing-override error. |
| crates/superposition_core/src/format.rs | Plumbs optional context description through shared detailed-config builder and validates description. |
| crates/superposition_core/src/ffi.rs | Updates FFI docs example to include _description_. |
| crates/frontend/src/utils.rs | Adds raw-body request support and refactors retry wrapper to accept a body builder closure. |
| crates/frontend/src/types.rs | Adds Import route segment. |
| crates/frontend/src/pages/import_config.rs | New Import UI page (file upload, preview summary, apply, confirm on delete/replace). |
| crates/frontend/src/pages.rs | Registers the new import page module. |
| crates/frontend/src/components/side_nav.rs | Adds “Import” entry to the side navigation. |
| crates/frontend/src/app.rs | Wires /import route to the ImportConfig page. |
| crates/frontend/src/api.rs | Adds config_import::import_config client for the import endpoints (raw TOML/JSON body). |
| crates/frontend/Cargo.toml | Enables web-sys features needed for file upload and reading. |
| crates/context_aware_config/src/helpers.rs | Fetches context descriptions from DB and includes them in generated DetailedConfig. |
| crates/context_aware_config/src/api/config/import.rs | New import executor implementing create_only/upsert/replace + dry-run via transactional rollback. |
| crates/context_aware_config/src/api/config/handlers.rs | Adds POST /{format}/import handler with max payload size limit. |
| crates/context_aware_config/src/api/config.rs | Exposes the new import module. |
| clients/python/provider/examples/config.toml | Updates provider example config to include _description_. |
| clients/python/provider-sdk-tests/config.toml | Updates provider SDK test config to include _description_. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub fn from_file_name(file_name: &str) -> Self { | ||
| if file_name.to_lowercase().ends_with(".json") { | ||
| Self::Json | ||
| } else { | ||
| Self::Toml | ||
| } |
| const IMPORT_WORKSPACE = "importtestws"; | ||
| const suffix = Math.random().toString(36).substring(7); | ||
|
|
| test("create_only uses existing dimension positions for new contexts", async () => { | ||
| const body = JSON.stringify({ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
tests/src/config_import.test.ts (1)
385-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert descriptions after the JSON round-trip.
This test only verifies creation counts. It passes if JSON export drops descriptions. After re-import, assert the context, dimension, and default-config descriptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/config_import.test.ts` around lines 385 - 402, Extend the round-trip test around importConfig to assert that descriptions are preserved after re-import, covering the context, dimensions, and default configurations. Use the imported configuration or existing expected fixtures to validate each description while retaining the current zero-creation assertions.smithy/models/config.smithy (1)
217-225: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
ImportStrategyforImportConfigOutput.strategy.
ImportConfigOutput.strategyis declared as an unconstrainedString, while the import operation inputs use the existingImportStrategyenum and the Rust response serializes the summary strategy through that enum. Model the response field asImportStrategyso generated clients get the same constrained type.Proposed fix
- strategy: String + strategy: ImportStrategy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smithy/models/config.smithy` around lines 217 - 225, Change the strategy field in ImportConfigOutput from String to the existing ImportStrategy enum, preserving its required and notProperty traits so generated clients use the constrained type consistent with import inputs and Rust serialization.crates/frontend/src/utils.rs (1)
441-456: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider the workspace-lock retry policy for raw-body writes.
request_raw_bodyhard-codesRetryPolicy::None. Config import is a workspace write, and other write paths userequest_with_workspace_lock_retry. If the workspace is locked, the import fails immediately with a 409 instead of retrying. Accept the policy as a parameter, or useRetryPolicy::WorkspaceLockfor the import call. The closure already clones the body per attempt, so retries are safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/utils.rs` around lines 441 - 456, Update request_raw_body to support workspace-lock retries instead of hard-coding RetryPolicy::None, either by accepting a RetryPolicy parameter or by using RetryPolicy::WorkspaceLock for the config import call. Preserve the existing body cloning in the request closure so each retry can safely resend the raw body.crates/context_aware_config/src/api/config/import.rs (1)
578-591: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the Redis and webhook failures.
put_config_in_redisandexecute_webhook_callresults are discarded withlet _ =. A Redis refresh failure leaves a stale config cache after a committed import, and no operator signal exists. Log the error at warn level in both cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/context_aware_config/src/api/config/import.rs` around lines 578 - 591, The committed import path currently discards failures from put_config_in_redis and execute_webhook_call. Replace the let _ result handling in this branch with error-aware handling that logs each failure at warn level, including useful error details and context, while preserving the existing success flow.crates/frontend/src/pages/import_config.rs (3)
277-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the file size against the backend limit before upload.
The backend caps the import payload at 10 MiB (
MAX_IMPORT_SIZEincrates/context_aware_config/src/api/config/handlers.rs). The page reads the file size at Line 283 but does not compare it. A larger file is read fully into memory and then rejected by the server with a generic payload error. Reject it here with a clear message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/pages/import_config.rs` around lines 277 - 289, Validate the selected file’s size in the file-selection handler before resetting state or starting the read, using the backend’s 10 MiB import limit represented by MAX_IMPORT_SIZE. For oversized files, reject immediately with a clear user-facing message and do not proceed with file reading or upload; preserve the existing flow for files within the limit.
299-328: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
onload.forget()leaks one closure per file selection.
Closure::forgetgives the closure to the JavaScript heap permanently. Each file selection allocates a newFileReaderand a new closure, so repeated selections accumulate memory. Store the closure in aStoredValuetied to the component instead, so it drops when the component unmounts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/pages/import_config.rs` around lines 299 - 328, Replace the per-selection onload.forget() in the file-reading flow with component-lifetime storage using a StoredValue, retaining the closure while the FileReader may invoke it and allowing it to drop on component unmount. Update the relevant FileReader/onload setup so repeated selections no longer permanently leak closures, while preserving the generation check and existing alert behavior.
433-512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGenerate the strategy buttons from a list.
The three strategy buttons repeat the same 25-line block and differ only in the strategy value, icon, title, and description. Define an array of these four values and render the buttons with a loop. This removes the duplication and keeps the click handler in one place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/pages/import_config.rs` around lines 433 - 512, Refactor the repeated strategy buttons in the import configuration view into a list containing each strategy’s value, icon, title, and description, then render them through a loop. Use the loop item to drive strategy_choice_class, strategy_icon_class, labels, and the shared on:click handler that sets strategy_rws and calls clear_results; preserve the existing three strategies and grid layout.crates/context_aware_config/src/api/config/handlers.rs (1)
686-721: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTake the workspace write permit only for supported formats.
import_handleracquiresWorkspaceWritePermitbefore it inspectsformat. A request foryamltherefore locks the workspace and then returns 501 without any write. Reject the unsupported format before you acquire the permit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/context_aware_config/src/api/config/handlers.rs` around lines 686 - 721, Update import_handler so it matches or validates the MarkupFormat before acquiring WorkspaceWritePermit, allowing Toml and Json requests to proceed through handle_import while returning the existing NotImplemented response for Yaml without locking the workspace. Move the permit acquisition and connection lookup into the supported-format path, preserving the current responses and import behavior.crates/context_aware_config/src/helpers.rs (1)
87-106: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid loading descriptions for compact configuration generation.
generate_caccallsget_context_data, but Line 143 discards the descriptions. The query still reads every description and allocates the descriptions map. This adds database I/O and memory use to config-version and Redis generation without changingConfig.Split compact and detailed context loading, or make description loading optional.
Based on the supplied call path,
generate_cacdiscards the descriptions at Line 143.Also applies to: 143-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/context_aware_config/src/helpers.rs` around lines 87 - 106, Update get_context_data and its generate_cac call path so compact configuration generation does not select, load, or allocate context descriptions that are discarded. Split compact and detailed loading or make descriptions optional, while preserving description population for callers that require detailed context data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/context_aware_config/src/api/config/import.rs`:
- Around line 354-384: Update write_context so validations::validate_context and
validations::validate_overrides run for every import strategy, not only when
effective_config is Some. Use effective_config’s dimensions and default_configs
for CreateOnly, otherwise use parsed.dimensions and parsed.default_configs for
Upsert and Replace, while preserving the existing error reporting and weight
calculation.
- Around line 493-509: Update the ImportStrategy::Replace deletion logic in the
import flow to only remove contexts owned by this import, excluding experiment
variant contexts from both the candidate query or deletion condition. Preserve
deletion of missing import-owned context IDs and the existing summary updates.
- Around line 198-213: Update the existing-dimension branch in the
import/Replace flow to include value_compute_function_name from info in the
diesel update set, matching the insert path. Ensure imported values replace the
stored value while preserving the other dimension fields and update behavior.
In `@crates/superposition_core/src/format/json.rs`:
- Around line 83-88: Make JsonContext.description required by replacing the
optional field and removing its default/skip-serialization behavior. Update the
JSON import validation to reject overrides lacking _description_ and preserve
document metadata instead of falling back to the generic import description. In
JSON export, make the context_descriptions lookup mandatory and return a
serialization error when no description entry exists, covering the related
import/export paths around JsonContext.
In `@crates/superposition_types/src/lib.rs`:
- Around line 57-64: Update MarkupFormat::from_file_name to recognize filenames
ending in .yaml or .yml and return the YAML variant, while preserving JSON
detection and the existing fallback behavior for other extensions.
In `@docs/docs/superposition-config-file/format-specification.md`:
- Around line 362-395: Make the TOML and JSON complete override examples
equivalent by adding meaningful _description_ fields to every JSON override,
matching the entries shown in the TOML example. Update the override validation
rules to explicitly require _description_ for each override.
In `@tests/src/config_import.test.ts`:
- Around line 20-21: Derive IMPORT_WORKSPACE from the per-run suffix so each
config import test run uses an isolated workspace, while preserving the existing
suffix generation. In the workspace setup around CreateWorkspaceCommand, only
ignore an error when it explicitly indicates the workspace already exists;
propagate or fail on all other errors instead of treating every failure as
successful setup.
---
Nitpick comments:
In `@crates/context_aware_config/src/api/config/handlers.rs`:
- Around line 686-721: Update import_handler so it matches or validates the
MarkupFormat before acquiring WorkspaceWritePermit, allowing Toml and Json
requests to proceed through handle_import while returning the existing
NotImplemented response for Yaml without locking the workspace. Move the permit
acquisition and connection lookup into the supported-format path, preserving the
current responses and import behavior.
In `@crates/context_aware_config/src/api/config/import.rs`:
- Around line 578-591: The committed import path currently discards failures
from put_config_in_redis and execute_webhook_call. Replace the let _ result
handling in this branch with error-aware handling that logs each failure at warn
level, including useful error details and context, while preserving the existing
success flow.
In `@crates/context_aware_config/src/helpers.rs`:
- Around line 87-106: Update get_context_data and its generate_cac call path so
compact configuration generation does not select, load, or allocate context
descriptions that are discarded. Split compact and detailed loading or make
descriptions optional, while preserving description population for callers that
require detailed context data.
In `@crates/frontend/src/pages/import_config.rs`:
- Around line 277-289: Validate the selected file’s size in the file-selection
handler before resetting state or starting the read, using the backend’s 10 MiB
import limit represented by MAX_IMPORT_SIZE. For oversized files, reject
immediately with a clear user-facing message and do not proceed with file
reading or upload; preserve the existing flow for files within the limit.
- Around line 299-328: Replace the per-selection onload.forget() in the
file-reading flow with component-lifetime storage using a StoredValue, retaining
the closure while the FileReader may invoke it and allowing it to drop on
component unmount. Update the relevant FileReader/onload setup so repeated
selections no longer permanently leak closures, while preserving the generation
check and existing alert behavior.
- Around line 433-512: Refactor the repeated strategy buttons in the import
configuration view into a list containing each strategy’s value, icon, title,
and description, then render them through a loop. Use the loop item to drive
strategy_choice_class, strategy_icon_class, labels, and the shared on:click
handler that sets strategy_rws and calls clear_results; preserve the existing
three strategies and grid layout.
In `@crates/frontend/src/utils.rs`:
- Around line 441-456: Update request_raw_body to support workspace-lock retries
instead of hard-coding RetryPolicy::None, either by accepting a RetryPolicy
parameter or by using RetryPolicy::WorkspaceLock for the config import call.
Preserve the existing body cloning in the request closure so each retry can
safely resend the raw body.
In `@smithy/models/config.smithy`:
- Around line 217-225: Change the strategy field in ImportConfigOutput from
String to the existing ImportStrategy enum, preserving its required and
notProperty traits so generated clients use the constrained type consistent with
import inputs and Rust serialization.
In `@tests/src/config_import.test.ts`:
- Around line 385-402: Extend the round-trip test around importConfig to assert
that descriptions are preserved after re-import, covering the context,
dimensions, and default configurations. Use the imported configuration or
existing expected fixtures to validate each description while retaining the
current zero-creation assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fa1f305-2955-4b2b-8d09-c551f6563f84
📒 Files selected for processing (34)
clients/python/provider-sdk-tests/config.tomlclients/python/provider/examples/config.tomlcrates/context_aware_config/src/api/config.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/config/import.rscrates/context_aware_config/src/helpers.rscrates/frontend/Cargo.tomlcrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/pages.rscrates/frontend/src/pages/import_config.rscrates/frontend/src/types.rscrates/frontend/src/utils.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/format.rscrates/superposition_core/src/format/json.rscrates/superposition_core/src/format/tests/json.rscrates/superposition_core/src/format/tests/toml.rscrates/superposition_core/src/format/toml.rscrates/superposition_core/tests/format_integration.rscrates/superposition_core/tests/test_filter_debug.rscrates/superposition_types/src/api/config.rscrates/superposition_types/src/config.rscrates/superposition_types/src/lib.rsdocs/docs/superposition-config-file/format-specification.mddocs/docs/superposition-config-file/import-export.mddocs/docs/superposition-config-file/intro.mdexamples/superposition_config_file_examples/example.tomlexamples/superposition_toml_example/README.mdsmithy/models/config.smithysmithy/models/main.smithytests/src/config_import.test.tstooling/lsp/vscode-extension/test.super.toml
| if exists { | ||
| diesel::update( | ||
| dim_dsl::dimensions.filter(dim_dsl::dimension.eq(name)), | ||
| ) | ||
| .set(( | ||
| dim_dsl::schema.eq(info.schema.clone()), | ||
| dim_dsl::position.eq(position), | ||
| dim_dsl::dimension_type.eq(info.dimension_type.clone()), | ||
| dim_dsl::dependency_graph.eq(info.dependency_graph.clone()), | ||
| dim_dsl::last_modified_at.eq(Utc::now()), | ||
| dim_dsl::last_modified_by.eq(email), | ||
| dim_dsl::description.eq(&description), | ||
| dim_dsl::change_reason.eq(change_reason.clone()), | ||
| )) | ||
| .schema_name(schema_name) | ||
| .execute(conn)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The dimension update path drops value_compute_function_name.
The insert path sets value_compute_function_name from info (Line 221). The update path does not set it. An existing dimension therefore keeps its stored value even when the imported file specifies a different one. This contradicts the Replace intent of mirroring the file.
🐛 Proposed fix
dim_dsl::dependency_graph.eq(info.dependency_graph.clone()),
+ dim_dsl::value_compute_function_name
+ .eq(info.value_compute_function_name.clone()),
dim_dsl::last_modified_at.eq(Utc::now()),📝 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.
| if exists { | |
| diesel::update( | |
| dim_dsl::dimensions.filter(dim_dsl::dimension.eq(name)), | |
| ) | |
| .set(( | |
| dim_dsl::schema.eq(info.schema.clone()), | |
| dim_dsl::position.eq(position), | |
| dim_dsl::dimension_type.eq(info.dimension_type.clone()), | |
| dim_dsl::dependency_graph.eq(info.dependency_graph.clone()), | |
| dim_dsl::last_modified_at.eq(Utc::now()), | |
| dim_dsl::last_modified_by.eq(email), | |
| dim_dsl::description.eq(&description), | |
| dim_dsl::change_reason.eq(change_reason.clone()), | |
| )) | |
| .schema_name(schema_name) | |
| .execute(conn)?; | |
| if exists { | |
| diesel::update( | |
| dim_dsl::dimensions.filter(dim_dsl::dimension.eq(name)), | |
| ) | |
| .set(( | |
| dim_dsl::schema.eq(info.schema.clone()), | |
| dim_dsl::position.eq(position), | |
| dim_dsl::dimension_type.eq(info.dimension_type.clone()), | |
| dim_dsl::dependency_graph.eq(info.dependency_graph.clone()), | |
| dim_dsl::value_compute_function_name | |
| .eq(info.value_compute_function_name.clone()), | |
| dim_dsl::last_modified_at.eq(Utc::now()), | |
| dim_dsl::last_modified_by.eq(email), | |
| dim_dsl::description.eq(&description), | |
| dim_dsl::change_reason.eq(change_reason.clone()), | |
| )) | |
| .schema_name(schema_name) | |
| .execute(conn)?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/context_aware_config/src/api/config/import.rs` around lines 198 - 213,
Update the existing-dimension branch in the import/Replace flow to include
value_compute_function_name from info in the diesel update set, matching the
insert path. Ensure imported values replace the stored value while preserving
the other dimension fields and update behavior.
| let dimensions = effective_config | ||
| .map(|config| &config.dimensions) | ||
| .unwrap_or(&parsed.dimensions); | ||
|
|
||
| if let Some(config) = effective_config { | ||
| validations::validate_context(&ctx.condition, dimensions).map_err( | ||
| |errors| { | ||
| bad_argument!( | ||
| "Context '{}' is invalid for existing dimensions: {:?}", | ||
| ctx.id, | ||
| errors | ||
| ) | ||
| }, | ||
| )?; | ||
| validations::validate_overrides( | ||
| &override_, | ||
| &config.default_configs, | ||
| ) | ||
| .map_err(|errors| { | ||
| bad_argument!( | ||
| "Context '{}' has invalid overrides for existing default configs: {:?}", | ||
| ctx.id, | ||
| errors | ||
| ) | ||
| })?; | ||
| } | ||
|
|
||
| let weight = calculate_context_weight(&ctx.condition, dimensions) | ||
| .map_err(|e| { | ||
| bad_argument!("Failed to compute context weight: {}", e) | ||
| })?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate contexts for every strategy, not only CreateOnly.
effective_config is Some only when the strategy is CreateOnly (Line 482). In write_context, both validations::validate_context and validations::validate_overrides run inside if let Some(config) = effective_config. For Upsert and Replace, which are the default and the destructive strategies, the importer writes contexts with no condition validation and no override validation. A file can therefore persist a context whose condition references an unknown dimension, or whose overrides do not match the default config schema.
For Upsert and Replace the file content is authoritative and already written, so validation can run against parsed.dimensions and parsed.default_configs.
🛠️ Proposed direction
- if let Some(config) = effective_config {
- validations::validate_context(&ctx.condition, dimensions).map_err(
+ let default_configs = effective_config
+ .map(|config| &config.default_configs)
+ .unwrap_or(&parsed.default_configs);
+
+ validations::validate_context(&ctx.condition, dimensions).map_err(
|errors| {
bad_argument!(
- "Context '{}' is invalid for existing dimensions: {:?}",
+ "Context '{}' is invalid for the resolved dimensions: {:?}",
ctx.id,
errors
)
},
)?;
- validations::validate_overrides(
- &override_,
- &config.default_configs,
- )
+ validations::validate_overrides(&override_, default_configs)
.map_err(|errors| {
bad_argument!(
- "Context '{}' has invalid overrides for existing default configs: {:?}",
+ "Context '{}' has invalid overrides: {:?}",
ctx.id,
errors
)
})?;
- }Also applies to: 482-491
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/context_aware_config/src/api/config/import.rs` around lines 354 - 384,
Update write_context so validations::validate_context and
validations::validate_overrides run for every import strategy, not only when
effective_config is Some. Use effective_config’s dimensions and default_configs
for CreateOnly, otherwise use parsed.dimensions and parsed.default_configs for
Upsert and Replace, while preserving the existing error reporting and weight
calculation.
| if self.options.strategy == ImportStrategy::Replace { | ||
| let schema_name = &self.workspace.schema_name; | ||
| let file_ctx_ids: HashSet<&str> = | ||
| parsed.contexts.iter().map(|ctx| ctx.id.as_str()).collect(); | ||
| let db_ctx_ids: Vec<String> = ctx_dsl::contexts | ||
| .select(ctx_dsl::id) | ||
| .schema_name(schema_name) | ||
| .load::<String>(self.conn)?; | ||
|
|
||
| for id in db_ctx_ids { | ||
| if !file_ctx_ids.contains(id.as_str()) { | ||
| diesel::delete(ctx_dsl::contexts.filter(ctx_dsl::id.eq(&id))) | ||
| .schema_name(schema_name) | ||
| .execute(self.conn)?; | ||
| record(&mut summary.contexts, Outcome::Deleted); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find writers of the contexts table outside the config import path.
set -euo pipefail
rg -n --type=rust -C4 'ctx_dsl::contexts|contexts::dsl|insert_into\(contexts' crates \
-g '!crates/context_aware_config/src/api/config/import.rs' | head -200
# Look for experiment variant context creation
rg -n --type=rust -C4 'variant_id|variant.*context|create_variant' crates/experimentation_platform/src | head -120Repository: juspay/superposition
Length of output: 24869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the import Replace branch and nearby helpers/schemas.
sed -n '430,540p' crates/context_aware_config/src/api/config/import.rs
printf '\n--- import helpers related to bulk operations around import ---\n'
rg -n --type=rust -C3 'bulk-operations|ContextAction|PutRequest|Delete|execute\(self\.conn\)' crates/context_aware_config/src/api/config crates/context_aware_config/src/api/context crates/context_aware_config/src/helpers.rs | head -260
printf '\n--- contexts schema fields ---\n'
fd -a 'schema\.rs' crates | rg 'context_aware_config|superposition_types' | head -20 | xargs -r -I{} sh -c 'echo "### {}"; rg -n -C4 "table!|contexts|override_id|context" {} | head -120'Repository: juspay/superposition
Length of output: 30319
Scope Replace deletions to import-owned contexts.
The contexts table is also used for experiment variant contexts, and this branch deletes every context id that does not appear in the imported config file. Restrict the delete set to contexts that this import owns, or exclude experiment-owned contexts before deleting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/context_aware_config/src/api/config/import.rs` around lines 493 - 509,
Update the ImportStrategy::Replace deletion logic in the import flow to only
remove contexts owned by this import, excluding experiment variant contexts from
both the candidate query or deletion condition. Preserve deletion of missing
import-owned context IDs and the existing summary updates.
| #[serde( | ||
| rename = "_description_", | ||
| default, | ||
| skip_serializing_if = "Option::is_none" | ||
| )] | ||
| description: Option<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require JSON context descriptions.
JSON accepts an override without _description_. The import path then stores the generic import description instead of document metadata. JSON export can also omit _description_. This conflicts with the required-description contract used by TOML.
Make JsonContext.description required. Validate it directly during import. Return a serialization error when context_descriptions lacks an entry.
Proposed fix
struct JsonContext {
#[serde(rename = "_context_")]
context: Map<String, Value>,
- #[serde(rename = "_description_", default, skip_serializing_if = "Option::is_none")]
- description: Option<String>,
+ #[serde(rename = "_description_")]
+ description: String,
#[serde(flatten)]
overrides: Map<String, Value>,
}
- let description = ctx.description
- .map(validate_context_description::<JsonFormat>)
- .transpose()?;
+ let description = validate_context_description::<JsonFormat>(ctx.description)?;
- description: context_descriptions.get(&ctx.id).cloned(),
+ description: context_descriptions.get(&ctx.id).cloned().ok_or_else(|| {
+ JsonFormat::serialization_error(format!(
+ "Missing description for context '{}'",
+ ctx.id
+ ))
+ })?,Also applies to: 139-144, 175-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/superposition_core/src/format/json.rs` around lines 83 - 88, Make
JsonContext.description required by replacing the optional field and removing
its default/skip-serialization behavior. Update the JSON import validation to
reject overrides lacking _description_ and preserve document metadata instead of
falling back to the generic import description. In JSON export, make the
context_descriptions lookup mandatory and return a serialization error when no
description entry exists, covering the related import/export paths around
JsonContext.
| impl MarkupFormat { | ||
| pub fn from_file_name(file_name: &str) -> Self { | ||
| if file_name.to_lowercase().ends_with(".json") { | ||
| Self::Json | ||
| } else { | ||
| Self::Toml | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect YAML file names as YAML.
from_file_name("config.yaml") returns Toml. The frontend then sends the YAML file to /config/toml/import with application/toml. The user receives a TOML parse error instead of the server's explicit YAML unsupported response.
Proposed fix
pub fn from_file_name(file_name: &str) -> Self {
- if file_name.to_lowercase().ends_with(".json") {
- Self::Json
- } else {
- Self::Toml
+ let file_name = file_name.to_lowercase();
+ if file_name.ends_with(".json") {
+ Self::Json
+ } else if file_name.ends_with(".yaml") || file_name.ends_with(".yml") {
+ Self::Yaml
+ } else {
+ Self::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.
| impl MarkupFormat { | |
| pub fn from_file_name(file_name: &str) -> Self { | |
| if file_name.to_lowercase().ends_with(".json") { | |
| Self::Json | |
| } else { | |
| Self::Toml | |
| } | |
| } | |
| impl MarkupFormat { | |
| pub fn from_file_name(file_name: &str) -> Self { | |
| let file_name = file_name.to_lowercase(); | |
| if file_name.ends_with(".json") { | |
| Self::Json | |
| } else if file_name.ends_with(".yaml") || file_name.ends_with(".yml") { | |
| Self::Yaml | |
| } else { | |
| Self::Toml | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/superposition_types/src/lib.rs` around lines 57 - 64, Update
MarkupFormat::from_file_name to recognize filenames ending in .yaml or .yml and
return the YAML variant, while preserving JSON detection and the existing
fallback behavior for other extensions.
| # Bike rides are cheaper | ||
| [[overrides]] | ||
| _context_ = { vehicle_type = "bike" } | ||
| _description_ = "Context description" | ||
| per_km_rate = 15.0 | ||
|
|
||
| # Cab rides have premium pricing | ||
| [[overrides]] | ||
| _context_ = { vehicle_type = "cab" } | ||
| _description_ = "Context description" | ||
| per_km_rate = 25.0 | ||
|
|
||
| # Bangalore cabs have specific rate | ||
| [[overrides]] | ||
| _context_ = { city = "Bangalore", vehicle_type = "cab" } | ||
| _description_ = "Context description" | ||
| per_km_rate = 22.0 | ||
|
|
||
| # Early morning surge in Delhi | ||
| [[overrides]] | ||
| _context_ = { city = "Delhi", vehicle_type = "cab", hour_of_day = 6 } | ||
| _description_ = "Context description" | ||
| surge_factor = 5.0 | ||
|
|
||
| # Evening surge in Delhi | ||
| [[overrides]] | ||
| _context_ = { city = "Delhi", vehicle_type = "cab", hour_of_day = 18 } | ||
| _description_ = "Context description" | ||
| surge_factor = 5.0 | ||
|
|
||
| # South India cohort pricing | ||
| [[overrides]] | ||
| _context_ = { city_cohort = "south" } | ||
| _description_ = "Context description" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the TOML and JSON complete examples equivalent.
Lines 365-395 add _description_ to every TOML override. The JSON document described as the same configuration at Lines 437-444 omits _description_ from every override. Add a meaningful _description_ field to each JSON override. Update the override validation rules to state this requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/superposition-config-file/format-specification.md` around lines 362
- 395, Make the TOML and JSON complete override examples equivalent by adding
meaningful _description_ fields to every JSON override, matching the entries
shown in the TOML example. Update the override validation rules to explicitly
require _description_ for each override.
| const IMPORT_WORKSPACE = "importtestws"; | ||
| const suffix = Math.random().toString(36).substring(7); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a unique workspace for each test run.
IMPORT_WORKSPACE is fixed, but this suite mutates it with replace. A previous run causes the first import to report updates instead of the expected creations. Concurrent runs can also delete each other's entities. Derive the workspace name from suffix. Do not treat every CreateWorkspaceCommand error as successful setup.
Proposed fix
-const IMPORT_WORKSPACE = "importtestws";
const suffix = Math.random().toString(36).substring(7);
+const IMPORT_WORKSPACE = `importtestws_${suffix}`;Also applies to: 188-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/config_import.test.ts` around lines 20 - 21, Derive
IMPORT_WORKSPACE from the per-run suffix so each config import test run uses an
isolated workspace, while preserving the existing suffix generation. In the
workspace setup around CreateWorkspaceCommand, only ignore an error when it
explicitly indicates the workspace already exists; propagate or fail on all
other errors instead of treating every failure as successful setup.
c7d2f6e to
83b753e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
29e462a to
b1ad18b
Compare
| max_bulk_operations | ||
| )); | ||
| } | ||
| let mut change_reasons = Vec::with_capacity(operations.len()); |
b1ad18b to
89357b8
Compare
eddd5ab to
38a7707
Compare
| ); | ||
| } | ||
|
|
||
| let mut actions = Vec::with_capacity(operations.len()); |
| } | ||
|
|
||
| let mut actions = Vec::with_capacity(operations.len()); | ||
| let mut affected_dimensions = Vec::with_capacity(operations.len()); |
38a7707 to
edfb0b0
Compare
| // ── Phase 1: async validation & preparation ── | ||
| let mut prepared_ops = | ||
| Vec::with_capacity(if ops.len() > 100 { 100 } else { ops.len() }); | ||
| let mut prepared_ops = Vec::with_capacity(ops.len()); |
| let tags = parse_config_tags(custom_headers.config_tags)?; | ||
| validate_bulk_size(request.operations.len())?; | ||
| let operations = request.into_inner().operations; | ||
| let mut change_reasons = Vec::with_capacity(operations.len()); |
edfb0b0 to
e8f4827
Compare
e8f4827 to
1cb8ffa
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit
New Features
Documentation