diff --git a/canonical_state_comments.py b/canonical_state_comments.py new file mode 100644 index 0000000..cc819ac --- /dev/null +++ b/canonical_state_comments.py @@ -0,0 +1,171 @@ +"""Canonical state comment validation helpers (#495). + +These helpers validate durable issue/PR/discussion state handoff comments. +They are intentionally pure and do not post comments; MCP mutation hooks are +owned by #496. +""" + +from __future__ import annotations + +import re + +CANONICAL_HEADINGS = ( + "canonical issue state", + "canonical pr state", + "canonical discussion summary", +) + +REQUIRED_FIELDS = ("STATE", "WHO_IS_NEXT", "NEXT_ACTION", "NEXT_PROMPT") +ALLOWED_NEXT_ACTORS = { + "controller", + "author", + "reviewer", + "merger", + "reconciler", + "user", +} + +VAGUE_NEXT_ACTIONS = { + "continue", + "handle this", + "do it", + "fix it", + "proceed", + "follow up", + "next", + "tbd", + "todo", + "n/a", + "none", +} + +_FIELD_RE = re.compile(r"^\s*(?:[-*]\s*)?([A-Z][A-Z0-9_ ]+)\s*:\s*(.*)$") +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) +_CLAIMS_STATE_UPDATE_RE = re.compile( + r"canonical\s+(?:issue|pr|discussion)?\s*state|" + r"state\s+comment\s+(?:posted|created|updated)|" + r"next[- ]action\s+comment\s+(?:posted|created|updated)", + re.IGNORECASE, +) + + +def extract_state_fields(text: str | None) -> dict[str, str]: + """Return upper-case labeled fields from a canonical state block.""" + fields: dict[str, str] = {} + current_key: str | None = None + for line in (text or "").splitlines(): + match = _FIELD_RE.match(line) + if match: + current_key = match.group(1).strip().upper().replace(" ", "_") + fields[current_key] = match.group(2).strip() + continue + stripped = line.strip() + if current_key and stripped and not stripped.startswith("#"): + existing = fields.get(current_key, "") + fields[current_key] = ( + f"{existing}\n{stripped}" if existing else stripped + ) + return fields + + +def contains_canonical_state_block(text: str | None) -> bool: + lower = (text or "").lower() + return any(heading in lower for heading in CANONICAL_HEADINGS) + + +def claims_canonical_state_update(text: str | None) -> bool: + """Return True when text claims a canonical state/next-action update.""" + return bool(_CLAIMS_STATE_UPDATE_RE.search(text or "")) + + +def _empty_or_placeholder(value: str | None) -> bool: + value = (value or "").strip().lower() + return not value or value in {"none", "n/a", "unknown", "tbd", "<...>"} + + +def _vague_next_action(value: str | None) -> bool: + normalized = re.sub(r"\s+", " ", (value or "").strip().lower()) + return normalized in VAGUE_NEXT_ACTIONS + + +def validate_canonical_state_comment(text: str | None) -> dict: + """Validate a canonical state comment or embedded final-report block. + + Returns a dict with ``valid`` and ``reasons``. The validator focuses on + fields that make continuation possible: current state, next actor, next + action, and paste-ready next prompt, plus a few contradiction checks. + """ + fields = extract_state_fields(text) + reasons: list[str] = [] + + for field in REQUIRED_FIELDS: + if _empty_or_placeholder(fields.get(field)): + reasons.append(f"missing required canonical state field: {field}") + + actor = (fields.get("WHO_IS_NEXT") or "").strip().lower() + if actor and actor not in ALLOWED_NEXT_ACTORS: + reasons.append( + "WHO_IS_NEXT must be one of: " + + ", ".join(sorted(ALLOWED_NEXT_ACTORS)) + ) + + if _vague_next_action(fields.get("NEXT_ACTION")): + reasons.append("NEXT_ACTION is too vague for durable continuation") + + state = (fields.get("STATE") or "").strip().lower().replace("-", "_") + if "ready_to_merge" in state or ( + state == "approved" and "pr" in (text or "").lower() + ): + review_status = (fields.get("REVIEW_STATUS") or "").lower() + head_sha = fields.get("HEAD_SHA") or "" + merge_ready = (fields.get("MERGE_READY") or "").lower() + if "approved" not in review_status: + reasons.append("ready-to-merge state requires approved REVIEW_STATUS") + if not _FULL_SHA_RE.search(head_sha): + reasons.append("ready-to-merge state requires full HEAD_SHA proof") + if merge_ready and not merge_ready.startswith(("yes", "true")): + reasons.append("ready-to-merge state contradicts MERGE_READY") + + if "superseded" in state: + canonical = fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_BY") + if _empty_or_placeholder(canonical): + reasons.append("superseded state requires canonical item proof") + + if "blocked" in state: + blockers = fields.get("BLOCKERS") or "" + if _empty_or_placeholder(blockers): + reasons.append("blocked state requires BLOCKERS/unblock condition") + + return { + "valid": not reasons, + "fields": fields, + "reasons": reasons, + } + + +def validate_final_report_state_update(report_text: str | None) -> dict: + """Validate canonical state-update claims inside a final report.""" + text = report_text or "" + if not claims_canonical_state_update(text) and not contains_canonical_state_block(text): + return { + "applicable": False, + "valid": True, + "reasons": [], + } + + if not contains_canonical_state_block(text): + return { + "applicable": True, + "valid": False, + "reasons": [ + "final report claims a canonical state update but includes no canonical state block" + ], + } + + result = validate_canonical_state_comment(text) + return { + "applicable": True, + "valid": result["valid"], + "fields": result["fields"], + "reasons": result["reasons"], + } diff --git a/docs/canonical-state-comments.md b/docs/canonical-state-comments.md new file mode 100644 index 0000000..555a91b --- /dev/null +++ b/docs/canonical-state-comments.md @@ -0,0 +1,183 @@ +# Canonical State Comments + +Gitea is the durable system of record for workflow continuation. When a +comment changes issue, PR, or discussion state, it should leave enough +information for the next role to continue without private chat history. + +Canonical comments answer: + +- what state the object is in +- who acts next +- what the next actor should do +- the exact prompt the next actor should run +- which proof, blocker, or dependency matters + +Non-workflow discussion comments do not need this template. + +## Issue State + +Use this when an issue becomes ready, blocked, in progress, PR-open, +superseded, merged, or otherwise changes workflow direction. + +```text +## Canonical Issue State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +RELATED_DISCUSSION: + + +RELATED_PRS: +- #... + +BRANCH: + + +HEAD_SHA: +<40-character SHA or none> + +VALIDATION: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +``` + +## PR State + +Use this when a PR needs review, receives changes requested, is approved, +is stale, is superseded, or becomes ready for merge. + +```text +## Canonical PR State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +ISSUE: +#... + +BASE: + + +HEAD: + + +HEAD_SHA: +<40-character SHA> + +REVIEW_STATUS: + + +VALIDATION: + + +BLOCKERS: + + +SUPERSEDES: + + +SUPERSEDED_BY: + + +MERGE_READY: + + +LAST_UPDATED_BY: + +``` + +## Discussion Summary + +Discussions should normally have at least five substantive comments before +conversion into issues. A controller may waive that only for tiny mechanical, +urgent, or explicitly trivial work. + +```text +## Canonical Discussion Summary + +STATE: + + +WHO_IS_NEXT: + + +DECISION: + + +WHY: + + +SUBSTANTIVE_COMMENTS: + + +ISSUES_TO_CREATE_OR_CREATED: +- #... + +DEPENDENCY_ORDER: + + +NON_GOALS: + + +OPEN_QUESTIONS: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +LAST_UPDATED_BY: + +``` + +## Validation Rules + +The final-report validator rejects canonical state update claims when the +report omits the canonical block or when the block lacks: + +- `STATE` +- `WHO_IS_NEXT` +- `NEXT_ACTION` +- `NEXT_PROMPT` + +It also rejects vague next actions such as `continue`, ready-to-merge states +without approval/head-SHA proof, superseded states without canonical item +proof, and blocked states without an unblock condition. diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index e0ff616..1c4f05d 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -7,6 +7,11 @@ package of the MCP Control Plane: creating issues, implementing them, opening and reviewing pull requests, merging, and closing out — safely and reproducibly. +Canonical state comments for durable issue/PR/discussion continuation are +documented in [`canonical-state-comments.md`](canonical-state-comments.md). +Use them when a workflow-changing comment needs to leave the next actor, next +action, and paste-ready prompt in Gitea. + > For the **project-agnostic** version of these operating rules (issue-first, > isolated worktrees, no self-review/merge, profile safety, cleanup, fail-closed) > that can be copied into any repository, see the reusable skill diff --git a/final_report_validator.py b/final_report_validator.py index 352a3ff..bf9baa4 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -1042,10 +1042,29 @@ def _rule_reviewer_review_mutation( ) +def _rule_shared_canonical_state_update(report_text: str) -> list[dict[str, str]]: + from canonical_state_comments import validate_final_report_state_update + + result = validate_final_report_state_update(report_text) + if not result.get("applicable") or result.get("valid"): + return [] + return [ + validator_finding( + "shared.canonical_state_update", + "block", + "Canonical state update", + reason, + "include a canonical state block with STATE, WHO_IS_NEXT, NEXT_ACTION, and NEXT_PROMPT", + ) + for reason in (result.get("reasons") or ["invalid canonical state update"]) + ] + + _SHARED_ISSUE_LOCK_RULES = ( _rule_shared_issue_lock_external_state, _rule_shared_manual_lock_pr_override, _rule_shared_author_reviewer_same_run, + _rule_shared_canonical_state_update, ) _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..956289b 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -167,6 +167,7 @@ Ready-to-copy task prompts live in [`templates/`](templates/): - [`reconcile-closed-not-merged-pr.md`](templates/reconcile-closed-not-merged-pr.md) - [`worktree-cleanup.md`](templates/worktree-cleanup.md) - [`release-tag.md`](templates/release-tag.md) +- [`canonical-state-comments.md`](templates/canonical-state-comments.md) ## Adapting to a project @@ -182,4 +183,4 @@ Ready-to-copy task prompts live in [`templates/`](templates/): Releases follow SemVer from remote `master` only, after full test suite passes. See [`templates/release-tag.md`](templates/release-tag.md) and -`scripts/release-tag`. \ No newline at end of file +`scripts/release-tag`. diff --git a/skills/llm-project-workflow/templates/canonical-state-comments.md b/skills/llm-project-workflow/templates/canonical-state-comments.md new file mode 100644 index 0000000..d6ec2fd --- /dev/null +++ b/skills/llm-project-workflow/templates/canonical-state-comments.md @@ -0,0 +1,148 @@ +# Template: canonical state comments + +Use these only for workflow-changing comments. Casual discussion does not need +the full template. + +## Issue + +```text +## Canonical Issue State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +RELATED_DISCUSSION: + + +RELATED_PRS: +- #... + +BRANCH: + + +HEAD_SHA: +<40-character SHA or none> + +VALIDATION: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +``` + +## PR + +```text +## Canonical PR State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +ISSUE: +#... + +BASE: + + +HEAD: + + +HEAD_SHA: +<40-character SHA> + +REVIEW_STATUS: + + +VALIDATION: + + +BLOCKERS: + + +SUPERSEDES: + + +SUPERSEDED_BY: + + +MERGE_READY: + + +LAST_UPDATED_BY: + +``` + +## Discussion + +```text +## Canonical Discussion Summary + +STATE: + + +WHO_IS_NEXT: + + +DECISION: + + +WHY: + + +SUBSTANTIVE_COMMENTS: + + +ISSUES_TO_CREATE_OR_CREATED: +- #... + +DEPENDENCY_ORDER: + + +NON_GOALS: + + +OPEN_QUESTIONS: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +LAST_UPDATED_BY: + +``` diff --git a/tests/test_canonical_state_comments.py b/tests/test_canonical_state_comments.py new file mode 100644 index 0000000..8f8c5ae --- /dev/null +++ b/tests/test_canonical_state_comments.py @@ -0,0 +1,143 @@ +"""Tests for canonical state comment validation (#495).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from canonical_state_comments import ( # noqa: E402 + validate_canonical_state_comment, + validate_final_report_state_update, +) + + +GOOD_ISSUE_STATE = """## Canonical Issue State + +STATE: +ready-for-author + +WHO_IS_NEXT: +author + +NEXT_ACTION: +Implement the focused validator and open a PR. + +NEXT_PROMPT: +Find issue #495, load the work-issue workflow, implement the validator, and open a PR. + +WHAT_HAPPENED: +Controller selected the canonical tracking issue. + +WHY: +The duplicate issue points here as canonical. + +RELATED_PRS: +none + +BRANCH: +none + +HEAD_SHA: +none + +VALIDATION: +none + +BLOCKERS: +none +""" + + +class TestCanonicalStateComments(unittest.TestCase): + def test_good_issue_state_validates(self): + result = validate_canonical_state_comment(GOOD_ISSUE_STATE) + self.assertTrue(result["valid"]) + self.assertEqual(result["reasons"], []) + + def test_missing_next_actor_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace("WHO_IS_NEXT:\nauthor", "WHO_IS_NEXT:\n") + ) + self.assertFalse(result["valid"]) + self.assertIn("WHO_IS_NEXT", " ".join(result["reasons"])) + + def test_missing_next_prompt_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace( + "NEXT_PROMPT:\nFind issue #495, load the work-issue workflow, implement the validator, and open a PR.", + "NEXT_PROMPT:\n", + ) + ) + self.assertFalse(result["valid"]) + self.assertIn("NEXT_PROMPT", " ".join(result["reasons"])) + + def test_vague_next_action_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace( + "NEXT_ACTION:\nImplement the focused validator and open a PR.", + "NEXT_ACTION:\ncontinue", + ) + ) + self.assertFalse(result["valid"]) + self.assertIn("too vague", " ".join(result["reasons"])) + + def test_ready_to_merge_requires_approval_and_head_sha(self): + text = """## Canonical PR State + +STATE: +ready-to-merge + +WHO_IS_NEXT: +merger + +NEXT_ACTION: +Merge PR #12 after checking the current head. + +NEXT_PROMPT: +Load the merge workflow and merge PR #12 if gates pass. + +ISSUE: +#11 + +HEAD_SHA: +abc123 + +REVIEW_STATUS: +none + +MERGE_READY: +yes +""" + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + reasons = " ".join(result["reasons"]) + self.assertIn("approved REVIEW_STATUS", reasons) + self.assertIn("full HEAD_SHA", reasons) + + def test_superseded_requires_canonical_item(self): + text = GOOD_ISSUE_STATE.replace("STATE:\nready-for-author", "STATE:\nsuperseded") + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + self.assertIn("canonical item", " ".join(result["reasons"])) + + def test_blocked_requires_unblock_condition(self): + text = GOOD_ISSUE_STATE.replace("STATE:\nready-for-author", "STATE:\nblocked") + text = text.replace("BLOCKERS:\nnone", "BLOCKERS:\n") + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + self.assertIn("BLOCKERS", " ".join(result["reasons"])) + + def test_final_report_claim_without_block_is_rejected(self): + report = "Posted canonical state comment on issue #495." + result = validate_final_report_state_update(report) + self.assertTrue(result["applicable"]) + self.assertFalse(result["valid"]) + + def test_final_report_without_state_claim_not_applicable(self): + result = validate_final_report_state_update("ordinary final report") + self.assertFalse(result["applicable"]) + self.assertTrue(result["valid"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 88c5b23..6c510e4 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -345,6 +345,92 @@ class TestReconciliationRules(unittest.TestCase): ) +class TestCanonicalStateUpdateRules(unittest.TestCase): + def test_claimed_state_comment_without_block_blocks(self): + report = _reconcile_handoff() + "\nPosted canonical state comment on PR #99." + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "shared.canonical_state_update" for f in result["findings"]) + ) + + def test_state_comment_missing_next_prompt_blocks(self): + report = ( + _reconcile_handoff() + + """ + +## Canonical PR State + +STATE: +needs-review + +WHO_IS_NEXT: +reviewer + +NEXT_ACTION: +Review PR #99. + +NEXT_PROMPT: + +ISSUE: +#98 + +HEAD_SHA: +0fdc8f582026b72a229d59a172c0a63ac4aaeaf9 + +REVIEW_STATUS: +none + +MERGE_READY: +no +""" + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + reasons = " ".join(f["reason"] for f in result["findings"]) + self.assertIn("NEXT_PROMPT", reasons) + + def test_valid_state_comment_claim_passes_shared_rule(self): + report = ( + _reconcile_handoff() + + """ + +## Canonical PR State + +STATE: +needs-review + +WHO_IS_NEXT: +reviewer + +NEXT_ACTION: +Review PR #99 against the pinned head. + +NEXT_PROMPT: +Load the review workflow and review PR #99. + +ISSUE: +#98 + +HEAD_SHA: +0fdc8f582026b72a229d59a172c0a63ac4aaeaf9 + +REVIEW_STATUS: +none + +MERGE_READY: +no + +BLOCKERS: +none +""" + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertFalse( + any(f["rule_id"] == "shared.canonical_state_update" for f in result["findings"]) + ) + + class TestCanonicalReconcileSchema(unittest.TestCase): """Issue #307: reconciliation workflows emit only the canonical schema."""