From ed8cdb3519408889e9eb2ac6eb3a87fd76f6190e Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 03:47:20 -0400 Subject: [PATCH 1/4] test: an app-source diff runs the suites that mention what it changed Fails today: choose_ci_suite.py takes no app diff, so a pull request that changes Sources/ without touching cmuxTests/ runs no behavior test. #12822 changed AgentQuitProcessOwnership and main's agent-restore suites broke. Co-Authored-By: Claude Opus 5.5 --- tests/test_ci_change_areas.py | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index 0e937718295c..e42274c41016 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -4809,6 +4809,49 @@ def outputs(paths: list[str], hunks: str | None = None) -> dict[str, str]: assert result["unit_in_admission"] == "false", result +def test_an_app_source_diff_runs_the_suites_that_mention_what_it_changed() -> None: + """#12822 changed AgentQuitProcessOwnership in Sources/ and main broke. + + A pull request that changes app code runs the suites whose tests mention + the changed declaration, on the changed-suites run, instead of no behavior + test at all. Without a readable app diff it adds nothing. + """ + script = ROOT / "scripts/ci/choose_ci_suite.py" + path = "Sources/App/AgentQuitProcessOwnership.swift" + declaration = next( + number for number, line in enumerate((ROOT / path).read_text(encoding="utf-8").splitlines(), 1) + if line.startswith("struct AgentQuitProcessOwnership") + ) + with tempfile.TemporaryDirectory() as directory: + changed = Path(directory) / "changed.txt" + changed.write_text(f"{path}\n") + labels = Path(directory) / "labels.txt" + labels.write_text("") + app_diff = Path(directory) / "app.diff" + + def outputs(hunks: str | None) -> dict[str, str]: + extra = [] + if hunks is not None: + app_diff.write_text(hunks) + extra = ["--app-diff-from", str(app_diff)] + run = subprocess.run( + [sys.executable, str(script), "--event-name", "pull_request", + "--pull-request-policy", "compile-only", "--labels-file", str(labels), + "--files-from", str(changed), "--root", str(ROOT), *extra], + capture_output=True, text=True, check=True, + ) + return dict(line.split("=", 1) for line in run.stdout.splitlines()) + + result = outputs(f"--- a/{path}\n+++ b/{path}\n@@ -{declaration},1 +{declaration},1 @@\n") + assert result["unit_suite"] == "true", result + assert result["unit_canary"] == "false", result + assert "cmuxTests/AgentQuitOwnershipTests" in result["unit_selectors"].split(), result + + for unreadable in (None, ""): + result = outputs(unreadable) + assert "cmuxTests/AgentQuitOwnershipTests" not in result["unit_selectors"].split(), result + + def test_compile_admission_runs_changed_suites_that_need_no_worker() -> None: """A few changed suites run on the runner that compiled them. From b09d88ab023eec520482cb547a5f5b407f8a7f2f Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 04:35:09 -0400 Subject: [PATCH 2/4] ci: run the suites that mention an app-source change on the changed-suites run A pull request that changes Sources/ or a macOS/Shared package without touching cmuxTests/ ran no behavior test, and #14044, #12822 and #13055 each broke suites they never ran. choose_ci_suite.py now takes the app diff and adds the suites reverse_test_impact.py names, within what the changed-suites budget has left. It only adds: no readable diff or a selector error adds nothing, and a suite that would overflow the budget or leave the changed-suites lane is dropped rather than widening the run to seven shards. Suites with known-main failures are skipped, since a known failure that passes fails a changed-suites run. On #12822's tree the run goes from its 4 edited suites to those plus the selector's picks, 6.3 min of measured test time; selection takes ~10 s. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 17 ++++++--- scripts/ci/choose_ci_suite.py | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6c85c186a04..f15d94f7cee8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -190,6 +190,10 @@ jobs: # Absent reads as "every line of each changed file". git diff --no-renames -U0 "$BASE_SHA" "$MERGE_SHA" -- cmuxTests .github/workflows/ci-macos.yml \ > /tmp/cmux-ci-tests.diff || rm -f /tmp/cmux-ci-tests.diff + # The changed lines of app code, so choose_ci_suite.py can add the + # suites whose tests mention what changed (reverse_test_impact.py). + git diff --no-renames -U0 "$BASE_SHA" "$MERGE_SHA" -- Sources Packages/macOS Packages/Shared CLI \ + > /tmp/cmux-ci-app.diff || rm -f /tmp/cmux-ci-app.diff if [ ! -s /tmp/cmux-ci-changed-files.txt ]; then echo "PR diff is empty; skipping product-area CI." @@ -592,6 +596,9 @@ jobs: if [ -f /tmp/cmux-ci-tests.diff ]; then files_args+=(--diff-from /tmp/cmux-ci-tests.diff) fi + if [ -f /tmp/cmux-ci-app.diff ]; then + files_args+=(--app-diff-from /tmp/cmux-ci-app.diff) + fi python3 scripts/ci/choose_ci_suite.py \ --event-name "$EVENT_NAME" \ --pull-request-policy "$PULL_REQUEST_POLICY" \ @@ -903,11 +910,11 @@ jobs: python3 scripts/ci/find_admitted_build.py --repository "$GITHUB_REPOSITORY" --branch "$BRANCH" \ --fingerprint "$FINGERPRINT" --current-run-id "$GITHUB_RUN_ID" --github-output "$GITHUB_OUTPUT" - # Report only. A pull request that changes app code but no cmuxTests/ file - # runs no app-host behavior test today; reverse_test_impact.py names the - # suites that could observe the change and what they would cost. The answer - # goes to the step summary and a 14-day artifact so its recall can be - # measured against later failures on main. It is its own job so it never + # Report only. choose_ci_suite.py in `changes` already adds the suites + # reverse_test_impact.py names to the changed-suites run; this job records + # the full answer, what was considered and what it would cost, in the step + # summary and a 14-day artifact so its recall can be measured against later + # failures on main. It is its own job so it never # delays routing: nothing needs it, ci-status does not wait for it, it has # no outputs, and it cannot fail the run (continue-on-error at job level). # diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index fef29872fe8c..a0defadb1e09 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -232,6 +232,56 @@ def changed_unit_selectors( return suites +def reverse_unit_selectors( + root: Path, paths: Iterable[str] | None, app_diff: str | None, already: list[str] +) -> list[str]: + """Suites that could observe an app-source change, within what the budget has left. + + A pull request that changes Sources/ or a macOS/Shared package without + touching cmuxTests/ otherwise runs no behavior test. reverse_test_impact.py + names the suites whose tests mention what the diff changed; this keeps the + ones that fit beside `already` in one changed-suites run. It only adds: + anything it cannot judge (no diff, a selector error) adds nothing, and a + suite that would push the run past its budget or out of the changed-suites + lane is left out rather than turning the run into seven shards. Suites + with entries in app-host-known-failures.json are left out too: a known + failure that happens to pass fails a changed-suites run, which is right for + a suite the pull request edited and wrong for one it only reached. + """ + if paths is None or app_diff is None or not app_diff.strip(): + return [] + try: + import reverse_test_impact as reverse + + if not any(reverse.is_app_path(path.strip()) for path in paths): + return [] + selection = reverse.select(reverse.read_root(root), app_diff) + if not selection.reached: + return [] + timings = load_timings(DEFAULT_TIMINGS_PATH) + default_ms = (timings or {}).get("default_test_ms", reverse.FALLBACK_TEST_MS) + costs = reverse.suite_costs(root, timings) + spent = sum(costs.get(selector.split("/", 1)[1], default_ms) for selector in already) + if spent >= CHANGED_SUITES_BUDGET_MS: + return [] + catalog = json.loads((root / "scripts/ci/app-host-known-failures.json").read_text(encoding="utf-8")) + known = {identifier.split("/", 1)[0] for identifier in catalog.get("tests", {})} + workflow = (root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + data = reverse.report(selection, costs, default_ms, CHANGED_SUITES_BUDGET_MS - spent) + chosen: list[str] = [] + for suite in data["would_run"]: + selector = f"cmuxTests/{suite}" + if suite in known or selector in already: + continue + if strict_steps(workflow, already + chosen + [selector]) is None: + continue + chosen.append(selector) + return chosen + except Exception as error: # an addition only: never the reason a run fails + print(f"::warning::Reverse test impact selection failed: {error!r}", file=sys.stderr) + return [] + + def job_lines(workflow: str, job: str) -> range | None: """1-based line numbers of `job` in a workflow's text, header included.""" lines = workflow.splitlines() @@ -404,6 +454,10 @@ def main(argv: list[str]) -> int: "--diff-from", help="`git diff -U0` of cmuxTests/ and ci-macos.yml; omit to count every line of a changed file", ) + parser.add_argument( + "--app-diff-from", + help="`git diff -U0` of Sources/, Packages/ and CLI/; adds the suites that could observe it", + ) parser.add_argument("--root", type=Path, default=Path.cwd()) args = parser.parse_args(argv) @@ -429,6 +483,13 @@ def main(argv: list[str]) -> int: except (OSError, UnicodeError): diff = None + app_diff = None + if args.app_diff_from: + try: + app_diff = Path(args.app_diff_from).read_text(encoding="utf-8", errors="replace") + except OSError: + app_diff = None + full = wants_full_suite(args.event_name, args.pull_request_policy, labels) unit = wants_unit_suite(args.event_name, args.pull_request_policy, labels, paths) gap = coverage_gap(args.event_name, full, paths, labels, unit_suite=unit) @@ -437,6 +498,14 @@ def main(argv: list[str]) -> int: asked_for_every_suite = full or UNIT_SUITE_LABEL in {label.strip() for label in labels or ()} selectors = [] if not unit or asked_for_every_suite else changed_unit_selectors(args.root, paths, diff) canary = False + # A narrowed run (or none yet) also takes the suites that could observe + # the app-source change; an empty `selectors` under `unit` is already + # every suite. + if not asked_for_every_suite and (selectors or not unit): + reached = reverse_unit_selectors(args.root, paths, app_diff, selectors) + if reached: + selectors = selectors + reached + unit = True if not unit: # Nothing else asked for the unit tests, so a consumer edit takes the # one-suite canary rather than seven shards. ci.yml drops it again when From f106fda1c0e1759752bc07e5ac561774cef078be Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 04:42:52 -0400 Subject: [PATCH 3/4] ci: add only real batch suites, keep strict steps and admission unchanged Review of the first cut: the selector also names helper types in cmuxTests/ (SidebarTestManualClock, RejectingRestoreTabDelegate), and a selector that matches no test fails the run. Only suites the shared batch discovers are added now, and none whose addition changes the strict steps, since those run outside the budget. A run the selector alone asked for is a canary, so a re-push of admitted inputs still reuses the build, and reached suites take the changed-suites worker instead of adding up to the whole budget to compile admission. On #12822's tree: 373 suites, 9.1 min of measured time, no strict steps, off admission. Co-Authored-By: Claude Opus 5.5 --- scripts/ci/choose_ci_suite.py | 27 ++++++++++++++++++++------- tests/test_ci_change_areas.py | 5 ++++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index a0defadb1e09..c7639bb16a82 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -243,10 +243,14 @@ def reverse_unit_selectors( ones that fit beside `already` in one changed-suites run. It only adds: anything it cannot judge (no diff, a selector error) adds nothing, and a suite that would push the run past its budget or out of the changed-suites - lane is left out rather than turning the run into seven shards. Suites - with entries in app-host-known-failures.json are left out too: a known - failure that happens to pass fails a changed-suites run, which is right for - a suite the pull request edited and wrong for one it only reached. + lane is left out rather than turning the run into seven shards. Only + suites the shared batch discovers are added: the selector also names + helper types in cmuxTests/, and a selector that matches no test fails the + run. A suite a strict step owns is left out, since that step runs apart + from the budget. Suites with entries in app-host-known-failures.json are + left out too: a known failure that happens to pass fails a changed-suites + run, which is right for a suite the pull request edited and wrong for one + it only reached. """ if paths is None or app_diff is None or not app_diff.strip(): return [] @@ -267,13 +271,15 @@ def reverse_unit_selectors( catalog = json.loads((root / "scripts/ci/app-host-known-failures.json").read_text(encoding="utf-8")) known = {identifier.split("/", 1)[0] for identifier in catalog.get("tests", {})} workflow = (root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + batch_suites = {selector.identifier.split("/")[1] for selector in discover_selectors(root)} data = reverse.report(selection, costs, default_ms, CHANGED_SUITES_BUDGET_MS - spent) chosen: list[str] = [] for suite in data["would_run"]: selector = f"cmuxTests/{suite}" - if suite in known or selector in already: + if suite in known or suite not in batch_suites or selector in already: continue - if strict_steps(workflow, already + chosen + [selector]) is None: + steps = strict_steps(workflow, already + chosen) + if strict_steps(workflow, already + chosen + [selector]) != steps: continue chosen.append(selector) return chosen @@ -498,12 +504,17 @@ def main(argv: list[str]) -> int: asked_for_every_suite = full or UNIT_SUITE_LABEL in {label.strip() for label in labels or ()} selectors = [] if not unit or asked_for_every_suite else changed_unit_selectors(args.root, paths, diff) canary = False + reached: list[str] = [] # A narrowed run (or none yet) also takes the suites that could observe # the app-source change; an empty `selectors` under `unit` is already # every suite. if not asked_for_every_suite and (selectors or not unit): reached = reverse_unit_selectors(args.root, paths, app_diff, selectors) if reached: + # Alone, these ride on a compile this run pays for, like the + # consumer canary: a re-push of admitted inputs reuses the build + # and drops them rather than compiling again. + canary = not unit selectors = selectors + reached unit = True if not unit: @@ -516,7 +527,9 @@ def main(argv: list[str]) -> int: if selectors: workflow = (args.root / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") steps = strict_steps(workflow, selectors) or [] - in_admission = runs_in_admission(args.root, paths, diff, selectors, steps, canary) + # Suites reached this way can fill the whole budget; they take the + # changed-suites worker rather than holding compile admission. + in_admission = runs_in_admission(args.root, paths, diff, selectors, steps, canary or bool(reached)) lines = [ f"full_suite={'true' if full else 'false'}", f"unit_suite={'true' if unit else 'false'}", diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index e42274c41016..7ba5b7e81f61 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -4844,7 +4844,10 @@ def outputs(hunks: str | None) -> dict[str, str]: result = outputs(f"--- a/{path}\n+++ b/{path}\n@@ -{declaration},1 +{declaration},1 @@\n") assert result["unit_suite"] == "true", result - assert result["unit_canary"] == "false", result + # Like the consumer canary, these ride only on a compile the run pays + # for, and take the changed-suites worker rather than admission. + assert result["unit_canary"] == "true", result + assert result["unit_in_admission"] == "false", result assert "cmuxTests/AgentQuitOwnershipTests" in result["unit_selectors"].split(), result for unreadable in (None, ""): From 3f610dcaf41ea571d82b66d8fb4408e09ce611bb Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 04:49:17 -0400 Subject: [PATCH 4/4] ci: keep the consumer canary beside reached suites, and test the filters Re-review follow-ups: a consumer edit that also changes app source kept only the reached suites and lost cmuxTests/CmuxSSHURLRequestTests; it now keeps both. The ci.yml note on canary runs names reached suites too, and the regression test checks every selector is a suite the shared batch runs and that no strict step is added. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 5 +++-- scripts/ci/choose_ci_suite.py | 6 ++++++ tests/test_ci_change_areas.py | 7 +++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f15d94f7cee8..6eae56c3eeec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,8 +69,9 @@ jobs: linux_guard_source: ${{ steps.linux_guards.outputs.linux_guard_source }} ghosttykit_release: ${{ steps.unchanged_inputs.outputs.compile_admitted == 'true' && 'false' || steps.linux_guards.outputs.ghosttykit_release }} full_suite: ${{ steps.suite.outputs.full_suite }} - # A consumer canary (choose_ci_suite.py) only rides on a compile this run - # pays for; when either reuse check finds one, it is dropped. + # A canary run (choose_ci_suite.py: the consumer canary, or suites only + # the reverse test impact selector reached) only rides on a compile this + # run pays for; when either reuse check finds one, it is dropped. unit_suite: ${{ steps.suite.outputs.unit_canary == 'true' && (steps.unchanged_inputs.outputs.compile_admitted == 'true' || steps.admitted.outputs.compile_admitted == 'true') && 'false' || steps.suite.outputs.unit_suite }} unit_selectors: ${{ !(steps.suite.outputs.unit_canary == 'true' && (steps.unchanged_inputs.outputs.compile_admitted == 'true' || steps.admitted.outputs.compile_admitted == 'true')) && steps.suite.outputs.unit_selectors || '' }} unit_strict_steps: ${{ steps.suite.outputs.unit_strict_steps }} diff --git a/scripts/ci/choose_ci_suite.py b/scripts/ci/choose_ci_suite.py index c7639bb16a82..ce134d244914 100755 --- a/scripts/ci/choose_ci_suite.py +++ b/scripts/ci/choose_ci_suite.py @@ -515,6 +515,12 @@ def main(argv: list[str]) -> int: # consumer canary: a re-push of admitted inputs reuses the build # and drops them rather than compiling again. canary = not unit + if canary: + # Keep the consumer canary a consumer edit would have taken. + selectors = [ + selector for selector in consumer_canary_selectors(args.root, paths, diff) + if selector not in reached + ] selectors = selectors + reached unit = True if not unit: diff --git a/tests/test_ci_change_areas.py b/tests/test_ci_change_areas.py index 7ba5b7e81f61..a4a1978f0723 100755 --- a/tests/test_ci_change_areas.py +++ b/tests/test_ci_change_areas.py @@ -4849,6 +4849,13 @@ def outputs(hunks: str | None) -> dict[str, str]: assert result["unit_canary"] == "true", result assert result["unit_in_admission"] == "false", result assert "cmuxTests/AgentQuitOwnershipTests" in result["unit_selectors"].split(), result + # Only suites the shared batch runs: the selector also names helper + # types, and a selector matching no test fails the run. + sys.path.insert(0, str(ROOT / "scripts/ci")) + from cmux_unit_test_shard import discover_selectors + batch = {f"cmuxTests/{s.identifier.split('/')[1]}" for s in discover_selectors(ROOT)} + assert set(result["unit_selectors"].split()) <= batch, set(result["unit_selectors"].split()) - batch + assert result["unit_strict_steps"] == "", result for unreadable in (None, ""): result = outputs(unreadable)