Repository navigation
rebase: surface assertions an infra-failed run already reported - #172
Conversation
A structural end does not mean the run found nothing. test_loop classifies
timeout / watchdog kill / harness crash as INFRASTRUCTURE (Rev 8 2.3) and
keeps them out of failed_tests so the push gate blocks instead of passing
assertions through. That classification is correct and unchanged here.
What was wrong is that the branch recorded only the label and continued,
never inspecting the run's log -- so assertion failures the suite had ALREADY
reported before it died were discarded.
Observed live on 2026-09-19: simple_diffusion_test reported 10 quantization
assertion failures and was then killed. The run summary read
tests: 30 passed, 1 failed (1 infra), 3 skipped
so the campaign looked clean apart from an infrastructure nuisance, and the
gate message named only a timeout -- which made a correct block look like
flakiness worth overriding. The 10 failures stayed invisible until Buildkite
#3055 re-found them as two distinct upstream contract changes
(vllm-project/vllm-omni#7820).
The infra record now names the ids, reusing ci_loop.extract_failed_test_ids
rather than adding a second parser:
killed: timeout - 2 assertion failure(s) reported before the structural
end: tests/...::TestA::test_one, tests/...::TestB::test_two
TestRunResult.output carries either log TEXT or a PATH to the log file: the
production caller (engine/steps/rebase_v3._to_result) passes
outcome.log_file. _log_text() reads the file when it is one, so this fires
for the real timeout/watchdog runs it exists for; a read failure degrades to
"no ids found" and never changes the classification. The first version of
this change parsed the pathname and would have been inert in production --
caught by the staged-diff review gate, not by my own tests, which had only
supplied inline text.
Tests: assertions surfaced from inline text AND from a log path; an
unreadable log stays structural; an infra-only failure keeps the terse
record. The existing structural test is untouched and passes, which is the
guard that this does not weaken the classification.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95283dea0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| shown = ", ".join(found[:5]) | ||
| more = f" (+{len(found) - 5} more)" if len(found) > 5 else "" |
There was a problem hiding this comment.
When an infrastructure-failed run reports more than five assertions, this truncates the only persisted record to five IDs and replaces the rest with a count. Because infra_failures is subsequently rendered into FINAL_SUMMARY.md, the documented 10-failure scenario still hides half of the exact regressions and may conceal a separate failure cluster. Persist the complete ID list, or apply truncation only in a presentation layer where the full data remains available.
Useful? React with 👍 / 👎.
The miss this fixes
During the v0.30.0 campaign the run summary read:
That reads as "clean apart from an infrastructure nuisance". It wasn't.
simple_diffusion_testhad reported 10 quantization assertion failures and was then killed. Because the infra branch recorded only the label and moved on, those ids were discarded with the log.The push gate blocked on that signal and was right — but its message named only a timeout, which made a correct block look like flakiness worth overriding. The 10 failures stayed invisible until Buildkite #3055 re-found them, as two distinct upstream contract changes (vllm-project/vllm-omni#7820).
Change
test_loopstill classifies timeout / watchdog kill / harness crash as structural (Rev 8 §2.3): out offailed_tests, no baseline split, no debug agent, push gate blocks. That is unchanged — the existingtest_test_loop_infra_is_structuralis untouched and passes, which is the guard.What changes is that the infra record now names what the run did find:
Reuses
ci_loop.extract_failed_test_idsrather than adding a second parser.One thing worth reviewing closely
TestRunResult.outputcarries either log text or a path to the log file — the production caller (engine/steps/rebase_v3._to_result) passesoutcome.log_file. My first version parsed the pathname and would have been inert in production; my own tests missed it because they supplied inline text. The staged-diff review gate caught it._log_text()now reads the file whenoutputis one, and degrades to "no ids found" on a read error so a filesystem problem can never change the classification.Tests
Four new cases: assertions surfaced from inline text; from a log path (the production shape); an unreadable log stays structural; an infra-only failure keeps the terse record.
Full suite green;
check_spec_freshness --strict0 stale.🤖 Generated with Claude Code