fix(imap): dedupe accounts by wire identity; doctor's "connection failed: undefined" (#138); version-guard CHANGELOG hardening - #139
Merged
Conversation
…led: undefined" (#138) Two defects with one shape: the legacy singular IMAP env keys were treated as a special identity separate from APPLE_MAIL_MCP_IMAP_ACCOUNTS. 1. listImapAccountSpecs deduped on the account LABEL, so declaring one mailbox twice — once via the legacy keys, once as an ACCOUNTS entry under a different nickname — yielded two specs. Every merge-across-accounts caller then visited that mailbox twice. Dedupe now keys on the resolved (host, port, user) triple via a new imapIdentityKey(), which poolKey() delegates to so the pool and the enumerator cannot drift. The collapsed nickname is kept as an alias so it still addresses the mailbox. 2. imapHealthCheck's "is IMAP configured?" gate tested only the legacy APPLE_MAIL_MCP_IMAP_USER, so an ACCOUNTS-only setup short-circuited to {configured:false, ok:false} with no error field — which doctor rendered as the literal "connection failed: undefined" for every account, without ever attempting a connection. It now asks the same enumerator every other caller asks, and doctor never interpolates a bare optional. Also hardens version-guard.yml's CHANGELOG rules: release headings are now append-only, and "## [Unreleased]" must be empty on a bump.
…tical across all four repos
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.
Three changes. Two are user-visible IMAP bugs with a shared shape; the third hardens
version-guard.ymland is applied byte-identically across all four repos.Release: 2.10.7.
1. One mailbox declared twice was counted twice (
get-unread-count,get-mail-stats)listImapAccountSpecs()deduplicated accounts on the label:A config that declares the same mailbox through both the legacy singular keys (
APPLE_MAIL_MCP_IMAP_USER+_IMAP_ACCOUNT) and anAPPLE_MAIL_MCP_IMAP_ACCOUNTSentry carrying a different nickname walks straight past that comparison — the guard only catches the case where someone reuses the same name. Nothing downstream re-checks, so every caller that fans out over the account list visits the mailbox twice and sums it twice.Measured live, on a four-identity config where two identities are one Gmail mailbox, both builds queried within seconds of each other:
get-unread-count(all accounts)robert.b.sweet@gmail.comPersonal Gmail(same mailbox)rob@superiortech.ioiCloud23 = 8 + 8 + 7 + 0. The true total is 15.
get-mail-statsinflated its message and unread totals the same way.Fix
Dedupe on the mailbox's actual wire identity — the resolved
(host, port, user)triple — via a newimapIdentityKey(). The connection pool'spoolKey()now delegates to the same function, so the pool's notion of "the same account" and the enumerator's are literally one definition and cannot drift apart. The label check is kept as a secondary guard so two genuinely distinct mailboxes still can't share one nickname.Normalisation is deliberately asymmetric: host is folded case-insensitively (DNS is), the user local part is not — RFC 5321 leaves it case-sensitive and only the receiving server may fold it, so folding it here could silently drop a real account. Double-counting is the lesser failure.
…and collapsing the duplicate does not break the nickname it collapsed
Worth calling out because the first cut of this change did regress: once
Personal Gmailstopped being its own account,account: "Personal Gmail"no longer resolved and fell through to AppleScript, which reportedCan't get account "Personal Gmail". Caught on live data, not by the unit tests.The dropped entry's label is now retained as an alias of the surviving account, so a caller already passing that nickname keeps resolving to the same mailbox — it is simply no longer a second account. The three places that matched a selector against an account (the read-routing gate, the config resolver, the composite-id ownership check) were three copies of
s.accountLabel === x || s.user === x; they now share onespecMatchesSelector()predicate.2.
doctorreportedconnection failed: undefinedfor every account (#138)Reported by @jarrah31, whose write-up isolates the guard, the call path and the reason the field is absent — the diagnosis below is his, confirmed independently before I read it.
imapHealthCheck's "is IMAP configured?" gate tested only the legacy singular variable:So a config declaring its accounts solely through the documented multi-account array — no legacy keys at all — short-circuits to
{configured: false, ok: false}carrying noerrorfield, for every account, without ever attempting a connection.doctorinterpolates that absent field bare, and the user gets the literal wordundefinedwhere the diagnosis should be. Every other IMAP path enumerates the array correctly, which is exactly why the tools work while onlydoctordisagrees.Reproduced before fixing:
Fix
The gate now asks the same enumerator every other caller asks:
@jarrah31 suggested
!deps.accountinstead, which also fixes the reported symptom —doctoris currently the only caller and always passes a label. I went with the enumerator because it is the same question the rest of the module asks, and it stays correct if a future caller passes no account (under the!deps.accountform, a bareimapHealthCheck()on an array-only config would still wrongly report unconfigured).Two follow-ons in the same spirit:
doctorno longer interpolates the optional field bare — it falls back to words, so no future health-check failure can renderundefined. There is a test asserting the rendered report never contains that string.IMAP not configurederror now namesAPPLE_MAIL_MCP_IMAP_ACCOUNTStoo, instead of pointing only at the legacy variable — the same asymmetry, in user-facing text.docs/IMAP-SETUP.md, since @jarrah31 reasonably read the docs as saying array-only was unsupported ("the docs describe the legacy vars as defining the first/default account and the array as adding the rest"). The "Multiple accounts" section now states the array is sufficient on its own and that the first entry becomes the default; the env-var reference no longer claims onlyAPPLE_MAIL_MCP_IMAP_USERenables IMAP. It also documents the rule fix 1 enforces: don't declare one mailbox twice.Note both bugs are the same underlying mistake: treating the legacy singular keys as a privileged, separate identity rather than one row in the account list.
3.
version-guard.yml: CHANGELOG history is append-only, and[Unreleased]must be empty on a bumpTwo holes in the rule added in 2.10.5. Pure insertion: 70 lines added, 0 removed. Workflow
name: version-guard, job keyrequire-version-bump, and both triggers are untouched.apple-notes-mcp, apple-numbers-mcp and apple-photos-mcp already merged this change;
version-guard.ymlis inconformance-check.sh's byte-identical set, and the file on this branch is now byte-identical to all three (sha256 be0df4ea…). (I initially also rewrote the file's header comment to describe all three CHANGELOG rules — a nicety the parent task marked optional. The siblings did not take it, so it would have broken the IDENTICAL set. Reverted; the last commit on this branch is that revert.)Hole 1 — heading deletion. The existing rule proves only that the new version has a heading, which a PR can satisfy by renaming an existing one: retitling
## [1.1.12]to## [1.1.13]leaves 1.1.13 documented and erases 1.1.12. apple-numbers-mcp #54 did exactly that, and since nothing downstream readsCHANGELOG.mdit stayed invisible until an audit. Every## [X.Y.Z]heading present at the base must now still be present.Closed outside the bump branch, because a rename can land in a PR that bumps or one that does not. Compared against the checked-out tree, not
git show HEAD_SHA:…— underpull_requestthe checkout is the merge commit, so a branch left open across a release already contains main's newer headings; readingHEAD_SHAwould false-fail every such PR. Membership is tested withwhile read+grep -qxFrather thancomm, matchingconformance-check.sh's documented reasoning (no sort/locale assumptions; version strings are full of regex metacharacters).Hole 2 — the
[Unreleased]drain. The heading rule says nothing about notes still parked under## [Unreleased], which the release drains: everything on main ships in the next publish, so prose left under that marker describes released behaviour while claiming to be unreleased, and nothing renames the section later. Nothing guarded this at all. Closed inside the bump branch. It first asserts the marker exists —dependabot-rebuild.ymlhard-exits without it — then that the body is whitespace-only, printing the offending content indented. Both useindex($0,"## [Unreleased]")==1(conformance-check.sh's idiom, avoiding awk\[portability questions); the terminatorindex($0,"## ")==1correctly ignores### Fixedsub-headings.Edge cases
--unshallowwhen--is-shallow-repository, then hard-fail if the base commit is still unreachable rather than skipping. Skipping would be the same false-pass classconformance-check.sh's preflights exist to prevent. (In practice a truly depth-1 clone already dies one step earlier, at the pre-existinggit diff BASE...HEAD; this is belt-and-braces.)::notice::and continue. Distinguished from an unreachable base commit, which fails.set -euo pipefail— every new pipeline is either anif/||condition or ends in a command that cannot fail. No unguardedpipefailexits.Accepted trade-off
Deliberately archiving old entries out of
CHANGELOG.mdnow fails. Intentional and documented in the comment: the file is the only record of what each published version contains, and the guard cannot distinguish archival from the rename that erased 1.1.12. Second-order: if## [Unreleased]on main ever became non-empty, every bumping PR would fail until the notes are filed — that is the drain being enforced, and it fails loudly. All four repos have an empty## [Unreleased]on main today.Verification
IMAP fixes
doctorconnects toimap.gmail.com×2 andimap.mail.me.com— three✅, noundefined.imapHealthCheckconfigured-gate both ways, anddoctornever renderingundefined.test436 passed / 31 files,typecheck,lint(0 errors; 10 pre-existingno-explicit-anywarnings, none in touched files),format:checkall clean.version-guard
8806992) and post-merge (8b0de21) CHANGELOGs → fails, naming 1.1.12. The hole is provably closed on the actual incident.mainCHANGELOGs → heading extractor finds 74 / 63 / 21 / 24 headings,## [Unreleased]present and empty in all four. No false positives; no repo is blocked.[Unreleased]+ bump (fails); same content with no bump (passes — correctly scoped); marker deleted (fails); whitespace-only[Unreleased](passes); date-only heading edit with the version preserved (passes); CHANGELOG deleted (fails); first commit with no base CHANGELOG (passes); and both pre-existing rules still behaving (no-change passes, shipped-bytes-without-bump fails).dependabot-rebuild.yml's exact auto-bump node snippet against all four real CHANGELOGs, then applied all three rules to each result: PASS ×4, including photos' leading# Changelogtitle line.preserves every release heading,documents 2.10.7,'## [Unreleased]' is empty,Shipped bytes changed and version is bumped — OK, exit 0.bash -nclean,prettier --checkclean, diff vsmainis a pure insertion (70 0).main— all four hash tobe0df4ea…, soconformance-check.sh's IDENTICAL set holds once this merges.Notes for the maintainer
version-guard.ymlbyte-identity is verified, not assumed. The other three repos merged this change first; apple-mail-mcp was the odd one out (180 lines vs their 250). This branch brings it to 250 lines,sha256 be0df4ea69af02461606da9fcf19390b81b9158bb14a5d9aa45f683451518681— matching all three exactly, soconformance-check.shpasses on merge.config.json. The code fix makes the double-count impossible, but~/Library/Application Support/apple-mail-mcp/config.jsonstill declaresrobert.b.sweet@gmail.comtwice — the legacyAPPLE_MAIL_MCP_IMAP_ACCOUNT/_IMAP_USER/_IMAP_KEYCHAIN_*block and thePersonal Gmailentry inAPPLE_MAIL_MCP_IMAP_ACCOUNTSare the same mailbox. Dropping either one is now cosmetic rather than load-bearing. Not touched by this PR.Fixes #138.