Conversation
…search_anforanden in motions.ts Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
This PR implements three missing MCP tools in the motions article generator to transform basic motion listings into comprehensive opposition intelligence with full-text analysis, government response tracking, and debate context.
Changes:
- Restored
REQUIRED_TOOLSfrom 1 to 4 tools with graceful degradation on failures - Added three MCP enrichment calls:
search_dokument_fulltext,analyze_g0v_by_department, andsearch_anforanden - Extended content generation with "Government Department Engagement" section and enhanced cross-references
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/news-types/motions.ts |
Implemented three MCP tools with try-catch error handling, updated REQUIRED_TOOLS to 4, expanded crossReferences sources |
scripts/data-transformers/content-generators.ts |
Added Government Department Engagement section with HTML-escaped output |
scripts/data-transformers/types.ts |
Added govDeptData field to ArticleContentData interface |
scripts/types/content.ts |
Added govEngagement field to ContentLabelSet interface |
scripts/data-transformers/constants.ts |
Added govEngagement label translations for all 14 languages |
tests/news-types/motions.test.ts |
Extended mock client with request and searchSpeeches, added 8 new tests for tool calls and graceful degradation |
Comments suppressed due to low confidence (1)
scripts/news-types/motions.ts:333
- The
generateSourcesfunction inscripts/data-transformers/metadata.tsdoesn't have mappings for the three new tools:search_dokument_fulltext,analyze_g0v_by_department, andsearch_anforanden. While the tools are correctly called and their results used in article generation, the article footer's "Data Sources" section won't include human-readable names for these enrichment sources. Consider adding mappings like: "Full-text Document Analysis", "Government Department Tracking", and "Parliamentary Debate Speeches" to improve transparency about data sources used in the intelligence analysis.
const sources: string[] = generateSources([
'get_motioner',
'search_dokument_fulltext',
'analyze_g0v_by_department',
'search_anforanden',
]);
| mockClientInstance.searchSpeeches.mockRejectedValueOnce(new Error('Tool unavailable')); | ||
| const result = await motionsModule.generateMotions({ languages: ['en'] }); | ||
| expect(result.success).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for analyze_g0v_by_department graceful degradation. The implementation calls client.request() twice (for search_dokument_fulltext at line 257 and analyze_g0v_by_department at line 279), but there's no test verifying that failure of the second tool call is handled gracefully. The test at line 160 only covers the first request call failure using mockRejectedValueOnce. Add a test that specifically verifies analyze_g0v_by_department failure doesn't break article generation.
| }); | |
| }); | |
| it('should degrade gracefully when analyze_g0v_by_department fails', async () => { | |
| const originalRequestImpl = mockClientInstance.request.getMockImplementation(); | |
| // First call (search_dokument_fulltext) uses the existing implementation | |
| mockClientInstance.request.mockImplementationOnce( | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (...args: any[]) => originalRequestImpl && originalRequestImpl(...args) | |
| ); | |
| // Second call (analyze_g0v_by_department) fails | |
| mockClientInstance.request.mockRejectedValueOnce(new Error('Tool unavailable')); | |
| const result = await motionsModule.generateMotions({ languages: ['en'] }); | |
| expect(result.success).toBe(true); | |
| }); |
There was a problem hiding this comment.
Added a dedicated test 'should degrade gracefully when analyze_g0v_by_department fails' in commit 1ef7492. It uses mockResolvedValueOnce({}) for the first request call (search_dokument_fulltext) and mockRejectedValueOnce(new Error(...)) for the second (analyze_g0v_by_department), confirming the article still generates successfully when that tool fails.
scripts/news-types/motions.ts
Outdated
| try { | ||
| const debateQuery = motions[0]?.titel || motions[0]?.title || ''; | ||
| if (debateQuery) { | ||
| const speeches = await client.searchSpeeches({ text: debateQuery, rm: '2025/26', limit: 10 }) as Array<Record<string, unknown>>; |
There was a problem hiding this comment.
Hardcoded riksmöte value '2025/26' will become outdated when the parliamentary year changes (typically September/October). The Swedish riksmöte follows the pattern YYYY/YY where the year changes each September. Consider extracting a utility function to calculate the current riksmöte dynamically based on the current date, or at minimum document when this value needs to be updated. This same hardcoded value appears in monthly-review.ts and weekly-review.ts, indicating a broader maintainability concern across the codebase.
There was a problem hiding this comment.
Fixed in commit 1ef7492. Added getCurrentRiksmote(date) utility that computes the session string dynamically (month >= 8 ? year : year - 1, since getMonth() is 0-based so September = 8). The search_anforanden call now uses getCurrentRiksmote() instead of the hardcoded '2025/26'.
|
@copilot apply changes based on the comments in this thread |
…öte calculation Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
| }); | ||
| } | ||
| } catch (err) { | ||
| console.warn(' ⚠ search_dokument_fulltext unavailable:', (err as Error).message); |
There was a problem hiding this comment.
In the search_dokument_fulltext catch block, the tool failure is swallowed without recording the tool in mcpCalls. This can cause cross-reference validation to report missing required tools, and the article can still list the tool in sources even though it didn’t produce data. Record the attempted call in mcpCalls with an empty result (and optionally an error marker) when the request fails, or derive sources from the actual mcpCalls list.
| console.warn(' ⚠ search_dokument_fulltext unavailable:', (err as Error).message); | |
| console.warn(' ⚠ search_dokument_fulltext unavailable:', (err as Error).message); | |
| // Record attempted tool call with empty result so validation and sources stay consistent | |
| mcpCalls.push({ tool: 'search_dokument_fulltext', result: [] }); |
There was a problem hiding this comment.
Fixed in ca4ea6f. The catch block now pushes { tool: 'search_dokument_fulltext', result: [] } to mcpCalls so validation and source attribution stay consistent even on failure.
| mcpCalls.push({ tool: 'analyze_g0v_by_department', result: govDeptData }); | ||
| console.log(` 🏛 Gov dept analysis: ${govDeptData.length} departments`); | ||
| } catch (err) { | ||
| console.warn(' ⚠ analyze_g0v_by_department unavailable:', (err as Error).message); |
There was a problem hiding this comment.
analyze_g0v_by_department failures currently only log a warning and don’t add an entry to mcpCalls. Since this tool is in REQUIRED_TOOLS, missing the record can fail cross-reference validation even though generation returns success. Add a fallback mcpCalls entry (empty array/result + optional error info) in the catch path so downstream validation and reporting remain consistent.
| console.warn(' ⚠ analyze_g0v_by_department unavailable:', (err as Error).message); | |
| const error = err as Error; | |
| console.warn(' ⚠ analyze_g0v_by_department unavailable:', error.message); | |
| // Fallback MCP call record to keep REQUIRED_TOOLS cross-reference consistent | |
| mcpCalls.push({ | |
| tool: 'analyze_g0v_by_department', | |
| result: [], | |
| error: { | |
| message: error.message, | |
| name: error.name, | |
| }, | |
| } as MCPCallRecord); |
There was a problem hiding this comment.
Fixed in ca4ea6f. The catch block now pushes { tool: 'analyze_g0v_by_department', result: [] } to keep mcpCalls consistent with REQUIRED_TOOLS.
| } | ||
| } | ||
| } catch (err) { | ||
| console.warn(' ⚠ search_anforanden unavailable:', (err as Error).message); |
There was a problem hiding this comment.
search_anforanden failures are not recorded in mcpCalls, but the tool remains required and listed as a source. To keep validation and source attribution accurate during graceful degradation, add an mcpCalls entry in the catch path (e.g., empty speeches array and/or captured error message).
| console.warn(' ⚠ search_anforanden unavailable:', (err as Error).message); | |
| console.warn(' ⚠ search_anforanden unavailable:', (err as Error).message); | |
| mcpCalls.push({ tool: 'search_anforanden', result: [] }); |
There was a problem hiding this comment.
Fixed in ca4ea6f. The catch block now pushes { tool: 'search_anforanden', result: [] } to mcpCalls.
scripts/news-types/motions.ts
Outdated
| ftDocs.forEach((ftDoc, i) => { | ||
| const m = motions[i] as Record<string, unknown>; | ||
| if (m && !m['fullText']) { | ||
| m['fullText'] = ftDoc.fullText || ftDoc.summary || ''; | ||
| } | ||
| }); |
There was a problem hiding this comment.
search_dokument_fulltext is queried once using only the first motion’s title, but the results are then assigned to motions by index and written into each motion’s fullText field. This can easily attach unrelated text to other motions and pollute downstream summaries/policy analysis. Consider keeping these documents as a separate related-docs collection, or do per-motion lookup / match on dok_id (when available) rather than index-based assignment.
| ftDocs.forEach((ftDoc, i) => { | |
| const m = motions[i] as Record<string, unknown>; | |
| if (m && !m['fullText']) { | |
| m['fullText'] = ftDoc.fullText || ftDoc.summary || ''; | |
| } | |
| }); | |
| const primaryMotion = motions[0] as Record<string, unknown> | undefined; | |
| if (primaryMotion && ftDocs.length > 0) { | |
| // Attach the most relevant full text only to the motion that was used for the query | |
| if (!primaryMotion['fullText']) { | |
| const bestDoc = ftDocs[0] as Record<string, unknown>; | |
| primaryMotion['fullText'] = (bestDoc['fullText'] as string) || (bestDoc['summary'] as string) || ''; | |
| } | |
| // Optionally expose all related documents for downstream "Policy Alternative" rendering | |
| if (!primaryMotion['policyAlternativeDocs']) { | |
| primaryMotion['policyAlternativeDocs'] = ftDocs; | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in ca4ea6f. Replaced the forEach index loop with a targeted assignment: only motions[0] (the motion used for the query) receives fullText from ftDocs[0]. All related docs are exposed on primaryMotion['policyAlternativeDocs'] when multiple results are returned.
| if (deptName) { | ||
| content += deptCount |
There was a problem hiding this comment.
deptCount is checked using a truthy test, so a legitimate value of 0 will be treated as “missing” and won’t render in parentheses. Prefer checking for null/undefined (e.g., deptCount !== null && deptCount !== undefined) so 0 still displays correctly.
| if (deptName) { | |
| content += deptCount | |
| const hasDeptCount = deptCount !== null && deptCount !== undefined; | |
| if (deptName) { | |
| content += hasDeptCount |
There was a problem hiding this comment.
Fixed in ca4ea6f. Changed to const hasDeptCount = deptCount !== null && deptCount !== undefined so a value of 0 renders correctly in parentheses.
| it('should degrade gracefully when search_dokument_fulltext fails', async () => { | ||
| mockClientInstance.request.mockRejectedValueOnce(new Error('Tool unavailable')); | ||
| const result = await motionsModule.generateMotions({ languages: ['en'] }); | ||
| expect(result.success).toBe(true); | ||
| }); | ||
|
|
||
| it('should degrade gracefully when analyze_g0v_by_department fails', async () => { | ||
| // First request call (search_dokument_fulltext) succeeds; second (analyze_g0v_by_department) fails | ||
| mockClientInstance.request | ||
| .mockResolvedValueOnce({}) | ||
| .mockRejectedValueOnce(new Error('Tool unavailable')); | ||
| const result = await motionsModule.generateMotions({ languages: ['en'] }); | ||
| expect(result.success).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Current tests assert generation succeeds when enrichment tools fail, but they don’t verify that the failed tool is still recorded in mcpCalls (which downstream cross-reference validation relies on). Add assertions in the graceful-degradation tests to ensure mcpCalls includes the failed tool name with an empty result/error marker.
There was a problem hiding this comment.
Fixed in ca4ea6f. All three graceful-degradation tests now assert result.mcpCalls?.map(c => c.tool) contains the failed tool name, verifying the empty-result fallback entry is recorded.
|
@copilot apply changes based on the comments in this thread |
…ignment, fix deptCount null check Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Signed-off-by: James Pether Sörling <pethers@users.noreply.github.com>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
scripts/data-transformers/types.ts:101
- The ArticleContentData interface now has two fields for essentially the same data from analyze_g0v_by_department:
govDeptData(line 97): An array of department records, used by motions.tsdepartmentAnalysis(line 101): A single object containing a 'departments' property, used by propositions.ts
This creates an inconsistent API where different news types use different field names and shapes for the same MCP tool response. Consider standardizing on one field name and shape across all news types. For example, consolidate to use govDeptData as the direct array (since it's simpler), and update propositions.ts to extract the array from the response before passing it to generateArticleContent.
/** Government department analysis from analyze_g0v_by_department */
govDeptData?: Record<string, unknown>[];
/** Full-text search results for policy substance extraction */
fullTextResults?: unknown[];
/** Government department analysis from analyze_g0v_by_department */
departmentAnalysis?: Record<string, unknown>;
REQUIRED_TOOLSinscripts/news-types/motions.tsto include all 4 toolssearch_dokument_fulltextcall with graceful error handlinganalyze_g0v_by_departmentcall with graceful error handlingsearch_anforandencall with graceful error handlingfullTextresults to motions for "Policy Alternative" renderingspeechesresults to motions for "Party Positioning" renderinggovDeptDataoptional field toArticleContentDatainscripts/data-transformers/types.tsgenerateMotionsContentinscripts/data-transformers/content-generators.tsgovEngagementlabel to all 14 languages inscripts/data-transformers/constants.tsgovEngagementtoContentLabelSetinterface inscripts/types/content.tstests/news-types/motions.test.tsmock to includerequestandsearchSpeechesanalyze_g0v_by_departmentgraceful degradation'2025/26'riksmöte withgetCurrentRiksmote()utility functionmcpCallswith empty result in all three catch blocks (comments #2862867925, #2862867944, #2862867954)fullTextassignment — attach only to primary motion used for query (comment #2862867967)deptCounttruthy check to!== null && !== undefinedso0renders correctly (comment #2862867980)mcpCallsassertions to all three graceful-degradation tests (comment #2862867990)Original prompt
This section details on the original issue you should resolve
<issue_title>Improve Motions: Add full-text analysis, government response tracking, and debate context</issue_title>
<issue_description>## 📋 Issue Type
Enhancement — Implement missing MCP tools for opposition motions: full-text analysis, government response tracking, and debate context
🎯 Objective
Implement the three TODO MCP tools in
scripts/news-types/motions.ts:search_dokument_fulltext(full motion text analysis),analyze_g0v_by_department(government department response tracking), andsearch_anforanden(debate context and party positioning). These tools will transform motion articles from basic listings into strategic opposition intelligence.📊 Current State
scripts/news-types/motions.ts(403 lines)get_motioner✅ (only tool)search_dokument_fulltext❌,analyze_g0v_by_department❌,search_anforanden❌get_motioner) — reduced from original 4 to avoid validation failures🚀 Desired State
🔧 Implementation Approach
1. Add
search_dokument_fulltextintegration (~80 lines)scripts/data-transformers/document-analysis.tsfor opposition strategy detection2. Add
analyze_g0v_by_departmentintegration (~60 lines)3. Add
search_anforandenintegration (~60 lines)4. Update REQUIRED_TOOLS
Restore full 4-tool list:
get_motioner,search_dokument_fulltext,analyze_g0v_by_department,search_anforanden5. Enhanced article templates
🤖 Recommended Agent
code-quality-engineer— TypeScript implementation matching existing MCP integration patterns✅ Acceptance Criteria
search_dokument_fulltextintegrated for policy alternative extractionanalyze_g0v_by_departmentintegrated for government response trackingsearch_anforandenintegrated for debate context and party positioning📚 References
scripts/news-types/motions.ts:57-60(TODO comments)scripts/news-types/motions.ts:189-196(REQUIRED_TOOLS update comment)search_dokument_fulltext,analyze_g0v_by_department,search_anforandenscripts/data-transformers/document-analysis.ts(opposition strategy detection)scripts/data-transformers/policy-analysis.tsscripts/news-types/weekly-review.ts(full enrichment pipeline example)</issue_description>
<agent_i...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.