c4aff999ba
The second audit pass. Four things, and the first two were reported. The Prompts page put a screen of variables and a screen of preview above the editor, so the tabs began two screens down and switching one had to drag the whole page to be any use -- and on a short tab it could not drag far enough, leaving the panel stranded above a screenful of nothing. Editor first, reference after, bar sticky. Custom themes were three fixed slots: fifty-seven empty colour boxes on a fresh instance and no way to make a fourth theme. One block per theme plus a blank one, colours behind a disclosure. Both measured rather than argued about -- rendered through TestClient and driven under headless Chromium, where the tab bar moved 385->642px before and does not move now, and the themes page went from 5495px to 2820px. Asking where generated images go found the other two. Deleting a chat cascades to the attachment rows and leaves every file on disk; the helper written for exactly that was called from one place, and it was not the delete button, a schedule's chat, a helper's chat or deleting an account. Underneath it, `claim` bound message_id and never chat_id, so anything picked before a chat existed kept an empty chat_id forever -- which six readers filter on, so those files were also unnamed in the prompt, unopenable in the canvas, and invisible to the one caller the cleanup had. And folders nest now. The route has handled parent_id since folders existed, with a cycle guard and a depth cap the move path never applied; the sidebar has always drawn a tree. Nothing could ask for one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
265 lines
14 KiB
Markdown
265 lines
14 KiB
Markdown
# 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.
|
||
|
||
**A third, found by asking where generated images go.** `db.delete(chat)`
|
||
cascades to the message and attachment *rows* and leaves every file on disk;
|
||
`files.remove_files_for_chats` was written for exactly that and was called from
|
||
one place, the temporary-chat sweep. The delete button, a schedule's task chat,
|
||
a helper's chat and account deletion all went straight to `db.delete`.
|
||
|
||
Underneath it, a second fault that made the first invisible: `files.claim`
|
||
bound `message_id` and never `chat_id`, so anything uploaded on the *new-chat*
|
||
screen kept an empty `chat_id` for the rest of its life. Six readers filter on
|
||
that column, so for those files the harness did not name them among the attached
|
||
documents, the canvas refused to open them, and the one caller the cleanup
|
||
helper had found nothing to remove. Both fixed, and `chat_service.delete_chats`
|
||
is now the only way a chat is deleted.
|
||
|
||
*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.
|
||
|
||
## Verified by rendering, not by reading — *Stage 2*
|
||
|
||
There is no browser extension on this machine, so `/admin/prompts` and
|
||
`/admin/customization` were checked by rendering them through `TestClient`,
|
||
rewriting the asset URLs to `file://`, and driving headless Chromium against the
|
||
result. Worth recording because the first run of that harness **measured an
|
||
unstyled page**: the templates emit absolute `http://testserver/static/…` URLs
|
||
via `url_for`, not `/static/…`, so the replacement matched nothing and no CSS
|
||
loaded. It reported all five tab panels visible at once — a dramatic finding
|
||
that was entirely an artefact. The harness now fails loudly on any unrewritten
|
||
asset URL.
|
||
|
||
What it measured once fixed:
|
||
|
||
- `/admin/prompts`, tab bar's on-screen position across the five tabs:
|
||
**385 → 642px before** (the page moving under the reader), **195px constant
|
||
after**, with `scrollTop` staying at 0.
|
||
- `/admin/customization`, document height: **5495px before, 2820px after**.
|
||
|
||
## 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.
|