Skip to content

fix(cli): emails search covers received mail, not just sent - #197

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/db244cd4-search-inbound-blind
Aug 5, 2026
Merged

fix(cli): emails search covers received mail, not just sent#197
andrei-hasna merged 2 commits into
mainfrom
fix/db244cd4-search-inbound-blind

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

The defect

emails search <query> described itself as "Search email by subject, from, or to" and searched the SENT folder only. On one real mailbox that is ~691 sent messages against ~173,000 inbound — a confident, rc=0 zero over 0.4% of the corpus.

emails search "past due"                 rc=0  rows=0
emails inbox list --search "past due"    rc=0  rows=400

Two live investigations were driven off that zero before anyone re-measured.

What makes it worse than an ordinary bug: the obvious positive control passes anyway. A vendor name appears in sent mail too, so searching one returns hits and certifies the instrument while it is pointed at the wrong population. The command also already printed the word sent in its own header (Self-hosted sent search "past due": no messages found.) and that did not stop anyone.

Root cause, and why the fix is shared code

The blindness existed independently in both surfaces, by different routes:

surface route
self-hosted / /v1 selfHostedSentSearchlistMailbox("sent")
local SQLite searchEmails, which enumerates the outbound ledger

Two implementations, one blindness. Both now run a single shared mailboxSearch over the routed MailDataSource, because two copies of a scope rule is exactly how the defect came to exist in two places at once. Per Fix Once, a help-text patch would have been a symptom fix.

What changed

  • mailboxSearch — defaults to inbox + sent, merges newest-first, dedupes by id, and paginates across the merged result rather than one side of it (each folder is asked for the whole offset+limit window, then sliced after the merge; taking limit from each would drop rows that sort into the window from the other side).
  • --folder <name> reaches any single folder, so sent-only search stays available.
  • emails email search keeps the sent-only contract its own namespace declares ("Sent email log, search, and history"). That is the compatible escape hatch for any caller that wanted sent-only.
  • A zero-result now names the folders searched and the folders skipped, rather than leaving the gap implicit.
  • parseCliFolder refuses an unrecognised folder instead of coercing it to inbox at exit 0.

Deliberate axis choice, stated rather than implied

archived, spam and trash are not in the default set — they are the user's own "not my working set" classifications. Because that is a real blind spot, the zero-result path names them outright and points at --folder.

Behaviour changes reviewers should look at

  1. emails search --json output shape changes on the LOCAL store. It previously returned sent-log rows (sent_at, status); it now returns the same SelfHostedEmailSummary shape the /v1 surface already returned (date, kind, is_read, …). This makes the two surfaces consistent, but a local consumer reading sent_at will break. The /v1 surface's shape is unchanged.
  2. The top-level verb's scope widens from sent-only to inbox+sent. Intentional — that is the defect.

Tests

Regression tests assert the literal outcome — a term carried only by inbound mail is returned — never that a banner or marker appears, because a test keyed on wording would pass against the broken build. Every fixture carries a needle that exists on exactly one side of the inbound/outbound line.

Coverage on both surfaces, since a fix proven on one says nothing about the other. The pre-existing /v1 test that asserted "searches outbound mail only, ignoring matching inbound mail" is reversed in place with the reason recorded — the blindness was asserted behaviour, not an oversight, so anyone re-reading that file needs to see the expectation moved on purpose.

Two notes recorded in the test files rather than smoothed away:

  • src/cli/commands/email-log.local.test.ts cannot run on unmodified main — all 10 tests fail in setupDb with SQLiteError: FOREIGN KEY constraint failed (measured at d3ece11, 0 pass, 10 fail). Pre-existing, out of scope here, and the reason the new local coverage is a separate file using the harness from inbox.local.test.ts, which passes.
  • The local fixture initially could not express a sent message (storeInboundEmail derives is_sent from label_ids and silently ignores an is_sent field), so a "received AND sent together" test passed while both rows were inbound. Only the --folder sent assertion could see it. That is the same coverage-bounded-by-axes failure this PR is about, one level down.

Verification

Full suite, unpiped, redirected to a file:

 4293 pass
 156 skip
 0 fail
 20932 expect() calls
Ran 4449 tests across 289 files. [830.17s]
runner exit: 0

Required boundary gate:

bun run no-cloud:source  ->  26 pass, 0 fail, rc=0

Base is unmoved, so the reviewed tree is what would land:

origin/main : d3ece11da414411172c5089704ccf9114a78f4f8
merge-base  : d3ece11da414411172c5089704ccf9114a78f4f8

Adversarial review status — NOT YET OBTAINED

Stated plainly rather than left to look done. Four factory run db244cd4 --review attempts produced no verdict, each blocked before the reviewer executed:

attempt backend / profile blocker
1 codewith, auto 401 refresh_token_invalidated — ChatGPT auth expired
2 claude, auto (account005) profile dir carries account028 with 1 live session attached
3 claude, --profile account030 no profile named "account030" for tool "claude"
4 claude, --profile account006 same, for factory's own auto-pick

Attempts 3 and 4 contradict accounts list, which shows both profiles present for claude. Every run also logged resolve ok — … @ d3ece11d (main), not this branch head — so what the reviewer would have read is unestablished, since it never launched.

This PR should not merge until an independent adversarial review is obtained.

Refs: todos db244cd4


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #197 @ d5401ea — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Candidate reviewed

  • Confirmed HEAD d5401ea against freshly fetched origin/main d3ece11.
  • Read git log --oneline origin/main..HEAD, the diff stat, and the full diff for all five changed files: src/cli/commands/email-log-search.local.test.ts, src/cli/commands/email-log.local.ts, src/cli/commands/email-log.remote.ts, src/cli/commands/email-log.test.ts, and src/lib/mail-types.ts.
  • Read surrounding command registration, summary formatting, numeric/folder parsing, MailDataSource, SQLite mailbox SQL, API-backed mailbox filtering, /v1/messages authentication/filter handling, and the related regression tests.
  • Manually traced the changed trust boundary: CLI query/folder/since/pagination input -> routed mail data source -> parameterized SQLite query or authenticated tenant-scoped /v1/messages reads -> bounded merged summaries. The change is read-only and preserves deny/fail behavior for invalid folders and backend errors.

Commands and gates

  • bun install — exit 0; 324 packages installed. Setup only, not the repository test gate.
  • bun run test — exit 0; 4,293 pass, 156 skip, 0 fail; 20,932 expect() calls; 4,449 tests across 289 files.
  • This repository declares no typecheck script; no typecheck gate was invented or run.

Blocking P0/P1 findings

  • None. No reachable correctness, secret/security, tenant-isolation, data/session-integrity, unsafe-mutation, or required-gate defect was found in this candidate.

Non-blocking follow-ups

  • None.
  • Residual coverage note: the declared hermetic gate skipped 156 environment-gated tests, while the changed local SQLite and API-backed search behaviors are exercised by the passing suite.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #197 @ d5401ea — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Correction to the earlier GO after the merge-time branch-policy check exposed an applicable required gate that was not visible in the package-script run:

Blocking P1 gate failure

  • The required GitHub container-runtime check failed at this head (Actions run 30893167948, job 91939780223), so branch policy correctly refused the merge.
  • Trivy reported two fixed HIGH findings in the shipped runtime image: fast-uri 3.1.4 (fixed in 3.1.5 on this major line) and ip-address 10.2.0 (fixed in 10.3.1).
  • The dependency paths are live in the runtime graph through @modelcontextprotocol/sdk: ajv -> fast-uri and express-rate-limit -> ip-address.

Named remedy

  • Update the existing fast-uri override to 3.1.5 and add an ip-address override at 10.3.1, refresh bun.lock, rerun the declared hermetic test gate, and let the required remote container scan verify the shipped image.

This NO_GO is limited to that named required-gate defect and its direct regressions. The code-path review and bun run test result reported in the prior comment remain otherwise unchanged.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Merge disposition: left open; no branch-protection or release-age control was bypassed.

The requested squash merge command exited 1 because the required container-runtime check is failing. I tested the named dependency remedy, but bun install refused fast-uri@3.1.5 because it is still inside the enforced 604800-second release-age quarantine. The attempted manifest edit was fully reverted; no remediation commit or push was made.

What must happen before this PR can merge:

  • After the fixed packages clear the quarantine, or through a separately reviewed narrow supply-chain exception, update fast-uri to a fixed release and ip-address to 10.3.1, refresh bun.lock, and rerun CI.
  • The required container-runtime check must pass.
  • The same reviewer must then perform a focused re-review limited to those dependency changes and the gate regression, replacing the current-sha NO_GO with a GO at the new head.

`emails search <query>` described itself as "Search email by subject,
from, or to" and searched the SENT folder only. On the fleet mailbox that
is ~691 sent messages against ~173,000 inbound — a confident, rc=0 zero
over 0.4% of the corpus.

    emails search "past due"                 rc=0  rows=0
    emails inbox list --search "past due"    rc=0  rows=400

Two live investigations were driven off that zero before anyone
re-measured. The obvious positive control passes anyway: a vendor name
appears in sent mail too, so searching one returns hits and certifies the
instrument while it is pointed at the wrong population.

The blindness existed independently in BOTH surfaces by different routes
— the self-hosted one through `selfHostedSentSearch`/`listMailbox("sent")`,
the local one through `searchEmails`, which enumerates the outbound
ledger. Both now run a single shared `mailboxSearch` over the routed
MailDataSource, because two copies of a scope rule is how the defect came
to exist in two places at once.

- `mailboxSearch` defaults to inbox + sent, merges newest-first, dedupes,
  and paginates across the merged result rather than one side of it.
- `--folder <name>` reaches any single folder, so sent-only search stays
  available; `emails email search` keeps the sent-only contract its own
  namespace declares.
- A zero-result now NAMES the folders searched and the folders skipped.
  The old header already printed the word "sent" and that did not stop
  two workers reading its zero as "not in the mailbox".
- `parseCliFolder` refuses an unrecognised folder instead of coercing it
  to "inbox" at exit 0.

Regression tests assert the LITERAL outcome — a term carried only by
inbound mail IS returned — never that a banner or marker appears, because
a test keyed on wording would pass against the broken build. Coverage on
both the /v1 surface and the local SQLite surface, since a fix proven on
one says nothing about the other. The pre-existing /v1 test that asserted
"searches outbound mail only, ignoring matching inbound mail" is reversed
in place with the reason recorded: the blindness was asserted behaviour,
not an oversight.

Refs: todos db244cd4

Agent: Herminia
The no-cloud source boundary guard bans /\b(?:saas|fleet)\b|cloud_/i in
every tracked file, and two explanatory comments added by the previous
commit used the banned word while citing the mailbox the defect was
measured on:

    src/cli/commands/email-log.remote.ts: hosted implementation vocabulary
    src/cli/commands/email-log.test.ts:   hosted implementation vocabulary

That is a real regression introduced by 4a20a16 and it failed a required
gate (`bun run no-cloud:source`, part of prepack and prepublishOnly).
The measurement is what carries the argument, not the operator noun, so
the sentences keep the ~691-of-~173,000 figure and lose the word.

    before: 2 fail  (no-cloud-boundary + one unrelated timeout)
    after : 26 pass, 0 fail on `bun run no-cloud:source`

Refs: todos db244cd4

Agent: Herminia
@andrei-hasna
andrei-hasna force-pushed the fix/db244cd4-search-inbound-blind branch from d5401ea to 97729ea Compare August 5, 2026 04:32
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #197 @ 97729ea… — lens: received-mail-coverage-and-regression, reviewer seneca (1 of 1)

Reviewed in my own detached worktree cut from the PR head, never a shared checkout.
gh head, my worktree HEAD, and the sha I tested all agree: 97729ea14c4421d58303302fd5d6d89dfc07b136.

Does it make emails search cover received mail? Yes — and I proved the tests can fail

A passing test proves nothing until it can fail, so I reverted only the three source files to the
merge base and kept the PR's tests:

control: mailboxSearch in email-log.remote.ts -> 0   (source really is pre-fix)
control: db244cd4 marker in email-log.test.ts -> 4   (tests really are the PR's)
RESULT:  rc=1,  22 pass,  11 FAIL

All eleven coverage assertions failed on both surfaces. Restored in the same breath — worktree clean,
33 pass, 0 fail, rc=0. The regression tests genuinely bind the behaviour.

I also wrote my own probe rather than trusting only the author's, against a real file-backed SQLite
store, with a needle existing solely in received mail:

PASS  inbound-only needle is returned          got=["URGENT arrears notice","arrears follow-up"]
PASS  --folder inbox narrows to inbound        got=["URGENT arrears notice"]
PASS  --folder sent narrows to outbound        got=["arrears follow-up"]
PASS  NEGATIVE CONTROL: absent term returns 0  rows=0
FAILURES: 0

The negative control is the load-bearing one: a search that ignored the query and returned everything
would have satisfied every positive assertion.

Merge/pagination is correct for a non-obvious reason I checked rather than assumed — asking each folder
for the whole offset+limit window is only sound if each returns newest-first, and both do
(data.local.ts:410 DESC default; store.ts:2400 and messages.ts:478 ORDER BY … DESC). No import
cycle: the sole email-log.local string in the .remote module is a comment.

Does it break anything? Nothing attributable to this PR

src/cli + src/lib sweep: 1535 pass, 6 fail — all six are mine, not the PR's. Every one is
SyntaxError: Export named 'GetIdentityVerificationAttributesCommand' not found in @aws-sdk/client-ses,
a local module-resolution artefact. All six files are byte-identical between b168dd0 and 97729ea
(control: mail-types.ts correctly reports DIFFERS), and CI's verify passed on this sha.

The earlier NO_GO was correct, is substantive, and is now genuinely resolved

The container-runtime red was a real Trivy finding, not only a stale merge ref. It is resolved because
the versions actually changed — not because the scan stopped reaching them. Four things all hold:

  1. Still scanned. Failing run 30893167948: fast-uri … | 1 |, CVE-2026-18446 | HIGH | 3.1.4;
    ip-address … | 1 |, CVE-2026-69192 | 10.2.0. Passing run 30975420316: same paths, same
    node-pkg type, count 0. 237 node-pkg rows, so the scan is not empty.
  2. Gate unchanged. git diff --stat 024ad847 b168dd0e -- .github/ → no changes (control: same
    command over the whole tree reports 554 insertions). Same action pin, Trivy v0.70.0,
    severity: CRITICAL,HIGH, exit-code: 1.
  3. Gate actually ran, and CI has its own vacuity check. Step 5 Require positive runtime analyzer
    coverage
    (a jq assertion on ArtifactType/OS/lang-pkgs) success, step 10 Reject high or critical
    success. A gate that could have fired and did not.
  4. Versions genuinely changed, from main — not from this PR. d3ece11 pinned fast-uri=3.1.4 with
    no ip-address override; b168dd0 pins 3.1.5/10.3.1. The bump landed in fix(ui): bound the label-summary scan that made emails ui spin at ~92% CPU #198 (024ad84), which
    also added the sanctioned minimumReleaseAgeExcludes = ["fast-uri"] to bunfig.toml — the
    per-package escape hatch, quarantine intact for everything else. Dockerfile:19 is
    bun install --production --frozen-lockfile, so the image gets exactly those versions.

Worth stating precisely: the vulnerable pins were main's, inherited. This PR touches 5 source files
and neither package.json nor bun.lock. The rebase fixed the stale merge ref and pulled in #198's
remediation — two causes, one remedy.

Non-blocking findings

  • P2-1 — the same defect survives on a third surface. src/mcp/tools/email-ops.ts:305 registers
    search_emails, described "Search emails by subject, from address, or to address", and at 315-316
    calls searchEmails — the outbound ledger. Identical blindness, identical misleading description.
    Pre-existing and untouched here, so it does not block. But file it before closing db244cd4, or
    closing the row publishes a false all-clear. The PR's own reasoning about two copies of a scope rule
    applies to the third.
  • P2-2emails search --json shape change on the local store (sent_atdate). Real breaking
    change, already disclosed; wants a changelog line at release.
  • P2-3mailboxSearch lives in .remote while .local imports it; right home is a shared module.
  • P3-1byNewestFirst uses Date.parse(x.date || ""); a NaN comparator gives unspecified order.
    TuiMessage.date is a required string so I did not prove this reachable — latent only.
  • P3-2 — with --folder unread, the "Not searched" list names inbox, though unread ⊆ inbox.

What I did not check

I did not exercise the live /v1 mailbox against the real ~173k-message store (no credentials; the three
known-poisoning EMAILS_* vars were present in my shell and I stripped all three with env -u for every
run). I did not audit runtime packages beyond the two named CVEs, nor scan the published npm tarball. I
did not re-run the full 4449-test suite locally — I relied on CI's verify, green on this sha.

Verdict is mine; the merge decision is not. Not merging.

@andrei-hasna
andrei-hasna merged commit 1ee950e into main Aug 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant