diff --git a/.github/scripts/collect.py b/.github/scripts/collect.py index 921ec01..e2a8a3f 100644 --- a/.github/scripts/collect.py +++ b/.github/scripts/collect.py @@ -36,11 +36,12 @@ import pathlib import re import subprocess +import tempfile import sys sys.path.insert(0, str(pathlib.Path(__file__).resolve().parents[2])) -from progress import files, gate # noqa: E402 +from progress import files, gate, gh as gh_mod, plan as plan_mod, window # noqa: E402 # Where the reported window has to live. `to_sha` is checked for reachability from this branch, which # tracks the newest TauCeti commit with published documentation. @@ -183,6 +184,63 @@ def rev_parse(repo, ref): return proc.stdout.strip() or None if proc.returncode == 0 else None +def bootstrap_cursor(area, repo=CODE_REPO, ref=CODE_REF): + """The one legitimate `from_sha` for a roadmap's FIRST report, or None if it cannot be computed. + + A first report has no cursor on `main` to continue from, so its starting point has to be pinned + some other way, or whoever files it also decides where that roadmap's history begins -- and + windows only move forward, so everything earlier becomes unreportable for good. + + Three earlier attempts tried to answer this through the REST API, by lowest pull request number, + by merge timestamp, and by walking the commits endpoint, and all three were wrong. The question is + about position on the first-parent chain; the API offers neither first-parent traversal nor any + ordering guarantee, and this history is not linear, so ancestry checks cannot recover it. + + So do not ask the API. Clone the code repository and ask git, which is what the planner does, with + the SAME functions over the same data -- so the two agree by construction rather than by luck, + which is what the previous attempts got wrong. + + `--filter=blob:none --no-checkout` fetches commits without file contents: about a second and three + megabytes, since only commit subjects are read. This is not a checkout of pull-request content -- + it is the upstream code repository, nothing from the pull request reaches it, and nothing in it is + executed. + """ + proc = subprocess.run( + ["gh", "pr", "list", "--repo", repo, "--state", "merged", + "--label", f"{ROADMAP_LABEL_PREFIX}{area}", "--limit", "100000", "--json", "number"], + capture_output=True, text=True, + ) + if proc.returncode != 0: + return None + try: + labelled = [int(r["number"]) for r in json.loads(proc.stdout)] + except (json.JSONDecodeError, KeyError, TypeError, ValueError): + return None + if not labelled: + return None + + with tempfile.TemporaryDirectory() as tmp: + clone = pathlib.Path(tmp) / "code" + cloned = subprocess.run( + ["git", "clone", "--filter=blob:none", "--no-checkout", "--single-branch", + "--branch", ref, "-q", f"https://github.com/{repo}", str(clone)], + capture_output=True, text=True, + ) + if cloned.returncode != 0: + return None + try: + # The same refusal the planner makes, from the same helper: a labelled pull request that + # is in this history but unrecognised in it would otherwise move the cursor past itself. + if plan_mod.unaccounted_prs(clone, labelled, ref=ref): + return None + found = window.earliest_merged(clone, labelled, ref=ref) + if found is None: + return None + return window.first_parent_before(clone, found[1]) + except (window.GitError, gh_mod.GhError): + return None + + def compare_status(repo, base, head): """`status` from a two-dot-three comparison, or None when either end is not a commit. @@ -318,6 +376,7 @@ def blob_for(basename): # wholesale replacement of the archived log then looked like a valid append. old_status = old_progress = last_report_at = None area_exists = False + expected_bootstrap = None old_paths = {} current_cursor = None parents = {gate.PATH_RE.match(p).group(1) for p in by_path} @@ -344,6 +403,9 @@ def blob_for(basename): except files.FormatError: # An unparseable log on main is a real problem, but saying so is the gate's job. current_cursor = None + if current_cursor is None and area_exists: + # Only for a first report, so at most once per roadmap ever. + expected_bootstrap = bootstrap_cursor(area) # Only check-runs, and only from the head we pinned. Commit statuses are deliberately NOT # collected: any repository writer can POST one under any context, so they are not evidence, and @@ -365,6 +427,7 @@ def blob_for(basename): "base_repo": args.repo, "code_window": resolve_window(new_progress), "area_exists": area_exists, + "expected_bootstrap_from_sha": expected_bootstrap, "last_report_at": last_report_at, # The collector's own clock, never anything from the pull request. "collected_at": datetime.datetime.now(datetime.timezone.utc).isoformat(), diff --git a/progress/gate.py b/progress/gate.py index 32f5754..e85bd6a 100644 --- a/progress/gate.py +++ b/progress/gate.py @@ -66,9 +66,9 @@ EX_REFUSED = 3 # The minimum gap between two reports for the SAME roadmap, enforced here rather than only in the -# planner. Set below the planner's 24h cadence so it never refuses a legitimate report, while still -# capping how fast the announcement channel can be driven. -MIN_REPORT_INTERVAL_HOURS = 20.0 +# planner. Kept below `plan.IDLE_HOURS` so it never refuses a report the planner considered due, +# while still capping how fast one roadmap can drive the announcement channel. +MIN_REPORT_INTERVAL_HOURS = 6.0 def _parse_iso(text): @@ -408,7 +408,7 @@ def decide(pr, changed_files, tree_entries, old_status, new_status_bytes, old_pr new_progress_bytes, check_runs, base_repo, current_main_cursor=None, compare_status=None, behind_by=None, main_sha="", old_paths=None, code_window=None, last_report_at=None, now=None, - area_exists=None): + area_exists=None, expected_bootstrap_from_sha=None): """Run the whole gate. Returns `{"area", "head_sha", "section"}` or raises `Refused`. `current_main_cursor` is the area's cursor read from **freshly fetched `main`**, not from the @@ -432,30 +432,23 @@ def decide(pr, changed_files, tree_entries, old_status, new_status_bytes, old_pr # area's earliest labelled pull request) and it is required exactly. Refusing first reports # outright would have been safe and useless: thirteen of the fourteen roadmaps have never been # reported, so almost nothing would be left for automation to do. - # A roadmap's FIRST report is not auto-merged, and this is a considered retreat rather than an - # oversight. + # A roadmap's FIRST report has no cursor on `main` to continue from, so its starting point is + # pinned against the code repository instead: the collector clones it and asks git, using the same + # functions the planner uses over the same data, so the two agree by construction. # - # Every later report is pinned: `from_sha` must equal the cursor already on `main`. A first report - # has no cursor to continue from, so whoever files it also decides where that roadmap's history - # begins, and because windows only move forward, everything before that point is unreportable for - # good. Checking that choice means asking whether any labelled pull request merged before it -- - # an ancestry question about the first-parent chain. - # - # Three implementations tried to answer it here and all three were wrong: by lowest pull request - # number (numbers are assigned at open time, not merge time), by merge timestamp (no relation to - # position), and by walking the commits endpoint (which offers neither first-parent traversal nor - # any ordering guarantee). The REST API cannot express first-parent membership, and this history - # is not linear, so it cannot be recovered by ancestry checks either. - # - # An uncheckable property should not be pretended to be checked. A human bootstraps each roadmap - # once -- fourteen reviews, ever -- and every report after that is unattended. The generator still - # writes the first report; only merging it needs a person. + # Three earlier versions tried to answer this through the REST API -- by lowest pull request + # number, by merge timestamp, and by walking the commits endpoint -- and all three were wrong, + # because the question is about position on the first-parent chain and the API expresses neither + # first-parent traversal nor any ordering guarantee. A fourth refused first reports outright, + # which was sound but made a person merge fourteen of them by hand. if not current_main_cursor: - _refuse( - f"{parent}/{area} has no reported history yet. A first report decides where that " - f"roadmap's log begins, which cannot be verified mechanically, so it is left for human " - f"review; every later report for it merges unattended" - ) + expect = expected_bootstrap_from_sha or "" + if not expect: + _refuse( + f"{parent}/{area} has no reported history yet, and where that history begins could " + f"not be determined, so a first report cannot be checked" + ) + current_main_cursor = expect check_modes(tree_entries, [f"{parent}/{area}/{name}" for name in ALLOWED_BASENAMES]) section = check_content( area, old_status, new_status_bytes, old_progress, new_progress_bytes, @@ -524,6 +517,7 @@ def main(argv=None): # not silently skip the window check. code_window=data.get("code_window"), area_exists=data.get("area_exists"), + expected_bootstrap_from_sha=data.get("expected_bootstrap_from_sha"), last_report_at=data.get("last_report_at"), now=data.get("collected_at"), current_main_cursor=data.get("current_main_cursor"), diff --git a/progress/plan.py b/progress/plan.py index 9beb0d5..f193dc7 100644 --- a/progress/plan.py +++ b/progress/plan.py @@ -7,7 +7,8 @@ Two thresholds, both overridable so they can be raised as the project grows: * `IDLE_HOURS` -- an update is due only if *no* area has been updated for this long. This paces the - whole project to about one report a day rather than one per area. + whole project as a whole rather than per area: at 8 hours, about three reports a day across + fourteen roadmaps. * `MIN_PRS` -- the winning area must have at least this many PRs in its window. Without a floor, a quiet day produces a padded report about three commits. """ @@ -20,12 +21,12 @@ from . import files, gh, window from .window import CODE_REF -IDLE_HOURS = 24.0 +IDLE_HOURS = 8.0 MIN_PRS = 10 # How long an open progress pull request keeps marking its area in flight. Past this it is assumed # stuck rather than pending: one full cadence period is long enough for any pull request that was # going to merge to have merged, and the merge check re-runs on every push and on CI completing. -STALE_PR_HOURS = 24.0 +STALE_PR_HOURS = 8.0 # The commit-subject prefix that marks a merged progress update. `apply` uses it as the PR title # prefix, and a squash merge carries the PR title into the commit subject, so this one string links @@ -142,6 +143,35 @@ def area_window(repo_dir, area_prs, from_sha, to_sha): return [n for n in numbers if n in wanted] +def unaccounted_prs(repo_dir, area_prs, ref=CODE_REF): + """Labelled pull requests that are in `ref`'s history but were not recognised in it. + + `earliest_merged` finds pull requests by matching their number in commit subjects, so one whose + merge subject does not carry its number is invisible -- and the starting point would then be + computed from a later merge, silently skipping it. Two implementations agreeing does not catch + this: they share the omission, which is exactly how three earlier versions of this check passed + review while being wrong. + + A missing number is usually benign: the pull request merged after the documented tip, so it is + not in this history yet. That is distinguished here by asking GitHub for its merge commit and + testing whether that commit is actually in `ref`. Only the unrecognised ones are looked up, and + for a bootstrap those are the handful merged since the last documentation build. + + Anything genuinely present but unrecognised is returned, and the caller refuses rather than + guessing a cursor. + """ + log = window.git(["log", "--first-parent", "--format=%s", ref], repo_dir) + seen = {window.pr_number_of_subject(s) for s in log.splitlines()} + seen.discard(None) + out = [] + for number in sorted(set(area_prs) - seen): + raw = gh.gh(["api", f"repos/{gh.CODE_REPO}/pulls/{int(number)}", "--jq", + '.merge_commit_sha // ""']).strip() + if raw and window.is_ancestor(repo_dir, raw, ref): + out.append(number) + return out + + def bootstrap_from_sha(repo_dir, area, area_prs, ref=CODE_REF): """A `from_sha` for an area that has never been reported, or None if it has no merged PRs. @@ -156,6 +186,15 @@ def bootstrap_from_sha(repo_dir, area, area_prs, ref=CODE_REF): # number would put the cursor after any labelled pull request that opened later but merged # sooner, making that work unreportable for good. Two of the fourteen roadmaps had exactly that # shape when this was written. + # Refuse rather than guess if any labelled pull request is in this history but was not recognised + # in it: the cursor would then be computed from a later merge and skip it, permanently. + unaccounted = unaccounted_prs(repo_dir, numbers, ref=ref) + if unaccounted: + raise window.GitError( + f"{area} has labelled pull requests in {ref} whose merge commits carry no pull request " + f"number ({', '.join(f'#{n}' for n in unaccounted[:5])}); the start of its history " + f"cannot be determined from commit subjects" + ) found = window.earliest_merged(repo_dir, numbers, ref=ref) earliest, merge = found if found else (min(numbers), None) if merge is None: @@ -310,16 +349,6 @@ def build_plan( continue bootstrapped = False - if from_sha is None and not only_area: - # A roadmap with no log yet needs a FIRST report, and the gate does not auto-merge those: - # where a roadmap's history begins cannot be verified mechanically, so a human decides it - # once. Selecting such an area automatically would generate a report every day, have it - # refused every day, and burn a writing round each time. - # - # Naming the area explicitly is how a person asks for that first report. After it lands, - # the area has a cursor and is picked up automatically like any other. - skipped.append(f"{area}: never reported; run with --area {area} to bootstrap it") - continue if from_sha is None: from_sha = bootstrap_from_sha(code_dir, area, area_prs, ref=ref) bootstrapped = True diff --git a/tests/test_gate.py b/tests/test_gate.py index bd74f32..3e8b041 100644 --- a/tests/test_gate.py +++ b/tests/test_gate.py @@ -110,7 +110,7 @@ def make_window(**over): def call(pr=None, changed=None, tree=None, content=None, checks=None, cursor=FROM, window=-1, compare_status="ahead", behind_by=0, old_paths=None, last_report_at=None, now=NOW, - area_exists=True): + area_exists=True, expected_bootstrap=None): old_status, new_status, old_progress, new_progress = content or make_content() return gate.decide( pr=pr or make_pr(), @@ -126,6 +126,7 @@ def call(pr=None, changed=None, tree=None, content=None, checks=None, cursor=FRO last_report_at=last_report_at, now=now, area_exists=area_exists, + expected_bootstrap_from_sha=expected_bootstrap, current_main_cursor=cursor, compare_status=compare_status, behind_by=behind_by, @@ -200,16 +201,20 @@ def test_a_fork_that_force_pushes_after_opening_gains_nothing(): refuses(lambda: call(pr=pr, checks=[build_run(head_sha="0" * 40)]), "names head") -def test_a_first_report_is_never_auto_merged(): - """Where a roadmap's log begins cannot be verified mechanically, so a human decides it once. +def test_a_first_report_must_start_where_the_roadmap_starts(): + """A bootstrap has no cursor to continue from, so it would otherwise pick its own start, and + windows only move forward -- anything before it is unreportable for good. - Three implementations tried: by lowest pull request number, by merge timestamp, and by walking - the commits endpoint. The REST API expresses neither first-parent traversal nor an ordering - guarantee, and this history is not linear, so ancestry checks cannot recover it either. Refusing - is the honest outcome; every report after the first merges unattended. + The collector clones the code repository and asks git, using the same functions the planner uses + over the same data, so the two agree by construction rather than by luck. Three earlier versions + tried to answer this through the REST API and all three were wrong. """ - reason = refuses(lambda: call(cursor=None), "no reported history yet") - assert "human review" in reason + assert call(cursor=None, expected_bootstrap=FROM)["area"] == AREA + refuses(lambda: call(cursor=None, expected_bootstrap="e" * 40), "expected eeeeeee") + + +def test_a_first_report_is_refused_when_the_start_is_unknown(): + refuses(lambda: call(cursor=None, expected_bootstrap=None), "could not be determined") def test_refuses_an_invented_roadmap(): @@ -230,7 +235,7 @@ def test_refuses_a_second_report_for_the_same_roadmap_too_soon(): Every one would pass every other check, and every one would post to Zulip. The cadence is therefore enforced here as well as in the planner. """ - refuses(lambda: call(last_report_at="2026-07-30T02:00:00Z"), "reported 10.0h ago") + refuses(lambda: call(last_report_at="2026-07-30T09:00:00Z"), "reported 3.0h ago") def test_allows_a_report_once_the_interval_has_passed(): diff --git a/tests/test_in_flight.py b/tests/test_in_flight.py index af99fbc..8241541 100644 --- a/tests/test_in_flight.py +++ b/tests/test_in_flight.py @@ -59,9 +59,9 @@ def test_the_stale_note_says_what_to_do(): def test_the_cutoff_boundary_still_blocks(): - blocked, _ = plan.in_flight_areas([pr(hours_old=24.0)], now=NOW, stale_hours=24.0, owners={OURS}) + blocked, _ = plan.in_flight_areas([pr(hours_old=8.0)], now=NOW, stale_hours=8.0, owners={OURS}) assert set(blocked) == {"PDE"}, "exactly at the cutoff is still in flight" - blocked, _ = plan.in_flight_areas([pr(hours_old=24.1)], now=NOW, stale_hours=24.0, owners={OURS}) + blocked, _ = plan.in_flight_areas([pr(hours_old=8.1)], now=NOW, stale_hours=8.0, owners={OURS}) assert blocked == {} @@ -96,6 +96,13 @@ def test_the_default_cutoff_matches_the_cadence(): assert plan.STALE_PR_HOURS == plan.IDLE_HOURS +def test_the_server_side_limit_never_refuses_a_report_the_planner_thinks_due(): + """The gate's per-roadmap gap must stay under the planner's cadence, or reports are generated + and then refused.""" + from progress import gate + assert gate.MIN_REPORT_INTERVAL_HOURS < plan.IDLE_HOURS + + def test_a_strangers_pull_request_does_not_mark_an_area_in_flight(): """Anyone may open one on a `progress/*` branch. If a stranger's counted, they could freeze a diff --git a/tests/test_window.py b/tests/test_window.py index 9fb8879..c79365b 100644 --- a/tests/test_window.py +++ b/tests/test_window.py @@ -142,8 +142,8 @@ def test_cadence_never_updated(): def test_cadence_too_recent_raises(): - commits = [("2026-07-30T02:00:00Z", "progress: PDE 2026-07-30 (#9)")] - raises(plan.NotDue, lambda: plan.check_cadence(commits, now=NOW), "10.0h ago") + commits = [("2026-07-30T09:00:00Z", "progress: PDE 2026-07-30 (#9)")] + raises(plan.NotDue, lambda: plan.check_cadence(commits, now=NOW), "3.0h ago") def test_cadence_old_enough_passes(): @@ -274,6 +274,55 @@ def test_earliest_merged_ignores_unlabelled_pull_requests(): assert window.earliest_merged(d, [], ref="main") is None assert window.earliest_merged(d, [12345], ref="main") is None + +def test_a_labelled_pr_with_no_number_in_its_subject_fails_closed(): + """The hazard: `earliest_merged` finds pull requests by matching numbers in commit subjects. + + One whose merge subject omits its number is invisible, so the cursor would be computed from a + LATER merge and skip it, permanently. Two implementations agreeing does not catch this -- they + share the omission, which is how three earlier versions of this check passed review while wrong. + """ + import os, subprocess, tempfile + from progress import gh as gh_mod, plan as plan_mod + with tempfile.TemporaryDirectory() as d: + env = dict(os.environ, GIT_AUTHOR_NAME="t", GIT_AUTHOR_EMAIL="t@e", + GIT_COMMITTER_NAME="t", GIT_COMMITTER_EMAIL="t@e") + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=d, check=True, capture_output=True) + for subject in ("root", "feat: no number here at all", "feat: later one (#20)"): + subprocess.run(["git", "commit", "-q", "--allow-empty", "-m", subject], + cwd=d, check=True, capture_output=True, env=env) + # #10 merged as the numberless commit; resolve it the way the real check does. + numberless = subprocess.run(["git", "rev-parse", "HEAD~1"], cwd=d, + capture_output=True, text=True).stdout.strip() + orig = gh_mod.gh + gh_mod.gh = lambda args, **kw: numberless + "\n" + try: + assert plan_mod.unaccounted_prs(d, [10, 20], ref="main") == [10] + raises(window.GitError, + lambda: plan_mod.bootstrap_from_sha(d, "PDE", [10, 20], ref="main"), + "no pull request number") + finally: + gh_mod.gh = orig + + +def test_a_labelled_pr_merged_after_the_tip_is_not_flagged(): + """Benign: it is not in this history yet, and a later window will cover it.""" + import os, subprocess, tempfile + from progress import gh as gh_mod, plan as plan_mod + with tempfile.TemporaryDirectory() as d: + env = dict(os.environ, GIT_AUTHOR_NAME="t", GIT_AUTHOR_EMAIL="t@e", + GIT_COMMITTER_NAME="t", GIT_COMMITTER_EMAIL="t@e") + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=d, check=True, capture_output=True) + for subject in ("root", "feat: ours (#5)"): + subprocess.run(["git", "commit", "-q", "--allow-empty", "-m", subject], + cwd=d, check=True, capture_output=True, env=env) + orig = gh_mod.gh + gh_mod.gh = lambda args, **kw: "\n" # no merge commit we can see + try: + assert plan_mod.unaccounted_prs(d, [5, 99], ref="main") == [] + finally: + gh_mod.gh = orig + for _name, _fn in sorted(globals().items()): if _name.startswith("test_") and callable(_fn): check(_name, _fn)