fix(graph): preserve auto-quoted thread history in update_draft#71
Merged
stevenobiajulu merged 2 commits intomainfrom May 3, 2026
Merged
Conversation
update_draft on a reply draft was PATCHing the body wholesale via buildGraphBody, which dropped the auto-quoted thread (divider + From:/Sent:/To:/Subject: + prior message) that prepareReplyDraft had merged in. The recipient saw only the new caller content with no reply chain below. When the body field is changing, updateDraft now GETs the current draft body and detects Graph's quoted-thread markers (divRplyFwdMsg or <hr><div><b>From:</b>). If detected, the new caller fragment replaces only the content between <body> and the first <hr>, preserving the divider and everything after it. If no quoted thread is detected (fresh drafts created without reply_to), behavior is unchanged: PATCH the body wholesale via buildGraphBody. Two new helpers — hasQuotedReplyThread and replaceCallerFragmentPreservingQuote — sit alongside the existing mergeQuotedReplyHtml and reuse the same stripHtmlBodyWrappers / wrapPlainTextAsHtml / truncateBody helpers used by prepareReplyDraft. Closes #70
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Peer review (Codex + Gemini) flagged a real consistency bug and surfaced
DRY-ness improvements:
- hasQuotedReplyThread returned true on `divRplyFwdMsg` alone, but
replaceCallerFragmentPreservingQuote required `<hr>` to splice. If
Graph emitted the id without that divider, updateDraft would prepend
the new fragment without removing the old one (silent duplicate).
- Three helpers (mergeQuotedReplyHtml, hasQuotedReplyThread,
replaceCallerFragmentPreservingQuote) all parsed the same <body>/<hr>
anatomy independently — easy for them to drift.
Consolidate around a single anatomy parser, findGraphQuotedReplyRegion,
which returns { bodyOpenEnd, dividerStart } | null. Both create-path
(mergeQuotedReplyHtml inserts at bodyOpenEnd) and update-path (updateDraft
replaces bodyOpenEnd..dividerStart) consume the same anchors, so detection
and splicing agree by construction. The English `<b>From:</b>` fallback
regex (untested, speculative i18n) is gone — `<hr>` presence is the only
signal we currently exercise.
Also narrows updateDraft's GET to ?$select=body — only the body field is
needed for splice detection.
Tests now derive the realistic post-prepareReplyDraft fixture from
QUOTED_REPLY_BODY using the same insertion logic the production code uses,
so the round-trip stays in sync if either side changes. The redundant
bodyHtml-wrapper test was dropped (already covered by the existing
"Caller-supplied full HTML document has wrappers stripped" test which
shares the same stripHtmlBodyWrappers helper).
Net: -97 / +57 lines, 133/133 tests pass, 188/188 scenarios covered.
Member
Author
|
Pushed Real bug caught: Previous version had Fix: Consolidate the three helpers around a single anatomy parser Other improvements:
Net: -97 / +57, still 188/188 spec coverage, 133/133 tests pass. |
stevenobiajulu
added a commit
that referenced
this pull request
May 3, 2026
End-to-end smoke testing surfaced a critical pre-existing bug and Gemini peer review flagged three smaller cleanups in the new `call` subcommand. **Critical: bin discarded runCli's exit code (regression now pinned)** `packages/email-agent-mcp/bin/email-agent-mcp.js` previously called `runCli()` and only handled thrown errors with `process.exit(1)`. Non-zero return values (e.g., `2` from invalid args, `2` from unknown command) were silently discarded — `email-agent-mcp call no_such_tool` returned `0` to the shell despite printing an error. Affected ALL subcommands; just first visible with `call`'s scriptable surface. Fix: route the bin through the existing `runCliDirect` helper, which sets `process.exitCode` (not `process.exit()`) so `serve` stays alive for the MCP stdio handshake while one-shot commands propagate their codes. Export `runCliDirect` from `@usejunior/email-mcp/index.ts`. Added a regression test under `cli/Direct entrypoint lifecycle` that pins the propagation in place — `runCliDirect(['bogus-command'])` must set `process.exitCode = 2`. **runCall improvements (per Gemini review)** 1. Skip `WatchedAllowlist` entirely for one-shot use — call the loader directly via `loadSendAllowlist(path)`. Avoids spinning up an FS watcher we immediately tear down. 2. Drop the redundant `await waitForInit(state)` after `await ensureProvider(state)` — `ensureProvider` already awaits init internally. 3. Catch `z.ZodError` explicitly in the executeTool catch block instead of regex-matching on error message text. More robust to Zod error format changes. Net: 178/178 tests pass, 194/194 spec coverage, lint clean, end-to-end smoke test against real Microsoft Graph confirms exit codes 0/2/3 now reach the shell correctly and the new `call` surface integrates with the PR #71 update_draft fix (a `call create_draft` → `call update_draft` → `call read_email` flow preserves Graph's auto-quoted thread).
9 tasks
stevenobiajulu
added a commit
that referenced
this pull request
May 3, 2026
* feat(cli): add `call` subcommand for one-shot tool invocation
The MCP server is a long-lived stdio process: source-code changes don't
take effect until the parent harness (Claude Code, Cursor, etc.) restarts.
Add a CLI subcommand that runs the same actions in a fresh process per
invocation, sidestepping the restart cycle and making tools scriptable
from cron, launchd, hooks, and shell pipelines.
Surface:
email-agent-mcp call <tool> --args '<json>'
email-agent-mcp call <tool> --args-file <path>
email-agent-mcp call <tool> --args-stdin
email-agent-mcp call --list # enumerate tools
email-agent-mcp call <tool> --schema # print input JSON Schema
Architecture: extract `executeTool()` as the shared dispatch primitive.
`handleToolCall()` (MCP transport) becomes a thin wrapper that calls
`executeTool()` then formats the result into the MCP `content` envelope
(preserving the `download_attachment` resource special-case). `runCall()`
in the CLI calls `executeTool()` then writes raw JSON to stdout. This
keeps `serve` and `call` parity by construction — they share the action
registry and the dispatch path, only the output formatting diverges.
Differences vs. `serve`:
- Eager provider init (no demo-mode fallback) — auth failures surface as
exit codes instead of masquerading as `connecting` results
- Snapshot allowlist (no WatchedAllowlist FS watcher in a one-shot process)
Output:
- stdout = JSON tool result (pretty for TTY, compact for pipe via
`process.stdout.isTTY`)
- stderr = logs / errors only, so `call ... | jq` works
- Exit codes: 0 success, 2 CLI/argument/schema/unknown-tool error,
3 typed tool failure ({success:false,...}) or runtime throw
Out of scope (per peer review with Codex + Gemini):
- Schema-generated per-tool flags (heterogeneous tool inputs, low ROI vs.
the --args JSON path)
- JSON-RPC over stdin as a public surface (confusing UX, requires MCP
envelope knowledge — keep as internal debug if useful later)
- Cross-process token-refresh locking (existing single-flight is
process-local; mostly fine for read-path `call` invocations, low risk)
Closes #72.
Test coverage:
- 5 new openspec scenarios under cli/Call Subcommand + cli/Exit Codes
- Unit: parseCliArgs handles call/--args/--args-file/--args-stdin/--list/--schema
- Unit: executeTool returns raw action result; handleToolCall wraps it
- Unit: getActionInputJsonSchema exposes input schema
- Unit: runCall exits with 2 on unknown tool, malformed JSON, missing tool name
- Unit: runCall exits with 3 on typed tool failure (vi.doMock stub)
- Unit: formatJsonForOutput pretty-vs-compact based on isTty
- 177/177 tests pass, 194/194 spec coverage
- Smoke-tested live against real Microsoft Graph API
* fix(cli): propagate non-zero exit codes from runCli; tighten runCall
End-to-end smoke testing surfaced a critical pre-existing bug and Gemini
peer review flagged three smaller cleanups in the new `call` subcommand.
**Critical: bin discarded runCli's exit code (regression now pinned)**
`packages/email-agent-mcp/bin/email-agent-mcp.js` previously called
`runCli()` and only handled thrown errors with `process.exit(1)`. Non-zero
return values (e.g., `2` from invalid args, `2` from unknown command) were
silently discarded — `email-agent-mcp call no_such_tool` returned `0` to
the shell despite printing an error. Affected ALL subcommands; just first
visible with `call`'s scriptable surface.
Fix: route the bin through the existing `runCliDirect` helper, which sets
`process.exitCode` (not `process.exit()`) so `serve` stays alive for the
MCP stdio handshake while one-shot commands propagate their codes. Export
`runCliDirect` from `@usejunior/email-mcp/index.ts`. Added a regression
test under `cli/Direct entrypoint lifecycle` that pins the propagation in
place — `runCliDirect(['bogus-command'])` must set `process.exitCode = 2`.
**runCall improvements (per Gemini review)**
1. Skip `WatchedAllowlist` entirely for one-shot use — call the loader
directly via `loadSendAllowlist(path)`. Avoids spinning up an FS watcher
we immediately tear down.
2. Drop the redundant `await waitForInit(state)` after `await ensureProvider(state)`
— `ensureProvider` already awaits init internally.
3. Catch `z.ZodError` explicitly in the executeTool catch block instead of
regex-matching on error message text. More robust to Zod error format
changes.
Net: 178/178 tests pass, 194/194 spec coverage, lint clean, end-to-end
smoke test against real Microsoft Graph confirms exit codes 0/2/3 now
reach the shell correctly and the new `call` surface integrates with the
PR #71 update_draft fix (a `call create_draft` → `call update_draft` →
`call read_email` flow preserves Graph's auto-quoted thread).
* fix(cli): forward --mailbox to tool input; bypass eager init for get_mailbox_status
Codex peer review caught two correctness bugs in the new `call` subcommand
that the previous round of testing missed.
**Bug 1 (medium): --mailbox silently ignored**
`call` parsed `--mailbox` (the same flag `serve`/`watch` use) but never
forwarded it to the tool. Mailbox-sensitive tools route via
`resolveMailboxContext(state, input.mailbox)`, which reads from the parsed
tool input — not from CLI opts. Result: `email-agent-mcp call delete_email
--mailbox personal --args '{...}'` would silently target the default
mailbox. Real risk for write/delete operations.
Fix: merge `opts.mailbox` into `args.mailbox` when args don't already
specify one. In-args value still wins so explicit input is never
overridden by an ambient flag.
**Bug 2 (medium): eager init blocked the diagnostic tool**
`get_mailbox_status` is intentionally non-blocking — it inspects
`state.status` (pending/connecting/not_configured/error) and reports the
state as its result, by design. The eager-init gate I added in `runCall`
short-circuited it with exit 3 exactly when it would be most useful (e.g.,
checking why the provider is broken).
Fix: skip ensureProvider when the tool name is `get_mailbox_status`. This
is the only known non-blocking tool; if more are added, this can grow into
an annotation lookup.
Tests: 3 new scenarios under cli/Call Subcommand Mailbox Routing and
cli/Call Subcommand Diagnostic Tools. 181/181 tests pass, 196/196 spec
coverage. Verified live against real Microsoft Graph:
- `call get_mailbox_status` returns the diagnostic state without exit 3
- `call list_emails --mailbox bogus-name --args '{...}'` exits 3 with
"Mailbox not configured" (instead of silently using the default)
Merged
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
1 participant
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
update_drafton a reply draft was PATCHing the body wholesale viabuildGraphBody, which dropped the auto-quoted thread thatprepareReplyDraft(fix fix(graph): preserve auto-quoted thread history in reply_to_email #52) had merged in.updateDraftnow GETs the current draft body, detects the quoted thread (divRplyFwdMsgor<hr>followed by<b>From:</b>), and splices the new caller fragment in above the divider — preserving the divider, header block, and prior message body.buildGraphBodybehavior on fresh drafts (no quoted-thread marker), so non-reply drafts are unaffected.Closes #70.
Test plan
email-graph-provider.test.ts:update_draft preserves Graph auto-quoted thread— old caller content gone, new content inserted, divider + header + prior body preservedupdate_draft on a fresh draft replaces body wholesale— no quoted-thread marker → unchanged behavior, falls through tobuildGraphBodyupdate_draft with bodyHtml preserves quoted thread and strips outer wrappers— exactly one<html>and<body>in the merged contentDraft-Then-Send via createReplyAllnpm run test:run --workspace packages/provider-microsoft— 134/134 passnpm run lint --workspace packages/provider-microsoft— cleannpm run check:spec-coverage— 188/188 scenarios covered