You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
🚧 Files skipped from review as they are similar to previous changes (25)
config/clippy.toml
.github/workflows/docker-linux.yaml
services/websockify/Cargo.toml
.github/workflows/codeql.yaml
services/ws-modules/rcomm1/pkg/package.json
.github/workflows/test.yaml
Dockerfile.windows
services/ws-server/Cargo.toml
services/ws-modules/rdata1/pkg/module.R
.gitignore
services/ws-modules/rdata1/pkg/et_ws_rdata1.js
.github/workflows/upstream-cache.yaml
services/ws-modules/rcomm1/pkg/module.R
services/ws-modules/rcomm1/pkg/et_ws_rcomm1.js
.github/workflows/docker-windows.yaml
services/ws-server/src/lib.rs
.mise/config.r.toml
services/ws-web-runner/tests/modules.rs
.github/workflows/coverage.yaml
Dockerfile
services/websockify/src/lib.rs
.github/workflows/dependencies.yaml
services/websockify/tests/relay.rs
.mise/config.toml
libs/edge-toolkit/src/config.rs
📝 Walkthrough
Walkthrough
The PR adds browser-only R modules using webR, introduces a fixed-target WebSocket-to-TCP relay with SOCKS5 support, registers the relay in the WebSocket server, updates R tooling and CI environments, and hardens mise command error handling.
R environments provision webR and build vendored assets; rdata1 and rcomm1 add JavaScript/R browser workflows, with runner validation for Deno’s classic-worker limitation.
A fixed loopback-target /websockify relay supports direct byte forwarding, binary subprotocol negotiation, and restricted SOCKS5 CONNECT handling with end-to-end tests.
We reviewed changes in f2b4c0e...9848cf1 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer TIP This summary will be updated as you push new changes.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR significantly exceeds the scope of simply adding R language support. It introduces a websockify relay service and implements a workspace-wide refactor of command execution and linting.
Codacy analysis identifies that this PR is not up to standards, largely due to 108 new issues and 12 code clones. The most critical findings concern the websockify service, which is high-complexity (46) and lacks test coverage for its core handshake logic, and the browser-based R modules which contain potential security vulnerabilities and high duplication. Furthermore, several required test scenarios related to environment compatibility (Deno) and graceful failure handling are missing.
About this PR
Please provide a PR description explaining the motivation for the websockify relay service and the workspace-wide command-error refactor. The current title 'Add R lang' does not adequately reflect the scope of these changes.
Test suggestions
Relay bridges bytes in both directions (echo test)
Consider implementing these tests if applicable:
1. R modules fail gracefully in Deno environment due to classic worker limitation
2. Command availability check (is_available) handles missing binary gracefully via empty PATH
Low confidence findings
The Zig REST client regeneration includes changes to core types like CancellationToken and HttpObserver. Confirm if these are purely side effects of a tool version bump or if they require specific validation.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
The function setupAgent was used before it was defined. Declaring functions before their first use improves readability and ensures compatibility with strict linter configurations.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Template literals in dynamic import() calls are flagged as a potential security vulnerability (XSS). While the base URL is a constant here, consider using a static path if possible to satisfy security requirements.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
This file shares a high degree of duplication with services/ws-modules/rdata1/pkg/et_ws_rdata1.js. The initialization and agent setup logic are almost identical. Consider extracting this shared logic into a common JavaScript utility in services/ws-modules/common/.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The handshake and protocol detection phase lacks a timeout. A client can open a connection and stop sending data, causing the spawned relay task to hang indefinitely. Given this file's high complexity (46) and lack of coverage, this is a high-risk area for task leaks.
Try wrapping the protocol detection and SOCKS5 handshake logic (lines 247-266) with a tokio::time::timeout of 10 seconds.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: ES2015 property shorthands are forbidden in this configuration to ensure cross-browser compatibility. Use explicit key-value pairs instead.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Cancellation is unresponsive while streamDelimiterLimit is waiting for data. If the server is not sending events, the task will stay blocked in the reader and won't check the cancellation token until the next frame arrives.
Keep the browser-only and Deno limitation in README.md, and reduce CLAUDE.md to the module names plus its unique tooling guidance.
README.md#L256-L257: retain this as the canonical browser-runner behavior description.
CLAUDE.md#L313-L314: remove the repeated browser-only/Deno explanation while retaining the unique MISE_ENV=r and JS-linting guidance.
Based on learnings: Document each thing exactly once in its most relevant location; do not duplicate explanations or add cross-references to other documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 256 - 257, Keep the webR browser-only and Deno runner
limitation in README.md at the identified section as the single canonical
explanation. In CLAUDE.md lines 313-314, remove the repeated limitation text
while retaining the module names and unique MISE_ENV=r and JavaScript-linting
guidance; do not add cross-references.
Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/test-helpers/src/lib.rs`:
- Around line 25-35: Update with_empty_path to create a unique empty temporary
directory and set PATH to that directory via temp_env::with_var while executing
run. Ensure the directory remains alive for the callback duration and preserve
the callback’s return value, guaranteeing bare-name Command resolution cannot
find executables through fallback search locations.
In `@services/ws-modules/rdata1/pkg/et_ws_rdata1.js`:
- Around line 14-21: Update init() to validate the fetch response for
R_SOURCE_URL before reading its body or passing it to webR.evalRVoid(). Reject
or throw a clear load error when the response is not successful, while
preserving the existing cache-busting request and R evaluation flow for
successful responses.
In `@utilities/int-gen/src/lib.rs`:
- Line 68: Wrap the documentation comment line near the listed ws.yaml,
rest.yaml, ws.schema.json, ws.kdl, and et:ws-messages WIT artifacts so every
line is at most 120 characters, preserving the existing wording and meaning.
---
Nitpick comments:
In `@README.md`:
- Around line 256-257: Keep the webR browser-only and Deno runner limitation in
README.md at the identified section as the single canonical explanation. In
CLAUDE.md lines 313-314, remove the repeated limitation text while retaining the
module names and unique MISE_ENV=r and JavaScript-linting guidance; do not add
cross-references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d89bbfb5-accf-4b00-9aab-ce4a4f090b32
📥 Commits
Reviewing files that changed from the base of the PR and between 15af375 and b0d9f11.
⛔ Files ignored due to path filters (2)
Cargo.lock is excluded by !**/*.lock
generated/zig-rest/src/et_rest_client.zig is excluded by !**/generated/**
Rust std::process::Command path resolution empty PATH Windows CreateProcess search order POSIX execvp
💡 Result:
In Rust, the path resolution behavior for std::process::Command differs significantly between Unix and Windows [1][2]. On Unix, Rust relies on the underlying OS execvp syscall [2][3]. If the program is not an absolute path, execvp searches the PATH environment variable [2]. If PATH is explicitly cleared or removed, execvp falls back to an OS-defined default (typically /bin:/usr/bin), not the parent process's PATH [4][2]. On Windows, Rust resolves the executable path manually before calling the Windows API CreateProcessW [1][4]. This means Rust does not rely on CreateProcessW's internal, and often insecure, search logic [1][5]. When the program is not an absolute path, Rust searches in this order [1][2]: 1. The child's PATH, if explicitly set via Command::env [1][2]. 2. The directory of the current executable [1][2]. 3. The system directory (GetSystemDirectoryW) [1][2]. 4. The Windows directory (GetWindowsDirectoryW) [1][2]. 5. The parent process's PATH [1][2]. A key difference is how Rust handles a missing or empty PATH on Windows compared to Unix. On Windows, if PATH is cleared or removed via env_clear or env_remove, Rust skips the first step but still proceeds to search the parent process's PATH at step 5 [1][4]. This prevents the potential failure mode seen on Unix where clearing PATH loses access to all non-default locations [4][2]. For security, it is highly recommended to use absolute paths for the program or to explicitly set the PATH environment variable when spawning processes to avoid unexpected executable resolution [4][2].
In Rust, setting the environment variable PATH to an empty string does not trigger special behavior within the standard library itself; rather, it sets the environment variable to an empty value, and the subsequent behavior depends on how the operating system or specific library functions interpret that value [1][2][3]. Regarding the temp_env crate, with_var simply leverages std::env::set_var to set the environment variable for the duration of a closure [4][5]. If you pass an empty string as the value to with_var, it will set PATH to "" [4]. Key considerations include: 1. Standard Library Behavior: std::env::set_var(key, value) is an unsafe function (as of recent Rust editions) used to set environment variables [3][6]. It does not validate the content of the value beyond checking for NUL characters [3]. Setting PATH to an empty string via this function simply updates the process environment [3]. 2. Interpretation of Empty PATH: - On Unix-like systems, tools and shells often interpret an empty component in the PATH environment variable as the current directory (.), but this is a convention of the consuming program or shell, not an inherent property of the Rust standard library [7]. - On Windows, behavior varies significantly, and an empty or malformed PATH can lead to security issues or failures to find executables, sometimes causing systems to fall back to default search paths or ignore the variable entirely [8][9]. 3. temp_env Usage: The temp_env crate manages the restoration of the previous environment state after the closure finishes [4][5]. Passing Some("") to with_var will effectively set the environment variable to an empty string for that scope [4][10]. If you are modifying PATH to control subprocess execution via std::process::Command, be aware that Command has specific behaviors regarding PATH inheritance and overriding [1][2]. If you clear the environment or specifically set PATH, you may affect how child processes locate executables [2]. It is generally recommended to use absolute paths or verify the resulting environment if you are manipulating PATH for security or stability [2].
site:doc.rust-lang.org std::process::Command Windows PATH search order parent PATH execvp empty PATH default lookup
💡 Result:
On Windows, Rust's std::process::Command does not rely on the OS (e.g., via execvp) to resolve the executable path; instead, it performs the resolution itself before spawning the process [1][2][3]. When Command::new is provided with a program name that is not an absolute path, it searches for the executable in the following order [1][4]: 1. The child's PATH environment variable (if explicitly set via the env method) [1][2]. 2. The directory of the current executable [1][4]. 3. The system directory (retrieved via GetSystemDirectoryW) [1][2]. 4. The Windows directory (retrieved via GetWindowsDirectoryW) [1][4]. 5. The parent process's PATH environment variable [1][2]. Important nuances regarding this behavior include: - PATH Clearing: Unlike on Unix, where clearing or removing the PATH variable can result in an OS-defined fallback (often /bin:/usr/bin), clearing the PATH on Windows via env_clear or env_remove skips only the first step (the child's explicit PATH). The parent process's PATH is still searched at step 5 [1][2][3]. - Implementation Note: Because Rust resolves the path manually, it behaves differently than traditional execvp calls, which generally rely on the environment's current PATH. This design ensures consistent cross-platform behavior while accounting for Windows-specific search conventions [1][3].
Use a dedicated empty directory for PATH isolation.PATH="" is not a portable way to force a bare-name Command to fail: Unix can still fall back to default search paths, and Windows has additional executable lookup locations beyond PATH. Point PATH at a unique empty temp directory, or use a stricter isolation mechanism if you need a guaranteed miss.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/test-helpers/src/lib.rs` around lines 25 - 35, Update with_empty_path to
create a unique empty temporary directory and set PATH to that directory via
temp_env::with_var while executing run. Ensure the directory remains alive for
the callback duration and preserve the callback’s return value, guaranteeing
bare-name Command resolution cannot find executables through fallback search
locations.
Check fetch response status before evaluating as R source.
fetch() only rejects on network failure, not HTTP error status. If /module.R returns a non-2xx (e.g. 404), the error page body still gets passed to webR.evalRVoid(), producing a confusing R-side parse error instead of a clear load failure.
🛡️ Proposed fix
- const rSource = await (await fetch(`${R_SOURCE_URL}?v=${Date.now()}`)).text();+ const rResp = await fetch(`${R_SOURCE_URL}?v=${Date.now()}`);+ if (!rResp.ok) throw new Error(`rdata1: failed to fetch module.R (${rResp.status})`);+ const rSource = await rResp.text();
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if(!rResp.ok)thrownewError(`rdata1: failed to fetch module.R (${rResp.status})`);
constrSource=awaitrResp.text();
awaitwebR.evalRVoid(rSource);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-modules/rdata1/pkg/et_ws_rdata1.js` around lines 14 - 21, Update
init() to validate the fetch response for R_SOURCE_URL before reading its body
or passing it to webR.evalRVoid(). Reject or throw a clear load error when the
response is not successful, while preserving the existing cache-busting request
and R evaluation flow for successful responses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Documentation
Bug Fixes