Skip to content

Commit ce1000f

Browse files
tzhouamclaude
andauthored
rebase: surface assertions an infra-failed run already reported (#172)
## The miss this fixes During the v0.30.0 campaign the run summary read: ``` tests: 30 passed, 1 failed (1 infra), 3 skipped ✗ push_gate: precommit red; phase-3 infrastructure failure: simple_diffusion_test: timeout ``` That reads as "clean apart from an infrastructure nuisance". It wasn't. `simple_diffusion_test` had 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_loop` still classifies timeout / watchdog kill / harness crash as **structural** (Rev 8 §2.3): out of `failed_tests`, no baseline split, no debug agent, push gate blocks. **That is unchanged** — the existing `test_test_loop_infra_is_structural` is untouched and passes, which is the guard. What changes is that the infra record now names what the run *did* find: ``` killed: timeout — 2 assertion failure(s) reported before the structural end: tests/…::TestA::test_one, tests/…::TestB::test_two ``` Reuses `ci_loop.extract_failed_test_ids` rather than adding a second parser. ## One thing worth reviewing closely `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`. 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 when `output` is 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 --strict` 0 stale. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 4e1b701 commit ce1000f

3 files changed

Lines changed: 157 additions & 2 deletions

File tree

‎doc/architecture/SPEC/rebase_engine.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# rebase_engine/ —— 规范
22

3-
<!-- verified-against: 2026-09-19 -->
3+
<!-- verified-against: 2026-09-21 -->
44

55
`LOC ~7500(26 个模块) · repo-rebase-v3 的原生 rebase 引擎 · refactor-status: ok`
66

@@ -144,3 +144,13 @@ provider;两条路径共用同一 prompt、同一工具面与同一 plan gate
144144
循环而不是判 module 失败——但重启会丢掉已积累的上下文,所以预算过低时
145145
agent 会反复从头探索。flash 级模型在同一 module 上约需 pro 级 2.3 倍轮次
146146
(140+ vs 62),150 的旧上限会被截断三次仍未完成。
147+
148+
## 结构性失败仍要报告已产生的断言(2026-09-21)
149+
150+
`test_loop` 把 timeout / watchdog kill / harness crash 归为**结构性**失败:
151+
不进 `failed_tests`、不做 baseline 对比、push gate 直接阻断——这部分不变。
152+
但结构性结束**不等于**这次运行什么都没发现:一个 suite 可能先报了断言失败
153+
再被杀掉。那些 node id 过去随 output 一起丢弃,报告只剩"1 个 infra 失败"。
154+
现在 infra 记录里会带上已报告的断言 id(复用 `ci_loop.extract_failed_test_ids`)。
155+
实例:2026-09-19 `simple_diffusion_test` 超时前已报 10 个量化断言失败,全部不可见,
156+
直到 Buildkite #3055 重新发现。

‎src/infermatrix_copilot/rebase_engine/test_loop.py‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,33 @@
2727

2828
from .substate import Substate
2929

30+
31+
from .ci_loop import extract_failed_test_ids
32+
3033
log = logging.getLogger(__name__)
3134

35+
def _log_text(output: str) -> str:
36+
"""Return the log CONTENTS for `TestRunResult.output`.
37+
38+
`output` carries either the log text or a PATH to the log file — the
39+
production caller (`engine/steps/rebase_v3._to_result`) passes
40+
`outcome.log_file`. Parsing the pathname finds nothing, so read the file
41+
when it is one. A read failure must never change the structural
42+
classification, so it degrades to "no ids found".
43+
"""
44+
if not output:
45+
return ""
46+
if "\n" not in output and len(output) < 4096:
47+
try:
48+
candidate = Path(output)
49+
if candidate.is_file():
50+
return candidate.read_text(errors="replace")
51+
except OSError:
52+
return ""
53+
return output
54+
55+
56+
3257

3358
@dataclass(frozen=True)
3459
class TestRunResult:
@@ -141,9 +166,27 @@ def checkpoint(current: str = "") -> None:
141166
# STRUCTURAL (Rev 8 §2.3): timeouts / watchdog kills / harness
142167
# crashes never enter the baseline-vs-regression split or the
143168
# assertion pass-through — the push gate blocks on these
169+
#
170+
# A structural end does NOT mean the run produced no findings: a
171+
# suite can report assertion failures and THEN be killed. Those
172+
# ids were previously discarded with the output, so the report
173+
# read "1 infra failure" while the log held real regressions
174+
# (observed 2026-09-19: simple_diffusion_test timed out after 10
175+
# quantization assertions, all invisible until CI re-found them).
176+
# Surface them in the record; the classification is unchanged.
177+
found = sorted(extract_failed_test_ids(_log_text(result.output)))
178+
detail = f"{slug}: {result.infra}"
179+
if found:
180+
shown = ", ".join(found[:5])
181+
more = f" (+{len(found) - 5} more)" if len(found) > 5 else ""
182+
detail += (f" — {len(found)} assertion failure(s) reported "
183+
f"before the structural end: {shown}{more}")
184+
log.warning(" INFRA FAILURE (%s) with %d assertion "
185+
"failure(s) already reported: %s", result.infra,
186+
len(found), shown)
144187
log.warning(" INFRA FAILURE (%s): %s (rc=%d)", result.infra,
145188
label, result.rc)
146-
infra_failures.append(f"{slug}: {result.infra}")
189+
infra_failures.append(detail)
147190
infra_slugs.add(slug)
148191
failed += 1
149192
checkpoint()

‎test/test_assembly.py‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,108 @@ def baseline_fn(slug):
263263
assert result2["infra_failures"] == ["timeout: timeout"]
264264

265265

266+
def test_infra_failure_surfaces_assertions_it_already_reported(tmp_path):
267+
"""A structural end does not mean the run found nothing.
268+
269+
A suite can report assertion failures and THEN be killed. The loop must
270+
keep the STRUCTURAL classification (out of `failed_tests`, push gate
271+
blocks) while still naming the ids in the record -- otherwise the report
272+
reads "1 infra failure" and real regressions stay invisible.
273+
274+
Regression test for 2026-09-19: simple_diffusion_test timed out after
275+
reporting 10 quantization assertion failures; all 10 were dropped with
276+
the output and only CI re-found them.
277+
"""
278+
sub = Substate(tmp_path, "run-surface")
279+
jobs = [{"slug": "killed", "label": "killed", "min_gpus": 1}]
280+
output = (
281+
"FAILED tests/diffusion/models/glm_image/test_q.py::TestA::test_one\n"
282+
"FAILED tests/diffusion/models/glm_image/test_q.py::TestB::test_two\n"
283+
"[watchdog/kill] tier=1 test=killed\n"
284+
)
285+
runs = {"killed": tl.TestRunResult(1, output=output, infra="timeout")}
286+
287+
async def debug_fn(slug, label, rc, out):
288+
return False
289+
290+
result = asyncio.run(tl.run_test_loop(
291+
jobs, substate=sub, run_fn=lambda s: runs[s],
292+
baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn))
293+
294+
# classification is unchanged: still structural, still not an assertion
295+
assert result["failed_tests"] == []
296+
assert result["failed"] == 1
297+
record = result["infra_failures"][0]
298+
assert record.startswith("killed: timeout")
299+
# ...but the findings it DID produce are now named
300+
assert "2 assertion failure(s)" in record
301+
assert "TestA::test_one" in record and "TestB::test_two" in record
302+
303+
304+
def test_infra_failure_reads_assertions_from_a_log_PATH(tmp_path):
305+
"""The production caller passes a PATH, not log text.
306+
307+
`engine/steps/rebase_v3._to_result` sets TestRunResult.output to
308+
`outcome.log_file`. Parsing the pathname finds nothing, so the loop must
309+
read the file -- otherwise this surfacing never fires for the real
310+
timeout/watchdog runs it exists for.
311+
"""
312+
sub = Substate(tmp_path, "run-path")
313+
log_file = tmp_path / "simple_diffusion_test.log"
314+
log_file.write_text(
315+
"collected 5802 items\n"
316+
"FAILED tests/diffusion/models/glm_image/test_q.py::TestA::test_one\n"
317+
"FAILED tests/diffusion/models/minimax_h3/test_c.py::test_two\n"
318+
"[watchdog/kill] tier=1 test=simple_diffusion_test\n"
319+
)
320+
jobs = [{"slug": "killed", "label": "killed", "min_gpus": 1}]
321+
runs = {"killed": tl.TestRunResult(1, output=str(log_file), infra="timeout")}
322+
323+
async def debug_fn(slug, label, rc, out):
324+
return False
325+
326+
result = asyncio.run(tl.run_test_loop(
327+
jobs, substate=sub, run_fn=lambda s: runs[s],
328+
baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn))
329+
record = result["infra_failures"][0]
330+
assert "2 assertion failure(s)" in record
331+
assert "TestA::test_one" in record
332+
assert result["failed_tests"] == [] # still structural
333+
334+
335+
def test_infra_failure_unreadable_log_stays_structural(tmp_path):
336+
"""An unreadable/missing log must not change the classification."""
337+
sub = Substate(tmp_path, "run-unreadable")
338+
jobs = [{"slug": "gone", "label": "gone", "min_gpus": 1}]
339+
missing = str(tmp_path / "does_not_exist.log")
340+
runs = {"gone": tl.TestRunResult(1, output=missing, infra="timeout")}
341+
342+
async def debug_fn(slug, label, rc, out):
343+
return False
344+
345+
result = asyncio.run(tl.run_test_loop(
346+
jobs, substate=sub, run_fn=lambda s: runs[s],
347+
baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn))
348+
assert result["infra_failures"] == ["gone: timeout"]
349+
assert result["failed_tests"] == []
350+
351+
352+
def test_infra_failure_without_assertions_is_unchanged(tmp_path):
353+
"""A genuine infra-only failure keeps the original terse record."""
354+
sub = Substate(tmp_path, "run-terse")
355+
jobs = [{"slug": "oom", "label": "oom", "min_gpus": 1}]
356+
runs = {"oom": tl.TestRunResult(1, output="Killed\n", infra="timeout")}
357+
358+
async def debug_fn(slug, label, rc, out):
359+
return False
360+
361+
result = asyncio.run(tl.run_test_loop(
362+
jobs, substate=sub, run_fn=lambda s: runs[s],
363+
baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn))
364+
assert result["infra_failures"] == ["oom: timeout"]
365+
366+
367+
266368
# -- CI build ledger + monitor -------------------------------------------------
267369

268370
class FakeCI:

0 commit comments

Comments
 (0)