diff --git a/README.md b/README.md index e099658..5ebd8ac 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,7 @@ Any MCP-compatible agent (Antigravity, Claude Code, etc.) can call these tools n | `gitea_whoami` | Read-only: identify the authenticated Gitea account (safe metadata only) | | `gitea_get_profile` | Read-only: describe the active runtime execution profile (safe metadata only) | | `gitea_check_pr_eligibility` | Read-only: check if the current identity/profile may review/approve/request_changes/merge a PR | +| `gitea_assess_conflict_fix_classification` | Read-only: classify conflict-fix need from a live PR head re-fetch before creating a conflict-fix worktree | | `gitea_submit_pr_review` | Gated review mutation: comment/approve/request_changes, only after identity+profile+eligibility gates pass (no merge, no self-approval) | | `gitea_mark_issue` | Claim/release an issue (start/done) | | `gitea_list_labels` | List all available labels in a repository | diff --git a/canonical_comment_validator.py b/canonical_comment_validator.py new file mode 100644 index 0000000..a2d05a0 --- /dev/null +++ b/canonical_comment_validator.py @@ -0,0 +1,408 @@ +"""Fail-closed validation for workflow-changing Gitea comments (#496).""" + +from __future__ import annotations + +import re +from typing import Any + +VALID_ROLES = frozenset({ + "controller", + "author", + "reviewer", + "merger", + "reconciler", + "user", +}) + +_BASE_REQUIRED = ("STATE", "WHO_IS_NEXT", "NEXT_ACTION", "NEXT_PROMPT", "WHY") + +_ISSUE_REQUIRED = _BASE_REQUIRED + ("BLOCKERS", "VALIDATION") +_PR_REQUIRED = _BASE_REQUIRED + ( + "ISSUE", + "HEAD_SHA", + "REVIEW_STATUS", + "MERGE_READY", + "BLOCKERS", + "VALIDATION", +) +_SUPERSESSION_REQUIRED = _BASE_REQUIRED + ( + "CANONICAL_ITEM", + "SUPERSEDED_ITEM", + "CLOSE_OR_KEEP_OPEN", +) + +_MACHINE_MARKERS = ( + "", + "", + "", +) + +_WORKFLOW_TRIGGERS = re.compile( + r"\b(?:" + r"blocked|unblocked|ready(?:\s+for\s+(?:review|merge|author))?|" + r"ready-to-merge|approved|approve|request\s+changes|changes\s+requested|" + r"superseded|duplicate|canonical|next\s+action|next\s+actor|who\s+is\s+next|" + r"author\s+should|reviewer\s+should|merger\s+should|reconciler\s+should|" + r"controller\s+should|issue\s+complete|pr\s+open|pr\s+merged|close\s+this|" + r"do\s+not\s+merge|needs?\s+rebase|stale\s+approval|contaminated\s+review|" + r"merge\s+ready|ready\s+for\s+merge" + r")\b", + re.IGNORECASE, +) + +_CANONICAL_HEADINGS = ( + "## Canonical Issue State", + "## Canonical PR State", + "## Canonical Discussion Summary", +) + +_FIELD_RE = re.compile( + r"^([A-Z][A-Z0-9_]*)\s*:\s*(.*)$", + re.MULTILINE, +) + +_VAGUE_NEXT_ACTIONS = frozenset({ + "continue", + "handle this", + "fix it", + "follow up", + "follow-up", + "see above", + "see review", + "tbd", + "todo", + "as needed", + "proceed", + "next steps", + "will check", + "investigate", +}) + +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) +_SHORT_SHA_RE = re.compile(r"\b[0-9a-f]{7,40}\b", re.IGNORECASE) +_PR_REF_RE = re.compile(r"(?:PR\s*#|pull\s*#)\d+|\b#\d{2,}\b", re.IGNORECASE) +_APPROVAL_PROOF_RE = re.compile( + r"\b(?:approved|approval_at_current_head|APPROVE)\b", + re.IGNORECASE, +) +_UNBLOCK_RE = re.compile( + r"\b(?:unblock|until|after|once|when|requires?|must)\b", + re.IGNORECASE, +) + + +def _parse_fields(body: str) -> dict[str, str]: + """Parse KEY: value fields, including values continued on following lines.""" + fields: dict[str, str] = {} + current_key: str | None = None + current_lines: list[str] = [] + + def _flush() -> None: + nonlocal current_key, current_lines + if current_key is not None: + fields[current_key] = "\n".join(current_lines).strip() + current_key = None + current_lines = [] + + for line in (body or "").splitlines(): + if line.startswith("## "): + _flush() + continue + match = re.match(r"^([A-Z][A-Z0-9_]*)\s*:\s*(.*)$", line) + if match: + _flush() + current_key = match.group(1).strip().upper() + rest = match.group(2) + current_lines = [rest] if rest else [] + elif current_key is not None: + current_lines.append(line) + _flush() + return fields + + +def _is_machine_generated(body: str) -> bool: + text = body or "" + return any(marker in text for marker in _MACHINE_MARKERS) + + +def _has_canonical_heading(body: str) -> bool: + return any(h in (body or "") for h in _CANONICAL_HEADINGS) + + +def is_workflow_changing_comment(body: str) -> bool: + """True when comment text implies a workflow/state transition.""" + text = (body or "").strip() + if not text: + return False + if _is_machine_generated(text): + return False + if _has_canonical_heading(text): + return True + if _WORKFLOW_TRIGGERS.search(text): + return True + fields = _parse_fields(text) + if "STATE" in fields or "WHO_IS_NEXT" in fields: + return True + return False + + +def infer_comment_context(body: str, *, explicit: str | None = None) -> str: + if explicit: + return explicit + text = body or "" + if "## Canonical Discussion Summary" in text: + return "discussion_summary" + if "## Canonical PR State" in text: + return "pr_comment" + if "## Canonical Issue State" in text: + return "issue_comment" + fields = _parse_fields(text) + if fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_ITEM"): + return "supersession" + if any(k in fields for k in ("HEAD_SHA", "REVIEW_STATUS", "MERGE_READY", "ISSUE")): + return "pr_comment" + return "issue_comment" + + +def _required_fields_for_context(context: str) -> tuple[str, ...]: + if context in ("pr_comment", "pr_review"): + return _PR_REQUIRED + if context == "supersession": + return _SUPERSESSION_REQUIRED + if context == "discussion_summary": + return _BASE_REQUIRED + ("DECISION", "SUBSTANTIVE_COMMENTS") + return _ISSUE_REQUIRED + + +def _is_vague_next_action(value: str) -> bool: + normalized = re.sub(r"\s+", " ", (value or "").strip().lower()) + normalized = normalized.rstrip(".") + if not normalized: + return True + if normalized in _VAGUE_NEXT_ACTIONS: + return True + if len(normalized) < 12: + return True + return False + + +def _next_prompt_ok(value: str) -> bool: + text = (value or "").strip() + if len(text) < 40: + return False + if text.lower() in {"n/a", "none", "tbd", "todo"}: + return False + return True + + +def _state_value(fields: dict[str, str]) -> str: + return (fields.get("STATE") or "").strip().lower() + + +def _suggested_template(context: str) -> str: + if context == "pr_comment" or context == "pr_review": + return ( + "## Canonical PR State\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "```text\n\n```\n" + "WHAT_HAPPENED:\n" + "WHY:\n" + "ISSUE:\n" + "HEAD_SHA:\n" + "REVIEW_STATUS:\n" + "MERGE_READY:\n" + "BLOCKERS:\n" + "VALIDATION:\n" + "LAST_UPDATED_BY:\n" + ) + if context == "supersession": + return ( + "## Canonical PR State\n\n" + "STATE:\nsuperseded\n" + "WHO_IS_NEXT:\nreconciler\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "WHY:\n" + "CANONICAL_ITEM:\n" + "SUPERSEDED_ITEM:\n" + "CLOSE_OR_KEEP_OPEN:\n" + ) + if context == "discussion_summary": + return ( + "## Canonical Discussion Summary\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "DECISION:\n" + "WHY:\n" + "SUBSTANTIVE_COMMENTS:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + ) + return ( + "## Canonical Issue State\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "```text\n\n```\n" + "WHAT_HAPPENED:\n" + "WHY:\n" + "RELATED_PRS:\n" + "BLOCKERS:\n" + "VALIDATION:\n" + "LAST_UPDATED_BY:\n" + ) + + +def _build_correction_message( + *, + missing_fields: list[str], + vague_fields: list[str], + extra_reasons: list[str], + context: str, +) -> str: + parts = [ + "Canonical comment validation failed (fail closed before posting).", + ] + if missing_fields: + parts.append("Missing fields: " + ", ".join(missing_fields) + ".") + if vague_fields: + parts.append("Vague or invalid fields: " + ", ".join(vague_fields) + ".") + parts.extend(extra_reasons) + parts.append("Fill the suggested template and retry.") + parts.append("Suggested template:\n" + _suggested_template(context)) + return "\n".join(parts) + + +def assess_canonical_comment( + body: str, + *, + context: str | None = None, + force_workflow: bool = False, +) -> dict[str, Any]: + """Validate outgoing comment text before a Gitea mutation.""" + text = (body or "").strip() + ctx = infer_comment_context(text, explicit=context) + + if not text: + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + if _is_machine_generated(text): + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + workflow = force_workflow or is_workflow_changing_comment(text) + if not workflow: + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + fields = _parse_fields(text) + required = _required_fields_for_context(ctx) + missing = [name for name in required if not (fields.get(name) or "").strip()] + + vague: list[str] = [] + extra: list[str] = [] + + who = (fields.get("WHO_IS_NEXT") or "").strip().lower() + if who and who not in VALID_ROLES: + vague.append("WHO_IS_NEXT") + extra.append( + f"WHO_IS_NEXT must be one of: {', '.join(sorted(VALID_ROLES))}." + ) + + next_action = fields.get("NEXT_ACTION") or "" + if next_action and _is_vague_next_action(next_action): + vague.append("NEXT_ACTION") + + next_prompt = fields.get("NEXT_PROMPT") or "" + if not _next_prompt_ok(next_prompt): + if "NEXT_PROMPT" not in missing: + vague.append("NEXT_PROMPT") + + state = _state_value(fields) + blockers = (fields.get("BLOCKERS") or "").strip() + if "blocked" in state: + if not blockers or blockers.lower() in {"none", "n/a"}: + missing.append("BLOCKERS (unblock condition)") + elif not _UNBLOCK_RE.search(blockers): + vague.append("BLOCKERS") + extra.append("BLOCKED state requires an explicit unblock condition in BLOCKERS.") + + if "superseded" in state: + canon = (fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_BY") or "").strip() + superseded = (fields.get("SUPERSEDED_ITEM") or fields.get("SUPERSEDES") or "").strip() + if not canon and "CANONICAL_ITEM" not in missing: + missing.append("CANONICAL_ITEM") + if not superseded and "SUPERSEDED_ITEM" not in missing: + missing.append("SUPERSEDED_ITEM") + + if "ready-to-merge" in state or "ready to merge" in state: + head_sha = fields.get("HEAD_SHA") or "" + merge_ready = fields.get("MERGE_READY") or "" + review_status = fields.get("REVIEW_STATUS") or "" + validation = fields.get("VALIDATION") or "" + proof_blob = " ".join((head_sha, merge_ready, review_status, validation)) + has_sha = bool(_FULL_SHA_RE.search(proof_blob) or _SHORT_SHA_RE.search(proof_blob)) + has_approval = bool(_APPROVAL_PROOF_RE.search(proof_blob)) + if not has_sha or not has_approval: + extra.append( + "ready-to-merge STATE requires approval proof and HEAD_SHA in " + "REVIEW_STATUS, MERGE_READY, HEAD_SHA, or VALIDATION." + ) + + if ctx in ("pr_comment", "pr_review"): + head_sha = (fields.get("HEAD_SHA") or "").strip() + if not head_sha or not _SHORT_SHA_RE.search(head_sha): + if "HEAD_SHA" not in missing: + missing.append("HEAD_SHA") + + if ctx == "issue_comment" and _PR_REF_RE.search(text): + related = (fields.get("RELATED_PRS") or "").strip() + if not related or related.lower() in {"none", "n/a", "-"}: + missing.append("RELATED_PRS") + + allowed = not missing and not vague and not extra + correction = "" + if not allowed: + correction = _build_correction_message( + missing_fields=missing, + vague_fields=vague, + extra_reasons=extra, + context=ctx, + ) + + return { + "allowed": allowed, + "is_workflow_comment": True, + "context": ctx, + "missing_fields": missing, + "vague_fields": vague, + "extra_reasons": extra, + "correction_message": correction, + "suggested_template": _suggested_template(ctx) if not allowed else "", + } \ No newline at end of file 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/canonical_thread_handoff.py b/canonical_thread_handoff.py new file mode 100644 index 0000000..aa0a85b --- /dev/null +++ b/canonical_thread_handoff.py @@ -0,0 +1,213 @@ +"""Canonical Thread Handoff (CTH) protocol for issue and PR comments (#505). + +A CTH comment is the authoritative workflow handoff in a Gitea issue or PR +thread. It records current state, decisions, blockers, proof, and the exact +next prompt/action for the next LLM or person. +""" + +from __future__ import annotations + +import re +from datetime import datetime, timezone +from typing import Any + +MARKER = "" + +CTH_TERM = "Canonical Thread Handoff" +CTH_ABBREV = "CTH" + +CTH_TYPES = frozenset({ + "State Handoff", + "Controller Decision", + "Author Handoff", + "Reviewer Handoff", + "Merger Handoff", + "Supersession Notice", + "Blocker", +}) + +_REQUIRED_BASE_FIELDS = ( + "status", + "next owner", + "current blocker", + "decision", + "proof", + "next action", + "ready-to-paste prompt", +) + +_HEADING_RE = re.compile( + r"^##\s+CTH:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_FIELD_RE = re.compile( + r"^([A-Za-z][A-Za-z0-9 /-]*):\s*(.+?)\s*$", + re.MULTILINE, +) + + +def format_cth_body( + *, + cth_type: str, + status: str, + next_owner: str, + current_blocker: str = "none", + decision: str = "none", + proof: str = "none", + next_action: str = "none", + ready_to_paste_prompt: str = "none", + extra_fields: dict[str, str] | None = None, +) -> str: + """Render a canonical CTH comment body.""" + normalized_type = (cth_type or "").strip() + if normalized_type not in CTH_TYPES: + raise ValueError( + f"unknown CTH type '{cth_type}'; expected one of {sorted(CTH_TYPES)}" + ) + lines = [ + MARKER, + f"## CTH: {normalized_type}", + "", + f"Status: {(status or 'unknown').strip() or 'unknown'}", + f"Next owner: {(next_owner or 'unknown').strip() or 'unknown'}", + f"Current blocker: {(current_blocker or 'none').strip() or 'none'}", + f"Decision: {(decision or 'none').strip() or 'none'}", + f"Proof: {(proof or 'none').strip() or 'none'}", + f"Next action: {(next_action or 'none').strip() or 'none'}", + f"Ready-to-paste prompt: {(ready_to_paste_prompt or 'none').strip() or 'none'}", + ] + for key, value in (extra_fields or {}).items(): + label = (key or "").strip() + if not label: + continue + lines.append(f"{label}: {(value or 'none').strip() or 'none'}") + return "\n".join(lines) + + +def parse_cth_comment(body: str) -> dict[str, Any] | None: + """Parse one CTH comment body, or None when not a CTH comment.""" + text = body or "" + if MARKER not in text and not _HEADING_RE.search(text): + return None + heading = _HEADING_RE.search(text) + if not heading: + return None + cth_type = heading.group(1).strip() + fields: dict[str, str] = {} + for match in _FIELD_RE.finditer(text): + key = match.group(1).strip().lower() + if key.startswith("cth"): + continue + fields[key] = match.group(2).strip() + return { + "cth_type": cth_type, + "fields": fields, + "raw_body": text, + } + + +def assess_cth_comment(body: str) -> dict[str, Any]: + """Validate a CTH comment has required base fields and a known type.""" + parsed = parse_cth_comment(body) + reasons: list[str] = [] + if not parsed: + return { + "valid": False, + "block": True, + "reasons": ["comment is not a Canonical Thread Handoff (CTH)"], + "parsed": None, + } + + cth_type = parsed.get("cth_type") or "" + if cth_type not in CTH_TYPES: + reasons.append( + f"unknown CTH type '{cth_type}'; expected one of {sorted(CTH_TYPES)}" + ) + + fields = parsed.get("fields") or {} + missing = [name for name in _REQUIRED_BASE_FIELDS if not (fields.get(name) or "").strip()] + if missing: + reasons.append( + "CTH missing required fields: " + ", ".join(missing) + ) + + vague_prompt = (fields.get("ready-to-paste prompt") or "").strip().lower() + if vague_prompt in {"", "none", "tbd", "n/a", "todo"}: + reasons.append("ready-to-paste prompt must be concrete and paste-ready") + + return { + "valid": not reasons, + "block": bool(reasons), + "reasons": reasons, + "parsed": parsed, + "cth_type": cth_type, + } + + +def find_latest_cth(comments: list[dict[str, Any]]) -> dict[str, Any] | None: + """Return the newest valid CTH comment from a Gitea comment list.""" + latest: dict[str, Any] | None = None + latest_ts: datetime | None = None + for comment in comments or []: + body = comment.get("body") or "" + parsed = parse_cth_comment(body) + if not parsed: + continue + created = _parse_timestamp(comment.get("created_at")) + if latest is None or (created and (latest_ts is None or created >= latest_ts)): + latest = { + "comment_id": comment.get("id"), + "created_at": comment.get("created_at"), + "author": (comment.get("user") or {}).get("login"), + **parsed, + } + latest_ts = created + return latest + + +def assess_cth_supersedes_non_cth( + *, + latest_cth: dict[str, Any] | None, + narrative_claim: str, +) -> dict[str, Any]: + """Fail closed when narrative relies on stale non-CTH state despite newer CTH.""" + if not latest_cth: + return {"block": False, "reasons": []} + text = (narrative_claim or "").strip() + if not text: + return {"block": False, "reasons": []} + if parse_cth_comment(text): + return {"block": False, "reasons": []} + return { + "block": True, + "reasons": [ + "workflow narrative must treat the latest CTH comment as authoritative; " + f"found newer CTH type '{latest_cth.get('cth_type')}' " + f"(comment #{latest_cth.get('comment_id')})" + ], + } + + +def _parse_timestamp(value: str | None) -> datetime | None: + if not value: + return None + text = value.strip() + if text.endswith("Z"): + text = text[:-1] + "+00:00" + try: + parsed = datetime.fromisoformat(text) + except ValueError: + return None + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +EXAMPLE_SCENARIOS = ( + "pr_approved_ready_for_merger", + "pr_request_changes_to_author", + "duplicate_superseded_pr_closure", + "blocked_dirty_root_worktree", + "stale_head_requires_fresh_review", + "issue_implementation_handoff", +) \ No newline at end of file diff --git a/conflict_fix_classification.py b/conflict_fix_classification.py new file mode 100644 index 0000000..86815b5 --- /dev/null +++ b/conflict_fix_classification.py @@ -0,0 +1,266 @@ +"""Live PR head re-pin before conflict-fix classification (#522). + +Open PR inventory fields (mergeable, head_sha) can be stale. Author sessions +must re-fetch live PR state and pin the live head before classifying a PR as +conflicted or creating a conflict-fix worktree. +""" + +from __future__ import annotations + +import re +from typing import Any + +_FULL_SHA = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE) + +_INVENTORY_HEAD_RE = re.compile( + r"(?:inventory head sha|stale inventory head sha)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_LIVE_HEAD_RE = re.compile( + r"(?:live head sha|pinned head sha|live pr head sha)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_REPINS_TOOL_RE = re.compile( + r"gitea_assess_conflict_fix_classification", + re.IGNORECASE, +) +_CLASSIFICATION_RE = re.compile( + r"conflict[- ]fix classification\s*:\s*(\S+)", + re.IGNORECASE, +) + +CLASSIFICATION_STALE_INVENTORY_SKIP = "stale_inventory_skip" +CLASSIFICATION_CONFLICT_FIX_NEEDED = "conflict_fix_needed" +CLASSIFICATION_LIVE_MERGEABLE = "live_mergeable_skip" +CLASSIFICATION_INCOMPLETE = "incomplete_live_repin" + +_VALID_CLASSIFICATIONS = frozenset({ + CLASSIFICATION_STALE_INVENTORY_SKIP, + CLASSIFICATION_CONFLICT_FIX_NEEDED, + CLASSIFICATION_LIVE_MERGEABLE, + CLASSIFICATION_INCOMPLETE, +}) + + +def _normalize_sha(value: str | None) -> str | None: + text = (value or "").strip().lower() + if not text: + return None + return text if _FULL_SHA.match(text) else None + + +def assess_conflict_fix_classification( + *, + pr_number: int, + inventory_head_sha: str | None = None, + inventory_mergeable: bool | None = None, + live_head_sha: str | None = None, + live_mergeable: bool | None = None, +) -> dict[str, Any]: + """Classify whether conflict-fix work is justified from live PR state. + + Inventory fields are advisory only. Live head + live mergeable are required + before any conflict-fix worktree may be created. + """ + inv_head = _normalize_sha(inventory_head_sha) + live_head = _normalize_sha(live_head_sha) + reasons: list[str] = [] + + if not isinstance(pr_number, int) or pr_number <= 0: + return { + "classification": CLASSIFICATION_INCOMPLETE, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": False, + "inventory_stale_mergeable": False, + "pinned_head_sha": None, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": ["pr_number must be a positive integer (fail closed)"], + } + + if live_head is None: + reasons.append( + "live PR head SHA missing or not a full 40-char hex SHA; " + "re-fetch the PR before conflict-fix classification (fail closed)" + ) + if live_mergeable is None: + reasons.append( + "live PR mergeable value missing; re-fetch the PR before " + "conflict-fix classification (fail closed)" + ) + + if reasons: + return { + "classification": CLASSIFICATION_INCOMPLETE, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": bool( + inv_head and live_head and inv_head != live_head + ), + "inventory_stale_mergeable": ( + inventory_mergeable is not None + and live_mergeable is not None + and bool(inventory_mergeable) != bool(live_mergeable) + ), + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": reasons, + } + + inventory_stale_head = bool(inv_head and inv_head != live_head) + inventory_stale_mergeable = ( + inventory_mergeable is not None + and bool(inventory_mergeable) != bool(live_mergeable) + ) + + # Live mergeable wins: do not start conflict-fix work. + if live_mergeable is True: + classification = ( + CLASSIFICATION_STALE_INVENTORY_SKIP + if ( + inventory_mergeable is False + or inventory_stale_head + or inventory_stale_mergeable + ) + else CLASSIFICATION_LIVE_MERGEABLE + ) + note = [] + if inventory_stale_head: + note.append( + f"inventory head {inv_head} differs from live head {live_head}; " + "use live head only" + ) + if inventory_mergeable is False: + note.append( + "inventory reported mergeable:false but live PR is mergeable:true; " + "skip author conflict-fix mutation" + ) + return { + "classification": classification, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": inventory_stale_head, + "inventory_stale_mergeable": inventory_stale_mergeable, + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": note, + } + + # live_mergeable is False → conflict-fix may proceed on the pinned live head. + note = [] + if inventory_stale_head: + note.append( + f"inventory head {inv_head} differs from live head {live_head}; " + "pin and use live head only for conflict-fix work" + ) + if inventory_mergeable is True: + note.append( + "inventory reported mergeable:true but live PR is mergeable:false; " + "trust live mergeable and proceed only with live head pin" + ) + note.append( + f"live PR #{pr_number} is mergeable:false at pinned head {live_head}; " + "conflict-fix worktree allowed for that head only" + ) + return { + "classification": CLASSIFICATION_CONFLICT_FIX_NEEDED, + "worktree_allowed": True, + "skip_author_mutation": False, + "inventory_stale_head": inventory_stale_head, + "inventory_stale_mergeable": inventory_stale_mergeable, + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": note, + } + + +def assess_conflict_fix_classification_final_report( + report_text: str, + **_kwargs: Any, +) -> dict[str, Any]: + """Require live-head re-pin proof when a report claims conflict-fix work (#522).""" + text = report_text or "" + lower = text.lower() + mentions_conflict_fix = ( + "conflict-fix" in lower + or "conflict fix" in lower + or "conflict_fix" in lower + ) + if not mentions_conflict_fix: + return {"proven": True, "reasons": [], "applicable": False} + + reasons: list[str] = [] + if not _REPINS_TOOL_RE.search(text): + reasons.append( + "conflict-fix report must cite gitea_assess_conflict_fix_classification " + "(live head re-pin tool)" + ) + + live_match = _LIVE_HEAD_RE.search(text) + if not live_match: + reasons.append( + "conflict-fix report must state Live head SHA: <40-char hex> " + "(or Pinned head SHA / Live PR head SHA)" + ) + else: + live_sha = _normalize_sha(live_match.group(1)) + if live_sha is None: + reasons.append("live head SHA is not a full 40-char hex digest") + + inv_match = _INVENTORY_HEAD_RE.search(text) + # Inventory head is optional but recommended when classification is stale skip. + class_match = _CLASSIFICATION_RE.search(text) + if not class_match: + reasons.append( + "conflict-fix report must state Conflict-fix classification: " + f"<{'|'.join(sorted(_VALID_CLASSIFICATIONS))}>" + ) + else: + classification = class_match.group(1).strip().lower().replace(" ", "_") + # allow hyphenated forms + classification = classification.replace("-", "_") + if classification not in _VALID_CLASSIFICATIONS: + reasons.append( + f"unknown conflict-fix classification {class_match.group(1)!r}; " + f"expected one of {sorted(_VALID_CLASSIFICATIONS)}" + ) + + if inv_match and live_match: + inv_sha = _normalize_sha(inv_match.group(1)) + live_sha = _normalize_sha(live_match.group(1)) + if inv_sha and live_sha and inv_sha != live_sha: + # Require explicit note that live head was used. + if "use live head" not in lower and "live head only" not in lower: + reasons.append( + "inventory head differs from live head; report must state that " + "the live head was used exclusively" + ) + + return { + "proven": not reasons, + "reasons": reasons, + "applicable": True, + "inventory_head_sha": _normalize_sha(inv_match.group(1)) if inv_match else None, + "live_head_sha": _normalize_sha(live_match.group(1)) if live_match else None, + "classification": ( + class_match.group(1).strip().lower().replace("-", "_").replace(" ", "_") + if class_match + else None + ), + } diff --git a/create_issue.py b/create_issue.py index 6ec1b4b..5a93532 100644 --- a/create_issue.py +++ b/create_issue.py @@ -29,6 +29,7 @@ from gitea_auth import ( get_credentials, resolve_remote, add_remote_args, api_request, repo_api_url, ) +import issue_workflow_labels def main(argv=None): @@ -38,6 +39,16 @@ def main(argv=None): parser.add_argument("--body", default="", help="Issue body text.") parser.add_argument("--body-file", help="Read issue body from this file ('-' for stdin).") + parser.add_argument("--label", action="append", default=[], + help="Existing label name to apply; may be repeated.") + parser.add_argument("--type-label", + help="Issue type, e.g. feature or type:feature.") + parser.add_argument("--status-label", + help="Initial status, e.g. ready or status:ready.") + parser.add_argument("--discussion", action="store_true", + help="Apply type:discussion.") + parser.add_argument("--require-workflow-labels", action="store_true", + help="Fail unless one type:* and one status:* label are supplied.") args = parser.parse_args(argv) host, org, repo = resolve_remote(args) @@ -50,6 +61,24 @@ def main(argv=None): with open(args.body_file, "r", encoding="utf-8") as fh: body = fh.read() + requested_labels = issue_workflow_labels.labels_for_new_issue( + issue_type=args.type_label, + initial_status=args.status_label, + extra_labels=args.label, + discussion=args.discussion, + ) + label_assessment = issue_workflow_labels.assess_issue_labels( + requested_labels, + discussion=args.discussion, + ) + if args.require_workflow_labels and not label_assessment["valid"]: + print( + "Workflow label validation failed: " + + "; ".join(label_assessment["errors"]), + file=sys.stderr, + ) + return 1 + user, password = get_credentials(host) if not user or not password: print(f"Could not get credentials for {host} " @@ -59,11 +88,34 @@ def main(argv=None): import base64 auth = f"Basic {base64.b64encode(f'{user}:{password}'.encode()).decode()}" - url = f"{repo_api_url(host, org, repo)}/issues" + base = repo_api_url(host, org, repo) + url = f"{base}/issues" try: + label_ids = [] + if requested_labels: + existing = api_request("GET", f"{base}/labels", auth) + by_name = {lb["name"]: lb["id"] for lb in existing} + missing = [name for name in requested_labels if name not in by_name] + if missing: + print(f"Missing labels: {missing}", file=sys.stderr) + return 1 + label_ids = [by_name[name] for name in requested_labels] data = api_request("POST", url, auth, {"title": args.title, "body": body}) + if label_ids: + api_request( + "PUT", + f"{base}/issues/{data.get('number')}/labels", + auth, + {"labels": label_ids}, + ) print(f"Issue #{data.get('number')}: {data.get('html_url')}") + if not label_assessment["valid"]: + print( + "Workflow label recommendation: " + + "; ".join(label_assessment["errors"]), + file=sys.stderr, + ) return 0 except RuntimeError as e: print(f"Error: {e}", file=sys.stderr) 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/canonical-thread-handoff.md b/docs/canonical-thread-handoff.md new file mode 100644 index 0000000..0e45000 --- /dev/null +++ b/docs/canonical-thread-handoff.md @@ -0,0 +1,142 @@ +# Canonical Thread Handoff (CTH) + +**CTH** = **Canonical Thread Handoff** + +A CTH comment is the authoritative workflow handoff in a Gitea issue or PR +thread. It records current state, decisions, blockers, proof, and the exact +next prompt/action for the next LLM or person. + +CTH comments complement — but do not replace — formal Gitea review state. +A CTH may summarize an APPROVE or REQUEST_CHANGES decision, yet merge gates +still require the live Gitea review verdict. + +## CTH comment types + +- `CTH: State Handoff` +- `CTH: Controller Decision` +- `CTH: Author Handoff` +- `CTH: Reviewer Handoff` +- `CTH: Merger Handoff` +- `CTH: Supersession Notice` +- `CTH: Blocker` + +## Required base template + +```md +## CTH: + +Status: +Next owner: +Current blocker: +Decision: +Proof: +Next action: +Ready-to-paste prompt: +``` + +Extended fields may map to canonical issue/PR state templates from #495 when +that layer is available. Until then, keep the base fields complete. + +## Discovery and posting rules + +Before acting in an issue or PR thread: + +1. **Find the latest CTH comment** before doing work. +2. Treat the **latest valid CTH** as the current handoff state. +3. **Post a new CTH** when you finish, block, skip, supersede, request + changes, approve, or hand off. +4. Do **not** rely on stale non-CTH comments when a newer CTH exists. +5. Casual discussion comments do not need to be CTH comments. + +## Examples + +### PR approved and ready for merger + +```md +## CTH: Reviewer Handoff + +Status: approved_at_current_head +Next owner: merger +Current blocker: none +Decision: APPROVE recorded at head abc123... +Proof: gitea_submit_pr_review performed; visible verdict APPROVE +Next action: eligible merger merges PR #N with pinned expected_head_sha +Ready-to-paste prompt: Merge PR #N for issue #M if live head still abc123... and merge gates pass. +``` + +### PR request-changes back to author + +```md +## CTH: Reviewer Handoff + +Status: request_changes_at_current_head +Next owner: author +Current blocker: unresolved findings in validation report +Decision: REQUEST_CHANGES at head def456... +Proof: gitea_submit_pr_review performed; blocking review visible +Next action: author fixes findings and pushes; reviewer re-validates fresh head +Ready-to-paste prompt: Fix PR #N review findings, push to feat/issue-M-..., post Author Handoff CTH. +``` + +### Duplicate / superseded PR closure + +```md +## CTH: Supersession Notice + +Status: superseded +Next owner: controller +Current blocker: duplicate branch/PR work +Decision: close PR #N; continue on PR #M +Proof: duplicate gate linked open PR #M for issue #K +Next action: controller closes superseded PR and records canonical state +Ready-to-paste prompt: Close superseded PR #N; confirm PR #M remains canonical for issue #K. +``` + +### Blocked workflow due to dirty root/worktree + +```md +## CTH: Blocker + +Status: blocked_preflight +Next owner: operator +Current blocker: dirty control checkout / branches worktree mismatch +Decision: stop before mutation +Proof: verify_preflight_purity failed; workspace diagnostics attached +Next action: repair worktree or relaunch MCP from branches/ worktree +Ready-to-paste prompt: Repair dirty workspace, relaunch MCP from branches/review-prN, retry review. +``` + +### Stale head requiring fresh review + +```md +## CTH: Reviewer Handoff + +Status: stale_head +Next owner: reviewer +Current blocker: live head differs from pinned review head +Decision: prior approval not valid for current head +Proof: expected_head_sha mismatch at merge gate +Next action: re-run validation and post fresh review decision +Ready-to-paste prompt: Re-validate PR #N at live head, dry-run review gates, submit fresh verdict. +``` + +### Issue implementation handoff + +```md +## CTH: Author Handoff + +Status: implementation_complete_pending_review +Next owner: reviewer +Current blocker: none +Decision: PR #N ready for review at head fedcba... +Proof: tests passed in branches/issue-M-...; PR opened with Closes #M +Next action: reviewer acquires lease and validates PR #N +Ready-to-paste prompt: Review PR #N for issue #M; pin head fedcba... before mutations. +``` + +## Related + +- [`llm-workflow-runbooks.md`](llm-workflow-runbooks.md) +- [`../skills/llm-project-workflow/templates/canonical-thread-handoff.md`](../skills/llm-project-workflow/templates/canonical-thread-handoff.md) +- #495 — canonical next-action comment fields +- #496 — fail-closed validation for workflow-changing comments \ No newline at end of file diff --git a/docs/gitea-dual-namespace-deployment.md b/docs/gitea-dual-namespace-deployment.md index 6ee7e8e..f25f09f 100644 --- a/docs/gitea-dual-namespace-deployment.md +++ b/docs/gitea-dual-namespace-deployment.md @@ -22,9 +22,12 @@ launched with exactly one static execution profile: | Namespace (MCP server name) | Profile (role) | Typical use | |-----------------------------|----------------|-------------| | `gitea-author` | an author profile | implement issues, push branches, open PRs, comment | -| `gitea-reviewer` | a reviewer profile | review, approve/request changes, merge | +| `gitea-reviewer` | a reviewer profile | review, approve/request changes | +| `gitea-merger` | a merger profile | merge PRs after approval and verification | | `gitea-reconciler` | a reconciler profile | close already-landed open PRs after ancestry proof (#304 profile; #310 close tool) | +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + Properties: - **One process, one credential.** Each namespace authenticates as exactly @@ -106,6 +109,14 @@ syntax to the client): "GITEA_MCP_CONFIG": "", "GITEA_MCP_PROFILE": "" } + }, + "gitea-merger": { + "command": "/venv/bin/python3", + "args": ["/mcp_server.py"], + "env": { + "GITEA_MCP_CONFIG": "", + "GITEA_MCP_PROFILE": "" + } } } } diff --git a/docs/gitea-execution-profiles.md b/docs/gitea-execution-profiles.md index 3d9958f..970f5d4 100644 --- a/docs/gitea-execution-profiles.md +++ b/docs/gitea-execution-profiles.md @@ -311,7 +311,8 @@ To make Gitea MCP profile activation and runtime identity state explicit, the fo ### 2. Dual MCP Namespaces Recommendation For security-sensitive or high-risk tasks, the preferred safety model uses separate, isolated MCP server instances (namespaces/sessions) launched with static profiles: - `gitea-author`: Exposes tools configured with author permissions; cannot perform approvals or merges. -- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews and merges. +- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews. Review and merge are separate workflow roles. A reviewer approval is not merge authorization. +- `gitea-merger`: Exposes tools configured with merger permissions; used for PR merges. This layout maintains physical separation of credentials and prevents privilege escalation within a single session. This is the model accepted in #139; deployment details, rationale, and client setup live in diff --git a/docs/label-taxonomy.md b/docs/label-taxonomy.md index e3cf98b..d1c400d 100644 --- a/docs/label-taxonomy.md +++ b/docs/label-taxonomy.md @@ -1,34 +1,98 @@ # Label Taxonomy -This document catalogs the issue labels used for MCP workflows, including Jenkins and GlitchTip (observability). +This document defines the canonical issue labels used by MCP workflows. -> **Approval Required:** Do not create or apply new labels in `manage_labels.py` without explicit owner approval of this document. +Every issue should carry: -## Existing Labels +- one `type:*` label +- one `status:*` label -* **`jenkins`** - * Description: Jenkins integration - * Color: `d93f0b` - * Use: Used to mark issues, PRs, or tasks that involve the `jenkins-mcp` boundaries, CI/CD designs, or build failures. +Discussion-only issues must carry `type:discussion`. -* **`glitchtip`** - * Description: GlitchTip integration - * Color: `b60205` - * Use: Used to mark issues related to the `glitchtip-mcp` boundary and observability integration. +## Issue Type Labels -## Proposed / Missing Labels +| Label | Use | +| --- | --- | +| `type:bug` | Bug or defect | +| `type:feature` | Feature or enhancement | +| `type:process` | Process or policy work | +| `type:workflow` | Workflow automation or guidance | +| `type:guardrail` | Safety gate or guardrail | +| `type:docs` | Documentation work | +| `type:test` | Tests or test infrastructure | +| `type:discussion` | Discussion-only issue | +| `type:umbrella` | Umbrella or tracker issue | +| `type:cleanup` | Cleanup or hygiene work | -* **`observability`** - * Proposed Description: Observability, metrics, and monitoring tasks - * Proposed Color: `5319e7` - * Use: Broader than GlitchTip alone; covers logging, metrics, traces, and general observability pipeline improvements. +## Workflow Status Labels -* **`source:glitchtip`** - * Proposed Description: Issue filed automatically by GlitchTip orchestration - * Proposed Color: `b60205` - * Use: Applied automatically by the orchestrator when a GlitchTip error event is converted into a Gitea issue. +Only one `status:*` label should be active on an issue at a time. When an issue +moves forward, tooling must remove the old `status:*` label and apply the new +one. -* **`status:triage`** - * Proposed Description: Issue needs human or orchestrator triage - * Proposed Color: `fbca04` - * Use: Used for incoming issues (especially automated ones like `source:glitchtip`) that have not yet been evaluated for priority or resolution. +| Label | Use | +| --- | --- | +| `status:triage` | Issue needs triage | +| `status:ready` | Issue is ready for work | +| `status:claimed` | Issue is claimed | +| `status:in-progress` | Issue is being worked on | +| `status:blocked` | Issue is blocked | +| `status:needs-review` | Issue work needs review | +| `status:pr-open` | A linked PR is open | +| `status:approved` | Linked PR is approved | +| `status:merged` | Linked PR is merged | +| `status:reconcile` | Issue needs reconciliation | +| `status:done` | Issue workflow is complete | +| `status:duplicate` | Issue is a duplicate | +| `status:wontfix` | Issue will not be fixed | + +## Transition Rules + +Suggested lifecycle: + +1. New issue created: `status:triage` or `status:ready` +2. Issue selected by an author: `status:claimed` +3. Author starts work: `status:in-progress` +4. Work is blocked: `status:blocked` +5. PR opened: `status:pr-open` +6. PR approved: `status:approved` +7. PR merged but issue still needs closure/reconciliation: `status:reconcile` +8. Issue fully complete: `status:done` +9. Duplicate issue: `status:duplicate` +10. Won't-fix issue: `status:wontfix` + +The helper module `issue_workflow_labels.py` is the source of truth for the +canonical label specs and status transition replacement behavior. + +## Discussion Issues + +Discussion issues must be labeled `type:discussion`. + +A discussion issue should not be treated as implementation-ready unless it also +has a clear implementation status and next action. + +If a discussion produces implementation work, either: + +1. convert the discussion issue into an implementation issue by changing labels + and adding acceptance criteria, or +2. create child implementation issues and leave the discussion issue as + `type:discussion`. + +## Tooling + +- `manage_labels.py --create-labels` creates the canonical `type:*` and + `status:*` labels. +- `gitea_create_issue` recommends `type:*` and `status:*` labels when missing + and can apply supplied label names. +- `gitea_mark_issue(..., action="start")` replaces old `status:*` labels with + `status:in-progress`. +- `gitea_create_pr` fails closed before PR creation if `status:pr-open` cannot + be applied to the locked issue, then applies it after the PR is created. +- `gitea_set_issue_labels` accepts an explicit `worktree_path` so author + sessions can satisfy the branches-only mutation guard while changing labels. + +## Existing Non-Workflow Labels + +Existing non-workflow labels such as `mcp`, `workflow`, `labels`, `tracker`, +`jenkins`, `glitchtip`, `documentation`, and `testing` remain valid topical +labels. They do not replace the required `type:*` and `status:*` labels. diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 5f2e31a..a31487b 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 @@ -408,6 +413,13 @@ The guard blocks when the **control checkout** (repository root) is: `branches/...` directories are disposable role worktrees; the root checkout is the stable orchestration surface only. +## No direct-import mutation path (#558) + +Never `import gitea_mcp_server` or call `gitea_auth.get_auth_header` / +keychain fill from a raw shell to bypass MCP preflight. + +Use the official MCP daemon only. See [`docs/mcp-daemon-import-guard.md`](mcp-daemon-import-guard.md). + ## Bootstrap Review Path for self-hosted MCP fixes (#557) When a PR fixes Gitea-Tools MCP workflow code that the **live daemon** still @@ -634,19 +646,24 @@ loop and do **not** substitute WebFetch/Playwright/manual base64. - **Profile:** issue-manager or author (any profile allowed to create issues). - **Steps:** create the parent/roadmap issue; create child issues; apply the minimal label set; link children to the parent. +- **Labels:** new issues should carry one `type:*` label and one `status:*` + label. Discussion-only issues must carry `type:discussion`. See + [`label-taxonomy.md`](label-taxonomy.md). - **Prompt:** `Using the issue-manager profile, create issue "" with body <body>, then create child issues for <list> and link them to the parent.` ### Implement an issue and open a PR - **Profile:** author. -- **Steps:** claim the issue (`status:in-progress`); create an isolated branch - worktree from latest `master` under `branches/` (`feat/issue-<n>-...` / +- **Steps:** claim the issue (`status:in-progress`, replacing any old + `status:*` label); create an isolated branch worktree from latest `master` + under `branches/` (`feat/issue-<n>-...` / `fix/...` / `docs/...`); `cd` into that worktree; implement narrowly; add or update tests if behavior changes; run the full suite; commit with an - issue-linked message; open a PR to `master`. **Do not** review or merge your - own PR. Include an `LLM Handoff Metadata` block (with `LLM-Agent-SHA`) in - the PR body — see [`llm-agent-sha.md`](llm-agent-sha.md). + issue-linked message; open a PR to `master`; move the issue to + `status:pr-open`. **Do not** review or merge your own PR. Include an + `LLM Handoff Metadata` block (with `LLM-Agent-SHA`) in the PR body — see + [`llm-agent-sha.md`](llm-agent-sha.md). - **Prompt:** `Use an author profile to implement issue #N and open a PR to master. Do not self-review or self-merge.` @@ -687,10 +704,10 @@ loop and do **not** substitute WebFetch/Playwright/manual base64. ### Merge a PR - **Profile:** merger (allowed to merge; must **not** be the PR author). -- **Steps:** confirm eligibility; require explicit confirmation - (`MERGE PR <n>`); optionally pin head SHA / changed-file set; merge only when - Gitea reports the PR mergeable (branch-protection checks satisfied). No force, - no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). +- **Steps:** + - Merger workflow starts only after formal approval at current head. Reviewer workflow ends with review decision and separate merger handoff. + - Confirm eligibility; require explicit confirmation (`MERGE PR <n>`); optionally pin head SHA / changed-file set; merge only when Gitea reports the PR mergeable (branch-protection checks satisfied). No force, no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). + - Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - **Prompt:** `Use any eligible merger profile to merge PR #N if checks pass and it is mergeable. Confirm with "MERGE PR N". Do not force-merge.` @@ -779,7 +796,7 @@ an author. |---|---|---|---|---| | Review PR (`review_pr`) | reviewer (e.g. `sysadmin` / `prgs-reviewer`) | read, gated review verdicts | commits, pushes, file edits, author comments, merge without eligibility | active profile is an author profile — stop immediately; do **not** switch to author-side fixes unless the operator explicitly re-tasks | | Address PR change requests (`address_pr_change_requests`) | author (e.g. `jcwalker3` / `prgs-author`) | commit/push fixes to the PR branch, PR comment summarizing fixes | review verdicts, approve, request-changes, merge | active profile lacks branch push | -| Merge PR (`merge_pr`) | reviewer/merger | gated merge after eligibility + approval | merging own PR, merging without pinned head match | active profile is an author profile, or any merge gate fails | +| Merge PR (`merge_pr`) | merger (e.g. `sysadmin` / `prgs-merger`) | gated merge after eligibility + approval | merging own PR, merging without pinned head match, reviewing PRs | active profile is an author or reviewer-only profile, or any merge gate fails | | Comment on issue discussion (`comment_issue`) | any profile with `gitea.issue.comment` | issue thread comments | review verdicts, closing via comment | permission missing (`gitea.pr.comment` does **not** imply it) | | Comment on PR (`comment_pr`) | any profile with `gitea.pr.comment` | PR thread comments | review verdicts | permission missing | | Author implementation (`create_branch`/`push_branch`/`create_pr`) | author | branch, commit, push, open PR | self-review, self-merge | profile lacks the author permissions | @@ -829,6 +846,27 @@ Never imply full-suite success unless the full-suite command itself passed (`full_suite_passed: true`). A report that hides a failed or skipped check is worse than a failing report. +## Canonical Thread Handoff (CTH) + +**CTH** = **Canonical Thread Handoff** — the authoritative workflow handoff +comment in a Gitea issue or PR thread. See +[`canonical-thread-handoff.md`](canonical-thread-handoff.md) for types, +templates, and examples. + +Before acting in an issue or PR thread: + +1. **Find the latest CTH comment** before doing work. +2. Treat the **latest valid CTH** as the current handoff state. +3. **Post a new CTH** when you finish, block, skip, supersede, request + changes, approve, or hand off. +4. Do **not** rely on stale non-CTH comments when a newer CTH exists. + +A CTH summarizes workflow state for the next session. Formal Gitea review +verdicts remain authoritative for merge gates — a CTH is not merge approval +by itself. + +Template: [`../skills/llm-project-workflow/templates/canonical-thread-handoff.md`](../skills/llm-project-workflow/templates/canonical-thread-handoff.md) + ## Controller Handoff (required, every task) Every task — implementation, review, merge, triage, documentation, @@ -901,6 +939,55 @@ comments. Related (do not duplicate): #494/#495 lifecycle state, #501 mutation-ledger consistency, #505 CTH umbrella, #496 workflow comment gate when merged. +## Canonical comment validation (#496) + +Workflow-changing issue/PR/review comments must carry durable next-action +state. Casual discussion is still allowed. + +The MCP server runs `canonical_comment_validator.assess_canonical_comment` +**before** posting through: + +- `gitea_create_issue_comment` +- `gitea_submit_pr_review` / `gitea_dry_run_pr_review` (non-empty review bodies) +- `gitea_reconcile_already_landed_pr` when `post_comment=True` +- internal structured comment helpers (machine lease/heartbeat markers stay exempt) + +Detection examples: + +- **Allowed:** `Thanks, I will check this.` +- **Rejected:** `Blocked, author should fix.` (workflow trigger without canonical fields) + +When validation fails, the tool returns `canonical_comment_validation` with +`allowed: false`, `missing_fields`, `vague_fields`, `correction_message`, and +`suggested_template`. **No Gitea API call is made.** + +Minimum workflow comment fields: + +```text +STATE: +WHO_IS_NEXT: +NEXT_ACTION: +NEXT_PROMPT: +WHY: +``` + +`WHO_IS_NEXT` must be one of: `controller`, `author`, `reviewer`, `merger`, +`reconciler`, `user`. + +Issue comments also require `RELATED_PRS`, `BLOCKERS`, and `VALIDATION` when +they mention PR work. PR comments/reviews also require `ISSUE`, `HEAD_SHA`, +`REVIEW_STATUS`, `MERGE_READY`, `BLOCKERS`, and `VALIDATION`. + +Special states: + +- `STATE: blocked` — `BLOCKERS` must name an explicit unblock condition. +- `STATE: superseded` — requires `CANONICAL_ITEM` and `SUPERSEDED_ITEM`. +- `STATE: ready-to-merge` — requires approval proof and head SHA in + `HEAD_SHA`, `REVIEW_STATUS`, `MERGE_READY`, or `VALIDATION`. + +Final reports must not claim a comment was posted when +`canonical_comment_validation.allowed` is false (#496 AC14). + ## Fail-closed behavior Before any mutating action the workflow verifies identity, active profile, @@ -1012,6 +1099,7 @@ When posting a Canonical Thread Handoff after a binding blocker: ## Related documents +- [`reviewer-handoff-consistency.md`](reviewer-handoff-consistency.md) — reject contradictory reviewer handoffs (#501). - [`issue-acceptance-gate.md`](issue-acceptance-gate.md) — controller issue-acceptance audit after PR merge (#500). - [`../skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md) — portable cross-project LLM workflow skill. - [`gitea-execution-profiles.md`](gitea-execution-profiles.md) — the profile model. diff --git a/docs/mcp-daemon-import-guard.md b/docs/mcp-daemon-import-guard.md new file mode 100644 index 0000000..8378dbd --- /dev/null +++ b/docs/mcp-daemon-import-guard.md @@ -0,0 +1,23 @@ +# MCP daemon import and keychain guard (#558) + +## Problem + +During deadlock debugging, agents imported `gitea_mcp_server` / ran credential +helpers from a raw shell, bypassing preflight purity and role gates. + +## Rule + +Mutation auth and keychain fill require a **sanctioned MCP daemon** process. + +| Context | Allowed | +|---------|---------| +| Official MCP entrypoint (`mcp_server.py` / `gitea_mcp_server` `__main__`) sets `GITEA_MCP_SANCTIONED_DAEMON=1` | yes | +| pytest | yes | +| `GITEA_ALLOW_DIRECT_MCP_IMPORT=1` (operator/tests only) | yes | +| bare `python -c 'import gitea_auth; get_auth_header(...)'` | **no** | +| keychain fill without daemon | **no** unless `GITEA_ALLOW_KEYCHAIN_CLI=1` | + +## Operator note + +LLM sessions must never set the allow-direct-import or allow-keychain-cli +overrides. Those are human-only escape hatches. diff --git a/docs/reviewer-handoff-consistency.md b/docs/reviewer-handoff-consistency.md new file mode 100644 index 0000000..27d8108 --- /dev/null +++ b/docs/reviewer-handoff-consistency.md @@ -0,0 +1,35 @@ +# Reviewer Handoff Consistency + +Reviewer and final-review controller handoffs must not contradict themselves. +A narrative that says a merge happened, a review was blocked, or a terminal +mutation budget was consumed must match the mutation ledger fields in the same +handoff. + +## What gets validated + +`reviewer_handoff_consistency.assess_reviewer_handoff_consistency()` checks: + +- merge claims appear under `Merge mutations` or `MCP/Gitea mutations` +- terminal-mutation-budget claims name the exact prior mutation in the ledger +- blocked review submission is not paired with "final decision marked" +- reviewer lease acquisition includes a `Review decision` +- blocked/rejected mutations include proof fields: + - tool called + - mutation attempted + - mutation rejected + - no server-side state changed + +`final_report_validator` applies this as `reviewer.handoff_consistency` on +`review_pr` reports and fails closed. + +## Blocked review template + +When `gitea_submit_pr_review` fails closed, use +`reviewer_handoff_consistency.render_blocked_review_handoff_template()` or the +copy in +[`skills/llm-project-workflow/templates/blocked-review-handoff.md`](../skills/llm-project-workflow/templates/blocked-review-handoff.md). + +## Related + +- #331 — file-mutation ledger alignment +- #501 — contradictory narrative vs ledger detection \ No newline at end of file diff --git a/docs/webui-local-dev.md b/docs/webui-local-dev.md index 96225b6..23d4c56 100644 --- a/docs/webui-local-dev.md +++ b/docs/webui-local-dev.md @@ -43,7 +43,8 @@ Optional environment variables: | `/api/projects` | JSON registry export | | `/prompts` | Prompt library with per-prompt copy buttons (#428) | | `/api/prompts` | JSON prompt export with workflow hashes | -| `/runtime` | Stub — MCP runtime health (#430) | +| `/runtime` | MCP runtime health and stale detection (#430) | +| `/api/runtime` | JSON runtime health export | | `/audit` | Stub — report audit paste (#431) | | `/worktrees` | Stub — hygiene dashboard (#432) | | `/leases` | Lease and collision visibility (#433) | @@ -91,8 +92,17 @@ in-progress claim inventory (#268), reviewer PR lease comments when present and duplicate local branches per issue. Links to collision-history backend issues (#267, #268, #400, #407) are included. No lease acquire/release from UI. +## Runtime health (#430) + +`/runtime` surfaces read-only MCP/runtime diagnostics for the default registry +project: active profile and role kind, authenticated identity (when credentials +are available), config model/mode, local vs remote `master` SHA sync, shell +health, workflow/schema SHA-256 hashes, and stale-runtime warnings when the +checkout is behind merged safety-gate changes. Restart guidance links to #420; +no tokens or MCP restart actions are exposed. + ## Tests ```bash -pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py tests/test_webui_lease_visibility.py -q +pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py tests/test_webui_lease_visibility.py tests/test_webui_runtime_health.py -q ``` \ No newline at end of file diff --git a/docs/wiki/Identity-and-Profiles.md b/docs/wiki/Identity-and-Profiles.md index 763a3bb..fb6000d 100644 --- a/docs/wiki/Identity-and-Profiles.md +++ b/docs/wiki/Identity-and-Profiles.md @@ -16,12 +16,21 @@ bind an authenticated Gitea identity to an allowed operation set. - **Allowed:** branch create/push, PR create, issue comment/create/close, repo commit, read. - **Forbidden:** PR approve, merge, request_changes. -### Reviewer / merger +### Reviewer - **Profile:** `prgs-reviewer` - **Typical identity:** `sysadmin` -- **Allowed:** PR review/approve/merge/request_changes, issue comment, read. -- **Forbidden:** branch push, PR create, repo commit. +- **Allowed:** PR review/approve/request_changes, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR merge. + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + +### Merger + +- **Profile:** `prgs-merger` +- **Typical identity:** `sysadmin` +- **Allowed:** PR merge, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR approve, PR review, PR request_changes. ## Configuration diff --git a/final_report_validator.py b/final_report_validator.py index a11aa10..a16b4a0 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -13,6 +13,7 @@ from typing import Any, Callable import issue_acceptance_gate import issue_lock_provenance +import reviewer_handoff_consistency import thread_state_ledger_validator from mcp_native_cleanup_proof import assess_mcp_native_cleanup_proof from post_merge_cleanup_proof import assess_post_merge_cleanup_proof @@ -30,6 +31,7 @@ from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ "review_pr", + "merge_pr", "reconcile_already_landed", "author_issue", "work_issue", @@ -41,6 +43,8 @@ FINAL_REPORT_TASK_KINDS = frozenset({ _TASK_KIND_ALIASES = { "review": "review_pr", "review-merge-pr": "review_pr", + "merge": "merge_pr", + "merge_pr": "merge_pr", "reconcile-landed-pr": "reconcile_already_landed", "reconcile_already_landed": "reconcile_already_landed", "work-issue": "work_issue", @@ -49,6 +53,7 @@ _TASK_KIND_ALIASES = { _HANDOFF_ROLE_BY_TASK = { "review_pr": "review", + "merge_pr": "merger", "reconcile_already_landed": None, "author_issue": "author", "work_issue": "author", @@ -196,6 +201,23 @@ _PAGINATION_PROOF_RE = re.compile( re.IGNORECASE, ) _GIT_FETCH_RE = re.compile(r"\bgit\s+fetch\b", re.IGNORECASE) +_CANONICAL_VALIDATION_REJECTED_RE = re.compile( + r"canonical comment validation failed|" + r"canonical_comment_validation|" + r'"allowed"\s*:\s*false', + re.IGNORECASE, +) +_COMMENT_POSTED_CLAIM_RE = re.compile( + r"(?:issue comments posted\s*:\s+(?!none\b)\S|" + r"pr comments posted\s*:\s+(?!none\b)\S|" + r"comment_id\s*[:=]\s*\d+|" + r"gitea comment (?:was )?posted|" + r"posted (?:issue|pr|canonical) (?:state )?comment|" + r"comment posted successfully|" + r"mcp/gitea mutations\s*:\s*[^;\n]*comment posted|" + r"reconciliation mutations\s*:\s*[^;\n]*comment posted)", + re.IGNORECASE, +) _READONLY_DIAG_RE = re.compile( r"read[- ]only diagnostics\s*:\s*(.+)$", re.IGNORECASE | re.MULTILINE, @@ -382,6 +404,43 @@ def _rule_shared_email_disclosure(report_text: str) -> list[dict[str, str]]: ) +def _rule_shared_canonical_comment_post_claim( + report_text: str, + *, + action_log: list[dict] | None = None, +) -> list[dict[str, str]]: + """#496: reports must not claim comment posted when validator rejected.""" + text = report_text or "" + rejected_in_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text)) + rejected_in_log = False + if action_log: + for entry in action_log: + validation = entry.get("canonical_comment_validation") or {} + if validation.get("allowed") is False: + rejected_in_log = True + break + result = entry.get("result") or {} + nested = result.get("canonical_comment_validation") or {} + if nested.get("allowed") is False: + rejected_in_log = True + break + if not (rejected_in_report or rejected_in_log): + return [] + if not _COMMENT_POSTED_CLAIM_RE.search(text): + return [] + return [ + validator_finding( + "shared.canonical_comment_post_claim", + "block", + "MCP/Gitea mutations", + "report claims a Gitea comment was posted while canonical comment " + "validation rejected the workflow comment", + "do not claim comment posted when validator fail-closed; repair " + "the canonical comment body and retry posting", + ) + ] + + def _rule_reviewer_legacy_workspace_mutations( report_text: str, *, @@ -599,6 +658,27 @@ def _rule_reviewer_stale_head_proof(report_text: str) -> list[dict[str, str]]: ) +def _rule_conflict_fix_classification_proof(report_text: str) -> list[dict[str, str]]: + from conflict_fix_classification import ( + assess_conflict_fix_classification_final_report, + ) + + text = report_text or "" + result = assess_conflict_fix_classification_final_report(text) + if result.get("proven"): + return [] + return _findings_from_reasons( + "author.conflict_fix_classification_proof", + result.get("reasons") or [], + field="Conflict-fix classification", + severity="block", + safe_next_action=( + "call gitea_assess_conflict_fix_classification, state the live head " + "SHA, and state the classification before creating a conflict-fix worktree" + ), + ) + + def _rule_conflict_fix_push_proof(report_text: str) -> list[dict[str, str]]: from pr_work_lease import assess_conflict_fix_final_report @@ -1112,6 +1192,25 @@ def _rule_reviewer_validation_structured( ) +def _rule_reviewer_handoff_consistency(report_text: str) -> list[dict[str, str]]: + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency( + report_text + ) + if result.get("proven"): + return [] + return _findings_from_reasons( + "reviewer.handoff_consistency", + result.get("reasons") or [], + field="Review mutations", + severity="block", + safe_next_action=( + "rewrite reviewer handoff so narrative claims match the mutation " + "ledger; use the blocked-review handoff template when submission " + "was rejected" + ), + ) + + def _rule_reviewer_mutation_ledger( report_text: str, *, @@ -1281,6 +1380,26 @@ def _rule_reviewer_review_mutation( def _rule_shared_two_comment_workflow(report_text: str) -> list[dict[str, str]]: """#507: tagged Controller Handoff must pair with Thread State Ledger.""" return thread_state_ledger_validator.findings_for_final_report(report_text) + + +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"]) + ] + + def _rule_reviewer_mutation_capability_proof(report_text: str) -> list[dict[str, str]]: from reviewer_mutation_capability_proof import assess_mutation_capability_proof @@ -1329,6 +1448,7 @@ _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, ) _SHARED_TWO_COMMENT_RULES = ( @@ -1338,12 +1458,17 @@ _SHARED_CLEANUP_PROOF_RULES = ( _rule_shared_mcp_native_cleanup_proof, ) +_SHARED_CANONICAL_COMMENT_RULES = ( + _rule_shared_canonical_comment_post_claim, +) + _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "review_pr": [ _rule_shared_controller_handoff, _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_legacy_workspace_mutations, _rule_reviewer_vague_mutations_none, @@ -1362,6 +1487,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_already_landed_eligible, _rule_reviewer_already_landed_state, _rule_reviewer_target_branch_freshness, + _rule_reviewer_handoff_consistency, _rule_reviewer_workflow_load_boundary, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, @@ -1370,11 +1496,23 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { *_SHARED_CLEANUP_PROOF_RULES, _rule_reviewer_stale_head_proof, ], + "merge_pr": [ + _rule_shared_controller_handoff, + _rule_shared_email_disclosure, + *_SHARED_ISSUE_LOCK_RULES, + _rule_reviewer_legacy_workspace_mutations, + _rule_reviewer_vague_mutations_none, + _rule_reviewer_mutation_categories, + _rule_reviewer_git_fetch_readonly, + _rule_reviewer_linked_issue, + _rule_reviewer_stale_head_proof, + ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, *_SHARED_CLEANUP_PROOF_RULES, _rule_reconcile_stale_author_fields, @@ -1393,6 +1531,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_vague_mutations_none, ], @@ -1401,9 +1540,11 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_shared_issue_acceptance_gate, _rule_reviewer_vague_mutations_none, + _rule_conflict_fix_classification_proof, _rule_conflict_fix_push_proof, _rule_worktree_cleanup_audit_proof, ], @@ -1412,6 +1553,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, ], "inventory": [ @@ -1419,6 +1561,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reconcile_pagination_proof, ], @@ -1427,6 +1570,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_TWO_COMMENT_RULES, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, ], } diff --git a/gitea-mcp.v2-contexts.example.json b/gitea-mcp.v2-contexts.example.json index 4bdad56..bd5b1ee 100644 --- a/gitea-mcp.v2-contexts.example.json +++ b/gitea-mcp.v2-contexts.example.json @@ -52,8 +52,19 @@ "execution_profile": "example-reviewer", "audit_label": "example-reviewer", "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, - "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes", "merge"], - "forbidden_operations": ["branch", "commit", "push", "open_pr"] + "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "merge"] + }, + "example-merger": { + "enabled": true, + "context": "example-context", + "role": "merger", + "username": "reviewer-user", + "execution_profile": "example-merger", + "audit_label": "example-merger", + "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, + "allowed_operations": ["read", "comment", "issue.comment", "merge"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "approve", "review", "request_changes"] } }, "projects": { @@ -63,7 +74,8 @@ "default_owner": "Example-Org", "default_repo": "Example-Repo", "default_author_profile": "example-author", - "default_reviewer_profile": "example-reviewer" + "default_reviewer_profile": "example-reviewer", + "default_merger_profile": "example-merger" } }, "rules": { diff --git a/gitea_auth.py b/gitea_auth.py index 5e30213..167357b 100644 --- a/gitea_auth.py +++ b/gitea_auth.py @@ -104,6 +104,10 @@ def get_credentials(host): # 3. Optional fallback to macOS Keychain via git credential fill if not user and not password and os.environ.get("GITEA_USE_KEYCHAIN") == "1": + # #558: block raw keychain dumps outside the sanctioned MCP daemon. + import mcp_daemon_guard + + mcp_daemon_guard.assert_keychain_access_allowed() cmd_parts = ["git", "creden" + "tial", "fi" + "ll"] try: p = subprocess.Popen( @@ -116,6 +120,8 @@ def get_credentials(host): user = line.split("=", 1)[1] elif line.startswith("password="): password = line.split("=", 1)[1] + except mcp_daemon_guard.UnsanctionedRuntimeError: + raise except Exception: pass @@ -124,6 +130,11 @@ def get_credentials(host): def get_auth_header(host): """Return an ``Authorization`` header value for *host*.""" + # #558: resolving credentials for API mutation must not happen via ad-hoc + # direct imports that bypass the MCP daemon preflight wall. + import mcp_daemon_guard + + mcp_daemon_guard.assert_sanctioned_mutation_runtime("get_auth_header") host_key = host.lower().strip() # 1. Try Token-based auth from dynamic configs diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 22804eb..f818eb0 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -137,13 +137,13 @@ def verify_mutation_authority(remote: str | None, host: str | None = None, ) # Reviewer/author role pivot boundary: only an authorized pivot - # (recorded by gitea_activate_profile) may cross author → reviewer. - if (required_role == "reviewer" + # (recorded by gitea_activate_profile) may cross author -> reviewer/merger. + if (required_role in ("reviewer", "merger") and "author" in str(data.get("initial_profile")).lower() - and "reviewer" in str(active_profile).lower()): + and ("reviewer" in str(active_profile).lower() or "merger" in str(active_profile).lower())): if not data.get("role_pivot_authorized"): raise RuntimeError( - "Attempted reviewer mutation from author session without " + f"Attempted {required_role} mutation from author session without " "authorized role pivot (fail closed)" ) @@ -562,7 +562,7 @@ def _enforce_branches_only_author_mutation(worktree_path: str | None = None) -> Reviewer, merger, and reconciler roles are exempt: reconciler ``close_pr`` is a Gitea metadata mutation and must not require ``GITEA_AUTHOR_WORKTREE`` - (#468). Non-author namespaces use dedicated workspace env vars (#510). + (#468, #483). Non-author namespaces use dedicated workspace env vars (#510). The exemption honours BOTH the effective workspace role and the actual profile role (#540). ``comment_issue`` preflight stamps @@ -818,6 +818,7 @@ import root_checkout_guard # noqa: E402 import remote_repo_guard # noqa: E402 import issue_claim_heartbeat # noqa: E402 import issue_work_duplicate_gate # noqa: E402 +import issue_workflow_labels # noqa: E402 import reviewer_pr_lease # noqa: E402 import merger_lease_adoption # noqa: E402 import merged_cleanup_reconcile # noqa: E402 @@ -827,6 +828,7 @@ import reconciliation_workflow # noqa: E402 import audit_reconciliation_mode # noqa: E402 import review_merge_state_machine # noqa: E402 import pr_work_lease # noqa: E402 +import conflict_fix_classification # noqa: E402 import native_mcp_preference # noqa: E402 import thread_state_ledger_validator # noqa: E402 import master_parity_gate # noqa: E402 @@ -837,6 +839,7 @@ import master_parity_gate # noqa: E402 # Read-only operations are never blocked by staleness. _STARTUP_PARITY = master_parity_gate.capture_startup_parity(PROJECT_ROOT) import worktree_cleanup_audit # noqa: E402 +import canonical_comment_validator as ccv # noqa: E402 # Keyed issue-lock storage (#443): per remote/org/repo/issue files under @@ -1176,6 +1179,108 @@ def extract_linked_issue_numbers(text: str | None, branch_name: str | None = Non issues.update(int(m) for m in pattern.findall(branch_name)) return sorted(list(issues)) +def _repo_label_id_map(base: str, auth: str) -> dict[str, int]: + labels = api_get_all(f"{base}/labels", auth) or [] + return { + str(lb["name"]): int(lb["id"]) + for lb in labels + if isinstance(lb, dict) and lb.get("name") and lb.get("id") is not None + } + +def _issue_label_names(base: str, auth: str, issue_number: int) -> list[str]: + issue = api_request("GET", f"{base}/issues/{issue_number}", auth) or {} + return issue_workflow_labels.label_names(issue) + +def _put_issue_label_names( + *, + base: str, + auth: str, + issue_number: int, + names: list[str], + label_ids_by_name: dict[str, int] | None = None, +) -> list[dict]: + by_name = label_ids_by_name or _repo_label_id_map(base, auth) + missing = [name for name in names if name not in by_name] + if missing: + raise RuntimeError( + f"The following labels do not exist on the repository: {missing}. " + "Create the canonical workflow labels first." + ) + ids = [by_name[name] for name in names] + return api_request( + "PUT", + f"{base}/issues/{issue_number}/labels", + auth, + {"labels": ids}, + ) + +def _transition_issue_status( + *, + base: str, + auth: str, + issue_number: int, + status: str, + label_ids_by_name: dict[str, int] | None = None, +) -> dict: + by_name = label_ids_by_name or _repo_label_id_map(base, auth) + target_status = issue_workflow_labels.canonical_status_label(status) + if target_status not in by_name: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "reasons": [ + f"required workflow status label '{target_status}' does not exist" + ], + } + current = _issue_label_names(base, auth, issue_number) + updated = issue_workflow_labels.transition_status_labels(current, target_status) + _put_issue_label_names( + base=base, + auth=auth, + issue_number=issue_number, + names=updated, + label_ids_by_name=by_name, + ) + return { + "success": True, + "performed": True, + "issue_number": issue_number, + "target_status": target_status, + "labels": updated, + } + +def _prepare_issue_status_transition( + *, + base: str, + auth: str, + issue_number: int, + status: str, +) -> dict: + by_name = _repo_label_id_map(base, auth) + target_status = issue_workflow_labels.canonical_status_label(status) + if target_status not in by_name: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "reasons": [ + f"required workflow status label '{target_status}' does not exist" + ], + } + current = _issue_label_names(base, auth, issue_number) + updated = issue_workflow_labels.transition_status_labels(current, target_status) + return { + "success": True, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "labels": updated, + "label_ids_by_name": by_name, + } + def release_in_progress_label(issue_numbers: list[int], remote: str, host: str | None, org: str | None, repo: str | None) -> dict: if not issue_numbers: return {} @@ -1508,6 +1613,11 @@ def gitea_create_issue( host: str | None = None, org: str | None = None, repo: str | None = None, + labels: list[str] | None = None, + issue_type: str | None = None, + initial_status: str | None = None, + discussion: bool = False, + require_workflow_labels: bool = False, allow_duplicate_override: bool = False, split_from_issue: int | None = None, worktree_path: str | None = None, @@ -1521,6 +1631,12 @@ def gitea_create_issue( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + labels: Optional labels to apply by name after creating the issue. + issue_type: Optional canonical type, e.g. 'feature' or 'type:feature'. + initial_status: Optional initial workflow status, e.g. 'ready'. + discussion: If True, require/apply type:discussion. + require_workflow_labels: If True, fail closed unless labels include one + type:* and one status:* label. allow_duplicate_override: Operator-approved split after duplicate found. split_from_issue: Existing duplicate issue number when overriding. worktree_path: Optional path to verify branches-only guard. @@ -1552,6 +1668,40 @@ def gitea_create_issue( return blocked verify_preflight_purity(remote, worktree_path=worktree_path, task="create_issue") base = repo_api_url(h, o, r) + requested_labels = issue_workflow_labels.labels_for_new_issue( + issue_type=issue_type, + initial_status=initial_status, + extra_labels=labels, + discussion=discussion, + ) + label_assessment = issue_workflow_labels.assess_issue_labels( + requested_labels, + discussion=discussion, + require_type=True, + require_status=True, + ) + if require_workflow_labels and not label_assessment["valid"]: + return { + "success": False, + "performed": False, + "number": None, + "workflow_label_validation": label_assessment, + "reasons": label_assessment["errors"], + } + label_ids_by_name: dict[str, int] | None = None + if requested_labels: + label_ids_by_name = _repo_label_id_map(base, auth) + missing = [name for name in requested_labels if name not in label_ids_by_name] + if missing: + return { + "success": False, + "performed": False, + "number": None, + "workflow_label_validation": label_assessment, + "reasons": [ + f"requested labels do not exist on the repository: {missing}" + ], + } open_issues = api_get_all(f"{base}/issues?state=open&type=issues", auth) closed_issues = api_get_all( f"{base}/issues?state=closed&type=issues", auth, limit=100 @@ -1586,7 +1736,29 @@ def gitea_create_issue( _audit("create_issue", host=h, remote=remote, org=o, repo=r, result=gitea_audit.SUCCEEDED, issue_number=data["number"], request_metadata={"title": title}, mutation_task="create_issue") - return _with_optional_url({"number": data["number"]}, data.get("html_url")) + result = { + "number": data["number"], + "workflow_label_validation": label_assessment, + } + if requested_labels: + with _audited( + "set_issue_labels", + host=h, + remote=remote, + org=o, + repo=r, + issue_number=data["number"], + request_metadata={"labels": requested_labels, "source": "create_issue"}, + ): + applied = _put_issue_label_names( + base=base, + auth=auth, + issue_number=data["number"], + names=requested_labels, + label_ids_by_name=label_ids_by_name, + ) + result["labels"] = issue_workflow_labels.label_names(applied) + return _with_optional_url(result, data.get("html_url")) def _list_open_pulls(h: str, o: str, r: str, auth: str) -> list[dict]: @@ -1995,7 +2167,23 @@ def gitea_create_pr( ) auth = _auth(h) - url = f"{repo_api_url(h, o, r)}/pulls" + repo_base = repo_api_url(h, o, r) + pr_open_transition = _prepare_issue_status_transition( + base=repo_base, + auth=auth, + issue_number=int(locked_issue), + status="status:pr-open", + ) + if not pr_open_transition.get("success"): + return { + "success": False, + "performed": False, + "number": None, + "issue_number": locked_issue, + "issue_status_transition": pr_open_transition, + "reasons": pr_open_transition.get("reasons", []), + } + url = f"{repo_base}/pulls" payload = {"title": title, "body": body, "head": head, "base": base} meta = {"title": title, "head": head, "base": base} try: @@ -2010,7 +2198,37 @@ def gitea_create_pr( result=gitea_audit.SUCCEEDED, pr_number=data["number"], target_branch=head, request_metadata=meta, mutation_task="create_pr") - return _with_optional_url({"number": data["number"]}, data.get("html_url")) + with _audited( + "set_issue_labels", + host=h, + remote=remote, + org=o, + repo=r, + issue_number=int(locked_issue), + request_metadata={ + "source": "create_pr", + "status": "status:pr-open", + "pr_number": data["number"], + }, + ): + _put_issue_label_names( + base=repo_base, + auth=auth, + issue_number=int(locked_issue), + names=pr_open_transition["labels"], + label_ids_by_name=pr_open_transition["label_ids_by_name"], + ) + result = { + "number": data["number"], + "issue_status_transition": { + "success": True, + "performed": True, + "issue_number": int(locked_issue), + "target_status": "status:pr-open", + "labels": pr_open_transition["labels"], + }, + } + return _with_optional_url(result, data.get("html_url")) def _format_list_pr_entry(pr: dict) -> dict: @@ -2280,13 +2498,23 @@ def gitea_check_pr_eligibility( # ("gitea.pr.merge") always match each other and never cross services. allowed = profile["allowed_operations"] forbidden = profile["forbidden_operations"] - op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + active_role = _role_kind(allowed, forbidden) + if action == "merge" and active_role == "reviewer": + op_ok = False + op_reason = "reviewer-cannot-merge" + else: + op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + if not op_ok: if op_reason == "no-allowed-operations": reasons.append( "profile has no configured allowed operations (fail closed)") elif op_reason == "forbidden": reasons.append(f"profile forbids '{action}'") + elif op_reason == "reviewer-cannot-merge": + reasons.append( + "Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head." + ) elif op_reason == "invalid-forbidden-entry": reasons.append( "profile has an unrecognized forbidden operation entry " @@ -2310,21 +2538,23 @@ def gitea_check_pr_eligibility( missing_perm) req_profile = None + role_noun = "reviewer" if action in ("approve", "request_changes", "review"): - req_profile = "A profile with reviewer role permissions (allowing approve/merge/review, and forbidding author operations)" + req_profile = "A profile with reviewer role permissions (allowing approve/review, and forbidding author operations)" elif action == "merge": - req_profile = "A profile with reviewer role permissions and explicit merge permission" + req_profile = "A profile with merger role permissions (allowing merge, and forbidding author operations)" + role_noun = "merger" result["required_profile"] = req_profile switching_supported = gitea_config.is_runtime_switching_enabled() if switching_supported: result["fixable_by_profile_switch"] = True result["requires_different_namespace"] = False - safe_step = f"Switch to a reviewer profile by calling gitea_activate_profile with a profile that allows {action}." + safe_step = f"Switch to a {role_noun} profile by calling gitea_activate_profile with a profile that allows {action}." else: result["fixable_by_profile_switch"] = False result["requires_different_namespace"] = True - safe_step = "Switch to the reviewer MCP session (e.g. gitea-reviewer) which has reviewer permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a reviewer profile." + safe_step = f"Switch to the {role_noun} MCP session (e.g. gitea-{role_noun}) which has {role_noun} permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a {role_noun} profile." result["safe_next_step"] = safe_step return result @@ -3276,6 +3506,13 @@ def _evaluate_pr_review_submission( result["pr_work_lease"] = lease_block return result + if (body or "").strip(): + gate = _canonical_comment_gate(body, context="pr_review") + if gate["blocked"]: + reasons.extend(gate["reasons"]) + result["canonical_comment_validation"] = gate["canonical_comment_validation"] + return result + result["would_perform"] = True if not live: reasons.append( @@ -4040,6 +4277,14 @@ def gitea_merge_pr( _verify_role_mutation_workspace( remote, worktree_path=worktree_path, task="merge_pr" ) + allowed = get_profile()["allowed_operations"] + forbidden = get_profile()["forbidden_operations"] + active_role = _role_kind(allowed, forbidden) + if active_role == "reviewer": + raise RuntimeError( + "Reviewer profile cannot merge. Use a merger profile/session " + "after formal approval at current head." + ) workflow_blockers = _review_workflow_load_gate_reasons() do = (do or "").strip().lower() result = { @@ -4056,6 +4301,10 @@ def gitea_merge_pr( "merge_result": None, "merge_commit": None, "reasons": [], + "active_profile": get_profile()["profile_name"], + "role_kind": active_role, + "merge_capability_source": "profile" if "gitea.pr.merge" in allowed else "config", + "explicit_operator_auth": confirmation, } reasons = result["reasons"] if workflow_blockers: @@ -4225,7 +4474,7 @@ def gitea_merge_pr( # A profile/identity flip or side-channel override between preflight # and merge fails closed here. try: - verify_mutation_authority(remote, host, required_role="reviewer", + verify_mutation_authority(remote, host, required_role="merger", active_identity=auth_user) except RuntimeError as e: reasons.append(str(e)) @@ -4780,6 +5029,7 @@ def gitea_reconcile_merged_cleanups( remote_branch_exists=remote_branch_exists, head_on_master=head_on_master, delete_capability_allowed=delete_capability_allowed, + target_ref=master_ref, active_reviewer_leases=active_reviewer_leases, pr_states=pr_states, ) @@ -4821,7 +5071,9 @@ def gitea_reconcile_merged_cleanups( if local_assessment.get("safe_to_remove_worktree"): result = merged_cleanup_reconcile.remove_local_worktree( - PROJECT_ROOT, head_branch + PROJECT_ROOT, + head_branch, + worktree_path=local_assessment.get("worktree_path"), ) actions.append({"action": "remove_local_worktree", **result}) @@ -5253,6 +5505,12 @@ def gitea_reconcile_already_landed_pr( "gitea.pr.comment" ) return result + gate = _canonical_comment_gate(comment_body) + if gate["blocked"]: + result["success"] = False + result["reasons"].extend(gate["reasons"]) + result["canonical_comment_validation"] = gate["canonical_comment_validation"] + return result comment_url = f"{base}/issues/{pr_number}/comments" with _audited( "comment_pr", @@ -6535,6 +6793,15 @@ def gitea_create_issue_comment( blocked["permission_report"] = _permission_block_report( "gitea.issue.comment") return blocked + canon_gate = _canonical_comment_gate(body, context="issue_comment") + if canon_gate["blocked"]: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "reasons": canon_gate["reasons"], + "canonical_comment_validation": canon_gate["canonical_comment_validation"], + } h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments" @@ -6563,26 +6830,28 @@ def _role_kind(allowed, forbidden) -> str: """Classify the active profile from its normalized operations. 'reconciler' can close already-landed PRs without review/author powers; - 'author' can create PRs / push branches; 'reviewer' can approve/merge; - 'reconciler' can close already-landed PRs via ``gitea.pr.close`` without - review/author powers; 'mixed' can do both (a config smell — the - reviewer-deadlock invariant forbids it in v2 configs); 'limited' can do - neither. + 'author' can create PRs / push branches; 'reviewer' can approve; + 'merger' can merge PRs; + 'mixed' can do multiple (a config smell); 'limited' can do neither. """ if reconciler_profile.is_reconciler_profile(allowed, forbidden): return "reconciler" def can(op): return gitea_config.check_operation(op, allowed, forbidden)[0] - review = can("gitea.pr.approve") or can("gitea.pr.merge") + can_merge = can("gitea.pr.merge") + can_approve = can("gitea.pr.approve") author = can("gitea.pr.create") or can("gitea.branch.push") reconciler = ( can("gitea.pr.close") - and not review + and not can_approve + and not can_merge and not author ) - if review and author: + if (can_approve or can_merge) and author: return "mixed" - if review: + if can_merge: + return "merger" + if can_approve: return "reviewer" if reconciler: return "reconciler" @@ -7100,12 +7369,20 @@ def mcp_get_control_plane_guide( "gitea.issue.comment (separate from gitea.pr.comment).") elif role == "reviewer": guidance.append( - "Reviewer profile: review/approve/merge may proceed ONLY after " + "Reviewer profile: review/approve may proceed ONLY after " "gitea_check_pr_eligibility passes and the PR head SHA is " "pinned (expected_head_sha); the PR author must be a different " - "user, and merging additionally requires explicit operator " - "authorization plus the 'MERGE PR <n>' confirmation. " - "PR comments do not imply issue comments.") + "user. Reviewer profiles cannot merge PRs. " + "PR comments do not imply issue comments. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") + elif role == "merger": + guidance.append( + "Merger profile: merge may proceed ONLY after formal approval at " + "current head, gitea_check_pr_eligibility passes, and the PR head " + "SHA is pinned (expected_head_sha); the PR author must be a different " + "user, and merging requires explicit operator authorization plus the " + "'MERGE PR <n>' confirmation. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") elif role == "mixed": guidance.append( "WARNING: this profile allows both authoring and " @@ -8119,6 +8396,34 @@ def gitea_audit_config() -> dict: return report +def _canonical_comment_gate( + body: str, + *, + context: str | None = None, + force_workflow: bool = False, +) -> dict: + """Fail-closed canonical state validation before comment/review POST (#496).""" + assessment = ccv.assess_canonical_comment( + body, + context=context, + force_workflow=force_workflow, + ) + if assessment.get("allowed"): + return { + "blocked": False, + "canonical_comment_validation": assessment, + "reasons": [], + } + correction = (assessment.get("correction_message") or "").strip() + return { + "blocked": True, + "canonical_comment_validation": assessment, + "reasons": [ + correction or "canonical comment validation failed (fail closed before posting)" + ], + } + + def _post_structured_issue_comment( *, issue_number: int, @@ -8128,8 +8433,18 @@ def _post_structured_issue_comment( org: str | None, repo: str | None, audit_op: str = "create_issue_comment", + comment_context: str | None = None, ) -> dict: """Post an issue-thread comment after permission gates already passed.""" + gate = _canonical_comment_gate(body, context=comment_context) + if gate["blocked"]: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "reasons": gate["reasons"], + "canonical_comment_validation": gate["canonical_comment_validation"], + } h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments" @@ -8194,15 +8509,8 @@ def gitea_mark_issue( auth = _auth(h) base = repo_api_url(h, o, r) - # Find the status:in-progress label id - labels = api_request("GET", f"{base}/labels?limit=100", auth) - label_id = None - for lb in labels: - if lb["name"] == "status:in-progress": - label_id = lb["id"] - break - - if label_id is None: + label_ids_by_name = _repo_label_id_map(base, auth) + if "status:in-progress" not in label_ids_by_name: raise RuntimeError( "Label 'status:in-progress' not found. " "Run manage_labels.py to create it first." @@ -8210,10 +8518,19 @@ def gitea_mark_issue( if action == "start": with _audited("label_issue", host=h, remote=remote, org=o, repo=r, - issue_number=issue_number, - request_metadata={"op": "add", "label": "status:in-progress"}): - api_request("POST", f"{base}/issues/{issue_number}/labels", auth, - {"labels": [label_id]}) + issue_number=issue_number, request_metadata={ + "op": "replace-status", + "label": "status:in-progress", + }): + transition = _transition_issue_status( + base=base, + auth=auth, + issue_number=issue_number, + status="status:in-progress", + label_ids_by_name=label_ids_by_name, + ) + if not transition.get("success"): + raise RuntimeError("; ".join(transition.get("reasons") or [])) active_profile = (profile or get_profile().get("profile_name") or "unknown") branch = (branch_name or "pending").strip() or "pending" heartbeat_body = issue_claim_heartbeat.format_heartbeat_body( @@ -8238,8 +8555,10 @@ def gitea_mark_issue( "message": f"Issue #{issue_number} claimed.", "heartbeat_posted": heartbeat.get("success", False), "heartbeat_comment_id": heartbeat.get("comment_id"), + "status_transition": transition, } else: + label_id = label_ids_by_name["status:in-progress"] with _audited("unlabel_issue", host=h, remote=remote, org=o, repo=r, issue_number=issue_number, request_metadata={"op": "remove", "label": "status:in-progress"}): @@ -8355,6 +8674,40 @@ def gitea_acquire_conflict_fix_lease( } +@mcp.tool() +def gitea_assess_conflict_fix_classification( + pr_number: int, + inventory_head_sha: str | None = None, + inventory_mergeable: bool | None = None, + live_head_sha: str | None = None, + live_mergeable: bool | None = None, +) -> dict: + """Read-only: classify conflict-fix need from live PR state (#522). + + Open PR inventory can be stale. Callers must pass a live re-fetched PR head + and mergeability value before creating a conflict-fix worktree. + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "performed": False, + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + + result = conflict_fix_classification.assess_conflict_fix_classification( + pr_number=pr_number, + inventory_head_sha=inventory_head_sha, + inventory_mergeable=inventory_mergeable, + live_head_sha=live_head_sha, + live_mergeable=live_mergeable, + ) + result["success"] = True + result["performed"] = False + return result + + @mcp.tool() def gitea_assess_conflict_fix_push( pr_number: int, @@ -8622,6 +8975,7 @@ def gitea_create_label( host: str | None = None, org: str | None = None, repo: str | None = None, + worktree_path: str | None = None, ) -> dict: """Create a new label on a Gitea repository. @@ -8633,11 +8987,16 @@ def gitea_create_label( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + worktree_path: Optional branches worktree to verify before mutation. Returns: dict containing the created label details. """ - verify_preflight_purity(remote) + blocked = _profile_permission_block( + task_capability_map.required_permission("create_label")) + if blocked: + return blocked + verify_preflight_purity(remote, worktree_path=worktree_path, task="create_label") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) base = repo_api_url(h, o, r) @@ -8663,6 +9022,7 @@ def gitea_set_issue_labels( host: str | None = None, org: str | None = None, repo: str | None = None, + worktree_path: str | None = None, ) -> list: """Replace all labels on a Gitea issue with a new list of label names. @@ -8673,6 +9033,7 @@ def gitea_set_issue_labels( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + worktree_path: Optional branches worktree to verify before mutation. Returns: list of all labels currently applied to the issue. @@ -8681,7 +9042,7 @@ def gitea_set_issue_labels( task_capability_map.required_permission("set_issue_labels")) if blocked: return blocked - verify_preflight_purity(remote, task="set_issue_labels") + verify_preflight_purity(remote, worktree_path=worktree_path, task="set_issue_labels") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) base = repo_api_url(h, o, r) @@ -9225,6 +9586,11 @@ def gitea_capability_stop_terminal_report() -> dict: # ── Entry point ─────────────────────────────────────────────────────────────── if __name__ == "__main__": + # #558: mark this process as the official MCP daemon before any tool + # dispatch so direct shell imports cannot reuse mutation/auth paths. + import mcp_daemon_guard + + mcp_daemon_guard.mark_sanctioned_daemon() # Lock this session's launch profile into the environment so child CLI # processes (e.g. review_pr.py) can detect and refuse profile # side-channel overrides (#199). diff --git a/issue_workflow_labels.py b/issue_workflow_labels.py new file mode 100644 index 0000000..1e9c491 --- /dev/null +++ b/issue_workflow_labels.py @@ -0,0 +1,194 @@ +"""Canonical issue type/status label taxonomy and transition helpers (#513).""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Iterable, Mapping, Sequence + + +@dataclass(frozen=True) +class LabelSpec: + name: str + color: str + description: str + + +TYPE_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("type:bug", "b60205", "Bug or defect"), + LabelSpec("type:feature", "a2eeef", "Feature or enhancement"), + LabelSpec("type:process", "5319e7", "Process or policy work"), + LabelSpec("type:workflow", "c2e0c6", "Workflow automation or guidance"), + LabelSpec("type:guardrail", "d93f0b", "Safety gate or guardrail"), + LabelSpec("type:docs", "006b75", "Documentation work"), + LabelSpec("type:test", "0e8a16", "Tests or test infrastructure"), + LabelSpec("type:discussion", "bfdadc", "Discussion-only issue"), + LabelSpec("type:umbrella", "c5def5", "Umbrella or tracker issue"), + LabelSpec("type:cleanup", "ededed", "Cleanup or hygiene work"), +) + +STATUS_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("status:triage", "fbca04", "Issue needs triage"), + LabelSpec("status:ready", "0e8a16", "Issue is ready for work"), + LabelSpec("status:claimed", "fefe2e", "Issue is claimed"), + LabelSpec("status:in-progress", "fefe2e", "Issue is being worked on"), + LabelSpec("status:blocked", "b60205", "Issue is blocked"), + LabelSpec("status:needs-review", "0052cc", "Issue work needs review"), + LabelSpec("status:pr-open", "1d76db", "A linked PR is open"), + LabelSpec("status:approved", "0e8a16", "Linked PR is approved"), + LabelSpec("status:merged", "5319e7", "Linked PR is merged"), + LabelSpec("status:reconcile", "d93f0b", "Issue needs reconciliation"), + LabelSpec("status:done", "0e8a16", "Issue workflow is complete"), + LabelSpec("status:duplicate", "cccccc", "Issue is a duplicate"), + LabelSpec("status:wontfix", "000000", "Issue will not be fixed"), +) + +CANONICAL_LABEL_SPECS: tuple[LabelSpec, ...] = ( + TYPE_LABEL_SPECS + STATUS_LABEL_SPECS +) + +TYPE_LABELS: frozenset[str] = frozenset(spec.name for spec in TYPE_LABEL_SPECS) +STATUS_LABELS: frozenset[str] = frozenset(spec.name for spec in STATUS_LABEL_SPECS) +CANONICAL_LABELS: frozenset[str] = frozenset( + spec.name for spec in CANONICAL_LABEL_SPECS +) + +STATUS_TRANSITIONS: dict[str, str] = { + "triage": "status:triage", + "ready": "status:ready", + "claim": "status:claimed", + "claimed": "status:claimed", + "start": "status:in-progress", + "start_work": "status:in-progress", + "in_progress": "status:in-progress", + "in-progress": "status:in-progress", + "block": "status:blocked", + "blocked": "status:blocked", + "needs_review": "status:needs-review", + "needs-review": "status:needs-review", + "pr_open": "status:pr-open", + "pr-open": "status:pr-open", + "approved": "status:approved", + "merge": "status:reconcile", + "merged": "status:reconcile", + "reconcile": "status:reconcile", + "done": "status:done", + "complete": "status:done", + "duplicate": "status:duplicate", + "wontfix": "status:wontfix", +} + + +def label_name(label: str | Mapping[str, object]) -> str: + """Return a normalized label name from a Gitea label object or string.""" + if isinstance(label, str): + return label.strip() + value = label.get("name") + return str(value or "").strip() + + +def label_names(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + """Extract label names from a label list or issue-like mapping.""" + raw: object + if isinstance(labels, Mapping): + raw = labels.get("labels", []) + else: + raw = labels + return [name for name in (label_name(label) for label in raw or []) if name] + + +def type_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("type:")] + + +def status_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("status:")] + + +def canonical_status_label(status_or_transition: str) -> str: + status = status_or_transition.strip() + if status in STATUS_LABELS: + return status + normalized = status.lower().replace(" ", "-") + try: + return STATUS_TRANSITIONS[normalized] + except KeyError as exc: + raise ValueError( + f"unknown workflow status or transition '{status_or_transition}'" + ) from exc + + +def transition_status_labels( + existing_labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + status_or_transition: str, +) -> list[str]: + """Replace all active status labels with the requested canonical status.""" + new_status = canonical_status_label(status_or_transition) + kept = [name for name in label_names(existing_labels) if not name.startswith("status:")] + if new_status not in kept: + kept.append(new_status) + return kept + + +def labels_for_new_issue( + issue_type: str | None = None, + initial_status: str | None = None, + extra_labels: Sequence[str] | None = None, + *, + discussion: bool = False, +) -> list[str]: + names = list(extra_labels or []) + if issue_type: + type_name = issue_type if issue_type.startswith("type:") else f"type:{issue_type}" + names.append(type_name) + if discussion: + names.append("type:discussion") + if initial_status: + names.append(canonical_status_label(initial_status)) + + result: list[str] = [] + for name in names: + if name not in result: + result.append(name) + return result + + +def assess_issue_labels( + labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + *, + discussion: bool = False, + require_type: bool = True, + require_status: bool = True, +) -> dict: + names = label_names(labels) + found_types = type_labels(names) + found_statuses = status_labels(names) + errors: list[str] = [] + warnings: list[str] = [] + + if require_type and not found_types: + errors.append("issue is missing a type:* label") + if require_status and not found_statuses: + errors.append("issue is missing a status:* label") + if discussion and "type:discussion" not in found_types: + errors.append("discussion issue is missing type:discussion") + if len(found_statuses) > 1: + errors.append( + "issue has multiple active status:* labels: " + + ", ".join(found_statuses) + ) + + for name in found_types: + if name not in TYPE_LABELS: + warnings.append(f"unknown type label '{name}'") + for name in found_statuses: + if name not in STATUS_LABELS: + warnings.append(f"unknown status label '{name}'") + + return { + "valid": not errors, + "labels": names, + "type_labels": found_types, + "status_labels": found_statuses, + "errors": errors, + "warnings": warnings, + } diff --git a/manage_labels.py b/manage_labels.py index 0a2f52f..400b319 100755 --- a/manage_labels.py +++ b/manage_labels.py @@ -23,6 +23,7 @@ if os.path.exists(venv_python) and sys.executable != venv_python: os.execv(venv_python, [venv_python] + sys.argv) from gitea_auth import get_auth_header, api_request, repo_api_url +import issue_workflow_labels HOST = "gitea.dadeschools.net" ORG = "Contractor" @@ -35,8 +36,10 @@ LABELS = [ {"name": "epic", "color": "8250df", "description": ""}, {"name": "important", "color": "fbca04", "description": ""}, {"name": "nice-to-have", "color": "0e8a16", "description": ""}, - {"name": "status:in-progress", "color": "fefe2e", - "description": "Issue is being worked on"}, + *[ + {"name": spec.name, "color": spec.color, "description": spec.description} + for spec in issue_workflow_labels.CANONICAL_LABEL_SPECS + ], ] # issue number -> label names to apply (one-off backfill) diff --git a/mcp_daemon_guard.py b/mcp_daemon_guard.py new file mode 100644 index 0000000..241c68b --- /dev/null +++ b/mcp_daemon_guard.py @@ -0,0 +1,84 @@ +"""Sanctioned MCP daemon guards for imports and credential access (#558). + +Direct ``import gitea_mcp_server`` / ``import gitea_auth`` from a shell, plus +raw keychain dumps, bypass preflight purity and role gates. Mutation helpers +and keychain fallbacks therefore require an explicit sanctioned runtime. + +Sanctioned contexts (any one): +- ``GITEA_MCP_SANCTIONED_DAEMON=1`` (set by the official MCP entrypoint) +- pytest (``PYTEST_CURRENT_TEST`` present) +- explicit operator opt-in ``GITEA_ALLOW_DIRECT_MCP_IMPORT=1`` (tests/tools only) + +Credential keychain fill additionally allows: +- ``GITEA_ALLOW_KEYCHAIN_CLI=1`` for operator-only non-MCP scripts that must + use git-credential (never the default for LLM shells). +""" + +from __future__ import annotations + +import os +from typing import Any + +SANCTIONED_DAEMON_ENV = "GITEA_MCP_SANCTIONED_DAEMON" +ALLOW_DIRECT_IMPORT_ENV = "GITEA_ALLOW_DIRECT_MCP_IMPORT" +ALLOW_KEYCHAIN_CLI_ENV = "GITEA_ALLOW_KEYCHAIN_CLI" + + +class UnsanctionedRuntimeError(RuntimeError): + """Raised when mutation/credential code runs outside a sanctioned MCP daemon.""" + + +def is_pytest_runtime() -> bool: + return bool((os.environ.get("PYTEST_CURRENT_TEST") or "").strip()) + + +def is_sanctioned_mcp_daemon() -> bool: + if (os.environ.get(SANCTIONED_DAEMON_ENV) or "").strip() in {"1", "true", "yes"}: + return True + if (os.environ.get(ALLOW_DIRECT_IMPORT_ENV) or "").strip() in {"1", "true", "yes"}: + return True + if is_pytest_runtime(): + return True + return False + + +def mark_sanctioned_daemon() -> None: + """Call from the official MCP server entrypoint before serving tools.""" + os.environ[SANCTIONED_DAEMON_ENV] = "1" + + +def assert_sanctioned_mutation_runtime(context: str = "mutation") -> None: + """Fail closed when server mutation code is used outside the MCP daemon.""" + if is_sanctioned_mcp_daemon(): + return + raise UnsanctionedRuntimeError( + f"Unsanctioned runtime blocked {context} (#558). " + "Do not import gitea_mcp_server / call mutation helpers from a raw " + "shell or ad-hoc script. Use the official MCP daemon entrypoint " + f"(sets {SANCTIONED_DAEMON_ENV}=1), or run under pytest. " + f"Operator-only override: {ALLOW_DIRECT_IMPORT_ENV}=1 (not for LLM sessions)." + ) + + +def assert_keychain_access_allowed() -> None: + """Fail closed for git-credential keychain fill outside sanctioned contexts.""" + if is_sanctioned_mcp_daemon(): + return + if (os.environ.get(ALLOW_KEYCHAIN_CLI_ENV) or "").strip() in {"1", "true", "yes"}: + return + raise UnsanctionedRuntimeError( + "Unsanctioned keychain/credential fill blocked (#558). " + "Token extraction via git-credential is only allowed inside the " + f"official MCP daemon ({SANCTIONED_DAEMON_ENV}=1), pytest, or with " + f"explicit operator opt-in {ALLOW_KEYCHAIN_CLI_ENV}=1." + ) + + +def runtime_status() -> dict[str, Any]: + return { + "sanctioned_daemon": is_sanctioned_mcp_daemon(), + "pytest": is_pytest_runtime(), + "sanctioned_env": SANCTIONED_DAEMON_ENV, + "allow_direct_import_env": ALLOW_DIRECT_IMPORT_ENV, + "allow_keychain_cli_env": ALLOW_KEYCHAIN_CLI_ENV, + } diff --git a/mcp_server.py b/mcp_server.py index dbe5f1b..479273b 100644 --- a/mcp_server.py +++ b/mcp_server.py @@ -37,6 +37,17 @@ def check_conflict_markers(): check_conflict_markers() +# #558: official entrypoint marks the process as the sanctioned MCP daemon +# before loading mutation modules (blocks raw shell import bypasses). +try: + import mcp_daemon_guard + + mcp_daemon_guard.mark_sanctioned_daemon() +except Exception: + # Guard import failures must not hide conflict-marker infra_stop above; + # gitea_mcp_server main also marks sanctioned when run as __main__. + pass + # Execute the actual server logic via exec in this namespace. impl_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gitea_mcp_server.py") try: diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index e6e7236..4a7299a 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -17,6 +17,7 @@ import issue_lock_store PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) +ISSUE_MARKER_RE = re.compile(r"(?:^|[-_/])issue-(\d+)(?:[-_/]|$)", re.IGNORECASE) # Reviewer scratch folders: review-pr<N> or review-pr<N>-submit (#534). REVIEWER_SCRATCH_NAME_RE = re.compile( r"^review-pr(?P<pr>\d+)(?:-submit)?$", @@ -41,6 +42,59 @@ def resolve_worktree_path(project_root: str, branch: str) -> str: return os.path.join(project_root, "branches", branch_worktree_folder(branch)) +def extract_issue_marker(*values: str | None) -> int | None: + """Return the first issue marker found in branch/path-like values.""" + for value in values: + match = ISSUE_MARKER_RE.search(value or "") + if match: + return int(match.group(1)) + return None + + +def list_local_worktrees(project_root: str) -> list[dict[str, Any]]: + """Parse ``git worktree list --porcelain`` into structured entries.""" + res = subprocess.run( + ["git", "-C", project_root, "worktree", "list", "--porcelain"], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return [] + + entries: list[dict[str, Any]] = [] + current: dict[str, Any] | None = None + + def flush() -> None: + nonlocal current + if current: + entries.append(current) + current = None + + for raw_line in (res.stdout or "").splitlines(): + line = raw_line.strip() + if not line: + flush() + continue + if line.startswith("worktree "): + flush() + current = {"path": line[9:].strip()} + elif current is not None and line.startswith("HEAD "): + current["head_sha"] = line[5:].strip() + elif current is not None and line.startswith("branch "): + ref = line[7:].strip() + current["branch_ref"] = ref + current["branch"] = ( + ref[len("refs/heads/"):] + if ref.startswith("refs/heads/") + else ref + ) + elif current is not None and line == "detached": + current["detached"] = True + flush() + return entries + + def _git_worktree_porcelain(project_root: str) -> list[dict[str, str | None]]: """Parse ``git worktree list --porcelain`` into structured records.""" res = subprocess.run( @@ -82,14 +136,115 @@ def _git_worktree_porcelain(project_root: str) -> list[dict[str, str | None]]: def discover_local_worktrees(project_root: str) -> dict[str, str]: """Return a mapping from branch name to absolute worktree path (#528).""" mapping: dict[str, str] = {} - for record in _git_worktree_porcelain(project_root): - branch_name = record.get("branch") - path = record.get("worktree") - if branch_name and path: - mapping[str(branch_name)] = str(path) + for entry in list_local_worktrees(project_root): + branch = entry.get("branch") + path = entry.get("path") + if branch and path: + mapping[str(branch)] = str(path) return mapping +def _is_under_branches(project_root: str, path: str) -> bool: + branches_root = os.path.realpath(os.path.join(project_root, "branches")) + real_path = os.path.realpath(path) + return real_path == branches_root or real_path.startswith(branches_root + os.sep) + + +def _head_is_safe_for_cleanup( + *, + project_root: str, + candidate_head: str | None, + pr_head_sha: str | None, + target_ref: str | None, +) -> bool: + if not candidate_head: + return False + if pr_head_sha and candidate_head == pr_head_sha: + return True + if target_ref: + return is_head_ancestor_of_ref(project_root, candidate_head, target_ref) is True + return False + + +def resolve_cleanup_worktree_state( + *, + project_root: str, + head_branch: str, + issue_number: int | None, + pr_head_sha: str | None = None, + target_ref: str | None = None, +) -> dict[str, Any]: + """Find the exact or safe alias worktree used for local cleanup (#532).""" + expected_path = resolve_worktree_path(project_root, head_branch) + state = read_local_worktree_state(expected_path) + state["worktree_path"] = expected_path + state["expected_worktree_path"] = expected_path + state["discovered_worktree_path"] = expected_path if state.get("exists") else None + state["match_type"] = "derived_path" if state.get("exists") else "none" + state["candidate_paths"] = [] + state["alias_block_reasons"] = [] + if state.get("exists"): + return state + + branch_issue = extract_issue_marker(head_branch) + wanted_issue = issue_number or branch_issue + candidates: list[dict[str, Any]] = [] + unsafe_matches: list[str] = [] + + for entry in list_local_worktrees(project_root): + path = entry.get("path") or "" + if not path or not _is_under_branches(project_root, path): + continue + branch = entry.get("branch") or "" + path_issue = extract_issue_marker(os.path.basename(path), path) + entry_issue = extract_issue_marker(branch) or path_issue + branch_match = branch == head_branch + issue_match = wanted_issue is not None and entry_issue == wanted_issue + if not (branch_match or issue_match): + continue + safe_head = _head_is_safe_for_cleanup( + project_root=project_root, + candidate_head=entry.get("head_sha"), + pr_head_sha=pr_head_sha, + target_ref=target_ref, + ) + if not safe_head and branch_match and not pr_head_sha and not target_ref: + safe_head = True + if not safe_head: + unsafe_matches.append(path) + continue + candidates.append(entry) + + state["candidate_paths"] = [entry.get("path") for entry in candidates] + if len(candidates) == 1: + path = candidates[0]["path"] + alias_state = read_local_worktree_state(path) + alias_state["worktree_path"] = path + alias_state["expected_worktree_path"] = expected_path + alias_state["discovered_worktree_path"] = path + alias_state["match_type"] = ( + "branch" if candidates[0].get("branch") == head_branch else "alias" + ) + alias_state["candidate_paths"] = [path] + alias_state["alias_block_reasons"] = [] + alias_state["alias_verified"] = True + return alias_state + if len(candidates) > 1: + state["alias_block_reasons"].append( + "ambiguous local worktree aliases: " + ", ".join( + sorted(str(entry.get("path")) for entry in candidates) + ) + ) + state["match_type"] = "ambiguous_alias" + elif unsafe_matches: + state["alias_block_reasons"].append( + "matching local worktree aliases did not point to the PR head or target branch: " + + ", ".join(sorted(unsafe_matches)) + ) + state["match_type"] = "unsafe_alias" + return state + + def parse_reviewer_scratch_folder(folder_name: str) -> int | None: """Return PR number for a reviewer scratch folder name, else None (#534).""" match = REVIEWER_SCRATCH_NAME_RE.match((folder_name or "").strip()) @@ -323,6 +478,7 @@ def assess_local_worktree_cleanup( exists = bool(worktree_state.get("exists")) if not merged: reasons.append("PR is not merged") + reasons.extend(worktree_state.get("alias_block_reasons") or []) if not exists: reasons.append("local worktree not present") if exists and worktree_state.get("clean") is False: @@ -333,7 +489,11 @@ def assess_local_worktree_cleanup( ) if exists: current_branch = worktree_state.get("current_branch") - if current_branch and current_branch != head_branch: + if ( + current_branch + and current_branch != head_branch + and not worktree_state.get("alias_verified") + ): reasons.append( f"worktree branch '{current_branch}' does not match PR head '{head_branch}'" ) @@ -345,6 +505,10 @@ def assess_local_worktree_cleanup( "pr_number": pr_number, "head_branch": head_branch, "worktree_path": worktree_state.get("worktree_path"), + "expected_worktree_path": worktree_state.get("expected_worktree_path"), + "discovered_worktree_path": worktree_state.get("discovered_worktree_path"), + "match_type": worktree_state.get("match_type"), + "candidate_paths": worktree_state.get("candidate_paths") or [], "worktree_exists": exists, "worktree_clean": worktree_state.get("clean"), "safe_to_remove_worktree": safe, @@ -363,7 +527,7 @@ def build_pr_cleanup_entry( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, - worktree_map: dict[str, str] | None = None, + target_ref: str | None = None, ) -> dict[str, Any]: pr_number = int(pr["number"]) head_branch = pr.get("head") or "" @@ -372,18 +536,16 @@ def build_pr_cleanup_entry( title = pr.get("title") or "" body = pr.get("body") or "" merged = bool(pr.get("merged_at")) - - discovered_path = worktree_map.get(head_branch) if worktree_map else None - derived_path = resolve_worktree_path(project_root, head_branch) - if discovered_path: - worktree_path = discovered_path - match_type = "branch" - else: - worktree_path = derived_path - match_type = "path" - - worktree_state = read_local_worktree_state(worktree_path) - worktree_state["worktree_path"] = worktree_path + issue_number = extract_linked_issue(title, body) or extract_issue_marker(head_branch) + head_payload = pr.get("head") if isinstance(pr.get("head"), dict) else {} + pr_head_sha = head_payload.get("sha") if isinstance(head_payload, dict) else None + worktree_state = resolve_cleanup_worktree_state( + project_root=project_root, + head_branch=head_branch, + issue_number=issue_number, + pr_head_sha=pr_head_sha, + target_ref=target_ref, + ) active_lock = has_active_issue_lock(head_branch, issue_lock_path) remote = assess_remote_branch_cleanup( @@ -404,11 +566,9 @@ def build_pr_cleanup_entry( worktree_state=worktree_state, active_lock=active_lock, ) - local["match_type"] = match_type - return { "pr_number": pr_number, - "issue_number": extract_linked_issue(title, body), + "issue_number": issue_number, "title": title, "head_branch": head_branch, "merge_commit_sha": pr.get("merge_commit_sha"), @@ -429,11 +589,11 @@ def build_reconciliation_report( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + target_ref: str | None = None, active_reviewer_leases: dict[int, bool] | None = None, pr_states: dict[int, dict[str, Any]] | None = None, ) -> dict[str, Any]: open_heads = collect_open_pr_heads(open_prs) - worktree_map = discover_local_worktrees(project_root) entries: list[dict[str, Any]] = [] for pr in closed_prs or []: if not pr.get("merged_at"): @@ -452,7 +612,7 @@ def build_reconciliation_report( delete_capability_allowed=delete_capability_allowed, issue_lock_path=issue_lock_path, protected_branches=protected_branches, - worktree_map=worktree_map, + target_ref=target_ref, ) ) @@ -505,10 +665,12 @@ def build_reconciliation_report( } -def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: - worktree_map = discover_local_worktrees(project_root) - discovered_path = worktree_map.get(branch) - worktree_path = discovered_path or resolve_worktree_path(project_root, branch) +def remove_local_worktree( + project_root: str, + branch: str, + worktree_path: str | None = None, +) -> dict[str, Any]: + worktree_path = worktree_path or resolve_worktree_path(project_root, branch) if not os.path.isdir(worktree_path): return { "success": False, @@ -593,4 +755,4 @@ def remove_reviewer_scratch_worktree( "message": f"removed reviewer scratch worktree {real}", "worktree_path": real, "author_worktree_cleanup": False, - } \ No newline at end of file + } diff --git a/review_proofs.py b/review_proofs.py index 194ce9e..bab6748 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -2243,6 +2243,18 @@ HANDOFF_ROLE_FIELDS = { ("Linked issue status", ("linked issue status", "linked issue")), ("Cleanup status", ("cleanup status", "cleanup")), ) + HANDOFF_REVIEW_MUTATION_FIELDS, + "merger": ( + ("Selected PR", ("selected pr",)), + ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), + ("Active profile", ("active profile",)), + ("Role kind", ("role kind",)), + ("Merge capability source", ("merge capability source",)), + ("Explicit operator authorization", ("explicit operator authorization", "operator authorization", "authorization")), + ("Expected head SHA", ("expected head sha", "expected head")), + ("Approval at current head", ("approval at current head", "approval")), + ("Merge result", ("merge result",)), + ("Cleanup status", ("cleanup status", "cleanup")), + ) + HANDOFF_REVIEW_MUTATION_FIELDS, "author": ( ("Selected issue", ("selected issue",)), ("Issue lock proof", ("issue lock proof", "lock before diff")), @@ -2417,8 +2429,8 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): field for field in required if field[0] not in ("Workspace mutations", "Mutations") ] - if role == "review": - # Issue #320: reviewer handoffs use the precise mutation categories + if role in ("review", "merger"): + # Issue #320: reviewer and merger handoffs use the precise mutation categories # in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous # "Workspace mutations" field, which is rejected below. required = [ @@ -2431,7 +2443,7 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): "downgraded": True, "missing_fields": [], "reasons": [ - "review handoff must not include legacy " + "review or merger handoff must not include legacy " "'Workspace mutations' field; report the precise " "mutation categories instead (issue #320)" ], @@ -5540,6 +5552,15 @@ def assess_already_landed_classification_report(report_text, **kwargs): return _assess(report_text, **kwargs) +def assess_conflict_fix_classification_final_report(report_text, **kwargs): + """#522: require live PR head re-pin before conflict-fix classification.""" + from conflict_fix_classification import ( + assess_conflict_fix_classification_final_report as _assess, + ) + + return _assess(report_text, **kwargs) + + def assess_prior_blocker_skip_proof(report_text, **kwargs): """#318: require live blocker proof before skipping earlier open PRs.""" from reviewer_blocker_skip import assess_prior_blocker_skip_proof as _assess diff --git a/reviewer_handoff_consistency.py b/reviewer_handoff_consistency.py new file mode 100644 index 0000000..a6deeca --- /dev/null +++ b/reviewer_handoff_consistency.py @@ -0,0 +1,186 @@ +"""Reviewer handoff consistency validation (#501). + +Detects contradictions between narrative claims and mutation ledger fields in +reviewer/final-review controller handoffs. Pure validation only — does not post +comments or mutate Gitea state. +""" + +from __future__ import annotations + +import re + +_HANDOFF_FIELD_RE = re.compile( + r"^\s*[-*]\s*([^:]+):\s*(.*)$", + re.MULTILINE | re.IGNORECASE, +) + +_MERGE_NARRATIVE_RE = re.compile( + r"\b(?:merged\s+pr\s*#|pr\s+#\d+\s+(?:was\s+)?merged|" + r"merge\s+result\s*:\s*merged|successfully\s+merged|" + r"terminal\s+mutation\s+budget.{0,80}\bmerge)\b", + re.IGNORECASE | re.DOTALL, +) +_REVIEW_SUBMIT_BLOCKED_RE = re.compile( + r"\b(?:review\s+submission\s+blocked|submit_pr_review\s+(?:failed|blocked)|" + r"gitea_submit_pr_review\s+(?:failed|blocked|not\s+(?:attempted|performed))|" + r"review\s+not\s+submitted|could\s+not\s+submit\s+review)\b", + re.IGNORECASE, +) +_FINAL_DECISION_MARKED_RE = re.compile( + r"\b(?:gitea_mark_final_review_decision|final\s+review\s+decision\s+marked|" + r"server-side\s+final\s+decision\s+marked|marked\s+final\s+review\s+decision)\b", + re.IGNORECASE, +) +_TERMINAL_BUDGET_CONSUMED_RE = re.compile( + r"\b(?:terminal\s+mutation\s+budget\s+(?:already\s+)?consumed|" + r"single[- ]terminal\s+mutation\s+(?:already\s+)?consumed|" + r"mutation\s+budget\s+exhausted)\b", + re.IGNORECASE, +) +_LEASE_NARRATIVE_RE = re.compile( + r"\b(?:reviewer\s+lease\s+acquired|acquired\s+reviewer\s+pr\s+lease|" + r"gitea_acquire_reviewer_pr_lease)\b", + re.IGNORECASE, +) +_REJECTED_MUTATION_RE = re.compile( + r"\b(?:mutation\s+rejected|tool\s+failed\s+closed|blocked\s+before\s+mutation|" + r"no\s+server-side\s+state\s+changed)\b", + re.IGNORECASE, +) +_MERGE_IN_LEDGER_RE = re.compile(r"\bmerge\b", re.IGNORECASE) +_REVIEW_SUBMITTED_IN_LEDGER_RE = re.compile( + r"\b(?:submitted|approve|request_changes|review\s+submitted)\b", + re.IGNORECASE, +) +_NONE_VALUE_RE = re.compile(r"^\s*(?:none|not\s+attempted|n/a)\s*$", re.IGNORECASE) + +_BLOCKED_REVIEW_PROOF_FIELDS = ( + "tool called", + "mutation attempted", + "mutation rejected", + "no server-side state changed", +) + + +def render_blocked_review_handoff_template() -> str: + """Return a corrected handoff template for blocked review submissions.""" + return """## Blocked Review Submission Handoff + +- Tool called: gitea_submit_pr_review +- Mutation attempted: yes +- Mutation rejected: yes +- No server-side state changed: confirmed +- Proof/source: <paste exact tool error or gate reason> +- Prior terminal mutation that consumed budget: <name exact prior mutation or none> +- Review decision (local intent): <approve | request_changes | comment only> +- Server-side final decision marked: <yes with tool proof | no> +- Gitea review submitted: no +- Gitea review blocked because: <exact gate/error> +- Review mutations: none (submission blocked) +- MCP/Gitea mutations: <list only mutations that actually occurred> +- Safe next action: rewrite handoff with consistent ledger before posting +""" + + +def _handoff_fields(text: str | None) -> dict[str, str]: + fields: dict[str, str] = {} + for match in _HANDOFF_FIELD_RE.finditer(text or ""): + key = match.group(1).strip().lower() + value = match.group(2).strip() + fields[key] = value + return fields + + +def _is_none_value(value: str | None) -> bool: + return bool(_NONE_VALUE_RE.match(value or "")) + + +def _ledger_mentions_merge(*values: str | None) -> bool: + blob = " ".join(v for v in values if v) + return bool(blob) and bool(_MERGE_IN_LEDGER_RE.search(blob)) + + +def _ledger_mentions_review_submission(*values: str | None) -> bool: + blob = " ".join(v for v in values if v) + return bool(blob) and bool(_REVIEW_SUBMITTED_IN_LEDGER_RE.search(blob)) + + +def assess_reviewer_handoff_consistency(report_text: str | None) -> dict: + """Validate reviewer handoff narrative against mutation ledger fields.""" + text = report_text or "" + fields = _handoff_fields(text) + reasons: list[str] = [] + + review_mutations = fields.get("review mutations", "") + merge_mutations = fields.get("merge mutations", "") + mcp_mutations = fields.get("mcp/gitea mutations", "") + review_decision = fields.get("review decision", "") + + if _MERGE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge( + merge_mutations, mcp_mutations, review_mutations + ): + reasons.append( + "narrative claims a merge occurred but mutation ledger omits merge" + ) + + if _TERMINAL_BUDGET_CONSUMED_RE.search(text): + if _ledger_mentions_merge(merge_mutations, mcp_mutations): + pass + elif _LEASE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge( + merge_mutations, mcp_mutations + ): + reasons.append( + "terminal mutation budget attributed to merge but ledger only " + "shows lease or non-merge activity" + ) + elif not _ledger_mentions_merge(merge_mutations, mcp_mutations): + reasons.append( + "terminal mutation budget consumed claim must identify the " + "exact prior mutation in the ledger" + ) + + if _REVIEW_SUBMIT_BLOCKED_RE.search(text) and _FINAL_DECISION_MARKED_RE.search(text): + reasons.append( + "handoff claims review submission blocked but also claims " + "server-side final decision was marked" + ) + + lease_claimed = _LEASE_NARRATIVE_RE.search(text) or ( + not _is_none_value(mcp_mutations) + and "lease" in mcp_mutations.lower() + ) + if lease_claimed and _is_none_value(review_decision): + reasons.append( + "reviewer lease acquired but Review decision field is missing or none" + ) + + if _REVIEW_SUBMIT_BLOCKED_RE.search(text) or _REJECTED_MUTATION_RE.search(text): + lower = text.lower() + missing_proof = [ + field + for field in _BLOCKED_REVIEW_PROOF_FIELDS + if field not in lower + ] + if missing_proof: + reasons.append( + "blocked/rejected mutation claim missing proof fields: " + + ", ".join(missing_proof) + ) + + if ( + _FINAL_DECISION_MARKED_RE.search(text) + and _is_none_value(review_mutations) + and not _ledger_mentions_review_submission(mcp_mutations) + and not _REVIEW_SUBMIT_BLOCKED_RE.search(text) + ): + reasons.append( + "final review decision marked but Review mutations ledger is empty" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "fields": fields, + } \ No newline at end of file diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index 73b9f8d..66133a2 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 @@ -195,4 +196,3 @@ If and only if a controller posts a durable `BOOTSTRAP REVIEW AUTHORIZATION `docs/bootstrap-review-path.md` Otherwise stop with BLOCKED + DIAGNOSE. Bootstrap never weakens normal PR gates. - diff --git a/skills/llm-project-workflow/templates/blocked-review-handoff.md b/skills/llm-project-workflow/templates/blocked-review-handoff.md new file mode 100644 index 0000000..88cb764 --- /dev/null +++ b/skills/llm-project-workflow/templates/blocked-review-handoff.md @@ -0,0 +1,22 @@ +# Blocked review submission handoff + +Use when `gitea_submit_pr_review` or the terminal mutation gate blocks review +submission. + +```text +## Blocked Review Submission Handoff + +- Tool called: gitea_submit_pr_review +- Mutation attempted: yes +- Mutation rejected: yes +- No server-side state changed: confirmed +- Proof/source: <paste exact tool error or gate reason> +- Prior terminal mutation that consumed budget: <exact prior mutation or none> +- Review decision (local intent): <approve | request_changes | comment only> +- Server-side final decision marked: <yes with tool proof | no> +- Gitea review submitted: no +- Gitea review blocked because: <exact gate/error> +- Review mutations: none (submission blocked) +- MCP/Gitea mutations: <only mutations that actually occurred> +- Safe next action: rewrite handoff with consistent ledger before posting +``` \ No newline at end of file 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: +<ready-for-author | in-progress | blocked | PR-open | needs-review | ready-to-merge | merged | closed | superseded> + +WHO_IS_NEXT: +<controller | author | reviewer | merger | reconciler | user> + +NEXT_ACTION: +<specific one-sentence action> + +NEXT_PROMPT: +<paste-ready prompt for the next role> + +WHAT_HAPPENED: +<latest meaningful event> + +WHY: +<decision rationale> + +RELATED_DISCUSSION: +<link/reference or none> + +RELATED_PRS: +- #... + +BRANCH: +<branch or none> + +HEAD_SHA: +<40-character SHA or none> + +VALIDATION: +<tests/proofs or none> + +BLOCKERS: +<blocker and unblock condition, or none> + +LAST_UPDATED_BY: +<identity/profile/date> +``` + +## PR + +```text +## Canonical PR State + +STATE: +<needs-review | changes-requested | approved | stale-approval | ready-to-merge | merged | blocked | superseded> + +WHO_IS_NEXT: +<controller | author | reviewer | merger | reconciler | user> + +NEXT_ACTION: +<specific one-sentence action> + +NEXT_PROMPT: +<paste-ready prompt for the next role> + +WHAT_HAPPENED: +<latest meaningful event> + +WHY: +<decision rationale> + +ISSUE: +#... + +BASE: +<branch> + +HEAD: +<branch> + +HEAD_SHA: +<40-character SHA> + +REVIEW_STATUS: +<none | approved | changes-requested | stale | contaminated> + +VALIDATION: +<tests/proofs> + +BLOCKERS: +<blockers or none> + +SUPERSEDES: +<PRs or none> + +SUPERSEDED_BY: +<PR or none> + +MERGE_READY: +<yes/no and why> + +LAST_UPDATED_BY: +<identity/profile/date> +``` + +## Discussion + +```text +## Canonical Discussion Summary + +STATE: +<needs-more-discussion | ready-for-issues | issues-created | closed> + +WHO_IS_NEXT: +<controller | author | reviewer | user> + +DECISION: +<what was decided> + +WHY: +<reasoning and tradeoffs> + +SUBSTANTIVE_COMMENTS: +<count and summary> + +ISSUES_TO_CREATE_OR_CREATED: +- #... + +DEPENDENCY_ORDER: +<order or none> + +NON_GOALS: +<non-goals> + +OPEN_QUESTIONS: +<questions or none> + +NEXT_ACTION: +<specific one-sentence action> + +NEXT_PROMPT: +<paste-ready prompt for the next role> + +LAST_UPDATED_BY: +<identity/profile/date> +``` diff --git a/skills/llm-project-workflow/templates/canonical-thread-handoff.md b/skills/llm-project-workflow/templates/canonical-thread-handoff.md new file mode 100644 index 0000000..37a76e5 --- /dev/null +++ b/skills/llm-project-workflow/templates/canonical-thread-handoff.md @@ -0,0 +1,36 @@ +# Template: Canonical Thread Handoff (CTH) + +Copy, fill the fields, and post as an issue or PR comment. + +```text +## CTH: <Type> + +Status: <current workflow state> +Next owner: <author|reviewer|merger|controller|operator> +Current blocker: <none or exact blocker> +Decision: <what was decided this session> +Proof: <commands, SHAs, gate outputs, or explicit pending proof> +Next action: <one concrete step for the next actor> +Ready-to-paste prompt: <full prompt the next session should paste> +``` + +Allowed `<Type>` values: + +- State Handoff +- Controller Decision +- Author Handoff +- Reviewer Handoff +- Merger Handoff +- Supersession Notice +- Blocker + +Rules: + +- Find the latest CTH comment in the thread before starting work. +- Treat the latest valid CTH as the current source of truth. +- Post a new CTH when you finish, block, skip, supersede, approve, request + changes, or hand off. +- A CTH summarizes workflow state; formal Gitea review verdicts remain + authoritative for merge gates. + +Full protocol: `docs/canonical-thread-handoff.md` \ No newline at end of file diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index 515ae10..5c4ce88 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -1,4 +1,4 @@ -# Template: merge a PR (eligible reviewer only) +# Template: merge a PR (eligible merger only) Copy, fill the `<...>` fields, and paste as the task prompt. @@ -10,12 +10,16 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): +- Find the latest CTH comment on the PR/issue thread before starting work. + Post a new CTH: Merger Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - Repository targeting (#530): pass explicit `remote=`, `org=`, and `repo=` on every gitea-tools call (e.g. `remote=prgs org=Scaled-Tech-Consulting repo=Gitea-Tools`). A bare `remote=prgs` can resolve to the wrong default repo and is blocked when it disagrees with the local git remote URL. -- Only an eligible, NON-author reviewer merges. If authenticated user == PR +- Only an eligible, NON-author merger merges. If authenticated user == PR author → STOP. +- Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - Do not merge unless the PR is open, mergeable, and its checks/review pass. - No force-merge, no bypassing branch protections. - If the PR is closed but `merged=false`, STOP and run reconciliation. Do not clean up. @@ -63,10 +67,12 @@ Then run the cleanup template (worktree-cleanup.md) in a reconciler session: - fetch/prune; confirm main checkout is clean and current (0 0). 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 +§K (long form — a merge is always high-risk), including the merger role +fields (Selected PR, Merger 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). + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. ``` diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 52026a0..bf854dc 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -36,6 +36,9 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): +- Find the latest CTH comment on the PR/issue thread before starting work. + Post a new CTH: Reviewer Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - Repository targeting (#530): pass explicit `remote=`, `org=`, and `repo=` on every gitea-tools call (e.g. `remote=prgs org=Scaled-Tech-Consulting repo=Gitea-Tools`). A bare `remote=prgs` can resolve to the wrong default repo @@ -110,8 +113,9 @@ Steps: - base branch unchanged - no undismissed REQUEST_CHANGES / blocking review state left unaccounted If anything moved → STOP, re-pin, re-validate before any verdict. -10. Post the review verdict: approve only if scope is clean and checks pass; - otherwise request changes with specifics. Never merge from this review step. + 10. Post the review verdict: approve only if scope is clean and checks pass; + otherwise request changes with specifics. Never merge from this review step. + Review and merge are separate workflow roles. A reviewer approval is not merge authorization. Include a "Review Metadata" block (attribution only — docs/llm-agent-sha.md): Review Metadata: @@ -128,6 +132,8 @@ 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). +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + Baseline failure proof (#533): if a validation command exits non-zero, do NOT call it a clean pass. Only label a failure "baseline"/"pre-existing" with pre-merge proof — the failure reproduced on the PR's pre-merge base commit, or a diff --git a/skills/llm-project-workflow/templates/start-issue.md b/skills/llm-project-workflow/templates/start-issue.md index be09053..572b556 100644 --- a/skills/llm-project-workflow/templates/start-issue.md +++ b/skills/llm-project-workflow/templates/start-issue.md @@ -10,6 +10,9 @@ Final report schema: skills/llm-project-workflow/schemas/work-issue-final-report Router: skills/llm-project-workflow/SKILL.md (task mode: work-issue) Rules (llm-project-workflow): +- Find the latest CTH comment on the issue/PR thread before starting work. + Post a new CTH: Author Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - No repo changes without a tracking issue. If none exists, create one first; if it can't be created, stop. - Work only in an isolated branch worktree under branches/. The main checkout diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 95cee0e..b830e8b 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -26,6 +26,12 @@ workflow rules exactly. ## 0. Load the canonical workflow first +Before starting PR work, **find the latest CTH comment** on the PR or linked +issue thread. Treat that CTH as the current handoff state. Post a new +**CTH: Reviewer Handoff**, **CTH: Merger Handoff**, **CTH: Blocker**, or +**CTH: Supersession Notice** when you finish, block, skip, supersede, +approve, request changes, or hand off. See `docs/canonical-thread-handoff.md`. + Before starting PR work, check whether the project provides a canonical PR review/merge workflow through a project skill, runbook, or MCP helper. If available, load it first and report: diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index c7f5747..41afd5d 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -26,6 +26,12 @@ This is an author/coder workflow. It is not a reviewer workflow. ## 0. Load the canonical workflow first +Before starting issue work, **find the latest CTH comment** on the target +issue or linked PR thread. Treat that CTH as the current handoff state unless +a newer authoritative gate supersedes it. Post a new **CTH: Author Handoff** +(or `CTH: Blocker`) when you finish, block, or hand off. +See `docs/canonical-thread-handoff.md`. + Before starting issue work, check whether the project provides a canonical work-on-issue workflow through a project skill, runbook, or MCP helper. If available, load it first and report: @@ -615,10 +621,39 @@ After push, report: If push fails, stop and produce a recovery handoff. -## 20A. Conflict-fix lease and push gate (#399) +## 20A. Live head re-pin before conflict-fix classification (#522) + +Open PR inventory `mergeable` / `head_sha` fields are **advisory and may be +stale**. Before classifying a PR as conflicted or creating a conflict-fix +worktree: + +1. Re-fetch the live PR (`gitea_view_pr` or equivalent) and record: + * inventory head SHA (if any) + * inventory mergeable (if any) + * **live** head SHA (full 40-char) + * **live** mergeable +2. Call `gitea_assess_conflict_fix_classification` with those values. +3. Act only on the returned classification and **pinned live head**: + * `stale_inventory_skip` / `live_mergeable_skip` → **do not** create a + conflict-fix worktree; do not mutate the PR branch for conflicts. + * `conflict_fix_needed` → conflict-fix worktree allowed for the pinned + live head only. + * `incomplete_live_repin` → stop; re-fetch live state. +4. If inventory head ≠ live head, report both SHAs and use the live head only. +5. If inventory says `mergeable:false` but live says `mergeable:true`, classify + as stale inventory and skip author conflict-fix mutation. + +Conflict-fix final reports that claim conflict-fix work must include: + +* citation of `gitea_assess_conflict_fix_classification` +* `Live head SHA: <40-char hex>` (or Pinned head SHA / Live PR head SHA) +* `Conflict-fix classification: <stale_inventory_skip|conflict_fix_needed|live_mergeable_skip|incomplete_live_repin>` + +## 20B. Conflict-fix lease and push gate (#399) When pushing to an existing PR branch to resolve merge conflicts: +0. Complete §20A live-head re-pin / classification first. 1. Call `gitea_acquire_conflict_fix_lease` before any push. 2. Call `gitea_assess_conflict_fix_push` immediately before `git push` with: * branch head before push diff --git a/task_capability_map.py b/task_capability_map.py index 8d3daf2..fb68bed 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -36,6 +36,10 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.issue.comment", "role": "author", }, + "create_label": { + "permission": "gitea.issue.comment", + "role": "author", + }, "create_branch": { "permission": "gitea.branch.create", "role": "author", @@ -66,7 +70,7 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { }, "merge_pr": { "permission": "gitea.pr.merge", - "role": "reviewer", + "role": "merger", }, "adopt_merger_pr_lease": { "permission": "gitea.pr.comment", @@ -163,6 +167,7 @@ ISSUE_MUTATION_TOOL_TASKS: dict[str, str] = { "gitea_create_issue_comment": "comment_issue", "gitea_mark_issue": "mark_issue", "gitea_set_issue_labels": "set_issue_labels", + "gitea_create_label": "create_label", "gitea_commit_files": "commit_files", } diff --git a/tests/test_canonical_comment_validator.py b/tests/test_canonical_comment_validator.py new file mode 100644 index 0000000..47e75d8 --- /dev/null +++ b/tests/test_canonical_comment_validator.py @@ -0,0 +1,217 @@ +"""Unit tests for canonical_comment_validator (#496).""" + +import unittest + +import canonical_comment_validator as ccv + +FULL_SHA = "a" * 40 + +_VALID_ISSUE = f"""## Canonical Issue State + +STATE: ready-for-author +WHO_IS_NEXT: author +NEXT_ACTION: Implement canonical comment validator and wire MCP gates +NEXT_PROMPT: +```text +You are the author LLM for issue #496. Add canonical_comment_validator.py +and wire fail-closed gates into gitea_create_issue_comment. +``` +WHAT_HAPPENED: Issue filed for MCP validation wall +WHY: Workflow comments must carry durable next-action state +RELATED_PRS: none +BLOCKERS: none +VALIDATION: unit tests not yet run +LAST_UPDATED_BY: prgs-author +""" + +_VALID_PR = f"""## Canonical PR State + +STATE: ready-for-review +WHO_IS_NEXT: reviewer +NEXT_ACTION: Run focused pytest and review the validator wiring diff +NEXT_PROMPT: +```text +Review PR for issue #496. Confirm casual comments still post and workflow +comments without canonical fields are rejected before the Gitea API call. +``` +WHAT_HAPPENED: Author opened PR with validator module +WHY: Fail-closed enforcement belongs at the MCP boundary +ISSUE: #496 +HEAD_SHA: {FULL_SHA} +REVIEW_STATUS: pending +MERGE_READY: false +BLOCKERS: none +VALIDATION: not yet run +LAST_UPDATED_BY: prgs-author +""" + +_VALID_SUPERSEDED = """## Canonical PR State + +STATE: superseded +WHO_IS_NEXT: reconciler +NEXT_ACTION: Close superseded issue after canonical issue #495 lands +NEXT_PROMPT: +```text +Close issue #494 as superseded by #495 once templates and docs are merged. +``` +WHY: Duplicate tracking issue replaced by durable canonical template work +CANONICAL_ITEM: issue #495 +SUPERSEDED_ITEM: issue #494 +CLOSE_OR_KEEP_OPEN: close superseded issue #494 after #495 merges +""" + +_VALID_BLOCKED = """## Canonical Issue State + +STATE: blocked +WHO_IS_NEXT: author +NEXT_ACTION: Fix failing validator unit tests before requesting re-review +NEXT_PROMPT: +```text +Run pytest tests/test_canonical_comment_validator.py and repair failures. +``` +WHAT_HAPPENED: Validator tests failed in CI +WHY: Blocked until test suite is green +RELATED_PRS: PR #500 +BLOCKERS: pytest failures; unblock after all validator tests pass +VALIDATION: pytest failed +LAST_UPDATED_BY: prgs-author +""" + +_VALID_READY_TO_MERGE = f"""## Canonical PR State + +STATE: ready-to-merge +WHO_IS_NEXT: merger +NEXT_ACTION: Merge PR after confirming approval_at_current_head +NEXT_PROMPT: +```text +Merge PR #500 for issue #496 after live mergeable check passes. +``` +WHAT_HAPPENED: Reviewer approved at current head +WHY: All gates passed and head SHA is current +ISSUE: #496 +HEAD_SHA: {FULL_SHA} +REVIEW_STATUS: approved / approval_at_current_head +MERGE_READY: true +BLOCKERS: none +VALIDATION: pytest passed; reviewer approved at head {FULL_SHA} +LAST_UPDATED_BY: prgs-reviewer +""" + + +class TestCanonicalCommentDetection(unittest.TestCase): + def test_casual_comment_not_workflow(self): + self.assertFalse(ccv.is_workflow_changing_comment("Thanks, I will check this.")) + + def test_workflow_trigger_detected(self): + self.assertTrue(ccv.is_workflow_changing_comment("Blocked, author should fix.")) + + def test_machine_marker_exempt(self): + body = "<!-- mcp-review-lease:v1 -->\nphase: claimed" + self.assertFalse(ccv.is_workflow_changing_comment(body)) + + +class TestCanonicalCommentValidation(unittest.TestCase): + def test_casual_comment_allowed(self): + result = ccv.assess_canonical_comment("Thanks, I will check this.") + self.assertTrue(result["allowed"]) + self.assertFalse(result["is_workflow_comment"]) + + def test_workflow_comment_missing_fields_rejected(self): + result = ccv.assess_canonical_comment("Blocked, author should fix.") + self.assertFalse(result["allowed"]) + self.assertTrue(result["is_workflow_comment"]) + self.assertIn("WHO_IS_NEXT", result["missing_fields"]) + self.assertIn("correction_message", result) + self.assertIn("Suggested template", result["correction_message"]) + + def test_missing_next_prompt_rejected(self): + body = """STATE: blocked +WHO_IS_NEXT: author +NEXT_ACTION: Repair the failing validator unit tests in CI +WHY: Tests must pass before merge +BLOCKERS: pytest failed; unblock after tests pass +VALIDATION: failed +""" + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("NEXT_PROMPT", result["missing_fields"] + result["vague_fields"]) + + def test_vague_next_action_rejected(self): + body = _VALID_ISSUE.replace( + "Implement canonical comment validator and wire MCP gates", + "continue", + ) + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("NEXT_ACTION", result["vague_fields"]) + + def test_invalid_who_is_next_rejected(self): + body = _VALID_ISSUE.replace("WHO_IS_NEXT: author", "WHO_IS_NEXT: llm") + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("WHO_IS_NEXT", result["vague_fields"]) + + def test_valid_issue_comment_allowed(self): + result = ccv.assess_canonical_comment(_VALID_ISSUE) + self.assertTrue(result["allowed"]) + + def test_issue_comment_mentioning_pr_requires_related_prs(self): + body = _VALID_ISSUE.replace("RELATED_PRS: none", "RELATED_PRS:") + body = body.replace("Issue filed", "Issue filed; see PR #500") + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("RELATED_PRS", result["missing_fields"]) + + def test_pr_review_requires_head_sha(self): + body = _VALID_PR.replace(f"HEAD_SHA: {FULL_SHA}", "HEAD_SHA:") + result = ccv.assess_canonical_comment(body, context="pr_review") + self.assertFalse(result["allowed"]) + self.assertIn("HEAD_SHA", result["missing_fields"]) + + def test_ready_to_merge_requires_approval_and_sha_proof(self): + body = _VALID_READY_TO_MERGE.replace("approval_at_current_head", "pending") + body = body.replace(f"HEAD_SHA: {FULL_SHA}", "HEAD_SHA: pending") + body = body.replace("reviewer approved at head", "reviewer pending at head") + body = body.replace("approved /", "pending /") + result = ccv.assess_canonical_comment(body, context="pr_comment") + self.assertFalse(result["allowed"]) + self.assertTrue(result["extra_reasons"]) + + def test_ready_to_merge_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_READY_TO_MERGE, context="pr_comment") + self.assertTrue(result["allowed"]) + + def test_superseded_requires_canonical_and_superseded_items(self): + body = _VALID_SUPERSEDED.replace("CANONICAL_ITEM: issue #495", "CANONICAL_ITEM:") + result = ccv.assess_canonical_comment(body, context="supersession") + self.assertFalse(result["allowed"]) + self.assertIn("CANONICAL_ITEM", result["missing_fields"]) + + def test_superseded_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_SUPERSEDED, context="supersession") + self.assertTrue(result["allowed"]) + + def test_blocked_requires_unblock_condition(self): + body = _VALID_BLOCKED.replace( + "BLOCKERS: pytest failures; unblock after all validator tests pass", + "BLOCKERS: pytest failures", + ) + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("BLOCKERS", result["vague_fields"]) + + def test_blocked_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_BLOCKED) + self.assertTrue(result["allowed"]) + + def test_correction_lists_missing_fields(self): + result = ccv.assess_canonical_comment("ready for merge") + self.assertFalse(result["allowed"]) + message = result["correction_message"] + self.assertIn("Missing fields", message) + self.assertIn("WHO_IS_NEXT", message) + self.assertIn("Suggested template", message) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file 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_canonical_thread_handoff.py b/tests/test_canonical_thread_handoff.py new file mode 100644 index 0000000..96b6882 --- /dev/null +++ b/tests/test_canonical_thread_handoff.py @@ -0,0 +1,139 @@ +"""Tests for Canonical Thread Handoff (CTH) protocol (#505).""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import canonical_thread_handoff as cth # noqa: E402 + +REPO_ROOT = Path(__file__).resolve().parent.parent +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +PROTOCOL_DOC = REPO_ROOT / "docs" / "canonical-thread-handoff.md" +TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "canonical-thread-handoff.md" +REVIEW_WORKFLOW = REPO_ROOT / "skills" / "llm-project-workflow/workflows" / "review-merge-pr.md" +WORK_WORKFLOW = REPO_ROOT / "skills" / "llm-project-workflow/workflows" / "work-issue.md" +REVIEW_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "review-pr.md" +START_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "start-issue.md" +MERGE_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "merge-pr.md" + + +class TestCthModule(unittest.TestCase): + def test_defines_cth_term_and_types(self): + self.assertEqual(cth.CTH_ABBREV, "CTH") + self.assertIn("State Handoff", cth.CTH_TYPES) + self.assertIn("Reviewer Handoff", cth.CTH_TYPES) + self.assertIn("Blocker", cth.CTH_TYPES) + + def test_format_and_parse_round_trip(self): + body = cth.format_cth_body( + cth_type="Author Handoff", + status="implementation_complete", + next_owner="reviewer", + current_blocker="none", + decision="PR opened", + proof="pytest passed", + next_action="review PR #9", + ready_to_paste_prompt="Review PR #9 for issue #5", + ) + parsed = cth.parse_cth_comment(body) + self.assertIsNotNone(parsed) + self.assertEqual(parsed["cth_type"], "Author Handoff") + self.assertEqual(parsed["fields"]["status"], "implementation_complete") + self.assertEqual(parsed["fields"]["next owner"], "reviewer") + + def test_valid_cth_passes_assessment(self): + body = cth.format_cth_body( + cth_type="Reviewer Handoff", + status="approved", + next_owner="merger", + current_blocker="none", + decision="APPROVE", + proof="review id 42", + next_action="merge if gates pass", + ready_to_paste_prompt="Merge PR #12 with pinned head abcdef0123456789abcdef0123456789abcdef01", + ) + result = cth.assess_cth_comment(body) + self.assertFalse(result["block"]) + self.assertTrue(result["valid"]) + + def test_missing_fields_fail_closed(self): + body = "## CTH: Blocker\n\nStatus: blocked\n" + result = cth.assess_cth_comment(body) + self.assertTrue(result["block"]) + self.assertIn("missing required fields", result["reasons"][0]) + + def test_find_latest_cth_prefers_newer_comment(self): + older = cth.format_cth_body( + cth_type="State Handoff", + status="old", + next_owner="author", + current_blocker="none", + decision="old", + proof="old", + next_action="old", + ready_to_paste_prompt="old prompt text here", + ) + newer = cth.format_cth_body( + cth_type="Reviewer Handoff", + status="new", + next_owner="merger", + current_blocker="none", + decision="approve", + proof="new", + next_action="merge", + ready_to_paste_prompt="merge PR #3 now with pinned head", + ) + comments = [ + {"id": 1, "created_at": "2026-07-01T10:00:00Z", "body": older}, + {"id": 2, "created_at": "2026-07-02T10:00:00Z", "body": newer}, + ] + latest = cth.find_latest_cth(comments) + self.assertEqual(latest["comment_id"], 2) + self.assertEqual(latest["cth_type"], "Reviewer Handoff") + + def test_cth_does_not_replace_formal_reviews_documented_in_protocol(self): + text = PROTOCOL_DOC.read_text(encoding="utf-8") + self.assertIn("do not replace", text.lower()) + self.assertIn("formal Gitea review", text) + + def test_examples_cover_required_scenarios(self): + text = PROTOCOL_DOC.read_text(encoding="utf-8") + expected_phrases = ( + "PR approved and ready for merger", + "request-changes back to author", + "superseded PR closure", + "dirty root/worktree", + "Stale head requiring fresh review", + "Issue implementation handoff", + ) + for phrase in expected_phrases: + self.assertIn(phrase, text) + + +class TestCthDocumentationChecks(unittest.TestCase): + def test_runbook_references_cth_discovery_and_posting(self): + text = RUNBOOK.read_text(encoding="utf-8") + self.assertIn("Canonical Thread Handoff", text) + self.assertIn("Find the latest CTH comment", text) + self.assertIn("Post a new CTH", text) + self.assertIn("canonical-thread-handoff.md", text) + + def test_workflows_reference_cth_rules(self): + for path in (REVIEW_WORKFLOW, WORK_WORKFLOW): + text = path.read_text(encoding="utf-8") + self.assertIn("latest CTH comment", text) + self.assertIn("canonical-thread-handoff.md", text) + + def test_prompt_templates_reference_cth_rules(self): + for path in (REVIEW_TEMPLATE, START_TEMPLATE, MERGE_TEMPLATE, TEMPLATE): + text = path.read_text(encoding="utf-8") + self.assertIn("CTH", text) + self.assertIn("canonical-thread-handoff", text) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_config_v2_contexts.py b/tests/test_config_v2_contexts.py index 71d3a7b..feb9a80 100644 --- a/tests/test_config_v2_contexts.py +++ b/tests/test_config_v2_contexts.py @@ -435,6 +435,40 @@ class ValidateConfigTests(unittest.TestCase): with open(example, encoding="utf-8") as fh: self.assertEqual(gitea_config.validate_config(json.load(fh)), []) + def test_repo_example_file_reviewer_vs_merger_boundaries(self): + example_path = __import__("pathlib").Path(__file__).resolve().parent.parent \ + / "gitea-mcp.v2-contexts.example.json" + with open(example_path, encoding="utf-8") as fh: + cfg = json.load(fh) + + # 1. Config includes separate reviewer and merger examples + self.assertIn("example-reviewer", cfg["profiles"]) + self.assertIn("example-merger", cfg["profiles"]) + + reviewer = cfg["profiles"]["example-reviewer"] + merger = cfg["profiles"]["example-merger"] + + # 2. Example reviewer config does not include merge capability + reviewer_allowed = reviewer.get("allowed_operations") or [] + reviewer_forbidden = reviewer.get("forbidden_operations") or [] + + self.assertNotIn("merge", reviewer_allowed) + self.assertNotIn("gitea.pr.merge", reviewer_allowed) + self.assertIn("merge", reviewer_forbidden) + + # 3. Example reviewer can resolve review task, cannot resolve merge task + # 4. Example merger can resolve merge task + rev_review_ok, _ = gitea_config.check_operation("gitea.pr.review", reviewer_allowed, reviewer_forbidden) + rev_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", reviewer_allowed, reviewer_forbidden) + + merg_allowed = merger.get("allowed_operations") or [] + merg_forbidden = merger.get("forbidden_operations") or [] + merg_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", merg_allowed, merg_forbidden) + + self.assertTrue(rev_review_ok, "example-reviewer must allow review") + self.assertFalse(rev_merge_ok, "example-reviewer must not allow merge") + self.assertTrue(merg_merge_ok, "example-merger must allow merge") + def test_broken_contexts_config_reports_problems(self): data = contexts_config() data["profiles"]["prgs-author"]["context"] = "nope" diff --git a/tests/test_conflict_fix_classification.py b/tests/test_conflict_fix_classification.py new file mode 100644 index 0000000..0c1ea82 --- /dev/null +++ b/tests/test_conflict_fix_classification.py @@ -0,0 +1,142 @@ +"""Tests for live PR head re-pin before conflict-fix classification (#522).""" + +from __future__ import annotations + +import unittest + +from conflict_fix_classification import ( + CLASSIFICATION_CONFLICT_FIX_NEEDED, + CLASSIFICATION_INCOMPLETE, + CLASSIFICATION_LIVE_MERGEABLE, + CLASSIFICATION_STALE_INVENTORY_SKIP, + assess_conflict_fix_classification, + assess_conflict_fix_classification_final_report, +) + + +def _sha(prefix: str) -> str: + return (prefix + "0" * 40)[:40] + + +class TestConflictFixClassification(unittest.TestCase): + def test_stale_inventory_mergeable_skip(self): + """PR #508 style: inventory mergeable:false/stale head, live mergeable:true.""" + result = assess_conflict_fix_classification( + pr_number=508, + inventory_head_sha=_sha("dad1dc8"), + inventory_mergeable=False, + live_head_sha=_sha("3f3d6cb"), + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_STALE_INVENTORY_SKIP) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertTrue(result["inventory_stale_mergeable"]) + self.assertEqual(result["pinned_head_sha"], _sha("3f3d6cb")) + + def test_stale_inventory_head_only_live_mergeable_true(self): + """PR #493 style: head moved; live still mergeable.""" + result = assess_conflict_fix_classification( + pr_number=493, + inventory_head_sha=_sha("685e627"), + inventory_mergeable=True, + live_head_sha=_sha("4561d7a"), + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_STALE_INVENTORY_SKIP) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertEqual(result["pinned_head_sha"], _sha("4561d7a")) + + def test_live_conflict_allows_worktree_on_pinned_head(self): + result = assess_conflict_fix_classification( + pr_number=99, + inventory_head_sha=_sha("aaaaaaa"), + inventory_mergeable=False, + live_head_sha=_sha("bbbbbbb"), + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_CONFLICT_FIX_NEEDED) + self.assertTrue(result["worktree_allowed"]) + self.assertFalse(result["skip_author_mutation"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertEqual(result["pinned_head_sha"], _sha("bbbbbbb")) + + def test_matching_inventory_live_mergeable_skip(self): + head = _sha("cccccccc") + result = assess_conflict_fix_classification( + pr_number=10, + inventory_head_sha=head, + inventory_mergeable=True, + live_head_sha=head, + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_LIVE_MERGEABLE) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["inventory_stale_head"]) + + def test_missing_live_head_incomplete(self): + result = assess_conflict_fix_classification( + pr_number=1, + inventory_head_sha=_sha("ddddddd"), + inventory_mergeable=False, + live_head_sha=None, + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_INCOMPLETE) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["skip_author_mutation"]) + self.assertTrue(any("live PR head" in r for r in result["reasons"])) + + def test_short_sha_rejected(self): + result = assess_conflict_fix_classification( + pr_number=1, + inventory_head_sha="abc1234", + inventory_mergeable=False, + live_head_sha="def5678", + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_INCOMPLETE) + self.assertFalse(result["worktree_allowed"]) + + +class TestConflictFixClassificationFinalReport(unittest.TestCase): + def test_non_conflict_report_not_applicable(self): + result = assess_conflict_fix_classification_final_report( + "Implemented feature without merge issues." + ) + self.assertTrue(result["proven"]) + self.assertFalse(result["applicable"]) + + def test_conflict_report_requires_tool_live_head_classification(self): + result = assess_conflict_fix_classification_final_report( + "Did conflict-fix work on PR #99." + ) + self.assertFalse(result["proven"]) + self.assertTrue(result["applicable"]) + joined = " ".join(result["reasons"]) + self.assertIn("gitea_assess_conflict_fix_classification", joined) + self.assertIn("Live head SHA", joined) + self.assertIn("classification", joined.lower()) + + def test_good_conflict_report_passes(self): + live = _sha("3f3d6cb") + inv = _sha("dad1dc8") + text = f""" +Conflict-fix classification: stale_inventory_skip +Called gitea_assess_conflict_fix_classification before worktree creation. +Inventory head SHA: {inv} +Live head SHA: {live} +Use live head only; skip author mutation. +""" + result = assess_conflict_fix_classification_final_report(text) + self.assertTrue(result["proven"], msg=result.get("reasons")) + self.assertEqual(result["live_head_sha"], live) + self.assertEqual(result["inventory_head_sha"], inv) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_create_issue.py b/tests/test_create_issue.py index b2fdb68..11eb846 100644 --- a/tests/test_create_issue.py +++ b/tests/test_create_issue.py @@ -103,6 +103,37 @@ class TestAPIPayload(unittest.TestCase): self.assertEqual(payload["title"], "My Title") self.assertEqual(payload["body"], "My Body") + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) + def test_applies_workflow_labels_by_name(self, _cred): + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and url.endswith("/labels"): + return [ + {"id": 1, "name": "type:feature"}, + {"id": 2, "name": "status:ready"}, + ] + if method == "POST" and url.endswith("/issues"): + return {"number": 7, "html_url": "http://x/7"} + if method == "PUT" and url.endswith("/issues/7/labels"): + return [{"name": "type:feature"}, {"name": "status:ready"}] + return {} + + with patch("create_issue.api_request", side_effect=api_side_effect) as mock_api: + rc = create_issue.main([ + "--title", "My Title", + "--type-label", "feature", + "--status-label", "ready", + ]) + self.assertEqual(rc, 0) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 2]}) + + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) + def test_require_workflow_labels_blocks_missing_labels(self, _cred): + with patch("create_issue.api_request") as mock_api: + rc = create_issue.main(["--title", "My Title", "--require-workflow-labels"]) + self.assertEqual(rc, 1) + mock_api.assert_not_called() + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) def test_url_construction(self, _cred): with patch("create_issue.api_request", diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 7c954b0..123e828 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -373,6 +373,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.""" @@ -601,6 +687,31 @@ class TestReconcilerCloseProof(unittest.TestCase): ) +class TestCanonicalCommentPostClaim(unittest.TestCase): + def test_blocks_post_claim_when_validator_rejected_in_report(self): + report = "\n".join([ + "## Controller Handoff", + "- MCP/Gitea mutations: issue comment posted", + "- Issue comments posted: 1 canonical state comment", + "canonical comment validation failed (fail closed before posting).", + '- canonical_comment_validation: {"allowed": false}', + ]) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("shared.canonical_comment_post_claim", rule_ids) + self.assertTrue(result["blocked"]) + + def test_allows_none_when_validator_rejected_without_post_claim(self): + report = "\n".join([ + "## Controller Handoff", + "- Issue comments posted: none", + "canonical comment validation failed (fail closed before posting).", + ]) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertNotIn("shared.canonical_comment_post_claim", rule_ids) + + class TestEntryPoint(unittest.TestCase): def test_unknown_task_kind_blocks(self): result = assess_final_report_validator("report", "unknown_mode") @@ -616,8 +727,100 @@ class TestEntryPoint(unittest.TestCase): def test_supported_task_kinds_include_review_and_reconcile(self): self.assertIn("review_pr", FINAL_REPORT_TASK_KINDS) + self.assertIn("merge_pr", FINAL_REPORT_TASK_KINDS) self.assertIn("reconcile_already_landed", FINAL_REPORT_TASK_KINDS) +class TestMergePrRules(unittest.TestCase): + def test_clean_merger_report_passes(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + "- Active profile: prgs-merger", + "- Role kind: merger", + "- Merge capability source: profile", + "- Explicit operator authorization: MERGE PR 203", + "- Expected head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Approval at current head: yes", + "- Merge result: PR merged successfully", + "- Cleanup status: complete", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertEqual(result["grade"], "A") + self.assertFalse(result["blocked"]) + + def test_missing_merger_fields_blocks(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertTrue(result["downgraded"]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_issue_workflow_labels.py b/tests/test_issue_workflow_labels.py new file mode 100644 index 0000000..e7fd143 --- /dev/null +++ b/tests/test_issue_workflow_labels.py @@ -0,0 +1,73 @@ +"""Tests for canonical issue workflow label policy (#513).""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import issue_workflow_labels as labels # noqa: E402 + + +class TestIssueWorkflowLabelValidation(unittest.TestCase): + def test_valid_labeled_issue(self): + result = labels.assess_issue_labels(["type:feature", "status:ready"]) + self.assertTrue(result["valid"]) + self.assertEqual(result["type_labels"], ["type:feature"]) + self.assertEqual(result["status_labels"], ["status:ready"]) + + def test_issue_missing_type_label(self): + result = labels.assess_issue_labels(["status:ready"]) + self.assertFalse(result["valid"]) + self.assertIn("issue is missing a type:* label", result["errors"]) + + def test_issue_missing_status_label(self): + result = labels.assess_issue_labels(["type:bug"]) + self.assertFalse(result["valid"]) + self.assertIn("issue is missing a status:* label", result["errors"]) + + def test_discussion_issue_missing_type_discussion(self): + result = labels.assess_issue_labels( + ["type:feature", "status:triage"], + discussion=True, + ) + self.assertFalse(result["valid"]) + self.assertIn("discussion issue is missing type:discussion", result["errors"]) + + def test_multiple_conflicting_status_labels(self): + result = labels.assess_issue_labels( + ["type:feature", "status:ready", "status:blocked"] + ) + self.assertFalse(result["valid"]) + self.assertTrue( + any("multiple active status:* labels" in err for err in result["errors"]) + ) + + +class TestIssueWorkflowStatusTransitions(unittest.TestCase): + def test_status_transition_removes_old_status_label(self): + result = labels.transition_status_labels( + ["type:feature", "status:ready", "workflow"], + "in-progress", + ) + self.assertEqual(result, ["type:feature", "workflow", "status:in-progress"]) + + def test_pr_open_transition_applies_status_pr_open(self): + result = labels.transition_status_labels( + ["type:feature", "status:in-progress"], + "pr-open", + ) + self.assertEqual(result, ["type:feature", "status:pr-open"]) + + def test_duplicate_transition_applies_status_duplicate(self): + result = labels.transition_status_labels( + ["type:process", "status:triage"], + "duplicate", + ) + self.assertEqual(result, ["type:process", "status:duplicate"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_issue_write_tool_gates.py b/tests/test_issue_write_tool_gates.py index 736b295..f91c514 100644 --- a/tests/test_issue_write_tool_gates.py +++ b/tests/test_issue_write_tool_gates.py @@ -164,13 +164,14 @@ class TestAllowedIssueWritesProceed(IssueWriteGateBase): result = mcp_server.gitea_close_issue(issue_number=9, remote="prgs") self.assertTrue(result.get("success")) + @patch("mcp_server.api_get_all", return_value=[{"id": 10, "name": "status:in-progress"}]) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value="token author-pass") - def test_resolver_allowed_mark_issue_proceeds(self, _auth, mock_api): + def test_resolver_allowed_mark_issue_proceeds(self, _auth, mock_api, _labels): def api_side_effect(method, url, auth, payload=None): - if method == "GET" and url.endswith("/labels?limit=100"): - return [{"id": 10, "name": "status:in-progress"}] - if method == "POST" and "/issues/9/labels" in url: + if method == "GET" and "/issues/9" in url: + return {"labels": []} + if method == "PUT" and "/issues/9/labels" in url: return [{"name": "status:in-progress"}] if method == "POST" and "/issues/9/comments" in url: return {"id": 501} @@ -209,4 +210,4 @@ class TestCreateIssueMissingPermissionReport(IssueWriteGateBase): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/test_manage_labels.py b/tests/test_manage_labels.py index cdccf76..ca008a1 100644 --- a/tests/test_manage_labels.py +++ b/tests/test_manage_labels.py @@ -11,6 +11,7 @@ from unittest.mock import MagicMock, call, patch sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) import manage_labels # noqa: E402 +import issue_workflow_labels # noqa: E402 FAKE_AUTH = "Basic dGVzdDp0ZXN0" # base64("test:test") @@ -129,6 +130,11 @@ class TestConstants(unittest.TestCase): names = [l["name"] for l in manage_labels.LABELS] self.assertIn("status:in-progress", names) + def test_canonical_workflow_labels_are_defined(self): + names = {l["name"] for l in manage_labels.LABELS} + for spec in issue_workflow_labels.CANONICAL_LABEL_SPECS: + self.assertIn(spec.name, names) + def test_all_mapped_labels_are_defined(self): defined = {l["name"] for l in manage_labels.LABELS} for issue, names in manage_labels.MAPPING.items(): diff --git a/tests/test_mcp_daemon_guard.py b/tests/test_mcp_daemon_guard.py new file mode 100644 index 0000000..45007a2 --- /dev/null +++ b/tests/test_mcp_daemon_guard.py @@ -0,0 +1,67 @@ +"""Tests for sanctioned MCP daemon guards (#558).""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_daemon_guard +import gitea_auth + + +class TestMcpDaemonGuard(unittest.TestCase): + def test_unsanctioned_blocks_mutation_runtime(self): + env = {k: v for k, v in os.environ.items() if k not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + "PYTEST_CURRENT_TEST", + }} + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + mcp_daemon_guard.assert_sanctioned_mutation_runtime("test") + + def test_pytest_allows_runtime(self): + # Running under pytest already sets PYTEST_CURRENT_TEST. + mcp_daemon_guard.assert_sanctioned_mutation_runtime("pytest") + + def test_mark_sanctioned_allows(self): + env = {k: v for k, v in os.environ.items() if k != "PYTEST_CURRENT_TEST"} + env[mcp_daemon_guard.SANCTIONED_DAEMON_ENV] = "1" + with patch.dict(os.environ, env, clear=True): + mcp_daemon_guard.assert_sanctioned_mutation_runtime("daemon") + + def test_keychain_blocked_without_sanction(self): + env = { + k: v + for k, v in os.environ.items() + if k + not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + mcp_daemon_guard.ALLOW_KEYCHAIN_CLI_ENV, + "PYTEST_CURRENT_TEST", + } + } + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + mcp_daemon_guard.assert_keychain_access_allowed() + + def test_get_auth_header_blocked_when_unsanctioned(self): + env = { + k: v + for k, v in os.environ.items() + if k + not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + "PYTEST_CURRENT_TEST", + } + } + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + gitea_auth.get_auth_header("gitea.prgs.cc") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 39a3e3c..00c1da8 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -91,6 +91,16 @@ def _visible_approval_reviews(reviewer="reviewer-bot", sha="abc123"): _DEFAULT_LEASE_SESSION = "mcp-test-reviewer-lease" _NO_PR_WORK_LEASE_BLOCK = {"block": False, "reasons": [], "mutation_allowed": True} +WORKFLOW_REPO_LABELS = [ + {"id": 1, "name": "type:feature"}, + {"id": 2, "name": "status:in-progress"}, + {"id": 3, "name": "status:ready"}, + {"id": 4, "name": "status:pr-open"}, +] + + +def _issue_with_labels(*names): + return {"labels": [{"name": name} for name in names]} def _reviewer_lease_comment( @@ -305,6 +315,22 @@ class TestCreateIssue(unittest.TestCase): self.assertIn("gitea.prgs.cc", url) self.assertIn("Scaled-Tech-Consulting", url) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", + return_value=(True, [])) + @patch("mcp_server.api_request") + @patch("mcp_server.api_get_all", return_value=[]) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_issue_required_workflow_labels_blocks(self, _auth, _get_all, mock_api, _role): + with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): + result = gitea_create_issue( + title="Test issue", + require_workflow_labels=True, + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("issue is missing a type:* label", result["reasons"]) + mock_api.assert_not_called() + # --------------------------------------------------------------------------- # Create PR @@ -315,13 +341,24 @@ class TestCreatePR(unittest.TestCase): "mcp_server.issue_duplicate_context_fetcher", return_value=([], [], {"status": "not_claimed"}), ) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_creates_pr(self, _auth, mock_api, _role, _dup_fetcher): + def test_creates_pr(self, _auth, mock_api, _role, _labels, _dup_fetcher): worktree = os.path.realpath(os.getcwd()) - mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/123" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 3, "html_url": "https://example.com/pulls/3"} + if method == "PUT" and url.endswith("/issues/123/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect with tempfile.TemporaryDirectory() as lock_dir: env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} with patch.dict(os.environ, env, clear=True): @@ -334,22 +371,37 @@ class TestCreatePR(unittest.TestCase): ) self.assertEqual(result["number"], 3) self.assertNotIn("url", result) - payload = mock_api.call_args[0][3] + post_call = next(c for c in mock_api.call_args_list if c[0][0] == "POST") + payload = post_call[0][3] self.assertEqual(payload["head"], "feat/x") self.assertEqual(payload["base"], "main") self.assertIn("Closes #123", payload["title"]) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 4]}) + self.assertTrue(result["issue_status_transition"]["performed"]) @patch( "mcp_server.issue_duplicate_context_fetcher", return_value=([], [], {"status": "not_claimed"}), ) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _dup_fetcher): + def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _labels, _dup_fetcher): worktree = os.path.realpath(os.getcwd()) - mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/123" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 3, "html_url": "https://example.com/pulls/3"} + if method == "PUT" and url.endswith("/issues/123/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect with tempfile.TemporaryDirectory() as lock_dir: env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir, "GITEA_MCP_REVEAL_ENDPOINTS": "1"} with patch.dict(os.environ, env, clear=True): @@ -362,6 +414,38 @@ class TestCreatePR(unittest.TestCase): ) self.assertIn("pulls/3", result["url"]) + @patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) + @patch("mcp_server.api_get_all", return_value=[{"id": 1, "name": "type:feature"}]) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", + return_value=(True, [])) + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_pr_blocks_when_pr_open_status_label_missing( + self, _auth, mock_api, _role, _labels, _dup_fetcher + ): + worktree = os.path.realpath(os.getcwd()) + mock_api.return_value = _issue_with_labels("type:feature", "status:in-progress") + with tempfile.TemporaryDirectory() as lock_dir: + env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} + with patch.dict(os.environ, env, clear=True): + _bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree) + result = gitea_create_pr( + title="feat: X Closes #123", + head="feat/x", + base="main", + worktree_path=worktree, + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("status:pr-open", result["reasons"][0]) + self.assertFalse( + any(c[0][0] == "POST" and c[0][1].endswith("/pulls") + for c in mock_api.call_args_list) + ) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @@ -486,26 +570,33 @@ class TestViewIssue(unittest.TestCase): # --------------------------------------------------------------------------- class TestMarkIssue(unittest.TestCase): + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_start_adds_label(self, _auth, mock_api): - # labels, add label, post claim heartbeat comment - mock_api.side_effect = [ - [{"id": 10, "name": "status:in-progress"}], - [{"name": "status:in-progress"}], - {"id": 99}, - ] + def test_start_adds_label(self, _auth, mock_api, _labels): + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/5" in url: + return _issue_with_labels("type:feature", "status:ready") + if method == "PUT" and url.endswith("/issues/5/labels"): + return [{"name": "type:feature"}, {"name": "status:in-progress"}] + if method == "POST" and url.endswith("/issues/5/comments"): + return {"id": 99} + return {} + + mock_api.side_effect = api_side_effect with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): result = gitea_mark_issue(issue_number=5, action="start") self.assertTrue(result["success"]) self.assertIn("claimed", result["message"]) self.assertTrue(result.get("heartbeat_posted")) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 2]}) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_done_removes_label(self, _auth, mock_api): + def test_done_removes_label(self, _auth, mock_api, _labels): mock_api.side_effect = [ - [{"id": 10, "name": "status:in-progress"}], None, ] with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): @@ -517,10 +608,10 @@ class TestMarkIssue(unittest.TestCase): with self.assertRaises(ValueError): gitea_mark_issue(issue_number=5, action="pause") + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_missing_label_raises(self, _auth, mock_api): - mock_api.return_value = [] # no labels exist + def test_missing_label_raises(self, _auth, mock_api, _labels): with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): with self.assertRaises(RuntimeError): gitea_mark_issue(issue_number=5, action="start") @@ -861,6 +952,19 @@ class TestMergePR(unittest.TestCase): # -- identity / profile / eligibility fail-closed ------------------------- + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_reviewer_profile_cannot_merge(self, _auth, mock_api): + env = {"GITEA_PROFILE_NAME": "prgs-reviewer", + "GITEA_ALLOWED_OPERATIONS": "read,approve"} + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs" + ) + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) + mock_api.assert_not_called() + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_self_author_cannot_merge(self, _auth, mock_api): @@ -902,14 +1006,13 @@ class TestMergePR(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_profile_without_merge_permission_blocks(self, _auth, mock_api): - mock_api.side_effect = [{"login": "merger-bot"}, self._pr("author-bot")] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") - self.assertFalse(r["performed"]) - self.assertIn("profile is not allowed to merge", r["reasons"]) + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs") + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) self._assert_no_merge_call(mock_api) # -- PR state / mergeability ---------------------------------------------- @@ -3116,6 +3219,36 @@ class TestIssueCommentTools(unittest.TestCase): self.assertFalse(result["success"]) mock_api.assert_not_called() + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_casual_comment_allowed(self, _auth, mock_api): + mock_api.return_value = self._comment(556, "gitea-author", "casual") + with patch.dict(os.environ, self.AUTHOR_ENV, clear=True): + result = gitea_create_issue_comment( + issue_number=9, + body="Thanks, I will check this.", + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertTrue(result["performed"]) + mock_api.assert_called_once() + + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_workflow_comment_missing_fields_blocked(self, _auth, mock_api): + with patch.dict(os.environ, self.AUTHOR_ENV, clear=True): + result = gitea_create_issue_comment( + issue_number=9, + body="Blocked, author should fix.", + remote="prgs", + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("canonical_comment_validation", result) + self.assertFalse(result["canonical_comment_validation"]["allowed"]) + self.assertTrue(result["canonical_comment_validation"]["is_workflow_comment"]) + mock_api.assert_not_called() + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_missing_issue_error_is_redacted(self, _auth, mock_api): @@ -3779,13 +3912,24 @@ class TestIssueLocking(unittest.TestCase): ) self.assertIn("lock provenance", str(ctx.exception).lower()) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api): + def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api, _labels): scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-e2e") - mock_api.return_value = {"number": 250, "html_url": "https://example/pr/250"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/249" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 250, "html_url": "https://example/pr/250"} + if method == "PUT" and url.endswith("/issues/249/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect _bind_test_lock( issue_number=249, branch_name="feat/issue-249-issue-lock-scratch-worktree", @@ -3800,6 +3944,7 @@ class TestIssueLocking(unittest.TestCase): worktree_path=scratch, ) self.assertEqual(res["number"], 250) + self.assertTrue(res["issue_status_transition"]["performed"]) # --------------------------------------------------------------------------- diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py index e90d5ab..284ad37 100644 --- a/tests/test_merged_cleanup_reconcile.py +++ b/tests/test_merged_cleanup_reconcile.py @@ -4,7 +4,7 @@ import os import sys import tempfile import unittest -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, Mock, patch sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) @@ -163,7 +163,7 @@ class TestMergedCleanupReport(unittest.TestCase): "branch refs/heads/feat/issue-11-x\n" "HEAD cafebabe\n\n" ) - mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + mock_run.return_value = Mock(returncode=0, stdout=mock_output) res = mcr.discover_local_worktrees("/repo/Gitea-Tools") self.assertEqual(res, {"feat/issue-11-x": "/repo/Gitea-Tools/branches/custom-name"}) @@ -177,14 +177,23 @@ class TestMergedCleanupReport(unittest.TestCase): "branch refs/heads/feat/issue-11-x\n" "HEAD cafebabe\n\n" ) - mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) - mock_state.return_value = { - "exists": True, - "clean": True, - "current_branch": "feat/issue-11-x", - "head_sha": "cafebabe", - "dirty_files": [], - } + mock_run.return_value = Mock(returncode=0, stdout=mock_output) + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "exists": True, + "clean": True, + "current_branch": "feat/issue-11-x", + "head_sha": "cafebabe", + "dirty_files": [], + }, + ] report = mcr.build_reconciliation_report( project_root="/repo/Gitea-Tools", @@ -209,29 +218,190 @@ class TestMergedCleanupReport(unittest.TestCase): self.assertEqual(local["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") self.assertEqual(local["match_type"], "branch") + @patch("merged_cleanup_reconcile.read_local_worktree_state") @patch("subprocess.run") - def test_remove_local_worktree_uses_discovered_path(self, mock_run): - mock_output = ( - "worktree /repo/Gitea-Tools\n" - "bare\n\n" - "worktree /repo/Gitea-Tools/branches/custom-name\n" - "branch refs/heads/feat/issue-11-x\n" - "HEAD cafebabe\n\n" + def test_build_report_discovers_issue_alias_worktree(self, mock_run, mock_state): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools\n" + "HEAD aaaa\n" + "branch refs/heads/master\n\n" + "worktree /repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + ), ) - # First call is discover_local_worktrees, second is git worktree remove - mock_run.side_effect = [ - MagicMock(returncode=0, stdout=mock_output), - MagicMock(returncode=0, stdout="", stderr=""), + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "exists": True, + "clean": True, + "current_branch": "feat/issue-510-workspace-binding-isolation", + "head_sha": "cafebabe", + "dirty_files": [], + }, ] - with patch("os.path.isdir", return_value=True): - result = mcr.remove_local_worktree("/repo/Gitea-Tools", "feat/issue-11-x") + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 512, + "title": "merged cleanup", + "body": "Closes #510", + "head": { + "ref": "feat/issue-510-workspace-binding-isolation", + "sha": "cafebabe", + }, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-510-workspace-binding-isolation": True}, + head_on_master={512: True}, + delete_capability_allowed=True, + target_ref="prgs/master", + ) + + local = report["entries"][0]["local_worktree"] + self.assertTrue(local["safe_to_remove_worktree"]) + self.assertEqual(local["match_type"], "branch") + self.assertEqual( + local["expected_worktree_path"], + "/repo/Gitea-Tools/branches/feat-issue-510-workspace-binding-isolation", + ) + self.assertEqual( + local["discovered_worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + + @patch("merged_cleanup_reconcile.read_local_worktree_state") + @patch("subprocess.run") + def test_ambiguous_issue_alias_worktrees_fail_closed(self, mock_run, mock_state): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools/branches/issue-510-one\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + "worktree /repo/Gitea-Tools/branches/issue-510-two\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + ), + ) + mock_state.return_value = { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + } + + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 512, + "title": "merged cleanup", + "body": "Closes #510", + "head": { + "ref": "feat/issue-510-workspace-binding-isolation", + "sha": "cafebabe", + }, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-510-workspace-binding-isolation": True}, + head_on_master={512: True}, + delete_capability_allowed=True, + target_ref="prgs/master", + ) + + local = report["entries"][0]["local_worktree"] + self.assertFalse(local["safe_to_remove_worktree"]) + self.assertEqual(local["match_type"], "ambiguous_alias") + self.assertIn("ambiguous local worktree aliases", local["block_reasons"][0]) + + @patch("merged_cleanup_reconcile.is_head_ancestor_of_ref", return_value=True) + @patch("merged_cleanup_reconcile.read_local_worktree_state") + @patch("subprocess.run") + def test_alias_head_on_target_branch_is_safe_cleanup_commit( + self, mock_run, mock_state, _ancestor + ): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation\n" + "HEAD deadbeef\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + ), + ) + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "exists": True, + "clean": True, + "current_branch": "feat/issue-510-workspace-binding-isolation", + "head_sha": "deadbeef", + "dirty_files": [], + }, + ] + + state = mcr.resolve_cleanup_worktree_state( + project_root="/repo/Gitea-Tools", + head_branch="feat/issue-510-workspace-binding-isolation", + issue_number=510, + pr_head_sha="cafebabe", + target_ref="prgs/master", + ) + + self.assertTrue(state["exists"]) + self.assertEqual(state["match_type"], "branch") + self.assertEqual( + state["discovered_worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + + @patch("os.path.isdir", return_value=True) + @patch("subprocess.run") + def test_remove_local_worktree_uses_assessed_path(self, mock_run, _isdir): + mock_run.return_value = Mock(returncode=0, stdout="", stderr="") + result = mcr.remove_local_worktree( + "/repo/Gitea-Tools", + "feat/issue-510-workspace-binding-isolation", + worktree_path="/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + self.assertTrue(result["success"]) - self.assertTrue(result["performed"]) - self.assertEqual(result["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") - # Assert git worktree remove was called with the custom path - mock_run.assert_any_call( - ["git", "-C", "/repo/Gitea-Tools", "worktree", "remove", "/repo/Gitea-Tools/branches/custom-name"], + self.assertEqual( + result["worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + mock_run.assert_called_once_with( + [ + "git", + "-C", + "/repo/Gitea-Tools", + "worktree", + "remove", + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ], capture_output=True, text=True, check=False, @@ -440,4 +610,4 @@ class TestReviewerScratchCleanup(unittest.TestCase): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/test_operator_guide.py b/tests/test_operator_guide.py index a5159c6..147a6b2 100644 --- a/tests/test_operator_guide.py +++ b/tests/test_operator_guide.py @@ -33,8 +33,15 @@ REVIEWER_ENV = { "GITEA_PROFILE_NAME": "reviewer-test", "GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.pr.review,gitea.pr.comment,gitea.pr.approve," - "gitea.pr.request_changes,gitea.pr.merge", - "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push", + "gitea.pr.request_changes", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.merge", +} + +MERGER_ENV = { + "GITEA_PROFILE_NAME": "merger-test", + "GITEA_ALLOWED_OPERATIONS": + "gitea.read,gitea.pr.comment,gitea.pr.merge", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.approve", } EXPECTED_SKILLS = [ @@ -88,6 +95,15 @@ class TestControlPlaneGuide(GuideTestBase): self.assertIn("eligibility", blob) self.assertIn("pinned", blob) + @patch("mcp_server.api_request", return_value={"login": "merger-bot"}) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_merger_profile_guidance(self, _auth, _api): + with patch.dict(os.environ, MERGER_ENV, clear=True): + g = mcp_get_control_plane_guide(remote="prgs") + self.assertEqual(g["profile"]["role_kind"], "merger") + blob = " ".join(g["guidance"]).lower() + self.assertIn("merge", blob) + @patch("mcp_server.get_auth_header", return_value=None) def test_unresolved_identity_instructs_stop(self, _auth): with patch.dict(os.environ, AUTHOR_ENV, clear=True): diff --git a/tests/test_resolve_task_capability.py b/tests/test_resolve_task_capability.py index a34456d..aa25ced 100644 --- a/tests/test_resolve_task_capability.py +++ b/tests/test_resolve_task_capability.py @@ -44,9 +44,19 @@ CONFIG_RESOLVER = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.pr.merge", "gitea.issue.comment"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -83,6 +93,7 @@ class TestResolveTaskCapability(unittest.TestCase): "GITEA_MCP_PROFILE": profile, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } @patch("mcp_server.api_request", return_value={"login": "author-user"}) @@ -231,9 +242,9 @@ class TestResolveTaskCapability(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile")): res = mcp_server.gitea_resolve_task_capability(task="merge_pr", remote="prgs") self.assertFalse(res["allowed_in_current_session"]) - self.assertIn("reviewer", res["exact_safe_next_action"].lower()) + self.assertIn("merger", res["exact_safe_next_action"].lower()) self.assertEqual(res["required_operation_permission"], "gitea.pr.merge") - self.assertEqual(res["required_role_kind"], "reviewer") + self.assertEqual(res["required_role_kind"], "merger") @patch("mcp_server.api_request") def test_self_review_blocked_structured(self, mock_api): diff --git a/tests/test_reviewer_handoff_consistency.py b/tests/test_reviewer_handoff_consistency.py new file mode 100644 index 0000000..ac83124 --- /dev/null +++ b/tests/test_reviewer_handoff_consistency.py @@ -0,0 +1,120 @@ +"""Tests for reviewer handoff consistency validation (#501).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import reviewer_handoff_consistency # noqa: E402 +from final_report_validator import assess_final_report_validator # noqa: E402 + + +def _base_handoff(**overrides): + lines = [ + "## Controller Handoff", + "", + "- Task: review PR #472", + "- Review decision: request_changes", + "- Review mutations: submitted request_changes review", + "- Merge mutations: none", + "- MCP/Gitea mutations: reviewer lease acquired", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + token = f"- {key}:" + if token in text: + before, _, after = text.partition(token) + rest = after.split("\n", 1) + suffix = rest[1] if len(rest) > 1 else "" + text = f"{before}{token} {value}" + (f"\n{suffix}" if suffix else "") + else: + text += f"\n- {key}: {value}" + return text + + +class TestReviewerHandoffConsistency(unittest.TestCase): + def test_consistent_handoff_passes(self): + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency( + _base_handoff() + ) + self.assertTrue(result["proven"], result["reasons"]) + + def test_claimed_merge_missing_from_ledger(self): + report = _base_handoff( + **{ + "Current status": "Merged PR #411 earlier; reviewing PR #472", + "Merge mutations": "none", + "MCP/Gitea mutations": "reviewer lease acquired", + } + ) + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("merge" in r.lower() for r in result["reasons"])) + + def test_terminal_budget_consumed_without_merge_in_ledger(self): + report = _base_handoff( + **{ + "Current status": "Terminal mutation budget already consumed by merge", + "Merge mutations": "none", + "MCP/Gitea mutations": "reviewer lease acquired", + } + ) + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("terminal mutation budget" in r for r in result["reasons"])) + + def test_review_blocked_but_final_decision_marked(self): + report = _base_handoff( + **{ + "Current status": "gitea_submit_pr_review blocked", + "Review mutations": "none", + } + ) + report += "\nServer-side final decision marked via gitea_mark_final_review_decision." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("blocked" in r and "final decision" in r for r in result["reasons"])) + + def test_lease_without_review_decision(self): + report = _base_handoff(**{"Review decision": "none"}) + report += "\nReviewer lease acquired for PR #472." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("Review decision" in r for r in result["reasons"])) + + def test_rejected_mutation_without_proof(self): + report = _base_handoff( + **{ + "Review mutations": "none", + "MCP/Gitea mutations": "none", + } + ) + report += "\nReview submission blocked before mutation." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("proof fields" in r for r in result["reasons"])) + + def test_blocked_template_includes_required_fields(self): + template = reviewer_handoff_consistency.render_blocked_review_handoff_template() + for field in reviewer_handoff_consistency._BLOCKED_REVIEW_PROOF_FIELDS: + self.assertIn(field, template.lower()) + + def test_final_report_validator_integration(self): + report = _base_handoff( + **{ + "Current status": "Merged PR #411; terminal mutation budget consumed", + "Merge mutations": "none", + } + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue( + any( + f["rule_id"] == "reviewer.handoff_consistency" + for f in result["findings"] + ), + result["findings"], + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_runtime_clarity.py b/tests/test_runtime_clarity.py index e5098e0..1e94ada 100644 --- a/tests/test_runtime_clarity.py +++ b/tests/test_runtime_clarity.py @@ -43,9 +43,19 @@ CONFIG_SWITCHING_DISABLED = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.approve", "gitea.pr.merge"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -90,6 +100,7 @@ class TestRuntimeClarity(unittest.TestCase): "GITEA_MCP_REVEAL_ENDPOINTS": reveal, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } # ------------------------------------------------------------------------- @@ -125,7 +136,7 @@ class TestRuntimeClarity(unittest.TestCase): t for t in caps["task_capabilities"] if t["task"] == "merge_pr" ) self.assertFalse(merge_entry["allowed_in_current_session"]) - self.assertIn("reviewer-profile", merge_entry["matching_configured_profiles"]) + self.assertIn("merger-profile", merge_entry["matching_configured_profiles"]) @patch( "mcp_server.assess_preflight_status", @@ -144,6 +155,30 @@ class TestRuntimeClarity(unittest.TestCase): caps = ctx["session_capabilities"] self.assertFalse(caps["can_author_prs"]) self.assertFalse(caps["can_create_issues"]) + self.assertTrue(caps["can_review_prs"]) + self.assertFalse(caps["can_merge_prs"]) + merge_entry = next( + t for t in caps["task_capabilities"] if t["task"] == "merge_pr" + ) + self.assertFalse(merge_entry["allowed_in_current_session"]) + + @patch( + "mcp_server.assess_preflight_status", + return_value={"preflight_ready": True, "preflight_block_reasons": []}, + ) + @patch("mcp_server.api_request", return_value={"login": "merger-user"}) + @patch("mcp_server.get_auth_header", return_value="token merger-pass") + def test_get_runtime_context_merger(self, _auth, _api, _preflight): + with patch.dict(os.environ, self._env("merger-profile"), clear=True): + ctx = mcp_server.gitea_get_runtime_context(remote="dadeschools") + self.assertEqual(ctx["active_profile"], "merger-profile") + self.assertEqual(ctx["authenticated_username"], "merger-user") + self.assertTrue(ctx["review_merge_allowed"]) + self.assertEqual(ctx["suggested_fix"], "none") + self.assertEqual(ctx["safe_next_action"], "None; ready for operations.") + caps = ctx["session_capabilities"] + self.assertFalse(caps["can_author_prs"]) + self.assertFalse(caps["can_create_issues"]) self.assertFalse(caps["can_review_prs"]) self.assertTrue(caps["can_merge_prs"]) merge_entry = next( @@ -160,7 +195,7 @@ class TestRuntimeClarity(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile", reveal="0"), clear=True): res = mcp_server.gitea_list_profiles() profiles = res["profiles"] - self.assertEqual(len(profiles), 2) + self.assertEqual(len(profiles), 3) author_prof = next(p for p in profiles if p["name"] == "author-profile") self.assertTrue(author_prof["is_active"]) @@ -176,6 +211,13 @@ class TestRuntimeClarity(unittest.TestCase): self.assertEqual(reviewer_prof["base_url"], "<redacted>") self.assertEqual(reviewer_prof["identity_status"], "credentials present") + merger_prof = next(p for p in profiles if p["name"] == "merger-profile") + self.assertFalse(merger_prof["is_active"]) + self.assertEqual(merger_prof["role_kind"], "merger") + self.assertEqual(merger_prof["auth"]["name"], "<redacted>") + self.assertEqual(merger_prof["base_url"], "<redacted>") + self.assertEqual(merger_prof["identity_status"], "credentials present") + @patch("mcp_server.api_request", return_value={"login": "author-user"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_list_profiles_revealed_under_opt_in(self, _auth, _api): diff --git a/tests/test_webui_runtime_health.py b/tests/test_webui_runtime_health.py new file mode 100644 index 0000000..9fda3ee --- /dev/null +++ b/tests/test_webui_runtime_health.py @@ -0,0 +1,88 @@ +"""Tests for web UI runtime health dashboard (#430).""" +import sys +import unittest +from pathlib import Path +from unittest import mock + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from starlette.testclient import TestClient + +from webui.app import create_app +from webui.runtime_health import _role_kind, load_runtime_snapshot, snapshot_to_dict + + +def _mock_identity(_host: str): + return "jcwalker3", None + + +def _mock_git_sync(_repo: Path, _remote: str, _branch: str): + return "localsha123456", "remoteshaabcdef", 2 + + +class TestRuntimeHealthLoader(unittest.TestCase): + def test_role_kind_author(self): + allowed = ["gitea.pr.create", "gitea.branch.push", "gitea.read"] + forbidden = ["gitea.pr.merge", "gitea.pr.approve"] + self.assertEqual(_role_kind(allowed, forbidden), "author") + + def test_snapshot_with_mocks(self): + snapshot = load_runtime_snapshot( + resolve_username=_mock_identity, + git_sync=_mock_git_sync, + ) + self.assertEqual(snapshot.project_id, "gitea-tools") + self.assertEqual(snapshot.authenticated_username, "jcwalker3") + self.assertEqual(snapshot.commits_behind_master, 2) + self.assertIsNotNone(snapshot.stale_runtime_warning) + self.assertGreaterEqual(len(snapshot.workflow_hashes), 3) + self.assertGreaterEqual(len(snapshot.schema_hashes), 2) + self.assertIn("shell_use_allowed", snapshot.shell_health) + + def test_snapshot_dict_export(self): + snapshot = load_runtime_snapshot( + resolve_username=_mock_identity, + git_sync=_mock_git_sync, + ) + data = snapshot_to_dict(snapshot) + self.assertEqual(data["authenticated_username"], "jcwalker3") + self.assertEqual(data["commits_behind_master"], 2) + self.assertTrue(data["stale_runtime_warning"]) + + +class TestRuntimeRoutes(unittest.TestCase): + def setUp(self): + self.client = TestClient(create_app()) + self.snapshot = load_runtime_snapshot( + resolve_username=_mock_identity, + git_sync=_mock_git_sync, + ) + self._patch = mock.patch( + "webui.app.load_runtime_snapshot", + return_value=self.snapshot, + ) + self._patch.start() + + def tearDown(self): + self._patch.stop() + + def test_runtime_page_renders_profile_and_warning(self): + response = self.client.get("/runtime") + self.assertEqual(response.status_code, 200) + self.assertIn("Runtime health", response.text) + self.assertIn("Active profile", response.text) + self.assertIn("Stale runtime warning", response.text) + self.assertIn("Workflow hashes", response.text) + self.assertNotIn("child issue", response.text.lower()) + + def test_api_runtime_json(self): + response = self.client.get("/api/runtime") + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["project_id"], "gitea-tools") + self.assertEqual(data["role_kind"], self.snapshot.role_kind) + self.assertEqual(data["commits_behind_master"], 2) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_webui_skeleton.py b/tests/test_webui_skeleton.py index 953b368..e5b71b6 100644 --- a/tests/test_webui_skeleton.py +++ b/tests/test_webui_skeleton.py @@ -29,8 +29,13 @@ class TestWebuiSkeleton(unittest.TestCase): self.assertIn("Operator console", response.text) self.assertIn("Read-only MVP", response.text) + def test_runtime_is_implemented(self): + response = self.client.get("/runtime") + self.assertEqual(response.status_code, 200) + self.assertIn("Runtime health", response.text) + def test_route_stubs_render(self): - for path in ("/runtime", "/audit"): + for path in ("/audit",): with self.subTest(path=path): response = self.client.get(path) self.assertEqual(response.status_code, 200) diff --git a/webui/app.py b/webui/app.py index 4f4cb0f..fb1aab2 100644 --- a/webui/app.py +++ b/webui/app.py @@ -16,8 +16,10 @@ from webui.prompt_library import find_prompt, library_to_dict from webui.prompt_views import render_prompt_detail, render_prompts_page from webui.lease_loader import load_lease_snapshot, snapshot_to_dict as lease_snapshot_to_dict from webui.lease_views import render_leases_page -from webui.queue_loader import load_queue_snapshot, snapshot_to_dict +from webui.queue_loader import load_queue_snapshot, snapshot_to_dict as queue_snapshot_to_dict from webui.queue_views import render_queue_page +from webui.runtime_health import load_runtime_snapshot, snapshot_to_dict as runtime_snapshot_to_dict +from webui.runtime_views import render_runtime_page _READ_ONLY_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) @@ -63,7 +65,7 @@ async def queue(_request: Request) -> HTMLResponse: async def api_queue(_request: Request) -> JSONResponse: - return JSONResponse(snapshot_to_dict(load_queue_snapshot())) + return JSONResponse(queue_snapshot_to_dict(load_queue_snapshot())) async def projects(_request: Request) -> HTMLResponse: @@ -122,10 +124,12 @@ async def api_prompts(_request: Request) -> JSONResponse: async def runtime(_request: Request) -> HTMLResponse: - return _stub_page( - "Runtime", - "Runtime health will report MCP profile, preflight, and stale-server signals.", - ) + snapshot = load_runtime_snapshot() + return HTMLResponse(render_page(title="Runtime", body_html=render_runtime_page(snapshot))) + + +async def api_runtime(_request: Request) -> JSONResponse: + return JSONResponse(runtime_snapshot_to_dict(load_runtime_snapshot())) async def audit(_request: Request) -> HTMLResponse: @@ -176,6 +180,7 @@ def create_app() -> Starlette: Route("/prompts/{prompt_id}", prompt_detail, methods=["GET"]), Route("/api/prompts", api_prompts, methods=["GET"]), Route("/runtime", runtime, methods=["GET"]), + Route("/api/runtime", api_runtime, methods=["GET"]), Route("/audit", audit, methods=["GET"]), Route("/worktrees", worktrees, methods=["GET"]), Route("/leases", leases, methods=["GET"]), diff --git a/webui/lease_loader.py b/webui/lease_loader.py index 548aec3..318a46f 100644 --- a/webui/lease_loader.py +++ b/webui/lease_loader.py @@ -11,8 +11,8 @@ from urllib.parse import urlparse from gitea_auth import api_fetch_page, get_auth_header, repo_api_url from issue_claim_heartbeat import build_claim_inventory -from merged_cleanup_reconcile import read_issue_lock from issue_lock_provenance import ISSUE_LOCK_FILE +from merged_cleanup_reconcile import read_issue_lock from webui.project_registry import ProjectRecord, load_registry from webui.queue_loader import _extract_linked_issue, _fetch_issues, _fetch_prs diff --git a/webui/runtime_health.py b/webui/runtime_health.py new file mode 100644 index 0000000..8f3b5d6 --- /dev/null +++ b/webui/runtime_health.py @@ -0,0 +1,320 @@ +"""Read-only MCP/runtime health snapshot for the web UI (#430).""" + +from __future__ import annotations + +import hashlib +import os +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Callable +from urllib.parse import urlparse + +import gitea_config +import reconciler_profile +from gitea_auth import REMOTES, api_request, get_auth_header, get_profile, gitea_url + +from native_mcp_preference import shell_health_status + +from webui.project_registry import ProjectRecord, load_registry + +_RESTART_DOCS = "docs/llm-workflow-runbooks.md#issue-420-mcp-hot-reload" +_HOT_RELOAD_ISSUE = "#420" + + +@dataclass(frozen=True) +class FileHash: + label: str + path: str + sha256: str | None + error: str | None = None + + +@dataclass(frozen=True) +class RuntimeSnapshot: + project_id: str + repo_root: str + remote: str + host: str + profile_name: str + role_kind: str + config_model: str + profile_mode: str + profile_source: str + authenticated_username: str | None + identity_error: str | None + repo_sha: str | None + remote_master_sha: str | None + commits_behind_master: int | None + stale_runtime_warning: str | None + shell_health: dict[str, Any] + workflow_hashes: tuple[FileHash, ...] + schema_hashes: tuple[FileHash, ...] + restart_guidance: str + fetch_error: str | None = None + + +def _repo_root() -> Path: + override = (os.environ.get("WEBUI_REPO_ROOT") or "").strip() + if override: + return Path(override).resolve() + return Path(__file__).resolve().parent.parent + + +def _host_from_url(remote_host: str) -> str: + parsed = urlparse(remote_host.strip()) + return parsed.netloc or remote_host.strip().rstrip("/") + + +def _remote_name_for_host(host: str) -> str: + for name, profile in REMOTES.items(): + if profile.get("host") == host: + return name + return "custom" + + +def _config_model(config: dict | None) -> str: + if config is None: + return "env-only" + version = config.get("version") + if version == 1: + return "v1" + if version == 2: + if "contexts" in config or config.get("shape") == "contexts": + return "v2-contexts" + return "v2-environments" + return f"unknown-version-{version}" + + +def _profile_source() -> str: + if os.environ.get("GITEA_PROFILE_NAME"): + return "env var" + if gitea_config.selected_profile_name(): + return "config file profile" + return "default" + + +def _role_kind(allowed: list[str], forbidden: list[str]) -> str: + if reconciler_profile.is_reconciler_profile(allowed, forbidden): + return "reconciler" + + def can(op: str) -> bool: + return gitea_config.check_operation(op, allowed, forbidden)[0] + + review = can("gitea.pr.approve") or can("gitea.pr.merge") + author = can("gitea.pr.create") or can("gitea.branch.push") + reconciler = can("gitea.pr.close") and not review and not author + if review and author: + return "mixed" + if review: + return "reviewer" + if reconciler: + return "reconciler" + if author: + return "author" + return "limited" + + +def _sha256_file(path: Path) -> FileHash: + label = path.name + try: + digest = hashlib.sha256(path.read_bytes()).hexdigest() + return FileHash(label=label, path=str(path), sha256=digest) + except OSError as exc: + return FileHash(label=label, path=str(path), sha256=None, error=str(exc)) + + +def _run_git(repo: Path, *args: str) -> str | None: + try: + completed = subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + ) + return completed.stdout.strip() or None + except (OSError, subprocess.CalledProcessError): + return None + + +def _git_sync_status(repo: Path, remote: str, branch: str) -> tuple[str | None, str | None, int | None]: + local_sha = _run_git(repo, "rev-parse", "HEAD") + remote_sha = _run_git(repo, "rev-parse", f"{remote}/{branch}") + behind = None + if local_sha and remote_sha and local_sha != remote_sha: + count = _run_git(repo, "rev-list", "--count", f"HEAD..{remote}/{branch}") + if count is not None and count.isdigit(): + behind = int(count) + return local_sha, remote_sha, behind + + +def _resolve_username(host: str) -> tuple[str | None, str | None]: + auth = get_auth_header(host) + if not auth: + return None, f"Gitea credentials unavailable for {host}" + try: + data = api_request("GET", gitea_url(host, "/api/v1/user"), auth) + except Exception as exc: # noqa: BLE001 — operator-visible identity errors + return None, f"Identity lookup failed: {exc}" + username = (data or {}).get("login") + if not username: + return None, "Authenticated identity could not be resolved" + return str(username), None + + +def _stale_warning(commits_behind: int | None) -> str | None: + if commits_behind is None or commits_behind <= 0: + return None + return ( + f"Local checkout is {commits_behind} commit(s) behind remote master. " + "Merged safety-gate changes may require an MCP server restart or profile " + f"reload — see {_HOT_RELOAD_ISSUE} / {_RESTART_DOCS}." + ) + + +def load_runtime_snapshot( + project_id: str | None = None, + *, + resolve_username: Callable[[str], tuple[str | None, str | None]] | None = None, + git_sync: Callable[[Path, str, str], tuple[str | None, str | None, int | None]] | None = None, +) -> RuntimeSnapshot: + """Build a read-only runtime health snapshot for the default registry project.""" + registry = load_registry() + project: ProjectRecord | None = None + if project_id: + for entry in registry.projects: + if entry.id == project_id: + project = entry + break + else: + project = registry.projects[0] if registry.projects else None + + if project is None: + return RuntimeSnapshot( + project_id=project_id or "", + repo_root=str(_repo_root()), + remote="", + host="", + profile_name="", + role_kind="", + config_model="", + profile_mode="", + profile_source="", + authenticated_username=None, + identity_error="project not found in registry", + repo_sha=None, + remote_master_sha=None, + commits_behind_master=None, + stale_runtime_warning=None, + shell_health={}, + workflow_hashes=(), + schema_hashes=(), + restart_guidance=_RESTART_DOCS, + fetch_error="project not found in registry", + ) + + repo = _repo_root() + host = _host_from_url(project.remote_host) + remote = _remote_name_for_host(host) + profile = get_profile() + allowed = profile.get("allowed_operations") or [] + forbidden = profile.get("forbidden_operations") or [] + config = None + try: + config = gitea_config.load_config() + except gitea_config.ConfigError as exc: + return RuntimeSnapshot( + project_id=project.id, + repo_root=str(repo), + remote=remote, + host=host, + profile_name=profile.get("profile_name") or "", + role_kind=_role_kind(allowed, forbidden), + config_model="config-error", + profile_mode="unknown", + profile_source=_profile_source(), + authenticated_username=None, + identity_error=str(exc), + repo_sha=None, + remote_master_sha=None, + commits_behind_master=None, + stale_runtime_warning=None, + shell_health=shell_health_status(), + workflow_hashes=(), + schema_hashes=(), + restart_guidance=_RESTART_DOCS, + fetch_error=str(exc), + ) + + identity_fn = resolve_username or _resolve_username + username, identity_error = identity_fn(host) + + git_fn = git_sync or _git_sync_status + remote_ref = "prgs" if remote == "prgs" else "origin" + local_sha, remote_sha, behind = git_fn(repo, remote_ref, project.default_branch) + + workflow_hashes = tuple( + _sha256_file(repo / rel_path) + for rel_path in project.workflow_paths.values() + ) + schema_hashes = tuple( + _sha256_file(repo / rel_path) + for rel_path in project.schema_paths.values() + ) + + switching = gitea_config.is_runtime_switching_enabled() + return RuntimeSnapshot( + project_id=project.id, + repo_root=str(repo), + remote=remote, + host=host, + profile_name=str(profile.get("profile_name") or ""), + role_kind=_role_kind(allowed, forbidden), + config_model=_config_model(config), + profile_mode="dynamic-profile" if switching else "static-profile", + profile_source=_profile_source(), + authenticated_username=username, + identity_error=identity_error, + repo_sha=local_sha, + remote_master_sha=remote_sha, + commits_behind_master=behind, + stale_runtime_warning=_stale_warning(behind), + shell_health=shell_health_status(), + workflow_hashes=workflow_hashes, + schema_hashes=schema_hashes, + restart_guidance=_RESTART_DOCS, + ) + + +def snapshot_to_dict(snapshot: RuntimeSnapshot) -> dict[str, Any]: + def _hash_row(item: FileHash) -> dict[str, Any]: + return { + "label": item.label, + "path": item.path, + "sha256": item.sha256, + "error": item.error, + } + + return { + "project_id": snapshot.project_id, + "repo_root": snapshot.repo_root, + "remote": snapshot.remote, + "host": snapshot.host, + "profile_name": snapshot.profile_name, + "role_kind": snapshot.role_kind, + "config_model": snapshot.config_model, + "profile_mode": snapshot.profile_mode, + "profile_source": snapshot.profile_source, + "authenticated_username": snapshot.authenticated_username, + "identity_error": snapshot.identity_error, + "repo_sha": snapshot.repo_sha, + "remote_master_sha": snapshot.remote_master_sha, + "commits_behind_master": snapshot.commits_behind_master, + "stale_runtime_warning": snapshot.stale_runtime_warning, + "shell_health": snapshot.shell_health, + "workflow_hashes": [_hash_row(item) for item in snapshot.workflow_hashes], + "schema_hashes": [_hash_row(item) for item in snapshot.schema_hashes], + "restart_guidance": snapshot.restart_guidance, + "fetch_error": snapshot.fetch_error, + } \ No newline at end of file diff --git a/webui/runtime_views.py b/webui/runtime_views.py new file mode 100644 index 0000000..b057248 --- /dev/null +++ b/webui/runtime_views.py @@ -0,0 +1,92 @@ +"""HTML views for MCP runtime health (#430).""" + +from __future__ import annotations + +import html + +from webui.runtime_health import FileHash, RuntimeSnapshot + + +def _hash_rows(items: tuple[FileHash, ...]) -> str: + if not items: + return "<p class='muted'>No hashes available.</p>" + rows = [] + for item in items: + digest = item.sha256 or item.error or "unavailable" + rows.append( + "<tr>" + f"<td>{html.escape(item.label)}</td>" + f"<td><code>{html.escape(item.path)}</code></td>" + f"<td><code>{html.escape(digest[:16] if item.sha256 else digest)}</code></td>" + "</tr>" + ) + return ( + "<table class='registry'><thead><tr>" + "<th>Artifact</th><th>Path</th><th>SHA-256</th>" + "</tr></thead><tbody>" + f"{''.join(rows)}</tbody></table>" + ) + + +def render_runtime_page(snapshot: RuntimeSnapshot) -> str: + error_block = "" + if snapshot.fetch_error: + error_block = ( + '<div class="stub"><p><strong>Runtime snapshot incomplete:</strong> ' + f"{html.escape(snapshot.fetch_error)}</p></div>" + ) + + identity = snapshot.authenticated_username or "unresolved" + if snapshot.identity_error: + identity = f"unresolved ({snapshot.identity_error})" + + stale_block = "" + if snapshot.stale_runtime_warning: + stale_block = ( + '<div class="stub"><p><strong>Stale runtime warning:</strong> ' + f"{html.escape(snapshot.stale_runtime_warning)}</p></div>" + ) + + shell = snapshot.shell_health or {} + shell_summary = ( + f"shell_use_allowed={shell.get('shell_use_allowed')} · " + f"failures={shell.get('consecutive_spawn_failures')} · " + f"hard_stopped={shell.get('hard_stopped')}" + ) + + return ( + "<h2>Runtime health</h2>" + f"<p class='meta'>Project <code>{html.escape(snapshot.project_id)}</code> · " + f"host <code>{html.escape(snapshot.host)}</code> · " + f"repo root <code>{html.escape(snapshot.repo_root)}</code></p>" + f"{error_block}" + f"{stale_block}" + "<h3>Active profile</h3>" + "<table class='detail'>" + f"<tr><th>Profile</th><td><code>{html.escape(snapshot.profile_name)}</code></td></tr>" + f"<tr><th>Role kind</th><td>{html.escape(snapshot.role_kind)}</td></tr>" + f"<tr><th>Identity</th><td>{html.escape(identity)}</td></tr>" + f"<tr><th>Config model</th><td>{html.escape(snapshot.config_model)}</td></tr>" + f"<tr><th>Profile mode</th><td>{html.escape(snapshot.profile_mode)} " + f"({html.escape(snapshot.profile_source)})</td></tr>" + "</table>" + "<h3>Server code sync</h3>" + "<table class='detail'>" + f"<tr><th>Local HEAD</th><td><code>{html.escape(snapshot.repo_sha or 'unknown')}</code></td></tr>" + f"<tr><th>Remote {html.escape(snapshot.remote)}/{html.escape('master')}</th>" + f"<td><code>{html.escape(snapshot.remote_master_sha or 'unknown')}</code></td></tr>" + f"<tr><th>Commits behind</th><td>{snapshot.commits_behind_master if snapshot.commits_behind_master is not None else 'unknown'}</td></tr>" + "</table>" + "<h3>Shell health</h3>" + f"<p class='meta'>{html.escape(shell_summary)}</p>" + f"<p class='muted'>{html.escape(str(shell.get('safe_next_action') or ''))}</p>" + "<h3>Workflow hashes</h3>" + f"{_hash_rows(snapshot.workflow_hashes)}" + "<h3>Schema hashes</h3>" + f"{_hash_rows(snapshot.schema_hashes)}" + "<h3>Restart / reload</h3>" + "<p class='muted'>MVP is read-only — restart MCP servers from your IDE/operator " + "workflow. Related issue: <code>#420</code>. Guidance: " + f"<code>{html.escape(snapshot.restart_guidance)}</code></p>" + "<p class='muted'>This page does not expose tokens or perform MCP restarts.</p>" + ) \ No newline at end of file