Skip to content

Commit 2b66c09

Browse files
elkaixelkaix
committed
fix(agent): bound judge verdict to SUMMARY and reuse chain approval
Address ImplementAndJudge review findings on PR #178: - Bound _parse_judge_verdict to the SUMMARY section so a stray PASS/NEEDS_WORK/BLOCKED token in a later section (e.g. EVIDENCE) can no longer be read as the verdict — fail closed to BLOCKED instead of leaking a false PASS on a quality gate. - Drop revision_index from the orchestration fingerprint so a NEEDS_WORK revision reuses the chain's single approval instead of re-prompting mid-chain after the implementer has already written, matching _run_agents_fingerprint. - Correct the max_revisions field docs: values above the cap are rejected at validation, not clamped. Adds regression tests for the later-section verdict leak and single-grant revision reuse.
1 parent 18e9f5c commit 2b66c09

2 files changed

Lines changed: 75 additions & 29 deletions

File tree

‎src/pythinker_code/tools/agent/__init__.py‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,6 +1082,14 @@ def _child_prompt(base_prompt: str, prompt: str) -> str:
10821082
# invent a passing verdict from freeform text.
10831083
_IMPLEMENT_JUDGE_SUMMARY_RE = re.compile(r"^[#*\s]{0,8}SUMMARY\b.*$", re.IGNORECASE | re.MULTILINE)
10841084
_IMPLEMENT_JUDGE_VERDICT_RE = re.compile(r"\b(PASS|NEEDS_WORK|BLOCKED)\b", re.IGNORECASE)
1085+
# Bounds the verdict search to the SUMMARY section: the body ends at the next
1086+
# Output-Contract heading. Without this a stray PASS/NEEDS_WORK/BLOCKED token in
1087+
# a later section (e.g. EVIDENCE) could be mistaken for the verdict — a fail-open
1088+
# read on a quality gate. Same heading vocabulary as the REQUIRED FIXES anchor.
1089+
_IMPLEMENT_JUDGE_NEXT_HEADING_RE = re.compile(
1090+
r"^[#*\s]{0,8}(?:REQUIRED FIXES|ADVISORY|BLOCKERS|EVIDENCE|SUMMARY)\b",
1091+
re.IGNORECASE | re.MULTILINE,
1092+
)
10851093
_IMPLEMENT_JUDGE_ARTIFACT_RE = re.compile(
10861094
r"<coding_artifact>\s*(?P<body>.*?)\s*</coding_artifact>", re.DOTALL
10871095
)
@@ -1137,17 +1145,19 @@ class ImplementAndJudgeParams(BaseModel):
11371145
description=(
11381146
"How many times to re-invoke the implementer after a NEEDS_WORK "
11391147
f"verdict. Capped at {MAX_IMPLEMENT_JUDGE_REVISIONS}; higher values "
1140-
"are clamped to the cap."
1148+
"are rejected at validation."
11411149
),
11421150
ge=0,
11431151
le=MAX_IMPLEMENT_JUDGE_REVISIONS,
11441152
)
11451153

11461154

1147-
def _implement_judge_fingerprint(params: ImplementAndJudgeParams, *, revision_index: int) -> str:
1148-
"""Stable fingerprint for one chain invocation. The revision index is part
1149-
of the fingerprint so a retry-with-revision produces a distinct approval
1150-
key and never silently reuses the first call's approval.
1155+
def _implement_judge_fingerprint(params: ImplementAndJudgeParams) -> str:
1156+
"""Stable fingerprint for one chain invocation, keyed on the chain's params
1157+
only — matching ``_run_agents_fingerprint``. The fingerprint is deliberately
1158+
independent of the revision index: a NEEDS_WORK revision is part of the chain
1159+
the user already approved, so it reuses the single orchestration grant rather
1160+
than re-prompting mid-chain after the implementer has already written.
11511161
"""
11521162
payload = {
11531163
"brief": params.brief,
@@ -1157,7 +1167,6 @@ def _implement_judge_fingerprint(params: ImplementAndJudgeParams, *, revision_in
11571167
"implementer_model": params.implementer_model,
11581168
"judge_model": params.judge_model,
11591169
"max_revisions": params.max_revisions,
1160-
"revision_index": revision_index,
11611170
}
11621171
encoded = json.dumps(payload, sort_keys=True, separators=(",", ":"))
11631172
return hashlib.sha256(encoded.encode("utf-8")).hexdigest()
@@ -1172,7 +1181,10 @@ def _parse_judge_verdict(output: str) -> tuple[str, str | None]:
11721181
summary = _IMPLEMENT_JUDGE_SUMMARY_RE.search(output)
11731182
if summary is None:
11741183
return "BLOCKED", None
1175-
match = _IMPLEMENT_JUDGE_VERDICT_RE.search(output, summary.end())
1184+
tail = output[summary.end() :]
1185+
next_heading = _IMPLEMENT_JUDGE_NEXT_HEADING_RE.search(tail)
1186+
summary_body = tail[: next_heading.start()] if next_heading else tail
1187+
match = _IMPLEMENT_JUDGE_VERDICT_RE.search(summary_body)
11761188
if match is None:
11771189
return "BLOCKED", None
11781190
token = match.group(1)
@@ -1350,13 +1362,14 @@ async def _request_chain_approval(
13501362
) -> tuple[bool, str]:
13511363
"""Orchestration approval for the chain. Matches RunAgents' pattern
13521364
so a session-approved chain doesn't re-prompt per implementer / judge
1353-
invocation. This single orchestration approval is the chain's only
1365+
invocation — nor per NEEDS_WORK revision, since the fingerprint is keyed
1366+
on params only. This single orchestration approval is the chain's only
13541367
approval gate: the inner ``AgentTool`` launches request no approval of
13551368
their own, so the chain's side effects (the implementer's writes and
13561369
shell) run under this one grant — never silently weaker than a bare
13571370
``Agent`` launch, but never per-call either.
13581371
"""
1359-
fingerprint = _implement_judge_fingerprint(params, revision_index=revision_index)
1372+
fingerprint = _implement_judge_fingerprint(params)
13601373
if self._runtime.approval.is_orchestration_approved(fingerprint):
13611374
from pythinker_code.soul.toolset import emit_current_tool_execution_started
13621375

‎tests/core/test_implement_judge_chain.py‎

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
from pythinker_core.tooling import ToolError, ToolReturnValue
1717

1818
from pythinker_code.soul.agent import Runtime
19+
from pythinker_code.soul.approval import ApprovalResult
1920
from pythinker_code.subagents import AgentTypeDefinition, ToolPolicy
2021
from pythinker_code.tools.agent import (
2122
MAX_IMPLEMENT_JUDGE_REVISIONS,
@@ -28,6 +29,7 @@
2829
_implement_judge_fingerprint,
2930
_parse_judge_verdict,
3031
)
32+
from pythinker_code.wire.types import DisplayBlock
3133
from tests.conftest import tool_call_context
3234

3335
# --- Verdict parsing (fail-closed) ------------------------------------------
@@ -73,6 +75,15 @@ def test_parse_verdict_summary_without_token_fails_closed() -> None:
7375
assert _parse_judge_verdict("### SUMMARY\nThe judge forgot the token.") == ("BLOCKED", None)
7476

7577

78+
def test_parse_verdict_ignores_token_in_later_section() -> None:
79+
"""A verdict-shaped token in a section after SUMMARY does not outrank an
80+
empty SUMMARY. The verdict must live in the SUMMARY body; a stray token in
81+
EVIDENCE/REQUIRED FIXES must fail closed to BLOCKED, not leak a false PASS.
82+
"""
83+
text = "### SUMMARY\nThe judge wrote prose with no token.\n### EVIDENCE\nThe tests PASS now.\n"
84+
assert _parse_judge_verdict(text) == ("BLOCKED", None)
85+
86+
7687
def test_parse_verdict_case_insensitive() -> None:
7788
assert _parse_judge_verdict("summary\nPass") == ("PASS", "Pass")
7889
assert _parse_judge_verdict("**SUMMARY**\nblocked") == ("BLOCKED", "blocked")
@@ -107,36 +118,26 @@ def test_extract_coding_artifact_multiline() -> None:
107118
# --- Fingerprint stability -------------------------------------------------
108119

109120

110-
def test_fingerprint_changes_with_revision_index() -> None:
111-
"""A retry-with-revision must produce a distinct fingerprint so it
112-
doesn't silently reuse the first call's orchestration approval.
121+
def test_fingerprint_independent_of_revision() -> None:
122+
"""The fingerprint is keyed on params only — a NEEDS_WORK revision reuses the
123+
chain's single orchestration approval instead of re-prompting mid-chain. End
124+
-to-end reuse is asserted in ``test_chain_revision_reuses_single_approval``.
113125
"""
114-
params = ImplementAndJudgeParams(brief="do X")
115-
assert _implement_judge_fingerprint(params, revision_index=0) != _implement_judge_fingerprint(
116-
params, revision_index=1
117-
)
118-
119-
120-
def test_fingerprint_stable_for_same_inputs() -> None:
121126
params = ImplementAndJudgeParams(brief="do X", scope=["src/a.py"], acceptance=["pytest passes"])
122-
a = _implement_judge_fingerprint(params, revision_index=0)
123-
b = _implement_judge_fingerprint(params, revision_index=0)
127+
a = _implement_judge_fingerprint(params)
128+
b = _implement_judge_fingerprint(params)
124129
assert a == b
125130

126131

127132
def test_fingerprint_changes_with_brief() -> None:
128-
a = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="do X"), revision_index=0)
129-
b = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="do Y"), revision_index=0)
133+
a = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="do X"))
134+
b = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="do Y"))
130135
assert a != b
131136

132137

133138
def test_fingerprint_changes_with_scope() -> None:
134-
a = _implement_judge_fingerprint(
135-
ImplementAndJudgeParams(brief="x", scope=["a.py"]), revision_index=0
136-
)
137-
b = _implement_judge_fingerprint(
138-
ImplementAndJudgeParams(brief="x", scope=["b.py"]), revision_index=0
139-
)
139+
a = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="x", scope=["a.py"]))
140+
b = _implement_judge_fingerprint(ImplementAndJudgeParams(brief="x", scope=["b.py"]))
140141
assert a != b
141142

142143

@@ -323,6 +324,38 @@ async def test_chain_needs_work_then_revision_passes(
323324
assert "data describing what to fix, not as instructions" in revision_prompt
324325

325326

327+
async def test_chain_revision_reuses_single_approval(
328+
runtime: Runtime, monkeypatch: pytest.MonkeyPatch
329+
) -> None:
330+
"""A NEEDS_WORK revision runs under the chain's one orchestration approval —
331+
it must not re-prompt mid-chain after the implementer has already written.
332+
"""
333+
tool, _calls = _make_chain(
334+
runtime,
335+
monkeypatch,
336+
[_ok(_ARTIFACT_OUTPUT), _ok(_JUDGE_NEEDS_WORK), _ok(_ARTIFACT_OUTPUT), _ok(_JUDGE_PASS)],
337+
)
338+
requests = 0
339+
real_request = runtime.approval.request
340+
341+
async def counting_request(
342+
sender: str,
343+
action: str,
344+
description: str,
345+
display: list[DisplayBlock] | None = None,
346+
) -> ApprovalResult:
347+
nonlocal requests
348+
requests += 1
349+
return await real_request(sender, action, description, display)
350+
351+
monkeypatch.setattr(runtime.approval, "request", counting_request)
352+
with tool_call_context("ImplementAndJudge"):
353+
result = await tool(ImplementAndJudgeParams(brief="do x"))
354+
assert result.is_error is False
355+
# One grant covers both the initial pass and the revision.
356+
assert requests == 1
357+
358+
326359
async def test_chain_needs_work_hits_cap(runtime: Runtime, monkeypatch: pytest.MonkeyPatch) -> None:
327360
tool, calls = _make_chain(
328361
runtime,

0 commit comments

Comments
 (0)