Sync: a refused flag or move fails the pass after its suggestions were filed, and shows the server's own words #22

New issue

Closed

opened 2026-10-01 10:48:01 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-01 10:48:01 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo-imap at cc09c6381, following every mailbox call to what the person is shown when it fails, and running sync_connection against a fake client that refuses the flag and the move.

Related to postulo/postulo#335.

Opening the folder, searching and fetching are wrapped into MailboxError with a sentence for the person and the server's answer in the log. Flagging, moving, and the folder checks are not. They run after the suggestions of the pass were created, so a refusal there throws away the report of a pass that did its work, marks the connection as failed, and puts the far side's text in front of the person, which the module's docstring says never happens. sync() also lets MailboxError itself escape, so even the wrapped failures reach the person with a class name in front.

Related to #1 (closed), which took server errors out of Test for connecting and signing in; the calls below were left out.

Where:
- src/postulo_imap/mailbox.py:210-219, Mailbox.mark and Mailbox.move: add_flags, folder_exists, create_folder and move with no try, unlike select, unread_uids and fetch above them.
- src/postulo_imap/sync.py:214-217: the flag or the move, after the loop that calls suggest(). sync() has only finally: close(client).
- src/postulo_imap/sync.py:140, test(): client.folder_exists(folder) is inside a try that catches MailboxError only.
- src/postulo_imap/sync.py:167-175: connect, select, unread_uids and fetch raise MailboxError, and sync() does not turn it into SyncReport(error=…).
- src/postulo_imap/mailbox.py:179-184: the docstring of unread_uids describes what happens with "a server that refuses keywords", but use_keyword comes only from the setting (sync.py:173); PERMANENTFLAGS is never read.
- src/postulo_imap/mailbox.py:20-24: "Whatever the far side answered … goes to the log … and never into the message the person sees".
- In postulo: plugins/syncing.py:83-85 records f"{type(error).__name__}: {error}" for any exception a sync raises, and plugins/views.py:402-404 does the same for Test.
What happens: run on the snapshot with the plugin's own fake mailbox and the core's sync_connection.
1. Move it to a subfolder on a server without the MOVE extension. IMAPClient.move is decorated @require_capability("MOVE") (imapclient 3.1.0) and there is no copy-and-delete fallback. One suggestion is filed, then: last_error is CapabilityError: Server does not support MOVE capability, last_summary is nothing to do, last_ok_at stays empty. The report that said one message was pulled is gone. Move mode does not use the keyword, so the next pass reads the whole window again and fails the same way, for as long as the setting stands. The same follows when the server will not create the subfolder.
2. Flag it on a folder the account may read but not write (a shared or read-only folder). select_folder succeeds, the suggestions are filed, and the refused STORE raises IMAPClientError("store failed: <the server's answer>"). With the fake raising that, last_error was the answer word for word, error: store failed: [READ-ONLY] Mailbox is read-only, …, and one suggestion had been filed. The could not be matched note of such a pass is lost with the report.
3. A server that accepts the STORE but does not keep new keywords (RFC 3501 allows it to ignore the change or keep it for the session only; reasoned, no real server was contacted) raises nothing: flag mode silently behaves as untouched mode and every pass reads the whole window again, which is the case the docstring claims to handle.
4. test() with folder_exists raising returned no TestResult; the exception left the plugin, and the core shows Test failed: followed by the exception's class and text.
5. A folder that does not exist: last_error is MailboxError: The folder “Nope” could not be opened. The sentence written for the person arrives with a Python class name in front, and the core logs a traceback at error level for what is an ordinary wrong setting. tests/test_imap.py:633 asserts only the substring, so it passes, and tests/test_imap.py:650-655 pins the exception leaving sync().
Why it matters: the person is told the sync failed when their suggestions are in the queue, the time of the last good run never advances, and what they read is a library's or a server's sentence, not one of the plugin's translated ones. In move mode the failure repeats on every pass together with the full download.
Fix:
- In sync(), catch MailboxError and return SyncReport(error=str(error)), so the person's sentence arrives without the class name and the log is not given a traceback for it.
- Wrap marking and moving: on failure log the server's answer, keep the report, and add a translated note ("read, but could not be flagged; the next pass will read them again"), so the counts and the other notes survive.
- After select_folder, read PERMANENTFLAGS from its answer; when \* is absent in flag mode, use the unflagged search and say so in a note, as the docstring already describes.
- When the server has no MOVE, fall back to copy, \Deleted and expunge of those uids, or refuse the setting in Test with a sentence that says the server cannot move messages.
- Wrap folder_exists in test() the same way as the calls around it.

Done when

tests/test_imap.py has a test in which the fake client's move raises after one recognised message was read: sync() returns a report with pulled == 1, no error, and a note that the messages could not be moved. Today the exception leaves sync().
The same for add_flags raising in flag mode, asserting that the server's text is in the log and not in the report.
tests/test_imap.py:627-633 asserts connection.last_error == "The folder “Nope” could not be opened." exactly; today the class name MailboxError and a colon come first. The test at :650-655 asserts the same sentence on the returned report and still that the client was logged out.
A test in which the fake client's folder_exists raises: ImapSync().test(CONFIG) returns a TestResult that is not ok and does not contain the exception's text. Today it raises.

Found on 2026-09-29 in a read-only review of postulo-imap at cc09c6381, following every mailbox call to what the person is shown when it fails, and running `sync_connection` against a fake client that refuses the flag and the move. Related to postulo/postulo#335. Opening the folder, searching and fetching are wrapped into `MailboxError` with a sentence for the person and the server's answer in the log. Flagging, moving, and the folder checks are not. They run after the suggestions of the pass were created, so a refusal there throws away the report of a pass that did its work, marks the connection as failed, and puts the far side's text in front of the person, which the module's docstring says never happens. `sync()` also lets `MailboxError` itself escape, so even the wrapped failures reach the person with a class name in front. Related to #1 (closed), which took server errors out of *Test* for connecting and signing in; the calls below were left out. - **Where:** - `src/postulo_imap/mailbox.py:210-219`, `Mailbox.mark` and `Mailbox.move`: `add_flags`, `folder_exists`, `create_folder` and `move` with no `try`, unlike `select`, `unread_uids` and `fetch` above them. - `src/postulo_imap/sync.py:214-217`: the flag or the move, after the loop that calls `suggest()`. `sync()` has only `finally: close(client)`. - `src/postulo_imap/sync.py:140`, `test()`: `client.folder_exists(folder)` is inside a `try` that catches `MailboxError` only. - `src/postulo_imap/sync.py:167-175`: `connect`, `select`, `unread_uids` and `fetch` raise `MailboxError`, and `sync()` does not turn it into `SyncReport(error=…)`. - `src/postulo_imap/mailbox.py:179-184`: the docstring of `unread_uids` describes what happens with "a server that refuses keywords", but `use_keyword` comes only from the setting (`sync.py:173`); `PERMANENTFLAGS` is never read. - `src/postulo_imap/mailbox.py:20-24`: "Whatever the far side answered … goes to the log … and never into the message the person sees". - In postulo: `plugins/syncing.py:83-85` records `f"{type(error).__name__}: {error}"` for any exception a sync raises, and `plugins/views.py:402-404` does the same for *Test*. - **What happens:** run on the snapshot with the plugin's own fake mailbox and the core's `sync_connection`. 1. *Move it to a subfolder* on a server without the MOVE extension. `IMAPClient.move` is decorated `@require_capability("MOVE")` (imapclient 3.1.0) and there is no copy-and-delete fallback. One suggestion is filed, then: `last_error` is `CapabilityError: Server does not support MOVE capability`, `last_summary` is `nothing to do`, `last_ok_at` stays empty. The report that said one message was pulled is gone. Move mode does not use the keyword, so the next pass reads the whole window again and fails the same way, for as long as the setting stands. The same follows when the server will not create the subfolder. 2. *Flag it* on a folder the account may read but not write (a shared or read-only folder). `select_folder` succeeds, the suggestions are filed, and the refused `STORE` raises `IMAPClientError("store failed: <the server's answer>")`. With the fake raising that, `last_error` was the answer word for word, `error: store failed: [READ-ONLY] Mailbox is read-only, …`, and one suggestion had been filed. The *could not be matched* note of such a pass is lost with the report. 3. A server that accepts the `STORE` but does not keep new keywords (RFC 3501 allows it to ignore the change or keep it for the session only; reasoned, no real server was contacted) raises nothing: flag mode silently behaves as untouched mode and every pass reads the whole window again, which is the case the docstring claims to handle. 4. `test()` with `folder_exists` raising returned no `TestResult`; the exception left the plugin, and the core shows *Test failed:* followed by the exception's class and text. 5. A folder that does not exist: `last_error` is `MailboxError: The folder “Nope” could not be opened.` The sentence written for the person arrives with a Python class name in front, and the core logs a traceback at error level for what is an ordinary wrong setting. `tests/test_imap.py:633` asserts only the substring, so it passes, and `tests/test_imap.py:650-655` pins the exception leaving `sync()`. - **Why it matters:** the person is told the sync failed when their suggestions are in the queue, the time of the last good run never advances, and what they read is a library's or a server's sentence, not one of the plugin's translated ones. In move mode the failure repeats on every pass together with the full download. - **Fix:** - In `sync()`, catch `MailboxError` and return `SyncReport(error=str(error))`, so the person's sentence arrives without the class name and the log is not given a traceback for it. - Wrap marking and moving: on failure log the server's answer, keep the report, and add a translated note ("read, but could not be flagged; the next pass will read them again"), so the counts and the other notes survive. - After `select_folder`, read `PERMANENTFLAGS` from its answer; when `\*` is absent in flag mode, use the unflagged search and say so in a note, as the docstring already describes. - When the server has no MOVE, fall back to copy, `\Deleted` and expunge of those uids, or refuse the setting in *Test* with a sentence that says the server cannot move messages. - Wrap `folder_exists` in `test()` the same way as the calls around it. ## Done when - `tests/test_imap.py` has a test in which the fake client's `move` raises after one recognised message was read: `sync()` returns a report with `pulled == 1`, no `error`, and a note that the messages could not be moved. Today the exception leaves `sync()`. - The same for `add_flags` raising in flag mode, asserting that the server's text is in the log and not in the report. - `tests/test_imap.py:627-633` asserts `connection.last_error == "The folder “Nope” could not be opened."` exactly; today the class name `MailboxError` and a colon come first. The test at `:650-655` asserts the same sentence on the returned report and still that the client was logged out. - A test in which the fake client's `folder_exists` raises: `ImapSync().test(CONFIG)` returns a `TestResult` that is not ok and does not contain the exception's text. Today it raises.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:48:01 +00:00

tiagoagueda added the

bug

label

2026-10-01 10:48:01 +00:00

tiagoagueda referenced this issue from a commit

2026-10-19 07:00:13 +00:00

Fix the review findings: reading, classifying, matching, the form and what a refusal does

tiagoagueda referenced this issue

2026-10-19 07:00:17 +00:00

Fix the open bug and security issues (#8-#22, #24) #25