docs/plugins.md described an SDK that no longer exists. It now covers messages.raw/sample, actions.move_to_special, the envelope's account_id / mailbox_special_use / account_state, the message.changed transition payload, the message.actions.menu slot, Slot.title and .wm-menu-item — with the reasons, not just the signatures. M3's acceptance is that a plugin can be written from this file alone, and it had drifted. Acceptance 4 (§C.8): a test that teaches 20 spam + 20 ham to a real engine and checks the classifier generalises to held-out mail, keeps ham under the banner threshold, and does not leak between users. It skips when no engine is reachable. It does NOT assert the literal move_threshold — a synthetic fixture cannot reach 12.0 once the SMTP-time symbols are zeroed, which is the whole point of the tuning. Recorded as OQ-8 rather than quietly asserting something weaker. Bayes ignores training until min_learns, stock 200. For a personal mailbox that means every correction appears to be ignored for months, so it is now 20. The bootstrap would have made this filter worse. Accounts do not sync their provider's spam folder by default, so the real corpus here is 3 junk beside 1,294 aged inbox — teaching both as found trains "everything is ham". It now refuses below 10 spam samples (leaving the account unmarked so a later run can do it properly) and otherwise caps ham at 3x the spam count. Run against the live account, it correctly declined: "only 3 spam samples (need 10); enable spam-folder sync to build a corpus" Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SqJYt7jD7oUEkoY68sBZo
201 lines
11 KiB
Markdown
201 lines
11 KiB
Markdown
# Open Questions
|
|
|
|
Per the setup rules, deviations and unresolved decisions are recorded here rather
|
|
than guessed silently.
|
|
|
|
## OQ-8 — Appendix C acceptance 4's threshold is not reachable by a fixture
|
|
|
|
**Status:** resolved by testing the property rather than the number; recorded
|
|
because the test deviates from the spec's literal wording.
|
|
|
|
**The criterion (§C.8.4):** after 20 spam + 20 ham teachings, "a held-out
|
|
obvious-spam fixture scores above `move_threshold` for that user and a ham
|
|
fixture below `banner_threshold`".
|
|
|
|
**What happens:** the ham half holds easily. The spam half does not — a held-out
|
|
spam fixture scores around 0.9 against a 12.0 threshold, even though `BAYES_SPAM`
|
|
fires correctly and the engine reports 160+ messages learned.
|
|
|
|
**Why, and why it is not a bug:** real spam crosses 12.0 by accumulating many
|
|
symbols, most of which describe its *delivery* — no DKIM, unknown sending host,
|
|
dates that do not line up. This deployment deliberately zeroes those
|
|
(`docker/rspamd/local.d/groups.conf`), because scoring already-delivered mail on
|
|
SMTP-time facts was firing on 11.3% of ordinary mail. A synthetic fixture has no
|
|
delivery history at all, so once those symbols contribute nothing, almost the
|
|
only thing left to score is Bayes itself — and one classifier symbol cannot carry
|
|
a message to 12.0 on its own, nor should it.
|
|
|
|
Asserting the literal number would therefore require either un-tuning the filter
|
|
or writing fixtures that fake plausible delivery headers — testing the fixture
|
|
generator rather than the learning loop.
|
|
|
|
**What the test asserts instead** (`tests/plugins/test_antispam_corpus.py`):
|
|
after training, Bayes recognises held-out spam it has never seen; it does not cry
|
|
spam at held-out ham; held-out ham stays below `banner_threshold`; spam outscores
|
|
ham; and an untaught user sees neither the symbol nor the score — the per-user
|
|
isolation the criterion also asks for.
|
|
|
|
## OQ-6 — Appendix C needs SDK surfaces the frozen §11 API does not have
|
|
|
|
**Status:** resolved by additive SDK extension; recorded here because C.8.1 says
|
|
"zero core diffs".
|
|
|
|
**Problem.** C1 cannot be written against the M3 SDK as frozen:
|
|
|
|
1. **§C.4 step 4** requires moving a message to Junk "through the existing move
|
|
machinery, so the verdict propagates to the IMAP server". `PluginActions`
|
|
exposed only `set_flags` and `trash`. `trash` moves the *whole thread*, which
|
|
is right for Delete and wrong for a spam verdict — one spam reply would drag a
|
|
legitimate conversation into Junk. (The `filters` plugin had already noted the
|
|
same gap: "a `move` action would follow the same pattern".)
|
|
2. **§C.4 step 3** posts the *raw blob* to Rspamd. `PluginMessages` exposed
|
|
`envelope()` and `header()` — no way to get the whole RFC822.
|
|
3. **§C.4 step 1** processes "only messages landing in `special_use='inbox'`
|
|
mailboxes". Neither the `message.new` payload nor `envelope()` said where a
|
|
message landed.
|
|
|
|
**Resolution.** Three additive changes to `app/plugins/`, no change to any core
|
|
mail path:
|
|
|
|
- `PluginActions.move_to_special(user_id, message_id, special_use)` — per-message
|
|
move wrapping the same optimistic-update + `move_job` path as the core
|
|
endpoint. Returns False (never raises) when the account has no such folder.
|
|
- `PluginMessages.raw(user_id, message_id) -> bytes | None`.
|
|
- `envelope()` now also returns `account_id` and `mailbox_special_use`.
|
|
|
|
**C2 needed two more, on the same reasoning:**
|
|
|
|
4. **§C.5 passive learning** reads the user's own filing as a verdict, which
|
|
needs the *transition* — "now in Junk" and "moved out of Junk" are different
|
|
facts. `message.changed` carried only the destination, so `move_job` now also
|
|
publishes `previous_mailbox_id`, `special_use` and `previous_special_use`.
|
|
5. **§C.5 provider bootstrap** samples up to 500 provider-Junk messages and 500
|
|
aged Inbox messages. The SDK could fetch a message by id but not enumerate a
|
|
folder, so `PluginMessages.sample(account_id, special_use, limit,
|
|
older_than_days)` returns ids only — the caller pulls bodies one at a time,
|
|
so a 500-message sample never materialises 500 bodies.
|
|
|
|
**Reading of "zero core diffs":** taken to mean the plugin must not require
|
|
changes to core *mail flow or behaviour* — with the plugin absent, mail handling
|
|
is byte-identical, which remains true. Extending the plugin SDK is what the SDK
|
|
is for, and all three additions are useful beyond antispam. If the intent was
|
|
literally "no file under `app/` changes", C1 is not implementable and the spec
|
|
needs amending instead.
|
|
|
|
**Also note:** §C.2 itself mandates compose additions (`rspamd`,
|
|
`redis-rspamd`), which are core-repo diffs by any reading. They are placed behind
|
|
a compose profile so a deployment that does not want the engine is unaffected.
|
|
|
|
## OQ-7 — Rspamd cannot be reached through the SDK's guarded HTTP
|
|
|
|
**Status:** resolved, deliberate.
|
|
|
|
`sdk.http` applies the §10.6 SSRF guard, which rejects private addresses. That is
|
|
correct for user-supplied CalDAV/CardDAV URLs, but Rspamd lives at
|
|
`http://rspamd:11333` on the compose network — a private address by definition.
|
|
The guard would forbid the only address that works.
|
|
|
|
The antispam plugin therefore uses `httpx` directly for engine calls. This is
|
|
safe for the reason the guard exists: the URL comes from `RSPAMD_URL` in the
|
|
operator's environment, never from user input, so there is no request-forgery
|
|
surface to protect. User-supplied URLs in this plugin (there are none today)
|
|
would still have to go through `sdk.http`.
|
|
|
|
## OQ-1 — Meilisearch container healthcheck reports `unhealthy` (IPv6 `localhost`)
|
|
|
|
**Status:** resolved — approved deviation applied. The healthcheck URL in
|
|
`docker/docker-compose.yml` was changed from `localhost` to `127.0.0.1`; the
|
|
container now reports `healthy`.
|
|
|
|
**Symptom:** `docker compose ps` shows `meilisearch` as `unhealthy`, even though the
|
|
service is fully up and reachable from the host (`curl http://127.0.0.1:7700/health`
|
|
returns `{"status":"available"}`).
|
|
|
|
**Root cause:** the spec's healthcheck is
|
|
`wget -qO- http://localhost:7700/health | grep -q available`. Inside the
|
|
`getmeili/meilisearch:v1.8` container, `/etc/hosts` maps `localhost` to both
|
|
`127.0.0.1` and `::1`, and busybox `wget` tries the IPv6 `::1` first. Meilisearch
|
|
listens IPv4-only on `0.0.0.0:7700` (`MEILI_HTTP_ADDR=0.0.0.0:7700`), so the `::1`
|
|
connection is refused and the healthcheck fails. `wget http://127.0.0.1:7700/health`
|
|
from inside the container succeeds.
|
|
|
|
**Impact:** cosmetic for direct use (the service works), but real for automation:
|
|
`docker compose up --wait` (used by the `reset-db` Make target) and the
|
|
"all must be healthy" verification step will hang/fail on this host.
|
|
|
|
**Proposed minimal fix (one word):** change the healthcheck URL from `localhost`
|
|
to `127.0.0.1`:
|
|
`test: ["CMD-SHELL", "wget -qO- http://127.0.0.1:7700/health | grep -q available"]`.
|
|
This is a deviation from the frozen setup doc, so it is left for explicit approval.
|
|
|
|
## OQ-5 — Relative BLOB_ROOT breaks across processes with different CWDs
|
|
|
|
**Status:** resolved in dev by using an absolute path.
|
|
|
|
`BLOB_ROOT=./var/blobs` (relative) resolves against each process's working
|
|
directory. Per the dev-setup doc the API runs from the repo root (`uvicorn
|
|
--app-dir backend`) while the worker runs "from backend/", so blobs written by the
|
|
worker land in `backend/var/blobs` but the API reads `./var/blobs` at the repo
|
|
root — a different, empty directory. This only surfaced once the API began reading
|
|
blobs (M1 re-sanitize-on-render / attachments); before that the API only served
|
|
sanitized HTML from Postgres.
|
|
|
|
**Fix:** `.env.dev` now sets an absolute `BLOB_ROOT`. Additionally
|
|
`GET /messages/{id}/html` falls back to the stored (possibly stale) sanitized HTML
|
|
if a blob is unreadable, so a path/permission issue degrades gracefully instead of
|
|
500-ing. `.env.example` keeps the relative default with a note; production should
|
|
always use an absolute path or run both processes from the same CWD.
|
|
|
|
## OQ-4 — Duplicate Message-ID / bulk-sender over-merging (threading)
|
|
|
|
**Status:** resolved with two refinements (both consistent with §7.1's stated intent).
|
|
|
|
**Symptom (found while testing):** home.pl's welcome mailer reuses one static
|
|
`Message-ID` (and `Date`) for every recipient. Two welcome mails — one per test
|
|
account, 104 days apart, byte-identical — collapsed into a single thread.
|
|
|
|
**Cause & fixes:**
|
|
1. **Duplicate Message-ID (§7.1, was only half-implemented).** The indexer now
|
|
synthesizes a non-authoritative id when a Message-ID is already present in a
|
|
**different account** (a separate delivery). A **same-account** duplicate keeps
|
|
the real id, so Gmail-style INBOX + All Mail copies of one message still thread
|
|
together. Implemented in `app/indexer/indexer.py::_resolve_message_id`; new
|
|
index `ix_messages_user_msgid` (migration 0002).
|
|
2. **Subject fallback (§7.3 refinement).** The literal rule ("same subject + within
|
|
7 days + shares ≥1 participant") over-merges bulk mail, because the shared
|
|
participant can be just the common sender. Subject joining now keys on the
|
|
**incoming message's recipients (To/Cc)** overlapping the thread — a genuine
|
|
reply is always addressed to someone in the thread, a bulk blast to another
|
|
account is not. `app/threading_/threader.py::_find_by_subject`.
|
|
|
|
Both are minor tightenings of the spec's anti-over-merge intent, not behavior the
|
|
spec forbids. Covered by `tests/indexer/test_duplicate_msgid.py` and updated
|
|
threading vectors.
|
|
|
|
## OQ-3 — `__Host-` session cookie vs. plain-HTTP dev
|
|
|
|
**Status:** resolved with a documented dev fallback.
|
|
|
|
SPEC §6.1/§10.3 mandate the `__Host-session` cookie prefix, which the browser
|
|
only accepts when the cookie is `Secure` **and** sent over HTTPS. The dev setup
|
|
(`dev-environment-setup.md`) serves the API over plain HTTP (uvicorn :8000 behind
|
|
the Vite HTTP proxy), so a `__Host-`/`Secure` cookie would be silently dropped and
|
|
login could never work in dev.
|
|
|
|
**Decision:** cookie naming/flags are derived per-request from the scheme
|
|
(`app/identity/cookies.py`): over HTTPS we emit the spec-mandated
|
|
`__Host-session` + `Secure`; over plain HTTP (dev only) we fall back to `session`
|
|
without `Secure`. Production (behind TLS) is fully spec-compliant. `SameSite=Lax`,
|
|
`HttpOnly`, `Path=/` always. The CSRF cookie follows the same scheme rule but is
|
|
readable by JS (double-submit, SPEC §10.3).
|
|
|
|
## OQ-2 — Repository was not a git repo (resolved)
|
|
|
|
**Status:** resolved.
|
|
|
|
The setup doc's conda `activate.d` hook locates `.env.dev` via
|
|
`git rev-parse --show-toplevel`, and the overall workflow (commit-per-increment,
|
|
per-plugin alembic branches) assumes git. The working directory was not a git repo,
|
|
so `.env.dev` did not auto-load on `conda activate`. Ran `git init` (branch `main`);
|
|
the hook now loads the env correctly. No spec deviation — the doc already assumes git.
|