diff --git a/SOURCES.md b/SOURCES.md index 59c0088..626694c 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -517,8 +517,10 @@ History-changing operations use the shared PR preservation reference to back up retained local files affected by tracking deletions and verify restoration. Scoped PR layers follow `pr`’s publish step, which links every layer in T3; CLI -publication does not substitute for that link. Layers get `pr`’s review step before +publication does not substitute for that link. Layers get `pr`’s Prepare and Review steps before `submit`, or `link` when it creates PRs or marks them ready, and `gh stack merge` runs only from `merge`’s land step. +Each layer runs its held `review-fix-loop` status command unchanged once its SHA is pushed; +after a restack, the loop reruns for that layer as a repair pass. Ordinary publication uses `submit --auto --open` or `link --open`, with drafts only on request, to match `pr`. Branch deletion after a merge goes through `repo-cleanup` @@ -1068,10 +1070,12 @@ other nonignored untracked source. - [mattpocock/skills / skills/engineering/pr](https://github.com/mattpocock/skills/tree/d81f3a183412e71a5b1e84ca21bc1a35eea03a60/skills/engineering/pr) — PR-body structure, visual selection, evidence, and merge-risk guidance adapted into `references/pr-body.md`; import baseline `d81f3a183412e71a5b1e84ca21bc1a35eea03a60`. Upstream credits [HumanLayer / show-me](https://github.com/humanlayer/skills/tree/ca7c8088db69e315a8b2deea43820270457f8f3c/plugins/show-me/skills/show-me) for visual guidance; the origin revision used by Matt is unrecorded. HumanLayer MIT notice checked at `ca7c8088db69e315a8b2deea43820270457f8f3c` in `licenses/humanlayer-skills.txt`. The local name and UI are `pr`, with automatic discovery. It owns PR text, publication, and -watching. Fixing feedback moved to `review-sweep` and merging to `merge`. Publishing reuses -matching review evidence, and runs `show-dont-tell` then the review loop when there is none. +watching. Fixing feedback moved to `review-sweep` and merging to `merge`. Publishing runs `show-dont-tell`, commits, then runs the review +loop on that commit and pushes after it; earlier review counts only for the same commit. `scripts/pr_state.py` reads one head's checks, threads, comments, and reviews through every page, replacing hand-read readiness with `blockers`, `pending`, and `to_settle`. +It also gates on the head's `review-fix-loop` commit status context, kept apart from CI +checks, including a same-named check run: anything but success blocks. The body guide uses Summary/Evidence/Merge Danger by default, defers to project PR templates, and preserves local why/effect/root-cause/stack facts. It expands visual selection to algorithm, runtime, UI, file, interaction, diff, and complete diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 123af24..8213af1 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -35,7 +35,11 @@ their callers and use separate stacks for independent work. [PR's publish step](../pr/SKILL.md#publish). A CLI operation doesn't link its layers to the thread. - Before `gh stack submit`, or `gh stack link` when it creates PRs or marks them ready, run - [PR's review step](../pr/SKILL.md#publish) on each layer against its immediate base. + PR's [Prepare and Review steps](../pr/SKILL.md#publish) on each layer against its immediate + base. + Run each layer's held `review-fix-loop` status command unchanged once its SHA is pushed, as + [Review Fix Loop's finish step](../review-fix-loop/SKILL.md#6-finish) says. After a restack, + rerun the loop for that layer as a [repair pass](../review-fix-loop/SKILL.md#4-route-repairs). Land through [Merge](../merge/SKILL.md): run `gh stack merge` only from its land step. diff --git a/skills/implement/SKILL.md b/skills/implement/SKILL.md index c43abc6..5459e31 100644 --- a/skills/implement/SKILL.md +++ b/skills/implement/SKILL.md @@ -168,9 +168,9 @@ reported as a gap, and the user can open what you showed. keep: go to Merge, which runs review, opens the PR, and lands it. Changes go back to stage 3. Scrap goes to [Repo Cleanup](../repo-cleanup/SKILL.md). - **PR:** PR runs review, publishes, and watches to the requested state. -- **Land:** go to Merge; a stack lands bottom-up as one merge set. Until hard - gates exist, Merge's requirements are the gate: review of the current head, - required checks, and passing proof. Land is done when GitHub reports every PR +- **Land:** go to Merge; a stack lands bottom-up as one merge set. Merge's + requirements are the gate: review of the current head, which `pr_state` + enforces, required checks, and passing proof. Land is done when GitHub reports every PR merged and the result is shown. A closed PR did not land. Provisional work never merges: Land becomes Try, and a requested draft or diff --git a/skills/merge/SKILL.md b/skills/merge/SKILL.md index eeb8d63..a2acdce 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -32,6 +32,12 @@ limits. PR open and use [Show Me](../show-me/SKILL.md) to show the user what happened, including anything that landed first. + `pr_state` blocks a head without a passing `review-fix-loop` status, including after a base + update or restack. A caller with its own repair owner gets the gap back. Without one, take + the head through PR's [Prepare and Review steps](../pr/SKILL.md#publish), publish any fixes, + and read the state again. A new head of reviewed work is a + [repair pass](../review-fix-loop/SKILL.md#4-route-repairs). + A caller that keeps its own repair owner gets each failure, comment, and stop back, and resumes that owner. You repair nothing. @@ -42,12 +48,10 @@ limits. branch, or for a stack layer its parent in the scoped set. A different base is a stop. Every PR, including docs-only changes, needs review of its current head that includes - [Show, Don't Tell](../show-dont-tell/SKILL.md), plus required verification. Reuse matching - evidence. A scoped rerun after a repair counts. So does review from before the base moved, - once you've read the new base commits and they don't affect the files or behavior this change - depends on. If evidence is missing or stale, a caller with its own repair owner gets the gap - back. Otherwise run [PR's review step](../pr/SKILL.md#publish), publish any fixes, and go back - to step 2. + [Show, Don't Tell](../show-dont-tell/SKILL.md), plus required verification. The head's + passing `review-fix-loop` status records the review and its Show, Don't Tell. Step 2 restores + a missing one. Route missing or stale verification the same way as a stale review, then go + back to step 2. A head you didn't verify holds commits nobody reviewed. Read them, then go back to step 2. It counts as verified once the evidence above covers it. diff --git a/skills/pr/SKILL.md b/skills/pr/SKILL.md index 1497836..233ed3a 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -24,17 +24,20 @@ remote description. Writing text commits and pushes nothing. ## Publish -Watching an existing PR starts at step 3. - -1. **Get it reviewed.** Reuse review evidence that matches the complete diff against the - immediate base and included [Show, Don't Tell](../show-dont-tell/SKILL.md). Otherwise run - Show, Don't Tell on that diff, including docs-only changes, then - [Review Fix Loop](../review-fix-loop/SKILL.md) on the result. Continue once review and - verification pass. -2. **Publish it.** Follow [publish](references/publish.md). Publish ready for review unless - the user asks for a draft. In T3, link each PR you create, push to, or take over, including - every stack layer. -3. **Watch it.** Follow [watch](references/watch.md). +Watching an existing PR starts at step 4. + +1. **Prepare it.** Run [Show, Don't Tell](../show-dont-tell/SKILL.md) on the complete diff + against the immediate base, including docs-only changes, unless earlier evidence covers + this change. Then [commit](references/publish.md#commit). +2. **Get it reviewed.** Run [Review Fix Loop](../review-fix-loop/SKILL.md) on the committed + head. A new head of reviewed work, such as after a base update or restack, is a + [repair pass](../review-fix-loop/SKILL.md#4-route-repairs). Earlier review of this exact + commit [finishes it](../review-fix-loop/SKILL.md#6-finish) and posts the status. Continue + once review and verification pass. +3. **Publish it.** Follow [publish](references/publish.md#push). Publish ready for review + unless the user asks for a draft. In T3, link each PR you create, push to, or take over, + including every stack layer. +4. **Watch it.** Follow [watch](references/watch.md). ## Report diff --git a/skills/pr/references/publish.md b/skills/pr/references/publish.md index ed04a06..c40b118 100644 --- a/skills/pr/references/publish.md +++ b/skills/pr/references/publish.md @@ -1,17 +1,23 @@ # Publish -## Commit and push +## Commit -Inspect status, the complete diff, and staged files. Stage only owned paths and -preserve unrelated work. Keep the feature branch; on the default branch, create +Commit before review, so the review covers a committed head. Inspect status, the +complete diff, and staged files. Stage only owned paths and preserve unrelated +work. Keep the feature branch; on the default branch, create `agent/` first. Use the caller's immediate base, including a parent layer for stacks. -Commit owned edits with a terse subject, run checks needed by final staging, -and push with upstream tracking. Push existing ahead commits without an empty -commit; reuse an already published head when nothing changed. For a caller-owned -restack, use force-with-lease only on the named branch after review. Return a -lease failure to the caller as changed remote state. +Commit owned edits with a terse subject, and run checks needed by final staging. + +## Push + +Push after review, with upstream tracking. Push existing ahead commits without an +empty commit; reuse an already published head when nothing changed. Then run any +held `review-fix-loop` status command as +[Review Fix Loop's finish step](../../review-fix-loop/SKILL.md#6-finish) says. For a +caller-owned restack, use force-with-lease only on the named branch after review. +Return a lease failure to the caller as changed remote state. Before rebasing or restacking, follow [Preserve local files](preserve-local-files.md) for retained files affected by tracking deletions. Verify restoration after the diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index c44ec3a..886714a 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -33,6 +33,10 @@ when a result looks wrong. fixes something, check that review evidence covers the change. If it's missing or stale, rerun [review](../SKILL.md#publish) on the affected scope. Then [publish](publish.md), post held replies, and read the state again. +- **No passing review status:** take the head through PR's + [Prepare and Review steps](../SKILL.md#publish), unless posting its status already failed; + then report that blocker. A new head of reviewed work is a + [repair pass](../../review-fix-loop/SKILL.md#4-route-repairs). - **A missing check:** explain it, including workflows filtered out by a stacked PR's base or by draft status. - **Behind or conflicted:** read what the new base commits touch, then decide with @@ -64,6 +68,9 @@ Ready means `pr_state` reports `ready`, every `to_settle` item has a decision, a [feedback handling](../../review-sweep/references/pr-feedback.md#finish) is complete. Report any unmet repository requirement. +`pr_state` blocks until the head has a passing `review-fix-loop` commit status. It reports that +status under `review_status`, apart from `checks`. + GitHub stops nothing on a repo with no required checks, so `pr_state` then holds every check to that bar: a failure blocks, and a head with workflows but no checks waits. One of these still counts as ready once Review Sweep dismissed it for this head with evidence, such as the same diff --git a/skills/pr/scripts/pr_state.py b/skills/pr/scripts/pr_state.py index ee32216..0b9f8f5 100644 --- a/skills/pr/scripts/pr_state.py +++ b/skills/pr/scripts/pr_state.py @@ -15,6 +15,8 @@ PROVISIONAL_HEADING = "## Provisional" PROVISIONAL_BLOCKER = (f"the body has an open '{PROVISIONAL_HEADING}' section; " "it must not merge until that section is removed") +REVIEW_CONTEXT = "review-fix-loop" +REVIEW_BLOCKER = f"no passing '{REVIEW_CONTEXT}' status on this head; run Review Fix Loop on it" # baseRefOid stops at the PR's last sync with its base, so read the branch's live tip instead. SUMMARY_QUERY = """ @@ -210,6 +212,10 @@ def check_result(node): return {"name": name, "state": state, "required": node["isRequired"], "url": url} +def is_review_record(node): + return node["__typename"] == "StatusContext" and node["context"] == REVIEW_CONTEXT + + def has_workflows(gh, pr, head): pages = graphql_pages(gh, WORKFLOWS_QUERY, owner=pr["owner"], name=pr["name"], expression=f"{head}:.github/workflows") @@ -227,7 +233,9 @@ def read_state(gh, pr): """Read until the PR holds still, so every part describes one state of it.""" for _ in range(READ_ATTEMPTS): summary = read_summary(gh, pr) - checks = [check_result(node) for node in all_nodes(gh, CHECKS_QUERY, pr, checks_page)] + nodes = all_nodes(gh, CHECKS_QUERY, pr, checks_page) + records = [check_result(node) for node in nodes if is_review_record(node)] + checks = [check_result(node) for node in nodes if not is_review_record(node)] state = { "summary": summary, "behind_by": commits_behind_base(gh, pr, summary), @@ -236,6 +244,7 @@ def read_state(gh, pr): "comments": all_nodes(gh, COMMENTS_QUERY, pr), "reviews": all_nodes(gh, REVIEWS_QUERY, pr), "checks": checks, + "review_status": records[0] if records else None, "expects_checks": bool(checks) or has_workflows(gh, pr, summary["headRefOid"]), } if read_summary(gh, pr) == summary: @@ -274,7 +283,9 @@ def assess_merge_state(state, blockers, pending): blockers.append("the branch is behind its base") elif merge_state == "BLOCKED" and not (pending or blockers): blockers.append("GitHub reports an unmet merge requirement") - elif merge_state == "UNSTABLE" and all(check["state"] == "pass" for check in state["checks"]): + elif merge_state == "UNSTABLE" and all( + check["state"] == "pass" for check in [*state["checks"], state["review_status"]] + if check): blockers.append("GitHub reports a failing status that no check here shows; " "read the head and test-merge targets") @@ -293,6 +304,8 @@ def assess(state, expected_head=None): if state.get("provisional"): blockers.append(PROVISIONAL_BLOCKER) + if (state["review_status"] or {}).get("state") != "pass": + blockers.append(REVIEW_BLOCKER) if summary["isDraft"]: blockers.append("the PR is a draft") if summary["reviewDecision"] == "CHANGES_REQUESTED": diff --git a/skills/pr/scripts/test_pr_state.py b/skills/pr/scripts/test_pr_state.py index 3563ff0..f7cc6bf 100644 --- a/skills/pr/scripts/test_pr_state.py +++ b/skills/pr/scripts/test_pr_state.py @@ -30,9 +30,13 @@ def check_run(name="ci", status="COMPLETED", conclusion="SUCCESS", required=True "detailsUrl": f"https://ci/{name}", "isRequired": required} -def status_context(state="SUCCESS", required=True): - return {"__typename": "StatusContext", "context": "legacy", "state": state, - "targetUrl": "https://ci/legacy", "isRequired": required} +def status_context(state="SUCCESS", required=True, context="legacy"): + return {"__typename": "StatusContext", "context": context, "state": state, + "targetUrl": f"https://ci/{context}", "isRequired": required} + + +def review_status(state="SUCCESS", required=False): + return status_context(state, required, context="review-fix-loop") def thread_comment(number=1): @@ -59,10 +63,11 @@ class FakeGitHub: """Answer gh calls by query, returning every page only when asked to paginate.""" def __init__(self, summaries=None, threads=(), comments=(), reviews=(), checks=(check_run(),), - page_size=100, rollup=True, behind_by=0, thread_comments=(), workflows=False): + review=review_status(), page_size=100, behind_by=0, thread_comments=(), + workflows=False): self.summaries = list(summaries or [summary()]) self.threads, self.comments, self.reviews = list(threads), list(comments), list(reviews) - self.checks, self.page_size, self.rollup = list(checks), page_size, rollup + self.checks, self.page_size = [*checks, *([review] if review else [])], page_size self.behind_by, self.thread_comments = behind_by, list(thread_comments) self.workflows, self.compared = workflows, [] @@ -98,7 +103,7 @@ def answer(self, query): return self.wrap({"page": page} for page in self.pages(self.comments)) if "reviews(" in query: return self.wrap({"page": page} for page in self.pages(self.reviews)) - rollups = [{"page": page} for page in self.pages(self.checks)] if self.rollup else [None] + rollups = [{"page": page} for page in self.pages(self.checks)] if self.checks else [None] return self.wrap({"commits": {"nodes": [{"commit": {"statusCheckRollup": rollup}}]}} for rollup in rollups) @@ -199,7 +204,7 @@ def test_any_running_check_waits(self): self.assertEqual(run(FakeGitHub(checks=checks))[0], 3) def test_head_with_workflows_waits_for_its_checks(self): - code, result = run(FakeGitHub(rollup=False, workflows=True)) + code, result = run(FakeGitHub(checks=(), workflows=True)) self.assertEqual((code, result["status"]), (3, "pending")) self.assertIn("no checks reported yet", result["pending"][0]) @@ -213,7 +218,7 @@ def test_requested_reviewers_are_waited_for(self): "review requested from a hidden reviewer"])) def test_no_checks_on_the_head_is_flagged(self): - code, result = run(FakeGitHub(rollup=False)) + code, result = run(FakeGitHub(checks=())) self.assertEqual(code, 0) self.assertEqual(result["to_settle"], ["no checks reported on this head"]) @@ -268,6 +273,74 @@ def test_verified_head_must_match(self): self.assertIn("the head moved", result["blockers"][0]) +class ReviewGate(unittest.TestCase): + def test_head_without_the_review_status_blocks(self): + """The rollup covers only the last commit, so a status left on an older one is absent.""" + for checks in ([check_run()], []): + with self.subTest(checks=checks): + code, result = run(FakeGitHub(checks=checks, review=None)) + self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) + + def test_review_blocker_sends_every_head_to_the_loop(self): + self.assertEqual(pr_state.REVIEW_BLOCKER, "no passing 'review-fix-loop' status on this " + "head; run Review Fix Loop on it") + + def test_passing_review_status_lets_the_head_land(self): + code, result = run(FakeGitHub()) + self.assertEqual((code, result["status"], result["review_status"]["state"]), + (0, "ready", "pass")) + + def test_ci_rules_are_unchanged_beside_a_passing_review_status(self): + cases = (([check_run(conclusion="FAILURE")], "required check failed: ci https://ci/ci"), + ([check_run(conclusion="FAILURE", required=False)], + "check failed, and GitHub requires none here: ci https://ci/ci")) + for checks, blocker in cases: + with self.subTest(blocker=blocker): + code, result = run(FakeGitHub(checks=checks)) + self.assertEqual((code, result["blockers"]), (2, [blocker])) + + def test_closed_and_merged_prs_need_no_review_status(self): + merged = summary(state="MERGED", mergeCommit={"oid": "c" * 40}) + self.assertEqual(run(FakeGitHub([merged], review=None))[1]["status"], "merged") + code, result = run(FakeGitHub([summary(state="CLOSED")], review=None)) + self.assertEqual((code, result["blockers"]), (2, ["the PR is closed"])) + + def test_review_status_other_than_success_blocks(self): + for state in ("PENDING", "EXPECTED", "FAILURE", "ERROR"): + for merge_state in ("CLEAN", "UNSTABLE"): + with self.subTest(state=state, merge_state=merge_state): + code, result = run(FakeGitHub([summary(mergeStateStatus=merge_state)], + review=review_status(state))) + self.assertEqual((code, result["blockers"], result["pending"]), + (2, [pr_state.REVIEW_BLOCKER], [])) + + def test_same_named_job_is_a_ci_check_not_the_record(self): + code, result = run(FakeGitHub(checks=[check_run("review-fix-loop")], review=None)) + self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) + self.assertIsNone(result["review_status"]) + self.assertEqual([check["name"] for check in result["checks"]], ["review-fix-loop"]) + + def test_only_the_status_counts_in_any_order_across_pages(self): + job, status = check_run("review-fix-loop"), review_status("FAILURE") + for nodes in ([job, status], [status, job]): + with self.subTest(first=nodes[0]["__typename"]): + code, result = run(FakeGitHub(checks=nodes, review=None, page_size=1)) + self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) + self.assertEqual(result["review_status"]["state"], "fail") + self.assertEqual([check["state"] for check in result["checks"]], ["pass"]) + + def test_failed_same_named_job_blocks_beside_a_passing_status(self): + failed_job = check_run("review-fix-loop", conclusion="FAILURE") + code, result = run(FakeGitHub(checks=[failed_job])) + self.assertEqual((code, result["blockers"]), + (2, ["required check failed: review-fix-loop https://ci/review-fix-loop"])) + + def test_required_review_status_is_not_a_ci_check(self): + _, result = run(FakeGitHub(review=review_status("ERROR", required=True))) + self.assertEqual(result["blockers"], [pr_state.REVIEW_BLOCKER]) + self.assertEqual([check["name"] for check in result["checks"]], ["ci"]) + + PROVISIONAL_BODY = ("## Summary\nAdds the list.\n\n## Provisional\n" "Rests on secret-plan #12; clears when it merges.\n") diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 81400de..04b0535 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -21,6 +21,12 @@ requirements, and verification evidence with the [review brief](../review/references/review-brief.md). Establish its workflow/pass identity before dispatch and retain it with the findings through every pass. +For PR work, every pass starts with nothing in the PR's scope uncommitted, and its snapshot is +HEAD. Unrelated local work stays as it is and out of the snapshot. The caller commits any +in-scope edits, including Show, Don't Tell's and repairs, before the pass that assesses them. +Before each pass, run `git status --porcelain`, which also lists untracked files; anything it +prints must be unrelated work the brief excludes, and the brief records it. + Select initial discovery from the first matching row, honoring explicit user requests: @@ -120,6 +126,10 @@ no routed fix stay out and keep their valid coverage. | An ownership boundary, guarantee, structure, or types | Thermo, plus Adversarial when it can change failure behavior | | Failure-sensitive behavior, or a plausible material consequence still uncertain | Adversarial on the affected contracts | +A new head from a base update or restack of reviewed work is a repair pass too, routed by what +changed: usually Primary, plus Adversarial when it's failure-sensitive. It is not a new initial +discovery. + Scope the pass to all changes since the last completed pass and their affected behavior, consumers, and dependencies, with the review brief's later-round inputs, including unresolved findings. Each reviewer checks the repair and its spillover @@ -164,6 +174,25 @@ accepted findings and checks are settled on it, and the final code/substantive-instruction pass needs no fixes. Missing required coverage and accepted findings block completion. +When the final snapshot is a commit headed for a PR, record completion on it, because +[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. +Record it only once Show, Don't Tell covers the change: PR's +[Prepare step](../pr/SKILL.md#publish) ran on it, or earlier evidence covers it. A +local-diff review with no PR planned finishes with its snapshot evidence; if it later becomes a +PR, the loop finishes on that commit. Retained coverage finishes only on the SHA it assessed, +and any other SHA gets a pass. Build this command with the final pass's full SHA and a +description of at most 140 characters: + +```sh +gh api repos///statuses/ -f state=success \ + -f context=review-fix-loop -f description=": passes" +``` + +Run it once that SHA is on the remote; GitHub rejects a SHA it doesn't have, and a PR isn't +needed. Until then, hand it off unchanged, and whoever pushes that SHA runs it. When posting +fails, such as for a missing +permission or a fork, report it to the user as a blocker; another run won't fix it. + In the Orchestration handoff, map child tasks to workflow/pass, review assignments, criteria, and configured models, and state which reviewer each follow-up task follows and the prior evidence it received. Include the reviewed base/snapshot,