-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement cwl js expressions executor #6
feat: implement cwl js expressions executor #6
Conversation
WalkthroughThis pull request represents a significant restructuring of the Zefiro project's CWL (Common Workflow Language) parsing and handling components. The primary changes involve renaming the Changes
Sequence DiagramsequenceDiagram
participant User
participant CWLSchema
participant JsExecutor
participant DenoRuntime
User->>CWLSchema: Parse CWL Document
CWLSchema->>JsExecutor: Create with inputs
JsExecutor->>DenoRuntime: Initialize Runtime
User->>JsExecutor: Execute JavaScript Expression
JsExecutor->>DenoRuntime: Run Script
DenoRuntime-->>JsExecutor: Return Result
JsExecutor-->>User: Provide Evaluated Result
Possibly related issues
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Nitpick comments (5)
zefiro-pipeline-engine/zefiro-cwl-js-exec/src/exec.rs (1)
41-79
: Add more test cases for edge cases.While the current tests are good, consider adding tests for:
- Invalid JavaScript syntax
- Execution timeouts
- Large input values
- Error cases
Example test case:
#[test] fn test_jsexecutor_invalid_syntax() { let mut executor = JsExecutor::new( &json!({}), &json!({}) ).expect("Failed to initialize JavaScript engine"); let result = executor.run("invalid { syntax"); assert!(result.is_err()); }zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/requirements.rs (1)
Line range hint
71-73
: Consider enhancing InlineJavascriptRequirement for better JS support.The
InlineJavascriptRequirement
struct is currently empty. Consider adding fields to support JavaScript expression configuration, such as:
- expressionLib: for shared JavaScript expressions
- engineConfig: for JavaScript engine configuration
-pub struct InlineJavascriptRequirement; +#[derive(Clone, Debug, Deserialize, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct InlineJavascriptRequirement { + #[serde(default)] + pub expression_lib: Option<Vec<String>>, + #[serde(default)] + pub engine_config: Option<HashMap<String, Value>> +}zefiro-pipeline-engine/zefiro-cwl-parser/src/values/types.rs (1)
101-113
: Consider adding validation for CWL value types.The
CwlValueType
enum should validate numeric ranges according to CWL spec:pub enum CwlValueType { - Int(i32), - Long(i64), - Float(f32), - Double(f64), + Int(#[serde(with = "validate_int")] i32), + Long(#[serde(with = "validate_long")] i64), + Float(#[serde(with = "validate_float")] f32), + Double(#[serde(with = "validate_double")] f64), String(String), Path(CwlPath), Array(Vec<Self>), }zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/document.rs (1)
45-48
: Enhance version validation error message.The version validation error message could be more helpful by indicating the supported version:
ensure!( MINIMAL_CWL_VERSION == version, - "Unsupported CWL version: {version}" + "Unsupported CWL version: {}. This implementation supports {} only", + version, MINIMAL_CWL_VERSION );zefiro-pipeline-engine/zefiro-cwl-parser/README.md (1)
27-27
: Update example paths to use consistent directory structureThe example paths should be consistent with the actual project structure. Consider using more descriptive test data paths.
- let schema = CwlSchema::from_path("test_data/cwl/clt-step-schema.yml").unwrap(); + let schema = CwlSchema::from_path("zefiro-pipeline-engine/zefiro-cwl-parser/test_data/cwl/clt-step-schema.yml").unwrap(); - let values = CwlValues::from_path("test_data/cwl/clt-step-values.yml").unwrap(); + let values = CwlValues::from_path("zefiro-pipeline-engine/zefiro-cwl-parser/test_data/cwl/clt-step-values.yml").unwrap();Also applies to: 73-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Cargo.toml
(1 hunks)README.md
(1 hunks)release.sh
(1 hunks)zefiro-core/zefiro-cwl-parser/README.md
(0 hunks)zefiro-core/zefiro-cwl-parser/examples/data/clt-step-values.yml
(0 hunks)zefiro-core/zefiro-cwl-parser/src/values/types.rs
(0 hunks)zefiro-pipeline-engine/Cargo.toml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js-exec/src/exec.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js-exec/src/lib.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml
(2 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/README.md
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/lib.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/command_line_tool.rs
(4 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/document.rs
(6 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/requirements.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/types.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/workflow.rs
(2 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/values/document.rs
(2 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/src/values/types.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/test_data/cwl/clt-step-values.yml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/test_data/inputs/file.txt
(1 hunks)
💤 Files with no reviewable changes (3)
- zefiro-core/zefiro-cwl-parser/README.md
- zefiro-core/zefiro-cwl-parser/examples/data/clt-step-values.yml
- zefiro-core/zefiro-cwl-parser/src/values/types.rs
✅ Files skipped from review due to trivial changes (6)
- README.md
- zefiro-pipeline-engine/zefiro-cwl-parser/test_data/inputs/file.txt
- zefiro-pipeline-engine/zefiro-cwl-parser/src/lib.rs
- zefiro-pipeline-engine/Cargo.toml
- zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml
- zefiro-pipeline-engine/zefiro-cwl-parser/src/values/document.rs
🧰 Additional context used
🪛 GitHub Actions: Continuous Integration
Cargo.toml
[error] Manifest path does not exist. The Cargo.toml file is missing for the zefiro-core package.
🪛 LanguageTool
zefiro-pipeline-engine/zefiro-cwl-parser/README.md
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...l_input).unwrap(); Ok(()) } ``` ### How to execute JavaScript expressions?
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (11)
zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/types.rs (1)
6-7
: LGTM! Good use of constants for CWL class types.The constants improve maintainability by centralizing string literals and follow Rust naming conventions.
zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/command_line_tool.rs (3)
1-2
: LGTM! Clean import organization and constant usage.Good use of the new constants and clear organization of imports.
32-32
: LGTM! Consistent use of constants in default methods.The default methods properly use the new constants, improving maintainability.
Also applies to: 36-36
51-51
: Verify impact of renamingCommandLineBinding
toInputBinding
.While the rename to
InputBinding
is more specific, we should verify all usages are updated.Also applies to: 75-75
✅ Verification successful
Rename from
CommandLineBinding
toInputBinding
is complete and consistent🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to CommandLineBinding rg "CommandLineBinding" --type rust # Search for all uses of InputBinding to verify consistent usage rg "InputBinding" --type rustLength of output: 434
zefiro-pipeline-engine/zefiro-cwl-js-exec/src/lib.rs (1)
1-3
: LGTM! Clean module organization.The module structure follows Rust best practices with clear public exports.
zefiro-pipeline-engine/zefiro-cwl-js-exec/src/exec.rs (1)
25-38
: LGTM! Good error handling with context.The error handling is well-implemented with clear context messages using anyhow.
zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/workflow.rs (2)
2-3
: LGTM! Clean import restructuring.The imports are properly organized, replacing the removed
SUPPORTED_CWL_VERSIONS
withMINIMAL_CWL_VERSION
.
29-29
: LGTM! Simplified version handling.The change from
SUPPORTED_CWL_VERSIONS
toMINIMAL_CWL_VERSION
simplifies version handling and makes the code more maintainable.Also applies to: 33-33
zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/requirements.rs (1)
4-4
: LGTM! Clear constant definition.The
MINIMAL_CWL_VERSION
constant is appropriately defined with a clear value of "v1.2".zefiro-pipeline-engine/zefiro-cwl-parser/src/schema/document.rs (1)
166-174
: LGTM! Good test coverage for error cases.The
FailingWriter
implementation properly simulates I/O errors for testing error handling.Cargo.toml (1)
9-11
:⚠️ Potential issueFix pipeline failure by removing old package reference
The CI pipeline is failing because it's still looking for the zefiro-core package. The workspace has been correctly restructured, but remnants of the old structure need to be cleaned up.
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] Manifest path does not exist. The Cargo.toml file is missing for the zefiro-core package.
zefiro-pipeline-engine/zefiro-cwl-parser/test_data/cwl/clt-step-values.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml (1)
24-25
: Consider additional testing toolsFor a JavaScript execution engine, consider adding:
proptest
for property-based testing of JavaScript expression handlingcargo-fuzz
for finding security issues through fuzzingExample addition:
[dev-dependencies] rstest = "0.24.0" +proptest = "1.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml
🔇 Additional comments (1)
zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml (1)
19-22
: Review security implications of JavaScript executionUsing
deno_core
for JavaScript execution requires careful consideration of security boundaries. Please ensure:
- JavaScript execution is properly sandboxed
- Input validation is implemented
- Resource limits are enforced
- Execution timeouts are configured
Let's check if there are any security-related configurations in the codebase:
✅ Verification successful
JavaScript execution security boundaries are appropriate
The current implementation provides adequate security isolation:
- JavaScript runtime is limited to expression evaluation
- No access to sensitive APIs, filesystem, or network
- Input/output strictly controlled through JSON serialization
- Execution context is isolated and minimal
While timeouts and resource limits could be added, they aren't critical given the limited scope of expression evaluation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security-related configurations rg -i "sandbox|timeout|limit" --type rustLength of output: 725
Script:
#!/bin/bash # Search for deno_core usage and configuration ast-grep --pattern 'deno_core::$_' # Also check for JsRuntime initialization ast-grep --pattern 'JsRuntime::$_' # And general error handling patterns rg "deno_core::(error|JsRuntime)" --type rust -A 3Length of output: 245
Script:
#!/bin/bash # Get the complete implementation of exec.rs cat zefiro-pipeline-engine/zefiro-cwl-js-exec/src/exec.rs # Look for any tests or examples that might show security practices fd -e rs -e md . zefiro-pipeline-engine/zefiro-cwl-js-exec --exec cat {}Length of output: 5302
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…-expressions-in-cwl-schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
zefiro-pipeline-engine/zefiro-cwl-js/src/exec.rs (2)
26-38
: Handle potential exceptions during script execution and result deserializationConsider adding more robust error handling when executing the script and deserializing the result to capture and relay JavaScript exceptions or parsing errors back to the caller.
You might enhance the
run
method to include detailed error messages:pub fn run(&mut self, script: &str) -> Result<String, Error> { let result = self .runtime .execute_script("<eval>", script.to_string()) - .context("Failed to execute JavaScript expression")?; + .with_context(|| format!("Failed to execute JavaScript expression:\n{}", script))?; let scope = &mut self.runtime.handle_scope(); let local_result = v8::Local::new(scope, result); let result_json: serde_json::Value = serde_v8::from_v8(scope, local_result) - .context("Failed to deserialize result")?; + .context("Failed to deserialize JavaScript execution result")?; Ok(result_json.to_string()) }This provides more context in error scenarios, aiding in debugging.
47-79
: Ensure test cases cover edge conditions and error handlingThe current tests validate expected successful executions. Consider adding test cases that cover edge conditions, such as invalid scripts, malformed inputs, or execution errors, to ensure the
JsExecutor
handles these gracefully.For example, add a test case for a script with a syntax error:
#[rstest] #[case( json!({}), json!({}), "this is not valid JavaScript!", "Error" )]And adjust the test to handle expected errors appropriately.
.github/workflows/cd.yml (1)
Line range hint
1-44
: Consider enhancing the CD workflow robustness.The workflow could benefit from additional safeguards and improvements:
- Add a test job as a prerequisite to publishing
- Add version verification to ensure the tag matches the crate version
- Pin the Rust toolchain version for reproducible builds
Example improvements:
jobs: test: name: Run tests runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 - uses: dtolnay/[email protected] # Pin Rust version - run: cargo test --all-features publish-crates-io: needs: [generate-changelog, test] # Add test dependency # ... existing config ... steps: # ... existing steps ... - name: Verify version matches tag run: | CARGO_VERSION=$(cargo metadata --manifest-path zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml --format-version 1 | jq -r '.packages[0].version') if [ "v$CARGO_VERSION" != "${{ env.RELEASE_VERSION }}" ]; then echo "Version mismatch: Cargo.toml ($CARGO_VERSION) != Tag (${{ env.RELEASE_VERSION }})" exit 1 fi🧰 Tools
🪛 yamllint (1.35.1)
[error] 44-44: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/cd.yml
(1 hunks)Cargo.toml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js/Cargo.toml
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js/README.md
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js/src/exec.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-js/src/lib.rs
(1 hunks)zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml
(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- zefiro-pipeline-engine/zefiro-cwl-js/README.md
- zefiro-pipeline-engine/zefiro-cwl-js/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml
🧰 Additional context used
🪛 GitHub Actions: Continuous Integration
Cargo.toml
[error] Manifest path does not exist. The Cargo.toml file is missing from the zefiro-core directory.
🔇 Additional comments (3)
zefiro-pipeline-engine/zefiro-cwl-js/src/lib.rs (1)
1-3
: Module structure and re-export look goodThe addition of the
exec
module and re-exportingJsExecutor
provides a clean interface for users. This enhances modularity and usability.Cargo.toml (1)
9-11
:⚠️ Potential issueUpdate CI/CD pipeline configurations to reflect new project structure
The pipeline failure indicates that the CI/CD configuration still references the removed
zefiro-core
directory. Update the pipeline scripts and any configuration files to align with the newzefiro-pipeline-engine
structure.Please investigate the CI/CD setup and modify paths accordingly. For example, update any references in
.github/workflows
and other CI scripts fromzefiro-core
tozefiro-pipeline-engine
.Run the following script to identify remnants of the old directory in configuration files:
This will help locate and correct outdated references.
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] Manifest path does not exist. The Cargo.toml file is missing from the zefiro-core directory.
.github/workflows/cd.yml (1)
36-36
: LGTM! Path updates align with project restructuring.The manifest path changes from
zefiro-core
tozefiro-pipeline-engine
correctly reflect the project restructuring.Also applies to: 43-44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
zefiro-core/zefiro-cwl/src/schema/requirements.rs (1)
Line range hint
78-79
: Consider enhancing InlineJavascriptRequirement for JS expression support.Given that this PR aims to implement JavaScript expression execution, the
InlineJavascriptRequirement
struct might need additional fields to support:
- Expression evaluation configuration
- JavaScript runtime settings
- Expression caching options
Would you like me to propose an enhanced implementation that includes these features?
🧹 Nitpick comments (7)
zefiro-core/zefiro-cwl/src/exprs/exec.rs (2)
29-29
: Avoid unnecessary cloning of the script stringCalling
script.to_string()
creates an unnecessary clone of the&str
. Sinceexecute_script
accepts a&str
, you can passscript
directly.Update the code as follows:
.execute_script("<eval>", script.to_string()) .context("Failed to execute JavaScript expression")?; + .execute_script("<eval>", script) + .context("Failed to execute JavaScript expression")?;
47-79
: Expand test coverage for error scenariosThe current tests cover typical use cases. To ensure robustness, add tests for error scenarios:
- Invalid JavaScript code execution.
- Malformed
cwl_inputs
orcwl_self
.- Scripts that return non-JSON-serializable values.
[unit_tests]
Adding these tests will help catch potential issues and improve the reliability of the
JsExecutor
.zefiro-core/zefiro-cwl/README.md (2)
65-65
: Add an article for clarity in the headingThe heading would be clearer with the article "a" included.
Change:
-### How to parse CWL Values document? +### How to parse a CWL Values document?🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: You might be missing the article “the” here.
Context: ...); Ok(()) }### How to parse CWL Values document?
rust use zefiro_cw...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
88-88
: Provide examples for executing JavaScript expressionsThe section "### How to execute JavaScript expressions?" is currently empty. Including examples will help users understand how to use this feature.
Consider adding sample code demonstrating how to execute JavaScript expressions using the
JsExecutor
.🧰 Tools
🪛 LanguageTool
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...l_input).unwrap(); Ok(()) } ``` ### How to execute JavaScript expressions?(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
zefiro-core/zefiro-cwl/src/values/types.rs (3)
1-32
: LGTM! Well-structured CWL File representation.The struct design follows CWL spec requirements with clear documentation and appropriate use of optional fields. The
Default
trait implementation is a good choice for optional fields.Consider adding custom error types instead of using raw
io::Error
to provide more context-specific error handling. This would help distinguish between file system errors and CWL-specific errors.#[derive(Debug, thiserror::Error)] pub enum CwlError { #[error("IO error: {0}")] Io(#[from] std::io::Error), #[error("Invalid path: {0}")] InvalidPath(String), // Add more error variants as needed }
82-92
: Enhance CwlDirectory with directory-specific functionality.The current implementation is minimal and could benefit from additional features.
Consider adding methods for:
- Directory existence validation
- Listing directory contents
- Directory size calculation
Example implementation:
impl CwlDirectory { pub fn exists(&self) -> bool { Path::new(&self.location).is_dir() } pub fn list_contents(&self) -> io::Result<Vec<CwlPath>> { // Implementation for listing directory contents } }
94-113
: LGTM! Well-designed type hierarchy with proper serialization.The enum design with appropriate Serde attributes ensures correct serialization/deserialization of CWL types.
Consider implementing common operations for CwlValueType:
impl CwlValueType { pub fn is_file(&self) -> bool { matches!(self, CwlValueType::Path(CwlPath::File(_))) } pub fn as_string(&self) -> Option<&String> { match self { CwlValueType::String(s) => Some(s), _ => None } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Cargo.toml
(1 hunks)zefiro-cli/Cargo.toml
(1 hunks)zefiro-core/Cargo.toml
(1 hunks)zefiro-core/zefiro-cwl/Cargo.toml
(1 hunks)zefiro-core/zefiro-cwl/README.md
(1 hunks)zefiro-core/zefiro-cwl/src/exprs/exec.rs
(1 hunks)zefiro-core/zefiro-cwl/src/exprs/mod.rs
(1 hunks)zefiro-core/zefiro-cwl/src/lib.rs
(1 hunks)zefiro-core/zefiro-cwl/src/schema/command_line_tool.rs
(4 hunks)zefiro-core/zefiro-cwl/src/schema/document.rs
(7 hunks)zefiro-core/zefiro-cwl/src/schema/requirements.rs
(1 hunks)zefiro-core/zefiro-cwl/src/schema/types.rs
(1 hunks)zefiro-core/zefiro-cwl/src/schema/workflow.rs
(2 hunks)zefiro-core/zefiro-cwl/src/values/document.rs
(4 hunks)zefiro-core/zefiro-cwl/src/values/types.rs
(1 hunks)zefiro-core/zefiro-cwl/test_data/cwl/clt-step-values.yml
(1 hunks)zefiro-core/zefiro-cwl/test_data/inputs/file.txt
(1 hunks)zefiro-ui/Cargo.toml
(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- zefiro-core/zefiro-cwl/test_data/inputs/file.txt
- zefiro-core/zefiro-cwl/src/lib.rs
- zefiro-core/zefiro-cwl/src/exprs/mod.rs
- zefiro-cli/Cargo.toml
- zefiro-ui/Cargo.toml
- zefiro-core/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🪛 LanguageTool
zefiro-core/zefiro-cwl/README.md
[uncategorized] ~65-~65: You might be missing the article “the” here.
Context: ...); Ok(()) } ### How to parse CWL Values document?
rust use zefiro_cw...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...l_input).unwrap(); Ok(()) } ``` ### How to execute JavaScript expressions?
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test suite
🔇 Additional comments (22)
zefiro-core/zefiro-cwl/Cargo.toml (2)
2-3
: Version downgrade requires documentation.The package has been renamed and version downgraded to "0.0.0-pre". This significant version change suggests a breaking change or major restructuring.
Please document the breaking changes and migration guide in the changelog or README.md:
✅ Verification successful
Version downgrade is appropriate and well-documented
The version
0.0.0-pre
correctly reflects the package's early development status. The README.md clearly documents that it "supports only some fields" and its current limitations, which is appropriate for a pre-release version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if breaking changes are documented rg -i "breaking.*changes|migration|upgrade" README.md CHANGELOG.mdLength of output: 121
Script:
#!/bin/bash # Find all markdown files fd -e md # Check if this is a recent addition git log --oneline -- zefiro-core/zefiro-cwl/Cargo.toml | head -n 5Length of output: 53
Script:
#!/bin/bash # Check package README content cat zefiro-core/zefiro-cwl/README.mdLength of output: 2106
19-24
: Review security implications of new dependencies.The addition of
deno_core
for JavaScript execution introduces new security considerations:
- Ensure proper sandboxing of JavaScript execution
- Consider adding security policy documentation
- Verify the stability of the specific Deno version
Additionally, verify if the exact versions of new dependencies (
serde_json
,sha1
) are required or if they can use version ranges for better flexibility.Run these checks:
✅ Verification successful
JavaScript execution is properly sandboxed and secure ✅
The implementation shows appropriate security measures:
- Uses Deno's V8 runtime with default security restrictions
- Execution is limited to expression evaluation without file system or network access
- Input/output is strictly controlled through JSON serialization
- Proper error handling and context isolation is implemented
The exact version pinning for dependencies is appropriate for security-critical components. No additional security documentation is required as the implementation follows CWL specification requirements for JavaScript expression evaluation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories in new dependencies cargo audit # Look for existing security documentation rg -i "security|sandbox|javascript.*execution" docs/ README.mdLength of output: 220
Script:
#!/bin/bash # Search for any documentation files fd -e md -e txt # Look for security-related content in all text files rg -i "security|sandbox|javascript" --type-add 'text:*.{md,txt,toml,rs}' -t text # Look for Deno configuration and initialization ast-grep --pattern 'deno_core::$_' # Look for JavaScript execution patterns ast-grep --pattern 'execute($$$)'Length of output: 1695
Script:
#!/bin/bash # Check JavaScript execution implementation cat zefiro-core/zefiro-cwl/src/exprs/exec.rs # Look for test files to verify security measures fd -e rs testLength of output: 2620
zefiro-core/zefiro-cwl/src/exprs/exec.rs (1)
35-35
: Handle potential deserialization errorsWhen deserializing the JavaScript execution result, any non-JSON-serializable values could cause an error. This might occur if the script returns
undefined
, functions, or symbols.Consider adding error handling to manage cases where the result is not JSON-serializable. You could return an appropriate error message or handle specific cases as needed.
zefiro-core/zefiro-cwl/src/schema/types.rs (1)
6-7
: LGTM! Good use of constants for CWL class names.The introduction of
WF_CWL_CLASS
andCLT_CWL_CLASS
constants improves maintainability by centralizing the class names and preventing typos.zefiro-core/zefiro-cwl/src/schema/command_line_tool.rs (2)
1-2
: LGTM! Consistent use of minimal version and class constants.The changes align with the new versioning strategy using
MINIMAL_CWL_VERSION
and class constants.
51-51
: Verify impact of renaming CommandLineBinding to InputBinding.While the rename to
InputBinding
is more semantically accurate, this change could break external code depending on theCommandLineBinding
type.Also applies to: 75-75
✅ Verification successful
Rename from CommandLineBinding to InputBinding is safe to proceed
The package is in pre-release state (version 0.0.0-pre) and the type rename is clean with no remaining references to the old name in the codebase. The only occurrence of "CommandLineBinding" is in a documentation URL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to CommandLineBinding rg "CommandLineBinding" -t rustLength of output: 165
Script:
#!/bin/bash # Find Cargo.toml files in zefiro-cwl fd Cargo.toml --type f --exec cat {} \; -p zefiro-cwlLength of output: 156
Script:
#!/bin/bash # Find and show Cargo.toml content cat zefiro-core/zefiro-cwl/Cargo.toml 2>/dev/null || echo "Cargo.toml not found" # Search for both old and new type names in rust files echo "=== Searching for InputBinding usage ===" rg "InputBinding" -t rust echo -e "\n=== Searching for CommandLineBinding struct/type declarations or uses ===" ast-grep --pattern 'struct CommandLineBinding' -l rust ast-grep --pattern 'type CommandLineBinding' -l rustLength of output: 1367
zefiro-core/zefiro-cwl/src/schema/workflow.rs (2)
2-3
: LGTM! Consistent use of minimal version and class constants.The changes align with the new versioning strategy using
MINIMAL_CWL_VERSION
andWF_CWL_CLASS
.
29-29
: LGTM! Default methods updated correctly.The default methods now use the new constants consistently.
Also applies to: 33-33
zefiro-core/zefiro-cwl/src/schema/requirements.rs (1)
4-4
: LGTM! Consistent version constant.The
MINIMAL_CWL_VERSION
constant is correctly defined and aligns with the CWL spec version v1.2.zefiro-core/zefiro-cwl/src/values/document.rs (4)
31-32
: LGTM: Import and example path updatesThe import path and example file path have been correctly updated to reflect the new project structure.
52-52
: LGTM: Import path updateImport path correctly updated to use the new package name.
81-81
: LGTM: Import path update in documentationDocumentation example correctly updated to use the new package name.
108-108
: LGTM: Test file paths updateTest case file paths have been properly updated to match the new project structure.
Also applies to: 114-114
zefiro-core/zefiro-cwl/src/schema/document.rs (6)
2-5
: LGTM: Import organizationImports are well-organized and the new constants improve code maintainability.
51-52
: LGTM: Use of constants for class typesGood use of constants for class types improves maintainability and reduces the risk of typos.
166-174
: LGTM: Error handling improvementGood addition of the
FailingWriter
struct for testing error cases. This improves test coverage for error scenarios.
176-179
: LGTM: Write error test coverageGood test coverage for write error scenarios.
Line range hint
1-187
: Missing JavaScript expression execution implementationThe PR objectives mention implementing JavaScript expression execution in CWL schema, but the changes in this file don't include this functionality.
Let's check if the implementation exists elsewhere:
46-47
: Verify CWL version compatibility impactThe change from supporting multiple CWL versions to requiring an exact version match might impact compatibility with existing workflows.
Run this script to check for CWL version usage across the codebase:
zefiro-core/zefiro-cwl/test_data/cwl/clt-step-values.yml (1)
1-4
: Verify test data file existenceThe configuration looks correct, but we should verify the referenced test files exist.
✅ Verification successful
Test data configuration is correct ✅
The input file exists at the correct location (
zefiro-core/zefiro-cwl/test_data/inputs/file.txt
), and the output file path is properly configured for test execution. The missing output file is expected as it will be generated during the test run.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if referenced test files exist echo "Checking for referenced test files:" ls -l test_data/inputs/file.txt test_data/inputs/output.txt 2>/dev/null || echo "Warning: Referenced test files not found"Length of output: 233
Script:
#!/bin/bash # Search for similar test files in the repository echo "Searching for test data files:" fd -t f "file.txt|output.txt" --exclude node_modules echo -e "\nChecking test_data directory structure:" fd . test_data -t dLength of output: 388
zefiro-core/zefiro-cwl/src/values/types.rs (2)
1-113
: Verify CWL specification compliance.Please ensure the implementation aligns with the CWL specification by running conformance tests.
Run the following script to check for CWL spec files in the repository:
✅ Verification successful
Implementation aligns with documented CWL v1.2 subset support
The types implementation correctly follows the CWL v1.2 specification for the documented subset of features:
- File and Directory objects contain required fields as per spec
- Value types cover all basic CWL data types
- Implementation is explicitly scoped to support only essential fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for CWL conformance tests and specification files # Search for CWL test files echo "Searching for CWL test files..." fd -e cwl -e yaml -e yml . | grep -i "test\|conformance" # Search for specification references echo "Searching for CWL spec references..." rg -i "cwl.*specification\|commonwl.org"Length of output: 427
Script:
#!/bin/bash # Description: Check documentation and test files for CWL specification details # Check README and documentation files echo "Checking documentation..." fd README -t f fd -e md . | xargs cat # Check test file contents echo -e "\nChecking test file contents..." cat zefiro-core/zefiro-cwl/test_data/cwl/clt-step-schema.yml cat zefiro-core/zefiro-cwl/test_data/cwl/clt-step-values.yml # Look for CWL version references echo -e "\nChecking for CWL version references..." rg -i "cwl.*version|cwlVersion"Length of output: 6010
46-51
: Add path validation in extract_path_info.The method accepts any string as a path without validation.
Add basic path validation to prevent potential security issues:
pub fn extract_path_info<F, T>(path: &str, provided: Option<T>, extractor: F) -> Option<T> where F: Fn(&Path) -> Option<T>, { if path.contains("..") { return None; // Prevent directory traversal } provided.or_else(|| extractor(Path::new(path))) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
zefiro-core/zefiro-cwl/src/exprs/exec.rs (1)
14-16
:⚠️ Potential issueSanitize inputs to prevent code injection
The current implementation directly interpolates
cwl_inputs
andcwl_self
into the JavaScript code usingformat!
, which may lead to code injection if these values contain malicious content.Apply this diff to fix the issue:
let init_script = format!( - r#"const inputs = {}; const self = {};"#, - cwl_inputs, cwl_self + r#"const inputs = JSON.parse('{}'); const self = JSON.parse('{}');"#, + serde_json::to_string(cwl_inputs)?, serde_json::to_string(cwl_self)? );
🧹 Nitpick comments (5)
zefiro-core/zefiro-cwl/src/exprs/exec.rs (1)
47-78
: Add test cases for edge cases and error scenariosCurrent tests only cover happy paths. Consider adding tests for:
- Empty/null inputs
- Invalid JSON
- Malformed JavaScript
- Script timeout
- Memory exhaustion
Example test cases:
#[rstest] #[case( json!(null), json!(null), "inputs;", "null" )] #[case( json!({}), json!([]), "while(true) {}", "TimeoutError" // Should panic with timeout )] #[should_panic(expected = "SyntaxError")] #[case( json!({}), json!([]), "invalid js syntax {", "" )]zefiro-core/zefiro-cwl/README.md (3)
27-28
: Improve error handling in examplesReplace
unwrap()
with proper error handling to demonstrate best practices:- let schema = CwlSchema::from_path("test_data/cwl/clt-step-schema.yml").unwrap(); + let schema = CwlSchema::from_path("test_data/cwl/clt-step-schema.yml") + .context("Failed to parse CWL schema from file")?; - let schema = CwlSchema::from_string(yaml_str).unwrap(); + let schema = CwlSchema::from_string(yaml_str) + .context("Failed to parse CWL schema from string")?;Also applies to: 58-59
107-108
: Improve JavaScript execution exampleThe current example needs improvements in error handling and result processing:
- let js_executor = JsExecutor::new(&cwl_inputs, &cwl_self); - js_executor?.run(js_script); + let mut js_executor = JsExecutor::new(&cwl_inputs, &cwl_self) + .context("Failed to initialize JavaScript executor")?; + let result = js_executor.run(js_script) + .context("Failed to execute JavaScript")?; + println!("Execution result: {}", result);Also consider adding a security notice about input validation and sanitization.
7-8
: Enhance documentation with additional sectionsConsider adding the following sections to improve documentation completeness:
- Security Considerations
- Input validation
- Script execution limits
- Sandbox environment
- Error Handling
- Common errors
- Error recovery strategies
- Supported CWL Features
- Detailed list of supported v1.2 fields
- Known limitations
- Version Compatibility
- CWL version support
- Breaking changes
zefiro-core/zefiro-cwl/src/values/types.rs (1)
35-37
: Consider returning a reference in thelocation
method to avoid cloning.The
location
method currently clones thelocation
field, which may be unnecessary. Returning a reference&str
can improve performance by avoiding unnecessary cloning.Apply this diff to optimize the method:
- pub fn location(&self) -> String { - self.location.clone() + pub fn location(&self) -> &str { + &self.location }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
zefiro-core/zefiro-cwl/README.md
(1 hunks)zefiro-core/zefiro-cwl/src/exprs/exec.rs
(1 hunks)zefiro-core/zefiro-cwl/src/exprs/mod.rs
(1 hunks)zefiro-core/zefiro-cwl/src/lib.rs
(1 hunks)zefiro-core/zefiro-cwl/src/values/types.rs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zefiro-core/zefiro-cwl/src/exprs/mod.rs
- zefiro-core/zefiro-cwl/src/lib.rs
🧰 Additional context used
🪛 LanguageTool
zefiro-core/zefiro-cwl/README.md
[uncategorized] ~65-~65: You might be missing the article “the” here.
Context: ...); Ok(()) } ### How to parse CWL Values document?
rust use zefiro_cw...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...l_input).unwrap(); Ok(()) } ### How to execute JavaScript expressions?
...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (2)
zefiro-core/zefiro-cwl/src/values/types.rs (2)
8-32
: TheCwlFile
struct is well-defined and appropriately annotated.The structure of the
CwlFile
with Serde annotations allows for proper serialization and deserialization.
102-114
: TheCwlValueType
enum is properly defined for representing CWL value types.The use of an
untagged
enum is appropriate for handling various data types in CWL.
Summary by CodeRabbit
Release Notes
Project Status
Version Changes
0.0.0-pre
Core Library Updates
zefiro-cwl-parser
tozefiro-cwl
Workspace Restructuring