feat: add local/remote provider implementations following new structure#930
Merged
ayushjain17 merged 16 commits intomainfrom Mar 25, 2026
Merged
feat: add local/remote provider implementations following new structure#930ayushjain17 merged 16 commits intomainfrom
ayushjain17 merged 16 commits intomainfrom
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a comprehensive data source abstraction layer ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalResolutionProvider
participant DataSource
participant RefreshStrategy
participant Cache
Client->>LocalResolutionProvider: init(evaluation_context)
LocalResolutionProvider->>DataSource: fetch_config()
DataSource-->>LocalResolutionProvider: ConfigData
LocalResolutionProvider->>Cache: store config
LocalResolutionProvider->>RefreshStrategy: start refresh mechanism
RefreshStrategy-->>LocalResolutionProvider: ready
Note over LocalResolutionProvider: Provider Ready
Client->>LocalResolutionProvider: eval_with_context(flag_key, context)
LocalResolutionProvider->>Cache: get cached config
Cache-->>LocalResolutionProvider: Config
LocalResolutionProvider->>LocalResolutionProvider: merge contexts + filter
LocalResolutionProvider-->>Client: resolution result
RefreshStrategy->>LocalResolutionProvider: refresh()
LocalResolutionProvider->>DataSource: fetch_config()
DataSource-->>LocalResolutionProvider: ConfigData
LocalResolutionProvider->>Cache: update cache
sequenceDiagram
participant Client
participant SuperpositionAPIProvider
participant ResponseCache
participant SDK
participant RemoteAPI
Client->>SuperpositionAPIProvider: resolve_all_features(evaluation_context)
alt Cache Hit & Not Expired
SuperpositionAPIProvider->>ResponseCache: lookup(context_key)
ResponseCache-->>SuperpositionAPIProvider: cached result
SuperpositionAPIProvider-->>Client: cached features
else Cache Miss or Expired
SuperpositionAPIProvider->>SDK: get_resolved_config(query_context)
SDK->>RemoteAPI: HTTP GET /config
RemoteAPI-->>SDK: Config Document
SDK-->>SuperpositionAPIProvider: Document
SuperpositionAPIProvider->>SuperpositionAPIProvider: convert to Map<String, Value>
SuperpositionAPIProvider->>ResponseCache: store(context_key, result)
SuperpositionAPIProvider-->>Client: features
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
This PR introduces a new trait-based architecture for the superposition_provider crate, adding pluggable data sources (HTTP + file), local in-process resolution, and remote API-based resolution, along with supporting utilities and examples.
Changes:
- Added
SuperpositionDataSourceabstraction withHttpDataSource+FileDataSourceimplementations (including watch support for file). - Added new providers:
LocalResolutionProvider(cached local eval + refresh strategies) andSuperpositionAPIProvider(remote resolved-config + optional response cache). - Refactored shared conversion/config-filtering utilities and updated related core/types behavior and examples.
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| smithy/patches/python.patch | Updates smithy patch context to match regenerated Python SDK model formatting. |
| docs/plans/2026-02-14-configuration-resolver-plan.md | Adds an implementation plan for the provider refactor. |
| docs/plans/2026-02-14-configuration-resolver-design.md | Adds a design doc describing the new architecture/modules. |
| crates/superposition_types/src/overridden.rs | Adds an “into” prefix filter helper for overrides maps. |
| crates/superposition_types/src/config/tests.rs | Updates tests to account for consuming Config::filter_* APIs. |
| crates/superposition_types/src/config.rs | Changes filtering APIs to consume Config and adds a combined filter(...) helper. |
| crates/superposition_provider/src/utils.rs | Refactors SDK → types conversions and experiment/config conversion helpers. |
| crates/superposition_provider/src/types.rs | Adds Watch + Manual refresh strategies and watch-related types. |
| crates/superposition_provider/src/traits.rs | Introduces trait-based bulk resolution + experiment metadata traits (plus typed resolvers). |
| crates/superposition_provider/src/remote_provider.rs | Adds SuperpositionAPIProvider with response caching and remote resolve call. |
| crates/superposition_provider/src/provider.rs | Adapts legacy provider code to use new conversion helpers. |
| crates/superposition_provider/src/local_provider.rs | Adds LocalResolutionProvider with caching + polling/on-demand/watch/manual refresh. |
| crates/superposition_provider/src/lib.rs | Exposes new modules and re-exports for the new architecture. |
| crates/superposition_provider/src/data_source/http.rs | Adds HTTP data source for config + experiments (conditional requests via If-Modified-Since). |
| crates/superposition_provider/src/data_source/file.rs | Adds file/TOML data source with filesystem watch notifications. |
| crates/superposition_provider/src/data_source.rs | Defines the SuperpositionDataSource trait and fetch response/container types. |
| crates/superposition_provider/src/conversions.rs | Adds shared conversions for OpenFeature ↔ serde_json ↔ smithy Document. |
| crates/superposition_provider/src/client.rs | Extends legacy client refresh strategy handling and updates conversion call sites. |
| crates/superposition_provider/examples/polling_example.rs | Demonstrates OpenFeature usage with polling refresh strategy. |
| crates/superposition_provider/examples/local_with_fallback_example.rs | Demonstrates HTTP primary + file fallback provider usage via OpenFeature client. |
| crates/superposition_provider/examples/local_http_example.rs | Demonstrates direct trait usage with HTTP-backed local provider. |
| crates/superposition_provider/examples/local_file_watch_example.rs | Demonstrates file watch strategy with local provider. |
| crates/superposition_provider/examples/local_file_example.rs | Demonstrates on-demand refresh strategy with file data source. |
| crates/superposition_provider/examples/config.toml | Adds an example TOML config for file-based examples. |
| crates/superposition_provider/Cargo.toml | Adds dependencies needed for new provider/data sources (notify, env_logger, derive_more). |
| crates/superposition_core/src/ffi_legacy.rs | Updates variant resolution plumbing after experiment API return-type changes. |
| crates/superposition_core/src/ffi.rs | Updates variant resolution plumbing after experiment API return-type changes. |
| crates/superposition_core/src/experiment.rs | Introduces ExperimentConfig and changes experiment filtering/variant APIs to return non-Result. |
| crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql | Adjusts casbin seed policies (currently introduces duplication). |
| crates/cac_client/src/lib.rs | Uses new combined Config::filter(...) helper for prefix/dimension filtering. |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClient.java | Minor auth-scheme ordering change in generated Java SDK. |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClient.java | Minor auth-scheme ordering change in generated Java SDK. |
| Cargo.lock | Updates lockfile for new Rust dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+70
| Number::Float(val) => Value::Number( | ||
| serde_json::Number::from_f64(val) | ||
| .unwrap_or_else(|| serde_json::Number::from(0)), | ||
| ), |
There was a problem hiding this comment.
document_to_value (and evaluation_context_to_value) silently converts non-finite floats (NaN/±Inf) into 0 via unwrap_or_else(|| ...from(0)). This can mask data issues and change semantics. Consider returning Value::Null, an error, or preserving the float as a string instead of silently coercing to zero.
Comment on lines
+189
to
+205
| let mut guard = self.watcher.lock().map_err(|e| { | ||
| SuperpositionError::ConfigError(format!( | ||
| "Failed to lock watcher mutex: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| *guard = None; | ||
|
|
||
| let mut tx_guard = self.broadcast_tx.lock().map_err(|e| { | ||
| SuperpositionError::ConfigError(format!( | ||
| "Failed to lock broadcast mutex: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| *tx_guard = None; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
FileDataSource uses std::sync::Mutex and locks it inside the async close() method. This can block the Tokio runtime if contention occurs. Consider switching watcher/broadcast_tx to tokio::sync::Mutex (or keep close() synchronous) to avoid blocking in async contexts.
Suggested change
| let mut guard = self.watcher.lock().map_err(|e| { | |
| SuperpositionError::ConfigError(format!( | |
| "Failed to lock watcher mutex: {}", | |
| e | |
| )) | |
| })?; | |
| *guard = None; | |
| let mut tx_guard = self.broadcast_tx.lock().map_err(|e| { | |
| SuperpositionError::ConfigError(format!( | |
| "Failed to lock broadcast mutex: {}", | |
| e | |
| )) | |
| })?; | |
| *tx_guard = None; | |
| Ok(()) | |
| // Move blocking std::sync::Mutex operations off the Tokio runtime using spawn_blocking | |
| let watcher = self.watcher.clone(); | |
| let broadcast_tx = self.broadcast_tx.clone(); | |
| tokio::task::spawn_blocking(move || -> Result<()> { | |
| let mut guard = watcher.lock().map_err(|e| { | |
| SuperpositionError::ConfigError(format!( | |
| "Failed to lock watcher mutex: {}", | |
| e | |
| )) | |
| })?; | |
| *guard = None; | |
| let mut tx_guard = broadcast_tx.lock().map_err(|e| { | |
| SuperpositionError::ConfigError(format!( | |
| "Failed to lock broadcast mutex: {}", | |
| e | |
| )) | |
| })?; | |
| *tx_guard = None; | |
| Ok(()) | |
| }) | |
| .await | |
| .map_err(|e| { | |
| SuperpositionError::ConfigError(format!( | |
| "Failed to join close() blocking task: {}", | |
| e | |
| )) | |
| })? |
Collaborator
Author
There was a problem hiding this comment.
have made some changes in this section, recheck the current code and then suggest
Comment on lines
+68
to
+75
| if let Some(filter) = filter { | ||
| if filter { | ||
| experiment_builder = experiment_builder | ||
| .dimension_match_strategy(DimensionMatchStrategy::Subset); | ||
| } else { | ||
| experiment_builder = experiment_builder | ||
| .dimension_match_strategy(DimensionMatchStrategy::Exact); | ||
| } |
There was a problem hiding this comment.
fetch_candidate_active_experiments and fetch_matching_active_experiments appear to select the opposite DimensionMatchStrategy from what the method names imply (candidate should use Subset matching; matching should use Exact). Consider replacing the filter: Option<bool> flag with an explicit enum (or swap the boolean handling) so the strategy is unambiguous and correct.
Comment on lines
+186
to
+203
| // 3. Extract context and targeting_key | ||
| let (query_data, _targeting_key) = | ||
| conversions::evaluation_context_to_query(context); | ||
|
|
||
| // 4. Build and send the get_resolved_config request | ||
| // Always fetch WITHOUT prefix filter so we can cache the full result | ||
| let mut builder = client | ||
| .get_resolved_config() | ||
| .workspace_id(&self.options.workspace_id) | ||
| .org_id(&self.options.org_id); | ||
|
|
||
| // Set context dimensions from evaluation context | ||
| let sdk_context: HashMap<String, Document> = query_data | ||
| .into_iter() | ||
| .map(|(k, v)| (k, conversions::value_to_document(v))) | ||
| .collect(); | ||
| builder = builder.set_context(Some(sdk_context)); | ||
|
|
There was a problem hiding this comment.
resolve_remote extracts (query_data, _targeting_key) but never sends the targeting key/identifier to the resolve endpoint. The SDK provides get_resolved_config_with_identifier(...) (and GetResolvedConfigInput also has a context_id field), so without passing the identifier the server may produce incorrect experiment bucketing / variant resolution. Consider using get_resolved_config_with_identifier and setting identifier from EvaluationContext.targeting_key (or set context_id if that's what the API expects).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql (1)
95-104:⚠️ Potential issue | 🔴 CriticalSQL syntax error and duplicate rules.
Two issues here:
Syntax error: Line 97 is missing a trailing comma. Since lines 100-101 add new tuple values to the
INSERTstatement, a comma is required after line 97's closing parenthesis. This will cause the SQL statement to fail.Duplicate entries: Lines 99-101 are exact duplicates of lines 95-97 (same comment, same two policy rows). This appears to be an accidental duplication—possibly from a rebase or merge conflict mentioned in the commit history.
The duplicate block should be removed entirely.
🐛 Proposed fix: Remove the duplicate block
-- experiment_config ('g3', 'experiment_config:get', 'read', '', '', '', ''), ('p', 'experiment_config_reader', '*', 'experiment_config', 'read', '*', '') - -- experiment_config - ('g3', 'experiment_config:get', 'read', '', '', '', ''), - ('p', 'experiment_config_reader', '*', 'experiment_config', 'read', '*', '') - -- TODO: Figure out what to do about Auth and Organisation resources... ON CONFLICT (ptype, v0, v1, v2, v3, v4, v5) DO NOTHING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql` around lines 95 - 104, Remove the duplicated policy block and fix the missing comma after the first ('p', 'experiment_config_reader', '*', 'experiment_config', 'read', '*', '') tuple so the INSERT list is valid: delete the repeated lines that reintroduce the same ('g3', 'experiment_config:get', 'read', '', '', '', '') and ('p', 'experiment_config_reader', '*', 'experiment_config', 'read', '*', '') tuples, and ensure the remaining tuples in the INSERT are separated by commas so the ON CONFLICT (ptype, v0, v1, v2, v3, v4, v5) DO NOTHING; statement parses correctly.
🧹 Nitpick comments (10)
smithy/patches/python.patch (1)
9-21: VERSION environment variable is properly configured in CI/CD but missing fallback for local development.The release workflow correctly sets
VERSION: ${{ needs.tag-release.outputs.version }}when building Python packages, so CI/CD builds will succeed. However, without a fallback, local development builds will fail if developers don't explicitly set theVERSIONenvironment variable.For convenience during local development, consider adding a default version to handle the case when
VERSIONis unset:[tool.hatch.version] source = "env" variable = "VERSION" default = "0.0.0-dev"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/patches/python.patch` around lines 9 - 21, Add a fallback default for hatch version resolution so local builds don't fail when the VERSION env var is missing: update the [tool.hatch.version] block (the section you added with source = "env" and variable = "VERSION") to include a default = "0.0.0-dev" entry so hatch will use that value when the VERSION environment variable is unset; keep the existing dynamic = ["version"] setting in the top-level metadata as-is.crates/superposition_provider/src/provider.rs (1)
125-126: Good refactor to shared conversion utility.Moving
evaluation_context_to_queryto theconversionsmodule promotes code reuse across providers.Note: The
evaluation_context.clone()call creates a copy. Ifevaluation_context_to_querycould accept a reference instead, the clone could be avoided. This is a minor optimization opportunity if the conversion function doesn't need ownership.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/provider.rs` around lines 125 - 126, The call to conversions::evaluation_context_to_query currently clones evaluation_context; update the conversions module function signature evaluation_context_to_query to accept a reference (e.g., &EvaluationContext or the concrete type used) rather than taking ownership, update all call sites (including the call in provider.rs that currently does evaluation_context.clone()) to pass a reference (evaluation_context or &evaluation_context), and adjust the function body in conversions::evaluation_context_to_query to use the referenced value; this removes the unnecessary clone while preserving behavior—ensure you update any tests or other usages that relied on ownership accordingly.crates/superposition_provider/Cargo.toml (1)
30-33: Consider updatingnotifyto the latest stable version.The
env_logger = "0.11"is current (latest stable is 0.11.10). However,notify = "7"is behind the latest stable version 8.2.0. Consider updating tonotify = "8"for the latest stable features and improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/Cargo.toml` around lines 30 - 33, Update the notify dependency in Cargo.toml from "7" to the current stable major (e.g., "8" or "8.2.0") to pick up the latest fixes and API improvements; change the version string for the notify entry, run cargo update / cargo check to ensure compatibility, and adjust any code using notify APIs if compilation errors appear (look for usages of notify in the crate to update callsites to the v8 API).crates/superposition_provider/examples/local_file_watch_example.rs (1)
33-40: Add a graceful exit path for the watch loop.The infinite loop has no break condition, so the example cannot perform structured shutdown/cleanup. Consider handling termination and then closing the provider explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/examples/local_file_watch_example.rs` around lines 33 - 40, The example's infinite loop around provider.resolve_all_features(context.clone()) needs a graceful exit: add signal handling (e.g., listen for Ctrl-C or SIGTERM) and break the loop when signaled, then perform cleanup by calling the provider's shutdown/close method (or drop the provider explicitly) before exiting; update the loop to poll both the timer and a shutdown trigger so resolve_all_features and println! still run until the shutdown signal is received, then call the provider's cleanup API (or ensure provider is dropped) to perform structured shutdown.crates/superposition_provider/examples/local_with_fallback_example.rs (1)
39-40: Replace fixed startup delay with readiness-based wait.Using a hardcoded 2-second sleep is timing-sensitive and can still race on slower starts. Prefer waiting until the first successful evaluation (or timeout) before entering the main loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/examples/local_with_fallback_example.rs` around lines 39 - 40, Replace the fixed sleep(Duration::from_secs(2)).await startup wait with a readiness-based loop: repeatedly attempt the provider's first feature evaluation (e.g., call the evaluation method used later such as evaluate or evaluate_flag) until it returns a successful result or a hard timeout elapses (use tokio::time::timeout or Instant + loop + sleep intervals); on success continue to the main loop, on timeout bail with an error. Update the code around the sleep call to perform this check, use small backoff sleeps between attempts, and surface clear error logging if the readiness check fails.crates/superposition_provider/src/data_source/file.rs (1)
17-21: Consider usingtokio::sync::Mutexfor async-safe locking.Using
std::sync::Mutexin an async context can block the tokio runtime thread if there's lock contention. While the locks are held briefly here,tokio::sync::Mutexwould be more idiomatic and avoid potential issues.♻️ Suggested change
-use std::sync::{Arc, Mutex}; +use std::sync::Arc; +use tokio::sync::Mutex;Note: This would require making
watch()async, which may require trait changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/data_source/file.rs` around lines 17 - 21, The current FileDataSource uses std::sync::Mutex for watcher and broadcast_tx which can block the Tokio runtime; change both fields to use tokio::sync::Mutex (e.g., Arc<tokio::sync::Mutex<Option<RecommendedWatcher>>> and Arc<tokio::sync::Mutex<Option<broadcast::Sender<()>>>>) so locking is async-safe, update all places that lock those mutexes (e.g., the watch() method and any constructors/cleanup) to await the locks, and make watch() async (and propagate necessary trait/signature changes) so you can .await the tokio mutex guards rather than blocking the runtime.docs/plans/2026-02-14-configuration-resolver-design.md (1)
18-32: Add language specifier to fenced code block.The directory tree code block is missing a language specifier. This triggers markdown lint warnings and may affect rendering in some viewers.
📝 Suggested fix
-``` +```text crates/superposition_provider/src/ ├── lib.rs # Re-exports🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-02-14-configuration-resolver-design.md` around lines 18 - 32, The fenced code block showing the directory tree starting with "crates/superposition_provider/src/" is missing a language specifier; update the opening fence to include a language (e.g., change "```" to "```text") so markdown linters stop flagging it and rendering is consistent. Locate the block in docs/plans/2026-02-14-configuration-resolver-design.md where the tree begins with "crates/superposition_provider/src/" and add the language tag to the opening backticks; no other content changes are needed.crates/superposition_provider/src/remote_provider.rs (1)
43-51: Expired cache entries are not removed on read.The
get()method returnsNonefor expired entries but doesn't remove them from the map. Stale entries accumulate untilput()triggers eviction at capacity. This could cause memory bloat in read-heavy scenarios with long TTLs.Consider adding cleanup in
get():♻️ Suggested improvement
- fn get(&self, key: &str) -> Option<&Map<String, Value>> { - self.entries.get(key).and_then(|entry| { - if entry.created_at.elapsed() < self.ttl { - Some(&entry.value) - } else { - None - } - }) - } + fn get(&mut self, key: &str) -> Option<Map<String, Value>> { + if let Some(entry) = self.entries.get(key) { + if entry.created_at.elapsed() < self.ttl { + return Some(entry.value.clone()); + } + // Remove expired entry + self.entries.remove(key); + } + None + }Note: This would require changing the cache lock from read to write on cache hits, which has a performance trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/remote_provider.rs` around lines 43 - 51, get() currently returns None for expired entries but leaves them in self.entries, causing stale entries to accumulate; modify get() (the method that reads self.entries and checks entry.created_at against self.ttl) to remove expired entries from the map when detected instead of just returning None — e.g., acquire write access to entries (instead of only a read) and call remove(key) when entry.created_at.elapsed() >= self.ttl so the stale entry is evicted on read; keep the existing behavior for valid entries and be mindful of the lock upgrade/performance trade-off versus leaving get() read-only and relying on put()/capacity eviction.crates/superposition_provider/src/traits.rs (1)
43-73: Factor the typed resolvers through one helper.These five methods duplicate the same
resolve_all_features+ lookup + error-wrapping flow. Any future change to logging,ResolutionDetailsmetadata, or missing-flag handling now has to stay in sync in five places. A single generic helper with a per-type extractor closure would make this much harder to drift.Also applies to: 75-105, 107-137, 139-169, 171-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/traits.rs` around lines 43 - 73, The five typed resolvers (e.g., resolve_bool) duplicate the same pattern of calling resolve_all_features, checking config.get(flag_key), transforming the value or producing EvaluationError/ResolutionDetails, so introduce a single generic helper (e.g., resolve_typed or resolve_with_extractor) that accepts &self, flag_key: &str, evaluation_context: EvaluationContext, and an extractor closure Fn(&serde_json::Value) -> Option<T> (or similar) and returns EvaluationResult<ResolutionDetails<T>>; implement the common logging and error-wrapping (including EvaluationErrorCode::FlagNotFound, TypeMismatch, and General) and creation of ResolutionDetails inside that helper, then refactor resolve_bool and the other resolvers to call this helper with the appropriate extractor (e.g., value.as_bool, value.as_i64/into, as_str, etc.) so all lookup, logging, and error construction logic lives in one place.crates/superposition_provider/src/utils.rs (1)
307-380: Use move semantics consistently in the new owned-response path.These helpers now consume
Vec<ExperimentResponse>/Vec<ExperimentGroupResponse>, but the body still clones IDs and bucket fields back out. Destructuring the owned SDK structs and moving those strings directly would shave allocations from every refresh without changing behavior.Also applies to: 383-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/utils.rs` around lines 307 - 380, The loop currently clones string fields from owned SDK structs (e.g., exp.id.clone(), variant.id.clone()) despite consuming Vec<superposition_sdk::types::ExperimentResponse>; change to destructure and move fields out of the owned structs to avoid allocations: in the for exp in response loop, pattern-match/destructure the ExperimentResponse (id, context, variants, traffic_percentage, status, etc.) and for each variant destructure VariantResponse (id, variant_type, context_id, override_id, overrides) so you can use the owned id/context/overrides directly when building Variant and FfiExperiment instead of calling .clone(); keep conversions::hashmap_to_map and Exp::<Overrides>::try_from usage but feed them the moved values, and remove the cloning calls for FfiExperiment.id and Variant.id to use ownership moves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/superposition_provider/examples/polling_example.rs`:
- Around line 41-42: The parsed poll_interval and print_interval can be zero
which causes a busy loop; update the parsing logic around poll_interval and
print_interval (the variables assigned from env_or) to validate and reject or
clamp zero values—e.g., after parse, if value == 0 then set to a safe minimum
(1) or return an error/panic with a clear message so the loop that awaits on
these intervals cannot spin without delay; ensure the same validation is applied
wherever these intervals are consumed (the polling loop using poll_interval) and
document the behaviour.
In `@crates/superposition_provider/src/client.rs`:
- Around line 98-103: The current match arms for RefreshStrategy::Watch and
RefreshStrategy::Manual in the CacConfig handling only log info and do not
implement watch/manual behavior; update those arms in the match (where CacConfig
is processed) to explicitly reject unsupported strategies by returning an error
(or at minimum logging a warning and returning Err) instead of silently
continuing—refer to the RefreshStrategy::Watch and RefreshStrategy::Manual
variants and the CacConfig handling code to implement this change, and ensure
existing callers receive a clear error indicating that only Polling and OnDemand
are supported for CacConfig.
In `@crates/superposition_provider/src/conversions.rs`:
- Around line 67-70: The current use of
serde_json::Number::from_f64(...).unwrap_or_else(|| Number::from(0)) in
document_to_value and evaluation_context_to_value silently turns NaN/Inf into 0;
instead detect when from_f64 returns None and handle it explicitly — replace the
unwrap_or_else call with logic that checks the Option and maps None to a
Value::Null (or otherwise drop the field or return an Err if you prefer stricter
behavior), i.e., call serde_json::Number::from_f64(val) and if it is Some(n) use
Value::Number(n) else use Value::Null (or propagate an error) in both
document_to_value and evaluation_context_to_value so non-finite floats are not
corrupted to zero.
- Around line 11-17: The numeric conversion mishandles positive integers by
using serde_json::Number::as_i64() first and always mapping to
aws_smithy_types::Number::NegInt; update the Value::Number branch in
conversions.rs to properly distinguish signedness — either call as_u64() before
as_i64(), or check the i64 value's sign (if let Some(i) = n.as_i64() { if i < 0
{ Document::Number(Number::NegInt(i)) } else { Document::Number(Number::PosInt(i
as u64)) } }) and keep the existing as_f64() float branch; ensure you reference
Document::Number and aws_smithy_types::Number::{NegInt,PosInt} when making the
change.
- Around line 89-99: The Struct arm in evaluation_context_to_value currently
downcasts to HashMap and uses unwrap_or_default, silently converting unknown
struct shapes to {}; change this to surface an error instead: update
evaluation_context_to_value (or its API boundary) to return a Result<Value,
Error> (or propagate a custom error) and, in the
EvaluationContextFieldValue::Struct(s) branch, detect when
downcast_ref::<HashMap<String, EvaluationContextFieldValue>>() returns None and
return Err(...) with a clear message including the actual TypeId/other type info
(or use expect with a clear message at the API boundary) rather than defaulting
to an empty object; this ensures non-HashMap structs are caught (refer to
evaluation_context_to_value and the EvaluationContextFieldValue::Struct
handling) so callers can handle or reject unexpected struct shapes.
In `@crates/superposition_provider/src/data_source/file.rs`:
- Around line 126-148: In watch() the code drops the mutex (tx_guard) before
calling notify::recommended_watcher which can register a watcher that may never
be stored if another thread wins the double-check, causing a transient resource
leak; fix by ensuring only one watcher is created: either move the
creation/registration of notify::recommended_watcher (and tx/tx_clone) inside
the same critical section where you check/store the tx (i.e., do not drop
tx_guard until after you have confirmed no existing tx and have stored the newly
created watcher), or implement an atomic claim (OnceCell/AtomicBool) around
watcher creation so a thread “wins” creation before calling recommended_watcher;
reference the watch() function, tx_guard, tx_clone, and watcher when making the
change.
In `@crates/superposition_provider/src/local_provider.rs`:
- Around line 288-300: The experiment refresh Err(e) branch currently overwrites
the overall `resp` even when the config refresh earlier succeeded; update the
experiment error handling in `local_provider.rs` so it does not replace a prior
success — e.g., in the `Err(e)` branch for the experiment refresh, log the
failure with `log::warn!` but only assign `resp = Err(e)` if `resp` is already
an Err (or simply drop the assignment to preserve a previous Ok); refer to the
`resp` variable and the `Err(e)` match arm to locate and change the behavior.
In `@docs/plans/2026-02-14-configuration-resolver-plan.md`:
- Around line 13-15: The heading hierarchy jumps from the document root to "###
Task 1"; fix it by inserting an intermediate level-2 header (e.g., add "##
Tasks" immediately before the existing "### Task 1") or by promoting "### Task
1" to "## Task 1" so heading levels increment by one; update the header that
contains "### Task 1" in docs/plans/2026-02-14-configuration-resolver-plan.md
accordingly.
---
Outside diff comments:
In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql`:
- Around line 95-104: Remove the duplicated policy block and fix the missing
comma after the first ('p', 'experiment_config_reader', '*',
'experiment_config', 'read', '*', '') tuple so the INSERT list is valid: delete
the repeated lines that reintroduce the same ('g3', 'experiment_config:get',
'read', '', '', '', '') and ('p', 'experiment_config_reader', '*',
'experiment_config', 'read', '*', '') tuples, and ensure the remaining tuples in
the INSERT are separated by commas so the ON CONFLICT (ptype, v0, v1, v2, v3,
v4, v5) DO NOTHING; statement parses correctly.
---
Nitpick comments:
In `@crates/superposition_provider/Cargo.toml`:
- Around line 30-33: Update the notify dependency in Cargo.toml from "7" to the
current stable major (e.g., "8" or "8.2.0") to pick up the latest fixes and API
improvements; change the version string for the notify entry, run cargo update /
cargo check to ensure compatibility, and adjust any code using notify APIs if
compilation errors appear (look for usages of notify in the crate to update
callsites to the v8 API).
In `@crates/superposition_provider/examples/local_file_watch_example.rs`:
- Around line 33-40: The example's infinite loop around
provider.resolve_all_features(context.clone()) needs a graceful exit: add signal
handling (e.g., listen for Ctrl-C or SIGTERM) and break the loop when signaled,
then perform cleanup by calling the provider's shutdown/close method (or drop
the provider explicitly) before exiting; update the loop to poll both the timer
and a shutdown trigger so resolve_all_features and println! still run until the
shutdown signal is received, then call the provider's cleanup API (or ensure
provider is dropped) to perform structured shutdown.
In `@crates/superposition_provider/examples/local_with_fallback_example.rs`:
- Around line 39-40: Replace the fixed sleep(Duration::from_secs(2)).await
startup wait with a readiness-based loop: repeatedly attempt the provider's
first feature evaluation (e.g., call the evaluation method used later such as
evaluate or evaluate_flag) until it returns a successful result or a hard
timeout elapses (use tokio::time::timeout or Instant + loop + sleep intervals);
on success continue to the main loop, on timeout bail with an error. Update the
code around the sleep call to perform this check, use small backoff sleeps
between attempts, and surface clear error logging if the readiness check fails.
In `@crates/superposition_provider/src/data_source/file.rs`:
- Around line 17-21: The current FileDataSource uses std::sync::Mutex for
watcher and broadcast_tx which can block the Tokio runtime; change both fields
to use tokio::sync::Mutex (e.g.,
Arc<tokio::sync::Mutex<Option<RecommendedWatcher>>> and
Arc<tokio::sync::Mutex<Option<broadcast::Sender<()>>>>) so locking is
async-safe, update all places that lock those mutexes (e.g., the watch() method
and any constructors/cleanup) to await the locks, and make watch() async (and
propagate necessary trait/signature changes) so you can .await the tokio mutex
guards rather than blocking the runtime.
In `@crates/superposition_provider/src/provider.rs`:
- Around line 125-126: The call to conversions::evaluation_context_to_query
currently clones evaluation_context; update the conversions module function
signature evaluation_context_to_query to accept a reference (e.g.,
&EvaluationContext or the concrete type used) rather than taking ownership,
update all call sites (including the call in provider.rs that currently does
evaluation_context.clone()) to pass a reference (evaluation_context or
&evaluation_context), and adjust the function body in
conversions::evaluation_context_to_query to use the referenced value; this
removes the unnecessary clone while preserving behavior—ensure you update any
tests or other usages that relied on ownership accordingly.
In `@crates/superposition_provider/src/remote_provider.rs`:
- Around line 43-51: get() currently returns None for expired entries but leaves
them in self.entries, causing stale entries to accumulate; modify get() (the
method that reads self.entries and checks entry.created_at against self.ttl) to
remove expired entries from the map when detected instead of just returning None
— e.g., acquire write access to entries (instead of only a read) and call
remove(key) when entry.created_at.elapsed() >= self.ttl so the stale entry is
evicted on read; keep the existing behavior for valid entries and be mindful of
the lock upgrade/performance trade-off versus leaving get() read-only and
relying on put()/capacity eviction.
In `@crates/superposition_provider/src/traits.rs`:
- Around line 43-73: The five typed resolvers (e.g., resolve_bool) duplicate the
same pattern of calling resolve_all_features, checking config.get(flag_key),
transforming the value or producing EvaluationError/ResolutionDetails, so
introduce a single generic helper (e.g., resolve_typed or
resolve_with_extractor) that accepts &self, flag_key: &str, evaluation_context:
EvaluationContext, and an extractor closure Fn(&serde_json::Value) -> Option<T>
(or similar) and returns EvaluationResult<ResolutionDetails<T>>; implement the
common logging and error-wrapping (including EvaluationErrorCode::FlagNotFound,
TypeMismatch, and General) and creation of ResolutionDetails inside that helper,
then refactor resolve_bool and the other resolvers to call this helper with the
appropriate extractor (e.g., value.as_bool, value.as_i64/into, as_str, etc.) so
all lookup, logging, and error construction logic lives in one place.
In `@crates/superposition_provider/src/utils.rs`:
- Around line 307-380: The loop currently clones string fields from owned SDK
structs (e.g., exp.id.clone(), variant.id.clone()) despite consuming
Vec<superposition_sdk::types::ExperimentResponse>; change to destructure and
move fields out of the owned structs to avoid allocations: in the for exp in
response loop, pattern-match/destructure the ExperimentResponse (id, context,
variants, traffic_percentage, status, etc.) and for each variant destructure
VariantResponse (id, variant_type, context_id, override_id, overrides) so you
can use the owned id/context/overrides directly when building Variant and
FfiExperiment instead of calling .clone(); keep conversions::hashmap_to_map and
Exp::<Overrides>::try_from usage but feed them the moved values, and remove the
cloning calls for FfiExperiment.id and Variant.id to use ownership moves.
In `@docs/plans/2026-02-14-configuration-resolver-design.md`:
- Around line 18-32: The fenced code block showing the directory tree starting
with "crates/superposition_provider/src/" is missing a language specifier;
update the opening fence to include a language (e.g., change "```" to "```text")
so markdown linters stop flagging it and rendering is consistent. Locate the
block in docs/plans/2026-02-14-configuration-resolver-design.md where the tree
begins with "crates/superposition_provider/src/" and add the language tag to the
opening backticks; no other content changes are needed.
In `@smithy/patches/python.patch`:
- Around line 9-21: Add a fallback default for hatch version resolution so local
builds don't fail when the VERSION env var is missing: update the
[tool.hatch.version] block (the section you added with source = "env" and
variable = "VERSION") to include a default = "0.0.0-dev" entry so hatch will use
that value when the VERSION environment variable is unset; keep the existing
dynamic = ["version"] setting in the top-level metadata as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c8fad75-9f1c-4017-a374-9223d62c942e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClient.javaclients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClient.javacrates/cac_client/src/lib.rscrates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlcrates/superposition_core/src/experiment.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/ffi_legacy.rscrates/superposition_provider/Cargo.tomlcrates/superposition_provider/examples/config.tomlcrates/superposition_provider/examples/local_file_example.rscrates/superposition_provider/examples/local_file_watch_example.rscrates/superposition_provider/examples/local_http_example.rscrates/superposition_provider/examples/local_with_fallback_example.rscrates/superposition_provider/examples/polling_example.rscrates/superposition_provider/src/client.rscrates/superposition_provider/src/conversions.rscrates/superposition_provider/src/data_source.rscrates/superposition_provider/src/data_source/file.rscrates/superposition_provider/src/data_source/http.rscrates/superposition_provider/src/lib.rscrates/superposition_provider/src/local_provider.rscrates/superposition_provider/src/provider.rscrates/superposition_provider/src/remote_provider.rscrates/superposition_provider/src/traits.rscrates/superposition_provider/src/types.rscrates/superposition_provider/src/utils.rscrates/superposition_types/src/config.rscrates/superposition_types/src/config/tests.rscrates/superposition_types/src/overridden.rsdocs/plans/2026-02-14-configuration-resolver-design.mddocs/plans/2026-02-14-configuration-resolver-plan.mdsmithy/patches/python.patch
Comment on lines
+67
to
+70
| Number::Float(val) => Value::Number( | ||
| serde_json::Number::from_f64(val) | ||
| .unwrap_or_else(|| serde_json::Number::from(0)), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does serde_json::Number::from_f64returnNone for NaN or infinite values?
💡 Result:
Yes, serde_json::Number::from_f64 returns None for NaN or infinite values. It explicitly converts only finite f64 values to a Number, as infinite and NaN values are not valid JSON numbers per the specification. The official documentation states: "Converts a finite f64 to a Number. Infinite or NaN values are not JSON numbers." with example assert!(Number::from_f64(f64::NAN).is_none);. The source code confirms: if f.is_finite { ... } else { None }.
Citations:
- 1: https://docs.serde.rs/serde_json/struct.Number.html
- 2: https://docs.rs/serde_json/latest/serde_json/struct.Number.html
- 3: http://docs.rs/crate/serde_json/1.0.149/target-redirect/src/serde_json/number.rs.html
- 4: https://docs.serde.rs/src/serde_json/number.rs.html
🏁 Script executed:
# First, let's check if the file exists and examine the specific lines mentioned
if [ -f "crates/superposition_provider/src/conversions.rs" ]; then
echo "=== Lines 67-70 ==="
sed -n '67,70p' crates/superposition_provider/src/conversions.rs | cat -n
echo ""
echo "=== Lines 83-86 ==="
sed -n '83,86p' crates/superposition_provider/src/conversions.rs | cat -n
echo ""
echo "=== Full context around lines 67-70 (60-75) ==="
sed -n '60,75p' crates/superposition_provider/src/conversions.rs | cat -n
echo ""
echo "=== Full context around lines 83-86 (76-92) ==="
sed -n '76,92p' crates/superposition_provider/src/conversions.rs | cat -n
else
echo "File not found"
fiRepository: juspay/superposition
Length of output: 2398
🏁 Script executed:
# Search for all occurrences of from_f64 in the file to ensure we catch all similar patterns
echo "=== All from_f64 usages in conversions.rs ==="
rg "from_f64" crates/superposition_provider/src/conversions.rs -B 2 -A 2 -nRepository: juspay/superposition
Length of output: 701
Silent conversion of non-finite floats to zero causes data corruption.
serde_json::Number::from_f64 returns None for NaN and infinite values (not valid JSON numbers per specification), but this code silently converts them to 0 via unwrap_or_else. This is data corruption for config values and targeting context. Return an error, drop the field, or surface null instead.
Affects two locations:
- Lines 67-70:
document_to_valuefunction - Lines 83-86:
evaluation_context_to_valuefunction
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_provider/src/conversions.rs` around lines 67 - 70, The
current use of serde_json::Number::from_f64(...).unwrap_or_else(||
Number::from(0)) in document_to_value and evaluation_context_to_value silently
turns NaN/Inf into 0; instead detect when from_f64 returns None and handle it
explicitly — replace the unwrap_or_else call with logic that checks the Option
and maps None to a Value::Null (or otherwise drop the field or return an Err if
you prefer stricter behavior), i.e., call serde_json::Number::from_f64(val) and
if it is Some(n) use Value::Number(n) else use Value::Null (or propagate an
error) in both document_to_value and evaluation_context_to_value so non-finite
floats are not corrupted to zero.
Comment on lines
+89
to
+99
| EvaluationContextFieldValue::Struct(s) => { | ||
| // Convert struct to serde_json::Value | ||
| let struct_map = s | ||
| .downcast_ref::<HashMap<String, EvaluationContextFieldValue>>() | ||
| .map(|m| { | ||
| m.iter() | ||
| .map(|(k, v)| (k.clone(), evaluation_context_to_value(v.clone()))) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); | ||
| Value::Object(struct_map) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the OpenFeature Rust SDK, can EvaluationContextFieldValue::Structhold arbitrary structs viaArc::new(...)ornew_struct(...)?
💡 Result:
Yes, in the OpenFeature Rust SDK, EvaluationContextFieldValue::Struct can hold arbitrary structs. Official documentation and examples explicitly show three ways to provide arbitrary user-defined structs (assuming they implement necessary traits like Serialize/Deserialize for provider compatibility):
Citations:
- 1: https://docs.openfeature.dev/docs/reference/sdks/server/rust
- 2: https://openfeature.dev/docs/reference/sdks/server/rust/
- 3: https://github.com/open-feature/rust-sdk
- 4: https://linkedin.com/in/yurii-musolov-37415b211
🏁 Script executed:
# First, let's look at the file and the specific lines mentioned
cat -n crates/superposition_provider/src/conversions.rs | head -120 | tail -40Repository: juspay/superposition
Length of output: 2015
🏁 Script executed:
# Get more context - see the full function
cat -n crates/superposition_provider/src/conversions.rs | head -150Repository: juspay/superposition
Length of output: 6012
🏁 Script executed:
# Search for the function definition to understand its purpose
rg "fn evaluation_context_to_value" -A 5 -B 5Repository: juspay/superposition
Length of output: 1058
🏁 Script executed:
# Check how EvaluationContextFieldValue is defined and used
rg "EvaluationContextFieldValue" --type rust | head -20Repository: juspay/superposition
Length of output: 1279
🏁 Script executed:
# Look for actual uses of EvaluationContextFieldValue::Struct in the codebase
rg "EvaluationContextFieldValue::Struct\|new_struct" -B 3 -A 3Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check test files for how structs are created
fd "\.rs$" -type f | xargs rg -l "test\|spec" | head -10Repository: juspay/superposition
Length of output: 234
🏁 Script executed:
# Look for imports or dependencies related to open_feature
rg "open_feature" --type rust -B 2 -A 2 | head -40Repository: juspay/superposition
Length of output: 3185
🏁 Script executed:
# Check Cargo.toml to understand the openfeature version/dependency
fd "Cargo.toml" -type f | xargs rg "open_feature|open-feature"Repository: juspay/superposition
Length of output: 9527
🏁 Script executed:
# Let's look at the utils.rs file to understand how StructValue is created
cat -n crates/superposition_provider/src/utils.rs | grep -A 30 "StructValue"Repository: juspay/superposition
Length of output: 3256
🏁 Script executed:
# Let's also check the actual signature of EvaluationContextFieldValue
rg "enum EvaluationContextFieldValue\|struct StructValue" -A 10Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check the OpenFeature SDK to understand the Struct variant
rg "EvaluationContextFieldValue::Struct" -B 2 -A 5Repository: juspay/superposition
Length of output: 877
🏁 Script executed:
# Search for any test cases involving Struct in this codebase
rg "Struct\(" -A 3 -B 3 crates/superposition_provider/tests/Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for how EvaluationContext is constructed with Struct values
rg "EvaluationContext\|custom_fields" crates/superposition_provider/tests/integration_test.rs -A 5 -B 2Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Let's check the Cargo.lock to see the exact OpenFeature version and maybe find more info
rg -A 5 "name = \"open-feature\"" Cargo.lock | head -20Repository: juspay/superposition
Length of output: 279
🏁 Script executed:
# Look for how structs might be created - check for Any, Arc, or Box patterns
rg "Any\|Arc::new\|Box::new" crates/superposition_provider/src/ | head -15Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there are example usages that create structs with the SDK
find crates/superposition_provider/examples -name "*.rs" -exec cat {} \; | grep -A 5 -B 5 "Struct\|custom_fields"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Let's see if there's documentation about the conversion function's intent
cat -n crates/superposition_provider/src/conversions.rs | head -79 | tail -20Repository: juspay/superposition
Length of output: 1006
🏁 Script executed:
# Let's check what type annotation is used for the Struct field
rg "type.*Struct\|Struct.*Any\|Struct.*Arc\|Struct.*Box" --type rust -B 2 -A 2Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there's a definition or import of Struct in the codebase that shows what it contains
rg "pub.*Struct\|struct Struct" crates/superposition_provider/src/ -B 2 -A 3Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look at the exact type stored by checking the downcast call more carefully
rg "downcast_ref" -B 3 -A 3Repository: juspay/superposition
Length of output: 50376
🏁 Script executed:
# Look at the actual OpenFeature SDK code to understand the Struct type
# The downcast_ref suggests it's using std::any::Any
rg "std::any\|Any\|downcast" crates/superposition_provider/src/conversions.rs -B 5 -A 5Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Let's look for documentation or comments in the codebase about how structs should be handled
rg "struct\|Struct" crates/superposition_provider/src/conversions.rs -B 2 -A 2 -iRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there are any comments or documentation about the struct handling
cat crates/superposition_provider/src/conversions.rs | grep -i "struct\|downcast" -A 3 -B 3Repository: juspay/superposition
Length of output: 735
🏁 Script executed:
# Look for any error logs or panic statements related to struct handling
rg "todo\|unimplemented\|panic" crates/superposition_provider/src/conversions.rs -B 2 -A 2Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there are any GitHub issues or TODO comments in the provider directory
rg "TODO\|FIXME\|XXX\|HACK" crates/superposition_provider/src/ -B 1 -A 1Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Let's search for the actual OpenFeature SDK definition online through cargo docs
# First, check what the Struct field actually contains by looking at imports
head -10 crates/superposition_provider/src/conversions.rsRepository: juspay/superposition
Length of output: 381
Silent data loss when non-HashMap structs are passed to evaluation_context_to_value.
The EvaluationContextFieldValue::Struct variant can hold arbitrary types via OpenFeature's SDK, but this code only handles HashMap<String, EvaluationContextFieldValue>. When the downcast fails, it silently returns an empty object instead of erroring, causing valid evaluation context data to be lost. This can affect targeting decisions unexpectedly. Consider returning an error or asserting the struct shape at the API boundary instead of silently defaulting to {}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_provider/src/conversions.rs` around lines 89 - 99, The
Struct arm in evaluation_context_to_value currently downcasts to HashMap and
uses unwrap_or_default, silently converting unknown struct shapes to {}; change
this to surface an error instead: update evaluation_context_to_value (or its API
boundary) to return a Result<Value, Error> (or propagate a custom error) and, in
the EvaluationContextFieldValue::Struct(s) branch, detect when
downcast_ref::<HashMap<String, EvaluationContextFieldValue>>() returns None and
return Err(...) with a clear message including the actual TypeId/other type info
(or use expect with a clear message at the API boundary) rather than defaulting
to an empty object; this ensures non-HashMap structs are caught (refer to
evaluation_context_to_value and the EvaluationContextFieldValue::Struct
handling) so callers can handle or reject unexpected struct shapes.
Contributor
There was a problem hiding this comment.
@ayushjain17, here is a suggested fix. The core change converts evaluation_context_to_value to return Result<Value, String> so the downcast failure is surfaced to callers, and updates the two callers in the same file to propagate or filter the error:
crates/superposition_provider/src/conversions.rs
pub fn evaluation_context_to_value(value: EvaluationContextFieldValue) -> Result<Value, String> {
let v = match value {
EvaluationContextFieldValue::Bool(b) => Value::Bool(b),
EvaluationContextFieldValue::Int(i) => Value::Number(serde_json::Number::from(i)),
EvaluationContextFieldValue::Float(f) => Value::Number(
serde_json::Number::from_f64(f)
.unwrap_or_else(|| serde_json::Number::from(0)),
),
EvaluationContextFieldValue::String(s) => Value::String(s),
EvaluationContextFieldValue::DateTime(dt) => Value::String(dt.to_string()),
EvaluationContextFieldValue::Struct(s) => {
let struct_map = s
.downcast_ref::<HashMap<String, EvaluationContextFieldValue>>()
.ok_or_else(|| {
format!(
"EvaluationContextFieldValue::Struct downcast failed: \
expected HashMap<String, EvaluationContextFieldValue>, \
got {:?}",
s.type_id()
)
})?;
let map: serde_json::Map<String, Value> = struct_map
.iter()
.map(|(k, v)| {
evaluation_context_to_value(v.clone()).map(|val| (k.clone(), val))
})
.collect::<Result<_, _>>()?;
Value::Object(map)
}
};
Ok(v)
}
pub fn evaluation_context_to_query(
ctx: EvaluationContext,
) -> (Map<String, Value>, Option<String>) {
let targeting_key = ctx.targeting_key.clone();
let map = ctx
.custom_fields
.into_iter()
.filter_map(|(k, v)| {
match evaluation_context_to_value(v) {
Ok(val) => Some((k, val)),
Err(e) => {
log::warn!("Skipping evaluation context field '{}': {}", k, e);
None
}
}
})
.collect();
(map, targeting_key)
}
pub fn evaluation_context_to_map(context: EvaluationContext) -> Map<String, Value> {
let mut map: Map<String, Value> = context
.custom_fields
.into_iter()
.filter_map(|(k, v)| {
match evaluation_context_to_value(v) {
Ok(val) => Some((k, val)),
Err(e) => {
log::warn!("Skipping evaluation context field '{}': {}", k, e);
None
}
}
})
.collect();
if let Some(tk) = context.targeting_key {
map.insert("targeting_key".to_string(), Value::String(tk));
}
log::debug!("evaluation_context_to_map: {} keys", map.len());
map
}Key changes:
evaluation_context_to_valuenow returnsResult<Value, String>— theStructdowncast uses.ok_or_else(...)with a descriptive error message including the actualTypeIdfor debuggability, and the?operator propagates it.evaluation_context_to_queryandevaluation_context_to_mapusefilter_map+ alog::warn!to skip fields that fail conversion (rather than silently producing{}), so the evaluation context remains usable even if one field is malformed.
Note: If strict rejection is preferred over best-effort skipping (i.e., an entirely invalid context should fail fast), you could change the callers to also return
Resultand propagate with?. Thefilter_map+ warn approach is chosen here as a pragmatic balance — it avoids cascading theResulttype all the way up toFeatureProviderresolution methods.
🧠 Learnings used
Learnt from: knutties
Repo: juspay/superposition PR: 926
File: docs/docs/superposition-config-file/config-file-compatibility.md:13-14
Timestamp: 2026-03-21T12:43:53.252Z
Learning: In the `juspay/superposition` repository, `crates/cac_toml/src/lib.rs` (`cac_toml`) is a **stale/outdated** format and should NOT be used as the reference for validating or reviewing current SuperTOML syntax. The current SuperTOML shape (using `[default-configs]`, `[[overrides]]`, and `_context_`) in documentation is intentional and correct. Do not raise issues based on `cac_toml`'s parser expectations.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 910
File: crates/frontend/src/api.rs:159-166
Timestamp: 2026-03-12T06:30:36.695Z
Learning: In `crates/frontend/src/api.rs`, using `matches!(scope, AuthzScope::Admin)` where `scope: AuthzScope` (non-Copy enum with `Clone, PartialEq, Eq`) followed by passing `scope` to subsequent calls (e.g., `casbin_url_and_headers`) is valid and compiles correctly. Do not flag `matches!` on a non-Copy enum value as a move issue when the matched variant (`Admin`) has no fields and the code path that uses `scope` afterward is only reached when the condition is false. This pattern is accepted by the Rust compiler in the `superposition` codebase.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
ayushjain17
force-pushed
the
recovery-branch-3
branch
from
45e86f8 to
72bf694
Compare
ayushjain17
force-pushed
the
recovery-branch-3
branch
from
a9042d8 to
6f23d43
Compare
sauraww
reviewed
Mar 24, 2026
| Number::NegInt(val) => Value::Number(serde_json::Number::from(val)), | ||
| Number::Float(val) => Value::Number( | ||
| serde_json::Number::from_f64(val) | ||
| .unwrap_or_else(|| serde_json::Number::from(0)), |
Collaborator
There was a problem hiding this comment.
This looks like a legit concern , Incase of invalid values we are defaulting to 0 , Any specific reason to use this as default ?
sauraww
reviewed
Mar 25, 2026
sauraww
requested changes
Mar 25, 2026
mahatoankitkumar
requested changes
Mar 25, 2026
| context: Option<Map<String, Value>>, | ||
| prefix_filter: Option<Vec<String>>, | ||
| last_fetched_at: Option<DateTime<Utc>>, | ||
| filter: Option<bool>, |
Collaborator
There was a problem hiding this comment.
Should this be Option<DimensionMatchStrategy> directly?
Collaborator
Author
There was a problem hiding this comment.
not needed, this is a helper function, it is meant to do the heavy lifting
Collaborator
There was a problem hiding this comment.
But seems unnecessary operation
Collaborator
There was a problem hiding this comment.
the match would just simplify to
if let Some(strategy) = strategy {
experiment_builder = experiment_builder.dimension_match_strategy(strategy);
}
6fdced9 to
4493284
Compare
|
|
||
| pub struct FileDataSource { | ||
| file_path: PathBuf, | ||
| watcher: Arc<Mutex<Option<WatcherInner>>>, |
Collaborator
There was a problem hiding this comment.
Do we need this arc now?
watcher is only accessed through &self
|
|
||
| pub struct HttpDataSource { | ||
| options: SuperpositionOptions, | ||
| client: Arc<Client>, |
Collaborator
There was a problem hiding this comment.
same here
HttpDataSource itself will be behind an Arc at the caller side
sauraww
reviewed
Mar 25, 2026
| *cached = None; | ||
| } | ||
|
|
||
| // Set status to NotReady |
Collaborator
There was a problem hiding this comment.
Can we also add this :
{
let mut ctx = self.global_context.write().await;
*ctx = EvaluationContext::default();
}
Collaborator
There was a problem hiding this comment.
Global_context retains the old value. If init is called again, this is overwritte. But if someone calls resolve* between close and re-init, they'd evaluate against stale context.
ayushjain17
force-pushed
the
recovery-branch-3
branch
from
4493284 to
2dcf566
Compare
mahatoankitkumar
approved these changes
Mar 25, 2026
Collaborator
mahatoankitkumar
left a comment
mahatoankitkumar
left a comment
There was a problem hiding this comment.
Remote Provider comments left to be resolved.
sauraww
reviewed
Mar 25, 2026
| sleep(Duration::from_millis(debounce_ms)).await; | ||
| while watch_stream.receiver.try_recv().is_ok() {} | ||
|
|
||
| let last_fetched_at = { |
Collaborator
There was a problem hiding this comment.
Can we simple reuse the logic of do_refresh :
if let Err(e) = provider_clone.do_refresh().await {
log::error!(
"LocalResolutionProvider: watch refresh failed: {}",
e
);
}
2dcf566 to
4a415ba
Compare
Collaborator
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
ayushjain17
force-pushed
the
recovery-branch-3
branch
from
4a415ba to
5ad6937
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
smithy/patches/python.patch (1)
7-10:⚠️ Potential issue | 🟠 MajorVerify VERSION env var is set when installing from source or in local development.
Lines 10 and 20 make packaging depend on the
VERSIONenvironment variable via Hatchling's env source. The official release workflow (.github/workflows/release.yamlline 414–492) properly setsVERSIONfrom git tags before building Python packages, so production releases to PyPI are unaffected. However, source-based installs (e.g.,pip install git+https://...or local editable installs) will fail ifVERSIONis not explicitly set beforehand.Recommendation: Document the VERSION requirement in installation instructions (
clients/python/sdk/README.md,clients/python/provider/README.md) or add a build-time fallback (e.g., parsing version fromgit describeor a VERSION file) to improve reliability for developers and downstream consumers building from source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/patches/python.patch` around lines 7 - 10, The pyproject change replaced a static version with dynamic = ["version"], which makes package builds depend on the VERSION env var; update the repo so source/local installs don't fail by either (A) adding a build-time fallback that reads VERSION from git (e.g., git describe) or a VERSION file when the VERSION env var is missing – implement this fallback in the build step that resolves the dynamic version used by "dynamic = ['version']", or (B) document the requirement clearly by adding a note to clients/python/sdk/README.md and clients/python/provider/README.md that the VERSION environment variable must be set for pip install from source or editable installs; pick one approach and implement it (fallback code for build resolution or README updates) and ensure the dynamic version resolution references the same VERSION lookup used for packaging.
🧹 Nitpick comments (8)
crates/superposition_types/src/overridden.rs (1)
22-34: Deduplicate the prefix-match predicate.
filter_into_config_keys_by_prefixcopies the samestarts_withscan asfilter_config_keys_by_prefix, so the owned and borrowed paths can drift on the next semantics change. Please extract the predicate into one helper and keep only the iteration strategy different.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/overridden.rs` around lines 22 - 34, Both functions duplicate the same starts_with predicate; extract that predicate into a single helper (e.g., fn key_matches_any_prefix(key: &str, prefix_list: &HashSet<String>) -> bool) and replace the inline closure in filter_into_config_keys_by_prefix and filter_config_keys_by_prefix to call this helper so only the iteration/ownership strategy differs; ensure the helper accepts a &str key and &HashSet<String> prefixes and returns a bool so it can be used from both the owned (into_iter) and borrowed (iter) variants without duplicating logic.crates/cac_client/src/lib.rs (1)
163-167: Add a regression test for the combined filter path.This endpoint now relies entirely on
Config::filter(...)when bothquery_dataandprefixare present. The updated tests cover prefix and dimension filtering separately, but one combined case incrates/superposition_types/src/config/tests.rswould lock in the semantics this call site depends on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cac_client/src/lib.rs` around lines 163 - 167, Add a regression test in the config tests module that exercises the combined filter path where both query_data and prefix are present: construct a Config instance (the cac variable), provide non-empty query_data and a non-empty prefix set, call cac.to_owned().filter(query_data.as_ref(), prefix.map(HashSet::from_iter).as_ref()) (the same call site that produces filtered_config), and assert the resulting Config contains only entries that satisfy both the dimension/query filters and the prefix filtering; use Config::filter, query_data, prefix and filtered_config names to mirror the production flow.crates/superposition_provider/src/types.rs (1)
123-126: Encapsulate the receiver behindWatchStreammethods.
WatchStreamis part of the public provider surface and currently exposestokio::sync::broadcast::Receiver<()>as a public field, hard-coding tokio broadcast semantics into the API. Since all internal accesses to.receiverare within this crate (inlocal_provider.rs), moving the field private and adding forwarding methods likerecv()andtry_recv()would keep the watch transport swappable without a future breaking change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/types.rs` around lines 123 - 126, Make the WatchStream API encapsulate the tokio broadcast receiver: change the pub field receiver in struct WatchStream to a private field (e.g., receiver: tokio::sync::broadcast::Receiver<()>), add public forwarding methods recv(&mut self) and try_recv(&mut self) that delegate to the inner receiver's recv and try_recv, and update internal call sites in local_provider.rs to use these methods instead of accessing .receiver directly; this preserves current behavior while hiding tokio-specific types from the public surface so the transport can be swapped later.crates/superposition_provider/src/data_source/file.rs (1)
172-182: Usingstd::sync::Mutexin async context.The
close()method is async but usesstd::sync::Mutex::lock(). If the mutex is contended (e.g., another thread is inwatch()), this will block the async runtime's thread.Consider using
tokio::sync::Mutexfor the watcher field, or usetry_lock()with appropriate handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/data_source/file.rs` around lines 172 - 182, The async close() method is using std::sync::Mutex::lock(), which can block the async runtime; change the watcher field to use tokio::sync::Mutex (or another async-aware mutex) and update usages (e.g., in close() and watch()) to await watcher.lock().await, or if you prefer non-blocking fallback use try_lock() and handle the WouldBlock case; locate the watcher field definition and the close() method plus any watch() function and replace the sync Mutex usage with tokio::sync::Mutex and adjust calls to use .lock().await (and remove map_err wrapping accordingly).crates/superposition_provider/src/remote_provider.rs (1)
292-302: TODO: Implement server-side experiment resolution.The
get_applicable_variantscurrently returns an empty vector with a comment indicating experiments are handled server-side. Verify this aligns with the intended behavior for remote providers, or implement proper experiment metadata retrieval if needed.Would you like me to help implement experiment metadata retrieval for the remote provider?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/remote_provider.rs` around lines 292 - 302, The method get_applicable_variants on SuperpositionAPIProvider currently returns an empty Vec with a TODO; confirm intended behavior and either (A) keep explicit no-op by documenting that remote server resolves experiments and return Ok(vec![]) with a comment referencing server-side resolution, or (B) implement metadata retrieval by calling the provider's remote API: use SuperpositionAPIProvider's HTTP client/transport to send an evaluation request using the EvaluationContext, deserialize the returned experiment/variant payload into Vec<String>, and return that; update FeatureExperimentMeta impl accordingly and handle errors by mapping transport/parse failures into the Result error type. Ensure you modify the impl of FeatureExperimentMeta for SuperpositionAPIProvider and the get_applicable_variants signature only, using existing client methods on SuperpositionAPIProvider to perform the remote call and parsing.crates/superposition_provider/src/data_source/http.rs (1)
70-78: Simplify dimension match strategy logic.This could be simplified since
filteris already anOption<bool>:♻️ Suggested simplification
- if let Some(filter) = filter { - if filter { - experiment_builder = experiment_builder - .dimension_match_strategy(DimensionMatchStrategy::Subset); - } else { - experiment_builder = experiment_builder - .dimension_match_strategy(DimensionMatchStrategy::Exact); - } - } + if let Some(filter) = filter { + let strategy = if filter { + DimensionMatchStrategy::Subset + } else { + DimensionMatchStrategy::Exact + }; + experiment_builder = experiment_builder.dimension_match_strategy(strategy); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/data_source/http.rs` around lines 70 - 78, The current nested if-let can be simplified: when filter is Some(bool) set the experiment_builder dimension_match_strategy to DimensionMatchStrategy::Subset if true or DimensionMatchStrategy::Exact if false; replace the if let Some(filter) { if filter { ... } else { ... } } block with a single concise branch (e.g. if let Some(f) = filter { let strategy = if f { DimensionMatchStrategy::Subset } else { DimensionMatchStrategy::Exact }; experiment_builder = experiment_builder.dimension_match_strategy(strategy); }) using the existing experiment_builder, filter variable, dimension_match_strategy method, and DimensionMatchStrategy variants.crates/superposition_provider/src/traits.rs (1)
50-52: Consider efficiency: bulk resolution for single flag lookup.The
resolve_typedmethod callsresolve_all_featuresto retrieve the entire config map, then extracts a single flag. This is fine if bulk resolution is cached (as it likely is inLocalResolutionProvider), but worth noting for implementors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/traits.rs` around lines 50 - 52, The current resolve_typed implementation is inefficient for single-flag lookups because it always calls resolve_all_features; add a new async trait method (e.g., resolve_feature or resolve_single_feature) to provide a direct single-flag resolution path that implementors like LocalResolutionProvider can override for efficiency, give it a default implementation that delegates to resolve_all_features and extracts flag_key (so existing providers still work), and then change resolve_typed to call this new resolve_feature method instead of resolve_all_features.crates/superposition_provider/src/conversions.rs (1)
167-174: Add doc comment explaining array-to-struct conversion. Thevalue_to_structfunction converts all JSON values, including arrays, toStructValuewith index-based keys when struct-only contexts require it. This is intentional fallback behavior, but the existing doc comment at line 155 doesn't explain why arrays are converted or when this conversion occurs. Add clarification (e.g., "Arrays are converted to structs with string indices to support struct-only resolution contexts").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/conversions.rs` around lines 167 - 174, Update the doc comment for the value_to_struct function to explicitly state that arrays are intentionally converted into openfeature::StructValue with stringified index keys (using value_to_openfeature_value for each element) as a fallback for struct-only resolution contexts; clarify when this conversion occurs (i.e., when a JSON array is encountered in value_to_struct) and why (to support systems that expect StructValue rather than Array), and mention that element conversion uses index.to_string() as field keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/superposition_provider/src/data_source.rs`:
- Around line 55-67: ConfigData (and similarly ExperimentData) must not generate
fetched_at with Utc::now() for revalidation; change the constructors so callers
provide the revalidation token explicitly (e.g., add a fetched_at: DateTime<Utc>
parameter to ConfigData::new and ExperimentData::new and remove the Utc::now()
default), update the struct docs to state fetched_at is the external
revalidation token (Last-Modified/file mtime/etc.), and update all call sites
that construct ConfigData/ExperimentData to pass the appropriate token from HTTP
Last-Modified, file mtime, or equivalent instead of relying on a local
timestamp.
In `@crates/superposition_provider/src/data_source/file.rs`:
- Around line 41-43: The log message in fetch_config incorrectly claims "always
returning cached data" while the code actually reads the file on every call;
update the log in FileDataSource::fetch_config (the branch checking
if_modified_since.is_some()) to accurately state that if_modified_since is
ignored/not implemented and that the file is read fresh each time (reference:
if_modified_since variable and fetch_config function) so logs aren't misleading.
In `@crates/superposition_provider/src/data_source/http.rs`:
- Around line 50-55: The current conversion of if_modified_since uses
timestamp_nanos_opt() and aws_smithy_types::DateTime::from_nanos, which can
return None for dates outside the nanos range; detect when that conversion fails
and don't silently drop the conditional—instead log a warning (e.g., via
tracing::warn or log::warn) that the nanosecond conversion failed including the
original if_modified_since value, and then attempt a fallback conversion using
milliseconds (timestamp_millis() -> aws_smithy_types::DateTime::from_millis)
before giving up; ensure the final DateTime passed to
experiment_builder.if_modified_since comes from either the nanos conversion or
the millis fallback and that a warning is emitted when falling back or when both
conversions fail.
In `@crates/superposition_provider/src/local_provider.rs`:
- Around line 718-720: The SuperpositionDataSource::close() trait impl currently
calls self.close() causing infinite recursion; change it to delegate to the
provider's inherent close implementation instead (call the struct's inherent
close method rather than the trait method), e.g. invoke the provider's existing
inherent close(...) (the close() implemented earlier in this file) via the
concrete type (e.g. LocalProvider::close(self).await) so the trait impl forwards
to the internal close logic rather than recursing.
In `@crates/superposition_provider/src/remote_provider.rs`:
- Around line 304-314: The initialize method currently ignores the provided
EvaluationContext; update SuperpositionAPIProvider::initialize to store the
incoming _context into the provider's shared context field (e.g., self.context
or similar) instead of discarding it: acquire the provider's context container
(Arc/RwLock/Mutex as defined on the struct), clone or wrap the
&EvaluationContext as needed to make it owned (or convert to an Arc) and write
it into that field, then update status to ProviderStatus::Ready and remove the
TODO. Ensure you use the exact identifiers: initialize,
SuperpositionAPIProvider, EvaluationContext and the struct field name used to
hold the provider context (self.context or equivalent) so the provider actually
keeps the global context for later calls.
---
Outside diff comments:
In `@smithy/patches/python.patch`:
- Around line 7-10: The pyproject change replaced a static version with dynamic
= ["version"], which makes package builds depend on the VERSION env var; update
the repo so source/local installs don't fail by either (A) adding a build-time
fallback that reads VERSION from git (e.g., git describe) or a VERSION file when
the VERSION env var is missing – implement this fallback in the build step that
resolves the dynamic version used by "dynamic = ['version']", or (B) document
the requirement clearly by adding a note to clients/python/sdk/README.md and
clients/python/provider/README.md that the VERSION environment variable must be
set for pip install from source or editable installs; pick one approach and
implement it (fallback code for build resolution or README updates) and ensure
the dynamic version resolution references the same VERSION lookup used for
packaging.
---
Nitpick comments:
In `@crates/cac_client/src/lib.rs`:
- Around line 163-167: Add a regression test in the config tests module that
exercises the combined filter path where both query_data and prefix are present:
construct a Config instance (the cac variable), provide non-empty query_data and
a non-empty prefix set, call cac.to_owned().filter(query_data.as_ref(),
prefix.map(HashSet::from_iter).as_ref()) (the same call site that produces
filtered_config), and assert the resulting Config contains only entries that
satisfy both the dimension/query filters and the prefix filtering; use
Config::filter, query_data, prefix and filtered_config names to mirror the
production flow.
In `@crates/superposition_provider/src/conversions.rs`:
- Around line 167-174: Update the doc comment for the value_to_struct function
to explicitly state that arrays are intentionally converted into
openfeature::StructValue with stringified index keys (using
value_to_openfeature_value for each element) as a fallback for struct-only
resolution contexts; clarify when this conversion occurs (i.e., when a JSON
array is encountered in value_to_struct) and why (to support systems that expect
StructValue rather than Array), and mention that element conversion uses
index.to_string() as field keys.
In `@crates/superposition_provider/src/data_source/file.rs`:
- Around line 172-182: The async close() method is using
std::sync::Mutex::lock(), which can block the async runtime; change the watcher
field to use tokio::sync::Mutex (or another async-aware mutex) and update usages
(e.g., in close() and watch()) to await watcher.lock().await, or if you prefer
non-blocking fallback use try_lock() and handle the WouldBlock case; locate the
watcher field definition and the close() method plus any watch() function and
replace the sync Mutex usage with tokio::sync::Mutex and adjust calls to use
.lock().await (and remove map_err wrapping accordingly).
In `@crates/superposition_provider/src/data_source/http.rs`:
- Around line 70-78: The current nested if-let can be simplified: when filter is
Some(bool) set the experiment_builder dimension_match_strategy to
DimensionMatchStrategy::Subset if true or DimensionMatchStrategy::Exact if
false; replace the if let Some(filter) { if filter { ... } else { ... } } block
with a single concise branch (e.g. if let Some(f) = filter { let strategy = if f
{ DimensionMatchStrategy::Subset } else { DimensionMatchStrategy::Exact };
experiment_builder = experiment_builder.dimension_match_strategy(strategy); })
using the existing experiment_builder, filter variable, dimension_match_strategy
method, and DimensionMatchStrategy variants.
In `@crates/superposition_provider/src/remote_provider.rs`:
- Around line 292-302: The method get_applicable_variants on
SuperpositionAPIProvider currently returns an empty Vec with a TODO; confirm
intended behavior and either (A) keep explicit no-op by documenting that remote
server resolves experiments and return Ok(vec![]) with a comment referencing
server-side resolution, or (B) implement metadata retrieval by calling the
provider's remote API: use SuperpositionAPIProvider's HTTP client/transport to
send an evaluation request using the EvaluationContext, deserialize the returned
experiment/variant payload into Vec<String>, and return that; update
FeatureExperimentMeta impl accordingly and handle errors by mapping
transport/parse failures into the Result error type. Ensure you modify the impl
of FeatureExperimentMeta for SuperpositionAPIProvider and the
get_applicable_variants signature only, using existing client methods on
SuperpositionAPIProvider to perform the remote call and parsing.
In `@crates/superposition_provider/src/traits.rs`:
- Around line 50-52: The current resolve_typed implementation is inefficient for
single-flag lookups because it always calls resolve_all_features; add a new
async trait method (e.g., resolve_feature or resolve_single_feature) to provide
a direct single-flag resolution path that implementors like
LocalResolutionProvider can override for efficiency, give it a default
implementation that delegates to resolve_all_features and extracts flag_key (so
existing providers still work), and then change resolve_typed to call this new
resolve_feature method instead of resolve_all_features.
In `@crates/superposition_provider/src/types.rs`:
- Around line 123-126: Make the WatchStream API encapsulate the tokio broadcast
receiver: change the pub field receiver in struct WatchStream to a private field
(e.g., receiver: tokio::sync::broadcast::Receiver<()>), add public forwarding
methods recv(&mut self) and try_recv(&mut self) that delegate to the inner
receiver's recv and try_recv, and update internal call sites in
local_provider.rs to use these methods instead of accessing .receiver directly;
this preserves current behavior while hiding tokio-specific types from the
public surface so the transport can be swapped later.
In `@crates/superposition_types/src/overridden.rs`:
- Around line 22-34: Both functions duplicate the same starts_with predicate;
extract that predicate into a single helper (e.g., fn
key_matches_any_prefix(key: &str, prefix_list: &HashSet<String>) -> bool) and
replace the inline closure in filter_into_config_keys_by_prefix and
filter_config_keys_by_prefix to call this helper so only the iteration/ownership
strategy differs; ensure the helper accepts a &str key and &HashSet<String>
prefixes and returns a bool so it can be used from both the owned (into_iter)
and borrowed (iter) variants without duplicating logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf9485bb-273f-453d-ac8f-ecc418cdc5b3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
crates/cac_client/src/lib.rscrates/context_aware_config/src/helpers.rscrates/experimentation_platform/src/api/experiment_config/handlers.rscrates/experimentation_platform/src/api/experiment_groups/helpers.rscrates/experimentation_platform/src/api/experiments/helpers.rscrates/service_utils/src/redis.rscrates/superposition_core/src/experiment.rscrates/superposition_core/src/ffi.rscrates/superposition_core/src/ffi_legacy.rscrates/superposition_provider/Cargo.tomlcrates/superposition_provider/examples/config.tomlcrates/superposition_provider/examples/local_file_example.rscrates/superposition_provider/examples/local_file_watch_example.rscrates/superposition_provider/examples/local_http_example.rscrates/superposition_provider/examples/local_with_fallback_example.rscrates/superposition_provider/examples/polling_example.rscrates/superposition_provider/src/client.rscrates/superposition_provider/src/conversions.rscrates/superposition_provider/src/data_source.rscrates/superposition_provider/src/data_source/file.rscrates/superposition_provider/src/data_source/http.rscrates/superposition_provider/src/lib.rscrates/superposition_provider/src/local_provider.rscrates/superposition_provider/src/provider.rscrates/superposition_provider/src/remote_provider.rscrates/superposition_provider/src/traits.rscrates/superposition_provider/src/types.rscrates/superposition_provider/src/utils.rscrates/superposition_types/src/config.rscrates/superposition_types/src/config/tests.rscrates/superposition_types/src/overridden.rssmithy/patches/python.patch
ayushjain17
force-pushed
the
recovery-branch-3
branch
2 times, most recently
from
5a0e097 to
933e966
Compare
sauraww
reviewed
Mar 25, 2026
sauraww
reviewed
Mar 25, 2026
sauraww
reviewed
Mar 25, 2026
sauraww
reviewed
Mar 25, 2026
sauraww
reviewed
Mar 25, 2026
| e | ||
| ); | ||
| if resp.is_ok() { | ||
| resp = Err(e); |
Collaborator
There was a problem hiding this comment.
Propagating the error here .
Can we remove either of them ?
Collaborator
Author
There was a problem hiding this comment.
we are propagating the error this only
ayushjain17
force-pushed
the
recovery-branch-3
branch
from
933e966 to
40beaa2
Compare
sauraww
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#745
Summary by CodeRabbit
New Features
Documentation