From ee90a5e7a2b5f53f402d1f10e05b21005cf6eb6e Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Mon, 13 Jul 2026 17:37:13 -0400 Subject: [PATCH] fix(validator): align final-report validator with canonical schema (Closes #698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the final-report validator defects from Issue #698 (original lead plus the independent reproduction recorded during the PR #703 formal review, comment 11246): - Legacy fields: the review/merger required-field tables no longer demand 'Pinned reviewed head', 'Scratch worktree used', 'Worktree path', 'Worktree dirty', 'Mutations', 'Next', 'Issue/PR', 'Branch/SHA', or 'Files changed' — names the canonical review-merge-final-report schema forbids or replaces. The canonical names ('Reviewed head SHA', 'Review worktree path/dirty', 'Safe next action', mutation categories) are required instead, with backward-compatible aliases where the schema permits them. - Structured proof: workflow-load helper results are recognized in colon, key=value, and JSON renderings; validation pass proof is accepted anywhere in the Validation field value (for example 'focused 50 passed; full 2665 passed'). - Mutation inference: review mutations are inferred only from authoritative evidence (performed=true and not gated); read-only diagnostics and pre-API rejections no longer count as mutations. - Lease release vs cleanup: canonical reviewer lease release (release tool call or terminal phase=released marker) is lease lifecycle, not post-merge cleanup, and no longer triggers the branch/worktree cleanup checklist; genuine delete/remove claims still require full proof. - Blocked reports: a legitimately blocked run that states an explicit 'Reviewed/Candidate head SHA: none' with no verdict, merge, or started validation owes no head proofs; approval-time and merge-time live-head proofs are demanded only once the corresponding phase begins, and a report that states no head at all still fails closed. - action_log robustness: malformed (non-dict) entries and non-list logs are reported as clear sanitized findings (position and type only, no content echo) instead of crashing with AttributeError; a defective validator rule now fails closed with a sanitized block finding rather than raising a secondary exception. 27 new regression tests, including canonical fixtures modeled on the PR #703 formal-review handoff and the blocked preflight report from the prior #698 reproductions. Two legacy-field test fixtures updated to the canonical schema. Full suite: 2663 passed, 6 skipped, 161 subtests. Co-Authored-By: Claude Fable 5 --- final_report_validator.py | 103 +++- post_merge_cleanup_proof.py | 35 +- pr_work_lease.py | 73 ++- review_final_report_schema.py | 7 +- review_proofs.py | 45 +- ...e_698_report_validator_schema_alignment.py | 461 ++++++++++++++++++ tests/test_review_proofs.py | 18 +- 7 files changed, 701 insertions(+), 41 deletions(-) create mode 100644 tests/test_issue_698_report_validator_schema_alignment.py diff --git a/final_report_validator.py b/final_report_validator.py index fac6a78..a487cf6 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -134,16 +134,22 @@ _TARGET_BRANCH_SHA_RE = re.compile( r"target branch sha\s*:\s*[0-9a-f]{40}", re.IGNORECASE, ) +# #698: structured proof is rendered in several equivalent shapes — +# `workflow_hash: abc...`, `workflow_hash=abc...`, or JSON +# `"workflow_hash": "abc..."`. Recognize all of them; demanding one exact +# punctuation style rejects legitimate structured workflow-load proof. _WORKFLOW_LOAD_HELPER_RE = re.compile( - r"workflow[- ]load helper result\s*:", + r"workflow[-_ ]load[-_ ]helper[-_ ]result\s*[:=]", re.IGNORECASE, ) _WORKFLOW_LOAD_HASH_RE = re.compile( - r"workflow[- ]load helper result[\s\S]{0,400}?workflow[_ ]hash\s*:\s*[0-9a-f]{12}", + r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?" + r"workflow[_ ]hash\"?\s*[:=]\s*\"?[0-9a-f]{12}", re.IGNORECASE, ) _WORKFLOW_LOAD_BOUNDARY_RE = re.compile( - r"workflow[- ]load helper result[\s\S]{0,400}?boundary[_ ]status\s*:\s*(?:clean|violation)", + r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?" + r"boundary[_ ]status\"?\s*[:=]\s*\"?(?:clean|violation)", re.IGNORECASE, ) _WORKFLOW_FILE_VIEW_NARRATIVE_RE = re.compile( @@ -244,6 +250,59 @@ def _normalize_task_kind(task_kind: str | None) -> str: return _TASK_KIND_ALIASES.get(raw, raw) +def _iter_action_entries(action_log: list | None) -> list[dict]: + """Yield only structured (dict) action-log entries (#698). + + Callers must never crash on malformed entries (strings, numbers, null) + that reach the validator from LLM-composed or partially parsed logs; + :func:`sanitize_action_log` reports them separately. + """ + return [e for e in (action_log or []) if isinstance(e, dict)] + + +def sanitize_action_log( + action_log: list | None, +) -> tuple[list[dict], list[dict[str, str]]]: + """Split an action log into structured entries and sanitized findings (#698). + + Malformed entries become clear, sanitized ``warning`` findings — the + offending value's content is never echoed back (only its position and + type), so secrets or garbage in a broken log cannot leak into validation + errors, and validation itself proceeds without secondary exceptions. + """ + if action_log is None: + return [], [] + if not isinstance(action_log, (list, tuple)): + return [], [ + validator_finding( + "shared.action_log_malformed", + "downgrade", + "Action log", + "action_log is not a list of structured entries " + f"(got {type(action_log).__name__}); it was ignored", + "pass action_log as a list of dict entries", + ) + ] + entries: list[dict] = [] + findings: list[dict[str, str]] = [] + for index, entry in enumerate(action_log): + if isinstance(entry, dict): + entries.append(entry) + continue + findings.append( + validator_finding( + "shared.action_log_malformed", + "downgrade", + "Action log", + f"action_log entry {index} is not a structured mapping " + f"(got {type(entry).__name__}); the entry was ignored", + "repair the malformed action_log entry or drop it before " + "revalidating", + ) + ) + return entries, findings + + def validator_finding( rule_id: str, severity: str, @@ -421,7 +480,7 @@ def _rule_shared_canonical_comment_post_claim( rejected_in_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text)) rejected_in_log = False if action_log: - for entry in action_log: + for entry in _iter_action_entries(action_log): validation = entry.get("canonical_comment_validation") or {} if validation.get("allowed") is False: rejected_in_log = True @@ -475,9 +534,13 @@ def _rule_reviewer_vague_mutations_none( action_log: list[dict] | None = None, mutations_observed: bool = False, ) -> list[dict[str, str]]: + # #698: infer review mutations only from authoritative evidence — an + # entry proves a mutation only when it affirmatively records + # performed=true and was not gated. Read-only diagnostics and pre-API + # rejections (entries without a performed flag) are not mutations. performed = any( - e.get("performed") is not False and not e.get("gated_rejected") - for e in (action_log or []) + e.get("performed") is True and not e.get("gated_rejected") + for e in _iter_action_entries(action_log) ) if not (mutations_observed or performed): return [] @@ -536,7 +599,7 @@ def _rule_reviewer_git_fetch_readonly( text = report_text or "" fetch_observed = any( _GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or "")) - for e in (action_log or []) + for e in _iter_action_entries(action_log) ) or _GIT_FETCH_RE.search(text) if not fetch_observed: return [] @@ -977,7 +1040,7 @@ def _rule_reviewer_target_branch_freshness( fields = _handoff_fields(text) fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any( _GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or "")) - for e in (action_log or []) + for e in _iter_action_entries(action_log) ) target_sha_reported = bool(_TARGET_BRANCH_SHA_RE.search(text)) or any( "target branch" in key and "sha" in key and _FULL_SHA_RE.search(value) @@ -1735,6 +1798,12 @@ def assess_final_report_validator( checks: dict[str, Any] = {} findings: list[dict[str, str]] = [] + # #698: malformed action_log data must never crash validation with a + # secondary exception; malformed entries surface as sanitized findings. + sanitized_action_log, action_log_findings = sanitize_action_log(action_log) + action_log = sanitized_action_log + findings.extend(action_log_findings) + if normalized_kind == "issue_filing" and issue_filing_lock is not None: checks["issue_filing"] = assess_issue_filing_final_report( report_text, @@ -1763,7 +1832,23 @@ def assess_final_report_validator( } for rule in _RULES_BY_TASK.get(normalized_kind, ()): - findings.extend(_call_rule(rule, report_text, normalized_kind, rule_kwargs)) + try: + findings.extend( + _call_rule(rule, report_text, normalized_kind, rule_kwargs) + ) + except Exception as exc: # #698: fail closed with a sanitized error + findings.append( + validator_finding( + "shared.validator_rule_error", + "block", + "Validator", + f"validator rule '{getattr(rule, '__name__', 'unknown')}' " + f"failed with {type(exc).__name__} (details withheld; " + "sanitized)", + "file a validator defect with the rule name; do not " + "bypass final-report validation", + ) + ) grade, blocked, downgraded = _aggregate_grade(findings) reasons = [f"{f['rule_id']}: {f['reason']}" for f in findings] diff --git a/post_merge_cleanup_proof.py b/post_merge_cleanup_proof.py index 6562a04..e439dca 100644 --- a/post_merge_cleanup_proof.py +++ b/post_merge_cleanup_proof.py @@ -86,6 +86,22 @@ _WRONG_BRANCH_RE = re.compile( r"deleted branch (?:does not match|!=|differs from) (?:merged )?pr head", re.IGNORECASE, ) +# #698: canonical reviewer lease lifecycle operations are NOT post-merge +# cleanup. Releasing a reviewer PR lease (or posting its terminal +# phase=released marker) happens after every review — merged or not — and +# must never trigger the post-merge branch/worktree cleanup checklist. +_LEASE_LIFECYCLE_RE = re.compile( + r"(?:gitea_release_reviewer_pr_lease|gitea_abandon_workflow_lease|" + r"release[d]? (?:the )?(?:reviewer|workflow) (?:pr )?lease|" + r"reviewer (?:pr )?lease release[d]?|" + r"lease (?:marker|comment).{0,40}phase\s*[:=]\s*released|" + r"phase\s*[:=]\s*released)", + re.IGNORECASE, +) +_CLEANUP_MUTATIONS_VALUE_RE = re.compile( + r"cleanup mutations\s*:\s*([^\n]+)", + re.IGNORECASE, +) def _claims_remote_delete(text: str) -> bool: @@ -204,16 +220,19 @@ def assess_post_merge_cleanup_proof( for field in _worktree_cleanup_fields_present(text) ) - if (remote_delete or worktree_remove) and not (remote_delete or worktree_remove): - pass - if not remote_delete and not worktree_remove: - cleanup_mutations = re.search( - r"cleanup mutations\s*:\s*(?!none\b)\S", - text, - re.IGNORECASE, + value_match = _CLEANUP_MUTATIONS_VALUE_RE.search(text) + value = (value_match.group(1).strip() if value_match else "") + value_lower = value.lower() + substantive = bool(value) and value_lower not in { + "none", "n/a", "not applicable", + } + # #698: reviewer lease release / terminal lease markers are lease + # lifecycle, not post-merge cleanup — no checklist owed. + lease_lifecycle_only = substantive and bool( + _LEASE_LIFECYCLE_RE.search(value) ) - if cleanup_mutations: + if substantive and not lease_lifecycle_only: reasons.append( "cleanup mutations reported without post-merge cleanup proof checklist" ) diff --git a/pr_work_lease.py b/pr_work_lease.py index e2b3f21..2997462 100644 --- a/pr_work_lease.py +++ b/pr_work_lease.py @@ -403,10 +403,37 @@ _REVIEWER_ACTIVE_RE = re.compile( r"whether any reviewer was active\s*:\s*(yes|no|true|false)", re.IGNORECASE, ) +# #698 phase detection for phase-specific head proofs. +_NO_REVIEWED_HEAD_RE = re.compile( + r"(?:reviewed head sha|candidate head sha)\s*:\s*none\b", + re.IGNORECASE, +) +_VERDICT_RECORDED_RE = re.compile( + r"review decision\s*:\s*(?:approve[d]?|request[_ ]changes)\b" + r"|review_status\s*:\s*(?:approved|request_changes)\b" + r"|terminal review mutation\s*:\s*(?!none\b)\S", + re.IGNORECASE, +) +_MERGE_ATTEMPTED_RE = re.compile( + r"merge result\s*:\s*(?:merged|success|performed|failed|attempted)\b" + r"|merge mutations\s*:\s*(?!none\b|not applicable\b)\S", + re.IGNORECASE, +) +_VALIDATION_STARTED_RE = re.compile( + r"validation\s*:\s*(?!none\b|not run\b|not applicable\b|not started\b)" + r"[^\n]*(?:pass|fail|ran|executed|\d+\s+passed)", + re.IGNORECASE, +) def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]: - """Final-report proof for reviewed vs live head SHAs (#399 AC 6).""" + """Final-report proof for reviewed vs live head SHAs (#399 AC 6). + + #698: head proofs are phase-specific. A legitimately blocked run that + never began validation (no reviewed head, no formal verdict, no merge) + owes none of them; approval-time and merge-time live-head proofs are + owed only once the corresponding phase actually begins. + """ text = report_text or "" reasons: list[str] = [] reviewed = _normalize_sha(_REVIEWED_HEAD_RE.search(text).group(1) if _REVIEWED_HEAD_RE.search(text) else None) @@ -422,15 +449,49 @@ def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]: ) push_during = _PUSH_DURING_VALIDATION_RE.search(text) - if not reviewed: + # Phase detection from the report's own claims. + no_head_stated = bool(_NO_REVIEWED_HEAD_RE.search(text)) + verdict_recorded = bool(_VERDICT_RECORDED_RE.search(text)) + merge_attempted = bool(_MERGE_ATTEMPTED_RE.search(text)) + validation_started = bool(reviewed) or bool(_VALIDATION_STARTED_RE.search(text)) + blocked_before_validation = ( + no_head_stated + and not reviewed + and not verdict_recorded + and not merge_attempted + and not validation_started + ) + + if blocked_before_validation: + return { + "proven": True, + "block": False, + "reasons": [], + "reviewed_head_sha": None, + "live_head_sha_before_approval": None, + "live_head_sha_before_merge": None, + "push_during_validation": ( + push_during.group(1).lower() if push_during else None + ), + "phase": "blocked_before_validation", + } + + if not reviewed and not no_head_stated: + # The head must always be STATED — either a SHA or an explicit + # 'none'. Silence is not a phase claim and fails closed. + reasons.append( + "reviewed head SHA not stated in final report " + "(state the SHA or an explicit 'none')" + ) + elif not reviewed and (validation_started or verdict_recorded or merge_attempted): reasons.append("reviewed head SHA not stated in final report") - if not live_approval: + if verdict_recorded and not live_approval: reasons.append("final live head SHA before approval not stated") - if not live_merge: + if merge_attempted and not live_merge: reasons.append("final live head SHA before merge not stated") - if not push_during: + if validation_started and not push_during: reasons.append("whether push occurred during validation not stated") - elif reviewed and live_approval and reviewed != live_approval: + if reviewed and live_approval and reviewed != live_approval: reasons.append("live head before approval differs from reviewed head SHA") elif reviewed and live_merge and reviewed != live_merge: reasons.append("live head before merge differs from reviewed head SHA") diff --git a/review_final_report_schema.py b/review_final_report_schema.py index e428015..10cb5ad 100644 --- a/review_final_report_schema.py +++ b/review_final_report_schema.py @@ -25,8 +25,13 @@ _REVIEWED_HEAD_RE = re.compile( r"(?:pinned reviewed head|reviewed head sha)\s*:\s*([0-9a-f]{7,40})", re.IGNORECASE, ) +# #698: validation pass proof appears in several legitimate shapes — +# "Validation: pass", "Validation: focused 50 passed; full 2665 passed", +# or structured "validation_status: pass". Accept pass evidence anywhere in +# the Validation field's value, not only as its first token. _VALIDATION_PASS_RE = re.compile( - r"validation\s*:\s*(?:pass|passed|strong|ok|green)", + r"validation(?:_status)?\s*:[^\n]{0,300}?" + r"(?:\bpass(?:ed)?\b|\bstrong\b|\bok\b|\bgreen\b|\d+\s+passed)", re.IGNORECASE, ) _MERGED_CLAIM_RE = re.compile( diff --git a/review_proofs.py b/review_proofs.py index b4b0ea2..146f039 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -858,9 +858,16 @@ _WALKTHROUGH_ARTIFACT_RE = re.compile(r"walkthrough\.md", re.I) def _performed_file_mutations(action_log: list[dict] | None) -> list[dict]: - """Return performed local file mutations, excluding gated rejections.""" + """Return performed local file mutations, excluding gated rejections. + + Non-dict entries (malformed JSON, LLM mistakes) are ignored instead of + raising ``AttributeError`` (#698): a malformed ledger entry can never be + authoritative mutation evidence. + """ performed: list[dict] = [] for entry in action_log or []: + if not isinstance(entry, dict): + continue if entry.get("gated_rejected") or entry.get("performed") is False: continue action = (entry.get("action") or "").strip().lower() @@ -2228,24 +2235,31 @@ HANDOFF_REVIEW_MUTATION_FIELDS = ( ) HANDOFF_ROLE_FIELDS = { + # #698: the review/merger required-field sets must stay aligned with the + # canonical schema (skills/llm-project-workflow/schemas/ + # review-merge-final-report.md). The schema explicitly FORBIDS the legacy + # fields 'Pinned reviewed head', 'Scratch worktree used', and 'Workspace + # mutations' — a validator must never demand a field the schema bans. "review": ( ("Selected PR", ("selected pr",)), ("Reviewer eligibility", ("reviewer eligibility", "eligibility")), - ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), - ("Worktree path", ("worktree path", "starting worktree path")), - ("Worktree dirty", ("worktree dirty", "whether worktree was dirty")), - ("Scratch worktree used", ("scratch worktree used", "scratch clone used", - "scratch worktree")), + ("Reviewed head SHA", ("reviewed head sha", "candidate head sha")), + ("Review worktree path", ("review worktree path", "worktree path", + "starting worktree path")), + ("Review worktree dirty", ("review worktree dirty", "worktree dirty", + "whether worktree was dirty")), ("Unrelated local mutations", ("unrelated local mutations", - "unrelated files modified")), + "unrelated files modified", + "file edits by reviewer")), ("Review decision", ("review decision", "decision")), ("Merge result", ("merge result",)), ("Linked issue status", ("linked issue status", "linked issue")), ("Cleanup status", ("cleanup status", "cleanup")), + ("Safe next action", ("safe next action", "next")), ) + HANDOFF_REVIEW_MUTATION_FIELDS, "merger": ( ("Selected PR", ("selected pr",)), - ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), + ("Reviewed head SHA", ("reviewed head sha", "candidate head sha")), ("Active profile", ("active profile",)), ("Role kind", ("role kind",)), ("Merge capability source", ("merge capability source",)), @@ -2433,9 +2447,22 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): # 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. + # Issue #698: the canonical review-merge schema has no 'Mutations', + # 'Next', 'Issue/PR', 'Branch/SHA', or 'Files changed' fields — their + # content lives in the precise mutation categories, 'Safe next + # action', 'Selected PR'/'Linked issue', head-SHA fields, and 'Files + # reviewed'. Requiring the legacy names rejects canonical reports. + _non_canonical_for_review = { + "Workspace mutations", + "Mutations", + "Next", + "Issue/PR", + "Branch/SHA", + "Files changed", + } required = [ field for field in required - if field[0] != "Workspace mutations" + if field[0] not in _non_canonical_for_review ] if any(label.startswith("workspace mutations") for label in labels): return { diff --git a/tests/test_issue_698_report_validator_schema_alignment.py b/tests/test_issue_698_report_validator_schema_alignment.py new file mode 100644 index 0000000..a02b97e --- /dev/null +++ b/tests/test_issue_698_report_validator_schema_alignment.py @@ -0,0 +1,461 @@ +"""Regression tests for #698: final-report validator vs canonical schema. + +Covers the original #698 lead plus the independent reproduction recorded +during the PR #703 formal review (issue #698 comment 11246): + +1. non-dict ``action_log`` entries must fail structured, never crash; +2. the validator must not demand legacy fields the canonical schema forbids + (``Pinned reviewed head``, ``Scratch worktree used``, ``Worktree path``, + ``Worktree dirty``, ``Mutations``, ``Next``); +3. a legitimately blocked report (``Candidate head SHA: none``, no formal + verdict) must not owe approval/merge live-head proofs; +4. canonical reviewer lease release must not be misclassified as post-merge + cleanup; +5. structured workflow-load and validation proof must be recognized; +6. review mutations are inferred only from authoritative evidence. +""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import final_report_validator as frv # noqa: E402 +import post_merge_cleanup_proof as pmcp # noqa: E402 +import pr_work_lease as pwl # noqa: E402 +import review_proofs as rp # noqa: E402 +from review_final_report_schema import ( # noqa: E402 + assess_review_final_report_schema, +) + +REVIEWED_HEAD = "a" * 40 +LIVE_HEAD = "a" * 40 + + +def _pr703_style_report( + *, + decision: str = "request_changes", + cleanup_mutations: str = ( + "released reviewer PR lease via gitea_release_reviewer_pr_lease " + "(terminal lease marker phase=released posted)" + ), +) -> str: + """Canonical-schema report modeled on the PR #703 formal review handoff.""" + return f""" +Formal review completed with a REQUEST_CHANGES verdict submitted and read +back via the native review API. + +## Controller Handoff + +- Task: review-merge-pr +- Repo: Scaled-Tech-Consulting/Gitea-Tools +- Role: reviewer +- Identity: sysadmin / prgs-reviewer +- Active profile: prgs-reviewer +- Runtime context: neutral workspace binding +- Selected PR: 703 +- Linked issue: #702 open +- Eligibility class: reviewable +- Queue ordering policy: oldest eligible first +- Inventory pagination proof: has_more=false, total_count=8 +- Earlier PRs skipped: none +- Candidate head SHA: {REVIEWED_HEAD} +- Reviewed head SHA: {REVIEWED_HEAD} +- Target branch: master +- Target branch SHA: {"2" * 40} +- Already-landed gate: not landed +- Author-safety result: pass (author differs from reviewer) +- Prior request-changes state: none +- Review worktree used: true +- Review worktree path: branches/review-pr-703-independent +- Review worktree inside branches: true +- Review worktree HEAD state: detached at pinned head +- Review worktree dirty before validation: clean +- Review worktree dirty after validation: clean +- Baseline worktree used: false +- Baseline worktree path: none +- Files reviewed: 4 +- Validation: focused 50 passed; related 94 passed; full 2665 passed, 6 skipped +- Official validation integrity status: intact +- Terminal review mutation: one REQUEST_CHANGES review submitted and read back +- Review decision: {decision} +- Merge preflight: not run +- Merge result: none +- Linked issue status: open (live fetch proof: gitea_view_issue) +- Main checkout branch: master +- Main checkout dirty state: clean +- Main checkout updated: false +- File edits by reviewer: none +- Worktree/index mutations: none +- Git ref mutations: git fetch prgs (recorded) +- MCP/Gitea mutations: review submission and lease comments only +- Review mutations: one formal REQUEST_CHANGES verdict +- Merge mutations: none +- Cleanup mutations: {cleanup_mutations} +- External-state mutations: none +- Read-only diagnostics: gitea_view_pr, gitea_get_pr_review_feedback +- Blockers: findings F1-F6 recorded on the PR thread +- Current status: review complete; author remediation required +- Safe next action: author addresses findings and pushes a new head +- Safety statement: no merge attempted; no self-review; no root-checkout edits +- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean +- Live head SHA before approval: {LIVE_HEAD} +- Pushes occurred during validation: no +""" + + +def _blocked_preflight_report() -> str: + """Blocked-run report modeled on the #702 comment 11164 reproduction.""" + return """ +Fresh review preflight stopped before any worktree or validation work. + +## Controller Handoff + +- Task: review-merge-pr +- Repo: Scaled-Tech-Consulting/Gitea-Tools +- Role: reviewer +- Identity: sysadmin / prgs-reviewer +- Active profile: prgs-reviewer +- Runtime context: stale workspace binding detected +- Selected PR: 701 +- Linked issue: #699 open +- Eligibility class: blocked-before-validation +- Queue ordering policy: oldest eligible first +- Inventory pagination proof: has_more=false, total_count=8 +- Earlier PRs skipped: none +- Candidate head SHA: none +- Reviewed head SHA: none +- Target branch: master +- Target branch SHA: none +- Already-landed gate: not run +- Author-safety result: not run +- Prior request-changes state: none +- Review worktree used: false +- Review worktree path: none +- Review worktree inside branches: not applicable +- Review worktree HEAD state: not applicable +- Review worktree dirty before validation: not applicable +- Review worktree dirty after validation: not applicable +- Baseline worktree used: false +- Baseline worktree path: none +- Files reviewed: 0 +- Validation: not run +- Official validation integrity status: not applicable +- Terminal review mutation: none +- Review decision: none +- Merge preflight: not run +- Merge result: none +- Linked issue status: open (live fetch proof: gitea_view_issue) +- Main checkout branch: master +- Main checkout dirty state: clean +- Main checkout updated: false +- File edits by reviewer: none +- Worktree/index mutations: none +- Git ref mutations: none +- MCP/Gitea mutations: none +- Review mutations: none +- Merge mutations: none +- Cleanup mutations: none +- External-state mutations: none +- Read-only diagnostics: gitea_view_pr, gitea_get_runtime_context +- Blockers: runtime bound to a foreign task worktree; mutation prohibited +- Current status: stopped before validation began +- Next actor: operator +- Next action: repair the runtime workspace binding, then rerun the full + review workflow in a fresh reviewer session +- Next prompt: Act as REVIEWER for PR 701 after the operator repairs the + runtime binding; acquire the lease before any validation. +- Safe next action: operator repairs runtime binding, then a fresh reviewer + reruns the full workflow +- Safety statement: no lease acquired; no verdict recorded; no source edits +- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean +""" + + +class TestActionLogRobustness(unittest.TestCase): + """#698 original lead: non-dict action_log must not crash validation.""" + + MALFORMED = [ + "git fetch prgs", + 42, + None, + {"action": "edit", "path": "x.py", "performed": True, "tracked": True}, + ] + + def test_assess_final_report_validator_survives_malformed_entries(self): + result = frv.assess_final_report_validator( + _pr703_style_report(), + "review_pr", + action_log=self.MALFORMED, + ) + self.assertIsInstance(result, dict) + rule_ids = {f["rule_id"] for f in result["findings"]} + self.assertIn("shared.action_log_malformed", rule_ids) + + def test_malformed_entry_errors_are_sanitized(self): + _entries, findings = frv.sanitize_action_log(["secret-token-abc123"]) + self.assertEqual(len(findings), 1) + reason = findings[0]["reason"] + self.assertNotIn("secret-token-abc123", reason) + self.assertIn("str", reason) + self.assertIn("entry 0", reason) + + def test_non_list_action_log_is_reported_not_raised(self): + entries, findings = frv.sanitize_action_log("not-a-list") + self.assertEqual(entries, []) + self.assertEqual(len(findings), 1) + self.assertIn("not a list", findings[0]["reason"]) + + def test_performed_file_mutations_skips_non_dict_entries(self): + performed = rp._performed_file_mutations( + ["oops", {"action": "edited", "path": "a.py"}] + ) + self.assertEqual(len(performed), 1) + self.assertEqual(performed[0]["path"], "a.py") + + def test_schema_entrypoint_survives_string_only_log(self): + result = assess_review_final_report_schema( + _pr703_style_report(), + action_log=["just a string", "another string"], + ) + self.assertIsInstance(result, dict) + + +class TestLegacyFieldRequirementsRemoved(unittest.TestCase): + """#698: prohibited legacy fields must not be REQUIRED of reports.""" + + PROHIBITED = ( + "Pinned reviewed head", + "Scratch worktree used", + "Worktree path", + "Worktree dirty", + "Workspace mutations", + "Mutations", + "Next", + "Issue/PR", + "Branch/SHA", + "Files changed", + ) + + def test_review_role_field_table_has_no_prohibited_requirements(self): + names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["review"]] + for prohibited in ("Pinned reviewed head", "Scratch worktree used", + "Worktree path", "Worktree dirty"): + self.assertNotIn(prohibited, names) + + def test_merger_role_field_table_has_no_pinned_reviewed_head(self): + names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["merger"]] + self.assertNotIn("Pinned reviewed head", names) + + def test_canonical_report_missing_fields_never_include_prohibited(self): + result = rp.assess_controller_handoff( + _pr703_style_report(), role="review" + ) + for prohibited in self.PROHIBITED: + self.assertNotIn(prohibited, result.get("missing_fields") or []) + + def test_canonical_pr703_report_satisfies_required_fields(self): + result = rp.assess_controller_handoff( + _pr703_style_report(), role="review" + ) + self.assertEqual(result.get("missing_fields") or [], []) + self.assertEqual(result.get("verdict"), "complete") + + +class TestBlockedReportAccepted(unittest.TestCase): + """#698: blocked run with no reviewed head / verdict is legitimate.""" + + def test_stale_head_proof_waived_before_validation(self): + result = pwl.assess_reviewer_stale_head_final_report( + _blocked_preflight_report() + ) + self.assertTrue(result["proven"]) + self.assertEqual(result.get("phase"), "blocked_before_validation") + + def test_blocked_report_passes_schema_validation(self): + result = assess_review_final_report_schema(_blocked_preflight_report()) + blocking = [ + f for f in result["findings"] if f["severity"] == "block" + ] + self.assertEqual(blocking, [], blocking) + + def test_verdict_phase_still_demands_approval_head_proof(self): + report = _blocked_preflight_report().replace( + "- Review decision: none", + "- Review decision: approve", + ).replace( + "- Candidate head SHA: none", + f"- Candidate head SHA: {REVIEWED_HEAD}", + ) + result = pwl.assess_reviewer_stale_head_final_report(report) + self.assertFalse(result["proven"]) + joined = " ".join(result["reasons"]) + self.assertIn("before approval", joined) + + def test_merge_phase_still_demands_merge_head_proof(self): + report = _pr703_style_report().replace( + "- Merge result: none", + "- Merge result: merged", + ) + result = pwl.assess_reviewer_stale_head_final_report(report) + self.assertFalse(result["proven"]) + self.assertIn( + "final live head SHA before merge not stated", + result["reasons"], + ) + + def test_validation_phase_demands_push_disclosure(self): + report = _pr703_style_report().replace( + "- Pushes occurred during validation: no\n", "" + ) + result = pwl.assess_reviewer_stale_head_final_report(report) + self.assertFalse(result["proven"]) + self.assertIn( + "whether push occurred during validation not stated", + result["reasons"], + ) + + +class TestLeaseReleaseVsPostMergeCleanup(unittest.TestCase): + """#698 (PR #703 review reproduction): lease release is not cleanup.""" + + def test_lease_release_cleanup_mutations_do_not_demand_checklist(self): + result = pmcp.assess_post_merge_cleanup_proof(_pr703_style_report()) + self.assertFalse(result["block"], result["reasons"]) + + def test_release_tool_name_alone_is_recognized(self): + report = _pr703_style_report( + cleanup_mutations="gitea_release_reviewer_pr_lease comment 11244" + ) + result = pmcp.assess_post_merge_cleanup_proof(report) + self.assertFalse(result["block"], result["reasons"]) + + def test_substantive_non_lease_cleanup_still_demands_checklist(self): + report = _pr703_style_report( + cleanup_mutations="deleted stale scratch directory manually" + ) + result = pmcp.assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_remote_branch_delete_claims_still_demand_full_proof(self): + report = _pr703_style_report( + cleanup_mutations="gitea_delete_branch removed the remote branch" + ) + result = pmcp.assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + self.assertTrue( + any("remote branch deletion missing" in r for r in result["reasons"]) + ) + + def test_full_schema_run_accepts_lease_release_report(self): + result = assess_review_final_report_schema(_pr703_style_report()) + lease_cleanup_blocks = [ + f for f in result["findings"] + if f["rule_id"] == "reviewer.post_merge_cleanup_proof" + ] + self.assertEqual(lease_cleanup_blocks, [], lease_cleanup_blocks) + + +class TestStructuredProofRecognition(unittest.TestCase): + """#698: structured workflow-load and validation proof must be accepted.""" + + def test_key_value_workflow_proof_recognized(self): + findings = frv._rule_reviewer_workflow_load_boundary( + _pr703_style_report() + ) + self.assertEqual(findings, [], findings) + + def test_colon_form_workflow_proof_still_recognized(self): + report = _pr703_style_report().replace( + "- Workflow-load helper result: workflow_hash=da045d1e1f1f " + "boundary_status=clean", + "- Workflow-load helper result: workflow_hash: da045d1e1f1f, " + "boundary_status: clean", + ) + findings = frv._rule_reviewer_workflow_load_boundary(report) + self.assertEqual(findings, [], findings) + + def test_incomplete_structured_proof_still_blocks(self): + report = _pr703_style_report().replace( + "workflow_hash=da045d1e1f1f boundary_status=clean", + "workflow_hash=da045d1e1f1f", + ) + findings = frv._rule_reviewer_workflow_load_boundary(report) + self.assertTrue(findings) + self.assertIn("boundary_status", findings[0]["reason"]) + + def test_validation_counts_accepted_as_pass_proof(self): + # "Validation: focused 50 passed; ..." must satisfy the reviewed-head + # validation-proof rule (PR #703 reproduction). + result = assess_review_final_report_schema(_pr703_style_report()) + head_blocks = [ + f for f in result["findings"] + if f["rule_id"] == "reviewer.reviewed_head_without_validation" + ] + self.assertEqual(head_blocks, [], head_blocks) + + +class TestAuthoritativeMutationInference(unittest.TestCase): + """#698: review mutations inferred only from authoritative evidence.""" + + def test_read_only_entries_do_not_imply_mutations(self): + report = "## Controller Handoff\n- Mutations: none\n" + findings = frv._rule_reviewer_vague_mutations_none( + report, + action_log=[ + {"action": "gitea_view_pr"}, + {"action": "gitea_get_pr_review_feedback", "performed": False}, + ], + ) + self.assertEqual(findings, [], findings) + + def test_performed_mutation_still_blocks_vague_none(self): + report = "## Controller Handoff\n- Mutations: none\n" + findings = frv._rule_reviewer_vague_mutations_none( + report, + action_log=[{"action": "edit", "path": "a.py", "performed": True}], + ) + self.assertTrue(findings) + + def test_gated_rejection_is_not_a_mutation(self): + report = "## Controller Handoff\n- Mutations: none\n" + findings = frv._rule_reviewer_vague_mutations_none( + report, + action_log=[ + {"action": "edit", "path": "a.py", "performed": True, + "gated_rejected": True}, + ], + ) + self.assertEqual(findings, [], findings) + + +class TestValidatorRuleErrorContainment(unittest.TestCase): + """#698: a defective rule fails closed with a sanitized error.""" + + def test_rule_exception_becomes_sanitized_block_finding(self): + def _boom(report_text): + raise ValueError("raw secret detail that must not leak") + + original = frv._RULES_BY_TASK["review_pr"] + frv._RULES_BY_TASK["review_pr"] = [_boom] + try: + result = frv.assess_final_report_validator( + "report body", "review_pr" + ) + finally: + frv._RULES_BY_TASK["review_pr"] = original + self.assertTrue(result["blocked"]) + finding = next( + f for f in result["findings"] + if f["rule_id"] == "shared.validator_rule_error" + ) + self.assertNotIn("raw secret detail", finding["reason"]) + self.assertIn("ValueError", finding["reason"]) + self.assertIn("_boom", finding["reason"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_review_proofs.py b/tests/test_review_proofs.py index 03d3a6a..dba3c4e 100644 --- a/tests/test_review_proofs.py +++ b/tests/test_review_proofs.py @@ -957,17 +957,20 @@ class TestControllerHandoff(unittest.TestCase): if not line.startswith("- Workspace mutations:")) result = assess_controller_handoff(review_base, role="review") self.assertEqual(result["verdict"], "incomplete") - self.assertIn("Pinned reviewed head", result["missing_fields"]) - self.assertIn("Worktree path", result["missing_fields"]) + # #698: the canonical schema forbids the legacy fields, so the + # validator must demand the canonical names instead. + self.assertIn("Reviewed head SHA", result["missing_fields"]) + self.assertIn("Review worktree path", result["missing_fields"]) self.assertIn("Merge result", result["missing_fields"]) + for legacy in ("Pinned reviewed head", "Scratch worktree used"): + self.assertNotIn(legacy, result["missing_fields"]) complete = review_base + "\n" + "\n".join([ "- Selected PR: #999", "- Reviewer eligibility: passed", - "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", - "- Worktree path: /repo/branches/review-pr-999", - "- Worktree dirty: no", - "- Scratch worktree used: yes (/repo/branches/review-pr-999)", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Review worktree path: /repo/branches/review-pr-999", + "- Review worktree dirty before validation: no", "- Unrelated local mutations: none", "- Review decision: approve", "- Merge result: merged", @@ -1125,10 +1128,9 @@ class TestReviewHandoffPreciseMutationCategories(unittest.TestCase): "- Safety: no self-review; no self-merge; no secrets", "- Selected PR: #999", "- Reviewer eligibility: passed", - "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", "- Worktree path: /repo/branches/review-pr-999", "- Worktree dirty: no", - "- Scratch worktree used: yes (/repo/branches/review-pr-999)", "- Unrelated local mutations: none", "- Review decision: approve", "- Merge result: none", -- 2.43.7