fix: encode MODSEQ message data item with parentheses - #723
Conversation
RFC 7162's `fetch-mod-resp` production parenthesizes the value:
fetch-mod-resp = "MODSEQ" SP "(" permsg-modsequence ")"
but `MessageDataItem::ModSeq` was encoded as `MODSEQ 624140003`. The
decoder already required the parentheses, so a response carrying this
data item did not survive a decode -> encode -> decode round trip.
Only the *response* data item is parenthesized; the *command* data item
name (`MessageDataItemName::ModSeq`) is the bare `MODSEQ` and is
unchanged.
Adds a known-answer encode test and a round-trip test over RFC 7162
Section 3.1.4.2's Example 13, plus the `fetch-mod-resp` production in
`msg_att_dynamic`'s ABNF doc comment.
Fixes duesee#722
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LQkXHvJk9Ls8v9odBhonf
There was a problem hiding this comment.
Thank you! I left some comments inline and also have a more general request.
I feel that most comments are redundant given that we include the ABNF definition. They are not wrong but IMAP made a few... let's say... "questionable" syntax choices so what people generally should do is to ignore all prose and stick to the ABNF anyway. So I feel there is no need to point out these curiosities.
Also, I have not decided yet how to handle AI contributions and this would be the first one (co-authored: claude). Given this PR merely adjusts a string (by adding ( and )) and adds some (straightforward) tests, I would ask to "redo" the PR without AI. This way I could merge it w/o having to devide how I feel about AI contributions just yet.
| ctx.write_all(b"] ")?; | ||
| size.encode_ctx(ctx) | ||
| } | ||
| // Note: RFC 7162's `fetch-mod-resp` parenthesizes the value: |
There was a problem hiding this comment.
I feel we can remove all this comments when we just put the ABNF from the RFC.
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| #[cfg(feature = "ext_condstore_qresync")] |
There was a problem hiding this comment.
Could we move all tests to imap-codec/src/extensions/condstore_qresync.rs?
| #[test] | ||
| fn test_encode_message_data_item_modseq() { | ||
| // RFC 7162, Section 3.1.4.2: `Syntax: MODSEQ ( <permsg-modsequence> )`. | ||
| // Note that only the *response* data item is parenthesized; the *command* data item |
2 participants
Fixes #722.
The defect
RFC 7162 §7 defines the FETCH response data item as
but
MessageDataItem::ModSeqencoded asMODSEQ 624140003— without the parentheses. The decoder (msg_att_dynamic) already required them, so the two disagreed and a response carrying this data item did not survive a decode → encode → decode round trip:Note the asymmetry that likely caused this: the command data item name is the bare
MODSEQ, and only the response data item is parenthesized.MessageDataItemName::ModSeqwas already correct and is unchanged here.The change
imap-codec/src/codec/encode.rs— one line,MODSEQ {value}→MODSEQ ({value}), with a note recording the command/response asymmetry so it doesn't get "simplified" back.imap-codec/src/fetch.rs— two regression tests, both gated onext_condstore_qresyncand both failing before the fix:test_encode_message_data_item_modseq— known-answer encode ofMODSEQ (624140003).test_kat_inverse_response_fetch_modseq—kat_inverse_responseover all three FETCH responses from RFC 7162 §3.1.4.2, Example 13.fetch-mod-respproduction tomsg_att_dynamic's ABNF doc comment, which previously only documentedFLAGS.No API change, so no SemVer impact.
Verification
justisn't installed here, so I ran the CI legs directly, all withRUSTFLAGS/RUSTDOCFLAGS=-D warnings:cargo +nightly fmt --checkcargo clippy --workspace --all-targets --exclude imap-codec-bench, and again with--all-featurescargo test --workspace --exclude imap-types-fuzz --exclude imap-codec-fuzz --all-targets --exclude imap-codec-bench, and again with--all-featurescargo test -p imap-codec --no-default-features --features ext_condstore_qresync(the feature in isolation)cargo doc --no-deps --document-private-items --keep-going --all-featuresAll green. Reverting just the encoder line turns both new tests red, so they do fence the bug.
🤖 Generated with Claude Code
https://claude.ai/code/session_012LQkXHvJk9Ls8v9odBhonf