diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 79 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 159 |
2 files changed, 174 insertions, 64 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 2e941ec..f537385 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -7751,3 +7751,82 @@ new action and the existing ones gathered under it rather than duplicated. - **Item 160 unblocked this** on 2026-08-25: the status bar exists, so a manual save reports through `refreshDraftStatus()` like an autosave. Route `Ctrl+S` through `saveDraftNow()` and the reporting is already done. + +## 162. Delete fails while a sync is renaming the file underneath it + +**Observed (user, 2026-08-25):** deleting a draft reported `Cannot move +<file> to <account>/Trash`. + +**Cause (verified against the live Maildir, not read):** a stale path, and +neither Delete nor item 158 is at fault. + +1. The composer autosaves a draft as `<name>:2,D` and item 158 indexes it + under exactly that filename. +2. **mbsync uploads it and RENAMES it** to `<name>,U=<uid>:2,D`, recording the + server UID in the filename. +3. notmuch still holds the pre-`U=` name until that sync's `notmuch new` runs. +4. `moveMessages()` reads the filename from notmuch and calls + `QFile::rename()` on a path that no longer exists. It fails, the error is + emitted, and the message is skipped. + +Measured, in this order: `notmuch search --output=files` named a file that was +not on disk while `Background sync running...` was up, and the same query was +clean once the sync finished, with the file present under its new `,U=4` name. +That is why it reads as intermittent, and why it heals itself. + +**It is truthful and it loses nothing.** The move is skipped, no wrong folder +is created, no file is destroyed, and the next sync reconciles. The defect is +that the message blames a folder for a timing problem, and that the action +silently does nothing when the user asked for something. + +**This is `CLAUDE.md`'s `,U=` trap from the other side.** `MaildirName::fresh()` +exists because CARRYING that infix across a folder boundary produced +`Maildir error: duplicate UID` on real mail. Here mbsync is ADDING it and the +index lags; the same infix, the opposite direction. + +**Approach, and it needs a decision.** Two candidates: + +- **Refuse the move while a sync holds the lock.** `SyncMonitor` already + reports this, and the held-edit machinery from items 97 and 106 already + exists for exactly this shape: a tag edit made during a sync is held and + flushed when it ends. Delete would join it rather than inventing anything. + This is the likelier right answer, since it matches what every other + mutation already does. +- **Re-resolve the filename** from notmuch immediately before the rename and + re-query the message if the path is gone. Smaller, but it races the same + window it is trying to close, and a second lookup can be stale by the time + it is used. + +**Constraints.** + +- **Delete reaches the real mail server.** Read `CLAUDE.md`'s item 103 notes + before touching `moveMessages()`: a wrong folder name is created, adopted by + mbsync, and propagated to every other client. +- Whatever is built, **the message must say a sync is running**, not name a + folder. The current wording sent the user looking for a broken folder + configuration, which was correct and configured. +- A test cannot see this in the ordinary fixture layout, where nothing renames + a file underneath the index. Driving it means renaming the file between the + index write and the move, which is what the reproducer has to do. + +**Fixed 2026-08-25.** `moveMessages()` re-resolves by MESSAGE ID when the +recorded path is gone: one `reindexFolder()` of that directory, then the +filename from `notmuch_message_get_filenames()` that exists on disk. Bounded +to a single retry, so a genuinely missing file still reports rather than +becoming a silent no-op. + +**The hold candidate above was investigated and rejected**, and the reason +matters more than the fix. `sendMove()` ALREADY refuses while a sync holds the +lock and queues onto `m_heldMoves` (items 97 and 106 built it), yet the defect +still fired. `aSyncHoldsTheWriteLock()` tracks notmuch's write lock, while this +window sits between mbsync's RENAME and that sync's `notmuch new`; mbsync +renames throughout its run without touching that lock, so the damaging window +is open when there is nothing to observe. Refusing on the lock guards the wrong +resource. That refusal is correct for its own purpose and was left alone. + +Covered by `moveMessagesRecoversWhenASyncRenamedTheFile()` and +`moveMessagesStillReportsAMessageThatIsReallyGone()`, the second pinning the +bounded half. The test renames without reindexing, which is the window mbsync +opens, and asserts the database still names the old path so it cannot pass +against a fixture that quietly reindexed. Mutation-checked. + diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index b390bab..f60ee7f 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -235,8 +235,8 @@ taking that too literally. | 160 | The composer never says a draft was autosaved | feedback | S | **done** 2026-08-25, unreleased. A status bar on the composer: the age line left, the `○ unsaved content` cue beside it. **The fix is a funnel, not a label.** `m_dirty` had SEVEN writers and four of them clear it, only two of which are a save, so a cue hung off the save path silently missed the constructor and the send; `setDirty()` is the one writer now and refreshes both cues plus `setWindowModified()`. Presentation was **reworked after the user looked at it**: it first reused item 151's yellow ribbon treatment, which reads as a misplaced widget on a bare status label, and the cue sat in the permanent (right-hand) tray. Two defects found by probing rather than by reading, see the section | | 161 | The composer has no menu bar | discoverability | S | **done** 2026-08-25, unreleased. File / Edit / Format, to the user's own chosen scope. **Save draft (`Ctrl+S`) is the only NEW action**; everything else is gathered, and the menus show the toolbar's own `QAction` objects rather than copies, per item 140's rule. Two needed hand-building: the HTML toggle is a `QToolButton` and cannot go in a menu, so a checkable twin mirrors it BOTH ways; and the signature entry takes the switch's own `QMenu` pointer, since that menu is rebuilt when the signatures change and copied entries would go stale. Edit's entries follow the editor's own `undoAvailable`/`copyAvailable`. `theMenuBarReachesEveryComposerAction()` is item 132's rule applied to the composer, walking the real menu bar and finding actions by `findChildren`, so a future action added to the toolbar and forgotten in the menus fails without touching the test | -| 162 | Delete fails while a sync is renaming the file underneath it | defect | S | open, 2026-08-25, found by hand. `Cannot move <file> to <folder>`. NOT a Delete defect and not item 158's: mbsync renames an uploaded file to add its `,U=<uid>` infix, and notmuch keeps the pre-`U=` name until that sync's `notmuch new` runs, so `moveMessages` renames a path that no longer exists. Truthful, harmless and SELF-HEALING, which is why it reads as intermittent: verified a ghost present mid-sync and gone after. The message names a folder as though the folder were the problem. Delete reaches the real mail server, so read `CLAUDE.md` before touching it | -| 163 | The message pane shows a stale path and reports the message unreadable | defect | S | open, 2026-08-25, found by hand. Same root as 162 and a DIFFERENT site: the model keeps the filename a row was loaded with, mbsync renames the file to add `,U=<uid>`, and `MimeParser` then opens a path that no longer exists and honestly reports "could not be parsed". notmuch is CORRECT by then; the UI is behind, so 162's fix (refuse the write while a sync runs) does not touch this. The read path needs to recover rather than refuse | +| 162 | Delete fails while a sync is renaming the file underneath it | defect | S | **done, 2026-08-25.** mbsync renames an uploaded file to add its `,U=<uid>` infix and notmuch keeps the pre-`U=` name until that sync's `notmuch new` runs, so `moveMessages` renamed a path that no longer existed and Delete silently did nothing while blaming the destination folder. `moveMessages` now re-resolves by MESSAGE ID when the recorded path is gone: one reindex of that directory, then the filename that exists on disk. Bounded to one retry, so a file genuinely gone still reports. Holding the move during a sync was the other candidate and is NOT the fix: `sendMove` already refuses on notmuch's write lock, but this window sits between mbsync's rename and that sync's `notmuch new`, which touches no lock | +| 163 | The message pane shows a stale path, and the composer forks the draft | defect | S | open, 2026-08-25, found by hand. The model keeps the filename a row was loaded with, mbsync renames the file to add `,U=<uid>`, and `MimeParser` then opens a path that no longer exists. notmuch is CORRECT by then; the MODEL is behind. **A SECOND site found 2026-08-25 and it is the worse half:** `openComposerFor` passes `ref.filePath` to `forDraft`, the parse fails, and the reopen is REFUSED before any composer exists, so the user composes a fresh draft whose autosave has no previous path to unlink. The old revision survives, each save mints a new Message-ID, and both reach the server. Item 162's fix does not touch either site: that one re-resolves inside the worker, these hold a stale path in the UI | | 164 | A draft this application saved keeps `inbox` | defect | S | open, 2026-08-25, **cause corrected 2026-08-25**. The first diagnosis blamed a missing drafts helper and was WRONG: `NOT_ARRIVALS` in `qtmaildirconf.py` is `("sent", "drafts")`, the folder list includes every account's drafts folder, and `notmuch count` confirms the carve-out query MATCHES the affected draft. The carve-out is scoped to `tag:new`, and the draft carries `inbox` while `tag:new` is 0, so it was never in scope when the hook ran. Measured separately: an mbsync-style rename does NOT re-add `new.tags`, so the retag theory is out too. What remains unestablished is WHICH pass tagged it; establish that before writing code | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1341,64 +1341,7 @@ The 70-second duration recorded above fits a `QTRY_*` waiting for a file that is never going to appear, which is consistent with a wrong destination rather than a slow one. -## 162. Delete fails while a sync is renaming the file underneath it - -**Observed (user, 2026-08-25):** deleting a draft reported `Cannot move -<file> to <account>/Trash`. - -**Cause (verified against the live Maildir, not read):** a stale path, and -neither Delete nor item 158 is at fault. - -1. The composer autosaves a draft as `<name>:2,D` and item 158 indexes it - under exactly that filename. -2. **mbsync uploads it and RENAMES it** to `<name>,U=<uid>:2,D`, recording the - server UID in the filename. -3. notmuch still holds the pre-`U=` name until that sync's `notmuch new` runs. -4. `moveMessages()` reads the filename from notmuch and calls - `QFile::rename()` on a path that no longer exists. It fails, the error is - emitted, and the message is skipped. - -Measured, in this order: `notmuch search --output=files` named a file that was -not on disk while `Background sync running...` was up, and the same query was -clean once the sync finished, with the file present under its new `,U=4` name. -That is why it reads as intermittent, and why it heals itself. - -**It is truthful and it loses nothing.** The move is skipped, no wrong folder -is created, no file is destroyed, and the next sync reconciles. The defect is -that the message blames a folder for a timing problem, and that the action -silently does nothing when the user asked for something. - -**This is `CLAUDE.md`'s `,U=` trap from the other side.** `MaildirName::fresh()` -exists because CARRYING that infix across a folder boundary produced -`Maildir error: duplicate UID` on real mail. Here mbsync is ADDING it and the -index lags; the same infix, the opposite direction. - -**Approach, and it needs a decision.** Two candidates: - -- **Refuse the move while a sync holds the lock.** `SyncMonitor` already - reports this, and the held-edit machinery from items 97 and 106 already - exists for exactly this shape: a tag edit made during a sync is held and - flushed when it ends. Delete would join it rather than inventing anything. - This is the likelier right answer, since it matches what every other - mutation already does. -- **Re-resolve the filename** from notmuch immediately before the rename and - re-query the message if the path is gone. Smaller, but it races the same - window it is trying to close, and a second lookup can be stale by the time - it is used. - -**Constraints.** - -- **Delete reaches the real mail server.** Read `CLAUDE.md`'s item 103 notes - before touching `moveMessages()`: a wrong folder name is created, adopted by - mbsync, and propagated to every other client. -- Whatever is built, **the message must say a sync is running**, not name a - folder. The current wording sent the user looking for a broken folder - configuration, which was correct and configured. -- A test cannot see this in the ordinary fixture layout, where nothing renames - a file underneath the index. Driving it means renaming the file between the - index write and the move, which is what the reproducer has to do. - -## 163. The message pane shows a stale path and reports the message unreadable +## 163. The message pane shows a stale path, and the composer forks the draft **Observed (user, 2026-08-25):** selecting a draft filled the pane with `(unreadable message)` and `This message could not be parsed.`, naming a file @@ -1425,6 +1368,38 @@ parse, re-resolve the message id through notmuch and retry once before reporting. `recoverStaleThread()` already exists for the neighbouring problem (item 91 reuses it) and is the shape to follow. +**A SECOND site, found 2026-08-25 while hand-testing item 164, and its +consequence is worse than the pane's.** `MainWindow::openComposerFor()` passes +`ref.filePath` to `ComposeContextBuilder::forDraft()`, and `MessageRef::filePath` +is `notmuch_message_get_filename()` captured when the QUERY ran. After mbsync +renames an uploaded draft, a row loaded before that sync names a file that no +longer exists, `MimeParser::parse` fails, `forDraft` returns an empty context +and the composer reports "That draft could not be read". + +Measured, in order: draft written 10:42, sync at 10:50:43 logged "Detected 2 +file renames" and the file became `,U=5`, the user reopened it at 10:52 and +saw the error. + +**The pane's version is cosmetic and self-repairing. This one silently forks +the draft.** The refusal happens BEFORE `openComposer()`, so the user is left +with no composer for that draft. Composing again opens a FRESH one with +`m_draftPath` empty, so its first autosave has no previous path to unlink: +it writes a new file, `DraftStore` unlinks nothing, and the old revision +stays. Both are then uploaded and both reach the server. + +Two properties make this worse than it first reads. Each save mints a NEW +Message-ID (four saves in the hand test produced four ids), so the revisions +are distinct MESSAGES to notmuch rather than two files of one, and no dedup +anywhere will collapse them. And the unlink machinery is entirely correct +throughout, which is why the code reads fine: `forDraft` sets `draftPath`, +the composer seeds `m_draftPath`, the autosave passes it as `previousPath`, +and `DraftStore::write` unlinks it. The comment at `composecontext.cpp:501` +already names this exact failure ("one message becomes two"). None of it runs, +because the reopen was refused before any of it was reached. + +So the fix below covers this site too, and fixing only the pane would leave +the draft-forking half in place. + **Constraints.** - **A retry must be bounded.** A message that genuinely cannot be parsed @@ -1433,6 +1408,13 @@ reporting. `recoverStaleThread()` already exists for the neighbouring problem - Re-resolving by id is what makes this safe; re-scanning the folder is not, since two files can carry the same id. - The placeholder wording is correct and should stay for the genuine case. +- **The composer site must recover, not merely report better.** A clearer + error still leaves the user composing a second copy of their own draft. +- Whether a draft should keep a STABLE Message-ID across revisions is a + separate question this entry does not decide. It is what turns a stale path + into two server-side messages rather than one replaced file, and it wants + its own item; note that a draft's id is not yet the sent message's id, so + changing it is not obviously free. ## 164. A draft this application saved keeps `inbox` @@ -1469,10 +1451,59 @@ indexes a draft from the application itself, outside `notmuch new`, and a file already known to the database is not a new file on the next pass. But that is a hypothesis and the last two hypotheses here were both wrong. -**Approach.** Reproduce first, in a throwaway database, driving the real -sequence: index the file the way `indexDraftFile()` does, then rename it the -way mbsync does, then `notmuch new`, then the hook. Only then decide whether -the fix belongs in the hook's scope, in item 158's indexing, or in neither. +**The reproducer was built (2026-08-25) and it settles the mechanism.** Seven +variants were driven in throwaway databases, modelling `indexDraftFile()` with +a real `notmuch_database_index_file` call rather than the CLI, because no CLI +command indexes an untracked path without applying `new.tags`. + +What the sweep established, each measured rather than reasoned: + +- `index_file` applies **no tags at all**. A draft the application indexes is + therefore never in `tag:new` scope, and the hook has nothing to carve out. +- Whenever the file IS in `tag:new` scope, the carve-out strips `inbox` + correctly, in every filename shape tried: `:2,DS`, `:2,D`, no info suffix, + in `cur/` and in `new/`, with and without the `,U=4` infix. The real file's + shape (`,U=4:2,D`) is among them. +- It survives the orderings too: `notmuch new` first then the app's index, + the app's index first then the rename, an autosave landing between + `notmuch new` and the hook, and the stale-path `remove_message` that makes + the renamed file arrive as new mail. All six left the draft clean. +- The `D` flag is what puts `draft` on the message (`synchronize_flags`), and + the `S` flag is what removes `unread`. The affected file is `:2,D`, which is + why it carries `unread`, and that matches the reported tag set exactly. + +**The one variant that reproduces it** is the general shape rather than a +filename detail: a pass where `inbox` is applied while `tag:new` has ALREADY +been consumed. Modelled as a file indexed at a path the carve-out does not +cover and moved into the drafts folder afterwards, it ends in precisely the +live end state, `draft inbox unread` in Drafts with `,U=4` and `tag:new` at 0. +Nothing revisits a message once the marker is gone, so the tag is permanent. + +**What is still NOT established, and the next step.** The affected account +writes drafts straight to `<account>/Drafts`, which the carve-out +covers (verified against the live config and the live query, which matches the +message by id today), so the reproducing variant's premise does not hold for +it as written. The live log for the pass that added it reads + + 10:10:52 Added 1 new message to the database. Detected 9 file renames. + 10:10:52 post-new: sent-folder carve-out applied over 9 folder(s) + +so the hook DID run on that pass, over a path the query covers, and logged +success. The remaining candidates are all about what the path or the marker +looked like at that instant, not about the query text: the carve-out logs +"applied" on a `notmuch tag` that matched zero messages, so a successful log +line is not evidence the message was in scope. Instrumenting the hook to log +the carve-out's MATCH COUNT, and leaving it to run until the next draft, is +the cheapest way to close it, and is a log-only change to code that tags real +mail unattended. + +The filename also rules one thing in: `1787645266.M802P16149Q3.<host>` is +exactly `MaildirName::fresh()` output, so the application wrote this file. It +is not a draft another client left behind. + +The reproducer scripts are throwaway and were not kept; `indexfile.c` is +fifteen lines around one `notmuch_database_index_file` call and is trivial to +rebuild from this entry if the instrumentation points back at the hook. **Constraints.** |