From 95283dea0eda5a829e73f6b12ac17642be51f3cb Mon Sep 17 00:00:00 2001 From: tzhouam Date: Mon, 21 Sep 2026 00:09:02 +0800 Subject: [PATCH] rebase: surface assertions an infra-failed run already reported 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) --- doc/architecture/SPEC/rebase_engine.md | 12 ++- .../rebase_engine/test_loop.py | 45 +++++++- test/test_assembly.py | 102 ++++++++++++++++++ 3 files changed, 157 insertions(+), 2 deletions(-) diff --git a/doc/architecture/SPEC/rebase_engine.md b/doc/architecture/SPEC/rebase_engine.md index 66f834ea0..e4e154a34 100644 --- a/doc/architecture/SPEC/rebase_engine.md +++ b/doc/architecture/SPEC/rebase_engine.md @@ -1,6 +1,6 @@ # rebase_engine/ —— 规范 - + `LOC ~7500（26 个模块） · repo-rebase-v3 的原生 rebase 引擎 · refactor-status: ok` @@ -144,3 +144,13 @@ provider；两条路径共用同一 prompt、同一工具面与同一 plan gate 循环而不是判 module 失败——但重启会丢掉已积累的上下文，所以预算过低时 agent 会反复从头探索。flash 级模型在同一 module 上约需 pro 级 2.3 倍轮次 （140+ vs 62），150 的旧上限会被截断三次仍未完成。 + +## 结构性失败仍要报告已产生的断言（2026-09-21） + +`test_loop` 把 timeout / watchdog kill / harness crash 归为**结构性**失败： +不进 `failed_tests`、不做 baseline 对比、push gate 直接阻断——这部分不变。 +但结构性结束**不等于**这次运行什么都没发现：一个 suite 可能先报了断言失败 +再被杀掉。那些 node id 过去随 output 一起丢弃，报告只剩"1 个 infra 失败"。 +现在 infra 记录里会带上已报告的断言 id（复用 `ci_loop.extract_failed_test_ids`）。 +实例：2026-09-19 `simple_diffusion_test` 超时前已报 10 个量化断言失败，全部不可见， +直到 Buildkite #3055 重新发现。 diff --git a/src/infermatrix_copilot/rebase_engine/test_loop.py b/src/infermatrix_copilot/rebase_engine/test_loop.py index 2827b6107..7307eb496 100644 --- a/src/infermatrix_copilot/rebase_engine/test_loop.py +++ b/src/infermatrix_copilot/rebase_engine/test_loop.py @@ -27,8 +27,33 @@ from .substate import Substate + +from .ci_loop import extract_failed_test_ids + log = logging.getLogger(__name__) +def _log_text(output: str) -> str: + """Return the log CONTENTS for `TestRunResult.output`. + + `output` carries either the log text or a PATH to the log file — the + production caller (`engine/steps/rebase_v3._to_result`) passes + `outcome.log_file`. Parsing the pathname finds nothing, so read the file + when it is one. A read failure must never change the structural + classification, so it degrades to "no ids found". + """ + if not output: + return "" + if "\n" not in output and len(output) < 4096: + try: + candidate = Path(output) + if candidate.is_file(): + return candidate.read_text(errors="replace") + except OSError: + return "" + return output + + + @dataclass(frozen=True) class TestRunResult: @@ -141,9 +166,27 @@ def checkpoint(current: str = "") -> None: # STRUCTURAL (Rev 8 §2.3): timeouts / watchdog kills / harness # crashes never enter the baseline-vs-regression split or the # assertion pass-through — the push gate blocks on these + # + # A structural end does NOT mean the run produced no findings: a + # suite can report assertion failures and THEN be killed. Those + # ids were previously discarded with the output, so the report + # read "1 infra failure" while the log held real regressions + # (observed 2026-09-19: simple_diffusion_test timed out after 10 + # quantization assertions, all invisible until CI re-found them). + # Surface them in the record; the classification is unchanged. + found = sorted(extract_failed_test_ids(_log_text(result.output))) + detail = f"{slug}: {result.infra}" + if found: + shown = ", ".join(found[:5]) + more = f" (+{len(found) - 5} more)" if len(found) > 5 else "" + detail += (f" — {len(found)} assertion failure(s) reported " + f"before the structural end: {shown}{more}") + log.warning(" INFRA FAILURE (%s) with %d assertion " + "failure(s) already reported: %s", result.infra, + len(found), shown) log.warning(" INFRA FAILURE (%s): %s (rc=%d)", result.infra, label, result.rc) - infra_failures.append(f"{slug}: {result.infra}") + infra_failures.append(detail) infra_slugs.add(slug) failed += 1 checkpoint() diff --git a/test/test_assembly.py b/test/test_assembly.py index 98d7ad698..f339d3d26 100644 --- a/test/test_assembly.py +++ b/test/test_assembly.py @@ -263,6 +263,108 @@ def baseline_fn(slug): assert result2["infra_failures"] == ["timeout: timeout"] +def test_infra_failure_surfaces_assertions_it_already_reported(tmp_path): + """A structural end does not mean the run found nothing. + + A suite can report assertion failures and THEN be killed. The loop must + keep the STRUCTURAL classification (out of `failed_tests`, push gate + blocks) while still naming the ids in the record -- otherwise the report + reads "1 infra failure" and real regressions stay invisible. + + Regression test for 2026-09-19: simple_diffusion_test timed out after + reporting 10 quantization assertion failures; all 10 were dropped with + the output and only CI re-found them. + """ + sub = Substate(tmp_path, "run-surface") + jobs = [{"slug": "killed", "label": "killed", "min_gpus": 1}] + output = ( + "FAILED tests/diffusion/models/glm_image/test_q.py::TestA::test_one\n" + "FAILED tests/diffusion/models/glm_image/test_q.py::TestB::test_two\n" + "[watchdog/kill] tier=1 test=killed\n" + ) + runs = {"killed": tl.TestRunResult(1, output=output, infra="timeout")} + + async def debug_fn(slug, label, rc, out): + return False + + result = asyncio.run(tl.run_test_loop( + jobs, substate=sub, run_fn=lambda s: runs[s], + baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn)) + + # classification is unchanged: still structural, still not an assertion + assert result["failed_tests"] == [] + assert result["failed"] == 1 + record = result["infra_failures"][0] + assert record.startswith("killed: timeout") + # ...but the findings it DID produce are now named + assert "2 assertion failure(s)" in record + assert "TestA::test_one" in record and "TestB::test_two" in record + + +def test_infra_failure_reads_assertions_from_a_log_PATH(tmp_path): + """The production caller passes a PATH, not log text. + + `engine/steps/rebase_v3._to_result` sets TestRunResult.output to + `outcome.log_file`. Parsing the pathname finds nothing, so the loop must + read the file -- otherwise this surfacing never fires for the real + timeout/watchdog runs it exists for. + """ + sub = Substate(tmp_path, "run-path") + log_file = tmp_path / "simple_diffusion_test.log" + log_file.write_text( + "collected 5802 items\n" + "FAILED tests/diffusion/models/glm_image/test_q.py::TestA::test_one\n" + "FAILED tests/diffusion/models/minimax_h3/test_c.py::test_two\n" + "[watchdog/kill] tier=1 test=simple_diffusion_test\n" + ) + jobs = [{"slug": "killed", "label": "killed", "min_gpus": 1}] + runs = {"killed": tl.TestRunResult(1, output=str(log_file), infra="timeout")} + + async def debug_fn(slug, label, rc, out): + return False + + result = asyncio.run(tl.run_test_loop( + jobs, substate=sub, run_fn=lambda s: runs[s], + baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn)) + record = result["infra_failures"][0] + assert "2 assertion failure(s)" in record + assert "TestA::test_one" in record + assert result["failed_tests"] == [] # still structural + + +def test_infra_failure_unreadable_log_stays_structural(tmp_path): + """An unreadable/missing log must not change the classification.""" + sub = Substate(tmp_path, "run-unreadable") + jobs = [{"slug": "gone", "label": "gone", "min_gpus": 1}] + missing = str(tmp_path / "does_not_exist.log") + runs = {"gone": tl.TestRunResult(1, output=missing, infra="timeout")} + + async def debug_fn(slug, label, rc, out): + return False + + result = asyncio.run(tl.run_test_loop( + jobs, substate=sub, run_fn=lambda s: runs[s], + baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn)) + assert result["infra_failures"] == ["gone: timeout"] + assert result["failed_tests"] == [] + + +def test_infra_failure_without_assertions_is_unchanged(tmp_path): + """A genuine infra-only failure keeps the original terse record.""" + sub = Substate(tmp_path, "run-terse") + jobs = [{"slug": "oom", "label": "oom", "min_gpus": 1}] + runs = {"oom": tl.TestRunResult(1, output="Killed\n", infra="timeout")} + + async def debug_fn(slug, label, rc, out): + return False + + result = asyncio.run(tl.run_test_loop( + jobs, substate=sub, run_fn=lambda s: runs[s], + baseline_fn=lambda s: tl.TestRunResult(0), debug_fn=debug_fn)) + assert result["infra_failures"] == ["oom: timeout"] + + + # -- CI build ledger + monitor ------------------------------------------------- class FakeCI: