diff --git a/src/lembas/services/agent/session.py b/src/lembas/services/agent/session.py index 2369bed..44d7529 100644 --- a/src/lembas/services/agent/session.py +++ b/src/lembas/services/agent/session.py @@ -160,6 +160,37 @@ def _allow_for(chat: Chat) -> tuple[str, ...]: return tools_service.scoped_allow(chat) +def refresh(db: DBSession, agent: AgentContext) -> AgentContext: + """Re-read the two things a person can change while a reply is running. + + The mode and the chat's own allow list, and nothing else. Everything else on + the context is fixed for the life of a chat (the connection, the directory) + or is an instance setting nobody is editing mid-reply. + + Called once per round rather than once per reply. The reply-long snapshot it + replaces made both controls do nothing until the next turn: switching to + Auto during a long agent reply went on asking about every call, and + "Always allow this" was accepted, written to the row, and then ignored for + the rest of the reply that had just asked. Both look exactly like a control + that does not work, because for that reply they were. + + Once per *round* and not more often, because a round's calls are authorised + together: what is already queued was decided under the mode that was in + force when it was queued, and switching to Auto must not retroactively + approve it. Mutated in place -- `as_approved` copies field references, so a + replacement here would leave the approved copy of this round pointing at the + old one. + """ + chat = db.get(Chat, agent.chat_id) + if chat is None: + return agent + + agent.mode = chat.agent_mode if chat.agent_mode in policy.MODES else policy.MODE_MANUAL + instance = settings_store.agents(db) + agent.allow = (*(instance.get("allow_default") or ()), *_allow_for(chat)) + return agent + + def resolve(db: DBSession, chat: Chat, user: User | None) -> AgentContext | None: """This chat's agent setup, or None if it has none it can use. diff --git a/src/lembas/services/agent/tools.py b/src/lembas/services/agent/tools.py index 2c645f0..d51cc62 100644 --- a/src/lembas/services/agent/tools.py +++ b/src/lembas/services/agent/tools.py @@ -1015,14 +1015,23 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: name="plan_update", family=FAMILY_AGENT, description=( - "Keep the plan current while you carry it out. Call it when a " - "task finishes, when something you find changes what needs doing, " - "and when a task turns out to be unnecessary — as you go, not at " - "the end. The plan is what somebody reads to see where you are.\n" + "Keep the plan current while you carry it out. The plan is what " + "somebody reads to see where you are, so it has to be updated as " + "you go and not written up at the end.\n" "\n" - "Quote the ids from the plan in your prompt: tasks are t1, t2 and " - "so on, objectives are o1. This does not end your turn; carry on " - "with the work afterwards." + "Mark a task 'doing' when you start it and 'done' when you have " + "checked it actually works — not when you have written the code " + "for it. Use 'dropped' for a task that turned out to be " + "unnecessary, and say why in its note. Add tasks the plan did " + "not anticipate as you discover them.\n" + "\n" + "One call carries as many changes as you like: finishing one " + "task and starting the next is a single call, not two. Use the " + "ids exactly as they appear in the plan above — tasks are t1, " + "t2 and so on, objectives o1, phases p1.\n" + "\n" + "This does not end your turn and is not a progress report to " + "stop after. Carry straight on with the work." ), parameters={ "type": "object", diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 173f61c..876b73f 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -36,6 +36,7 @@ from lembas.services import metrics as metrics_service from lembas.services import prompts as prompts_service from lembas.services import tools as tools_service from lembas.services.agent import policy as agent_policy +from lembas.services.agent import session as agent_session from lembas.services.agent import tools as agent_tools from lembas.services.llm.openai_client import ( LLMError, @@ -632,6 +633,15 @@ async def _run(generation: Generation) -> None: # card, `policy.decide`, and the runner. See `_arguments_for`. arguments = _arguments_for(tool_context, calls) + # The mode and the chat's allow list, re-read. Both are things a + # person changes *while watching this reply*, and both were + # snapshotted for its whole life -- so switching to Auto went on + # asking about every call, and "Always allow this" was stored and + # then ignored until the next turn. Between rounds, never within + # one: what this round has already queued was decided under the mode + # that was in force when it was queued. + _refresh_agent(tool_context) + # Decided before anything runs, never during. A round's calls run # together under a semaphore, and four people-shaped pauses inside # that gather would queue behind each other invisibly -- see @@ -952,6 +962,23 @@ def _gave_up(generation, why: str) -> None: generation.touch() +def _refresh_agent(context) -> None: + """Pick up a mode or an allow-list change made while this reply is running. + + Its own short session: `ToolContext` is a session-free snapshot precisely so + nothing in a tool holds a live one, and this is one primary-key lookup plus + a settings read on a loop that is already doing network work per round. + + Silent on failure. A chat deleted mid-reply is not a reason to fail the + reply, and the reply is about to end anyway. + """ + agent = getattr(context, "agent", None) + if agent is None: + return + with contextlib.suppress(Exception), session_scope() as db: + agent_session.refresh(db, agent) + + def _wrap_up(generation, why: str, payload: dict, *, name: str = "budget") -> tuple[list, dict]: """A budget has run out. Withdraw the tools and ask for an answer. diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index 17f9b32..953f090 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -64,7 +64,19 @@ log = logging.getLogger(__name__) # so they are bounded whatever this is. What this bounds is the *fragments* # growing without anybody noticing -- so it is set above the sum of what those # budgets grant, with room for the plan and the memories beside them. -MAX_HARNESS_CHARS = 16000 +# +# 20,000 rather than 16,000, which the shipped set had grown to within 1,300 +# characters of. A ceiling this close to the content is one the next fragment +# crosses, and crossing it is silent: `assemble` cuts the tail, and the tail is +# the project's own AGENTS.md. `tests/test_harness.py` pins a margin now as well +# as a fit, so the room is a fact rather than a hope. +MAX_HARNESS_CHARS = 20000 + +# How much of the ceiling the shipped fragments may occupy at full budget. The +# rest is headroom for an administrator's own wording, which is the thing this +# limit exists to leave room for -- an override is usually longer than the +# default it replaces, not shorter. +HARNESS_MARGIN = 0.2 # How many attached filenames to name in the prompt. Enough to show what the # tags will look like, few enough that a chat with thirty files does not spend diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index f26e00f..fba9eaa 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -685,6 +685,42 @@ BUILTIN: tuple[Fragment, ...] = ( "not stop halfway to report progress and wait to be told to continue." ), ), + Fragment( + key="core.engineering", + label="Working on code", + group=GROUP_CORE, + order=112, + families=("agent",), + hint="An agent chat only, where there is a machine to check things on. " + "Every line here is about the gap between having written something and " + "knowing it works, which is the one a model closes by asserting rather " + "than by testing: the failure is not bad code, it is confident code " + "nobody ran. Deliberately about *conduct* rather than about any " + "language — style belongs to the project, and its own AGENTS.md is " + "where a project says so.", + default=( + "- Working on code, on this machine:\n" + " - Run what you write. A script you have not run is a draft, and " + "“this should work” is not a result. If you cannot run it, say that " + "plainly rather than implying you did.\n" + " - Find out how the project is built, tested and linted before " + "guessing — a README, a Makefile, a pyproject or package.json — and use " + "what is there rather than a command you would have chosen.\n" + " - Read a file before changing it, and match what is around you: the " + "naming, the error handling, the way the existing code is laid out. Code " + "that reads as though it came from somewhere else is a cost even when it " + "works.\n" + " - Change one thing, check it, then change the next. A dozen edits " + "checked at the end leave you without the one that broke it.\n" + " - Read what a failure actually says. Guessing at a fix and running it " + "again is slower than reading the error once, and it hides the cause.\n" + " - Do not silence a problem to make output clean: a broadened except, a " + "removed assertion or a skipped test buys a green run and keeps the bug.\n" + " - Say what you did and what you checked, including what you could not " + "check. If something is still broken, say so — being told a job is " + "finished when it is not is worse than being told it is hard." + ), + ), Fragment( key="core.objective", label="Working to an objective", @@ -1115,13 +1151,16 @@ BUILTIN: tuple[Fragment, ...] = ( "the point of it is being able to see where things are while they are " "still moving.", default=( - "- There is a plan for this work, set out below. Keep it current: call " - "plan_update when a task or a phase finishes, when something you find " - "changes what needs doing, and when a task turns out to be unnecessary. " - "Do it as you go rather than at the end — the plan is what somebody reads " - "to see where you are. If what you find makes the plan wrong rather than " - "merely incomplete, say so and ask with ask_user rather than quietly " - "planning something else." + "- There is a plan for this work, set out below. Keep it current with " + "plan_update as you go rather than at the end: mark a task “doing” when " + "you start it and “done” once you have checked it works, drop one that " + "turns out to be unnecessary, and add work the plan did not anticipate " + "when you find it. Several changes go in one call. The plan is what " + "somebody reads to see where you are, so a plan updated only at the end " + "is a report rather than a plan. Updating it is bookkeeping, not a " + "milestone — carry straight on with the work afterwards. If what you find " + "makes the plan wrong rather than merely incomplete, say so and ask with " + "ask_user rather than quietly planning something else." ), ), Fragment( diff --git a/src/lembas/web/static/js/composer.js b/src/lembas/web/static/js/composer.js index f68c771..9055b64 100644 --- a/src/lembas/web/static/js/composer.js +++ b/src/lembas/web/static/js/composer.js @@ -440,6 +440,22 @@ document.addEventListener("htmx:afterSwap", paint); document.addEventListener("htmx:afterSettle", paint); + /* And after the *request*, a frame later, which is the one that matters on + send. htmx fires afterSwap and afterSettle before afterRequest, and the + composer empties itself from `hx-on::after-request` -- so every repaint + above ran while the box still held the message, and the highlight stayed + behind over an empty field until the next keystroke repainted it. + + A frame later for two reasons: `form.reset()` fires its `reset` event + *before* the fields are actually cleared, and reading the value in the same + turn would paint the text that is about to disappear. */ + function repaintSoon() { + if (window.requestAnimationFrame) window.requestAnimationFrame(paint); + else setTimeout(paint, 0); + } + document.addEventListener("htmx:afterRequest", repaintSoon); + document.addEventListener("reset", repaintSoon); + document.addEventListener("click", function (event) { if (!event.target.closest(".composer-menu") && !event.target.closest("[data-composer-input]")) hide(); diff --git a/tests/test_agent_mode.py b/tests/test_agent_mode.py index 45a4ce0..fdb8b16 100644 --- a/tests/test_agent_mode.py +++ b/tests/test_agent_mode.py @@ -233,3 +233,86 @@ def test_an_unrecognised_mode_is_ignored(client: TestClient, db, registered, wan db.refresh(chat) assert chat.agent_mode == policy.MODE_EDIT + + +# --- Changing it while a reply is running --------------------------------------- +def test_the_mode_is_re_read_between_rounds(client: TestClient, db, registered): + """The reported bug. The mode was snapshotted for the whole reply, so + switching to Auto during a long agent reply went on asking about every + call -- which looks exactly like a control that does not work, because for + that reply it was one. + + Between rounds and not within one: a round's calls are authorised together, + and switching must not retroactively approve what is already queued. + """ + from lembas.db.models import User + from lembas.services.agent import session as agent_session + + chat = _agent_chat(db, mode=policy.MODE_MANUAL) + user = db.scalars(select(User)).first() + agent = agent_session.resolve(db, chat, user) + assert agent.mode == policy.MODE_MANUAL + + client.patch(f"/api/chats/{chat.id}", data={"agent_mode": policy.MODE_AUTO}) + # The route wrote through its own session; this one still holds the row it + # loaded. `refresh` opens a fresh session in the real path, so this is the + # test catching up rather than the behaviour under test. + db.expire_all() + + agent_session.refresh(db, agent) + assert agent.mode == policy.MODE_AUTO + + +def test_always_allow_reaches_the_reply_that_asked(client: TestClient, db, registered): + """The same bug, in the place nobody reported because it is quieter: the + verdict was accepted, written to the row, and then ignored for the rest of + the reply that had just asked about it.""" + from lembas.db.models import User + from lembas.services.agent import session as agent_session + + chat = _agent_chat(db, mode=policy.MODE_MANUAL) + user = db.scalars(select(User)).first() + agent = agent_session.resolve(db, chat, user) + assert "pytest" not in agent.allow + + chat.scope_json = {**(chat.scope_json or {}), "allow": ["pytest"]} + db.commit() + + agent_session.refresh(db, agent) + assert "pytest" in agent.allow + + +def test_refreshing_keeps_what_this_reply_has_read(client: TestClient, db, registered): + """`read_paths` is what `file_edit` checks before applying a patch, and it + is a fact about this reply rather than about the row. Mutating in place is + what keeps it -- and keeps the approved copy of a round, which holds field + references rather than a copy.""" + from lembas.db.models import User + from lembas.services.agent import session as agent_session + + chat = _agent_chat(db, mode=policy.MODE_MANUAL) + user = db.scalars(select(User)).first() + agent = agent_session.resolve(db, chat, user) + agent.read_paths.add("/project/main.py") + approved = agent.as_approved() + + agent_session.refresh(db, agent) + + assert "/project/main.py" in agent.read_paths + assert "/project/main.py" in approved.read_paths + + +def test_an_unknown_mode_on_the_row_refreshes_to_manual(client: TestClient, db, registered): + """A row that predates a rename has to fail towards asking, here as much as + in `resolve`.""" + from lembas.db.models import User + from lembas.services.agent import session as agent_session + + chat = _agent_chat(db, mode=policy.MODE_AUTO) + user = db.scalars(select(User)).first() + agent = agent_session.resolve(db, chat, user) + + chat.agent_mode = "reckless" + db.commit() + agent_session.refresh(db, agent) + assert agent.mode == policy.MODE_MANUAL diff --git a/tests/test_harness.py b/tests/test_harness.py index ce49e67..2f20236 100644 --- a/tests/test_harness.py +++ b/tests/test_harness.py @@ -477,3 +477,14 @@ def test_the_shipped_defaults_fit_under_the_ceiling(db, owner): assert not out.endswith("…"), f"the preamble was truncated at {len(out):,} characters" assert "A" * 100 in out, "the project's own instructions were cut off entirely" assert "L" * 100 in out, "the project listing was cut off" + + # Fitting is not enough. It fitted with 1,300 characters to spare once, and + # a ceiling that close to the content is one the next fragment crosses -- + # silently, and by cutting the tail, which is the project's own AGENTS.md. + # The margin is also what an administrator's own wording goes in: an + # override is usually longer than the default it replaces. + room = harness.MAX_HARNESS_CHARS - len(out) + assert room >= harness.MAX_HARNESS_CHARS * harness.HARNESS_MARGIN, ( + f"only {room:,} characters of headroom left under " + f"{harness.MAX_HARNESS_CHARS:,}; raise the ceiling or shorten a fragment" + )