fix(goals): failed quality gates re-run every boundary instead of replaying a status fingerprint
`_check_gates()` skipped a failed gate whenever sha256(git HEAD + `git status --porcelain`) matched the last failure. Porcelain sees neither the contents of an untracked or already-modified file nor inputs outside the repo, so a repaired input replayed the stale failure and burned retries until the goal auto-paused (#110649). The gate now runs on every eligible boundary; the retry cap still bounds a genuinely stuck red suite. `workspace_fingerprint` and `GoalGate.last_failed_fingerprint` are removed with their only consumer (old persisted state ignores the extra key on load). Based on the analysis in #110649 (JsonDaRula69) and PR #110658 (KoNit-K), whose `git diff HEAD` hash still misses untracked contents and adds a full diff per boundary.
This commit is contained in:
@@ -9,7 +9,6 @@ failures are fail-OPEN (``continue``); the turn budget is the backstop.
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import hashlib
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
@@ -347,8 +346,6 @@ class GoalGate:
|
||||
attempts: int = 0
|
||||
last_exit_code: Optional[int] = None
|
||||
last_output_tail: str = ""
|
||||
# Workspace fingerprint at the last FAILED run — skips re-running an identical gate unchanged.
|
||||
last_failed_fingerprint: str = ""
|
||||
|
||||
def to_dict(self) -> Dict[str, Any]:
|
||||
return asdict(self)
|
||||
@@ -364,33 +361,9 @@ class GoalGate:
|
||||
attempts=int(data.get("attempts") or 0),
|
||||
last_exit_code=(int(data["last_exit_code"]) if data.get("last_exit_code") is not None else None),
|
||||
last_output_tail=str(data.get("last_output_tail") or ""),
|
||||
last_failed_fingerprint=str(data.get("last_failed_fingerprint") or ""),
|
||||
)
|
||||
|
||||
|
||||
def workspace_fingerprint(cwd: Optional[str] = None) -> str:
|
||||
"""sha256 of ``git rev-parse HEAD`` + ``git status --porcelain``; "" outside git (never matches,
|
||||
so gates always re-run — a safe fallback)."""
|
||||
workdir = cwd or os.getcwd()
|
||||
try:
|
||||
outputs = []
|
||||
for argv, timeout in (
|
||||
(["git", "rev-parse", "HEAD"], 10),
|
||||
(["git", "status", "--porcelain"], 30),
|
||||
):
|
||||
proc = subprocess.run(
|
||||
argv, capture_output=True, text=True, encoding="utf-8", errors="replace",
|
||||
timeout=timeout, cwd=workdir, stdin=subprocess.DEVNULL, env=noninteractive_git_env(),
|
||||
)
|
||||
if proc.returncode != 0:
|
||||
return ""
|
||||
outputs.append(proc.stdout)
|
||||
blob = outputs[0].strip() + "\n" + outputs[1]
|
||||
return hashlib.sha256(blob.encode("utf-8", "replace")).hexdigest()
|
||||
except Exception:
|
||||
return ""
|
||||
|
||||
|
||||
def run_gate(gate: GoalGate, *, cwd: Optional[str] = None) -> Tuple[bool, int, str]:
|
||||
"""Run one gate through the shell. Returns ``(passed, exit_code, output_tail)``; a timeout kills
|
||||
the process and counts as exit code -1."""
|
||||
@@ -1293,31 +1266,25 @@ class GoalManager:
|
||||
def _check_gates(self) -> Optional[Dict[str, Any]]:
|
||||
"""Run quality gates in order; return a decision dict on failure.
|
||||
|
||||
An unchanged workspace since the last failure of the same gate is NOT re-run — the recorded
|
||||
failure is replayed and the attempt count advances, so a stalled agent can't spin re-running
|
||||
an identical red suite.
|
||||
Every eligible boundary re-executes a failed gate. A git HEAD+porcelain fingerprint used to
|
||||
replay the recorded failure when "nothing changed", but porcelain sees neither the contents
|
||||
of an untracked or already-modified file nor inputs outside the repo, so a repaired input
|
||||
was replayed as still-failing until retry exhaustion paused the goal (#110649). The
|
||||
retry cap below still bounds a genuinely stuck red suite.
|
||||
"""
|
||||
state = self._state
|
||||
if state is None or not state.gates:
|
||||
return None
|
||||
|
||||
fingerprint = workspace_fingerprint()
|
||||
for gate in state.gates:
|
||||
unchanged = bool(fingerprint) and gate.last_exit_code not in (None, 0) and gate.last_failed_fingerprint == fingerprint
|
||||
if unchanged:
|
||||
passed, exit_code, tail = False, int(gate.last_exit_code or -1), gate.last_output_tail
|
||||
else:
|
||||
passed, exit_code, tail = run_gate(gate)
|
||||
passed, exit_code, tail = run_gate(gate)
|
||||
gate.last_exit_code = exit_code
|
||||
gate.last_output_tail = tail
|
||||
if passed:
|
||||
gate.attempts = 0
|
||||
gate.last_failed_fingerprint = ""
|
||||
continue
|
||||
|
||||
gate.attempts += 1
|
||||
gate.last_failed_fingerprint = fingerprint
|
||||
skipped_note = " (workspace unchanged since last failure — not re-run)" if unchanged else ""
|
||||
|
||||
if gate.attempts > gate.max_retries:
|
||||
return self._pause_decision(
|
||||
@@ -1338,7 +1305,7 @@ class GoalManager:
|
||||
"active", True, prompt, "gate_failed",
|
||||
f"gate failed (exit {exit_code}): $ {gate.command}",
|
||||
f"✗ Quality gate failed ({state.turns_used}/{state.max_turns} turns, "
|
||||
f"attempt {gate.attempts}/{gate.max_retries}){skipped_note}: $ {gate.command}",
|
||||
f"attempt {gate.attempts}/{gate.max_retries}): $ {gate.command}",
|
||||
)
|
||||
|
||||
self._save()
|
||||
@@ -1719,7 +1686,7 @@ def run_kanban_goal_loop(
|
||||
|
||||
__all__ = [
|
||||
"GoalState", "GoalContract", "GoalGate", "GoalManager", "parse_contract", "draft_contract", "run_gate",
|
||||
"workspace_fingerprint", "CONTINUATION_PROMPT_TEMPLATE", "CONTINUATION_PROMPT_WITH_SUBGOALS_TEMPLATE",
|
||||
"CONTINUATION_PROMPT_TEMPLATE", "CONTINUATION_PROMPT_WITH_SUBGOALS_TEMPLATE",
|
||||
"CONTINUATION_PROMPT_WITH_CONTRACT_TEMPLATE", "JUDGE_USER_PROMPT_TEMPLATE",
|
||||
"JUDGE_USER_PROMPT_WITH_SUBGOALS_TEMPLATE", "JUDGE_USER_PROMPT_WITH_CONTRACT_TEMPLATE",
|
||||
"DRAFT_CONTRACT_SYSTEM_PROMPT", "KANBAN_GOAL_CONTINUATION_TEMPLATE", "KANBAN_GOAL_FINALIZE_TEMPLATE",
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
"""Tests for /goal quality gates (GoalGate, run_gate, GoalManager gate flow)."""
|
||||
|
||||
import json
|
||||
import subprocess
|
||||
import sys
|
||||
import time
|
||||
from unittest.mock import patch
|
||||
@@ -161,8 +162,7 @@ def test_status_line_mentions_gates():
|
||||
def test_failing_gate_short_circuits_judge():
|
||||
mgr = _mgr_with_goal("gate-fail-sid")
|
||||
mgr.add_gate("exit 5")
|
||||
with patch("hermes_cli.goals.judge_goal") as mock_judge, \
|
||||
patch("hermes_cli.goals.workspace_fingerprint", return_value=""):
|
||||
with patch("hermes_cli.goals.judge_goal") as mock_judge:
|
||||
decision = mgr.evaluate_after_turn("I think it's done!")
|
||||
mock_judge.assert_not_called()
|
||||
assert decision["verdict"] == "gate_failed"
|
||||
@@ -190,8 +190,7 @@ def test_gate_retry_exhaustion_pauses_goal():
|
||||
mgr = _mgr_with_goal("gate-exhaust-sid")
|
||||
mgr.add_gate("exit 1")
|
||||
mgr.state.gates[0].max_retries = 2
|
||||
with patch("hermes_cli.goals.judge_goal") as mock_judge, \
|
||||
patch("hermes_cli.goals.workspace_fingerprint", return_value=""):
|
||||
with patch("hermes_cli.goals.judge_goal") as mock_judge:
|
||||
d1 = mgr.evaluate_after_turn("attempt one")
|
||||
d2 = mgr.evaluate_after_turn("attempt two")
|
||||
d3 = mgr.evaluate_after_turn("attempt three")
|
||||
@@ -204,38 +203,34 @@ def test_gate_retry_exhaustion_pauses_goal():
|
||||
assert "gate" in (mgr.state.paused_reason or "")
|
||||
|
||||
|
||||
def test_unchanged_workspace_skips_rerun():
|
||||
mgr = _mgr_with_goal("gate-unchanged-sid")
|
||||
mgr.add_gate("exit 1")
|
||||
with patch("hermes_cli.goals.workspace_fingerprint", return_value="fp-1"), \
|
||||
patch("hermes_cli.goals.judge_goal"):
|
||||
mgr.evaluate_after_turn("turn 1")
|
||||
# Second turn, same fingerprint — run_gate must NOT run again.
|
||||
with patch("hermes_cli.goals.run_gate") as mock_run:
|
||||
d2 = mgr.evaluate_after_turn("turn 2")
|
||||
mock_run.assert_not_called()
|
||||
assert d2["verdict"] == "gate_failed"
|
||||
assert "unchanged" in d2["message"]
|
||||
def test_failed_gate_reruns_when_untracked_file_content_changes(tmp_path, monkeypatch):
|
||||
"""#110649: `git status --porcelain` reports the same `?? untracked/` for `before` and
|
||||
`after`, so a status-based cache replayed the stale failure; the gate must execute again."""
|
||||
for argv in (["init", "-q"], ["config", "user.email", "t@example.com"], ["config", "user.name", "T"],
|
||||
["commit", "-q", "--allow-empty", "-m", "baseline"]):
|
||||
subprocess.run(["git", *argv], cwd=tmp_path, check=True, capture_output=True)
|
||||
result = tmp_path / "untracked" / "result.txt"
|
||||
result.parent.mkdir()
|
||||
result.write_text("before", encoding="utf-8")
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
|
||||
def test_changed_workspace_reruns_gate():
|
||||
mgr = _mgr_with_goal("gate-changed-sid")
|
||||
mgr.add_gate("exit 1")
|
||||
with patch("hermes_cli.goals.judge_goal"):
|
||||
with patch("hermes_cli.goals.workspace_fingerprint", return_value="fp-1"):
|
||||
mgr.evaluate_after_turn("turn 1")
|
||||
with patch("hermes_cli.goals.workspace_fingerprint", return_value="fp-2"), \
|
||||
patch("hermes_cli.goals.run_gate", return_value=(False, 1, "still red")) as mock_run:
|
||||
mgr.evaluate_after_turn("turn 2")
|
||||
mock_run.assert_called_once()
|
||||
mgr = _mgr_with_goal("gate-content-sid")
|
||||
mgr.add_gate(f"grep -q after {result}")
|
||||
with patch("hermes_cli.goals.judge_goal", return_value=("done", "ok", False, None, False)) as judge:
|
||||
d1 = mgr.evaluate_after_turn("turn 1")
|
||||
result.write_text("after", encoding="utf-8")
|
||||
d2 = mgr.evaluate_after_turn("turn 2")
|
||||
assert d1["verdict"] == "gate_failed"
|
||||
assert d2["verdict"] == "done"
|
||||
judge.assert_called_once()
|
||||
assert mgr.state.gates[0].attempts == 0
|
||||
|
||||
|
||||
def test_gate_continuation_respects_turn_budget():
|
||||
mgr = GoalManager(session_id="gate-budget-sid", default_max_turns=1)
|
||||
mgr.set("budget goal")
|
||||
mgr.add_gate("exit 1")
|
||||
with patch("hermes_cli.goals.judge_goal"), \
|
||||
patch("hermes_cli.goals.workspace_fingerprint", return_value=""):
|
||||
with patch("hermes_cli.goals.judge_goal"):
|
||||
decision = mgr.evaluate_after_turn("only turn")
|
||||
assert decision["status"] == "paused"
|
||||
assert decision["should_continue"] is False
|
||||
|
||||
@@ -160,13 +160,6 @@ def test_working_diff_is_safe(malicious_repo):
|
||||
assert _fired(marker) == []
|
||||
|
||||
|
||||
def test_goals_fingerprint_is_safe(malicious_repo):
|
||||
from hermes_cli.goals import workspace_fingerprint
|
||||
repo, marker = malicious_repo
|
||||
workspace_fingerprint(str(repo))
|
||||
assert _fired(marker) == []
|
||||
|
||||
|
||||
def test_web_git_diff_is_safe(malicious_repo):
|
||||
from hermes_cli import web_git
|
||||
repo, marker = malicious_repo
|
||||
|
||||
@@ -145,7 +145,7 @@ How it works, each turn:
|
||||
|
||||
1. **Gates run before the judge.** If any gate fails, the judge is *not called* — a red gate is deterministic evidence the goal isn't done. The gate's exit code and output tail (last ~3 KB) become the continuation prompt, so the agent iterates against the actual failure instead of a vibe.
|
||||
2. **All gates pass → normal judging.** The LLM judge then decides done/blocked/continue/wait exactly as before.
|
||||
3. **Unchanged workspace → no re-run.** If a gate failed and nothing changed in the workspace since (tracked via a git fingerprint of HEAD + working-tree status), the gate is not re-run — the recorded failure is replayed and the attempt count advances. A stuck agent can't burn wall-clock re-running an identical red suite. Outside a git repo, gates simply always re-run.
|
||||
3. **Every boundary re-runs a failed gate.** The command executes against the current inputs each time; a stale result is never replayed, so a gate whose input you just repaired passes on the next boundary. The retry cap bounds a genuinely stuck red suite.
|
||||
4. **Retries are bounded.** Each gate defaults to 3 retries and a 5-minute timeout. When a gate exhausts its retries the goal auto-pauses (like the turn budget) with a message telling you to fix it manually, remove the gate, or `/goal resume`.
|
||||
|
||||
Gates persist with the goal in `SessionDB.state_meta` (they survive `/resume` and context compression), and gate management (`/goal gate …`) is safe mid-run on the gateway — gates only run at turn boundary.
|
||||
|
||||
Reference in New Issue
Block a user