diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index ab34838..62307b4 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "0.6.4" +__version__ = "0.6.5" diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index 58b9a5d..6fa7b53 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -1584,6 +1584,14 @@ async def answer_interaction( form = await request.form() verdict = str(form.get("verdict") or "").strip() + # Why they refused, in their own words. A card-level field rather than a + # `text.` one: the card covers everything in the round, so one reason + # answers the round -- and on an approval card `text.` already means a + # corrected command, which is a different thing arriving in the same shape. + # Only read on a refusal, so a reason typed and then abandoned by pressing + # Allow cannot travel with a permission. + reason = str(form.get("reason") or "").strip() if verdict == interaction.DENY else "" + # Gathered in two passes because one field can now arrive several times: a # question the model marked `multiple` is checkboxes, and every ticked one # posts under the same name. A `setdefault` would keep the first and lose @@ -1642,6 +1650,7 @@ async def answer_interaction( interaction_id, verdict=verdict, answers=answers, + reason=reason, ) response = Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 731e627..6d79ef1 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -288,6 +288,7 @@ def answer( *, verdict: str = "", answers: dict[str, str] | None = None, + reason: str = "", ) -> bool: """Resolve whichever running reply is parked on this interruption. @@ -300,7 +301,7 @@ def answer( if generation.chat_id != chat_id or pending is None or pending.id != interaction_id: continue outcome = verdict if verdict in _VERDICTS else interaction.ANSWER - return pending.resolve(outcome, answers=answers) + return pending.resolve(outcome, answers=answers, reason=reason) return False @@ -1629,6 +1630,20 @@ def _not_allowed(item: interaction.Item, reply: interaction.Reply) -> ToolOutcom Told plainly, and told to stop rather than to try again: a model that reads "not allowed" as "not allowed *that way*" will spend the rest of the reply looking for a way round, which is the opposite of what the refusal meant. + + A refusal that came with a reason is told differently, and the difference is + the whole point of offering the box. "Ask what they would prefer" is the + right thing to say to a model that has been given nothing to go on, and + exactly the wrong thing to say to one that has just been told -- it spends a + round asking a question whose answer is on the screen above it. So when there + is a reason the model is pointed at it and told to carry on from it, and the + "do not look for a way round" half is kept, because that half is about the + refusal and holds either way. + + The reason is the *reader's* words, not a model's and not a machine's, which + is why it is stated as theirs and needs no fence: this is the one thing in a + tool result that is not untrusted. It is bounded at `MAX_REASON_CHARS` when + the reply is built, so nothing here has to think about length. """ event = { "name": item.tool_name, @@ -1643,6 +1658,17 @@ def _not_allowed(item: interaction.Item, reply: interaction.Reply) -> ToolOutcom "about to do and why.", {**event, "status": "error", "error": "Not answered."}, ) + if reply.reason: + return ToolOutcome( + f"They declined this, and said why:\n\n{reply.reason}\n\n" + "Take that as their instruction and carry on from it. Do not try the " + "same thing another way, and do not ask them to repeat what they have " + "just told you.", + # On the event as well as in the result, so somebody scrolling back + # through the transcript can see why a step was refused rather than + # only that it was. + {**event, "status": "error", "error": f"Declined: {reply.reason}"}, + ) return ToolOutcome( "They declined this. Do not try it another way — say what you were " "going to do and ask what they would prefer.", diff --git a/src/lembas/services/interaction.py b/src/lembas/services/interaction.py index d47cbeb..5952494 100644 --- a/src/lembas/services/interaction.py +++ b/src/lembas/services/interaction.py @@ -66,6 +66,12 @@ OTHER = "__other__" # card somebody is meant to read at a glance. MAX_OPTION_CHARS = 240 +# How much of a refusal's reason is carried back to the model. Generous, because +# this is the reader saying what they want instead and truncating that mid-clause +# is worse than the tokens it saves -- but bounded, because it lands in a tool +# result inside a request that already has a window to fit in. +MAX_REASON_CHARS = 2000 + @dataclass(frozen=True) class Option: @@ -143,7 +149,9 @@ class Interruption: def kind(self) -> str: return KIND_QUESTION if any(i.kind == KIND_QUESTION for i in self.items) else KIND_APPROVAL - def resolve(self, outcome: str, *, answers: dict[str, str] | None = None) -> bool: + def resolve( + self, outcome: str, *, answers: dict[str, str] | None = None, reason: str = "" + ) -> bool: """Complete this pause. Idempotent -- a second answer is ignored. Returns whether this call was the one that answered it, which is what @@ -152,7 +160,13 @@ class Interruption: """ if self._future is None or self._future.done(): return False - self._future.set_result(Reply(outcome=outcome, answers=dict(answers or {}))) + self._future.set_result( + Reply( + outcome=outcome, + answers=dict(answers or {}), + reason=reason.strip()[:MAX_REASON_CHARS], + ) + ) return True @@ -162,11 +176,19 @@ class Reply: `answers` is keyed by `Item.key`, so a card carrying four questions comes back as four answers in one go. An approval carries none: the verdict is - the whole of it. + the whole of it -- except for `reason`. + + `reason` is why the reader refused, in their own words, and it belongs to + the *card* rather than to an item. The card already covers everything in the + round for the reason `interaction` opens with, one verdict answers the lot, + and somebody who says "not in that directory" is saying it about the round. + Keeping it off `answers` also keeps it clear of `text.`, which on an + approval card already means something else entirely -- a corrected command. """ outcome: str answers: dict[str, str] = field(default_factory=dict) + reason: str = "" @property def permitted(self) -> bool: @@ -239,6 +261,7 @@ __all__ = [ "KIND_QUESTION", "MAX_OPTIONS", "MAX_QUESTIONS", + "MAX_REASON_CHARS", "PERMITTED", "Interruption", "Item", diff --git a/src/lembas/web/static/css/chat.css b/src/lembas/web/static/css/chat.css index 90be814..0e43f7b 100644 --- a/src/lembas/web/static/css/chat.css +++ b/src/lembas/web/static/css/chat.css @@ -649,6 +649,24 @@ .interaction__write { display: flex; gap: var(--sp-2); flex: 1 1 16rem; min-width: 0; } .interaction__write .input { flex: 1; min-width: 0; } +/* Saying why, instead of only saying no. `flex: 1 0 100%` takes the whole row + of the wrapping action bar rather than squeezing in beside Allow: this is a + sentence being written, not a fourth button. */ +.interaction__deny { + flex: 1 0 100%; + display: flex; + flex-direction: column; + gap: var(--sp-2); + min-width: 0; +} +.interaction__deny-input { width: 100%; resize: vertical; } +.interaction__deny-actions { + display: flex; + flex-wrap: wrap; + gap: var(--sp-2); + align-items: center; +} + /* --- Stop, notes and editing ---------------------------------------------- */ .msg__status { font-size: var(--text-xs); color: var(--ink-faint); font-style: italic; } .msg__status:empty { display: none; } diff --git a/src/lembas/web/templates/chat/_interaction.html b/src/lembas/web/templates/chat/_interaction.html index 4945a6a..b668e24 100644 --- a/src/lembas/web/templates/chat/_interaction.html +++ b/src/lembas/web/templates/chat/_interaction.html @@ -152,16 +152,59 @@ {% endfor %} -
+ {# + Refusing, with or without saying why. + + A bare "Don't" tells the model it was refused and nothing else, so it + does the only sensible thing left and asks what you would rather -- which + costs a round to say something you already knew when you pressed the + button. The box is how you skip that round: "not in that directory, use + /srv" is a refusal and an instruction in one, and `_not_allowed` changes + what it tells the model when there is one. + + Both halves are always in the DOM with one hidden, for the reason the + edit box above gives: a field created by a click is a field that submits + nothing when the click handler fails. And the textarea is disabled while + hidden, so a card refused plainly cannot post an empty `reason` that + would have to be told apart from one somebody cleared. + #} +
- + + +
+ + {# Never type="password", for the reason the "Something else" box + above states: a chat transcript is not a place to keep secrets. #} + {# `--deny-input` and not `--reason`: `.interaction__reason` already + means *our* reason for stopping, which is the opposite direction. #} + +
+ + +
+
{% endif %} diff --git a/tests/test_agent_command_edit.py b/tests/test_agent_command_edit.py index 9cc8c2c..36674b1 100644 --- a/tests/test_agent_command_edit.py +++ b/tests/test_agent_command_edit.py @@ -105,7 +105,7 @@ def _shell_call(command: str, *, call_id: str = "c1") -> dict: } -async def _authorise_with(context, calls, *, answers, verdict=interaction.ALLOW): +async def _authorise_with(context, calls, *, answers, verdict=interaction.ALLOW, reason=""): """Run `_authorise` and answer the card it puts up.""" generation = generation_service.Generation(chat_id="x", message_id="y") arguments = generation_service._arguments_for(context, calls) @@ -120,7 +120,7 @@ async def _authorise_with(context, calls, *, answers, verdict=interaction.ALLOW) await asyncio.sleep(0.01) pending = generation.pending - pending.resolve(verdict, answers=answers) + pending.resolve(verdict, answers=answers, reason=reason) decided, allowed, edited = await task return arguments, decided, allowed, edited, pending @@ -223,6 +223,47 @@ async def test_declining_ignores_the_edit(db, user_id, machine): assert arguments[0]["command"] == "pytest" +async def test_a_refusal_with_a_reason_answers_every_call_in_the_round( + db, user_id, machine +): + """One card covers the round, so one reason answers the round. + + That is the design claim worth pinning: the reader said "not on production" + about what they were shown, and what they were shown was both commands. A + reason attached to one of them would be the card describing something other + than what it asked about. + """ + context = _context(db, user_id, machine) + calls = [_shell_call("pytest", call_id="c1"), _shell_call("make deploy", call_id="c2")] + + _arguments, decided, allowed, _edited, _ = await _authorise_with( + context, + calls, + answers={}, + verdict=interaction.DENY, + reason="not on production", + ) + + assert allowed == set(), "nothing ran" + assert set(decided) == {0, 1}, "and every call was answered without the runner" + for outcome in decided.values(): + assert "not on production" in outcome.content + assert outcome.event["error"] == "Declined: not on production" + + +async def test_a_refusal_with_no_reason_is_unchanged(db, user_id, machine): + """The path everything already took, still taking it.""" + context = _context(db, user_id, machine) + calls = [_shell_call("pytest")] + + _arguments, decided, _allowed, _edited, _ = await _authorise_with( + context, calls, answers={}, verdict=interaction.DENY + ) + + assert decided[0].event["error"] == "Declined." + assert "ask what they would prefer" in decided[0].content + + async def test_only_the_edited_call_in_a_round_is_changed(db, user_id, machine): """One card covers the whole round, and the answers are keyed per item.""" context = _context(db, user_id, machine) diff --git a/tests/test_agent_interaction.py b/tests/test_agent_interaction.py index 497ab74..d741027 100644 --- a/tests/test_agent_interaction.py +++ b/tests/test_agent_interaction.py @@ -766,7 +766,7 @@ def test_the_endpoint_gathers_every_answer_at_once(client, db, registered, user_ chat_id, message_id = _chat_that_can_ask(db, user_id) seen: dict = {} - def capture(chat, interaction_id, *, verdict="", answers=None): + def capture(chat, interaction_id, *, verdict="", answers=None, reason=""): seen["chat"] = chat seen["id"] = interaction_id seen["verdict"] = verdict @@ -811,7 +811,7 @@ def test_the_endpoint_passes_a_verdict_through_untouched(client, db, registered, from lembas.api import chats as chats_api original = chats_api.generation_service.answer - chats_api.generation_service.answer = lambda c, i, *, verdict="", answers=None: ( + chats_api.generation_service.answer = lambda c, i, *, verdict="", answers=None, reason="": ( seen.update(verdict=verdict, answers=answers) or True ) try: @@ -921,8 +921,9 @@ def test_several_ticked_answers_all_come_back(client, db, registered, user_id): from lembas.api import chats as chats_api original = chats_api.generation_service.answer - chats_api.generation_service.answer = lambda chat, iid, *, verdict="", answers=None: ( - seen.update(answers=answers) or True + chats_api.generation_service.answer = ( + lambda chat, iid, *, verdict="", answers=None, reason="": seen.update(answers=answers) + or True ) try: client.post( @@ -944,8 +945,9 @@ def test_something_else_ticked_with_an_empty_box_says_nothing(client, db, regist from lembas.api import chats as chats_api original = chats_api.generation_service.answer - chats_api.generation_service.answer = lambda chat, iid, *, verdict="", answers=None: ( - seen.update(answers=answers) or True + chats_api.generation_service.answer = ( + lambda chat, iid, *, verdict="", answers=None, reason="": seen.update(answers=answers) + or True ) try: client.post( @@ -965,8 +967,9 @@ def test_a_ticked_option_and_something_else_come_back_together(client, db, regis from lembas.api import chats as chats_api original = chats_api.generation_service.answer - chats_api.generation_service.answer = lambda chat, iid, *, verdict="", answers=None: ( - seen.update(answers=answers) or True + chats_api.generation_service.answer = ( + lambda chat, iid, *, verdict="", answers=None, reason="": seen.update(answers=answers) + or True ) try: client.post( @@ -977,3 +980,168 @@ def test_a_ticked_option_and_something_else_come_back_together(client, db, regis chats_api.generation_service.answer = original assert seen["answers"] == {"q0": "Tea, or juice"} + + +# --- Refusing, and saying why -------------------------------------------------- +# A bare "Don't" tells the model it was refused and nothing else, so it asks what +# you would rather -- a whole round spent on something you knew when you pressed +# the button. The box is how that round is skipped, and these are the two halves +# that have to hold: the reason reaches the model, and it changes what the model +# is told to do next. +def _approval(key: str = "a0", **kwargs) -> interaction.Item: + return interaction.Item( + index=0, + key=key, + kind=interaction.KIND_APPROVAL, + tool_name="shell_run", + title="Run a command", + detail="rm -rf build", + **kwargs, + ) + + +async def test_a_refusal_can_carry_a_reason(): + pause = interaction.build("p", [_approval()], timeout=5) + pause.resolve(interaction.DENY, reason=" not in that directory, use /srv ") + + reply = await pause._future + + assert reply.permitted is False + assert reply.reason == "not in that directory, use /srv", "trimmed, as typed" + + +async def test_a_plain_refusal_carries_none(): + """Which is what keeps the two wordings apart downstream.""" + pause = interaction.build("p", [_approval()], timeout=5) + pause.resolve(interaction.DENY) + + assert (await pause._future).reason == "" + + +async def test_a_reason_is_bounded(): + """It lands in a tool result inside a request that has a window to fit in.""" + pause = interaction.build("p", [_approval()], timeout=5) + pause.resolve(interaction.DENY, reason="x" * (interaction.MAX_REASON_CHARS + 500)) + + assert len((await pause._future).reason) == interaction.MAX_REASON_CHARS + + +def test_the_reason_reaches_the_model_and_redirects_it(): + """The instruction has to change, not just the text. "Ask what they would + prefer" is right for a model given nothing to go on and exactly wrong for one + that has just been told -- it spends a round asking a question whose answer is + on the screen above it.""" + outcome = generation_service._not_allowed( + _approval(), interaction.Reply(outcome=interaction.DENY, reason="use /srv instead") + ) + + assert "use /srv instead" in outcome.content + assert "carry on from it" in outcome.content + assert "do not ask them to repeat" in outcome.content.lower() + assert "ask what they would prefer" not in outcome.content + # The half that holds either way: a refusal is not an invitation to find + # another route to the same thing. + assert "Do not try the same thing another way" in outcome.content + + +def test_a_refusal_without_a_reason_is_worded_as_it_always_was(): + outcome = generation_service._not_allowed( + _approval(), interaction.Reply(outcome=interaction.DENY) + ) + + assert "ask what they would prefer" in outcome.content + assert outcome.event["error"] == "Declined." + + +def test_the_reason_is_in_the_transcript_too(): + """So somebody scrolling back sees why a step was refused rather than only + that it was.""" + outcome = generation_service._not_allowed( + _approval(), interaction.Reply(outcome=interaction.DENY, reason="wrong host") + ) + + assert outcome.event["error"] == "Declined: wrong host" + + +def test_an_unanswered_card_is_not_a_refusal_with_a_reason(): + """Nobody said anything, so there is nothing to pass on -- and the wording + has to stay the one about nobody answering.""" + outcome = generation_service._not_allowed( + _approval(), interaction.Reply(outcome=interaction.EXPIRED) + ) + + assert "Nobody answered" in outcome.content + assert outcome.event["error"] == "Not answered." + + +def test_the_card_offers_the_box_and_names_the_field(client, db, registered, user_id): + pause = interaction.Interruption(id="p1", items=(_approval(),)) + + html = _render(pause) + + assert "Give reason" in html + assert 'name="reason"' in html + # Both halves in the DOM with one hidden, for the reason the edit box gives: + # a field created by a click submits nothing when the click handler fails. + assert 'x-show="explaining"' in html + # Disabled while hidden, so a card refused plainly cannot post an empty + # `reason` that would have to be told apart from one somebody cleared. + assert ':disabled="!explaining"' in html + + +def test_the_question_card_offers_no_deny_box(): + """It is not a refusal, and it has its own free-text row already.""" + html = _render(interaction.Interruption(id="p1", items=(_item(),))) + + assert "Give reason" not in html + assert 'name="reason"' not in html + + +def test_the_endpoint_passes_the_reason_on(client, db, registered, user_id): + chat_id, _message_id = _chat_that_can_ask(db, user_id) + seen: dict = {} + + from lembas.api import chats as chats_api + + original = chats_api.generation_service.answer + chats_api.generation_service.answer = ( + lambda c, i, *, verdict="", answers=None, reason="": seen.update( + verdict=verdict, reason=reason + ) + or True + ) + try: + client.post( + f"/api/chats/{chat_id}/interaction/p1", + data={"verdict": "deny", "reason": "not on production"}, + ) + finally: + chats_api.generation_service.answer = original + + assert seen == {"verdict": "deny", "reason": "not on production"} + + +def test_a_reason_never_travels_with_a_permission(client, db, registered, user_id): + """Typed into the box, then Allow pressed instead. Reading it there would put + the reader's "no, because…" onto a call that went ahead.""" + chat_id, _message_id = _chat_that_can_ask(db, user_id) + seen: dict = {} + + from lembas.api import chats as chats_api + + original = chats_api.generation_service.answer + chats_api.generation_service.answer = ( + lambda c, i, *, verdict="", answers=None, reason="": seen.update( + verdict=verdict, reason=reason + ) + or True + ) + try: + client.post( + f"/api/chats/{chat_id}/interaction/p1", + data={"verdict": "allow", "reason": "I changed my mind"}, + ) + finally: + chats_api.generation_service.answer = original + + assert seen == {"verdict": "allow", "reason": ""}