From 4f10d54f78b95c39763ce9e8ab8151b6a896ac76 Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 22:22:19 -0500 Subject: [PATCH 1/7] Gate merges on a review-fix-loop commit status Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 4 ++ skills/merge/SKILL.md | 15 ++++--- skills/pr/references/publish.md | 5 ++- skills/pr/references/watch.md | 5 +++ skills/pr/scripts/pr_state.py | 23 ++++++++++- skills/pr/scripts/test_pr_state.py | 66 ++++++++++++++++++++++++++---- skills/review-fix-loop/SKILL.md | 13 ++++++ 7 files changed, 113 insertions(+), 18 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index 59c0088..dbd083a 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -1059,6 +1059,8 @@ other nonignored untracked source. skips, and stable snapshots. Generic delegation and handoff use Orchestration, with tasks mapped to review assignments and workflow/pass identities; findings use Review Sweep with stable issue IDs and per-issue repair/check evidence. + Finishing on a pushed PR head posts the `review-fix-loop` commit status that + `pr_state` requires before merge. ## pr @@ -1072,6 +1074,8 @@ watching. Fixing feedback moved to `review-sweep` and merging to `merge`. Publis matching review evidence, and runs `show-dont-tell` then the review loop when there is none. `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, kept apart from CI checks: +missing or failed blocks, and pending waits. 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/merge/SKILL.md b/skills/merge/SKILL.md index eeb8d63..46be27f 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -42,12 +42,15 @@ 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 + `review-fix-loop` status records that review, and `pr_state` blocks without it. + [Review Fix Loop](../review-fix-loop/SKILL.md#6-finish) posts it, including after a scoped + rerun for a repair. After a base update or rebase makes a new head, read the new base commits. + If they don't affect the files or behavior this change depends on, carry the review over: + post the status on the new head with a description such as + `carried from after base update`. Otherwise the evidence is stale. 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. 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/references/publish.md b/skills/pr/references/publish.md index ed04a06..066b1e8 100644 --- a/skills/pr/references/publish.md +++ b/skills/pr/references/publish.md @@ -27,5 +27,6 @@ Use [PR body](pr-body.md) for the title and description. Refresh them when fixes change scope, evidence, or landing risk. Mark ready unless a draft was requested. Confirm the remote head matches the intended local head, the base is correct, -and one PR represents that branch. Record its URL and remote head for -[watching](watch.md). +and one PR represents that branch. Post the `review-fix-loop` status from the +loop's handoff once that head is the reviewed snapshot. Record its URL and remote +head for [watching](watch.md). diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index c44ec3a..6b18d27 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -64,6 +64,11 @@ 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` status, and waits while it's +pending. It reports that status under `review`, apart from `checks`. +[Review Fix Loop](../../review-fix-loop/SKILL.md#6-finish) posts it when it finishes on the head, +and [Merge](../../merge/SKILL.md) carries it over a base update that doesn't affect the change. + 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..a7dbe84 100644 --- a/skills/pr/scripts/pr_state.py +++ b/skills/pr/scripts/pr_state.py @@ -15,6 +15,11 @@ 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_MISSING = (f"no '{REVIEW_CONTEXT}' status on this head; run Review Fix Loop on it, " + "or re-post the status after a base update as merge allows") +REVIEW_PENDING = f"the '{REVIEW_CONTEXT}' status is pending on this head" +REVIEW_FAILED = f"the '{REVIEW_CONTEXT}' status failed 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 = """ @@ -227,7 +232,8 @@ 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)] + results = [check_result(node) for node in all_nodes(gh, CHECKS_QUERY, pr, checks_page)] + checks = [result for result in results if result["name"] != REVIEW_CONTEXT] state = { "summary": summary, "behind_by": commits_behind_base(gh, pr, summary), @@ -236,6 +242,8 @@ def read_state(gh, pr): "comments": all_nodes(gh, COMMENTS_QUERY, pr), "reviews": all_nodes(gh, REVIEWS_QUERY, pr), "checks": checks, + "review": next((result for result in results if result["name"] == REVIEW_CONTEXT), + None), "expects_checks": bool(checks) or has_workflows(gh, pr, summary["headRefOid"]), } if read_summary(gh, pr) == summary: @@ -263,6 +271,15 @@ def assess_checks(state, blockers, pending, to_settle): to_settle.append("no checks reported on this head") +def assess_review(review, blockers, pending): + if review is None: + blockers.append(REVIEW_MISSING) + elif review["state"] == "fail": + blockers.append(REVIEW_FAILED) + elif review["state"] == "pending": + pending.append(REVIEW_PENDING) + + def assess_merge_state(state, blockers, pending): summary = state["summary"] mergeable, merge_state = summary["mergeable"], summary["mergeStateStatus"] @@ -274,7 +291,8 @@ 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"]] if check): blockers.append("GitHub reports a failing status that no check here shows; " "read the head and test-merge targets") @@ -293,6 +311,7 @@ def assess(state, expected_head=None): if state.get("provisional"): blockers.append(PROVISIONAL_BLOCKER) + assess_review(state["review"], blockers, pending) 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..5376086 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,51 @@ 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_MISSING])) + + def test_passing_review_status_lets_the_head_land(self): + code, result = run(FakeGitHub()) + self.assertEqual((code, result["status"], result["review"]["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_pending_review_status_waits(self): + for merge_state in ("CLEAN", "UNSTABLE"): + with self.subTest(merge_state=merge_state): + code, result = run(FakeGitHub([summary(mergeStateStatus=merge_state)], + review=review_status("PENDING"))) + self.assertEqual((code, result["pending"], result["blockers"]), + (3, [pr_state.REVIEW_PENDING], [])) + + def test_review_status_is_not_a_ci_check(self): + for review, blocker in ((review_status("FAILURE"), "status failed"), + (review_status("ERROR", required=True), "status failed")): + with self.subTest(review=review): + _, result = run(FakeGitHub(checks=[check_run()], review=review)) + self.assertEqual(len(result["blockers"]), 1) + self.assertIn(blocker, result["blockers"][0]) + self.assertNotIn(pr_state.REVIEW_CONTEXT, json.dumps(result["checks"])) + + 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..df2a1a7 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -164,6 +164,19 @@ 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. +Record completion on the PR's head, because +[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging without it. +Confirm with `gh pr view --json headRefOid` that the pushed head is the final +snapshot, then post a status on its full SHA, with a description of at most 140 characters: + +```sh +gh api repos///statuses/ -f state=success \ + -f context=review-fix-loop -f description=": passes" +``` + +When the final snapshot isn't pushed yet, put the command in the handoff. The caller posts it +once the PR's head is that snapshot. + 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, From aee4d1aec41f67b916b849108ea183d4d84354ad Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 22:35:43 -0500 Subject: [PATCH 2/7] Tighten the review-fix-loop merge gate Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 7 ++-- skills/gh-stack/SKILL.md | 2 ++ skills/merge/SKILL.md | 10 +++--- skills/pr/references/publish.md | 6 ++-- skills/pr/references/watch.md | 4 +-- skills/pr/scripts/pr_state.py | 33 ++++++++---------- skills/pr/scripts/test_pr_state.py | 54 ++++++++++++++++++++---------- skills/review-fix-loop/SKILL.md | 13 +++---- 8 files changed, 72 insertions(+), 57 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index dbd083a..f47f226 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -519,6 +519,7 @@ 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 `submit`, or `link` when it creates PRs or marks them ready, and `gh stack merge` runs only from `merge`’s land step. +After `submit` or `push`, each layer's held `review-fix-loop` status is posted on its head. 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` @@ -1059,8 +1060,6 @@ other nonignored untracked source. skips, and stable snapshots. Generic delegation and handoff use Orchestration, with tasks mapped to review assignments and workflow/pass identities; findings use Review Sweep with stable issue IDs and per-issue repair/check evidence. - Finishing on a pushed PR head posts the `review-fix-loop` commit status that - `pr_state` requires before merge. ## pr @@ -1074,8 +1073,8 @@ watching. Fixing feedback moved to `review-sweep` and merging to `merge`. Publis matching review evidence, and runs `show-dont-tell` then the review loop when there is none. `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, kept apart from CI checks: -missing or failed blocks, and pending waits. +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..d9e043e 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -36,6 +36,8 @@ their callers and use separate stacks for independent work. 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. + After `submit` or `push`, post each layer's held `review-fix-loop` status on that layer's + head, per [Review Fix Loop's finish step](../review-fix-loop/SKILL.md#6-finish). Land through [Merge](../merge/SKILL.md): run `gh stack merge` only from its land step. diff --git a/skills/merge/SKILL.md b/skills/merge/SKILL.md index 46be27f..e319d8a 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -43,11 +43,11 @@ limits. 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. The head's - `review-fix-loop` status records that review, and `pr_state` blocks without it. - [Review Fix Loop](../review-fix-loop/SKILL.md#6-finish) posts it, including after a scoped - rerun for a repair. After a base update or rebase makes a new head, read the new base commits. - If they don't affect the files or behavior this change depends on, carry the review over: - post the status on the new head with a description such as + `review-fix-loop` status records that review, and `pr_state` blocks without it. Review Fix + Loop posts it, including after a scoped rerun for a repair. After a base update or rebase + makes a new head, read the new base commits. If they don't affect the files or behavior this + change depends on, carry the review over: post the status on the new head with + [the loop's command](../review-fix-loop/SKILL.md#6-finish) and the description `carried from after base update`. Otherwise the evidence is stale. 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. diff --git a/skills/pr/references/publish.md b/skills/pr/references/publish.md index 066b1e8..4471628 100644 --- a/skills/pr/references/publish.md +++ b/skills/pr/references/publish.md @@ -27,6 +27,6 @@ Use [PR body](pr-body.md) for the title and description. Refresh them when fixes change scope, evidence, or landing risk. Mark ready unless a draft was requested. Confirm the remote head matches the intended local head, the base is correct, -and one PR represents that branch. Post the `review-fix-loop` status from the -loop's handoff once that head is the reviewed snapshot. Record its URL and remote -head for [watching](watch.md). +and one PR represents that branch. Post a held `review-fix-loop` status command +once the reviewed snapshot is pushed. Record its URL and remote head for +[watching](watch.md). diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index 6b18d27..32e9199 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -64,8 +64,8 @@ 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` status, and waits while it's -pending. It reports that status under `review`, apart from `checks`. +`pr_state` blocks until the head has a passing `review-fix-loop` commit status. It reports that +status under `review_status`, apart from `checks`. [Review Fix Loop](../../review-fix-loop/SKILL.md#6-finish) posts it when it finishes on the head, and [Merge](../../merge/SKILL.md) carries it over a base update that doesn't affect the change. diff --git a/skills/pr/scripts/pr_state.py b/skills/pr/scripts/pr_state.py index a7dbe84..afb5613 100644 --- a/skills/pr/scripts/pr_state.py +++ b/skills/pr/scripts/pr_state.py @@ -16,10 +16,8 @@ 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_MISSING = (f"no '{REVIEW_CONTEXT}' status on this head; run Review Fix Loop on it, " - "or re-post the status after a base update as merge allows") -REVIEW_PENDING = f"the '{REVIEW_CONTEXT}' status is pending on this head" -REVIEW_FAILED = f"the '{REVIEW_CONTEXT}' status failed on this head; run Review Fix Loop on it" +REVIEW_BLOCKER = (f"no passing '{REVIEW_CONTEXT}' status on this head; run Review Fix Loop " + "on it, or re-post the status after a base update as merge allows") # baseRefOid stops at the PR's last sync with its base, so read the branch's live tip instead. SUMMARY_QUERY = """ @@ -215,6 +213,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") @@ -232,8 +234,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) - results = [check_result(node) for node in all_nodes(gh, CHECKS_QUERY, pr, checks_page)] - checks = [result for result in results if result["name"] != REVIEW_CONTEXT] + 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), @@ -242,8 +245,7 @@ def read_state(gh, pr): "comments": all_nodes(gh, COMMENTS_QUERY, pr), "reviews": all_nodes(gh, REVIEWS_QUERY, pr), "checks": checks, - "review": next((result for result in results if result["name"] == REVIEW_CONTEXT), - None), + "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: @@ -271,15 +273,6 @@ def assess_checks(state, blockers, pending, to_settle): to_settle.append("no checks reported on this head") -def assess_review(review, blockers, pending): - if review is None: - blockers.append(REVIEW_MISSING) - elif review["state"] == "fail": - blockers.append(REVIEW_FAILED) - elif review["state"] == "pending": - pending.append(REVIEW_PENDING) - - def assess_merge_state(state, blockers, pending): summary = state["summary"] mergeable, merge_state = summary["mergeable"], summary["mergeStateStatus"] @@ -292,7 +285,8 @@ def assess_merge_state(state, blockers, pending): 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"], state["review"]] if check): + 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") @@ -311,7 +305,8 @@ def assess(state, expected_head=None): if state.get("provisional"): blockers.append(PROVISIONAL_BLOCKER) - assess_review(state["review"], blockers, pending) + 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 5376086..ace69dd 100644 --- a/skills/pr/scripts/test_pr_state.py +++ b/skills/pr/scripts/test_pr_state.py @@ -279,11 +279,11 @@ def test_head_without_the_review_status_blocks(self): 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_MISSING])) + self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) def test_passing_review_status_lets_the_head_land(self): code, result = run(FakeGitHub()) - self.assertEqual((code, result["status"], result["review"]["state"]), (0, "ready", "pass")) + 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"), @@ -300,22 +300,40 @@ def test_closed_and_merged_prs_need_no_review_status(self): code, result = run(FakeGitHub([summary(state="CLOSED")], review=None)) self.assertEqual((code, result["blockers"]), (2, ["the PR is closed"])) - def test_pending_review_status_waits(self): - for merge_state in ("CLEAN", "UNSTABLE"): - with self.subTest(merge_state=merge_state): - code, result = run(FakeGitHub([summary(mergeStateStatus=merge_state)], - review=review_status("PENDING"))) - self.assertEqual((code, result["pending"], result["blockers"]), - (3, [pr_state.REVIEW_PENDING], [])) - - def test_review_status_is_not_a_ci_check(self): - for review, blocker in ((review_status("FAILURE"), "status failed"), - (review_status("ERROR", required=True), "status failed")): - with self.subTest(review=review): - _, result = run(FakeGitHub(checks=[check_run()], review=review)) - self.assertEqual(len(result["blockers"]), 1) - self.assertIn(blocker, result["blockers"][0]) - self.assertNotIn(pr_state.REVIEW_CONTEXT, json.dumps(result["checks"])) + 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" diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index df2a1a7..7151011 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -164,18 +164,19 @@ 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. -Record completion on the PR's head, because -[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging without it. -Confirm with `gh pr view --json headRefOid` that the pushed head is the final -snapshot, then post a status on its full SHA, with a description of at most 140 characters: +Record completion on the final snapshot's commit, because +[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. A +commit status needs only the repository and the SHA, with or without a PR. Check that the +commit is on the remote, for example that `git ls-remote ` names it as the +branch head. Then post a status on its full SHA, with a description of at most 140 characters: ```sh gh api repos///statuses/ -f state=success \ -f context=review-fix-loop -f description=": passes" ``` -When the final snapshot isn't pushed yet, put the command in the handoff. The caller posts it -once the PR's head is that snapshot. +When it isn't on the remote yet, put this exact command in the handoff. Whoever pushes that +snapshot posts it once the commit is on the remote. In the Orchestration handoff, map child tasks to workflow/pass, review assignments, criteria, and configured models, and state which reviewer each follow-up task From d3af2a46c3ae5125220b650a3aed908c35245ee2 Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 22:48:53 -0500 Subject: [PATCH 3/7] Bind the review record to the reviewed commit Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 5 +++-- skills/gh-stack/SKILL.md | 5 +++-- skills/implement/SKILL.md | 6 +++--- skills/merge/SKILL.md | 25 +++++++++++++++++-------- skills/pr/SKILL.md | 14 ++++++++------ skills/pr/references/publish.md | 26 ++++++++++++++++---------- skills/pr/references/watch.md | 4 ++-- skills/pr/scripts/pr_state.py | 2 +- skills/pr/scripts/test_pr_state.py | 8 +++++++- skills/review-fix-loop/SKILL.md | 19 +++++++++++-------- 10 files changed, 71 insertions(+), 43 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index f47f226..e097f0f 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -519,7 +519,8 @@ 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 `submit`, or `link` when it creates PRs or marks them ready, and `gh stack merge` runs only from `merge`’s land step. -After `submit` or `push`, each layer's held `review-fix-loop` status is posted on its head. +Each layer runs its held `review-fix-loop` status command unchanged once its SHA is pushed; +after a restack, merge's carry-over rule or another loop run covers that layer. 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` @@ -1069,7 +1070,7 @@ 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 +watching. Fixing feedback moved to `review-sweep` and merging to `merge`. Publishing commits before review and pushes after it. It reuses matching review evidence, and runs `show-dont-tell` then the review loop when there is none. `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`. diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index d9e043e..adc0588 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -36,8 +36,9 @@ their callers and use separate stacks for independent work. 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. - After `submit` or `push`, post each layer's held `review-fix-loop` status on that layer's - head, per [Review Fix Loop's finish step](../review-fix-loop/SKILL.md#6-finish). + 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, + use [Merge's carry-over rule](../merge/SKILL.md) or rerun the loop for that layer. 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 e319d8a..27ab1ee 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -32,6 +32,21 @@ 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. Carry-over is the only + way a review moves to a new head, and it needs all three: + + - The old head has a success `review-fix-loop` status. + - The change is identical: `git range-diff .. ..` + shows the PR's own commits unchanged, or the old and new diffs against their merge bases + have the same `git patch-id --stable`. + - The new base commits don't affect the files or behavior this change depends on. + + Then post the status on the new head with + [the loop's command](../review-fix-loop/SKILL.md#6-finish) and the description + `carried from after base update`. Otherwise the review is stale. A caller with + its own repair owner gets the gap back. Without one, run + [PR's review step](../pr/SKILL.md#publish), publish any fixes, and read the state again. + A caller that keeps its own repair owner gets each failure, comment, and stop back, and resumes that owner. You repair nothing. @@ -43,14 +58,8 @@ limits. 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. The head's - `review-fix-loop` status records that review, and `pr_state` blocks without it. Review Fix - Loop posts it, including after a scoped rerun for a repair. After a base update or rebase - makes a new head, read the new base commits. If they don't affect the files or behavior this - change depends on, carry the review over: post the status on the new head with - [the loop's command](../review-fix-loop/SKILL.md#6-finish) and the description - `carried from after base update`. Otherwise the evidence is stale. 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. + `review-fix-loop` status records that review, and 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..9d3ba45 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -24,17 +24,19 @@ remote description. Writing text commits and pushes nothing. ## Publish -Watching an existing PR starts at step 3. +Watching an existing PR starts at step 4. -1. **Get it reviewed.** Reuse review evidence that matches the complete diff against the +1. **Commit it.** Follow [commit](references/publish.md#commit), so review sees a committed + head. +2. **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). +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 4471628..9176300 100644 --- a/skills/pr/references/publish.md +++ b/skills/pr/references/publish.md @@ -1,17 +1,24 @@ # 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. +Commit review repairs the same way. + +## 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 @@ -27,6 +34,5 @@ Use [PR body](pr-body.md) for the title and description. Refresh them when fixes change scope, evidence, or landing risk. Mark ready unless a draft was requested. Confirm the remote head matches the intended local head, the base is correct, -and one PR represents that branch. Post a held `review-fix-loop` status command -once the reviewed snapshot is pushed. Record its URL and remote head for +and one PR represents that branch. Record its URL and remote head for [watching](watch.md). diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index 32e9199..c57aeaf 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -33,6 +33,8 @@ 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:** run [Review Fix Loop](../../review-fix-loop/SKILL.md#6-finish) + on the head, or, after a base update, apply [Merge's carry-over rule](../../merge/SKILL.md). - **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 @@ -66,8 +68,6 @@ 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`. -[Review Fix Loop](../../review-fix-loop/SKILL.md#6-finish) posts it when it finishes on the head, -and [Merge](../../merge/SKILL.md) carries it over a base update that doesn't affect the change. 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 diff --git a/skills/pr/scripts/pr_state.py b/skills/pr/scripts/pr_state.py index afb5613..e7e7956 100644 --- a/skills/pr/scripts/pr_state.py +++ b/skills/pr/scripts/pr_state.py @@ -17,7 +17,7 @@ "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, or re-post the status after a base update as merge allows") + "on it, or apply merge's carry-over rule after a base update") # baseRefOid stops at the PR's last sync with its base, so read the branch's live tip instead. SUMMARY_QUERY = """ diff --git a/skills/pr/scripts/test_pr_state.py b/skills/pr/scripts/test_pr_state.py index ace69dd..7516d20 100644 --- a/skills/pr/scripts/test_pr_state.py +++ b/skills/pr/scripts/test_pr_state.py @@ -281,9 +281,15 @@ def test_head_without_the_review_status_blocks(self): code, result = run(FakeGitHub(checks=checks, review=None)) self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) + def test_review_blocker_names_the_loop_and_merges_carry_over(self): + self.assertIn("run Review Fix Loop on it", pr_state.REVIEW_BLOCKER) + self.assertIn("merge's carry-over rule", pr_state.REVIEW_BLOCKER) + self.assertNotIn("re-post", pr_state.REVIEW_BLOCKER) + 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")) + 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"), diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 7151011..93d26b8 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -164,19 +164,22 @@ 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. -Record completion on the final snapshot's commit, because -[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. A -commit status needs only the repository and the SHA, with or without a PR. Check that the -commit is on the remote, for example that `git ls-remote ` names it as the -branch head. Then post a status on its full SHA, with a description of at most 140 characters: +Record completion on the exact commit the loop reviewed, because +[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. +For PR work, the final snapshot is a commit. When it ends with uncommitted edits, the caller +commits them and confirms with `git status --porcelain` that nothing in scope is left +uncommitted. Build this command with that commit's full SHA and a description of at most 140 +characters: ```sh -gh api repos///statuses/ -f state=success \ +gh api repos///statuses/ -f state=success \ -f context=review-fix-loop -f description=": passes" ``` -When it isn't on the remote yet, put this exact command in the handoff. Whoever pushes that -snapshot posts it once the commit is on the remote. +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. A head with +another SHA gets a record only through [Merge's carry-over rule](../merge/SKILL.md) or another +run of this loop. In the Orchestration handoff, map child tasks to workflow/pass, review assignments, criteria, and configured models, and state which reviewer each follow-up task From b72bd1bba5af08ba696644f08ffd6042c0fce125 Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 23:12:21 -0500 Subject: [PATCH 4/7] Drop review carry-over; the loop alone posts the status Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 7 ++++--- skills/gh-stack/SKILL.md | 2 +- skills/merge/SKILL.md | 19 +++++-------------- skills/pr/SKILL.md | 10 +++++----- skills/pr/references/watch.md | 4 ++-- skills/pr/scripts/pr_state.py | 3 +-- skills/pr/scripts/test_pr_state.py | 7 +++---- skills/review-fix-loop/SKILL.md | 17 +++++++++-------- 8 files changed, 30 insertions(+), 39 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index e097f0f..7e98945 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -520,7 +520,7 @@ Scoped PR layers follow `pr`’s publish step, which links every layer in T3; CL publication does not substitute for that link. Layers get `pr`’s review step 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, merge's carry-over rule or another loop run covers that layer. +after a restack, the loop reruns for that layer. 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` @@ -1070,8 +1070,9 @@ 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 commits before review and pushes after it. It 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 commits before review and pushes after it. It +runs `show-dont-tell` then the review loop, which counts matching earlier review as +retained coverage. `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 diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index adc0588..636eb3a 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -38,7 +38,7 @@ their callers and use separate stacks for independent work. [PR's review step](../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, - use [Merge's carry-over rule](../merge/SKILL.md) or rerun the loop for that layer. + rerun the loop for that layer. Land through [Merge](../merge/SKILL.md): run `gh stack merge` only from its land step. diff --git a/skills/merge/SKILL.md b/skills/merge/SKILL.md index 27ab1ee..cafafdc 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -32,20 +32,11 @@ 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. Carry-over is the only - way a review moves to a new head, and it needs all three: - - - The old head has a success `review-fix-loop` status. - - The change is identical: `git range-diff .. ..` - shows the PR's own commits unchanged, or the old and new diffs against their merge bases - have the same `git patch-id --stable`. - - The new base commits don't affect the files or behavior this change depends on. - - Then post the status on the new head with - [the loop's command](../review-fix-loop/SKILL.md#6-finish) and the description - `carried from after base update`. Otherwise the review is stale. A caller with - its own repair owner gets the gap back. Without one, run - [PR's review step](../pr/SKILL.md#publish), publish any fixes, and read the state again. + `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, run + [PR's review step](../pr/SKILL.md#publish), publish any fixes, and read the state again. The + loop's [coverage rule](../review-fix-loop/SKILL.md#coverage-rule) limits that pass to what + changed. A caller that keeps its own repair owner gets each failure, comment, and stop back, and resumes that owner. You repair nothing. diff --git a/skills/pr/SKILL.md b/skills/pr/SKILL.md index 9d3ba45..5d62d47 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -28,11 +28,11 @@ Watching an existing PR starts at step 4. 1. **Commit it.** Follow [commit](references/publish.md#commit), so review sees a committed head. -2. **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. **Get it reviewed.** Run [Show, Don't Tell](../show-dont-tell/SKILL.md) on the complete + diff against the immediate base, including docs-only changes, unless matching earlier + review included it. Then run [Review Fix Loop](../review-fix-loop/SKILL.md) on the result. + Matching earlier review is its [retained coverage](../review-fix-loop/SKILL.md#coverage-rule), + and finishing on it 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. diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index c57aeaf..4b988cd 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -33,8 +33,8 @@ 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:** run [Review Fix Loop](../../review-fix-loop/SKILL.md#6-finish) - on the head, or, after a base update, apply [Merge's carry-over rule](../../merge/SKILL.md). +- **No passing review status:** run [Review Fix Loop](../../review-fix-loop/SKILL.md) on the + head. - **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 diff --git a/skills/pr/scripts/pr_state.py b/skills/pr/scripts/pr_state.py index e7e7956..0b9f8f5 100644 --- a/skills/pr/scripts/pr_state.py +++ b/skills/pr/scripts/pr_state.py @@ -16,8 +16,7 @@ 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, or apply merge's carry-over rule after a base update") +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 = """ diff --git a/skills/pr/scripts/test_pr_state.py b/skills/pr/scripts/test_pr_state.py index 7516d20..f7cc6bf 100644 --- a/skills/pr/scripts/test_pr_state.py +++ b/skills/pr/scripts/test_pr_state.py @@ -281,10 +281,9 @@ def test_head_without_the_review_status_blocks(self): code, result = run(FakeGitHub(checks=checks, review=None)) self.assertEqual((code, result["blockers"]), (2, [pr_state.REVIEW_BLOCKER])) - def test_review_blocker_names_the_loop_and_merges_carry_over(self): - self.assertIn("run Review Fix Loop on it", pr_state.REVIEW_BLOCKER) - self.assertIn("merge's carry-over rule", pr_state.REVIEW_BLOCKER) - self.assertNotIn("re-post", 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()) diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 93d26b8..52d9e56 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -106,6 +106,9 @@ it needs. ## 4. Route repairs +For PR work, the caller commits each repair before the check or pass that assesses it, so +every pass assesses a commit. + Ordinary prose and wording-only fixes use targeted checks. For code or substantive instruction fixes, start a new pass that sends each fix to a new task on the original reviewer's model for what it changes, under Orchestration's @@ -164,12 +167,10 @@ 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. -Record completion on the exact commit the loop reviewed, because -[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. -For PR work, the final snapshot is a commit. When it ends with uncommitted edits, the caller -commits them and confirms with `git status --porcelain` that nothing in scope is left -uncommitted. Build this command with that commit's full SHA and a description of at most 140 -characters: +Record completion on the commit the final pass assessed, because +[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. Any +edit after that pass is a new snapshot. Build this command with that commit's full SHA and a +description of at most 140 characters: ```sh gh api repos///statuses/ -f state=success \ @@ -178,8 +179,8 @@ gh api repos///statuses/ -f state=success \ 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. A head with -another SHA gets a record only through [Merge's carry-over rule](../merge/SKILL.md) or another -run of this loop. +another SHA needs another run of this loop. 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 From a221398e7e5c52754ec9745f49f1d508db72ffe6 Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 23:22:02 -0500 Subject: [PATCH 5/7] Scope the review record to committed PR snapshots Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 7 +++---- skills/gh-stack/SKILL.md | 2 +- skills/merge/SKILL.md | 3 +-- skills/pr/SKILL.md | 13 ++++++------- skills/pr/references/publish.md | 1 - skills/pr/references/watch.md | 2 +- skills/review-fix-loop/SKILL.md | 22 ++++++++++++++-------- 7 files changed, 26 insertions(+), 24 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index 7e98945..35c9a38 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -520,7 +520,7 @@ Scoped PR layers follow `pr`’s publish step, which links every layer in T3; CL publication does not substitute for that link. Layers get `pr`’s review step 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. +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` @@ -1070,9 +1070,8 @@ 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 commits before review and pushes after it. It -runs `show-dont-tell` then the review loop, which counts matching earlier review as -retained coverage. +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 diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 636eb3a..ee0cce1 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -38,7 +38,7 @@ their callers and use separate stacks for independent work. [PR's review step](../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. + 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/merge/SKILL.md b/skills/merge/SKILL.md index cafafdc..cb8b0b4 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -35,8 +35,7 @@ limits. `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, run [PR's review step](../pr/SKILL.md#publish), publish any fixes, and read the state again. The - loop's [coverage rule](../review-fix-loop/SKILL.md#coverage-rule) limits that pass to what - changed. + loop reviews the new head as 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. diff --git a/skills/pr/SKILL.md b/skills/pr/SKILL.md index 5d62d47..bb66b91 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -26,13 +26,12 @@ remote description. Writing text commits and pushes nothing. Watching an existing PR starts at step 4. -1. **Commit it.** Follow [commit](references/publish.md#commit), so review sees a committed - head. -2. **Get it reviewed.** Run [Show, Don't Tell](../show-dont-tell/SKILL.md) on the complete - diff against the immediate base, including docs-only changes, unless matching earlier - review included it. Then run [Review Fix Loop](../review-fix-loop/SKILL.md) on the result. - Matching earlier review is its [retained coverage](../review-fix-loop/SKILL.md#coverage-rule), - and finishing on it posts the status. Continue once review and verification pass. +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 review of the + current head included it. Then [commit](references/publish.md#commit). +2. **Get it reviewed.** Run [Review Fix Loop](../review-fix-loop/SKILL.md) on the committed + head. 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. diff --git a/skills/pr/references/publish.md b/skills/pr/references/publish.md index 9176300..c40b118 100644 --- a/skills/pr/references/publish.md +++ b/skills/pr/references/publish.md @@ -9,7 +9,6 @@ work. Keep the feature branch; on the default branch, create a parent layer for stacks. Commit owned edits with a terse subject, and run checks needed by final staging. -Commit review repairs the same way. ## Push diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index 4b988cd..ddd1c80 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -34,7 +34,7 @@ when a result looks wrong. 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:** run [Review Fix Loop](../../review-fix-loop/SKILL.md) on the - head. + head, unless posting its status already failed; then report that blocker. - **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 diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 52d9e56..fe00e08 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -21,6 +21,9 @@ 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 on a clean worktree, and its snapshot is HEAD. The caller +commits any edits, including Show, Don't Tell's and repairs, before the pass that assesses them. + Select initial discovery from the first matching row, honoring explicit user requests: @@ -106,9 +109,6 @@ it needs. ## 4. Route repairs -For PR work, the caller commits each repair before the check or pass that assesses it, so -every pass assesses a commit. - Ordinary prose and wording-only fixes use targeted checks. For code or substantive instruction fixes, start a new pass that sends each fix to a new task on the original reviewer's model for what it changes, under Orchestration's @@ -123,6 +123,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 @@ -167,9 +171,11 @@ 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. -Record completion on the commit the final pass assessed, because -[`pr_state`](../pr/references/watch.md#ready-or-blocked) blocks merging a head without it. Any -edit after that pass is a new snapshot. Build this command with that commit's full SHA and a +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. 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 @@ -178,8 +184,8 @@ gh api repos///statuses/ -f state=success \ ``` 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. A head with -another SHA needs another run of this loop. When posting fails, such as for a missing +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, From 48823f8b5ce32905361d3c3ae93ab91220ccf49e Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 23:31:27 -0500 Subject: [PATCH 6/7] Route every stamped head through PR's Prepare and Review steps Co-Authored-By: Claude Opus 5.5 --- SOURCES.md | 2 +- skills/gh-stack/SKILL.md | 3 ++- skills/merge/SKILL.md | 10 ++++++---- skills/pr/SKILL.md | 10 ++++++---- skills/pr/references/watch.md | 6 ++++-- skills/review-fix-loop/SKILL.md | 7 +++++-- 6 files changed, 24 insertions(+), 14 deletions(-) diff --git a/SOURCES.md b/SOURCES.md index 35c9a38..626694c 100644 --- a/SOURCES.md +++ b/SOURCES.md @@ -517,7 +517,7 @@ 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. diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index ee0cce1..8213af1 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -35,7 +35,8 @@ 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). diff --git a/skills/merge/SKILL.md b/skills/merge/SKILL.md index cb8b0b4..c0c50c0 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -33,9 +33,10 @@ limits. 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, run - [PR's review step](../pr/SKILL.md#publish), publish any fixes, and read the state again. The - loop reviews the new head as a [repair pass](../review-fix-loop/SKILL.md#4-route-repairs). + 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. @@ -48,7 +49,8 @@ limits. 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. The head's - `review-fix-loop` status records that review, and step 2 restores a missing one. Route + `review-fix-loop` status records the loop's review, and PR's Prepare step supplies Show, + Don't Tell; check that both happened. Step 2 restores a missing review. 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 diff --git a/skills/pr/SKILL.md b/skills/pr/SKILL.md index bb66b91..233ed3a 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -27,11 +27,13 @@ remote description. Writing text commits and pushes nothing. 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 review of the - current head included it. Then [commit](references/publish.md#commit). + 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. 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. + 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. diff --git a/skills/pr/references/watch.md b/skills/pr/references/watch.md index ddd1c80..886714a 100644 --- a/skills/pr/references/watch.md +++ b/skills/pr/references/watch.md @@ -33,8 +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:** run [Review Fix Loop](../../review-fix-loop/SKILL.md) on the - head, unless posting its status already failed; then report that blocker. +- **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 diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index fe00e08..3d61210 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -21,8 +21,11 @@ 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 on a clean worktree, and its snapshot is HEAD. The caller -commits any edits, including Show, Don't Tell's and repairs, before the pass that assesses them. +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, confirm that `git status --porcelain -- ` prints nothing, and +record that in the brief. Select initial discovery from the first matching row, honoring explicit user requests: From b33879f7ab68947b36eaa6f4b7c4ff6a2eedfa4e Mon Sep 17 00:00:00 2001 From: Sree Raman <1271305+sreejithraman@users.noreply.github.com> Date: Sun, 8 Nov 2026 23:43:09 -0500 Subject: [PATCH 7/7] Let the review status imply Show, Don't Tell; check the whole tree Co-Authored-By: Claude Opus 5.5 --- skills/merge/SKILL.md | 6 +++--- skills/review-fix-loop/SKILL.md | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/skills/merge/SKILL.md b/skills/merge/SKILL.md index c0c50c0..a2acdce 100644 --- a/skills/merge/SKILL.md +++ b/skills/merge/SKILL.md @@ -49,9 +49,9 @@ limits. 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. The head's - `review-fix-loop` status records the loop's review, and PR's Prepare step supplies Show, - Don't Tell; check that both happened. Step 2 restores a missing review. Route - missing or stale verification the same way as a stale review, then go back to step 2. + 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/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 3d61210..04b0535 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -24,8 +24,8 @@ 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, confirm that `git status --porcelain -- ` prints nothing, and -record that in the brief. +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: @@ -175,7 +175,9 @@ 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. A +[`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