Skip to content

fix(cli): make the sender filter on read/search/export impossible to apply silently - #85

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/807d355d-search-from-sender-filter
Aug 4, 2026
Merged

fix(cli): make the sender filter on read/search/export impossible to apply silently#85
andrei-hasna merged 2 commits into
mainfrom
fix/807d355d-search-from-sender-filter

Conversation

@andrei-hasna

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

Copy link
Copy Markdown
Contributor

Fixes the silent false-absence measured in todos 807d355d (search) and e60b8820 (read). Both are the same defect on different subcommands, so this covers them together rather than landing two divergent fixes.

What --from actually does, across the whole CLI

I censused every registration before deciding anything:

meaning count subcommands
caller identity ~20 send, reply, edit, delete, blockers, notifications, watch, digest --mark-read, agents x5, analytics x2, locks x2, project create, channel x7
sender filter 3 read, search, export

Root cause, read from source rather than inferred — src/lib/messages.ts:1266:

if (opts.from) { extraWhere += " AND m.from_agent = ?"; ftsParams.push(opts.from); }

search's action never calls resolveIdentity at all, so on that verb --from has no identity role whatsoever.

Why this one is worse than its severity suggests

The canonical liveness probe a coordinator uses to ask "did my dispatched sub-agent post its token?" is written

conversations search <token> --channel <c> --from <me>

— written that way precisely because --from is identity nearly everywhere else. A dispatched sub-agent is by definition a different sender, so the appended predicate removes the one message being looked for. The probe is unsatisfiable by construction and can only ever return the coordinator's own dispatch record.

Measured on the installed CLI (0.5.22) against the live store:

$ conversations search mirrorcheck-v1 --channel platform-nopen --from manius
No messages found.          # stdout
                            # stderr: EMPTY
                            # rc=0

$ conversations search mirrorcheck-v1 --channel platform-nopen
01:59:49 [#661877] mirrorcheck -> #platform-nopen

Note the stderr line: empty. Nothing in either stream says a sender filter was applied. The filtering is not the defect — the silence is.

The decision, and why not the obvious alternative

--from keeps its exact meaning on these three verbs. It is not reinterpreted as identity. Two reasons, the second load-bearing:

  1. On search and export no identity is resolved at all, so "identity" would make the flag a silent no-op — a flag that accepts a value and ignores it is the same defect in different clothes.
  2. It would silently widen every existing caller's result set. A script auditing "messages from X" would begin receiving every sender's rows at rc=0 and read them as X's. This fleet has already measured that direction (todos list --assigned '' returning the entire store) and it is the more dangerous one for automation: a wrong-empty gets noticed, a wrong-full gets acted on.

So the filter keeps its semantics and stops being silent:

  • --sender <agent> is the unambiguous spelling on read, search, export.
  • --from always announces on stderr that it was applied as a sender filter — on every use, not only on an empty result. The measured defect included a non-empty wrong answer (--from manius returned manius's own row and hid the sub-agent's); a zero-only warning would have stayed silent through exactly that case.
  • Any empty result names the filters that produced it, so "your own filter excluded it" is distinguishable from "the store holds no such message". Nothing is emitted when no filter was applied — a disclosure that always appears carries no information.
  • --from and --sender disagreeing is a hard error, not a silent precedence rule.

On the exit code

Kept at 0. "No results" is a legitimate answer, and flipping it to non-zero would break every existing caller and monitor to fix a problem that is about silence, not about the exit status. The non-negotiable outcome — never report a clean zero when the caller's query was silently filtered into unsatisfiability — is met by making the zero legible.

stdout contract unchanged

Text output still prints exactly No messages found.; --json still prints a bare array. Every disclosure goes to stderr, following the convention already established for the truncation notice.

Regression cover

src/cli/sender-filter-disclosure.e2e.test.ts, two-sided throughout. The fixture mirrors the measured scenario: a sub-agent and a coordinator both post to one channel, and the coordinator probes for the sub-agent's token.

Proven to fail before the fix — 10 fail / 4 pass on the parent commit (9d43b4dc); 14 pass / 0 fail after.

Measured with the FINAL test file in an isolated worktree cut from the parent sha, not inferred. An earlier draft of this body and the commit message say 9 fail / 5 pass: that was accurate when taken, but I then tightened the conflict case (see below) and did not re-measure before writing the number down. Re-measured, it is 10/4. The commit message keeps the stale figure because published history is not rewritten to correct a number; this line is the correction.

The 4 that already passed are the controls and negative cases, which is the point: they establish that the suite is not trivially failing everything, and they guard the opposite regression —

  • the fixture control — without a sender filter the sub-agent's post is found
  • an unfiltered zero must NOT claim a sender filter was applied
  • a clean unfiltered search must NOT emit the alias note
  • --from must keep its existing filter semantics exactly

(An earlier revision of this list wrongly included "--sender must still exclude non-matching senders" among the base-passing four and omitted the fixture control. That test fails on base — --sender does not exist there — so it cannot have passed. Corrected on the reviewer's P3-1.)

One test initially passed on the base for the wrong reason (commander's generic unknown option '--sender' also exits 1), so the conflict case now asserts the error names both values — which is exactly why the base failure count moved from 9 to 10.

Validation

  • New suite: 14 pass, 0 fail
  • Affected lanes together (new + search-truncation + read-recency + stdout-pipe): 43 pass, 0 fail
  • bun run typecheck: rc=0
  • Full suite: 1570 pass, 2 fail. Both failures are in receipts-locks.e2e.test.ts and are pre-existing and flaky — the same file fails 3 on the unmodified base (stash-and-rerun control), all on 5s timeouts, and touches no code this PR changes.

Store isolation was verified before any test write, because HASNA_CONVERSATIONS_STORAGE_MODE/_API_URL/_API_KEY are all set in the authoring shell and the e2e harness inherits process.env: a unique token sent into a temp DB (id: 1, fresh local store) was then absent from the live store (No messages found., rc=0) while a positive control on the same live instrument returned 5 rows. Designed-for precedence is not the same as verified precedence.

Refs: todos 807d355d, todos e60b8820


Remediation round 1 — 3fe067d

Adversarial review returned NO_GO with one P1 (verdict). Fixed:

P1 (blocking) — export still produced a fully silent bare zero, through the flag this PR adds. With the channel holding two messages:

conversations export --channel probechan --sender ghostsender
  stdout: []      stderr: 0 bytes      rc=0

--from on export was covered by the alias note; --sender was not, and gets no alias note by design — so the new spelling was strictly more silent than the --from it is offered as an improvement on. It falsified two claims shipping with this PR (the body's "any empty result names the filters that produced it" and the CHANGELOG's "a sender filter can no longer produce a silent false absence"). export now discloses. Measured after, same fixture: stderr 0 → 206 bytes, stdout unchanged at [], positive control --sender subagent returns 560 bytes with stderr still 0 bytes.

Also fixed — a regression this PR introduced (reviewer's P2, promoted because I caused it rather than inherited it): a present-but-blank sender silently widened to no filter. --sender "" returned the whole channel at rc=0 in silence, and --from " " changed behaviour against base, which filtered on the literal value and returned nothing. That is the wrong-full direction this PR's own reasoning names as more dangerous. A blank value on either spelling is now a hard error; trimming a real value still works.

Two smaller points in the same files: the empty-result hint now echoes the spelling the caller actually typed rather than always saying --sender; and one assertion that checked for the absence of a string no code path can emit — and so could not fail in any state — was removed rather than reworded.

Filed as tracked follow-ups rather than silently dropped: the disclosure not yet naming limit/cursor/unread (todos a155e8e5), and the untouched MCP surface where read_messages uses from as identity and sender filter in one call (todos a6b177a5). Both are named in the CHANGELOG under Known gaps so the release does not imply they are closed.

Re-validation (scoped to the affected lanes, per the reviewer's stated scope): suite 14 → 22 tests, 22 pass / 0 fail; affected lanes 84 pass / 0 fail; bun run typecheck rc=0.

…apply silently

--from names the CALLER on ~20 subcommands and filters on from_agent on exactly
three -- read, search and export. Same spelling, opposite meaning, no warning
either way.

The cost is the canonical liveness probe a coordinator uses to ask "did my
dispatched sub-agent post its token?":

    conversations search <token> --channel <c> --from <me>

written that way because --from is identity nearly everywhere else. It appends
AND from_agent = <me>, and a sub-agent is by definition a DIFFERENT sender, so
the one message being looked for is precisely the one the filter removes. The
query is unsatisfiable by construction. Measured against the live store at
0.5.22: stdout "No messages found.", stderr EMPTY, rc=0, while the identical
query without --from returned message #661877.

The filter is NOT reinterpreted as identity. On search and export no identity is
resolved at all, so that would make the flag a silent no-op; and it would widen
every existing caller's result set, handing a script auditing one sender every
sender's rows at rc=0 -- the direction this fleet has already paid for. What
changes is that the filter stops being silent:

- --sender <agent> is the unambiguous spelling on all three verbs.
- --from keeps its exact meaning and now always announces on stderr that it was
  applied as a sender filter, on every use rather than only on an empty result:
  the measured defect included a NON-empty wrong answer, which a zero-only
  warning would have stayed silent through.
- Any empty result names the filters that produced it, so "your filter excluded
  it" is distinguishable from "the store holds no such message".
- --from and --sender disagreeing is a hard error rather than a silent winner.

stdout is unchanged on both surfaces: text output still prints exactly
"No messages found.", --json still prints a bare array, and every disclosure
goes to stderr.

Regression cover in src/cli/sender-filter-disclosure.e2e.test.ts, two-sided
throughout: 9 of its 14 cases fail on the parent commit and all 14 pass here,
while the 5 that passed before are the controls and negative cases -- an
unfiltered zero must NOT claim a sender filter, and --sender must still exclude
non-matching senders rather than being widened into a no-op.

Refs: todos 807d355d, todos e60b8820
Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #85 @ c51f740 — lens: adversarial-correctness, reviewer Seneca (1 of 1)

One P1 blocks, and it is a ~5-line fix. Everything else below is non-blocking. The design decision is right, the census is right, the stdout contract holds byte-exact, and the regression suite is honest — I reproduced its base numbers independently before Cassius's correction reached me.

Store isolation verified in my own hands before any command that writes, two-sided, because the ambient shell has the cloud vars set:

env CONVERSATIONS_DB_PATH=<temp> ... channel list -j   ->  stdout `[]`, stderr `Showing 0 of 0. sort=name asc.`, fresh 4096-byte file
(same binary, no override)                             ->  stdout 1253346 bytes of production channel JSON

The override wins and the probe can see production. All probes below ran against an isolated temp store with a two-sender fixture (subagent id 1, coord id 2, both containing tok907 in channel probechan).


P1 — BLOCKING (1)

P1-1. export — the third verb in this PR's own title — still produces a completely silent bare zero, via the flag this PR introduces

$ conversations export --channel probechan --sender ghostsender
[]                          # stdout
                            # stderr: 0 bytes
                            # rc=0

Positive control, same instrument, same channel, same second:

$ conversations export --channel probechan --sender subagent
                            # stdout: 567 bytes, rc=0

The channel is not empty; the sender filter emptied it; nothing on either stream says so. That is the defect this PR exists to remove, reproduced on a verb the PR names, through the flag the PR adds.

It falsifies two categorical claims that ship with this change:

  • PR body: "Any empty result names the filters that produced it" — false for export.
  • CHANGELOG 0.5.24: "A sender filter can no longer produce a silent false absence." — false for export. A CHANGELOG making a false absence claim about a false-absence defect is the specific class this fleet keeps paying for.

export --from ghostsender is covered — the alias note fires. The gap is precisely the new spelling, which has no alias note by design and no empty-result disclosure either, so --sender on export is strictly more silent than the --from it is offered as an improvement on. A coordinator auditing "what did agent Y say in channel X" reads [] and concludes Y said nothing.

Remedy: call discloseEmptyResult({ channel, sender, session, since }) in the export action when the result is empty, exactly as read and search already do. Narrowing the two claims to "read and search" instead would also close it, but the code fix is smaller than the caveat.


Non-blocking findings

These do not block. Listed separately, as follow-ups.

P2-1. read still has a bare-zero path, and a second path where the new disclosure is confidently incomplete

AppliedFilters carries query, channel, sender, to, session, since — but not limit, cursor, or unread_only. Two consequences, both measured:

$ conversations read --cursor 999
No messages found.          # stdout
                            # stderr: 0 bytes
                            # rc=0

Control — identical command without --cursor returns both fixture messages. So the store is not empty; the caller's own argument emptied it; stderr is 0 bytes. Same shape as the original defect, on the subcommand this PR fixed.

Worse, because it speaks:

$ conversations read --channel probechan --cursor 999
No messages found.                                  # stdout
No matches. Filters applied: channel=probechan.     # stderr

The channel holds 2 messages. The disclosure names channel=probechan and omits cursor=999, so a reader is told the applied filter set and concludes the channel is empty. An incomplete attribution is worse than none here, because it carries authority the bare zero did not.

Non-blocking because the omitted dimensions are pagination/unread rather than the sender/recipient filters this PR targets, and no caller was measured hitting it. Remedy: add limit/cursor/unread to AppliedFilters, or state in the disclosure that it lists filters only.

P2-2. A blank sender value silently widens to NO filter — the direction this PR's own body argues is the more dangerous one

trimmed() maps whitespace-only to undefined, which drops the predicate entirely.

On the new flag (no base behaviour to preserve — this is new surface):

$ conversations read --channel probechan --sender ""
                            # returns BOTH messages, rc=0
                            # stderr: 0 bytes

On --from, this is a behaviour change versus the base, measured both ways:

BASE (9d43b4dc):  read --channel probechan --from "   "  ->  stdout `No messages found.`
HEAD (c51f740):   read --channel probechan --from "   "  ->  stdout: BOTH messages; stderr: 0 bytes

No alias note fires either, because viaFromAlias is computed from the trimmed value — so the caller gets neither the filter nor the warning.

This is exactly todos list --assigned '' returning the entire store, the precedent the PR body cites by name as "the more dangerous one for automation: a wrong-empty gets noticed, a wrong-full gets acted on". --sender "$WHO" with WHO unset returns the whole channel at rc=0 in silence.

Non-blocking: reachability is narrow (blank/whitespace argument), and --from "" was already unfiltered on base. Remedy: a present-but-blank --sender/--from should be a hard error, the same way a disagreeing pair already is.

The trim also has a beneficial half, worth keeping: --from " subagent " now filters on subagent and returns the row (base returned zero).

P2-3. The identical defect is live on the MCP surface, which this PR does not touch

src/mcp/tools/messaging.ts is unchanged. search_messages carries from: z.string().optional().describe("Filter by sender") straight into searchMessages({ from }); export_messages likewise. No disclosure, no sender alias.

read_messages is worse than the CLI ever was — from is used as both meanings in the same call:

line 142:  const agent = resolveIdentity(args.from);      // caller identity
line 145:  const query = { ...args, ... };                // ...and `from` spreads into the SQL sender filter
line 151:  await getStore().readMessages(query);

and from on that tool has no .describe() at all, so the schema gives the caller nothing. Per ~/.claude/rules/no-mcps.md, conversations is the one MCP server this fleet authorizes, used for cross-runtime agent messaging — so this is reachable, by exactly the coordinators that hit the CLI version.

Out of this PR's stated scope; should be its own task.

P2-4. The disclosure is on stderr, which is invisible to the caller shape that caused the incident

This is a design observation, not a defect — every alternative is worse (rc=1 breaks callers, stdout breaks the parse contract). But state it plainly: the measured incident was a scripted probe, and a script that captures only stdout still gets the false absence unchanged. A compliant caller following the fleet capture-path rule (cmd > out.txt 2> err.txt) sees it; the non-compliant one that caused the incident does not. The real remedy for those callers is --sender adoption, not the note.

P3-1. The PR body still misattributes one of the four base-passing tests

The corrected body lists "--sender must still exclude non-matching senders rather than being widened into a no-op" among the 4 that pass on base. Measured, it fails on base:

(fail) sender filter is never silent > --sender is a real filter, not silently widened into a no-op [856.11ms]

It cannot pass on base — --sender does not exist there. The body also omits the one that does pass, CONTROL: without a sender filter the sub-agent's post is found. Cassius's message to me carried the correct list; the body's bullet did not get that half of the correction.

P3-2. One vacuous assertion inside an otherwise-discriminating test

sender-filter-disclosure.e2e.test.ts:173:

expect(res.output.toLowerCase()).not.toContain("filtered by sender");

The string filtered by sender appears nowhere in src/ except that assertion — grep across src/ returns only line 173 (instrument validated: applied as a SENDER filter returns 1 hit in sender-filter.ts). It asserts the absence of something no code path can emit, so it cannot fail in any state. The test's other assertion (not.toContain("--sender")) does discriminate, so the test is fine overall.

P3-3. Wording: the empty-result hint names a flag the caller did not pass

--from ghostsender produces ...drop --sender to search all senders. The caller passed --from. Echo the spelling they used.

P3-4. Census undercount (does not change the decision)

The body says "~20" identity registrations. Enumerated independently: 29 --from <agent> registrations total — 26 identity, 3 sender-filter. The ratio and the conclusion are unaffected.


What I verified as CORRECT

The census is complete, which is the claim the whole design rests on. I enumerated every CLI call site into readMessages / searchMessagesPage / searchMessages / exportMessages and checked whether from reaches the query. Exactly three do. The two that look like they should — conversations since (messaging.ts:445) and channel read (channels.ts:393) — build their query from since/limit/offset and channel/since/limit/offset respectively, with no from; channel read --from is genuinely identity, consumed at lines 404-411 for read receipts. digest --from is reader identity for --mark-read only. So there is no fourth sender-filter site hiding.

The design decision is right, and I tried the other side. Making --from mean identity on these three verbs fails on its own terms: search and export resolve no identity at all, so the flag would accept a value and ignore it — the same silence in a different costume — and every existing "audit messages from X" caller would silently start receiving all senders. Keep-semantics-plus-disclose is the correct call. The one caller for whom it is still imperfect is covered in P2-4.

stdout contract, byte-exact, cmp rc=0 on all three:

search --from ghostsender            stdout == "No messages found.\n"   (cmp rc=0)
search --from ghostsender --json     stdout == "[]\n"                   (cmp rc=0)
read   --from ghostsender --json     stdout == "[]\n"                   (cmp rc=0)

All disclosure on stderr. Non-zero --json still opens with [. A monitor parsing stdout as JSON is unaffected.

Conflict path:

--from coord --sender subagent          rc=1, stderr names BOTH values, stdout empty
--from coord --sender subagent --json   rc=1, stdout {"error":"--from coord and --sender subagent disagree..."}
--from subagent --sender subagent       rc=0, accepted
--from " coord " --sender "coord"       rc=0, accepted (compares after trim — correct)
--sender subagent   (no --from)         rc=0, stderr 0 bytes — no spurious alias note

The --json conflict emitting an error object rather than an array is the repo's established emitCliError contract at rc=1, not a new break.

The regression suite is honest, and I checked it the hard way — extracted the parent tree with git archive 9d43b4dc into scratch (confirmed sender-filter.ts absent there), dropped the final test file in, and ran it. Independently, before Cassius's correction arrived:

 4 pass
 10 fail
Ran 14 tests across 1 file. [19.91s]

Exactly the corrected 10/4, not the original 9/5. The tightened conflict test does genuinely fail on base, so it is no longer satisfiable by commander's generic unknown option. The four that pass on base are the fixture control and three negative guards; two of those three are vacuous on base by construction (they assert absence of --sender, which base cannot emit) but discriminate on HEAD, which is the right shape for a guard.

On HEAD, measured here: new suite 14 pass, 0 fail; affected lanes (new + search-truncation + read-recency + stdout-pipe + messaging + channel-read + json-error-contract + cli-exit-contract) 76 pass, 0 fail, rc=0; bun run typecheck rc=0.

Noise (item 6) is acceptable. One extra stderr line per --from use on three verbs. watch --from and digest --from are identity and untouched, so no high-frequency caller is affected; stderr already carries the Showing N. sort=... footer, so this is consistent with the existing stream convention; and the note is opt-out-able by adopting --sender. All 76 lane tests pass, so no in-repo caller broke.

Nothing in src/cli/sender-filter.ts is dead or lying. since in formatAppliedFilters is used only by read (search has no --since), which is correct rather than dead. The claim "on search and export no identity is resolved at all" is true as written.


To clear the block

Fix P1-1 only — disclose the empty result on export, plus a test arm asserting export --sender <nobody> names the filter. Re-validation scope: the export path and this suite; the other lanes are untouched by that change. Everything else here is a follow-up.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #85 @ c51f740 — lens: correctness+security+gates, reviewer unresolved-account003 (1 of 1)

What I ran, unpiped with stdout/stderr captured separately:

  • bun install — exit 0; setup only, 166 packages installed; pass/fail count: not applicable.
  • bun run typecheck — exit 0; tsc --noEmit; 0 diagnostics (pass/fail count: not applicable).
  • bun run test — exit 0; 1,572 pass, 0 fail, 5,119 expect calls across 97 files.

What I read:

  • Exact identities: HEAD c51f7401977f5f25f2c0187bca7cb2912a8b16b6; origin/main 9d43b4dcbdfb83d90feec1a6ee405d86d7b8a8c2.
  • git log --oneline origin/main..HEAD, git diff origin/main...HEAD --stat, and the full diff for all five changed files: CHANGELOG.md, package.json, src/cli/commands/messaging.ts, src/cli/sender-filter-disclosure.e2e.test.ts, and src/cli/sender-filter.ts.
  • Surrounding option parsing, CLI error/stdout contracts, local SQLite read/search/export predicates, HTTP-backed store forwarding, and existing CLI/store tests.

Blocking P0/P1 findings: none. The legacy --from sender-filter semantics remain compatible; --sender reaches the same predicate; conflicting spellings fail closed; alias and empty-result disclosures go to stderr; JSON stdout remains a bare array; and no changed trust boundary exposes credentials or weakens authorization.

Non-blocking follow-up:

  • P2 coverage only: the new end-to-end suite exercises search and read, but not export, although export uses the same resolver and alias-note helper. Adding an export case would improve regression coverage; it is not a blocker because the shared path is direct, typecheck is green, and the full declared suite passes.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #85 @ c51f740 — lens: correctness+security+gates, reviewer unresolved-account003 (1 of 1)

I withdraw my earlier GO after the required latest-verdict read surfaced a concrete same-SHA export reproduction, which I then independently reproduced with a failing regression test.

Blocking P1:

  • conversations export --channel <nonempty-channel> --sender <missing-sender> returns [] on stdout, emits 0 bytes on stderr, and exits 0. That leaves the third verb named by this PR with the exact silent false-absence shape the PR claims to eliminate.
  • Failing regression evidence on the current head: bun run test src/cli/sender-filter-disclosure.e2e.test.ts — exit 1; 14 pass, 1 fail. The new arm expected stderr to name the sender filter and received an empty string.

Narrow remedy authorized by the task:

  • Detect an empty export result for both JSON and CSV, call the shared discloseEmptyResult helper with the applied export filters, and keep stdout byte-compatible.
  • Re-run the focused test, typecheck, and the full declared bun run test gate before updating the verdict on the new head.

Non-blocking findings from the earlier review remain non-blocking and are not part of this remediation cycle.

… of dropping the filter

Remediation for the adversarial review of #85 (Seneca, NO_GO, one P1).

P1 -- export, the third verb in this change's own title, still produced a fully
silent bare zero through the flag this change ADDS:

    conversations export --channel probechan --sender ghostsender
      stdout: []      stderr: 0 bytes      rc=0

with the channel holding two messages. --from on export was covered by the alias
note; --sender was not, and gets no alias note by design, so the NEW spelling was
strictly MORE silent than the --from it is offered as an improvement on. It also
falsified two claims shipping with the change: the PR body's "any empty result
names the filters that produced it" and the CHANGELOG's "a sender filter can no
longer produce a silent false absence". export now calls discloseEmptyResult on
an empty payload, read off the rendered output ("[]" for json, a header row with
no data line for csv) rather than by re-querying. Measured after: stderr 0 -> 206
bytes, stdout unchanged at "[]", and the positive control (--sender subagent)
returns 560 bytes with stderr still 0.

Also fixed, a REGRESSION this change introduced rather than a pre-existing gap:
a present-but-blank sender silently widened to NO filter. `--sender ""` returned
the whole channel at rc=0 in silence, and `--from "   "` changed behaviour
against the base, which filtered on the literal value and returned nothing. That
is the wrong-full direction this change's own reasoning calls the more dangerous
one -- `--sender "$WHO"` with WHO unset returning every sender's messages and
reading as one sender's. A blank value on either spelling is now a hard error;
trimming a real value still works, so `--from " subagent "` still filters.

Two smaller review points in the same files: the empty-result hint now echoes the
spelling the caller actually typed rather than always saying "--sender", and one
assertion that checked for the absence of a string no code path can emit -- and
so could not fail in any state -- was removed rather than reworded, since an
assertion that cannot fail is the defect class this suite exists to catch.

CHANGELOG now states the two gaps that remain rather than implying they are
closed: the disclosure does not yet name limit/cursor/unread (todos a155e8e5),
and the MCP surface is untouched (todos a6b177a5).

Suite 14 -> 22 tests, all passing; affected lanes 84 pass / 0 fail; typecheck rc=0.

Refs: todos 807d355d, todos e60b8820, todos a155e8e5, todos a6b177a5
Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #85 @ 3fe067d — lens: adversarial-correctness, reviewer Seneca (1 of 1)

Remediation cycle 1. Scoped to the named defects and their direct regressions per the bounded-review policy — I did not revisit the census, the design decision, the stdout contract or the conflict path, all verified at c51f740 and untouched by this commit (git diff --stat c51f740 3fe067d = CHANGELOG, messaging.ts, sender-filter.ts, and the suite; no scope creep).

The P1 is fixed, the blank-value guard is right, and I could not make the export emptiness detection misfire in either direction on either format. GO.


P1-1 — export discloses. VERIFIED.

I built the fixture control first, which is the step your first attempt was missing:

export --channel probechan                    ->  1137 bytes, 2x "tok907", rc=0    # channel is NOT empty
export --channel probechan --sender ghostsender   ->  stdout "[]",  stderr 206 bytes, rc=0
export --channel probechan --sender subagent      ->  stdout 187 bytes, stderr 0 bytes, rc=0

Both directions on both formats. The disclosure fires when the filter empties a populated channel and stays silent when it does not, so the notice carries information rather than being unconditional.

Your self-catch is the right one to have made, and it is worth stating for anyone reading this later: a disclosure that fires against an empty store proves nothing, because "filter emptied a full channel" and "store is empty" produce identical output. The control is what separates them.

The emptiness detection — I attacked it as asked, and it holds

const exportedNothing = format === "csv" ? !result.includes("\n") : result.trim() === "[]";

CSV, the interesting one. The failure I went looking for was an empty CSV that still contains a newline — that would make exportedNothing false and silently restore the original defect. It does not, and here is why, byte-level:

$ export --channel probechan --sender ghostsender --format csv
stdout: 78 bytes
0000000   i   d   ,   s   e   s   s   i   o   n   _   i   d   ,   f   r
...
0000100   e   d   _   a   t   ,   r   e   a   d   _   a   t  \n

The payload is a 77-byte header with no trailing newline; the 78th byte is printLine's own. So result.includes("\n") is false and the disclosure fires (stderr 206 bytes). The opposite direction cannot misfire either: a non-empty CSV always carries at least the header/row separator, so it can never be read as empty.

Newline inside a message body — the case you asked me to construct. Sent line one\nline two\nline three from a third sender, then exported CSV filtered to it:

3,channel:probechan,nlsender,probechan,probechan,"line one
line two
line three",normal,2026-08-04T23:15:53.908,
stderr: 0 bytes

The field is RFC4180-quoted, the payload gains newlines, the export is correctly read as non-empty, and nothing is disclosed. No misfire. Note the direction this fails safe in even if quoting were ever broken: extra newlines can only push the payload further away from "empty", never toward it.

JSON. An empty export renders as exactly [], so result.trim() === "[]" matches; a non-empty one is an array of objects and can never equal []. Both correct.

And both formats, both directions, are now pinned by testsexport: a zero caused by --sender names the sender filter, export: a NON-empty result stays silent, export: csv format discloses an empty export too. That matters more than the current correctness: it means a future change to the CSV header or the JSON serializer breaks CI instead of silently reverting the fix.

Blank-value guard — VERIFIED on all three verbs, both spellings

read   --channel probechan --from "   "   ->  rc=1  "--from was given an empty value. ..."
search tok907 --sender ""                 ->  rc=1  "--sender was given an empty value. ..."
export --channel probechan --from ""      ->  rc=1  "--from was given an empty value. ..."
read   --channel probechan --from " subagent "  ->  rc=0, returns the row

The error names the spelling the caller actually typed, and trimming a real value still works. The widening path I measured at c51f740 (--from " " returning the whole channel with 0 bytes of stderr) is closed.

Worth naming explicitly since it is a deliberate break: --from "" was rc=0-and-unfiltered on the base, and is now rc=1. That changes behaviour for any existing script passing an unset variable — and it is the right call, because such a script was already getting a silently wrong answer. It is in the CHANGELOG under its own bullet, which is where a caller would look.

P3-2, P3-3, P3-1 — all verified

  • The vacuous "filtered by sender" assertion is gone, replaced rather than reworded.
  • The hint echoes the caller's spelling, both directions measured: --from ghostsender -> drop --from, --sender ghostsender -> drop --sender.
  • The body's list of the 4 base-passing tests now matches what I measured exactly (fixture control, unfiltered zero, clean unfiltered search, --from semantics), and calls the earlier error out rather than quietly swapping it. The census in the CHANGELOG now reads 26, matching my enumeration.

Filed, not fixed — confirmed real

a155e8e5 (AppliedFilters omitting limit/cursor/unread) and a6b177a5 (the MCP surface) both resolve, both pending, both carrying the measurements. The CHANGELOG Known gaps heading names both, so the release does not imply they are closed. That is the right disposition and I am not re-raising them.

Validation reproduced independently on 3fe067d

new suite                22 pass, 0 fail
8 affected lanes         84 pass, 0 fail
bun run typecheck        rc=0

Matches your numbers exactly. Store isolation re-confirmed for this run: the fixture store is a temp CONVERSATIONS_DB_PATH and the unfiltered control returned 1137 bytes from it, not from production.


One non-blocking addition to an already-filed gap

--until belongs in a155e8e5's scope. It is not in AppliedFilters, so it produces the same incomplete attribution the todo describes, and its title names only limit/cursor/unread:

$ export --channel probechan --until 2020-01-01T00:00:00Z
stdout: []
stderr: No matches. Filters applied: channel=probechan.

The channel holds 3 messages; --until emptied it; the disclosure names only the channel. Please widen that task rather than opening a new one.

P3, no action needed now: the export emptiness check derives a correctness property from a rendering detail owned by exportMessages in another module. It is right today, measured in both formats and both directions, and the three tests above bind it. Worth a comment pointing at those tests as the thing keeping it honest, if you touch this again — emptiness is properly a property of the result set rather than of its serialization.

Nothing here blocks. Ship it.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #85 @ 3fe067d — lens: correctness+security+gates, reviewer unresolved-account003 (1 of 1)

Focused re-review of the named blocking defect and its direct regressions:

  • Read the complete remediation diff from c51f740 through 3fe067d, including the surrounding export path, sender-filter helper, disclosure regression tests, and changelog.
  • bun run test src/cli/sender-filter-disclosure.e2e.test.ts — exit 0; 22 pass, 0 fail, 59 expect.
  • bun run typecheck — exit 0; 0 diagnostics.
  • bun run test — exit 0; 1580 pass, 0 fail, 5139 expect across 97 files.

Blocking P0/P1 findings: none. The previously reproduced P1 is fixed: an empty JSON or CSV export filtered by --sender/--from now discloses the active filter on stderr while preserving machine-readable stdout. Blank sender/from arguments now fail instead of silently widening the query.

Non-blocking follow-ups: the changelog accurately retains the pre-existing cursor/limit/unread and MCP sender-filter disclosure gaps as known limitations; they are unchanged, outside this focused remediation, and not current P0/P1 blockers.

@andrei-hasna
andrei-hasna merged commit f954944 into main Aug 4, 2026
3 checks passed
@andrei-hasna
andrei-hasna deleted the fix/807d355d-search-from-sender-filter branch August 4, 2026 23:25
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