Conversation
Add CI workflow for PR/push, create tsconfig.test.json with allowImportingTsExtensions, fix mock data types, add VS Code settings
…ta + text Add typed output interfaces, update 127 tests, update all docs, add GitHub Actions CI
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 4 of the Mastertour MCP server, adding venue research capabilities and completing a major architecture refactoring. The PR adds 3 new venue-focused tools, introduces structured output types across all 12 tools, implements dependency injection for better testability, and establishes CI/CD with GitHub Actions.
Key Changes:
- Added venue research tools:
search_past_venues,get_venue_details,get_upcoming_shows - Refactored to use dependency injection pattern with singleton client instance
- Introduced structured
ToolResult<T>return type for all tools providing both typed data and human-readable text - Created shared utilities: formatters module and tour iterator for code reuse
- Established GitHub Actions CI workflow with Node 20.x/22.x matrix testing
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/config.ts |
New configuration module with fail-fast validation for environment variables |
src/auth/oauth.ts |
Refactored OAuth client to support explicit credential injection |
src/index.ts |
Refactored server initialization to use dependency injection pattern |
src/types/outputs.ts |
New structured output type definitions for all tool return values |
src/utils/formatters.ts |
Shared formatting utilities (dates, fields, contacts, search normalization) |
src/utils/tourIterator.ts |
Async generator for iterating tours/days with filtering |
src/tools/searchPastVenues.ts |
New tool for searching venues from historical tours |
src/tools/getVenueDetails.ts |
New tool for retrieving complete venue information |
src/tools/getUpcomingShows.ts |
New tool for listing upcoming shows across tours |
src/tools/*.ts (updates) |
All existing tools updated to return ToolResult<T> structured outputs |
src/api/client.ts |
Added getTourAll and getDayEvents methods for venue research |
tests/**/*.test.ts |
Updated imports from .js to .ts, assertions changed to use result.text |
.github/workflows/ci.yml |
New GitHub Actions CI workflow |
vitest.config.ts |
Added typecheck configuration |
tsconfig.test.json |
New TypeScript config for test files |
docs/*.md |
Updated documentation reflecting Phase 4 completion and architecture changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/unit/searchPastVenues.test.ts
Outdated
| }, | ||
| }; | ||
|
|
||
| const mockDayEvents: Record<string, any[]> = { |
There was a problem hiding this comment.
Using any[] for the type defeats TypeScript's type safety. Consider using a more specific type like DayEvent[] or creating a proper mock type.
| const contacts: VenueContactOutput[] = extractContacts(venueEvent.venueContacts); | ||
|
|
||
| const data: VenueDetailsOutput = { | ||
| dayId: venueId, // Using venueId as identifier |
There was a problem hiding this comment.
Same issue as in searchPastVenues: the dayId field is being assigned venueId instead of an actual day ID. This creates confusion about what the field represents. Consider renaming to venueId in the output type or storing the actual day ID.
| dayId: venueId, // Using venueId as identifier | |
| dayId: venueEvent.dayId, |
| * reducing code duplication in venue research tools. | ||
| */ | ||
|
|
||
| import type { MasterTourClient, TourInfo, EventDayInfo, DayEvent, TourAllResponse } from '../api/client.js'; |
There was a problem hiding this comment.
The import uses .js extension for TypeScript files, but this might be inconsistent with other imports in the codebase. Consider whether imports should consistently use .ts extensions for TypeScript source files, especially given that test files have been updated to use .ts extensions.
| import type { MasterTourClient, TourInfo, EventDayInfo, DayEvent, TourAllResponse } from '../api/client.js'; | |
| import type { MasterTourClient, TourInfo, EventDayInfo, DayEvent, TourAllResponse } from '../api/client.ts'; |
|
|
||
| // Build structured data | ||
| const outputResults: VenueSearchResultOutput[] = results.map((v) => ({ | ||
| dayId: v.venueId, // Using venueId as dayId since we don't track specific days |
There was a problem hiding this comment.
The dayId field is being misused here - it's assigned the venueId instead of an actual day ID. This creates a misleading API where the field name doesn't match the data. Consider either renaming the field in VenueSearchResultOutput to venueId, or storing the actual day ID where this venue was found.
| dayId: v.venueId, // Using venueId as dayId since we don't track specific days | |
| dayId: '', // No specific day ID is tracked for these aggregated venue results |
| expect(mockClient.getTourSummary).toHaveBeenCalledWith('tour123', today); | ||
| }); | ||
| }); | ||
| });; |
There was a problem hiding this comment.
There's a double semicolon at the end of this line. Remove one semicolon.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| MASTERTOUR_CONSUMER_KEY: test-key | ||
| MASTERTOUR_CONSUMER_SECRET: test-secret | ||
| MASTERTOUR_ACCESS_TOKEN: test-token | ||
| MASTERTOUR_TOKEN_SECRET: test-secret |
There was a problem hiding this comment.
The environment variables in the CI workflow don't match the actual configuration used by the application. The code uses MASTERTOUR_KEY and MASTERTOUR_SECRET (from config.ts), but the CI workflow sets MASTERTOUR_CONSUMER_KEY, MASTERTOUR_CONSUMER_SECRET, MASTERTOUR_ACCESS_TOKEN, and MASTERTOUR_TOKEN_SECRET. This mismatch will cause CI tests to fail if they rely on environment configuration.
| MASTERTOUR_CONSUMER_KEY: test-key | |
| MASTERTOUR_CONSUMER_SECRET: test-secret | |
| MASTERTOUR_ACCESS_TOKEN: test-token | |
| MASTERTOUR_TOKEN_SECRET: test-secret | |
| MASTERTOUR_KEY: test-key | |
| MASTERTOUR_SECRET: test-secret |
[WIP] Update to address feedback on Feature/first use case PR
adds all that we can actually accomplish for a reasonable TM use case. includes refactors and cleanups as we move into semi-functional mode. also adds basic CI to run tests with each PR.