Facts port: the snapshot's diff and files read by one compare pinned to the head commit (#284)

adapters/facts.py

@@ -3,9 +3,8 @@
 
 The pull request is read first and once more last (`adapters/pull.unchanged`): a push, a retarget, or any change
 GitHub records on it between makes the parts of two pull requests, and the snapshot is refused, to be read again at the
-next pass. A head pushed away and back
-between the two reads is not seen: the diff and the files are the pull request's, not pinned to its head commit, so they
-may be of the commit between (the known limit; pinning them is still to be built). A part that cannot be read
+next pass. The diff and the files are read by one compare pinned to the first read's head commit (issue 284), so a head
+pushed away and back between the two reads still gives the parts of that commit. A part that cannot be read
 refuses the whole; a list that was not read whole says so in its `complete` flag, and the Kernel defers on it.
 
 One part is not read yet, and says so: `acceptance` is null, and whole only where no issue the pull request closes
@@ -46,8 +45,8 @@ def snapshot(transport, repo: str, number: int, identities: dict, accounts: time
     own, title = pull.own(first, identities)
     branch = pull.base(transport, first)
     pusher, pushed_at = pull.last_push(transport, first)
-    content = pull.diff(transport, first)
-    paths = pull.changed_paths(transport, first)
+    content = pull.diff(transport, first, branch["head"])
+    paths = pull.changed_paths(transport, first, branch["head"])
     events, timeline_whole = timeline.events(transport, first)
     reviews, reviews_whole = timeline.reviews(transport, first, events, accounts, branch["head"])
     writer = timeline.since_writer(events, accounts)

adapters/pull.py

@@ -15,7 +15,7 @@
 
 from adapters.github import DIFF, Unanswered, raw, request
 
-PAGE = 100          # files on one page of a pull request's list of files
+COMPARED = 300      # the files a compare lists at most, as GitHub documents it, unverified; more is read short
 # What GitHub says of a file. A rename and a copy carry the name the file had; no other status does.
 STATUSES = {"added", "removed", "modified", "renamed", "copied", "changed", "unchanged"}
 MOVED = {"renamed", "copied"}
@@ -189,21 +189,36 @@ def _counts(text: str):
     return files, added, deleted
 
 
-def diff(transport, pull: dict) -> dict:
-    """`diff`, `content_id` and `complete.diff`, from the pull request's own diff against its merge base.
+def _compare(pull: dict, base_head: str) -> str:
+    """The compare of the base branch's head with the pull request's head commit, as its first read gave it: the one
+    read the diff and the files are taken from, so both are of that commit, whatever is pushed while the snapshot is
+    read (issue 284's ruling)."""
+    head = str(pull["head"].get("sha", ""))
+    if not SHA.match(head):
+        raise Unanswered("the pull request's head commit cannot be read")
+    if not SHA.match(str(base_head)):
+        raise Unanswered("the base branch's head commit cannot be read")
+    return "/repos/%s/compare/%s...%s" % (pull["base"]["repo"]["full_name"], base_head, head)
+
+
+def diff(transport, pull: dict, base_head: str) -> dict:
+    """`diff`, `content_id` and `complete.diff`, from the diff of the head commit against its merge base with the base
+    branch's head, `GET /repos/{repo}/compare/{base_head}...{head}`, pinned to the head of the pull request's first read
+    (issue 284). P-29 shows those bytes equal the pull request's own diff (`trials/P-29.base.json`), so the content id is
+    the one `adapters/timeline.py` gives a review's commit.
 
     The diff is checked against the counts of the same pull request's read, its files, added and deleted lines: a
     difference is a short read, `complete.diff` false. The content id is the sha256 of the bytes GitHub sent. A 406
     `too_large` is GitHub's whole answer that it will not serve the diff: `truncated`, read whole, which holds the pull
     request as too large; its `content_id` is the head commit's, so any push is new content and nothing is read as
-    unchanged (decision 0002). Any other 406 is refused. No read is built from `patch`."""
+    unchanged (decision 0002). P-29 grounds that answer for a pull request's diff; compare's is read the same,
+    unverified. A 404, a head no longer there, refuses the snapshot. Any other 406 is refused. No read is built from
+    `patch`."""
     files, added, deleted = pull.get("changed_files"), pull.get("additions"), pull.get("deletions")
     if not all(isinstance(n, int) and not isinstance(n, bool) and n >= 0 for n in (files, added, deleted)):
         raise Unanswered("the pull request's counts cannot be read")
-    head = str(pull["head"].get("sha", ""))
-    if not SHA.match(head):
-        raise Unanswered("the pull request's head commit cannot be read")
-    path = "/repos/%s/pulls/%d" % (pull["base"]["repo"]["full_name"], pull["number"])
+    path = _compare(pull, base_head)
+    head = str(pull["head"]["sha"])
     answer = transport.send(request("GET", path, accept=DIFF))
     size = {"files": files, "lines": added + deleted}
     if answer["status"] == 406:
@@ -220,37 +235,38 @@ def diff(transport, pull: dict) -> dict:
             "complete": whole}
 
 
-def changed_paths(transport, pull: dict) -> dict:
-    """`changed_paths` and `complete.changed_paths`: every file the pull request changes, page by page, each by its path
-    and, for a rename or a copy, the path it had before. Whole only when every page was read and the list holds exactly
-    as many files as the pull request's count, each once; GitHub stops listing at 3,000, and a page past the end is a
-    200 with nothing in it, so a list that ends short of the count is read as short, never as the whole (P-29, decision
-    0020)."""
+def changed_paths(transport, pull: dict, base_head: str) -> dict:
+    """`changed_paths` and `complete.changed_paths`: every file the head commit changes against its merge base, from the
+    same compare as the diff, pinned to the head of the pull request's first read (issue 284), each by its path and,
+    for a rename or a copy, the path it had before. Whole only when the list holds exactly as many files as the pull
+    request's count, each once. A compare lists at most COMPARED files, as GitHub documents it (no trial records it), so
+    a change of more is read short by its count alone, with nothing asked, and the Kernel defers it for the Maintainer:
+    no answer GitHub gives for such a change can refuse the snapshot (issue 284's ruling; the pull request's own list
+    gave 3,000, P-29)."""
     want = pull.get("changed_files")
     if not isinstance(want, int) or isinstance(want, bool) or want < 0:
         raise Unanswered("the pull request's count of files cannot be read")
-    base = "/repos/%s/pulls/%d/files?per_page=%d" % (pull["base"]["repo"]["full_name"], pull["number"], PAGE)
-    paths, page = [], 1
-    while True:
-        rows = _ok(transport.send(request("GET", base if page == 1 else "%s&page=%d" % (base, page))), "the files")
-        if not isinstance(rows, list):
-            raise Unanswered("a page of files is not a list")
-        for row in rows:
-            name = row.get("filename") if isinstance(row, dict) else None
-            if not isinstance(name, str) or not name:
-                return {"changed_paths": paths, "complete": False}
-            status = row.get("status")
-            moved = status in MOVED
-            before = row.get("previous_filename") if moved else None
-            # A status GitHub does not give, a move without its old name, or an old name on anything but a move, is a
-            # row that cannot be read: the list is short, never read as an ordinary change.
-            if status not in STATUSES or (moved and not (isinstance(before, str) and before)) \
-                    or (not moved and row.get("previous_filename") is not None):
-                return {"changed_paths": paths, "complete": False}
-            paths.append({"path": name, "previous": before})
-        if len(rows) < PAGE or len(paths) >= want:
-            break
-        page += 1
+    if want > COMPARED:
+        return {"changed_paths": [], "complete": False}
+    path = _compare(pull, base_head)
+    compared = _ok(transport.send(request("GET", path)), "the compare")
+    rows = compared.get("files") if isinstance(compared, dict) else None
+    if not isinstance(rows, list):
+        raise Unanswered("the compare's files are not a list")
+    paths = []
+    for row in rows:
+        name = row.get("filename") if isinstance(row, dict) else None
+        if not isinstance(name, str) or not name:
+            return {"changed_paths": paths, "complete": False}
+        status = row.get("status")
+        moved = status in MOVED
+        before = row.get("previous_filename") if moved else None
+        # A status GitHub does not give, a move without its old name, or an old name on anything but a move, is a
+        # row that cannot be read: the list is short, never read as an ordinary change.
+        if status not in STATUSES or (moved and not (isinstance(before, str) and before)) \
+                or (not moved and row.get("previous_filename") is not None):
+            return {"changed_paths": paths, "complete": False}
+        paths.append({"path": name, "previous": before})
     names = [p["path"] for p in paths]
     whole = len(paths) == want and len(set(names)) == len(names)
     return {"changed_paths": paths, "complete": whole}

docs/architecture/software-factory-design.md

@@ -327,8 +327,8 @@ for contract tests, so one schema has three uses.
 | `mergeable` | Whether the head merges cleanly into the base: true, false, or null while GitHub has not worked it out. As GitHub last worked it out: a move of the base alone does not make GitHub work it out again (P-29) |
 | `diff_size` | Files and lines changed |
 | `head`, `base`, `merge_base` | Commit hashes: the head commit; the base branch's head, read at the pass, never the pull request's `base.sha`, which moves only when the head is pushed (P-29); and the merge base GitHub gives for the head against that branch, `compare`'s `merge_base_commit`. None is in `schemas/facts.schema.json` until something reads it |
-| `content_id` | Hash of the pull request's own diff against its merge base, computed by the port |
-| `changed_paths` | One record for each file in that diff, as many as `diff_size` counts files, each by its own path once, from the top of the repo, with `previous`, the path a renamed or copied file had before, null for any other change. A list of another length, a record not in that shape, or a path with an empty, `.` or `..` name, is unreadable, `paths_unreadable`, for the Maintainer. The Kernel matches each against the protected paths and the Lenses' inputs, and never shows one (§4.2, decision 0020) |
+| `content_id` | Hash of the head commit's diff against its merge base, computed by the port. It is read by compare, `GET /repos/{repo}/compare/{base}...{head}`, at the head of the snapshot's first read and the base branch's head the pass read, so a head pushed away and back during the snapshot still gives that commit's content (issue 284). P-29 shows those bytes are the pull request's own diff. A `406` `too_large` reads as truncated, with the head commit's hash as its id: P-29 grounds that answer for the pull request's diff, not for compare's |
+| `changed_paths` | One record for each file in that diff, as many as `diff_size` counts files, each by its own path once, from the top of the repo, with `previous`, the path a renamed or copied file had before, null for any other change. A list of another length, a record not in that shape, or a path with an empty, `.` or `..` name, is unreadable, `paths_unreadable`, for the Maintainer. The Kernel matches each against the protected paths and the Lenses' inputs, and never shows one (§4.2, decision 0020). Read from the same compare as `content_id`, which lists at most 300 files, as GitHub documents it (unverified): a change of more is read short by its count alone, with nothing asked, and the Kernel defers it for the Maintainer (issue 284) |
 | `label_events` | Every `labeled` and `unlabeled` event on the timeline: the label, who added or removed it (kind, name and write access), and when |
 | `reviews` | Who submitted it (`by`: kind, name and whether that actor has write access, as a thread's opening actor), state (`approved`, `changes_requested`, `commented` or `dismissed`, as GitHub holds it now), commit, `content_id`, `submitted_at`; and, from the timeline, `dismissed_by`: who dismissed it, of what kind and with what access, and `dismissed_from`: what it was before (`approved`, `changes_requested` or `commented`). Both are null unless it was dismissed |
 | `threads` | Thread id, path, line, when it was opened, resolved, outdated, opening actor and resolving actor, each with its kind and whether it has write access (the resolving actor null unless resolved); and the Finding's `category` and `severity`. For a required Lens's thread both come from that Lens's own Finding record, in its Run's `applied.json` (§3.7, §3.10), joined by `surface.key`, never from the comment text, which anyone with write access can edit. A person's thread is `human`, with no severity; an Advisory reviewer's, which no Finding record names, has neither |

docs/decisions/0020-a-change-to-the-factorys-own-rules-or-a-lenss-inputs-is-reviewed-by-a-model-lens.md

@@ -97,5 +97,6 @@ not weaken the Lens that judges it, and that story comes next.
 | The session hooks ask about `config/apps.json`, and list a Lens's input at a stop without asking about it | `tests/test_hooks.py::test_an_edit_elsewhere_is_not_asked_about`, `tests/test_hooks.py::test_a_lens_input_changed_is_listed_as_reported` |
 | In the simulator a Role, the dependency bot and the Maintainer change the list, the list is read short or leaves a path out, and nothing that changes it is approved, enqueued, left queued or merged without every model Lens's review of that head (I-45) | `tests/test_simulator.py::test_the_seeds_change_the_list_by_a_role_the_dependency_bot_and_the_maintainer` |
 | The port reads every path the pull request changes, both names of a rename among them, and says when the list was cut short. "Port: the paths a pull request changes, and both names of a rename" | `tests/test_adapters.py::test_every_file_is_read_and_the_list_is_whole_when_it_holds_the_count`, `tests/test_adapters.py::test_a_list_github_stops_short_of_the_count_is_never_the_whole`, `tests/test_adapters.py::test_a_rename_or_a_copy_gives_both_names_and_one_without_its_old_name_is_short`, `tests/test_adapters.py::test_a_renamed_file_is_read_with_its_old_name_from_a_fixture` |
+| The paths are read from the compare of the base branch's head with the head of the snapshot's first read, never off the pull request, so they are that commit's; a compare lists at most 300 files, as GitHub documents it (unverified), so a change of more is read short by its count alone and deferred for the Maintainer. "Facts port: the snapshot's diff and files pinned to the head commit" | `tests/test_adapters.py::test_a_file_list_read_at_another_commit_is_not_asked_for`, `tests/test_adapters.py::test_a_list_github_stops_short_of_the_count_is_never_the_whole`, `tests/test_adapters.py::test_a_list_of_up_to_three_hundred_files_is_whole` |
 | `apply` refuses a Role's push to a protected path or a Lens's input that no model Lens has reviewed. "Harness: apply refuses a push to the list that no model Lens has reviewed" | waiting on `harness/` |
 | What GitHub holds is recorded: the `main` ruleset requires no review, as `config/rulesets.json` says, so a ruleset that requires the machine user's approval changes that file, and a live ruleset that differs from it is drift | `tests/test_tools.py::test_a_changed_rule_is_drift` |

sim/grounding.py

@@ -126,6 +126,8 @@
     ("a_pull_request_lists_the_paths_it_changes", "Every file a pull request changes is read once, as many as the diff "
      "counts, by its path from the top of the repo, with the path a renamed file had before; an unchanged copy is "
      "listed as an added file", ("P-43",), "trials/P-43.json", None),
+    # The port reads a compare, not this list (issue 284): a change of more files than a compare lists, 300 as GitHub
+    # documents it and no trial records, is read short by changed_files alone, which the fixture's 3,001 shows.
     ("a_path_list_can_leave_out_a_file", "A list of the files a pull request changes can leave one out while it says it "
      "was read in full; it then holds fewer records than the diff has files: GitHub's list stops at its cap with a 200 "
      "and an empty next page, and only changed_files shows it short", ("P-29",), "trials/P-29.truncated.json", None),