Fix News Realtime Monitor timeout: track breaking news errors, enforce script timeouts#1047
Conversation
…nt workflow timeout - Increment stats.errors when breaking news generation fails (was silently swallowed) - Add 20-minute timeout wrapper to script execution in news-realtime-monitor.md - Add elapsed time checks before generation and validation phases - Add 5-minute timeout to validation script - Tighten time budget (35/40 min cutoffs vs 38/43) with explicit safe output instructions - Add error handling tests for breaking news module Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
github-actions
Bot
added
documentation
🔍 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 addresses a failure mode in the “News Realtime Monitor” agent workflow by improving failure signaling for breaking-news generation and adding explicit time-budget enforcement in the workflow runbook to avoid exhausting the 45-minute execution window.
Changes:
- Increment
stats.errorswhen breaking-news generation throws, sorunCli()can exit non-zero on failure. - Add timeout wrappers and elapsed-time guardrails to the
news-realtime-monitor.mdworkflow instructions to prevent long-running/hanging script/validation steps. - Add unit tests covering breaking-news error handling paths (missing event data, MCP tool failures).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
scripts/generate-news-enhanced/index.ts |
Tracks breaking-news exceptions by incrementing stats.errors to influence CLI exit code. |
.github/workflows/news-realtime-monitor.md |
Adds elapsed-time checks and timeout wrappers around generation/validation steps to keep the agent within budget. |
tests/news-types/breaking-news.test.ts |
Adds tests for early-failure behavior and resilience to MCP tool errors. |
| } catch (err: unknown) { | ||
| console.error('❌ Error generating breaking news:', (err as Error).message); | ||
| stats.errors++; | ||
| } |
There was a problem hiding this comment.
generateBreakingNews() returns a GenerationResult with success: false on internal failures (it catches and returns) rather than throwing. In that case this try/catch won’t run and stats.errors will still remain 0, so runCli() may exit 0 even though breaking generation failed. Capture the returned result and increment stats.errors (and log the error) when result.success === false.
| GENERATE_CMD="source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts --types=breaking --languages=${LANG_ARG} --skip-existing" | ||
| timeout 1200 bash -c "$GENERATE_CMD" |
There was a problem hiding this comment.
LANG_ARG can come directly from ${{ github.event.inputs.languages }} and may contain spaces (e.g. "en, sv"). In GENERATE_CMD, --languages=${LANG_ARG} will then be split into multiple arguments and break parsing. Quote/escape the languages argument in the generated command (e.g. ensure --languages receives exactly one shell token) and avoid constructing an unescaped shell string where possible.
| GENERATE_CMD="source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts --types=breaking --languages=${LANG_ARG} --skip-existing" | |
| timeout 1200 bash -c "$GENERATE_CMD" | |
| timeout 1200 bash -lc 'source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts --types=breaking --languages="$LANG_ARG" --skip-existing' |
| GENERATE_CMD="source scripts/mcp-setup.sh && npx tsx scripts/generate-news-enhanced.ts --types=breaking --languages=${LANG_ARG} --skip-existing" | ||
| timeout 1200 bash -c "$GENERATE_CMD" | ||
| SCRIPT_EXIT=$? |
There was a problem hiding this comment.
This builds a shell command string that includes workflow-dispatch input (LANGUAGES_INPUT → LANG_ARG) and executes it via bash -c. That pattern is vulnerable to command injection if the input contains shell metacharacters. Prefer invoking timeout/npx directly (no bash -c) and keep the user-supplied value only in a properly quoted argument (or validate it against an allow-list).
| - `>= 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 |
There was a problem hiding this comment.
The elapsed-time snippet uses START_TIME directly, but if someone runs this block in a fresh shell without START_TIME set, 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-initializing START_TIME) in the snippet so it’s safe to run standalone like the later generation/validation guards.
| ```bash | |
| ```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)}" |
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…nd injection, safe START_TIME
- Check generateBreakingNews() return value for success: false (not just exceptions)
- Fix command injection: export LANG_ARG and use single-quoted bash -lc instead of
interpolating user input into a command string via bash -c
- Make elapsed-time snippet safe for standalone execution by sourcing timing.env
and using : "${START_TIME:=$(date +%s)}" fallback
- Apply same START_TIME safety to generation and validation time checks
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all 4 review comments in
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| 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=$? | ||
| if [ "$SCRIPT_EXIT" -eq 124 ]; then | ||
| echo "⚠️ Script timed out after 20 minutes — proceeding with whatever was generated" |
There was a problem hiding this comment.
timeout returns exit code 124 on timeout, but this value is kept in SCRIPT_EXIT. A later decision point treats non-zero SCRIPT_EXIT + no new articles as a script error (manual fallback), which contradicts the note “proceeding with whatever was generated” and can waste time near the deadline. Consider normalizing SCRIPT_EXIT to 0 on 124 (soft failure) or updating the subsequent decision logic to treat 124 as a non-fatal timeout case.
| 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 |
| if [ -z "$NEW_ARTICLES" ]; then | ||
| echo "No new breaking news articles were created." | ||
| export LANG_ARG | ||
|
|
There was a problem hiding this comment.
For scheduled runs, ${{ github.event.inputs.languages }} is empty, so the snippet falls back to LANGUAGES_INPUT="all", which triggers 14-language generation and can significantly increase runtime. This conflicts with the workflow description (“core languages (EN, SV)”) and undermines the new time-budget enforcement. Consider defaulting the empty-input case to en,sv (or another explicit core preset) and reserving all for manual dispatch.
| @@ -160,8 +160,13 @@ 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); | |||
| stats.errors++; | |||
| } | |||
There was a problem hiding this comment.
The breaking-news path now increments stats.errors both when generateBreakingNews() returns { success: false } and when it throws. There are unit tests for generateNews() in tests/generate-news-enhanced-part2.test.ts, but none that exercise the breaking case (default articleTypes is ['week-ahead']), so this new exit-code-critical behavior isn’t covered. Add a focused test that forces articleTypes to include breaking and mocks generateBreakingNews() to return failure/throw, then asserts stats.errors increments.
|
@copilot apply changes based on the comments in this thread |
… breaking error tests
1. Normalize SCRIPT_EXIT to 0 on timeout (exit 124) to prevent manual
fallback when partial content was produced
2. Default empty LANGUAGES_INPUT to 'en,sv' instead of 'all' for
scheduled runs — matches workflow description and time budget
3. Add 4 focused tests for generateNews() breaking case:
- stats.errors increments on {success: false}
- stats.errors increments on thrown exception
- stats.errors stays 0 on {success: true}
- error message is logged from failed result
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
…ide effects Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
…l(1) Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
…e writes
Use hoisted vi.mock('fs') instead of vi.spyOn() in beforeAll to ensure
filesystem mocking is active before the dynamic import triggers
generateNews() side effects.
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all 3 review comments across commits
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| if (!moduleExports) return; | ||
|
|
There was a problem hiding this comment.
This early return means the test passes without executing assertions when moduleExports is null (e.g., if the import failed). Replace with an assertion or throw so the test fails when the module can’t be imported.
| if (!moduleExports) return; | ||
|
|
There was a problem hiding this comment.
This early return means the test passes without executing assertions when moduleExports is null (e.g., if the import failed). Replace with an assertion or throw so the test fails when the module can’t be imported.
| if (!moduleExports) return; | ||
|
|
There was a problem hiding this comment.
This early return means the test passes without executing assertions when moduleExports is null (e.g., if the import failed). Replace with an assertion or throw so the test fails when the module can’t be imported.
| 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=$? | ||
| if [ "$SCRIPT_EXIT" -eq 124 ]; then | ||
| 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 | ||
| fi |
There was a problem hiding this comment.
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).
| | 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 (handles all 14 languages) | |
There was a problem hiding this comment.
The time budget table says the generation script “handles all 14 languages”, but this workflow now defaults scheduled runs to en,sv. This wording is likely to confuse operators/agents about expected runtime and output; consider aligning the table text with the new default behavior (core languages by default, “all” via manual dispatch).
| | Generate | 8–30 | Run `generate-news-enhanced.ts` script (handles all 14 languages) | | |
| | Generate | 8–30 | Run `generate-news-enhanced.ts` script (core languages by default; supports all 14 languages via \`languages=all\`) | |
| stats.errors++; | ||
| } | ||
| } catch (err: unknown) { | ||
| console.error('❌ Error generating breaking news:', (err as Error).message); |
There was a problem hiding this comment.
In this catch block, (err as Error).message can log undefined if a non-Error is thrown/rejected. Prefer using err instanceof Error ? err.message : String(err) to ensure a useful message is always logged.
| console.error('❌ Error generating breaking news:', (err as Error).message); | |
| console.error( | |
| '❌ Error generating breaking news:', | |
| err instanceof Error ? err.message : String(err), | |
| ); |
| moduleExports = await import('../scripts/generate-news-enhanced/index.js') as unknown as GenerateNewsModule; | ||
| } catch (e: unknown) { | ||
| console.error('Import failed:', e instanceof Error ? e.message : String(e)); | ||
| moduleExports = null; |
There was a problem hiding this comment.
The test suite can silently pass if the module import fails: beforeAll catches the import error, sets moduleExports=null, and the individual tests then return early. That means these tests may provide a false sense of coverage. Prefer failing fast (e.g., rethrow in beforeAll or assert moduleExports is non-null at the start of each test).
| moduleExports = null; | |
| throw e; |
| if (!moduleExports) return; | ||
|
|
There was a problem hiding this comment.
This early return means the test passes without executing assertions when moduleExports is null (e.g., if the import failed). Replace with an assertion or throw so the test fails when the module can’t be imported.
|
@copilot apply changes based on the comments in this thread |
…ion, safe error logging 1. Tests: replace silent early returns with `expect(moduleExports).not.toBeNull()` assertions; remove try/catch in beforeAll so import failures fail the suite immediately instead of silently passing all tests. 2. Workflow: only normalize timeout exit (124→0) when NEW_ARTICLES is non-empty; keep non-zero exit when timeout produces no content so the error/fallback path triggers correctly. Update time-budget table to reflect en,sv default. 3. index.ts: use `err instanceof Error ? err.message : String(err)` in catch block to avoid logging `undefined` for non-Error rejects. Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
…ort failure Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all 8 review comments in
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
Workflow run #22947592952 failed because the agent consumed the full 45-minute budget without calling a safe output tool. Two root causes: silent error swallowing in breaking news generation, and no timeout enforcement on script execution.
Bug fix: breaking news error tracking
The
breakingcase ingenerateNews()had two gaps in error tracking:catchblock never incrementedstats.errors, so thrown exceptions were silently swallowed. The catch block now also useserr instanceof Error ? err.message : String(err)to safely log any thrown value.generateBreakingNews()returns{success: false}on internal failures rather than throwing, so thetry/catchalone was insufficient. Now the returnedGenerationResultis captured andstats.errorsis incremented whenresult.success === false.This matters because
runCli()usesstats.errors > 0to decide exit code 1 vs 0, which the agent uses to determine whether generation succeeded.Timeout enforcement in workflow MD
timeout 1200(20 min) to prevent indefinite hangs on slow MCP servertimeout 300(5 min)SCRIPT_EXITto 0 whenNEW_ARTICLESis non-empty (partial content produced); keep the non-zero exit when timeout produces no content so the error/fallback path triggers correctly. ATIMED_OUTflag tracks timeout state separately from the exit code.Scheduled run language default
For scheduled runs (empty
github.event.inputs.languages), the workflow now defaults toen,sv(core languages) instead ofall(14 languages). This matches the workflow description ("core languages EN, SV") and keeps generation within the time budget. Manual dispatch can still specify"all"for full 14-language generation. The time budget table text has been updated to reflect the new default.Security: fix command injection in workflow bash
Replaced
bash -c "$GENERATE_CMD"(which interpolated user-suppliedLANG_ARGinto a command string) withexport LANG_ARG+ single-quotedbash -lc '...--languages="$LANG_ARG"...'. The variable is now resolved from the environment inside the subshell, eliminating command injection risk from workflow dispatch input containing shell metacharacters.Robustness: standalone-safe elapsed-time snippets
All elapsed-time check blocks now source
/tmp/gh-aw/agent/timing.envand use: "${START_TIME:=$(date +%s)}"as a fallback, so they produce correct results even when run in a fresh shell withoutSTART_TIMEset.Tests
breaking-news.test.ts: null/undefined event data, mcpCalls on early failure, MCP tool failure resiliencegenerate-news-breaking-errors.test.tsexercisinggenerateNews()witharticleTypes=['breaking']: verifiesstats.errorsis exactly 1 whengenerateBreakingNews()returns{success: false}, exactly 1 whengetSharedClient()throws, exactly 0 on success, and that the error message is logged. Tests use fail-fast imports (no silent early returns) so import failures surface immediately rather than producing false passes.Original 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.