Bind approvals and feedback to the revision they were about

Fixes counterexamples 1-4 from specs/orchestrator.qnt: a repeated
'approve', 'approve rev-1' meant for the architecture, approving on both
channels, and 'approve' plus an afterthought all landed on the next
plan, one the operator had not seen.

- Revisions are numbered across the work item (A4 already required
  monotonic tags; the code numbered per gate), so rev-N names one plan.
- Each reply is stamped on arrival with the revision that was up: in
  Matrix the one whose approval request had been sent (a plain value
  the transport thread reads, restored on boot), in Gitea the latest
  plan comment older than the reply (board.posts).
- Gate.evaluate applies a reply that names no revision only if its stamp
  is the current revision. Otherwise an approval is not applied and
  feedback rejects nothing (it is still in the transcript), and the
  agent says which revision is up. 'approve rev-N' binds by its number.
- Spec A4 states both rules. The model applies them; scenarios 1-4 now
  expect the properties to hold, and both held over 200,000 random
  traces and Apalache's exhaustive check to depth 8.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE

nisse-dev/tests/test_approval_binding.py

@@ -0,0 +1,147 @@
+"""Approvals and feedback bind to the plan revision they were about (A4)."""
+
+import io
+from pathlib import Path
+
+from nisse_dev.canonical import UserMessage
+from nisse_dev.control import (
+    AWAIT_MILESTONE_SIGNOFF,
+    Orchestrator,
+    StubAgent,
+)
+from nisse_dev.gitea import Comment
+from nisse_dev.oplog import OpLog
+from nisse_dev.state import StateStore
+from nisse_dev.transport import InMemoryTransport
+
+
+class Held:
+    """A worker that queues jobs until released, like one under load."""
+
+    def __init__(self) -> None:
+        self.jobs: list = []
+        self.busy = False
+
+    def submit(self, job) -> None:
+        self.jobs.append(job)
+
+    def call(self, job, timeout=None):
+        return job()
+
+    def run(self) -> None:
+        while self.jobs:
+            self.jobs.pop(0)()
+
+
+class Board:
+    def __init__(self) -> None:
+        self.next_id = 100
+
+    def open_project_issue(self, project: str, task_id: str) -> int:
+        return 10
+
+    def post_plan(self, issue: int, gate: str, rev: int, plan: str):
+        self.next_id += 1
+
+        class Posted:
+            comment_id = self.next_id
+
+        return Posted()
+
+    def echo(self, issue: int, text: str) -> None:
+        pass
+
+
+def build(tmp_path: Path, worker: Held | None = None, board: Board | None = None):
+    store = StateStore(tmp_path / "state.db", oplog=OpLog("nisse-dev", io.StringIO()))
+    transport = InMemoryTransport()
+    orch = Orchestrator(
+        store,
+        transport,
+        StubAgent(),
+        operators={"don"},
+        gitea_operators={"don"},
+        board=board,
+        **({"worker": worker} if worker else {}),
+    )
+    orch.start()
+    return store, transport, orch
+
+
+def gate(store: StateStore) -> dict:
+    return store.get_state().data.get("gate") or {}
+
+
+def test_revisions_run_across_the_task(tmp_path: Path) -> None:
+    store, transport, _ = build(tmp_path)
+    transport.receive("don", "use demo")
+    assert gate(store)["rev"] == 1
+    transport.receive("don", "approve")
+    assert gate(store) == {**gate(store), "name": "milestone-1", "rev": 2}
+    assert "milestone-1 rev-2" in transport.outbound[-1].text
+
+
+def test_a_stale_approval_is_answered_and_a_fresh_one_works(tmp_path: Path) -> None:
+    worker = Held()
+    store, transport, _ = build(tmp_path, worker)
+    transport.receive("don", "use demo")
+    worker.run()
+    transport.receive("don", "approve")
+    transport.receive("don", "approve")  # both while arch rev-1 was up
+    worker.run()
+    assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF
+    assert not gate(store)["approved"]
+    assert "arrived while rev-1 was up" in transport.outbound[-1].text
+    transport.receive("don", "approve")  # now about milestone-1 rev-2
+    worker.run()
+    assert gate(store)["approved"]
+
+
+def test_stale_feedback_is_recorded_but_rejects_nothing(tmp_path: Path) -> None:
+    worker = Held()
+    store, transport, _ = build(tmp_path, worker)
+    transport.receive("don", "use demo")
+    worker.run()
+    transport.receive("don", "approve")
+    transport.receive("don", "also, keep it in one file")
+    worker.run()
+    assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF
+    assert gate(store)["rev"] == 2
+    assert "not taken as feedback" in transport.outbound[-1].text
+    said = [
+        e.event.text for e in store.read_events() if isinstance(e.event, UserMessage)
+    ]
+    assert "also, keep it in one file" in said  # the planner still hears it
+
+
+def test_a_gitea_reply_binds_to_the_latest_plan_posted_before_it(
+    tmp_path: Path,
+) -> None:
+    board = Board()
+    store, transport, orch = build(tmp_path, board=board)
+    transport.receive("don", "use demo")  # arch rev-1 posted as comment 101
+    orch._handle_plan_comment_now(
+        Comment(id=102, author="don", body="approve", created_at="t")
+    )
+    # Approved arch rev-1; milestone-1 rev-2 is now posted as comment 103.
+    assert gate(store) == {**gate(store), "name": "milestone-1", "approved": False}
+    # A reply older than that post was about arch rev-1: not applied.
+    orch._handle_plan_comment_now(
+        Comment(id=102, author="don", body="approve", created_at="t")
+    )
+    assert not gate(store)["approved"]
+    # One after it is about milestone-1 rev-2.
+    orch._handle_plan_comment_now(
+        Comment(id=104, author="don", body="approve", created_at="t")
+    )
+    assert gate(store)["approved"]
+
+
+def test_the_plan_up_survives_a_restart(tmp_path: Path) -> None:
+    _, transport, _ = build(tmp_path)
+    transport.receive("don", "use demo")  # arch rev-1 up; no board, so unsaved
+    store, transport, _ = build(tmp_path)  # restart at the open gate
+    transport.receive("don", "approve")  # stamped with the restored rev-1
+    assert store.get_state().state == AWAIT_MILESTONE_SIGNOFF
+    assert gate(store)["name"] == "milestone-1"
+    assert gate(store)["rev"] == 2

nisse-dev/tests/test_control.py

@@ -128,7 +128,9 @@ def test_start_milestone_requires_approval_first(h: Harness) -> None:
     h.transport.receive("don", "approve")
     h.transport.receive("don", "start milestone 1")
     assert h.state == AWAIT_MILESTONE_SIGNOFF  # it was rejection feedback
-    h.transport.receive("don", "approve rev-2")
+    # Revisions run across the task: arch rev-1, milestone-1 rev-2, and
+    # after that rejection milestone-1 rev-3.
+    h.transport.receive("don", "approve rev-3")
     h.transport.receive("don", "start milestone 1")
     assert executed(h)
 

nisse-dev/tests/test_gates.py

@@ -67,3 +67,39 @@ def test_a_sentence_containing_approve_is_still_feedback() -> None:
     result = gate().evaluate("don", "I approve of most of it", channel="transport")
     assert isinstance(result, Rejected)
     assert result.feedback == "I approve of most of it"
+
+
+# --- binding by arrival (A4): a reply that names no revision is about the one
+# that was up when it arrived.
+
+
+def test_a_bare_approval_about_an_earlier_revision_is_not_applied() -> None:
+    result = gate().evaluate("don", "approve", channel="transport", about=1)
+    assert isinstance(result, Ignored)
+    assert "arrived while rev-1 was up" in result.notice
+    assert "arch rev-2" in result.notice
+
+
+def test_feedback_about_an_earlier_revision_is_not_a_rejection() -> None:
+    result = gate().evaluate("don", "also add docs", channel="transport", about=1)
+    assert isinstance(result, Ignored)
+    assert "not taken as feedback on arch rev-2" in result.notice
+
+
+def test_a_reply_before_any_plan_was_up_is_not_applied() -> None:
+    result = gate().evaluate("don", "approve", channel="transport", about=0)
+    assert isinstance(result, Ignored)
+    assert "no plan" in result.notice
+
+
+def test_replies_about_the_current_revision_apply() -> None:
+    assert isinstance(gate().evaluate("don", "approve", "transport", about=2), Approved)
+    result = gate().evaluate("don", "add docs", "transport", about=2)
+    assert isinstance(result, Rejected)
+
+
+def test_a_named_revision_binds_by_its_number() -> None:
+    # "approve rev-2" says which plan it means, whenever it arrived.
+    assert isinstance(
+        gate().evaluate("don", "approve rev-2", "gitea", about=1), Approved
+    )

nisse-dev/tests/test_quint_counterexamples.py

@@ -191,7 +191,6 @@ class Agent:
 XFAIL = pytest.mark.xfail(strict=True, reason="counterexample; fix pending")
 
 
-@XFAIL
 def test_double_approve_approves_an_unseen_plan(tmp_path: Path) -> None:
     """doubleApproveApprovesAnUnseenPlanTest"""
     worker = QueueingWorker()
@@ -205,7 +204,6 @@ def test_double_approve_approves_an_unseen_plan(tmp_path: Path) -> None:
     assert not agent.gate().get("approved")
 
 
-@XFAIL
 def test_named_revision_approves_the_wrong_gate(tmp_path: Path) -> None:
     """namedRevisionApprovesTheWrongGateTest"""
     worker = QueueingWorker()
@@ -218,7 +216,6 @@ def test_named_revision_approves_the_wrong_gate(tmp_path: Path) -> None:
     assert not agent.gate().get("approved")
 
 
-@XFAIL
 def test_approval_on_both_channels_approves_the_next_plan(tmp_path: Path) -> None:
     """approvalOnBothChannelsApprovesTheNextPlanTest"""
     gitea = FakeGitea()
@@ -237,7 +234,6 @@ def test_approval_on_both_channels_approves_the_next_plan(tmp_path: Path) -> Non
     assert not agent.gate().get("approved")
 
 
-@XFAIL
 def test_an_afterthought_rejects_the_next_plan(tmp_path: Path) -> None:
     """anAfterthoughtRejectsTheNextPlanTest"""
     worker = QueueingWorker()
@@ -250,7 +246,7 @@ def test_an_afterthought_rejects_the_next_plan(tmp_path: Path) -> None:
     # The note was about the architecture; the milestone plan was not
     # rejected by it.
     assert agent.state == AWAIT_MILESTONE_SIGNOFF
-    assert agent.gate().get("rev") == 1
+    assert agent.gate().get("rev") == 2  # arch was rev-1; not re-planned
 
 
 @XFAIL

nisse-dev/tests/test_stop.py

@@ -229,8 +229,12 @@ def test_stop_reaches_work_running_on_the_worker(tmp_path: Path, word: str) -> N
         cancel=cancel,
     )
     worker.call(orch.start)
-    for text in ("use demo", "approve", "approve", "start milestone 1"):
+    for text in ("use demo", "approve", "approve"):
         transport.receive("don", text)
+        # Reply only once the plan is up: a reply stamped with an earlier
+        # revision is not applied to a later one.
+        worker.call(lambda: None)
+    transport.receive("don", "start milestone 1")
     assert executor.running.wait(5)
 
     transport.receive("don", "status")  # delivered while the milestone runs