Require and validate Controller Handoff sections in workflow final reports
- Upgrade SKILL.md §K compact format to the issue #182 canonical field set (Task/Repo/Role/Identity/Issue-PR/Branch-SHA/Files/Validation/Mutations/ Current status/Blockers/Next/Safety) plus role-specific field lists for review/merge, author, and queue/inventory tasks. - Point the review-pr, merge-pr, and start-issue template handoff lines at the exactly-titled Controller Handoff section with their role fields. - Add review_proofs.assess_controller_handoff(): reports without the exact section are 'missing' (downgraded), present-but-partial are 'incomplete' with the absent fields listed, and role extras are enforced per role. - Add TestControllerHandoff (8 tests) including a SKILL.md doc-contract test so the documented requirement cannot silently rot. The handoff supplements the full report; full-report validation rules are unchanged. Closes #182 Co-Authored-By: Claude Fable 5 <[email protected]>
This commit is contained in:
Claude Fable 5
@@ -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": [],
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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).
|
||||
```
|
||||
|
||||
@@ -52,7 +52,10 @@ Steps:
|
||||
- MCP-Profile: <profile name>
|
||||
- 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).
|
||||
```
|
||||
|
||||
@@ -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).
|
||||
```
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user