diff --git a/CHANGELOG.md b/CHANGELOG.md index c857b62..54e45a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,42 @@ for 1.0.0 have something to be assembled from. ## Unreleased +## 0.9.9 + +**The first of five audit passes before 1.0.0** — everything that landed between +0.8.1 and 0.9.8 read as a whole rather than one feature at a time. This one is +the main logic, the harness, and every instruction a model is given. + +- Fixed: **every model was told the time in a zone with no name.** On any + account that had not chosen a timezone — which is the default state of every + account — the date line shipped as "Times the person gives you are in + unless they say otherwise", on every request. The code claimed in two places + that the line disappeared instead. It never had. +- Fixed: **the prompt preview could not show most of what it previews.** Eleven + fragments are gated on things that only exist once there is a real chat, and + the preview has none — so the whole agent surface, both scheduling fragments + and the helper warning were missing from it whatever you ticked. Editing + `tool.agent` and pressing preview showed a system message without `tool.agent` + in it, and nothing said so. Two new controls come with the fix: what kind of + chat to preview as, and which agent mode. +- Fixed: **a model in Plan mode was told to use a tool it did not have.** + `plan_update` is withdrawn in that mode in favour of `plan_submit`, but its + guidance appeared whenever a plan existed — directly under the line saying + anything not in your tool list does not exist. +- Fixed: **reading one knowledge document could fill the whole context window.** + Every other reader caps what it returns and says so; this one returned the + document whole, and its description said "in full", so it did exactly what it + claimed. A long PDF is now cut at 40,000 characters with the model told. +- Fixed: **the guidance about helpers on a machine was wrong in both + directions.** It denied that a helper can write files, which is a documented + option of the tool beside it, and it named seven of the twenty-three commands + a helper may run — so a model avoided commands it was allowed to use. Both are + now checked against the real list and the real schema by tests, because prose + and a constant drift the moment one is edited alone. +- The tool description for delegating no longer claims a helper gets "the same + tools". It gets deliberately fewer, and sizing a task against the wrong set is + how a whole phase gets planned around something that will refuse it. + - The Updates page notices when the update helper on a host was installed for a **different channel** than the page follows. It is declared in two places — `lembas.env` and the systemd unit — and only the installer writes both, so diff --git a/docs/notes/audit-0.9.md b/docs/notes/audit-0.9.md new file mode 100644 index 0000000..f1e7eea --- /dev/null +++ b/docs/notes/audit-0.9.md @@ -0,0 +1,231 @@ +# The 0.9 audit + +A working document, written during the pass over everything that landed between +`0.8.1` and `0.9.8` -- 16 commits, 163 files, +16,795 / −1,608. It exists so the +findings survive the session that produced them, and so the security and testing +stages have something to work from rather than re-deriving it. + +**This file is temporary.** Everything in it either becomes a `CHANGELOG.md` +line, a paragraph in `CLAUDE.md`, or an entry under "After 1.0.0" in `PLAN.md`. +It is deleted before the release. + +Each finding carries a disposition: + +- **fixed** -- landed, with a test +- **later** -- real, but the fix changes what a feature does, so it is the + user's call and it is written up rather than done +- **stated** -- correct as it stands, and recorded so the next audit does not + spend an hour rediscovering that it is deliberate + +--- + +## The harness and the instruction prompts + +### {{timezone}} shipped as a hole — *fixed* + +`clock.name_for` returns `""` for anybody who has never chosen a zone, which is +the default state of every account. `substitute` drops a line only when the +whole line is **blank** after expansion, and this variable sits inside a +sentence, so every such request carried: + +``` +- Times the person gives you are in unless they say otherwise. +``` + +Both the comment at `harness.py:191` and the fragment's own hint asserted the +line disappeared. Neither had ever been true. The sibling call site +(`schedule/compile.py:102`) has always had the fallback. + +Worth noting *why* the fallback is right rather than merely convenient: `stamp` +is `clock.now_for(user)`, which already falls back to the server's zone, and +`{{now}}` already prints its offset. The times were already in that zone. +Withholding the label from a value the model had been given was not restraint. + +### The admin preview could not show most of what it previews — *fixed* + +`admin_prompts.py` built its variables with `context_variables(db, user, [], None)` +— no tools, no chat — and backfilled six names. Every other `requires` gate +stayed empty, so **eleven fragments could never appear in a preview whatever an +administrator ticked**: the whole agent surface, both scheduling fragments and +the helper warning. Editing `tool.agent` and pressing preview showed a system +message with `tool.agent` missing from it, silently. + +Fixed with sample values, which is what `SAMPLE_DOCUMENTS` already did for +attachments, plus two controls the "Tools offered" row could not reach — "This +chat is" (ordinary / scheduled task / helper) and an agent-mode select. The +samples are gated **exactly as `context_variables` gates the real values**, +because a preview that admits a fragment the real request would not is worse +than one that omits it. + +### Fragments named tools that had been withdrawn — *fixed for Plan mode, later for the rest* + +`resolve_tools` filters per **tool**; `harness._families` re-derives gates per +**family**. So a family survives on its readers while its writers are gone, and +the family-gated fragment still names them — under `core.tool_list` saying +"Anything not named there does not exist here — calling it costs a round and +returns nothing." + +**Fixed:** Plan mode. `agent/tools.py:1185` withdraws `plan_update` there, and +`tool.plan_update` was gated on `{{plan}}`, which is set whenever a plan exists +in any mode. A new `plan_editable` variable — the plan text, blank in Plan mode +— now gates it. Writing the test found a second instance the audit had missed: +`context.plan` also said "Change it with plan_update", so that sentence moved to +the fragment whose job it is. + +**Later:** a read-only helper keeps seven fragments naming fifteen withdrawn +write tools (`notes_create`, `report_write`, `file_write`, `skill_create`, the +three schedule writers…). The principled fix is the split `tool.skills` / +`tool.skills_write` already demonstrates, applied to `notes`, `report`, +`schedule` and `agent_edits` — four more fragment pairs. That is a prompt +restructure rather than a bug fix and it is proposed for after 1.0.0. The cost +today is bounded: `{{tool_names}}` is authoritative and the model has it, so a +helper wastes at most one round discovering a writer is absent. + +### Three tool descriptions disagreed with their runners — *all fixed* + +- **`subagent_run` opened "a second model with the same tools"** and contradicted + itself two sentences later. The child is forced to `ask:False`, + `subagent:False`, `write:False` with commands pinned to `SAFE_COMMANDS`. The + leading clause is what a model reads when sizing a task. +- **`knowledge_get` returned `extracted_text` whole** while every sibling reader + capped and said so — `fetch` at 20k, `file_read`, the memories block, the + skill index, the project listing. `MAX_EXTRACTED_CHARS` defaults to 120,000, + so one call on a long PDF filled an ordinary window with nothing reporting it. + Its description said "in full", which is why this read as correct: the tool did + exactly what it claimed. Now `MAX_DOCUMENT_CHARS = 40_000`, cut with the model + told, which is what `fetch` and `file_read` both do. +- **`_run_ask_user` read `args["question"]`**, singular, against a schema + declaring `questions` and a list — so its event always carried an empty + `query`. Harmless only because the path is documented unreachable, which is + exactly why nothing caught it. + +### Fragment text that was false — *fixed* + +`tool.subagent_agent` said a helper "reads and reports … and nothing else, in +every mode" beside a `write` parameter that makes one write files, and named +**seven** of the **twenty-three** commands in `SAFE_COMMANDS` — so a model +avoided commands it was allowed, which costs nothing visible and is therefore +never reported. Both halves are now pinned by tests against the real list and +the real schema, because prose and a constant drift the moment one is edited +alone. + +The distinction the wording now has to carry, and did not before: **commands** +are fixed to the read-only list in every mode, since an unattended chat cannot +approve anything; **files** may be written, but only by a helper asked for with +`write` and only from Edit or Auto. + +### `tool.background` promises a notification that can be off — *later* + +"When a background job finishes you are told in a new turn" has no `requires` +for `agents.background_notify`, and the runner itself branches on that flag and +says "Check on it with `job_output`" instead. One fragment, two behaviours. The +fix is a variable and a gate, the same shape as `plan_editable`; deferred only +because it wants the split above rather than a third one-off. + +### `FAMILY_ASK` has no fragment at all — *later* + +Every other family in `FAMILIES` contributes one. `ask_user` is offered in +nearly every chat and **all** of its guidance lives in its schema description, +which is the one thing an administrator cannot edit. Proposed rather than done: +it is new prompt text, and new prompt text in an audit stage is how an audit +turns into a feature. + +### Smaller, all *stated* + +- **`model_name` is resolved on every request and rendered by no shipped + fragment.** It is documented in `VARIABLES` and available to an + administrator's override, so it is an escape hatch rather than dead work. The + cost is one small `select` on a table with tens of rows. +- **Four pairs of fragments share an `order`**, which undercuts "sparse so a + custom tool can be slotted between two of these". Ties break alphabetically + and deterministically. One has a readable consequence: `context.schedule` + sorts below `context.memories`, so a scheduled task's own instruction appears + under the reader's remembered facts. +- **`core.rounds` is dark on a stock install**, since `DEFAULT_CHAT_ROUNDS = 0`. + Deliberate — `core.keep_working` is its complement and exactly one ever fires. +- **A row-backed fragment with empty `guidance` can never be selected**, so for + those "never configured" and "deliberately switched off" are the same state — + the one distinction the absent-vs-empty convention exists to preserve. + +--- + +## Controls + +The four sweeps `CLAUDE.md` is shaped around were run by hand over every +template and **all four came back clean**: 68 htmx verbs against 179 registered +routes with zero method mismatches, 88 plain form actions and 30 +`fetch`/`htmx.ajax` call sites with zero, every verb-carrying element declaring +its own `hx-target` or `hx-swap="none"` checked against every ancestor, no empty +verb attributes, both `form="…"` sites correct, and every `hx-target`, +`hx-include` and `from:` selector resolving to an id that exists. + +Making each one a **test that sweeps** is Stage 2's work. A one-off audit that +finds nothing is worth much less than the same audit made permanent. + +## Capabilities that are built, documented and unreachable — *Stage 2* + +The `Model.params_json` and `library.share` family: the code is right, the route +validates, and nothing can reach it. + +- **Folder nesting.** `PATCH /api/folders/{id}` handles `parent_id` with a cycle + guard and `MAX_DEPTH = 8`, `partials/_folder.html` recurses, and `README.md` + advertises "arbitrarily nested". No template or script submits `parent_id`. +- **Moving a chat into a folder.** `api/chats.py` handles `folder_id` on PATCH; + only the composer submits it, at creation. `chat/_composer.html` even carries + a comment reasoning about "the only way into a folder was to make the chat + elsewhere and move it". +- **`Connection.extra_headers_json`** is read into every LLM request and written + by no form, so its documented use — OpenRouter's `HTTP-Referer` — is + unreachable. *later*: nothing advertises it, so nothing is currently untrue. + +Written and never read: `Chat.compacted_at`, `User.last_login_at`, +`Schedule.last_fire_at`, `Schedule.compiled_at`. Neither read nor written: +`general.require_approval` (its comment says "reserved"), and the `back` form +parameter on two `admin_models` routes. + +*Stated:* `Message.parent_id` and `content_parts_json` are deliberate forward +-looking columns; `Chat.archived` is already recorded in `PLAN.md` as a column +nothing surfaces. + +## Documentation — *Stage 2* + +- `README.md` is stale by six phases. "Planned: Image generation · OCR · + semantic search" — two of the three shipped. Nothing under Features mentions + schedules, reports, subagents, branding, quotas, web push, updates, Docker or + LXC. +- `CLAUDE.md` says "Three topics live in `docs/notes/`" and lists seven. +- `CLAUDE.md` says 2088 tests; there are 2092 before this stage. +- `tests/__pycache__/test_zz_{dump,live}*.pyc` are stale bytecode for two files + that no longer exist. + +## For the security stage + +Carried forward rather than answered here: + +- the three hand-rolled redirect loops each re-run `check_url` per hop + (confirmed); does each also drop the secret when a hop leaves its origin? +- `admin_tools.py` calls `check_url(server.url, allow_private=True)` + unconditionally at save — a syntax check, with the per-row flag governing at + call time. Confirm that reading is right. +- the subagent restriction is the one gate that fails open unattended. + `_child_scope` forcing `ask:False`, `subagent:False`, `write:False` and + pinning `allow` to `SAFE_COMMANDS` is confirmed present; the question is + whether every path reaches it. +- `pyproject.toml` pins no upper bounds and `deploy/update.sh` runs + `pip install -e` on every update, so a breaking upstream release arrives on a + button press. pip's `only-if-needed` default limits the blast radius, which is + why this is a note rather than an emergency. + +## For the testing stage + +- `api/library.py`: 28 routes, effectively zero HTTP coverage. Highest exposure + — auth, ownership and permission gates all live at that boundary. +- No real `sync_schema` upgrade test exists. `conftest.py` runs `create_all` + then `sync_schema`, so the differ is only ever exercised as a no-op. Hard rule + 4 has no other safety net. +- `cli.py`: zero tests, and `create-admin` is the documented bootstrap. +- SSE has three end-to-end tests; `services/sse.py` has none. +- `composer.js`, `terminal.js` and `audio.js` — 1,188 lines — have no assertion + of any kind. +- The suite has only ever run on Python 3.14. The Dockerfile ships 3.12 and + `requires-python` claims 3.11. diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index 1cf1196..e3c56de 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.9.8" +__version__ = "0.9.9" diff --git a/src/lembas/api/admin_prompts.py b/src/lembas/api/admin_prompts.py index 8816629..76880d4 100644 --- a/src/lembas/api/admin_prompts.py +++ b/src/lembas/api/admin_prompts.py @@ -18,6 +18,7 @@ from lembas.services import harness as harness_service from lembas.services import prompts as prompts_service from lembas.services import settings_store from lembas.services import tools as tools_service +from lembas.services.agent import policy from lembas.web.templating import render log = logging.getLogger(__name__) @@ -28,6 +29,49 @@ router = APIRouter(prefix="/admin/prompts", tags=["admin-prompts"]) # in place rather than imagined. An administrator can clear the field. SAMPLE_DOCUMENTS = "report.pdf, notes.txt" +# The rest of what a preview has to pretend, and the reason it must. +# +# `harness.context_variables` fills most `requires` gates only when it is handed +# a real `Chat` -- the machine, the directory, the plan, the project listing, a +# scheduled task's instruction, the flag saying this is a helper. The preview +# passes `chat=None`, so every one of those stayed empty and **eleven gated +# fragments could never appear in it at all**: the whole agent surface, both +# scheduling fragments, and the helper warning. An administrator editing +# `tool.agent` previewed a system message with `tool.agent` missing from it, and +# nothing said so. +# +# Samples rather than a transient Chat. `compose_from` takes plain variables +# precisely so this screen never has to build one, and a constructed row would +# need a connection, a profile and a directory that exist -- inventing an SSH +# host to render a paragraph is a worse trade than inventing the paragraph's +# values. This is what `SAMPLE_DOCUMENTS` has always done, extended to the rest. +SAMPLE_AGENT = { + "agent_target": "buildbox", + "agent_dir": "/srv/www/example", + "agent_rewound": "on 3 August at 14:20", + "background": "on", + "project_files": "src/\n app.py\n models.py\nREADME.md\npyproject.toml", + "agent_instructions": "Run the tests with `make check` before proposing a change.", + "agent_instructions_file": "AGENTS.md", + "plan": "1. [done] Read the failing test\n2. [doing] Fix the parser\n3. [todo] Add a case", +} + +SAMPLE_SCHEDULE = { + "schedule_instruction": "Summarise what changed in the repository since yesterday.", + "schedule_summary": "every weekday at 08:00", +} + +# Situations a chat can be in that are not a tool family, so nothing on the +# "Tools offered" row can reach them. `kind` and `parent_chat_id` in the model. +SITUATION_ORDINARY = "" +SITUATION_TASK = "task" +SITUATION_HELPER = "helper" +SITUATIONS = ( + (SITUATION_ORDINARY, "An ordinary chat"), + (SITUATION_TASK, "A scheduled task, running unattended"), + (SITUATION_HELPER, "A helper sent by another model"), +) + def _families_of(db: Db, names: list[str]) -> list[str]: """Keep only real family names, in the registry's order. @@ -54,6 +98,8 @@ def _variables( model_name: str = "", bases: str = "", documents: str = "", + situation: str = SITUATION_ORDINARY, + mode: str = "", ) -> dict[str, str]: """The preview's variable values. @@ -64,7 +110,14 @@ def _variables( No Chat row is made. `harness.compose_from` takes plain variables precisely so that this screen never has to build a transient one. + + The samples are gated exactly as `context_variables` gates the real values -- + the agent block on the `agent` family, the schedule and helper blocks on the + situation rather than on any family, because neither is a tool. A preview + that admitted a fragment the real request would not is worse than one that + omitted it, so the gating is mirrored rather than approximated. """ + from lembas.services.agent import policy from lembas.services.library import memories as memories_service from lembas.services.library import skills as skills_service @@ -79,6 +132,18 @@ def _variables( "document_names": documents, } ) + if "agent" in families: + values.update(SAMPLE_AGENT) + # A real one out of the table, not invented prose: this bullet *is* the + # mode guidance, so a made-up sentence here would preview wording that + # no request ever carries. + values["agent_mode"] = policy.MODE_GUIDANCE.get(mode, "") or policy.MODE_GUIDANCE[ + policy.MODE_EDIT + ] + if situation == SITUATION_TASK: + values.update(SAMPLE_SCHEDULE) + if situation == SITUATION_HELPER: + values["subagent"] = "yes" return values @@ -109,16 +174,27 @@ async def prompts_page(request: Request, db: Db, user: AdminUser, saved: bool = "variables": prompts_service.VARIABLES, # The legend shows what each name resolves to right now, with every # family on -- a legend nobody can check is just a list of words. - "resolved": _variables( - db, - user, - families=families, - model_name=models[0].label if models else "", - bases="Contracts, Recipes", - documents=SAMPLE_DOCUMENTS, - ), + # Every situation at once, unlike the preview: a chat is either a + # scheduled task or a helper and never both, but a legend is a + # reference rather than a rendering, and a name shown as empty + # because of the situation it was built in reads as a name that + # resolves to nothing. + "resolved": { + **_variables( + db, + user, + families=families, + model_name=models[0].label if models else "", + bases="Contracts, Recipes", + documents=SAMPLE_DOCUMENTS, + situation=SITUATION_TASK, + ), + "subagent": "yes", + }, "models": models, "families": families, + "situations": SITUATIONS, + "modes": policy.MODE_LABELS, "registry": sorted( tools_service.registry(db).values(), key=lambda t: (t.family, t.name) ), @@ -173,6 +249,8 @@ async def preview(request: Request, db: Db, user: AdminUser): model_name = str(form.get("preview_model") or "") bases = str(form.get("preview_bases") or "").strip() documents = str(form.get("preview_documents") or "").strip() + situation = str(form.get("preview_situation") or "") + mode = str(form.get("preview_mode") or "") variables = _variables( db, @@ -181,6 +259,8 @@ async def preview(request: Request, db: Db, user: AdminUser): model_name=model_name, bases=bases, documents=documents, + situation=situation, + mode=mode, ) body = harness_service.compose_from( db, diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index 819101f..4389331 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -188,10 +188,23 @@ def context_variables( "today": stamp.strftime("%A %-d %B %Y"), "now": stamp.strftime("%A %-d %B %Y, %H:%M (UTC%z)"), # Named so a model working out a schedule can say which zone it meant, - # and so `core.today` can carry it without a second fragment. Empty when - # nobody has chosen one, which drops the line rather than printing the - # server's zone as though it were a decision. - "timezone": clock.name_for(user), + # and so `core.today` can carry it without a second fragment. + # + # The fallback is load-bearing and used to be absent. `name_for` returns + # "" for anybody who has never chosen a zone -- the default state of + # every account -- and the comment here claimed that dropped the line + # rather than announcing the server's zone as a decision. It did not: + # `substitute` drops a line only when it is *blank* after expansion, and + # this variable sits inside a sentence, so every such request shipped + # "- Times the person gives you are in unless they say otherwise." + # + # Naming the server's zone was never the thing being avoided anyway. + # `stamp` is `clock.now_for(user)`, which already falls back to it, so + # `{{today}}` and `{{now}}` are *already* in that zone and `{{now}}` + # already prints its offset. Withholding the label from a value the + # model has been given is not restraint, it is a hole. This is the + # fallback `schedule/compile.py` has always had, for the same reason. + "timezone": clock.name_for(user) or str(clock.server_zone()), "instance_name": branding.for_db(db).name, "user_name": (user.name or "") if user is not None else "", "model_name": "", @@ -241,6 +254,7 @@ def context_variables( "agent_instructions": "", "agent_instructions_file": "", "plan": "", + "plan_editable": "", # Empty everywhere but a scheduled task's own chat, which is what makes # it the gate on `core.unattended` as well as the content of # `context.schedule`. Two fragments, one variable, and no way for the @@ -346,6 +360,17 @@ def _agent_values(db: DBSession, chat, user) -> dict[str, str]: # `agent_session.resolve`. A plan the model cannot see is a plan it # cannot keep current, which is the whole of why this is here. "plan": plans_service.render_block(context.plan), + # Whether `plan_update` is actually in this request, which is not the + # same question as whether there is a plan. `agent/tools.py` drops it in + # Plan mode -- that mode ends with `plan_submit` instead -- so gating its + # guidance on `plan` alone told a model in Plan mode to "keep it current + # with plan_update as you go" about a tool that was not there, directly + # under `core.tool_list` saying anything unnamed does not exist. The + # fragment's own hint claimed the two coincided. They do not, and this + # is the variable that makes them. + "plan_editable": ( + plans_service.render_block(context.plan) if context.mode != policy.MODE_PLAN else "" + ), **_project_instructions(db, chat, context, settings_store), } diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index ecfcc17..09c47c6 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -219,8 +219,14 @@ VARIABLES: tuple[Variable, ...] = ( "The current plan", "The plan this agent chat is working to, with its ids, finished phases " "collapsed and the active one shown in full. Empty when there is none, " - "which is what keeps both the plan section and plan_update's guidance " - "out of every chat that is not carrying one.", + "which is what keeps the plan section out of every chat not carrying one.", + ), + Variable( + "plan_editable", + "The plan, when it can be changed", + "The same text as {{plan}}, but blank in Plan mode -- where plan_update " + "is withdrawn and the turn ends with plan_submit instead. Gates that " + "tool's guidance, so a mode without the tool is not told to use it.", ), Variable( "agent_instructions", @@ -614,9 +620,10 @@ BUILTIN: tuple[Fragment, ...] = ( order=20, variables=("today", "timezone"), hint="A model has no clock. Without this it cannot tell whether what it " - "recalls is current, and will not think to check. The timezone line " - "carries its own variable, so it disappears on an instance where nobody " - "has chosen one rather than announcing the server's as a decision.", + "recalls is current, and will not think to check. {{timezone}} is the " + "reader's own zone, or the server's where they have not chosen one -- " + "the same zone the date above is already stated in, so it names a value " + "rather than making a decision.", default=( "Today is {{today}}. Your training data stops well before this, so treat " "anything time-sensitive as something to check rather than something you " @@ -1314,20 +1321,28 @@ BUILTIN: tuple[Fragment, ...] = ( families=("subagent",), requires=("agent_target",), hint="The agent-chat half, gated on `agent_target` so it appears only " - "where there is a machine. What it has to say is what a helper cannot " - "do there, because the failure otherwise is a model planning a whole " - "phase around a helper that will refuse every step of it — a helper " - "reads and may run a short list of read-only commands, and nothing " - "else, whatever mode this chat is in.", + "where there is a machine. What it has to say is where the edge is, " + "because the failure otherwise is a model planning a whole phase around " + "a helper that will refuse every step of it. Two edges, and they are " + "different: **commands** are pinned to a read-only list in every mode, " + "since an unattended chat cannot approve anything; **files** may be " + "written, but only by a helper asked for with write and only from Edit " + "or Auto. An earlier version denied the second outright — which is a " + "documented parameter of the tool beside it — and named seven of the " + "twenty-three allowed commands, so a model avoided commands it had.", default=( "- A helper on this machine reads and reports. It can list and read files " - "and run the ordinary read-only commands — ls, cat, grep, find, git status, " - "git log, git diff — and nothing else, in every mode, because there is " - "nobody there to approve anything. Send one to find out where something " - "lives, to read a subsystem and describe it, or to check whether a pattern " - "holds across a tree; make the changes yourself once it reports.\n" - "- Do not send one to build, test, install or run anything: it will be " - "refused a step in and come back having done nothing.\n" + "and run the read-only commands — ls, pwd, cat, head, tail, wc, file, stat, " + "du, df, tree, find, grep, rg, and git status, log, show, diff, branch and " + "remote. Send one to find out where something lives, to read a subsystem " + "and describe it, or to check whether a pattern holds across a tree.\n" + "- Do not send one to build, test, install or run anything. That list is " + "the whole of what it may run, in every mode, because there is nobody " + "there to approve anything else — a helper asked to run the tests is " + "refused a step in and comes back having done nothing.\n" + "- A helper asked for with write can also write and edit files, and only " + "from Edit or Auto mode. Use it for a mechanical change across many files " + "that you have already decided on; keep the deciding for yourself.\n" "- Ask for what you want back, not for a summary. “The three files that " "define X and what each does” is usable; “look into X” comes back as prose " "you have to read the codebase to check." @@ -1539,12 +1554,14 @@ BUILTIN: tuple[Fragment, ...] = ( group=GROUP_TOOLS, order=255, families=("agent",), - requires=("plan",), - hint="Appears once a plan exists, which is also when plan_update is " - "offered. It is about doing the bookkeeping as the work goes rather " - "than at the end -- a plan updated only at the end is a report, and " - "the point of it is being able to see where things are while they are " - "still moving.", + requires=("plan_editable",), + hint="Appears when plan_update is actually offered: a plan exists and " + "this is not Plan mode, which withdraws it in favour of plan_submit. " + "Gated on {{plan}} it appeared in Plan mode too, telling a model to " + "use a tool it did not have. It is about doing the bookkeeping as the " + "work goes rather than at the end -- a plan updated only at the end is " + "a report, and 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 with " "plan_update as you go rather than at the end: mark a task “doing” when " @@ -1567,16 +1584,18 @@ BUILTIN: tuple[Fragment, ...] = ( requires=("plan",), variables=("plan",), hint="The plan as it stands, including what has already been ticked " - "off. A plan the model cannot see is a plan it cannot update, which " - "is what the whole of plan_update depends on. The ids are shown " - "because they are what plan_update takes.", + "off. A plan the model cannot see is a plan it cannot work to. Shown in " + "every mode including Plan, where the tool for changing it is withdrawn " + "-- so this says what the plan *is* and leaves how to change it to " + "`tool.plan_update`, which is gated on that tool actually being there. " + "The ids are shown because they are what plan_update takes.", default=( "### The current plan\n" "\n" "{{plan}}\n" "\n" - "This is the plan as it stands now. Change it with plan_update rather " - "than restating it in your answer, and quote the ids above." + "This is the plan as it stands now. Work to it, and quote the ids above " + "rather than restating the plan in your answer." ), ), Fragment( diff --git a/src/lembas/services/subagent.py b/src/lembas/services/subagent.py index 4f5080d..8c031c1 100644 --- a/src/lembas/services/subagent.py +++ b/src/lembas/services/subagent.py @@ -516,14 +516,17 @@ def tool_defs() -> list[ToolDef]: family=FAMILY_SUBAGENT, description=( "Hand one self-contained piece of work to a helper — a second " - "model with the same tools that works on its own and gives you " - "its answer. Use it to cover several independent areas at once: " - "call it several times in one turn and each runs in parallel. " - "It cannot ask you or the reader anything, cannot delegate " - "further, and starts knowing nothing about this conversation, " - "so the task must say everything it needs. Do not use it for " - "something you could do in one call yourself, or for anything " - "needing a decision only the reader can make." + "model that works on its own and gives you its answer. Use it " + "to cover several independent areas at once: call it several " + "times in one turn and each runs in parallel. It gets a " + "narrower set of tools than you: it reads, it cannot ask you or " + "the reader anything, it cannot delegate further, it changes " + "nothing unless you set write, and on a machine it may run only " + "a fixed list of read-only commands. It starts knowing nothing " + "about this conversation, so the task must say everything it " + "needs. Do not use it for something you could do in one call " + "yourself, or for anything needing a decision only the reader " + "can make." ), parameters={ "type": "object", diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 7feb2de..2adf164 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -188,6 +188,21 @@ RISKS = (RISK_READ, RISK_WRITE, RISK_EXECUTE, RISK_ASK) # single page. Cut with the model told so, rather than refused. MAX_FETCH_CHARS = 20_000 +# The same bound for a knowledge document, and it was missing. `knowledge_get` +# returned `extracted_text` whole while every sibling reader capped and said so +# -- `fetch` above, `file_read`, the memories block, the skill index, the +# project listing. `MAX_EXTRACTED_CHARS` is 120_000 by default and an +# administrator can raise it, so one call on a long PDF filled an ordinary +# window with nothing anywhere reporting that it had. +# +# Larger than a fetched page on purpose. Somebody put this document in the +# library deliberately and named it in a search; a page the model followed a +# link to is a guess. Cut with the model told so rather than refused, which is +# what `fetch` and `file_read` both do -- a reader that fails on exactly the +# documents worth reading is worse than one that hands back the first part and +# says there was more. +MAX_DOCUMENT_CHARS = 40_000 + @dataclass class ToolContext: @@ -471,6 +486,14 @@ async def _run_knowledge_get(context: ToolContext, args: dict[str, Any]) -> Tool "results": [{"title": document.title, "id": document.id}], } body = document.extracted_text or document.extraction_error or "(no text)" + if len(body) > MAX_DOCUMENT_CHARS: + body = ( + f"{body[:MAX_DOCUMENT_CHARS]}\n\n" + f"[Cut off here. This document is {len(document.extracted_text or ''):,} " + f"characters and the first {MAX_DOCUMENT_CHARS:,} are above. Search it " + "with knowledge_search to find the part you need.]" + ) + event["truncated"] = True return ToolOutcome(f"{document.title}\n\n{body}", event) @@ -912,8 +935,21 @@ async def _run_ask_user(context: ToolContext, args: dict[str, Any]) -> ToolOutco session-free snapshot that deliberately holds no way to reach one. Getting here means some other path called `run_tool` directly, and saying so is better than returning an empty answer the model would treat as a reply. + + It read `args["question"]`, singular, against a schema that declares + `questions` and a list -- so the event it built always carried an empty + `query`, and the card showed a refusal with no sign of what had been asked. + Harmless only because this path is unreachable, which is exactly why nothing + caught it: schema drift on a branch no test exercises. Tolerant of the same + spellings `generation._questions_in` accepts, rather than importing it, + which would be a circular import for one field on a dead path. """ - question = str(args.get("question") or "").strip() + asked: Any = args.get("questions") or args.get("question") or "" + if isinstance(asked, list): + asked = asked[0] if asked else "" + if isinstance(asked, dict): + asked = asked.get("question") or "" + question = str(asked).strip() return ToolOutcome( "That question could not be put to anyone, so it has gone unanswered. " "Carry on without it, or say what you need.", @@ -997,7 +1033,11 @@ REGISTRY: dict[str, ToolDef] = { ToolDef( name="knowledge_get", family=FAMILY_KNOWLEDGE, - description="Read one knowledge document in full, by the id a search returned.", + description=( + "Read one knowledge document, by the id a search returned. A " + "long one is cut off at the end rather than refused, and you " + "are told when that happened." + ), parameters=_object({"id": _STRING}, ["id"]), run=_run_knowledge_get, ), diff --git a/src/lembas/web/templates/admin/prompts.html b/src/lembas/web/templates/admin/prompts.html index c0c82f0..b13c7d7 100644 --- a/src/lembas/web/templates/admin/prompts.html +++ b/src/lembas/web/templates/admin/prompts.html @@ -82,6 +82,33 @@ + {# + The two axes a tool family cannot reach. Without them every fragment + gated on a chat -- the whole agent surface, both scheduling fragments and + the helper warning -- was missing from the preview whatever was ticked + below, because the preview passes no Chat and those variables are only + filled when there is one. + #} +
+ + +
+
+ + +

+ Only reaches the prompt with agent ticked below. The + machine, directory and plan shown there are samples. +

+
Tools offered {% for family in families %} diff --git a/tests/test_admin_prompts.py b/tests/test_admin_prompts.py index 7636efc..8d819e8 100644 --- a/tests/test_admin_prompts.py +++ b/tests/test_admin_prompts.py @@ -34,6 +34,21 @@ def test_the_page_lists_every_fragment(client: TestClient, registered): assert f'name="prompt.{fragment.key}"' in page, fragment.key +def test_the_preview_controls_are_inside_what_the_preview_includes( + client: TestClient, registered +): + """`hx-include="#prompt-form, #preview-controls"` is the whole wiring, so a + control placed outside that container is submitted by nothing and changes + nothing -- with no error, which is this codebase's recurring failure. Assert + the containment rather than the markup of any one field. + """ + page = client.get("/admin/prompts").text + controls = page.split('id="preview-controls"', 1)[1].split("
\n ", 1)[0] + for name in ("preview_model", "preview_bases", "preview_documents", + "preview_situation", "preview_mode", "preview_family"): + assert f'name="{name}"' in controls, name + + def test_the_page_is_refused_to_a_plain_user(client: TestClient, plain_user): assert client.get("/admin/prompts").status_code == 403 assert client.post("/admin/prompts", data={}).status_code == 403 @@ -135,6 +150,51 @@ def test_the_preview_escapes_what_a_model_wrote(client: TestClient, db, register assert "<img src=x" in body +def test_the_preview_can_reach_the_fragments_that_need_a_chat(client: TestClient, registered): + """Eleven fragments are gated on variables `context_variables` fills only + when it is handed a real `Chat`, and the preview hands it `None`. So the + entire agent surface, both scheduling fragments and the helper warning were + absent from every preview whatever was ticked -- an administrator editing + `tool.agent` previewed a system message with `tool.agent` missing from it, + and nothing said so. The samples are what close that. + """ + agent = client.post( + "/admin/prompts/preview", + data={"preview_family": ["agent"], "preview_mode": "plan"}, + ).text + # tool.agent (agent_target), and the mode picked rather than a fixed one. + assert "buildbox" in agent + assert "Plan** mode" in agent + # tool.project_files, context.agent_instructions, context.plan, tool.background + assert "pyproject.toml" in agent + assert "AGENTS.md" in agent + assert "Fix the parser" in agent + + task = client.post( + "/admin/prompts/preview", data={"preview_situation": "task", "preview_family": []} + ).text + assert "every weekday at 08:00" in task + + helper = client.post( + "/admin/prompts/preview", data={"preview_situation": "helper", "preview_family": []} + ).text + assert "buildbox" not in helper + assert helper != task + + +def test_the_preview_gates_its_samples_exactly_as_a_real_request_would( + client: TestClient, registered +): + """A preview that admitted a fragment the real request would not is worse + than one that omitted it, so the samples follow the same gates: the agent + block on the family, the other two on the situation and on no family at all. + """ + plain = client.post("/admin/prompts/preview", data={"preview_family": ["notes"]}).text + assert "buildbox" not in plain + assert "AGENTS.md" not in plain + assert "every weekday at 08:00" not in plain + + def test_the_preview_warns_when_the_cap_would_cut_it_off(client: TestClient, db, registered): settings_store.update(db, {"max_harness_chars": 60}, key=settings_store.PROMPTS) body = client.post("/admin/prompts/preview", data={"preview_family": ["web_search"]}).text diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index 20e28f3..9a8e245 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -986,6 +986,67 @@ async def test_an_ordinary_chat_is_not_asked_to_narrate(db, user_id, machine): assert "Settle what you are setting out to achieve" not in text +def _give_a_plan(db, chat): + """A plan on the chat, the way `_plan_of` finds one: a message carrying it + and the chat pointing at that message.""" + from lembas.services import plans + + message = Message( + chat_id=chat.id, + role=ROLE_ASSISTANT, + content="", + plan_json=plans.build(title="Fix the parser", steps="Read it\nChange it"), + ) + db.add(message) + db.commit() + chat.plan_message_id = message.id + db.commit() + + +async def test_plan_mode_is_not_told_to_use_a_tool_it_does_not_have(db, user_id, machine): + """`agent/tools.py` withdraws `plan_update` in Plan mode -- that mode ends + with `plan_submit` instead. Its guidance was gated on {{plan}}, which is set + whenever a plan exists in any mode, so a model in Plan mode was told to + "keep it current with plan_update as you go" about a tool that was not in + its list, directly under `core.tool_list` saying anything unnamed does not + exist. The fragment's own hint asserted the two coincided. + """ + from lembas.services import harness + + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_PLAN) + _give_a_plan(db, chat) + user = db.get(User, user_id) + + offered = tools_service.resolve_tools(db, chat, user) + assert "plan_update" not in offered.by_name + + values = harness.context_variables(db, user, offered.schemas, chat) + # The plan is still shown -- a model that cannot see it cannot submit a + # better one. Only the instruction to *edit* it goes. + assert values["plan"] + assert values["plan_editable"] == "" + + text = harness.compose(db, user, offered.schemas, chat) + assert "Fix the parser" in text + assert "plan_update" not in text + + +async def test_a_mode_that_has_plan_update_is_still_told_about_it(db, user_id, machine): + """The other half, or the gate is just a deletion.""" + from lembas.services import harness + + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_EDIT) + _give_a_plan(db, chat) + user = db.get(User, user_id) + + offered = tools_service.resolve_tools(db, chat, user) + assert "plan_update" in offered.by_name + + values = harness.context_variables(db, user, offered.schemas, chat) + assert values["plan_editable"] == values["plan"] != "" + assert "plan_update" in harness.compose(db, user, offered.schemas, chat) + + async def test_an_agent_chat_is_told_its_real_round_budget(db, user_id, machine): """MAX_ROUNDS is one. An agent chat gets hundreds, and telling it one would be a false fact about its own budget on every turn.""" diff --git a/tests/test_harness.py b/tests/test_harness.py index 2f20236..1ce954a 100644 --- a/tests/test_harness.py +++ b/tests/test_harness.py @@ -37,6 +37,37 @@ def test_no_tools_means_no_tool_guidance(db, owner): assert harness.compose(db, owner, None) == text +def test_the_date_line_names_a_zone_when_nobody_has_chosen_one(db, owner): + """The shipped default, on the shipped configuration. + + `clock.name_for` returns "" for an account that has never chosen a zone, + which is every account until somebody visits the settings page. The variable + sits *inside* a sentence, and `substitute` drops a line only when the whole + line is blank after expansion -- so an unresolved zone was never a dropped + line, it was "in unless", shipped on every request. Both the comment in + `harness.py` and the fragment's own hint claimed otherwise, and no test + asked. + """ + assert not (owner.settings_json or {}).get("timezone") + text = harness.compose(db, owner, []) + + line = next(ln for ln in text.splitlines() if "Times the person gives you" in ln) + assert "in unless" not in line + assert " " not in line + # Whatever the host is set to, it has to be *something*. + zone = line.split(" are in ", 1)[1].split(" unless", 1)[0] + assert zone.strip() + + +def test_the_date_line_uses_the_readers_own_zone_when_they_have_one(db, owner): + """And a chosen zone is preferred to the server's, which is the whole point + of the variable -- the fallback must not have flattened it.""" + owner.settings_json = {**(owner.settings_json or {}), "timezone": "Pacific/Auckland"} + db.commit() + text = harness.compose(db, owner, []) + assert "are in Pacific/Auckland unless" in text + + def test_clearing_the_core_fragments_restores_an_empty_harness(db, owner): """The behaviour change is a default, not a rule: an administrator who wants nothing sent to a tool-less model can still have exactly that.""" diff --git a/tests/test_subagent.py b/tests/test_subagent.py index c758902..599dc4d 100644 --- a/tests/test_subagent.py +++ b/tests/test_subagent.py @@ -672,3 +672,41 @@ def test_a_helpers_chat_is_sized_by_its_own_numbers(db): assert limits.steps == 7 assert limits.wall_seconds == 111.0 assert agent_session.resolve(db, parent, _user(db)).limits.steps == 200 + + +# --- What the model is told it may do ----------------------------------------- +def test_the_guidance_names_every_command_a_helper_may_run(): + """`SAFE_COMMANDS` holds twenty-three entries; the fragment named seven of + them and said "and nothing else", so a model avoided commands it had -- + which costs nothing visible and is therefore never reported. Pinned rather + than proof-read, because the two drift the moment one is edited alone. + """ + from lembas.services import prompts + + text = next(f for f in prompts.BUILTIN if f.key == "tool.subagent_agent").default + for entry in subagent_service.SAFE_COMMANDS: + if entry.startswith("file_"): + continue # a tool name, not a command -- see SAFE_COMMANDS' comment + # The word a model would look for: the command itself, or for git the + # subcommand, since the prose shares one "git" across all six. + parts = entry.removesuffix("*").strip().split() + word = parts[1] if parts[0] == "git" else parts[0] + assert word in text, f"{entry!r} is allowed and unmentioned" + + +def test_the_guidance_does_not_deny_a_parameter_the_tool_offers(): + """It said a helper "reads and reports ... and nothing else, in every mode", + beside a `write` parameter that makes one write files. A model reads the + prose, not the schema, so the parameter was effectively unreachable. + """ + from lembas.services import prompts + + text = next(f for f in prompts.BUILTIN if f.key == "tool.subagent_agent").default + assert "write" in text + assert "nothing else, in every mode" not in text + + schema = subagent_service.tool_defs()[0] + assert "write" in schema.parameters["properties"] + # And the command list really is fixed whatever `write` says, which is the + # distinction the wording now has to carry. + assert "in every mode" in text diff --git a/tests/test_tools.py b/tests/test_tools.py index e44eb5d..f7c00bc 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -415,6 +415,56 @@ async def test_a_failed_fetch_does_not_kill_the_reply(monkeypatch): assert "not reachable" in outcome.content +async def test_a_long_knowledge_document_is_cut_and_the_model_told(db, user_id): + """It was the one reader with no bound at all. `fetch` caps and says so, + `file_read` caps and says so, the memories block, the skill index and the + project listing all carry budgets -- and `knowledge_get` returned + `extracted_text` whole. Its description said "in full", which is why this + read as correct: the tool did exactly what it claimed, and what it claimed + was one call away from filling the window with nothing reporting it. + """ + from lembas.db.models import User + from lembas.services.library import documents as documents_service + + owner = db.get(User, user_id) + base = documents_service.create_base(db, owner=owner, name="Long") + document = documents_service.store_upload( + db, owner=owner, payload=b"x" * 90_000, filename="big.txt", + title="Big", base=base, + ) + + outcome = await tools_service.run_tool( + tools_service.ToolContext(owner_id=user_id), + "knowledge_get", + json.dumps({"id": document.id}), + ) + assert len(outcome.content) < tools_service.MAX_DOCUMENT_CHARS + 500 + assert "Cut off here" in outcome.content + assert outcome.event["truncated"] is True + + +async def test_a_short_knowledge_document_is_returned_whole(db, user_id): + """The cap must not become a truncation notice on ordinary documents.""" + from lembas.db.models import User + from lembas.services.library import documents as documents_service + + owner = db.get(User, user_id) + base = documents_service.create_base(db, owner=owner, name="Short") + document = documents_service.store_upload( + db, owner=owner, payload=b"The mallorn is golden.", filename="a.txt", + title="Mallorn", base=base, + ) + + outcome = await tools_service.run_tool( + tools_service.ToolContext(owner_id=user_id), + "knowledge_get", + json.dumps({"id": document.id}), + ) + assert "The mallorn is golden." in outcome.content + assert "Cut off here" not in outcome.content + assert "truncated" not in outcome.event + + async def test_a_long_page_is_cut_and_the_model_told(monkeypatch): """120_000 characters is roughly thirty thousand tokens. One call would fill an ordinary window and spend an agent chat's whole output budget."""