Skip to content

Facts port: the snapshot's diff and files pinned to the head commit, so a head pushed away and back is seen #284

Closed

Description

@don-petry

Part of the epic "Epic: slice 1, first approval. A Release on next reviews a pull request to this repo, and the machine user's approval stands".

Why. adapters/facts.py reads the pull request first and once more last (pull.unchanged). A head pushed away and back between the two reads is not seen, because the diff and the file list are read off the pull request, not pinned to the head commit. An author with push access could push an approved commit B, then their own A, within seconds. The snapshot would then pair B's content id with head A, and an approval keyed on content would stand for A. The security pass on #278 found this, and the code states it as a known limit.

Done when

  • pull.diff reads GET /repos/{repo}/compare/{base_head}...{head_sha} with the diff Accept, as timeline._content does. P-29 (trials/P-29.base.json exchange 24) shows those bytes equal the pull request's own diff, so the content id is unchanged. Whether too_large is grounded on compare is said in the row.
  • The file list is pinned too, or a snapshot whose head was pushed during the read is refused. Compare lists at most 300 files and the pull request's list allows 3,000 (decision 0020), so this choice is ruled in this story first.
  • A test shows that a diff read at another commit than the head cannot give the snapshot's content id, with a mutant; the known-limit sentence in adapters/facts.py is removed.

Design. §3.3; decisions 0002, 0020, 0044.

Activity

  1. don-petry commented on Oct 21, 2026

    @don-petry
    ContributorAuthor

    Working ruling (lane a, 2026-10-21). The Maintainer may overturn it in review.

    Today. adapters/pull.py reads the diff with GET /repos/{repo}/pulls/{n} and the diff Accept, and the files with GET /repos/{repo}/pulls/{n}/files. Both are read off the pull request at whatever head it has at that moment. pull.unchanged compares the first and last reads of the pull request, so it does not see a head pushed away and back between them. A snapshot can then pair head A with content of commit B.

    Ruling.

    1. The diff is pinned to the head commit. It is read as GET /repos/{repo}/compare/{base_head}...{head_sha} with the diff Accept, where head_sha is the head of the snapshot's first read of the pull request, and base_head is the base branch's head as the pass read it. That is the read adapters/timeline.py::_content already makes for a review's commit, so a review's content id and the head's are made the same way. P-29 (trials/P-29.base.json) shows the compare diff's bytes equal the pull request's own diff, so the content id is unchanged.
      • A 406 too_large from compare reads as the pull request's does: truncated, with the head commit's id hashed. P-29 grounds the pull request's too_large (trials/P-29.truncated.json) but not compare's, so the row says so.
      • A 404, a commit no longer there, refuses the snapshot.
    2. The file list is pinned the same way. It is read from the same compare, GET /repos/{repo}/compare/{base_head}...{head_sha} as JSON, files, each with its filename, status and previous_filename (P-29 exchange 10 holds such a read).
      • Compare lists at most 300 files. A list shorter than the pull request's changed_files, read in the same snapshot at that head, is read short: complete.changed_paths false, and the Kernel defers.
      • So the Factory does not read a change of more than 300 files. It waits for the Maintainer. Decision 0020's 3,000 becomes 300 for the pinned read.
    3. Why 300 and not a second, unpinned read. Pairing a file list with the wrong commit is the hole this story closes. A protected path missed in that list would let a change to the Factory's own rules skip the model review (decision 0020). Refusing more than 300 files costs little: max_diff_lines (4,000) already holds most such changes, and a person reads the rest.
    4. The snapshot. With the diff and the files pinned, a head pushed away and back between the first and last reads gives the parts of one commit, the first read's head. adapters/facts.py drops its known-limit sentence.

    Build. pull.diff and pull.changed_paths read compare at the first read's head. A test shows a diff read at another commit cannot give the snapshot's content id, and another shows a file list of more than 300 is read short. Each has a mutant. The design's §3.3 row, and a row on decision 0020 (renumbered as its rows allow), say so.

  2. added a commit that references this issue on Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions