diff --git a/adapters/pull.py b/adapters/pull.py index 665d5f8d..4f0a4fc7 100644 --- a/adapters/pull.py +++ b/adapters/pull.py @@ -68,8 +68,10 @@ def unchanged(transport, pull: dict) -> bool: which no trial has asked yet. So a snapshot is never of a pull request that changed between its first read and its last; one that did is read again at the next pass.""" again = read(transport, pull["base"]["repo"]["full_name"], pull["number"]) - return (_same(again, pull) and isinstance(pull.get("updated_at"), str) - and again.get("updated_at") == pull["updated_at"]) + if not (isinstance(pull.get("updated_at"), str) and isinstance(again.get("updated_at"), str)): + # A time that cannot be read is no move: the read GitHub did not answer, not one that earns a pass's grace (#658). + raise Unanswered("the pull request's updated_at cannot be read") + return _same(again, pull) and again["updated_at"] == pull["updated_at"] def _account(actor) -> tuple: diff --git a/controller/AGENTS.md b/controller/AGENTS.md index a827453a..3b7ec39e 100644 --- a/controller/AGENTS.md +++ b/controller/AGENTS.md @@ -11,7 +11,7 @@ Not built yet, by the stories that hold their rules: relay.py, backpressure.py, | The pass record holds what the Kernel was given, and replays every Decision | `tests/test_controller.py::test_the_pass_record_holds_what_the_kernel_was_given_and_replays_it` | | A Run id is minted for an admission, an approval, or a queue act once its workflow is named | `tests/test_controller.py::test_a_queue_act_gets_a_run_id_and_a_dispatch_once_its_workflow_is_named` | | A Run is dispatched by its run-name; a capacity fault defers model Runs for its window | `tests/test_controller.py::test_the_runs_to_dispatch_are_named_by_their_run_names`, `tests/test_controller.py::test_a_capacity_fault_defers_model_runs_for_its_window` | -| An item whose facts are not facts, or listed twice, is left out, as one alert; any other is decided as without it | `tests/test_controller.py::test_an_item_whose_facts_are_not_facts_is_left_out_and_raises_one_alert` | +| An item whose facts are not facts, or listed twice, is left out as one alert; a pull request moved once, with none; the rest are decided | `tests/test_controller.py::test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert` | | An issue is asked of the issues' Kernel, under its own Lifecycle, and its repo's Pause is read | `tests/test_controller.py::test_an_issue_is_asked_of_the_issues_kernel_with_its_own_lifecycle` | | A record that cannot validate is refused whole, and nothing is dispatched | `tests/test_controller.py::test_a_record_that_cannot_validate_is_refused_and_nothing_is_dispatched` | | A Pause on the org or a repo is a Pause; a value but exact `true`, exact `false` or none is unreadable | `tests/test_controller.py::test_a_pause_on_the_org_or_the_repo_is_a_pause_and_any_other_value_is_unreadable` | diff --git a/controller/__main__.py b/controller/__main__.py index 8081c240..4fd3d8e0 100644 --- a/controller/__main__.py +++ b/controller/__main__.py @@ -13,7 +13,8 @@ effects step holds to it, and the view's three files under `view/`. The Release is the one tag `factory/vX.Y.Z` on the commit the pass checked out (`harness/steps.release`), read before any key. A snapshot that cannot be read leaves its pull -request out of the pass, by its item, as one alert. A list of open pull requests not read whole, a Release, a key or +request out of the pass, by its item and the code of why, as one alert; one that moved while it was read, and had not in +the last pass, is deferred with no alert and its writes are owed to the next pass (#658). A list of open pull requests not read whole, a Release, a key or a run id that cannot be read, or a pass `pass_.py` refuses, writes nothing, so that nothing is dispatched; the step prints one code and exits 1, and nothing it prints is GitHub's text.""" from __future__ import annotations @@ -30,7 +31,7 @@ from adapters.facts import accounts_of as accounts, dispatcher_of as dispatcher, opener_of as opener, queue_of from adapters.github import Live, Unanswered from controller import configuration as configured, pass_, reads as reads_, reconcile, writes as writes_ -from controller.shared import Refused +from controller.shared import Refused, key as page from harness import steps ROOT = pathlib.Path(__file__).resolve().parent.parent @@ -92,11 +93,12 @@ def dated(answer) -> list: return [{"date": at["date"], "age": at["age"]}] if at else [] -def writes_seen(transport, config: dict, numbers: list, kept: dict, before, since: str, clock) -> tuple: +def writes_seen(transport, config: dict, numbers: list, kept: dict, before, since: str, clock, owed=()) -> tuple: """`(seen/1, the pull requests read short)`: what each Factory identity wrote on what this pass served, or on what the pass before served or could not read and has left (decision 0061). What a snapshot could not read is short, and one that is short is read again by the next pass, so that nothing leaves unread. So is a pass with no records before it to say - what has left. Nothing is guessed.""" + what has left. Nothing is guessed. One `owed`, a pull request that moved for the first time, is not read: it is owed + to the next pass, as one read short is, and leaves this reconciliation whole (#658).""" controller = config["controller"] home, accounts_held = controller["home"]["repo"], controller["accounts"] identities, _ = seen_.identities(accounts_held) @@ -105,6 +107,9 @@ def writes_seen(transport, config: dict, numbers: list, kept: dict, before, sinc served = {e["facts"]["item"]["number"] for e in last} | set(reconcile.owed(before)) parts, unread = [], [] for number, held in [(n, kept.get(n)) for n in numbers] + [(n, seen_.LEFT) for n in sorted(served - set(numbers))]: + if number in owed: + unread.append(number) + continue try: part = seen_.read(transport, {"repo": home, "type": "pull_request", "number": number}, held, identities, apps, since) @@ -176,11 +181,17 @@ def read(transport, config: dict, run_id: str, previous, now: str, clock, starte pulls.append({"facts": facts, "title": title, "head": head}) kept[number] = held facts_reads += dated(through.first) - except Unanswered: - pulls.append({"facts": {"item": {"repo": home, "type": "pull_request", "number": number}}, "title": None}) + except Unanswered as err: + # Its code goes in the record: one that moved while it was read is not one GitHub did not answer (#658). + pulls.append({"facts": {"item": {"repo": home, "type": "pull_request", "number": number}}, "title": None, + "unread": reads_.MOVED if isinstance(err, facts_.Moved) else reads_.UNANSWERED}) before = pass_.earlier(reads_.previous_records(previous), now) since = utc(lambda: clock() - WINDOW - controller["not_run_seconds"]) - seen, unread = writes_seen(transport, config, numbers, kept, before, since, clock) + # A pull request that moved for the first time is owed to the next pass's reconciliation, not read short here. + left = [e for e in pulls if "unread" in e] + graced = pass_.moved_once([e["facts"]["item"] for e in left], [e["unread"] for e in left], before) + owed = {e["facts"]["item"]["number"] for e in left if page(e["facts"]["item"]) in graced} + seen, unread = writes_seen(transport, config, numbers, kept, before, since, clock, owed) return {"started": started, "pause": pause_.read(transport, home.split("/")[0], [home]), "records": runs, "pull_requests": pulls, "issues": [], "facts_reads": facts_reads, "usage": {}, "seen": seen, "unread": unread, "effects": recorded_effects(transport, config, runs, int(run_id), before, since)} diff --git a/controller/pass_.py b/controller/pass_.py index 8208a167..83b07793 100644 --- a/controller/pass_.py +++ b/controller/pass_.py @@ -262,6 +262,20 @@ def status_head(entry: dict, kernel: dict) -> dict: return {} if pull_request.left_alone(facts, kernel) else {"head": entry["head"]} +def moved_once(left_out: list, why: list, before) -> set: + """The pages of the items left out because they moved while read that the last pass record read and did not leave + out as moved: each is deferred with no alert, once, and its writes are owed to the next pass (#658). It is derived + from that record, never kept: a last record that cannot be read, or holds no `left_out`, cannot say, so each move + counts as a second.""" + last = before[-1] if isinstance(before, list) and before else None + held = last.get("left_out") if isinstance(last, dict) else None + if not isinstance(held, list): + return set() + again = {key(e["item"]) for e in held if e["code"] == reads_.MOVED and e["item"] is not None} + return {key(i) for i, code in zip(left_out, why) if code == reads_.MOVED and reads_.item(i) is not None + and i["type"] == "pull_request" and key(i) not in again} + + def run(reads: dict, config: dict, now: str, previous, mint) -> dict: """One pass: the record, the Runs to dispatch, the acts, the view, the alerts and whether a successor starts.""" rules = reads_.configured(config["controller"]) @@ -270,9 +284,13 @@ def run(reads: dict, config: dict, now: str, previous, mint) -> dict: previous, before = (None, []) if before is None else (before, before) # Facts that are not facts, and an item listed twice, never reach the Kernel and no record could hold them: the # item is left out of this pass, as one alert, and every other item is decided as it would be without it. - pulls, left_out = reads_.items(copy.deepcopy(reads.get("pull_requests")), "facts") - issues, also = reads_.items(copy.deepcopy(reads.get("issues")), "issue-facts") - left_out += also + pulls, left_out, why = reads_.items(copy.deepcopy(reads.get("pull_requests")), "facts") + issues, also, why_also = reads_.items(copy.deepcopy(reads.get("issues")), "issue-facts") + left_out, why = left_out + also, why + why_also + # A pull request that moved while it was read, and had not in the last pass, is deferred with no alert: the pass its + # change starts reads it again. Any other left out, or one that moved twice in a row, is the alert (#658). + graced = moved_once(left_out, why, before) + alarmed = [i for i in left_out if reads_.item(i) is None or key(i) not in graced] repos = {e["facts"]["item"]["repo"] for e in pulls + issues} records = reads.get("records") @@ -304,6 +322,7 @@ def asked(entries: list, decide, lifecycle: dict) -> list: + [dict(r, read="facts") if isinstance(r, dict) else r for r in reads.get("facts_reads") or []], "pull_requests": asked(pulls, pull_request.decide, config["lifecycles"]["pull_request"]), "issues": asked(issues, issue.decide, config["lifecycles"]["issue"]), + "left_out": [{"item": reads_.item(i), "code": code} for i, code in zip(left_out, why)], "reconciliation": reconcile_.entries(found), "reconciled": reconcile_.carried(before, reads.get("effects"), reads.get("seen"), now, reads.get("unread") or ())} checked(record, "pass") @@ -343,7 +362,7 @@ def gap(later: str, earlier_: str) -> bool: due, names_due = credentials_.due(config.get("credentials"), now) causes = automatic_causes(runs, now, rules["automatic_pause"]) if automatic is not None else [] alerts = health_.alerts([e["decision"]["reason"] for e in entries], context, capacity, causes, - bool(left_out), raised, unnamed, reconcile_.alerts(found), due, + bool(alarmed), raised, unnamed, reconcile_.alerts(found), due, whole and never_started(runs, now, rules["automatic_pause"])) decided = [e["facts"]["item"] for e in entries] titles = {key(e["facts"]["item"]): e.get("title") if isinstance(e.get("title"), str) else "" diff --git a/controller/reads.py b/controller/reads.py index cae9cc9d..d27217c7 100644 --- a/controller/reads.py +++ b/controller/reads.py @@ -42,8 +42,10 @@ def _read(entry) -> bool: def items(entries, kind: str) -> tuple: - """`(kept, left_out)`: the entries whose facts are facts of `kind`, and the item of each that is not, as one list. - An item listed twice is left out every time it is listed, so that one pass never decides it twice.""" + """`(kept, left_out, why)`: the entries whose facts are facts of `kind`, the item of each that is not, as one list, + and why each is left out, as `pass/1` keeps it (#658): listed twice; else the facts port's word for a read that + failed, the entry's `unread`; else facts that are not facts. An item listed twice is left out every time it is + listed, so that one pass never decides it twice.""" if not isinstance(entries, list): # A list of items that could not be read is no evidence that there are none: a pass that wrote an empty feed # from it would look healthy. It is refused, and the not-run alert reports it. @@ -52,9 +54,26 @@ def items(entries, kind: str) -> tuple: # An item listed twice is counted wherever it can be named, readable or not, so a bad copy leaves the good one out. seen = [_named(e) for e in entries] kept = [e for e in readable if seen.count(key(e["facts"]["item"])) == 1] + out = [e for e in entries if not any(e is k for k in kept)] left_out = [e.get("facts", {}).get("item") if isinstance(e, dict) and isinstance(e.get("facts"), dict) else None - for e in entries if not any(e is k for k in kept)] - return kept, [i if isinstance(i, dict) else None for i in left_out] + for e in out] + why = [TWICE if _named(e) is not None and seen.count(_named(e)) > 1 + else e["unread"] if isinstance(e, dict) and e.get("unread") in UNREAD else NOT_FACTS for e in out] + return kept, [i if isinstance(i, dict) else None for i in left_out], why + + +# Why an item is left out of a pass (#658): a read that moved while it was read, which earns one pass's grace; one GitHub +# did not answer; facts that are not facts; an item listed twice. +MOVED, UNANSWERED, NOT_FACTS, TWICE = "pull_moved", "facts_unanswered", "not_facts", "listed_twice" +UNREAD = (MOVED, UNANSWERED) + + +def item(found): + """`found` if it is an item as `facts/1` spells one, which a pass record can keep; None otherwise.""" + return found if (isinstance(found, dict) and set(found) == {"repo", "type", "number"} + and isinstance(found["repo"], str) and found["repo"].count("/") == 1 + and "" not in found["repo"].split("/") and found["type"] in ("pull_request", "issue") + and type(found["number"]) is int and found["number"] >= 1) else None def _named(entry): diff --git a/docs/decisions/0172-a-pull-request-that-moved-while-a-pass-read-it-is-deferred-once-with-no-alert.md b/docs/decisions/0172-a-pull-request-that-moved-while-a-pass-read-it-is-deferred-once-with-no-alert.md new file mode 100644 index 00000000..77396819 --- /dev/null +++ b/docs/decisions/0172-a-pull-request-that-moved-while-a-pass-read-it-is-deferred-once-with-no-alert.md @@ -0,0 +1,25 @@ +# 0172. A pull request that moved while a pass read it is deferred once with no alert + +Status: accepted. The Factory Builder's ruling of 2026-11-08 on issue 658, "Controller: a pull request that changes while a pass reads it raises facts_unreadable and reconciliation_unreadable, though Moved means read it again". From 2026-11-08 05:38Z, Factory alerts were opened seven times and closed within minutes. Each time, one pull request had merged, taken a commit or taken a comment less than a minute before the pass read it. The snapshot raised `Moved`, which means "read it again at the next pass". The entry took it as any read GitHub did not answer, so it raised `facts_unreadable`, and the item's writes, never read, left reconciliation short. It adds a grace pass and amends nothing else: decision 0056's tracking issue still opens on a pass's first cause. + +## Decision + +| What | Decided | Because | +|---|---|---| +| Why an item is left out | `pass/1` keeps `left_out`: each item the pass left out, in the order read, with its code. `pull_moved` is a snapshot that moved while it was read. `facts_unanswered` is any other read the facts port could not make. `not_facts` is facts that do not hold to their schema, and `listed_twice` an item listed twice, whatever its read said. The item is null where none can be named. A record written before it holds none | The cause is seen in the record, not inferred: which read failed, on which item | +| The grace pass | A pull request whose code is `pull_moved`, and which the last pass record read did not leave out as `pull_moved`, is deferred with no alert: no facts, no Decision and no act, as now, but not `facts_unreadable`. Its writes are not read in this pass. They are owed to the next, as a pull request read short is, so this reconciliation stays whole. The next pass reads it again: one a run's end starts, or the next scheduled one, at most twenty minutes on | A merge, a push or a comment during a read is the Factory working, not a fault. A person assigned to an issue that closes itself minutes later learns nothing from it | +| What still alerts at once | A second move in a row. Any other code. An issue left out as moved: the grace is a pull request's. A read whose `updated_at` cannot be read is no move: `adapters/pull.py` raises it as a read GitHub did not answer, `facts_unanswered`. A last record that cannot be read, that is not there, or that holds no `left_out`: it cannot say the move is a first, so the move counts as a second | Something that never settles still reaches a person, and only `Moved` earns the grace. A pass fails closed | +| Derived, never kept | Whether a move is a first is read from the last pass record at each pass, in `controller/pass_.py`'s `moved_once`. The entry asks the same of it to leave the owed writes unread | State is derived from facts and run records at every call, never stored | +| Not here | `adapters/checks.py` still raises a plain `Unanswered` when the head moves while the checks are read. That read shows as `facts_unanswered` and alerts at once. Making it a `Moved` would also change how the approving act classes it | The record's codes will show whether it happens. It is a change of its own | + +## Why + +The tracking issue is how the Factory's faults reach the Maintainer. Seven alerts in a day for pull requests that were only changing would teach him to ignore it. Deferring a moved pull request for one pass keeps the Kernel's rule: an unreadable fact defers. Raising it on the second move keeps the alert for what does not settle. + +## Boundaries + +| Boundary | Check | +|---|---| +| A pull request that moved once is deferred with no alert, and its record keeps `pull_moved`. One that moved twice in a row is the alert. A last record that left it out for another reason, left another item out as moved, or left out one it could not name, makes the move a first. An issue left out as moved is the alert. One read clean after a move is decided. Any other code is the alert at once, and so is a move after a last record that cannot be read, is not there or holds no `left_out`. An item listed twice is `listed_twice`, and one that names no item is kept as null | `tests/test_controller.py::test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert`, `tests/test_controller.py::test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once`, `controller/pass_.py`, `controller/reads.py`, `schemas/pass.schema.json`, mutant: "a pull request that moved once is the alert", mutant: "an issue that moved earns the grace", mutant: "what the last record left out for any reason counts as a move", mutant: "a nameless move in the last record is read as an item", mutant: "a move in the last pass earns the grace again", mutant: "a last record that cannot say grants the grace", mutant: "any read left out earns the grace", mutant: "the record keeps an item that names none", mutant: "an item listed twice is left out by its read's code", mutant: "a read's code of any word is kept", mutant: "any dict is an item a record keeps" | +| The entry keeps a moved snapshot as `pull_moved` and any other as `facts_unanswered`. A first move's writes are owed and not read, so reconciliation is whole. A second move's, or one after a record that cannot say, are read short | `tests/test_controller.py::test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once`, `controller/__main__.py`, mutant: "the entry keeps no code of why a snapshot failed", mutant: "a pull request that moved once is read short", mutant: "every pull request left out is owed" | +| A read whose `updated_at` cannot be read, the first or the last, is a read GitHub did not answer, never a move | `tests/test_adapters.py::test_a_pull_request_whose_updated_at_cannot_be_read_has_not_moved`, `adapters/pull.py`, mutant: "a read with no time is a move" | diff --git a/examples/invalid/pass.a-left-out-code-in-words.json b/examples/invalid/pass.a-left-out-code-in-words.json new file mode 100644 index 00000000..6b45ed7a --- /dev/null +++ b/examples/invalid/pass.a-left-out-code-in-words.json @@ -0,0 +1,383 @@ +{ + "schema": "pass/1", + "pass": "p_01JA7P", + "release": "factory/v0.3.1", + "now": "2026-10-11T13:00:00Z", + "started_by": "own_repo", + "context": { + "paused": false, + "active_runs": 0, + "model_deferred_until": null, + "passes_since": "2026-10-11T12:55:00Z", + "unidentified_runs": [] + }, + "runs": [ + { + "run": "r_01JA6Z", + "item": { + "repo": "combsmith/software-factory", + "type": "pull_request", + "number": 42 + }, + "role": "pr-review", + "intent": "review-change", + "content_id": "sha256:5d0c", + "status": "completed", + "conclusion": "success", + "outcome": null, + "started_at": "2026-10-11T12:05:00Z" + } + ], + "reads": [ + { + "read": "runs", + "date": "2026-10-11T12:59:58Z", + "age": 0 + }, + { + "read": "facts", + "date": "2026-10-11T12:59:59Z", + "age": 0 + } + ], + "pull_requests": [ + { + "facts": { + "schema": "facts/1", + "item": { + "repo": "combsmith/software-factory", + "type": "pull_request", + "number": 42 + }, + "state": "open", + "is_draft": false, + "class": "maintainer", + "author": "don-petry", + "last_pusher": "don-petry", + "targets_protected": true, + "mergeable": true, + "content_id": "sha256:5d0c", + "diff": { + "files": 3, + "lines": 120, + "truncated": false, + "characters": 4800 + }, + "changed_paths": [ + { + "path": "kernel/derive.py", + "previous": "kernel/derive_old.py" + }, + { + "path": "tests/test_decide.py", + "previous": null + }, + { + "path": "docs/glossary.md", + "previous": null + } + ], + "label_events": [ + { + "label": "factory:hold", + "action": "labeled", + "by": { + "kind": "human", + "name": "don-petry", + "can_write": true + }, + "at": "2026-10-11T11:40:00Z" + }, + { + "label": "factory:hold", + "action": "unlabeled", + "by": { + "kind": "human", + "name": "don-petry", + "can_write": true + }, + "at": "2026-10-11T11:45:00Z" + } + ], + "last_push_at": "2026-10-11T12:00:00Z", + "last_reopened_at": null, + "description_edited_at": null, + "expired_reviews": [], + "last_writer_action_at": "2026-10-11T12:00:00Z", + "reviews": [ + { + "by": { + "kind": "machine_user", + "name": "donpetry-bot", + "can_write": true + }, + "state": "dismissed", + "commit": "4b1a9c0", + "content_id": "sha256:4b1a", + "submitted_at": "2026-10-11T11:50:00Z", + "dismissed_by": { + "kind": "human", + "name": "don-petry", + "can_write": true + }, + "dismissed_from": "approved" + }, + { + "by": { + "kind": "human", + "name": "a-colleague", + "can_write": true + }, + "state": "changes_requested", + "commit": "5d0c7e2", + "content_id": "sha256:5d0c", + "submitted_at": "2026-10-11T12:18:00Z", + "dismissed_by": null, + "dismissed_from": null + } + ], + "threads": [ + { + "id": "PRRT_kw7", + "resolved": false, + "opened_by": { + "kind": "lens", + "name": "pr-review", + "can_write": true + }, + "category": "functional", + "severity": "major", + "resolved_by": null, + "opened_at": "2026-10-11T12:15:00Z", + "ruling": null + }, + { + "id": "PRRT_kw1", + "resolved": false, + "opened_by": { + "kind": "lens", + "name": "coderabbit", + "can_write": false + }, + "category": null, + "severity": null, + "resolved_by": null, + "opened_at": "2026-10-11T12:15:00Z", + "ruling": null + }, + { + "id": "PRRT_kw3", + "resolved": true, + "opened_by": { + "kind": "human", + "name": "a-colleague", + "can_write": true + }, + "category": "human", + "severity": null, + "resolved_by": { + "kind": "human", + "name": "a-colleague", + "can_write": true + }, + "opened_at": "2026-10-11T12:16:00Z", + "ruling": null + } + ], + "checks": [ + { + "name": "verify", + "app_id": 15368, + "workflow": ".github/workflows/verify.yml", + "conclusion": "success", + "completed_at": "2026-10-11T12:06:00Z" + }, + { + "name": "Secret scan (gitleaks)", + "app_id": 15368, + "workflow": ".github/workflows/ci.yml", + "conclusion": "success", + "completed_at": "2026-10-11T12:06:00Z" + }, + { + "name": "CodeQL", + "app_id": 57789, + "workflow": null, + "conclusion": "success", + "completed_at": "2026-10-11T12:06:00Z" + } + ], + "queue": false, + "acts_since_writer": 2, + "acceptance": null, + "complete": { + "acceptance": true, + "reviews": true, + "threads": true, + "checks": true, + "diff": true, + "changed_paths": true, + "runs": true, + "timeline": true + }, + "read_at": "2026-10-11T12:20:00Z" + }, + "decision": { + "schema": "decision/1", + "item": { + "repo": "combsmith/software-factory", + "type": "pull_request", + "number": 42 + }, + "state": "addressing", + "reason": "human_finding", + "evidence": [ + "PRRT_kw7", + "a-colleague" + ], + "attention": "needs_you", + "outcome": "admit", + "admit": { + "role": "dev-lead", + "intent": "fix-findings", + "content_id": "sha256:5d0c" + } + }, + "run": "r_01JA7Q", + "head": "fc6f2b03e078f5d8de03dfaa142afbd00dec35ea" + } + ], + "issues": [ + { + "facts": { + "schema": "issue-facts/1", + "item": { + "repo": "combsmith/software-factory", + "type": "issue", + "number": 41 + }, + "state": "open", + "closed_first_at": null, + "author": { + "kind": "human", + "name": "don-petry", + "can_write": true + }, + "type": "Task", + "label_names": [ + "kernel", + "factory:triage" + ], + "assignees": [ + "don-petry" + ], + "life_started_at": "2026-10-11T09:00:00Z", + "reopened_last_by": "never", + "reopened_last_at": null, + "content_id": "sha256:41c1", + "content_changed_at": "2026-10-11T09:30:00Z", + "label_events": [ + { + "label": "factory:triage", + "action": "labeled", + "by": { + "kind": "app", + "name": "combsmith-sf-controller[bot]", + "can_write": true + }, + "at": "2026-10-11T09:31:00Z" + } + ], + "linked": [ + { + "number": 146, + "class": "agent", + "opener": { + "kind": "app", + "name": "combsmith-sf-dev-lead[bot]", + "can_write": true + }, + "open": true, + "draft": true, + "opened_at": "2026-10-11T09:40:00Z", + "merged_at": null, + "closed_at": null, + "empty": false, + "label_events": [], + "closed_issue_at": null, + "draft_events": [] + }, + { + "number": 140, + "class": "outside", + "opener": { + "kind": "human", + "name": "stranger", + "can_write": false + }, + "open": false, + "draft": false, + "opened_at": "2026-10-11T09:35:00Z", + "merged_at": null, + "closed_at": "2026-10-11T10:00:00Z", + "empty": false, + "label_events": [ + { + "label": "factory:admit", + "action": "labeled", + "by": { + "kind": "human", + "name": "don-petry", + "can_write": true + }, + "at": "2026-10-11T09:45:00Z" + } + ], + "closed_issue_at": null, + "draft_events": [] + } + ], + "role_comments": [ + { + "by": "combsmith-sf-controller[bot]", + "at": "2026-10-11T09:31:00Z" + } + ], + "last_writer_action_at": "2026-10-11T09:30:00Z", + "last_writer_comment_at": null, + "acts_since_writer": 1, + "branch_head": "9f2c41d07a3be58e1c6d0b4a7e25f3c18d90ab6e", + "complete": { + "timeline": true, + "linked": true, + "runs": true + }, + "read_at": "2026-10-11T12:00:00Z" + }, + "decision": { + "schema": "issue-decision/1", + "item": { + "repo": "combsmith/software-factory", + "type": "issue", + "number": 41 + }, + "state": "not_served", + "reason": "left_out", + "evidence": [], + "attention": "none", + "outcome": "nothing" + }, + "run": null + } + ], + "left_out": [ + { + "item": { + "repo": "combsmith/software-factory", + "type": "pull_request", + "number": 43 + }, + "code": "moved" + } + ] +} diff --git a/schemas/pass.schema.json b/schemas/pass.schema.json index 60dab9ca..2ddc018b 100644 --- a/schemas/pass.schema.json +++ b/schemas/pass.schema.json @@ -174,6 +174,29 @@ "pattern": "^[A-Z][A-Z0-9_]*$" } }, + "left_out": { + "description": "Each item the pass left out, in the order read, with why: a read that moved while it was read (`pull_moved`), one GitHub did not answer (`facts_unanswered`), facts that are not facts, or an item listed twice. The item is null where none can be named. The next pass reads it to tell a second move from a first (#658). Absent in a record written before it", + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["item", "code"], + "properties": { + "item": { + "description": "The item as `facts/1` spells one, or null", + "type": ["object", "null"], + "additionalProperties": false, + "required": ["repo", "type", "number"], + "properties": { + "repo": {"type": "string", "pattern": "^[^/]+/[^/]+$"}, + "type": {"enum": ["pull_request", "issue"]}, + "number": {"type": "integer", "minimum": 1} + } + }, + "code": {"enum": ["pull_moved", "facts_unanswered", "not_facts", "listed_twice"]} + } + } + }, "reconciliation": { "description": "What reconciliation found, as the view says it, each once: each write a Factory identity made that no Effect recorded, and each merge read as an Override, ids and codes alone (decision 0061). Absent in a record written before reconciliation", "type": "array", diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 9349abf4..483728ec 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -3381,6 +3381,23 @@ def test_a_snapshot_of_a_pull_request_that_changed_while_it_was_read_is_refused( with self.assertRaises(facts.Moved): self.read(6) + def test_a_pull_request_whose_updated_at_cannot_be_read_has_not_moved(self): # issue 658 + # Either read without a time is a read GitHub did not answer, not a move: it earns no pass's grace. + first = body("P-29.base", 1) + + class Answers: + def __init__(self, answer): + self.answer = answer + + def send(self, request): + return {"status": 200, "headers": {}, "body": copy.deepcopy(self.answer)} + for was, again in ((dict(first, updated_at=None), first), (first, dict(first, updated_at=None))): + with self.assertRaises(github.Unanswered) as raised: + pull.unchanged(Answers(again), was) + self.assertNotIsInstance(raised.exception, facts.Moved) + self.assertTrue(pull.unchanged(Answers(first), first)) + self.assertFalse(pull.unchanged(Answers(dict(first, updated_at="2026-10-11T13:00:00Z")), first)) + def test_a_push_that_names_its_pusher_and_no_time_is_refused(self): seq = self.sequence(1) entry = seq[2]["response"]["body"][0] diff --git a/tests/test_controller.py b/tests/test_controller.py index e0c5c89f..16f2bfe6 100644 --- a/tests/test_controller.py +++ b/tests/test_controller.py @@ -287,6 +287,56 @@ def test_an_item_whose_facts_are_not_facts_is_left_out_and_raises_one_alert(self self.assertEqual(len(twice["dispatch"]), 1) self.assertEqual(twice["left_out"], [dict(support.ITEM, number=42)] * 2 + [dict(test_issue.ITEM, number=7)] * 2) + def test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert(self): # issue 658 + # Moved while it was read, and not in the last pass: deferred, no facts and no act, with no alert, and the record + # keeps why. Moved again in the next pass, it is the alert; read clean after a move, it is decided. + item = dict(support.ITEM, number=43) + + def left(code="pull_moved"): + return {"facts": {"item": dict(item)}, "title": None, "unread": code} + clean = earlier("2026-10-11T12:50:00Z") + once = passed(reads(pulls=[pr(42), left()], runs=[run()]), previous=[clean]) + alone = passed(reads(pulls=[pr(42)], runs=[run()]), previous=[clean]) + self.assertEqual((decisions(once), once["dispatch"]), (decisions(alone), alone["dispatch"])) + self.assertEqual(once["alerts"], []) + self.assertEqual(once["record"]["left_out"], [{"item": item, "code": "pull_moved"}]) + self.assertEqual(shared.errors(once["record"], "pass"), []) + moved = dict(once["record"], now="2026-10-11T12:55:00Z") + twice = passed(reads(pulls=[pr(42), left()], runs=[run()]), previous=[clean, moved]) + self.assertEqual(twice["alerts"], ["facts_unreadable"]) + # What the last record left out for another reason, or another item it left out as moved, or one it could not + # name, makes this move a first (the Claude review). + for held in ([{"item": item, "code": "facts_unanswered"}], [{"item": dict(item, number=44), "code": "pull_moved"}], + [{"item": None, "code": "pull_moved"}]): + last = dict(clean, now="2026-10-11T12:55:00Z", left_out=held) + out = passed(reads(pulls=[pr(42), left()], runs=[run()]), previous=[clean, last]) + self.assertEqual(out["alerts"], [], held) + settled = passed(reads(pulls=[pr(42), pr(43)], runs=[run()]), previous=[clean, moved]) + self.assertEqual(([d["item"]["number"] for d in decisions(settled)], settled["alerts"]), ([42, 43], [])) + # An issue left out as moved earns no grace: the grace is a pull request's (Muse). + issue_moved = {"facts": {"item": dict(test_issue.ITEM, number=7)}, "title": None, "unread": "pull_moved"} + out = passed(reads(pulls=[pr(42)], issues=[issue_moved], runs=[run()]), previous=[clean]) + self.assertEqual(out["alerts"], ["facts_unreadable"]) + # Any other read left out is the alert at once: one GitHub did not answer, or facts that are not facts. + for entry in (left("facts_unanswered"), {"facts": {"item": dict(item)}, "title": None}, left("anything")): + out = passed(reads(pulls=[pr(42), entry], runs=[run()]), previous=[clean]) + self.assertEqual(out["alerts"], ["facts_unreadable"], entry) + self.assertEqual(out["record"]["left_out"], [{"item": item, "code": "not_facts"}]) + # A last record that cannot be read, none at all, or one written before it kept what it left out, cannot say the + # move is a first: it is the alert. + older = dict(clean) + del older["left_out"] + for previous in (None, (), [older], [dict(clean, now="2026-10-11T12:50:00")]): + out = passed(reads(pulls=[pr(42), left()], runs=[run()]), previous=previous) + self.assertEqual(out["alerts"], ["facts_unreadable"], previous) + # An item listed twice is so whatever its read says; an item that names none is kept as null. + twin = passed(reads(pulls=[left(), left()], runs=[run()]), previous=[clean]) + self.assertEqual((twin["alerts"], [e["code"] for e in twin["record"]["left_out"]]), + (["facts_unreadable"], ["listed_twice"] * 2)) + nameless = passed(reads(pulls=[{"facts": {"item": {"number": 0}}, "unread": "pull_moved"}]), previous=[clean]) + self.assertEqual((nameless["alerts"], nameless["record"]["left_out"]), + (["facts_unreadable"], [{"item": None, "code": "pull_moved"}])) + def test_the_pass_asks_the_kernel_through_its_entry_points_alone(self): # issue 193, ruling 8, amended for 270 # The entry, `__main__.py`, is the one place that reads the clock and the environment (Entry, below). for module in sorted(m for m in (ROOT / "controller").glob("*.py") if m.name != "__main__.py"): @@ -1202,6 +1252,55 @@ def whole(transport, item, kept, identities, apps, since): # And it cannot say what has left, so what it saw is short, though each read of it was whole. self.assertIn("reconciliation_unreadable", files["pass.json"]["alerts"]) + def test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once(self): # issue 658 + # A snapshot that moved while it was read is kept in the record as `pull_moved`, one GitHub did not answer as + # `facts_unanswered`. Moved for the first time, its writes are not read: they are owed to the next pass, and this + # reconciliation is whole. Moved again, or after a record that cannot say, it is read short, as any is. + spied = [] + + def snapshot(raised): + def read(transport, repo, number, identities, who, runs_whole, clock, *, kept, with_head, opener, recorded, + opened): + if number == 8: + raise raised + return support.facts(item=dict(support.ITEM, number=number)), "A title", "a" * 40 + return read + + def read(transport, item, kept, identities, apps, since): + spied.append(item["number"]) + return ([], [], kept is not None) + + def entry_with(raised, records): + del spied[:] + with unittest.mock.patch.object(entry.facts_, "snapshot", snapshot(raised)), \ + unittest.mock.patch.object(entry.passes, "records", lambda *a: records), \ + unittest.mock.patch.object(entry.seen_, "read", read), \ + unittest.mock.patch.object(entry.seen_, "pushes", lambda *a, **k: ([], True)), \ + unittest.mock.patch.object(entry.applied, "effects", + lambda *a: {"applied": [], "read": [], "at": {}, "whole": True}), \ + unittest.mock.patch.object(entry.passes, "written", + lambda *a: {"written": [], "read": [], "at": {}, "whole": True}): + code, _, files, _ = self.run_entry(Hub(self.table(numbers=[7, 8]))) + self.assertEqual(code, 0) + return files["pass.json"] + + before = json.loads((ROOT / "examples" / "pass.json").read_text()) + before.update(now="2026-10-12T11:58:00Z", left_out=[]) + item = {"repo": self.HOME, "type": "pull_request", "number": 8} + once = entry_with(entry.facts_.Moved("moved"), [before]) + self.assertEqual(once["left_out"], [{"item": item, "code": "pull_moved"}]) + self.assertEqual((7 in spied, 8 in spied, once["reconciled"]["owed"]), (True, False, [8])) + self.assertFalse({"facts_unreadable", "reconciliation_unreadable"} & set(once["alerts"])) + older = dict(before) + del older["left_out"] + for previous in ([dict(before, left_out=once["left_out"])], [older]): + again = entry_with(entry.facts_.Moved("moved"), previous) + self.assertEqual((8 in spied, again["reconciled"]["owed"]), (True, [8])) + self.assertLessEqual({"facts_unreadable", "reconciliation_unreadable"}, set(again["alerts"])) + unanswered = entry_with(github.Unanswered("no"), [before]) + self.assertEqual(unanswered["left_out"], [{"item": item, "code": "facts_unanswered"}]) + self.assertLessEqual({"facts_unreadable", "reconciliation_unreadable"}, set(unanswered["alerts"])) + def test_each_snapshot_is_read_with_the_configured_accounts_and_kept(self): # decisions 0005, 0060 handed = [] diff --git a/tools/mutants.py b/tools/mutants.py index 2964fd16..2e555c39 100644 --- a/tools/mutants.py +++ b/tools/mutants.py @@ -3518,8 +3518,8 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ' if pusher is not None and pushed_at is None:\n', ' if False:\n', ('tests.test_adapters',)), ('a change that kept the head is not noticed', 'adapters/pull.py', - ' and again.get("updated_at") == pull["updated_at"])\n', - ' )\n', ('tests.test_adapters',)), + ' return _same(again, pull) and again["updated_at"] == pull["updated_at"]\n', + ' return _same(again, pull)\n', ('tests.test_adapters',)), ('a workflow run that is no object is read as none', 'adapters/checks.py', ' if not (run is None or isinstance(run, dict)) or not (file is None or isinstance(file, dict)):\n', ' if False:\n', ('tests.test_adapters',)), @@ -7407,8 +7407,8 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ' d = decide(facts, runs, context, kernel, now)\n', ' d = decide(facts, [], context, kernel, now)\n', ('tests.test_controller.Record.test_the_pass_record_holds_what_the_kernel_was_given_and_replays_it',)), ("an issue is asked under the pull request's Lifecycle", 'controller/pass_.py', - 'config["lifecycles"]["issue"]),\n "reconciliation"', - 'config["lifecycles"]["pull_request"]),\n "reconciliation"', ('tests.test_controller.Record.test_an_issue_is_asked_of_the_issues_kernel_with_its_own_lifecycle',)), + 'config["lifecycles"]["issue"]),\n "left_out"', + 'config["lifecycles"]["pull_request"]),\n "left_out"', ('tests.test_controller.Record.test_an_issue_is_asked_of_the_issues_kernel_with_its_own_lifecycle',)), ("an issue's repo is not read for a Pause", 'controller/pass_.py', 'for e in pulls + issues}', 'for e in pulls}', ('tests.test_controller.Record.test_an_issue_is_asked_of_the_issues_kernel_with_its_own_lifecycle',)), @@ -10710,8 +10710,50 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ' if not whole:\n raise Stop("pull_requests_unreadable")\n', '', ('tests.test_controller.Entry.test_what_the_entry_cannot_read_writes_nothing',)), ('a snapshot that cannot be read stops the pass', 'controller/__main__.py', - ' except Unanswered:\n pulls.append(', - ' except ZeroDivisionError:\n pulls.append(', ('tests.test_controller.Entry.test_the_entry_reads_through_the_ports_and_hands_the_pass_nothing_else',)), + ' except Unanswered as err:\n', + ' except ZeroDivisionError as err:\n', ('tests.test_controller.Entry.test_the_entry_reads_through_the_ports_and_hands_the_pass_nothing_else',)), + # Issue 658: a pull request that moved while it was read is deferred once with no alert, by the code its record keeps. + ("a pull request that moved once is the alert", 'controller/pass_.py', + ' alarmed = [i for i in left_out if reads_.item(i) is None or key(i) not in graced]', + ' alarmed = left_out', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert', 'tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once')), + ("a move in the last pass earns the grace again", 'controller/pass_.py', + ' again = {key(e["item"]) for e in held if e["code"] == reads_.MOVED and e["item"] is not None}', + ' again = set()', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert', 'tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once')), + ("a last record that cannot say grants the grace", 'controller/pass_.py', + ' if not isinstance(held, list):\n return set()\n again', + ' if not isinstance(held, list):\n held = []\n again', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert', 'tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once')), + ("any read left out earns the grace", 'controller/pass_.py', + 'if code == reads_.MOVED and reads_.item(i) is not None\n', + 'if reads_.item(i) is not None\n', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert', 'tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once')), + ("an issue that moved earns the grace", 'controller/pass_.py', + ' and i["type"] == "pull_request" and key(i) not in again}', ' and key(i) not in again}', + ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("what the last record left out for any reason counts as a move", 'controller/pass_.py', + 'for e in held if e["code"] == reads_.MOVED and e["item"] is not None}', 'for e in held if e["item"] is not None}', + ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("a nameless move in the last record is read as an item", 'controller/pass_.py', + 'for e in held if e["code"] == reads_.MOVED and e["item"] is not None}', 'for e in held if e["code"] == reads_.MOVED}', + ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("a read with no time is a move", 'adapters/pull.py', + ' if not (isinstance(pull.get("updated_at"), str) and isinstance(again.get("updated_at"), str)):\n', + ' if False:\n', ('tests.test_adapters.Snapshot.test_a_pull_request_whose_updated_at_cannot_be_read_has_not_moved',)), + ("the record keeps an item that names none", 'controller/pass_.py', + '"left_out": [{"item": reads_.item(i), "code": code}', '"left_out": [{"item": i, "code": code}', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("an item listed twice is left out by its read's code", 'controller/reads.py', + ' why = [TWICE if _named(e) is not None and seen.count(_named(e)) > 1', ' why = [TWICE if False', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("a read's code of any word is kept", 'controller/reads.py', + 'else e["unread"] if isinstance(e, dict) and e.get("unread") in UNREAD else NOT_FACTS', + 'else e.get("unread", NOT_FACTS) if isinstance(e, dict) else NOT_FACTS', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("any dict is an item a record keeps", 'controller/reads.py', + ' return found if (isinstance(found, dict) and set(found) == {"repo", "type", "number"}', + ' return found if (isinstance(found, dict)', ('tests.test_controller.Record.test_a_pull_request_that_moved_while_read_is_deferred_once_with_no_alert',)), + ("the entry keeps no code of why a snapshot failed", 'controller/__main__.py', + '"unread": reads_.MOVED if isinstance(err, facts_.Moved) else reads_.UNANSWERED', + '"unread": reads_.UNANSWERED', ('tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once',)), + ("a pull request that moved once is read short", 'controller/__main__.py', + ' if number in owed:\n unread.append(number)\n continue\n', '', ('tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once',)), + ("every pull request left out is owed", 'controller/__main__.py', + 'for e in left if page(e["facts"]["item"]) in graced}', 'for e in left}', ('tests.test_controller.Entry.test_a_snapshot_that_moved_is_kept_by_its_code_and_its_writes_are_owed_once',)), ('a run id of any form is read', 'controller/__main__.py', ' if not (isinstance(run_id, str) and RUN_ID.fullmatch(run_id)):\n raise Stop("run_unreadable")\n', '', ('tests.test_controller.Entry.test_what_the_entry_cannot_read_writes_nothing',)),