Skip to content

fix(ci): the overlap sweep grades a stale base too wide to fetch whole - #3803

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/overlap-sweep-splits-a-capped-base-side
Sep 16, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/overlap-sweep-splits-a-capped-base-side

Conversation

@cagataycali

Copy link
Copy Markdown
Member

check_merge_base_overlap.py --all-open reads each pull request's base side -- what landed on main since the branch forked -- from one compare call, whose files list GitHub caps at 300. A capped list is indistinguishable from a complete one, so the sweep named those pull requests unevaluated rather than intersecting a set it knew was short.

The width is not incidental. M..base grows for as long as a branch sits in review, so the ranges that reach the cap are precisely the longest-standing stale bases -- and a stale base under a path the branch edits is the composition that put main red at 0e636f8, arriving from the base instead of from a sibling. The sweep was declining to grade exactly the pull requests most exposed to the relation it exists to report.

Measured on the open set of 29

section of the report before after
stale base under a path the branch edits 10 15
base changed a module their tests name 1 2
unevaluated 10 0

All 10 declined were stale bases. The five that became findings:

pull request behind main path main changed that it also edits state
#3496 255 strands_robots/policies/lerobot_async/policy.py green, auto-merge armed
#3615 152 strands_robots/policies/lerobot_local/policy.py green, auto-merge armed
#3577 179 strands_robots/policies/groot/policy.py green, auto-merge armed
#3343 285 strands_robots/simulation/isaac/simulation.py +10 more green, auto-merge armed
#3400 345 strands_robots/drivers/booster.py, its test green

Each edits a file main has since changed, has never been re-evaluated against it, and four can merge unattended.

The change

files is capped where the payload's commits is not, so a range too wide to read whole still carries a boundary to split it at. A capped compare is re-read as base...boundary + boundary...head, each through the same path and so split again if still capped. Each half is strictly shorter than the range it came from, so it terminates.

flowchart LR
    A["compare base...head<br/>files = 300 = cap"] -->|before| B(["unevaluated"])
    A -->|after| C["split at a commit<br/>the payload already carries"]
    C --> D["base...boundary"]
    C --> E["boundary...head"]
    D --> F(["union = the set<br/>the cap was hiding"])
    E --> F
    style B fill:#ffd6d6
    style F fill:#d6ffd6
Loading

The union is never short of the capped list, which is the direction that matters. It can be longer: a path one commit created and another removed nets out of M..base and survives in a half -- and main did touch that path, so the extra row is the conservative side of a check whose failure mode is a missed overlap.

Checked against git diff on five capped pull requests

pull request git diff paths assembled from halves missing transient extras requests
#3205 528 531 0 3 10
#3427 607 609 0 2 10
#3496 769 777 0 8 17
#3615 519 526 0 7 10
#3673 380 382 0 2 5

Every extra was verified absent at both ends of the range and touched by two commits inside it -- created and removed, exactly the predicted class.

The floor keeps the old refusal and names which floor it hit: one commit whose own diff reaches the cap has nothing left to split on. The pairwise mode is untouched -- it reads the head side, and an unreadable base side has never removed a pull request from it.

Tests

Three pins, all failing on the pre-fix script (verified by swapping only that file back): a capped range is read as its halves; a half still capped is split again; a single commit at the cap is named with no commit boundary to split. The existing truncation pin passes unchanged -- its range offers no boundary -- and its docstring, which claimed this was "the one cap the sweep cannot route around", now says which shape actually cannot be.

mutant
only the lower half is read CAUGHT
the halves are intersected, not unioned CAUGHT
the payload's commits are dropped CAUGHT
exactly-at-the-cap counts as complete CAUGHT
a boundary-less range is split anyway CAUGHT

tests/test_merge_base_overlap.py 113 passed. ruff check clean, mypy clean over 2132 files, ruff format --check clean on the changed files. The 92-file scope naming this script or AGENTS.md: 3162 passed, 2 failed -- both reproduced byte-identically on a clean main checkout (dataset_recorder.py duplicate import inspect, unitree_g1 config discovery) and neither in a file this touches. --base-ref main --head HEAD: no overlap.

`--all-open` read each pull request's base side -- the paths that landed on
main since the branch forked -- from one compare call, whose `files` list
GitHub caps at 300. A capped list is indistinguishable from a complete one in
the payload, so the sweep named those pull requests unevaluated rather than
intersecting a set it knew was short. The width is not incidental: M..base
grows for as long as a branch sits in review, so the ranges that reach the cap
are exactly the longest-standing stale bases, and a stale base under a path the
branch edits is the composition that put main red at 0e636f8 arriving from the
base instead of from a sibling. On the open set of 29 it declined 10 pull
requests, every one of them a stale base.

`files` is capped where the payload's `commits` is not, so a range too wide to
read whole still carries a boundary to split it at. A capped compare is now
re-read as base...boundary and boundary...head, each through the same path and
so split again if it is capped too; each half is strictly shorter than the
range it came from, so this terminates. Checked against git diff over five
capped pull requests on the live queue at 380-780 paths each: no path missing
in any of them, at 5-17 requests. The floor keeps the old refusal and names
which floor it hit: one commit whose own diff reaches the cap has nothing left
to split on.
@cagataycali
cagataycali enabled auto-merge (squash) September 16, 2026 16:22
@cagataycali

Copy link
Copy Markdown
Member Author

call-test-lint is red on one test inherited from main, not from this branch: test_stop_policy_resolves_the_only_rollout.py::test_an_empty_name_stops_the_one_policy_running, the exited key #3798 fixes. 22 604 passed beside it, and every other check is green. Auto-merge is armed, so this lands once main is green.

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The --all-open sweep previously declined to grade a PR whose base side (M..base) hit the compare endpoint's 300-file cap, which systematically excluded exactly the longest-standing stale bases from the stale-base mode. This PR routes around the cap by splitting a capped range at a commit boundary the same payload already carries (files is capped, commits is not), recursively re-reading each half through compare_paths until it fits; the union can only over-report (transient created-then-removed paths), which is the safe direction for a check whose failure mode is a missed overlap. The old refusal is kept for the one shape that genuinely cannot be split -- a single commit whose own diff reaches the cap -- and the error message now names that boundary condition. The ApiError raised at that floor flows through the existing per-PR lookup_failures handling, so an unreadable base side still leaves the PR in the pairwise mode (pinned by the new single-commit test). No public API, wire format, or persisted schema changes; _compare_payload's widened return tuple is private to the script.

What's good

  • Three new regression tests pin the split, the recursive re-split, and the boundary-less refusal; I independently verified all 113 tests pass at the head SHA and spot-checked two of the claimed mutants (lower-half-only and exactly-at-cap-as-complete), both caught.
  • Termination argument is sound: the boundary index is always strictly inside the range (len//2 - 1 <= len-2), so each half is strictly shorter, and the floor raises rather than intersecting a known-short set.
  • Changelog fragment present, AGENTS.md updated in the same PR, docstrings match the new semantics (including the stale "one cap the sweep cannot route around" claim in the old test), ASCII-clean, no host paths.

auto-merge was automatically disabled September 16, 2026 19:04

Pull request was closed

@cagataycali cagataycali reopened this Sep 16, 2026
@cagataycali
cagataycali enabled auto-merge (squash) September 16, 2026 19:07
@cagataycali
cagataycali merged commit 7d4b121 into strands-labs:main Sep 16, 2026
17 of 18 checks passed
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.

3 participants