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
This commit is contained in:
committed by
YeonGyu-Kim
parent
cc1d9cf030
commit
eb1e104742
+133
-2
@@ -1,5 +1,5 @@
|
|||||||
import { afterEach, describe, expect, it, mock } from "bun:test";
|
import { afterEach, describe, expect, it, mock, spyOn } from "bun:test";
|
||||||
import { chmodSync, existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"
|
import { chmodSync, existsSync, mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"
|
||||||
import { tmpdir } from "node:os"
|
import { tmpdir } from "node:os"
|
||||||
import { join } from "node:path"
|
import { join } from "node:path"
|
||||||
import { mergeConfigs, parseConfigPartially } from "./plugin-config";
|
import { mergeConfigs, parseConfigPartially } from "./plugin-config";
|
||||||
@@ -801,6 +801,137 @@ describe("loadPluginConfig", () => {
|
|||||||
expect(config.agents?.oracle).toBeUndefined()
|
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 () => {
|
it("should migrate legacy basenames found in ancestor directories", async () => {
|
||||||
// given
|
// given
|
||||||
const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-legacy-"))
|
const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-walk-legacy-"))
|
||||||
|
|||||||
+36
-30
@@ -4,6 +4,7 @@ import * as path from "path";
|
|||||||
import { OhMyOpenCodeConfigSchema, type OhMyOpenCodeConfig } from "./config";
|
import { OhMyOpenCodeConfigSchema, type OhMyOpenCodeConfig } from "./config";
|
||||||
import {
|
import {
|
||||||
log,
|
log,
|
||||||
|
containsPath,
|
||||||
deepMerge,
|
deepMerge,
|
||||||
getOpenCodeConfigDir,
|
getOpenCodeConfigDir,
|
||||||
addConfigLoadError,
|
addConfigLoadError,
|
||||||
@@ -24,7 +25,7 @@ function resolveHomeDirectory(): string {
|
|||||||
return process.env.HOME ?? process.env.USERPROFILE ?? homedir()
|
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)) {
|
if (!path.basename(detectedPath).startsWith(LEGACY_CONFIG_BASENAME)) {
|
||||||
return detectedPath
|
return detectedPath
|
||||||
}
|
}
|
||||||
@@ -245,34 +246,38 @@ export function loadPluginConfig(
|
|||||||
|
|
||||||
// Auto-copy legacy config file to canonical name if needed
|
// Auto-copy legacy config file to canonical name if needed
|
||||||
if (userDetected.format !== "none") {
|
if (userDetected.format !== "none") {
|
||||||
userConfigPath = migrateLegacyAndResolveCanonicalPath(userConfigPath)
|
userConfigPath = resolveConfigPathAfterLegacyMigration(userConfigPath)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Walk up from directory to $HOME for ancestor configs (closest first)
|
// Pin the walk to $HOME only when the start directory is inside it. Outside
|
||||||
// This subsumes the previous single project-config load: the closest hit is
|
// $HOME the walker would otherwise reach FS root and surface unrelated configs
|
||||||
// the project's own .opencode/oh-my-openagent.json[c], and walking continues
|
// in /tmp, /opt, etc.
|
||||||
// up so configs in ~/work/ (etc.) apply to all subprojects.
|
const homeDirectory = resolveHomeDirectory()
|
||||||
const ancestorConfigPaths = findProjectOpencodePluginConfigFiles(
|
const stopDirectory = containsPath(homeDirectory, directory) ? homeDirectory : directory
|
||||||
|
const ancestorConfigPathsNearestFirst = findProjectOpencodePluginConfigFiles(
|
||||||
directory,
|
directory,
|
||||||
resolveHomeDirectory(),
|
stopDirectory,
|
||||||
)
|
)
|
||||||
log("Walked ancestor plugin configs", {
|
log("Walked ancestor plugin configs", {
|
||||||
paths: ancestorConfigPaths,
|
paths: ancestorConfigPathsNearestFirst,
|
||||||
count: ancestorConfigPaths.length,
|
count: ancestorConfigPathsNearestFirst.length,
|
||||||
|
stopDirectory,
|
||||||
})
|
})
|
||||||
|
|
||||||
// Migrate any legacy basenames among ancestors and warn on dual-config presence
|
// Migrate any legacy basenames among ancestors and warn on dual-config presence
|
||||||
const canonicalAncestorPaths = ancestorConfigPaths.map((ancestorPath) => {
|
const canonicalAncestorPathsNearestFirst = ancestorConfigPathsNearestFirst.map(
|
||||||
const opencodeDir = path.dirname(ancestorPath)
|
(ancestorPath) => {
|
||||||
const ancestorDetected = detectPluginConfigFile(opencodeDir)
|
const opencodeDir = path.dirname(ancestorPath)
|
||||||
if (ancestorDetected.legacyPath) {
|
const ancestorDetected = detectPluginConfigFile(opencodeDir)
|
||||||
log("Canonical plugin config detected alongside legacy config. Remove the legacy file to avoid confusion.", {
|
if (ancestorDetected.legacyPath) {
|
||||||
canonicalPath: ancestorDetected.path,
|
log("Canonical plugin config detected alongside legacy config. Remove the legacy file to avoid confusion.", {
|
||||||
legacyPath: ancestorDetected.legacyPath,
|
canonicalPath: ancestorDetected.path,
|
||||||
})
|
legacyPath: ancestorDetected.legacyPath,
|
||||||
}
|
})
|
||||||
return migrateLegacyAndResolveCanonicalPath(ancestorPath)
|
}
|
||||||
})
|
return resolveConfigPathAfterLegacyMigration(ancestorPath)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
// Load user config first (base). Parse empty config through Zod to apply field defaults.
|
// Load user config first (base). Parse empty config through Zod to apply field defaults.
|
||||||
const userConfig = loadConfigFromPath(userConfigPath, ctx)
|
const userConfig = loadConfigFromPath(userConfigPath, ctx)
|
||||||
@@ -289,12 +294,11 @@ export function loadPluginConfig(
|
|||||||
let config: OhMyOpenCodeConfig =
|
let config: OhMyOpenCodeConfig =
|
||||||
userConfig ?? OhMyOpenCodeConfigSchema.parse({});
|
userConfig ?? OhMyOpenCodeConfigSchema.parse({});
|
||||||
|
|
||||||
// Merge ancestor configs from farthest to nearest, so closer overrides farther.
|
const canonicalAncestorPathsFarthestFirst = [...canonicalAncestorPathsNearestFirst].reverse()
|
||||||
// Walker returns nearest-first; reverse for merge order.
|
|
||||||
const defaultGitMaster = OhMyOpenCodeConfigSchema.parse({}).git_master
|
const defaultGitMaster = OhMyOpenCodeConfigSchema.parse({}).git_master
|
||||||
const ancestorGitMasterOverrides: Array<Record<string, unknown>> = []
|
const ancestorGitMasterOverridesFarthestFirst: Array<Record<string, unknown>> = []
|
||||||
|
|
||||||
for (const ancestorPath of canonicalAncestorPaths.slice().reverse()) {
|
for (const ancestorPath of canonicalAncestorPathsFarthestFirst) {
|
||||||
const ancestorConfig = loadConfigFromPath(ancestorPath, ctx)
|
const ancestorConfig = loadConfigFromPath(ancestorPath, ctx)
|
||||||
const ancestorOverrides = loadExplicitGitMasterOverrides(ancestorPath)
|
const ancestorOverrides = loadExplicitGitMasterOverrides(ancestorPath)
|
||||||
|
|
||||||
@@ -314,19 +318,21 @@ export function loadPluginConfig(
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (ancestorOverrides) {
|
if (ancestorOverrides) {
|
||||||
ancestorGitMasterOverrides.push(ancestorOverrides)
|
ancestorGitMasterOverridesFarthestFirst.push(ancestorOverrides)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if (userGitMasterOverrides || ancestorGitMasterOverrides.length > 0) {
|
if (userGitMasterOverrides || ancestorGitMasterOverridesFarthestFirst.length > 0) {
|
||||||
|
const mergedAncestorGitMaster: Record<string, unknown> = {}
|
||||||
|
for (const override of ancestorGitMasterOverridesFarthestFirst) {
|
||||||
|
Object.assign(mergedAncestorGitMaster, override)
|
||||||
|
}
|
||||||
config = {
|
config = {
|
||||||
...config,
|
...config,
|
||||||
git_master: {
|
git_master: {
|
||||||
...defaultGitMaster,
|
...defaultGitMaster,
|
||||||
...(userGitMasterOverrides ?? {}),
|
...(userGitMasterOverrides ?? {}),
|
||||||
// Ancestors are pushed far-to-near; Object.assign with an empty seed
|
...mergedAncestorGitMaster,
|
||||||
// applies each in order so the nearest (last) wins.
|
|
||||||
...Object.assign({}, ...ancestorGitMasterOverrides),
|
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user