atlas-iac/testing/tests/test_hermes_cli_review_contract.py

314 lines
11 KiB
Python
Raw Permalink Normal View History

hermes: close the review-role gaps found reviewing PR 22 Repairs the blockers from the independent review of the previous head. The role-aware verdict contract was correct but reachable only through one call site and only on cards written with real newlines, so most real review cards never used it. * The role-blind call is no longer a weaker classifier that can reject before the role-aware judge runs. Without a card there is no defensible role-dependent judgement, so `unfinished_result_reason()` applies only the card-independent checks. That closes the short-circuit at every call site, including the one PR15 moves to `cli_lane_execution`, and it makes a journalled terminal record accepted under one version of these semantics re-validate under any other instead of being quarantined into a re-dispatch of an already-accepted task. * The lane resolves the role from the card and passes it, and records it in the Kanban metadata. The verdict contract binds only where the lane can buy another turn: in single-shot mode a rejection discards the worker's real result, so review cards keep the relaxation without gaining any rejection single-shot mode did not already have. * Card scope expands the literal \n escapes the board stores in one-line bodies, so explicit `Hermes-Task-Role` / `Hermes-Expected-Output` directives are honoured on the 6 of 78 live cards that carry no real newline, and the read-only, verdict and mutation heuristics stop being cut apart by them. * Card scope now ends at the first non-card H2 and at the runner's controller evidence, which is emitted under its own heading. Goal-controller rejection history can no longer sit inside the card, and an upstream heading rename fails closed instead of admitting history into role resolution. * The inference recognises the SHIP/BLOCK-shaped deliverables real cards actually use: 21 of 78 live cards resolve to review, up from 10, with no implementation card misclassified. Cards asking for a findings list rather than a verdict deliberately stay on the model judge, since the verdict is the review contract's only gate. * A declared review role no longer outranks mutation evidence: a report that changed files falls back to the implementation regime. * Judge reasons go through the agent runtime's canonical redactor, extended for the two shapes it deliberately passes through and this lane handles - `scheme://user:secret@host` and a credential named in prose - while a 40-hex commit SHA survives as evidence. Regressions cover the recovered t_dbdcd739 incident, a verbatim snapshot of every live card on three boards with a hand-labelled expected role, the upgrade and single-shot properties against the previous gate, the upstream context-heading contract, and the end-to-end `execute_claim` shape that used to burn every goal turn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-17 17:22:43 +00:00
"""Upgrade, single-shot and redaction contracts for the Hermes goal gate."""
from __future__ import annotations
import importlib.util
import json
import re
import sys
from pathlib import Path
import pytest
ROOT = Path(__file__).parents[2]
SPEC = importlib.util.spec_from_file_location(
"cli_lane_review_contract_test",
ROOT / "services/hermes/scripts/cli_lane_goal.py",
)
assert SPEC and SPEC.loader
goal = importlib.util.module_from_spec(SPEC)
sys.modules[SPEC.name] = goal
SPEC.loader.exec_module(goal)
# A synthetic 40-character lowercase hex string. It has the shape of a Gitea
# personal access token *and* of a git SHA-1; no real credential is read here.
SYNTHETIC_HEX = "0123456789abcdef" * 2 + "01234567"
REAL_SHA = "b080b5f622ae0998213f3287762aea30dc931a73"
REPORTS = {
"block_review": {
"status": "completed",
"summary": (
"Independent read-only review of PR #18. Verdict: BLOCK. Deployment "
"verification is pending on two of three nodes, which is exactly why "
"the reviewed change is unfit to ship."
),
"changed_files": [],
"tests_run": ["pytest testing/tests: 337 passed"],
"artifacts": [],
"findings": ["P0 - coordinator.py:248 compares int to str run ids."],
"blockers": [],
},
"bare_token_review": {
"status": "completed",
"summary": (
"Independent read-only review of PR #18 concludes BLOCK because the "
"coordinator drops a lease on its first tick and deployment "
"verification is pending on two of three nodes."
),
"changed_files": [],
"tests_run": [],
"artifacts": [],
"findings": ["P0 - the lease is dropped before the first heartbeat."],
"blockers": [],
},
"terse_ship_audit": {
"status": "completed",
"summary": "Decision: ship. Audit done.",
"changed_files": [],
"tests_run": ["ruff check: clean"],
"artifacts": [],
"findings": [],
"blockers": [],
},
"block_audit_without_findings": {
"status": "completed",
"summary": (
"The generated-strategy audit harness was inspected end to end and "
"the verdict is BLOCK on the release train for now."
),
"changed_files": [],
"tests_run": ["pytest -q: 61 passed"],
"artifacts": [],
"findings": [],
"blockers": [],
},
"finished_implementation": {
"status": "completed",
"summary": "Every acceptance criterion passed and the branch was pushed.",
"changed_files": ["services/hermes/scripts/cli_lane_goal.py"],
"tests_run": ["pytest -q: 12 passed"],
"artifacts": [],
"findings": [],
"blockers": [],
},
"unfinished_implementation": {
"status": "completed",
"summary": "The broad rerun remains active before the final push.",
"changed_files": ["services/hermes/scripts/cli_lane_goal.py"],
"tests_run": [],
"artifacts": [],
"findings": [],
"blockers": [],
},
"incomplete_turn": {
"status": "incomplete",
"summary": "Only two of six boundaries were read.",
"changed_files": [],
"tests_run": [],
"artifacts": [],
"findings": [],
"blockers": [],
},
"completed_with_blockers": {
"status": "completed",
"summary": "The review finished but the head could not be resolved.",
"changed_files": [],
"tests_run": [],
"artifacts": [],
"findings": [],
"blockers": ["the Gitea API was unreachable"],
},
}
def _base_unfinished_result_reason(result):
"""The completion gate exactly as main d8f2d818 shipped it.
Kept as a local oracle so the upgrade properties below are checked against
the previous semantics rather than against the implementation under test.
"""
status = str(result.get("status") or "")
summary = str(result.get("summary") or "").strip()
blockers = result.get("blockers")
if status == "incomplete":
return summary or "worker explicitly reported incomplete work"
if status != "completed":
return None
if isinstance(blockers, list) and any(str(item).strip() for item in blockers):
return "worker reported blockers while claiming completion"
tests = result.get("tests_run")
evidence = [summary]
if isinstance(tests, list):
evidence.extend(str(item) for item in tests)
match = goal.UNFINISHED_EVIDENCE.search("\n".join(evidence))
if match:
return f"completion evidence says work is unfinished: {match.group(0).strip()}"
return None
def _accepted(result, *, role):
return goal.unfinished_result_reason(result, role=role) is None
@pytest.mark.parametrize("name", sorted(REPORTS))
def test_a_role_blind_replay_accepts_whatever_the_lane_accepted(name):
"""PR15 re-runs this gate on journalled terminal records without a card.
A record accepted by any version of the lane must stay valid, or an upgrade
quarantines it and re-dispatches an already-accepted task.
"""
result = REPORTS[name]
accepted_somewhere = _base_unfinished_result_reason(result) is None or any(
_accepted(result, role=role)
for role in (goal.REVIEW_ROLE, goal.IMPLEMENTATION_ROLE)
)
if accepted_somewhere:
assert goal.unfinished_result_reason(result) is None
def test_role_blind_replay_keeps_the_unfinished_work_integrity_check():
"""PR15's ``_terminal_record_valid`` relies on this without a card."""
assert goal.unfinished_result_reason(REPORTS["completed_with_blockers"])
assert goal.unfinished_result_reason(REPORTS["incomplete_turn"])
assert goal.unfinished_result_reason(REPORTS["unfinished_implementation"])
assert goal.unfinished_result_reason(
{**REPORTS["terse_ship_audit"], "summary": "tests are still running"}
)
def test_role_blind_replay_exempts_only_a_complete_review_deliverable():
"""The exemption is exactly the shape the role-aware lane calls a review."""
complete = REPORTS["bare_token_review"]
mutated = {**complete, "changed_files": ["src/a.py"]}
verdictless = {**complete, "summary": complete["summary"].replace("BLOCK", "no")}
assert _base_unfinished_result_reason(complete)
assert _accepted(complete, role=goal.REVIEW_ROLE)
assert goal.unfinished_result_reason(complete) is None
assert goal.unfinished_result_reason(mutated)
assert goal.unfinished_result_reason(verdictless)
@pytest.mark.parametrize("name", sorted(REPORTS))
def test_implementation_completion_is_byte_for_byte_the_previous_gate(name):
result = REPORTS[name]
assert goal.unfinished_result_reason(
result, role=goal.IMPLEMENTATION_ROLE
) == (
goal.sanitize_reason(_base_unfinished_result_reason(result))
if _base_unfinished_result_reason(result)
else None
)
def test_the_two_single_shot_outcomes_that_change_are_the_documented_ones():
"""Single-shot runs this gate as its only completion check.
The verdict contract binds there too, so a review's own deliverable decides
the outcome rather than prose about the artifact it reviewed.
"""
now_completes = REPORTS["bare_token_review"]
now_blocks = (REPORTS["terse_ship_audit"], REPORTS["block_audit_without_findings"])
assert _base_unfinished_result_reason(now_completes)
assert _accepted(now_completes, role=goal.REVIEW_ROLE)
for result in now_blocks:
assert _base_unfinished_result_reason(result) is None
problem = goal.unfinished_result_reason(result, role=goal.REVIEW_ROLE)
assert problem and problem.startswith(goal.READ_ONLY_GUARD)
@pytest.mark.parametrize(
("text", "secret"),
[
(
f"cloned https://hermes:{SYNTHETIC_HEX}@scm.bstein.dev/titan/x.git",
hermes: close the review-role gaps found reviewing PR 22 Repairs the blockers from the independent review of the previous head. The role-aware verdict contract was correct but reachable only through one call site and only on cards written with real newlines, so most real review cards never used it. * The role-blind call is no longer a weaker classifier that can reject before the role-aware judge runs. Without a card there is no defensible role-dependent judgement, so `unfinished_result_reason()` applies only the card-independent checks. That closes the short-circuit at every call site, including the one PR15 moves to `cli_lane_execution`, and it makes a journalled terminal record accepted under one version of these semantics re-validate under any other instead of being quarantined into a re-dispatch of an already-accepted task. * The lane resolves the role from the card and passes it, and records it in the Kanban metadata. The verdict contract binds only where the lane can buy another turn: in single-shot mode a rejection discards the worker's real result, so review cards keep the relaxation without gaining any rejection single-shot mode did not already have. * Card scope expands the literal \n escapes the board stores in one-line bodies, so explicit `Hermes-Task-Role` / `Hermes-Expected-Output` directives are honoured on the 6 of 78 live cards that carry no real newline, and the read-only, verdict and mutation heuristics stop being cut apart by them. * Card scope now ends at the first non-card H2 and at the runner's controller evidence, which is emitted under its own heading. Goal-controller rejection history can no longer sit inside the card, and an upstream heading rename fails closed instead of admitting history into role resolution. * The inference recognises the SHIP/BLOCK-shaped deliverables real cards actually use: 21 of 78 live cards resolve to review, up from 10, with no implementation card misclassified. Cards asking for a findings list rather than a verdict deliberately stay on the model judge, since the verdict is the review contract's only gate. * A declared review role no longer outranks mutation evidence: a report that changed files falls back to the implementation regime. * Judge reasons go through the agent runtime's canonical redactor, extended for the two shapes it deliberately passes through and this lane handles - `scheme://user:secret@host` and a credential named in prose - while a 40-hex commit SHA survives as evidence. Regressions cover the recovered t_dbdcd739 incident, a verbatim snapshot of every live card on three boards with a hand-labelled expected role, the upgrade and single-shot properties against the previous gate, the upstream context-heading contract, and the end-to-end `execute_claim` shape that used to burn every goal turn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-17 17:22:43 +00:00
SYNTHETIC_HEX,
),
(f"exported GITEA_TOKEN={SYNTHETIC_HEX} into the lane", SYNTHETIC_HEX),
(f"the pat {SYNTHETIC_HEX} was reused by the askpass helper", SYNTHETIC_HEX),
(f"Authorization: token {SYNTHETIC_HEX}", SYNTHETIC_HEX),
("token ghp_" + "A" * 24, "ghp_" + "A" * 24),
("the secret is xoxb-1234567890-abcdefghij", "xoxb-1234567890-abcdefghij"),
(f"reviewed exact head {REAL_SHA} against base main", None),
],
)
def test_judge_reasons_redact_credentials_but_keep_commit_evidence(text, secret):
"""A 40-hex PAT and a 40-hex commit SHA are the same string in isolation.
Redaction is anchored on the surrounding syntax so credentials disappear
and the exact head a review pins its findings to survives as evidence.
"""
reason = goal.sanitize_reason(text)
if secret is None:
assert reason == text
else:
assert secret not in reason
assert "[redacted]" in reason or "***" in reason
def test_redaction_falls_back_when_the_agent_runtime_is_absent(monkeypatch):
monkeypatch.setattr(goal, "_canonical_redact", None)
reason = goal.sanitize_reason(
f"token ghp_{'A' * 24} and https://ci:{SYNTHETIC_HEX}@scm.bstein.dev/x.git"
)
assert "ghp_" not in reason
assert SYNTHETIC_HEX not in reason
assert reason.count("[redacted]") == 2
def test_redaction_delegates_to_the_canonical_helper_when_it_is_installed(monkeypatch):
seen = []
def spy(text, **kwargs):
seen.append(kwargs)
return text.replace("OPAQUE", "***")
monkeypatch.setattr(goal, "_canonical_redact", spy)
assert goal.sanitize_reason("value OPAQUE here") == "value *** here"
assert seen == [{"force": True}]
def test_the_canonical_redactor_is_the_one_the_lane_runtime_ships():
canonical = pytest.importorskip("agent.redact")
assert goal._canonical_redact is canonical.redact_sensitive_text
def test_the_upstream_worker_context_headings_are_still_the_ones_we_cut_on():
"""Pin ``hermes_cli.kanban_db.build_worker_context``'s H2 contract.
``card_scope`` allow-lists CARD_SECTIONS and drops every other H2. A rename
upstream fails closed rather than leaking history into role resolution, but
it would also truncate real cards, so the coupling is asserted explicitly.
"""
kanban_db = pytest.importorskip("hermes_cli.kanban_db")
source = Path(kanban_db.__file__).read_text(encoding="utf-8")
emitted = {
match.group(1).replace("@", "").strip()
for match in re.finditer(r'lines\.append\(f?"## ([^"{]+)', source)
}
assert emitted == {*goal.CARD_SECTIONS, *goal.HISTORY_SECTIONS}
def test_the_controller_evidence_heading_is_not_a_card_section():
assert goal.CONTROLLER_EVIDENCE_HEADING.startswith("## ")
assert goal.CONTROLLER_EVIDENCE_HEADING[3:] not in goal.CARD_SECTIONS
assert goal.card_scope(
f"## Body\nreview only\n\n{goal.CONTROLLER_EVIDENCE_HEADING}\npush it"
) == "## Body\nreview only\n\n"
def test_every_report_shape_round_trips_through_the_result_schema():
for name, result in REPORTS.items():
assert set(result) == {
"status",
"summary",
"changed_files",
"tests_run",
"artifacts",
"findings",
"blockers",
}, name
assert result["status"] in goal.RESULT_STATUSES
assert json.loads(json.dumps(result)) == result