feat(review-workflow): raise A-bar with capability, sweep, live-state, and role-boundary proofs (#179)
Extends review_proofs.py with the four #179 proofs, the successor set to the #173 checkout/inventory proofs: - assess_capability_evidence: a capability claim (review_pr, merge_pr, ...) counts only with exact evidence citing gitea_resolve_task_capability output or equivalent runtime context; no claims at all fails closed. - assess_sweep_evidence: secret/provenance sweeps must state the exact command/script/pattern/named method, the scope scanned, and a boolean result; vague summaries are downgraded and a missing sweep fails closed. - assess_live_state_recheck: an explicit pre-mutation recheck must prove the PR is still open, the live head equals the pinned head (full 40-hex), the base branch is unchanged, and blocking review state was checked and absent; not performing it fails closed. - assess_role_boundary: a reviewer run using an author namespace (or vice versa) is clean only with an explicit justification; unreported namespace usage fails closed. build_final_report now takes the four proofs as keyword arguments: any missing or failed proof downgrades the grade, merge_allowed additionally requires the proven live-state recheck, and a merge performed without it is a blocked violation. Existing #173 semantics are unchanged otherwise; gates only get stricter. tests/test_review_proofs.py adds 29 tests covering the issue's harness assertions: capability claims without evidence downgraded, vague sweeps downgraded, missing/stale live-state recheck downgrades and blocks merge (violation when a merge is claimed anyway), unjustified author-namespace use downgraded, and the #173 positive baseline preserved. SKILL.md sections F/G and the review-pr/merge-pr templates now require the capability evidence, exact sweep, pre-verdict and pre-merge live-state rechecks, and reviewer-namespace discipline. Closes #179 Co-Authored-By: Claude Fable 5 <[email protected]>
This commit is contained in:
+229
-12
@@ -330,22 +330,167 @@ def assess_self_review_contamination(reviewer_identity, pr_author,
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def assess_role_boundary(proof):
|
def assess_capability_evidence(capability_claims):
|
||||||
|
"""#179 gap 1: a capability claim needs exact evidence, not assertion.
|
||||||
|
|
||||||
|
*capability_claims* is a list of ``{'task', 'allowed',
|
||||||
|
'evidence_source'}`` dicts, one per capability the report claims (e.g.
|
||||||
|
review_pr, merge_pr). Each claim must name its task, be allowed, and
|
||||||
|
cite an exact evidence source (``gitea_resolve_task_capability`` output
|
||||||
|
or equivalent runtime-context evidence). No claims at all fails closed.
|
||||||
|
"""
|
||||||
|
reasons = []
|
||||||
|
claims = capability_claims or []
|
||||||
|
if not claims:
|
||||||
|
reasons.append(
|
||||||
|
"no capability evidence provided; capability checks may not be "
|
||||||
|
"claimed as passed"
|
||||||
|
)
|
||||||
|
for claim in claims:
|
||||||
|
task = (claim.get("task") or "").strip() or "<unnamed task>"
|
||||||
|
if claim.get("allowed") is not True:
|
||||||
|
reasons.append(
|
||||||
|
f"capability '{task}' is not proven allowed; fail closed"
|
||||||
|
)
|
||||||
|
if not (claim.get("evidence_source") or "").strip():
|
||||||
|
reasons.append(
|
||||||
|
f"capability '{task}' claimed without exact evidence "
|
||||||
|
"(cite gitea_resolve_task_capability output or equivalent)"
|
||||||
|
)
|
||||||
|
proven = not reasons
|
||||||
|
return {"proven": proven, "reasons": reasons, "claims": len(claims)}
|
||||||
|
|
||||||
|
|
||||||
|
def assess_sweep_evidence(sweep):
|
||||||
|
"""#179 gap 2: secret/provenance sweeps must state exact method + scope.
|
||||||
|
|
||||||
|
*sweep* keys: ``command`` (the exact command, script, grep pattern, or
|
||||||
|
named sweep method), ``scope`` (what was scanned, e.g. 'full PR diff
|
||||||
|
against prgs/master'), ``clean`` (bool result). A vague summary without
|
||||||
|
the exact method is downgraded; a missing sweep fails closed.
|
||||||
|
"""
|
||||||
|
if not sweep:
|
||||||
|
return {
|
||||||
|
"verdict": "missing",
|
||||||
|
"proven": False,
|
||||||
|
"reasons": ["no secret/provenance sweep reported; fail closed"],
|
||||||
|
}
|
||||||
|
reasons = []
|
||||||
|
if not (sweep.get("command") or "").strip():
|
||||||
|
reasons.append(
|
||||||
|
"sweep method/command not stated exactly (command, script, "
|
||||||
|
"pattern, or named sweep method required)"
|
||||||
|
)
|
||||||
|
if not (sweep.get("scope") or "").strip():
|
||||||
|
reasons.append("sweep scope not stated (what diff/files were scanned)")
|
||||||
|
if not isinstance(sweep.get("clean"), bool):
|
||||||
|
reasons.append("sweep result not stated as clean/not-clean")
|
||||||
|
verdict = "exact" if not reasons else "vague"
|
||||||
|
return {
|
||||||
|
"verdict": verdict,
|
||||||
|
"proven": verdict == "exact",
|
||||||
|
"reasons": reasons,
|
||||||
|
"clean": sweep.get("clean") if isinstance(sweep.get("clean"), bool)
|
||||||
|
else None,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def assess_live_state_recheck(recheck):
|
||||||
|
"""#179 gap 3: explicit live-state recheck before review/merge mutation.
|
||||||
|
|
||||||
|
*recheck* keys: ``pr_state``, ``pinned_head_sha``, ``live_head_sha``,
|
||||||
|
``pinned_base_ref``, ``live_base_ref``, ``blocking_change_requests``.
|
||||||
|
Proven only when the PR is still open, the live head equals the pinned
|
||||||
|
head (full 40-hex), the base branch is unchanged, and blocking review
|
||||||
|
state was checked and is absent. Not performing the recheck fails
|
||||||
|
closed and blocks mutation.
|
||||||
|
"""
|
||||||
|
if not recheck:
|
||||||
|
return {
|
||||||
|
"proven": False,
|
||||||
|
"block": True,
|
||||||
|
"reasons": [
|
||||||
|
"final live-state recheck not performed before mutation; "
|
||||||
|
"fail closed"
|
||||||
|
],
|
||||||
|
}
|
||||||
|
reasons = []
|
||||||
|
if (recheck.get("pr_state") or "").strip().lower() != "open":
|
||||||
|
reasons.append(
|
||||||
|
f"PR state is '{recheck.get('pr_state')}', not open; stop"
|
||||||
|
)
|
||||||
|
|
||||||
|
pinned = (recheck.get("pinned_head_sha") or "").strip().lower()
|
||||||
|
live = (recheck.get("live_head_sha") or "").strip().lower()
|
||||||
|
if not (_FULL_SHA.match(pinned) and _FULL_SHA.match(live)):
|
||||||
|
reasons.append(
|
||||||
|
"pinned/live head SHAs missing or not full 40-hex; fail closed"
|
||||||
|
)
|
||||||
|
elif pinned != live:
|
||||||
|
reasons.append(
|
||||||
|
"live head SHA no longer equals the pinned head; re-pin and "
|
||||||
|
"re-validate before mutation"
|
||||||
|
)
|
||||||
|
|
||||||
|
base_pinned = _normalize_ref(recheck.get("pinned_base_ref"))
|
||||||
|
base_live = _normalize_ref(recheck.get("live_base_ref"))
|
||||||
|
if not base_pinned or not base_live:
|
||||||
|
reasons.append("base refs missing from live-state recheck; fail closed")
|
||||||
|
elif base_pinned != base_live:
|
||||||
|
reasons.append(
|
||||||
|
f"base branch changed from '{base_pinned}' to '{base_live}'"
|
||||||
|
)
|
||||||
|
|
||||||
|
blocking = recheck.get("blocking_change_requests")
|
||||||
|
if blocking is None:
|
||||||
|
reasons.append(
|
||||||
|
"blocking review state not checked; fail closed"
|
||||||
|
)
|
||||||
|
elif blocking:
|
||||||
|
reasons.append(
|
||||||
|
"an undismissed REQUEST_CHANGES / blocking review state remains "
|
||||||
|
"unresolved"
|
||||||
|
)
|
||||||
|
|
||||||
|
proven = not reasons
|
||||||
|
return {"proven": proven, "block": not proven, "reasons": reasons}
|
||||||
|
|
||||||
|
|
||||||
|
def assess_role_boundary(proof=None, *, task_role=None, namespaces_used=None,
|
||||||
|
justification=None):
|
||||||
"""Assess reviewer/author role separation for blind queue workflows.
|
"""Assess reviewer/author role separation for blind queue workflows.
|
||||||
|
|
||||||
Issue #175 blocks a reviewer queue task from silently becoming author
|
Issue #175 blocks a reviewer queue task from silently becoming author
|
||||||
implementation work. The workflow may use both namespaces only when that
|
implementation work. Issue #179 also requires reviewer workflows to
|
||||||
mixed use is explicit, justified, and non-mutating; author mutations after
|
report namespace use and justify any foreign namespace calls. This helper
|
||||||
a reviewer queue task require an explicit operator authorization.
|
accepts both forms:
|
||||||
|
|
||||||
*proof* keys:
|
- the #175 dict proof with mutation details, or
|
||||||
``task_role`` ('reviewer' or 'author'), ``task_kind`` (for example
|
- the #179 keyword form: ``task_role``, ``namespaces_used``,
|
||||||
'blind_pr_queue_review'), ``reviewer_namespace_used``,
|
``justification``.
|
||||||
``author_namespace_used``, ``author_mutations`` (list), ``review_mutations``
|
|
||||||
(list), ``operator_authorized_author_work``, ``mixed_namespace_justification``,
|
|
||||||
``scratch_evidence_claimed``, and ``scratch_evidence_durable``.
|
|
||||||
"""
|
"""
|
||||||
proof = proof or {}
|
if proof is None:
|
||||||
|
namespaces_reported = namespaces_used is not None
|
||||||
|
namespaces = list(namespaces_used or [])
|
||||||
|
proof = {
|
||||||
|
"task_role": task_role,
|
||||||
|
"reviewer_namespace_used": any(
|
||||||
|
"reviewer" in (namespace or "").lower()
|
||||||
|
for namespace in namespaces
|
||||||
|
),
|
||||||
|
"author_namespace_used": any(
|
||||||
|
"author" in (namespace or "").lower()
|
||||||
|
for namespace in namespaces
|
||||||
|
),
|
||||||
|
"mixed_namespace_justification": justification,
|
||||||
|
"author_mutations": [],
|
||||||
|
"review_mutations": [],
|
||||||
|
"_namespaces_used": namespaces,
|
||||||
|
"_namespaces_reported": namespaces_reported,
|
||||||
|
}
|
||||||
|
else:
|
||||||
|
proof = dict(proof or {})
|
||||||
|
|
||||||
task_role = (proof.get("task_role") or "").strip().lower()
|
task_role = (proof.get("task_role") or "").strip().lower()
|
||||||
task_kind = (proof.get("task_kind") or "").strip().lower()
|
task_kind = (proof.get("task_kind") or "").strip().lower()
|
||||||
author_mutations = list(proof.get("author_mutations") or [])
|
author_mutations = list(proof.get("author_mutations") or [])
|
||||||
@@ -364,6 +509,8 @@ def assess_role_boundary(proof):
|
|||||||
|
|
||||||
if task_role not in {"reviewer", "author"}:
|
if task_role not in {"reviewer", "author"}:
|
||||||
reasons.append("task role missing or unknown; role boundary unproven")
|
reasons.append("task role missing or unknown; role boundary unproven")
|
||||||
|
if proof.get("_namespaces_reported") is False:
|
||||||
|
reasons.append("namespaces used were not reported; fail closed")
|
||||||
|
|
||||||
if task_role == "reviewer":
|
if task_role == "reviewer":
|
||||||
if author_mutations and not authorized:
|
if author_mutations and not authorized:
|
||||||
@@ -409,18 +556,35 @@ def assess_role_boundary(proof):
|
|||||||
status = "clean"
|
status = "clean"
|
||||||
safe_next_action = "proceed"
|
safe_next_action = "proceed"
|
||||||
|
|
||||||
|
namespaces = proof.get("_namespaces_used")
|
||||||
|
if namespaces is None:
|
||||||
|
namespaces = []
|
||||||
|
if reviewer_used:
|
||||||
|
namespaces.append("gitea-reviewer")
|
||||||
|
if author_used:
|
||||||
|
namespaces.append("gitea-author")
|
||||||
|
foreign = [
|
||||||
|
namespace for namespace in namespaces
|
||||||
|
if task_role and task_role not in (namespace or "").lower()
|
||||||
|
]
|
||||||
|
|
||||||
return {
|
return {
|
||||||
"status": status,
|
"status": status,
|
||||||
"clean": status == "clean",
|
"clean": status == "clean",
|
||||||
|
"proven": status == "clean",
|
||||||
"reasons": reasons,
|
"reasons": reasons,
|
||||||
"violations": violations,
|
"violations": violations,
|
||||||
"safe_next_action": safe_next_action,
|
"safe_next_action": safe_next_action,
|
||||||
|
"foreign_namespaces": foreign,
|
||||||
|
"justified": bool(mixed_justification),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def build_final_report(checkout_proof, inventory, validation, contamination,
|
def build_final_report(checkout_proof, inventory, validation, contamination,
|
||||||
identity_eligible, merge_performed,
|
identity_eligible, merge_performed,
|
||||||
issue_status_verified, role_boundary=None):
|
issue_status_verified,
|
||||||
|
capability_evidence=None, sweep=None, live_state=None,
|
||||||
|
role_boundary=None):
|
||||||
"""Required behavior 6 + acceptance criteria: one report, distinct proofs.
|
"""Required behavior 6 + acceptance criteria: one report, distinct proofs.
|
||||||
|
|
||||||
Combines the individual proof verdicts into the final-report fields the
|
Combines the individual proof verdicts into the final-report fields the
|
||||||
@@ -431,6 +595,13 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
- 'downgraded' — one or more proofs missing/weak; do not merge.
|
- 'downgraded' — one or more proofs missing/weak; do not merge.
|
||||||
- 'blocked' — a *violation*: a merge was claimed although the proofs
|
- 'blocked' — a *violation*: a merge was claimed although the proofs
|
||||||
did not allow one.
|
did not allow one.
|
||||||
|
|
||||||
|
#179 raises the A bar: the report must also carry exact capability
|
||||||
|
evidence (``assess_capability_evidence``), an exact secret/provenance
|
||||||
|
sweep (``assess_sweep_evidence``), a pre-mutation live-state recheck
|
||||||
|
(``assess_live_state_recheck`` — also required for ``merge_allowed``),
|
||||||
|
and a clean role boundary (``assess_role_boundary``). Omitting any of
|
||||||
|
them downgrades; a merge without the live recheck is a violation.
|
||||||
"""
|
"""
|
||||||
contamination_status = contamination.get("status", "unknown")
|
contamination_status = contamination.get("status", "unknown")
|
||||||
checkout_proven = bool(checkout_proof.get("proven"))
|
checkout_proven = bool(checkout_proof.get("proven"))
|
||||||
@@ -443,6 +614,29 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
}
|
}
|
||||||
role_status = role_boundary.get("status", "warning")
|
role_status = role_boundary.get("status", "warning")
|
||||||
|
|
||||||
|
capability_evidence = capability_evidence or {
|
||||||
|
"proven": False,
|
||||||
|
"reasons": ["capability evidence not provided (#179)"],
|
||||||
|
}
|
||||||
|
sweep = sweep or {
|
||||||
|
"verdict": "missing",
|
||||||
|
"proven": False,
|
||||||
|
"reasons": ["secret/provenance sweep evidence not provided (#179)"],
|
||||||
|
}
|
||||||
|
live_state = live_state or {
|
||||||
|
"proven": False,
|
||||||
|
"block": True,
|
||||||
|
"reasons": ["pre-mutation live-state recheck not provided (#179)"],
|
||||||
|
}
|
||||||
|
role_boundary = role_boundary or {
|
||||||
|
"proven": False,
|
||||||
|
"reasons": ["role-boundary/namespace usage not reported (#179)"],
|
||||||
|
}
|
||||||
|
capability_proven = bool(capability_evidence.get("proven"))
|
||||||
|
sweep_proven = bool(sweep.get("proven"))
|
||||||
|
live_state_proven = bool(live_state.get("proven"))
|
||||||
|
role_boundary_clean = bool(role_boundary.get("proven"))
|
||||||
|
|
||||||
downgrade_reasons = []
|
downgrade_reasons = []
|
||||||
if not identity_eligible:
|
if not identity_eligible:
|
||||||
downgrade_reasons.append("identity/profile not eligible for review")
|
downgrade_reasons.append("identity/profile not eligible for review")
|
||||||
@@ -466,6 +660,23 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
downgrade_reasons.extend(role_boundary.get("reasons", []))
|
downgrade_reasons.extend(role_boundary.get("reasons", []))
|
||||||
if not issue_status_verified:
|
if not issue_status_verified:
|
||||||
downgrade_reasons.append("linked issue status not verified")
|
downgrade_reasons.append("linked issue status not verified")
|
||||||
|
if not capability_proven:
|
||||||
|
downgrade_reasons.append("exact capability evidence missing (#179)")
|
||||||
|
downgrade_reasons.extend(capability_evidence.get("reasons", []))
|
||||||
|
if not sweep_proven:
|
||||||
|
downgrade_reasons.append(
|
||||||
|
f"secret/provenance sweep evidence is "
|
||||||
|
f"{sweep.get('verdict', 'missing')} (#179)"
|
||||||
|
)
|
||||||
|
downgrade_reasons.extend(sweep.get("reasons", []))
|
||||||
|
if not live_state_proven:
|
||||||
|
downgrade_reasons.append(
|
||||||
|
"pre-mutation live-state recheck missing or failed (#179)"
|
||||||
|
)
|
||||||
|
downgrade_reasons.extend(live_state.get("reasons", []))
|
||||||
|
if not role_boundary_clean:
|
||||||
|
downgrade_reasons.append("role/namespace boundary not clean (#179)")
|
||||||
|
downgrade_reasons.extend(role_boundary.get("reasons", []))
|
||||||
|
|
||||||
merge_allowed = (
|
merge_allowed = (
|
||||||
identity_eligible
|
identity_eligible
|
||||||
@@ -474,6 +685,8 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
and role_status == "clean"
|
and role_status == "clean"
|
||||||
and validation_claimable
|
and validation_claimable
|
||||||
and validation.get("verdict") != "invalid"
|
and validation.get("verdict") != "invalid"
|
||||||
|
# #179: no merge without a proven final live-state recheck.
|
||||||
|
and live_state_proven
|
||||||
)
|
)
|
||||||
|
|
||||||
violations = []
|
violations = []
|
||||||
@@ -508,6 +721,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
"merge_allowed": merge_allowed,
|
"merge_allowed": merge_allowed,
|
||||||
"merge_performed": bool(merge_performed),
|
"merge_performed": bool(merge_performed),
|
||||||
"issue_status_verified": bool(issue_status_verified),
|
"issue_status_verified": bool(issue_status_verified),
|
||||||
|
"capability_evidence_proven": capability_proven,
|
||||||
|
"sweep_verdict": sweep.get("verdict"),
|
||||||
|
"live_state_recheck_proven": live_state_proven,
|
||||||
|
"role_boundary_clean": role_boundary_clean,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -232,19 +232,35 @@ Worktree folder = branch with `/` replaced by `-`
|
|||||||
validation result after the command has completed and its output has
|
validation result after the command has completed and its output has
|
||||||
been read (`review_proofs.assess_validation_report`).
|
been read (`review_proofs.assess_validation_report`).
|
||||||
10. **Do not merge if checks fail. Do not merge if the reviewer is the author.**
|
10. **Do not merge if checks fail. Do not merge if the reviewer is the author.**
|
||||||
11. The final report must distinguish (`review_proofs.build_final_report`):
|
11. **#179 A-bar proofs** (all fail closed when missing —
|
||||||
|
`review_proofs.assess_capability_evidence`, `assess_sweep_evidence`,
|
||||||
|
`assess_live_state_recheck`, `assess_role_boundary`):
|
||||||
|
- Capability claims must cite exact `gitea_resolve_task_capability`
|
||||||
|
output (or runtime context); a bare "capability checks passed" is
|
||||||
|
downgraded.
|
||||||
|
- The secret/provenance sweep must state the exact command/script/
|
||||||
|
pattern/named method and the scope scanned.
|
||||||
|
- Immediately before submitting a review verdict (and again before any
|
||||||
|
merge), re-read live PR state and prove: still open, live head ==
|
||||||
|
pinned head, base unchanged, no unresolved blocking review state.
|
||||||
|
- Reviewer runs stay in the reviewer namespace; any author-namespace
|
||||||
|
call requires an explicit justification in the report.
|
||||||
|
12. The final report must distinguish (`review_proofs.build_final_report`):
|
||||||
identity eligible; PR author different from reviewer; session
|
identity eligible; PR author different from reviewer; session
|
||||||
contamination absent (with evidence); role boundary clean; validation
|
contamination absent (with evidence); validation performed on the pinned
|
||||||
performed on the pinned head; merge performed; issue status verified. If
|
head; capability evidence; sweep verdict; live-state recheck; role
|
||||||
any proof is missing, stop or downgrade the result instead of merging
|
boundary; merge performed; issue status verified. If any proof is
|
||||||
confidently.
|
missing, stop or downgrade the result instead of merging confidently.
|
||||||
|
|
||||||
## G. Merge / cleanup workflow
|
## G. Merge / cleanup workflow
|
||||||
|
|
||||||
Only an eligible (non-author) reviewer merges. Before merging: always verify
|
Only an eligible (non-author) reviewer merges. Before merging: always verify
|
||||||
the authenticated identity **and** the PR author; respect runtime profile
|
the authenticated identity **and** the PR author; cite exact capability
|
||||||
gates; run independent validation (do not trust the author's reported
|
evidence for merge_pr (#179); respect runtime profile gates; run independent
|
||||||
results); and merge with a **pinned head SHA** and, where supported, the
|
validation (do not trust the author's reported results); perform the **final
|
||||||
|
live-state recheck** (#179 — PR still open, live head == pinned head, base
|
||||||
|
unchanged, no unresolved blocking review state) immediately before the merge
|
||||||
|
mutation; and merge with a **pinned head SHA** and, where supported, the
|
||||||
**expected changed-file set**, so a moved head or widened diff refuses the
|
**expected changed-file set**, so a moved head or widened diff refuses the
|
||||||
merge. After a real merge:
|
merge. After a real merge:
|
||||||
|
|
||||||
|
|||||||
@@ -20,10 +20,21 @@ Steps:
|
|||||||
*If the current identity does not match the required role (or is the PR author), STOP. Relaunch/switch to the correct profile first.*
|
*If the current identity does not match the required role (or is the PR author), STOP. Relaunch/switch to the correct profile first.*
|
||||||
2. Verify authenticated identity + active profile.
|
2. Verify authenticated identity + active profile.
|
||||||
3. Confirm PR #<pr>: author (not you), state open, mergeable, review approved. Check if PR body uses `Closes #N` or `Fixes #N`; if it uses `Implements #N` or `Refs #N`, manual closing will be needed in step 29.
|
3. Confirm PR #<pr>: author (not you), state open, mergeable, review approved. Check if PR body uses `Closes #N` or `Fixes #N`; if it uses `Implements #N` or `Refs #N`, manual closing will be needed in step 29.
|
||||||
4. If any gate fails → STOP and report.
|
4. Capability evidence (#179): cite the exact gitea_resolve_task_capability
|
||||||
4. Merge with explicit confirmation (e.g. confirmation="MERGE PR <pr>"),
|
output (or runtime context) proving merge_pr is allowed — a bare
|
||||||
optionally pinning the reviewed head SHA / changed-file set.
|
"capability checks passed" claim is downgraded.
|
||||||
5. Confirm remote master now contains the merge commit (or the expected changes if squash merged).
|
5. Final live-state recheck (#179), immediately before the merge mutation —
|
||||||
|
re-read the live PR and prove:
|
||||||
|
- PR still open
|
||||||
|
- live head SHA still equals the pinned/reviewed head SHA
|
||||||
|
- base branch unchanged
|
||||||
|
- no undismissed REQUEST_CHANGES / blocking review state remains
|
||||||
|
If any recheck fails → STOP, re-pin, re-validate.
|
||||||
|
6. If any gate fails → STOP and report.
|
||||||
|
7. Merge with explicit confirmation (e.g. confirmation="MERGE PR <pr>"),
|
||||||
|
pinning the reviewed head SHA (expected_head_sha) and, where supported,
|
||||||
|
the changed-file set.
|
||||||
|
8. Confirm remote master now contains the merge commit (or the expected changes if squash merged).
|
||||||
*Note: Gitea PR "closed" state is NOT equivalent to "merged". Do not assume a closed PR succeeded without verifying the actual landed changes.*
|
*Note: Gitea PR "closed" state is NOT equivalent to "merged". Do not assume a closed PR succeeded without verifying the actual landed changes.*
|
||||||
|
|
||||||
Then run the cleanup template (worktree-cleanup.md):
|
Then run the cleanup template (worktree-cleanup.md):
|
||||||
|
|||||||
@@ -36,6 +36,11 @@ Steps:
|
|||||||
- Target task role: reviewer identity (must NOT be the PR author)
|
- Target task role: reviewer identity (must NOT be the PR author)
|
||||||
*If the current identity does not match the required role (or is the PR author), STOP. Relaunch/switch to the correct profile first.*
|
*If the current identity does not match the required role (or is the PR author), STOP. Relaunch/switch to the correct profile first.*
|
||||||
2. Verify your authenticated identity (whoami) and the active profile.
|
2. Verify your authenticated identity (whoami) and the active profile.
|
||||||
|
Capability evidence (#179): cite the exact gitea_resolve_task_capability
|
||||||
|
output (or runtime context) for review_pr (and merge_pr if merging later);
|
||||||
|
a bare "capability checks passed" claim is downgraded. Stay in the
|
||||||
|
reviewer namespace: any author-namespace call must be justified in the
|
||||||
|
report (#179).
|
||||||
3. Fetch the PR facts: PR author, head SHA, state (must be open), base branch.
|
3. Fetch the PR facts: PR author, head SHA, state (must be open), base branch.
|
||||||
Pin the head SHA in your notes; every later step validates THAT SHA.
|
Pin the head SHA in your notes; every later step validates THAT SHA.
|
||||||
4. If authenticated user == PR author → STOP (no self-review).
|
4. If authenticated user == PR author → STOP (no self-review).
|
||||||
@@ -58,12 +63,23 @@ Steps:
|
|||||||
If HEAD does not match the pinned head → STOP before review/merge.
|
If HEAD does not match the pinned head → STOP before review/merge.
|
||||||
7. Confirm the worktree is clean. Inspect the FULL diff; confirm scope matches
|
7. Confirm the worktree is clean. Inspect the FULL diff; confirm scope matches
|
||||||
issue #<n>; flag any unrelated files, secrets, or formatting churn. Check that the PR body correctly uses Gitea-closing keywords (`Closes #N` or `Fixes #N`) instead of non-closing ones (`Implements #N`, `Refs #N`).
|
issue #<n>; flag any unrelated files, secrets, or formatting churn. Check that the PR body correctly uses Gitea-closing keywords (`Closes #N` or `Fixes #N`) instead of non-closing ones (`Implements #N`, `Refs #N`).
|
||||||
|
Secret/provenance sweep must be exact (#179): state the exact command,
|
||||||
|
script, grep pattern, or named sweep method AND the scope scanned (e.g.
|
||||||
|
`git diff prgs/master...HEAD | grep -inE '<pattern>'`); "checked the diff
|
||||||
|
for secrets" alone is downgraded.
|
||||||
8. Run the test suite; report the exact command and exact results — pass/fail
|
8. Run the test suite; report the exact command and exact results — pass/fail
|
||||||
plus passed/skipped/failed counts, any ignored paths and why they are safe
|
plus passed/skipped/failed counts, any ignored paths and why they are safe
|
||||||
to ignore, and whether the command differs from the repository's canonical
|
to ignore, and whether the command differs from the repository's canonical
|
||||||
validation command. Only claim a result after the output has been read.
|
validation command. Only claim a result after the output has been read.
|
||||||
9. Post the review verdict: approve only if scope is clean and checks pass;
|
9. Final live-state recheck (#179), immediately before submitting the review
|
||||||
otherwise request changes with specifics. Never merge from this review step.
|
verdict — re-read the live PR and prove:
|
||||||
|
- PR still open
|
||||||
|
- live head SHA still equals the pinned head SHA from step 3
|
||||||
|
- base branch unchanged
|
||||||
|
- no undismissed REQUEST_CHANGES / blocking review state left unaccounted
|
||||||
|
If anything moved → STOP, re-pin, re-validate before any verdict.
|
||||||
|
10. Post the review verdict: approve only if scope is clean and checks pass;
|
||||||
|
otherwise request changes with specifics. Never merge from this review step.
|
||||||
Include a "Review Metadata" block (attribution only — docs/llm-agent-sha.md):
|
Include a "Review Metadata" block (attribution only — docs/llm-agent-sha.md):
|
||||||
|
|
||||||
Review Metadata:
|
Review Metadata:
|
||||||
|
|||||||
@@ -21,10 +21,13 @@ import unittest
|
|||||||
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
from review_proofs import ( # noqa: E402
|
from review_proofs import ( # noqa: E402
|
||||||
|
assess_capability_evidence,
|
||||||
assess_controller_handoff,
|
assess_controller_handoff,
|
||||||
assess_inventory_completeness,
|
assess_inventory_completeness,
|
||||||
|
assess_live_state_recheck,
|
||||||
assess_role_boundary,
|
assess_role_boundary,
|
||||||
assess_self_review_contamination,
|
assess_self_review_contamination,
|
||||||
|
assess_sweep_evidence,
|
||||||
assess_validation_report,
|
assess_validation_report,
|
||||||
build_final_report,
|
build_final_report,
|
||||||
pr_inventory_trust_gate,
|
pr_inventory_trust_gate,
|
||||||
@@ -116,6 +119,63 @@ def _good_role_boundary():
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _good_capability_evidence():
|
||||||
|
return assess_capability_evidence([
|
||||||
|
{
|
||||||
|
"task": "review_pr",
|
||||||
|
"allowed": True,
|
||||||
|
"evidence_source": (
|
||||||
|
"gitea_resolve_task_capability(review_pr) output: "
|
||||||
|
"allowed_in_current_session=true, profile prgs-reviewer"
|
||||||
|
),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"task": "merge_pr",
|
||||||
|
"allowed": True,
|
||||||
|
"evidence_source": (
|
||||||
|
"gitea_resolve_task_capability(merge_pr) output: "
|
||||||
|
"allowed_in_current_session=true, profile prgs-reviewer"
|
||||||
|
),
|
||||||
|
},
|
||||||
|
])
|
||||||
|
|
||||||
|
|
||||||
|
def _good_sweep(**overrides):
|
||||||
|
sweep = {
|
||||||
|
"command": (
|
||||||
|
"git diff prgs/master...HEAD | grep -inE "
|
||||||
|
"'password|token|secret|api[_-]?key|authorization|bearer|https?://'"
|
||||||
|
),
|
||||||
|
"scope": "full PR diff against prgs/master",
|
||||||
|
"clean": True,
|
||||||
|
}
|
||||||
|
sweep.update(overrides)
|
||||||
|
return assess_sweep_evidence(sweep)
|
||||||
|
|
||||||
|
|
||||||
|
def _good_live_state(**overrides):
|
||||||
|
recheck = {
|
||||||
|
"pr_state": "open",
|
||||||
|
"pinned_head_sha": PINNED,
|
||||||
|
"live_head_sha": PINNED,
|
||||||
|
"pinned_base_ref": "master",
|
||||||
|
"live_base_ref": "master",
|
||||||
|
"blocking_change_requests": False,
|
||||||
|
}
|
||||||
|
recheck.update(overrides)
|
||||||
|
return assess_live_state_recheck(recheck)
|
||||||
|
|
||||||
|
|
||||||
|
def _good_role_boundary_179(**overrides):
|
||||||
|
kwargs = {
|
||||||
|
"task_role": "reviewer",
|
||||||
|
"namespaces_used": ["gitea-reviewer"],
|
||||||
|
"justification": None,
|
||||||
|
}
|
||||||
|
kwargs.update(overrides)
|
||||||
|
return assess_role_boundary(**kwargs)
|
||||||
|
|
||||||
|
|
||||||
class TestCheckoutProof(unittest.TestCase):
|
class TestCheckoutProof(unittest.TestCase):
|
||||||
"""Required behavior 1 + 2: prove HEAD == pinned PR head or stop."""
|
"""Required behavior 1 + 2: prove HEAD == pinned PR head or stop."""
|
||||||
|
|
||||||
@@ -466,6 +526,9 @@ class TestFinalReport(unittest.TestCase):
|
|||||||
"identity_eligible": True,
|
"identity_eligible": True,
|
||||||
"merge_performed": False,
|
"merge_performed": False,
|
||||||
"issue_status_verified": True,
|
"issue_status_verified": True,
|
||||||
|
"capability_evidence": _good_capability_evidence(),
|
||||||
|
"sweep": _good_sweep(),
|
||||||
|
"live_state": _good_live_state(),
|
||||||
"role_boundary": _good_role_boundary(),
|
"role_boundary": _good_role_boundary(),
|
||||||
}
|
}
|
||||||
kwargs.update(overrides)
|
kwargs.update(overrides)
|
||||||
@@ -879,5 +942,209 @@ class TestPRInventoryTrustGate(unittest.TestCase):
|
|||||||
self.assertTrue(res["corroborated"])
|
self.assertTrue(res["corroborated"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestCapabilityEvidence(unittest.TestCase):
|
||||||
|
"""#179 gap 1: capability claims need exact evidence."""
|
||||||
|
|
||||||
|
def test_evidence_backed_claims_are_proven(self):
|
||||||
|
result = _good_capability_evidence()
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
self.assertEqual(result["reasons"], [])
|
||||||
|
|
||||||
|
def test_claim_without_evidence_source_is_not_proven(self):
|
||||||
|
result = assess_capability_evidence([
|
||||||
|
{"task": "review_pr", "allowed": True, "evidence_source": ""},
|
||||||
|
])
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(any("evidence" in r.lower() for r in result["reasons"]))
|
||||||
|
|
||||||
|
def test_no_claims_at_all_fails_closed(self):
|
||||||
|
result = assess_capability_evidence([])
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_disallowed_task_is_not_proven(self):
|
||||||
|
result = assess_capability_evidence([
|
||||||
|
{
|
||||||
|
"task": "merge_pr",
|
||||||
|
"allowed": False,
|
||||||
|
"evidence_source": "gitea_resolve_task_capability output",
|
||||||
|
},
|
||||||
|
])
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestSweepEvidence(unittest.TestCase):
|
||||||
|
"""#179 gap 2: secret/provenance sweep must be exact."""
|
||||||
|
|
||||||
|
def test_exact_sweep_is_proven(self):
|
||||||
|
result = _good_sweep()
|
||||||
|
self.assertEqual(result["verdict"], "exact")
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_vague_sweep_without_command_is_downgraded(self):
|
||||||
|
result = _good_sweep(command="")
|
||||||
|
self.assertEqual(result["verdict"], "vague")
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_sweep_without_scope_is_downgraded(self):
|
||||||
|
result = _good_sweep(scope="")
|
||||||
|
self.assertEqual(result["verdict"], "vague")
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_missing_sweep_fails_closed(self):
|
||||||
|
result = assess_sweep_evidence(None)
|
||||||
|
self.assertEqual(result["verdict"], "missing")
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_unstated_result_is_downgraded(self):
|
||||||
|
result = _good_sweep(clean=None)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestLiveStateRecheck(unittest.TestCase):
|
||||||
|
"""#179 gap 3: explicit pre-mutation live-state recheck."""
|
||||||
|
|
||||||
|
def test_clean_recheck_is_proven(self):
|
||||||
|
result = _good_live_state()
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
self.assertFalse(result["block"])
|
||||||
|
|
||||||
|
def test_missing_recheck_fails_closed(self):
|
||||||
|
result = assess_live_state_recheck(None)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["block"])
|
||||||
|
|
||||||
|
def test_closed_pr_blocks(self):
|
||||||
|
result = _good_live_state(pr_state="closed")
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["block"])
|
||||||
|
|
||||||
|
def test_moved_head_blocks(self):
|
||||||
|
result = _good_live_state(live_head_sha=OTHER)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(any("head" in r.lower() for r in result["reasons"]))
|
||||||
|
|
||||||
|
def test_changed_base_blocks(self):
|
||||||
|
result = _good_live_state(live_base_ref="develop")
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_unresolved_blocking_reviews_block(self):
|
||||||
|
result = _good_live_state(blocking_change_requests=True)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_unchecked_blocking_state_fails_closed(self):
|
||||||
|
result = _good_live_state(blocking_change_requests=None)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestRoleBoundary179(unittest.TestCase):
|
||||||
|
"""#179 gap 4: reviewer flows avoid unjustified author-namespace use."""
|
||||||
|
|
||||||
|
def test_native_namespace_only_is_clean(self):
|
||||||
|
result = _good_role_boundary_179()
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_foreign_namespace_without_justification_is_downgraded(self):
|
||||||
|
result = _good_role_boundary_179(
|
||||||
|
namespaces_used=["gitea-reviewer", "gitea-author"]
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(any("justif" in r.lower() for r in result["reasons"]))
|
||||||
|
|
||||||
|
def test_foreign_namespace_with_justification_is_clean(self):
|
||||||
|
result = _good_role_boundary_179(
|
||||||
|
namespaces_used=["gitea-reviewer", "gitea-author"],
|
||||||
|
justification=(
|
||||||
|
"author namespace read-only whoami used to evidence "
|
||||||
|
"self-review contamination status"
|
||||||
|
),
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_unreported_namespaces_fail_closed(self):
|
||||||
|
result = _good_role_boundary_179(namespaces_used=None)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestFinalReport179Bar(unittest.TestCase):
|
||||||
|
"""#179 acceptance adds capability, sweep, live-state, and role proofs."""
|
||||||
|
|
||||||
|
def _report(self, **overrides):
|
||||||
|
kwargs = {
|
||||||
|
"checkout_proof": _good_checkout(),
|
||||||
|
"inventory": _good_inventory(),
|
||||||
|
"validation": _good_validation(),
|
||||||
|
"contamination": _good_contamination(),
|
||||||
|
"identity_eligible": True,
|
||||||
|
"merge_performed": False,
|
||||||
|
"issue_status_verified": True,
|
||||||
|
"capability_evidence": _good_capability_evidence(),
|
||||||
|
"sweep": _good_sweep(),
|
||||||
|
"live_state": _good_live_state(),
|
||||||
|
"role_boundary": _good_role_boundary(),
|
||||||
|
}
|
||||||
|
kwargs.update(overrides)
|
||||||
|
return build_final_report(**kwargs)
|
||||||
|
|
||||||
|
def test_all_179_proofs_present_is_grade_a(self):
|
||||||
|
report = self._report()
|
||||||
|
self.assertEqual(report["grade"], "A")
|
||||||
|
self.assertTrue(report["capability_evidence_proven"])
|
||||||
|
self.assertEqual(report["sweep_verdict"], "exact")
|
||||||
|
self.assertTrue(report["live_state_recheck_proven"])
|
||||||
|
self.assertTrue(report["role_boundary_clean"])
|
||||||
|
|
||||||
|
def test_missing_capability_evidence_downgrades(self):
|
||||||
|
report = self._report(capability_evidence=None)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertFalse(report["capability_evidence_proven"])
|
||||||
|
|
||||||
|
def test_unevidenced_capability_claim_downgrades(self):
|
||||||
|
report = self._report(
|
||||||
|
capability_evidence=assess_capability_evidence([
|
||||||
|
{"task": "review_pr", "allowed": True, "evidence_source": ""},
|
||||||
|
])
|
||||||
|
)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
|
||||||
|
def test_vague_sweep_downgrades(self):
|
||||||
|
report = self._report(sweep=_good_sweep(command=""))
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertEqual(report["sweep_verdict"], "vague")
|
||||||
|
|
||||||
|
def test_missing_sweep_downgrades(self):
|
||||||
|
report = self._report(sweep=None)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
|
||||||
|
def test_missing_live_state_recheck_downgrades_and_blocks_merge(self):
|
||||||
|
report = self._report(live_state=None)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertFalse(report["merge_allowed"])
|
||||||
|
self.assertFalse(report["live_state_recheck_proven"])
|
||||||
|
|
||||||
|
def test_stale_live_state_blocks_merge(self):
|
||||||
|
report = self._report(live_state=_good_live_state(live_head_sha=OTHER))
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertFalse(report["merge_allowed"])
|
||||||
|
|
||||||
|
def test_merge_claim_without_live_recheck_is_a_violation(self):
|
||||||
|
report = self._report(live_state=None, merge_performed=True)
|
||||||
|
self.assertEqual(report["grade"], "blocked")
|
||||||
|
self.assertTrue(report["violations"])
|
||||||
|
|
||||||
|
def test_unjustified_author_namespace_downgrades(self):
|
||||||
|
report = self._report(
|
||||||
|
role_boundary=_good_role_boundary_179(
|
||||||
|
namespaces_used=["gitea-reviewer", "gitea-author"]
|
||||||
|
)
|
||||||
|
)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertFalse(report["role_boundary_clean"])
|
||||||
|
|
||||||
|
def test_positive_baseline_from_173_still_holds(self):
|
||||||
|
report = self._report()
|
||||||
|
self.assertTrue(report["inventory_complete"])
|
||||||
|
self.assertTrue(report["validated_on_pinned_head"])
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user