wienen/mailcat
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
35d78a27aa364195a2260052fd5f419284f64e4a
mailcat/reports/script_review_findings.md
T
wienen 4780c18d70 Initial commit 
Claude has done a lot of work, but there are still some errors which
cause the mails to be distributed to wrong mail boxes. I am now going to
let codex and claude compete. They will have their own branches and
merge to main as soon as we are somewhere where we can switch AI.
2026-07-04 11:18:05 +02:00

383 lines
14 KiB
Markdown
Raw Blame History

# Script review: mailbox creation and redistribution
Date: 2026-07-04
Scope reviewed:
- `maak_mappen.py`
- `verplaats_bestaand.py`
- `mailrules.sieve`
- `upload_sieve.py`
- Supporting scripts: `download_mailbox.py`, `analyse_mailbox.py`, `review_mailinglists.py`, `unsubscribe.py`, `kopieer_naar_backup.py`, `dagelijks_overzicht.py`
Static checks:
- `python3 -m py_compile *.py` passes.
- `verplaats_log.json` currently contains 14 completed step-1 folders and 3166 processed INBOX UIDs, so at least one real redistribution run has been attempted.
## Executive summary
The folder creation script mostly works, but redistribution has several real defects that can explain failed or incomplete moves.
The highest-risk issue is in `verplaats_bestaand.py`: step 1 opens source folders read-only and then tries to mark messages as deleted. On a normal IMAP server, this makes a true move impossible. The script may copy messages but fail to delete originals, while still logging the folder as completed. That can leave duplicates and prevent a later rerun from correcting the folder.
There is also a folder contract mismatch: `maak_mappen.py` does not create `INBOX.Technisch` or `INBOX.Technisch.DMARC`, while both the Sieve rules and redistribution route mail there. That will fail for those categories unless the folders already exist.
The logs are not strong enough for safe recovery: they record intended work as complete without validating that the source message disappeared and the destination message exists. If the first real run failed partially, the safest next action is to audit actual server folder counts before rerunning with the current log.
## Findings
### P0: Step 1 selects source folders read-only, then tries to delete messages
File: `verplaats_bestaand.py`
Relevant lines:
- Line 284: `mail.select(..., readonly=True)`
- Lines 301-303: copy the message and then `STORE +FLAGS \Deleted`
- Line 306: `EXPUNGE`
- Lines 307-310: reports success and logs the source folder as done
Problem:
Step 1 always selects the source folder with `readonly=True`, including during real execution. In read-only mode, `COPY` can succeed, but `STORE +FLAGS \Deleted` should fail or be ignored by the server. That means this code is not reliably moving messages; it may only copy them.
Impact:
- Messages may remain in their original folders.
- Duplicates may be created in destination folders.
- The script still appends the folder to `stap1_klaar`, so reruns skip it.
- This matches the symptom: folder creation works, redistribution appears to fail.
Recommended fix:
- Select read-only only in dry-run mode:
- `mail.select(..., readonly=dry)`
- Check both `COPY` and `STORE` results.
- Only log a folder as complete if all expected messages were copied and deleted.
- Prefer UID-based operations in step 1, consistent with step 2.
### P0: The redistribution log can mark failed or partial moves as complete
File: `verplaats_bestaand.py`
Relevant lines:
- Lines 301-304: `STORE` result is ignored.
- Lines 307-310: folder is logged as done regardless of whether `gekopieerd == n`.
- Lines 365-369: step 2 logs a UID as processed when `COPY` succeeds, even though `STORE` result is not checked inside `move_message`.
- Lines 256-268: `move_message()` returns success based only on `COPY`.
Problem:
The code treats `COPY OK` as a successful move. A move is only complete if:
1. The message was copied to the destination.
2. The source message was marked deleted.
3. The deletion was expunged, or a later expunge is guaranteed.
The current code does not verify steps 2 and 3 before writing progress to `verplaats_log.json`.
Impact:
- A rerun may skip messages that are still in the source folder.
- Failures from missing destination folders, read-only source folders, or permission issues can be hidden.
- `verplaats_log.json` is currently not a reliable source of truth.
Recommended fix:
- Make `move_message()` return false unless both `UID COPY` and `UID STORE +FLAGS \Deleted` return `OK`.
- Save detailed failures in the log with source, UID, destination, and server response.
- Do not append to `stap1_klaar` unless the final moved count equals the expected count.
- Before rerunning after the current failure, either archive/reset `verplaats_log.json` or write a repair mode that reconciles the log against actual server state.
### P1: `maak_mappen.py` omits folders targeted by redistribution and Sieve
Files:
- `maak_mappen.py`
- `verplaats_bestaand.py`
- `mailrules.sieve`
Relevant lines:
- `verplaats_bestaand.py` line 165 targets `Technisch.DMARC`.
- `mailrules.sieve` lines 230-241 targets `Technisch.DMARC` and `Technisch`.
- `maak_mappen.py` lines 85-193 contains no `Technisch` or `Technisch.DMARC`.
Problem:
The folder creator and routing targets are inconsistent. Static comparison found these Sieve targets missing from `maak_mappen.py`:
- `Technisch`
- `Technisch.DMARC`
The dynamic Sieve invoice path `Financieel.Facturen.${jaar}.${kwartaal}` is not directly comparable, but the concrete year/quarter folders for 2020-2026 are created.
Impact:
- Technical/DMARC messages fail to move if those folders do not already exist.
- Sieve `fileinto` for those folders can fail for new mail.
Recommended fix:
- Add `Technisch` and `Technisch.DMARC` to `maak_mappen.py`.
- Add a test or check that every static `fileinto` destination and every Python route destination exists in the folder creation list.
### P1: Step 1 assumes archive parent folders already exist
Files:
- `maak_mappen.py`
- `verplaats_bestaand.py`
Relevant lines:
- `verplaats_bestaand.py` lines 192-198 moves old archive subfolders into `INBOX.Archief.2020`, `INBOX.Archief.2021`, and `INBOX.Archief.2022`.
- `maak_mappen.py` lines 188-192 creates only `Archief.2023` through `Archief.2026`.
Problem:
The redistribution script targets `INBOX.Archief.2020`, `INBOX.Archief.2021`, and `INBOX.Archief.2022`, but the folder creator does not create them. In the current offline mailbox they exist, so this may work on the original account. It can fail on a fresh backup account or after recreating the target structure from scratch.
Impact:
- Archive redistribution may fail on accounts where those parent folders do not already exist.
- If `COPY` fails because the destination does not exist, step 1 still risks logging the source as done.
Recommended fix:
- Include `Archief`, `Archief.2020`, `Archief.2021`, and `Archief.2022` in `maak_mappen.py`, or have `verplaats_bestaand.py` create missing destination folders before moving.
### P1: `verplaats_bestaand.py` only routes INBOX, despite claiming broader routing
File: `verplaats_bestaand.py`
Relevant lines:
- Lines 8-9: docstring says "INBOX + overige mappen".
- Line 204: comment says folders can be routed per message.
- Line 205: `ROUTE_FOLDERS = ["INBOX"]`
- Lines 315-377: function is hard-coded to INBOX.
Problem:
The documentation implies broader per-message routing, but the implementation only processes `INBOX`. Existing messages in other folders are only moved if they are covered by the hard-coded whole-folder renames or by the invoice step.
Impact:
- Many existing messages outside INBOX remain unsorted.
- The user may interpret this as redistribution failing, when the script never attempted those folders.
Recommended fix:
- Either make the documentation explicit that only INBOX is routed, or implement routing for a configured set of non-system folders.
- If broad routing is implemented, exclude destination folders to avoid repeatedly reprocessing already sorted mail.
### P1: Routing exact-domain matching is weaker than the reports suggest
File: `verplaats_bestaand.py`
Relevant lines:
- Lines 226-228 extracts the exact sender domain.
- Lines 244-253 uses direct lookup only: `DOMAIN_LOOKUP.get(domain)`.
- Lines 75-166 contains hand-curated domains, including some but not all subdomains.
Problem:
The redistribution routes only exact domains. If mail comes from `mail.example.com`, but the route table contains only `example.com`, it will not match. Some subdomains are listed manually, but this is incomplete by design.
Impact:
- Valid messages can remain in INBOX as "Geen match".
- This can look like the redistribution failed even though the script ran.
Recommended fix:
- Add a parent-domain fallback, for example try `a.b.example.com`, then `b.example.com`, then `example.com`.
- Keep exceptions for multi-tenant senders where parent-domain matching is unsafe.
- Log unmatched high-volume domains for review.
### P1: Sieve paths may use a different namespace prefix than created folders
Files:
- `maak_mappen.py`
- `mailrules.sieve`
- `upload_sieve.py`
Relevant lines:
- `maak_mappen.py` line 16 uses `PREFIX = "INBOX."`.
- `mailrules.sieve` lines 5-7 notes this ambiguity.
- `upload_sieve.py` line 21 uses `SIEVE_PREFIX = ""`.
Problem:
Folder creation creates `INBOX.<path>`, while the uploaded Sieve file currently files into `<path>` without `INBOX.`. On some Dovecot configurations this is correct; on others, Sieve needs `INBOX.<path>`.
Impact:
- New incoming mail may not land in the created folders even if existing-mail redistribution is fixed.
- This affects Sieve/new mail more than `verplaats_bestaand.py`, which does use `INBOX.`.
Recommended fix:
- Verify the active server namespace with a test message.
- If Sieve delivery fails, set `SIEVE_PREFIX = "INBOX."` before upload.
- Consider adding a `--prefix` flag instead of editing a constant.
### P2: Step 3 has no per-message progress log
File: `verplaats_bestaand.py`
Relevant lines:
- Lines 382-427 handles invoice folders.
- No `log` writes are made for step 3.
Problem:
Invoice redistribution can move a large number of messages, but the script does not record which UIDs were successfully processed.
Impact:
- If the run is interrupted, it starts over for whatever remains.
- That is mostly safe if moves are truly atomic, but unsafe with the current weak move verification.
Recommended fix:
- Add the same robust move result logging recommended for step 2.
- Include source folder and UIDVALIDITY or use Message-ID based reconciliation.
### P2: Folder and IMAP LIST parsing is fragile for encoded or unusual names
Files:
- `download_mailbox.py`
- `kopieer_naar_backup.py`
- `dagelijks_overzicht.py`
- `maak_mappen.py`
Relevant examples:
- `download_mailbox.py` lines 22-30
- `kopieer_naar_backup.py` lines 36-41
- `dagelijks_overzicht.py` lines 89-94
- `maak_mappen.py` lines 216-223
Problem:
Several scripts parse `LIST` responses with regular expressions and inconsistent modified UTF-7 decoding. `maak_mappen.py` decodes names, but other scripts mostly do not.
Impact:
- Non-ASCII folders such as `Univé` can be mishandled in scripts that list/select folders from server responses.
- This is less likely to be the main redistribution failure because `verplaats_bestaand.py` mostly uses hard-coded folder names, but it is a reliability issue across the toolkit.
Recommended fix:
- Centralize IMAP folder encoding/decoding and LIST parsing in one helper module.
- Use that helper from all IMAP scripts.
### P2: `kopieer_naar_backup.py` silently ignores append failures
File: `kopieer_naar_backup.py`
Relevant lines:
- Lines 168-179 appends a message to the destination.
- Line 179 silently does `pass` on failure.
- Lines 270-274 marks the folder complete after one pass.
Problem:
A folder can be marked complete even if some message appends failed.
Impact:
- Backup completeness cannot be trusted from `backup_log.json` alone.
- This matters if the backup account is used as a safety net before live redistribution.
Recommended fix:
- Count append failures.
- Do not mark a folder complete if failures occurred.
- Log failed message IDs and server responses.
### P2: `upload_sieve.py` has an incomplete ManageSieve parser
File: `upload_sieve.py`
Relevant lines:
- Lines 38-39 expects to read capabilities.
- Lines 104-109 `_read_capabilities()` is `pass`.
- Lines 58-64 uses a non-synchronizing literal `{len+}`.
Problem:
The ManageSieve client is intentionally minimal. If the server sends multi-line capabilities before `OK`, or does not support non-synchronizing literals, this can behave unpredictably.
Impact:
- Upload may fail on stricter servers.
- If it appears to work on the current server, this is not the likely redistribution failure, because redistribution uses IMAP, not ManageSieve.
Recommended fix:
- Use a tested ManageSieve library, or implement response parsing properly.
- At minimum, capture and print full server responses when upload fails.
## Likely explanation of the current failure
Given the observed state:
- Folder creation appears to work.
- `verplaats_log.json` says all 14 step-1 folder renames were completed.
- `verplaats_log.json` says 3166 INBOX UIDs were processed.
- Step 1 selected folders read-only and then attempted deletion.
The most likely scenario is:
1. Destination folders were created.
2. Step 1 copied messages from old folders into new folders.
3. Deletion from the source folders failed because the source folder was selected read-only.
4. The script still wrote those old folders to `stap1_klaar`.
5. Rerunning the script now skips those old folders, so the failed move is not repaired.
Step 2 may have moved many INBOX messages, but because its log is UID-only and only checks `COPY`, its success should also be verified against actual mailbox state.
## Recommended recovery plan
1. Do not run `verplaats_bestaand.py --uitvoeren` again with the current script and current log.
2. Inspect actual server counts for:
- Old source folders in `stap1_klaar`
- Their intended destination folders
- INBOX
- Invoice folders
3. Archive `verplaats_log.json` before any repair run.
4. Fix move semantics:
- No read-only select during real moves.
- Check `COPY`, `STORE`, and final expunge behavior.
- Log failures explicitly.
5. Add missing folders to `maak_mappen.py`.
6. Add a dry-run audit mode that reports source and destination counts without modifying mail.
7. Only then run a repair mode that moves remaining source messages and avoids creating duplicates, preferably by comparing `Message-ID` in source and destination.
## Minimal code changes to prioritize
1. In `verplaats_bestaand.py`, change step-1 select from `readonly=True` to `readonly=dry`.
2. In `move_message()`, require `UID STORE` to return `OK` before returning success.
3. In step 1, use UID-based search/copy/store and log the folder complete only when all messages moved.
4. In `maak_mappen.py`, add:
- `Technisch`
- `Technisch.DMARC`
- `Archief`
- `Archief.2020`
- `Archief.2021`
- `Archief.2022`
5. Add a destination-existence preflight to `verplaats_bestaand.py` before any real move.
Reference in New Issue View Git Blame Copy Permalink