diff --git a/scripts/check_lane_claim.py b/scripts/check_lane_claim.py index d06f30f294..3336e419b2 100644 --- a/scripts/check_lane_claim.py +++ b/scripts/check_lane_claim.py @@ -2256,9 +2256,12 @@ def _parse_iso_utc(iso: str) -> datetime | None: def _run_check(payload: dict, my_lane: str, stale_threshold=None, now: datetime | None = None, my_paths: list[str] | None = None, - pr_states: dict[int, str] | None = None) -> int: + pr_states: dict[int, str] | None = None, + prs: list[dict] | None = None) -> int: """Issue-claim check: exit 1 if another lane blocks, 0 if clear. + Args: + Args: payload: `gh issue view --json ...` payload (or `from-json`). my_lane: caller lane `machine:workspace`. @@ -2274,6 +2277,13 @@ def _run_check(payload: dict, my_lane: str, stale_threshold=None, the caller's own claim), behaviour is unchanged: every other active claim blocks, regardless of its scope clause (we cannot prove disjointness, so we conservatively over-block). + prs: optional OPEN-PR list (testability seam, #16570). When `None`, + the OPEN-PR collision leg fetches live via + ``_gh_open_prs_with_files``; when supplied (typically + ``prs=[]`` in tests), the leg uses the caller's fixture and + skips the network call. Production CLI passes ``prs=None`` to + opt into the live fetch; tests pass ``prs=[]`` to stay + hermetic. """ events = _sort_events(payload) # One shared tracked-files walk feeds BOTH the #10881 lint and the @@ -2537,6 +2547,39 @@ def _run_check(payload: dict, my_lane: str, stale_threshold=None, my_scope, others, tracked, ) + # #16570 -- the missing leg in issue-mode: cross-lane OPEN-PR collisions + # on the same files. Before this, `check_lane_claim.py --paths ...` + # passed scope to the active-claim filter (so blocker claims were + # correctly intersected) but NEVER asked the OPEN-PR list. The leg + # exists in path-only mode (`_run_check_paths`) and is precisely what + # L898 pre-claim dispatches first; in issue mode it was structurally + # blind, returning CLEAR while a neighbour's PR already touched the + # same files. Compute it here when `my_paths` is given, parallel to + # the active-claim leg above. Failures to fetch are silenced (the + # active-claim leg still gives a useful verdict); a missing fetch is + # reported in `open_pr_collision_error` for forensics. + open_pr_collisions: list[dict] = [] + open_pr_self_overlap: list[dict] = [] + open_pr_collision_error: str | None = None + if my_paths: + # #16570 -- OPEN-PR leg. The leg uses a `prs` testability seam + # (same pattern as `_run_check_paths`): when `prs` is supplied, + # we trust the caller and skip the live `gh pr list`. When + # `prs` is `None`, we fetch live OPEN PRs via + # `_gh_open_prs_with_files`. Tests that don't care about the + # OPEN-PR leg pass `prs=[]`; the production CLI dispatcher + # passes `prs=None` (default) to opt in to the live fetch. + if prs is None: + try: + prs = _gh_open_prs_with_files() + except RuntimeError as exc: + open_pr_collision_error = str(exc) + prs = [] + if prs: + _collisions, _self = _classify_pr_collisions(my_paths, my_lane, prs) + open_pr_collisions = [_serialise_path_collision(c) for c in _collisions] + open_pr_self_overlap = [_serialise_path_collision(c) for c in _self] + summary = { "issue": payload.get("number"), "title": payload.get("title"), @@ -2753,6 +2796,16 @@ def _run_check(payload: dict, my_lane: str, stale_threshold=None, # always visible to a JSON sweep. Non-blocking by design -- it only # reports; it does not change the verdict. "dead_scope_globs": dead_scope_globs, + # #16570 -- OPEN-PR collisions on the caller's `--paths`. Always + # present in the summary; empty lists when `my_paths` was not + # supplied (the issue-mode caller has no paths to check) or when + # no OPEN PR of any lane intersects. Non-empty `open_pr_collisions` + # is the signal that drives the rc=2 verdict below -- the worker + # picked an issue whose files are already touched by another + # lane's PR, and the active-claim leg alone would have said CLEAR. + "open_pr_collisions": open_pr_collisions, + "open_pr_self_overlap": open_pr_self_overlap, + "open_pr_collision_error": open_pr_collision_error, } print(json.dumps(summary, ensure_ascii=False, indent=2)) @@ -3049,6 +3102,55 @@ def _run_check(payload: dict, my_lane: str, stale_threshold=None, f"avant de demarrer (#13336).", file=sys.stderr, ) + + # #16570 -- the missing leg. OPEN PRs of OTHER lanes touching the caller's + # `--paths` block the call at rc=2 (same contract as the active-claim + # leg, same exit code as the path-mode guard). The active-claim leg + # above would have returned CLEAR; this leg catches the case where the + # neighbour is WORKING the issue in an OPEN PR without holding a + # `[CLAIMED]` marker (the picker had measured this trap; this is the + # organ-level fix). Self-overlap (caller's OWN lane PR) is surfaced + # as `open_pr_self_overlap` but does NOT block. + if open_pr_collisions: + tagged = [c for c in open_pr_collisions if c.get("lane") is not None] + untagged = [c for c in open_pr_collisions if c.get("lane") is None] + msg = [ + f"\nBLOCKED: OPEN PR(s) of other lanes touch the requested " + f"--paths on #{payload.get('number')}. The active-claim leg " + f"above would have said CLEAR; this leg closes the gap (#16570).", + "", + ] + for c in tagged: + inter = ", ".join(c.get("files_intersecting", [])) + msg.append( + f" - #{c['number']} lane={c['lane']} head={c['headRefName']} " + f"files=[{inter}] -- {c['title']}" + ) + for c in untagged: + inter = ", ".join(c.get("files_intersecting", [])) + msg.append( + f" - #{c['number']} lane=UNREADABLE head={c['headRefName']} " + f"files=[{inter}] -- {c['title']}\n" + f" (no `Grain:` lane tag in body; cannot attribute. " + f"Treat as a potential collision: coordinate before pushing.)" + ) + msg.append( + "\nDo not start -- coordinate with the owner(s), or pick " + "another grain that does not intersect the open PR(s)." + ) + print("\n".join(msg), file=sys.stderr) + return 2 + if open_pr_self_overlap: + own_numbers = ", ".join( + f"#{c['number']}" for c in open_pr_self_overlap + ) + print( + f"\nCLEAR for paths {my_paths!r} on #{payload.get('number')} -- " + f"your own OPEN PR(s) already touch these paths ({own_numbers}). " + f"Resuming your own work is fine; the OPEN-PR leg does not " + f"block your own lane (#16570).", + file=sys.stderr, + ) return 0 @@ -3258,6 +3360,76 @@ def __init__(self, pr: dict, lane: str | None, files: list[str]) -> None: self.files = files # the PR files that intersect the patterns +def _classify_pr_collisions( + paths: list[str], + my_lane: str, + prs: list[dict], +) -> tuple[list[PathCollision], list[PathCollision]]: + """Pure helper: classify OPEN PRs by path-vs-lane intersection (#16570). + + Returns ``(collisions, self_overlap)``: + - ``collisions`` : OPEN PRs whose lane differs from ``my_lane`` + (including untagged PRs -- treated as a + potential collision; author is jsboige on + every PR, so the tag is the only signal). + - ``self_overlap`` : OPEN PRs whose lane equals ``my_lane`` -- the + caller is resuming their own work, not a + cross-lane collision. + + Extracted from ``_run_check_paths`` (#16570) so the issue-mode check + can share the same classification when ``--paths`` is supplied + alongside an issue number. Before the extraction, the issue-mode + code path skipped this leg entirely (the bug: a worker could claim + an issue on the strength of a CLEAR from the issue layer, while + an OPEN PR of another lane already touched the same files). + """ + collisions: list[PathCollision] = [] + self_overlap: list[PathCollision] = [] + for pr in prs: + pr_files = pr.get("files") or [] + if not pr_files: + continue + intersecting = [ + f.get("path", "") for f in pr_files + if f.get("path") and _path_matches(f["path"], paths) + ] + if not intersecting: + continue + lane = extract_lane(pr.get("body") or "") + coll = PathCollision( + pr=pr, lane=lane, files=[p for p in intersecting if p], + ) + # Three-way classification: + # - lane == my_lane -> self_overlap (resuming own work) + # - lane is None (no Grain tag) -> collisions (cannot attribute; + # author is jsboige on every PR, + # so the tag is the only signal; + # absence = uncertainty, treated + # as a potential collision -- not + # silently ignored) + # - lane != my_lane (tagged) -> collisions + if lane == my_lane: + self_overlap.append(coll) + else: + collisions.append(coll) + return collisions, self_overlap + + +def _serialise_path_collision(c: PathCollision) -> dict: + """Serialise a PathCollision for JSON output (#16570). + + Extracted from ``_run_check_paths`` so the issue-mode summary can use + the same shape for ``open_pr_collisions`` and ``open_pr_self_overlap``. + """ + return { + "number": c.number, + "title": c.title, + "headRefName": c.headRefName, + "lane": c.lane, + "files_intersecting": c.files, + } + + def _run_check_paths( paths: list[str], my_lane: str, @@ -3297,35 +3469,7 @@ def _run_check_paths( print(f"error: {exc}", file=sys.stderr) return 1 - collisions: list[PathCollision] = [] - self_overlap: list[PathCollision] = [] - for pr in prs: - pr_files = pr.get("files") or [] - if not pr_files: - continue - intersecting = [ - f.get("path", "") for f in pr_files - if f.get("path") and _path_matches(f["path"], paths) - ] - if not intersecting: - continue - lane = extract_lane(pr.get("body") or "") - coll = PathCollision( - pr=pr, lane=lane, files=[p for p in intersecting if p], - ) - # Three-way classification: - # - lane == my_lane -> self_overlap (resuming own work) - # - lane is None (no Grain tag) -> collisions (cannot attribute; - # author is jsboige on every PR, - # so the tag is the only signal; - # absence = uncertainty, treated - # as a potential collision -- not - # silently ignored) - # - lane != my_lane (tagged) -> collisions - if lane == my_lane: - self_overlap.append(coll) - else: - collisions.append(coll) + collisions, self_overlap = _classify_pr_collisions(paths, my_lane, prs) def _serialise(c: PathCollision) -> dict: return { diff --git a/scripts/tests/test_check_lane_claim.py b/scripts/tests/test_check_lane_claim.py index 6a11c8468d..b7121f5e2d 100644 --- a/scripts/tests/test_check_lane_claim.py +++ b/scripts/tests/test_check_lane_claim.py @@ -2047,6 +2047,7 @@ def test_check_override_paths_keeps_other_lane_free_on_non_matching_path(capsys) p, "myia-po-2025:CoursIA-2", my_paths=["scripts/check_lane_claim.py"], + prs=[], # #16570 -- opt out of live OPEN-PR fetch (test hermeticity) ) assert rc == 0 captured = capsys.readouterr() @@ -2871,6 +2872,7 @@ def test_run_check_clean_brace_scope_does_not_show_unparseable(capsys): rc = clc._run_check( p, "myia-po-2025:CoursIA", my_paths=["scripts/check_lane_claim.py"], + prs=[], # #16570 -- opt out of live OPEN-PR fetch (test hermeticity) ) assert rc == 0 out = capsys.readouterr().out @@ -5092,7 +5094,8 @@ def test_delivered_closed_lifts_active_claim(capsys): ) rc = clc._run_check(p, "myia-po-2023:CoursIA-2", pr_states=_pr_states(12253, "CLOSED"), - my_paths=["scripts/check_lane_claim.py"]) + my_paths=["scripts/check_lane_claim.py"], + prs=[]) # #16570 -- opt out of live OPEN-PR fetch assert rc == 0 out = _json_out(capsys.readouterr()) assert out["my_active_claim"] is False @@ -5118,7 +5121,8 @@ def test_delivered_lookup_failure_legacy_close(capsys): ) rc = clc._run_check(p, "myia-po-2023:CoursIA-2", pr_states={}, # 99999 not present -> legacy close - my_paths=["scripts/check_lane_claim.py"]) + my_paths=["scripts/check_lane_claim.py"], + prs=[]) # #16570 -- opt out of live OPEN-PR fetch assert rc == 0 out = _json_out(capsys.readouterr()) assert out["my_active_claim"] is False @@ -5144,7 +5148,8 @@ def test_delivered_without_pr_ref_still_lifts_active_claim(capsys): ) rc = clc._run_check(p, "myia-po-2023:CoursIA-2", pr_states=None, - my_paths=["scripts/check_lane_claim.py"]) + my_paths=["scripts/check_lane_claim.py"], + prs=[]) # #16570 -- opt out of live OPEN-PR fetch assert rc == 0 out = _json_out(capsys.readouterr()) assert out["my_active_claim"] is False @@ -5724,6 +5729,7 @@ def test_run_check_disjoint_joker_caller_clear(capsys): rc = clc._run_check( p, "myia-po-2023:CoursIA-2", my_paths=[f"{_RAG}/**"], + prs=[], # #16570 -- opt out of live OPEN-PR fetch (test hermeticity) ) assert rc == 0, ( f"disjoint joker scopes must not block each other (#10419): got rc={rc}" @@ -5920,11 +5926,15 @@ def test_amend_blocks_other_lane_on_intersecting_amended_scope(capsys): ) caller_path = "scripts/tests/test_check_lane_claim.py" rc_pre = clc._run_check( - payload(original), "myia-po-2023:CoursIA-2", my_paths=[caller_path] + payload(original), "myia-po-2023:CoursIA-2", + my_paths=[caller_path], + prs=[], # #16570 -- opt out of live OPEN-PR fetch (test hermeticity) ) assert rc_pre == 0, f"pre-amend scopes are disjoint, expected CLEAR, got rc={rc_pre}" rc_post = clc._run_check( - payload(original, amended), "myia-po-2023:CoursIA-2", my_paths=[caller_path] + payload(original, amended), "myia-po-2023:CoursIA-2", + my_paths=[caller_path], + prs=[], # #16570 -- opt out of live OPEN-PR fetch (test hermeticity) ) err = capsys.readouterr().err assert rc_post == 1, ( diff --git a/scripts/tests/test_lane_claim_16570_open_pr.py b/scripts/tests/test_lane_claim_16570_open_pr.py new file mode 100644 index 0000000000..79a550193e --- /dev/null +++ b/scripts/tests/test_lane_claim_16570_open_pr.py @@ -0,0 +1,250 @@ +"""Regression test for issue #16570. + +Symptom: `check_lane_claim.py --issue N --paths ` returned rc=0 (CLEAR) +even when another lane's OPEN PR already touched those same files -- the +issue-mode dispatch skipped the path-mode cross-lane collision check. The +picker had measured this trap; this is the organ-level fix. + +This file exercises the extracted helper ``_classify_pr_collisions`` and the +end-to-end classifier used by ``_run_check`` to surface those collisions in the +JSON summary under ``open_pr_collisions`` / ``open_pr_self_overlap``. + +No network: every input is a synthetic PR dict. The classifier is a pure +function over those dicts. +""" +from __future__ import annotations + +import sys +from dataclasses import dataclass +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +import check_lane_claim as clc # noqa: E402 + +MY_LANE = "myia-po-2024:CoursIA-2" +OTHER_LANE = "myia-po-2025:CoursIA-2" + + +# ----- fixtures -------------------------------------------------------------- + + +def _pr( + number: int, + body: str, + files: list[str], + head: str | None = None, +) -> dict: + """Build a synthetic PR dict matching the gh `pr list --json` shape.""" + return { + "number": number, + "title": f"PR #{number}", + "headRefName": head if head is not None else f"fix/{number}-test", + "body": body, + "files": [{"path": f, "path_is_clean": True} for f in files], + } + + +# A PR of ANOTHER lane tagging the same files -- collision. +PR_OTHER_LANE_INTERSECTS = _pr( + number=200, + body=( + "Grain: MED/tooling -- lane myia-po-2025:CoursIA-2\n\n" + "## Summary\nTouches scripts/check_lane_claim.py" + ), + files=["scripts/check_lane_claim.py", "scripts/lane_claim_epic_wide.py"], +) + +# A PR of MY lane tagging the same files -- self-overlap (resume own work). +PR_SELF_INTERSECTS = _pr( + number=210, + body=( + "Grain: DEEP/notebook-python -- lane myia-po-2024:CoursIA-2\n\n" + "## Summary\nfollow-up on check_lane_claim.py" + ), + files=["scripts/check_lane_claim.py"], +) + +# A PR with NO `Grain:` tag touching the same files -- fallback untagged, +# treated as a potential collision. +PR_UNTAGGED_INTERSECTS = _pr( + number=220, + body="## Summary\nUntagged PR that touches the script", + files=["scripts/tests/test_lane_claim.py"], +) + +# A PR of ANOTHER lane on UNRELATED files -- no intersection. +PR_OTHER_LANE_DISJOINT = _pr( + number=230, + body="Grain: LIGHT/guard -- lane myia-po-2025:CoursIA-2", + files=["README.md", "docs/harness/foo.md"], +) + +# A PR of ANOTHER lane with no `files` payload (cache miss / empty) -- skip. +PR_OTHER_NO_FILES = _pr( + number=240, + body="Grain: LIGHT/guard -- lane myia-po-2025:CoursIA-2", + files=[], +) + + +PATHS = ["scripts/check_lane_claim.py"] + + +# ----- _classify_pr_collisions direct --------------------------------------- + + +def test_positive_classifier_other_lane_blocks_collision() -> None: + """`other-lane PR intersecting files` -> collisions, NOT self_overlap.""" + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[PR_OTHER_LANE_INTERSECTS], + ) + assert len(collisions) == 1 + assert collisions[0].number == 200 + assert collisions[0].lane == OTHER_LANE + assert "scripts/check_lane_claim.py" in collisions[0].files + assert self_overlap == [] + + +def test_positive_classifier_self_lane_is_self_overlap() -> None: + """`same-lane PR intersecting files` -> self_overlap, NOT collisions.""" + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[PR_SELF_INTERSECTS], + ) + assert collisions == [] + assert len(self_overlap) == 1 + assert self_overlap[0].number == 210 + assert self_overlap[0].lane == MY_LANE + + +def test_positive_classifier_untagged_intersects_blocks_collision() -> None: + """`untagged PR intersecting files` -> collisions (uncertainty).""" + # use a different path under PATHS so this is independent of the prior + # test cases (the helper matches the FIRST file in PR_UNTAGGED_INTERSECTS + # against PATHS -- here we widen PATHS so both intersect). + collisions, self_overlap = clc._classify_pr_collisions( + paths=["scripts/tests/test_lane_claim.py"], + my_lane=MY_LANE, + prs=[PR_UNTAGGED_INTERSECTS], + ) + assert len(collisions) == 1 + assert collisions[0].number == 220 + assert collisions[0].lane is None + assert self_overlap == [] + + +def test_negative_classifier_disjoint_pr_is_ignored() -> None: + """`other-lane PR on disjoint files` -> neither list.""" + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[PR_OTHER_LANE_DISJOINT], + ) + assert collisions == [] + assert self_overlap == [] + + +def test_classifier_skips_pr_with_empty_files() -> None: + """`other-lane PR with no files payload` -> silently skipped (degraded cache). + + The classifier can't reason about a PR with no file payload. Skipping it + here does NOT mean the gate is bypassed; the next pipeline stage (the + `summary[]` builder) leaves `open_pr_collision_error` as the truth-source + when the cache is degraded. This test just locks the classifier's local + behaviour. + """ + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[PR_OTHER_NO_FILES], + ) + assert collisions == [] + assert self_overlap == [] + + +def test_classifier_three_way_mixed_input() -> None: + """`4 PRs, mixed lanes and disjoint` -> exactly one collision + one self-overlap.""" + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[ + PR_OTHER_LANE_INTERSECTS, # -> collision + PR_SELF_INTERSECTS, # -> self_overlap + PR_OTHER_LANE_DISJOINT, # -> ignored + PR_OTHER_NO_FILES, # -> skipped + ], + ) + assert sorted(c.number for c in collisions) == [200] + assert sorted(c.number for c in self_overlap) == [210] + + +# ----- _serialise_path_collision direct -------------------------------------- + + +def test_serialise_shape_matches_path_mode() -> None: + """JSON shape mirrors the path-mode leg exactly (#16570 compat).""" + collisions, _ = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=[PR_OTHER_LANE_INTERSECTS], + ) + serialised = clc._serialise_path_collision(collisions[0]) + assert set(serialised.keys()) == { + "number", "title", "headRefName", "lane", "files_intersecting", + } + assert serialised["number"] == 200 + assert serialised["lane"] == OTHER_LANE + assert serialised["headRefName"] == "fix/200-test" + assert "scripts/check_lane_claim.py" in serialised["files_intersecting"] + + +# ----- end-to-end: --run_check surface contract ----------------------------- +# +# The full `_run_check` requires more fixtures (issue payload, time payload, +# comments) and is exercised by the existing `test_check_lane_claim.py` +# integration suite. Here we lock the surface CONTRACT: that the summary dict +# carries the two new fields, with the same shape the path-mode leg uses. +# The synthetic PR list is fed through the helper directly. + + +@dataclass +class FakeSummaryCollector: + """Mirror the assignment lines added to ``_run_check`` (#16570).""" + open_pr_collisions: list[dict] + open_pr_self_overlap: list[dict] + open_pr_collision_error: str | None = None + + +def test_end_to_end_summary_carries_open_pr_collisions_field() -> None: + """The summary contract is fixed: when OPEN PRs intersect, the field is + populated with the same shape as the path-mode leg (#16570).""" + prs = [ + PR_OTHER_LANE_INTERSECTS, + PR_SELF_INTERSECTS, + PR_OTHER_LANE_DISJOINT, + ] + collisions, self_overlap = clc._classify_pr_collisions( + paths=PATHS, + my_lane=MY_LANE, + prs=prs, + ) + summary = FakeSummaryCollector( + open_pr_collisions=[clc._serialise_path_collision(c) for c in collisions], + open_pr_self_overlap=[ + clc._serialise_path_collision(c) for c in self_overlap + ], + ) + # One other-lane collision, one self-overlap. + assert len(summary.open_pr_collisions) == 1 + assert summary.open_pr_collisions[0]["number"] == 200 + assert summary.open_pr_collisions[0]["lane"] == OTHER_LANE + assert len(summary.open_pr_self_overlap) == 1 + assert summary.open_pr_self_overlap[0]["number"] == 210 + # The gate's `rc=2` decision lives in `_run_check`; at the helper level + # we only assert the population, not the exit code -- that's the contract + # the issue leg must honour when it consumes the summary. + assert summary.open_pr_collision_error is None