-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: auto-detect and prevent conflicts with external DCP/ELF plugins #558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,17 +62,25 @@ import { | |
| import { BackgroundManager } from "./features/background-agent"; | ||
| import { SkillMcpManager } from "./features/skill-mcp-manager"; | ||
| import { type HookName } from "./config"; | ||
| import { log } from "./shared"; | ||
| import { log, detectConflictingPlugins, warnAboutConflicts } from "./shared"; | ||
| import { loadPluginConfig } from "./plugin-config"; | ||
| import { createModelCacheState, getModelLimit } from "./plugin-state"; | ||
| import { createConfigHandler } from "./plugin-handlers"; | ||
|
|
||
| const OhMyOpenCodePlugin: Plugin = async (ctx) => { | ||
| // Start background tmux check immediately | ||
| startTmuxCheck(); | ||
|
|
||
| const pluginConfig = loadPluginConfig(ctx.directory, ctx); | ||
| const disabledHooks = new Set(pluginConfig.disabled_hooks ?? []); | ||
|
|
||
| const conflictDetection = detectConflictingPlugins(ctx.directory); | ||
| const autoDisabledHooks = new Set(conflictDetection.hooksToDisable); | ||
| const userDisabledHooks = new Set(pluginConfig.disabled_hooks ?? []); | ||
| const disabledHooks = new Set([...autoDisabledHooks, ...userDisabledHooks]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Logic error: Users cannot re-enable hooks that were auto-disabled. The current implementation merges both sets (auto-disabled + user-disabled), so Prompt for AI agents |
||
|
|
||
| if (conflictDetection.hasConflicts) { | ||
| warnAboutConflicts(conflictDetection.conflicts, conflictDetection.hooksToDisable); | ||
| } | ||
|
|
||
| const isHookEnabled = (hookName: HookName) => !disabledHooks.has(hookName); | ||
|
|
||
| const modelCacheState = createModelCacheState(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,160 @@ | ||||||
| import * as fs from "fs"; | ||||||
| import * as path from "path"; | ||||||
| import { log } from "./logger"; | ||||||
| import { getUserConfigDir } from "./config-path"; | ||||||
| import { parseJsonc } from "./jsonc-parser"; | ||||||
| import type { HookName } from "../config"; | ||||||
|
|
||||||
| interface OpenCodeConfig { | ||||||
| plugin?: string[]; | ||||||
| [key: string]: unknown; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Plugins that conflict with oh-my-opencode's built-in features | ||||||
| */ | ||||||
| const CONFLICTING_PLUGINS = { | ||||||
| // DCP (Dynamic Context Pruning) plugins | ||||||
| dcp: [ | ||||||
| "opencode-dcp", | ||||||
| "@opencode/dcp", | ||||||
| "opencode-dynamic-context-pruning", | ||||||
| ], | ||||||
| // ELF (External Language Framework / Context management) plugins | ||||||
| elf: ["opencode-elf", "@opencode/elf", "opencode-context"], | ||||||
| // Compaction plugins | ||||||
| compaction: [ | ||||||
| "opencode-compaction", | ||||||
| "@opencode/compaction", | ||||||
| "opencode-auto-compact", | ||||||
| ], | ||||||
| } as const; | ||||||
|
|
||||||
| /** | ||||||
| * Hooks that should be auto-disabled when conflicting plugins are detected | ||||||
| */ | ||||||
| const HOOKS_TO_DISABLE_ON_CONFLICT: Record<string, HookName[]> = { | ||||||
| dcp: [ | ||||||
| "anthropic-context-window-limit-recovery", | ||||||
| "preemptive-compaction", | ||||||
| "compaction-context-injector", | ||||||
| ], | ||||||
| elf: [ | ||||||
| "context-window-monitor", | ||||||
| "directory-agents-injector", | ||||||
| "directory-readme-injector", | ||||||
| ], | ||||||
| compaction: [ | ||||||
| "preemptive-compaction", | ||||||
| "anthropic-context-window-limit-recovery", | ||||||
| "compaction-context-injector", | ||||||
| ], | ||||||
| }; | ||||||
|
|
||||||
| /** | ||||||
| * Loads the main OpenCode config to detect installed plugins | ||||||
| */ | ||||||
| function loadOpencodeConfig(directory: string): OpenCodeConfig { | ||||||
| const projectConfigPaths = [ | ||||||
| path.join(directory, "opencode.json"), | ||||||
| path.join(directory, "opencode.jsonc"), | ||||||
| ]; | ||||||
|
|
||||||
| const userConfigPaths = [ | ||||||
| path.join(getUserConfigDir(), "opencode", "opencode.json"), | ||||||
| path.join(getUserConfigDir(), "opencode", "opencode.jsonc"), | ||||||
| ]; | ||||||
|
|
||||||
| const paths = [...projectConfigPaths, ...userConfigPaths]; | ||||||
|
|
||||||
| for (const configPath of paths) { | ||||||
| try { | ||||||
| if (fs.existsSync(configPath)) { | ||||||
| const content = fs.readFileSync(configPath, "utf-8"); | ||||||
| const config = parseJsonc<OpenCodeConfig>(content); | ||||||
| if (config.plugin && Array.isArray(config.plugin)) { | ||||||
| log(`Loaded OpenCode config from ${configPath}`, { | ||||||
| pluginCount: config.plugin.length, | ||||||
| }); | ||||||
| return config; | ||||||
| } | ||||||
| } | ||||||
| } catch (err) { | ||||||
| log(`Error loading OpenCode config from ${configPath}:`, err); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return {}; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Detects if any conflicting plugins are installed | ||||||
| */ | ||||||
| export function detectConflictingPlugins( | ||||||
| directory: string | ||||||
| ): { | ||||||
| hasConflicts: boolean; | ||||||
| conflicts: string[]; | ||||||
| hooksToDisable: HookName[]; | ||||||
| } { | ||||||
| const config = loadOpencodeConfig(directory); | ||||||
| const installedPlugins = config.plugin ?? []; | ||||||
|
|
||||||
| const conflicts: string[] = []; | ||||||
| const hooksToDisable = new Set<HookName>(); | ||||||
|
|
||||||
| for (const [type, pluginNames] of Object.entries(CONFLICTING_PLUGINS)) { | ||||||
| for (const pluginName of pluginNames) { | ||||||
| const hasPlugin = installedPlugins.some( | ||||||
| (p) => | ||||||
| p === pluginName || p.startsWith(`${pluginName}@`) || p.includes(pluginName) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/shared/plugin-compatibility.ts
Line: 110:110
Comment:
**logic:** The `p.includes(pluginName)` check is too broad - will match unrelated plugins. For example, if `pluginName` is `"dcp"`, this will match plugins like `"my-dcp-custom"`, `"cdcp-handler"`, or `"opencode-backup-dcp"`. Should use more precise matching.
```suggestion
p === pluginName || p.startsWith(`${pluginName}@`)
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: The Prompt for AI agents |
||||||
| ); | ||||||
|
|
||||||
| if (hasPlugin) { | ||||||
| conflicts.push(pluginName); | ||||||
| const hooksForType = HOOKS_TO_DISABLE_ON_CONFLICT[type] ?? []; | ||||||
| hooksForType.forEach((hook) => hooksToDisable.add(hook)); | ||||||
| log(`Detected conflicting plugin: ${pluginName} (type: ${type})`); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return { | ||||||
| hasConflicts: conflicts.length > 0, | ||||||
| conflicts, | ||||||
| hooksToDisable: Array.from(hooksToDisable), | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Logs warning messages about plugin conflicts | ||||||
| */ | ||||||
| export function warnAboutConflicts(conflicts: string[], hooksToDisable: HookName[]): void { | ||||||
| if (conflicts.length === 0) return; | ||||||
|
|
||||||
| console.warn("\n⚠️ oh-my-opencode: Plugin Conflict Detection"); | ||||||
| console.warn("═".repeat(60)); | ||||||
| console.warn( | ||||||
| `\nDetected ${conflicts.length} plugin(s) that may conflict with oh-my-opencode:` | ||||||
| ); | ||||||
| conflicts.forEach((plugin) => console.warn(` • ${plugin}`)); | ||||||
|
|
||||||
| console.warn( | ||||||
| `\nAuto-disabling ${hooksToDisable.length} oh-my-opencode hook(s) to prevent conflicts:` | ||||||
| ); | ||||||
| hooksToDisable.forEach((hook) => console.warn(` • ${hook}`)); | ||||||
|
|
||||||
| console.warn("\nTo manually control this behavior, add to your config:"); | ||||||
| console.warn(' ~/.config/opencode/oh-my-opencode.json'); | ||||||
| console.warn(' or'); | ||||||
| console.warn(' .opencode/oh-my-opencode.json'); | ||||||
| console.warn('\n {'); | ||||||
| console.warn(' "disabled_hooks": ['); | ||||||
| hooksToDisable.forEach((hook, i) => { | ||||||
| const comma = i < hooksToDisable.length - 1 ? "," : ""; | ||||||
| console.warn(` "${hook}"${comma}`); | ||||||
| }); | ||||||
| console.warn(' ]'); | ||||||
| console.warn(' }'); | ||||||
| console.warn("\n" + "═".repeat(60) + "\n"); | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: User cannot re-enable hooks that were auto-disabled. The current logic combines both sets, but users might want to override auto-detection (e.g., they know their DCP plugin is compatible). Consider implementing priority: if user explicitly lists a hook in
disabled_hooks, respect auto-detection; but provide a separateenabled_hooks_overrideconfig to force-enable specific hooks despite conflicts.Current behavior:
["preemptive-compaction"]disabled_hooks: []preemptive-compactionis still disabled (user cannot override)The README states "Override auto-detection by explicitly configuring
disabled_hooks" but this doesn't actually allow re-enabling auto-disabled hooks.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI