refactor(core): Extract shared utilities and remove code duplication (Vibe Kanban)

[gabe4coding]

Summary

This PR simplifies the @mcp-apps-kit/core package by extracting shared utilities and removing code duplication across adapters and plugins.

Changes Made

1. Extracted Shared buildAnnotations Function

• Files: adapters/mcp.ts, adapters/openai.ts, utils/metadata.ts
• Both MCP and OpenAI adapters had identical buildAnnotations private methods
• Extracted to a shared utility function in utils/metadata.ts
• Exported from the public API for potential external use

2. Consolidated safeStringify Utility

• Files: plugins/builtin/logging.ts, debug/logger.ts
• The logging plugin had its own implementation of safeStringify and getCircularReplacer
• Now imports from the existing debug/logger.ts implementation
• Eliminates ~30 lines of duplicate code

3. Cleaned Up Unused Imports

• File: events/types.ts
• Removed unused _ToolDefs import alias

4. Consolidated Import Statements

• File: utils/csp.ts
• Moved scattered UIDef import to the top with other imports
• Follows consistent code style conventions

Why These Changes?

• DRY Principle: Both adapters were maintaining identical annotation-building logic
• Maintainability: Single source of truth for shared functionality
• Consistency: Changes to annotation handling now only need to happen in one place
• Code Quality: Removes 44 lines of duplicate code (96 deletions → 52 additions)

Verification

• ✅ All 600 tests pass
• ✅ Lint passes with no errors
• ✅ TypeScript typecheck passes
• ✅ Build succeeds
• ✅ Public API backward compatible

---

This PR was written using Vibe Kanban

[coderabbitai]

Warning

Rate limit exceeded

@gabe4coding has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 26 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 91a8ac4 and 6e87ad3.

📒 Files selected for processing (7)

• packages/core/src/adapters/mcp.ts
• packages/core/src/adapters/openai.ts
• packages/core/src/events/types.ts
• packages/core/src/index.ts
• packages/core/src/plugins/builtin/logging.ts
• packages/core/src/utils/csp.ts
• packages/core/src/utils/metadata.ts

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #73: Extract Shared Utilities and Remove Code Duplication

Summary

This is an excellent refactoring PR that follows the DRY principle and improves code maintainability. The changes are well-structured, backward compatible, and properly tested.

---

✅ Strengths

1. Effective Code Deduplication

• buildAnnotations extraction (utils/metadata.ts:120-143): Excellent consolidation of identical annotation-building logic from both MCP and OpenAI adapters
• safeStringify consolidation (plugins/builtin/logging.ts): Smart reuse of existing implementation from debug/logger.ts
• Net reduction: 96 deletions vs 51 additions = 45 lines removed

2. Good Design Decisions

• ✅ Placed buildAnnotations in utils/metadata.ts - the right home for protocol-agnostic metadata utilities
• ✅ Exported from public API (index.ts:154) - allows external consumers to reuse if needed
• ✅ Marked as @internal - signals implementation detail while still being accessible
• ✅ Both MCP and OpenAI use identical annotation names - perfect candidate for sharing

3. Test Coverage

• ✅ Existing tests already cover buildAnnotations behavior through adapter tests (adapters.test.ts:110-149, 324-353)
• ✅ safeStringify is well-tested in logger.test.ts:57-86
• ✅ All 600 tests pass according to PR description

4. Clean Import Management

• ✅ Removed unused _ToolDefs import (events/types.ts:7)
• ✅ Consolidated UIDef import to top of file (utils/csp.ts:11)
• ✅ Updated adapter imports to use shared function (adapters/mcp.ts:11, adapters/openai.ts:11)

---

🔍 Code Quality Assessment

Type Safety: ✅ Excellent

• Proper TypeScript types throughout
• Uses Record<string, unknown> appropriately for annotation objects
• Optional parameter handling with proper undefined checks

Performance: ✅ No concerns

• No performance regression - same logic, just relocated
• Annotation building is a lightweight operation

Security: ✅ No concerns

• No security-sensitive changes
• Safe handling of undefined/null values

Backward Compatibility: ✅ Fully compatible

• Public API surface unchanged (only additions)
• Tool behavior remains identical
• Existing consumers unaffected

---

📝 Minor Observations

1. Export Strategy (Not an issue, just a note)

The buildAnnotations function is marked @internal but exported from the public API. This is intentional per the pattern in the codebase, but consider:

• Current approach (recommended): Export allows advanced users to reuse utilities if building custom adapters
• Alternative: Keep truly internal by not exporting - would require consumers to duplicate if needed

Recommendation: Keep as-is. The @internal JSDoc tag signals intent while maintaining flexibility.

2. safeStringify Behavioral Difference

The logger implementation (debug/logger.ts:109-139) handles more edge cases than the previous plugin implementation:

• ✅ Explicit undefined handling (return "undefined")
• ✅ String pass-through optimization
• ✅ Error object serialization with stack traces
• ✅ [Unstringifiable] fallback instead of String(obj)

This is actually an improvement - the logging plugin now benefits from more robust stringification.

---

🎯 Recommendations

High Priority: None ✅

All changes are production-ready.

Optional Enhancements (for future PRs):

1. Type Refinement: Consider a stricter return type for buildAnnotations:

   export function buildAnnotations(
     annotations?: ToolAnnotations
   ): Omit<ToolAnnotations, undefined> | undefined

   Though Record<string, unknown> is fine for the current use case.

2. Documentation: Add a brief comment in adapters explaining why buildAnnotations is shared:

   // Both MCP and OpenAI use identical annotation property names per spec
   const annotations = buildAnnotations(toolDef.annotations);

---

🧪 Testing Verification

Based on the code analysis:

• ✅ Adapter tests cover annotation building (adapters.test.ts lines 110-149 for MCP, 324-353 for OpenAI)
• ✅ Logger tests cover safeStringify (logger.test.ts lines 57-86)
• ✅ Integration tests ensure end-to-end behavior unchanged
• ✅ PR description confirms all checks pass

---

✨ Final Verdict

APPROVED - This is a high-quality refactoring that:

• Eliminates code duplication
• Improves maintainability
• Maintains backward compatibility
• Follows project conventions per CLAUDE.md
• Has comprehensive test coverage

The changes align perfectly with the project's code quality standards. Great work! 🎉

---

📊 Metrics

• Lines changed: +51 / -96 (net -45)
• Files modified: 7
• Test coverage: Maintained
• Breaking changes: None
• Security issues: None

---

Recommendation: Merge when ready ✅