fix: strengthen data quality safeguards in refresh-stats workflow#362
fix: strengthen data quality safeguards in refresh-stats workflow#362
Conversation
- Add significant-drop guard (>50% decrease from stored → reject as incomplete) - Fix countItems() partial data bug — return error field on pagination errors - Remove Promise.all() in Plenary Documents to avoid EP API rate limits - Skip all-zero monthly distributions and gate monthly data by credibility check - Revert corrupted 2025 data (speeches 1998→10000, documents 930→3516) - Revert corrupted 2026 data (parliamentaryQuestions 1355→6147, documents 285→4265) - Restore 2025 NI seats to 34 (consistent with mepCount 720) - Remove corrupted monthly data (all-zero procedures, incomplete speeches) Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/cb6301dd-edef-4871-86d2-ca6d8fa5e8d9 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/cb6301dd-edef-4871-86d2-ca6d8fa5e8d9 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Strengthens the refresh-stats automation to prevent scripts/generate-stats.ts --update from overwriting curated statistics in src/data/generatedStats.ts with incomplete/partial European Parliament API results, and restores previously corrupted stored values.
Changes:
- Added stricter “credibility” guards for API-derived counts (including a significant-drop check) and prevented partial pagination results from being written back.
- Switched Plenary Documents fetching from parallel to sequential to reduce EP API rate-limit pressure.
- Added monthly-data gating to avoid persisting all-zero (or otherwise unreliable) monthly distributions; reverted corrupted yearly and political-landscape values in
generatedStats.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/data/generatedStats.ts |
Reverts corrupted yearly totals and adjusts monthly/political-landscape stored data. |
scripts/generate-stats.ts |
Adds partial-fetch error marking, significant-drop credibility guard, sequential fetching for documents, and monthly write-back gating. |
.github/workflows/refresh-stats.yml |
Documents the new data-quality safeguards in workflow comments. |
| const message = err instanceof Error ? err.message : 'Unknown error'; | ||
| // If we already counted some items before the error, the accumulated | ||
| // count is a lower bound but NOT necessarily the complete count. | ||
| // Return the partial count with an error marker so callers (--update) | ||
| // don't overwrite stored values with incomplete data. | ||
| if (totalCount > 0) { | ||
| progress(`⚠️ ${label}: ${String(totalCount)} items (partial, error on page ${String(pageNum)})`); | ||
| return { total: totalCount }; | ||
| const partialNote = `Partial count (${String(totalCount)} items) — error on page ${String(pageNum)}: ${message}`; | ||
| progress(`⚠️ ${label}: ${partialNote}`); | ||
| return { total: totalCount, error: partialNote }; |
There was a problem hiding this comment.
countItems() now returns a non-null total even when the fetch is partial (and sets error to prevent --update writes). The function-level docstring above still says it returns total or null on failure, which is no longer accurate and can mislead future callers. Update the docstring to document the “partial total + error marker” contract explicitly (including that total is a lower bound when error is set).
| // Collect monthly data only for successful fetches (no note/error) | ||
| // and only when the monthly counts contain useful data (not all zeros). | ||
| // Monthly data is gated by the same credibility check as yearly totals | ||
| // to prevent writing incomplete monthly distributions. | ||
| if (comparison.monthlyCounts && comparison.apiValue !== null) { | ||
| const mField = METRIC_TO_FIELD[comparison.metric]; | ||
| if (mField && isUpdatableField(mField)) { | ||
| if (!monthlyUpdates[yv.year]) { | ||
| monthlyUpdates[yv.year] = {}; | ||
| const hasNonZeroMonth = comparison.monthlyCounts.some((c) => c > 0); | ||
| if (mField && isUpdatableField(mField) && hasNonZeroMonth) { | ||
| // Apply credibility check: only collect monthly data if the total is credible | ||
| const storedVal = comparison.storedValue; | ||
| if (isCredibleApiValue(comparison.apiValue, storedVal)) { | ||
| if (!monthlyUpdates[yv.year]) { | ||
| monthlyUpdates[yv.year] = {}; | ||
| } | ||
| monthlyUpdates[yv.year][mField] = comparison.monthlyCounts; | ||
| } |
There was a problem hiding this comment.
The monthly write-back logic keys off comparison.monthlyCounts + METRIC_TO_FIELD, which will also allow metrics like Procedures to be persisted into RAW_MONTHLY_DATA when they have a non-zero month. But procedure “monthly” counts are based on year-only extraction (YYYY-01-01), so storing them is misleading (and currently unused by distributeMonthly, which always synthetically distributes procedures). Consider restricting monthlyUpdates to the small allowlist of truly date-bucketable metrics (plenarySessions, speeches, events) or otherwise ensure non-date-capable metrics can’t be written into RAW_MONTHLY_DATA.
| { year: 2025, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 720, plenarySessions: 53, legislativeActsAdopted: 78, rollCallVotes: 420, committeeMeetings: 1980, parliamentaryQuestions: 4946, resolutions: 135, speeches: 1998, adoptedTexts: 347, procedures: 923, events: 2660, documents: 930, mepTurnover: 36, declarations: 3000, commentary: 'EP10 ramp-up year (720 MEPs from 27 EU countries). New committee chairs and rapporteurs established. Defence and security policy gained prominence. Strategic autonomy debates. Clean Industrial Deal proposals. Parliament adapting to new political balance with stronger ECR and right-wing presence.' }, | ||
| { year: 2026, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 718, plenarySessions: 54, legislativeActsAdopted: 114, rollCallVotes: 567, committeeMeetings: 2363, parliamentaryQuestions: 1355, resolutions: 180, speeches: 996, adoptedTexts: 104, procedures: 935, events: 2327, documents: 285, mepTurnover: 39, declarations: 1158, commentary: 'PARTIAL YEAR — data through Q1 2026. EP10 has 720 MEPs from 27 EU countries. Plenary session count (54) reflects full-year EP calendar; all other activity totals are projected full-year estimates based on 2021-2025 historical average with EP10 year-2 cycle adjustment. Actual Q1 activity: 10 plenary sittings completed (Jan-Feb), adopted texts and procedures accumulating. Defence spending, Clean Industrial Deal, and AI Act implementation dominating legislative agenda.' }, | ||
| { year: 2025, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 720, plenarySessions: 53, legislativeActsAdopted: 78, rollCallVotes: 420, committeeMeetings: 1980, parliamentaryQuestions: 4946, resolutions: 135, speeches: 10000, adoptedTexts: 347, procedures: 923, events: 2660, documents: 3516, mepTurnover: 36, declarations: 3000, commentary: 'EP10 ramp-up year (720 MEPs from 27 EU countries). New committee chairs and rapporteurs established. Defence and security policy gained prominence. Strategic autonomy debates. Clean Industrial Deal proposals. Parliament adapting to new political balance with stronger ECR and right-wing presence.' }, | ||
| { year: 2026, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 718, plenarySessions: 54, legislativeActsAdopted: 114, rollCallVotes: 567, committeeMeetings: 2363, parliamentaryQuestions: 6147, resolutions: 180, speeches: 996, adoptedTexts: 104, procedures: 935, events: 2327, documents: 4265, mepTurnover: 39, declarations: 1158, commentary: 'PARTIAL YEAR — data through Q1 2026. EP10 has 720 MEPs from 27 EU countries. Plenary session count (54) reflects full-year EP calendar; all other activity totals are projected full-year estimates based on 2021-2025 historical average with EP10 year-2 cycle adjustment. Actual Q1 activity: 10 plenary sittings completed (Jan-Feb), adopted texts and procedures accumulating. Defence spending, Clean Industrial Deal, and AI Act implementation dominating legislative agenda.' }, |
There was a problem hiding this comment.
The 2026 commentary says “all other activity totals are projected full-year estimates”, but speeches: 996 matches the Q1-only monthly data (297+314+385) rather than a full-year projection, while other fields (e.g. parliamentaryQuestions, documents) have been restored to full-year-scale values. Either adjust the commentary to reflect mixed semantics (some projected, some partial) or update the 2026 speech total to match the stated “projected” approach so the row is internally consistent.
| { year: 2026, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 718, plenarySessions: 54, legislativeActsAdopted: 114, rollCallVotes: 567, committeeMeetings: 2363, parliamentaryQuestions: 6147, resolutions: 180, speeches: 996, adoptedTexts: 104, procedures: 935, events: 2327, documents: 4265, mepTurnover: 39, declarations: 1158, commentary: 'PARTIAL YEAR — data through Q1 2026. EP10 has 720 MEPs from 27 EU countries. Plenary session count (54) reflects full-year EP calendar; all other activity totals are projected full-year estimates based on 2021-2025 historical average with EP10 year-2 cycle adjustment. Actual Q1 activity: 10 plenary sittings completed (Jan-Feb), adopted texts and procedures accumulating. Defence spending, Clean Industrial Deal, and AI Act implementation dominating legislative agenda.' }, | |
| { year: 2026, parliamentaryTerm: 'EP10 (2024-2029)', mepCount: 718, plenarySessions: 54, legislativeActsAdopted: 114, rollCallVotes: 567, committeeMeetings: 2363, parliamentaryQuestions: 6147, resolutions: 180, speeches: 996, adoptedTexts: 104, procedures: 935, events: 2327, documents: 4265, mepTurnover: 39, declarations: 1158, commentary: 'PARTIAL YEAR — data through Q1 2026. EP10 has 720 MEPs from 27 EU countries. Plenary session count (54) reflects the full-year EP calendar; speeches (996) reflect actual Q1 cumulative activity; other activity totals are projected full-year estimates based on 2021-2025 historical average with EP10 year-2 cycle adjustment. Actual Q1 activity: 10 plenary sittings completed (Jan-Feb), adopted texts and procedures accumulating. Defence spending, Clean Industrial Deal, and AI Act implementation dominating legislative agenda.' }, |
|
@copilot apply changes based on the comments in this thread |
…monthly data to date-capable metrics, fix 2026 commentary - Update countItems() docstring to document partial-total + error marker contract - Add MONTHLY_CAPABLE_FIELDS allowlist (plenarySessions, speeches, events) to prevent year-only metrics like procedures from polluting RAW_MONTHLY_DATA - Fix 2026 commentary to reflect mixed semantics: speeches (996) and adoptedTexts (104) are actual Q1 data, others are projected estimates Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/cfda45a2-cc2d-493f-9622-95a0c29408ec Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all three review comments in commit
|
| * **Return contract:** | ||
| * - Success: `{ total: N }` — complete count, no `error` field. | ||
| * - Partial: `{ total: N, error: "..." }` — `total` is a **lower | ||
| * bound** (items counted before a pagination error). The `error` | ||
| * field signals to `--update` callers that the count is incomplete | ||
| * and must NOT be written back to stored data. | ||
| * - Failure: `{ total: null, error: "..." }` — no items were | ||
| * retrieved; the API call failed outright. |
There was a problem hiding this comment.
The documented return contract says "Failure" ({ total: null, error }) means no items were retrieved, but the implementation returns { total: null, error } when MAX_PAGES_PER_METRIC is hit even if totalCount > 0. That makes a partial lower-bound indistinguishable from an outright failure and can hide useful diagnostics. Consider returning { total: totalCount, error: note } for the max-page-limit case (and keep callers skipping updates via the error field).
| function isCredibleApiValue(apiValue: number, storedValue: number): boolean { | ||
| if (apiValue >= MIN_CREDIBLE_VALUE) return true; | ||
| // API returned a tiny value — only trust it if stored is also small | ||
| return storedValue <= apiValue * 5; | ||
| // Guard 1: Very small API value when stored is much larger | ||
| if (apiValue < MIN_CREDIBLE_VALUE && storedValue > apiValue * 5) return false; | ||
|
|
||
| // Guard 2: Significant drop from a substantial stored value. | ||
| // Increases are always trusted (API has more data than stored). | ||
| // Only decreases beyond the threshold are flagged. | ||
| if ( | ||
| storedValue > MIN_STORED_FOR_DROP_CHECK && | ||
| apiValue < storedValue && | ||
| ((storedValue - apiValue) / storedValue) * 100 > MAX_ALLOWED_DROP_PERCENT |
There was a problem hiding this comment.
isCredibleApiValue() is now part of the critical data-integrity gate for --update and has multiple threshold-based branches. There don’t appear to be any automated tests covering these guard conditions, which increases the risk of regressions (e.g., edge cases around the 100-item threshold or the 50% drop boundary). Consider extracting this logic into a small module and adding Vitest unit tests for the key scenarios (tiny values, big drops, increases, boundary conditions).
|
@copilot apply changes based on the comments in this thread |
…ts, fix max-page-limit return - Extract isCredibleApiValue() and threshold constants into src/utils/credibilityCheck.ts - Add 21 Vitest unit tests covering guard 1 (tiny values), guard 2 (significant drops), increases, and boundary conditions (100-item threshold, 50% drop boundary) - Fix countItems() max-page-limit case to return partial count instead of null (preserves lower-bound diagnostic data while still marking as error) - Update countItems() docstring to reflect page-limit partial return Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/abaa5446-2b13-4657-bfcf-df66949a4f81 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/abaa5446-2b13-4657-bfcf-df66949a4f81 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied both review comments in commits
|
| import { EuropeanParliamentClient } from '../src/clients/europeanParliamentClient.js'; | ||
| import { GENERATED_STATS } from '../src/data/generatedStats.js'; | ||
| import type { YearlyStats, PoliticalGroupSnapshot, PoliticalLandscapeData } from '../src/data/generatedStats.js'; | ||
| import { isCredibleApiValue, MIN_CREDIBLE_VALUE, MAX_ALLOWED_DROP_PERCENT, MIN_STORED_FOR_DROP_CHECK } from '../src/utils/credibilityCheck.js'; |
There was a problem hiding this comment.
The import from credibilityCheck brings in MIN_CREDIBLE_VALUE, MAX_ALLOWED_DROP_PERCENT, and MIN_STORED_FOR_DROP_CHECK, but this file only uses isCredibleApiValue. With typical TS/ESLint settings this will fail lint/type-check due to unused imports. Remove the unused constants from the import, or reference them (e.g., in log output) if you intend to keep them here.
| import { isCredibleApiValue, MIN_CREDIBLE_VALUE, MAX_ALLOWED_DROP_PERCENT, MIN_STORED_FOR_DROP_CHECK } from '../src/utils/credibilityCheck.js'; | |
| import { isCredibleApiValue } from '../src/utils/credibilityCheck.js'; |
| expect(isCredibleApiValue(0, 0)).toBe(true); | ||
| }); | ||
|
|
||
| it('should accept large API value regardless of stored', () => { |
There was a problem hiding this comment.
This test name is misleading: isCredibleApiValue(10000, 20000) is accepted specifically because it’s exactly a 50% drop (which is allowed by the > MAX_ALLOWED_DROP_PERCENT condition), not because “large API value regardless of stored” is always credible. Consider renaming the test to reflect the actual boundary case being asserted so future readers don’t infer a broader rule than the implementation provides.
| it('should accept large API value regardless of stored', () => { | |
| it('should accept when the drop is exactly at MAX_ALLOWED_DROP_PERCENT', () => { |
|
@copilot apply changes based on the comments in this thread |
…t case Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/21447314-d5d0-4bb4-81b6-889d57661946 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied both review comments in commit
|
| const storedVal = comparison.storedValue; | ||
| if (isCredibleApiValue(comparison.apiValue, storedVal)) { | ||
| if (!monthlyUpdates[yv.year]) { | ||
| monthlyUpdates[yv.year] = {}; |
There was a problem hiding this comment.
The monthly update collection can omit an existing metric (e.g. because the yearly total fails the credibility check), but updateMonthlyData() later replaces the entire RAW_MONTHLY_DATA[year] entry with only the collected keys. That causes unrelated existing monthly series to be deleted (as seen in this PR where 2025 speeches disappeared). Consider changing the update logic to merge per-year monthly data: only overwrite the specific metric arrays present in monthlyUpdates[year], while preserving any existing metric arrays for that year that weren’t updated in this run.
| monthlyUpdates[yv.year] = {}; | |
| monthlyUpdates[yv.year] = { | |
| ...(RAW_MONTHLY_DATA[yv.year] ?? {}), | |
| }; |
|
@copilot apply changes based on the comments in this thread and investigate and improve failing checks |
…n docs feed test timeout Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/a4a7eb23-db58-49f1-8117-e2c31a718391 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied in commit
|
| let fieldMatch; | ||
| while ((fieldMatch = fieldPattern.exec(existingMatch[0])) !== null) { | ||
| const fieldName = fieldMatch[1]; | ||
| const values = fieldMatch[2].split(',').map((v) => Number(v.trim())); | ||
| if (fieldName !== undefined && values.length === 12) { |
There was a problem hiding this comment.
let fieldMatch; introduces an implicit any, and fieldMatch[2].split(...) assumes capture group 2 is always present. Please type fieldMatch as RegExpExecArray | null (or similar) and add a small guard for missing capture groups to keep this code type-safe under the repo’s strict TS settings and resilient to regex changes.
| let fieldMatch; | |
| while ((fieldMatch = fieldPattern.exec(existingMatch[0])) !== null) { | |
| const fieldName = fieldMatch[1]; | |
| const values = fieldMatch[2].split(',').map((v) => Number(v.trim())); | |
| if (fieldName !== undefined && values.length === 12) { | |
| let fieldMatch: RegExpExecArray | null = null; | |
| while ((fieldMatch = fieldPattern.exec(existingMatch[0])) !== null) { | |
| const fieldName = fieldMatch[1]; | |
| const rawValues = fieldMatch[2]; | |
| if (fieldName === undefined || rawValues === undefined) { | |
| continue; | |
| } | |
| const values = rawValues.split(',').map((v) => Number(v.trim())); | |
| if (values.length === 12) { |
|
@copilot apply changes based on the comments in this thread and fix issues in Integration and E2E Tests / Integration Tests (25.x) (pull_request) |
…s feed test timeout to 180s Agent-Logs-Url: https://github.com/Hack23/European-Parliament-MCP-Server/sessions/ed04e46f-326c-4427-8d63-04a026de8832 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied in commit
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: James Pether Sörling <pethers@users.noreply.github.com>
isCredibleApiValue()with significant-drop guardcountItems()partial data bug — return error field when pagination errors mid-wayPromise.all()in Plenary DocumentsMONTHLY_CAPABLE_FIELDSallowlistcountItems()to return partial count instead of nullisCredibleApiValue()intosrc/utils/credibilityCheck.tswith 20 unit tests covering all guard conditions and boundariesfieldMatchwithRegExpExecArray | nulland capture-group guardsget_plenary_documents_feedintegration test timeout from 120s to 180s