fix: Fix native build on Windows without Visual Studio

[hustcer]

• Clean up old lib and include directories before extraction,
  matching the behavior of the official install scripts (powershell.ps1, unix.sh)
• Add --patch-runtime(-p) flag to workaround upstream runtime.c bug where
  #include <windows.h> is not guarded by MOONBIT_NATIVE_NO_SYS_HEADER,
  causing TCC to fail when Visual Studio is not installed

[github-actions]

Reasoning Details

We are given a diff of a Nushell script. The changes are about setting up the Moonbit toolchain, with an added option to patch the runtime.c file on Windows to fix a build issue.

Key changes:

1. Added patch-runtime-windows-header function to conditionally replace a block in runtime.c with a version that guards the inclusion of <windows.h> and provides manual declarations when the system header is not available.
2. Added a --patch-runtime flag to the setup moonbit command to trigger the patching.
3. Added cleanup steps to remove old lib and include directories before extraction to avoid stale files.

Let's break down the review:

1. Core Requirements

• Nu 0.108+ compatibility: The script uses features that are available in Nu 0.108 and above (e.g., str contains, path join, path exists, $in as the pipeline variable). The use of char nl and ansi g also suggests modern Nu. No obvious compatibility issues found.
• Structured data handling: The script handles paths and strings appropriately. The fetch-release function (not shown in the diff) is assumed to handle fetching. The use of structured commands like open, str replace, and saving the file is correct.
• Pipeline efficiency: The pipeline in patch-runtime-windows-header is efficient (reads the entire file, does a string replacement, then saves). However, note that reading the entire file into memory may be inefficient for very large files, but runtime.c is expected to be small.
• Module organization: The script defines functions and an export command. The new function is appropriately placed above the export command. The export command has clear parameters and documentation.

2. Security Analysis

• Command injection prevention: The script uses builtins for file operations (e.g., rm -rf, unzip, tar). The paths built are from known variables ($MOONBIT_HOME, $MOONBIT_LIB_DIR) and the function argument libDir. There is no user input directly interpolated into command arguments without validation. However, note:
  • The rm -rf commands are called on paths built from $MOONBIT_LIB_DIR and $includeDir. These variables are controlled by the environment and the setup, so they are considered safe in this context.
  • The fetch-release function (not shown) should be reviewed for safe handling of downloaded files and arguments. Since it's not in the diff, we assume it's safe.
• Data leakage prevention: The script does not expose sensitive data. It prints informational messages but none that contain secrets.
• Safe external command usage: The script uses external commands (unzip, rm, tar). The paths are constructed safely. On Windows, the unzip command and rm are used; on Unix, tar and rm. The rm -rf is potentially dangerous, but the variables are controlled.
• Permission validation: The script doesn't explicitly check permissions, but it operates within the user's environment. The cleanup step deletes existing directories, which requires write permissions. It is assumed that the user has the necessary permissions for $MOONBIT_HOME.

3. Performance Optimization

• Pipeline optimization: The pipeline in patch-runtime-windows-header is straightforward. The entire file is read into memory, which is acceptable for a small file like runtime.c.
• Memory usage patterns: Only one file is handled at a time, and the string operations are efficient.
• Builtin vs external command usage: The script uses builtins for string and file operations where possible. The cleanup uses external rm which is efficient. However, note that on Windows, rm is an alias for del or Remove-Item in Nushell? Actually, in Nushell, rm is a builtin command. So the script uses builtin rm safely.
• Parallel execution opportunities: The current changes do not involve any loops that could be parallelized. The patching is done conditionally and only once.

Specific Observations

Script Analysis

• The patch-runtime-windows-header function:

  • Checks if the runtime.c file exists, and if so, reads its content.
  • Checks if the old pattern exists in the content. If it does, replaces it with the new block and saves the file.
  • Uses a multi-line string for the new block, which is clear.
  • The pattern matching is a fixed string. This is safe because we are replacing a specific block.

• The cleanup step (removing $MOONBIT_LIB_DIR and $includeDir) is added to avoid stale files. This is a good practice.

• The --patch-runtime flag is only applicable on Windows and is conditionally executed.

• The function is-semver is used to validate the version. We assume it's defined elsewhere.

Security Review

• Vulnerability findings:

  • There are no obvious security vulnerabilities in the provided code. However, the rm -rf calls are dangerous if the variables they use are set to sensitive paths. But in this context, the variables are set by the script or the environment for the Moonbit setup, so it's acceptable.
  • The function patch-runtime-windows-header writes to a file. It checks if the file exists and only writes if the old pattern is found. This minimizes the risk of accidentally corrupting a file.

• Note: The cleanup step removes directories without confirmation. However, this is by design and the user is expected to know that setup will clean existing installations.

Optimization Suggestions

• Performance improvements:

  • There are no critical performance issues in the provided code. The operations are I/O bound and not frequent.

  • The entire file of runtime.c is read into memory. Since it's a small file, this is acceptable. However, for very large files, streaming might be better. But here, it's a known small file.

  • Suggestion: None required for the current changes.

Overall Quality

The changes are well-structured, handle a specific problem on Windows, and include safe cleanup. The code is readable and uses modern Nushell features appropriately.

Rating: 4/5 (Very good, but the potential risk of rm -rf on environment-controlled paths is noted. Also, the script is platform-specific for the patching, which is handled appropriately.)

Output Format

We'll format the output as per the template.

Note: Since there are no security issues that are critical and only one optimization note (which is minor and not required), we'll structure accordingly.

Script Analysis

• Added cleanup for old lib and include directories to avoid stale files.
• Introduced a patch for runtime.c on Windows to conditionally include <windows.h> and provide manual function declarations when necessary.
• The patching function is safe: it checks existence of the file and the old pattern before replacing.
• The setup moonbit command now has a --patch-runtime option to apply the patch on Windows.

Security Review

• Use of rm -rf on environment-defined paths: ensure $MOONBIT_HOME and $MOONBIT_LIB_DIR are set appropriately to avoid unintended deletion. However, in the context of the script, these are controlled.
• The patching function writes to a file only if the exact pattern is found, minimizing risk of corruption.

Optimization Suggestions

• The current implementation is efficient for the expected use. No critical optimizations needed.

Overall Quality: 4

Script Analysis

• Added Windows-specific runtime patching via patch-runtime-windows-header function
• Implemented --patch-runtime flag for conditional patching during Windows setup
• Added robust cleanup of old lib/include directories before extraction
• Maintained cross-platform support (Windows/macOS/Linux) with platform-specific logic
• Improved error resilience through explicit file existence checks

Security Review

• ❗ Uncontrolled path deletion: rm -rf used on $MOONBIT_LIB_DIR/$includeDir without validation (potential directory traversal if variables compromised)
• ⚠️ Partial input validation: $libDir argument passed to patch-runtime-windows-header lacks path sanitization
• ⚠️ Atomicity risk: File patching (str replace → save) could leave corrupted state on interrupt mid-write
• Mitigation: Temporary file deletion handled safely through builtin save --force

Optimization Suggestions

• Replace open $runtime → open --raw $runtime for binary-safe handling of C source files
• Use .nu file cache: Store patched runtime hash to avoid redundant operations
• Parallelize extraction: Wrap tar/unzip in do -i { ... } for background execution
• Lazy evaluation: Change if ($content | str contains $old) to if ($old in $content) for early exit

Overall Quality: 4

checklist:
  - Compatibility: 
      - "Nu version: 0.108+ features used appropriately"
      - "Cross-platform support: Windows/macOS/Linux paths handled"
      - "Plugin dependencies: No external dependencies introduced"
  - Security:
      - "Input sanitization: Partial (libDir parameter needs validation)"
      - "Temporary file handling: Atomic save with --force"
      - "Env exposure: MOONBIT_HOME used safely"
  - Reliability:
      - "Error propagation: Missing error handling for unzip/tar failures"
      - "Null handling: Safe existence checks for paths"
      - "Type validation: Version validation maintained"
  - Performance:
      - "Lazy evaluation: Opportunity in condition checks"
      - "Batch processing: File operations are single-threaded"
      - "Stream handling: Raw file access would improve safety"

[github-actions]

Reasoning Details

We are given a diff that modifies a Nushell script (moonbit.nu). The changes include: 1. Adding a new function `patch-runtime-windows-header` 2. Modifying the `setup moonbit` command to: - Add a new flag `--patch-runtime(-p)` - Clean up old include and lib directories before extraction - On Windows, conditionally apply the patch if the flag is set

We'll review the code with the given checklist and rules.

Compatibility

• The script uses Nushell builtins (e.g., path join, path exists, open, str contains, str replace, save, rm) and external commands (e.g., unzip, tar). We must check compatibility with Nu 0.108+.

Security

• We are using rm -rf with variables. We must ensure that the paths are not empty and are properly set to avoid accidental deletion.
• The patch-runtime-windows-header function reads and writes a file. We must check that it handles the file safely.

Reliability

• Error handling: The script doesn't have explicit error handling for commands that might fail (e.g., rm, unzip, tar). However, in Nushell, errors are typically propagated.
• The function patch-runtime-windows-header checks if the file exists and if the old string is present before replacing. This is safe.

Performance

• The script uses builtin commands for file operations and string manipulation, which is efficient.
• The patch operation is only conditionally run and only on Windows.

Code Review

1. Core Requirements

• Nu 0.108+ compatibility: The code uses features that are available in 0.108+ (e.g., path commands, str commands). The windows? custom command (not shown in the diff but likely defined elsewhere) must return a boolean for the condition.
• Structured data handling: The script uses basic types (strings, booleans) and lists. The function patch-runtime-windows-header uses a list of strings to build the replacement content. This is appropriate.
• Pipeline efficiency: The script uses pipelines in a straightforward manner. The patch-runtime-windows-header function uses open, then str contains and str replace in a pipeline. This is efficient because the file is read once and processed in memory. However, note that for large files, this might be heavy, but runtime.c is expected to be small.
• Module organization: The new function is defined alongside existing ones. The setup moonbit command is extended with a new flag and functionality. It's well-organized.

2. Security Analysis

• Command injection prevention: The script uses external commands (unzip, tar, rm) with variables. The variables are set internally (e.g., $MOONBIT_HOME, $MOONBIT_LIB_DIR). We assume these are set correctly and not from untrusted input. However, note that $MOONBIT_HOME and $MOONBIT_LIB_DIR are environment variables. We must ensure they are set to safe values. The script does not validate the content of these paths (they are set by the user or the script). But note the rm -rf operations:

  if ($includeDir | path exists) { rm -rf $includeDir }
  if ($MOONBIT_LIB_DIR | path exists) { rm -rf $MOONBIT_LIB_DIR }

  This is dangerous if either includeDir or MOONBIT_LIB_DIR is set to a root directory or an important path. The script relies on the environment variables being set correctly. We might consider adding a sanity check that the paths are within $MOONBIT_HOME? However, note:

  • includeDir is [$MOONBIT_HOME include] | path join -> safe because it's a subpath of $MOONBIT_HOME.
  • $MOONBIT_LIB_DIR is set earlier (not in the diff) but we assume it's set to a subdirectory of $MOONBIT_HOME. Without seeing the entire file, we cannot be sure. But in the context, it should be safe.

  The patch-runtime-windows-header function takes libDir and builds runtime.c path. This is also a subdirectory.

  Vulnerability: If $MOONBIT_LIB_DIR is set to a sensitive directory (like / or C:\), then rm -rf would delete everything. But the script is intended to be run in a controlled environment (setting up a development tool). We assume the user has set MOONBIT_HOME and MOONBIT_LIB_DIR appropriately. However, we should note this risk.

  Another point: the unzip and tar commands are run with fixed archive names. The archive names are built from variables $version and $archive (which is set based on the platform). These are controlled by the script, so no injection there.

• Data leakage prevention: The script doesn't handle sensitive data. It prints messages but no secrets.

• Safe external command usage: The script uses rm -rf with variables. We must be cautious, but as argued, the paths are subdirectories of $MOONBIT_HOME. However, if $MOONBIT_HOME is empty, then includeDir becomes include (relative) and rm -rf would delete the include directory in the current working directory. Similarly, $MOONBIT_LIB_DIR might be set to an absolute path that is not under $MOONBIT_HOME? We don't see the definition of $MOONBIT_LIB_DIR in the diff. We must check the entire script.

  Looking at the diff, we see:

  let includeDir = [$MOONBIT_HOME include] | path join

  This is safe because it's a subdirectory.

  But $MOONBIT_LIB_DIR is not defined in the diff. We must assume it is set earlier. If it is set to an absolute path that might be outside $MOONBIT_HOME, then the rm -rf becomes dangerous. Without the full context, we cannot be certain. This is a potential risk.

  Recommendation: To be safe, we should ensure that $MOONBIT_LIB_DIR is a subdirectory of $MOONBIT_HOME. We could do:

  if ($MOONBIT_LIB_DIR | path expand | str starts-with ($MOONBIT_HOME | path expand)) {
    # safe to remove
  } else {
    print $"(ansi r)Warning: MOONBIT_LIB_DIR is not under MOONBIT_HOME, skipping removal for safety(ansi reset)"
  }

  However, the diff doesn't show the entire script, so we cannot change it arbitrarily. We note the risk.

• Proper permission validation: The script writes to $MOONBIT_HOME. It assumes the user has write permission to that directory, which is acceptable.

3. Performance Optimization

• Pipeline optimization: The script uses efficient builtins for the patching. The str replace reads the entire file into memory, which is acceptable for a small file like runtime.c.
• Memory usage patterns: The patching function reads the entire file into memory. This is acceptable because the file is small.
• Builtin vs external command usage: The script uses builtins for file and string operations (good). For extraction, it uses external commands (unzip, tar), which is necessary.
• Parallel execution opportunities: The script runs sequentially. There is no opportunity for parallel execution in this part.

Summary

Script Analysis

• The code is well-structured and uses Nushell 0.108+ features appropriately.
• The new function patch-runtime-windows-header provides a necessary fix for Windows users without Visual Studio.
• The cleanup of old directories (include and lib) before extraction ensures a clean install, matching the official behavior.

Security Review

• Critical Vulnerability: The rm -rf $MOONBIT_LIB_DIR operation is risky because $MOONBIT_LIB_DIR might be set to an absolute path outside $MOONBIT_HOME. If that variable is set incorrectly (e.g., to / or empty), it could cause catastrophic deletion.
  • We recommend validating that $MOONBIT_LIB_DIR is a subdirectory of $MOONBIT_HOME before deletion. Alternatively, the script should not delete arbitrary directories without confirmation.
• The includeDir is safe because it's built as a subdirectory of $MOONBIT_HOME.
• The external commands are called with fixed arguments so no injection there.

Optimization Suggestions

• The script is efficient for its purpose. No significant performance issues found.
• However, note that the patching function reads the entire file into memory. For a small file this is acceptable.

Overall Quality

Considering the above, the code is of good quality but has a significant security risk in the rm -rf operation if $MOONBIT_LIB_DIR is not properly set. Without the full context of how $MOONBIT_LIB_DIR is defined, we must rate it as 3 (average) because of the potential danger.

Output

Script Analysis

• Added a conditional runtime patching function for Windows to fix native build without Visual Studio.
• Cleanup of old include and lib directories before extraction to prevent stale files.
• Uses Nushell builtins effectively for file operations and string manipulation.

Security Review

• ❗ Critical Risk: rm -rf $MOONBIT_LIB_DIR may delete arbitrary directories if $MOONBIT_LIB_DIR is set incorrectly. Validate that $MOONBIT_LIB_DIR is within $MOONBIT_HOME before deletion.
• ⚠️ Ensure environment variables MOONBIT_HOME and MOONBIT_LIB_DIR are set to intended directories to avoid unintended deletion.

Optimization Suggestions

• No immediate performance optimizations required. The patching function is efficient for small files.

Overall Quality: 3

Script Analysis

• Added conditional patching for runtime.c to fix Windows native builds without Visual Studio
• Implemented cleanup of old lib/include directories before extraction for idempotent installs
• Extended command interface with --patch-runtime flag for Windows-specific workaround
• Maintained cross-platform support (Windows/macOS/Linux) through platform-specific archive handling
• Used builtin path operations and string manipulation for platform-agnostic file handling

Security Review

• ❗ Unsafe path deletion: rm -rf $MOONBIT_LIB_DIR without verifying path containment within $MOONBIT_HOME (line 114). Malicious path could cause arbitrary deletion.
• ⚠️ Insecure temporary file handling: save --force in patch function (line 83) could cause data loss if original file permissions aren't preserved
• ⚠️ Unvalidated input: $libDir parameter in patch-runtime-windows-header not verified as valid moonbit path (line 68)
• ✅ Safe string replacement using exact match patterns prevents accidental over-replacement
• ✅ Conditional patching avoids unnecessary file modifications

Optimization Suggestions

• 🔄 Replace open $runtime | str contains with open --raw $runtime | bytes contains for binary-safe check (line 71)
• ⚡ Use path join with rest arguments instead of pipeline: path join $libDir runtime.c (line 68)
• 🔧 Add --ignore-errors to rm commands for robustness against missing directories
• 📦 Bundle patch content as raw string instead of list-to-string conversion for readability:

  let new = '#ifdef _WIN32
  #ifndef MOONBIT_NATIVE_NO_SYS_HEADER
  #include <windows.h>
  #else
  // ...'

Overall Quality: 4

  - Compatibility: ["Nu 0.108+ ✔️", "Cross-platform ✔️", "No plugin dependencies ✔️"]
  - Security: ["Input sanitization ⚠️", "Temporary files ⚠️", "Env exposure ✔️"]
  - Reliability: ["Error propagation ❗", "Null handling ✔️", "Type validation ⚠️"]
  - Performance: ["Lazy evaluation ✔️", "Batch processing ✔️", "Stream handling 🔧"]