Skip to content

Support env vars for custom maven goal command - #1179

Open
Alex Boyko (BoykoAlex) wants to merge 5 commits into
microsoft:mainfrom
BoykoAlex:terminal-env
Open

Support env vars for custom maven goal command#1179
Alex Boyko (BoykoAlex) wants to merge 5 commits into
microsoft:mainfrom
BoykoAlex:terminal-env

Conversation

@BoykoAlex

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for passing environment variables through the “custom Maven goal” command path, plumbing an optional env object from the command registration down to terminal execution. It also adjusts terminal handling to re-apply environment variables on each invocation (useful when terminals are reused and/or shell startup files override initial env).

Changes:

  • Extend Utils.executeCustomGoal (and the maven.goal.custom command handler) to accept an optional env map and pass it to executeInTerminal.
  • Compute merged terminal env earlier in MavenTerminal.runInTerminal and (re-)export env vars into the active terminal session via a new setupEnvForShell helper.
  • Replace the WSL-only export workaround with a generalized cross-shell env export helper.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/utils/Utils.ts Adds optional env parameter to custom goal execution and forwards it to terminal execution.
src/extension.ts Updates command registration to accept and forward the optional env parameter.
src/mavenTerminal.ts Merges env from settings + invocation options and re-exports env vars into reused terminals via setupEnvForShell.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/mavenTerminal.ts
Comment on lines +200 to 217
function setupEnvForShell(terminal: vscode.Terminal, env: { [envKey: string]: string }): void {
const shellType: ShellType = currentWindowsShell();
Object.keys(env).forEach(key => {
const value: string = env[key];
switch (shellType) {
case ShellType.POWERSHELL:
terminal.sendText(`$env:${key}="${value}"`, true);
break;
case ShellType.CMD:
terminal.sendText(`set ${key}=${value}`, true);
break;
default:
// bash/zsh/Git Bash/WSL and anything else that understands POSIX export syntax.
terminal.sendText(`export ${key}="${value}"`, true);
break;
}
});
}
@BoykoAlex

Copy link
Copy Markdown
Contributor Author

Copilot's comments addressed.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/mavenTerminal.ts:167

  • currentWindowsShell() detects fish only when the executable basename is exactly fish. On Windows installs (e.g. MSYS2), the shell is commonly fish.exe, which would currently fall into the default case and then setupEnvForShell would send export KEY=... (fish errors on that syntax).
        case "fish":
            return ShellType.FISH;

src/mavenTerminal.ts:50

  • Because terminals are reused and you only (re-)export keys present in the current env object, variables that were exported in a previous invocation but are omitted in a later invocation will remain set in the terminal session. This can cause stale environment leaking between runs (e.g., an env override applied once may unintentionally affect subsequent commands). Consider tracking previously-applied keys per terminal and issuing the appropriate shell-specific unset/remove command for keys that are no longer present.
        // env is only applied once, at creation time.
        // See: https://github.com/microsoft/vscode/issues/205102, https://github.com/microsoft/vscode/issues/188235
        if (Object.keys(env).length > 0) {
            setupEnvForShell(this.terminals[name], env);
        }

Comment thread src/mavenTerminal.ts
Comment on lines +236 to +241
case ShellType.CMD:
// cmd.exe has no real quoting mechanism; wrapping the whole assignment in
// quotes protects spaces and operators (&, |, <, >, ^), but %VAR% references
// inside the value are still expanded by cmd itself and can't be escaped.
terminal.sendText(`set "${key}=${value}"`, true);
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — the CMD branch is injectable. A value like a"&echo INJECTED&rem " terminates the quoted assignment and runs the rest as a separate command.

Escaping ^ and " won't fully close it though: ^" still terminates the quoted assignment, %VAR% is expanded by cmd before caret escaping applies, and doubling ^ corrupts the stored value. Suggest dropping the CMD sendText path and creating a fresh terminal with createTerminal({ env }) when invocation-specific env is passed, so values never pass through shell parsing.

Two other things while you're in here:

  • Reused terminals only assign the keys present in the current call, so env from an earlier custom goal stays set for later runs (mavenTerminal.ts#L36-L49). A fresh terminal per env-carrying invocation would fix this too.
  • case "fish" only matches the bare basename (mavenTerminal.ts#L166). fish.exe on Windows falls through to export, which fish rejects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changyong Gong (@chagong)
Some notes from me about escaping on cmd:

Yes, this can become a nightmare especially when also pipes are involved. But I don't think that you have to give up and recreate the terminal here. If you are willing to use a temporary file for this, there is an alternative set /P syntax to read all values from the file without parsing, so no escaping is required at all to guard against command injection / parsing errors.
Use the following steps for this approach:

  1. Prepare the temporary file with only all values of env delimited by Windows line breaks (CR/LF), so at least line breaks in values should be filtered out
  2. Prepare the text sent to the terminal by using all keys of env at once, something like this:
const cmdText: string = "(" + Object.keys(env).map(key => `set /P ${key}=`).join("&") + `) < ${tempFileName}`;
  1. Send this text to the terminal as usual
  2. Delete the temporary file

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.

4 participants