atlas-iac/testing/tests/test_hermes_cli_review_goal.py
Hermes Agent 360d8aad8f 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:27:11 +00:00

479 lines
16 KiB
Python

"""Task-role and verdict-contract tests for the Hermes goal judge."""
from __future__ import annotations
import importlib.util
import json
import sys
from pathlib import Path
import pytest
SCRIPT = Path(__file__).parents[2] / "services/hermes/scripts/cli_lane_goal.py"
SPEC = importlib.util.spec_from_file_location("cli_lane_review_goal_test", SCRIPT)
assert SPEC and SPEC.loader
goal = importlib.util.module_from_spec(SPEC)
sys.modules[SPEC.name] = goal
SPEC.loader.exec_module(goal)
REVIEW_CARD = """# Kanban task t_review: Independent read-only review of the worker pool
Assignee: cli-claude-xhigh
## Body
Perform a fresh independent read-only release review of the pool implementation.
Do not edit files, comment, commit, push, merge, publish, or deploy.
Return strict SHIP or BLOCK with exact file/line and reproducible inputs.
"""
IMPLEMENTATION_CARD = """# Kanban task t_impl: Repair the pool blockers
Assignee: cli-auto
## Body
Repair every reported blocker, run the focused suites, and open a draft PR.
Return exact pushed SHA and evidence.
"""
POLLUTED_CARD = REVIEW_CARD + """
## Recent work by @cli-claude-xhigh
- t_prior: Implemented and pushed the pool. PR #18 is open at the exact pushed SHA.
## Comment thread
- shepherd: commit and push the repairs before returning.
"""
def _review(**overrides):
value = {
"status": "completed",
"summary": (
"Independent read-only review of PR #18 at head 2000252. Verdict: BLOCK. "
"Five reproducible P0 defects fire on the coordinator's first tick and the "
"review worktree was left pristine."
),
"changed_files": [],
"tests_run": ["pytest testing/tests/test_pool.py: 82 passed"],
"artifacts": ["/opt/data/workspace/evidence/t_review/RUN.md"],
"findings": ["P0-1 BLOCKER - coordinator.py:248 compares int to str run ids."],
"blockers": [],
}
value.update(overrides)
return value
def _implementation(**overrides):
value = {
"status": "completed",
"summary": "All acceptance criteria passed and the branch was pushed.",
"changed_files": ["src/example.py"],
"tests_run": ["pytest -q: 12 passed"],
"artifacts": [],
"findings": [],
"blockers": [],
}
value.update(overrides)
return value
def _no_judge(*_args, **_kwargs):
raise AssertionError("the model judge must not be consulted for a review card")
def test_completed_block_review_finalizes_without_consulting_the_model():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(),
open_request=_no_judge,
)
assert accepted is True
assert "BLOCK verdict with 1 finding(s)" in reason
assert "resolved by inferred" in reason
def test_completed_ship_review_finalizes_on_its_own_evidence():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(
summary=(
"Independent read-only review of PR #18 at head 2000252. Verdict: SHIP. "
"Every prioritized boundary was exercised and no defect survived."
),
findings=[],
),
open_request=_no_judge,
)
assert accepted is True
assert "SHIP verdict with 0 finding(s)" in reason
@pytest.mark.parametrize(
("role_line", "expected"),
[
("Hermes-Task-Role: review", goal.REVIEW_ROLE),
("Task-Role: audit", goal.REVIEW_ROLE),
("- Hermes Task Role = diagnostic", goal.REVIEW_ROLE),
("Hermes-Expected-Output: verdict", goal.REVIEW_ROLE),
("Hermes-Task-Role: implementation", goal.IMPLEMENTATION_ROLE),
],
)
def test_explicit_card_metadata_decides_the_task_role(role_line, expected):
card = IMPLEMENTATION_CARD.replace("## Body\n", f"## Body\n{role_line}\n")
assert goal.task_role(card, _review())[0] == expected
assert goal.task_role(card, _review())[1] == "directive"
def test_explicit_review_metadata_cannot_self_certify_a_changed_file_report():
card = REVIEW_CARD.replace("## Body\n", "## Body\nHermes-Task-Role: review\n")
mutated = _review(changed_files=["services/hermes/scripts/cli_lane_goal.py"])
assert goal.task_role(card, mutated) == (goal.IMPLEMENTATION_ROLE, "conflict")
accepted, reason = goal.judge_goal_completion(
card,
mutated,
open_request=lambda *_a, **_k: JudgeResponse("continue", "the card forbade edits"),
)
assert accepted is False
assert reason == "the card forbade edits"
# The declaration still holds when the report really did change nothing.
assert goal.task_role(card, _review()) == (goal.REVIEW_ROLE, "directive")
def test_appended_run_history_cannot_reassign_the_card_role():
assert goal.task_role(POLLUTED_CARD, _review()) == (goal.REVIEW_ROLE, "inferred")
assert "Recent work by" not in goal.card_scope(POLLUTED_CARD)
assert "Do not edit files" in goal.card_scope(POLLUTED_CARD)
def test_implementation_card_never_infers_a_review_role():
assert goal.task_role(IMPLEMENTATION_CARD, _review()) == (
goal.IMPLEMENTATION_ROLE,
"default",
)
@pytest.mark.parametrize(
("result", "expected"),
[
(_review(summary="Verdict: BLOCK."), "carries no reviewable rationale"),
(_review(findings=[]), "BLOCK verdict reports no findings"),
(
_review(summary=_review()["summary"].replace("Verdict: BLOCK.", "")),
"declares no explicit SHIP or BLOCK verdict",
),
(
_review(verdict="SHIP"),
"declares both SHIP and BLOCK verdicts",
),
(
_review(verdict="maybe"),
"review verdict field is not SHIP or BLOCK",
),
(
_review(
summary=(
"The reviewed change is fine overall. SHIP is defensible but BLOCK "
"is also arguable given the unresolved capacity question."
),
),
"names both SHIP and BLOCK without a verdict line",
),
(
_review(
summary=(
"Independent read-only review of PR #18. Verdict: SHIP. Every "
"prioritized boundary was exercised and nothing survived."
),
findings=[],
tests_run=[],
artifacts=[],
),
"no findings, tests, or artifacts as evidence",
),
],
)
def test_malformed_review_completion_fails_closed(result, expected):
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
result,
open_request=_no_judge,
)
assert accepted is False
assert expected in reason
assert reason.startswith(goal.READ_ONLY_GUARD)
def test_rejected_review_never_demands_an_implementation_edit():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(findings=[]),
open_request=_no_judge,
)
assert accepted is False
assert "do not modify the reviewed implementation" in reason
def test_incomplete_review_turn_is_resumed_under_the_read_only_guard():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(status="incomplete", summary="Only two of six boundaries were read."),
open_request=_no_judge,
)
assert accepted is False
assert reason.startswith(goal.READ_ONLY_GUARD)
assert "Only two of six boundaries were read." in reason
def test_blocked_review_reports_its_own_obstacle_not_a_verdict():
result = _review(
status="blocked",
summary="The pull request head could not be resolved from any SCM mirror.",
blockers=["Gitea API is unreachable from the review worktree."],
)
assert goal.unfinished_result_reason(result, role=goal.REVIEW_ROLE) is None
assert goal.unfinished_result_reason(result) is None
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD, result, open_request=_no_judge
)
assert accepted is False
assert "rather than a finished task" in reason
def test_conflicting_role_directives_hold_the_card_to_implementation():
card = REVIEW_CARD.replace(
"## Body\n",
"## Body\nHermes-Task-Role: review\nHermes-Task-Role: implementation\n",
)
assert goal.task_role(card, _review()) == (goal.IMPLEMENTATION_ROLE, "directive")
def test_incomplete_review_summary_is_bounded_before_it_leaves_the_judge():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(status="incomplete", summary="pending\n" + "y" * 4000),
open_request=_no_judge,
)
assert accepted is False
assert reason.startswith(goal.READ_ONLY_GUARD)
assert len(reason) <= goal.JUDGE_REASON_LIMIT
def test_review_claiming_completion_with_blockers_is_rejected():
accepted, reason = goal.judge_goal_completion(
REVIEW_CARD,
_review(blockers=["the diff could not be resolved"]),
open_request=_no_judge,
)
assert accepted is False
assert "worker reported blockers while claiming completion" in reason
def test_review_prose_about_the_reviewed_artifact_is_not_unfinished_work():
result = _review(
summary=(
"Independent read-only review. Verdict: BLOCK. The reviewed branch's CI "
"build is still running and its migration remains pending, which is why "
"the change is unfit to ship."
),
tests_run=["upstream pipeline: build is still running on the reviewed head"],
)
assert goal.unfinished_result_reason(result, role=goal.REVIEW_ROLE) is None
accepted, _reason = goal.judge_goal_completion(
REVIEW_CARD,
result,
open_request=_no_judge,
)
assert accepted is True
def test_implementation_prose_about_unfinished_work_is_still_rejected():
result = _implementation(summary="The broad rerun remains active before the push.")
assert goal.unfinished_result_reason(result, role=goal.IMPLEMENTATION_ROLE)
assert goal.judge_goal_completion(
IMPLEMENTATION_CARD, result, open_request=_no_judge
) == (False, "completion evidence says work is unfinished: remains active")
class JudgeResponse:
def __init__(self, verdict: str, reason: str):
self.body = json.dumps(
{"choices": [{"message": {"content": json.dumps(
{"verdict": verdict, "reason": reason}
)}}]}
).encode()
def __enter__(self):
return self
def __exit__(self, *_args):
return False
def read(self):
return self.body
def test_implementation_card_keeps_the_fail_closed_model_judge():
observed = {}
def request(req, timeout):
observed["payload"] = json.loads(req.data)
observed["timeout"] = timeout
return JudgeResponse("continue", "the remote head was never verified")
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(),
open_request=request,
)
assert accepted is False
assert reason == "the remote head was never verified"
role = json.loads(observed["payload"]["messages"][1]["content"])["task_role"]
assert role == goal.IMPLEMENTATION_ROLE
assert observed["timeout"] == 120
def test_implementation_completion_still_finalizes_on_an_accepting_judge():
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(),
open_request=lambda *_a, **_k: JudgeResponse("complete", "every criterion has evidence"),
)
assert accepted is True
assert reason == "every criterion has evidence"
def test_ambiguous_card_without_a_verdict_report_stays_on_the_model_judge():
ambiguous = """# Kanban task t_mixed: Review and repair the pool
## Body
Review the pool, then repair what you find. Do not modify unrelated files.
Return a verdict and open a draft PR with the repairs.
"""
accepted, _reason = goal.judge_goal_completion(
ambiguous,
_review(),
open_request=lambda *_a, **_k: JudgeResponse("continue", "the repairs are missing"),
)
assert goal.task_role(ambiguous, _review())[0] == goal.IMPLEMENTATION_ROLE
assert accepted is False
def test_judge_reason_is_bounded_sanitized_and_secret_free():
leaky = "line one\r\n\x07 token ghp_" + "A" * 24 + " " + "x" * 900
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(),
open_request=lambda *_a, **_k: JudgeResponse("continue", leaky),
)
assert accepted is False
assert len(reason) <= goal.JUDGE_REASON_LIMIT
assert "ghp_" not in reason
assert "[redacted]" in reason
assert "\n" not in reason and "\r" not in reason and "\x07" not in reason
def test_unavailable_judge_still_fails_closed_for_implementation_cards():
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(),
open_request=lambda *_a, **_k: JudgeResponse("unknown", "bad"),
)
assert accepted is False
assert "local completion judge unavailable" in reason
def test_bare_uppercase_verdict_token_is_accepted_as_a_last_resort():
result = _review(
summary=(
"Independent read-only review of PR #18 at head 2000252 concludes BLOCK "
"because five reproducible P0 defects fire on the first coordinator tick."
),
)
assert goal.review_verdict(result) == ("BLOCK", None)
accepted, _reason = goal.judge_goal_completion(
REVIEW_CARD, result, open_request=_no_judge
)
assert accepted is True
def test_unknown_role_directive_falls_through_to_the_next_contract_surface():
card = IMPLEMENTATION_CARD.replace(
"## Body\n",
"## Body\nHermes-Task-Role: gardener\nHermes-Expected-Output: verdict\n",
)
assert goal.task_role(card, _review()) == (goal.REVIEW_ROLE, "directive")
def test_unrecognized_directives_leave_the_role_to_inference():
card = REVIEW_CARD.replace("## Body\n", "## Body\nHermes-Task-Role: gardener\n")
assert goal.task_role(card, _review()) == (goal.REVIEW_ROLE, "inferred")
def test_malformed_evidence_fields_are_read_without_raising():
assert goal._strings("not-a-list") == []
result = _implementation(tests_run="pytest -q", summary="Everything shipped.")
assert goal.unfinished_result_reason(result, role=goal.IMPLEMENTATION_ROLE) is None
def test_a_blocked_implementation_report_is_rejected_without_the_read_only_guard():
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(status="blocked", summary="The remote was unreachable."),
open_request=_no_judge,
)
assert accepted is False
assert reason == "worker reported status 'blocked' rather than a finished task"
def test_implementation_rejection_reason_carries_no_read_only_guard():
accepted, reason = goal.judge_goal_completion(
IMPLEMENTATION_CARD,
_implementation(blockers=["the remote was unreachable"]),
open_request=_no_judge,
)
assert accepted is False
assert reason == "worker reported blockers while claiming completion"
def test_oversized_objectives_and_empty_reasons_stay_bounded():
compacted = goal._bounded("a" * 9000, 6000)
assert len(compacted) == 6000
assert "[goal evidence compacted]" in compacted
assert goal.sanitize_reason(" ") == "local judge supplied no reason"
def test_judging_never_mutates_the_reported_result():
result = _review()
snapshot = json.dumps(result, sort_keys=True)
goal.judge_goal_completion(REVIEW_CARD, result, open_request=_no_judge)
assert json.dumps(result, sort_keys=True) == snapshot