fix(validator): align final-report validator with canonical schema (Closes #698) #705
@@ -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]
|
||||
|
||||
@@ -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"
|
||||
)
|
||||
|
||||
+67
-6
@@ -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")
|
||||
|
||||
@@ -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(
|
||||
|
||||
+36
-9
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user