Skip to content

ai-summary: stop benign warnings flipping timeouts to CRASH - #141

Merged
ppetrovicTT merged 1 commit into
mainfrom
ppetrovic/ai-summary-timeout-not-crash
Jun 23, 2026
Merged

ppetrovicTT merged 1 commit into
mainfrom
ppetrovic/ai-summary-timeout-not-crash

Conversation

@ppetrovicTT

Copy link
Copy Markdown
Contributor

Problem

A pure GitHub timeout-minutes kill was reported as CRASH, blaming an unrelated compiler warning.

Real case: tt-metal run 27993444376 job 82855477860 (bh_lb_DeepSeek_PREFILL). The job ran 1h42m with tests passing, then GitHub killed the step on timeout-minutes. The summary concluded CRASH and cited a reader_combine.cpp -Wunused-but-set-variable warning (which is -Wno-error'd — a pure warning, not a build failure).

Root cause

The no-LLM infra:timeout fast-path (added in #134) was guarded by not extracted.error_sections. Benign warnings that contain the word "failed" — e.g. UMD hugepage bind_area_to_memory_nodeset() failed (Issue #893) — match the error regex and seed error_sections. That kicked a real marker-absence timeout off the deterministic path and onto the LLM, which has no TIMEOUT verdict in its enum and so labelled the truncated log a CRASH.

GitHub's own ##[error]The action has timed out. is a runner annotation emitted outside the run-with-log capture, so the tool never sees it — a marker-absence timeout is the only signal available, and it must not be overridable by warning noise.

Fix

  • cli.py: gate the no-LLM timeout path on status, not on the absence of error sections. A TIMEOUT status already means get_job_status ruled out crash / nonzero exit / test failure / eval failure, so any remaining error sections are noise. Set an explicit error_message so the conclusion is stated, not inferred.
  • summarize.py: gate the "Log truncated…" note on status_text != "TIMEOUT", matching the existing header flag — drops the redundant caveat on a pure timeout.
  • test_cli.py: new case — truncated log with warning-noise error_sections → infra:timeout, LLM not called.

Status stays TIMEOUT (RED). 275 tests pass.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 23, 2026 15:31
@ppetrovicTT
ppetrovicTT requested a review from a team as a code owner June 23, 2026 15:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts the ai_job_summary timeout handling so “marker-absence” GitHub timeout-minutes kills are reported deterministically as infra:timeout (TIMEOUT/RED) even when benign warning text seeds error_sections, preventing the LLM from misclassifying timeouts as CRASH.

Changes:

  • cli.py: Make the no-LLM timeout fast-path depend on job_status.status_text == "TIMEOUT" (not on error_sections being empty) and set an explicit timeout error_message.
  • summarize.py: Suppress the “Log truncated…” caveat when the job status itself is TIMEOUT, avoiding redundant messaging for pure timeouts.
  • test_cli.py: Add a regression test ensuring warning-noise (“failed” in warnings) doesn’t trigger an LLM call and still yields infra:timeout.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
.github/actions/ai_summary/tool/ai_job_summary/cli.py Makes marker-absence TIMEOUT classification deterministic and sets a concrete timeout error message (no LLM).
.github/actions/ai_summary/tool/ai_job_summary/summarize.py Avoids showing the truncation warning under Error Message for pure TIMEOUT statuses.
.github/actions/ai_summary/tool/ai_job_summary/tests/test_cli.py Adds coverage for timeout logs that include warning noise matching the error regex.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ppetrovicTT
ppetrovicTT force-pushed the ppetrovic/ai-summary-timeout-not-crash branch from 7e3546b to 5d626c3 Compare June 23, 2026 15:35
A marker-absence TIMEOUT already means crash/exit/test/eval were ruled
out, so leftover error sections are noise (e.g. UMD hugepage "...failed"
warnings). They were defeating the no-LLM timeout path, and the LLM has
no TIMEOUT verdict so it relabelled them CRASH. Gate the path on status,
not on the absence of error sections, and state the timeout explicitly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 23, 2026 15:39
@ppetrovicTT
ppetrovicTT force-pushed the ppetrovic/ai-summary-timeout-not-crash branch from 5d626c3 to 5275868 Compare June 23, 2026 15:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@ppetrovicTT
ppetrovicTT merged commit 78f8a16 into main Jun 23, 2026
4 checks passed
@ppetrovicTT
ppetrovicTT deleted the ppetrovic/ai-summary-timeout-not-crash branch June 23, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants