fix(issue-5): address phase 2a cross-review findings (F-A, F-B, R1, R2, R3)

Two reviewers raised five defects. Every one was reproduced here before being
accepted; none were rebutted.

F-A (claude, blocking) -- the CAS table had no row for two transitions the
wrapper legitimately makes, so `submit --type loop|discuss` silently stopped
working by default: the per-iteration re-dispatch was refused and its metadata
discarded, the exhausted-budget finalisation was refused, and the CLI returned 0
both times. Reproduced against the real modules. Fixed by naming those
transitions rather than carving an exception: `redispatch` and `finalize` are
requests gated on operator authority, so the wrapper can move a finished record
while an agent still cannot touch its own terminal. A re-dispatch must advance
the iteration, so a replay cannot resurrect a job. The operator CLI now exits
non-zero with a reason when a write is refused -- the silence is what let this
hide.

F-B (claude) -- moving the audit mirror out of the lock made status.json
last-writer-wins, so a slow flush could leave it behind the registry. It now
carries transition_seq and skips a write that would go backwards, restoring the
sequential consistency the lock used to provide for free.

R1 (codex, blocking) -- the policy called attribution with only the dispatch
record, so it proved the record was well-formed and never confronted the
identifiers the evidence carried. A finalized scenario with a different session,
a different attempt and a manifest from another attempt was still approved.
Observed identifiers are now collected from the timeline and every one must
agree; a manifest belonging to another attempt holds. Collecting every
occurrence rather than the first matters, because taking the first would drop
exactly the contradicting value this is meant to catch.

R2 (codex, blocking) -- a supplied expected_attempt was ignored when the record
had no attempt field, so a compare-and-swap with nothing to compare committed.
It now fails closed, which matters because 2b plans to gate inferred completion
on precisely that precondition.

R3 (codex) -- a status could ride in as an ordinary field, so `progress` could
commit a terminal from inside the boundary that exists to prevent it. Status is
now settable only by the request that owns it, unknown requests are refused, and
pending has an owning request instead of being an untyped merge.

Each of the eleven new regressions was checked against the pre-fix code first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UrDNJkHmm18uu7n7JwV5Qd
This commit is contained in:
GodopuandClaude Opus 5 committed 2026-09-06 19:33:24 +09:00
1 parent 17a2dcd988
commit 56d08efa95
6 files changed
+378 -23

No files matched your search

+192
View File
@@ -695,3 +695,195 @@ def test_g9_phase2a_spawns_no_daemon_and_calls_no_frozen_helper():
src = path.read_text(encoding="utf-8")
for forbidden in ("ensure_subscriber_daemon", "preexec_fn", "lib.sh"):
assert forbidden not in src, "{} references {}".format(path.name, forbidden)
# ==============================================================================
# 8. Cross-review regressions (job 9d1e0077)
#
# F-A (claude): the CAS table omitted two legitimate wrapper transitions, so
# `submit --type loop|discuss` silently stopped working by default
# and the CLI still exited 0.
# F-B (claude): the audit status mirror moved outside the lock and became
# last-writer-wins, so a slow flush could leave it behind.
# R1 (codex): the policy trusted the dispatch record's own flags and never
# confronted the identifiers the evidence carried.
# R2 (codex): a supplied expected_attempt was ignored when the record had none.
# R3 (codex): a raw status field rode in on a metadata merge.
# ==============================================================================
@pytest.mark.parametrize("mode", CAS_MODES)
def test_fa_wrapper_loop_redispatch_and_finalize_work(mam_sandbox, tmp_path, monkeypatch, mode):
"""F-A: the exact CLI calls the wrapper makes for --type loop|discuss.
Iteration reset hands a finished job to the next session; exhausting the
budget finalises it as an error. Both must work in either mode."""
monkeypatch.setenv("MAM_ENGINE_CAS", mode)
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import mqtt_common, registry
reg = _reg(tmp_path)
job_id = _make_job(reg, agent_session="herdr:claude", job_type="loop")
mqtt_common.update_job_status(job_id, reg, _source="publish_event", status="completed")
rc = registry.main(["--registry-dir", reg, "update", "--job", job_id,
"--agent-session", "herdr:rev", "--prompt", "review",
"--iteration", "2", "--role", "Reviewer", "--status", "pending"])
record = _load(reg, job_id)
assert rc == 0, "the wrapper's iteration reset must succeed"
assert record["status"] == "pending"
assert record["iteration"] == 2, "metadata must not be discarded"
assert record["agent_session"] == "herdr:rev"
mqtt_common.update_job_status(job_id, reg, _source="publish_event", status="completed")
rc2 = registry.main(["--registry-dir", reg, "status", "--job", job_id, "--set", "error"])
assert rc2 == 0
assert _load(reg, job_id)["status"] == "error"
def test_fa_redispatch_requires_operator_authority_and_a_forward_attempt(
mam_sandbox, tmp_path, monkeypatch
):
"""The new authority must not become a way for an agent to undo a terminal."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import engine
reg = _reg(tmp_path)
job_id = _make_job(reg)
engine.transition(job_id, "claim", registry_dir=reg)
engine.transition(job_id, "completed", registry_dir=reg)
agent = engine.transition(job_id, "redispatch", registry_dir=reg,
fields={"iteration": 2}, source="publish_event")
assert agent.committed is False and "operator authority" in agent.reason
stale = engine.transition(job_id, "redispatch", registry_dir=reg,
fields={"iteration": 1}, source="registry_cli")
assert stale.committed is False and "does not advance" in stale.reason
ok = engine.transition(job_id, "redispatch", registry_dir=reg,
fields={"iteration": 2}, source="registry_cli")
assert ok.committed and ok.to_status == "pending"
def test_fa_agent_still_cannot_move_a_terminal(mam_sandbox, tmp_path, monkeypatch):
"""The protection the CAS table exists for must survive the new authority."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import mqtt_common
reg = _reg(tmp_path)
job_id = _make_job(reg)
mqtt_common.update_job_status(job_id, reg, _source="publish_event", status="completed")
mqtt_common.update_job_status(job_id, reg, _source="publish_event", status="error")
assert _load(reg, job_id)["status"] == "completed"
def test_fa_a_refused_operator_write_exits_non_zero(mam_sandbox, tmp_path, monkeypatch):
"""The silence is what hid F-A: a refused write used to exit 0."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import engine, registry
reg = _reg(tmp_path)
job_id = _make_job(reg)
engine.transition(job_id, "claim", registry_dir=reg)
engine.transition(job_id, "completed", registry_dir=reg)
# cancelled after completed is a genuine conflict, and must be reported
rc = registry.main(["--registry-dir", reg, "status", "--job", job_id, "--set", "cancelled"])
assert rc != 0, "a refused operator write must not look like success"
assert _load(reg, job_id)["status"] == "completed"
def test_fb_a_late_status_mirror_cannot_overwrite_a_newer_one(mam_sandbox, tmp_path, monkeypatch):
"""F-B: the mirror runs outside the lock, so ordering rides on transition_seq."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import mqtt_common
reg = _reg(tmp_path)
job_id = _make_job(reg)
logs = mqtt_common.get_logs_dir()
mqtt_common.update_logged_status(job_id, "completed", logs, transition_seq=2)
mqtt_common.update_logged_status(job_id, "running", logs, transition_seq=1) # late flush
assert mqtt_common.read_logged_status(job_id, logs)["status"] == "completed"
# a genuinely newer transition still lands
mqtt_common.update_logged_status(job_id, "error", logs, transition_seq=3)
assert mqtt_common.read_logged_status(job_id, logs)["status"] == "error"
@pytest.mark.parametrize("field,value", [
("agent_session_uuid", "someone-else"),
("attempt_id", 999),
("job_id", "another-job"),
])
def test_r1_policy_rejects_evidence_that_contradicts_the_dispatch(field, value):
"""R1: attribution must confront what was observed, not just the record."""
from lib_py.loop import policy
import copy
scenario = json.loads((WATCHER_FIXTURES / "finalized_candidate.json").read_text(encoding="utf-8"))
assert policy.decide(scenario).decision == "candidate_approved"
mutated = copy.deepcopy(scenario)
mutated["timeline"][2]["payload"][field] = value
decision = policy.decide(mutated)
assert decision.decision == "hold", "{}={!r} must not be approved".format(field, value)
assert field in decision.reason
def test_r1_policy_rejects_a_manifest_from_another_attempt():
"""A finalized report proves *an* attempt finished, not necessarily this one."""
from lib_py.loop import policy
import copy
scenario = json.loads((WATCHER_FIXTURES / "finalized_candidate.json").read_text(encoding="utf-8"))
mutated = copy.deepcopy(scenario)
mutated["timeline"][1]["payload"]["report_manifest"]["attempt_id"] = 999
decision = policy.decide(mutated)
assert decision.decision == "hold" and "manifest" in decision.reason
def test_r2_expected_attempt_must_be_proved_not_merely_uncontradicted(
mam_sandbox, tmp_path, monkeypatch
):
"""R2: a precondition the record cannot answer must fail closed."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import engine, mqtt_common
reg = _reg(tmp_path)
job_id = _make_job(reg)
engine.transition(job_id, "claim", registry_dir=reg)
record = _load(reg, job_id)
record.pop("iteration", None)
record.pop("attempt_id", None)
mqtt_common._atomic_write_record(job_id, reg, record)
before = _dir_fingerprint(Path(reg))
res = engine.transition(job_id, "completed", registry_dir=reg, expected_attempt=999)
assert res.committed is False and "carries none" in res.reason
assert _dir_fingerprint(Path(reg)) == before, "a rejection must not write"
# with an attempt present the precondition behaves normally
engine.transition(job_id, "progress", registry_dir=reg, fields={"attempt_id": 7})
assert engine.transition(job_id, "completed", registry_dir=reg,
expected_attempt=8).committed is False
assert engine.transition(job_id, "completed", registry_dir=reg,
expected_attempt=7).committed is True
@pytest.mark.parametrize("request_", ["progress", "permission_required", "merge"])
def test_r3_a_status_cannot_ride_in_as_a_metadata_field(
mam_sandbox, tmp_path, monkeypatch, request_
):
"""R3: the boundary itself must refuse it, not just one wrapper."""
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import engine
reg = _reg(tmp_path)
job_id = _make_job(reg)
engine.transition(job_id, "claim", registry_dir=reg)
res = engine.transition(job_id, request_, registry_dir=reg,
fields={"status": "completed"})
assert res.committed is False and "must be requested" in res.reason
assert _load(reg, job_id)["status"] == "running"
def test_r3_an_unknown_request_is_refused(mam_sandbox, tmp_path, monkeypatch):
monkeypatch.chdir(mam_sandbox)
from lib_py.delegation import engine
reg = _reg(tmp_path)
job_id = _make_job(reg)
engine.transition(job_id, "claim", registry_dir=reg)
res = engine.transition(job_id, "not_a_request", registry_dir=reg, fields={"x": 1})
assert res.committed is False and "unknown request" in res.reason
assert "x" not in _load(reg, job_id)