Skip to content
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

Conversation

nsyzrantsev
Copy link
Contributor

@nsyzrantsev nsyzrantsev commented Jan 8, 2025

Summary by CodeRabbit

Release Notes

  • Project Status

    • Project is now marked as a Work In Progress (WIP)
    • Not yet suitable for production use
  • Version Changes

    • All packages updated to version 0.0.0-pre
    • Indicates pre-release development stage
  • Core Library Updates

    • Renamed zefiro-cwl-parser to zefiro-cwl
    • Enhanced CWL document parsing capabilities
    • Added JavaScript expression execution support
    • Improved file and directory type representations
  • Workspace Restructuring

    • Updated workspace member configuration
    • Simplified CWL version handling
    • Introduced more consistent naming conventions

@nsyzrantsev nsyzrantsev self-assigned this Jan 8, 2025
Copy link

coderabbitai bot commented Jan 8, 2025

Walkthrough

This 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 zefiro-cwl-parser to zefiro-cwl, introducing a new JavaScript expression execution mechanism using Deno runtime, and updating various project configurations to reflect a pre-release state. The modifications span multiple files across the project, focusing on improving the CWL parsing and expression evaluation capabilities.

Changes

File Change Summary
Cargo.toml - Version changed to "0.0.0-pre"
- Workspace member updated from "zefiro-cwl-parser" to "zefiro-cwl"
README.md Added WIP (Work In Progress) warning message
zefiro-core/zefiro-cwl/Cargo.toml - Renamed package from "zefiro-cwl-parser" to "zefiro-cwl"
- Updated version to "0.0.0-pre"
- Updated authors and categories
- Added new dependencies: deno_core, serde_json, sha1
zefiro-core/zefiro-cwl/src/ - Added new exprs module with JsExecutor
- Updated schema and values modules
- Introduced new constants and type definitions
Various CLI and Core Cargo.toml files Version changed to "0.0.0-pre"

Sequence Diagram

sequenceDiagram
    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
Loading

Possibly related issues

Possibly related PRs

Poem

🐰 A Rabbit's Ode to Code Transformation

From parser to cwl, we hop and we dance,
JavaScript expressions get a brand new chance!
Pre-release whiskers twitch with delight,
Deno runtime makes our workflow just right!
Zefiro evolves, one commit at a time! 🚀


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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:

  1. Invalid JavaScript syntax
  2. Execution timeouts
  3. Large input values
  4. 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 structure

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2527839 and 772b527.

📒 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 renaming CommandLineBinding to InputBinding.

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 to InputBinding 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 rust

Length 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 with MINIMAL_CWL_VERSION.


29-29: LGTM! Simplified version handling.

The change from SUPPORTED_CWL_VERSIONS to MINIMAL_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 issue

Fix 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-js-exec/src/exec.rs Outdated Show resolved Hide resolved
zefiro-pipeline-engine/zefiro-cwl-parser/Cargo.toml Outdated Show resolved Hide resolved
release.sh Outdated Show resolved Hide resolved
zefiro-pipeline-engine/zefiro-cwl-parser/README.md Outdated Show resolved Hide resolved
zefiro-pipeline-engine/zefiro-cwl-parser/README.md Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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 tools

For a JavaScript execution engine, consider adding:

  • proptest for property-based testing of JavaScript expression handling
  • cargo-fuzz for finding security issues through fuzzing

Example addition:

 [dev-dependencies]
 rstest = "0.24.0"
+proptest = "1.0"
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 772b527 and 8b66d0b.

📒 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 execution

Using deno_core for JavaScript execution requires careful consideration of security boundaries. Please ensure:

  1. JavaScript execution is properly sandboxed
  2. Input validation is implemented
  3. Resource limits are enforced
  4. 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 rust

Length 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 3

Length 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

zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml Outdated Show resolved Hide resolved
zefiro-pipeline-engine/zefiro-cwl-js-exec/Cargo.toml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
zefiro-core/zefiro-cwl/src/values/types.rs 0.00% 1 Missing ⚠️
Flag Coverage Δ
unit-tests 14.28% <0.00%> (-2.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
zefiro-core/zefiro-cwl/src/schema/document.rs 0.00% <ø> (ø)
zefiro-core/zefiro-cwl/src/values/document.rs 0.00% <ø> (ø)
zefiro-core/zefiro-cwl/src/values/types.rs 0.00% <0.00%> (ø)

Copy link

@coderabbitai coderabbitai bot left a 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 deserialization

Consider 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 handling

The 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:

  1. Add a test job as a prerequisite to publishing
  2. Add version verification to ensure the tag matches the crate version
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0221acb and 916b4b8.

📒 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 good

The addition of the exec module and re-exporting JsExecutor provides a clean interface for users. This enhances modularity and usability.

Cargo.toml (1)

9-11: ⚠️ Potential issue

Update 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 new zefiro-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 from zefiro-core to zefiro-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 to zefiro-pipeline-engine correctly reflect the project restructuring.

Also applies to: 43-44

zefiro-pipeline-engine/zefiro-cwl-js/src/exec.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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 string

Calling script.to_string() creates an unnecessary clone of the &str. Since execute_script accepts a &str, you can pass script 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 scenarios

The current tests cover typical use cases. To ensure robustness, add tests for error scenarios:

  • Invalid JavaScript code execution.
  • Malformed cwl_inputs or cwl_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 heading

The 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 expressions

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 796fcdf and 52ba976.

📒 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.md

Length 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 5

Length of output: 53


Script:

#!/bin/bash
# Check package README content
cat zefiro-core/zefiro-cwl/README.md

Length of output: 2106


19-24: Review security implications of new dependencies.

The addition of deno_core for JavaScript execution introduces new security considerations:

  1. Ensure proper sandboxing of JavaScript execution
  2. Consider adding security policy documentation
  3. 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.md

Length 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 test

Length of output: 2620

zefiro-core/zefiro-cwl/src/exprs/exec.rs (1)

35-35: Handle potential deserialization errors

When 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 and CLT_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 the CommandLineBinding 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 rust

Length of output: 165


Script:

#!/bin/bash
# Find Cargo.toml files in zefiro-cwl
fd Cargo.toml --type f --exec cat {} \; -p zefiro-cwl

Length 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 rust

Length 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 and WF_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 updates

The import path and example file path have been correctly updated to reflect the new project structure.


52-52: LGTM: Import path update

Import path correctly updated to use the new package name.


81-81: LGTM: Import path update in documentation

Documentation example correctly updated to use the new package name.


108-108: LGTM: Test file paths update

Test 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 organization

Imports are well-organized and the new constants improve code maintainability.


51-52: LGTM: Use of constants for class types

Good use of constants for class types improves maintainability and reduces the risk of typos.


166-174: LGTM: Error handling improvement

Good addition of the FailingWriter struct for testing error cases. This improves test coverage for error scenarios.


176-179: LGTM: Write error test coverage

Good test coverage for write error scenarios.


Line range hint 1-187: Missing JavaScript expression execution implementation

The 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 impact

The 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 existence

The 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 d

Length 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)))
}

zefiro-core/zefiro-cwl/Cargo.toml Show resolved Hide resolved
zefiro-core/zefiro-cwl/src/exprs/exec.rs Show resolved Hide resolved
zefiro-core/zefiro-cwl/src/values/types.rs Outdated Show resolved Hide resolved
zefiro-core/zefiro-cwl/src/values/types.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a 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 issue

Sanitize inputs to prevent code injection

The current implementation directly interpolates cwl_inputs and cwl_self into the JavaScript code using format!, 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 scenarios

Current 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 examples

Replace 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 example

The 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 sections

Consider 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 the location method to avoid cloning.

The location method currently clones the location 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

📥 Commits

Reviewing files that changed from the base of the PR and between fc683c6 and bd03917.

📒 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: The CwlFile struct is well-defined and appropriately annotated.

The structure of the CwlFile with Serde annotations allows for proper serialization and deserialization.


102-114: The CwlValueType enum is properly defined for representing CWL value types.

The use of an untagged enum is appropriate for handling various data types in CWL.

zefiro-core/zefiro-cwl/src/exprs/exec.rs Show resolved Hide resolved
@nsyzrantsev nsyzrantsev changed the title feat: implement logic to execute and render js expressions in cwl schema feat: implement cwl js expressions executor Jan 10, 2025
@nsyzrantsev nsyzrantsev merged commit 5682707 into main Jan 10, 2025
9 checks passed
@nsyzrantsev nsyzrantsev deleted the 5-implement-a-logic-to-execute-and-render-js-expressions-in-cwl-schema branch January 10, 2025 16:16
@nsyzrantsev nsyzrantsev linked an issue Jan 10, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a logic to execute and render js expressions in CWL Schema
1 participant