fix(outlook-addon): request ReadWriteItem, not ReadWriteMailbox

[dobby-coder]

Closes #254.

apps/outlook-addon/manifest.xml requested ReadWriteMailbox, the highest permission tier. It now requests ReadWriteItem.

Why the narrower tier is the right one

Microsoft annotates every Office.js member in @types/office-js with its Minimum permission level, and that annotation is the method the add-in permissions doc itself prescribes for sizing the request. I re-derived the audit from #242 against HEAD and @types/office-js@1.0.601 rather than taking it on trust, mapping every Office.* and item.* member reached from src/ to its annotation.

The maximum across the whole set is read/write item, from six compose writes:

Member	Annotated tier
Body.setAsync	read/write item
MessageCompose.removeAttachmentAsync	read/write item
MessageCompose.addFileAttachmentFromBase64Async	read/write item
MessageCompose.saveAsync	read/write item
InternetHeaders.setAsync	read/write item
InternetHeaders.removeAsync	read/write item

Everything else the code reaches is read item or restricted: Subject.getAsync / setAsync, Recipients.getAsync, From.getAsync, Body.getAsync, InternetHeaders.getAsync, getAttachmentsAsync, getAttachmentContentAsync, getAllInternetHeadersAsync, addHandlerAsync, NotificationMessages.replaceAsync / removeAsync, UserProfile.emailAddress / displayName, and roamingSettings (restricted).

The complementary check came back empty. The mailbox-tier members an add-in can reach have no hits in src/: makeEwsRequestAsync, getSelectedItemsAsync, loadItemByIdAsync, masterCategories, sendAsync. Neither do getCallbackTokenAsync, restUrl, ewsUrl or getSharedPropertiesAsync. The read flow avoids EWS deliberately, reading attachment bytes through getAttachmentContentAsync in both modes (office-helpers.ts). The two LaunchEvents do not force the wider tier either, and the banned legacy ItemSend is unused.

The second commit is part of the same concern rather than a drive-by: getReadAttachmentContent was documented as reading "by REST or makeEwsRequest fallback" and has neither path. "The read flow avoids EWS deliberately" is the justification for the narrower tier, and that comment is the first thing anyone auditing the claim reads.

The test

test/manifest-permission.test.ts pins both halves, because the declared constant on its own would only restate itself:

1. The manifest declares exactly ReadWriteItem.
2. No member needing the mailbox tier is reachable from src/.

The second is what keeps the first honest. It derives the mailbox-tier member set from the annotations in @types/office-js instead of a hand-written list, so a types bump that moves a member between tiers is picked up here. Two filters turn 593 annotations into names whose bare presence in src/ is proof of a reach for the wider tier:

1. Drop any name also annotated at a lower tier. subject, getAsync and removeAsync all appear at read/write mailbox (on SelectedItemDetails and MasterCategories) as well as on the ordinary item, so matching those by name would flag every well-behaved call.
2. Drop any name whose every declaring interface is handed out by another surviving name. hasAttachment and itemMode are annotated read/write mailbox and nowhere else, but they sit on SelectedItemDetails, which an add-in can only obtain by calling getSelectedItemsAsync — already in the set. They add no reach.

What survives is the five gates, and the floor names them one by one rather than counting the set. It strips comments before scanning, since the question is what the code reaches and not what the code talks about; the corrected comment in office-helpers.ts names makeEwsRequestAsync in prose and is correctly not flagged. It fails loudly rather than asserting nothing if it cannot parse the annotations or the src/ tree.

Verified by mutation in both directions, not by inspection:

• Manifest reverted to ReadWriteMailbox → check 1 fails, 2 passes.
• A makeEwsRequestAsync call planted in src/ → check 2 fails naming the file and the member, 1 passes.
• The makeEwsRequestAsync declaration in @types/office-js reshuffled out of the parser's reach, so the derivation loses it → fails with makeEwsRequestAsync is no longer derivable as mailbox-tier, so the scan is blind to it. Same for sendAsync.
• An ordinary local named hasAttachment / itemMode planted in src/lib/mime.ts → passes, no false claim.
• A /* opened inside a // comment above a real call, with an ordinary JSDoc block below it → fails. The whole-file block-comment regex this replaced reported 41/41 there, blind to the call.
• A gate named on the continuation line of a multi-line block comment → passes, no false claim.
• All restored → 41/41 pass.

What I ran

Check	Result
pnpm validate (source manifest)	valid
pnpm build + pnpm validate:dist, edge origins	valid, dist/manifest.xml:43 reads ReadWriteItem
pnpm build + pnpm validate:dist, production origins	valid
pnpm typecheck (app) and pnpm -r typecheck	clean
pnpm check-version	1.0.1.0 / 1.0.1 agree
Lint + prettier, with the exact globs from Lint, typecheck & build	clean
pnpm -r test	pass, including the two new assertions

The two entrypoint size warnings on taskpane.js and yivi-dialog.js are the documented baseline, not a regression.

Still needed before merge, and I could not do it

The tier is enforced by Outlook at each API call, so a green CI run does not prove the flows still work. The manual acceptance step in #254 stands: sideload dist/manifest.xml and confirm compose (encrypt and send, including plaintext attachment removal and the postguard.encrypted attachment) and read (decrypt a message) end to end. I have no Outlook host or M365 account here.

What I can offer instead is the static half: every member the flows call is annotated at or below read/write item, so no call in either flow should be denied. That is an argument, not a test on a real host.

Out of scope, deliberately

apps/website/static/downloads/postguard-outlook-manifest.xml:43 also says ReadWriteMailbox. It is a mirror of the manifest attached to the last outlook-addin-v* release, written by apps/website/scripts/sync-addons.mjs, and that release genuinely shipped the wider tier. It updates itself on the next release; hand-editing it would put the mirror out of step with the artifact it mirrors. Noted in apps/outlook-addon/CLAUDE.md so the next person does not go looking.

Per #240 a changed permission tier freezes existing installations until an administrator re-consents, which is the argument for landing this before the first AppSource submission rather than after.

Review round 1 (a5100e7)

The guard's mailboxOnly.length >= 5 floor could not notice the derivation losing the members it existed to protect. The derived set was seven names, not the five every comment claimed: hasAttachment and itemMode padded it, so the floor could lose both makeEwsRequestAsync and sendAsync and still clear >= 5. Reproduced on the previous head — with the makeEwsRequestAsync declaration reshuffled the way an overload split does and a real item.makeEwsRequestAsync(...) call in src/lib/mime.ts, the suite reported 41/41 pass.

• The candidate set now drops names whose every declaring interface is handed out by another candidate, which removes exactly hasAttachment and itemMode and leaves the five gates. That also clears a false positive: \b<name>\b fired on const hasAttachment = parts.length > 0. Narrowing the set rather than the regex keeps the broad match, which still catches item["makeEwsRequestAsync"] and destructuring that member-access matching would miss.
• The count floor is now a per-gate assertion, so a derivation that drops one fails naming which gate went missing. The scan set stays derived.
• The five-name enumeration in the test header, the changeset and this description is now what the code derives; each gained a word on what "reachable" excludes.

423371a — the comment strip, not asked for

stripComments ran a whole-file /\/\*[\s\S]*?\*\//g over each src/ file. A /* that opens inside a // line comment has no closer of its own, so that regex pairs it with the next */ in the file and deletes every line in between, call sites included. With a // ... the extracted ./*.ts modules ... line at the top of src/lib/mime.ts, a real item.makeEwsRequestAsync(...) below it and an ordinary JSDoc block after that, the guard reported 41/41 with the call unseen.

src/ carries no such opener today, so this was latent rather than live, and it is outside the three findings — flagging it here rather than burying it. The reason I fixed it instead of filing it: it is the same failure the blocking finding was about, in a file this PR introduces, so it would land as a hole in a brand-new guard. Say the word and I will pull it into its own PR.

Stripping block comments per line, the usual remedy, trades that hole for its mirror: the continuation lines of a multi-line block comment survive the strip, so a JSDoc paragraph explaining why we avoid makeEwsRequestAsync gets reported as a call to it — and office-helpers.ts was given exactly that kind of comment in this PR's second commit. So the strip now walks the source left to right: // is consumed to end of line before any /* on that line is looked at, and a /* is followed to its */ however many lines away that is. The :// exemption stays, so a URL in a string does not swallow its line.

Verification

Re-ran against @types/office-js@1.0.601: pnpm test 41/41, pnpm typecheck clean, the exact CI eslint and prettier globs clean, pnpm validate valid, pnpm build with the production origins plus pnpm validate:dist valid (dist/manifest.xml reads ReadWriteItem), and pnpm -r test green across all five workspace packages (344 + 41 + 212 + 20 + 2 files).

E2E Tests, which stalled on its Playwright download in the last round, passed on this head.

The manual sideload step in #254 is still outstanding and still cannot be done from here.

[dobby-coder]

CI note, not caused by this diff: the E2E Tests required context is stuck on its Install Playwright chromium step, 25 minutes into a job whose last successful run took 65 seconds end to end (run 32126615055). The sibling PR #258 hit the same stall four minutes earlier, so it looks like a browser-download stall on the runner rather than anything in either branch. This PR touches no file under apps/website, which is the only thing that suite covers.

Every other check passed, including all the outlook-addon contexts (Lint, typecheck & build, Unit tests, nginx config test (outlook-addon), Baked URLs resolve, Image builds (PR, no push)) and Integration complete.

It needs a re-run once the CDN recovers. I cannot trigger one: the app token has no actions: write.

[dobby-coder]

VERDICT: request-changes

Rules Dobby 2, cycle 1. The permission narrowing itself is sound and I verified it independently — the guard's own broad scan finds zero mailbox-tier members across all 29 files of src/, which is the conservative direction, and ReadWriteItem genuinely covers the six compose writes. The manifest change, the corrected office-helpers.ts comment, the changeset and the CLAUDE.md note are all right, and pinning the tier with a test rather than a bare constant is the right call.

The findings are all in the new guard, and one of them is blocking: the test currently passes on a tree where a real makeEwsRequestAsync call has been added to src/. Details and a verified one-click fix inline.

#	severity	where
1	bug	manifest-permission.test.ts:89 — the >= 5 floor is padded by two names that gate nothing, so the scan can go silently blind
2	nit	manifest-permission.test.ts:104 — \b<name>\b fires on ordinary locals (and the obvious fix is worse — measured)
3	nit	the five-name claim, in the test header, the PR body and the changeset

Findings 1 and 3 are the same root cause: the derived set is seven names, not the five every comment says, and the two extras are exactly what makes the floor unable to fail.

Nothing here is a reason to widen the tier again — the fix is in the guard, not the manifest.

CI is green apart from E2E Tests, which has been in_progress for ~45 minutes on a run whose other 25 jobs finished; worth a glance when the branch is next pushed, but it is not what is blocking this. Left in draft, as requested changes.

[dobby-coder]

All three findings from cycle 1 are addressed in a5100e7, and each thread has a reply and is resolved.

One thing I changed that nobody asked for, so it should not be found by surprise: 423371a replaces the guard's stripComments. It ran a whole-file /\/\*[\s\S]*?\*\//g over each src/ file, and a /* that opens inside a // line comment has no closer of its own, so that regex pairs it with the next */ and deletes every line in between. With a // ... the extracted ./*.ts modules ... line at the top of src/lib/mime.ts, a real item.makeEwsRequestAsync(...) call below it and an ordinary JSDoc block after that, the suite reported 41/41 with the call unseen — the same shape as the blocking finding, one function over.

src/ has no such opener today, so it was latent. I fixed it rather than filing it because the guard is new in this PR and would otherwise land with the hole in it; if you would rather keep this PR to the three findings, I will lift that commit out into its own.

Worth knowing for whoever reviews the shape: stripping block comments per line, which is the usual remedy, trades the hole for its mirror. The continuation lines of a multi-line block comment survive, so the JSDoc-style comment about avoiding makeEwsRequestAsync would be reported as a call to it. The strip now walks the source left to right, which covers both.

Nine mutations, all against @types/office-js@1.0.601:

mutation	want	got
untouched src/ and types	pass	41/41
item.makeEwsRequestAsync(...) in src/	red	red
item["makeEwsRequestAsync"](...)	red	red
const { makeEwsRequestAsync } = item	red	red
/* in a // comment above a real call	red	red (was 41/41)
const hasAttachment = parts.length > 0	pass	41/41
gate named in a comment only	pass	41/41
makeEwsRequestAsync / sendAsync declaration moved out of the parser's reach	red, naming the gate	red, naming the gate
manifest back to ReadWriteMailbox	red	red

Locally green: pnpm test 41/41, pnpm typecheck, the exact CI eslint and prettier globs, pnpm validate, pnpm build with the production origins plus pnpm validate:dist (dist/manifest.xml reads ReadWriteItem), and pnpm -r test across all five packages. E2E Tests, which stalled on its Playwright download last round, passed on a5100e7.

The manual sideload in #254 is still the one acceptance item I cannot do from here.

[dobby-coder]

VERDICT: approve

Rules Dobby 2, cycle 2. All three cycle-1 findings are fixed and I re-verified each one against HEAD rather than taking the delta on trust. No new findings, from the review pass or the rule sweep.

The cycle-1 findings, re-checked at 423371a

#	cycle-1 finding	state at HEAD
1 (bug)	the mailboxOnly.length >= 5 floor was padded by two names that gate nothing, so the derivation could lose makeEwsRequestAsync and sendAsync and stay green	fixed — the floor is gone, replaced by a per-gate assert.ok(mailboxOnly.includes(gate)) over all five names (manifest-permission.test.ts:178). A dropped gate now fails naming itself.
2 (nit)	\b<name>\b fired on ordinary locals like const hasAttachment = ...	fixed — the reachability filter (:155) drops names whose every declaring interface is handed out by another candidate, which removes exactly hasAttachment and itemMode. The broad match is kept, so item["makeEwsRequestAsync"] and destructuring are still caught.
3 (nit)	the five-name claim did not match the seven names the code derived	fixed — the test header, CLAUDE.md, the changeset and the PR body all now name the same five, and that is what the code derives.

What I checked independently

• The tier argument itself. A broad grep of src/ for the five mailbox-tier members plus getCallbackTokenAsync, restUrl, ewsUrl and getSharedPropertiesAsync returns exactly one hit: the new // comment in office-helpers.ts. Prose, not a call. So the narrowing is sound and the corrected comment is correctly not flagged.
• The comment strip. The left-to-right scan is the shape that covers both failure directions — the whole-file regex pair deletes real code after a /* opened inside a // line, and the per-line strip invents a hit off a JSDoc continuation line. The one residual gap is a /* or a bare // inside a string or regex literal, which the scan would still treat as a comment. Grepped for all three shapes across src/: none present, so it is latent and the scan is the right call over a tokenizer.
• The guard's scope. The scan covers src/ only, so I enumerated the producers myself instead of trusting the description. All six webpack entry points are under src/; the only Office.js mentions elsewhere in the package are a comment in webpack.config.js and the test files. src/ is the complete set for shipped add-in code.
• The silent-skip risk in the parser. membersByTier has the if (!name) continue shape that normally makes a source-parsing guard fail open — a declaration it cannot parse is dropped with nothing said. The per-gate assertion is what converts that from a silent skip into a named failure, which is why finding 1 mattered beyond the arithmetic.
• CI, on this HEAD. Green across the board, nothing still in progress. The addon suite reports tests 41 / pass 41 / fail 0 / cancelled 0 — checked for cancelled, not just fail 0 — and both new assertions appear as passing by name. E2E Tests, which was stuck in_progress at cycle 1, passed in 1m10s. The new test is also covered by CI's eslint, prettier and typecheck globs, so it is gated, not just present.

Rule sweep

Selected 17 rules against the changed surfaces — source-text guard construction, source parsing, node:test output, guard scope, PR/prose hygiene, changeset and draft handling. No breaches. Notably clean on the two that most often bite this shape: the comment-strip rule and the "make optional exactly what the source makes optional" rule.

Before merge

The outstanding item is the one the PR names and I cannot do either: the tier is enforced by Outlook per API call, so green CI does not prove the compose and read flows still work. The manual sideload acceptance step in #254 stands — sideload dist/manifest.xml, confirm compose (encrypt and send, including plaintext attachment removal and the postguard.encrypted attachment) and read (decrypt) end to end. The static argument is that every member either flow calls is annotated at or below read/write item, so no call should be denied; that is an argument, not a test on a real host.

Flipping out of draft so it can be reviewed and that step scheduled. No advisory involved, so nothing here is held back.