WIP: feat: using activation script to initialize the terminal #1377
Conversation
WalkthroughThis PR introduces automatic ESP-IDF activation script sourcing in terminal sessions by exposing the activation script path as an environment variable, replacing the terminal connector with a custom implementation that sources the script on connect, and simplifying the launcher delegate by removing environment and shell customization logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as SetupToolsInIde
participant Launcher as IDFConsoleLauncherDelegate
participant Plugin as plugin.xml Registry
participant Connector as IdfTerminalProcessConnector
participant Process as Terminal Process
Setup->>Setup: Get activation script path
Setup->>Setup: Store in envVarsFromActivationScriptMap
Setup->>Setup: Set ESP_IDF_ACTIVATION_SCRIPT env var
Launcher->>Plugin: Request terminal connector
Plugin->>Connector: Instantiate IdfTerminalProcessConnector
Connector->>Launcher: Return connector instance
Launcher->>Process: Initiate terminal connection
Process->>Connector: connect(ITerminalControl)
Connector->>Connector: Retrieve ESP_IDF_ACTIVATION_SCRIPT
alt Script Present
Connector->>Connector: Build OS-specific source command
Connector->>Process: Write source command to stream
Note over Process: PowerShell (Windows):<br/>Remove-Module PSReadLine<br/>. "path/to/script"
Note over Process: Unix:<br/>. "path/to/script"
Process->>Process: Execute activation script
else Script Missing
Connector->>Process: Write error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring particular attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
137-137: Consider using a shared constant for the environment variable key.The string
"ESP_IDF_ACTIVATION_SCRIPT"is duplicated here and inIdfTerminalProcessConnector.ENV_SCRIPT_KEY. Extract this to a shared constant in a common location (e.g.,IDFEnvironmentVariables) to prevent drift and ease maintenance.bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IdfTerminalProcessConnector.java (2)
52-58: Consider logging silently-ignored exceptions for troubleshooting.The
IOExceptionis silently swallowed, which may make debugging difficult if terminal output fails. Consider adding at least a debug-level log entry.🔎 Suggested improvement
private void writeToTerminal(OutputStream out, String message) { try { out.write(message.getBytes(StandardCharsets.UTF_8)); out.flush(); - } catch (IOException ignored) { + } catch (IOException e) { + Logger.log(e); // Best-effort write, log for troubleshooting } }
21-24: Consider logging when process is null.When
getProcess()returnsnull, the method returns silently without any indication of failure. This could make troubleshooting difficult. Consider adding a log entry or diagnostic message.🔎 Suggested improvement
var process = getProcess(); if (process == null) { + Logger.log("IdfTerminalProcessConnector: Process is null after connect"); return; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(2 hunks)bundles/com.espressif.idf.terminal.connector/plugin.xml(2 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java(6 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IdfTerminalProcessConnector.java(1 hunks)
🔇 Additional comments (5)
bundles/com.espressif.idf.terminal.connector/plugin.xml (1)
7-13: LGTM!The connector extension point is correctly updated to reference the new
IdfTerminalProcessConnectorclass, enabling the activation script integration for terminal sessions.bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (4)
191-193: Verify PowerShell availability without full path.Using
"powershell.exe"assumes PowerShell is on the system PATH. While this is typically true for modern Windows, consider whether the full path (C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe) would be more robust, or add a fallback tocmd.exeif PowerShell is unavailable.
213-217: LGTM!Good defensive null-check for
connectorIdwith a sensible default fallback toESP_IDF_CONSOLE_CONNECTOR_ID.
268-285: LGTM - simplified settings construction.The removal of explicit environment propagation aligns with the new architecture where
IdfTerminalProcessConnectorsources the activation script to initialize the terminal environment. The settings construction is cleaner and correctly preserves essential properties (image, PTY, echo, line separator, listeners, working directory).
280-284: The pattern matching syntax used here is compatible with the project's Java version. The project requires Java 17+, which fully supports this feature (pattern matching for instanceof was finalized in Java 16 and is standard in Java 17+). No changes needed.
| var activationScriptPath = idfInstalled.getActivationScript(); | ||
| arguemnts = ToolsUtility.getExportScriptCommand(activationScriptPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -name "SetupToolsInIde.java" -type fRepository: espressif/idf-eclipse-plugin
Length of output: 302
🏁 Script executed:
git ls-files | grep -i "SetupToolsInIde.java"Repository: espressif/idf-eclipse-plugin
Length of output: 156
🏁 Script executed:
cat -n bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java | head -150Repository: espressif/idf-eclipse-plugin
Length of output: 6618
🏁 Script executed:
git ls-files | grep -i "IdfInstalled.java"Repository: espressif/idf-eclipse-plugin
Length of output: 156
🏁 Script executed:
cat bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.javaRepository: espressif/idf-eclipse-plugin
Length of output: 1163
🏁 Script executed:
git ls-files | grep -i "ToolsUtility.java"Repository: espressif/idf-eclipse-plugin
Length of output: 158
🏁 Script executed:
cat bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.javaRepository: espressif/idf-eclipse-plugin
Length of output: 3015
Add null-check for activation script path.
If idfInstalled.getActivationScript() returns null, the value will be added directly to the command list in ToolsUtility.getExportScriptCommand() (lines 71, 76, 80) without validation, and subsequently passed to processBuilderFactory.runInBackground() and the environment variables map at line 137. Validate the path before proceeding.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
around lines 103-104, add a null-check for idfInstalled.getActivationScript()
before calling ToolsUtility.getExportScriptCommand(...); if activationScriptPath
is null, log a clear error or warning and abort/return early (or throw a
descriptive exception) so you do NOT call getExportScriptCommand with null and
do NOT pass a null value into processBuilderFactory.runInBackground() or the
environment map at line 137; ensure subsequent code paths that expect arguemnts
handle the early return safely.
| if (Platform.OS_WIN32.equals(Platform.getOS())) { | ||
| // Windows (PowerShell): Remove-Module: Fixes the color/white-screen issue. | ||
| command = "Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + scriptPath + "\"\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } else { | ||
| // Linux / macOS (Bash/Zsh): | ||
| command = ". \"" + scriptPath + "\"\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } |
There was a problem hiding this comment.
Potential command injection vulnerability - sanitize script path.
The scriptPath is directly interpolated into shell commands without escaping. If the path contains special characters (quotes, backticks, $(...) on Unix, or " on Windows), it could lead to command injection or shell errors.
Consider validating the path or escaping shell metacharacters before constructing the command.
🔎 Example mitigation approach
if (Platform.OS_WIN32.equals(Platform.getOS())) {
// Windows (PowerShell): Remove-Module: Fixes the color/white-screen issue.
- command = "Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + scriptPath + "\"\r\n";
+ // Escape embedded quotes in path for PowerShell
+ String escapedPath = scriptPath.replace("\"", "`\"");
+ command = "Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + escapedPath + "\"\r\n";
} else {
// Linux / macOS (Bash/Zsh):
- command = ". \"" + scriptPath + "\"\r\n";
+ // Escape embedded quotes/special chars for shell
+ String escapedPath = scriptPath.replace("'", "'\\''");
+ command = ". '" + escapedPath + "'\r\n";
}📝 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 (Platform.OS_WIN32.equals(Platform.getOS())) { | |
| // Windows (PowerShell): Remove-Module: Fixes the color/white-screen issue. | |
| command = "Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + scriptPath + "\"\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } else { | |
| // Linux / macOS (Bash/Zsh): | |
| command = ". \"" + scriptPath + "\"\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } | |
| if (Platform.OS_WIN32.equals(Platform.getOS())) { | |
| // Windows (PowerShell): Remove-Module: Fixes the color/white-screen issue. | |
| // Escape embedded quotes in path for PowerShell | |
| String escapedPath = scriptPath.replace("\"", "`\""); | |
| command = "Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + escapedPath + "\"\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } else { | |
| // Linux / macOS (Bash/Zsh): | |
| // Escape embedded quotes/special chars for shell | |
| String escapedPath = scriptPath.replace("'", "'\\''"); | |
| command = ". '" + escapedPath + "'\r\n"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IdfTerminalProcessConnector.java
around lines 36 to 42, the code directly interpolates scriptPath into a shell
command which can lead to command injection or shell parsing errors; instead
validate and sanitize the path and avoid composing a single shell command
string. Fix by verifying the scriptPath is an absolute existing file with no
null bytes and matches an allowed pattern (no unescaped quotes, backticks or
shell metacharacters), then either (preferred) invoke the script via a safe
argument-based API (e.g., use ProcessBuilder with the shell and script passed as
separate arguments or execute the script file directly) or escape/quote the
scriptPath properly for the target shell (escape embedded double quotes on
Windows and properly single-quote or use POSIX shell-escaping on Unix); ensure
any added escaping is centralized in a helper and add unit tests to cover
malicious inputs.
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use proper logging instead of e.printStackTrace().
The project uses Logger.log(e) for exception logging (as seen in SetupToolsInIde.java). Replace printStackTrace() with the project's logging infrastructure for consistency and proper log management.
🔎 Suggested fix
+import com.espressif.idf.core.logging.Logger;
...
} catch (IOException e) {
- e.printStackTrace();
+ Logger.log(e);
}📝 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.
| } catch (IOException e) { | |
| e.printStackTrace(); | |
| } | |
| } catch (IOException e) { | |
| Logger.log(e); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IdfTerminalProcessConnector.java
around lines 47 to 49, the catch block currently calls e.printStackTrace();
replace this with the project logging call Logger.log(e) (or the appropriate
static Logger class used elsewhere), ensure the Logger is imported or
fully-qualified, and keep the catch behavior otherwise unchanged so the
exception is recorded via the project's logging infrastructure instead of
printing to stderr.
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-1669)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.