Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/providers/ssh-git-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ export class SshGitProvider implements IGitProvider {
cwd,
stdin: plan.stdinPayload,
timeoutMs,
// Why: forward leading `VAR=value` env assignments from the override so
// the remote CLI runs with the same environment as a local spawn.
...(plan.env ? { env: plan.env } : {}),
operation
},
undefined,
Expand Down
46 changes: 46 additions & 0 deletions src/main/text-generation/commit-message-text-generation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,52 @@ describe('generateCommitMessageFromContext', () => {
)
})

it('spawns the real binary with leading VAR=value override tokens applied to the env', async () => {
const listeners = new Map<string, (value: unknown) => void>()
const child = {
pid: 123,
kill: vi.fn(),
stdout: { on: vi.fn((event, callback) => listeners.set(`stdout:${event}`, callback)) },
stderr: { on: vi.fn((event, callback) => listeners.set(`stderr:${event}`, callback)) },
stdin: { end: vi.fn() },
on: vi.fn((event, callback) => listeners.set(event, callback))
}
spawnMock.mockReturnValue(child as never)

const pending = generateCommitMessageFromContext(
{
branch: 'main',
stagedSummary: 'M\tREADME.md',
stagedPatch: '+hello'
},
{
agentId: 'claude',
model: 'sonnet',
agentCommandOverride: 'CLAUDE_CODE_NO_FLICKER=1 claude'
},
{
kind: 'local',
cwd: '/repo'
}
)

listeners.get('stdout:data')?.(Buffer.from('Add README note\n'))
listeners.get('close')?.(0)

await expect(pending).resolves.toMatchObject({
success: true,
message: 'Add README note'
})
// Why: the env-assignment prefix must become an env var, not the binary.
expect(spawnMock).toHaveBeenCalledWith(
'claude',
expect.not.arrayContaining(['CLAUDE_CODE_NO_FLICKER=1']),
expect.objectContaining({
env: expect.objectContaining({ CLAUDE_CODE_NO_FLICKER: '1' })
})
)
})

it('routes WSL local commit generation through the selected distro login shell', async () => {
await withPlatform('win32', async () => {
process.env.ORCA_HOST_ONLY_SECRET = 'do-not-leak'
Expand Down
17 changes: 12 additions & 5 deletions src/main/text-generation/commit-message-text-generation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,8 @@ function planModelDiscovery(
binary: command.binary,
args: [...command.prefixArgs, ...modelDiscovery.args],
stdinPayload: null,
label: spec.label
label: spec.label,
...(Object.keys(command.env).length ? { env: command.env } : {})
}
}
}
Expand All @@ -284,17 +285,21 @@ export async function discoverCommitMessageModelsLocal(

return new Promise((resolve) => {
let child: ChildProcess
const spawnEnv = env ?? process.env
try {
const planned = planModelDiscovery(spec, agentCommandOverride)
if (!planned.ok) {
resolve({ success: false, error: planned.error })
return
}
// Why: merge env-assignment prefixes from the override so model discovery
// runs the real binary with the same environment as generation.
const spawnEnv = planned.plan.env
? { ...(env ?? process.env), ...planned.plan.env }
: (env ?? process.env)
Comment on lines +296 to +298

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.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In Node.js child_process.spawn on Windows, how are duplicate environment keys that differ only by case (e.g., PATH vs Path) handled, and which value wins?

💡 Result:

In Node.js, when using child_process.spawn on Windows, duplicate environment keys that differ only by case are handled by a sanitization process that removes the duplicates [1]. Specifically, Node.js sorts the environment keys lexicographically and filters them so that only one entry remains for each case-insensitive key [1]. When multiple variants of a key exist (e.g., PATH and Path), the entry that comes first in lexicographic order (based on the original key string) is the one that is kept and passed to the spawned subprocess [2][1]. This behavior was implemented to address issues where certain applications or system processes become confused by the presence of duplicate environment variables on Windows, which natively treats environment variable names as case-insensitive [1]. Consequently, if you provide an environment object to spawn, you should ensure that only the desired version of any case-variant key is included, or be aware that the lexicographically first variant will effectively win [2][3].

Citations:


🏁 Script executed:

# Find and examine the file
find . -name "commit-message-text-generation.ts" -type f

Repository: stablyai/orca

Length of output: 118


🏁 Script executed:

# Check lines around 289-291 and 495-497 in the identified file
if [ -f "src/main/text-generation/commit-message-text-generation.ts" ]; then
  echo "=== Lines 280-300 ===" && sed -n '280,300p' "src/main/text-generation/commit-message-text-generation.ts" | cat -n
  echo ""
  echo "=== Lines 485-510 ===" && sed -n '485,510p' "src/main/text-generation/commit-message-text-generation.ts" | cat -n
fi

Repository: stablyai/orca

Length of output: 2409


🏁 Script executed:

# Search for any existing platform checks related to env or PATH in the file
rg -n "process\.platform|win32|darwin|linux" src/main/text-generation/commit-message-text-generation.ts

Repository: stablyai/orca

Length of output: 208


🏁 Script executed:

# Check if there are any existing PATH normalization utilities in the codebase
rg -n "PATH|path.*normalize|env.*case" --type ts --type js -A 2 | head -100

Repository: stablyai/orca

Length of output: 6881


Normalize PATH/Path environment key on Windows when merging plan.env.

These merges can result in both PATH and Path keys in the spawned environment. Node.js spawn on Windows sanitizes environment variables and keeps the lexicographically first key; PATH will win, causing any Path override to be silently ignored during process spawning.

-const spawnEnv = plan.env ? { ...(env ?? process.env), ...plan.env } : (env ?? process.env)
+let spawnEnv = plan.env ? { ...(env ?? process.env), ...plan.env } : (env ?? process.env)
+if (process.platform === 'win32' && plan.env) {
+  const pathOverride = plan.env.PATH ?? plan.env.Path
+  if (pathOverride !== undefined) {
+    delete spawnEnv.PATH
+    delete spawnEnv.Path
+    spawnEnv.Path = pathOverride
+  }
+}

Also applies to: 495-497

if (process.platform === 'win32' && options.wslDistro) {
child = wslAwareSpawn(planned.plan.binary, planned.plan.args, {
cwd: options.cwd,
env: buildWslLauncherEnv(env),
env: buildWslLauncherEnv(spawnEnv),
stdio: ['ignore', 'pipe', 'pipe'],
windowsHide: true,
wslDistro: options.wslDistro,
Expand Down Expand Up @@ -533,11 +538,13 @@ async function runLocalPlan(
return new Promise((resolve) => {
let child: ChildProcess
try {
const spawnEnv = env ?? process.env
// Why: `plan.env` carries leading `VAR=value` assignments from the agent
// command override; merge them over the base env so they reach the CLI.
const spawnEnv = plan.env ? { ...(env ?? process.env), ...plan.env } : (env ?? process.env)
if (process.platform === 'win32' && wslDistro) {
child = wslAwareSpawn(binary, args, {
cwd,
env: buildWslLauncherEnv(env),
env: buildWslLauncherEnv(spawnEnv),
stdio: ['pipe', 'pipe', 'pipe'],
windowsHide: true,
wslDistro,
Expand Down
62 changes: 62 additions & 0 deletions src/shared/commit-message-plan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,68 @@ describe('planCommitMessageGeneration', () => {
})
})

it('parses leading env assignments in a preset override into plan.env, spawning the real binary', () => {
const result = planCommitMessageGeneration(
{
agentId: 'claude',
model: 'sonnet',
agentCommandOverride: 'CLAUDE_CODE_NO_FLICKER=1 claude'
},
'PROMPT'
)

expect(result).toMatchObject({
ok: true,
plan: {
binary: 'claude',
env: { CLAUDE_CODE_NO_FLICKER: '1' }
}
})
// Why: the env-assignment token must not leak into the binary or argv.
if (result.ok) {
expect(result.plan.binary).toBe('claude')
expect(result.plan.args).not.toContain('CLAUDE_CODE_NO_FLICKER=1')
}
})

it('keeps env assignments before a wrapper binary in the override', () => {
const result = planCommitMessageGeneration(
{
agentId: 'codex',
model: 'gpt-5.4-mini',
agentCommandOverride: 'FOO=bar npx codex'
},
'PROMPT'
)

expect(result).toMatchObject({
ok: true,
plan: {
binary: 'npx',
env: { FOO: 'bar' }
}
})
if (result.ok) {
expect(result.plan.args.slice(0, 2)).toEqual(['codex', 'exec'])
}
})

it('omits plan.env when the override carries no env assignments', () => {
const result = planCommitMessageGeneration(
{
agentId: 'codex',
model: 'gpt-5.4-mini',
agentCommandOverride: 'npx codex'
},
'PROMPT'
)

expect(result.ok).toBe(true)
if (result.ok) {
expect(result.plan).not.toHaveProperty('env')
}
})

it('rejects invalid preset agent command overrides before spawning', () => {
const result = planCommitMessageGeneration(
{
Expand Down
29 changes: 22 additions & 7 deletions src/shared/commit-message-plan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import {
getCommitMessageModel,
isCustomAgentId
} from './commit-message-agent-spec'
import { planCustomCommand, tokenizeCustomCommandTemplate } from './commit-message-prompt'
import {
extractLeadingEnvAssignments,
planCustomCommand,
tokenizeCustomCommandTemplate
} from './commit-message-prompt'
import type { TuiAgent } from './types'

// Why: planning is a pure transformation from "user request + prompt text"
Expand All @@ -28,6 +32,12 @@ export type CommitMessagePlan = {
stdinPayload: string | null
/** Human-readable label used in error prefixes (e.g. "Claude failed: ..."). */
label: string
/**
* Environment variables parsed from leading `VAR=value` tokens in the agent
* command override / custom command. Merged over the spawn environment.
* Present only when assignments were supplied.
*/
env?: Record<string, string>
}

export type CommitMessagePlanResult =
Expand All @@ -37,21 +47,24 @@ export type CommitMessagePlanResult =
export function planAgentBinary(
defaultBinary: string,
commandOverride: string | undefined
): { ok: true; binary: string; prefixArgs: string[] } | { ok: false; error: string } {
):
| { ok: true; binary: string; prefixArgs: string[]; env: Record<string, string> }
| { ok: false; error: string } {
const command = commandOverride?.trim()
if (!command) {
return { ok: true, binary: defaultBinary, prefixArgs: [] }
return { ok: true, binary: defaultBinary, prefixArgs: [], env: {} }
}

const tokenized = tokenizeCustomCommandTemplate(command)
if (!tokenized.ok) {
return { ok: false, error: `Agent command override is invalid: ${tokenized.error}` }
}
const [binary, ...prefixArgs] = tokenized.tokens
const { env, rest } = extractLeadingEnvAssignments(tokenized.tokens)
const [binary, ...prefixArgs] = rest
if (!binary) {
return { ok: false, error: 'Agent command override must start with a binary name.' }
}
return { ok: true, binary, prefixArgs }
return { ok: true, binary, prefixArgs, env }
}

function planAdditionalAgentArgs(
Expand Down Expand Up @@ -126,7 +139,8 @@ export function planCommitMessageGeneration(
stdinPayload: planned.stdinPayload,
// Why: a custom command has no friendly name, so the binary doubles
// as the label in error prefixes ("ollama failed: ...").
label: planned.binary
label: planned.binary,
...(planned.env ? { env: planned.env } : {})
}
}
}
Expand Down Expand Up @@ -180,7 +194,8 @@ export function planCommitMessageGeneration(
binary: command.binary,
args: [...command.prefixArgs, ...args],
stdinPayload: spec.promptDelivery === 'stdin' ? prompt : null,
label: spec.label
label: spec.label,
...(Object.keys(command.env).length ? { env: command.env } : {})
}
}
}
65 changes: 65 additions & 0 deletions src/shared/commit-message-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
buildCommitPrompt,
cleanGeneratedCommitMessage,
extractAgentErrorMessage,
extractLeadingEnvAssignments,
planCustomCommand,
STAGED_DIFF_BYTE_BUDGET,
tokenizeCustomCommandTemplate,
Expand Down Expand Up @@ -220,12 +221,76 @@ describe('tokenizeCustomCommandTemplate', () => {
})
})

describe('extractLeadingEnvAssignments', () => {
it('peels a single leading VAR=value token off the front', () => {
const r = extractLeadingEnvAssignments(['CLAUDE_CODE_NO_FLICKER=1', 'claude', '-p'])
expect(r).toEqual({ env: { CLAUDE_CODE_NO_FLICKER: '1' }, rest: ['claude', '-p'] })
})

it('peels multiple consecutive assignments', () => {
const r = extractLeadingEnvAssignments(['FOO=bar', 'BAZ=qux', 'claude'])
expect(r).toEqual({ env: { FOO: 'bar', BAZ: 'qux' }, rest: ['claude'] })
})

it('stops at the first non-assignment token (later VAR=val stays an arg)', () => {
const r = extractLeadingEnvAssignments(['FOO=bar', 'claude', 'KEY=val'])
expect(r).toEqual({ env: { FOO: 'bar' }, rest: ['claude', 'KEY=val'] })
})

it('splits on the first = so values may contain =', () => {
const r = extractLeadingEnvAssignments(['URL=https://x?a=b', 'claude'])
expect(r).toEqual({ env: { URL: 'https://x?a=b' }, rest: ['claude'] })
})

it('allows empty values', () => {
const r = extractLeadingEnvAssignments(['EMPTY=', 'claude'])
expect(r).toEqual({ env: { EMPTY: '' }, rest: ['claude'] })
})

it('does not treat =value or a digit-led name as an assignment', () => {
expect(extractLeadingEnvAssignments(['=foo', 'claude'])).toEqual({
env: {},
rest: ['=foo', 'claude']
})
expect(extractLeadingEnvAssignments(['1AB=x', 'claude'])).toEqual({
env: {},
rest: ['1AB=x', 'claude']
})
})

it('returns an empty env when there are no assignments', () => {
expect(extractLeadingEnvAssignments(['claude', '-p'])).toEqual({
env: {},
rest: ['claude', '-p']
})
})
})

describe('planCustomCommand', () => {
it('routes prompt via stdin when {prompt} is absent', () => {
const r = planCustomCommand('claude -p', 'COMMIT MSG')
expect(r).toEqual({ ok: true, binary: 'claude', args: ['-p'], stdinPayload: 'COMMIT MSG' })
})

it('peels leading env assignments and spawns the real binary, not VAR=value', () => {
const r = planCustomCommand('CLAUDE_CODE_NO_FLICKER=1 claude -p', 'COMMIT MSG')
expect(r).toEqual({
ok: true,
binary: 'claude',
args: ['-p'],
stdinPayload: 'COMMIT MSG',
env: { CLAUDE_CODE_NO_FLICKER: '1' }
})
})

it('errors when the command is only env assignments', () => {
const r = planCustomCommand('FOO=bar', 'PROMPT')
expect(r.ok).toBe(false)
if (!r.ok) {
expect(r.error).toMatch(/binary name/i)
}
})

it('substitutes {prompt} as a whole token via argv', () => {
const r = planCustomCommand('codex exec {prompt}', 'PROMPT')
expect(r).toEqual({ ok: true, binary: 'codex', args: ['exec', 'PROMPT'], stdinPayload: null })
Expand Down
Loading