Repository navigation
Check other people's knowledge PRs daily and tell them why (design v8) #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,12 @@ | |
| the full quality gate (L1, L2, consistency) against CURRENT main with the | ||
| PR's changes applied; a pass gets an ``auto`` verdict. | ||
|
|
||
| Anything else goes to people once per head (with what would make it pass). | ||
| A staged external change set then follows the ordinary merge flow (verdict | ||
| comment, PR-stage kb-gate, merge queue). A new head, or a verdict whose | ||
| context moved, is simply judged again at the next poll. | ||
| A PR that does not pass is never merged (design v8, D12): the author is told | ||
| why in ONE findings comment on the PR, updated in place, and the PR is checked | ||
| again once a day (not on every push) until it passes. A passing PR follows the | ||
| ordinary merge flow: a signed verdict in a ``merge`` item, the publisher's | ||
| local gate, the merge. A passing head is not judged again; a new head, or a | ||
| merge refused meanwhile, is judged at the next daily pass. | ||
|
|
||
| Judging "on current main" requires the PR's pre-images to equal main's: | ||
| otherwise the merge queue would apply different changes than were signed, and | ||
|
|
@@ -24,6 +26,7 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from typing import Any | ||
|
|
||
| from ..knowledge_service.gate_verifier import MAINTAINER_PATH, WHITELIST_CODES, code_owners, parse_codeowners | ||
|
|
@@ -37,8 +40,15 @@ | |
| HUMAN_LABEL = "kb:human-approved" | ||
| GOVERNED = ("knowledge/repos/", "knowledge/general/") | ||
| SUFFIXES = (".md", ".yaml") | ||
| # statuses after which a head is judged again (the verdict can no longer verify) | ||
| REJUDGE = ("head_changed", "stale_context", "superseded") | ||
| # statuses after which a head is judged again at the next daily pass: it did | ||
| # not pass, or its verdict can no longer verify | ||
| REJUDGE = ("head_changed", "stale_context", "superseded", "failed", "human", "gate_failed", "rebuild_failed") | ||
| EXTERNAL_EVERY = 24 * 3600 # other people's PRs are checked once a day | ||
| CHECKED_CURSOR = "external_checked:{number}" # per PR: a failed pass never re-judges finished ones | ||
| FINDINGS_MARKER = "<!-- kb-findings:v1 -->" | ||
| # + PR number, kept GLOBALLY (a comment belongs to the PR, whatever | ||
| # repository scope it touches now): the latest findings until delivered | ||
| FINDINGS_CURSOR = "findings:" | ||
|
|
||
|
|
||
| def _governed(path: str) -> bool: | ||
|
|
@@ -157,11 +167,117 @@ def _ours(rt, number: int) -> bool: | |
| "paused", "shadow_recorded", "calibration_required", "approval_withdrawn"))) | ||
|
|
||
|
|
||
| def poll_external(rt) -> list[str]: | ||
| """One pass over open knowledge PRs; needs the scheduler's lease.""" | ||
| def poll_external(rt, *, force: bool = False) -> list[str]: | ||
| """A pass over open knowledge PRs; needs the scheduler's lease. Each PR is | ||
| checked at most once per ``EXTERNAL_EVERY``, recorded per PR in the ledger | ||
| as soon as it was checked: a restart, or a pass that fails half-way, never | ||
| checks a PR twice in a day. ``force`` ignores the daily limit.""" | ||
| owner = getattr(rt, "lease_owner", None) | ||
| if rt.outbox is None or not owner: | ||
| return [] | ||
| return renew_findings(rt) + _poll(rt, owner, force=force) | ||
|
|
||
|
|
||
| def _due(rt, number: int, force: bool) -> bool: | ||
| last = float(rt.ledger.get_cursor("*", CHECKED_CURSOR.format(number=number)) or 0) | ||
| return force or rt.clock() - last >= EXTERNAL_EVERY | ||
|
|
||
|
|
||
| def _checked(rt, number: int) -> None: | ||
| rt.ledger.set_cursor("*", CHECKED_CURSOR.format(number=number), str(rt.clock())) | ||
|
|
||
|
|
||
| def findings_text(number: int, head: str, status: str, reasons: list[str], today: str) -> str: | ||
| """The one comment the service keeps on another person's knowledge PR.""" | ||
| lines = [FINDINGS_MARKER, f"**Knowledge quality gate** · checked {today} at `{head[:12]}` " | ||
| "(checked again once a day)", ""] | ||
| if status in ("pr_open", "merged"): | ||
| lines.append("Result: **passed**. The publisher merges it after its final check on the exact merge result.") | ||
| elif status == "shadow_recorded": | ||
| lines.append("Result: **passed** (this repository is in shadow mode: nothing is merged automatically yet).") | ||
| else: | ||
| lines.append("Result: **not passed**. This PR will not be merged until the problems below are fixed; " | ||
| "push a fix and it is checked again at the next daily pass.") | ||
| lines += ["", "Problems:"] + [f"- {r}" for r in (reasons or ["the change needs a person's decision"])[:20]] | ||
| return "\n".join(lines) + "\n" | ||
|
|
||
|
|
||
| def _findings_lifecycle(rt, paths: list[str]): | ||
| """Which repository may carry the findings of a PR that belongs to none | ||
| (it spans several): only when every scope it touches publishes (a private | ||
| upstream's name must never appear), through one in auto_merge.""" | ||
| scopes = set() | ||
| for path in paths: | ||
| try: | ||
| scopes.add(repo_scope(path[len("knowledge/"):]).split("/")[-1]) | ||
| except Exception: # noqa: BLE001 - outside repos/ and general/ | ||
| continue | ||
| lifecycles = [rt.registry.get(name) for name in sorted(scopes)] | ||
| if not lifecycles or any(lc is None or not lc.publishes for lc in lifecycles): | ||
| return None | ||
| return next((lc for lc in lifecycles if lc.auto_merge), None) | ||
|
|
||
|
|
||
| def post_findings(rt, lifecycle, number: int, head: str, status: str, reasons: list[str]) -> None: | ||
| """Hand the findings comment to the publisher (never in shadow mode, never | ||
| for a private upstream: those publish nothing).""" | ||
| if lifecycle is None or not lifecycle.auto_merge or not lifecycle.publishes: | ||
| return | ||
| comment = findings_text(number, head, status, reasons, rt.today()) | ||
| _issue_findings(rt, lifecycle.repo, number, head, comment) | ||
|
|
||
|
|
||
| def _issue_findings(rt, repo: str, number: int, head: str, comment: str, revision: float | None = None) -> None: | ||
| """Issue the findings and remember them (one record per PR) until the | ||
| publisher confirms delivery. ``revision`` is when the findings were | ||
| written: the publisher drops an item older than what it already posted, | ||
| and a renewal keeps the original revision, so it can never overtake newer | ||
| findings.""" | ||
| revision = rt.clock() if revision is None else revision | ||
| item = rt.outbox.issue(repo, "post_findings", { | ||
| "pr": number, "head_sha": head, "marker": FINDINGS_MARKER, "comment": comment, "revision": revision}) | ||
| rt.ledger.set_cursor("*", f"{FINDINGS_CURSOR}{number}", json.dumps({ | ||
| "repo": repo, "item_id": item.id, "expires_at": item.expires_at, "head_sha": head, "comment": comment, | ||
| "revision": revision, "delivered": False})) | ||
|
|
||
|
|
||
| def findings_delivered(rt, ack: dict) -> None: | ||
| """The publisher's ack for a findings item: only the latest one counts.""" | ||
| name = f"{FINDINGS_CURSOR}{int(ack.get('pr') or 0)}" | ||
| raw = rt.ledger.get_cursor("*", name) | ||
| if not raw or not ack.get("ok"): | ||
| return | ||
| record = json.loads(raw) | ||
| if record.get("item_id") == ack.get("item_id"): | ||
| rt.ledger.set_cursor("*", name, json.dumps({**record, "delivered": True})) | ||
|
|
||
|
|
||
| def renew_findings(rt) -> list[str]: | ||
| """Findings that expired undelivered (the publisher was down or GitHub | ||
| failed for a day) are issued again with the same text: an author is | ||
| never left without them, whatever the PR's status.""" | ||
| events = [] | ||
| for name, raw in rt.ledger.cursors_with_prefix("*", FINDINGS_CURSOR).items(): | ||
| record = json.loads(raw) | ||
| if record.get("delivered") or float(record.get("expires_at") or 0) > rt.clock(): | ||
| continue | ||
| lifecycle = rt.registry.get(record.get("repo")) | ||
| if lifecycle is None or not lifecycle.auto_merge or not lifecycle.publishes: | ||
| continue # its repository publishes nothing any more | ||
| number = int(name[len(FINDINGS_CURSOR):]) | ||
| _issue_findings(rt, lifecycle.repo, number, record["head_sha"], record["comment"], | ||
| revision=float(record.get("revision") or 0)) | ||
| events.append(f"findings reissued PR #{number}") | ||
| return events | ||
|
|
||
|
|
||
| def _decision_reasons(decision: dict) -> list[str]: | ||
| from .refine import reasons_from_decision | ||
|
|
||
| return reasons_from_decision(decision) | ||
|
|
||
|
|
||
| def _poll(rt, owner: str, *, force: bool = False) -> list[str]: | ||
| events: list[str] = [] | ||
| main_sha = rt.knowledge.fetch() | ||
| maintainers: set[str] | None = None | ||
|
|
@@ -172,6 +288,8 @@ def poll_external(rt) -> list[str]: | |
| human = HUMAN_LABEL in labels | ||
| if pr.get("draft") or not head or _ours(rt, number) or _judged(rt, number, head, "human" if human else "auto"): | ||
| continue | ||
| if not _due(rt, number, force): | ||
| continue # checked today already: once a day, not on every push | ||
|
Comment on lines
+291
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new daily guard is ineffective for a PR carrying Useful? React with 👍 / 👎. |
||
| if rt.knowledge.fetch_pull(number) != head: | ||
| continue # pushed while we looked: next poll | ||
| merge_base = rt.knowledge.merge_base(main_sha, head) | ||
|
|
@@ -182,7 +300,9 @@ def poll_external(rt) -> list[str]: | |
| path = "human" if human else "auto" | ||
| lifecycle, why = _scope([p for p in paths if p.startswith("knowledge/")], rt.registry) | ||
| if lifecycle is None: | ||
| events.append(_to_people(rt, None, number, head, f"PR #{number} {why}", path)) | ||
| events.append(_to_people(rt, None, number, head, f"PR #{number} {why}", path, | ||
| findings_via=_findings_lifecycle(rt, paths))) | ||
| _checked(rt, number) | ||
| continue | ||
| if human: | ||
| maintainers = maintainers if maintainers is not None else _maintainers(rt, main_sha) | ||
|
|
@@ -193,6 +313,7 @@ def poll_external(rt) -> list[str]: | |
| events.append(_to_people(rt, lifecycle, number, head, | ||
| f"PR #{number} changes paths outside the governed knowledge pages; " | ||
| f"a knowledge maintainer can approve it and add {HUMAN_LABEL}", path)) | ||
| _checked(rt, number) | ||
| continue | ||
| base_main = rt.knowledge.knowledge_files(main_sha) | ||
| pre = rt.knowledge.knowledge_files(merge_base) | ||
|
|
@@ -203,6 +324,7 @@ def poll_external(rt) -> list[str]: | |
| if moved: | ||
| events.append(_to_people(rt, lifecycle, number, head, | ||
| f"PR #{number} must be rebased: main changed {moved[:3]} since it branched", path)) | ||
| _checked(rt, number) | ||
| continue | ||
| head_files = dict(base_main) | ||
| for rel in rels: | ||
|
|
@@ -223,6 +345,7 @@ def poll_external(rt) -> list[str]: | |
| if issues: | ||
| events.append(_to_people(rt, lifecycle, number, head, f"PR #{number} fails L1: " + "; ".join( | ||
| f"{i.code} {i.path} {i.detail}" for i in issues[:3]), path)) | ||
| _checked(rt, number) | ||
| continue | ||
| detail.update(source="human-approved", review_ids=review_ids, reviewers=reviewers, | ||
| generator="human", judge="human", decision={"blocks": [], "consistency": []}, | ||
|
|
@@ -251,14 +374,17 @@ def poll_external(rt) -> list[str]: | |
| if r in head_files}, | ||
| "deleted": [r for r in rels if r not in head_files], "evidence": []}) | ||
| rt.ledger.stage_intake(owner, lifecycle.repo, changeset_id, kind=KIND, status=status, verdicts=[], | ||
| human_reason="" if status in ("pr_open", "shadow_recorded") | ||
| else f"external PR #{number}: " + "; ".join(detail["decision"].get("reasons", [])), | ||
| # the author fixes it (told in the findings comment): not people's queue | ||
| human_reason="", | ||
| drafted_events=[], detail=detail) | ||
| rt.ledger.update_changeset(changeset_id, pr_number=number, head_sha=head) | ||
| rt.trace("decision", context={"repo": lifecycle.repo, "changeset_id": changeset_id, "pr": number, | ||
| "playbook": "kb-external", "step": "gate"}, | ||
| result={"status": status, "source": detail["source"], | ||
| "reviewers": detail.get("reviewers", [])}) | ||
| post_findings(rt, lifecycle, number, head, status, | ||
| [] if status in ("pr_open", "shadow_recorded") else _decision_reasons(detail["decision"])) | ||
| _checked(rt, number) | ||
| events.append(f"external {changeset_id} PR #{number} {detail['source']} {status}") | ||
| return events | ||
|
|
||
|
|
@@ -274,14 +400,15 @@ def _lifecycle_bookkeeping(result, head_files: dict[str, str]) -> dict: | |
| "purges": list(result.purged)} | ||
|
|
||
|
|
||
| def _to_people(rt, lifecycle, number: int, head: str, reason: str, path: str) -> str: | ||
| """Record a head that people must handle, once (as a human change set).""" | ||
| def _to_people(rt, lifecycle, number: int, head: str, reason: str, path: str, findings_via=None) -> str: | ||
| """Record a head the gate cannot take as is; the author is told why.""" | ||
| repo = lifecycle.repo if lifecycle is not None else next(iter(rt.registry)) | ||
| changeset_id = rt.ledger.new_changeset_id(repo, KIND) | ||
| rt.save_changeset_files(changeset_id, {"base_sha": "", "files": {}, "deleted": [], "evidence": []}) | ||
| rt.ledger.stage_intake(rt.lease_owner, repo, changeset_id, kind=KIND, status="human", verdicts=[], | ||
| human_reason=reason, drafted_events=[], | ||
| human_reason="", drafted_events=[], | ||
| detail={"external_pr": number, "reason": reason, "path": path}) | ||
| rt.ledger.update_changeset(changeset_id, pr_number=number, head_sha=head) | ||
| post_findings(rt, lifecycle or findings_via, number, head, "human", [reason]) | ||
| return f"external {changeset_id} PR #{number} to people" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the wall clock as the ordering token does not ensure that later findings have a larger revision. If the clock moves backward after a VM restore/manual adjustment, the publisher treats the new findings as older at
_do_post_findingsand leaves the previous comment in place; equal timestamps can likewise let processing order decide which text wins. This can leave a passing comment visible after a later gate failure until the clock surpasses the old value, so revisions should come from a durable per-PR monotonic sequence rather thanrt.clock().Useful? React with 👍 / 👎.