From 38b9466aea7daa0ddde91c1de7d8082ac000c238 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 20:12:59 +0200 Subject: docs: close item 66, open 87 and 88, record the row-number trap Item 66 turns out not to have been the defect it was filed as. The pane was never blank: an unexpanded thread root rendered the CONVERSATION, and the same click rendered one message once the thread had been opened, because the model learned the root's message id only when the replies arrived. The user's step-by-step account is what separated the two halves; two probes against a real database had failed to reproduce the blank pane because there was none. Closed by carrying firstMessageId in the query and removing the conversation view, which the user asked for after being told the stubs not expanding was itself a defect and that the feature was being judged in a broken state. Two defects came out of it and are open. 87: auto mark-read still marks a whole thread, coherent while a root rendered the conversation and not any more. 88: threadAt(current.row()) answers about the wrong thread for a reply row, because a tree numbers rows per parent. 87 is blocked on 88 and the entry says why: a fix for 87 was written, mutation-checked, shipped and reverted the same evening after it marked an unrelated message read. CLAUDE.md gains the row-number trap as its own entry rather than leaving it implied by the item 20 note, plus the rule that a test for a write path must exercise the reply case: the reverted fix was green because it asserted on a root selection, the one case where row() is correct. The cid-prefixing note is corrected to say every caller now passes one message while explaining why the prefixing stays. The changelog carries a Removed entry and an Upgrading note, including that mark_read_delay_ms accepts a negative value to disable auto mark-read entirely, verified against config.h. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) (limited to 'CLAUDE.md') diff --git a/CLAUDE.md b/CLAUDE.md index a9cf33c..c63e167 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -398,6 +398,26 @@ standalone program containing none of this project's code. A size assertion there passes against both, and a mutation putting the bug back leaves the suite green. Assert on the stored value, and leave the frame to a hand test. +**`ThreadListModel::threadAt(int)` takes a ROW and is wrong for any index that +might be a reply.** A tree numbers rows per parent, so a reply's `row()` indexes +its siblings and `threadAt(current.row())` on the first reply of any thread +returns the FIRST THREAD IN THE LIST. This shipped in `markCurrentThreadRead`, +was mostly masked while the write it guarded was thread-wide, and became "a +random message was marked read" the moment a fix scoped that write to one +message (items 87 and 88). Reach the thread through the INDEX, never through a +row number, unless you have already established the index is a thread row. + +The general rule is in the item 20 note further down, "anything keyed on a row +NUMBER did not survive the port to a tree". This is a surviving instance, found +by corrupting real mail rather than by reading, so treat every remaining +`.row()` in `mainwindow.cpp` as suspect until checked. + +**A test for a mutation on a data-writing path must exercise the REPLY case, +not only the root.** The reverted fix above was mutation-checked and green: it +asserted on a root selection, which is the one case where `current.row()` is +correct. A green mutation check proves the test can fail, not that it covers the +case that matters. + **`test_mainwindow` can now drive a real worker, and three things about it will waste a session each.** `WorkerBackedWindow` builds a throwaway notmuch database and writes a `qtmaildir.conf` pointing at it; `wireWorker()` reads @@ -487,10 +507,13 @@ loosen any of these without an explicit decision. scheme-wide allow would let a hostile body reference `qtmaildir://anything` and be trusted. Consequence: `MessageView` **must** call `setDocumentUrl()` with the same URL it gives `setHtml()`, or nothing renders. -- A whole thread is one HTML document in one web view (a `QWebEngineView` per message would - spawn a Chromium render process each). That makes `cid:` ids collide across messages, so - every reference is rewritten to `cid:!`. **A `cidPrefix` must never contain - `!`** — it is the namespace separator. +- The pane renders a LIST of messages into one HTML document in one web view (a + `QWebEngineView` per message would spawn a Chromium render process each). That makes `cid:` + ids collide across messages, so every reference is rewritten to `cid:!`. + **A `cidPrefix` must never contain `!`** — it is the namespace separator. + Since item 66 removed the conversation view every caller passes exactly ONE message, so the + collision cannot currently arise; the prefixing stays because the list-rendering path does, + and a security property must not rest on every caller happening to pass one item. - Remote content grants are per-render and never sticky. - **Attachment filenames are untrusted input.** Reduce to basename, strip separators, resolve against the chosen directory, and refuse anything escaping it. Compare resolved paths as -- cgit v1.2.3