Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion .github/scripts/collect.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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}
Expand All @@ -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
Expand All @@ -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(),
Expand Down
46 changes: 20 additions & 26 deletions progress/gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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"),
Expand Down
55 changes: 42 additions & 13 deletions progress/plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""
Expand All @@ -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
Expand Down Expand Up @@ -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.

Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down
25 changes: 15 additions & 10 deletions tests/test_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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,
Expand Down Expand Up @@ -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():
Expand All @@ -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():
Expand Down
11 changes: 9 additions & 2 deletions tests/test_in_flight.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 == {}


Expand Down Expand Up @@ -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
Expand Down
Loading
Loading