ci: make a changed-suites run prove a known-failure fix - #14307
Conversation
The app-host ratchet tolerates every test listed in app-host-known-failures.json. In a PR's changed-suites run that makes a fix unprovable: #14076 went green while the suite it fixed failed 4/4, because the test was already listed. In changed-suites mode (CMUX_APP_HOST_UNIT_SELECTORS set), a listed test that passes now fails the run with RATCHET_KNOWN_NOW_PASSING and asks the PR to drop the entry. Once it is dropped, the same run must pass on its own merits. Main's full shards only report it, since a flaky entry can pass on any one run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe accounting command now detects known failures that pass. For changed suites, it reports those tests and fails the run until their entries are removed from the known-failures catalog. The batch runner enables this check when suite selectors are set. ChangesChanged-suite known-failure accounting
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Changed-suite runs now flag passing tests that remain in the known-failures catalog. No actionable merge-blocking issue remains after normal checks. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3d5b648 test: give the Cloud Desktop fixture a routable window for pane drops (manaflow-ai#14304) 9d53af4 ci: give a refused owned job one more try on the fleet before Blacksmith (manaflow-ai#14312) 379091b ci: let PR runs overflow to macOS 15 at 4 queued jobs deeper, not 12 (manaflow-ai#14319) df6efb5 test(cloud): key the refresh URL protocol stub per request, not by address (manaflow-ai#14239) 51bd322 ci: build only the CLI product for CLI-only changes (manaflow-ai#14212) 0345a5c ci: make a changed-suites run prove a known-failure fix (manaflow-ai#14307) 370b7f6 ci: fix the owned build state save step's argument count (manaflow-ai#14309) 319adff test: wait for the SSH cleanup policy bound after a restored-attach signal (manaflow-ai#14305) 0cc5be3 fix(portal): flush the coalesced live-resize pass on its first hop (manaflow-ai#14297) 5887891 test(minimal-mode): measure the toggle only after setup stops re-rendering (manaflow-ai#14298) 5fbcc48 ci: charge newer PR runs what they took on the owned pool, not a guess (manaflow-ai#14300) # Conflicts: # .github/workflows/ci-macos.yml
A PR that fixes a test listed in
scripts/ci/app-host-known-failures.jsoncurrently goes green whether or not the fix works, because the app-host ratchet tolerates every listed failure. #14076 merged that way: its changed-suites lane ran the suite it was fixing, failed 4 of 4 cases, and reported success (RATCHET_KNOWN_FAILURE ... known-main failures tolerated: 1).In a changed-suites run (
CMUX_APP_HOST_UNIT_SELECTORSset), a listed test that passes now fails the run withRATCHET_KNOWN_NOW_PASSING <id>and a line saying to drop the entry. With the entry removed, the run has to pass on its own. So a fix PR carries its catalog removal, and its green check means the test passed. Main's full shards only logRATCHET_KNOWN_NOW_PASSING, because a flaky entry can pass on any single run and main shouldn't fail for that.A PR that edits a listed suite for an unrelated reason and happens to make it pass will also be asked to drop the entry. That's intended: the catalog only shrinks, and a passing run in the PR that touched the suite is the evidence for shrinking it.
This applies to every catalog classification, including
timeout/hangandunknown, because #14076's target was atimeout/hangentry. The cost: a PR whose selected suites include a flaky entry can be asked to drop it after one lucky pass. Selection also pulls in suites that only reference a changed helper, not just suites the PR edited. If that turns out noisy, the next step is to limit the hard fail to suites the PR edits directly. Suites run as strict focused gates (FOCUSED_GATE_SELECTORS) never reachcheck-runand aren't covered.Validation, local (no build):
python3 tests/test_ci_app_host_result_accounting.pypasses, including a new case covering main-shard reporting, the changed-suites failure, and the pass after removal. The 7 ratchet and batch tests intests/test_ci_change_areas.pypass, including the one that drivesrun-app-host-unit-batches.shagainst a fake runner.bash -nis clean on the batch script.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Makes changed-suites runs fail when a test listed in
app-host-known-failures.jsonpasses, so a fix PR's green check now proves the fix works. Previously the ratchet tolerated every listed failure, so a fix could merge with the target test still failing.Behavior
RATCHET_KNOWN_NOW_PASSINGand fail until the entry is removed fromapp-host-known-failures.json; the run must then pass on its own.RATCHET_KNOWN_FAILURElines even when a different entry passes.Written for commit d3ac629. Summary will update on new commits.
Summary by CodeRabbit