diff --git a/src/cli/__snapshots__/model-fallback.test.ts.snap b/src/cli/__snapshots__/model-fallback.test.ts.snap index e20e589fc..541731684 100644 --- a/src/cli/__snapshots__/model-fallback.test.ts.snap +++ b/src/cli/__snapshots__/model-fallback.test.ts.snap @@ -5,60 +5,60 @@ exports[`generateModelConfig no providers available returns ULTIMATE_FALLBACK fo "$schema": "https://raw.githubusercontent.com/code-yeongyu/oh-my-openagent/dev/assets/oh-my-opencode.schema.json", "agents": { "atlas": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "explore": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "hephaestus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "librarian": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "metis": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "momus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "prometheus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "sisyphus-junior": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, "categories": { "artistry": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "deep": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "quick": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "ultrabrain": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-high": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-low": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "visual-engineering": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "writing": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, } @@ -83,7 +83,7 @@ exports[`generateModelConfig single native provider uses Claude models when only "variant": "max", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { "model": "anthropic/claude-opus-4-6", @@ -145,7 +145,7 @@ exports[`generateModelConfig single native provider uses Claude models with isMa "variant": "max", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { "model": "anthropic/claude-opus-4-6", @@ -366,20 +366,20 @@ exports[`generateModelConfig single native provider uses Gemini models when only "$schema": "https://raw.githubusercontent.com/code-yeongyu/oh-my-openagent/dev/assets/oh-my-opencode.schema.json", "agents": { "atlas": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "explore": { "model": "opencode/gpt-5-nano", }, "metis": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "momus": { "model": "google/gemini-3.1-pro-preview", "variant": "high", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { "model": "google/gemini-3.1-pro-preview", @@ -389,7 +389,7 @@ exports[`generateModelConfig single native provider uses Gemini models when only "model": "google/gemini-3.1-pro-preview", }, "sisyphus-junior": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, "categories": { @@ -426,20 +426,20 @@ exports[`generateModelConfig single native provider uses Gemini models with isMa "$schema": "https://raw.githubusercontent.com/code-yeongyu/oh-my-openagent/dev/assets/oh-my-opencode.schema.json", "agents": { "atlas": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "explore": { "model": "opencode/gpt-5-nano", }, "metis": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "momus": { "model": "google/gemini-3.1-pro-preview", "variant": "high", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { "model": "google/gemini-3.1-pro-preview", @@ -449,7 +449,7 @@ exports[`generateModelConfig single native provider uses Gemini models with isMa "model": "google/gemini-3.1-pro-preview", }, "sisyphus-junior": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, "categories": { @@ -465,7 +465,7 @@ exports[`generateModelConfig single native provider uses Gemini models with isMa "variant": "high", }, "unspecified-high": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-low": { "model": "google/gemini-3-flash-preview", @@ -929,7 +929,7 @@ exports[`generateModelConfig fallback providers uses ZAI model for librarian whe "$schema": "https://raw.githubusercontent.com/code-yeongyu/oh-my-openagent/dev/assets/oh-my-opencode.schema.json", "agents": { "atlas": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "explore": { "model": "opencode/gpt-5-nano", @@ -938,45 +938,45 @@ exports[`generateModelConfig fallback providers uses ZAI model for librarian whe "model": "zai-coding-plan/glm-4.7", }, "metis": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "momus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "multimodal-looker": { "model": "zai-coding-plan/glm-4.6v", }, "oracle": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "prometheus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "sisyphus": { "model": "zai-coding-plan/glm-5", }, "sisyphus-junior": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, "categories": { "quick": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "ultrabrain": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-high": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-low": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "visual-engineering": { "model": "zai-coding-plan/glm-5", }, "writing": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, } @@ -987,7 +987,7 @@ exports[`generateModelConfig fallback providers uses ZAI model for librarian wit "$schema": "https://raw.githubusercontent.com/code-yeongyu/oh-my-openagent/dev/assets/oh-my-opencode.schema.json", "agents": { "atlas": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "explore": { "model": "opencode/gpt-5-nano", @@ -996,45 +996,45 @@ exports[`generateModelConfig fallback providers uses ZAI model for librarian wit "model": "zai-coding-plan/glm-4.7", }, "metis": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "momus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "multimodal-looker": { "model": "zai-coding-plan/glm-4.6v", }, "oracle": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "prometheus": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "sisyphus": { "model": "zai-coding-plan/glm-5", }, "sisyphus-junior": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, "categories": { "quick": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "ultrabrain": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "unspecified-high": { "model": "zai-coding-plan/glm-5", }, "unspecified-low": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "visual-engineering": { "model": "zai-coding-plan/glm-5", }, "writing": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, }, } @@ -1273,7 +1273,7 @@ exports[`generateModelConfig mixed provider scenarios uses Gemini + Claude combi "variant": "max", }, "multimodal-looker": { - "model": "opencode/glm-4.7-free", + "model": "opencode/gpt-5-nano", }, "oracle": { "model": "google/gemini-3.1-pro-preview", diff --git a/src/cli/config-manager/add-plugin-to-opencode-config.ts b/src/cli/config-manager/add-plugin-to-opencode-config.ts index 53af77a82..90a78f0ae 100644 --- a/src/cli/config-manager/add-plugin-to-opencode-config.ts +++ b/src/cli/config-manager/add-plugin-to-opencode-config.ts @@ -1,5 +1,6 @@ import { readFileSync, writeFileSync } from "node:fs" import type { ConfigMergeResult } from "../types" +import { PLUGIN_NAME, LEGACY_PLUGIN_NAME } from "../../shared" import { getConfigDir } from "./config-context" import { ensureConfigDirectoryExists } from "./ensure-config-directory-exists" import { formatErrorWithSuggestion } from "./format-error-with-suggestion" @@ -7,8 +8,6 @@ import { detectConfigFormat } from "./opencode-config-format" import { parseOpenCodeConfigFileWithError, type OpenCodeConfig } from "./parse-opencode-config-file" import { getPluginNameWithVersion } from "./plugin-name-with-version" -const PACKAGE_NAME = "oh-my-opencode" - export async function addPluginToOpenCodeConfig(currentVersion: string): Promise { try { ensureConfigDirectoryExists() @@ -21,7 +20,7 @@ export async function addPluginToOpenCodeConfig(currentVersion: string): Promise } const { format, path } = detectConfigFormat() - const pluginEntry = await getPluginNameWithVersion(currentVersion, PACKAGE_NAME) + const pluginEntry = await getPluginNameWithVersion(currentVersion, PLUGIN_NAME) try { if (format === "none") { @@ -41,13 +40,24 @@ export async function addPluginToOpenCodeConfig(currentVersion: string): Promise const config = parseResult.config const plugins = config.plugin ?? [] - const existingIndex = plugins.findIndex((plugin) => plugin === PACKAGE_NAME || plugin.startsWith(`${PACKAGE_NAME}@`)) - if (existingIndex !== -1) { - if (plugins[existingIndex] === pluginEntry) { + // Check for existing plugin (either current or legacy name) + const currentNameIndex = plugins.findIndex( + (plugin) => plugin === PLUGIN_NAME || plugin.startsWith(`${PLUGIN_NAME}@`) + ) + const legacyNameIndex = plugins.findIndex( + (plugin) => plugin === LEGACY_PLUGIN_NAME || plugin.startsWith(`${LEGACY_PLUGIN_NAME}@`) + ) + + // If either name exists, update to new name + if (currentNameIndex !== -1) { + if (plugins[currentNameIndex] === pluginEntry) { return { success: true, configPath: path } } - plugins[existingIndex] = pluginEntry + plugins[currentNameIndex] = pluginEntry + } else if (legacyNameIndex !== -1) { + // Upgrade legacy name to new name + plugins[legacyNameIndex] = pluginEntry } else { plugins.push(pluginEntry) } diff --git a/src/cli/config-manager/bun-install.ts b/src/cli/config-manager/bun-install.ts index 32b9d033f..1ef20dc08 100644 --- a/src/cli/config-manager/bun-install.ts +++ b/src/cli/config-manager/bun-install.ts @@ -11,6 +11,8 @@ type BunInstallOutputMode = "inherit" | "pipe" interface RunBunInstallOptions { outputMode?: BunInstallOutputMode + /** Workspace directory to install to. Defaults to cache dir if not provided. */ + workspaceDir?: string } interface BunInstallOutput { @@ -65,7 +67,7 @@ function logCapturedOutputOnFailure(outputMode: BunInstallOutputMode, output: Bu export async function runBunInstallWithDetails(options?: RunBunInstallOptions): Promise { const outputMode = options?.outputMode ?? "pipe" - const cacheDir = getOpenCodeCacheDir() + const cacheDir = options?.workspaceDir ?? getOpenCodeCacheDir() const packageJsonPath = `${cacheDir}/package.json` if (!existsSync(packageJsonPath)) { diff --git a/src/cli/config-manager/detect-current-config.ts b/src/cli/config-manager/detect-current-config.ts index 5900f2777..3679d5bd6 100644 --- a/src/cli/config-manager/detect-current-config.ts +++ b/src/cli/config-manager/detect-current-config.ts @@ -1,5 +1,5 @@ import { existsSync, readFileSync } from "node:fs" -import { parseJsonc } from "../../shared" +import { parseJsonc, LEGACY_PLUGIN_NAME, PLUGIN_NAME } from "../../shared" import type { DetectedConfig } from "../types" import { getOmoConfigPath } from "./config-context" import { detectConfigFormat } from "./opencode-config-format" @@ -55,8 +55,12 @@ function detectProvidersFromOmoConfig(): { } } +function isOurPlugin(plugin: string): boolean { + return plugin === PLUGIN_NAME || plugin.startsWith(`${PLUGIN_NAME}@`) || + plugin === LEGACY_PLUGIN_NAME || plugin.startsWith(`${LEGACY_PLUGIN_NAME}@`) +} + export function detectCurrentConfig(): DetectedConfig { - const PACKAGE_NAME = "oh-my-opencode" const result: DetectedConfig = { isInstalled: false, hasClaude: true, @@ -82,7 +86,7 @@ export function detectCurrentConfig(): DetectedConfig { const openCodeConfig = parseResult.config const plugins = openCodeConfig.plugin ?? [] - result.isInstalled = plugins.some((plugin) => plugin.startsWith(PACKAGE_NAME)) + result.isInstalled = plugins.some(isOurPlugin) if (!result.isInstalled) { return result diff --git a/src/cli/config-manager/plugin-detection.test.ts b/src/cli/config-manager/plugin-detection.test.ts index 6d489b385..2d4d69945 100644 --- a/src/cli/config-manager/plugin-detection.test.ts +++ b/src/cli/config-manager/plugin-detection.test.ts @@ -52,6 +52,30 @@ describe("detectCurrentConfig - single package detection", () => { expect(result.isInstalled).toBe(true) }) + it("detects oh-my-openagent as installed (legacy name)", () => { + // given + const config = { plugin: ["oh-my-openagent"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(true) + }) + + it("detects oh-my-openagent with version pin as installed (legacy name)", () => { + // given + const config = { plugin: ["oh-my-openagent@3.11.0"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(true) + }) + it("returns false when plugin not present", () => { // given const config = { plugin: ["some-other-plugin"] } @@ -64,6 +88,18 @@ describe("detectCurrentConfig - single package detection", () => { expect(result.isInstalled).toBe(false) }) + it("returns false when plugin not present (even with similar name)", () => { + // given - not exactly oh-my-openagent + const config = { plugin: ["oh-my-openagent-extra"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(false) + }) + it("detects OpenCode Go from the existing omo config", () => { // given writeFileSync(testConfigPath, JSON.stringify({ plugin: ["oh-my-opencode"] }, null, 2) + "\n", "utf-8") @@ -130,6 +166,38 @@ describe("addPluginToOpenCodeConfig - single package writes", () => { expect(savedConfig.plugin).not.toContain("oh-my-opencode@3.10.0") }) + it("recognizes oh-my-openagent as already installed (legacy name)", async () => { + // given + const config = { plugin: ["oh-my-openagent"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = await addPluginToOpenCodeConfig("3.11.0") + + // then + expect(result.success).toBe(true) + const savedConfig = JSON.parse(readFileSync(testConfigPath, "utf-8")) + // Should upgrade to new name + expect(savedConfig.plugin).toContain("oh-my-opencode") + expect(savedConfig.plugin).not.toContain("oh-my-openagent") + }) + + it("replaces version-pinned oh-my-openagent@X.Y.Z with new name", async () => { + // given + const config = { plugin: ["oh-my-openagent@3.10.0"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = await addPluginToOpenCodeConfig("3.11.0") + + // then + expect(result.success).toBe(true) + const savedConfig = JSON.parse(readFileSync(testConfigPath, "utf-8")) + // Legacy should be replaced with new name + expect(savedConfig.plugin).toContain("oh-my-opencode") + expect(savedConfig.plugin).not.toContain("oh-my-openagent") + }) + it("adds new plugin when none exists", async () => { // given const config = {} diff --git a/src/cli/doctor/checks/system-plugin.ts b/src/cli/doctor/checks/system-plugin.ts index cd4969247..6abe089a5 100644 --- a/src/cli/doctor/checks/system-plugin.ts +++ b/src/cli/doctor/checks/system-plugin.ts @@ -1,7 +1,6 @@ import { existsSync, readFileSync } from "node:fs" -import { PACKAGE_NAME } from "../constants" -import { getOpenCodeConfigPaths, parseJsonc } from "../../../shared" +import { LEGACY_PLUGIN_NAME, PLUGIN_NAME, getOpenCodeConfigPaths, parseJsonc } from "../../../shared" export interface PluginInfo { registered: boolean @@ -24,18 +23,33 @@ function detectConfigPath(): string | null { } function parsePluginVersion(entry: string): string | null { - if (!entry.startsWith(`${PACKAGE_NAME}@`)) return null - const value = entry.slice(PACKAGE_NAME.length + 1) - if (!value || value === "latest") return null - return value + // Check for current package name + if (entry.startsWith(`${PLUGIN_NAME}@`)) { + const value = entry.slice(PLUGIN_NAME.length + 1) + if (!value || value === "latest") return null + return value + } + // Check for legacy package name + if (entry.startsWith(`${LEGACY_PLUGIN_NAME}@`)) { + const value = entry.slice(LEGACY_PLUGIN_NAME.length + 1) + if (!value || value === "latest") return null + return value + } + return null } function findPluginEntry(entries: string[]): { entry: string; isLocalDev: boolean } | null { for (const entry of entries) { - if (entry === PACKAGE_NAME || entry.startsWith(`${PACKAGE_NAME}@`)) { + // Check for current package name + if (entry === PLUGIN_NAME || entry.startsWith(`${PLUGIN_NAME}@`)) { return { entry, isLocalDev: false } } - if (entry.startsWith("file://") && entry.includes(PACKAGE_NAME)) { + // Check for legacy package name + if (entry === LEGACY_PLUGIN_NAME || entry.startsWith(`${LEGACY_PLUGIN_NAME}@`)) { + return { entry, isLocalDev: false } + } + // Check for file:// paths that include either name + if (entry.startsWith("file://") && (entry.includes(PLUGIN_NAME) || entry.includes(LEGACY_PLUGIN_NAME))) { return { entry, isLocalDev: true } } } @@ -76,7 +90,7 @@ export function getPluginInfo(): PluginInfo { registered: true, configPath, entry: pluginEntry.entry, - isPinned: pinnedVersion !== null && /^\d+\.\d+\.\d+/.test(pinnedVersion), + isPinned: pinnedVersion !== null && /^\d+\.\d+\.\d+/.test(pinnedVersion ?? ""), pinnedVersion, isLocalDev: pluginEntry.isLocalDev, } diff --git a/src/cli/run/runner.test.ts b/src/cli/run/runner.test.ts index cc1b29016..fa5d80b51 100644 --- a/src/cli/run/runner.test.ts +++ b/src/cli/run/runner.test.ts @@ -1,6 +1,6 @@ /// -import { describe, it, expect } from "bun:test" +import { describe, it, expect, beforeEach, afterEach, vi } from "bun:test" import type { OhMyOpenCodeConfig } from "../../config" import { resolveRunAgent, waitForEventProcessorShutdown } from "./runner" @@ -83,7 +83,6 @@ describe("resolveRunAgent", () => { }) describe("waitForEventProcessorShutdown", () => { - it("returns quickly when event processor completes", async () => { //#given const eventProcessor = new Promise((resolve) => { @@ -115,3 +114,44 @@ describe("waitForEventProcessorShutdown", () => { expect(elapsed).toBeGreaterThanOrEqual(timeoutMs - 10) }) }) + +describe("run with invalid model", () => { + it("given invalid --model value, when run, then returns exit code 1 with error message", async () => { + // given + const originalExit = process.exit + const originalError = console.error + const errorMessages: string[] = [] + const exitCodes: number[] = [] + + console.error = (...args: unknown[]) => { + errorMessages.push(args.map(String).join(" ")) + } + process.exit = ((code?: number) => { + exitCodes.push(code ?? 0) + throw new Error("exit") + }) as typeof process.exit + + try { + // when + // Note: This will actually try to run - but the issue is that resolveRunModel + // is called BEFORE the try block, so it throws an unhandled exception + // We're testing the runner's error handling + const { run } = await import("./runner") + + // This will throw because model "invalid" is invalid format + try { + await run({ + message: "test", + model: "invalid", + }) + } catch { + // Expected to potentially throw due to unhandled model resolution error + } + } finally { + // then - verify error handling + // Currently this will fail because the error is not caught properly + console.error = originalError + process.exit = originalExit + } + }) +}) diff --git a/src/cli/run/runner.ts b/src/cli/run/runner.ts index 84dd22a42..0730204a8 100644 --- a/src/cli/run/runner.ts +++ b/src/cli/run/runner.ts @@ -47,10 +47,11 @@ export async function run(options: RunOptions): Promise { const pluginConfig = loadPluginConfig(directory, { command: "run" }) const resolvedAgent = resolveRunAgent(options, pluginConfig) - const resolvedModel = resolveRunModel(options.model) const abortController = new AbortController() try { + const resolvedModel = resolveRunModel(options.model) + const { client, cleanup: serverCleanup } = await createServerConnection({ port: options.port, attach: options.attach, diff --git a/src/features/background-agent/constants.ts b/src/features/background-agent/constants.ts index bfd4b7ee2..0fbde2964 100644 --- a/src/features/background-agent/constants.ts +++ b/src/features/background-agent/constants.ts @@ -2,6 +2,7 @@ import type { PluginInput } from "@opencode-ai/plugin" import type { BackgroundTask, LaunchInput } from "./types" export const TASK_TTL_MS = 30 * 60 * 1000 +export const TERMINAL_TASK_TTL_MS = 30 * 60 * 1000 export const MIN_STABILITY_TIME_MS = 10 * 1000 export const DEFAULT_STALE_TIMEOUT_MS = 180_000 export const DEFAULT_MESSAGE_STALENESS_TIMEOUT_MS = 1_800_000 diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index 39e2dc420..66a0966a7 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -27,6 +27,7 @@ import { import { POLLING_INTERVAL_MS, TASK_CLEANUP_DELAY_MS, + TASK_TTL_MS, } from "./constants" import { subagentSessions } from "../claude-code-session-state" @@ -100,6 +101,8 @@ export interface SubagentSessionCreatedEvent { export type OnSubagentSessionCreated = (event: SubagentSessionCreatedEvent) => Promise +const MAX_TASK_REMOVAL_RESCHEDULES = 6 + export class BackgroundManager { @@ -1203,7 +1206,7 @@ export class BackgroundManager { this.completedTaskSummaries.delete(parentSessionID) } - private scheduleTaskRemoval(taskId: string): void { + private scheduleTaskRemoval(taskId: string, rescheduleCount = 0): void { const existingTimer = this.completionTimers.get(taskId) if (existingTimer) { clearTimeout(existingTimer) @@ -1213,17 +1216,29 @@ export class BackgroundManager { const timer = setTimeout(() => { this.completionTimers.delete(taskId) const task = this.tasks.get(taskId) - if (task) { - this.clearNotificationsForTask(taskId) - this.tasks.delete(taskId) - this.clearTaskHistoryWhenParentTasksGone(task.parentSessionID) - if (task.sessionID) { - subagentSessions.delete(task.sessionID) - SessionCategoryRegistry.remove(task.sessionID) + if (!task) return + + if (task.parentSessionID) { + const siblings = this.getTasksByParentSession(task.parentSessionID) + const runningOrPendingSiblings = siblings.filter( + sibling => sibling.id !== taskId && (sibling.status === "running" || sibling.status === "pending"), + ) + const completedAtTimestamp = task.completedAt?.getTime() + const reachedTaskTtl = completedAtTimestamp !== undefined && (Date.now() - completedAtTimestamp) >= TASK_TTL_MS + if (runningOrPendingSiblings.length > 0 && rescheduleCount < MAX_TASK_REMOVAL_RESCHEDULES && !reachedTaskTtl) { + this.scheduleTaskRemoval(taskId, rescheduleCount + 1) + return } - log("[background-agent] Removed completed task from memory:", taskId) - this.clearTaskHistoryWhenParentTasksGone(task?.parentSessionID) } + + this.clearNotificationsForTask(taskId) + this.tasks.delete(taskId) + this.clearTaskHistoryWhenParentTasksGone(task.parentSessionID) + if (task.sessionID) { + subagentSessions.delete(task.sessionID) + SessionCategoryRegistry.remove(task.sessionID) + } + log("[background-agent] Removed completed task from memory:", taskId) }, TASK_CLEANUP_DELAY_MS) this.completionTimers.set(taskId, timer) diff --git a/src/features/background-agent/task-completion-cleanup.test.ts b/src/features/background-agent/task-completion-cleanup.test.ts index 77c42d6f1..419faf296 100644 --- a/src/features/background-agent/task-completion-cleanup.test.ts +++ b/src/features/background-agent/task-completion-cleanup.test.ts @@ -1,6 +1,5 @@ -declare const require: (name: string) => any -const { describe, test, expect, afterEach } = require("bun:test") import { tmpdir } from "node:os" +import { afterEach, describe, expect, test } from "bun:test" import type { PluginInput } from "@opencode-ai/plugin" import { TASK_CLEANUP_DELAY_MS } from "./constants" import { BackgroundManager } from "./manager" @@ -157,17 +156,19 @@ function getRequiredTimer(manager: BackgroundManager, taskID: string): ReturnTyp } describe("BackgroundManager.notifyParentSession cleanup scheduling", () => { - describe("#given 2 tasks for same parent and task A completed", () => { - test("#when task B is still running #then task A is cleaned up from this.tasks after delay even though task B is not done", async () => { + describe("#given 3 tasks for same parent and task A completed first", () => { + test("#when siblings are still running or pending #then task A remains until siblings also complete", async () => { // given const { manager } = createManager(false) managerUnderTest = manager fakeTimers = installFakeTimers() - const taskA = createTask({ id: "task-a", parentSessionID: "parent-1", description: "task A", status: "completed", completedAt: new Date("2026-03-11T00:01:00.000Z") }) + const taskA = createTask({ id: "task-a", parentSessionID: "parent-1", description: "task A", status: "completed", completedAt: new Date() }) const taskB = createTask({ id: "task-b", parentSessionID: "parent-1", description: "task B", status: "running" }) + const taskC = createTask({ id: "task-c", parentSessionID: "parent-1", description: "task C", status: "pending" }) getTasks(manager).set(taskA.id, taskA) getTasks(manager).set(taskB.id, taskB) - getPendingByParent(manager).set(taskA.parentSessionID, new Set([taskA.id, taskB.id])) + getTasks(manager).set(taskC.id, taskC) + getPendingByParent(manager).set(taskA.parentSessionID, new Set([taskA.id, taskB.id, taskC.id])) // when await notifyParentSessionForTest(manager, taskA) @@ -177,8 +178,23 @@ describe("BackgroundManager.notifyParentSession cleanup scheduling", () => { // then expect(fakeTimers.getDelay(taskATimer)).toBeUndefined() - expect(getTasks(manager).has(taskA.id)).toBe(false) + expect(getTasks(manager).has(taskA.id)).toBe(true) expect(getTasks(manager).get(taskB.id)).toBe(taskB) + expect(getTasks(manager).get(taskC.id)).toBe(taskC) + + // when + taskB.status = "completed" + taskB.completedAt = new Date() + taskC.status = "completed" + taskC.completedAt = new Date() + await notifyParentSessionForTest(manager, taskB) + await notifyParentSessionForTest(manager, taskC) + const rescheduledTaskATimer = getRequiredTimer(manager, taskA.id) + expect(fakeTimers.getDelay(rescheduledTaskATimer)).toBe(TASK_CLEANUP_DELAY_MS) + fakeTimers.run(rescheduledTaskATimer) + + // then + expect(getTasks(manager).has(taskA.id)).toBe(false) }) }) diff --git a/src/features/background-agent/task-poller.ts b/src/features/background-agent/task-poller.ts index a62d3b442..9b17e6260 100644 --- a/src/features/background-agent/task-poller.ts +++ b/src/features/background-agent/task-poller.ts @@ -9,12 +9,11 @@ import { DEFAULT_MESSAGE_STALENESS_TIMEOUT_MS, DEFAULT_STALE_TIMEOUT_MS, MIN_RUNTIME_BEFORE_STALE_MS, + TERMINAL_TASK_TTL_MS, TASK_TTL_MS, } from "./constants" import { removeTaskToastTracking } from "./remove-task-toast-tracking" -const TERMINAL_TASK_TTL_MS = 30 * 60 * 1000 - const TERMINAL_TASK_STATUSES = new Set([ "completed", "error", diff --git a/src/features/boulder-state/storage.ts b/src/features/boulder-state/storage.ts index ab84368b7..c9ac83993 100644 --- a/src/features/boulder-state/storage.ts +++ b/src/features/boulder-state/storage.ts @@ -59,10 +59,13 @@ export function appendSessionId(directory: string, sessionId: string): BoulderSt if (!Array.isArray(state.session_ids)) { state.session_ids = [] } + const originalSessionIds = [...state.session_ids] state.session_ids.push(sessionId) if (writeBoulderState(directory, state)) { return state } + state.session_ids = originalSessionIds + return null } return state diff --git a/src/features/opencode-skill-loader/git-master-template-injection.test.ts b/src/features/opencode-skill-loader/git-master-template-injection.test.ts index 60ea0f0b3..bbe38645e 100644 --- a/src/features/opencode-skill-loader/git-master-template-injection.test.ts +++ b/src/features/opencode-skill-loader/git-master-template-injection.test.ts @@ -153,3 +153,25 @@ describe("#given git_env_prefix with commit footer", () => { }) }) }) + +describe("#given idempotency of prefixGitCommandsInBashCodeBlocks", () => { + describe("#when git_env_prefix is provided and template already has prefixed commands in env prefix section", () => { + it("#then does NOT double-prefix the already-prefixed commands", () => { + const result = injectGitMasterConfig(SAMPLE_TEMPLATE, { + commit_footer: false, + include_co_authored_by: false, + git_env_prefix: "GIT_MASTER=1", + }) + + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git status") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git add") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git commit") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git push") + + expect(result).toContain("GIT_MASTER=1 git status") + expect(result).toContain("GIT_MASTER=1 git add") + expect(result).toContain("GIT_MASTER=1 git commit") + expect(result).toContain("GIT_MASTER=1 git push") + }) + }) +}) diff --git a/src/features/opencode-skill-loader/git-master-template-injection.ts b/src/features/opencode-skill-loader/git-master-template-injection.ts index 3b8e9630a..fc1ba3e4a 100644 --- a/src/features/opencode-skill-loader/git-master-template-injection.ts +++ b/src/features/opencode-skill-loader/git-master-template-injection.ts @@ -72,8 +72,16 @@ function prefixGitCommandsInBashCodeBlocks(template: string, prefix: string): st function prefixGitCommandsInCodeBlock(codeBlock: string, prefix: string): string { return codeBlock - .replace(LEADING_GIT_COMMAND_PATTERN, `$1${prefix} git`) - .replace(INLINE_GIT_COMMAND_PATTERN, `$1${prefix} git`) + .split("\n") + .map((line) => { + if (line.includes(prefix)) { + return line + } + return line + .replace(LEADING_GIT_COMMAND_PATTERN, `$1${prefix} git`) + .replace(INLINE_GIT_COMMAND_PATTERN, `$1${prefix} git`) + }) + .join("\n") } function buildCommitFooterInjection( diff --git a/src/features/skill-mcp-manager/env-cleaner.test.ts b/src/features/skill-mcp-manager/env-cleaner.test.ts index 08da63388..75cfe348e 100644 --- a/src/features/skill-mcp-manager/env-cleaner.test.ts +++ b/src/features/skill-mcp-manager/env-cleaner.test.ts @@ -199,3 +199,236 @@ describe("EXCLUDED_ENV_PATTERNS", () => { } }) }) +describe("secret env var filtering", () => { + it("filters out ANTHROPIC_API_KEY", () => { + // given + process.env.ANTHROPIC_API_KEY = "sk-ant-api03-secret" + process.env.PATH = "/usr/bin" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.ANTHROPIC_API_KEY).toBeUndefined() + expect(cleanEnv.PATH).toBe("/usr/bin") + }) + + it("filters out AWS_SECRET_ACCESS_KEY", () => { + // given + process.env.AWS_SECRET_ACCESS_KEY = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + process.env.AWS_ACCESS_KEY_ID = "AKIAIOSFODNN7EXAMPLE" + process.env.HOME = "/home/user" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.AWS_SECRET_ACCESS_KEY).toBeUndefined() + expect(cleanEnv.AWS_ACCESS_KEY_ID).toBeUndefined() + expect(cleanEnv.HOME).toBe("/home/user") + }) + + it("filters out GITHUB_TOKEN", () => { + // given + process.env.GITHUB_TOKEN = "ghp_secrettoken123456789" + process.env.GITHUB_API_TOKEN = "another_secret_token" + process.env.SHELL = "/bin/bash" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.GITHUB_TOKEN).toBeUndefined() + expect(cleanEnv.GITHUB_API_TOKEN).toBeUndefined() + expect(cleanEnv.SHELL).toBe("/bin/bash") + }) + + it("filters out OPENAI_API_KEY", () => { + // given + process.env.OPENAI_API_KEY = "sk-secret123456789" + process.env.LANG = "en_US.UTF-8" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.OPENAI_API_KEY).toBeUndefined() + expect(cleanEnv.LANG).toBe("en_US.UTF-8") + }) + + it("filters out DATABASE_URL with credentials", () => { + // given + process.env.DATABASE_URL = "postgresql://user:password@localhost:5432/db" + process.env.DB_PASSWORD = "supersecretpassword" + process.env.TERM = "xterm-256color" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.DATABASE_URL).toBeUndefined() + expect(cleanEnv.DB_PASSWORD).toBeUndefined() + expect(cleanEnv.TERM).toBe("xterm-256color") + }) +}) + +describe("suffix-based secret filtering", () => { + it("filters variables ending with _KEY", () => { + // given + process.env.MY_API_KEY = "secret-value" + process.env.SOME_KEY = "another-secret" + process.env.TMPDIR = "/tmp" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.MY_API_KEY).toBeUndefined() + expect(cleanEnv.SOME_KEY).toBeUndefined() + expect(cleanEnv.TMPDIR).toBe("/tmp") + }) + + it("filters variables ending with _SECRET", () => { + // given + process.env.AWS_SECRET = "secret-value" + process.env.JWT_SECRET = "jwt-secret-token" + process.env.USER = "testuser" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.AWS_SECRET).toBeUndefined() + expect(cleanEnv.JWT_SECRET).toBeUndefined() + expect(cleanEnv.USER).toBe("testuser") + }) + + it("filters variables ending with _TOKEN", () => { + // given + process.env.ACCESS_TOKEN = "token-value" + process.env.BEARER_TOKEN = "bearer-token" + process.env.HOME = "/home/user" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.ACCESS_TOKEN).toBeUndefined() + expect(cleanEnv.BEARER_TOKEN).toBeUndefined() + expect(cleanEnv.HOME).toBe("/home/user") + }) + + it("filters variables ending with _PASSWORD", () => { + // given + process.env.DB_PASSWORD = "db-password" + process.env.APP_PASSWORD = "app-secret" + process.env.NODE_ENV = "production" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.DB_PASSWORD).toBeUndefined() + expect(cleanEnv.APP_PASSWORD).toBeUndefined() + expect(cleanEnv.NODE_ENV).toBe("production") + }) + + it("filters variables ending with _CREDENTIAL", () => { + // given + process.env.GCP_CREDENTIAL = "json-credential" + process.env.AZURE_CREDENTIAL = "azure-creds" + process.env.PWD = "/current/dir" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.GCP_CREDENTIAL).toBeUndefined() + expect(cleanEnv.AZURE_CREDENTIAL).toBeUndefined() + expect(cleanEnv.PWD).toBe("/current/dir") + }) + + it("filters variables ending with _API_KEY", () => { + // given + // given + process.env.STRIPE_API_KEY = "sk_live_secret" + process.env.SENDGRID_API_KEY = "SG.secret" + process.env.SHELL = "/bin/zsh" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.STRIPE_API_KEY).toBeUndefined() + expect(cleanEnv.SENDGRID_API_KEY).toBeUndefined() + expect(cleanEnv.SHELL).toBe("/bin/zsh") + }) +}) + +describe("safe environment variables preserved", () => { + it("preserves PATH", () => { + // given + process.env.PATH = "/usr/bin:/usr/local/bin" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.PATH).toBe("/usr/bin:/usr/local/bin") + }) + + it("preserves HOME", () => { + // given + process.env.HOME = "/home/testuser" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.HOME).toBe("/home/testuser") + }) + + it("preserves SHELL", () => { + // given + process.env.SHELL = "/bin/bash" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.SHELL).toBe("/bin/bash") + }) + + it("preserves LANG", () => { + // given + process.env.LANG = "en_US.UTF-8" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.LANG).toBe("en_US.UTF-8") + }) + + it("preserves TERM", () => { + // given + process.env.TERM = "xterm-256color" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.TERM).toBe("xterm-256color") + }) + + it("preserves TMPDIR", () => { + // given + process.env.TMPDIR = "/tmp" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.TMPDIR).toBe("/tmp") +}) +}) diff --git a/src/features/skill-mcp-manager/env-cleaner.ts b/src/features/skill-mcp-manager/env-cleaner.ts index 9a3faba79..9c6ebe1aa 100644 --- a/src/features/skill-mcp-manager/env-cleaner.ts +++ b/src/features/skill-mcp-manager/env-cleaner.ts @@ -1,10 +1,28 @@ // Filters npm/pnpm/yarn config env vars that break MCP servers in pnpm projects (#456) +// Also filters secret-containing env vars to prevent exposure to malicious stdio MCP servers (#B-02) export const EXCLUDED_ENV_PATTERNS: RegExp[] = [ + // npm/pnpm/yarn config patterns (original) /^NPM_CONFIG_/i, /^npm_config_/, /^YARN_/, /^PNPM_/, /^NO_UPDATE_NOTIFIER$/, + + // Specific high-risk secret env vars (explicit blocks) + /^ANTHROPIC_API_KEY$/i, + /^AWS_ACCESS_KEY_ID$/i, + /^AWS_SECRET_ACCESS_KEY$/i, + /^GITHUB_TOKEN$/i, + /^DATABASE_URL$/i, + /^OPENAI_API_KEY$/i, + + // Suffix-based patterns for common secret naming conventions + /_KEY$/i, + /_SECRET$/i, + /_TOKEN$/i, + /_PASSWORD$/i, + /_CREDENTIAL$/i, + /_API_KEY$/i, ] export function createCleanMcpEnvironment( diff --git a/src/hooks/auto-update-checker/hook/background-update-check.ts b/src/hooks/auto-update-checker/hook/background-update-check.ts index 0875c23a5..d2cc97dba 100644 --- a/src/hooks/auto-update-checker/hook/background-update-check.ts +++ b/src/hooks/auto-update-checker/hook/background-update-check.ts @@ -1,6 +1,9 @@ import type { PluginInput } from "@opencode-ai/plugin" +import { existsSync } from "node:fs" +import { join } from "node:path" import { runBunInstallWithDetails } from "../../../cli/config-manager" import { log } from "../../../shared/logger" +import { getOpenCodeCacheDir, getOpenCodeConfigPaths } from "../../../shared" import { invalidatePackage } from "../cache" import { PACKAGE_NAME } from "../constants" import { extractChannel } from "../version-channel" @@ -11,9 +14,36 @@ function getPinnedVersionToastMessage(latestVersion: string): string { return `Update available: ${latestVersion} (version pinned, update manually)` } -async function runBunInstallSafe(): Promise { +/** + * Resolves the active install workspace. + * Same logic as doctor check: prefer config-dir if installed, fall back to cache-dir. + */ +function resolveActiveInstallWorkspace(): string { + const configPaths = getOpenCodeConfigPaths({ binary: "opencode" }) + const cacheDir = getOpenCodeCacheDir() + + const configInstallPath = join(configPaths.configDir, "node_modules", PACKAGE_NAME, "package.json") + const cacheInstallPath = join(cacheDir, "node_modules", PACKAGE_NAME, "package.json") + + // Prefer config-dir if installed there, otherwise fall back to cache-dir + if (existsSync(configInstallPath)) { + log(`[auto-update-checker] Active workspace: config-dir (${configPaths.configDir})`) + return configPaths.configDir + } + + if (existsSync(cacheInstallPath)) { + log(`[auto-update-checker] Active workspace: cache-dir (${cacheDir})`) + return cacheDir + } + + // Default to config-dir if neither exists (matches doctor behavior) + log(`[auto-update-checker] Active workspace: config-dir (default, no install detected)`) + return configPaths.configDir +} + +async function runBunInstallSafe(workspaceDir: string): Promise { try { - const result = await runBunInstallWithDetails({ outputMode: "pipe" }) + const result = await runBunInstallWithDetails({ outputMode: "pipe", workspaceDir }) if (!result.success && result.error) { log("[auto-update-checker] bun install error:", result.error) } @@ -82,7 +112,8 @@ export async function runBackgroundUpdateCheck( invalidatePackage(PACKAGE_NAME) - const installSuccess = await runBunInstallSafe() + const activeWorkspace = resolveActiveInstallWorkspace() + const installSuccess = await runBunInstallSafe(activeWorkspace) if (installSuccess) { await showAutoUpdatedToast(ctx, currentVersion, latestVersion) diff --git a/src/hooks/auto-update-checker/hook/workspace-resolution.test.ts b/src/hooks/auto-update-checker/hook/workspace-resolution.test.ts new file mode 100644 index 000000000..79f374bd8 --- /dev/null +++ b/src/hooks/auto-update-checker/hook/workspace-resolution.test.ts @@ -0,0 +1,223 @@ +import type { PluginInput } from "@opencode-ai/plugin" +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test" +import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs" +import { join } from "node:path" + +type PluginEntry = { + entry: string + isPinned: boolean + pinnedVersion: string | null + configPath: string +} + +type ToastMessageGetter = (isUpdate: boolean, version?: string) => string + +function createPluginEntry(overrides?: Partial): PluginEntry { + return { + entry: "oh-my-opencode@3.4.0", + isPinned: false, + pinnedVersion: null, + configPath: "/test/opencode.json", + ...overrides, + } +} + +const TEST_DIR = join(import.meta.dir, "__test-workspace-resolution__") +const TEST_CACHE_DIR = join(TEST_DIR, "cache") +const TEST_CONFIG_DIR = join(TEST_DIR, "config") + +const mockFindPluginEntry = mock((_directory: string): PluginEntry | null => createPluginEntry()) +const mockGetCachedVersion = mock((): string | null => "3.4.0") +const mockGetLatestVersion = mock(async (): Promise => "3.5.0") +const mockExtractChannel = mock(() => "latest") +const mockInvalidatePackage = mock(() => {}) +const mockShowUpdateAvailableToast = mock( + async (_ctx: PluginInput, _latestVersion: string, _getToastMessage: ToastMessageGetter): Promise => {} +) +const mockShowAutoUpdatedToast = mock( + async (_ctx: PluginInput, _fromVersion: string, _toVersion: string): Promise => {} +) +const mockSyncCachePackageJsonToIntent = mock(() => ({ synced: true, error: null })) + +const mockRunBunInstallWithDetails = mock( + async (opts?: { outputMode?: string; workspaceDir?: string }) => { + return { success: true } + } +) + +mock.module("../checker", () => ({ + findPluginEntry: mockFindPluginEntry, + getCachedVersion: mockGetCachedVersion, + getLatestVersion: mockGetLatestVersion, + revertPinnedVersion: mock(() => false), + syncCachePackageJsonToIntent: mockSyncCachePackageJsonToIntent, +})) +mock.module("../version-channel", () => ({ extractChannel: mockExtractChannel })) +mock.module("../cache", () => ({ invalidatePackage: mockInvalidatePackage })) +mock.module("../../../cli/config-manager", () => ({ + runBunInstallWithDetails: mockRunBunInstallWithDetails, +})) +mock.module("./update-toasts", () => ({ + showUpdateAvailableToast: mockShowUpdateAvailableToast, + showAutoUpdatedToast: mockShowAutoUpdatedToast, +})) +mock.module("../../../shared/logger", () => ({ log: () => {} })) +mock.module("../../../shared", () => ({ + getOpenCodeCacheDir: () => TEST_CACHE_DIR, + getOpenCodeConfigPaths: () => ({ + configDir: TEST_CONFIG_DIR, + configJson: join(TEST_CONFIG_DIR, "opencode.json"), + configJsonc: join(TEST_CONFIG_DIR, "opencode.jsonc"), + packageJson: join(TEST_CONFIG_DIR, "package.json"), + omoConfig: join(TEST_CONFIG_DIR, "oh-my-opencode.json"), + }), + getOpenCodeConfigDir: () => TEST_CONFIG_DIR, +})) + +// Mock constants BEFORE importing the module +const ORIGINAL_PACKAGE_NAME = "oh-my-opencode" +mock.module("../constants", () => ({ + PACKAGE_NAME: ORIGINAL_PACKAGE_NAME, + CACHE_DIR: TEST_CACHE_DIR, + USER_CONFIG_DIR: TEST_CONFIG_DIR, +})) + +// Need to mock getOpenCodeCacheDir and getOpenCodeConfigPaths before importing the module +mock.module("../../../shared/data-path", () => ({ + getDataDir: () => join(TEST_DIR, "data"), + getOpenCodeStorageDir: () => join(TEST_DIR, "data", "opencode", "storage"), + getCacheDir: () => TEST_DIR, + getOmoOpenCodeCacheDir: () => join(TEST_DIR, "oh-my-opencode"), + getOpenCodeCacheDir: () => TEST_CACHE_DIR, +})) +mock.module("../../../shared/opencode-config-dir", () => ({ + getOpenCodeConfigDir: () => TEST_CONFIG_DIR, + getOpenCodeConfigPaths: () => ({ + configDir: TEST_CONFIG_DIR, + configJson: join(TEST_CONFIG_DIR, "opencode.json"), + configJsonc: join(TEST_CONFIG_DIR, "opencode.jsonc"), + packageJson: join(TEST_CONFIG_DIR, "package.json"), + omoConfig: join(TEST_CONFIG_DIR, "oh-my-opencode.json"), + }), +})) + +const modulePath = "./background-update-check?test" +const { runBackgroundUpdateCheck } = await import(modulePath) + +describe("workspace resolution", () => { + const mockCtx = { directory: "/test" } as PluginInput + const getToastMessage: ToastMessageGetter = (isUpdate, version) => + isUpdate ? `Update to ${version}` : "Up to date" + + beforeEach(() => { + // Setup test directories + if (existsSync(TEST_DIR)) { + rmSync(TEST_DIR, { recursive: true, force: true }) + } + mkdirSync(TEST_DIR, { recursive: true }) + + mockFindPluginEntry.mockReset() + mockGetCachedVersion.mockReset() + mockGetLatestVersion.mockReset() + mockExtractChannel.mockReset() + mockInvalidatePackage.mockReset() + mockRunBunInstallWithDetails.mockReset() + mockShowUpdateAvailableToast.mockReset() + mockShowAutoUpdatedToast.mockReset() + + mockFindPluginEntry.mockReturnValue(createPluginEntry()) + mockGetCachedVersion.mockReturnValue("3.4.0") + mockGetLatestVersion.mockResolvedValue("3.5.0") + mockExtractChannel.mockReturnValue("latest") + // Note: Don't use mockResolvedValue here - it overrides the function that captures args + mockSyncCachePackageJsonToIntent.mockReturnValue({ synced: true, error: null }) + }) + + afterEach(() => { + if (existsSync(TEST_DIR)) { + rmSync(TEST_DIR, { recursive: true, force: true }) + } + }) + + describe("#given config-dir install exists but cache-dir does not", () => { + it("installs to config-dir, not cache-dir", async () => { + //#given - config-dir has installation, cache-dir does not + mkdirSync(join(TEST_CONFIG_DIR, "node_modules", "oh-my-opencode"), { recursive: true }) + writeFileSync( + join(TEST_CONFIG_DIR, "package.json"), + JSON.stringify({ dependencies: { "oh-my-opencode": "3.4.0" } }, null, 2) + ) + writeFileSync( + join(TEST_CONFIG_DIR, "node_modules", "oh-my-opencode", "package.json"), + JSON.stringify({ name: "oh-my-opencode", version: "3.4.0" }, null, 2) + ) + + // cache-dir should NOT exist + expect(existsSync(TEST_CACHE_DIR)).toBe(false) + + //#when + await runBackgroundUpdateCheck(mockCtx, true, getToastMessage) + + //#then - install should be called with config-dir + const mockCalls = mockRunBunInstallWithDetails.mock.calls + expect(mockCalls[0][0]?.workspaceDir).toBe(TEST_CONFIG_DIR) + }) + }) + + describe("#given both config-dir and cache-dir exist", () => { + it("prefers config-dir over cache-dir", async () => { + //#given - both directories have installations + mkdirSync(join(TEST_CONFIG_DIR, "node_modules", "oh-my-opencode"), { recursive: true }) + writeFileSync( + join(TEST_CONFIG_DIR, "package.json"), + JSON.stringify({ dependencies: { "oh-my-opencode": "3.4.0" } }, null, 2) + ) + writeFileSync( + join(TEST_CONFIG_DIR, "node_modules", "oh-my-opencode", "package.json"), + JSON.stringify({ name: "oh-my-opencode", version: "3.4.0" }, null, 2) + ) + + mkdirSync(join(TEST_CACHE_DIR, "node_modules", "oh-my-opencode"), { recursive: true }) + writeFileSync( + join(TEST_CACHE_DIR, "package.json"), + JSON.stringify({ dependencies: { "oh-my-opencode": "3.4.0" } }, null, 2) + ) + writeFileSync( + join(TEST_CACHE_DIR, "node_modules", "oh-my-opencode", "package.json"), + JSON.stringify({ name: "oh-my-opencode", version: "3.4.0" }, null, 2) + ) + + //#when + await runBackgroundUpdateCheck(mockCtx, true, getToastMessage) + + //#then - install should prefer config-dir + const mockCalls2 = mockRunBunInstallWithDetails.mock.calls + expect(mockCalls2[0][0]?.workspaceDir).toBe(TEST_CONFIG_DIR) + }) + }) + + describe("#given only cache-dir install exists", () => { + it("falls back to cache-dir", async () => { + //#given - only cache-dir has installation + mkdirSync(join(TEST_CACHE_DIR, "node_modules", "oh-my-opencode"), { recursive: true }) + writeFileSync( + join(TEST_CACHE_DIR, "package.json"), + JSON.stringify({ dependencies: { "oh-my-opencode": "3.4.0" } }, null, 2) + ) + writeFileSync( + join(TEST_CACHE_DIR, "node_modules", "oh-my-opencode", "package.json"), + JSON.stringify({ name: "oh-my-opencode", version: "3.4.0" }, null, 2) + ) + + // config-dir should NOT exist + expect(existsSync(TEST_CONFIG_DIR)).toBe(false) + + //#when + await runBackgroundUpdateCheck(mockCtx, true, getToastMessage) + + //#then - install should fall back to cache-dir + const mockCalls3 = mockRunBunInstallWithDetails.mock.calls + expect(mockCalls3[0][0]?.workspaceDir).toBe(TEST_CACHE_DIR) + }) + }) +}) diff --git a/src/hooks/todo-continuation-enforcer/session-state.regression.test.ts b/src/hooks/todo-continuation-enforcer/session-state.regression.test.ts index 08cd5fb9a..f7e26e431 100644 --- a/src/hooks/todo-continuation-enforcer/session-state.regression.test.ts +++ b/src/hooks/todo-continuation-enforcer/session-state.regression.test.ts @@ -18,7 +18,7 @@ describe("createSessionStateStore regressions", () => { describe("#given external activity happens after a successful continuation", () => { describe("#when todos stay unchanged", () => { - test("#then it treats the activity as progress instead of stagnation", () => { + test("#then it keeps counting stagnation", () => { const sessionID = "ses-activity-progress" const todos = [ { id: "1", content: "Task 1", status: "pending", priority: "high" }, @@ -37,9 +37,9 @@ describe("createSessionStateStore regressions", () => { trackedState.abortDetectedAt = undefined const progressUpdate = sessionStateStore.trackContinuationProgress(sessionID, 2, todos) - expect(progressUpdate.hasProgressed).toBe(true) - expect(progressUpdate.progressSource).toBe("activity") - expect(progressUpdate.stagnationCount).toBe(0) + expect(progressUpdate.hasProgressed).toBe(false) + expect(progressUpdate.progressSource).toBe("none") + expect(progressUpdate.stagnationCount).toBe(1) }) }) }) @@ -72,7 +72,7 @@ describe("createSessionStateStore regressions", () => { describe("#given stagnation already halted a session", () => { describe("#when new activity appears before the next idle check", () => { - test("#then it resets the stop condition on the next progress check", () => { + test("#then it does not reset the stop condition", () => { const sessionID = "ses-stagnation-recovery" const todos = [ { id: "1", content: "Task 1", status: "pending", priority: "high" }, @@ -96,9 +96,9 @@ describe("createSessionStateStore regressions", () => { const progressUpdate = sessionStateStore.trackContinuationProgress(sessionID, 2, todos) expect(progressUpdate.previousStagnationCount).toBe(MAX_STAGNATION_COUNT) - expect(progressUpdate.hasProgressed).toBe(true) - expect(progressUpdate.progressSource).toBe("activity") - expect(progressUpdate.stagnationCount).toBe(0) + expect(progressUpdate.hasProgressed).toBe(false) + expect(progressUpdate.progressSource).toBe("none") + expect(progressUpdate.stagnationCount).toBe(MAX_STAGNATION_COUNT) }) }) }) diff --git a/src/hooks/todo-continuation-enforcer/session-state.ts b/src/hooks/todo-continuation-enforcer/session-state.ts index 810fdcfb4..8a151958f 100644 --- a/src/hooks/todo-continuation-enforcer/session-state.ts +++ b/src/hooks/todo-continuation-enforcer/session-state.ts @@ -16,8 +16,6 @@ interface TrackedSessionState { lastAccessedAt: number lastCompletedCount?: number lastTodoSnapshot?: string - activitySignalCount: number - lastObservedActivitySignalCount?: number } export interface ContinuationProgressUpdate { @@ -25,7 +23,7 @@ export interface ContinuationProgressUpdate { previousStagnationCount: number stagnationCount: number hasProgressed: boolean - progressSource: "none" | "todo" | "activity" + progressSource: "none" | "todo" } export interface SessionStateStore { @@ -98,17 +96,7 @@ export function createSessionStateStore(): SessionStateStore { const trackedSession: TrackedSessionState = { state: rawState, lastAccessedAt: Date.now(), - activitySignalCount: 0, } - trackedSession.state = new Proxy(rawState, { - set(target, property, value, receiver) { - if (property === "abortDetectedAt" && value === undefined) { - trackedSession.activitySignalCount += 1 - } - - return Reflect.set(target, property, value, receiver) - }, - }) sessions.set(sessionID, trackedSession) return trackedSession } @@ -137,7 +125,6 @@ export function createSessionStateStore(): SessionStateStore { const previousStagnationCount = state.stagnationCount const currentCompletedCount = todos?.filter((todo) => todo.status === "completed").length const currentTodoSnapshot = todos ? getTodoSnapshot(todos) : undefined - const currentActivitySignalCount = trackedSession.activitySignalCount const hasCompletedMoreTodos = currentCompletedCount !== undefined && trackedSession.lastCompletedCount !== undefined @@ -146,9 +133,6 @@ export function createSessionStateStore(): SessionStateStore { currentTodoSnapshot !== undefined && trackedSession.lastTodoSnapshot !== undefined && currentTodoSnapshot !== trackedSession.lastTodoSnapshot - const hasObservedExternalActivity = - trackedSession.lastObservedActivitySignalCount !== undefined - && currentActivitySignalCount > trackedSession.lastObservedActivitySignalCount const hadSuccessfulInjectionAwaitingProgressCheck = state.awaitingPostInjectionProgressCheck === true state.lastIncompleteCount = incompleteCount @@ -158,7 +142,6 @@ export function createSessionStateStore(): SessionStateStore { if (currentTodoSnapshot !== undefined) { trackedSession.lastTodoSnapshot = currentTodoSnapshot } - trackedSession.lastObservedActivitySignalCount = currentActivitySignalCount if (previousIncompleteCount === undefined) { state.stagnationCount = 0 @@ -173,9 +156,7 @@ export function createSessionStateStore(): SessionStateStore { const progressSource = incompleteCount < previousIncompleteCount || hasCompletedMoreTodos || hasTodoSnapshotChanged ? "todo" - : hasObservedExternalActivity - ? "activity" - : "none" + : "none" if (progressSource !== "none") { state.stagnationCount = 0 @@ -223,8 +204,6 @@ export function createSessionStateStore(): SessionStateStore { state.awaitingPostInjectionProgressCheck = false trackedSession.lastCompletedCount = undefined trackedSession.lastTodoSnapshot = undefined - trackedSession.activitySignalCount = 0 - trackedSession.lastObservedActivitySignalCount = undefined } function cancelCountdown(sessionID: string): void { diff --git a/src/hooks/todo-continuation-enforcer/stagnation-detection.test.ts b/src/hooks/todo-continuation-enforcer/stagnation-detection.test.ts index f309f44fa..2c9ce17df 100644 --- a/src/hooks/todo-continuation-enforcer/stagnation-detection.test.ts +++ b/src/hooks/todo-continuation-enforcer/stagnation-detection.test.ts @@ -3,6 +3,8 @@ import { describe, expect, it as test } from "bun:test" import { MAX_STAGNATION_COUNT } from "./constants" +import { handleNonIdleEvent } from "./non-idle-events" +import { createSessionStateStore } from "./session-state" import { shouldStopForStagnation } from "./stagnation-detection" describe("shouldStopForStagnation", () => { @@ -25,7 +27,7 @@ describe("shouldStopForStagnation", () => { }) }) - describe("#when activity progress is detected after the halt", () => { + describe("#when todo progress is detected after the halt", () => { test("#then it clears the stop condition", () => { const shouldStop = shouldStopForStagnation({ sessionID: "ses-recovered", @@ -35,7 +37,7 @@ describe("shouldStopForStagnation", () => { previousStagnationCount: MAX_STAGNATION_COUNT, stagnationCount: 0, hasProgressed: true, - progressSource: "activity", + progressSource: "todo", }, }) @@ -43,4 +45,60 @@ describe("shouldStopForStagnation", () => { }) }) }) + + describe("#given only non-idle tool and message events happen between idle checks", () => { + describe("#when todo state does not change across three idle cycles", () => { + test("#then stagnation count reaches three", () => { + // given + const sessionStateStore = createSessionStateStore() + const sessionID = "ses-non-idle-activity-without-progress" + const state = sessionStateStore.getState(sessionID) + const todos = [ + { id: "1", content: "Task 1", status: "pending", priority: "high" }, + { id: "2", content: "Task 2", status: "pending", priority: "medium" }, + ] + + sessionStateStore.trackContinuationProgress(sessionID, 2, todos) + + // when + state.awaitingPostInjectionProgressCheck = true + const firstCycle = sessionStateStore.trackContinuationProgress(sessionID, 2, todos) + + handleNonIdleEvent({ + eventType: "tool.execute.before", + properties: { sessionID }, + sessionStateStore, + }) + handleNonIdleEvent({ + eventType: "message.updated", + properties: { info: { sessionID, role: "assistant" } }, + sessionStateStore, + }) + + state.awaitingPostInjectionProgressCheck = true + const secondCycle = sessionStateStore.trackContinuationProgress(sessionID, 2, todos) + + handleNonIdleEvent({ + eventType: "tool.execute.after", + properties: { sessionID }, + sessionStateStore, + }) + handleNonIdleEvent({ + eventType: "message.part.updated", + properties: { info: { sessionID, role: "assistant" } }, + sessionStateStore, + }) + + state.awaitingPostInjectionProgressCheck = true + const thirdCycle = sessionStateStore.trackContinuationProgress(sessionID, 2, todos) + + // then + expect(firstCycle.stagnationCount).toBe(1) + expect(secondCycle.stagnationCount).toBe(2) + expect(thirdCycle.stagnationCount).toBe(3) + + sessionStateStore.shutdown() + }) + }) + }) }) diff --git a/src/plugin/event.test.ts b/src/plugin/event.test.ts index 8db998c43..67089e2a5 100644 --- a/src/plugin/event.test.ts +++ b/src/plugin/event.test.ts @@ -1,8 +1,15 @@ -import { describe, it, expect } from "bun:test" +import { describe, it, expect, afterEach } from "bun:test" import { createEventHandler } from "./event" +import { createChatMessageHandler } from "./chat-message" +import { _resetForTesting, setMainSession } from "../features/claude-code-session-state" +import { clearPendingModelFallback, createModelFallbackHook } from "../hooks/model-fallback/hook" -type EventInput = { event: { type: string; properties?: Record } } +type EventInput = { event: { type: string; properties?: unknown } } + +afterEach(() => { + _resetForTesting() +}) describe("createEventHandler - idle deduplication", () => { it("Order A (status→idle): synthetic idle deduped - real idle not dispatched again", async () => { @@ -66,7 +73,7 @@ type EventInput = { event: { type: string; properties?: Record //#then - synthetic idle dispatched once expect(dispatchCalls.length).toBe(1) expect(dispatchCalls[0].event.type).toBe("session.idle") - expect(dispatchCalls[0].event.properties?.sessionID).toBe(sessionId) + expect((dispatchCalls[0].event.properties as { sessionID?: string } | undefined)?.sessionID).toBe(sessionId) //#when - real session.idle arrives await eventHandler({ @@ -142,7 +149,7 @@ type EventInput = { event: { type: string; properties?: Record //#then - real idle dispatched once expect(dispatchCalls.length).toBe(1) expect(dispatchCalls[0].event.type).toBe("session.idle") - expect(dispatchCalls[0].event.properties?.sessionID).toBe(sessionId) + expect((dispatchCalls[0].event.properties as { sessionID?: string } | undefined)?.sessionID).toBe(sessionId) //#when - session.status with idle (generates synthetic idle) await eventHandler({ @@ -245,7 +252,7 @@ type EventInput = { event: { type: string; properties?: Record event: { type: "message.updated", }, - }) + } as any) //#then - both maps should be pruned (no dedup should occur for new events) // We verify by checking that a new idle event for same session is dispatched @@ -287,7 +294,7 @@ type EventInput = { event: { type: string; properties?: Record stopContinuationGuard: { event: async () => {} }, compactionTodoPreserver: { event: async () => {} }, atlasHook: { handler: async () => {} }, - }, + } as any, }) await eventHandlerWithMock({ @@ -426,7 +433,7 @@ describe("createEventHandler - event forwarding", () => { type: "session.deleted", properties: { info: { id: sessionID } }, }, - }) + } as any) //#then expect(forwardedEvents.length).toBe(1) @@ -435,3 +442,146 @@ describe("createEventHandler - event forwarding", () => { expect(deletedSessions).toEqual([sessionID]) }) }) + +describe("createEventHandler - retry dedupe lifecycle", () => { + it("re-handles same retry key after session recovers to idle status", async () => { + //#given + const sessionID = "ses_retry_recovery_rearm" + setMainSession(sessionID) + clearPendingModelFallback(sessionID) + + const abortCalls: string[] = [] + const promptCalls: string[] = [] + const modelFallback = createModelFallbackHook() + + const eventHandler = createEventHandler({ + ctx: { + directory: "/tmp", + client: { + session: { + abort: async ({ path }: { path: { id: string } }) => { + abortCalls.push(path.id) + return {} + }, + prompt: async ({ path }: { path: { id: string } }) => { + promptCalls.push(path.id) + return {} + }, + }, + }, + } as any, + pluginConfig: {} as any, + firstMessageVariantGate: { + markSessionCreated: () => {}, + clear: () => {}, + }, + managers: { + tmuxSessionManager: { + onSessionCreated: async () => {}, + onSessionDeleted: async () => {}, + }, + skillMcpManager: { + disconnectSession: async () => {}, + }, + } as any, + hooks: { + modelFallback, + stopContinuationGuard: { isStopped: () => false }, + } as any, + }) + + const chatMessageHandler = createChatMessageHandler({ + ctx: { + client: { + tui: { + showToast: async () => ({}), + }, + }, + } as any, + pluginConfig: {} as any, + firstMessageVariantGate: { + shouldOverride: () => false, + markApplied: () => {}, + }, + hooks: { + modelFallback, + stopContinuationGuard: null, + keywordDetector: null, + claudeCodeHooks: null, + autoSlashCommand: null, + startWork: null, + ralphLoop: null, + } as any, + }) + + const retryStatus = { + type: "retry", + attempt: 1, + message: "All credentials for model claude-opus-4-6-thinking are cooling down [retrying in 7m 56s attempt #1]", + next: 476, + } as const + + await eventHandler({ + event: { + type: "message.updated", + properties: { + info: { + id: "msg_user_retry_rearm", + sessionID, + role: "user", + modelID: "claude-opus-4-6-thinking", + providerID: "anthropic", + agent: "Sisyphus (Ultraworker)", + }, + }, + }, + } as any) + + //#when - first retry key is handled + await eventHandler({ + event: { + type: "session.status", + properties: { + sessionID, + status: retryStatus, + }, + }, + } as any) + + const firstOutput = { message: {}, parts: [] as Array<{ type: string; text?: string }> } + await chatMessageHandler( + { + sessionID, + agent: "sisyphus", + model: { providerID: "anthropic", modelID: "claude-opus-4-6-thinking" }, + }, + firstOutput, + ) + + //#when - session recovers to non-retry idle state + await eventHandler({ + event: { + type: "session.status", + properties: { + sessionID, + status: { type: "idle" }, + }, + }, + } as any) + + //#when - same retry key appears again after recovery + await eventHandler({ + event: { + type: "session.status", + properties: { + sessionID, + status: retryStatus, + }, + }, + } as any) + + //#then + expect(abortCalls).toEqual([sessionID, sessionID]) + expect(promptCalls).toEqual([sessionID, sessionID]) + }) +}) diff --git a/src/plugin/event.ts b/src/plugin/event.ts index d18ec3691..45765c025 100644 --- a/src/plugin/event.ts +++ b/src/plugin/event.ts @@ -421,6 +421,12 @@ export function createEventHandler(args: { const sessionID = props?.sessionID as string | undefined; const status = props?.status as { type?: string; attempt?: number; message?: string; next?: number } | undefined; + // Retry dedupe lifecycle: set key when a retry status is handled, clear it after recovery + // (non-retry idle) so future failures with the same key can trigger fallback again. + if (sessionID && status?.type === "idle") { + lastHandledRetryStatusKey.delete(sessionID); + } + if (sessionID && status?.type === "retry" && isModelFallbackEnabled && !isRuntimeFallbackEnabled) { try { const retryMessage = typeof status.message === "string" ? status.message : ""; diff --git a/src/shared/plugin-identity.ts b/src/shared/plugin-identity.ts index 65b190fe6..3de14c328 100644 --- a/src/shared/plugin-identity.ts +++ b/src/shared/plugin-identity.ts @@ -1,4 +1,5 @@ export const PLUGIN_NAME = "oh-my-opencode" +export const LEGACY_PLUGIN_NAME = "oh-my-openagent" export const CONFIG_BASENAME = "oh-my-opencode" export const LOG_FILENAME = "oh-my-opencode.log" export const CACHE_DIR_NAME = "oh-my-opencode" diff --git a/src/shared/shell-env.ts b/src/shared/shell-env.ts index b074baf51..bec4b8336 100644 --- a/src/shared/shell-env.ts +++ b/src/shared/shell-env.ts @@ -109,3 +109,44 @@ export function buildEnvPrefix( return "" } } + +/** + * Escape a value for use in a double-quoted shell -c command argument. + * + * In shell -c "..." strings, these characters have special meaning and must be escaped: + * - $ - variable expansion, command substitution $(...) + * - ` - command substitution `...` + * - \\ - escape character + * - " - end quote + * - ; | & - command separators + * - # - comment + * - () - grouping operators + * + * @param value - The value to escape + * @returns Escaped value safe for double-quoted shell -c argument + * + * @example + * ```ts + * // For malicious input + * const url = "http://localhost:3000'; cat /etc/passwd; echo '" + * const escaped = shellEscapeForDoubleQuotedCommand(url) + * // => "http://localhost:3000'\''; cat /etc/passwd; echo '" + * + * // Usage in command: + * const cmd = `/bin/sh -c "opencode attach ${escaped} --session ${sessionId}"` + * ``` + */ +export function shellEscapeForDoubleQuotedCommand(value: string): string { + // Order matters: escape backslash FIRST, then other characters + return value + .replace(/\\/g, "\\\\") // escape backslash first + .replace(/\$/g, "\\$") // escape dollar sign + .replace(/`/g, "\\`") // escape backticks + .replace(/"/g, "\\\"") // escape double quotes + .replace(/;/g, "\\;") // escape semicolon (command separator) + .replace(/\|/g, "\\|") // escape pipe (command separator) + .replace(/&/g, "\\&") // escape ampersand (command separator) + .replace(/#/g, "\\#") // escape hash (comment) + .replace(/\(/g, "\\(") // escape parentheses + .replace(/\)/g, "\\)") // escape parentheses +} diff --git a/src/shared/tmux/tmux-utils/pane-replace.ts b/src/shared/tmux/tmux-utils/pane-replace.ts index 53520439a..271ad79eb 100644 --- a/src/shared/tmux/tmux-utils/pane-replace.ts +++ b/src/shared/tmux/tmux-utils/pane-replace.ts @@ -3,6 +3,7 @@ import type { TmuxConfig } from "../../../config/schema" import { getTmuxPath } from "../../../tools/interactive-bash/tmux-path-resolver" import type { SpawnPaneResult } from "../types" import { isInsideTmux } from "./environment" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" export async function replaceTmuxPane( paneId: string, @@ -35,7 +36,8 @@ export async function replaceTmuxPane( await ctrlCProc.exited const shell = process.env.SHELL || "/bin/sh" - const opencodeCmd = `${shell} -c 'opencode attach ${serverUrl} --session ${sessionId}'` + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` const proc = spawn([tmux, "respawn-pane", "-k", "-t", paneId, opencodeCmd], { stdout: "pipe", @@ -60,6 +62,7 @@ export async function replaceTmuxPane( const titleStderr = await stderrPromise log("[replaceTmuxPane] WARNING: failed to set pane title", { paneId, + title, exitCode: titleExitCode, stderr: titleStderr.trim(), }) diff --git a/src/shared/tmux/tmux-utils/pane-spawn.test.ts b/src/shared/tmux/tmux-utils/pane-spawn.test.ts new file mode 100644 index 000000000..a4ca40cfe --- /dev/null +++ b/src/shared/tmux/tmux-utils/pane-spawn.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from "bun:test" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" + +describe("given a serverUrl with shell metacharacters", () => { + describe("when building tmux spawn command with double quotes", () => { + it("then serverUrl is escaped to prevent shell injection", () => { + const serverUrl = "http://localhost:3000'; cat /etc/passwd; echo '" + const sessionId = "test-session" + const shell = "/bin/sh" + + // Use double quotes for outer shell -c command, escape dangerous chars in URL + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + // The semicolon should be escaped so it's treated as literal, not separator + expect(opencodeCmd).toContain("\\;") + // The malicious content should be escaped - semicolons are now \\; + expect(opencodeCmd).not.toMatch(/[^\\];\s*cat/) + }) + }) + + describe("when building tmux replace command", () => { + it("then serverUrl is escaped to prevent shell injection", () => { + const serverUrl = "http://localhost:3000'; rm -rf /; '" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\;") + expect(opencodeCmd).not.toMatch(/[^\\];\s*rm/) + }) + }) +}) + +describe("given a normal serverUrl without shell metacharacters", () => { + describe("when building tmux spawn command", () => { + it("then serverUrl works correctly", () => { + const serverUrl = "http://localhost:3000" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain(serverUrl) + }) + }) +}) + +describe("given a serverUrl with dollar sign (command injection)", () => { + describe("when building tmux command", () => { + it("then dollar sign is escaped properly", () => { + const serverUrl = "http://localhost:3000$(whoami)" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + // The $ should be escaped to literal $ + expect(opencodeCmd).toContain("\\$") + }) + }) +}) + +describe("given a serverUrl with backticks (command injection)", () => { + describe("when building tmux command", () => { + it("then backticks are escaped properly", () => { + const serverUrl = "http://localhost:3000`whoami`" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\`") + }) + }) +}) + +describe("given a serverUrl with pipe operator", () => { + describe("when building tmux command", () => { + it("then pipe is escaped properly", () => { + const serverUrl = "http://localhost:3000 | ls" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\|") + }) + }) +}) diff --git a/src/shared/tmux/tmux-utils/pane-spawn.ts b/src/shared/tmux/tmux-utils/pane-spawn.ts index 18af7c536..2713eafbc 100644 --- a/src/shared/tmux/tmux-utils/pane-spawn.ts +++ b/src/shared/tmux/tmux-utils/pane-spawn.ts @@ -5,6 +5,7 @@ import type { SpawnPaneResult } from "../types" import type { SplitDirection } from "./environment" import { isInsideTmux } from "./environment" import { isServerRunning } from "./server-health" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" export async function spawnTmuxPane( sessionId: string, @@ -49,7 +50,8 @@ export async function spawnTmuxPane( log("[spawnTmuxPane] all checks passed, spawning...") const shell = process.env.SHELL || "/bin/sh" - const opencodeCmd = `${shell} -c 'opencode attach ${serverUrl} --session ${sessionId}'` + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` const args = [ "split-window", diff --git a/src/tools/delegate-task/category-resolver.test.ts b/src/tools/delegate-task/category-resolver.test.ts index 2257978f0..3c9124735 100644 --- a/src/tools/delegate-task/category-resolver.test.ts +++ b/src/tools/delegate-task/category-resolver.test.ts @@ -7,16 +7,22 @@ import * as connectedProvidersCache from "../../shared/connected-providers-cache describe("resolveCategoryExecution", () => { let connectedProvidersSpy: ReturnType | undefined let providerModelsSpy: ReturnType | undefined + let hasConnectedProvidersSpy: ReturnType | undefined + let hasProviderModelsSpy: ReturnType | undefined beforeEach(() => { mock.restore() connectedProvidersSpy = spyOn(connectedProvidersCache, "readConnectedProvidersCache").mockReturnValue(null) providerModelsSpy = spyOn(connectedProvidersCache, "readProviderModelsCache").mockReturnValue(null) + hasConnectedProvidersSpy = spyOn(connectedProvidersCache, "hasConnectedProvidersCache").mockReturnValue(false) + hasProviderModelsSpy = spyOn(connectedProvidersCache, "hasProviderModelsCache").mockReturnValue(false) }) afterEach(() => { connectedProvidersSpy?.mockRestore() providerModelsSpy?.mockRestore() + hasConnectedProvidersSpy?.mockRestore() + hasProviderModelsSpy?.mockRestore() }) const createMockExecutorContext = (): ExecutorContext => ({ @@ -27,7 +33,7 @@ describe("resolveCategoryExecution", () => { sisyphusJuniorModel: undefined, }) - test("returns clear error when category exists but required model is not available", async () => { + test("returns unpinned resolution when category cache is not ready on first run", async () => { //#given const args = { category: "deep", @@ -39,6 +45,9 @@ describe("resolveCategoryExecution", () => { enableSkillTools: false, } const executorCtx = createMockExecutorContext() + executorCtx.userCategories = { + deep: {}, + } const inheritedModel = undefined const systemDefaultModel = "anthropic/claude-sonnet-4-6" @@ -46,10 +55,10 @@ describe("resolveCategoryExecution", () => { const result = await resolveCategoryExecution(args, executorCtx, inheritedModel, systemDefaultModel) //#then - expect(result.error).toBeDefined() - expect(result.error).toContain("deep") - expect(result.error).toMatch(/model.*not.*available|requires.*model/i) - expect(result.error).not.toContain("Unknown category") + expect(result.error).toBeUndefined() + expect(result.actualModel).toBeUndefined() + expect(result.categoryModel).toBeUndefined() + expect(result.agentToUse).toBeDefined() }) test("returns 'unknown category' error for truly unknown categories", async () => { diff --git a/src/tools/delegate-task/category-resolver.ts b/src/tools/delegate-task/category-resolver.ts index 26e518529..f6c13a825 100644 --- a/src/tools/delegate-task/category-resolver.ts +++ b/src/tools/delegate-task/category-resolver.ts @@ -85,6 +85,7 @@ Available categories: ${allCategoryNames}`, let actualModel: string | undefined let modelInfo: ModelFallbackInfo | undefined let categoryModel: { providerID: string; modelID: string; variant?: string } | undefined + let isModelResolutionSkipped = false const overrideModel = sisyphusJuniorModel const explicitCategoryModel = userCategories?.[args.category!]?.model @@ -114,7 +115,9 @@ Available categories: ${allCategoryNames}`, systemDefaultModel, }) - if (resolution) { + if (resolution && "skipped" in resolution) { + isModelResolutionSkipped = true + } else if (resolution) { const { model: resolvedModel, variant: resolvedVariant } = resolution actualModel = resolvedModel @@ -161,7 +164,7 @@ Available categories: ${allCategoryNames}`, } const categoryPromptAppend = resolved.promptAppend || undefined - if (!categoryModel && !actualModel) { + if (!categoryModel && !actualModel && !isModelResolutionSkipped) { const categoryNames = Object.keys(enabledCategories) return { agentToUse: "", diff --git a/src/tools/delegate-task/model-selection.test.ts b/src/tools/delegate-task/model-selection.test.ts index 18d0d5f82..2f9b9c196 100644 --- a/src/tools/delegate-task/model-selection.test.ts +++ b/src/tools/delegate-task/model-selection.test.ts @@ -1,4 +1,5 @@ -import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test" +declare const require: (name: string) => any +const { afterEach, beforeEach, describe, expect, mock, spyOn, test } = require("bun:test") import { resolveModelForDelegateTask } from "./model-selection" import * as connectedProvidersCache from "../../shared/connected-providers-cache" @@ -22,7 +23,7 @@ describe("resolveModelForDelegateTask", () => { }) describe("#when availableModels is empty and no user model override", () => { - test("#then returns undefined to let OpenCode use system default", () => { + test("#then returns skipped sentinel to leave model unpinned", () => { const result = resolveModelForDelegateTask({ categoryDefaultModel: "anthropic/claude-sonnet-4-6", fallbackChain: [ @@ -32,7 +33,7 @@ describe("resolveModelForDelegateTask", () => { systemDefaultModel: "anthropic/claude-sonnet-4-6", }) - expect(result).toBeUndefined() + expect(result).toEqual({ skipped: true }) }) }) @@ -53,7 +54,7 @@ describe("resolveModelForDelegateTask", () => { }) describe("#when user set fallback_models but no cache exists", () => { - test("#then returns undefined (skip fallback resolution without cache)", () => { + test("#then returns skipped sentinel (skip fallback resolution without cache)", () => { const result = resolveModelForDelegateTask({ userFallbackModels: ["openai/gpt-5.4", "google/gemini-3.1-pro"], categoryDefaultModel: "anthropic/claude-sonnet-4-6", @@ -63,7 +64,7 @@ describe("resolveModelForDelegateTask", () => { availableModels: new Set(), }) - expect(result).toBeUndefined() + expect(result).toEqual({ skipped: true }) }) }) }) @@ -85,8 +86,7 @@ describe("resolveModelForDelegateTask", () => { systemDefaultModel: "anthropic/claude-sonnet-4-6", }) - expect(result).toBeDefined() - expect(result!.model).toBe("anthropic/claude-sonnet-4-6") + expect(result).toEqual({ model: "anthropic/claude-sonnet-4-6" }) }) }) @@ -100,8 +100,7 @@ describe("resolveModelForDelegateTask", () => { availableModels: new Set(["anthropic/claude-sonnet-4-6"]), }) - expect(result).toBeDefined() - expect(result!.model).toBe("anthropic/claude-sonnet-4-6") + expect(result).toEqual({ model: "anthropic/claude-sonnet-4-6" }) }) }) diff --git a/src/tools/delegate-task/model-selection.ts b/src/tools/delegate-task/model-selection.ts index 6beb24192..14b069ad7 100644 --- a/src/tools/delegate-task/model-selection.ts +++ b/src/tools/delegate-task/model-selection.ts @@ -51,7 +51,7 @@ export function resolveModelForDelegateTask(input: { fallbackChain?: FallbackEntry[] availableModels: Set systemDefaultModel?: string -}): { model: string; variant?: string } | undefined { +}): { model: string; variant?: string } | { skipped: true } | undefined { const userModel = normalizeModel(input.userModel) if (userModel) { return { model: userModel } @@ -60,7 +60,7 @@ export function resolveModelForDelegateTask(input: { // Before provider cache is created (first run), skip model resolution entirely. // OpenCode will use its system default model when no model is specified in the prompt. if (input.availableModels.size === 0 && !hasProviderModelsCache() && !hasConnectedProvidersCache()) { - return undefined + return { skipped: true } } const categoryDefault = normalizeModel(input.categoryDefaultModel) diff --git a/src/tools/delegate-task/subagent-resolver.ts b/src/tools/delegate-task/subagent-resolver.ts index 7e2a5c055..5567ec6cb 100644 --- a/src/tools/delegate-task/subagent-resolver.ts +++ b/src/tools/delegate-task/subagent-resolver.ts @@ -124,7 +124,7 @@ Create the work plan directly - that's your job as the planning agent.`, systemDefaultModel: undefined, }) - if (resolution) { + if (resolution && !('skipped' in resolution)) { const normalized = normalizeModelFormat(resolution.model) if (normalized) { const variantToUse = agentOverride?.variant ?? resolution.variant diff --git a/src/tools/skill/tools.test.ts b/src/tools/skill/tools.test.ts index 0c30d69d2..69839f5e6 100644 --- a/src/tools/skill/tools.test.ts +++ b/src/tools/skill/tools.test.ts @@ -525,3 +525,59 @@ describe("skill tool - dynamic discovery", () => { expect(result).not.toContain("SHOULD_BE_OVERRIDDEN") }) }) +describe("skill tool - dynamic description cache invalidation", () => { + it("rebuilds description after execute() discovers new skills", async () => { + // given: tool created with initial skills (no pre-provided skills) + // This triggers lazy description building + const tool = createSkillTool({}) + + // Get initial description - it will build from empty or disk skills + const initialDescription = tool.description + + // when: execute() is called, which clears cache AND gets fresh skills + // Note: In real scenario, execute() would discover new skills from disk + // For testing, we verify the mechanism: execute() should invalidate cachedDescription + + // Execute any skill to trigger the cache clear + getSkills flow + // Using a non-existent skill name to trigger the error path which still goes through getSkills() + try { + await tool.execute({ name: "nonexistent-skill-12345" }, mockContext) + } catch (e) { + // Expected to fail - skill doesn't exist + } + + // then: cachedDescription should be invalidated, so next description access should rebuild + // We verify by checking that the description getter triggers a rebuild + // Since we can't easily mock getAllSkills in this test, we verify the cache invalidation mechanism + + // The key assertion: after execute(), the description should be rebuildable + // If cachedDescription wasn't invalidated, it would still return old value + // We verify by checking that the tool still has valid description structure + expect(tool.description).toBeDefined() + expect(typeof tool.description).toBe("string") + }) + + it("description reflects fresh skills after execute() clears cache", async () => { + // given: tool created without pre-provided skills (will use disk discovery) + const tool = createSkillTool({}) + + // when: execute() is called with a skill that exists on disk (via mock) + // This simulates the real scenario: execute() discovers skills, cache should be invalidated + + // Execute to trigger the cache invalidation path + try { + // This will call getSkills() which clears cache + await tool.execute({ name: "nonexistent" }, mockContext) + } catch (e) { + // Expected + } + + // then: description should still work and not be stale + // The bug would cause it to return old cached value forever + const desc = tool.description + + // Verify description is a valid string (not stale/old) + expect(desc).toContain("skill") + }) +}) + diff --git a/src/tools/skill/tools.ts b/src/tools/skill/tools.ts index a5bc4b282..dd0934507 100644 --- a/src/tools/skill/tools.ts +++ b/src/tools/skill/tools.ts @@ -235,6 +235,7 @@ export function createSkillTool(options: SkillLoadOptions = {}): ToolDefinition }, async execute(args: SkillArgs, ctx?: { agent?: string }) { const skills = await getSkills() + cachedDescription = null const commands = getCommands() const requestedName = args.name.replace(/^\//, "")