Enhance the usage of Global Environment Vars

[chr1syy]

This pull request introduces a major enhancement to Maestro's environment variable management: global environment variables configured in Settings now apply to all AI agent processes, not just terminal sessions. This enables centralized configuration of API keys, proxies, tool paths, and more, streamlining automation and agent workflows. The documentation and UI have been updated to clarify this behavior and provide detailed usage instructions, and the codebase has been modified to ensure these variables are correctly applied and logged.

Global Environment Variable Support:

• Environment variables set in Settings → General → Shell Configuration → Global Environment Variables now apply to all agent processes as well as terminals, with clear precedence rules and immediate effect for new sessions. [1] [2] [3]
• Debug logging added to show when and how global environment variables are applied to agent or terminal processes. [1] [2]

Documentation and UI Updates:

• Added/expanded documentation in docs/configuration.md and docs/features.md explaining how to configure and use global environment variables, including precedence, example use cases, and override mechanisms. [1] [2] [3]
• Updated Settings UI text for clarity and added help text to explain that global environment variables affect both agents and terminals.

Codebase Changes:

• Refactored process spawning logic to always load and apply global environment variables from Settings for both agent and terminal processes. [1] [2] [3]
• Clarified that the CLI agent spawner does not have access to these global variables and instead relies on the environment Maestro is launched with, ensuring correct separation of concerns. [1] [2]

Other Documentation Improvements:

• Updated tables and formatting in docs/configuration.md for consistency and readability, including notification, sync, and platform support sections. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

These changes provide a more powerful and user-friendly way to manage environment variables across all Maestro workflows, improving automation, security, and portability.

Issues

closes #372

Summary by CodeRabbit

• New Features

  • Global Environment Variables: configure once in Settings to apply across terminals and agent processes.
  • Quick Action added to open Global Environment Variables.

• Documentation

  • New comprehensive Global Environment Variables guide and architecture.
  • Expanded Settings, Pre-release/Beta opt-in, Notifications, sleep-prevention/Linux desktop notes, and cross-device sync limitations.

• UI

  • Real-time per-entry validation, inline errors, and agent-override messaging for env vars.

• Tests

  • New integration and unit tests covering propagation, precedence, expansion, and edge cases.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Loads settings-stored shellEnvVars (Global Environment Variables) and applies them to spawned terminals and agent processes, merged with session-level envs (session > global > defaults). Updates spawn plumbing, env builder signatures, logging (globalEnvVarsCount), renderer UI, docs, and adds unit/integration tests and validation.

Changes

Cohort / File(s)	Summary
Documentation docs/configuration.md, docs/env-vars.md, docs/features.md	Adds comprehensive docs for Global Environment Variables: architecture, storage schema (shellEnvVars), precedence rules, examples, troubleshooting, pre-release notes, and feature listing.
IPC / Spawn Orchestration src/main/ipc/handlers/process.ts	Load globalShellEnvVars from settings; merge with session envs for terminals/agents; add globalEnvVarsCount metric and pass shellEnvVars in spawn options.
Env builder & Types src/main/process-manager/utils/envBuilder.ts, src/main/process-manager/types.ts	Extend buildChildProcessEnv(..., globalShellEnvVars?); enforce precedence (session > global > process), tilde expansion, Electron-var stripping, and update signatures/types.
Spawners src/main/process-manager/spawners/ChildProcessSpawner.ts, src/main/process-manager/spawners/PtySpawner.ts	Accept and thread shellEnvVars through spawn config; use unified env-building for PTY and non-PTY flows; add debug logging (sessionId, var count, key preview).
Main tests (unit & integration) src/main/process-manager/utils/__tests__/envBuilder.test.ts, src/__tests__/integration/process-global-env-vars.test.ts, src/__tests__/main/ipc/handlers/process.test.ts	New and expanded tests validating precedence, tilde expansion, Electron stripping, SSH merging, spawn propagation, resilience to edge cases, and regression scenarios.
Renderer UI src/renderer/components/SettingsModal.tsx, src/renderer/components/QuickActionsModal.tsx, src/renderer/components/shared/AgentConfigPanel.tsx	Settings: per-entry validation/errors, help tooltip, valid-count and storage text updates; Quick Actions: new "Configure Global Environment Variables" action; AgentConfigPanel: clarifies agent-level overrides.
CLI Notes src/cli/services/agent-spawner.ts	Added comments clarifying CLI agent spawners do not read settings-based globals and rely on the runtime environment.
Tests & Docs additions docs/*, src/__tests__/*	Large new doc docs/env-vars.md and numerous tests covering UI validation and env var propagation.

Sequence Diagram

sequenceDiagram
    participant User as User (Renderer)
    participant Settings as SettingsModal\n(Renderer)
    participant Store as Settings Store\n(Main)
    participant IPC as IPC Handler\n(Main)
    participant EnvBuilder as EnvBuilder\n(Main)
    participant Spawner as Process Spawner\n(Main)
    participant Process as Child Process\n(Agent/Terminal)

    User->>Settings: Configure global env vars
    Settings->>Store: Save `shellEnvVars`

    alt Spawn (Terminal or Agent)
        IPC->>Store: Load `shellEnvVars`
        IPC->>EnvBuilder: Request env (sessionEnvVars + shellEnvVars)
        EnvBuilder->>EnvBuilder: Merge (session > global > defaults)
        EnvBuilder->>EnvBuilder: Expand tilde, strip Electron vars
        EnvBuilder-->>IPC: Final ProcessEnv
        IPC->>Spawner: Spawn(processConfig + shellEnvVars, globalEnvVarsCount)
        Spawner->>Spawner: Log sessionId, count, keys
        Spawner->>Process: Create process with merged env
        Process->>Process: Use env (API keys, PATHs, etc.)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Enhance the usage of Global Environment Vars' accurately summarizes the main change: extending global environment variables support to agent processes.
Linked Issues check	✅ Passed	The PR successfully addresses all coding objectives from issue #372: applies global environment variables to agent processes, enforces precedence rules, maintains isolation, provides documentation, and updates UI accordingly.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing global environment variable support for agents and terminals. Documentation updates, UI enhancements, and process spawning refactors align with the stated objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/main/ipc/handlers/process.ts (1)

403-445: ⚠️ Potential issue | 🟠 Major

Global env vars aren’t applied to SSH remote agents.
The SSH stdin script is built with env: effectiveCustomEnvVars only, so globalShellEnvVars never reach remote sessions. This breaks the “global env vars apply to all agents” promise for SSH execution.

💡 Suggested fix (merge global + session env vars for SSH)

-						const sshCommand = await buildSshCommandWithStdin(sshResult.config, {
+						const mergedSshEnvVars = {
+							...globalShellEnvVars,
+							...(effectiveCustomEnvVars || {}),
+						};
+						const sshCommand = await buildSshCommandWithStdin(sshResult.config, {
 							command: remoteCommand,
 							args: sshArgs,
 							cwd: config.cwd,
-							env: effectiveCustomEnvVars,
+							env: mergedSshEnvVars,
 							// prompt is not passed as CLI arg - it goes via stdinInput
 							stdinInput,

If you want consistent tilde-expansion/stripping with local spawns, consider reusing the same env merge helper here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/ipc/handlers/process.ts` around lines 403 - 445, The SSH handler is
only passing effectiveCustomEnvVars to buildSshCommandWithStdin so
globalShellEnvVars never reach remote sessions; update the code that constructs
the env for buildSshCommandWithStdin to merge globalShellEnvVars with
effectiveCustomEnvVars (with session-specific vars taking precedence) and pass
that merged object as the env argument; reuse the existing
env-merge/tilde-expansion helper used elsewhere (or implement the same merge
logic) so tilde-expansion/stripping behavior matches local spawns and then pass
the mergedEnv into buildSshCommandWithStdin instead of effectiveCustomEnvVars.

src/main/process-manager/spawners/PtySpawner.ts (1)

69-90: ⚠️ Potential issue | 🟠 Major

Apply global env vars for non-terminal PTY agents too.
Right now only terminal PTY sessions merge shellEnvVars. If an agent requires PTY, this branch sets ptyEnv = process.env, which skips global env vars entirely.

💡 Suggested fix (merge globals for PTY agents)

-			} else {
-				// For AI agents in PTY mode: pass full env (they need NODE_PATH, etc.)
-				ptyEnv = process.env;
-			}
+			} else {
+				// For AI agents in PTY mode: include global env vars as well
+				ptyEnv = shellEnvVars ? { ...process.env, ...shellEnvVars } : process.env;
+			}

If you need the same tilde-expansion/stripping rules as buildChildProcessEnv, consider reusing that helper instead of a shallow merge.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/process-manager/spawners/PtySpawner.ts` around lines 69 - 90, The
non-terminal PTY branch in PtySpawner currently sets ptyEnv = process.env and
therefore ignores shellEnvVars; modify the non-terminal branch to merge
shellEnvVars into the PTY environment (using the same tilde-expansion/stripping
rules as buildChildProcessEnv if needed) so that agents running in PTY mode
receive global env vars; reference the PtySpawner code path that assigns ptyEnv,
the shellEnvVars variable, and reuse buildPtyTerminalEnv or buildChildProcessEnv
to perform a safe merge rather than a shallow overwrite.

🧹 Nitpick comments (1)

src/__tests__/integration/process-global-env-vars.test.ts (1)

1-434: These “integration tests” don’t exercise real behavior yet.
Most assertions are against static objects, so regressions won’t be caught.

Consider either (a) wiring real mocks/spies around IPC → ProcessManager → envBuilder, or (b) marking these as describe.todo/it.todo and moving the spec text to docs until real integration scaffolding exists.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/integration/process-global-env-vars.test.ts` around lines 1 -
434, The tests are currently only asserting static objects and don't exercise
real behavior; either convert them to explicit todos or make them true
integration tests by wiring mocks/spies around the IPC handler and process spawn
flow: replace these spec-style describes/its with describe.todo/it.todo and move
the prose to docs if you don't want runnable checks, or implement mocks that
return configured global shell env vars from settingsManager and assert the
actual calls and data flow through the IPC handler → ProcessManager.spawn →
buildChildProcessEnv / buildPtyTerminalEnv (and any ChildProcessSpawner usage),
using spies to verify spawn was called with shellEnvVars and that environments
are merged correctly (also include tests for invalid/global/session precedence
by asserting merge behavior rather than static objects).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/configuration.md`:
- Around line 74-82: The fenced code block under "Example Configuration" lacks a
language tag (triggering MD040); update the triple-backtick fence for the
environment variables example to include an "env" language specifier (i.e.,
change ``` to ```env) so the block containing ANTHROPIC_API_KEY, HTTP_PROXY,
HTTPS_PROXY, DEBUG, and MY_TOOL_PATH is fenced as env.

In `@docs/env-vars.md`:
- Around line 382-408: The markdown tables in docs/env-vars.md are not separated
by blank lines which triggers MD058; update the file so each table block (for
example the "Core Implementation", "Settings & Storage", and "Testing" tables)
is preceded and followed by a blank line, ensuring there is an empty line before
the opening | header row and after the closing row for each table so the linter
passes; no code changes required beyond inserting those blank lines around the
table blocks.
- Around line 45-110: The markdown has fenced code blocks without language tags
(the ASCII diagram under the top diagram block and the numbered steps under
"Data Flow: Settings → Spawned Process" / "For Terminal Sessions"); update those
fences to include a language identifier (e.g., change ``` to ```text) for the
diagram block and the numbered-list block so they pass MD040 linting while
preserving content and spacing.

In `@src/renderer/components/SettingsModal.tsx`:
- Around line 119-132: commitChanges currently writes entries into newEnvVars
even when validateEntry returns an error; update commitChanges (and use
validateEntry and EnvVarEntry) so that you only add an entry to newEnvVars when
validateEntry(entry) returns falsy (no error) and entry.key.trim() is non-empty.
Keep collecting errors into errors[entry.id] as before, call
setValidationErrors(errors), and only call setEnvVars(newEnvVars) after
filtering out invalid entries.
- Around line 1477-1505: The tooltip trigger (the wrapper div with className
"group relative ..." containing HelpCircle) is only hover-activated and not
keyboard-accessible; make it focusable by adding tabIndex={0} (or tabIndex={-1}
if you prefer programmatic focus) and ensure keyboard focus displays the tooltip
by relying on the existing "group-hover:block" behavior—replace or augment with
a focus-visible/focus:block approach (e.g., use group-focus:block or add a
focused class) so the sibling tooltip div becomes visible on focus as well; also
add outline-none to the trigger if following the style guideline and ensure the
tooltip content remains reachable/readable for screen readers.

---

Outside diff comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 403-445: The SSH handler is only passing effectiveCustomEnvVars to
buildSshCommandWithStdin so globalShellEnvVars never reach remote sessions;
update the code that constructs the env for buildSshCommandWithStdin to merge
globalShellEnvVars with effectiveCustomEnvVars (with session-specific vars
taking precedence) and pass that merged object as the env argument; reuse the
existing env-merge/tilde-expansion helper used elsewhere (or implement the same
merge logic) so tilde-expansion/stripping behavior matches local spawns and then
pass the mergedEnv into buildSshCommandWithStdin instead of
effectiveCustomEnvVars.

In `@src/main/process-manager/spawners/PtySpawner.ts`:
- Around line 69-90: The non-terminal PTY branch in PtySpawner currently sets
ptyEnv = process.env and therefore ignores shellEnvVars; modify the non-terminal
branch to merge shellEnvVars into the PTY environment (using the same
tilde-expansion/stripping rules as buildChildProcessEnv if needed) so that
agents running in PTY mode receive global env vars; reference the PtySpawner
code path that assigns ptyEnv, the shellEnvVars variable, and reuse
buildPtyTerminalEnv or buildChildProcessEnv to perform a safe merge rather than
a shallow overwrite.

---

Nitpick comments:
In `@src/__tests__/integration/process-global-env-vars.test.ts`:
- Around line 1-434: The tests are currently only asserting static objects and
don't exercise real behavior; either convert them to explicit todos or make them
true integration tests by wiring mocks/spies around the IPC handler and process
spawn flow: replace these spec-style describes/its with describe.todo/it.todo
and move the prose to docs if you don't want runnable checks, or implement mocks
that return configured global shell env vars from settingsManager and assert the
actual calls and data flow through the IPC handler → ProcessManager.spawn →
buildChildProcessEnv / buildPtyTerminalEnv (and any ChildProcessSpawner usage),
using spies to verify spawn was called with shellEnvVars and that environments
are merged correctly (also include tests for invalid/global/session precedence
by asserting merge behavior rather than static objects).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Surround tables with blank lines.
The table blocks should be separated by blank lines to satisfy MD058.

✍️ Example fix

 ### Core Implementation
+
 | File                                                       | Purpose                            |
 | ---------------------------------------------------------- | ---------------------------------- |
 | `src/main/process-manager/utils/envBuilder.ts`             | Environment construction functions |
 | `src/main/process-manager/spawners/ChildProcessSpawner.ts` | Agent spawning with env merging    |
 | `src/main/process-manager/spawners/PtySpawner.ts`          | Terminal spawning with env         |
 | `src/main/ipc/handlers/process.ts`                         | IPC handler loading global vars    |
+
 ### Settings & Storage

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/env-vars.md` around lines 382 - 408, The markdown tables in
docs/env-vars.md are not separated by blank lines which triggers MD058; update
the file so each table block (for example the "Core Implementation", "Settings &
Storage", and "Testing" tables) is preceded and followed by a blank line,
ensuring there is an empty line before the opening | header row and after the
closing row for each table so the linter passes; no code changes required beyond
inserting those blank lines around the table blocks.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/process-manager/spawners/PtySpawner.ts`:
- Around line 88-90: The PTY spawner currently only merges shellEnvVars into
ptyEnv and omits session-level customEnvVars; extract customEnvVars (and
isResuming if available) from the config in PtySpawner.ts and include them when
building ptyEnv—either call the existing buildChildProcessEnv(customEnvVars,
isResuming, shellEnvVars) like ChildProcessSpawner does, or merge with correct
precedence (ptyEnv = { ...process.env, ...shellEnvVars, ...customEnvVars }) so
customEnvVars override shell/global vars.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how customEnvVars flows through ProcessManager and spawners
echo "=== Check ProcessConfig type for customEnvVars ==="
rg -n "customEnvVars" --type=ts -C3 src/main/process-manager/

echo ""
echo "=== Check where PtySpawner is called and what config is passed ==="
rg -n "PtySpawner" --type=ts src/main/process-manager/ -C2

echo ""
echo "=== Check ProcessConfig interface ==="
rg -n "interface ProcessConfig\|type ProcessConfig" --type=ts -A20 src/main/

Repository: RunMaestro/Maestro

Length of output: 6970

---

🏁 Script executed:

#!/bin/bash
# Check PtySpawner around lines 88-90
echo "=== PtySpawner.ts around lines 88-90 ==="
sed -n '80,100p' src/main/process-manager/spawners/PtySpawner.ts | cat -n

echo ""
echo "=== Check if buildPtyTerminalEnv exists ==="
rg -n "buildPtyTerminalEnv" --type=ts src/main/process-manager/

echo ""
echo "=== Check PtySpawner.spawn signature and customEnvVars usage ==="
rg -n "spawn\|customEnvVars" --type=ts -A3 -B3 src/main/process-manager/spawners/PtySpawner.ts | head -80

echo ""
echo "=== Full PtySpawner constructor and spawn method signature ==="
sed -n '1,150p' src/main/process-manager/spawners/PtySpawner.ts | cat -n

Repository: RunMaestro/Maestro

Length of output: 7959

---

🏁 Script executed:

#!/bin/bash
# Check buildPtyTerminalEnv function
echo "=== buildPtyTerminalEnv function implementation ==="
sed -n '63,100p' src/main/process-manager/utils/envBuilder.ts | cat -n

echo ""
echo "=== Full buildPtyTerminalEnv including full function ==="
sed -n '40,130p' src/main/process-manager/utils/envBuilder.ts | cat -n

Repository: RunMaestro/Maestro

Length of output: 5406

---

Add customEnvVars support to PTY agents—currently missing session-level custom env vars.

For AI agents requiring PTY, the current merge at lines 88-90 only includes shellEnvVars (global), not customEnvVars (session-level). This differs from ChildProcessSpawner, which properly merges both via buildChildProcessEnv(customEnvVars, isResuming, shellEnvVars).

Extract customEnvVars from config and include it in the merge with appropriate precedence:

Fix

-		const { sessionId, toolType, cwd, command, args, shell, shellArgs, shellEnvVars } = config;
+		const { sessionId, toolType, cwd, command, args, shell, shellArgs, shellEnvVars, customEnvVars } = config;

[... lines 63-87 ...]

-			// For AI agents in PTY mode: merge global shell env vars with process env
-			// This ensures agents receive both system env vars (NODE_PATH, etc.) and global env vars
-			ptyEnv = shellEnvVars ? { ...process.env, ...shellEnvVars } : process.env;
+			// For AI agents in PTY mode: merge process env with global and session-level env vars
+			// Precedence: session vars > global vars > process.env
+			ptyEnv = { ...process.env, ...shellEnvVars, ...customEnvVars };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/process-manager/spawners/PtySpawner.ts` around lines 88 - 90, The
PTY spawner currently only merges shellEnvVars into ptyEnv and omits
session-level customEnvVars; extract customEnvVars (and isResuming if available)
from the config in PtySpawner.ts and include them when building ptyEnv—either
call the existing buildChildProcessEnv(customEnvVars, isResuming, shellEnvVars)
like ChildProcessSpawner does, or merge with correct precedence (ptyEnv = {
...process.env, ...shellEnvVars, ...customEnvVars }) so customEnvVars override
shell/global vars.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/configuration.md (1)

44-48: ⚠️ Potential issue | 🟡 Minor

Add a language tag to the example profile block.

This fenced block lacks a language identifier (MD040). Use text.

🔧 Suggested fix

-```
+```text
 Security researcher. macOS desktop. TypeScript and Python for tools.
 Direct communication, no fluff. Action over process. Push back on bad ideas.
 Generate markdown for Obsidian. CST timezone.

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/configuration.md around lines 44 - 48, Add the missing fenced-code
language tag to the example profile block so the markdown linter rule MD040 is
satisfied: locate the fenced block containing the profile text ("Security
researcher. macOS desktop. TypeScript and Python for tools. Direct
communication, no fluff. Action over process. Push back on bad ideas. Generate
markdown for Obsidian. CST timezone.") and change the opening fence from totext so it becomes a ```text fenced block.

</details>

</blockquote></details>

</blockquote></details>

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/env-vars.md`:
- Around line 154-187: Update the fenced code blocks in the examples so they
include language identifiers: add ```env to the KEY=VALUE blocks in Example 1,
Example 2, Example 3 and Example 4 (the Global/Session/Result snippets) and use
```text for the electron-store layout block labeled "electron-store" (also apply
the same change to the duplicate block referenced at lines 309-316). Ensure each
opening triple-backtick is followed by the proper language token and leave the
closing triple-backticks unchanged.

---

Outside diff comments:
In `@docs/configuration.md`:
- Around line 44-48: Add the missing fenced-code language tag to the example
profile block so the markdown linter rule MD040 is satisfied: locate the fenced
block containing the profile text ("Security researcher. macOS desktop.
TypeScript and Python for tools. Direct communication, no fluff. Action over
process. Push back on bad ideas. Generate markdown for Obsidian. CST timezone.")
and change the opening fence from ``` to ```text so it becomes a ```text fenced
block.

---

Duplicate comments:
In `@docs/env-vars.md`:
- Around line 382-408: The markdown tables under the headings "### Core
Implementation", "### Settings & Storage", and "### Testing" lack surrounding
blank lines (triggers MD058); fix by adding an empty line immediately before and
immediately after each table block so each table is separated from adjacent
headings/paragraphs (i.e., ensure a blank line above the table start and below
the table end for the three tables shown).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add language identifiers to fenced code blocks.

These example blocks still lack language tags, which triggers MD040. Use env for KEY=VALUE examples and text for the electron-store layout.

🔧 Suggested fix

-```
+```env
 Global: DEBUG=maestro:*
 Session: (not set)

 Result: DEBUG=maestro:*
-```
+```

-```
+```env
 Global:  API_KEY=global-key
 Session: API_KEY=session-key

 Result: API_KEY=session-key
-```
+```

-```
+```env
 Global:  API_KEY=key123
 Global:  DEBUG=on
 Session: DEBUG=off

 Result: API_KEY=key123, DEBUG=off
-```
+```

-```
+```env
 Global: WORKSPACE_PATH=~/my-workspace

 Result: WORKSPACE_PATH=/Users/john/my-workspace (expanded)
         (Same behavior on Windows with home directory)
-```
+```

-```
+```text
 electron-store
   → shellEnvVars: {
       "API_KEY": "sk-proj-xxxxx",
       "PROXY_URL": "http://proxy.local:8080",
       "DEBUG": "maestro:*"
     }
-```
+```

Also applies to: 309-316

🧰 Tools

🪛 markdownlint-cli2 (0.21.0)

[warning] 154-154: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 163-163: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 172-172: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 182-182: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/env-vars.md` around lines 154 - 187, Update the fenced code blocks in
the examples so they include language identifiers: add ```env to the KEY=VALUE
blocks in Example 1, Example 2, Example 3 and Example 4 (the
Global/Session/Result snippets) and use ```text for the electron-store layout
block labeled "electron-store" (also apply the same change to the duplicate
block referenced at lines 309-316). Ensure each opening triple-backtick is
followed by the proper language token and leave the closing triple-backticks
unchanged.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/__tests__/renderer/components/SettingsModal.test.tsx (1)

2443-2487: Add an assertion for the env‑var state in the special‑characters test.

Right now it only checks the warning text. Asserting whether the value is persisted (or not) prevents silent regressions.

✅ Suggested assertion

 			// Should show warning about special characters
 			expect(screen.getByText(/Value contains special characters/)).toBeInTheDocument();
 
-			// The value should still be added (warning, not error) but if we have strict validation,
-			// it won't be in the state. The current implementation adds it with a warning.
-			const lastCall = setShellEnvVars.mock.calls[setShellEnvVars.mock.calls.length - 1]?.[0] || {};
-			// Note: With current implementation, values with warnings still get added
-			// This is the current behavior - only full errors block the entry
+			const lastCall =
+				setShellEnvVars.mock.calls[setShellEnvVars.mock.calls.length - 1]?.[0] || {};
+			expect(lastCall['MY_VAR']).toBeUndefined();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/renderer/components/SettingsModal.test.tsx` around lines 2443 -
2487, Update the test "should not add entries with special characters in value
without quotes" in SettingsModal.test.tsx to assert the persisted env-var state
by inspecting the setShellEnvVars mock: after triggering the add and waiting,
grab the last call to setShellEnvVars and assert it contains the new key
"MY_VAR" with the value "value&with|special" (or assert absence if your intended
behavior is to reject such values); reference the setShellEnvVars mock and the
value/key inputs used in the test to locate where to add this assertion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 103-114: The validation currently returns an error for entries
with special chars in entry.value but the copy reads like a suggestion; update
the validation message in SettingsModal.tsx (the block that checks entry.value
for /[&|;`$<>()]/) to clearly state that quoting or escaping is required (e.g.,
"Value contains special characters; quoting or escaping is required") or
alternatively change the behavior to emit a non-blocking warning instead of
returning an error—adjust the validator (the function that uses
entry.key/entry.value) accordingly so the UI behavior matches the copy.
- Around line 118-139: The validationErrors map must be fully replaced with only
errors for the current entries when entries are re-synced: in commitChanges
(which computes errors and newEnvVars) ensure you call
setValidationErrors(errors) with the freshly built errors object (or set to {}
when no errors) so stale ids are removed—do not merge with previous state.
Update the commitChanges implementation to replace validationErrors (via
setValidationErrors(errors) or setValidationErrors({}) when errors is empty)
before calling setEnvVars(newEnvVars).

---

Duplicate comments:
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 1495-1510: Make the tooltip trigger keyboard-accessible by
updating the trigger element (the wrapper div around HelpCircle in
SettingsModal.tsx) to be focusable and not show a focus ring: add tabIndex={0}
and the "outline-none" class to that div, and change the tooltip’s visibility
classes (the div currently using "absolute hidden group-hover:block ...") to
also include "group-focus:block" so the tooltip shows on focus as well as hover;
optionally add an onKeyDown handler on the same wrapper to open/close on
Enter/Space if you want explicit keyboard activation.

---

Nitpick comments:
In `@src/__tests__/renderer/components/SettingsModal.test.tsx`:
- Around line 2443-2487: Update the test "should not add entries with special
characters in value without quotes" in SettingsModal.test.tsx to assert the
persisted env-var state by inspecting the setShellEnvVars mock: after triggering
the add and waiting, grab the last call to setShellEnvVars and assert it
contains the new key "MY_VAR" with the value "value&with|special" (or assert
absence if your intended behavior is to reject such values); reference the
setShellEnvVars mock and the value/key inputs used in the test to locate where
to add this assertion.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 266-270: The "Valid" count is being derived from local entries
which can include uncommitted additions; update the UI to compute the count from
the applied source of truth (envVars) instead of entries, e.g. replace the
entries.filter(...) usage with envVars.filter(...) and ensure the
validationErrors lookup uses the correct identifier present on envVars
(envVar.id or envVar.key as appropriate); alternatively, if you prefer to keep
entries, call commitChanges after addEntry so entries reflect applied envVars —
update the code in the SettingsModal component (references: entries, envVars,
addEntry, commitChanges, validationErrors).

---

Duplicate comments:
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 107-114: The validation in SettingsModal (the block that checks
entry.value with /[&|;`$<>()]/ and is used by commitChanges) currently returns a
message saying “Consider quoting or escaping.” but actually blocks the change;
update the message to explicitly state quoting/escaping is required (e.g.,
“Value contains special characters and must be quoted or escaped.”) so the copy
matches the blocking behavior; locate the conditional around entry.value and
update the returned string accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

“Valid” count should reflect applied env vars.

Line 266–270 derives the count from local entries. Since addEntry doesn’t call commitChanges, this can report more “loaded” vars than are actually applied. Prefer counting from envVars (source of truth) or call commitChanges after adding.

✅ Minimal fix (count from applied env vars)

-						✓ Valid ({entries.filter((e) => e.key.trim() && !validationErrors[e.id]).length}{' '}
+						✓ Valid ({Object.keys(envVars).length}{' '}
 						variables loaded)

📝 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.

Suggested change

	{entries.length > 0 && (
	<p className="text-xs opacity-60">
	✓ Valid ({entries.filter((e) => e.key.trim() && !validationErrors[e.id]).length}{' '}
	variables loaded)
	</p>
	{entries.length > 0 && (
	<p className="text-xs opacity-60">
	✓ Valid ({Object.keys(envVars).length}{' '}
	variables loaded)
	</p>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/SettingsModal.tsx` around lines 266 - 270, The
"Valid" count is being derived from local entries which can include uncommitted
additions; update the UI to compute the count from the applied source of truth
(envVars) instead of entries, e.g. replace the entries.filter(...) usage with
envVars.filter(...) and ensure the validationErrors lookup uses the correct
identifier present on envVars (envVar.id or envVar.key as appropriate);
alternatively, if you prefer to keep entries, call commitChanges after addEntry
so entries reflect applied envVars — update the code in the SettingsModal
component (references: entries, envVars, addEntry, commitChanges,
validationErrors).

[chr1syy]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/__tests__/main/ipc/handlers/process.test.ts`:
- Around line 1474-1569: Test fixtures use secret-like literals and currently
don't prove override precedence; update the test "should merge
globalShellEnvVars with effectiveCustomEnvVars when passing to SSH handler" to
use non-secret placeholders and add a duplicate key in the mocked global
shellEnvVars so sessionCustomEnvVars actually overrides it. Specifically, change
the mocked mockSettingsStore.get return for 'shellEnvVars' to non-secret values
(e.g., GLOBAL_API_KEY -> GLOBAL_KEY_PLACEHOLDER) and include SESSION_API_KEY
with a different global value so the sessionCustomEnvVars passed to
handlers.get('process:spawn') (sessionCustomEnvVars) can override it; then
assert via buildSshCommandWithStdin calls (mocked) and spawnCall.sshStdinScript
that SESSION_API_KEY from sessionCustomEnvVars is the final value. Ensure the
names buildSshCommandWithStdin, handlers.get('process:spawn'),
mockSettingsStore.get, sessionCustomEnvVars, and spawnCall.sshStdinScript are
used to locate and update the test.

[pedramamini]

@CodeRabbit please re-review

[coderabbitai]

@pedramamini: I'll perform a full re-review of all the changes in this PR.

---

🧠 Learnings used

Learnt from: CR
Repo: RunMaestro/Maestro PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T21:04:37.942Z
Learning: Applies to src/main/**/*.ts : Ensure SSH-spawned features use the correct agent type (don't hardcode 'claude-code'), pass custom agent configuration (customPath, customArgs, customEnvVars), and use agent's binaryName for remote execution.

---

Learnt from: CR
Repo: RunMaestro/Maestro PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T21:04:37.942Z
Learning: Applies to src/main/**/*.ts : When implementing features that spawn agent processes, support SSH remote execution by checking session.sshRemoteConfig and using wrapSpawnWithSsh() from src/main/utils/ssh-spawn-wrapper.ts.

✅ Actions performed

Full review triggered.

[coderabbitai]

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@docs/env-vars.md`:
- Around line 154-187: The fenced code blocks under "Example 1" through "Example
4" (MD040) lack language identifiers; update each triple-backtick block to
include an appropriate info string (e.g., ```env for environment-variable style
blocks or ```text where more appropriate) so the blocks are lint-compliant;
ensure you update every instance shown (the blocks after the Example headings
and the additional block referenced at lines 309–316) keeping the existing
content unchanged except for adding the language identifier.

In `@src/main/process-manager/spawners/PtySpawner.ts`:
- Around line 24-91: The PTY agent branch is calling
buildChildProcessEnv(undefined, false, shellEnvVars) which omits session-level
custom env vars, so ensure the session custom envs from the spawner config
(e.g., the variable holding session custom envs, likely customEnvVars or similar
on the config) are passed into buildChildProcessEnv when building ptyEnv for
non-terminal PTY agents; update the ptyEnv assignment in the else branch to pass
that session custom envs argument to buildChildProcessEnv (and adjust the
function call/order to match its signature) so session > global > process
precedence is preserved.

In `@src/renderer/components/SettingsModal.tsx`:
- Around line 266-270: The "Valid" count currently uses entries (in
SettingsModal render) which counts uncommitted new rows; change it to count from
the applied env state (envVars) instead: replace the expression
entries.filter((e) => e.key.trim() && !validationErrors[e.id]).length with a
derivation from envVars (e.g., envVars.filter(v => v.key.trim() &&
!validationErrors[v.id_or_mapping]).length) or, alternatively, commit new
entries when added so entries === envVars; update the reference to the correct
id/key field used by envVars to match validationErrors mapping so the displayed
"✓ Valid" number reflects only applied variables.
- Around line 97-114: The validation messages in validateEntry (EnvVarEntry) are
currently phrased as suggestions while the function treats them as blocking
errors; update the text to be explicit about invalidity: change the
variable-name message to something like "Invalid variable name: only letters,
numbers, and underscores allowed and must not start with a number." and change
the special-character message to "Invalid value: contains disallowed special
characters; quote or escape them if you intend to include them." Ensure these
updated strings replace the two current return messages in validateEntry so the
copy matches the blocking validation behavior.

[chr1syy]

Addressed latest review