Conversation
…e codes Fixes issue #83 plus two related bugs of the same shape in COPY and MOVE. The original issue only named APPEND; while re-auditing the extension command handlers I found the same pattern in handleCopy and handleMove. ## The bugs RFC 4315 Section 3: APPENDUID / COPYUID response codes carry the DESTINATION mailbox's UIDVALIDITY as their first argument. RFC 6851 Section 4 inherits the same COPYUID semantics for MOVE. Three handlers got this wrong: 1. **handleAppend** hardcoded `APPENDUID 1 ${uid}`. The literal `1` was used regardless of the actual destination mailbox's UIDVALIDITY. 2. **handleCopy** used `uidMap.uidValidity` which is the currently selected (source) folder's UIDVALIDITY, not the destination's. For cross-mailbox copies and for copies between folders that have different UIDVALIDITY values, this returned the wrong value. 3. **handleMove** had the same bug as handleCopy. Any IMAP client that caches by (uidValidity, uid) tuples (which is standard) would either think its cache is still valid when it is not, or invalidate on every COPY/MOVE/APPEND because the response UIDVALIDITY matched the source. ## The fix Extended the `ExtensionAdapter` interface. All three return types now include `uidValidity: number`: - `copyMessage(messageId, targetFolderId) -> { newUid, uidValidity }` - `moveMessage(messageId, targetFolderId) -> { newUid, uidValidity }` - `appendMessage(folderId, content, flags, date?) -> { uid, uidValidity, messageId }` The adapter knows the destination folder's UIDVALIDITY because it just wrote to that folder. No extra query is needed; this is a pure return-type extension. Handlers updated to use the returned validity: - `handleAppend`: `[APPENDUID ${result.uidValidity} ${result.uid}]` - `handleCopy`: captures `destUidValidity` on the first adapter call (all copied messages share one destination) and uses it in the COPYUID response. - `handleMove`: same pattern, captures `destUidValidity` on first moveMessage call. Added a "no messages copied/moved" guard: if the sequence set resolves to zero messages, the handler omits the COPYUID response code entirely and just returns tagged OK. Previously the code emitted `[COPYUID <stale> ]` with trailing empty strings. ## Breaking change `ExtensionAdapter` is a public interface consumers implement in their adapters. The return-type extension is a breaking change: existing adapters that return `{ newUid }` without `uidValidity` will fail typecheck on upgrade. The upgrade is mechanical: add `uidValidity` to the return from each method's adapter implementation. Pre-1.0 is the right time for this. The framework is shipping at 0.1.0 and no consumers have written adapters yet against the published packages. ## Tests Three new tests added to `packages/imap/tests/commands/extensions.test.ts`, each citing the RFC section it verifies: 1. `returns destination UIDVALIDITY in APPENDUID response (RFC 4315 Section 3)`: calls handleAppend with a mock adapter that returns `uidValidity: 42, uid: 300`. Asserts the response contains `[APPENDUID 42 300]`. Before the fix, this failed with `[APPENDUID 1 300]`. 2. `returns destination UIDVALIDITY in COPYUID response (RFC 4315 Section 3)`: source session has `uidValidity: 1`, mock copyMessage returns `uidValidity: 42`. Asserts the response contains `[COPYUID 42 `. Before the fix, this failed with `[COPYUID 1 ...]` (source validity). 3. `returns destination UIDVALIDITY in MOVE's COPYUID response (RFC 6851 Section 4)`: same setup for MOVE. Same failure mode before the fix. The existing substring-match tests (`toContain("APPENDUID")` and `toContain("COPYUID")`) were kept -- they still pass because the literal prefix appears in both old and new responses. The new tests exercise the actual validity value, which the old tests never checked. ## Verification - pnpm --filter @rafters/mail-imap test: 316 passing (313 existing + 3 new) - pnpm test (workspace): 617 passing across 37 files (was 614 + 3 new) - pnpm -r build: all 9 packages build ESM + dts - pnpm typecheck: clean across workspace - pnpm lint: 0 warnings, 0 errors - pnpm format:check: clean Closes #83. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes issue #83 (hardcoded `APPENDUID 1`) plus two related bugs of the same shape in COPY and MOVE that I found while re-auditing the extension handlers. Breaking change to the `ExtensionAdapter` interface -- the three return types now carry `uidValidity`.
The bugs
RFC 4315 Section 3 says APPENDUID and COPYUID carry the destination mailbox's UIDVALIDITY as their first argument. RFC 6851 Section 4 inherits the same semantics for MOVE. Three handlers got this wrong:
Any IMAP client that caches by `(uidValidity, uid)` tuples (which is standard) would either trust stale data or invalidate unnecessarily on every COPY/MOVE/APPEND.
The fix
Extended the `ExtensionAdapter` interface to return `uidValidity` from all three methods. The adapter knows the destination's UIDVALIDITY because it just wrote to that folder -- no extra query needed:
```typescript
interface ExtensionAdapter {
copyMessage(messageId, targetFolderId): Promise<{ newUid, uidValidity }>;
moveMessage(messageId, targetFolderId): Promise<{ newUid, uidValidity }>;
appendMessage(folderId, content, flags, date?): Promise<{ uid, uidValidity, messageId }>;
// ...
}
```
Handlers updated to use the returned validity:
Also added a "no messages copied/moved" guard: if the sequence set resolves to zero messages, the handler now omits the `COPYUID` response code entirely and returns tagged OK. Previously the code emitted `[COPYUID ]` with trailing empty strings.
Breaking change
`ExtensionAdapter` is a public interface consumers implement. The return-type extension is a breaking change -- existing adapter implementations that return `{ newUid }` without `uidValidity` will fail typecheck on upgrade. The upgrade is mechanical: add `uidValidity` to each method's return. Pre-1.0 is the right time; no consumers have written adapters yet against the published packages.
Tests
Three new tests in `packages/imap/tests/commands/extensions.test.ts`, each citing the RFC section it verifies:
The existing `toContain("APPENDUID")` / `toContain("COPYUID")` substring-match tests stayed -- they still pass because the prefix appears in both old and new responses. The new tests check the actual validity value, which the old tests never did.
Process note
I wrote the failing tests first, confirmed they reproduced the bugs, then fixed the implementation. This is the corollary I added to memory after yesterday's audit mistake with issue #82 (which I filed based on an incomplete parser read). Writing the failing test first would have caught that mistake too -- there was no way to write a failing test because the feature existed.
Verification
Closes #83.