-
-
Notifications
You must be signed in to change notification settings - Fork 2
Fix News Realtime Monitor timeout: track breaking news errors, enforce script timeouts #1047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d2ea234
bc1e2e7
36b3c39
d56c5dd
810e721
b55bf09
22dbbfd
7ce3b4d
3abf5cb
943e813
0147531
20d74bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -129,13 +129,25 @@ START_TIME=$(date +%s) | |||||||||
| |-------|---------|--------| | ||||||||||
| | Setup | 0–3 | Date check, `get_sync_status()` warm-up | | ||||||||||
| | Detect | 3–8 | Query MCP tools for today's activity | | ||||||||||
| | Generate | 8–35 | Run `generate-news-enhanced.ts` script (handles all 14 languages) | | ||||||||||
| | Validate | 35–38 | Run `validate-news-generation.sh` | | ||||||||||
| | Commit+PR | 38–43 | `git add && git commit`, then `safeoutputs___create_pull_request` | | ||||||||||
| | Generate | 8–30 | Run `generate-news-enhanced.ts` script (core languages by default; supports all 14 languages via `languages=all`) | | ||||||||||
| | Validate | 30–35 | Run `validate-news-generation.sh` | | ||||||||||
| | Commit+PR | 35–40 | `git add && git commit`, then `safeoutputs___create_pull_request` | | ||||||||||
|
|
||||||||||
| **Hard cutoffs** — check elapsed time before each phase: | ||||||||||
| - `>= 38 min` → Stop generating, commit what you have, create PR immediately | ||||||||||
| - `>= 43 min` → STOP ALL WORK, call safe output immediately | ||||||||||
| **Hard cutoffs** — check elapsed time before EVERY phase: | ||||||||||
| ```bash | ||||||||||
| # Restore START_TIME if available so this snippet is safe to run standalone | ||||||||||
| if [ -f /tmp/gh-aw/agent/timing.env ]; then | ||||||||||
| . /tmp/gh-aw/agent/timing.env | ||||||||||
| fi | ||||||||||
| # Fallback: if START_TIME is still unset, initialize it to "now" to avoid huge elapsed times | ||||||||||
| : "${START_TIME:=$(date +%s)}" | ||||||||||
|
|
||||||||||
| ELAPSED=$(( $(date +%s) - START_TIME )) | ||||||||||
| echo "⏱️ Elapsed: $((ELAPSED / 60))m $((ELAPSED % 60))s" | ||||||||||
| ``` | ||||||||||
| - `>= 35 min` → Stop generating, commit what you have, create PR immediately | ||||||||||
| - `>= 40 min` → STOP ALL WORK, call safe output tool (`safeoutputs___noop` or `safeoutputs___create_pull_request`) IMMEDIATELY — do NOT run any more bash commands | ||||||||||
| - **CRITICAL**: If you have not called a safe output tool and time is running out, call `safeoutputs___noop` immediately. Failing to call a safe output tool causes a workflow failure. | ||||||||||
|
|
||||||||||
| ## Step 1: Date Validation & MCP Health Check | ||||||||||
|
|
||||||||||
|
|
@@ -200,33 +212,53 @@ safeoutputs___noop({ "message": "No significant parliamentary events. Checked: v | |||||||||
| **🚨 ALWAYS use the TypeScript generation script — it handles MCP queries, HTML templating, all 14 languages, translation, and article quality internally.** | ||||||||||
|
|
||||||||||
| ```bash | ||||||||||
| # Use the languages workflow dispatch parameter (defaults to "all") | ||||||||||
| # Use the languages workflow dispatch parameter | ||||||||||
| # For scheduled runs (empty input), default to core languages (en,sv) to stay within time budget. | ||||||||||
| # Manual dispatch can specify "all" for 14-language generation. | ||||||||||
| LANGUAGES_INPUT="${{ github.event.inputs.languages }}" | ||||||||||
| [ -z "$LANGUAGES_INPUT" ] && LANGUAGES_INPUT="all" | ||||||||||
| [ -z "$LANGUAGES_INPUT" ] && LANGUAGES_INPUT="en,sv" | ||||||||||
|
|
||||||||||
| case "$LANGUAGES_INPUT" in | ||||||||||
| "nordic") LANG_ARG="en,sv,da,no,fi" ;; | ||||||||||
| "eu-core") LANG_ARG="en,sv,de,fr,es,nl" ;; | ||||||||||
| "all") LANG_ARG="en,sv,da,no,fi,de,fr,es,nl,ar,he,ja,ko,zh" ;; | ||||||||||
| *) LANG_ARG="$LANGUAGES_INPUT" ;; | ||||||||||
| esac | ||||||||||
|
|
||||||||||
| # Set up MCP connection and generate (source && npx must run on ONE line to preserve MCP_SERVER_URL) | ||||||||||
| source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts \ | ||||||||||
| --types=breaking \ | ||||||||||
| --languages="$LANG_ARG" \ | ||||||||||
| --skip-existing | ||||||||||
| SCRIPT_EXIT=$? | ||||||||||
| echo "Script exit code: $SCRIPT_EXIT" | ||||||||||
|
|
||||||||||
| # Check for newly generated files | ||||||||||
| TODAY="$(date +%Y-%m-%d)" | ||||||||||
| NEW_ARTICLES="$(git status --porcelain -- news/ | awk '{print $2}' | grep "${TODAY}-" || true)" | ||||||||||
| if [ -z "$NEW_ARTICLES" ]; then | ||||||||||
| echo "No new breaking news articles were created." | ||||||||||
| export LANG_ARG | ||||||||||
|
|
||||||||||
| # Check elapsed time before starting generation | ||||||||||
| source /tmp/gh-aw/agent/timing.env 2>/dev/null || true | ||||||||||
| : "${START_TIME:=$(date +%s)}" | ||||||||||
| ELAPSED=$(( $(date +%s) - START_TIME )) | ||||||||||
| if [ "$ELAPSED" -ge 2100 ]; then | ||||||||||
| echo "⏱️ Time budget exceeded (${ELAPSED}s >= 35min) — skipping generation" | ||||||||||
| SCRIPT_EXIT=0 | ||||||||||
| NEW_ARTICLES="" | ||||||||||
| else | ||||||||||
| echo "Newly generated articles:" | ||||||||||
| printf '%s\n' "$NEW_ARTICLES" | ||||||||||
| # Set up MCP connection and generate with 20-minute timeout | ||||||||||
| # LANG_ARG is exported so the subshell inherits it safely (no command-string interpolation) | ||||||||||
| timeout 1200 bash -lc 'source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts --types=breaking --languages="$LANG_ARG" --skip-existing' | ||||||||||
| SCRIPT_EXIT=$? | ||||||||||
| TIMED_OUT=false | ||||||||||
| if [ "$SCRIPT_EXIT" -eq 124 ]; then | ||||||||||
| echo "⚠️ Script timed out after 20 minutes — proceeding with whatever was generated" | ||||||||||
|
||||||||||
| echo "⚠️ Script timed out after 20 minutes — proceeding with whatever was generated" | |
| echo "⚠️ Script timed out after 20 minutes — proceeding with whatever was generated" | |
| # Treat timeout as soft failure to avoid triggering manual fallback if some content was produced | |
| SCRIPT_EXIT=0 |
Copilot
AI
Mar 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout handling normalizes exit code 124 to 0 unconditionally. If the script times out and produces no new articles, later logic will treat it like a clean “no events” run and may call noop instead of triggering the error/fallback path. Consider only treating timeout as a soft success when NEW_ARTICLES is non-empty (or track a separate TIMED_OUT flag and keep a non-zero exit when nothing was generated).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,7 +150,7 @@ export async function generateNews(): Promise<typeof stats> { | |
|
|
||
| console.log(` 📰 Lead story: "${topTitle}"`); | ||
|
|
||
| await generateBreakingNews({ | ||
| const breakingResult = await generateBreakingNews({ | ||
| languages, | ||
| eventContext: topTitle, | ||
| eventData: { | ||
|
|
@@ -160,8 +160,16 @@ export async function generateNews(): Promise<typeof stats> { | |
| }, | ||
| writeArticle: translatingWriteArticle, | ||
| }); | ||
| if (!breakingResult.success) { | ||
| console.error('❌ Breaking news generation failed:', breakingResult.error ?? 'unknown error'); | ||
| stats.errors++; | ||
| } | ||
| } catch (err: unknown) { | ||
| console.error('❌ Error generating breaking news:', (err as Error).message); | ||
| console.error( | ||
| '❌ Error generating breaking news:', | ||
| err instanceof Error ? err.message : String(err), | ||
| ); | ||
| stats.errors++; | ||
| } | ||
|
Comment on lines
153
to
173
|
||
| break; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| /** | ||
| * Tests that generateNews() correctly increments stats.errors when the | ||
| * 'breaking' article type fails — either via a returned {success: false} | ||
| * or a thrown exception. | ||
| * | ||
| * This covers the exit-code-critical behavior: runCli() uses stats.errors > 0 | ||
| * to decide exit code 1 vs 0, which the agent relies on to detect failures. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; | ||
| import type { GenerationStats, GenerationResult } from '../scripts/types/article.js'; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Hoisted mocks — vi.hoisted() runs before vi.mock() calls | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const { mockGenerateBreakingNews, mockStats, mockGetSharedClient } = vi.hoisted(() => { | ||
| const mockGenerateBreakingNews = vi.fn<(...args: unknown[]) => Promise<GenerationResult>>(); | ||
| const mockStats: GenerationStats = { | ||
| generated: 0, | ||
| errors: 0, | ||
| articles: [], | ||
| timestamp: new Date().toISOString(), | ||
| qualityScores: [] | ||
| }; | ||
| const mockGetSharedClient = vi.fn().mockResolvedValue({ | ||
| fetchVotingRecords: vi.fn().mockResolvedValue([{ doktyp: 'prop', titel: 'Test Prop' }]), | ||
| searchDocuments: vi.fn().mockResolvedValue([{ doktyp: 'prop', titel: 'Test Prop' }]) | ||
| }); | ||
| return { mockGenerateBreakingNews, mockStats, mockGetSharedClient }; | ||
| }); | ||
|
|
||
| // Mock fs module at the module level (hoisted before imports) to prevent file writes | ||
| vi.mock('fs', async (importOriginal) => { | ||
| const original = await importOriginal<typeof import('fs')>(); | ||
| return { | ||
| ...original, | ||
| default: { | ||
| ...original, | ||
| writeFileSync: vi.fn(), | ||
| mkdirSync: vi.fn(), | ||
| existsSync: vi.fn().mockReturnValue(false), | ||
| readdirSync: vi.fn().mockReturnValue([]), | ||
| }, | ||
| writeFileSync: vi.fn(), | ||
| mkdirSync: vi.fn(), | ||
| existsSync: vi.fn().mockReturnValue(false), | ||
| readdirSync: vi.fn().mockReturnValue([]), | ||
| }; | ||
| }); | ||
|
|
||
| // Mock the breaking-news module | ||
| vi.mock('../scripts/news-types/breaking-news.js', () => ({ | ||
| generateBreakingNews: mockGenerateBreakingNews | ||
| })); | ||
|
|
||
| // Mock the config module — set articleTypes to ['breaking'] and expose stats | ||
| vi.mock('../scripts/generate-news-enhanced/config.js', async (importOriginal) => { | ||
| const original = await importOriginal<typeof import('../scripts/generate-news-enhanced/config.js')>(); | ||
| return { | ||
| ...original, | ||
| articleTypes: ['breaking'], | ||
| stats: mockStats, | ||
| getSharedClient: mockGetSharedClient, | ||
| // Provide stable values for other config exports | ||
| languages: ['en'], | ||
| allRequestedLanguages: ['en'], | ||
| batchSize: undefined, | ||
| skipExistingArg: false, | ||
| requireMcp: false, | ||
| }; | ||
| }); | ||
|
|
||
| // Mock MCP client | ||
| vi.mock('../scripts/mcp-client.js', () => ({ | ||
| MCPClient: vi.fn().mockImplementation(() => ({ | ||
| fetchVotingRecords: vi.fn().mockResolvedValue([]), | ||
| searchDocuments: vi.fn().mockResolvedValue([]) | ||
| })), | ||
| getDefaultClient: vi.fn() | ||
| })); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Import after mocks | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| interface GenerateNewsModule { | ||
| readonly generateNews: () => Promise<GenerationStats>; | ||
| } | ||
|
|
||
| let moduleExports: GenerateNewsModule; | ||
|
|
||
| beforeAll(async () => { | ||
| moduleExports = await import('../scripts/generate-news-enhanced/index.js') as unknown as GenerateNewsModule; | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| describe('generateNews() — breaking news error tracking', () => { | ||
| beforeEach(() => { | ||
| // Reset stats before each test | ||
| mockStats.errors = 0; | ||
| mockStats.generated = 0; | ||
| mockStats.articles = []; | ||
| mockStats.qualityScores = []; | ||
| vi.clearAllMocks(); | ||
|
|
||
| // Re-setup shared client mock after clearAllMocks | ||
| mockGetSharedClient.mockResolvedValue({ | ||
| fetchVotingRecords: vi.fn().mockResolvedValue([{ doktyp: 'prop', titel: 'Test Prop' }]), | ||
| searchDocuments: vi.fn().mockResolvedValue([{ doktyp: 'prop', titel: 'Test Prop' }]) | ||
| }); | ||
| }); | ||
|
|
||
| it('should increment stats.errors when generateBreakingNews returns success=false', async () => { | ||
| mockGenerateBreakingNews.mockResolvedValueOnce({ | ||
| success: false, | ||
| error: 'MCP server unreachable' | ||
| }); | ||
|
|
||
| const result = await moduleExports.generateNews(); | ||
|
|
||
| expect(mockGenerateBreakingNews).toHaveBeenCalled(); | ||
| expect(result.errors).toBe(1); | ||
| }); | ||
|
|
||
| it('should increment stats.errors when generateBreakingNews throws an exception', async () => { | ||
| // Make getSharedClient itself throw so the catch block fires | ||
| mockGetSharedClient.mockRejectedValueOnce(new Error('Connection timeout')); | ||
|
|
||
| const result = await moduleExports.generateNews(); | ||
|
|
||
| expect(result.errors).toBe(1); | ||
| }); | ||
|
|
||
| it('should NOT increment stats.errors when generateBreakingNews returns success=true', async () => { | ||
| mockGenerateBreakingNews.mockResolvedValueOnce({ | ||
| success: true, | ||
| articles: [{ lang: 'en', html: '<p>Breaking</p>', slug: 'test', filename: 'test.html' }] | ||
| }); | ||
|
|
||
| const result = await moduleExports.generateNews(); | ||
|
|
||
| expect(mockGenerateBreakingNews).toHaveBeenCalled(); | ||
| expect(result.errors).toBe(0); | ||
| }); | ||
|
|
||
| it('should log the error message from failed generation result', async () => { | ||
| const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| mockGenerateBreakingNews.mockResolvedValueOnce({ | ||
| success: false, | ||
| error: 'Breaking: no significant events today' | ||
| }); | ||
|
|
||
| await moduleExports.generateNews(); | ||
|
|
||
| expect(consoleSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('Breaking news generation failed'), | ||
| expect.stringContaining('no significant events today') | ||
| ); | ||
|
|
||
| consoleSpy.mockRestore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The elapsed-time snippet uses
START_TIMEdirectly, but if someone runs this block in a fresh shell withoutSTART_TIMEset, Bash arithmetic will treat it as 0 and report an enormous elapsed time (epoch seconds), which can trigger incorrect “time budget exceeded” decisions. Consider sourcing/tmp/gh-aw/agent/timing.env(or re-initializingSTART_TIME) in the snippet so it’s safe to run standalone like the later generation/validation guards.