Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
ee90a5e7a2 |
@@ -134,16 +134,22 @@ _TARGET_BRANCH_SHA_RE = re.compile(
|
|||||||
r"target branch sha\s*:\s*[0-9a-f]{40}",
|
r"target branch sha\s*:\s*[0-9a-f]{40}",
|
||||||
re.IGNORECASE,
|
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(
|
_WORKFLOW_LOAD_HELPER_RE = re.compile(
|
||||||
r"workflow[- ]load helper result\s*:",
|
r"workflow[-_ ]load[-_ ]helper[-_ ]result\s*[:=]",
|
||||||
re.IGNORECASE,
|
re.IGNORECASE,
|
||||||
)
|
)
|
||||||
_WORKFLOW_LOAD_HASH_RE = re.compile(
|
_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,
|
re.IGNORECASE,
|
||||||
)
|
)
|
||||||
_WORKFLOW_LOAD_BOUNDARY_RE = re.compile(
|
_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,
|
re.IGNORECASE,
|
||||||
)
|
)
|
||||||
_WORKFLOW_FILE_VIEW_NARRATIVE_RE = re.compile(
|
_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)
|
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(
|
def validator_finding(
|
||||||
rule_id: str,
|
rule_id: str,
|
||||||
severity: 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_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text))
|
||||||
rejected_in_log = False
|
rejected_in_log = False
|
||||||
if action_log:
|
if action_log:
|
||||||
for entry in action_log:
|
for entry in _iter_action_entries(action_log):
|
||||||
validation = entry.get("canonical_comment_validation") or {}
|
validation = entry.get("canonical_comment_validation") or {}
|
||||||
if validation.get("allowed") is False:
|
if validation.get("allowed") is False:
|
||||||
rejected_in_log = True
|
rejected_in_log = True
|
||||||
@@ -475,9 +534,13 @@ def _rule_reviewer_vague_mutations_none(
|
|||||||
action_log: list[dict] | None = None,
|
action_log: list[dict] | None = None,
|
||||||
mutations_observed: bool = False,
|
mutations_observed: bool = False,
|
||||||
) -> list[dict[str, str]]:
|
) -> 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(
|
performed = any(
|
||||||
e.get("performed") is not False and not e.get("gated_rejected")
|
e.get("performed") is True and not e.get("gated_rejected")
|
||||||
for e in (action_log or [])
|
for e in _iter_action_entries(action_log)
|
||||||
)
|
)
|
||||||
if not (mutations_observed or performed):
|
if not (mutations_observed or performed):
|
||||||
return []
|
return []
|
||||||
@@ -536,7 +599,7 @@ def _rule_reviewer_git_fetch_readonly(
|
|||||||
text = report_text or ""
|
text = report_text or ""
|
||||||
fetch_observed = any(
|
fetch_observed = any(
|
||||||
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
|
_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)
|
) or _GIT_FETCH_RE.search(text)
|
||||||
if not fetch_observed:
|
if not fetch_observed:
|
||||||
return []
|
return []
|
||||||
@@ -977,7 +1040,7 @@ def _rule_reviewer_target_branch_freshness(
|
|||||||
fields = _handoff_fields(text)
|
fields = _handoff_fields(text)
|
||||||
fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any(
|
fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any(
|
||||||
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
|
_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_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)
|
"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] = {}
|
checks: dict[str, Any] = {}
|
||||||
findings: list[dict[str, str]] = []
|
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:
|
if normalized_kind == "issue_filing" and issue_filing_lock is not None:
|
||||||
checks["issue_filing"] = assess_issue_filing_final_report(
|
checks["issue_filing"] = assess_issue_filing_final_report(
|
||||||
report_text,
|
report_text,
|
||||||
@@ -1763,7 +1832,23 @@ def assess_final_report_validator(
|
|||||||
}
|
}
|
||||||
|
|
||||||
for rule in _RULES_BY_TASK.get(normalized_kind, ()):
|
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)
|
grade, blocked, downgraded = _aggregate_grade(findings)
|
||||||
reasons = [f"{f['rule_id']}: {f['reason']}" for f in 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",
|
r"deleted branch (?:does not match|!=|differs from) (?:merged )?pr head",
|
||||||
re.IGNORECASE,
|
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:
|
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)
|
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:
|
if not remote_delete and not worktree_remove:
|
||||||
cleanup_mutations = re.search(
|
value_match = _CLEANUP_MUTATIONS_VALUE_RE.search(text)
|
||||||
r"cleanup mutations\s*:\s*(?!none\b)\S",
|
value = (value_match.group(1).strip() if value_match else "")
|
||||||
text,
|
value_lower = value.lower()
|
||||||
re.IGNORECASE,
|
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(
|
reasons.append(
|
||||||
"cleanup mutations reported without post-merge cleanup proof checklist"
|
"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)",
|
r"whether any reviewer was active\s*:\s*(yes|no|true|false)",
|
||||||
re.IGNORECASE,
|
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]:
|
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 ""
|
text = report_text or ""
|
||||||
reasons: list[str] = []
|
reasons: list[str] = []
|
||||||
reviewed = _normalize_sha(_REVIEWED_HEAD_RE.search(text).group(1) if _REVIEWED_HEAD_RE.search(text) else None)
|
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)
|
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")
|
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")
|
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")
|
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")
|
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")
|
reasons.append("live head before approval differs from reviewed head SHA")
|
||||||
elif reviewed and live_merge and reviewed != live_merge:
|
elif reviewed and live_merge and reviewed != live_merge:
|
||||||
reasons.append("live head before merge differs from reviewed head SHA")
|
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})",
|
r"(?:pinned reviewed head|reviewed head sha)\s*:\s*([0-9a-f]{7,40})",
|
||||||
re.IGNORECASE,
|
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(
|
_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,
|
re.IGNORECASE,
|
||||||
)
|
)
|
||||||
_MERGED_CLAIM_RE = re.compile(
|
_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]:
|
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] = []
|
performed: list[dict] = []
|
||||||
for entry in action_log or []:
|
for entry in action_log or []:
|
||||||
|
if not isinstance(entry, dict):
|
||||||
|
continue
|
||||||
if entry.get("gated_rejected") or entry.get("performed") is False:
|
if entry.get("gated_rejected") or entry.get("performed") is False:
|
||||||
continue
|
continue
|
||||||
action = (entry.get("action") or "").strip().lower()
|
action = (entry.get("action") or "").strip().lower()
|
||||||
@@ -2228,24 +2235,31 @@ HANDOFF_REVIEW_MUTATION_FIELDS = (
|
|||||||
)
|
)
|
||||||
|
|
||||||
HANDOFF_ROLE_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": (
|
"review": (
|
||||||
("Selected PR", ("selected pr",)),
|
("Selected PR", ("selected pr",)),
|
||||||
("Reviewer eligibility", ("reviewer eligibility", "eligibility")),
|
("Reviewer eligibility", ("reviewer eligibility", "eligibility")),
|
||||||
("Pinned reviewed head", ("pinned reviewed head", "pinned head")),
|
("Reviewed head SHA", ("reviewed head sha", "candidate head sha")),
|
||||||
("Worktree path", ("worktree path", "starting worktree path")),
|
("Review worktree path", ("review worktree path", "worktree path",
|
||||||
("Worktree dirty", ("worktree dirty", "whether worktree was dirty")),
|
"starting worktree path")),
|
||||||
("Scratch worktree used", ("scratch worktree used", "scratch clone used",
|
("Review worktree dirty", ("review worktree dirty", "worktree dirty",
|
||||||
"scratch worktree")),
|
"whether worktree was dirty")),
|
||||||
("Unrelated local mutations", ("unrelated local mutations",
|
("Unrelated local mutations", ("unrelated local mutations",
|
||||||
"unrelated files modified")),
|
"unrelated files modified",
|
||||||
|
"file edits by reviewer")),
|
||||||
("Review decision", ("review decision", "decision")),
|
("Review decision", ("review decision", "decision")),
|
||||||
("Merge result", ("merge result",)),
|
("Merge result", ("merge result",)),
|
||||||
("Linked issue status", ("linked issue status", "linked issue")),
|
("Linked issue status", ("linked issue status", "linked issue")),
|
||||||
("Cleanup status", ("cleanup status", "cleanup")),
|
("Cleanup status", ("cleanup status", "cleanup")),
|
||||||
|
("Safe next action", ("safe next action", "next")),
|
||||||
) + HANDOFF_REVIEW_MUTATION_FIELDS,
|
) + HANDOFF_REVIEW_MUTATION_FIELDS,
|
||||||
"merger": (
|
"merger": (
|
||||||
("Selected PR", ("selected pr",)),
|
("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",)),
|
("Active profile", ("active profile",)),
|
||||||
("Role kind", ("role kind",)),
|
("Role kind", ("role kind",)),
|
||||||
("Merge capability source", ("merge capability source",)),
|
("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
|
# Issue #320: reviewer and merger handoffs use the precise mutation categories
|
||||||
# in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous
|
# in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous
|
||||||
# "Workspace mutations" field, which is rejected below.
|
# "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 = [
|
required = [
|
||||||
field for field in 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):
|
if any(label.startswith("workspace mutations") for label in labels):
|
||||||
return {
|
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:"))
|
if not line.startswith("- Workspace mutations:"))
|
||||||
result = assess_controller_handoff(review_base, role="review")
|
result = assess_controller_handoff(review_base, role="review")
|
||||||
self.assertEqual(result["verdict"], "incomplete")
|
self.assertEqual(result["verdict"], "incomplete")
|
||||||
self.assertIn("Pinned reviewed head", result["missing_fields"])
|
# #698: the canonical schema forbids the legacy fields, so the
|
||||||
self.assertIn("Worktree path", result["missing_fields"])
|
# 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"])
|
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([
|
complete = review_base + "\n" + "\n".join([
|
||||||
"- Selected PR: #999",
|
"- Selected PR: #999",
|
||||||
"- Reviewer eligibility: passed",
|
"- Reviewer eligibility: passed",
|
||||||
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||||
"- Worktree path: /repo/branches/review-pr-999",
|
"- Review worktree path: /repo/branches/review-pr-999",
|
||||||
"- Worktree dirty: no",
|
"- Review worktree dirty before validation: no",
|
||||||
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
|
|
||||||
"- Unrelated local mutations: none",
|
"- Unrelated local mutations: none",
|
||||||
"- Review decision: approve",
|
"- Review decision: approve",
|
||||||
"- Merge result: merged",
|
"- Merge result: merged",
|
||||||
@@ -1125,10 +1128,9 @@ class TestReviewHandoffPreciseMutationCategories(unittest.TestCase):
|
|||||||
"- Safety: no self-review; no self-merge; no secrets",
|
"- Safety: no self-review; no self-merge; no secrets",
|
||||||
"- Selected PR: #999",
|
"- Selected PR: #999",
|
||||||
"- Reviewer eligibility: passed",
|
"- Reviewer eligibility: passed",
|
||||||
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||||
"- Worktree path: /repo/branches/review-pr-999",
|
"- Worktree path: /repo/branches/review-pr-999",
|
||||||
"- Worktree dirty: no",
|
"- Worktree dirty: no",
|
||||||
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
|
|
||||||
"- Unrelated local mutations: none",
|
"- Unrelated local mutations: none",
|
||||||
"- Review decision: approve",
|
"- Review decision: approve",
|
||||||
"- Merge result: none",
|
"- Merge result: none",
|
||||||
|
|||||||
Reference in New Issue
Block a user