fix(group-chat): pass global shell env vars to moderator/participant spawns#887
Conversation
…spawns
Group Chat spawns in group-chat-router.ts were calling
processManager.spawn() without shellEnvVars, so environment variables
set in Settings → Shell Configuration → Global Environment Variables
were not merged into the child process environment for the moderator,
participants, synthesis, or recovery.
Individual (1-on-1) chats go through ipc/handlers/process.ts which
correctly reads settingsStore.get('shellEnvVars') and passes it to
spawn(), so this bug only affected Group Chats.
In practice this meant users relying on a globally-configured
ANTHROPIC_API_KEY (or ANTHROPIC_AUTH_TOKEN / ANTHROPIC_BASE_URL)
saw the moderator immediately fail with a synthetic auth-failed
stub from the Claude CLI:
{ "model": "<synthetic>", "error": "authentication_failed" }
because envBuilder.ts strips CLAUDECODE/CLAUDE_CODE_*/ELECTRON_* and
the merged shellEnvVars layer was missing, so the CLI launched with
no credentials.
Changes:
- Add getGlobalShellEnvVars() helper that reads from getSettingsStore()
(mirrors ipc/handlers/process.ts:230).
- Pass shellEnvVars at all four processManager.spawn() sites in
group-chat-router.ts (moderator, participant, synthesis, recovery).
- Add shellEnvVars to the IProcessManager spawn config type in
group-chat-moderator.ts to match ProcessConfig.
- Mock stores/getters in group-chat-router.test.ts so tests that
exercise these spawn paths don't blow up on uninitialized stores.
Fixes RunMaestro#886
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReads global shell environment variables from Settings and passes them into group-chat spawn paths; merges global vars with per-agent custom env (per-agent wins) and ensures SSH-wrapped spawns receive merged envs. Changes
Sequence Diagram(s)sequenceDiagram
participant Router as GroupChat Router
participant Settings as Settings Store
participant ProcMgr as ProcessManager
participant SSH as wrapSpawnWithSsh
Router->>Settings: get('shellEnvVars', {})
Router->>Router: merge(globalShellEnvVars, agent.customEnvVars)\n(agent customEnvVars take precedence)
Router->>ProcMgr: spawn({ customEnvVars: merged, shellEnvVars: globalShellEnvVars, ... })
ProcMgr->>SSH: wrapSpawnWithSsh(..., customEnvVars: merged, customEnvVarsForSsh: merged)
SSH->>ProcMgr: returns wrapped spawn
ProcMgr->>Router: spawn started / pid
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR fixes group-chat spawns missing global shell env vars (e.g. Confidence Score: 5/5Safe to merge — targeted, well-tested fix with no logic regressions. All four spawn sites are consistently updated, the SSH merge pattern correctly mirrors the individual-chat handler, per-agent precedence is preserved, and new tests directly verify the fix including the SSH precedence case. No P0/P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[routeUserMessage / routeModeratorResponse / respawnParticipantWithRecovery] --> B{SSH remote configured?}
B -- Yes --> C["mergeGlobalShellWithCustomEnv(perAgentEnvVars)\n= { ...globalShellEnvVars, ...perAgentEnvVars }"]
C --> D["wrapSpawnWithSsh({ customEnvVars: merged })"]
D --> E["spawnEnvVars = sshWrapped.customEnvVars"]
E --> F["processManager.spawn({ customEnvVars: spawnEnvVars,\n shellEnvVars: getGlobalShellEnvVars() })"]
B -- No --> G["spawnEnvVars = perAgentEnvVars"]
G --> F
F --> H["envBuilder merges shellEnvVars + customEnvVars\n(per-agent wins on conflict)"]
H --> I[Child process launched with full env]
Reviews (2): Last reviewed commit: "fix(group-chat): merge global shell env ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
src/main/group-chat/group-chat-router.ts (1)
778-827:⚠️ Potential issue | 🟠 MajorMerge global shell env vars into SSH wrapping input with custom env taking precedence.
The
wrapSpawnWithSsh()function correctly embedscustomEnvVarsinto the remote SSH command viabuildRemoteCommand(). However,getGlobalShellEnvVars()is not being merged into thecustomEnvVarspassed to the wrapper—it's only passed toprocessManager.spawn()after wrapping. For SSH sessions, this means global variables likeANTHROPIC_API_KEYare applied only to the localsshprocess, not forwarded to the remote agent.Merge
getGlobalShellEnvVars()into thecustomEnvVarsinput towrapSpawnWithSsh()at all four locations (lines 778–827, 1266–1319, 1690–1739, 1892–1938), with session/moderator custom env vars taking precedence. Custom env vars should override globals where both are set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 778 - 827, The SSH wrapper call is not receiving global shell env vars so globals (e.g., ANTHROPIC_API_KEY) aren't forwarded to the remote process; before calling wrapSpawnWithSsh (the calls that build the remote command), merge getGlobalShellEnvVars() into the customEnvVars passed in so that the merged object is passed as customEnvVars to wrapSpawnWithSsh — ensure the merge uses session/moderator custom vars (the existing configResolution.effectiveCustomEnvVars or getCustomEnvVarsCallback(...)) to override global keys when present; apply this merge in the wrapSpawnWithSsh invocations that set spawnEnvVars/spawnCommand/spawnArgs (the block using chat.moderatorConfig?.sshRemoteConfig and the three other similar blocks referenced in the review).
🧹 Nitpick comments (1)
src/__tests__/main/group-chat/group-chat-router.test.ts (1)
49-55: Make the settings mock configurable enough to test the regression.This mock keeps the suite isolated, but because it always returns the default, the new behavior would still pass if
group-chat-router.tsused the wrong key or stopped passingshellEnvVars. Consider returning a sentinel forshellEnvVarsin at least one moderator and participant spawn test and asserting it reachesmockProcessManager.spawn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/group-chat/group-chat-router.test.ts` around lines 49 - 55, The settings mock always returning the default hides regressions; update the vi.mock for getSettingsStore so get(key, defaultValue) returns a configurable value when key === 'shellEnvVars' (e.g., a sentinel object) but falls back to default for other keys, then in the moderator and participant spawn tests set that sentinel in the mocked settings and add assertions that mockProcessManager.spawn was called with an env containing that sentinel under shellEnvVars, targeting the getSettingsStore mock and mockProcessManager.spawn to validate the value flows through group-chat-router.ts.
🤖 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/group-chat/group-chat-moderator.ts`:
- Around line 31-32: The GenericProcessManager interface is missing six optional
fields used by spawn() in the router; update the GenericProcessManager
definition to add these optional properties with appropriate
types—shellEnvVars?: Record<string,string>, promptArgs?: string[], shell?:
string, runInShell?: boolean, sendPromptViaStdin?: boolean, and
sendPromptViaStdinRaw?: boolean—so the interface matches how spawn() is invoked
(e.g., in group-chat-router.ts) and prevents type mismatches.
---
Outside diff comments:
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 778-827: The SSH wrapper call is not receiving global shell env
vars so globals (e.g., ANTHROPIC_API_KEY) aren't forwarded to the remote
process; before calling wrapSpawnWithSsh (the calls that build the remote
command), merge getGlobalShellEnvVars() into the customEnvVars passed in so that
the merged object is passed as customEnvVars to wrapSpawnWithSsh — ensure the
merge uses session/moderator custom vars (the existing
configResolution.effectiveCustomEnvVars or getCustomEnvVarsCallback(...)) to
override global keys when present; apply this merge in the wrapSpawnWithSsh
invocations that set spawnEnvVars/spawnCommand/spawnArgs (the block using
chat.moderatorConfig?.sshRemoteConfig and the three other similar blocks
referenced in the review).
---
Nitpick comments:
In `@src/__tests__/main/group-chat/group-chat-router.test.ts`:
- Around line 49-55: The settings mock always returning the default hides
regressions; update the vi.mock for getSettingsStore so get(key, defaultValue)
returns a configurable value when key === 'shellEnvVars' (e.g., a sentinel
object) but falls back to default for other keys, then in the moderator and
participant spawn tests set that sentinel in the mocked settings and add
assertions that mockProcessManager.spawn was called with an env containing that
sentinel under shellEnvVars, targeting the getSettingsStore mock and
mockProcessManager.spawn to validate the value flows through
group-chat-router.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 759896af-b687-459a-9b7c-4bf2d60de108
📒 Files selected for processing (3)
src/__tests__/main/group-chat/group-chat-router.test.tssrc/main/group-chat/group-chat-moderator.tssrc/main/group-chat/group-chat-router.ts
| /** Global shell env vars from Settings → Shell Configuration (merged by envBuilder). */ | ||
| shellEnvVars?: Record<string, string>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find process-manager-like spawn config interfaces that may need shellEnvVars added.
rg -n -C8 'interface .*ProcessManager|spawn\(config:\s*\{' --iglob '*.ts'Repository: RunMaestro/Maestro
Length of output: 5080
🏁 Script executed:
#!/bin/bash
# Get full IProcessManager definition in group-chat-moderator.ts
echo "=== IProcessManager in group-chat-moderator.ts ==="
sed -n '21,40p' src/main/group-chat/group-chat-moderator.ts
echo ""
echo "=== GenericProcessManager in groupChat.ts ==="
sed -n '129,150p' src/main/ipc/handlers/groupChat.ts
echo ""
echo "=== GroomingProcessManager in context-groomer.ts ==="
sed -n '26,45p' src/main/utils/context-groomer.tsRepository: RunMaestro/Maestro
Length of output: 2190
🏁 Script executed:
#!/bin/bash
# Check how GenericProcessManager is used in groupChat.ts
echo "=== Search for GenericProcessManager usage ==="
rg -n "GenericProcessManager" src/main/ipc/handlers/groupChat.ts -A 3 -B 3
echo ""
echo "=== Check moderatorRunner and agentRunner types ==="
rg -n "moderatorRunner|agentRunner" src/main/ipc/handlers/groupChat.ts -A 2 -B 2
echo ""
echo "=== Check if GenericProcessManager is instantiated or casted ==="
grep -n "new\|as GenericProcessManager\|as IProcessManager" src/main/ipc/handlers/groupChat.ts || echo "No direct instantiation found"Repository: RunMaestro/Maestro
Length of output: 2495
🏁 Script executed:
#!/bin/bash
# Check where getProcessManager is called and what's passed to spawn()
echo "=== Find getProcessManager() calls ==="
rg -n "getProcessManager\(\)" src/main/ipc/handlers/groupChat.ts -A 5
echo ""
echo "=== Look for spawn() calls in groupChat.ts ==="
rg -n "\.spawn\(" src/main/ipc/handlers/groupChat.ts -A 8
echo ""
echo "=== Check GroupChatModerator instantiation ==="
rg -n "new GroupChatModerator\|GroupChatModerator\(" src/main/ipc/handlers/groupChat.ts -B 2 -A 5Repository: RunMaestro/Maestro
Length of output: 3785
🏁 Script executed:
#!/bin/bash
# Find spawnModerator and related spawn calls
echo "=== Search for spawnModerator function ==="
rg -n "function spawnModerator|const spawnModerator|export.*spawnModerator" src/main/ipc/handlers/groupChat.ts -A 20
echo ""
echo "=== Look for spawn calls within groupChat functions ==="
rg -n "processManager\.spawn\|\.spawn\(" src/main/ipc/handlers/groupChat.ts -B 3 -A 10
echo ""
echo "=== Check GroupChatModerator for spawn() calls ==="
rg -n "this\.processManager\.spawn\|processManager\.spawn" src/main/group-chat/group-chat-moderator.ts -B 2 -A 10Repository: RunMaestro/Maestro
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Simpler search for spawnModerator
echo "=== Plain search for spawnModerator ==="
grep -rn "spawnModerator" src/main/ipc/handlers/groupChat.ts src/main/group-chat/
echo ""
echo "=== Look for spawn in group-chat directory ==="
grep -n "\.spawn(" src/main/group-chat/group-chat-moderator.ts | head -20
echo ""
echo "=== Check what functions exist in groupChat.ts before line 200 ==="
sed -n '1,200p' src/main/ipc/handlers/groupChat.ts | grep -n "^\(function\|const\|export\)" | tail -20Repository: RunMaestro/Maestro
Length of output: 1781
🏁 Script executed:
#!/bin/bash
# Read spawnModerator implementation
echo "=== spawnModerator function ==="
sed -n '154,250p' src/main/group-chat/group-chat-moderator.ts
echo ""
echo "=== Look for spawn calls in the entire function ==="
grep -n "\.spawn(" src/main/group-chat/group-chat-moderator.ts -B 5 -A 15Repository: RunMaestro/Maestro
Length of output: 3163
🏁 Script executed:
#!/bin/bash
# Find actual spawn calls in group-chat-router.ts
echo "=== Spawn calls in group-chat-router.ts ==="
grep -n "\.spawn(" src/main/group-chat/group-chat-router.ts -B 5 -A 20 | head -80
echo ""
echo "=== Also check for shell-related fields ==="
grep -n "shellEnvVars\|shell:\|runInShell" src/main/group-chat/ -rRepository: RunMaestro/Maestro
Length of output: 5304
🏁 Script executed:
#!/bin/bash
# Check if there's any type casting or overriding of GenericProcessManager
echo "=== Where is processManager typed as GenericProcessManager vs IProcessManager? ==="
grep -n "processManager.*GenericProcessManager\|processManager.*IProcessManager" src/main/ipc/handlers/groupChat.ts
echo ""
echo "=== Check the actual type of processManager in group-chat-router.ts ==="
head -150 src/main/group-chat/group-chat-router.ts | grep -n "import.*ProcessManager\|ProcessManager\|processManager"
echo ""
echo "=== Verify the actual spawn call parameter types ==="
sed -n '817,834p' src/main/group-chat/group-chat-router.tsRepository: RunMaestro/Maestro
Length of output: 927
🏁 Script executed:
#!/bin/bash
# Final check: verify what fields are used in spawn calls to ensure completeness
echo "=== All unique fields passed to spawn() in group-chat-router.ts ==="
grep -o "^\s*[a-zA-Z]*:" src/main/group-chat/group-chat-router.ts | grep -A30 "\.spawn({" | sort -u
echo ""
echo "=== Compare against GenericProcessManager definition ==="
sed -n '129,150p' src/main/ipc/handlers/groupChat.ts
echo ""
echo "=== Compare against IProcessManager definition ==="
sed -n '21,45p' src/main/group-chat/group-chat-moderator.tsRepository: RunMaestro/Maestro
Length of output: 1763
Sync GenericProcessManager interface with IProcessManager spawn config.
GenericProcessManager is missing 6 optional fields that actual spawn() calls in group-chat-router.ts use: shellEnvVars, promptArgs, shell, runInShell, sendPromptViaStdin, and sendPromptViaStdinRaw. Adding these to GenericProcessManager will prevent type mismatches and keep the interface contracts aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/group-chat/group-chat-moderator.ts` around lines 31 - 32, The
GenericProcessManager interface is missing six optional fields used by spawn()
in the router; update the GenericProcessManager definition to add these optional
properties with appropriate types—shellEnvVars?: Record<string,string>,
promptArgs?: string[], shell?: string, runInShell?: boolean,
sendPromptViaStdin?: boolean, and sendPromptViaStdinRaw?: boolean—so the
interface matches how spawn() is invoked (e.g., in group-chat-router.ts) and
prevents type mismatches.
|
Thanks for the fix @josecsotomorales — nice diagnosis and the local-spawn path is clearly correct. Before we merge I'd like to get the SSH case sorted as well. CodeRabbit flagged this and after walking through the code I think it's a real gap: in all four The individual-chat path handles this at const mergedSshEnvVars = { ...globalShellEnvVars, ...(effectiveCustomEnvVars || {}) };…and feeds that merged object into the SSH command builder. Mirroring that here — merging Also worth tightening the test: the current Lint/tests all look clean and the scoping is good — just need the SSH path before this goes in. Thanks again! |
Addresses review feedback on the initial commit:
1. SSH path was still broken
wrapSpawnWithSsh() embeds only customEnvVars into the remote SSH stdin
script (ssh-spawn-wrapper.ts:171) and then returns customEnvVars: undefined
(line 189). The previous patch only added shellEnvVars to the *local*
processManager.spawn() call — for SSH sessions that lands on the local
ssh client process, not the remote agent, so group chats over SSH
continued to hit authentication_failed.
Mirror the individual-chat pattern at ipc/handlers/process.ts:437 by
merging global shell env vars into customEnvVars BEFORE calling
wrapSpawnWithSsh(), with per-agent effectiveCustomEnvVars taking
precedence. Applied at all four SSH wrap sites (moderator, participant,
synthesis, recovery) via a new mergeGlobalShellWithCustomEnv() helper.
2. GenericProcessManager type widened
The narrower GenericProcessManager interface in ipc/handlers/groupChat.ts
was missing shellEnvVars and several other optional spawn-config fields
already used by the router (promptArgs, shell, runInShell,
sendPromptViaStdin, sendPromptViaStdinRaw). Align it with IProcessManager
/ ProcessConfig.
3. Test tightening
The previous settings-store mock always returned the default, so a
regression that stopped threading shellEnvVars through would still pass.
Drive the mock from a mutable mockedShellEnvVars sentinel (reset in
beforeEach), and add three new assertions:
- moderator local spawn receives the sentinel as shellEnvVars
- participant local spawn receives the sentinel as shellEnvVars
- SSH participant spawn's wrapSpawnWithSsh input has the sentinel
merged into customEnvVars, with per-session customEnvVars winning
over the global for the same key
Refs PR review feedback (pedramamini, coderabbitai, greptile-apps).
|
Thanks for the review @pedramamini — great catch on the SSH path. Pushed 1. SSH gap closed. Added 2. 3. Test sentinel (Greptile's nit). The mock now reads from a mutable
Lint clean, 64/64 router tests pass (3 new), full suite 22524/22525 (same unrelated |
2 participants
Fixes #886.
Summary
Group Chat spawns in
group-chat-router.tswere callingprocessManager.spawn()withoutshellEnvVars, so environment variables set in Settings → Shell Configuration → Global Environment Variables were not merged into the child process environment. Individual (1-on-1) chats go throughipc/handlers/process.tswhich already passesshellEnvVarscorrectly, so this bug only affected Group Chats.In practice, users relying on a globally-configured
ANTHROPIC_API_KEY(orANTHROPIC_AUTH_TOKEN/ANTHROPIC_BASE_URL) saw the moderator immediately fail with a synthetic auth-failed stub from the Claude CLI:{ "model": "<synthetic>", "error": "authentication_failed" }because
envBuilder.tsstripsCLAUDECODE/CLAUDE_CODE_*/ELECTRON_*and the mergedshellEnvVarslayer was missing, so the CLI launched with no credentials.Changes
src/main/group-chat/group-chat-router.tsgetGlobalShellEnvVars()helper that reads fromgetSettingsStore()(mirrorsipc/handlers/process.ts:230).shellEnvVars: getGlobalShellEnvVars()at all fourprocessManager.spawn()sites: moderator (initial routing), participant, moderator synthesis, and participant recovery.src/main/group-chat/group-chat-moderator.tsshellEnvVars?: Record<string, string>to theIProcessManager.spawnconfig type so the router's calls typecheck. Matches the existingProcessConfig.shellEnvVarsfield.src/__tests__/main/group-chat/group-chat-router.test.tsstores/gettersso the router tests don't blow up on uninitialized stores (the 17 spawn-path tests previously passed because the router never calledgetSettingsStore(); now they need it stubbed).Test plan
npm run lint— cleannpx vitest run src/__tests__/main/group-chat/group-chat-router.test.ts— 61/61 passnpx vitest run(full suite) — 22521/22522 pass; the single failure (stats/integration.test.ts > electron-rebuild verification for better-sqlite3) is a native-module env check unrelated to this change and fails onmainin environments whereelectron-rebuild's gyp build can't complete.ANTHROPIC_API_KEYin Settings → Shell Configuration, start a Group Chat with Claude Code as moderator, confirm moderator responds withoutauthentication_failed.Notes
customEnvVars(per-agent / per-moderator) still wins overshellEnvVars(global) perenvBuilder.ts:199-238.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor