Match approvals and commands regardless of case and trailing punctuation
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m15s
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m15s
A phone keyboard's "Approve" did not match the approval pattern, so it counted as rejection feedback and threw the plan away; "Stop" did not reach the cancel flag; other capitalised commands got a hint instead of acting. gates.command_form() is how a message reads as a command: case, surrounding whitespace, inner runs of whitespace and a trailing '.' or '!' do not matter. The approval gate and every command match in the orchestrator (stop, status, abandon, use, start milestone, next milestone, project complete, the restart choices) use it. Only matching changes: the project name in 'use <name>' keeps its case, and feedback and answers are recorded and passed on as written. A stale revision is still refused, and approval still needs the whole message to be the command. Closes #64. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE
This commit is contained in:
1 parent
b893fba342
commit
6afd0cc914
5 files changed
+76
-15
No files matched your search
@@ -23,7 +23,14 @@ from typing import Any, Protocol
|
|||||||
|
|
||||||
from nisse_dev.cancel import Cancel
|
from nisse_dev.cancel import Cancel
|
||||||
from nisse_dev.canonical import GateOpened, PlanSubmitted, UserMessage
|
from nisse_dev.canonical import GateOpened, PlanSubmitted, UserMessage
|
||||||
from nisse_dev.gates import Approved, Gate, Ignored, PlanRevision, Rejected
|
from nisse_dev.gates import (
|
||||||
|
Approved,
|
||||||
|
Gate,
|
||||||
|
Ignored,
|
||||||
|
PlanRevision,
|
||||||
|
Rejected,
|
||||||
|
command_form,
|
||||||
|
)
|
||||||
from nisse_dev.git import GitError
|
from nisse_dev.git import GitError
|
||||||
from nisse_dev.gitea import GiteaError
|
from nisse_dev.gitea import GiteaError
|
||||||
from nisse_dev.llm import RoleMap
|
from nisse_dev.llm import RoleMap
|
||||||
@@ -165,6 +172,12 @@ class StubAgent:
|
|||||||
return SubmitPlan(f"[stub] {gate} plan for {project}{notes}")
|
return SubmitPlan(f"[stub] {gate} plan for {project}{notes}")
|
||||||
|
|
||||||
|
|
||||||
|
def _project_name(text: str) -> str:
|
||||||
|
"""The name in 'use <name>', as written: only the command word is
|
||||||
|
case-insensitive."""
|
||||||
|
return text.strip().rstrip(".!")[len("use") :].strip()
|
||||||
|
|
||||||
|
|
||||||
def _fresh_data() -> dict[str, Any]:
|
def _fresh_data() -> dict[str, Any]:
|
||||||
return {
|
return {
|
||||||
"revs": {},
|
"revs": {},
|
||||||
@@ -331,7 +344,7 @@ class Orchestrator:
|
|||||||
if msg.sender not in self._operators:
|
if msg.sender not in self._operators:
|
||||||
return
|
return
|
||||||
text = msg.text.strip()
|
text = msg.text.strip()
|
||||||
if text == "stop":
|
if command_form(text) == "stop":
|
||||||
# Straight to the flag, not behind the work in the queue: the
|
# Straight to the flag, not behind the work in the queue: the
|
||||||
# work is what it has to reach (A4).
|
# work is what it has to reach (A4).
|
||||||
if self._worker.busy:
|
if self._worker.busy:
|
||||||
@@ -434,9 +447,10 @@ class Orchestrator:
|
|||||||
state = self._store.get_state()
|
state = self._store.get_state()
|
||||||
current = state.state if state else IDLE
|
current = state.state if state else IDLE
|
||||||
text = msg.text.strip()
|
text = msg.text.strip()
|
||||||
if text == "status":
|
command = command_form(text)
|
||||||
|
if command == "status":
|
||||||
self._send_status(current)
|
self._send_status(current)
|
||||||
elif text == "abandon":
|
elif command == "abandon":
|
||||||
# Like status, a command in every waiting state: it takes
|
# Like status, a command in every waiting state: it takes
|
||||||
# precedence over reading the word as an answer or feedback.
|
# precedence over reading the word as an answer or feedback.
|
||||||
if current == IDLE:
|
if current == IDLE:
|
||||||
@@ -444,8 +458,8 @@ class Orchestrator:
|
|||||||
else:
|
else:
|
||||||
self._abandon(msg.sender)
|
self._abandon(msg.sender)
|
||||||
elif current == IDLE:
|
elif current == IDLE:
|
||||||
if text.startswith("use "):
|
if command.startswith("use "):
|
||||||
self._begin_project(text[4:].strip())
|
self._begin_project(_project_name(text))
|
||||||
else:
|
else:
|
||||||
self._transport.send_message("no active project; say 'use <project>'")
|
self._transport.send_message("no active project; say 'use <project>'")
|
||||||
elif current in (AWAIT_ARCH_SIGNOFF, AWAIT_MILESTONE_SIGNOFF):
|
elif current in (AWAIT_ARCH_SIGNOFF, AWAIT_MILESTONE_SIGNOFF):
|
||||||
@@ -592,7 +606,7 @@ class Orchestrator:
|
|||||||
name, rev = gate_data["name"], gate_data["rev"]
|
name, rev = gate_data["name"], gate_data["rev"]
|
||||||
milestone = self._data["milestone"]
|
milestone = self._data["milestone"]
|
||||||
if gate_data["approved"]:
|
if gate_data["approved"]:
|
||||||
if text == f"start milestone {milestone}":
|
if command_form(text) == f"start milestone {milestone}":
|
||||||
self._transition(
|
self._transition(
|
||||||
EXECUTING, cause=f"operator command: start milestone {milestone}"
|
EXECUTING, cause=f"operator command: start milestone {milestone}"
|
||||||
)
|
)
|
||||||
@@ -941,14 +955,15 @@ class Orchestrator:
|
|||||||
)
|
)
|
||||||
|
|
||||||
def _handle_pr_open(self, text: str) -> None:
|
def _handle_pr_open(self, text: str) -> None:
|
||||||
if text == "next milestone":
|
command = command_form(text)
|
||||||
|
if command == "next milestone":
|
||||||
self._unwatch("review")
|
self._unwatch("review")
|
||||||
self._data["milestone"] = int(self._data.get("milestone", 1)) + 1
|
self._data["milestone"] = int(self._data.get("milestone", 1)) + 1
|
||||||
self._data["gate"] = None
|
self._data["gate"] = None
|
||||||
self._data["pr"] = None
|
self._data["pr"] = None
|
||||||
self._transition(MILESTONE_PLANNING, cause="operator: next milestone")
|
self._transition(MILESTONE_PLANNING, cause="operator: next milestone")
|
||||||
self._run_planning_turn(f"milestone-{self._data['milestone']}")
|
self._run_planning_turn(f"milestone-{self._data['milestone']}")
|
||||||
elif text == "project complete":
|
elif command == "project complete":
|
||||||
self.stop_watching()
|
self.stop_watching()
|
||||||
self._data = {**_fresh_data()}
|
self._data = {**_fresh_data()}
|
||||||
self._project = None
|
self._project = None
|
||||||
@@ -1022,9 +1037,9 @@ class Orchestrator:
|
|||||||
if pending is None:
|
if pending is None:
|
||||||
# Reached only if state was forced externally or a pending item
|
# Reached only if state was forced externally or a pending item
|
||||||
# was lost; starting over must still work from here.
|
# was lost; starting over must still work from here.
|
||||||
if text.startswith("use "):
|
if command_form(text).startswith("use "):
|
||||||
self.stop_watching()
|
self.stop_watching()
|
||||||
self._begin_project(text[4:].strip())
|
self._begin_project(_project_name(text))
|
||||||
return
|
return
|
||||||
self._transport.send_message(
|
self._transport.send_message(
|
||||||
"waiting for direction, but I have no pending item to resume; "
|
"waiting for direction, but I have no pending item to resume; "
|
||||||
@@ -1061,7 +1076,7 @@ class Orchestrator:
|
|||||||
"""Answer to the question a restart asks (A3 step 7). Never a blind
|
"""Answer to the question a restart asks (A3 step 7). Never a blind
|
||||||
resume: 'continue' re-runs the interrupted planning turn, or returns
|
resume: 'continue' re-runs the interrupted planning turn, or returns
|
||||||
an interrupted milestone to its approved plan to be started again."""
|
an interrupted milestone to its approved plan to be started again."""
|
||||||
choice = text.strip().rstrip(".!").lower()
|
choice = command_form(text)
|
||||||
if choice not in ("continue", "re-plan", "replan"):
|
if choice not in ("continue", "re-plan", "replan"):
|
||||||
self._data["pending"] = pending
|
self._data["pending"] = pending
|
||||||
self._transport.ask_question(_BOOT_CHOICES)
|
self._transport.ask_question(_BOOT_CHOICES)
|
||||||
|
|||||||
@@ -50,6 +50,14 @@ class Ignored:
|
|||||||
_APPROVE_RE = re.compile(r"^approve(?:\s+(rev-\d+))?$")
|
_APPROVE_RE = re.compile(r"^approve(?:\s+(rev-\d+))?$")
|
||||||
|
|
||||||
|
|
||||||
|
def command_form(text: str) -> str:
|
||||||
|
"""How a message reads as a command: case, surrounding whitespace, runs
|
||||||
|
of inner whitespace and trailing '.' or '!' do not matter, so a phone's
|
||||||
|
"Approve." is still an approval. Only for matching; what the operator
|
||||||
|
wrote is what gets recorded and passed on."""
|
||||||
|
return " ".join(text.strip().rstrip(".!").split()).lower()
|
||||||
|
|
||||||
|
|
||||||
class Gate:
|
class Gate:
|
||||||
def __init__(self, revision: PlanRevision, operators: AbstractSet[str]) -> None:
|
def __init__(self, revision: PlanRevision, operators: AbstractSet[str]) -> None:
|
||||||
self._revision = revision
|
self._revision = revision
|
||||||
@@ -61,7 +69,7 @@ class Gate:
|
|||||||
if sender not in self._operators:
|
if sender not in self._operators:
|
||||||
return Ignored(notice=None)
|
return Ignored(notice=None)
|
||||||
text = text.strip()
|
text = text.strip()
|
||||||
match = _APPROVE_RE.match(text)
|
match = _APPROVE_RE.match(command_form(text))
|
||||||
if match is None:
|
if match is None:
|
||||||
return Rejected(feedback=text, approver=sender, channel=channel)
|
return Rejected(feedback=text, approver=sender, channel=channel)
|
||||||
named = match.group(1)
|
named = match.group(1)
|
||||||
|
|||||||
@@ -269,3 +269,17 @@ def test_use_starts_over_when_nothing_is_pending(h: Harness) -> None:
|
|||||||
h.store.transition(AWAIT_INPUT, cause="forced", project="demo", data={})
|
h.store.transition(AWAIT_INPUT, cause="forced", project="demo", data={})
|
||||||
h.transport.receive("don", "use demo")
|
h.transport.receive("don", "use demo")
|
||||||
assert h.state == AWAIT_ARCH_SIGNOFF
|
assert h.state == AWAIT_ARCH_SIGNOFF
|
||||||
|
|
||||||
|
|
||||||
|
def test_commands_ignore_case_but_the_project_name_keeps_it(h: Harness) -> None:
|
||||||
|
h.transport.receive("don", "Use Demo.")
|
||||||
|
assert h.state == AWAIT_ARCH_SIGNOFF
|
||||||
|
assert h.store.get_state().project == "Demo"
|
||||||
|
h.transport.receive("don", "Status")
|
||||||
|
assert "AWAIT_ARCH_SIGNOFF" in h.outbound_texts()[-1]
|
||||||
|
h.transport.receive("don", "Approve")
|
||||||
|
h.transport.receive("don", "Approve.")
|
||||||
|
h.transport.receive("don", "Start Milestone 1")
|
||||||
|
assert h.state == PR_OPEN
|
||||||
|
h.transport.receive("don", "Project Complete!")
|
||||||
|
assert h.state == IDLE
|
||||||
@@ -1,3 +1,5 @@
|
|||||||
|
import pytest
|
||||||
|
|
||||||
from nisse_dev.gates import Approved, Gate, Ignored, PlanRevision, Rejected
|
from nisse_dev.gates import Approved, Gate, Ignored, PlanRevision, Rejected
|
||||||
|
|
||||||
REVISION = PlanRevision(gate="arch", rev=2, content="the plan")
|
REVISION = PlanRevision(gate="arch", rev=2, content="the plan")
|
||||||
@@ -46,3 +48,22 @@ def test_approve_with_trailing_words_is_rejection() -> None:
|
|||||||
# typed word approve, nothing else).
|
# typed word approve, nothing else).
|
||||||
result = gate().evaluate("don", "approve, but rename the module", "transport")
|
result = gate().evaluate("don", "approve, but rename the module", "transport")
|
||||||
assert isinstance(result, Rejected)
|
assert isinstance(result, Rejected)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"text", ["Approve", "APPROVE", "Approve.", "approve!", " Approve rev-2 "]
|
||||||
|
)
|
||||||
|
def test_approval_ignores_case_and_trailing_punctuation(text: str) -> None:
|
||||||
|
# A phone keyboard's "Approve" used to count as rejection feedback (#64).
|
||||||
|
assert isinstance(gate().evaluate("don", text, channel="transport"), Approved)
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_capitalised_stale_approval_is_still_refused() -> None:
|
||||||
|
result = gate().evaluate("don", "Approve REV-1", channel="transport")
|
||||||
|
assert isinstance(result, Ignored)
|
||||||
|
|
||||||
|
|
||||||
|
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"
|
||||||
@@ -5,6 +5,8 @@ import threading
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
from nisse_dev.cancel import STOPPED_RESULT, Cancel
|
from nisse_dev.cancel import STOPPED_RESULT, Cancel
|
||||||
from nisse_dev.canonical import AssistantText, ToolCall, ToolResult, UserMessage
|
from nisse_dev.canonical import AssistantText, ToolCall, ToolResult, UserMessage
|
||||||
from nisse_dev.control import (
|
from nisse_dev.control import (
|
||||||
@@ -207,7 +209,8 @@ class BlocksUntilStopped:
|
|||||||
return ExecutorOutcome("completed", "never stopped")
|
return ExecutorOutcome("completed", "never stopped")
|
||||||
|
|
||||||
|
|
||||||
def test_stop_reaches_work_running_on_the_worker(tmp_path: Path) -> None:
|
@pytest.mark.parametrize("word", ["stop", "Stop!", " STOP "])
|
||||||
|
def test_stop_reaches_work_running_on_the_worker(tmp_path: Path, word: str) -> None:
|
||||||
# The bug this design fixes: the work used to run on the transport's own
|
# The bug this design fixes: the work used to run on the transport's own
|
||||||
# thread, so no message, stop included, was read until it finished.
|
# thread, so no message, stop included, was read until it finished.
|
||||||
worker = SerialWorker()
|
worker = SerialWorker()
|
||||||
@@ -231,7 +234,7 @@ def test_stop_reaches_work_running_on_the_worker(tmp_path: Path) -> None:
|
|||||||
assert executor.running.wait(5)
|
assert executor.running.wait(5)
|
||||||
|
|
||||||
transport.receive("don", "status") # delivered while the milestone runs
|
transport.receive("don", "status") # delivered while the milestone runs
|
||||||
transport.receive("don", "stop")
|
transport.receive("don", word) # a phone may capitalise it (#64)
|
||||||
worker.call(lambda: None) # wait for the stop to be recorded
|
worker.call(lambda: None) # wait for the stop to be recorded
|
||||||
|
|
||||||
texts = [m.text for m in transport.outbound]
|
texts = [m.text for m in transport.outbound]
|
||||||
|
|||||||
Reference in new issue
Block a user