From c51f7401977f5f25f2c0187bca7cb2912a8b16b6 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Wed, 5 Aug 2026 01:41:55 +0300 Subject: [PATCH 1/2] fix(cli): make the sender filter on read/search/export impossible to 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 --channel --from written that way because --from is identity nearly everywhere else. It appends AND from_agent = , 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 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 --- CHANGELOG.md | 8 +- package.json | 2 +- src/cli/commands/messaging.ts | 60 ++++- src/cli/sender-filter-disclosure.e2e.test.ts | 249 +++++++++++++++++++ src/cli/sender-filter.ts | 171 +++++++++++++ 5 files changed, 481 insertions(+), 9 deletions(-) create mode 100644 src/cli/sender-filter-disclosure.e2e.test.ts create mode 100644 src/cli/sender-filter.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 21fcfbd..f21f696 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,13 @@ All notable changes to this project will be documented in this file. ## Unreleased -## 0.5.23 - 2026-08-02 +## 0.5.24 - 2026-08-05 + +### Added +- **`--sender` is the unambiguous spelling of the sender filter on `read`, `search`, and `export`.** `--from` keeps working and keeps its exact meaning on those verbs, so no existing caller's result set changes; the two disagreeing is a hard error rather than a silent precedence rule. + +### Fixed +- **A sender filter can no longer produce a silent false absence.** `--from` names the CALLER on ~20 subcommands and filters on `from_agent` on three (`read`, `search`, `export`), so the canonical liveness probe `search --channel --from ` appended `AND from_agent = ` and became unsatisfiable by construction — a dispatched sub-agent is a different sender, so the one message being looked for is the one the filter removed. It answered `No messages found.` at rc=0 with an empty stderr. `--from` now always announces on stderr that it was applied as a sender filter, and any empty result names the filters that produced it, so a zero caused by the caller's own query is distinguishable from a genuinely empty store (#807d355d, #e60b8820). ### Added - **`conversations watch` can opt into full redacted channel content and monitor several identities in one process.** `--full-content` preserves actionable identifiers that the compact preview strips, while comma-separated `--from` values union independent inboxes without changing which identity owns writes (#74). diff --git a/package.json b/package.json index 1cc3a7e..1838dd0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hasna/conversations", - "version": "0.5.23", + "version": "0.5.24", "description": "Real-time CLI messaging for AI agents", "type": "module", "bin": { diff --git a/src/cli/commands/messaging.ts b/src/cli/commands/messaging.ts index e8e609f..8a4efaf 100644 --- a/src/cli/commands/messaging.ts +++ b/src/cli/commands/messaging.ts @@ -21,6 +21,13 @@ import type { DigestResult } from "../../lib/messages.js"; import { printErrorLine, printJson, printJsonLine, printLine } from "../../lib/stdout.js"; import { normalizeChannelName } from "../../lib/channel-names.js"; import { parseMessageReference } from "../../lib/message-reference.js"; +import { + discloseEmptyResult, + FROM_ALIAS_HELP, + noteSenderFilterAlias, + resolveSenderFilter, + SENDER_HELP, +} from "../sender-filter.js"; function quoteDigestCommandArg(value: string): string { return /^[A-Za-z0-9._:/@=-]+$/.test(value) ? value : `'${value.replace(/'/g, "'\\''")}'`; @@ -126,7 +133,8 @@ export function registerMessagingCommands(program: Command): void { .command("read") .description("Read messages") .option("--session ", "Filter by session ID") - .option("--from ", "Filter by sender") + .option("--sender ", SENDER_HELP) + .option("--from ", FROM_ALIAS_HELP) .option("--to ", "Filter by recipient") .option("--channel ", "Filter by channel") .option("--since ", "Messages after this ISO timestamp") @@ -138,10 +146,12 @@ export function registerMessagingCommands(program: Command): void { .option("--verbose", "Show full message bodies") .option("-j, --json", "Output as JSON") .action(async (opts) => { + const senderFilter = resolveSenderFilter(opts); + if (senderFilter.viaFromAlias) noteSenderFilterAlias(senderFilter.sender as string); const window = getCliWindow({ limit: opts.limit, cursor: opts.cursor }); const query = { session_id: opts.session, - from: opts.from, + from: senderFilter.sender, to: opts.to, channel: opts.channel, since: opts.since, @@ -160,6 +170,16 @@ export function registerMessagingCommands(program: Command): void { if (ids.length > 0) await await getStore().markReadByIds(ids, reader); } + if (messages.length === 0) { + discloseEmptyResult({ + channel: opts.channel, + sender: senderFilter.sender, + to: opts.to, + session: opts.session, + since: opts.since, + }); + } + if (opts.json) { printJson(messages); warnIfPageFull(messages.length, query.limit); @@ -292,7 +312,8 @@ export function registerMessagingCommands(program: Command): void { .description("Search messages by content") .argument("", "Search query string") .option("--channel ", "Filter by channel") - .option("--from ", "Filter by sender") + .option("--sender ", SENDER_HELP) + .option("--from ", FROM_ALIAS_HELP) .option("--to ", "Filter by recipient") .option("--limit ", "Max results to return (the server caps a single page at 500)", parseInt) .option("--cursor ", "Skip first N results for pagination", parseInt) @@ -325,12 +346,26 @@ channel, which is an ABSENCE claim. together. To enumerate a sender exhaustively, page a listing verb; do not infer a - population from a content search.`) + population from a content search. + + 3. --from IS A SENDER FILTER HERE, NOT YOUR IDENTITY. On nearly every other + subcommand --from names the caller; on search, read and export it appends + "AND from_agent = " to your query. So the liveness probe + + conversations search --channel --from + + is unsatisfiable by construction — a dispatched sub-agent is a DIFFERENT + sender, so the one message you are looking for is the one the filter + removes. It answered "No messages found." at rc=0 with an empty stderr + (todos 807d355d). --from still filters, and now always says so; --sender is + the unambiguous spelling. For identity, set CONVERSATIONS_AGENT_ID.`) .action(async (query, opts) => { const q = typeof query === "string" ? query.trim() : ""; if (!q) { emitCliError("Search query cannot be empty.", opts); } + const senderFilter = resolveSenderFilter(opts); + if (senderFilter.viaFromAlias) noteSenderFilterAlias(senderFilter.sender as string); const window = getCliWindow({ limit: opts.limit, cursor: opts.cursor }); // The store pages this verb now. `--json` used to pass the raw limit and @@ -339,11 +374,19 @@ channel, which is an ABSENCE claim. const result = await getStore().searchMessagesPage({ query: q, channel: opts.channel, - from: opts.from, + from: senderFilter.sender, to: opts.to, limit: opts.json ? opts.limit : window.limit, offset: opts.json ? opts.cursor : window.offset, }); + if (result.items.length === 0) { + discloseEmptyResult({ + query: q, + channel: opts.channel, + sender: senderFilter.sender, + to: opts.to, + }); + } const disclosure = { shown: result.items.length, hasMore: result.has_more, @@ -576,16 +619,19 @@ channel, which is an ABSENCE claim. .description("Export messages as JSON or CSV") .option("--channel ", "Filter by channel") .option("--session ", "Filter by session ID") - .option("--from ", "Filter by sender") + .option("--sender ", SENDER_HELP) + .option("--from ", FROM_ALIAS_HELP) .option("--since ", "Messages after this ISO date") .option("--until ", "Messages before this ISO date") .option("--format ", "Output format: json or csv", "json") .action(async (opts) => { + const senderFilter = resolveSenderFilter(opts); + if (senderFilter.viaFromAlias) noteSenderFilterAlias(senderFilter.sender as string); const format = opts.format === "csv" ? "csv" : "json"; const result = await getStore().exportMessages({ channel: opts.channel, session_id: opts.session, - from: opts.from, + from: senderFilter.sender, since: normalizeSince(opts.since), until: opts.until, format, diff --git a/src/cli/sender-filter-disclosure.e2e.test.ts b/src/cli/sender-filter-disclosure.e2e.test.ts new file mode 100644 index 0000000..1aa28ce --- /dev/null +++ b/src/cli/sender-filter-disclosure.e2e.test.ts @@ -0,0 +1,249 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { unlinkSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +/** + * End-to-end cover for todos 807d355d (`search`) and e60b8820 (`read`) — the + * same defect on two subcommands. + * + * `--from` means CALLER IDENTITY on ~20 of the CLI's subcommands and SENDER + * FILTER on exactly three (`read`, `search`, `export`). A caller who learned the + * dominant meaning and writes the canonical liveness probe + * + * conversations search --channel --from + * + * gets a SQL predicate `AND m.from_agent = ` bolted onto their query. The + * probe exists to ask "did my sub-agent post?", and a sub-agent is by definition + * a different sender — so the query is structurally unsatisfiable, and it + * answers "No messages found." at rc=0 with an EMPTY stderr. Measured on the + * live store at 0.5.22 before this change: the flag form returned 0 rows while + * the identical query without `--from` returned message #661877. + * + * The fix does NOT flip the filter's meaning. Silently widening a filter into a + * no-op is the same defect pointed the other way — the caller then gets every + * sender's rows at rc=0 and reads it as their own. What changes is that the + * filter is no longer SILENT, and that an unambiguous spelling exists. + * + * Every case below is asserted in both directions. A disclosure that never + * appears and one that always appears are equally worthless, so the negative + * cases — no filter, and a filter that legitimately matches nothing — are as + * load-bearing as the positive ones. + */ + +const TEST_DB = join(tmpdir(), `conversations-sender-filter-${Date.now()}.db`); +const CLI = ["bun", "run", "./src/cli/index.tsx"]; + +/** The measured scenario: a coordinator probing for a sub-agent's post. */ +const CHANNEL = "senderfilter-probe"; +const TOKEN = "subagenttoken807"; +const SUBAGENT = "subagent-807"; +const COORDINATOR = "coordinator-807"; + +function runCli(args: string[], agent: string) { + const result = Bun.spawnSync({ + cmd: [...CLI, ...args], + cwd: process.cwd(), + env: { + ...process.env, + // Precedence rule 1 in src/lib/store/index.ts: an explicit DB path wins + // over an exported cloud mode, so fleet credentials in the ambient + // environment cannot pull this suite onto the production store. Verified + // by measurement, not assumed — see the task record. + CONVERSATIONS_DB_PATH: TEST_DB, + CONVERSATIONS_AGENT_ID: agent, + FORCE_COLOR: "0", + }, + stdout: "pipe", + stderr: "pipe", + }); + return { + exitCode: result.exitCode, + stdout: new TextDecoder().decode(result.stdout), + stderr: new TextDecoder().decode(result.stderr), + /** Callers see one terminal; assert against both streams together. */ + get output() { + return `${new TextDecoder().decode(result.stdout)}${new TextDecoder().decode(result.stderr)}`; + }, + }; +} + +describe("sender filter is never silent", () => { + beforeAll(() => { + // A channel send is refused outright if the channel does not exist. + const created = runCli(["channel", "create", CHANNEL], COORDINATOR); + expect(created.exitCode).toBe(0); + + // The sub-agent posts the token the coordinator is looking for... + const sub = runCli( + ["send", `[${TOKEN}] sub-agent reporting in`, "--channel", CHANNEL], + SUBAGENT, + ); + expect(sub.exitCode).toBe(0); + + // ...and the coordinator has its own unrelated post in the same channel, + // so "0 rows" cannot be explained by an empty channel. + const coord = runCli( + ["send", "dispatch record, no token here", "--channel", CHANNEL], + COORDINATOR, + ); + expect(coord.exitCode).toBe(0); + }, 30_000); + + afterAll(() => { + for (const suffix of ["", "-wal", "-shm"]) { + try { unlinkSync(`${TEST_DB}${suffix}`); } catch {} + } + }); + + // ---- the message is reachable at all (fixture control) ---- + + test("CONTROL: without a sender filter the sub-agent's post is found", () => { + const res = runCli(["search", TOKEN, "--channel", CHANNEL], COORDINATOR); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + }); + + // ---- POSITIVE: findable via an unambiguous spelling ---- + + test("--sender selects the sub-agent's post", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--sender", SUBAGENT], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + }); + + test("--sender works on read as well as search", () => { + const res = runCli( + ["read", "--channel", CHANNEL, "--sender", SUBAGENT], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + }); + + // ---- POSITIVE: the false absence is disclosed ---- + + test("search: a zero caused by --from names the sender filter", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--from", COORDINATOR], + COORDINATOR, + ); + // The filter still filters — the row genuinely does not match. + expect(res.exitCode).toBe(0); + expect(res.stdout).not.toContain(SUBAGENT); + // ...but the caller is told WHY, and told that --from is a sender filter + // here rather than their identity. + expect(res.output).toContain("sender"); + expect(res.output).toContain(COORDINATOR); + expect(res.output.toLowerCase()).toContain("--sender"); + }); + + test("read: a zero caused by --from names the sender filter", () => { + const res = runCli( + ["read", "--channel", CHANNEL, "--from", "nobody-sent-anything"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.output).toContain("sender"); + expect(res.output).toContain("nobody-sent-anything"); + }); + + test("--from is disclosed even when it returns rows, because a filtered non-zero is wrong too", () => { + // The measured case included a NON-empty wrong answer: `--from manius` + // returned manius's own row and hid the sub-agent's. Warning only on zero + // would leave that case silent. + const res = runCli(["search", TOKEN, "--from", SUBAGENT], COORDINATOR); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + expect(res.stderr.toLowerCase()).toContain("--sender"); + }); + + // ---- NEGATIVE: the disclosure is not unconditional ---- + + test("an unfiltered zero does NOT claim a sender filter was applied", () => { + const res = runCli( + ["search", "zzqxnotarealtokenhere", "--channel", CHANNEL], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.output).not.toContain("--sender"); + expect(res.output.toLowerCase()).not.toContain("filtered by sender"); + }); + + test("a clean unfiltered search does NOT emit the alias note", () => { + const res = runCli(["search", TOKEN, "--channel", CHANNEL], COORDINATOR); + expect(res.exitCode).toBe(0); + expect(res.stderr).not.toContain("--sender"); + }); + + // ---- NEGATIVE: the filter still filters ---- + + test("--sender is a real filter, not silently widened into a no-op", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--sender", COORDINATOR], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).not.toContain(SUBAGENT); + expect(res.stdout).toContain("No messages found."); + }); + + test("--from keeps its existing filter semantics exactly", () => { + // Existing callers must not silently start receiving other senders' rows. + const res = runCli(["search", TOKEN, "--from", COORDINATOR], COORDINATOR); + expect(res.exitCode).toBe(0); + expect(res.stdout).not.toContain(SUBAGENT); + }); + + // ---- conflicting spellings never guess ---- + + test("--from and --sender disagreeing is a hard error, not a silent winner", () => { + const res = runCli( + ["search", TOKEN, "--from", COORDINATOR, "--sender", SUBAGENT], + COORDINATOR, + ); + expect(res.exitCode).toBe(1); + // Naming BOTH values is what distinguishes a real conflict error from + // commander's generic "unknown option '--sender'", which also exits 1 and + // would let this test pass against an unfixed build. + expect(res.output).toContain(COORDINATOR); + expect(res.output).toContain(SUBAGENT); + }); + + test("--from and --sender agreeing is accepted", () => { + const res = runCli( + ["search", TOKEN, "--from", SUBAGENT, "--sender", SUBAGENT], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + }); + + // ---- the --json stdout contract is unchanged ---- + + test("--json: stdout stays a bare array and the disclosure goes to stderr", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--from", COORDINATOR, "--json"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + const rows = JSON.parse(res.stdout); + expect(Array.isArray(rows)).toBe(true); + expect(rows).toHaveLength(0); + // Every monitor on this fleet parses stdout as an array, so the disclosure + // must not land there. + expect(res.stderr.toLowerCase()).toContain("--sender"); + }); + + // ---- help says what the flag does ---- + + test("search --help distinguishes the sender filter from caller identity", () => { + const res = runCli(["search", "--help"], COORDINATOR); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain("--sender"); + expect(res.stdout).toContain("identity"); + }); +}); diff --git a/src/cli/sender-filter.ts b/src/cli/sender-filter.ts new file mode 100644 index 0000000..64f7720 --- /dev/null +++ b/src/cli/sender-filter.ts @@ -0,0 +1,171 @@ +import chalk from "chalk"; +import { printErrorLine } from "../lib/stdout.js"; +import { emitCliError } from "./cli-error.js"; + +/** + * `--from` means two opposite things in this CLI, and the collision is silent. + * + * On roughly twenty subcommands — `send`, `reply`, `edit`, `delete`, `blockers`, + * `notifications`, `watch`, `digest --mark-read`, and every `agents`, + * `analytics`, `locks`, `channel` and `project` verb — `--from` names the CALLER: + * who you are. On exactly three — `read`, `search`, `export` — it is a FILTER on + * `m.from_agent`: who sent the message. Same spelling, opposite meaning, no + * warning either way. + * + * The cost is not hypothetical. The canonical liveness probe a coordinator uses + * to ask "did my dispatched sub-agent post its token?" is + * + * conversations search --channel --from + * + * written that way because `--from` is identity nearly everywhere else. A + * sub-agent is by definition a DIFFERENT sender, so the appended predicate makes + * the query unsatisfiable by construction: it can only ever return the + * coordinator's own dispatch record. Measured against the live store at 0.5.22, + * the flag form printed "No messages found." with an EMPTY stderr at rc=0, while + * the identical query without `--from` returned message #661877 (todos 807d355d; + * the same shape on `read` is e60b8820). + * + * WHAT THIS MODULE DOES NOT DO, deliberately: it does not flip `--from` to mean + * identity on these three verbs. Two reasons, and the second is the load-bearing + * one. + * + * 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 wearing different clothes. + * 2. It would silently WIDEN every existing caller's result set. A script + * auditing "messages from X" would start 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 is noticed, a wrong-full is + * acted upon. + * + * So the filter keeps its meaning and stops being silent. `--sender` is the + * unambiguous spelling; `--from` remains a working alias that always announces + * what it did; and a zero produced by any filter says which filters produced it. + */ + +/** Options shape shared by every sender-filtered verb. */ +export interface SenderFilterOptions { + from?: unknown; + sender?: unknown; + json?: boolean; + contract?: boolean; +} + +export interface ResolvedSenderFilter { + /** The sender to filter on, or undefined when no filter was requested. */ + sender: string | undefined; + /** True when the caller spelled it `--from`, which needs the alias note. */ + viaFromAlias: boolean; +} + +function trimmed(value: unknown): string | undefined { + if (typeof value !== "string") return undefined; + const text = value.trim(); + return text ? text : undefined; +} + +/** + * Resolve `--sender` / `--from` into one sender filter. + * + * Two different values is a hard error rather than a precedence rule. A caller + * who passes both plainly holds two beliefs about what the flags mean, and + * picking a winner silently would resolve that disagreement in favour of + * whichever one the implementer happened to order first — invisibly, in the one + * situation where the caller's confusion is already proven. + */ +export function resolveSenderFilter(opts: SenderFilterOptions): ResolvedSenderFilter { + const fromValue = trimmed(opts.from); + const senderValue = trimmed(opts.sender); + + if (fromValue && senderValue && fromValue !== senderValue) { + emitCliError( + `--from ${fromValue} and --sender ${senderValue} disagree about which sender to filter on. ` + + `On this subcommand --from is an alias for --sender (a filter on who SENT the message), ` + + `not your caller identity. Pass one of them, and set CONVERSATIONS_AGENT_ID for identity.`, + opts, + ); + } + + return { + sender: senderValue ?? fromValue, + viaFromAlias: Boolean(fromValue), + }; +} + +/** + * Announce, on stderr, that `--from` was applied as a SENDER FILTER. + * + * Emitted on EVERY use, not only when the result is empty. The measured defect + * included a NON-empty wrong answer — `--from manius` returned manius's own row + * and hid the sub-agent's — so a note that fired only on zero would have stayed + * silent through exactly the case where the caller reads a plausible result and + * never re-checks it. + * + * stderr rather than stdout so the `--json` array and the text result stay byte + * compatible for existing readers, while anyone at a terminal still sees it. + */ +export function noteSenderFilterAlias(sender: string): void { + printErrorLine( + chalk.yellow( + `Note: --from was applied as a SENDER filter (from_agent=${sender}), not as your caller identity. ` + + `On read/search/export --from selects who SENT a message; on every other subcommand it sets who you are. ` + + `Use --sender ${sender} to say so unambiguously, and CONVERSATIONS_AGENT_ID to set identity.`, + ), + ); +} + +export interface AppliedFilters { + query?: string; + channel?: string; + sender?: string; + to?: string; + session?: string; + since?: string; +} + +/** "query=\"tok\", channel=ops, sender=alice" — only the filters actually set. */ +export function formatAppliedFilters(filters: AppliedFilters): string { + const parts: string[] = []; + if (filters.query) parts.push(`query="${filters.query}"`); + if (filters.channel) parts.push(`channel=${filters.channel}`); + if (filters.sender) parts.push(`sender=${filters.sender}`); + if (filters.to) parts.push(`to=${filters.to}`); + if (filters.session) parts.push(`session=${filters.session}`); + if (filters.since) parts.push(`since=${filters.since}`); + return parts.join(", "); +} + +/** + * Make an empty result legible by naming the filters that produced it. + * + * A bare "No messages found." cannot distinguish "this store holds no such + * message" from "your own filter excluded it", and those two facts lead to + * opposite actions — the first ends a search, the second is a query bug. Nothing + * is emitted when no filter was applied, because a disclosure that always + * appears carries no information and gets tuned out. + * + * Goes to stderr in both output modes: the text surface's stdout stays exactly + * "No messages found." for anything matching on it, and the `--json` surface's + * stdout stays a bare array. + */ +export function discloseEmptyResult(filters: AppliedFilters): void { + const applied = formatAppliedFilters(filters); + if (!applied) return; + + const lines = [`No matches. Filters applied: ${applied}.`]; + if (filters.sender) { + lines.push( + `A sender filter excludes every message sent by anyone else, including a sub-agent you dispatched — ` + + `drop --sender to search all senders.`, + ); + } + printErrorLine(chalk.dim(lines.join(" "))); +} + +/** Help text for `--from` wherever it is a sender filter rather than identity. */ +export const FROM_ALIAS_HELP = + "Alias for --sender: filter by who SENT the message, NOT your caller identity (use CONVERSATIONS_AGENT_ID for that)"; + +/** Help text for the unambiguous spelling. */ +export const SENDER_HELP = "Filter by who SENT the message"; From 3fe067d797f334bf4879093a6fd1fa284e210431 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Wed, 5 Aug 2026 02:11:45 +0300 Subject: [PATCH 2/2] fix(cli): disclose an empty export, and refuse a blank sender instead 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 --- CHANGELOG.md | 7 +- src/cli/commands/messaging.ts | 24 +++++- src/cli/sender-filter-disclosure.e2e.test.ts | 89 +++++++++++++++++++- src/cli/sender-filter.ts | 39 ++++++++- 4 files changed, 153 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f21f696..4daa9cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,12 @@ All notable changes to this project will be documented in this file. - **`--sender` is the unambiguous spelling of the sender filter on `read`, `search`, and `export`.** `--from` keeps working and keeps its exact meaning on those verbs, so no existing caller's result set changes; the two disagreeing is a hard error rather than a silent precedence rule. ### Fixed -- **A sender filter can no longer produce a silent false absence.** `--from` names the CALLER on ~20 subcommands and filters on `from_agent` on three (`read`, `search`, `export`), so the canonical liveness probe `search --channel --from ` appended `AND from_agent = ` and became unsatisfiable by construction — a dispatched sub-agent is a different sender, so the one message being looked for is the one the filter removed. It answered `No messages found.` at rc=0 with an empty stderr. `--from` now always announces on stderr that it was applied as a sender filter, and any empty result names the filters that produced it, so a zero caused by the caller's own query is distinguishable from a genuinely empty store (#807d355d, #e60b8820). +- **A sender filter on `read`, `search`, or `export` can no longer produce a silent false absence.** `--from` names the CALLER on 26 subcommands and filters on `from_agent` on those three, so the canonical liveness probe `search --channel --from ` appended `AND from_agent = ` and became unsatisfiable by construction — a dispatched sub-agent is a different sender, so the one message being looked for is the one the filter removed. It answered `No messages found.` at rc=0 with an empty stderr. `--from` now always announces on stderr that it was applied as a sender filter, and an empty result from any of the three names the filters that produced it, so a zero caused by the caller's own query is distinguishable from a genuinely empty store (#807d355d, #e60b8820). +- **A blank `--sender` / `--from` is refused instead of silently dropping the filter.** `--sender "$WHO"` with `WHO` unset would otherwise return every sender's messages at exit code 0 and read as one sender's — the wrong-full direction, which is acted on rather than noticed. + +### Known gaps +- The disclosure covers the sender/recipient/channel/session/since dimensions. `--limit`, `--cursor`, and `--unread` are **not** yet named in it, so `read --cursor 999` against a populated channel is still a bare zero, and `read --channel X --cursor 999` prints an applied-filter line that omits the cursor. Tracked separately. +- The MCP surface (`src/mcp/tools/messaging.ts`) is unchanged and still carries the original ambiguity, including `read_messages` using `from` as caller identity and sender filter in the same call. Tracked separately. ### Added - **`conversations watch` can opt into full redacted channel content and monitor several identities in one process.** `--full-content` preserves actionable identifiers that the compact preview strips, while comma-separated `--from` values union independent inboxes without changing which identity owns writes (#74). diff --git a/src/cli/commands/messaging.ts b/src/cli/commands/messaging.ts index 8a4efaf..5472fbc 100644 --- a/src/cli/commands/messaging.ts +++ b/src/cli/commands/messaging.ts @@ -177,7 +177,7 @@ export function registerMessagingCommands(program: Command): void { to: opts.to, session: opts.session, since: opts.since, - }); + }, { senderFlag: senderFilter.flag }); } if (opts.json) { @@ -385,7 +385,7 @@ channel, which is an ABSENCE claim. channel: opts.channel, sender: senderFilter.sender, to: opts.to, - }); + }, { senderFlag: senderFilter.flag }); } const disclosure = { shown: result.items.length, @@ -636,6 +636,26 @@ channel, which is an ABSENCE claim. until: opts.until, format, }); + + // An export emptied by the caller's own filter is the same silent false + // absence this change exists to remove, and it reached review as a live + // defect: `export --sender ` printed "[]" with 0 bytes on stderr + // at rc=0, which made --sender on this verb strictly MORE silent than the + // --from it is offered as an improvement on. Emptiness is read off the + // rendered payload rather than re-querying: "[]" for json, headers with no + // data row for csv. + const exportedNothing = format === "csv" + ? !result.includes("\n") + : result.trim() === "[]"; + if (exportedNothing) { + discloseEmptyResult({ + channel: opts.channel, + sender: senderFilter.sender, + session: opts.session, + since: opts.since, + }, { senderFlag: senderFilter.flag }); + } + printLine(result); closeDb(); }); diff --git a/src/cli/sender-filter-disclosure.e2e.test.ts b/src/cli/sender-filter-disclosure.e2e.test.ts index 1aa28ce..113a125 100644 --- a/src/cli/sender-filter-disclosure.e2e.test.ts +++ b/src/cli/sender-filter-disclosure.e2e.test.ts @@ -170,7 +170,11 @@ describe("sender filter is never silent", () => { ); expect(res.exitCode).toBe(0); expect(res.output).not.toContain("--sender"); - expect(res.output.toLowerCase()).not.toContain("filtered by sender"); + // A second assertion here used to check for the absence of the string + // "filtered by sender", which appears nowhere in src/ except that assertion + // — so it could not fail in any state. Removed rather than reworded: an + // assertion that cannot fail is exactly the defect class this suite is about. + expect(res.output).not.toContain("sender="); }); test("a clean unfiltered search does NOT emit the alias note", () => { @@ -222,6 +226,89 @@ describe("sender filter is never silent", () => { expect(res.stdout).toContain(SUBAGENT); }); + // ---- export: the third sender-filtered verb ---- + + test("export: a zero caused by --sender names the sender filter", () => { + // Found in adversarial review: --sender on export was strictly MORE silent + // than the --from it is offered as an improvement on, because it gets no + // alias note by design and had no empty-result disclosure either. It printed + // "[]" with 0 bytes of stderr at rc=0. + const res = runCli( + ["export", "--channel", CHANNEL, "--sender", "ghostsender"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout.trim()).toBe("[]"); + expect(res.stderr).toContain("ghostsender"); + expect(res.stderr).toContain("--sender"); + }); + + test("export: a NON-empty result stays silent, so the notice means something", () => { + const res = runCli( + ["export", "--channel", CHANNEL, "--sender", SUBAGENT], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + expect(res.stderr).toBe(""); + }); + + test("export: csv format discloses an empty export too", () => { + const res = runCli( + ["export", "--channel", CHANNEL, "--sender", "ghostsender", "--format", "csv"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stderr).toContain("ghostsender"); + }); + + // ---- a blank sender is refused, never silently widened ---- + + test("--sender '' is a hard error, not an unfiltered read of the whole channel", () => { + // The dangerous direction: `--sender "$WHO"` with WHO unset would otherwise + // return every sender's messages at rc=0 and read as one sender's. + const res = runCli(["read", "--channel", CHANNEL, "--sender", ""], COORDINATOR); + expect(res.exitCode).toBe(1); + expect(res.stdout).not.toContain(SUBAGENT); + expect(res.output).toContain("--sender"); + }); + + test("--from with a whitespace-only value is refused rather than dropped", () => { + const res = runCli(["read", "--channel", CHANNEL, "--from", " "], COORDINATOR); + expect(res.exitCode).toBe(1); + expect(res.stdout).not.toContain(SUBAGENT); + }); + + test("a padded sender value still filters, rather than erroring", () => { + // Trimming a real value is the beneficial half and must survive the guard. + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--sender", ` ${SUBAGENT} `], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stdout).toContain(SUBAGENT); + }); + + // ---- the empty-result hint echoes the caller's own spelling ---- + + test("the hint names --from when the caller passed --from", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--from", "ghostsender"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stderr).toContain("drop --from to search all senders"); + }); + + test("the hint names --sender when the caller passed --sender", () => { + const res = runCli( + ["search", TOKEN, "--channel", CHANNEL, "--sender", "ghostsender"], + COORDINATOR, + ); + expect(res.exitCode).toBe(0); + expect(res.stderr).toContain("drop --sender to search all senders"); + }); + // ---- the --json stdout contract is unchanged ---- test("--json: stdout stays a bare array and the disclosure goes to stderr", () => { diff --git a/src/cli/sender-filter.ts b/src/cli/sender-filter.ts index 64f7720..b84c73b 100644 --- a/src/cli/sender-filter.ts +++ b/src/cli/sender-filter.ts @@ -57,6 +57,8 @@ export interface ResolvedSenderFilter { sender: string | undefined; /** True when the caller spelled it `--from`, which needs the alias note. */ viaFromAlias: boolean; + /** The spelling the caller actually typed, for messages that echo it back. */ + flag: string; } function trimmed(value: unknown): string | undefined { @@ -65,6 +67,11 @@ function trimmed(value: unknown): string | undefined { return text ? text : undefined; } +/** The flag was present on the command line but carries nothing usable. */ +function providedButBlank(value: unknown): boolean { + return typeof value === "string" && value.trim() === ""; +} + /** * Resolve `--sender` / `--from` into one sender filter. * @@ -75,6 +82,25 @@ function trimmed(value: unknown): string | undefined { * situation where the caller's confusion is already proven. */ export function resolveSenderFilter(opts: SenderFilterOptions): ResolvedSenderFilter { + // A PRESENT but blank value is an error, never "no filter". + // + // `--sender "$WHO"` with WHO unset would otherwise drop the predicate and + // return the ENTIRE channel at rc=0 in silence — the direction this file + // argues is the more dangerous one, arriving through the flag added to fix + // it. It is also a regression against the pre-change behaviour: `--from " "` + // used to filter on the literal whitespace and return nothing, so trimming it + // away silently converts a wrong-empty into a wrong-full. Both spellings are + // rejected rather than guessed. + if (providedButBlank(opts.sender) || providedButBlank(opts.from)) { + const flag = providedButBlank(opts.sender) ? "--sender" : "--from"; + emitCliError( + `${flag} was given an empty value. A blank sender is refused rather than ignored, because dropping ` + + `the filter would return every sender's messages at exit code 0 and read as one sender's. ` + + `Pass a sender name, or omit ${flag} entirely to search all senders.`, + opts, + ); + } + const fromValue = trimmed(opts.from); const senderValue = trimmed(opts.sender); @@ -87,9 +113,12 @@ export function resolveSenderFilter(opts: SenderFilterOptions): ResolvedSenderFi ); } + // `--sender` is the spelling to echo whenever it was given, even alongside an + // agreeing `--from`, because it is the one this CLI wants callers to keep. return { sender: senderValue ?? fromValue, viaFromAlias: Boolean(fromValue), + flag: senderValue ? "--sender" : "--from", }; } @@ -149,15 +178,21 @@ export function formatAppliedFilters(filters: AppliedFilters): string { * "No messages found." for anything matching on it, and the `--json` surface's * stdout stays a bare array. */ -export function discloseEmptyResult(filters: AppliedFilters): void { +export function discloseEmptyResult( + filters: AppliedFilters, + opts: { senderFlag?: string } = {}, +): void { const applied = formatAppliedFilters(filters); if (!applied) return; const lines = [`No matches. Filters applied: ${applied}.`]; if (filters.sender) { + // Name the spelling the caller actually typed. Telling someone who passed + // --from to "drop --sender" reads as advice about a flag they did not use. + const flag = opts.senderFlag ?? "--sender"; lines.push( `A sender filter excludes every message sent by anyone else, including a sub-agent you dispatched — ` + - `drop --sender to search all senders.`, + `drop ${flag} to search all senders.`, ); } printErrorLine(chalk.dim(lines.join(" ")));