fix(cli): emails search covers received mail, not just sent - #197
Conversation
|
[REVIEW] GO — #197 @ d5401ea — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1) Candidate reviewed
Commands and gates
Blocking P0/P1 findings
Non-blocking follow-ups
|
|
[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
Named remedy
This NO_GO is limited to that named required-gate defect and its direct regressions. The code-path review and |
|
Merge disposition: left open; no branch-protection or release-age control was bypassed. The requested squash merge command exited 1 because the required What must happen before this PR can merge:
|
`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
d5401ea to
97729ea
Compare
|
[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. Does it make
|
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=0zero over 0.4% of the corpus.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
sentin 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:
/v1selfHostedSentSearch→listMailbox("sent")searchEmails, which enumerates the outbound ledgerTwo implementations, one blindness. Both now run a single shared
mailboxSearchover the routedMailDataSource, 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 toinbox + 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 wholeoffset+limitwindow, then sliced after the merge; takinglimitfrom 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 searchkeeps 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.parseCliFolderrefuses an unrecognised folder instead of coercing it toinboxat exit 0.Deliberate axis choice, stated rather than implied
archived,spamandtrashare 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
emails search --jsonoutput shape changes on the LOCAL store. It previously returned sent-log rows (sent_at,status); it now returns the sameSelfHostedEmailSummaryshape the/v1surface already returned (date,kind,is_read, …). This makes the two surfaces consistent, but a local consumer readingsent_atwill break. The/v1surface's shape is unchanged.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
/v1test 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.tscannot run on unmodified main — all 10 tests fail insetupDbwithSQLiteError: FOREIGN KEY constraint failed(measured atd3ece11,0 pass, 10 fail). Pre-existing, out of scope here, and the reason the new local coverage is a separate file using the harness frominbox.local.test.ts, which passes.storeInboundEmailderivesis_sentfromlabel_idsand silently ignores anis_sentfield), so a "received AND sent together" test passed while both rows were inbound. Only the--folder sentassertion 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:
Required boundary gate:
Base is unmoved, so the reviewed tree is what would land:
Adversarial review status — NOT YET OBTAINED
Stated plainly rather than left to look done. Four
factory run db244cd4 --reviewattempts produced no verdict, each blocked before the reviewer executed:401 refresh_token_invalidated— ChatGPT auth expiredaccount005)account028with 1 live session attached--profile account030no profile named "account030" for tool "claude"--profile account006Attempts 3 and 4 contradict
accounts list, which shows both profiles present forclaude. Every run also loggedresolve 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
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.