diff --git a/review_proofs.py b/review_proofs.py index 7b08213..556cd09 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -361,3 +361,127 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "merge_performed": bool(merge_performed), "issue_status_verified": bool(issue_status_verified), } + + +# ── Controller Handoff validation (Issue #182) ──────────────────────────────── +# +# Every final report must end with a compact section titled exactly +# "Controller Handoff". Each required field is a (canonical name, aliases) +# pair; a field counts as present when any alias starts a bullet/label line +# inside the handoff section. + +HANDOFF_HEADING = "Controller Handoff" + +HANDOFF_BASE_FIELDS = ( + ("Task", ("task",)), + ("Repo", ("repo", "repository", "repo/state")), + ("Role", ("role",)), + ("Identity", ("identity",)), + ("Issue/PR", ("issue/pr", "issues/prs", "issue", "pr")), + ("Branch/SHA", ("branch/sha", "branch", "head sha")), + ("Files changed", ("files changed", "changed", "files")), + ("Validation", ("validation",)), + ("Mutations", ("mutations",)), + ("Current status", ("current status", "status")), + ("Blockers", ("blockers",)), + ("Next", ("next",)), + ("Safety", ("safety",)), +) + +HANDOFF_ROLE_FIELDS = { + "review": ( + ("Selected PR", ("selected pr",)), + ("Reviewer eligibility", ("reviewer eligibility", "eligibility")), + ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), + ("Review decision", ("review decision", "decision")), + ("Merge result", ("merge result",)), + ("Linked issue status", ("linked issue status", "linked issue")), + ("Cleanup status", ("cleanup status", "cleanup")), + ), + "author": ( + ("Selected issue", ("selected issue",)), + ("Claim/comment status", ("claim/comment status", "claim status", + "claim")), + ("PR number opened", ("pr number opened", "pr opened", "pr number")), + ("No review/merge confirmation", ("no review/merge", + "no review or merge")), + ), + "inventory": ( + ("Repositories checked", ("repositories checked", "repos checked")), + ("Open PR counts", ("open pr counts", "open pr count", + "open prs per repo")), + ("Selected PR or reason", ("selected pr", "none selected", + "reason none selected")), + ("Inventory completeness", ("inventory complete", "inventory scoped", + "inventory completeness")), + ), +} + + +def _handoff_section_lines(report_text): + """Return the lines of the Controller Handoff section, or None.""" + lines = (report_text or "").splitlines() + start = None + for i, line in enumerate(lines): + bare = line.strip().lstrip("#").strip().rstrip(":") + if bare == HANDOFF_HEADING: + start = i + 1 + break + if start is None: + return None + return lines[start:] + + +def assess_controller_handoff(report_text, role=None): + """Issue #182: final reports without a Controller Handoff downgrade. + + Verdicts: + - 'missing' — no exactly-titled section; the report is downgraded. + - 'incomplete' — section present but required fields absent (listed). + - 'complete' — all base fields plus the role-specific fields present. + + *role* is 'review', 'author', 'inventory', or None (base fields only). + The handoff supplements the full report; this helper never validates + the full report body, only the continuation summary. + """ + section = _handoff_section_lines(report_text) + if section is None: + return { + "verdict": "missing", + "downgraded": True, + "missing_fields": [name for name, _ in HANDOFF_BASE_FIELDS], + "reasons": [ + "final report has no section titled exactly " + f"'{HANDOFF_HEADING}'" + ], + } + + labels = [] + for line in section: + stripped = line.strip().lstrip("-*").strip() + if ":" in stripped: + labels.append(stripped.split(":", 1)[0].strip().lower()) + + required = list(HANDOFF_BASE_FIELDS) + required.extend(HANDOFF_ROLE_FIELDS.get(role or "", ())) + + missing = [] + for name, aliases in required: + if not any(label.startswith(alias) + for label in labels for alias in aliases): + missing.append(name) + + if missing: + return { + "verdict": "incomplete", + "downgraded": True, + "missing_fields": missing, + "reasons": [f"handoff missing required field: {m}" + for m in missing], + } + return { + "verdict": "complete", + "downgraded": False, + "missing_fields": [], + "reasons": [], + } diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index db85a89..ed0034a 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -286,22 +286,43 @@ current state immediately, without rereading the conversation. readability, not as a full human status report. PR bodies still carry the full review detail — the handoff never replaces PR documentation. -Compact format (default): +Compact format (default, canonical field set per issue #182): ```md ## Controller Handoff - Task: -- Repo/state: -- Issues/PRs: -- Changed: +- Repo: +- Role: +- Identity: +- Issue/PR: +- Branch/SHA: +- Files changed: - Validation: +- Mutations: +- Current status: - Blockers: -- Review: - Next: - Safety: ``` +Role-specific fields (append to the compact block): + +- review/merge tasks: `Selected PR:`, `Reviewer eligibility:`, + `Pinned reviewed head:`, `Review decision:`, `Merge result:`, + `Linked issue status:`, `Cleanup status:` +- author tasks: `Selected issue:`, `Claim/comment status:`, + `PR number opened:`, `No review/merge:` (explicit confirmation) +- queue/inventory tasks: `Repositories checked:`, `Open PR counts:`, + `Selected PR or reason none selected:`, `Inventory completeness:` + +The section title must be exactly `Controller Handoff`. +`review_proofs.assess_controller_handoff()` validates this section; reports +missing it (or missing required fields) are downgraded. The handoff never +replaces the full report — it is the compact continuation summary at the end, +and the full report must still carry exact validation results and mutation +confirmation. + The `Safety:` line is never omitted; it is usually: ```text diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index 8766ad6..4155fb6 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -35,5 +35,11 @@ Then run the cleanup template (worktree-cleanup.md): - delete remote branch, remove local branch + worktree folder - fetch/prune; confirm main checkout is clean and current (0 0). -Handoff: reviewer identity, merge result + commit, cleanup done, issue closed, PR metadata state/merged flag/hash, remote master hash, post-merge verification method used & verification results. +Handoff: end with a section titled exactly `Controller Handoff` per SKILL.md +§K (long form — a merge is always high-risk), including the review/merge role +fields (Selected PR, Reviewer eligibility, Pinned reviewed head, Review +decision, Merge result, Linked issue status, Cleanup status) plus: merge +commit, PR metadata state/merged flag/hash, remote master hash, and the +post-merge verification method used & verification results. Reports missing +the handoff are downgraded (review_proofs.assess_controller_handoff). ``` diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 3897a57..7770bad 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -52,7 +52,10 @@ Steps: - MCP-Profile: - Eligibility: passed/failed -Handoff: reviewer identity, PR author, scope verdict, checks + results, decision — -formatted per SKILL.md §K (compact by default; long form if a merge happened -or a gate blocked you); if you could not merge, name the exact gate. +Handoff: end with a section titled exactly `Controller Handoff` per SKILL.md +§K (compact by default; long form if a merge happened or a gate blocked you), +including the review/merge role fields: Selected PR, Reviewer eligibility, +Pinned reviewed head, Review decision, Merge result, Linked issue status, +Cleanup status. If you could not merge, name the exact gate. Reports missing +the handoff are downgraded (review_proofs.assess_controller_handoff). ``` diff --git a/skills/llm-project-workflow/templates/start-issue.md b/skills/llm-project-workflow/templates/start-issue.md index a3300a3..194d6aa 100644 --- a/skills/llm-project-workflow/templates/start-issue.md +++ b/skills/llm-project-workflow/templates/start-issue.md @@ -42,7 +42,10 @@ Steps: - Self-review allowed: no 9. Stop before review/merge — you are the author. -Handoff: issue #, branch, worktree path, files changed, checks + results, PR URL — -formatted as the compact Controller Handoff (SKILL.md §K; long form only on -the high-risk triggers); Review line: "Review needed — PR is open". +Handoff: end with a section titled exactly `Controller Handoff` per SKILL.md +§K (compact; long form only on the high-risk triggers), including the author +role fields: Selected issue, Claim/comment status, PR number opened, and an +explicit "No review/merge:" confirmation — plus branch, worktree path, files +changed, checks + results. Next line: "Review needed — PR is open". Reports +missing the handoff are downgraded (review_proofs.assess_controller_handoff). ``` diff --git a/tests/test_review_proofs.py b/tests/test_review_proofs.py index d67bfe6..6b8f032 100644 --- a/tests/test_review_proofs.py +++ b/tests/test_review_proofs.py @@ -20,6 +20,7 @@ import unittest sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) from review_proofs import ( # noqa: E402 + assess_controller_handoff, assess_inventory_completeness, assess_self_review_contamination, assess_validation_report, @@ -463,5 +464,107 @@ class TestFinalReport(unittest.TestCase): self.assertFalse(report["merge_allowed"]) +class TestControllerHandoff(unittest.TestCase): + """Issue #182: final reports must end with a Controller Handoff.""" + + BASE_HANDOFF = "\n".join([ + "## Controller Handoff", + "", + "- Task: implement issue #182", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: author", + "- Identity: jcwalker3 / prgs-author", + "- Issue/PR: #182 / PR #999", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: 700 passed, 6 skipped", + "- Mutations: one PR opened", + "- Current status: PR open", + "- Blockers: none", + "- Next: review PR #999", + "- Safety: no self-review; no self-merge; no secrets", + ]) + + def test_report_without_handoff_is_downgraded(self): + result = assess_controller_handoff("long report text, no handoff") + self.assertEqual(result["verdict"], "missing") + self.assertTrue(result["downgraded"]) + + def test_wrong_title_is_downgraded(self): + text = self.BASE_HANDOFF.replace( + "## Controller Handoff", "## Handoff Summary") + result = assess_controller_handoff(text) + self.assertEqual(result["verdict"], "missing") + + def test_complete_base_handoff_passes(self): + result = assess_controller_handoff( + "full report body...\n\n" + self.BASE_HANDOFF) + self.assertEqual(result["verdict"], "complete") + self.assertFalse(result["downgraded"]) + + def test_missing_base_fields_are_listed(self): + text = "\n".join( + line for line in self.BASE_HANDOFF.splitlines() + if not line.startswith(("- Mutations:", "- Safety:"))) + result = assess_controller_handoff(text) + self.assertEqual(result["verdict"], "incomplete") + self.assertTrue(result["downgraded"]) + self.assertIn("Mutations", result["missing_fields"]) + self.assertIn("Safety", result["missing_fields"]) + + def test_review_role_requires_review_fields(self): + result = assess_controller_handoff(self.BASE_HANDOFF, role="review") + self.assertEqual(result["verdict"], "incomplete") + self.assertIn("Pinned reviewed head", result["missing_fields"]) + self.assertIn("Merge result", result["missing_fields"]) + + complete = self.BASE_HANDOFF + "\n" + "\n".join([ + "- Selected PR: #999", + "- Reviewer eligibility: passed", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Review decision: approve", + "- Merge result: merged", + "- Linked issue status: closed", + "- Cleanup status: branch deleted", + ]) + result = assess_controller_handoff(complete, role="review") + self.assertEqual(result["verdict"], "complete") + + def test_author_role_requires_author_fields(self): + complete = self.BASE_HANDOFF + "\n" + "\n".join([ + "- Selected issue: #182", + "- Claim/comment status: comment-claimed", + "- PR number opened: #999", + "- No review/merge: confirmed", + ]) + result = assess_controller_handoff(complete, role="author") + self.assertEqual(result["verdict"], "complete") + + result = assess_controller_handoff(self.BASE_HANDOFF, role="author") + self.assertEqual(result["verdict"], "incomplete") + self.assertIn("No review/merge confirmation", result["missing_fields"]) + + def test_inventory_role_requires_inventory_fields(self): + complete = self.BASE_HANDOFF + "\n" + "\n".join([ + "- Repositories checked: Gitea-Tools, mcp-control-plane", + "- Open PR counts: 2 / 0", + "- Selected PR or reason: none eligible (self-authored)", + "- Inventory completeness: complete, no pagination needed", + ]) + result = assess_controller_handoff(complete, role="inventory") + self.assertEqual(result["verdict"], "complete") + + def test_skill_doc_declares_handoff_requirement(self): + # Doc-contract: SKILL.md must keep requiring the exact section and + # naming this validator, or the convention silently rots. + skill = ( + __import__("pathlib").Path(__file__).resolve().parent.parent + / "skills" / "llm-project-workflow" / "SKILL.md" + ).read_text(encoding="utf-8") + self.assertIn("## Controller Handoff", skill) + self.assertIn("assess_controller_handoff", skill) + self.assertIn("issue #182", skill) + + if __name__ == "__main__": unittest.main()