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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR replaces Node.js-based JavaScript execution with a rustyscript runtime integration, converts multiple validation functions to async with proper await handling throughout the codebase, and refactors context bulk operations to use a two-phase validation-then-execution approach. The changes include dependency updates (rustyscript added, serde pinned) and corresponding handler modifications across the configuration API. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler
participant Helper
participant rustyscript Runtime
participant Database
participant TokenBucket[Result/Response]
rect rgba(100, 150, 200, 0.5)
Note over Client,Database: New Flow: Rustyscript-based Execution
Client->>Handler: POST/PUT request with function code
Handler->>Helper: async create_ctx_from_put_req()
Helper->>rustyscript Runtime: inject_secrets_and_variables_into_code()
rustyscript Runtime->>rustyscript Runtime: Execute wrapped code in runtime
rustyscript Runtime-->>Helper: Return FunctionExecutionResponse + logs
Helper->>Database: Query validation context
Database-->>Helper: Return context metadata
Helper-->>Handler: Return prepared Context
Handler->>Database: Single transactional upsert/update
Database-->>Handler: Persist changes
Handler-->>TokenBucket: Return unified response
TokenBucket-->>Client: Response with execution logs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Datron
force-pushed
the
rustyscript-functions
branch
from
0481b56 to
ed56c9a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Datron
force-pushed
the
rustyscript-functions
branch
2 times, most recently
from
d013dd4 to
553a1d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/context_aware_config/src/api/default_config/handlers.rs (1)
259-291:⚠️ Potential issue | 🟠 MajorValidate value-only updates against the effective schema and validator.
valuealready falls back toexisting.value, but the checks below only run when the request also carries a newschemaorvalue_validation_function_name. A PATCH that changes onlyvaluecan therefore persist data that violates the key's current schema or validation function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines 259 - 291, The current checks only validate when req provides a new schema or validation function, allowing value-only PATCHes to bypass the existing key's schema/validator; fix by resolving the effective schema and effective validation function first (use req.schema.unwrap_or(existing.schema) and req.value_validation_function_name.unwrap_or(existing.value_validation_function_name)) and then validate the computed value against that effective schema (via try_into_jsonschema/jschema.validate) and call validate_default_config_with_function with the effective validation function name so value-only updates are validated against the existing constraints.crates/context_aware_config/src/api/config/handlers.rs (1)
352-385:⚠️ Potential issue | 🔴 CriticalPrepare the replacement context before deleting the current one.
This branch deletes the row first, then does description lookup, payload reconstruction, async validation, and upsert, while also discarding the
delete/upsertresults. Any failure leaves the original context removed andog_contexts/og_overridesupdated as if the replacement succeeded;query_description(...)will also miss once the delete actually succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/config/handlers.rs` around lines 352 - 385, The code currently calls context::delete(...) before constructing and validating the replacement, which can leave the original removed on any downstream failure; instead, first build and validate the replacement by calling construct_new_payload(request_payload), resolve description via query_description(...) if needed, and await create_ctx_from_put_req(...) to produce new_ctx, then perform the delete/replace step: call context::upsert(...) with the prepared new_ctx and only after that (or in a single transactional replace if available) remove or mark the old context via context::delete(...); also handle and propagate errors from construct_new_payload, query_description, create_ctx_from_put_req, and context::upsert (do not discard results) and ensure og_contexts/og_overrides are only updated after a successful replace.crates/context_aware_config/src/api/context/helpers.rs (1)
275-329:⚠️ Potential issue | 🟠 MajorDon't return JS console output in validation errors.
stdouthere is produced by user code after secret/variable injection, so a failing validator can leak injected secrets or other sensitive inputs viaconsole.log. Keep it out of the client-facingvalidation_error!and only log a redacted form server-side if you still need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/context/helpers.rs` around lines 275 - 329, In validation_function_executor, remove user JS console output (stdout) from any client-facing validation_error! messages to avoid leaking injected secrets; specifically, for the Err((err, stdout)) branch and the Ok branch where fn_output is false, keep the validation_error! calls (from validation_error!) free of stdout/console content and only reference non-sensitive context (fun_name, key); still log the stdout server-side using log::error or log::trace but redact or truncate it before logging (e.g., replace with "<redacted>" or a fixed-length preview) so run_js_function outputs are never returned to the caller.
🧹 Nitpick comments (3)
crates/experimentation_client/src/lib.rs (1)
356-356: Inconsistent formatting with similar pattern at line 399.This line was simplified to a single-line format, but the equivalent
StatusCode::OKhandler inget_experiment_groups(line 399) still uses the multi-line format with braces. For consistency, consider applying the same formatting to both locations.📝 Apply consistent formatting to line 399
- StatusCode::OK => { - log::info!("{} EXP: new experiment groups received, updating", tenant) - } + StatusCode::OK => log::info!("{} EXP: new experiment groups received, updating", tenant),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/experimentation_client/src/lib.rs` at line 356, Make the StatusCode::OK match arms consistent: change the multi-line/braced OK handler inside get_experiment_groups to the single-line form used at the other occurrence (the one shown as "StatusCode::OK => log::info!(\"{} EXP: new config received, updating\", tenant),"). Locate the match arm for StatusCode::OK in the get_experiment_groups function and collapse its block to a single expression without braces, keeping the same log message and tenant variable.crates/context_aware_config/src/api/functions/handlers.rs (1)
77-85: Consider usingtokio::task::spawn_blockingdirectly instead ofrustyscript::tokio.Using
rustyscript::tokio::runtime::Handle::current()couples this code to rustyscript's re-exported tokio. If rustyscript's tokio version diverges from the project's, this could cause issues. Usingtokio::task::spawn_blockingdirectly is more idiomatic and avoids the indirection.♻️ Suggested refactor
- let handle = rustyscript::tokio::runtime::Handle::current(); - let function = req.function.clone(); - handle - .spawn_blocking(move || compile_fn(&function)) + let function = req.function.clone(); + tokio::task::spawn_blocking(move || compile_fn(&function)) .await .map_err(|e| { log::error!("Function compilation task failed: {:?}", e); unexpected_error!("Failed to compile function: {}", e) })??;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/functions/handlers.rs` around lines 77 - 85, The code currently uses rustyscript::tokio::runtime::Handle::current() and its spawn_blocking to run compile_fn with req.function, which couples us to rustyscript's tokio re-export; replace that with the standard tokio::task::spawn_blocking to avoid the dependency. Change the block that calls rustyscript::tokio::runtime::Handle::current() and handle.spawn_blocking(move || compile_fn(&function)) to call tokio::task::spawn_blocking(move || compile_fn(&function)).await and preserve the existing error mapping/logging (the map_err closure and the double-?? handling) so behavior remains identical for compile_fn and req.function.crates/frontend/src/components/default_config_form.rs (1)
572-572: Consider wrapping the large update request inArcto avoid repeated clones.The
ChangeLogSummarycomponent accesseschange_typetwice viaStoredValue::get_value(): once at line 599 to extract the title/description (discriminant-only match), and again at line 637–640 to read the actualupdate_requestfields. SinceValuefrom serde_json is large and the first match only needs the enum variant, the wide payload gets cloned unnecessarily. WrappingDefaultConfigUpdateRequestinArceliminates both the clippy warning and the redundant clone, making the code clearer than a lint suppression.♻️ Suggested direction
+use std::sync::Arc; + #[derive(Clone)] -#[allow(clippy::large_enum_variant)] pub enum ChangeType { Delete, - Update(DefaultConfigUpdateRequest), + Update(Arc<DefaultConfigUpdateRequest>), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/default_config_form.rs` at line 572, The code is cloning a large serde_json::Value when ChangeLogSummary calls StoredValue::get_value() twice for change_type and update_request; instead wrap DefaultConfigUpdateRequest in an Arc (e.g., change the stored type to Arc<DefaultConfigUpdateRequest> and construct Arc::new(...) where instances are created) so StoredValue::get_value() returns/clones the Arc cheaply, update pattern matches in ChangeLogSummary to match on the enum discriminant without forcing a deep clone, and remove the #[allow(clippy::large_enum_variant)] suppression; relevant symbols: ChangeLogSummary, StoredValue::get_value(), change_type, DefaultConfigUpdateRequest, update_request, Value, Arc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 76: The serde dependency is pinned to an exact version string "=1.0.221";
change this to a caret requirement so patch updates are allowed (replace the
exact pin with "^1.0.221" for the serde entry in Cargo.toml) to permit
compatible patch-level updates while preserving the major/minor constraint.
In `@crates/context_aware_config/src/api/context/handlers.rs`:
- Around line 345-354: The pre-check calls validate_ctx with
Overrides::default(), which bypasses the actual overrides preserved by
operations::r#move; instead, load the current context from req.context (or via
the same code path that yields the environment) to obtain its override_ map and
pass that into validate_ctx (replace Overrides::default() with the loaded
context's overrides), so validation reads environment.overrides exactly as
operations::r#move will use them.
- Around line 770-801: The Replace and Move branches (ContextAction::Replace and
ContextAction::Move) are missing the same change-reason enforcement applied in
the Put branch; call validate_change_reason using the
update_request.change_reason (and the workspace_context and conn) before pushing
PreparedOperation::Replace or PreparedOperation::Move, so Replace/Move fail when
change reasons are required just like Put; ensure you validate prior to
validate_override_with_functions/other validations where appropriate and
propagate any error consistently.
- Around line 819-828: The validate_ctx call for bulk move is using
Overrides::default() which ignores the source context's overrides; instead
extract the existing context's override_ map (from workspace_context or the
source context structure) and pass those overrides into validate_ctx (replace
Overrides::default() with the overrides constructed from
workspace_context.override_ or equivalent) so validation uses the current
context's override values; keep the rest of the parameters (move_req.context ->
ctx_condition, state.master_encryption_key) unchanged and ensure the override
map is converted to the same Overrides type expected by validate_ctx.
In `@crates/context_aware_config/src/api/functions/handlers.rs`:
- Around line 153-159: The join error from spawn_blocking should be treated as
an internal server error while the compile_fn result should remain a
bad-argument error: change the map_err on the await of
rustyscript::tokio::runtime::Handle::current().spawn_blocking(...).await to
convert JoinError into unexpected_error (500) instead of bad_argument (400),
then apply the second ? to propagate compile_fn's error as bad_argument; locate
the block referencing compile_fn, spawn_blocking, and the current map_err and
split the error handling so JoinError maps to unexpected_error and the
compile_fn result maps to bad_argument.
In `@crates/context_aware_config/src/validation_functions.rs`:
- Around line 71-128: In run_js_function remove or redact the trace logs that
expose sensitive JS and execution data: delete or change the log::trace! calls
that reference wrapped_code ("Running function code: {:?}"), fn_output
("Function output: {:?}"), and stdout ("Function logs: {}") so they no longer
print raw or user-controlled content; if you need telemetry keep only
non-sensitive markers (e.g., log a static message or a hash/boolean success
flag) and ensure wrapped_code, fn_output, and stdout are never logged in plain
text.
---
Outside diff comments:
In `@crates/context_aware_config/src/api/config/handlers.rs`:
- Around line 352-385: The code currently calls context::delete(...) before
constructing and validating the replacement, which can leave the original
removed on any downstream failure; instead, first build and validate the
replacement by calling construct_new_payload(request_payload), resolve
description via query_description(...) if needed, and await
create_ctx_from_put_req(...) to produce new_ctx, then perform the delete/replace
step: call context::upsert(...) with the prepared new_ctx and only after that
(or in a single transactional replace if available) remove or mark the old
context via context::delete(...); also handle and propagate errors from
construct_new_payload, query_description, create_ctx_from_put_req, and
context::upsert (do not discard results) and ensure og_contexts/og_overrides are
only updated after a successful replace.
In `@crates/context_aware_config/src/api/context/helpers.rs`:
- Around line 275-329: In validation_function_executor, remove user JS console
output (stdout) from any client-facing validation_error! messages to avoid
leaking injected secrets; specifically, for the Err((err, stdout)) branch and
the Ok branch where fn_output is false, keep the validation_error! calls (from
validation_error!) free of stdout/console content and only reference
non-sensitive context (fun_name, key); still log the stdout server-side using
log::error or log::trace but redact or truncate it before logging (e.g., replace
with "<redacted>" or a fixed-length preview) so run_js_function outputs are
never returned to the caller.
In `@crates/context_aware_config/src/api/default_config/handlers.rs`:
- Around line 259-291: The current checks only validate when req provides a new
schema or validation function, allowing value-only PATCHes to bypass the
existing key's schema/validator; fix by resolving the effective schema and
effective validation function first (use req.schema.unwrap_or(existing.schema)
and
req.value_validation_function_name.unwrap_or(existing.value_validation_function_name))
and then validate the computed value against that effective schema (via
try_into_jsonschema/jschema.validate) and call
validate_default_config_with_function with the effective validation function
name so value-only updates are validated against the existing constraints.
---
Nitpick comments:
In `@crates/context_aware_config/src/api/functions/handlers.rs`:
- Around line 77-85: The code currently uses
rustyscript::tokio::runtime::Handle::current() and its spawn_blocking to run
compile_fn with req.function, which couples us to rustyscript's tokio re-export;
replace that with the standard tokio::task::spawn_blocking to avoid the
dependency. Change the block that calls
rustyscript::tokio::runtime::Handle::current() and handle.spawn_blocking(move ||
compile_fn(&function)) to call tokio::task::spawn_blocking(move ||
compile_fn(&function)).await and preserve the existing error mapping/logging
(the map_err closure and the double-?? handling) so behavior remains identical
for compile_fn and req.function.
In `@crates/experimentation_client/src/lib.rs`:
- Line 356: Make the StatusCode::OK match arms consistent: change the
multi-line/braced OK handler inside get_experiment_groups to the single-line
form used at the other occurrence (the one shown as "StatusCode::OK =>
log::info!(\"{} EXP: new config received, updating\", tenant),"). Locate the
match arm for StatusCode::OK in the get_experiment_groups function and collapse
its block to a single expression without braces, keeping the same log message
and tenant variable.
In `@crates/frontend/src/components/default_config_form.rs`:
- Line 572: The code is cloning a large serde_json::Value when ChangeLogSummary
calls StoredValue::get_value() twice for change_type and update_request; instead
wrap DefaultConfigUpdateRequest in an Arc (e.g., change the stored type to
Arc<DefaultConfigUpdateRequest> and construct Arc::new(...) where instances are
created) so StoredValue::get_value() returns/clones the Arc cheaply, update
pattern matches in ChangeLogSummary to match on the enum discriminant without
forcing a deep clone, and remove the #[allow(clippy::large_enum_variant)]
suppression; relevant symbols: ChangeLogSummary, StoredValue::get_value(),
change_type, DefaultConfigUpdateRequest, update_request, Value, Arc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d81689c-cbe7-4d3a-92c6-63630c54b012
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlcrates/cac_client/src/lib.rscrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/context/operations.rscrates/context_aware_config/src/api/context/validations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/context_aware_config/src/helpers.rscrates/context_aware_config/src/validation_functions.rscrates/experimentation_client/src/lib.rscrates/frontend/src/components/default_config_form.rscrates/superposition/src/webhooks/handlers.rscrates/superposition_core/src/helpers.rsnix/rust.nix
| rs-snowflake = "0.6.0" | ||
| serde = { version = "^1", features = ["derive"] } | ||
| rustyscript = { version = "0.12.3", default-features = false, features = ["safe_extensions", "web", "worker"] } | ||
| serde = { version = "=1.0.221", features = ["derive"] } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rustyscript 0.12.3 serde compatibility requirements
💡 Result:
rustyscript v0.12.3 depends on serde = "^1.0.149" (no extra serde features specified). That means it’s compatible with Serde 1.x, minimum 1.0.149 (i.e., >= 1.0.149 and < 2.0.0). [1]
Sources:
[1] (docs.rs)
Citations:
Remove the exact version pin on serde.
Using =1.0.221 is unnecessarily restrictive. While rustyscript 0.12.3 is compatible with serde 1.0.221 (it requires ^1.0.149), pinning to an exact version prevents patch-level updates that may include bug fixes or security improvements. Use ^1.0.221 instead to allow compatible patch updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 76, The serde dependency is pinned to an exact version
string "=1.0.221"; change this to a caret requirement so patch updates are
allowed (replace the exact pin with "^1.0.221" for the serde entry in
Cargo.toml) to permit compatible patch-level updates while preserving the
major/minor constraint.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let ctx_condition = req.context.clone().into_inner(); | ||
|
|
||
| let dimension_data_map = validate_ctx( | ||
| &mut conn, | ||
| &workspace_context, | ||
| ctx_condition, | ||
| Overrides::default(), | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Validate the move against the source overrides, not {}.
operations::r#move preserves the existing override set, but this pre-check always passes Overrides::default(). Any validation function that reads environment.overrides can be bypassed or can fail for the wrong reason. Load the current context first and validate with its override_ map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 345 -
354, The pre-check calls validate_ctx with Overrides::default(), which bypasses
the actual overrides preserved by operations::r#move; instead, load the current
context from req.context (or via the same code path that yields the environment)
to obtain its override_ map and pass that into validate_ctx (replace
Overrides::default() with the loaded context's overrides), so validation reads
environment.overrides exactly as operations::r#move will use them.
| ContextAction::Replace(update_request) => { | ||
| let change_reason = update_request.change_reason.clone(); | ||
| let (context_id, context) = match &update_request.context { | ||
| Identifier::Context(context) => { | ||
| let ctx_value: Map<String, Value> = | ||
| context.clone().into_inner().into(); | ||
| (hash(&Value::Object(ctx_value.clone())), ctx_value) | ||
| } | ||
| Identifier::Id(i) => { | ||
| let ctx_value: Context = dsl::contexts | ||
| .filter(dsl::id.eq(i.clone())) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .get_result::<Context>(&mut conn)?; | ||
| (i.clone(), ctx_value.value.into()) | ||
| } | ||
| }; | ||
| let r_override = update_request.override_.clone().into_inner(); | ||
|
|
||
| validate_override_with_functions( | ||
| &workspace_context, | ||
| &mut conn, | ||
| &r_override, | ||
| &context, | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; | ||
|
|
||
| prepared_ops.push(PreparedOperation::Replace { | ||
| update_request, | ||
| context_id, | ||
| change_reason, | ||
| }); |
There was a problem hiding this comment.
Bulk replace/move skip validate_change_reason.
The Put arm enforces change-reason validation, but Replace and Move do not. That makes /bulk-operations accept updates that /overrides and /move/{ctx_id} reject when change reasons are required.
Also applies to: 806-817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 770 -
801, The Replace and Move branches (ContextAction::Replace and
ContextAction::Move) are missing the same change-reason enforcement applied in
the Put branch; call validate_change_reason using the
update_request.change_reason (and the workspace_context and conn) before pushing
PreparedOperation::Replace or PreparedOperation::Move, so Replace/Move fail when
change reasons are required just like Put; ensure you validate prior to
validate_override_with_functions/other validations where appropriate and
propagate any error consistently.
| let ctx_condition = move_req.context.clone().into_inner(); | ||
|
|
||
| let dimension_data_map = validate_ctx( | ||
| &mut conn, | ||
| &workspace_context, | ||
| ctx_condition, | ||
| Overrides::default(), | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Bulk move has the same empty-override validation gap.
This path also calls validate_ctx(..., Overrides::default(), ...) even though the move keeps the source context's overrides. Please validate against the existing context's override_ map here too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 819 -
828, The validate_ctx call for bulk move is using Overrides::default() which
ignores the source context's overrides; instead extract the existing context's
override_ map (from workspace_context or the source context structure) and pass
those overrides into validate_ctx (replace Overrides::default() with the
overrides constructed from workspace_context.override_ or equivalent) so
validation uses the current context's override values; keep the rest of the
parameters (move_req.context -> ctx_condition, state.master_encryption_key)
unchanged and ensure the override map is converted to the same Overrides type
expected by validate_ctx.
| if let Some(fc) = &req.draft_code { | ||
| let handle = rustyscript::tokio::runtime::Handle::current(); | ||
| let function = fc.clone(); | ||
| handle | ||
| .spawn_blocking(move || compile_fn(&function)) | ||
| .await | ||
| .map_err(|err| bad_argument!("Invalid function code: {}", err))??; |
There was a problem hiding this comment.
Inconsistent error handling: JoinError should map to 500, not 400.
A JoinError from spawn_blocking indicates task panic or cancellation—an internal server error, not a bad user request. In create_handler (line 84), this correctly maps to unexpected_error (500), but here it maps to bad_argument (400).
The compilation error from compile_fn (the second ?) should be bad_argument, but the task join failure (first ?) should be unexpected_error.
🐛 Proposed fix
if let Some(fc) = &req.draft_code {
- let handle = rustyscript::tokio::runtime::Handle::current();
let function = fc.clone();
- handle
- .spawn_blocking(move || compile_fn(&function))
+ tokio::task::spawn_blocking(move || compile_fn(&function))
.await
- .map_err(|err| bad_argument!("Invalid function code: {}", err))??;
+ .map_err(|e| {
+ log::error!("Function compilation task failed: {:?}", e);
+ unexpected_error!("Failed to compile function: {}", e)
+ })?
+ .map_err(|err| bad_argument!("Invalid function code: {}", err))?;
}📝 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 let Some(fc) = &req.draft_code { | |
| let handle = rustyscript::tokio::runtime::Handle::current(); | |
| let function = fc.clone(); | |
| handle | |
| .spawn_blocking(move || compile_fn(&function)) | |
| .await | |
| .map_err(|err| bad_argument!("Invalid function code: {}", err))??; | |
| if let Some(fc) = &req.draft_code { | |
| let function = fc.clone(); | |
| tokio::task::spawn_blocking(move || compile_fn(&function)) | |
| .await | |
| .map_err(|e| { | |
| log::error!("Function compilation task failed: {:?}", e); | |
| unexpected_error!("Failed to compile function: {}", e) | |
| })? | |
| .map_err(|err| bad_argument!("Invalid function code: {}", err))?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/functions/handlers.rs` around lines 153 -
159, The join error from spawn_blocking should be treated as an internal server
error while the compile_fn result should remain a bad-argument error: change the
map_err on the await of
rustyscript::tokio::runtime::Handle::current().spawn_blocking(...).await to
convert JoinError into unexpected_error (500) instead of bad_argument (400),
then apply the second ? to propagate compile_fn's error as bad_argument; locate
the block referencing compile_fn, spawn_blocking, and the current map_err and
split the error handling so JoinError maps to unexpected_error and the
compile_fn result maps to bad_argument.
| pub fn run_js_function( | ||
| code: FunctionCode, | ||
| args: FunctionExecutionRequest, | ||
| runtime_version: FunctionRuntimeVersion, | ||
| ) -> String { | ||
| let payload = match runtime_version { | ||
| FunctionRuntimeVersion::V1 => FunctionPayload { | ||
| version: runtime_version, | ||
| payload: function_args.clone(), | ||
| }, | ||
| }; | ||
| ) -> Result<FunctionExecutionResponse, (String, Option<String>)> { | ||
| let wrapped_code = generate_wrapped_code(&code.0); | ||
| log::trace!("Running function code: {:?}", wrapped_code); | ||
|
|
||
| let module = Module::new("function.js", &wrapped_code); | ||
|
|
||
| let output_check = match function_args { | ||
| FunctionExecutionRequest::ValueValidationFunctionRequest { .. } => "output!=true", | ||
| FunctionExecutionRequest::ValueComputeFunctionRequest { .. } => { | ||
| "!(Array.isArray(output))" | ||
| } | ||
| FunctionExecutionRequest::ContextValidationFunctionRequest { .. } => { | ||
| "output!=true" | ||
| } | ||
| FunctionExecutionRequest::ChangeReasonValidationFunctionRequest { .. } => { | ||
| "output!=true" | ||
| } | ||
| let runtime_options = RuntimeOptions { | ||
| timeout: Duration::from_millis(1500), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| FUNCTION_EXECUTION_SNIPPET | ||
| .replace("{condition}", output_check) | ||
| .replace( | ||
| "{function-invocation}", | ||
| &format!( | ||
| "{}({})", | ||
| FUNCTION_NAME, | ||
| serde_json::to_string(&payload).unwrap_or("Invalid Payload".to_string()) | ||
| ), | ||
| ) | ||
| .replace(CODE_TOKEN, code_str) | ||
| } | ||
| let mut runtime = Runtime::new(runtime_options).map_err(|e| { | ||
| let err_str = e.to_string(); | ||
| (format!("Failed to create runtime: {}", err_str), None) | ||
| })?; | ||
|
|
||
| fn generate_wrapper_runtime(code_str: &str) -> String { | ||
| CODE_GENERATION_SNIPPET | ||
| .replace(FUNCTION_ENV_TOKEN, FUNCTION_ENV_VARIABLES) | ||
| .replace(CODE_TOKEN, code_str) | ||
| let payload = FunctionPayload { | ||
| version: runtime_version, | ||
| payload: args.clone(), | ||
| }; | ||
| let module_handle = runtime | ||
| .load_module(&module) | ||
| .map_err(|err| (err.to_string(), None))?; | ||
|
|
||
| let tokio_runtime = runtime.tokio_runtime(); | ||
|
|
||
| let fn_output = tokio_runtime | ||
| .block_on(async { | ||
| runtime | ||
| .call_function_async::<serde_json::Value>( | ||
| Some(&module_handle), | ||
| "execute", | ||
| json_args!(payload), | ||
| ) | ||
| .await | ||
| }) | ||
| .map_err(|err| (err.to_string(), None))?; | ||
|
|
||
| let stdout = runtime | ||
| .call_function::<Vec<String>>(Some(&module_handle), "getLogBuffer", json_args!()) | ||
| .map_err(|err| (err.to_string(), None))? | ||
| .join("\n"); | ||
| runtime | ||
| .call_function::<()>(Some(&module_handle), "clearLogBuffer", json_args!()) | ||
| .map_err(|err| (err.to_string(), None))?; | ||
| let function_type = FunctionType::from(&args); | ||
| log::trace!("Function output: {:?}", fn_output); | ||
| log::trace!("Function logs: {}", stdout); | ||
| Ok(FunctionExecutionResponse { | ||
| fn_output, | ||
| stdout, | ||
| function_type, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Drop the trace logs that print generated JS and execution output.
At this point code is already post-inject_secrets_and_variables_into_code, so wrapped_code can contain plaintext secrets. fn_output and stdout are also sensitive/user-controlled. Logging any of them leaks data into application logs.
Safer logging direction
- log::trace!("Running function code: {:?}", wrapped_code);
+ log::trace!("Running JS function");
@@
- log::trace!("Function output: {:?}", fn_output);
- log::trace!("Function logs: {}", stdout);
+ log::trace!("JS function execution completed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/validation_functions.rs` around lines 71 -
128, In run_js_function remove or redact the trace logs that expose sensitive JS
and execution data: delete or change the log::trace! calls that reference
wrapped_code ("Running function code: {:?}"), fn_output ("Function output:
{:?}"), and stdout ("Function logs: {}") so they no longer print raw or
user-controlled content; if you need telemetry keep only non-sensitive markers
(e.g., log a static message or a hash/boolean success flag) and ensure
wrapped_code, fn_output, and stdout are never logged in plain text.
2fb81b7 to
4f91805
Compare
Datron
force-pushed
the
rustyscript-functions
branch
from
4f91805 to
7973a26
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the NodeJS shell-based JavaScript execution path with rustyscript (embedded Deno runtime) and updates relevant handlers/validation flows to work with async validation and blocking execution where needed.
Changes:
- Introduces
rustyscriptas the JS runtime for function validation/execution (replacingnode -e/worker-thread snippets). - Refactors multiple API handlers/validators to
asyncand to.awaitchange-reason/function validations. - Adjusts context bulk operations to a two-phase “prepare then single transaction” flow.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| nix/rust.nix | Minor Nix expression cleanup for Darwin build inputs. |
| crates/superposition_core/src/helpers.rs | Simplifies error logging formatting. |
| crates/superposition/src/webhooks/handlers.rs | Awaits async validate_change_reason in webhook create/update. |
| crates/frontend/src/components/default_config_form.rs | Suppresses Clippy large enum variant warning. |
| crates/experimentation_client/src/lib.rs | Simplifies OK-status log formatting. |
| crates/context_aware_config/src/validation_functions.rs | Replaces NodeJS execution with rustyscript runtime + wrapper logging capture. |
| crates/context_aware_config/src/helpers.rs | Makes validate_change_reason async and routes through async function execution. |
| crates/context_aware_config/src/api/variables/handlers.rs | Awaits async change-reason validation. |
| crates/context_aware_config/src/api/type_templates/handlers.rs | Awaits async change-reason validation. |
| crates/context_aware_config/src/api/functions/handlers.rs | Runs function compile/test with blocking offload and async change-reason validation. |
| crates/context_aware_config/src/api/dimension/handlers.rs | Awaits async change-reason validation. |
| crates/context_aware_config/src/api/default_config/handlers.rs | Awaits async change-reason / function validations. |
| crates/context_aware_config/src/api/context/validations.rs | Minor logging formatting change for dimension values. |
| crates/context_aware_config/src/api/context/operations.rs | Refactors to accept prepared context data and externalized validations. |
| crates/context_aware_config/src/api/context/helpers.rs | Converts function validations and context creation/validation helpers to async + rustyscript execution. |
| crates/context_aware_config/src/api/context/handlers.rs | Refactors create/update/move/bulk context flows for async validation & prepared ops. |
| crates/context_aware_config/src/api/config/handlers.rs | Updates config reduction path to use async context creation helper. |
| crates/context_aware_config/Cargo.toml | Adds rustyscript dependency. |
| crates/cac_client/src/lib.rs | Simplifies OK-status log formatting. |
| Cargo.toml | Adds rustyscript dependency and pins serde version. |
| .env.example | Updates default RUST_LOG configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| .await | ||
| }) | ||
| .map_err(|err| (err.to_string(), None))?; |
There was a problem hiding this comment.
On execution errors (e.g., call_function_async), this function returns (err.to_string(), None) and drops any JS logs captured in logBuffer. This is a regression from the previous node-based path where stdout was returned on failure, and it makes debugging much harder. Consider attempting to call getLogBuffer in error paths and returning it as Some(stdout) when available.
| .map_err(|err| (err.to_string(), None))?; | |
| .map_err(|err| { | |
| // Attempt to preserve any JS logs captured in the log buffer on error. | |
| let stdout = runtime | |
| .call_function::<Vec<String>>(Some(&module_handle), "getLogBuffer", json_args!()) | |
| .map(|logs| logs.join("\n")) | |
| .ok(); | |
| // Best-effort cleanup; ignore any error from clearing the buffer. | |
| let _ = runtime | |
| .call_function::<()>(Some(&module_handle), "clearLogBuffer", json_args!()); | |
| (err.to_string(), stdout) | |
| })?; |
| let handle = rustyscript::tokio::runtime::Handle::current(); | ||
| let result = handle | ||
| .spawn_blocking(move || { | ||
| execute_fn( | ||
| &workspace_context, | ||
| &code, | ||
| &req, | ||
| version, | ||
| &mut conn, | ||
| &state.master_encryption_key, | ||
| ) | ||
| }) | ||
| .await | ||
| .map_err(|e| unexpected_error!("Function execution task failed: {}", e))? | ||
| .map_err(|(e, stdout)| { | ||
| bad_argument!( | ||
| "Function failed with error: {}, stdout: {:?}", | ||
| e, | ||
| stdout.unwrap_or_default() | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
This spawn_blocking closure captures/moves the Diesel PooledConnection (conn). Diesel PgConnection/PooledConnection is not Send, so this is very likely to fail to compile because spawn_blocking requires a Send + 'static closure. A safer approach is to do the DB-dependent work (injecting secrets/variables) on the current thread, then spawn_blocking only the JS execution (run_js_function) which is DB-independent.
| let handle = rustyscript::tokio::runtime::Handle::current(); | |
| let result = handle | |
| .spawn_blocking(move || { | |
| execute_fn( | |
| &workspace_context, | |
| &code, | |
| &req, | |
| version, | |
| &mut conn, | |
| &state.master_encryption_key, | |
| ) | |
| }) | |
| .await | |
| .map_err(|e| unexpected_error!("Function execution task failed: {}", e))? | |
| .map_err(|(e, stdout)| { | |
| bad_argument!( | |
| "Function failed with error: {}, stdout: {:?}", | |
| e, | |
| stdout.unwrap_or_default() | |
| ) | |
| })?; | |
| let result = execute_fn( | |
| &workspace_context, | |
| &code, | |
| &req, | |
| version, | |
| &mut conn, | |
| &state.master_encryption_key, | |
| ) | |
| .map_err(|(e, stdout)| { | |
| bad_argument!( | |
| "Function failed with error: {}, stdout: {:?}", | |
| e, | |
| stdout.unwrap_or_default() | |
| ) | |
| })?; |
| ContextAction::Replace(update_request) => { | ||
| let change_reason = update_request.change_reason.clone(); | ||
| let (context_id, context) = match &update_request.context { | ||
| Identifier::Context(context) => { | ||
| let ctx_value: Map<String, Value> = | ||
| context.clone().into_inner().into(); | ||
| (hash(&Value::Object(ctx_value.clone())), ctx_value) | ||
| } | ||
| Identifier::Id(i) => { | ||
| let ctx_value: Context = dsl::contexts | ||
| .filter(dsl::id.eq(i.clone())) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .get_result::<Context>(&mut conn)?; | ||
| (i.clone(), ctx_value.value.into()) | ||
| } | ||
| }; | ||
| let r_override = update_request.override_.clone().into_inner(); | ||
|
|
||
| validate_override_with_functions( | ||
| &workspace_context, | ||
| &mut conn, | ||
| &r_override, | ||
| &context, | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; | ||
|
|
||
| prepared_ops.push(PreparedOperation::Replace { | ||
| update_request, | ||
| context_id, | ||
| change_reason, | ||
| }); | ||
| } |
There was a problem hiding this comment.
In bulk operations, ContextAction::Replace does not call validate_change_reason, even though single-update endpoints do. This bypasses change-reason validation when enable_change_reason_validation is enabled. Add a validate_change_reason(...).await? for replace operations before preparing the transaction op.
| ContextAction::Move { | ||
| id: old_ctx_id, | ||
| request: move_req, | ||
| } => { | ||
| let description = match move_req.description.clone() { | ||
| Some(val) => val, | ||
| None => query_description( | ||
| Value::Object(move_req.context.clone().into_inner().into()), | ||
| &mut conn, | ||
| &workspace_context.schema_name, | ||
| )?, | ||
| }; | ||
|
|
||
| let ctx_condition = move_req.context.clone().into_inner(); | ||
|
|
||
| let dimension_data_map = validate_ctx( | ||
| &mut conn, | ||
| &workspace_context, | ||
| ctx_condition, | ||
| Overrides::default(), | ||
| &state.master_encryption_key, | ||
| &internal_user, | ||
| ) | ||
| .await?; | ||
|
|
||
| prepared_ops.push(PreparedOperation::Move { | ||
| old_ctx_id, | ||
| move_req, | ||
| description, | ||
| dimension_data_map, | ||
| }); | ||
| } |
There was a problem hiding this comment.
In bulk operations, ContextAction::Move does not call validate_change_reason, even though the dedicated move endpoint does. This bypasses change-reason validation for move requests. Add validate_change_reason(...).await? for move operations during the preparation phase.
| validate_override_with_functions( | ||
| &workspace_context, | ||
| &mut conn, | ||
| &r_override, | ||
| &context, | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; | ||
|
|
||
| let (override_resp, version_id) = | ||
| conn.transaction::<_, superposition::AppError, _>(|transaction_conn| { | ||
| let override_resp = operations::update( | ||
| &workspace_context, | ||
| req.into_inner(), | ||
| transaction_conn, | ||
| &user, | ||
| &state.master_encryption_key, | ||
| context_id, | ||
| ) |
There was a problem hiding this comment.
validate_override_with_functions now runs outside the DB transaction that applies the update. This introduces a TOCTOU window where the data/functions being validated could change before the write transaction starts, leading to inconsistent behavior compared to the previous in-transaction validation. If strong consistency is required, consider moving validation into the transaction (e.g., by running the async JS validation before opening the transaction but re-checking any DB-derived inputs inside the transaction).
| export function typeCheck() {{ | ||
| if (typeof execute === "undefined") {{ | ||
| throw new Error("execute function is not defined"); | ||
| }} |
There was a problem hiding this comment.
compile_fn's typeCheck only checks whether execute is defined, not that it's a function. This allows invalid code like const execute = 1; to pass compilation and then fail at runtime. Update the JS check to assert typeof execute === "function" (and ideally ensure it’s callable/async as expected).
| }} | |
| }} | |
| if (typeof execute !== "function") {{ | |
| throw new Error("execute is not a function"); | |
| }} |
| }; | ||
| ) -> Result<FunctionExecutionResponse, (String, Option<String>)> { | ||
| let wrapped_code = generate_wrapped_code(&code.0); | ||
| log::trace!("Running function code: {:?}", wrapped_code); |
There was a problem hiding this comment.
run_js_function logs the fully wrapped JS source at trace level. This will include injected secrets/variables and can leak sensitive data into logs (and can also be very large). Consider removing this log or redacting secrets / logging only a hash/ID of the function instead.
| log::trace!("Running function code: {:?}", wrapped_code); | |
| log::trace!("Running function code of length {}", wrapped_code.len()); |
Datron
force-pushed
the
rustyscript-functions
branch
3 times, most recently
from
ddebb64 to
3349ce1
Compare
Signed-off-by: datron <Datron@users.noreply.github.com>
Datron
force-pushed
the
rustyscript-functions
branch
from
3349ce1 to
8e738f8
Compare
|
@Datron there is a merge conflict in this file : Can you resolve ? |
| handle | ||
| .spawn_blocking(move || compile_fn(&function)) | ||
| .await | ||
| .map_err(|err| bad_argument!("Invalid function code: {}", err))??; |
There was a problem hiding this comment.
Can we handle the error in the similar way as we did it below.
| let result = handle | ||
| .spawn_blocking(move || run_js_function(code, args_owned, runtime_version)) | ||
| .await | ||
| .map_err(|e| { |
There was a problem hiding this comment.
Here as well , we have inconsistent error handling pattern, Can we unify it ?
Problem
We are using a nodeJS runtime via the shell to execute JS code within superposition. This setup has limitations on the size of responses, type validations and is fragile when making modifications
Solution
Replace the node runtime with rustyscript, a crate that embeds a deno runtime within rust. This makes it easy to run JS code through rust - with type validations, getting the right response and allows us to properly add on libraries like crypto and fetch
Post-deployment activity
Any JS functions using axios should be moved to use fetch
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores