feat: Implement get_voting_group and search_ledamoter in breaking-news generator#605
feat: Implement get_voting_group and search_ledamoter in breaking-news generator#605
Conversation
… to breaking-news Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
github-actions
bot
added
testing
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
This pull request implements two missing MCP tool integrations (get_voting_group and search_ledamoter) in the breaking news article generator to eliminate validation warnings and enrich articles with party-level voting breakdowns and MP profile context.
Changes:
- Added
fetchVotingGroup()MCP client method for party-level voting analysis - Integrated both new tools into
generateBreakingNews()with conditional calling based on event data (voteId triggers voting tools, topic triggers speech/MP tools) - Updated type definitions, exports, and tests to support the new functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/types/mcp.ts |
Added FetchVotingGroupParams interface for get_voting_group tool parameters |
scripts/mcp-client/client.ts |
Implemented fetchVotingGroup() method with dual response key handling (groups/votes) |
scripts/mcp-client/index.ts |
Added fetchVotingGroup export wrapper for default client |
scripts/mcp-client.ts |
Added fetchVotingGroup to barrel exports |
scripts/news-types/breaking-news.ts |
Integrated all 4 MCP tools conditionally; updated REQUIRED_TOOLS comments |
tests/news-types/breaking-news.test.ts |
Added test coverage for new fetchVotingGroup and fetchMPs methods |
scripts/news-types/breaking-news.ts
Outdated
| // Fetch related votes and party breakdown if event involves a vote | ||
| if (eventData.voteId) { | ||
| console.log(' 🔄 Fetching voting details...'); | ||
| const votes: unknown = await client.fetchVotingRecords({ punkt: eventData.voteId }); | ||
| mcpCalls.push({ tool: 'search_voteringar', result: votes }); | ||
|
|
||
| // Fetch party-level voting breakdown | ||
| console.log(' 🔄 Fetching party voting breakdown...'); | ||
| const votingGroup: unknown = await client.fetchVotingGroup({ punkt: eventData.voteId, groupBy: 'parti' }); | ||
| mcpCalls.push({ tool: 'get_voting_group', result: votingGroup }); | ||
| } | ||
|
|
||
| // Example: Fetch related speeches | ||
| // Fetch related speeches and MP context if topic provided | ||
| if (eventData.topic) { | ||
| console.log(' 🔄 Fetching related speeches...'); | ||
| const speeches: unknown = await client.searchSpeeches({ sok: eventData.topic }); | ||
| mcpCalls.push({ tool: 'search_anforanden', result: speeches }); | ||
|
|
||
| // Fetch MP profiles for speaker context using names from speeches | ||
| console.log(' 🔄 Fetching MP profiles for speaker context...'); | ||
| const speechList = Array.isArray(speeches) ? speeches as Array<Record<string, unknown>> : []; | ||
| const speakerName = speechList[0]?.['talare'] as string | undefined; | ||
| const mps: unknown = await client.fetchMPs(speakerName ? { namn: speakerName, limit: 1 } : { limit: 5 }); | ||
| mcpCalls.push({ tool: 'search_ledamoter', result: mps }); | ||
| } |
There was a problem hiding this comment.
The conditional MCP tool calls will cause cross-reference validation failures. The validation system (scripts/validate-cross-references.ts) requires ALL tools listed in REQUIRED_TOOLS_PER_TYPE to be called for the article to pass validation (line 129: allRequiredToolsUsed = missingTools.length === 0).
When only voteId is provided, search_anforanden and search_ledamoter won't be called. When only topic is provided, search_voteringar and get_voting_group won't be called. This causes validation to fail.
Following the established pattern from motions.ts (lines 266-332), propositions.ts (lines 252-289), week-ahead.ts (lines 285-320), and monthly-review.ts (lines 114-120), all required tools should be called unconditionally, wrapping them in try/catch blocks or .catch() handlers to gracefully handle failures while still recording the MCP call with an empty result.
Recommended approach: Always call all 4 tools, using reasonable default parameters when voteId/topic are not provided, and wrap each in try/catch to push empty results on failure. This maintains validation compliance while preserving the enrichment-only nature of the data.
There was a problem hiding this comment.
Fixed in c8aa415. All 4 tools are now called unconditionally, each in its own try/catch block. When voteId/topic are absent, reasonable defaults are used (e.g. { limit: 5 }). Each catch block pushes an empty result array to mcpCalls so cross-reference validation always sees all 4 tools recorded.
scripts/news-types/breaking-news.ts
Outdated
| // Fetch related votes and party breakdown if event involves a vote | ||
| if (eventData.voteId) { | ||
| console.log(' 🔄 Fetching voting details...'); | ||
| const votes: unknown = await client.fetchVotingRecords({ punkt: eventData.voteId }); | ||
| mcpCalls.push({ tool: 'search_voteringar', result: votes }); | ||
|
|
||
| // Fetch party-level voting breakdown | ||
| console.log(' 🔄 Fetching party voting breakdown...'); | ||
| const votingGroup: unknown = await client.fetchVotingGroup({ punkt: eventData.voteId, groupBy: 'parti' }); | ||
| mcpCalls.push({ tool: 'get_voting_group', result: votingGroup }); | ||
| } | ||
|
|
||
| // Example: Fetch related speeches | ||
| // Fetch related speeches and MP context if topic provided | ||
| if (eventData.topic) { | ||
| console.log(' 🔄 Fetching related speeches...'); | ||
| const speeches: unknown = await client.searchSpeeches({ sok: eventData.topic }); | ||
| mcpCalls.push({ tool: 'search_anforanden', result: speeches }); | ||
|
|
||
| // Fetch MP profiles for speaker context using names from speeches | ||
| console.log(' 🔄 Fetching MP profiles for speaker context...'); | ||
| const speechList = Array.isArray(speeches) ? speeches as Array<Record<string, unknown>> : []; | ||
| const speakerName = speechList[0]?.['talare'] as string | undefined; | ||
| const mps: unknown = await client.fetchMPs(speakerName ? { namn: speakerName, limit: 1 } : { limit: 5 }); | ||
| mcpCalls.push({ tool: 'search_ledamoter', result: mps }); | ||
| } |
There was a problem hiding this comment.
The MCP enrichment calls lack error handling. If fetchVotingGroup, fetchMPs, fetchVotingRecords, or searchSpeeches fail, the entire article generation will fail rather than gracefully degrading.
Following the established pattern from motions.ts (lines 266-332) which wraps each enrichment call in try/catch and pushes empty results on failure, these calls should have individual error handling. This allows articles to be generated even when some data sources are temporarily unavailable.
Wrap each MCP call block in try/catch, logging warnings on failure and pushing empty results to mcpCalls to maintain traceability.
There was a problem hiding this comment.
Fixed in c8aa415. Each of the 4 MCP calls is now wrapped in its own try/catch. On failure, a warning is logged and an empty result is pushed to mcpCalls, matching the pattern in motions.ts.
scripts/news-types/breaking-news.ts
Outdated
| // Fetch related speeches and MP context if topic provided | ||
| if (eventData.topic) { | ||
| console.log(' 🔄 Fetching related speeches...'); | ||
| const speeches: unknown = await client.searchSpeeches({ sok: eventData.topic }); |
There was a problem hiding this comment.
The searchSpeeches call uses parameter name 'sok' which is not defined in SearchSpeechesParams interface (scripts/types/mcp.ts:64-72). Other files in the codebase use 'text' parameter for search queries (see motions.ts:310, propositions.ts:284).
While 'sok' is technically allowed due to the [key: string]: unknown index signature, using the established 'text' parameter name would be more consistent with the rest of the codebase and clearer for future maintainers. Consider changing { sok: eventData.topic } to { text: eventData.topic } to match the pattern used in motions.ts and propositions.ts.
| const speeches: unknown = await client.searchSpeeches({ sok: eventData.topic }); | |
| const speeches: unknown = await client.searchSpeeches({ text: eventData.topic }); |
There was a problem hiding this comment.
Fixed in c8aa415. Changed to { text: eventData.topic, limit: 10 } to match the pattern used in motions.ts and propositions.ts.
|
@copilot apply changes based on the comments in this thread |
…king-news Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| }); | ||
|
|
||
| expect(mockClientInstance.fetchMPs).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test suite lacks coverage for the unconditional tool calling pattern. All existing tests provide either voteId or topic, but none verify that all 4 required tools are still called when only a minimal eventData (e.g., just slug) is provided.
Consider adding a test case like:
it('should call all 4 required tools even with minimal event data', async () => {
const eventData: BreakingEventData = { slug: 'test' }; // No voteId, no topic
await breakingNewsModule.generateBreakingNews({
languages: ['en'],
eventData
});
expect(mockClientInstance.fetchVotingRecords).toHaveBeenCalled();
expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalled();
expect(mockClientInstance.searchSpeeches).toHaveBeenCalled();
expect(mockClientInstance.fetchMPs).toHaveBeenCalled();
});This would explicitly verify the unconditional calling behavior required for cross-reference validation compliance.
| }); | |
| }); | |
| it('should call all tools even with minimal event data', async () => { | |
| const eventData: BreakingEventData = { | |
| slug: 'test' | |
| }; | |
| await breakingNewsModule.generateBreakingNews({ | |
| languages: ['en'], | |
| eventData | |
| }); | |
| expect(mockClientInstance.fetchVotingRecords).toHaveBeenCalled(); | |
| expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalled(); | |
| expect(mockClientInstance.searchSpeeches).toHaveBeenCalled(); | |
| expect(mockClientInstance.fetchMPs).toHaveBeenCalled(); | |
| }); |
There was a problem hiding this comment.
Added in fb8366d: should call all 4 required tools even with minimal event data passes a { slug: 'test' } event (no voteId, no topic) and asserts that all four mock methods — fetchVotingRecords, fetchVotingGroup, searchSpeeches, fetchMPs — were called.
| it('should fetch voting group if voteId provided', async () => { | ||
| const eventData: BreakingEventData = { | ||
| voteId: 'v123', | ||
| slug: 'test' | ||
| }; | ||
|
|
||
| await breakingNewsModule.generateBreakingNews({ | ||
| languages: ['en'], | ||
| eventData | ||
| }); | ||
|
|
||
| expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should fetch MP profiles if topic provided', async () => { | ||
| const eventData: BreakingEventData = { | ||
| topic: 'Budget debate', | ||
| slug: 'test' | ||
| }; | ||
|
|
||
| await breakingNewsModule.generateBreakingNews({ | ||
| languages: ['en'], | ||
| eventData | ||
| }); | ||
|
|
||
| expect(mockClientInstance.fetchMPs).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test titles "should fetch voting group if voteId provided" and "should fetch MP profiles if topic provided" are misleading because the implementation actually calls all 4 tools unconditionally (lines 230-273 in breaking-news.ts). The tools are called with enriched parameters when voteId/topic are available, but they're always called with fallback defaults otherwise.
Consider either:
- Renaming tests to reflect unconditional behavior (e.g., "should always fetch voting group (enriched when voteId provided)")
- Adding an explicit test that verifies all 4 tools are called even when only slug is provided (neither voteId nor topic), which would better demonstrate the unconditional calling pattern required for validation.
There was a problem hiding this comment.
Fixed in fb8366d. Renamed the two tests to should always fetch voting group (enriched with voteId when provided) and should always fetch MP profiles (enriched with speaker name when topic provided), and added the explicit unconditional test as suggested.
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
scripts/news-types/breaking-news.ts
Outdated
| * **IMPLEMENTATION STATUS:** | ||
| * - Actual implementation calls: search_voteringar, search_anforanden | ||
| * - TODO implementation: get_voting_group, search_ledamoter | ||
| * Note: This causes validation warnings but allows tests to pass. Full | ||
| * implementation should complete all four tool integrations. | ||
| * - All four MCP tools implemented: search_voteringar, get_voting_group, | ||
| * search_anforanden, search_ledamoter (all conditional on event data) |
There was a problem hiding this comment.
The header doc comment says the MCP tools are "all conditional on event data", but the implementation below calls all 4 tools unconditionally (using fallback params when voteId/topic are missing) to satisfy cross-reference validation. Please update this status note to reflect the unconditional calling behavior (and that enrichment happens when event data is present).
There was a problem hiding this comment.
Fixed in 89c3753. The header status note now reads: "all called unconditionally; parameters are enriched when voteId/topic are present in event data".
scripts/news-types/breaking-news.ts
Outdated
| * - search_voteringar: ✅ Implemented (conditional on voteId) | ||
| * - get_voting_group: ✅ Implemented (conditional on voteId) | ||
| * - search_anforanden: ✅ Implemented (conditional on topic) | ||
| * - search_ledamoter: ✅ Implemented (conditional on topic) |
There was a problem hiding this comment.
This IMPLEMENTATION STATUS block states each tool is conditional on voteId/topic, but the code now calls all tools regardless (with defaults) and only enriches parameters when voteId/topic are present. Updating these bullets will prevent future maintainers from reintroducing conditional calls that would break validation.
| * - search_voteringar: ✅ Implemented (conditional on voteId) | |
| * - get_voting_group: ✅ Implemented (conditional on voteId) | |
| * - search_anforanden: ✅ Implemented (conditional on topic) | |
| * - search_ledamoter: ✅ Implemented (conditional on topic) | |
| * - search_voteringar: ✅ Implemented (always called; parameters enriched when voteId is present) | |
| * - get_voting_group: ✅ Implemented (always called; parameters enriched when voteId is present) | |
| * - search_anforanden: ✅ Implemented (always called; parameters enriched when topic is present) | |
| * - search_ledamoter: ✅ Implemented (always called; parameters enriched when topic is present) |
There was a problem hiding this comment.
Fixed in 89c3753. All four bullets now say "always called; parameters enriched when voteId/topic is present".
| eventData | ||
| }); | ||
|
|
||
| expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalled(); |
There was a problem hiding this comment.
This test name claims the voting-group call is enriched with voteId, but it only asserts that the method was called. To actually verify the enrichment behavior, assert the call parameters (e.g., that fetchVotingGroup was called with punkt=voteId and groupBy='parti') and/or that the fallback params are used when voteId is absent.
| expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalled(); | |
| expect(mockClientInstance.fetchVotingGroup).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| punkt: eventData.voteId, | |
| groupBy: 'parti' | |
| }) | |
| ); |
There was a problem hiding this comment.
Fixed in 89c3753. The assertion now uses toHaveBeenCalledWith(expect.objectContaining({ punkt: eventData.voteId, groupBy: 'parti' })) to verify the enrichment parameters.
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…rt fetchVotingGroup params Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
scripts/types/mcp.ts
Outdated
| /** Parameters for party-level voting group lookup */ | ||
| export interface FetchVotingGroupParams { | ||
| bet?: string; | ||
| punkt?: string; | ||
| groupBy?: 'parti' | 'valkrets' | 'namn'; | ||
| rm?: string; | ||
| limit?: number; | ||
| [key: string]: unknown; | ||
| } |
There was a problem hiding this comment.
FetchVotingGroupParams breaks the existing naming pattern in this file: other fetch* methods use *Filters (e.g. FetchMPsFilters, FetchVotingFilters) while search* uses *Params. To keep the public API consistent and easier to discover, consider renaming this to FetchVotingGroupFilters (and updating the MCPClient method signature/imports accordingly).
There was a problem hiding this comment.
Fixed in 4812ca3. Renamed FetchVotingGroupParams → FetchVotingGroupFilters in scripts/types/mcp.ts and updated the import/usage in scripts/mcp-client/client.ts to match the *Filters naming convention used by other fetch* methods.
scripts/news-types/breaking-news.ts
Outdated
| * IMPLEMENTATION STATUS: | ||
| * - search_voteringar: ✅ Implemented (always called; parameters enriched when voteId is present) | ||
| * - get_voting_group: ✅ Implemented (always called; parameters enriched when voteId is present) | ||
| * - search_anforanden: ✅ Implemented (always called; parameters enriched when topic is present) | ||
| * - search_ledamoter: ✅ Implemented (always called; parameters enriched when topic is present) |
There was a problem hiding this comment.
The doc comment says search_ledamoter parameters are enriched when topic is present, but the code actually enriches fetchMPs only when a speakerName can be derived from the search_anforanden results (speechList[0]['talare']). Please update the status bullets (and the header status note above) to describe the real enrichment condition to avoid misleading future changes.
There was a problem hiding this comment.
Fixed in 4812ca3. Both the header status note and the IMPLEMENTATION STATUS bullet for search_ledamoter now accurately say "parameters enriched when a speaker name is found in search_anforanden results" (not "when topic is present").
|
|
||
| await breakingNewsModule.generateBreakingNews({ | ||
| languages: ['en'], | ||
| eventData | ||
| }); | ||
|
|
||
| expect(mockClientInstance.fetchMPs).toHaveBeenCalled(); |
There was a problem hiding this comment.
This test claims MP lookup is "enriched with speaker name", but it only asserts fetchMPs was called. Since the mock searchSpeeches currently resolves to [], the implementation will take the fallback path ({ limit: 5 }) and never exercise the namn enrichment. Either rename the test to remove the enrichment claim, or set searchSpeeches to return a speech with talare and assert fetchMPs was called with { namn: <speaker>, limit: 1 }.
| await breakingNewsModule.generateBreakingNews({ | |
| languages: ['en'], | |
| eventData | |
| }); | |
| expect(mockClientInstance.fetchMPs).toHaveBeenCalled(); | |
| const mockSpeakerName = 'Jane Doe'; | |
| mockClientInstance.searchSpeeches.mockResolvedValueOnce([ | |
| { | |
| talare: mockSpeakerName | |
| } as unknown as Record<string, unknown> | |
| ]); | |
| await breakingNewsModule.generateBreakingNews({ | |
| languages: ['en'], | |
| eventData | |
| }); | |
| expect(mockClientInstance.fetchMPs).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| namn: mockSpeakerName, | |
| limit: 1 | |
| }) | |
| ); |
There was a problem hiding this comment.
Fixed in 4812ca3. The test now mocks searchSpeeches to return [{ talare: 'Jane Doe' }], then asserts fetchMPs was called with expect.objectContaining({ namn: 'Jane Doe', limit: 1 }), actually exercising the enrichment path.
|
@copilot apply changes based on the comments in this thread |
…est MP enrichment path Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
FetchVotingGroupFilterstype toscripts/types/mcp.ts(renamed fromFetchVotingGroupParamsto match*Filtersnaming pattern)fetchVotingGroupmethod toscripts/mcp-client/client.tsfetchVotingGroupfromscripts/mcp-client/index.tsand barrelmcp-client.tsscripts/news-types/breaking-news.ts:textparameter forsearchSpeeches(matches codebase pattern)tests/news-types/breaking-news.test.ts:should call all 4 required tools even with minimal event datafetchVotingGroupassertstoHaveBeenCalledWith({ punkt: voteId, groupBy: 'parti' })searchSpeechesto return a speech withtalareand assertsfetchMPscalled with{ namn, limit: 1 }Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.