Skip to content

Conversation

@MQ37
Copy link
Contributor

@MQ37 MQ37 commented Oct 31, 2025

closes #115

This bothered me for a long time and decided to give it a go today. I believe these code changes make the codebase more maintainable and simple. This was partially vibe coded.

Changes:

  • got rid of the wrapper tool type
  • use only specific types based in the type discriminator - no nesting of the tool object
  • simplified code where I could

MQ37 added 4 commits October 31, 2025 14:19
- Convert ToolEntry to discriminated union with explicit type variants
  - HelperToolEntry: type: 'internal'
  - ActorToolEntry: type: 'actor'
  - ActorMcpToolEntry: type: 'actor-mcp'
- Remove actorFullName from all internal tools (11 tools across 9 files)
- Remove unused ToolType export
- Improves type safety and enables automatic type narrowing in TypeScript
- Remove wrapper object pattern (HelperToolEntry, ActorToolEntry, ActorMcpToolEntry)
- Implement flat discriminated union: ToolEntry = HelperTool | ActorTool | ActorMcpTool
- Add direct 'type' discriminator to tool interfaces (no nested .tool property)
- Introduce McpInputSchema type for strict MCP SDK schema alignment
- Replace all 'as any' type casts with proper McpInputSchema assertions
- Update 16 tool files to use flat structure pattern
- Update server.ts and proxy.ts to remove .tool property access
- Fix tools-loader.ts and tools.ts utility functions
- Update test files to reference flat structure (.name instead of .tool.name)
- Fix runtime error: serverUrl access in actor MCP tool handling

This refactoring improves:
- Type safety: Flat union eliminates property access ambiguity
- Code clarity: Direct tool properties instead of nested .tool wrapper
- MCP alignment: Proper inputSchema type matching MCP SDK requirements
- No 'any' types: Complete type safety throughout codebase
@github-actions github-actions bot added t-ai Issues owned by the AI team. tested Temporary label used only programatically for some analytics. labels Oct 31, 2025
@MQ37 MQ37 requested a review from jirispilka October 31, 2025 23:07
Copy link
Collaborator

@jirispilka jirispilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice! Thanks!

I only check couple of tools as the rest of the changes were quite similar (for other tools)

Consider renaming McpInputSchema. This is not descriptive IMO

@MQ37 MQ37 merged commit 80aed10 into master Nov 7, 2025
4 checks passed
@MQ37 MQ37 deleted the simplify-tool-type branch November 7, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-ai Issues owned by the AI team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Tool interface typing

2 participants