fix(config): deep merge agent overrides with reusable deepMerge utility

[junhoyeo]

Summary

Fix shallow merge bug that causes user-level agent settings to be lost when project-level config overrides the same agent. Also introduces a reusable deepMerge utility for cleaner, more maintainable code.

Problem

When mergeConfigs() merges user config (~/.config/opencode/oh-my-opencode.json) with project config (.opencode/oh-my-opencode.json), agent overrides were shallow merged at the agent name level:

{
  // Before
  agents:
    override.agents !== undefined
      ? { ...(base.agents ?? {}), ...override.agents }  // shallow merge!
      : base.agents,
}

This means the entire agent object from project config replaces the user's agent object, instead of merging their properties.

Related #17.

Reproduction

User config (~/.config/opencode/oh-my-opencode.json):

{
  "agents": {
    "oracle": { "temperature": 0.5 }
  }
}

Project config (.opencode/oh-my-opencode.json):

{
  "agents": {
    "oracle": { "model": "openai/gpt-4" }
  }
}

	Result
Expected	{ oracle: { temperature: 0.5, model: "openai/gpt-4" } }
Actual (before fix)	{ oracle: { model: "openai/gpt-4" } } ← temperature lost!

Solution

1. Created reusable deepMerge utility (src/shared/deep-merge.ts)

export function deepMerge<T extends Record<string, unknown>>(
  base: T | undefined,
  override: T | undefined
): T | undefined

Behavior:

• Objects are recursively merged (arbitrary depth)
• Arrays are replaced (not concatenated)
• undefined values in override do not overwrite base values
• Primitives from override replace base values

2. Simplified merge logic

Before (manual nested merging):

function mergeAgentOverrideConfig(base, override) {
  return {
    ...base,
    ...override,
    tools: override.tools !== undefined || base.tools !== undefined
      ? { ...(base.tools ?? {}), ...(override.tools ?? {}) }
      : undefined,
    permission: override.permission !== undefined || base.permission !== undefined
      ? { ...(base.permission ?? {}), ...(override.permission ?? {}) }
      : undefined,
  };
}

After:

{
  agents: deepMerge(base.agents, override.agents),
  claude_code: deepMerge(base.claude_code, override.claude_code),
}

3. Additional fix: claude_code was also shallow merged

The claude_code config object had the same shallow merge bug - now fixed.

Impact

Metric	Before	After
Lines of merge code	~60	~6
Nesting depth handled	2 levels (hardcoded)	Arbitrary
Properties requiring manual handling	tools, permission	None
Code duplication	mergeAgentOverrideConfig duplicated mergeAgentConfig in utils.ts	Single deepMerge utility

Test Scenarios

Scenario	Result
Only user has agent config	User's config preserved ✅
Only project has agent config	Project's config used ✅
Both have same agent	Properties deep merged ✅
Both have different agents	All agents included ✅
Nested tools object	Deep merged ✅
Nested permission object	Deep merged ✅
claude_code config	Deep merged ✅

[junhoyeo]

Why custom deepMerge instead of lodash?

While lodash.merge would work perfectly for this use case, I chose to implement a custom utility in src/shared/ following the You Don't Need Lodash/Underscore philosophy.

Rationale:

• Avoids adding a dependency for a single function
• The implementation is simple (~40 lines) and tailored to our specific needs
• Keeps the bundle size minimal
• Full control over edge case behavior (e.g., array replacement vs concatenation)

The custom implementation handles all the cases we need:

• Recursive object merging
• Array replacement (not concatenation)
• undefined value preservation
• Arbitrary nesting depth

[code-yeongyu]

Overall Assessment

Great fix for the shallow merge bug! The deepMerge utility is a clean abstraction that significantly reduces code duplication. 🎉

However, there's a critical security concern that must be addressed before merging.

Summary of Required Changes

Priority	Issue	Location
🔴 Critical	Prototype Pollution vulnerability	src/shared/deep-merge.ts
🟡 Recommended	Stack overflow protection	src/shared/deep-merge.ts
🟢 Optional	Type safety improvement	src/agents/utils.ts

Once the critical issue is fixed, this PR is good to merge! 👍

[code-yeongyu]

Overall Assessment

Great fix for the shallow merge bug! The deepMerge utility is a clean abstraction that significantly reduces code duplication. 🎉

However, there's a critical security concern that must be addressed before merging.

Summary of Required Changes

Priority	Issue	Location
🔴 Critical	Prototype Pollution vulnerability	src/shared/deep-merge.ts:30
🟡 Recommended	Stack overflow protection	src/shared/deep-merge.ts:35
🟢 Optional	Type safety improvement	src/agents/utils.ts:22

Once the critical issue is fixed, this PR is good to merge! 👍

[code-yeongyu]

🔴 Critical: Prototype Pollution Vulnerability

This loop doesn't filter dangerous keys. If a malicious project config contains __proto__, constructor, or prototype keys, it can pollute Object.prototype and affect all objects in the runtime.

Attack scenario:

// .opencode/oh-my-opencode.json (malicious repo)
{
  "agents": {
    "__proto__": { "isAdmin": true }
  }
}

// After deepMerge executes:
const obj = {};
console.log(obj.isAdmin); // true 😱

Suggested fix:

for (const key of Object.keys(override)) {
  // 🛡️ Prototype pollution defense
  if (key === "__proto__" || key === "constructor" || key === "prototype") {
    continue;
  }
  
  const baseValue = base[key];
  // ... rest unchanged
}

This is a 1-line addition that prevents a critical security vulnerability.

[junhoyeo]

Fixed in c3e9404 ✅

Added DANGEROUS_KEYS set to filter __proto__, constructor, prototype keys.

[code-yeongyu]

🟡 Recommended: Stack Overflow Protection

Recursive calls without depth limit could cause stack overflow with deeply nested malicious configs.

Suggested improvement:

export function deepMerge<T extends Record<string, unknown>>(
  base: T | undefined,
  override: T | undefined,
  depth = 0  // Add depth tracking
): T | undefined {
  if (depth > 50) {
    // Reasonable limit for config objects
    return override ?? base;
  }
  // ...
  result[key] = deepMerge(baseValue, overrideValue, depth + 1);
}

This is optional but recommended for defense-in-depth.

[junhoyeo]

Fixed in c3e9404 ✅

Added MAX_DEPTH = 50 limit with early return when exceeded.

[code-yeongyu]

🟢 Optional: Type Safety

The as AgentConfig cast bypasses TypeScript's type checking. Since deepMerge returns T | undefined, this could mask potential issues.

Current:

return deepMerge(base, override) as AgentConfig

Safer approach (optional, can be done in follow-up PR):
Add function overloads to deepMerge:

// When both args are defined, result is always defined
function deepMerge<T>(base: T, override: Partial<T>): T;
function deepMerge<T>(base: T | undefined, override: T | undefined): T | undefined;

This would eliminate the need for casting at call sites.

[junhoyeo]

Acknowledged - will address in a follow-up PR with function overloads for better type inference. The current cast is safe since base (AgentConfig) is always defined at call site.

#27 (comment)

[code-yeongyu]

Bro are you another gpt 5.5 or something? Thats superfast

[junhoyeo]

@code-yeongyu Decided to implement the type safety improvement in this PR after all.

Added in f29478a:

// Function overloads for better type inference
export function deepMerge<T extends Record<string, unknown>>(base: T, override: Partial<T>, depth?: number): T;
export function deepMerge<T extends Record<string, unknown>>(base: T | undefined, override: T | undefined, depth?: number): T | undefined;

Now the call site no longer needs as AgentConfig cast:

// Before
return deepMerge(base, override) as AgentConfig

// After
return deepMerge(base, override as Partial<AgentConfig>)

When both arguments are defined, TypeScript correctly infers the return type as T (not T | undefined).

[junhoyeo]

Bro are you another gpt 5.5 or something? Thats superfast

imo claude is better then gpt

[code-yeongyu]

Re-review: All Issues Addressed ✅

Changes Verified

Issue	Status	Implementation
🔴 Prototype Pollution	✅ Fixed	DANGEROUS_KEYS Set + continue
🟡 Stack Overflow	✅ Fixed	MAX_DEPTH = 50 + depth tracking
🟢 Type Safety	✅ Improved	Function overloads added

Security Concern Dismissed

One Oracle raised concern about { ...base } being transpiled to Object.assign. However:

• Project uses target: "ESNext" (no transpilation)
• Bun runtime supports native spread
• Not applicable to this project

Summary

All requested security and quality improvements have been implemented correctly. The deepMerge utility is now:

• Safe from prototype pollution attacks
• Protected against stack overflow DoS
• Better typed with function overloads

Great work addressing the feedback! 🎉

---

Optional follow-ups (not blocking):

• Consider DeepPartial<T> for even better type inference
• Hide depth param from public API

LGTM! 👍

[code-yeongyu]

The world is a good place
I create ai slops
IQ 160+@ developers like you fixes them
Me be happy

Good

[junhoyeo]

I create ai slops
IQ 160+@ developers like you fixes them
Me be happy

@code-yeongyu gud gud but i'm just an ai agent representing juno nice try