mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
main
40
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
13303b247e |
v0.3.9 milestone (#283)
* Run the end-to-end suite on pull requests into milestone and wip branches
A release's work is now collected on a milestone branch (milestone/v0.4.0)
before it reaches main, and the suite only ran on pull requests into main,
so nothing proposed for the milestone was tested. `milestone/**` joins
`main`, and `wip/**` comes along from wip/28, where it already runs the
suite on the phases of #28 — the same block on both, so they merge
without a conflict.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit
|
||
|
Paul LütznerandClaude Opus 5.5
|
9c76bbc636 |
The blob prune follows APP_STORAGE_DIR and clears orphaned thumbnails (#234)
app:prune:blobs looked under a hardcoded var/, so installs with another APP_STORAGE_DIR reclaimed nothing; it and the thumbnail cache now use APP_STORAGE_DIR. A third pass deletes attachment thumbnails whose part no longer exists. Closes #232, closes #233. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
fbf7c0ea6f |
The weekly blob prune only sweeps the per-account trees (#228)
app:prune:blobs deleted week-old profile pictures, signature images and backgrounds under var/uploads because no row named them. The orphan sweep now only enters numeric account directories and only files in each writer's exact layout, so everything else on those volumes is left alone, including directories added later. Closes #214. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
|
b453b79eb4 |
Single sign-on only: hide the password form and refuse password sign-ins (#210)
* Single sign-on only: hide the password form and refuse password sign-ins Adds Admin -> SSO 'Single sign-on only' (and OIDC_DISABLE_PASSWORD_LOGIN). The login page drops the password form and LoginFormAuthenticator refuses a password sign-in before looking the user up. It only applies while a whole provider is in force, the saving administrator must already be linked to the provider, and app:sso:allow-password-login brings the form back from the console. Config restore leaves it off. Off by default; one boolean column on oidc_config. Closes #169 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Single sign-on only: require a link to the provider being saved The admin form let the box be ticked by an administrator linked to any provider, including the one the page held before. Switching providers and ticking it in one save made a provider they had never used the only way in. It now compares the stored issuer with the one on the page, without a trailing slash, as OidcDiscovery does. Tests for what had none: the console command (clears the setting, keeps the provider), a restore over a row that had it on, the link to another provider, and a control showing the same password signs in with the setting off. The "right credentials" test now posts the right password. Docs: there is no iOS app. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Docs: put the iOS app back in the single sign-on page The previous commit dropped it on the wrong assumption that there is no iOS app. There is one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Paul Lützner <karatektus@gmail.com> |
||
|
Philippe Delodder
|
cbe14ea5ae |
Drop a header block the server repeats at the top of the body (#177)
Closes #173. Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com> |
||
|
Philippe Delodder
|
dda54a3876 |
Outlook's <![endif]--> no longer leaks into a message as text (#178)
Closes #175. Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com> |
||
|
|
09c503c61c |
Sign in through an OpenID Connect provider (Authentik, Keycloak…) (#106)
plMail could only sign people in with a local password. On an install that already runs Authentik or Keycloak, that is one more account and password per person, outside the provider's access policy. A second authenticator on the `main` firewall, beside the password form. Both end in the same token for the same User, so the user checker, the login throttle and TOTP two-factor apply unchanged. Authorization code flow with PKCE, state and nonce on league/oauth2-client's GenericProvider; endpoints come from the provider's discovery document. - Set up under a new Admin → SSO tab; the OIDC_* variables are the fallback for an install configured from its compose file, and a saved provider wins whole. This lets an administrator who points it at a provider of their own sign in as any account without a second factor; the security model says so. - The ID token's signature is not verified: it only ever comes from the token endpoint over TLS (OIDC Core § 3.1.3.7 step 6). So the issuer and endpoints must be https and no request to the provider follows a redirect. Accepting an ID token from anywhere else needs JWKS verification first. - An account is linked by (issuer, sub). The address is used for the first link only, and only when the provider reports it verified. - Auto-creation is off by default and creates an ordinary account with no usable password. - app:user:oidc-unlink forgets a link from the console, for somebody deleted and re-added at the provider. Migration: two nullable columns and a unique index on `user`, and a one-row table `oidc_config`. Nothing backfilled; reversible. Not run: Playwright on this branch. No automated test makes a round trip against a real provider; that was checked by hand on a local test stack. Closes #91 Co-Authored-By: Paul Lützner <mail@pluetzner.de> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
6f24bfabc3 |
Write down the Gmail messages an import declines, and stop fetching them again
An import sat at 21,964 of 24,199 and said "waiting" (#90). The 2,235 are messages the batch handler fetches and does not store: its headers tie the message to no account here (a draft, a chat, mail sent from a "send mail as" address), or Gmail lists it and answers 404. Declining left a debug line and nothing else, and the import's only test for "still to come" is "not stored". So: every hourly listing named all 2,235 again, and each was fetched in full again: 134,100 units of the 360,000 an hour a 6,000 project has the import could only end at BACKFILL_MAX_ATTEMPTS, a day of listings later; the account in the report was on attempt 18 the progress line could never reach Google's total, which counts them gmail_skipped_message records each one when it is declined: the reason, its labels and its From, To, Cc and Delivered-To. newGmailIds() treats a recorded id like a stored one, so a listing with nothing else outstanding settles at once an onlyMissing batch queued by an earlier listing drops them too ImportProgress counts them as done app:gmail:skipped prints the record by kind, label and address; --forget clears it so the next listing asks again A row goes when its message is stored after all. Whether some of these messages ought to be stored is a separate question and is not touched: the filter decides exactly as before. The record is what that question can now be answered from. Migration: one new, empty table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
6c018ded6d |
Reconnect an IDLE the server closed, instead of spinning on it
The library says "empty response" for a read that timed out and for a connection the server has closed, and the IDLE loop took both for a quiet mailbox. On a closed connection every read returns empty at once, so the loop never blocked again: a core per folder until the 29-minute re-issue. Lark and QQ close idle sessions within minutes, so for them this was every session (#94: 85 minutes of CPU in three hours for one inbox, against half a second for a Gmail one). connectionEnded() tells them apart by the stream being at end-of-file, or by the read having come back in under a second when a quiet one blocks for thirty — a TLS stream does not always report end-of-file. A connection that lived a minute or more is reconnected at once and counts as no failure. One lost sooner is thrown to the retry ladder, so a server that accepts IDLE and hangs up cannot become a reconnect loop in place of a read loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
748f0f302f |
Count a Gmail message fetched in full at 60 quota units, as Google bills it
Google's quota table gives messages.get one cost, 20. An import paced on that was refused at a third of its limit with no other call being made, so app:gmail:quota-probe fetched thirty messages each way in a minute of their own and the project's quota meter was read afterwards (#90): format=full, batched 1,800 units 60 each format=full, one at a time 1,800 60 format=raw 1,800 60 format=metadata 600 20 format=minimal 600 20 The table's 20 is a message without its body. The import's own traffic agrees: 96 fetches a minute beside 5,800 units on the meter. the cost 60 a message, so the pace starts where Google's count is and is not found by being refused requests five messages each, three seconds' worth of a 6,000 limit the share 85% of the limit, where 60% of a count three times too low had been nearly twice the limit the probe sets the meter's reading beside the table's 20, not beside whatever the pacer assumes On a project allowed 6,000 units that is 85 messages a minute, and the time left on the import line is reckoned from it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
ec6199ca74 |
Let the quota probe say which Gmail accounts there are
It asked for an account id, which nobody knows and the web interface does not show. The argument now takes an id or an address, and without one the command lists the active Gmail accounts and asks which — or, run without a terminal to ask on, prints the list and stops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
350c71a9dd |
Add app:gmail:quota-probe, to read off what Google charges for a fetch
Google's table says a messages.get is 20 quota units; its meter charged about 59 for each of an import's (#90). The traffic shows no other call to blame, and Google reports no cost per request, only the minute's total in the Cloud console. So this makes a minute's total mean something: a fixed number of stored messages fetched one way — batched as an import does it, or singly in each format — started just after a clock minute begins and followed by an empty one. The chart's value for that minute over the number of requests is the cost of one. The account's bulk fetching is held while it runs, by a new GmailQuotaPacer::hold(): the wait a refusal causes without what a refusal means, so no limit is learnt and no pace is cut. Nothing is stored and nothing in the mailbox is changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
4ca055a724 |
Let a backfill that re-reads mail be told how far back to read
`app:backfill events` and `app:backfill insights` re-read stored mail for what an extractor can now find in it. The usual occasion is a reader that was just fixed and a mail from this week; run over a whole mailbox they also dig up every calendar file and parcel since it began — and since calendar files that name nobody here are now offered rather than added, that is a card on every one of them. Both take --since and default to the last 24 hours: a count with h, d or w, a date, or "all". Anything else is refused and nothing runs. The command prints the window before it starts. A separate interface, WindowedBackfillTaskInterface, rather than a parameter on run(): the other tasks repair a table as a whole, and one that accepted a window and ignored it would look as though it had done what it was asked. Given --since, they say they take none. The insights walk is keyset on the id now instead of OFFSET. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
88b1744385 |
Leave the ingest worker to fetch mail, and nothing else
Adding an account put new mail behind hours of follow-up work. What
plMail does to a message once it has it — a model call where the
assistant sorts, event extraction, insight cards, receipts — was queued
on `ingest`, beside the syncs, for every message of the import, oldest
first. One consumer serves that queue. 988 envelopes pending and the
oldest an hour old was a mailbox being asked about newsletters from
2019 while mail on the server waited to be fetched.
`ingest` is now what talks to a mail provider. The rest moved:
enrich_live worker-live follow-up work for mail that has just
arrived on an account whose import is
over; never behind an import
enrich worker-enrich the same for an import's recent mail,
and search indexing
enrich_backlog worker-enrich old mail for the assistant, consumed
only when `enrich` is empty
release worker-release ends a hold (below); nothing else
Which queue is decided per batch, not per message class: the pipeline
asks InitialImportState once, carries the answer on PostIngestResult,
and EnrichmentRouter turns it into a stamp.
Only mail from the last 30 days (ENRICH_RECENT_DAYS) gets follow-up
work on arrival. Older mail is stored, threaded and filed by the rules.
The assistant is asked about it afterwards — newest first, a bounded
handful per run, and not until every account of that person has
finished importing (app:ai:classify-backlog, ClassificationCatchUp).
For the other steps `app:backfill` remains the way to read history.
ExtractInsightsStep and ProposeEventsStep ran their work inside the
sync and argued for it; both dispatch now.
New mail no longer appears in one tab and moves to another. For
somebody sorting by assistant the rules' category is a guess about to
be replaced, so where the verdict could still change it the message is
held: stamped before threading, its new thread given no category, which
every tab query already reads as "in no tab". The answer releases it.
Failing that a timed release does, after AiSettings::holdMaxSeconds —
60 by default — and the mail is shown where the rules put it; the model
call carries on and moves the mail when it answers. A message is asked
about once. Admin → AI has the switch, the ceiling, and the measured
delay over the last week beside them.
Three things about the release that were each wrong once first:
- It ran on the live worker, so it could not fire while that worker
was inside the call that was taking too long. It was moved to
export, which mixed mail appearing with mail leaving. It has a
queue and a worker of its own.
- The call was cut off at the ceiling and the message asked about
again later. The hold and the question are separate; only the hold
has a deadline.
- A late verdict was stored and moved nothing. It moves the mail.
Both workers still end at one model host, so bulk classification looks
at the live queue before each call and stands aside (LiveMailPriority).
One migration: two columns on `message`, two on `ai_settings`.
Not verified against a real import or a live model host: the tests pin
the queues, the hold and the release against in-memory transports and a
mocked host. Known and left: IMAP still imports oldest first; a reply
into an existing thread cannot be hidden; a Gmail account counts as
importing for up to an hour after its last message lands.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Opus 5.5
|
c8a21d1f7b |
Keep a link around the table it was wrapped around
A job alert whose every card is one link around a table had no working links to the jobs. The step that flattens a mail's stylesheet onto its elements parsed with libxml's HTML 4 parser, and HTML 4 does not let an <a> contain a table, so that parser closed the link where the table began. The stored body had an empty link and, after it, a card that went nowhere. The href survived, so nothing looked stripped. The inliner now works on a tree the HTML5 parser built, handed to the library as XML so the old parser never sees the markup. Every other step already parsed that way; this was the first one and the only one that did not. A body the hand-over cannot carry falls back to the old parse. Two things change with it for mail that was not broken, both towards what a browser does: misnested inline tags are repaired by the HTML5 rules, and Outlook's <o:p> no longer leaves empty paragraphs behind. The library's pretty-printing whitespace between blocks is gone too. app:backfill safe-html-table-links re-sanitizes stored mail with a table after a link. Checked against a planted message in the test stack: repaired, and the card shows its link on hover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
878229fb89 |
fix(deploy): a database that answers late no longer puts an error in the log
The admin log showed CRITICAL console (web) Error thrown while running command "dbal:run-sql -q 'SELECT 1'". Message: "... could not translate host name "database" to address: Name or service not known" for a php container that had started while the database container did not exist yet. The entrypoint waited for the database with a shell loop that ran `dbal:run-sql -q "SELECT 1"` once a second, and every miss was an uncaught console exception, which Symfony logs as CRITICAL. Only a miss that fell just before the database answered could be stored - the log is a table in that database - so the admin log showed exactly the misses that did not matter, and never a database that stayed away. app:db:wait is that loop in one process. It asks through PostgresStatusRepository::whyUnreachable(), retries a miss without logging it, and logs once, with the last error, when all sixty attempts have failed; the container log reads as before. isReachable() uses the same probe, so /healthz also hangs up after a failed check and dials afresh on the next one. Measured against a Postgres that refuses every other login, which makes "answered a moment later" deterministic: the old probe stored exactly the reported row, app:db:wait missed once, answered, and stored nothing. The real entrypoint in the app image, started before its database, waited through two misses and came up with nothing logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
6edc6c324c |
fix(deploy): the cache table exists before any worker reads it
A fresh install logged a warning about once a second from the worker, until an administrator happened to press "Restart workers": Failed to fetch key "workers.restart_requested_timestamp": ... relation "cache_items" does not exist The restart signal and the notice throttle live in cache pools on Symfony's DBAL adapter, which creates cache_items on its first write. Every Messenger consumer reads the signal on every loop, long before anything writes it, and each failed read is logged. The migrations leave the table out on purpose, because the adapter owns its DDL. app:db:migrate now has each DBAL adapter create its table when it is missing, after the migrations and inside the advisory lock, so php and the worker cannot race each other to it. The adapter still owns the DDL: it names its table through configureSchema() and creates it with createTable(). Best effort, like the statement statistics: a failure is a warning, not a container that will not start. Existing installs get the table on their next boot. The adapters are collected by class in a compiler pass rather than injected by pool id. In dev and test the profiler wraps every pool in a TraceableAdapter, which has no createTable(). It is the lookup DoctrineBundle uses for its schema listener, and it covers any DBAL pool added later. Checked on two fresh compose.yaml + compose.demo.yaml stacks: 95 warnings in 75 seconds without this, none with it, and Restart workers still reaches every consumer through the table created at boot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
1aadc53218 |
fix(security): prune finished jobs and expired trusted devices nightly
Both tables grew forever: TrustedDeviceRepository::pruneExpired() and BackgroundJobRepository::findFinishedBefore() had no caller. The nightly app:monitoring:prune now deletes expired trusted devices and background jobs finished more than 30 days ago (--job-days). The job lookup became a bulk DELETE, since hydrating would scale with the history and trip over any row whose kind the enum no longer lists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5.5
|
4b885ec4b7 |
fix(snooze): cap the due-snoozed batch by thread, not by joined row
findDueSnoozed() put setMaxResults() on a query fetch-joining the messages, so the limit counted message rows and the last thread in a batch could arrive with part of its conversation missing. Select the capped ids first, then load those threads whole. Covered by WakeSnoozedCommandTest (fails on the old query). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
422fd1f489 |
fix(imap): read the flag notification instead of re-listing the folder
An idling server announces a flag change as `* 12 FETCH (UID 5001 FLAGS (\Seen))` — which message, and its complete new flag set. The IDLE loop tested that line for the substrings "FETCH" and "FLAGS", learnt only that something had happened, and answered by dispatching a full mailbox sync: a fresh IMAP session and `UID FETCH 1:* (FLAGS)` over the whole folder, to rediscover what the line had just said. On a 1832-message inbox that is one full listing per message somebody marks read on their phone. It is also self-sustaining: the listing opens a second session, the server broadcasts flag state to the idling one, and that dispatches the next listing. Seen in the wild at roughly two per second indefinitely, nine identical jobs in the queue at a time, a sustained ten megabits a second of an installation talking to itself. Nothing was lost or corrupted, which is why it went unnoticed — the only symptom is bandwidth. readFlagChange() now parses the line into an ImapFlagNotice. When it carries a UID — optional in RFC 3501, required in RFC 9051, and sent by Dovecot and Gmail — the change is applied to that one message through ApplyRemoteFlagsMessage: a point lookup on the unique (mailbox, uid) and a write through ThreadStatusUpdater, which is where the echo guard lives and therefore the only door inbound flags may use. No connection, no listing, no round trip. Without a UID the line names a sequence number, and sequence numbers move under every expunge, so there is no honest way to resolve one here — that case still lists, and guessing would write the wrong message's flags with no error anywhere. The listing is now debounced at five seconds, leading edge plus a trailing flush from the read-timeout path so a burst that ends immediately still gets its sync. markSweepDue() moved into that flush: it forces the expensive full listing, and it was being called once per notification rather than once per burst. New mail is unaffected on the leading edge. Second line of defence in the handler, because the debounce is a property of one caller: a sync whose mailbox was synced within three seconds returns instead of reconnecting, so a duplicate from any future dispatcher costs a lookup rather than a folder listing. The parse is stricter than the substring test it replaces and refuses more: a sequence number is now required, which is what excludes the `* FLAGS (…)` and `* OK [PERMANENTFLAGS (…)]` lines every SELECT answers with. An empty `FLAGS ()` is kept — it means every flag was cleared, which is how "marked unread elsewhere" arrives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
755f3cf257 |
feat(users): an administrator can run the whole life of an account, and you can change your own password
Managing users was reachable and worked — list, search, create, promote, demote, remove — but three things around it did not, and one of them was load-bearing. ── You could not change your own password ────────────────────────────────── Nowhere. Not in Settings, not by mail, not at all; only an administrator setting one for you. Settings → Security now leads with a Password card that asks for the current password (an unmapped UserPassword constraint, so it is checked against the signed-in token rather than against anything the form was handed) and takes the new one twice. The route carries no id and the form has no data_class, so it structurally cannot become the admin form by accident. The floor those passwords are held to was three literal 12s in three files that happened to agree. It is now User::PASSWORD_MIN_LENGTH, read by all three. ── An account could only be removed, never switched off ──────────────────── Nothing sat between "leave it alone" and "destroy it", so the answer to "this person has left, keep their mail" was to delete them. There is a deactivated_at now, and a UserChecker registered on BOTH firewalls — the JMAP one included, because an app password outlives the browser session it was minted from and a check only on `main` would shut somebody out of the web UI while every mail client they had already connected kept syncing. The refusal is in checkPostAuth rather than checkPreAuth on purpose. checkPreAuth runs before the password is verified, so refusing there would answer "is this address a suspended account here?" to anyone who types an address — the question hide_user_not_found exists to keep unanswerable. After verification, the only person told is the one who could otherwise have signed in. Symfony runs user checkers at authentication only, so a session already open would have carried on until it happened to end. UserProvider::refreshUser() refuses the same two states on every request. Suspension also travels through config backup, which would otherwise have handed a suspended account its sign-in back on restore. ── Two smaller lies ──────────────────────────────────────────────────────── A search under three characters silently returned the unfiltered list, which a QueryBuilder has no way to complain about, so the floor was dropped rather than kept as something that could only mislead. And an admin declining to demote themselves got a success toast for a change that never happened; the refusal now says so, while the rest of the form still saves. ── Restoring a removed user is deliberately absent ───────────────────────── Removal overwrites the address, the name and the hash specifically to free the unique address for reuse, so a restore would have to invent all three and the original address may by then belong to somebody else. Doing it properly means snapshot columns and a guided flow that asks for a fresh address — a different change, which alters what removal means. Documented as absent on the screen an administrator would look for the button on, because switching an account off is the answer to the question people are actually asking. Also fixed en route: the modal form theme dropped help_translation_parameters, so the first field to use one rendered "at least %count% characters" on screen. And GermanIsInformalTest only ever globbed messages.de.yaml, leaving validators and the new security catalogue — exactly where a translator reaches for "Sie" — outside the test that exists to catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
2ca7824783 |
test(ai): two tests that only failed on a machine that had run the app
Both passed in CI against a fresh database and failed locally, which is the shape of a test that reads state it did not write. AiBackfillStateRepositoryTest works in a transaction, and the transaction unwinds what the tests write — not what was already there. The table is a singleton, so on any machine that has run the app its one row is present before the first test starts, carrying a real interactive_seen_at from whenever somebody last opened the composer. touchInteractive() refuses to move that stamp backwards, which is the behaviour it exists to guarantee, so a row stamped later than the fixed date the test picks made the test fail against its own input. The row is now cleared in setUp, inside the transaction, so each test establishes the state it asserts on; the rollback puts the original back. IndexNewMailCommandTest::testItSweepsEveryMailboxWhenNoneIsNamed asserted the queue was exactly its own fixtures after an UNSCOPED sweep. The first test in the same file carries a docblock warning about precisely this — "it would be an assertion about what else happens to be in the database, which is the shape of a test that fails on a second afternoon" — and scopes itself with --email to avoid it. The unscoped one relied instead on the other seeded mailboxes being empty, and they stopped being empty. It now asserts the claim that is actually worth holding: the mailbox nobody named still got swept, newest mail first, and the ceiling still stopped it short of draining the mailbox. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
92d6e8ffab |
feat(ai): index new mail when the model is warm, and once a night
New mail was embedded the moment it arrived. That is the wrong trade on a box
with one GPU: it is rare to search for mail that has just landed, and paying to
index every arrival competes with the two things somebody is actually waiting
on.
Two triggers replace it. A nightly sweep is the backstop, and a search is the
invitation — a search has just loaded the embedding model, so the cheapest
possible moment to index is immediately after one, when the model is warm and
nobody is waiting on it any more.
THE PART THAT MADE THIS MORE THAN A SCHEDULE CHANGE
The backfill stood aside after ANY interactive AI use, and a search counted.
That is precisely backwards for search, and it took both halves of the signal to
say so: lastInteractiveCallAt() matched
`feature IN ('search_query','writing_help')`, and the request subscriber stamped
`app_mail_search` alongside `app_compose_assist`.
The distinction the old code could not make is that there are two models with
completely different costs. The composer spends the 20.3 GiB writing model,
which is worth standing aside for and is the case the yielding was written for.
Search spends the sub-gigabyte embedder — the same model indexing uses. So a
finished search is not a reason to keep away, it is the best moment there is.
Both docblocks now cross-reference each other, so restoring one half alone reads
as obviously wrong.
The opportunistic trigger claims its rate-limit marker BEFORE it queries, so the
mailbox scan is inside the window rather than only the dispatch, and a search
that produced no vector at all spends nothing — one unreachable moment must not
cost the next five minutes of indexing.
EmbedMailStep is deleted rather than emptied. An empty step would still be built
and iterated on every ingest batch while carrying a name asserting that
embedding happens on arrival — an active lie in the one directory somebody reads
to find out what happens to new mail.
The finder matches on model and not on width, deliberately. What consumes its
ids asks about the model alone, so a width-aware finder would hand over a full
budget the handler then drops as already done: a nightly sweep that reports
itself busy and indexes nothing, for ever. The width mismatch already has an
owner in the coverage report's "stale" count, which tells the user to re-index.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Opus 5
|
92004ee576 |
fix(jobs): background work would not open, and finished work never left
Two faults in the same indicator, reported together as "it fades in and out but clicking does nothing" plus three "Marking as read" jobs stuck at partial progress that had stopped weeks earlier. The menu was hidden by Tailwind's `hidden` CLASS while ui--dropdown opens a menu by clearing the `hidden` PROPERTY and never touches classes. So the panel was display:none for the whole life of the page — and because the attribute was absent, the first click read the menu as already open and CLOSED it. No error, no console warning, nothing to search for. Every other dropdown in the app uses the attribute; this was the one that did not, and a static test now says so. The stuck jobs were a different fault with the same face. findVisibleForUser matched `state IN (queued, running)` with no time bound of any kind, so "still going" and "abandoned" were literally the same row and no amount of waiting would ever change it. A job that fails properly calls finish() and clears after five minutes; these never reached that line — a killed worker, a restart mid-deploy, a fatal that bypassed the failure path. The entity had no progress timestamp, so a long legitimate job could not be told from an abandoned one by time. It has one now, stamped by begin() and advance() so the counter and the stamp cannot come apart, the query is bounded by it, and app:jobs:reap marks the stragglers failed so they are seen once and then go. The migration backfills last_progress_at from created_at, which is the only value that gets both directions right: a job stranded weeks ago is instantly outside the window and swept, while a bulk action started two minutes before the deploy is inside it and left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
231d0ab0da |
feat(demo): a public demo mode, with mail that arrives on demand
plMail is hard to show. It is a mail client, so everything interesting only
happens once there is mail — and a stranger cannot reasonably be asked to hand
a demo server their IMAP password to see it. APP_DEMO_MODE fills that gap: mail
that is nobody's, and a button that makes more of it arrive.
One switch for the whole install, not a per-account flag. The promise is that
nothing reaches the network, and that is only checkable if there is one place
to check; a per-account flag would move the promise into every outbound path
individually, where it holds until the first one that forgets to ask. Read
through `default:` so an install that never heard of the variable is a normal
install — the accident worth designing against is a real deployment becoming a
demo, not the reverse.
**A mailbox per visitor.** /demo provisions a throwaway user, signs them in and
drops them in a seeded inbox. One user each rather than one shared account: two
people arriving at once would otherwise be marking each other's mail read and
reading each other's drafts, and the product would look broken while working
perfectly. app:demo:reap deletes them on a timer.
**Receiving.** A bar at the foot of the page walks a scripted queue — a reply,
an invoice with a real attachment, a delay notice, an HTML newsletter — and
wraps at the end, naming what comes next. Delivery goes through the real
PostIngestPipeline rather than writing rows by hand, because what is being
demonstrated is that plMail *does* something when mail arrives: it threads, it
categorises, it runs the filters the visitor just built, and it appears without
a reload. A hand-written row would put mail on screen and none of that would be
true, which is a worse demo than no button.
**Sending.** DemoMailSender is registered above every real sender and claims
every account while demo mode is on, so SmtpMailSender is never asked — "a demo
cannot reach a relay" becomes a property of the wiring rather than a rule each
sender has to remember. It answers a few seconds later, from whoever the visitor
addressed, on the thread they are still looking at.
**What is switched off**: adding or editing a mail account, OAuth and CalDAV
connect, calendar subscribe — a stranger typing their real credentials into a
box on someone else's server has handed their mail away, and there is no version
of that form that is safe to show. Mail sync, push renewal and calendar sync
no-op, since a demo's accounts point at documentation domains. Everything else
stays clickable: locking down all the non-mail surfaces produces a tour of a
product with the interesting doors locked.
Two things this needed that did not exist. The demo mailbox moved out of
SeedDemoMailboxCommand into DemoMailbox, shared with the provisioner, so the
README screenshots and the hosted demo cannot drift into being pictures of
different things. And deleting a user — this project had no such path, because
a self-hosted install's users leave by having their password changed. The
eraser discovers owned classes from Doctrine's metadata rather than a list:
twenty entities point at User today under two different property names, and a
hand-written list would be right until the next entity was added, at which
point the reaper would start failing on a foreign key at three in the morning.
compose.demo.yaml hardcodes APP_DEMO_MODE rather than interpolating it, and
ComposeEnvironmentTest now holds it to that. Compose reads Symfony's committed
.env, which ships the variable as 0, so `${APP_DEMO_MODE:-1}` would have
resolved to 0 for everyone who cloned the repo — and the failure is the bad
kind, where the stack comes up healthy and simply is not a demo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Opus 5
|
0dfe02dd4f |
feat(push): a way to cancel a Graph subscription plMail no longer knows
The companion to the renewal fix, for the orphans already out there.
"GraphNotification: unknown subscription" names an id, and the endpoint
that logs it can do nothing with one: without a matching account there is
no token to cancel it with, so it kept saying so once an hour until
Microsoft let the registration lapse — up to three days.
Naming the account on the command line is what makes it actionable. Any
account in the same mailbox can revoke it, and the id from the log line
is the only other thing needed:
php bin/console app:push:cancel-subscription --account=10 \
--subscription=701dc0d3-b4ed-4ad8-94f2-a263a90a30bb
Every test here is a wrong invocation, deliberately: the successful path
needs Microsoft, and what actually costs somebody an afternoon is a
command that takes a mistyped account id and says nothing useful. A Gmail
account is refused before any call goes out, since a DELETE carrying a
Google token would report whatever Microsoft said about it and explain
nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Fable 5
|
cded2b9bca |
fix(mail): recipient line shows the address, and account dots are one colour each
The at-a-glance recipient rendered an owned address as 'me' — on your own received mail that is every message, saying nothing. It shows the address the mail actually reached now. And the sidebar account dot was a green/grey status light identical for every connected account; it now carries the account's identity colour, the same one its rows wear in the unified lists, assigned by position so a handful of accounts can't collide the way a hash did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Fable 5
|
6e7bf67ad0 |
feat(mail): the message header says who the mail was addressed to
Opening a message showed the sender and a "Details" expander, and on IMAP that expander had no To: line inside it either. The template was not the problem. webklex hands To/Cc/Bcc over as an Attribute, which implements ArrayAccess and only ArrayAccess. `foreach ($attribute as $address)` over one of those is neither an error nor an iteration: PHP falls back to walking the object's public properties, and Attribute has none. The loop ran zero times, silently, for every message ever synced over IMAP — which is also why reply-all had nobody to reply to, why the thread's participants were derived from the sender alone, and why no contact was ever harvested from a recipient. Gmail parses the raw To header and Graph maps toRecipients; both were already right, and Gmail's enrichExisting path never touches recipients, so nothing there needed changing. Nothing was lost along with the recipients. The headers themselves reach the row by a different path and are all still there, so `bin/console app:backfill recipients` re-derives the columns from the row itself — no mail server, no raw MIME, no resync and no reset. It fills only what is empty, so a good capture is never overwritten by a poorer re-parse, and a row whose header genuinely names nobody stays empty and is counted as such. The header reads the same bag while rendering, so a mailbox nobody has backfilled reads exactly like one that has. The header row now carries the recipients instead of the word "Details": up to three names, "+N" for the rest, "me" for any address this account owns — aliases included, via ownedAddresses rather than the bare email — and, when there is genuinely nobody, "undisclosed recipients". A message with no To: is an ordinary thing (a bcc delivery, a list that hides its members); a line that silently disappears is not, because an absent line reads as a bug and a stated absence reads as a fact. The same header now also appears in the single-message view, which until now named the sender and nobody else at all. Every address in the expander gained a copy button, and the panel gained one for the whole block, both on the clipboard controller that was already there. It grew an optional icon target: an icon-only button had no confirmation at all, since the source highlight behind it is invisible when the source is. The block is composed in Twig and copied from a hidden element, so what lands on the clipboard is what the panel shows, translations and all — and it is assembled with map/join rather than through the address macro, because macro output is already-escaped markup and printing it again would put `Alice <alice@…>` on the clipboard. While in there: the six call sites that let the controller's English defaults through now pass translated ones. Reply-to, mailed-by and signed-by also start appearing on IMAP mail. The panel looked up `reply-to`; webklex stores `reply_to`. Header names are canonicalised to one spelling on the way in now, so both provider conventions land in the same bag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Fable 5
|
7ddbe23db9 |
feat(imap): mail read anywhere else is read here too, on every provider
plMail never re-read the flags of a message it had already stored. They were captured once at ingest and never again, because incremental sync asks for `lastSeenUid+1:*` and no UID below the high-water mark is ever looked at twice. Mail read on a phone stayed unread here forever; a star set in Roundcube never arrived. Not eventually — never. The folder listing carries flags now. `UID SEARCH ALL` answered presence and nothing else, and a separate flag pass would have cost that SEARCH *plus* a `UID FETCH 1:* (FLAGS)`; folding them together is one command rather than two and strictly cheaper than keeping them apart, at six times the bytes of the half it replaces. It is also one instant instead of two, so "what is here" and "what state is it in" cannot disagree. The sweep's every refusal — a rebuilt UIDVALIDITY, a listing suspicious enough to be a server rebuilding — refuses its flags with it. The echo race is the hard half. The user marks a message read, the row changes first because the database is the source of truth, and until the outbound job lands the server's honest answer is the old one; believing it would revert the user, which the propagator would see as a fresh local change and queue a job for, which the next pass would revert again. Comparing timestamps does not fix that — the local write happens BEFORE the read, so last-write-wins picks the server every time. What the local change is, is unconfirmed, so that is what is recorded: flagsTouchedAt, set where LabelChangePropagator queues the outbound op for all three providers, cleared where the server accepts it. Such a row is skipped whole rather than merged, because a message whose \Seen is in flight may have had its \Flagged changed remotely and a flag list cannot tell those apart. The guard expires after an hour, or a job lost for good would freeze a row's flags against the server permanently — the same bug in a smaller box. It is enforced in ThreadStatusUpdater::applyRemoteFlags(), which is where every provider's inbound flags land, because one rule enforced once is a rule no future caller can forget. That method is the inbound twin of markRead() and star(): the same seenAt, starredAt, thread star, thread recount and JMAP log, and no propagator call, since these changes came from the server. Gmail and Graph had the same gap and now do not. Graph's delta carries isRead and flag on every entry and partition() read only parentFolderId from it, on the stated belief that location was all that could have changed cheaply. Gmail was worse: relabelled ids are sent precisely because they are already stored, and the batch handler dropped every id it already had before fetching anything — re-applying the exact filter GmailApiSyncer documents those ids as bypassing. Idempotency never depended on it; the loop below looks each id up and enriches. The idle supervisor answers `* 4 FETCH (FLAGS (\Seen))` the way it answers EXPUNGE: which message is a sequence number it deliberately does not resolve, and since the flag pass rides on the listing it is the same clock — backdate sweptAt, dispatch, and a star set elsewhere lands in seconds rather than within two fifteen-minute cycles. Not the FLAGS and PERMANENTFLAGS lines every SELECT answers with, which describe what a folder permits and say nothing about a message. The race is pinned from both sides, which is the point: a local change survives a stale read, and a genuine remote change still lands. A guard that passes only the first has switched inbound flag sync off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Fable 5
|
43c6417f88 |
Stop a document's own charset outliving the bytes it described
"Stop believing a charset label the bytes disprove" fixed the MIME layer, and it holds: a part that stamps ISO-8859-1 over UTF-8 decodes correctly even inside a multipart invite, with a text/calendar part beside it. That was the whole reported shape, and none of it was at fault. The damage was one layer lower. By the time a body is stored it is UTF-8 whatever it arrived as, but the sender's own `<meta charset=iso-8859-1>` came through that conversion unchanged and now describes bytes that no longer exist. It still gets believed: the sanitizer parses with Dom\HTMLDocument, which follows the HTML encoding sniffing algorithm and prefers that tag to any default — so the rendered copy was decoded as latin-1 all over again and "über" reached the screen as "über". Only bodyHtmlSafe was ever wrong; bodyHtml held the correct text the entire time, which is why the plain-text body and the invite widget looked fine and the message did not. The declaration is now corrected before anything parses the document, which is what the HTML specification asks of anything that transcodes one. Note the argument is not isUtf8Despite()'s: that one weighs bytes against a label and declines the multi-byte cases as too close to call. Here there is nothing to weigh — the string is valid UTF-8, so every declaration to the contrary is stale rather than arguable, and a shift_jis one is as wrong as a latin-1 one. Because the damage was local, mail already showing mojibake can be repaired without a mail server: `app:backfill safe-html-charset` re-derives the rendered copy from the body as stored. That is what the previous fix could not offer, and the difference is worth naming — there the bytes were converted wrongly before the app saw them, so a resync ran the same wrong conversion again. Kept separate from the safe-html task, which fills a copy that is missing and can trust what is already there; this one distrusts copies that exist. Sender-side double encoding was tested and deliberately not repaired. A correctly-labelled UTF-8 part carrying "über" does render as "über", but that requires assuming a bug in someone else's system when there was a demonstrable one here, and no repair for it can be contradiction-based the way the label fix is: the bytes are valid, correctly labelled and self-consistent, so any trigger is a guess about what the author meant. "Ã" is an ordinary letter in Portuguese, Vietnamese and Icelandic, and a silent remap would destroy text that was correct while leaving no trace that it had. Mojibake a reader can see through; this they could not. Also: an invite written in latin-1 no longer costs its event. Invites are stored as they arrive and MessagePart keeps the bare MIME type without the charset parameter, so nothing converted them — and one 0xE4 in a SUMMARY was not a mangled title but an INSERT Postgres refused, taking the extraction with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
4d103cfbdf |
Write the handbook, and make it fail the build when it goes stale
README says what plMail is and gets you running; CONTRIBUTING says how to
change it. Nothing covered the part in between — using it, operating it,
and registering the Google and Microsoft applications it talks to. That
is docs/: 34 pages across features, installation, providers and
internals, mirrored to the GitHub wiki.
The wiki is a mirror and never a source. bin/mirror-wiki.php flattens the
tree into the wiki's flat namespace, rewrites every relative link,
generates the sidebar from the index so the two cannot disagree, and
stamps each page with a footer saying where it came from. A browser edit
survives until the next push to main. That cost is deliberate:
documentation not versioned with the code drifts from it, and the drift
is invisible until somebody follows an instruction that stopped being
true.
Which is the whole problem, so the checkable half is checked.
DocumentationCoverageTest fails the build when a command is missing from
the reference or documented and gone, when a variable in .env is absent
from the configuration page, when a relative link dangles, when a page is
unreachable from the index or promised and missing, or when a page has no
"Things that bite" section. It cannot check whether a paragraph is TRUE,
and says so — this is the floor, so review attention goes to the part a
machine cannot have an opinion about. CONTRIBUTING has claimed since it
was written that the command table "is part of the definition of done";
nothing enforced it until now, and it was wrong in three places.
Writing it found things. They are fixed here rather than documented
around, because a handbook describing a bug accurately is still a
handbook nobody can follow.
compose.yaml mounted no volume for var/attachments, var/raw
or var/uploads. Workers write those during sync
and the web container serves them back, so each
kept its own copy in its own layer — downloads
404, and everything died on a recreate. The
override and the TrueNAS file both mount them;
the file the README tells an operator to run did
not. The same omission cost real data once
before. See the CHANGELOG for moving what is
currently stranded.
app:calendar:materialise new, nightly. Occurrences are drawn when an
event is saved and nothing moved that window
again, so a weekly standup ran out of dates in
two years and its reminders stopped with them —
silently. The query it runs had existed since
the materialiser did, documented as exactly this
sweep, and had never been called.
Microsoft wizard asked for three of the nine permissions plMail
requests, and a token already issued is not
upgraded when the rest are added later.
GmailWatchService told you to grant Pub/Sub Publisher to
gmail-api@system.gserviceaccount.com. The
account is gmail-api-push@; a topic granted
exactly what the docblock asked for never
delivered a notification.
IcsExporter said it wrote no VALARM. It has written them
since alerts landed.
CLIENT_DEVELOPMENT.md said there was no JMAP calendar API and that
SearchSnippet/get was absent. Both shipped.
CONTRIBUTING said only app:calendar:push registers a push
channel; subscribing has done it since the
subscribe flow learned to.
Three docblocks named getters removed long ago, or described a
method two below the one they sat on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Opus 5
|
1d12ab5523 |
Put the last five entities on the trait, and stop pretending a timestamp can be null
EventSuppression, UploadedBlob, LogEntry, PushSubscription and ChangeLog still declared createdAt by hand. They take TimestampableTrait now, so no entity in the codebase tracks its own timestamps. Their tables only had created_at, so five gain updated_at. A row written once carries a column that mirrors the other, and that is the right trade: one rule for every entity beats a handful of bytes and an exception nobody can see from the entity. The migration adds nullable, backfills from created_at, then sets NOT NULL. Adding a NOT NULL column with no default fails outright on a populated table, and jmap_change_log is the busiest one in the schema -- every mail mutation writes a row. Both properties are non-nullable now, which is the change that mattered most. They were `?DateTimeImmutable = null` while the columns were NOT NULL, so PHPStan had 35 suppressed entries all saying the same thing: the property can hold a null the database cannot. Typing them honestly deleted all 35 without touching a line of logic, and turned up two dead null-checks -- one guarding a lastMessageAt that could never be null, one a branch in the participant scan -- which are now gone rather than baselined. Uninitialised before persist rather than null, so reading one on an unsaved entity throws instead of quietly returning nothing. PrePersist keeps whatever was already there, so a backfill replaying history still sets its own instant. Two writers bypass the ORM and had to be told about the new column: the log handler, which inserts through DBAL because it cannot flush an EntityManager that may be mid-failure, and the repository test that seeds rows the same way. The suite found both. One test was quietly worthless and is fixed: assertNotNull against a property that can no longer be null passes whether or not the callback ran. It compares the stamp to the clock now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
6081d8f004 |
Let the six remaining entities keep their own timestamps
Account, Contact, Label, Mailbox, Message and User declared createdAt and updatedAt by hand while eleven other entities used TimestampableTrait. They take the trait now, and the seventeen writes scattered across controllers, services and the seed commands are gone -- PreUpdate covers them and cannot be forgotten the way a manual write can. The trait's fields are private(set), which is what makes this safe rather than merely tidier: the writes did not become redundant, they stopped compiling, so none could be missed. Two that neither the suite nor a grep of controllers found turned up that way, in the seed commands, reported by PHPStan. Constructor writes stay. They are inside the entity so private(set) allows them, and they are load-bearing: attachMessageToThread falls back to createdAt when a message has no receivedAt or sentAt, which is exactly a new draft, so a value that only appeared at flush would cost it its place in the thread. One real behavioural change, pinned by a test rather than left to be found. PreUpdate fires only when Doctrine sees a mapped field change, so a save that changes nothing no longer bumps updatedAt. Three of the removed writes had that shape -- a form resubmitted unchanged, an account whose position in a reorder did not move -- and used to stamp the row anyway. The column now says a row changed only when it did. The test that matters most is the reflection one. A trait without #[ORM\HasLifecycleCallbacks] is inert and Doctrine says nothing at all: no error, no failure, the timestamps just quietly stop. It checks all six, and removing the attribute from one makes it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
50fc8131c9 |
Refuse to destroy a mailbox whose child was created in the same request
Mailbox/set destroy counts a label's children before refusing, and the create path set the child's parent without telling the parent about the child. Doctrine needs only the owning side to persist the row, which is why assigning parent looks like enough -- but the parent's in-memory collection stays empty for the rest of the request, so the guard counts nothing and lets the destroy through. JMAP makes that reachable rather than theoretical: create and destroy travel in the same Mailbox/set, and several method calls travel in one request. A client could add a child mailbox and delete its parent in a single round trip, and the refusal that exists to stop exactly that would not fire. Label::addChild already maintains both sides; the create path simply was not using it. Found by tests written for the coverage work rather than by anyone reading the code, and it is worth saying why they found it: the assertion came from the Android client's notes on server behaviour, which record mailboxHasChild as something that was observed once against a running stack. Turning an observation into an assertion is what turned a working behaviour into a failing one. Also adds the Command and Jmap coverage those tests came from -- 871 to 1048 -- and fixes three things they arrived with: two enums imported from the wrong namespace, and a mock with no expectations that should have been a stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
7b7737803d |
Give Account its state as properties, and close the last hole in the sweep
982 lines to 574, eighty-five accessors gone. Eight hooks, and every one of them is over the settings JSONB rather than a column: $syncLimit, $backfillTarget, $backfillAttempts clamp to zero, and $backfillRanAt converts between a DateTimeImmutable and the Unix timestamp the bag can actually hold. The four alias derivations are virtual get-only. Every real column is a plain property. The seven predicates split three ways, all checked against information_schema: isActive() and isPrimary() are boolean columns and keep their prefixes because the columns carry them; isPushEnabled() is the column push_enabled and so loses one. isMicrosoft() and isGmail() read two enums, isLabelSyncEnabled() reads an untyped JSON key, and isGmailWatchActive() compares an expiry to the clock -- all four stay methods, because none of them is reading a value. The point of doing this entity last was the sweep. It finds accessors this branch deleted that no class defines any more, so it was blind to getId() for as long as Account still had one. Account was the last, and with it gone the sweep covers all 261 deleted names: 254 of them exist nowhere, and every surviving call site is a vendor receiver -- Symfony's Session and Request, Messenger's id stamp, DateTimeInterface, webklex's own message and attachment. No plMail entity among them. Which was worth the wait, because five more latent fatals turned up on untyped receivers, and the suite and PHPStan saw none of them. The worst is ComposeType: findForUserOrdered() returns a bare array, so seven accessor calls on it were invisible, and that is the From dropdown -- composing a mail would have fataled. The others were an untyped closure parameter, an `object $account` parameter in the IDLE heartbeat, and two more repository-returns-array shapes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
d1b49e7036 |
Give Message its state as properties
695 lines to 275, sixty-nine accessors gone, and not one hook: nothing on this entity validated, normalised or derived anything, so every hook would have been syntax without a reason. Both predicates were the mapped-boolean case, checked against information_schema rather than assumed -- has_attachments and cancelled are real boolean NOT NULL columns and both methods were bare returns. So hasAttachments() keeps its prefix as $hasAttachments and isCancelled() becomes $cancelled without one, because in each case the property name is the column name and only one of those columns carries a prefix. $searchVector stays private, and now says why: Postgres generates it, PHP never writes it, and the queries that use it match the column in SQL. Two call sites could not be translated and had to be restructured, both for the same reason -- a property write is not a callable. AddressNormalizationBackfillTask passed setToAddresses() as a first-class callable, and JmapDraftWriter dispatched through a $setter variable. This batch also found two more latent fatals left by earlier ones, both on untyped receivers: ImapSuperviseCommand::reconcile() called Mailbox::getId() on a repository array, which would have crashed the supervisor's reconcile loop, and both seed commands called MessageThread::getId() on the reseed path. The orphan sweep cannot see these, and it is worth writing down why: it works by finding deleted accessor names that no class defines any more, so it is blind wherever the name survives elsewhere. getId() is the whole of that exposure. After this commit Account::getId() is the last one in src/, so every other ->getId() had to be read by hand -- and once Account lands, the sweep covers it too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
f87b59321c |
Give User its state as properties, and keep the eight the contracts require
624 lines to 483. The interface methods were derived by reading the interfaces rather than from memory: UserInterface declares only getRoles() and getUserIdentifier(), PasswordAuthenticatedUserInterface getPassword(), and the scheb bundles the five 2FA and backup-code methods. All eight stay, including the ones that look exactly like plain getters -- isTotpAuthenticationEnabled() in particular reads like the mapped-boolean case and is a contract. Three hooks, each with a reason. $timezone keeps its clamp. $backupCodes normalises with array_values, which is not decoration: the column is JSON, and array_filter() on spend leaves holes that encode as an object instead of a list. $calendarPaneWidth, $collapsedAdminPanels and $backupCodeCount are virtual reads off the settings bag and a count, so they have no column and Doctrine would refuse them one. $totpSecret and $totpConfirmedAt are private(set) deliberately: enrolment and confirmation move together, and a public write would let a secret be swapped in without the confirmation state resetting with it. isCalendarPaneOpen() stays a method. It reads a key out of a JSON bag that may be absent or hold anything, which is an interpretation rather than the boolean column that $isInline and $isSyncEnabled turned out to be. This batch also fixed a latent fatal the previous one left behind: MessageThreadRepository called getId() on a value typed mixed out of getResult(), and MessageThread::getId() had been deleted. The suite passed and PHPStan passed, because neither can see through an untyped receiver. Found by grepping for them on purpose. Five more of the same shape were masked by existing baseline entries, including one in a form that would have thrown on Settings -> Profile for any user with a picture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
c17b0fd10a |
Give MessageThread and Mailbox their state as properties
306 lines to 148 and 319 to 132, and between them exactly one hook: Mailbox's $label, which reads through the binding row and so is virtual and unmapped. Nothing else derived or validated anything, so everything else is a plain public property -- a hook that only assigns is noise with extra syntax. Both of Mailbox's predicates turned out to be the mapped-boolean case, checked against information_schema rather than assumed: is_sync_enabled and is_idle_enabled are real boolean columns and both methods were bare returns. So they keep the column's own name, $isSyncEnabled and $isIdleEnabled, rather than being tidied into $syncEnabled -- the property name is the column name. isSnoozed() stays a method and is the clearest case yet for why that rule exists: it compares a stored timestamp against the clock, so the same row answers differently as the snooze expires. That is not a read. Two rewrites were forced by PHP rather than taste. `?->prop =` is not a legal write target, so EmailPatchApplier null-checks the thread into a local first; and a chain ending `?? $this->createThread(...)->setProviderThreadKey(...)` became an explicit if, because there is no fluent return to lean on any more. Four call sites existed that PHPStan could not see, because their receivers were untyped -- an `object $mailbox` parameter and some array returns. The test suite caught them. Worth knowing for the two big entities still to come: static analysis is the net, and it has holes exactly where the types stop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Paul LütznerandClaude Opus 5
|
53ba299501 |
Stop four containers racing each other to run the same migration
A deploy carrying a migration brought up one service out of four. php,
imap-supervisor, messenger-worker and scheduler are the same image running the
same entrypoint, they start within milliseconds of each other against one
database, and every one of them migrates on boot. All four read the ledger
before any of them writes to it, so all four decide the same migration is
pending; one wins and the rest die on
SQLSTATE[42701]: Duplicate column: 7
ERROR: column "timezone" of relation "user" already exists
which under `set -e` is a container that never comes up. The timezone column in
v0.0.6 is what exposed it, not what caused it -- any migration would have done,
and `--all-or-nothing` makes it likelier rather than safer, because the losers
are held for as long as the whole batch takes instead of one statement.
Reproduced before it was fixed: four concurrent migrates against one database,
three exited 7 with that message verbatim. The same four under the lock all exit
0, one migrates and three find the ledger current.
The losers still run the migration rather than skipping it. Skipping would mean
a container coming up against a schema nobody in that process verified, which is
the failure this is supposed to prevent, one step later.
The lock cannot be a line of shell, which is the whole reason there is a command
here. `pg_advisory_lock()` is scoped to the session, so taking it with
`dbal:run-sql` hands it straight back when that process exits and leaves the
migration it was meant to protect running unguarded. It has to be held by the
connection that migrates, which means one PHP process doing both.
Waiting is bounded. A container OOM-killed mid-migration would otherwise hang
every other service in the stack forever with nothing on stdout to say why, so a
timeout reports what it was waiting for -- and refuses to migrate anyway, which
is the only answer that is not a guess about what the holder was doing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Paul LütznerandClaude Opus 5
|
4656665ffd |
Add snooze: put a conversation away, and bring it back
Snooze is archive-with-a-timer, expressed in labels like everything else
— Inbox off, Snoozed on — so the "a message carries at least one label"
invariant holds without a second mechanism, and the snoozed pile is
reachable by every path that already works on labels.
The web endpoint was the broken half. POST /status/thread/{id}/snooze
wrote snoozedUntil and nothing else: no labels moved and nothing
propagated, so the conversation stayed in the Inbox both locally and at
the provider while its row vanished from the list — and then the sweep
"woke" a thread that had never left. Both callers now go through
ThreadSnoozeService, so a snooze means the same thing whichever client
set it. ThreadSnoozeEndpointTest asserts on labels rather than on the
column, because the column was always written correctly and a test that
checked it would have passed against the broken implementation.
The UI was missing outright. The row button called an action whose
`until` defaulted to null and which Stimulus calls with the event alone,
so it cleared a snooze instead of setting one; the toolbar hardcoded
"tomorrow 8am" behind a comment asking for a picker. Both now open a
menu. The wake times are computed in the browser, deliberately: the
server has no timezone for the session, so "tomorrow morning" resolved
server-side would mean tomorrow morning wherever the container thinks it
is. "Later today" drops itself once the evening has passed rather than
resolving to a time already gone.
The Snoozed pile also had nowhere to live — no route and no sidebar
entry, so it was unreachable. Both added, gated on there being something
snoozed, on the same reasoning as Archive.
Thread/set is a plMail extension; RFC 8621's Thread is read-only. It
accepts exactly one property and refuses create and destroy outright:
threads come into being by mail arriving, and a client that could conjure
one would be describing something nothing else understands. Unlike the
web endpoint it refuses an unparseable date rather than substituting
"in 1 day" — for an API, a client told its write succeeded would find the
thread back at a time it never chose.
app:mail:wake-snoozed runs every minute, which is the unit people pick a
wake time in, and costs one indexed query when nothing is due.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|