From a97b4041886c6e0ef25881ce8812e6cb52cee88b Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 23 Sep 2026 03:16:24 -0700 Subject: [PATCH] test: guard the DerivedData boundary app-host cleanup enforces The workspace-rooted path this PR moves was invisible to every check. `check_every_app_host_home_is_identified_and_cleaned` already finds each job that prepares an app-host home and holds it to that contract, but it asserted only the two preconditions `prepare` needs, not the one `cleanup` enforces at runtime: DerivedData under RUNNER_TEMP. Extending that loop catches the shipped bug at its pattern rather than its instance. Reverting the workflow to the pre-fix path fails it, as does moving ci-macos.yml's shard DerivedData out of RUNNER_TEMP or dropping the publishing step. The test job's own `Clean owned DerivedData` arm also had no coverage: `step()` resolves an ambiguous name to the build job, so a typo in the test job's ownership pattern shipped green and failed only on a runner, under `if: always()`, after the tests had passed. Co-Authored-By: Claude Opus 5 --- tests/test_ci_app_host_home_isolation.py | 53 ++++++++++++++++++++++++ tests/test_ci_e2e_compilation_cache.py | 29 +++++++++++++ 2 files changed, 82 insertions(+) diff --git a/tests/test_ci_app_host_home_isolation.py b/tests/test_ci_app_host_home_isolation.py index 922dbb45654b..ffeab78a73b9 100644 --- a/tests/test_ci_app_host_home_isolation.py +++ b/tests/test_ci_app_host_home_isolation.py @@ -3,6 +3,7 @@ from pathlib import Path import os +import re import subprocess import tempfile import xml.etree.ElementTree as ET @@ -181,6 +182,57 @@ def acceptance_gate_problem(condition: object, preparation_id: str) -> str: return "" +def published_derived_data_value(job, steps) -> str | None: + """Return the CMUX_DERIVED_DATA_PATH a job publishes, before expansion. + + Both callers compute the path in a shell variable and export it through + `GITHUB_ENV`, so the literal that matters is the assignment, not the echo. + """ + environment = job.get("env") + if isinstance(environment, dict) and environment.get("CMUX_DERIVED_DATA_PATH"): + return str(environment["CMUX_DERIVED_DATA_PATH"]) + for step in steps: + script = str(step.get("run", "")) + export = re.search( + r'CMUX_DERIVED_DATA_PATH=(?P[^"\n]*)"?\s*>>\s*"?\$(?:\{)?GITHUB_ENV', + script, + ) + if export is None: + continue + value = export.group("value").strip() + name = re.fullmatch(r"\$\{?(?P[A-Za-z_][A-Za-z0-9_]*)\}?", value) + if name is None: + return value + assignment = re.search( + rf'^\s*{name.group("name")}="(?P[^"]*)"', script, re.MULTILINE + ) + return assignment.group("path") if assignment else None + return None + + +def require_derived_data_under_runner_temp(where, job, steps) -> None: + """Hold app-host callers to the boundary cleanup enforces at runtime. + + `cleanup-app-host-home.sh` refuses to inspect a host whose DerivedData + lives outside `RUNNER_TEMP`, and it runs under `if: always()`, so a job + that parks DerivedData anywhere else goes red *after* its tests pass. + `test-e2e.yml` shipped exactly that: the split lane inherited a + workspace-rooted path from the single-job form, which no other check + looked at because no earlier version of that lane cleaned up at all. + """ + value = published_derived_data_value(job, steps) + if value is None: + raise SystemExit( + f"FAIL: {where} prepares an app-host home without publishing " + "CMUX_DERIVED_DATA_PATH; cleanup requires it" + ) + if not re.match(r"\$\{?RUNNER_TEMP\}?/", value): + raise SystemExit( + f"FAIL: {where} puts DerivedData at {value!r}; app-host cleanup " + "only inspects hosts whose DerivedData is under RUNNER_TEMP" + ) + + def check_every_app_host_home_is_identified_and_cleaned() -> None: """Hold every job that prepares an app-host home to the same contract. @@ -216,6 +268,7 @@ def check_every_app_host_home_is_identified_and_cleaned() -> None: f"FAIL: {where} must publish CMUX_APP_HOST_SHARD; " "cmux_resolve_app_host_identity rejects an empty shard" ) + require_derived_data_under_runner_temp(where, job, steps) cleanups = [ step for step in steps if "cleanup-app-host-home.sh" in str(step.get("run", "")) diff --git a/tests/test_ci_e2e_compilation_cache.py b/tests/test_ci_e2e_compilation_cache.py index a832e46609d2..00edb21d08c1 100644 --- a/tests/test_ci_e2e_compilation_cache.py +++ b/tests/test_ci_e2e_compilation_cache.py @@ -204,6 +204,35 @@ def test_cleanup_removes_only_owned_paths(self): self.assertFalse(Path(values['CMUX_DERIVED_DATA_PATH']).exists()) self.assertFalse(Path(values['CMUX_E2E_COMPILATION_CACHE']).exists()) + def prepare_test_job(self): + for file in ('env', 'output'): + (self.root / file).write_text('') + result = self.run_step('Prepare isolated DerivedData', 'test') + self.assertEqual(result.returncode, 0, result.stderr) + return dict(line.split('=', 1) for file in ('env', 'output') + for line in (self.root / file).read_text().splitlines()) + + def test_the_test_job_cleans_up_the_product_it_restored(self): + # This cleanup runs under `if: always()`, so an ownership pattern that + # does not match the job's own prepared path turns a passing test run + # red after the tests have already succeeded. The path also has to stay + # under RUNNER_TEMP: app-host cleanup refuses to inspect a host whose + # DerivedData lives anywhere else. + values = self.prepare_test_job() + derived = Path(values['CMUX_DERIVED_DATA_PATH']) + self.assertTrue(derived.is_relative_to(self.root)) + self.assertFalse(derived.is_relative_to(self.workspace)) + self.assertNotIn('CMUX_E2E_COMPILATION_CACHE', values) + unrelated = self.root / 'keep' + unrelated.mkdir() + rejected = self.run_step('Clean owned DerivedData', 'test', + **dict(values, CMUX_DERIVED_DATA_PATH=str(unrelated))) + self.assertNotEqual(rejected.returncode, 0) + self.assertTrue(unrelated.exists()) + result = self.run_step('Clean owned DerivedData', 'test', **values) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertFalse(derived.exists()) + def test_only_successful_trusted_main_build_can_seed(self): values = self.prepare() cache = Path(values['CMUX_E2E_COMPILATION_CACHE'])