From eb1e104742e3d51fbb3eb2078d1852dac0a12d7a Mon Sep 17 00:00:00 2001 From: Matan Kushner Date: Mon, 4 May 2026 19:36:23 +0900 Subject: [PATCH] refactor(config): tighten walk-up config discovery from oracle review Stop the walk at the start directory when it sits outside $HOME so the walker never falls through to filesystem root. Without this guard a project at /tmp/x or /opt/projects/foo would surface unrelated configs in /tmp, /opt, or / itself. Also clean up reviewer-flagged friction: - Rename ancestor path/override variables to *NearestFirst / *FarthestFirst so the merge order is self-documenting and the mid-flight `.slice().reverse()` is no longer surprising. - Rename `migrateLegacyAndResolveCanonicalPath` to `resolveConfigPathAfterLegacyMigration` to reflect that the helper returns the path to load, which may still be the legacy path when migration could not run. - Replace `Object.assign({}, ...overrides)` with a named accumulator loop so the closer-wins ordering is obvious from the code instead of relying on a comment. Tests added: - start directory outside $HOME does not walk above itself - multi-ancestor git_master merge order (closer wins, distant fields still flow through) - agent_definitions in an ancestor resolves against that ancestor's own .opencode/ base path, not the start directory's --- src/plugin-config.test.ts | 135 +++++++++++++++++++++++++++++++++++++- src/plugin-config.ts | 66 ++++++++++--------- 2 files changed, 169 insertions(+), 32 deletions(-) diff --git a/src/plugin-config.test.ts b/src/plugin-config.test.ts index fbe4d8599..ee2dfa8c4 100644 --- a/src/plugin-config.test.ts +++ b/src/plugin-config.test.ts @@ -1,5 +1,5 @@ -import { afterEach, describe, expect, it, mock } from "bun:test"; -import { chmodSync, existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs" +import { afterEach, describe, expect, it, mock, spyOn } from "bun:test"; +import { chmodSync, existsSync, mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { mergeConfigs, parseConfigPartially } from "./plugin-config"; @@ -801,6 +801,137 @@ describe("loadPluginConfig", () => { expect(config.agents?.oracle).toBeUndefined() }) + it("should not walk above the start directory when start is outside $HOME", async () => { + // given + const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-outside-")) + const userConfigDir = join(rootDir, "user-config") + const homeDir = join(rootDir, "home") + const outsideHomeRoot = join(rootDir, "outside-home") + const projectDir = join(outsideHomeRoot, "proj") + + tempDirs.push(rootDir) + mkdirSync(userConfigDir, { recursive: true }) + mkdirSync(homeDir, { recursive: true }) + mkdirSync(join(outsideHomeRoot, ".opencode"), { recursive: true }) + mkdirSync(join(projectDir, ".opencode"), { recursive: true }) + + writeFileSync(join(userConfigDir, "oh-my-openagent.jsonc"), "{}") + writeFileSync( + join(outsideHomeRoot, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ agents: { oracle: { model: "outside-home/leak" } } }) + ) + writeFileSync( + join(projectDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ agents: { hephaestus: { model: "project/wins" } } }) + ) + + process.env.OPENCODE_CONFIG_DIR = userConfigDir + process.env.HOME = homeDir + + // when + const { loadPluginConfig } = await importFreshPluginConfigModule() + const config = loadPluginConfig(projectDir, {}) + + // then - project loads, but the parent above it (outside $HOME) is not walked into + expect(config.agents?.hephaestus?.model).toBe("project/wins") + expect(config.agents?.oracle).toBeUndefined() + }) + + it("should merge git_master overrides across ancestors with closer winning", async () => { + // given + const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-git-master-")) + const userConfigDir = join(rootDir, "user-config") + const homeDir = join(rootDir, "home") + const workDir = join(homeDir, "work") + const projectDir = join(workDir, "project") + + tempDirs.push(rootDir) + mkdirSync(userConfigDir, { recursive: true }) + mkdirSync(join(homeDir, ".opencode"), { recursive: true }) + mkdirSync(join(workDir, ".opencode"), { recursive: true }) + mkdirSync(join(projectDir, ".opencode"), { recursive: true }) + + writeFileSync(join(userConfigDir, "oh-my-openagent.jsonc"), "{}") + writeFileSync( + join(homeDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ + git_master: { + commit_footer: false, + include_co_authored_by: false, + git_env_prefix: "HOME=1", + }, + }) + ) + writeFileSync( + join(workDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ + git_master: { + include_co_authored_by: true, + }, + }) + ) + writeFileSync( + join(projectDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ + git_master: { + commit_footer: true, + }, + }) + ) + + process.env.OPENCODE_CONFIG_DIR = userConfigDir + process.env.HOME = homeDir + + // when + const { loadPluginConfig } = await importFreshPluginConfigModule() + const config = loadPluginConfig(projectDir, {}) + + // then project's commit_footer wins, work's include_co_authored_by wins, + // home's git_env_prefix is preserved since nobody else set it + expect(config.git_master).toEqual({ + commit_footer: true, + include_co_authored_by: true, + git_env_prefix: "HOME=1", + }) + }) + + it("should resolve agent_definitions relative to each ancestor's own .opencode directory", async () => { + // given + const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-agent-defs-")) + const userConfigDir = join(rootDir, "user-config") + const homeDir = join(rootDir, "home") + const workDir = join(homeDir, "work") + const projectDir = join(workDir, "project") + const workDefRelativePath = "./work-agent.md" + const projectDefRelativePath = "./project-agent.md" + + tempDirs.push(rootDir) + mkdirSync(userConfigDir, { recursive: true }) + mkdirSync(join(workDir, ".opencode"), { recursive: true }) + mkdirSync(join(projectDir, ".opencode"), { recursive: true }) + + writeFileSync(join(userConfigDir, "oh-my-openagent.jsonc"), "{}") + writeFileSync( + join(workDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ agent_definitions: [workDefRelativePath] }) + ) + writeFileSync( + join(projectDir, ".opencode", "oh-my-openagent.jsonc"), + JSON.stringify({ agent_definitions: [projectDefRelativePath] }) + ) + + process.env.OPENCODE_CONFIG_DIR = userConfigDir + process.env.HOME = homeDir + + // when + const { loadPluginConfig } = await importFreshPluginConfigModule() + const config = loadPluginConfig(projectDir, {}) + + // then each ancestor's relative path resolves against its own .opencode/ + expect(config.agent_definitions).toContain(join(realpathSync(workDir), ".opencode", "work-agent.md")) + expect(config.agent_definitions).toContain(join(realpathSync(projectDir), ".opencode", "project-agent.md")) + }) + it("should migrate legacy basenames found in ancestor directories", async () => { // given const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-legacy-")) diff --git a/src/plugin-config.ts b/src/plugin-config.ts index f745f9181..cedb3ac2d 100644 --- a/src/plugin-config.ts +++ b/src/plugin-config.ts @@ -4,6 +4,7 @@ import * as path from "path"; import { OhMyOpenCodeConfigSchema, type OhMyOpenCodeConfig } from "./config"; import { log, + containsPath, deepMerge, getOpenCodeConfigDir, addConfigLoadError, @@ -24,7 +25,7 @@ function resolveHomeDirectory(): string { return process.env.HOME ?? process.env.USERPROFILE ?? homedir() } -function migrateLegacyAndResolveCanonicalPath(detectedPath: string): string { +function resolveConfigPathAfterLegacyMigration(detectedPath: string): string { if (!path.basename(detectedPath).startsWith(LEGACY_CONFIG_BASENAME)) { return detectedPath } @@ -245,34 +246,38 @@ export function loadPluginConfig( // Auto-copy legacy config file to canonical name if needed if (userDetected.format !== "none") { - userConfigPath = migrateLegacyAndResolveCanonicalPath(userConfigPath) + userConfigPath = resolveConfigPathAfterLegacyMigration(userConfigPath) } - // Walk up from directory to $HOME for ancestor configs (closest first) - // This subsumes the previous single project-config load: the closest hit is - // the project's own .opencode/oh-my-openagent.json[c], and walking continues - // up so configs in ~/work/ (etc.) apply to all subprojects. - const ancestorConfigPaths = findProjectOpencodePluginConfigFiles( + // Pin the walk to $HOME only when the start directory is inside it. Outside + // $HOME the walker would otherwise reach FS root and surface unrelated configs + // in /tmp, /opt, etc. + const homeDirectory = resolveHomeDirectory() + const stopDirectory = containsPath(homeDirectory, directory) ? homeDirectory : directory + const ancestorConfigPathsNearestFirst = findProjectOpencodePluginConfigFiles( directory, - resolveHomeDirectory(), + stopDirectory, ) log("Walked ancestor plugin configs", { - paths: ancestorConfigPaths, - count: ancestorConfigPaths.length, + paths: ancestorConfigPathsNearestFirst, + count: ancestorConfigPathsNearestFirst.length, + stopDirectory, }) // Migrate any legacy basenames among ancestors and warn on dual-config presence - const canonicalAncestorPaths = ancestorConfigPaths.map((ancestorPath) => { - const opencodeDir = path.dirname(ancestorPath) - const ancestorDetected = detectPluginConfigFile(opencodeDir) - if (ancestorDetected.legacyPath) { - log("Canonical plugin config detected alongside legacy config. Remove the legacy file to avoid confusion.", { - canonicalPath: ancestorDetected.path, - legacyPath: ancestorDetected.legacyPath, - }) - } - return migrateLegacyAndResolveCanonicalPath(ancestorPath) - }) + const canonicalAncestorPathsNearestFirst = ancestorConfigPathsNearestFirst.map( + (ancestorPath) => { + const opencodeDir = path.dirname(ancestorPath) + const ancestorDetected = detectPluginConfigFile(opencodeDir) + if (ancestorDetected.legacyPath) { + log("Canonical plugin config detected alongside legacy config. Remove the legacy file to avoid confusion.", { + canonicalPath: ancestorDetected.path, + legacyPath: ancestorDetected.legacyPath, + }) + } + return resolveConfigPathAfterLegacyMigration(ancestorPath) + }, + ) // Load user config first (base). Parse empty config through Zod to apply field defaults. const userConfig = loadConfigFromPath(userConfigPath, ctx) @@ -289,12 +294,11 @@ export function loadPluginConfig( let config: OhMyOpenCodeConfig = userConfig ?? OhMyOpenCodeConfigSchema.parse({}); - // Merge ancestor configs from farthest to nearest, so closer overrides farther. - // Walker returns nearest-first; reverse for merge order. + const canonicalAncestorPathsFarthestFirst = [...canonicalAncestorPathsNearestFirst].reverse() const defaultGitMaster = OhMyOpenCodeConfigSchema.parse({}).git_master - const ancestorGitMasterOverrides: Array> = [] + const ancestorGitMasterOverridesFarthestFirst: Array> = [] - for (const ancestorPath of canonicalAncestorPaths.slice().reverse()) { + for (const ancestorPath of canonicalAncestorPathsFarthestFirst) { const ancestorConfig = loadConfigFromPath(ancestorPath, ctx) const ancestorOverrides = loadExplicitGitMasterOverrides(ancestorPath) @@ -314,19 +318,21 @@ export function loadPluginConfig( } if (ancestorOverrides) { - ancestorGitMasterOverrides.push(ancestorOverrides) + ancestorGitMasterOverridesFarthestFirst.push(ancestorOverrides) } } - if (userGitMasterOverrides || ancestorGitMasterOverrides.length > 0) { + if (userGitMasterOverrides || ancestorGitMasterOverridesFarthestFirst.length > 0) { + const mergedAncestorGitMaster: Record = {} + for (const override of ancestorGitMasterOverridesFarthestFirst) { + Object.assign(mergedAncestorGitMaster, override) + } config = { ...config, git_master: { ...defaultGitMaster, ...(userGitMasterOverrides ?? {}), - // Ancestors are pushed far-to-near; Object.assign with an empty seed - // applies each in order so the nearest (last) wins. - ...Object.assign({}, ...ancestorGitMasterOverrides), + ...mergedAncestorGitMaster, }, } }