From 0dab3116b794cef0c79075d2482ace45500f1b62 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 15 Apr 2026 10:43:44 +0900 Subject: [PATCH] fix: resolve 3 bugs (#3366, #3272, #3222) - shell-env: detect Git Bash via MSYSTEM env var when SHELL is unset (#3366) On some Git Bash installations SHELL is not set but MSYSTEM (MINGW64/MSYS) is always present. Check MSYSTEM before PSModulePath to avoid emitting PowerShell syntax in bash shells. - session-state: resolve legacy agent names in resolveRegisteredAgentName (#3272) Historical sessions stored agent names like 'Sisyphus (Ultraworker)' which don't match the current registered format. Fall back to getAgentConfigKey for legacy/parenthesized name resolution before returning the raw name. - config-migration: skip backup when file content is unchanged (#3222) Compare serialized config with existing file content before creating a timestamped .bak file. Only create backup when the on-disk content actually differs from the migrated content. --- .../claude-code-session-state/state.test.ts | 22 +++++++++ .../claude-code-session-state/state.ts | 14 +++++- src/hooks/non-interactive-env/index.test.ts | 37 ++++++++++++-- src/shared/migration/config-migration.test.ts | 49 +++++++++++++++++++ src/shared/migration/config-migration.ts | 30 +++++++++--- src/shared/shell-env.test.ts | 26 ++++++++++ src/shared/shell-env.ts | 7 +++ 7 files changed, 172 insertions(+), 13 deletions(-) diff --git a/src/features/claude-code-session-state/state.test.ts b/src/features/claude-code-session-state/state.test.ts index 69c482b40..c6898adbf 100644 --- a/src/features/claude-code-session-state/state.test.ts +++ b/src/features/claude-code-session-state/state.test.ts @@ -150,6 +150,28 @@ describe("claude-code-session-state", () => { expect(resolveRegisteredAgentName("Atlas - Plan Executor")).toBe("\u200B\u200B\u200B\u200BAtlas - Plan Executor") }) + test("should resolve legacy parenthesized names to registered agent", () => { + // given - agent registered with new display name format + registerAgentName("\u200BSisyphus - Ultraworker") + + // when - historical session has old parenthesized format + const resolved = resolveRegisteredAgentName("Sisyphus (Ultraworker)") + + // then - resolves to registered name via config key lookup + expect(resolved).toBe("\u200BSisyphus - Ultraworker") + }) + + test("should resolve bare lowercase name from historical session", () => { + // given - agent registered with new display name + registerAgentName("Prometheus - Plan Builder") + + // when - old session stored just "prometheus" + const resolved = resolveRegisteredAgentName("prometheus") + + // then + expect(resolved).toBe("Prometheus - Plan Builder") + }) + describe("#given atlas display name with zero-width prefix", () => { describe("#when checking registration without the zero-width prefix", () => { test("#then it treats the display name as registered", () => { diff --git a/src/features/claude-code-session-state/state.ts b/src/features/claude-code-session-state/state.ts index 496d655fd..049366166 100644 --- a/src/features/claude-code-session-state/state.ts +++ b/src/features/claude-code-session-state/state.ts @@ -52,7 +52,19 @@ export function resolveRegisteredAgentName(name: string | undefined): string | u } const normalizedName = normalizeRegisteredAgentName(name) - return registeredAgentAliases.get(normalizedName) ?? normalizeStoredAgentName(name) + const directMatch = registeredAgentAliases.get(normalizedName) + if (directMatch !== undefined) return directMatch + + // Resolve legacy/capitalized agent names (e.g. "Sisyphus (Ultraworker)") + // to their config key, then look up the registered alias for that key. + const configKey = getAgentConfigKey(name) + const normalizedConfigKey = normalizeRegisteredAgentName(configKey) + if (normalizedConfigKey !== normalizedName) { + const aliasMatch = registeredAgentAliases.get(normalizedConfigKey) + if (aliasMatch !== undefined) return aliasMatch + } + + return normalizeStoredAgentName(name) } /** @internal For testing only */ diff --git a/src/hooks/non-interactive-env/index.test.ts b/src/hooks/non-interactive-env/index.test.ts index 4e3419eb4..ecf5e06a9 100644 --- a/src/hooks/non-interactive-env/index.test.ts +++ b/src/hooks/non-interactive-env/index.test.ts @@ -12,6 +12,7 @@ describe("non-interactive-env hook", () => { originalEnv = { SHELL: process.env.SHELL, PSModulePath: process.env.PSModulePath, + MSYSTEM: process.env.MSYSTEM, CI: process.env.CI, OPENCODE_NON_INTERACTIVE: process.env.OPENCODE_NON_INTERACTIVE, } @@ -252,6 +253,7 @@ describe("non-interactive-env hook", () => { test("#given Windows with PowerShell env #when bash tool git command executes #then uses powershell syntax", async () => { delete process.env.SHELL + delete process.env.MSYSTEM process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" Object.defineProperty(process, "platform", { value: "win32" }) @@ -276,6 +278,7 @@ describe("non-interactive-env hook", () => { test("#given Windows without SHELL env #when bash tool git command executes #then uses cmd syntax", async () => { delete process.env.PSModulePath delete process.env.SHELL + delete process.env.MSYSTEM Object.defineProperty(process, "platform", { value: "win32" }) const hook = createNonInteractiveEnvHook(mockCtx) @@ -296,8 +299,7 @@ describe("non-interactive-env hook", () => { expect(cmd).not.toContain("export ") }) - test("#given Windows Git Bash environment #when git command executes #then uses detected shell syntax", async () => { - // Git Bash sets SHELL env var — detectShellType respects this + test("#given Windows Git Bash environment with SHELL #when git command executes #then uses unix syntax", async () => { delete process.env.PSModulePath process.env.SHELL = "/usr/bin/bash" Object.defineProperty(process, "platform", { value: "win32" }) @@ -313,14 +315,37 @@ describe("non-interactive-env hook", () => { ) const cmd = output.args.command as string - // Verify env prefix is applied (exact syntax depends on detected shell) - expect(cmd).toContain("git status") - expect(cmd.length).toBeGreaterThan("git status".length) + expect(cmd).toStartWith("export ") + expect(cmd).toContain("; git status") + expect(cmd).not.toContain("$env:") + }) + + test("#given Windows Git Bash via MSYSTEM without SHELL #when git command executes #then uses unix syntax", async () => { + delete process.env.SHELL + process.env.MSYSTEM = "MINGW64" + process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" + Object.defineProperty(process, "platform", { value: "win32" }) + + const hook = createNonInteractiveEnvHook(mockCtx) + const output: { args: Record; message?: string } = { + args: { command: "git status" }, + } + + await hook["tool.execute.before"]( + { tool: "bash", sessionID: "test", callID: "1" }, + output + ) + + const cmd = output.args.command as string + expect(cmd).toStartWith("export ") + expect(cmd).toContain("; git status") + expect(cmd).not.toContain("$env:") }) test("#given Windows platform #when chained git commands via bash tool #then uses cmd syntax", async () => { delete process.env.PSModulePath delete process.env.SHELL + delete process.env.MSYSTEM Object.defineProperty(process, "platform", { value: "win32" }) const hook = createNonInteractiveEnvHook(mockCtx) @@ -366,6 +391,7 @@ describe("non-interactive-env hook", () => { test("#given PSModulePath set on non-Windows #when git command executes #then uses powershell syntax", async () => { // PowerShell detection via PSModulePath should work regardless of platform delete process.env.SHELL + delete process.env.MSYSTEM process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" Object.defineProperty(process, "platform", { value: "linux" }) @@ -389,6 +415,7 @@ describe("non-interactive-env hook", () => { // Platform fallback: win32 without env hints should use cmd delete process.env.SHELL delete process.env.PSModulePath + delete process.env.MSYSTEM Object.defineProperty(process, "platform", { value: "win32" }) const hook = createNonInteractiveEnvHook(mockCtx) diff --git a/src/shared/migration/config-migration.test.ts b/src/shared/migration/config-migration.test.ts index 88f288e5b..35f8b574a 100644 --- a/src/shared/migration/config-migration.test.ts +++ b/src/shared/migration/config-migration.test.ts @@ -119,3 +119,52 @@ describe("migrateConfigFile sidecar write ordering", () => { expect(statSync(getSidecarPath(configPath)).isDirectory()).toBe(true) }) }) + +describe("migrateConfigFile backup skipping", () => { + test("skips backup when file content is identical after migration", () => { + // given - config with legacy key that migrates to same on-disk content + const workdir = createWorkdir() + const configPath = join(workdir, "oh-my-opencode.json") + const migratedContent = { + disabled_hooks: ["comment-checker"], + } + + // Write the already-migrated content to disk + writeFileSync(configPath, JSON.stringify(migratedContent, null, 2) + "\n") + + // rawConfig still has the legacy hook that will be removed + const rawConfig: Record = { + disabled_hooks: ["gpt-permission-continuation", "comment-checker"], + } + + // when + migrateConfigFile(configPath, rawConfig) + + // then - no backup file should be created since file content is unchanged + const files = require("fs").readdirSync(workdir) as string[] + const backupFiles = files.filter((f: string) => f.includes(".bak.")) + expect(backupFiles.length).toBe(0) + }) + + test("creates backup when file content actually changes", () => { + // given - config with model that needs migration + const workdir = createWorkdir() + const configPath = join(workdir, "oh-my-opencode.json") + const rawConfig = { + agents: { + prometheus: { model: "anthropic/claude-opus-4-5" }, + }, + } + + writeFileSync(configPath, JSON.stringify(rawConfig, null, 2) + "\n") + + // when + const needsWrite = migrateConfigFile(configPath, rawConfig as Record) + + // then - backup should be created since content changed + expect(needsWrite).toBe(true) + const files = require("fs").readdirSync(workdir) as string[] + const backupFiles = files.filter((f: string) => f.includes(".bak.")) + expect(backupFiles.length).toBe(1) + }) +}) diff --git a/src/shared/migration/config-migration.ts b/src/shared/migration/config-migration.ts index 11b34156f..792ca1083 100644 --- a/src/shared/migration/config-migration.ts +++ b/src/shared/migration/config-migration.ts @@ -143,20 +143,36 @@ export function migrateConfigFile( } if (needsWrite) { + let finalConfig = JSON.parse(JSON.stringify(copy)) as Record + const newContent = JSON.stringify(finalConfig, null, 2) + "\n" + + // Compare with existing file content to skip backup when unchanged. + // The config may still need an in-memory migration even if the file + // content is identical (e.g. removing a deleted hook from disabled_hooks + // results in content that was already written by a prior migration). + let existingContent: string | undefined + try { + existingContent = fs.readFileSync(configPath, "utf-8") + } catch { + // File may not exist yet + } + const contentChanged = existingContent !== newContent + const timestamp = new Date().toISOString().replace(/[:.]/g, "-") const backupPath = `${configPath}.bak.${timestamp}` let backupSucceeded = false - try { - fs.copyFileSync(configPath, backupPath) - backupSucceeded = true - } catch { - backupSucceeded = false + if (contentChanged) { + try { + fs.copyFileSync(configPath, backupPath) + backupSucceeded = true + } catch { + backupSucceeded = false + } } let writeSucceeded = false - let finalConfig = JSON.parse(JSON.stringify(copy)) as Record try { - writeFileAtomically(configPath, JSON.stringify(finalConfig, null, 2) + "\n") + writeFileAtomically(configPath, newContent) writeSucceeded = true } catch (err) { log(`Failed to write migrated config to ${configPath}:`, err) diff --git a/src/shared/shell-env.test.ts b/src/shared/shell-env.test.ts index 60a4aaba4..ef9aa52ea 100644 --- a/src/shared/shell-env.test.ts +++ b/src/shared/shell-env.test.ts @@ -10,6 +10,7 @@ describe("shell-env", () => { originalEnv = { SHELL: process.env.SHELL, PSModulePath: process.env.PSModulePath, + MSYSTEM: process.env.MSYSTEM, } }) @@ -47,6 +48,7 @@ describe("shell-env", () => { test("#given PSModulePath is set without SHELL #when detectShellType is called #then returns powershell", () => { delete process.env.SHELL + delete process.env.MSYSTEM process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" Object.defineProperty(process, "platform", { value: "win32" }) @@ -58,6 +60,7 @@ describe("shell-env", () => { test("#given Windows platform without PSModulePath #when detectShellType is called #then returns cmd", () => { delete process.env.PSModulePath delete process.env.SHELL + delete process.env.MSYSTEM Object.defineProperty(process, "platform", { value: "win32" }) const result = detectShellType() @@ -68,6 +71,7 @@ describe("shell-env", () => { test("#given non-Windows platform without SHELL env var #when detectShellType is called #then returns unix", () => { delete process.env.PSModulePath delete process.env.SHELL + delete process.env.MSYSTEM Object.defineProperty(process, "platform", { value: "linux" }) const result = detectShellType() @@ -94,6 +98,28 @@ describe("shell-env", () => { expect(result).toBe("unix") }) + + test("#given MSYSTEM set on Windows without SHELL #when detectShellType is called #then returns unix", () => { + delete process.env.SHELL + process.env.MSYSTEM = "MINGW64" + process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" + Object.defineProperty(process, "platform", { value: "win32" }) + + const result = detectShellType() + + expect(result).toBe("unix") + }) + + test("#given MSYSTEM set to MSYS without SHELL #when detectShellType is called #then returns unix", () => { + delete process.env.SHELL + process.env.MSYSTEM = "MSYS" + process.env.PSModulePath = "C:\\Program Files\\PowerShell\\Modules" + Object.defineProperty(process, "platform", { value: "win32" }) + + const result = detectShellType() + + expect(result).toBe("unix") + }) }) describe("shellEscape", () => { diff --git a/src/shared/shell-env.ts b/src/shared/shell-env.ts index d34ee3254..28041298a 100644 --- a/src/shared/shell-env.ts +++ b/src/shared/shell-env.ts @@ -21,6 +21,13 @@ export function detectShellType(): ShellType { return "unix" } + // Git Bash on Windows sets MSYSTEM (e.g. "MINGW64", "MINGW32", "MSYS") + // even when SHELL is not set. Detect this before PSModulePath which is + // always present on Windows regardless of the active shell. + if (process.env.MSYSTEM) { + return "unix" + } + if (process.env.PSModulePath) { return "powershell" }