Fix: context config graph id

[miles-kt-inkeep]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Ready	Preview	Comment	Oct 6, 2025 4:16pm
agents-manage-api	Ready	Preview	Comment	Oct 6, 2025 4:16pm
agents-manage-ui	Ready	Preview	Comment	Oct 6, 2025 4:16pm
agents-run-api	Ready	Preview	Comment	Oct 6, 2025 4:16pm

[claude]

Claude finished @miles-kt-inkeep's task —— View job

---

Code Review: Fix context config graph id

I'll provide a comprehensive code review focusing on technical quality, architectural considerations, and system impact.

Review Progress:

• Read repository context and patterns from CLAUDE.md
• Analyze changed files to understand the modifications
• Research related code and existing patterns
• Evaluate technical quality and architectural consistency
• Consider customer impact and breaking changes
• Provide comprehensive feedback and recommendations

---

Summary

This PR fixes a bug in how the graphId is propagated to context configurations. The issue was that when setting the context on a ContextConfigBuilder, the method was passing apiUrl instead of graphId as the third parameter, and the ContextConfig.setContext method signature wasn't properly updated to match this fix.

Technical Analysis

Changes Overview:

1. ContextConfig.ts (lines 185-209): Updated setContext method signature and implementation to properly handle graphId parameter
2. graph.ts (lines 154-156): Fixed the method call to pass this.graphId instead of apiUrl
3. Changeset: Added proper versioning for the bug fix

Technical Quality: ✅ GOOD

Strengths:

• Correct Parameter Mapping: The fix properly aligns method signature with usage pattern
• Database Consistency: Ensures graphId is correctly stored in the context config database record via this.config.graphId = graphId
• Logging Improvements: Updated logging to include the new graphId parameter for better debugging
• Type Safety: Maintains TypeScript type safety throughout the changes

Areas for Improvement:

🔴 CRITICAL: Missing Test Coverage

• Issue: The changes affect the setContext method which is a core configuration method, but there are no updated tests covering the new signature
• Risk: Future regressions could break the graphId propagation without detection
• Recommendation: Add unit tests in packages/agents-core/src/__tests__/context/ covering:

  describe('ContextConfigBuilder.setContext', () => {
    it('should properly set tenantId, projectId, and graphId', () => {
      const builder = contextConfig({ id: 'test', name: 'test' });
      builder.setContext('tenant1', 'project1', 'graph1');
      
      const obj = builder.toObject();
      expect(obj.tenantId).toBe('tenant1');
      expect(obj.projectId).toBe('project1'); 
      expect(obj.graphId).toBe('graph1');
    });
  });

🟡 MODERATE: Documentation Comment Inconsistency

• Issue: Comment on line 188 mentions "Set the context (tenantId, projectId, graphId)" but previously mentioned "baseURL"
• Fix: The comment was correctly updated, this is actually good
• Note: This is handled correctly in the PR

Architectural Considerations: ✅ GOOD

Consistency with Existing Patterns:

• Multi-tenant Architecture: Changes maintain proper tenant/project/graph scoping patterns used throughout the codebase
• Builder Pattern: Maintains the established builder pattern for context configuration
• Database Schema Alignment: The toObject() method correctly populates graphId field which aligns with the database schema (ContextConfigSelect)

Design Quality:

• Parameter Order: The method signature setContext(tenantId, projectId, graphId) follows a logical hierarchy (tenant > project > graph)
• State Management: Properly updates both instance variables and the config object for consistency
• Separation of Concerns: Configuration propagation is handled appropriately by the graph calling setContext

System Impact Analysis: ✅ LOW RISK

Breaking Changes: None

• This is an internal bug fix that corrects broken functionality
• No public API changes that would affect consumers

Data Flow Impact: ✅ Positive

• Before: Context configs were not getting correct graphId, potentially causing issues in multi-graph scenarios
• After: Context configs will be properly associated with their parent graph

Performance: No Impact

• Changes are minimal and don't introduce any performance concerns

Customer Impact: ✅ POSITIVE

User Experience:

• Bug Fix: Resolves potential issues where context configurations weren't properly associated with graphs
• Multi-tenancy: Improves reliability in multi-tenant scenarios where graphId disambiguation is critical
• No Regression Risk: Changes are backward compatible

Edge Cases Handled:

• The fix ensures context configs created in different graphs within the same project are properly distinguished
• Database queries filtering by graphId will now work correctly

Code Quality Assessment

Readability: ✅ Good

• Method signature is clear and self-documenting
• Parameter names follow established conventions
• Logging provides adequate debugging information

Maintainability: ⚠️ Needs Test Coverage

• Code is simple and maintainable
• Missing test coverage is the primary concern for long-term maintenance

Recommendations

🔴 HIGH PRIORITY - Add Tests

// Add to packages/agents-core/src/__tests__/context/ContextConfig.test.ts
describe('setContext method', () => {
  it('should update tenantId, projectId, and graphId', () => {
    const builder = contextConfig({ 
      id: 'test-config', 
      name: 'Test Config' 
    });
    
    builder.setContext('tenant-123', 'project-456', 'graph-789');
    
    const result = builder.toObject();
    expect(result.tenantId).toBe('tenant-123');
    expect(result.projectId).toBe('project-456');
    expect(result.graphId).toBe('graph-789');
  });

  it('should update internal state and config object consistently', () => {
    const builder = contextConfig({ id: 'test', name: 'test' });
    builder.setContext('t1', 'p1', 'g1');
    
    // Test that both internal state and config object are updated
    expect(builder.toObject().graphId).toBe('g1');
  });
});

🟢 LOW PRIORITY - Consider Integration Test

Add an integration test ensuring the graph properly propagates context to context configs:

// In packages/agents-sdk/src/__tests__/graph.test.ts
it('should propagate graphId to context config during setConfig', async () => {
  const contextConfig = mockContextConfig();
  const graph = agentGraph({ 
    id: 'test-graph',
    contextConfig,
    // ... other config
  });
  
  graph.setConfig('tenant1', 'project1', 'http://api');
  
  expect(contextConfig.setContext).toHaveBeenCalledWith(
    'tenant1', 
    'project1', 
    'test-graph'
  );
});

Conclusion

This is a well-executed bug fix that addresses a real issue in the codebase. The changes are minimal, focused, and maintain architectural consistency. The main gap is missing test coverage which should be addressed to prevent future regressions.

Approval Status: ✅ Approved with Test Coverage Requirement

The fix correctly resolves the graphId propagation issue and maintains system integrity. Once test coverage is added, this will be a solid improvement to the codebase reliability.

---