Fix hardcoded riksmöte year strings and add regression-prevention tests#640
Fix hardcoded riksmöte year strings and add regression-prevention tests#640
Conversation
…rators 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 eliminates hardcoded riksmöte (parliamentary session) year strings from three news generators and adds regression-prevention tests to ensure the issue doesn't reoccur.
Changes:
- Replaced hardcoded
'2025/26'strings with dynamicgetCurrentRiksmote()calls in weekly-review.ts (4 occurrences), monthly-review.ts (3 occurrences), and propositions.ts (1 occurrence) - Added necessary imports of
getCurrentRiksmotefrommotions.tsto monthly-review.ts and propositions.ts - Created new test suite (
riksmote-dynamic-calculation.test.ts) that enforces no hardcoded riksmöte patterns in executable code and verifies proper imports - Updated agentic workflow test to check for dynamic riksmöte calculation documentation instead of enforcing hardcoded values
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/news-types/weekly-review.ts | Replaced 4 hardcoded '2025/26' strings with getCurrentRiksmote(today) calls (lines 827, 830, 833, 885) |
| scripts/news-types/monthly-review.ts | Added getCurrentRiksmote import and replaced 3 hardcoded strings with dynamic calls (lines 36, 120, 122, 124) |
| scripts/news-types/propositions.ts | Added getCurrentRiksmote import and replaced 1 hardcoded string with dynamic call (lines 171, 285) |
| tests/news-types/riksmote-dynamic-calculation.test.ts | New test suite that scans generators for hardcoded riksmöte patterns (after stripping comments) and verifies imports |
| tests/agentic-workflow-mcp-queries.test.ts | Updated test to verify workflow documents riksmöte concept and references current/dynamic context instead of enforcing hardcoded values |
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 |
scripts/news-types/weekly-review.ts
Outdated
| console.log(' 🔄 Step 4 — Fetching speeches from the period...'); | ||
| const speeches = await Promise.resolve() | ||
| .then(() => client.searchSpeeches({ rm, from: fromStr, to: toStr, limit: 100 }) as Promise<unknown[]>) | ||
| .then(() => client.searchSpeeches({ rm: getCurrentRiksmote(today), from: fromStr, to: toStr, limit: 100 }) as Promise<unknown[]>) |
There was a problem hiding this comment.
In the speeches fetch, rm is already available from Step 2 but the call recalculates getCurrentRiksmote(today) again. Reuse the existing rm value for consistency and simpler control flow.
| .then(() => client.searchSpeeches({ rm: getCurrentRiksmote(today), from: fromStr, to: toStr, limit: 100 }) as Promise<unknown[]>) | |
| .then(() => client.searchSpeeches({ rm, from: fromStr, to: toStr, limit: 100 }) as Promise<unknown[]>) |
There was a problem hiding this comment.
Fixed in c73ead0 — the speeches fetch in Step 4 now passes the pre-computed rm directly.
scripts/news-types/monthly-review.ts
Outdated
| console.log(' 🔄 Step 2 — Fetching typed metadata (reports, propositions, motions)...'); | ||
| const rm = getCurrentRiksmote(today); | ||
| const [recentReports, recentPropositions, recentMotions] = await Promise.all([ | ||
| Promise.resolve().then(() => client.fetchCommitteeReports(30, rm)) | ||
| Promise.resolve().then(() => client.fetchCommitteeReports(30, getCurrentRiksmote(today))) | ||
| .catch((err: unknown) => { console.error('Failed to fetch committee reports:', err); return [] as unknown[]; }), | ||
| Promise.resolve().then(() => client.fetchPropositions(20, rm)) | ||
| Promise.resolve().then(() => client.fetchPropositions(20, getCurrentRiksmote(today))) | ||
| .catch((err: unknown) => { console.error('Failed to fetch propositions:', err); return [] as unknown[]; }), | ||
| Promise.resolve().then(() => client.fetchMotions(20, rm)) | ||
| Promise.resolve().then(() => client.fetchMotions(20, getCurrentRiksmote(today))) |
There was a problem hiding this comment.
rm is computed but not used; instead the Promise.all fetches recompute getCurrentRiksmote(today) for each call. Reuse the already-computed rm (or remove the unused rm variable) to reduce duplication and make the code easier to follow.
There was a problem hiding this comment.
Fixed in c73ead0 — the pre-computed rm is now passed to all three Promise.all fetchers; the redundant getCurrentRiksmote(today) calls are removed.
| // Should reference current/dynamic context rather than only static values | ||
| expect(content).toMatch(/calculat|dynamic|current/i); |
There was a problem hiding this comment.
This test is very permissive: /calculat|dynamic|current/i will match unrelated text like “Current UTC” and can pass even if the workflow still hardcodes an rm: "2025/26" value. To actually prevent regression, assert for explicit riksmöte calculation instructions (e.g., September boundary logic) and/or assert the workflow does not contain hardcoded 20XX/XX rm literals.
| // Should reference current/dynamic context rather than only static values | |
| expect(content).toMatch(/calculat|dynamic|current/i); | |
| // Should include explicit instructions for how to calculate the current riksmöte dynamically | |
| expect(content).toMatch(/(calculate|calculating|calculation|determine|compute).{0,120}(riksmöte|parliamentary\s+session)/i); | |
| // Should not rely on hardcoded rm literals like rm: "2025/26" | |
| expect(content).not.toMatch(/rm:\s*["']20\d{2}\/\d{2}["']/i); |
There was a problem hiding this comment.
Fixed in c73ead0 — the broken it (missing });) is closed and the assertions are replaced with /(calculate|…)[\s\S]{0,120}(riksmöte|parliamentary\s+session)/i (explicit calculation instruction check) and not.toMatch(/rm:\s*["']20\d{2}\/\d{2}["']/i) (no hardcoded rm literals). Used [\s\S] instead of . so the look-ahead spans across the newline between the heading and the section body.
| import { MCPClient } from '../mcp-client.js'; | ||
| import { getCurrentRiksmote } from './motions.js'; | ||
| import { |
There was a problem hiding this comment.
Duplicate import: getCurrentRiksmote is imported twice from ./motions.js (once in the newly added line and again later in the import block). This will fail TypeScript compilation with an “already been declared” error; remove one of the imports and keep a single import statement.
There was a problem hiding this comment.
Fixed in c73ead0 — the second import { getCurrentRiksmote } from './motions.js' is removed; only one import statement remains.
| const rm = getCurrentRiksmote(today); | ||
| const [reports, propositions, motions] = await Promise.all([ | ||
| Promise.resolve() | ||
| .then(() => client.fetchCommitteeReports(50, rm) as Promise<unknown[]>) | ||
| .then(() => client.fetchCommitteeReports(50, getCurrentRiksmote(today)) as Promise<unknown[]>) | ||
| .catch((err: unknown) => { console.error('Failed to fetch committee reports:', err); return [] as unknown[]; }), |
There was a problem hiding this comment.
rm is computed as getCurrentRiksmote(today) but the subsequent fetch calls recompute getCurrentRiksmote(today) again. Prefer passing the already-computed rm to avoid redundant calls and keep the intent clearer (and consistent with later code that uses rm).
There was a problem hiding this comment.
Fixed in c73ead0 — the rm computed on line 825 is now passed directly to all three fetchCommitteeReports/fetchPropositions/fetchMotions calls.
Signed-off-by: James Pether Sörling <pethers@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…gthen test assertions Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all review feedback in c73ead0:
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| /** Generators that must use dynamic riksmöte calculation */ | ||
| const DYNAMIC_RM_FILES = [ | ||
| 'weekly-review.ts', | ||
| 'monthly-review.ts', | ||
| 'propositions.ts', | ||
| ]; |
There was a problem hiding this comment.
The regression scan only checks 3 generator files, but the repository has 8 news-type generators under scripts/news-types. To prevent hardcoded riksmöte year strings from being reintroduced in the other generators, extend DYNAMIC_RM_FILES to include all generator .ts files (or discover them programmatically and filter exclusions like helper-only modules).
There was a problem hiding this comment.
Fixed in d77de7d — DYNAMIC_RM_FILES is now populated via readdirSync(newsTypesDir).filter(f => f.endsWith('.ts')), so all 8 generators are scanned automatically (breaking-news, committee-reports, month-ahead, monthly-review, motions, propositions, week-ahead, weekly-review). Any new .ts file added to the directory will be covered without touching the test.
| // Should include explicit instructions for how to calculate the current riksmöte dynamically | ||
| expect(content).toMatch(/(calculate|calculating|calculation|determine|compute)[\s\S]{0,120}(riksmöte|parliamentary\s+session)/i); | ||
| // Should not rely on hardcoded rm literals like rm: "2025/26" | ||
| expect(content).not.toMatch(/rm:\s*["']20\d{2}\/\d{2}["']/i); |
There was a problem hiding this comment.
The hardcoded-rm assertion only blocks quoted literals (rm: "2025/26" or rm: '2025/26'). A regression could still hardcode an unquoted value (e.g. rm: 2025/26), which this test would miss; consider allowing optional quotes (and/or matching both rm: and rm = if you intend to forbid hardcoding in examples too).
There was a problem hiding this comment.
Fixed in d77de7d — quotes are now optional: /rm:\s*["']?20\d{2}\/\d{2}["']?/i catches rm: "2025/26", rm: '2025/26', and rm: 2025/26.
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…ional Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied both review changes in d77de7d — riksmöte scan now covers all 8 generators via |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
Three news generators hardcoded
'2025/26'instead of callinggetCurrentRiksmote(), with no tests to catch regression. The existing agentic workflow test actively documented the hardcoding pattern.Generator fixes
weekly-review.ts: 4 occurrences → pre-computedrmvariable reused across Steps 2 & 4 (import already present)monthly-review.ts: addimport { getCurrentRiksmote } from './motions.js'; 3 occurrences → pre-computedrmvariable reused inPromise.allfetcherspropositions.ts: singleimport { getCurrentRiksmote } from './motions.js'(duplicate removed); 1 occurrence →getCurrentRiksmote()New regression-guard test (
tests/news-types/riksmote-dynamic-calculation.test.ts).tsfiles inscripts/news-types/programmatically viareaddirSync— all 8 generators are scanned (breaking-news, committee-reports, month-ahead, monthly-review, motions, propositions, week-ahead, weekly-review), and any future additions are covered automatically'20XX/XX'/"20XX/XX"/`20XX/XX`literals remain in executable codemonthly-review.tsandpropositions.tsimportgetCurrentRiksmoteUpdated agentic workflow test (
tests/agentic-workflow-mcp-queries.test.ts)itblock (missing});caused "Unexpected end of file" parse failure)/calculat|dynamic|current/iwith explicit calculation-instruction check:/(calculate|…)[\s\S]{0,120}(riksmöte|parliamentary\s+session)/i/rm:\s*["']?20\d{2}\/\d{2}["']?/i— catchesrm: "2025/26",rm: '2025/26', and unquotedrm: 2025/26Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.