feat(500): one end-of-turn request - the completion-section check folds into the reply check
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 1m7s
CI & Build / Python tests (push) Failing after 1m25s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 13s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 1m7s
CI & Build / Python tests (push) Failing after 1m25s
CI & Build / Build & push image (push) Skipped
The Stop hook sent the finished reply twice: scribe_report_check.sh checked a task-closing reply for the completion sections in shell and reported to /report-check, and scribe_reply_check.sh sent the same reply to /reply-rules for the rule hold. Now the reply goes once. When the turn closed a task the hook adds the close count and ids, and the server runs the section check (services/report_check, the same three patterns) beside the reply hold, folding both into one reason. The report_check adherence log is still written for every checked reply (milestone 409's number). A section hold marks the session, so its rewrite is sent back once with rewrite:true to record how it came out, and is never held. The block reason carries the completion shape's one line and points at list_reply_shapes rather than at the skill. #5496 (step 4 of milestone 500). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+103
-1
@@ -197,7 +197,8 @@ async def test_the_route_passes_the_reply_and_all_three_ledgers():
|
||||
with patch.object(routes.moment_delivery_svc, "reply_hold", hold):
|
||||
resp = await routes.reply_rules.__wrapped__()
|
||||
body = await resp.get_json()
|
||||
assert body == {"reason": "Held.", "rule_ids": [11], "moments": ["reply.report"]}
|
||||
assert body == {"reason": "Held.", "rule_ids": [11], "moments": ["reply.report"],
|
||||
"report_check": ""}
|
||||
assert hold.await_args.args == (7, "done")
|
||||
assert hold.await_args.kwargs == {
|
||||
"project_id": 3, "exclude": frozenset({5}), "held": frozenset({4}),
|
||||
@@ -205,6 +206,55 @@ async def test_the_route_passes_the_reply_and_all_three_ledgers():
|
||||
}
|
||||
|
||||
|
||||
async def _reply_route(body, *, hold_reason="", checked=None):
|
||||
from scribe.routes import plugin as routes
|
||||
|
||||
hold = AsyncMock(return_value={"reason": hold_reason, "rule_ids": [11] if hold_reason else [],
|
||||
"moments": ["reply.report"]})
|
||||
check = AsyncMock(return_value=checked or {"outcome": "passed", "reason": ""})
|
||||
app = Quart(__name__)
|
||||
async with app.test_request_context("/api/plugin/reply-rules", method="POST", json=body,
|
||||
query_string={"project_id": "3"}):
|
||||
g.user = type("U", (), {"id": 7})()
|
||||
with patch.object(routes.moment_delivery_svc, "reply_hold", hold), \
|
||||
patch.object(routes.report_check_svc, "check_reply", check):
|
||||
resp = await routes.reply_rules.__wrapped__()
|
||||
return await resp.get_json(), hold, check
|
||||
|
||||
|
||||
async def test_a_turn_that_closed_nothing_is_not_section_checked():
|
||||
_body, _hold, check = await _reply_route({"reply": "done"})
|
||||
check.assert_not_awaited()
|
||||
|
||||
|
||||
async def test_a_closing_turn_is_section_checked_and_both_holds_fold_into_one_reason():
|
||||
"""One end-of-turn request (milestone 500 step 4): the section check and
|
||||
the rule hold answer together, in one `reason`."""
|
||||
body, _hold, check = await _reply_route(
|
||||
{"reply": "done", "closed": 1, "closed_task_ids": [41, True, "x"], "rewrite": False},
|
||||
hold_reason="RULE HOLD", checked={"outcome": "blocked", "reason": "SECTIONS"},
|
||||
)
|
||||
assert body["reason"] == "SECTIONS\n\nRULE HOLD"
|
||||
assert body["report_check"] == "blocked"
|
||||
# JSON `true` is not task 1, and a string is not an id.
|
||||
assert check.await_args.kwargs == {"task_ids": [41], "rewrite": False, "project_id": 3}
|
||||
|
||||
|
||||
async def test_a_task_created_already_done_is_checked_without_an_id():
|
||||
_body, _hold, check = await _reply_route({"reply": "done", "closed": 1, "closed_task_ids": []})
|
||||
assert check.await_args.kwargs["task_ids"] == []
|
||||
|
||||
|
||||
async def test_the_rewrite_is_recorded_and_held_by_nothing():
|
||||
body, hold, check = await _reply_route(
|
||||
{"reply": "done", "closed": 1, "closed_task_ids": [41], "rewrite": True},
|
||||
checked={"outcome": "passed_after_rewrite", "reason": ""},
|
||||
)
|
||||
assert check.await_args.kwargs["rewrite"] is True
|
||||
hold.assert_not_awaited()
|
||||
assert body["reason"] == "" and body["report_check"] == "passed_after_rewrite"
|
||||
|
||||
|
||||
# ── the hook ────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@@ -251,6 +301,58 @@ def test_the_hook_blocks_in_the_servers_words_and_records_the_hold(tmp_path):
|
||||
assert seen[1]["exclude_rule_ids"] == ["11"]
|
||||
|
||||
|
||||
def _closing_transcript(tmp_path, *replies):
|
||||
lines = [
|
||||
{"type": "user", "message": {"role": "user", "content": "please finish it"}},
|
||||
{"type": "assistant", "message": {"content": [
|
||||
{"type": "tool_use", "id": "toolu_1", "name": "mcp__plugin_scribe_scribe__update_task",
|
||||
"input": {"task_id": 41, "status": "done"}}]}},
|
||||
{"type": "user", "message": {"content": [
|
||||
{"type": "tool_result", "tool_use_id": "toolu_1", "is_error": False, "content": "{}"}]}},
|
||||
] + [{"type": "assistant", "message": {"content": [{"type": "text", "text": r}]}} for r in replies]
|
||||
path = tmp_path / "t.jsonl"
|
||||
path.write_text("\n".join(json.dumps(x, separators=(",", ":")) for x in lines) + "\n")
|
||||
return path
|
||||
|
||||
|
||||
SECTION_HOLD = json.dumps({"reason": "Rewrite as a completion report.", "rule_ids": [],
|
||||
"moments": ["reply.report"], "report_check": "blocked"}).encode()
|
||||
|
||||
|
||||
def test_a_turn_that_closed_nothing_sends_no_close(tmp_path):
|
||||
t = _transcript(tmp_path, "Done.")
|
||||
with http_sink(by_path={"/api/plugin/reply-rules": QUIET}) as (port, seen):
|
||||
_run(tmp_path, port, t)
|
||||
assert "closed" not in json.loads(seen[0]["_body"])
|
||||
|
||||
|
||||
def test_a_section_hold_blocks_once_then_the_rewrite_is_reported_and_never_held(tmp_path):
|
||||
"""The completion-section check, folded into the one Stop request: the
|
||||
first stop is held in the server's words, the rewrite goes back once with
|
||||
`rewrite: true` so its outcome is recorded, and nothing after that is sent."""
|
||||
t = _closing_transcript(tmp_path, "All done, pushed it.")
|
||||
with http_sink(by_path={"/api/plugin/reply-rules": SECTION_HOLD}) as (port, seen):
|
||||
out = json.loads(_run(tmp_path, port, t))
|
||||
assert out == {"decision": "block", "reason": "Rewrite as a completion report."}
|
||||
first = json.loads(seen[0]["_body"])
|
||||
assert (first["closed"], first["closed_task_ids"], first["rewrite"]) == (1, [41], False)
|
||||
|
||||
rewritten = _closing_transcript(tmp_path, "All done, pushed it.", "Where this sits: …")
|
||||
assert _run(tmp_path, port, rewritten, active=True) == ""
|
||||
assert json.loads(seen[1]["_body"])["rewrite"] is True
|
||||
# The marker is spent: a further stop in the loop sends nothing.
|
||||
assert _run(tmp_path, port, rewritten, active=True) == ""
|
||||
assert len(seen) == 2
|
||||
|
||||
|
||||
def test_a_rule_hold_on_a_closing_turn_does_not_mark_a_rewrite(tmp_path):
|
||||
t = _closing_transcript(tmp_path, "Done.")
|
||||
with http_sink(by_path={"/api/plugin/reply-rules": HELD}) as (port, seen):
|
||||
_run(tmp_path, port, t)
|
||||
assert _run(tmp_path, port, t, active=True) == ""
|
||||
assert len(seen) == 1
|
||||
|
||||
|
||||
def test_the_hook_says_nothing_when_nothing_holds(tmp_path):
|
||||
t = _transcript(tmp_path, "Done.")
|
||||
with http_sink(by_path={"/api/plugin/reply-rules": QUIET}) as (port, _seen):
|
||||
|
||||
@@ -1,169 +0,0 @@
|
||||
"""The Stop hook that checks a task-closing reply for the completion sections
|
||||
(milestone 409 step 5).
|
||||
|
||||
Runs the real shell against synthetic transcripts in the shape Claude Code
|
||||
writes (one content block per JSONL line) and the shared HTTP sink. What it
|
||||
pins: silence on every turn that closed nothing; a block only when the
|
||||
instance recorded it, in the words the instance returned; one rewrite at most,
|
||||
recorded; and no block from another plugin's loop or a failed task write.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from tests.helpers import http_sink
|
||||
|
||||
HOOK = Path(__file__).resolve().parents[1] / "plugin" / "hooks" / "scribe_report_check.sh"
|
||||
TOOL = "mcp__plugin_scribe_scribe__update_task"
|
||||
GOOD = ('**Where this sits:** milestone 12 "Move the backups offsite", step 3 of 5.\n'
|
||||
"**What now works:** the sync runs nightly.\n**Needs you:** nothing.\n**Next:** alerts.")
|
||||
BAD = "All done, pushed it."
|
||||
REASON = "SERVER REASON: rewrite as a completion report"
|
||||
|
||||
|
||||
def _env(tmp_path, url="http://127.0.0.1:9"):
|
||||
for tool in ("curl", "bash"):
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"hook runtime tool {tool!r} not installed")
|
||||
return {"PATH": os.environ["PATH"], "SCRIBE_URL": url, "SCRIBE_TOKEN": "t",
|
||||
"TMPDIR": str(tmp_path), "HOME": str(tmp_path)}
|
||||
|
||||
|
||||
def _prompt(text="please finish it"):
|
||||
return {"type": "user", "message": {"role": "user", "content": text}}
|
||||
|
||||
|
||||
def _tool_use(tid="toolu_1", status="done", name=TOOL, task_id=41):
|
||||
return {"type": "assistant", "message": {"content": [
|
||||
{"type": "tool_use", "id": tid, "name": name, "input": {"task_id": task_id, "status": status}}]}}
|
||||
|
||||
|
||||
def _result(tid="toolu_1", is_error=False):
|
||||
return {"type": "user", "message": {"content": [
|
||||
{"type": "tool_result", "tool_use_id": tid, "is_error": is_error, "content": "{}"}]}}
|
||||
|
||||
|
||||
def _text(text):
|
||||
return {"type": "assistant", "message": {"content": [{"type": "text", "text": text}]}}
|
||||
|
||||
|
||||
def _transcript(tmp_path, lines):
|
||||
path = tmp_path / "t.jsonl"
|
||||
# Compact, like the file Claude Code writes ({"name":"…"}, no spaces).
|
||||
path.write_text("\n".join(json.dumps(line, separators=(",", ":")) for line in lines) + "\n")
|
||||
return path
|
||||
|
||||
|
||||
def _run(env, transcript, active=False, session="s1"):
|
||||
out = subprocess.run(
|
||||
["bash", str(HOOK)],
|
||||
input=json.dumps({"session_id": session, "transcript_path": str(transcript),
|
||||
"cwd": str(transcript.parent), "hook_event_name": "Stop",
|
||||
"stop_hook_active": active}),
|
||||
capture_output=True, text=True, env=env, timeout=30,
|
||||
)
|
||||
assert out.returncode == 0, out.stderr
|
||||
return out.stdout.strip()
|
||||
|
||||
|
||||
def _closing_turn(reply):
|
||||
return [_prompt(), _text("On it."), _tool_use(), _result(), _text(reply)]
|
||||
|
||||
|
||||
def test_a_turn_that_closed_nothing_is_silent_and_reports_nothing(tmp_path):
|
||||
with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
t = _transcript(tmp_path, [_prompt(), _tool_use(status="in_progress"), _result(), _text(BAD)])
|
||||
assert _run(env, t) == ""
|
||||
assert seen == []
|
||||
|
||||
|
||||
def test_a_complete_report_passes_silently_and_is_recorded(tmp_path):
|
||||
with http_sink(b'{"status":"ok"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
assert _run(env, _transcript(tmp_path, _closing_turn(GOOD))) == ""
|
||||
assert [q["outcome"] for q in seen] == [["passed"]]
|
||||
assert seen[0]["task_ids"] == ["41"]
|
||||
|
||||
|
||||
def test_a_missing_section_blocks_once_in_the_servers_words_then_records_the_rewrite(tmp_path):
|
||||
reply = json.dumps({"status": "ok", "reason": REASON}).encode()
|
||||
with http_sink(reply) as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
out = json.loads(_run(env, _transcript(tmp_path, _closing_turn(BAD))))
|
||||
assert out == {"decision": "block", "reason": REASON}
|
||||
assert seen[0]["outcome"] == ["blocked"]
|
||||
assert seen[0]["missing"] == ["where it sits,needs you,next"]
|
||||
|
||||
# The rewrite: Claude Code sets stop_hook_active; the hook records and never blocks again.
|
||||
rewritten = _transcript(tmp_path, _closing_turn(BAD) + [_text(GOOD)])
|
||||
assert _run(env, rewritten, active=True) == ""
|
||||
assert seen[1]["outcome"] == ["passed_after_rewrite"]
|
||||
assert _run(env, rewritten, active=True) == ""
|
||||
assert len(seen) == 2
|
||||
|
||||
|
||||
def test_a_rewrite_that_still_misses_is_recorded_and_not_blocked(tmp_path):
|
||||
reply = json.dumps({"status": "ok", "reason": REASON}).encode()
|
||||
with http_sink(reply) as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
t = _transcript(tmp_path, _closing_turn(BAD))
|
||||
_run(env, t)
|
||||
assert _run(env, t, active=True) == ""
|
||||
assert [q["outcome"][0] for q in seen] == ["blocked", "missing_after_rewrite"]
|
||||
|
||||
|
||||
def test_another_hooks_block_loop_is_left_alone(tmp_path):
|
||||
with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
assert _run(env, _transcript(tmp_path, _closing_turn(BAD)), active=True) == ""
|
||||
assert seen == []
|
||||
|
||||
|
||||
def test_a_task_write_that_failed_closed_nothing(tmp_path):
|
||||
with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
t = _transcript(tmp_path, [_prompt(), _tool_use(), _result(is_error=True), _text(BAD)])
|
||||
assert _run(env, t) == ""
|
||||
assert seen == []
|
||||
|
||||
|
||||
def test_a_task_closed_in_an_earlier_turn_does_not_count(tmp_path):
|
||||
with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
t = _transcript(tmp_path, _closing_turn(GOOD) + [_prompt("thanks, what else?"), _text(BAD)])
|
||||
assert _run(env, t) == ""
|
||||
assert seen == []
|
||||
|
||||
|
||||
def test_a_reply_not_yet_written_is_not_judged(tmp_path):
|
||||
with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
t = _transcript(tmp_path, [_prompt(), _tool_use(), _result()])
|
||||
assert _run(env, t) == ""
|
||||
assert seen == []
|
||||
|
||||
|
||||
def test_no_block_without_a_recorded_check(tmp_path):
|
||||
t = _transcript(tmp_path, _closing_turn(BAD))
|
||||
# Unreachable instance.
|
||||
assert _run(_env(tmp_path), t) == ""
|
||||
# An instance that answered but returned no reason.
|
||||
with http_sink(b'{"status":"ok"}') as (port, seen):
|
||||
assert _run(_env(tmp_path, f"http://127.0.0.1:{port}"), t, session="s2") == ""
|
||||
assert seen[0]["outcome"] == ["blocked"]
|
||||
|
||||
|
||||
def test_a_bare_id_does_not_count_as_placing_the_work(tmp_path):
|
||||
reply = json.dumps({"status": "ok", "reason": REASON}).encode()
|
||||
with http_sink(reply) as (port, seen):
|
||||
env = _env(tmp_path, f"http://127.0.0.1:{port}")
|
||||
bare = "Closed #41.\n**Needs you:** nothing.\n**Next:** #42."
|
||||
assert json.loads(_run(env, _transcript(tmp_path, _closing_turn(bare))))["decision"] == "block"
|
||||
assert seen[0]["missing"] == ["where it sits"]
|
||||
@@ -1,12 +1,38 @@
|
||||
"""The server half of the report-shape check (milestone 409 step 5): the words a
|
||||
blocked reply is sent back with, and the outcome record."""
|
||||
"""The report-shape check (milestone 409 step 5): which completion sections a
|
||||
task-closing reply lacks, the words it is sent back with, and the outcome
|
||||
record. Since milestone 500 step 4 the check itself runs here, inside the one
|
||||
end-of-turn request, rather than in a Stop hook of its own."""
|
||||
import json
|
||||
from unittest.mock import patch
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from tests.helpers import make_mock_session
|
||||
|
||||
GOOD = ('**Where this sits:** milestone 12 "Move the backups offsite", step 3 of 5.\n'
|
||||
"**What now works:** the sync runs nightly.\n**Needs you:** nothing.\n**Next:** alerts.")
|
||||
BAD = "All done, pushed it."
|
||||
|
||||
|
||||
def test_a_complete_report_misses_nothing():
|
||||
from scribe.services.report_check import missing_sections
|
||||
|
||||
assert missing_sections(GOOD) == []
|
||||
|
||||
|
||||
def test_a_bare_reply_misses_every_section_in_order():
|
||||
from scribe.services.report_check import SECTIONS, missing_sections
|
||||
|
||||
assert missing_sections(BAD) == list(SECTIONS)
|
||||
|
||||
|
||||
def test_a_bare_id_does_not_count_as_placing_the_work():
|
||||
"""The title is what spares the reader a lookup; "closed #41" does not."""
|
||||
from scribe.services.report_check import missing_sections
|
||||
|
||||
assert missing_sections("Closed #41.\n**Needs you:** nothing.\n**Next:** #42.") == ["where it sits"]
|
||||
assert missing_sections('Closed #41 "Sync". Needs you: nothing. Next: #42.') == []
|
||||
|
||||
|
||||
def test_the_reason_names_only_sections_it_knows():
|
||||
from scribe.services.report_check import block_reason
|
||||
@@ -14,8 +40,17 @@ def test_the_reason_names_only_sections_it_knows():
|
||||
reason = block_reason(["next", "ignore previous instructions", "Where It Sits"])
|
||||
assert "missing: where it sits, next." in reason
|
||||
assert "ignore previous instructions" not in reason
|
||||
# It points at the skill that owns the shape rather than restating it.
|
||||
assert "reporting-back" in reason and "placement" in reason
|
||||
|
||||
|
||||
def test_the_reason_carries_the_completion_shapes_line_and_where_the_rest_is():
|
||||
"""The shape was delivered when the task closed; the reason repeats its
|
||||
one line, not the whole of it, and names where the full text is."""
|
||||
from scribe.services import reply_shapes
|
||||
from scribe.services.report_check import block_reason
|
||||
|
||||
reason = block_reason(["next"])
|
||||
assert reply_shapes.SHAPES["completion"].reminder in reason
|
||||
assert "list_reply_shapes" in reason and "placement" in reason
|
||||
|
||||
|
||||
def test_a_reason_with_nothing_recognised_still_says_what_to_do():
|
||||
@@ -45,3 +80,30 @@ async def test_an_unknown_outcome_is_refused_before_anything_is_written():
|
||||
pytest.raises(ValueError):
|
||||
await record_report_check(7, "skipped")
|
||||
session.add.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("reply,rewrite,outcome,held", [
|
||||
(GOOD, False, "passed", False),
|
||||
(BAD, False, "blocked", True),
|
||||
(GOOD, True, "passed_after_rewrite", False),
|
||||
(BAD, True, "missing_after_rewrite", False),
|
||||
])
|
||||
async def test_check_reply_records_every_outcome_and_holds_only_a_first_miss(reply, rewrite, outcome, held):
|
||||
from scribe.services import report_check
|
||||
|
||||
record = AsyncMock()
|
||||
with patch.object(report_check, "record_report_check", record):
|
||||
got = await report_check.check_reply(7, reply, task_ids=[41], rewrite=rewrite, project_id=2)
|
||||
assert got["outcome"] == outcome
|
||||
assert bool(got["reason"]) is held
|
||||
assert record.await_args.args == (7, outcome)
|
||||
assert record.await_args.kwargs["task_ids"] == [41]
|
||||
|
||||
|
||||
async def test_an_unrecorded_check_holds_nothing():
|
||||
"""A hold the numbers cannot see is not one this check may make."""
|
||||
from scribe.services import report_check
|
||||
|
||||
with patch.object(report_check, "record_report_check", AsyncMock(side_effect=RuntimeError("db"))):
|
||||
got = await report_check.check_reply(7, BAD, task_ids=[41], rewrite=False)
|
||||
assert got == {"outcome": "", "reason": ""}
|
||||
|
||||
Reference in New Issue
Block a user