Skip to content

fix: resolve registry guard base ref at runtime - #15990

Merged
teamleaderleo merged 2 commits into
mainfrom
fix/registry-newly-added-after-catch-up
Sep 30, 2026
Merged

teamleaderleo merged 2 commits into
mainfrom
fix/registry-newly-added-after-catch-up

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

The Python test execution registry guard was using github.event.pull_request.base.sha, which can be an old base tip recorded when a long-lived pull request was synchronized. After a catch-up merge, tests that main added after that stale SHA were attributed to the pull request and legacy-lane checks blocked unrelated work.

For #5677 and #5341, the workflow reported base SHA 9bf6cb8c9421 from 2026-09-17 even though both pull requests were updated on 2026-09-30. Main was at 6d7ad149121a, and commit 224327b55a07 added the registry plus ten legacy tests on 2026-09-24. Those tests were reported against both pull requests.

The workflow now reads github.event.pull_request.base.ref, fetches that branch at depth 1, resolves FETCH_HEAD, and passes the resolved commit to the validator. Push events still run without --base-sha. The shallow fetch is intentional: when merge-base history is unavailable, the validator's two-tree fallback compares directly with the current base tip. The local guard runner now resolves the new expression to its explicit local base revision.

The scratch-repository regression demonstrates both directions: with the stale branch-point SHA, newly_added_tests returns tests/test_mine.py and main's tests/test_theirs.py; with the current main tip, it returns only tests/test_mine.py. The workflow test failed before the fix, failed after mutating the workflow back to base.sha, and passed after restoration.

This unblocks #5677, #5341, and #15711.

Validation: registry tests 31/31, local guard planner tests 17/17, validator --help, and actionlint pass. test_ci_change_areas.py passed 268/270; its two failures are unrelated GitHub API and stale-head environment failures. The guard sweep retains the known RUNNER_TEMP, registry variable unbound, Ghostty Zig, stored DispatchWorkItem, bash integration, and seed-derived-data failures, plus existing checkout/API failures in other guard blocks.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the Python test execution registry guard so long-lived pull requests no longer get blocked for tests added to main after their stale base SHA. Previously the workflow used github.event.pull_request.base.sha, which records the base tip from when the PR was last synchronized; after a catch-up merge, newer main commits were attributed to the PR and tripped legacy-lane checks for unrelated work, affecting #5677, #5341, and #15711.

The workflow now reads github.event.pull_request.base.ref, fetches that branch at depth 1, resolves FETCH_HEAD, and passes the resolved commit to the validator. The fetch stays shallow on purpose: when merge-base history is unavailable, the validator's two-tree fallback compares directly against the current base tip. Push events still run without --base-sha, and the local guard runner resolves the new expression to its explicit local base revision.

Written for commit 363ef86. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Registry validation now compares changes against the latest commit on the pull request’s base branch, including when workflows are planned outside GitHub Actions.
    • Validation runs without a base comparison when no base branch is available.
  • Tests

    • Added coverage for base-branch resolution, missing base branches, and current versus stale base inputs.

teamleaderleo and others added 2 commits September 30, 2026 05:27
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review: independent review of the workflow, local guard resolver, and regression tests found no correctness issues.\n\nFixed: no review findings required changes. The final delta also asserts the required shallow-fetch rationale comment.\n\nLeft: the local change-area suite and broad guard sweep retain unrelated existing API, runner, and checkout failures documented in the PR body.

@teamleaderleo
teamleaderleo enabled auto-merge (squash) September 30, 2026 12:55
@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d8da0b1d-ab35-4c16-96ca-44d6aae83b0b

📥 Commits

Reviewing files that changed from the base of the PR and between 5fbbc0c and 363ef86.

📒 Files selected for processing (4)
  • .github/workflows/ci-guards.yml
  • scripts/ci/run_ci_guards.py
  • tests/test_ci_run_guards.py
  • tests/test_ci_test_execution_registry.py
 _________________________________________________________
< Finding bugs faster than a kid with a magnifying glass. >
 ---------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@teamleaderleo
teamleaderleo merged commit eaec3f0 into main Sep 30, 2026
60 of 62 checks passed
@teamleaderleo
teamleaderleo deleted the fix/registry-newly-added-after-catch-up branch September 30, 2026 12:59
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 363ef86d68: every check was green at merge (12 verified; 16 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 30, 2026
eaec3f0 fix: resolve registry guard base ref at runtime (manaflow-ai#15990)
5fbbc0c [codex] Improve setup errors for missing Xcode and GhosttyKit cache locks (manaflow-ai#6443)
e53826c fix: ignore SwiftPM warning text in package lane (manaflow-ai#15975)

# Conflicts:
#	.github/workflows/ci-guards.yml
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review landed after the merge. Verdict LAND, which this already did, plus one coverage defect worth a follow-up.

The fix is correct and I proved it rather than reading it. The reviewer built a real fixture: a copy of the PR-head tree as a git repo, a bare origin, a branch adding a test, a /catch-up merge of main, GitHub's refs/pull/N/merge ref pushed to origin, and a depth-1 checkout of that merge ref, which is exactly what actions/checkout produces. Then ran the real step scripts from the old and new ci-guards.yml.

run base input exit result
old step base.sha = stale C0 1 tests/test_theirs_legacy.py: newly added tests may not enter the legacy migration lane
new step base.ref = main 0 Python test execution registry valid: 359 tests

Reproduced in both merge-ref shapes: the fast-forward case where the branch already contains main, and the real synthetic-merge case. Old 1, new 0 in both. Confirmed from the live CI log for check run 109895530723 that the new path actually executed here: CMUX_TEST_REGISTRY_BASE_REF: main, git fetch --no-tags --depth=1 origin main, validator 357 tests, green.

Degradation is loud, not silent, and better than it looks. A failed git fetch truncates .git/FETCH_HEAD to 0 bytes, so git rev-parse FETCH_HEAD fails under set -euo pipefail and the step exits 128. Proved by mutating the fetch to || true: still 128. So that rev-parse is load-bearing as a failure detector, not just a resolution step.

The divergence from #15943 is justified, and I want that on the record since the two guards now resolve their base differently. #15943's fetch-depth: 2 plus git rev-parse HEAD^1 would not have fixed this bug. Same fixture, depth-2 checkout, base HEAD^1: on the fast-forward merge ref, HEAD^1 resolves to the feature branch's previous head rather than main, because when a branch has already merged main the merge ref is the catch-up merge commit, whose first parent is the branch's old tip. That gives exit 1, the identical false failure. HEAD^1 is right for the lockfile and submodule guards, which compare against the merge parent, and wrong for this one, which must compare against the base branch. Do not "unify" these.

Variable rename is clean: CMUX_TEST_REGISTRY_BASE_SHA survives only as a local shell variable in the same run: block plus its two assertions, and the validator takes the base only via --base-sha argv with no env reader at all, so there is no fallback-to-something-plausible hazard.

Mutation testing: 13 mutations, 10 killed, 3 survived.

The one that matters, and it is a follow-up not a complaint

No test observes the guard's exit code, so the guard can be silently disabled. scripts/ci/validate_test_execution_registry.py:385, changing if added is None and base_sha: to if False: takes a branch that adds an unregistered tests/test_unreg_by_this_pr.py from EXIT=1 ... added by this pull request with no execution registry entry to EXIT=0 ... warning: test exists but has no execution registry entry, and both suites stay green. One line disables the entire hard-failure payload.

Root cause: every validate(...) call in tests/test_ci_test_execution_registry.py passes added= explicitly, about 20 call sites, so nothing drives the base_sha to newly_added_tests to hard-error wiring. And the new fixture test at tests/test_ci_test_execution_registry.py:427 replaces the validator invocation with printf "%s\n" "${args[@]}", so no test ever observes an exit code. The missing assertion is exactly the one the reviewer ran above.

I am opening a follow-up PR for that plus the next item.

Other non-blocking items

  1. merge_group is dropped. .github/workflows/ci-guards.yml:262 reads github.event.pull_request.base.ref || '', which is empty in a merge_group event, so the guard degrades to warnings only there. Its neighbour at :184 already handles this with || github.event.merge_group.base_sha. ci.yml does trigger on merge_group and the repo has 256 such runs. Pre-existing, since the old expression was equally empty, but this PR touched that exact line. One-line fix, and git fetch --no-tags --depth=1 origin refs/heads/main was verified to work so the refs/heads/... form needs no special handling. This matters more after this change, not less: now that unregistered tests are measured against current main, a file that slips through is "already on base" forever and never reddens anything again.
  2. A test asserts on a comment string. self.assertIn("Keep this fetch shallow", step) in the test_registry_workflow_resolves_the_current_base_ref block. Rewording that comment with zero behaviour change fails the suite, while the two mutations that matter survive. Dropping that assertion.
  3. The live base tip is non-reproducible, so two runs can compare against different commits. Verified to cause no false positives, since main-side additions read as D under --diff-filter=A. The one real difference: if main independently lands the same unregistered path, the PR's copy downgrades from error to warning. That is arguably correct under the validator's stated policy, so leaving it, recorded so nobody rediscovers it as a bug.
  4. Nothing pins the infrastructure-failure degradation. Flipping the lost-comparison warnings.append to errors.append passes both suites. That behaviour is deliberate and commented, and reversing it would redden every PR on a shallow-clone edge case, so it wants a cheap test alongside item 1.
  5. Flagged, not tested: scripts/ci/run_ci_guards.py maps base.ref to a raw SHA, so running the planner locally does git fetch --no-tags --depth=1 origin <sha> in a developer's real checkout, and --depth=1 into a complete clone can write shallow grafts. That is the known mechanism behind the recurring "cmux clone keeps going shallow" symptom. Pre-existing and not verified here, flagged only because the file is in the diff.

Guard sweep: 9 failures on PR head against 8 on merge base 6d7ad149121, same set both sides, and the one extra (Validate pipe-safe CI capture) is a load flake that is 0 0 0 0 standalone on both. Net delta attributable to this diff is zero. A git merge --no-commit --no-ff into current main is clean and all four suites pass identically in the merged tree.

— Raindrop g2 🫧 / Run: run_worker_20260930_3fc64ba6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant