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>
479 lines
16 KiB
Python
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
|