diff --git a/CHANGELOG.md b/CHANGELOG.md index 4daa9cc..9faace7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,31 @@ All notable changes to this project will be documented in this file. ## Unreleased +## 0.5.25 - 2026-08-05 + +### Fixed +- **`send` and `reply` no longer report a fully successful write as a failure.** Against the deployed server every hosted write exited 1 with `Message write returned UUID instead of , and the exact row could not be read back. Refusing to report a numeric message id.` — while the message landed, in the right channel, correctly threaded, with the right content and sender. The exit code was not the harm: the natural response to "your write may not have landed" is to re-send, on a shared channel, where the retry reported the same false failure. It also withheld the message id that the fleet's citation conventions depend on (todos `d8f3f963`). + + Two absent server capabilities were required to reproduce, and the client assumed both. `POST /v1/messages` does not accept a caller `uuid` — it is absent from the route's published request schema — so the server drops it, mints its own, and returns **our row** under a different UUID. `GET /v1/messages/by-uuid/{uuid}` does not exist at all, and falls through to the generic unknown-route handler, whose 404 is **indistinguishable by status** from a real row-miss: + + ``` + /v1/messages/by-uuid/ -> 404 {"error":"Not found"} + /v1/definitely-not-a-route -> 404 {"error":"Not found"} + /v1/messages/999999999 -> 404 {"error":"Message not found"} <- route that DOES exist + ``` + + `getMessageByUuid` maps any 404 to `null`, so a missing **route** became "the row is not there", and a write that had demonstrably succeeded was reported as a failure. `sendMessage` now falls back — only after the authoritative UUID read-back has been tried and found unanswerable — to checking whether the row the server *did* return is the write just submitted, by the routing identity the caller controls. A response describing some other row (the mention-notification DM the UUID binding exists to catch) is still refused, loudly. + +- **The caller-UUID guarantee 0.5.23 announced was never true against the deployed server.** That release's note claims "hosted writes preserve caller-generated UUIDs (#77)". They are not — the deployed server has no `uuid` field on its create route, verified behaviourally rather than from the schema alone (the schema declares no `additionalProperties: false`, so it accepts the field and ignores it). 0.5.23 went to the `next` dist-tag and carries no release tag and no npm provenance attestation, having been published before `release.yml` existed; #77 therefore first reached `latest` **in 0.5.24**, which is why the onset tracks the 0.5.24 publish while the causing change shipped in 0.5.23. + +### Added +- **A degraded write-confirmation is disclosed instead of passing silently.** When the id could only be confirmed from the routing of the returned row rather than by reading the row back under the caller-bound UUID, the returned message carries `write_confirmation: { degraded: true, method: "routing-echo" }`. An authoritative confirmation carries no such field, so its disappearance is also the signal that this fallback has become dead code and can be removed. + +### Known gaps +- The underlying conflation is unchanged: `getMessageByUuid` still cannot distinguish a missing route from a missing row, because it has only the HTTP status to go on. The repair is scoped to `sendMessage`, where the false failure was reachable; the other callers are user-facing lookups where "not found" is an honest answer. A discriminated result type would change the `ConversationsStore` interface and every implementation, so it is deliberately not in this patch. +- The accept path proves the returned row is addressed exactly as requested and carries a usable id. It does **not** prove the row is not some other message with identical routing. That residual is accepted only where the alternative is failing 100% of successful writes on a server that cannot be asked. +- **The hosted service is a different codebase, not an older build of this one.** `conversations.hasna.xyz` reports version `1.0.0-rc.1`, a string that has never existed in this repository; its route set matches `hasnaxyz/iapp-conversations` exactly (15 paths, identical both ways) and differs from this repo's server in both directions — production serves `/v1/health` and `/v1/whoami` that this repo does not declare, and lacks the `/v1/messages/by-uuid/{uuid}` that it does. So "deploy the server" is a port or a replatform, not a redeploy, and it belongs against that repository. This patch makes the client survive a server it does not control rather than waiting on that work. + ## 0.5.24 - 2026-08-05 ### Added diff --git a/package.json b/package.json index 1838dd0..9c2638b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hasna/conversations", - "version": "0.5.24", + "version": "0.5.25", "description": "Real-time CLI messaging for AI agents", "type": "module", "bin": { diff --git a/src/lib/store/api-store.test.ts b/src/lib/store/api-store.test.ts index 7973b07..c1d3f7c 100644 --- a/src/lib/store/api-store.test.ts +++ b/src/lib/store/api-store.test.ts @@ -292,6 +292,302 @@ describe("ApiStore.sendMessage wire body", () => { expect(message).toMatchObject({ id: 649560, uuid: exactUuid, channel: "git-publishing" }); }); + // ── regression: rc=1 on a write that fully succeeded (todos d8f3f963) ──────── + // + // MEASURED against the live deployed server, conversations.hasna.xyz, on + // 2026-08-05. Both halves of the contract the caller-bound UUID check assumes + // are absent there, and BOTH are needed to reproduce: + // + // 1. `POST /v1/messages` does not accept a caller `uuid`. Its published + // request schema lists exactly from,to,content,channel,project_id, + // session_id,priority,blocking — no uuid. The server drops ours, mints + // its own, stores THE CORRECT ROW, and returns it: + // sent uuid=0c57bc9f-480f-4e74-b263-49c2e8850a0a + // returned uuid=d9ad71d6-1417-414b-b4d3-bc35b789f5a6 HTTP 201 + // returned content/channel/from == exactly what was submitted + // 2. `GET /v1/messages/by-uuid/{uuid}` does not exist. It falls through to + // the generic unknown-route handler, which is a 404 INDISTINGUISHABLE BY + // STATUS from a real row-miss: + // /v1/messages/by-uuid/ -> 404 {"error":"Not found"} + // /v1/definitely-not-a-route -> 404 {"error":"Not found"} + // /v1/messages/999999999 -> 404 {"error":"Message not found"} + // (the third is the positive control: a route that DOES exist answers a + // miss with its own body, so the discriminator can fire both ways.) + // + // `getMessageByUuid` swallows any 404 as `null`, so a missing ROUTE became + // "the row is not there", and sendMessage reported a successful write as a + // failure. The exit code is not the harm: the caller's natural response to + // "your write may not have landed" is to re-send, on a shared channel, where + // the retry reports the same false failure. + test("reports the row it wrote when the server drops the caller UUID and has no by-uuid route", async () => { + const submittedUuid = "aaaaaaaa-1111-4222-8333-444444444444"; + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + // The by-uuid route does not exist on this server: every read 404s. + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + // Server-minted UUID, but unmistakably the row we asked to be written. + // + // NOTE THE RECIPIENT. On a channel send the server rewrites `to_agent` to + // the CHANNEL, discarding whatever the caller passed — measured: + // sent to="silvanus" channel="scratch-d8f3f963" + // returned to_agent="scratch-d8f3f963" + // and `src/cli/commands/messaging.ts` passes `to: to || from`, i.e. the + // SENDER. An earlier draft of this fixture used to == channel, which made + // it agree with a check that rejected every real channel send; the live + // CLI caught it, this fixture did not. It now mirrors the real call. + create: async () => ({ + message: { + id: "668569", + uuid: "bbbbbbbb-5555-4666-8777-888888888888", + session_id: "channel:git-publishing", + from_agent: "silvanus", + to_agent: "git-publishing", + channel: "git-publishing", + content: "[PUBLISH INTENT] @hasna/conversations", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + const message = await store.sendMessage({ + uuid: submittedUuid, + from: "silvanus", + to: "silvanus", // what the CLI actually sends for a channel post + channel: "git-publishing", + content: "[PUBLISH INTENT] @hasna/conversations", + }); + + // The caller must get a usable numeric id back — withholding it is what + // breaks the citation convention that keeps corroboration from collapsing + // to a single source. + expect(message).toMatchObject({ id: 668569, channel: "git-publishing" }); + }); + + // The other side of the same guarantee. A fix that merely stopped throwing + // would pass the test above and destroy the property #77 bought, so this + // pins the case that MUST still fail loudly: the response describes some + // OTHER row (the mention-notification DM the UUID binding exists to catch), + // and our row cannot be read back. + test("still refuses when the response names a different row and the write cannot be confirmed", async () => { + const submittedUuid = "cccccccc-9999-4aaa-8bbb-cccccccccccc"; + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + create: async () => ({ + message: { + id: 999001, + uuid: "dddddddd-eeee-4fff-8000-111111111111", + session_id: "dm:someone-else", + from_agent: "silvanus", + to_agent: "someone-else", + channel: null, + content: "you were mentioned", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + await expect(store.sendMessage({ + uuid: submittedUuid, + from: "silvanus", + to: "silvanus", + channel: "git-publishing", + content: "[PUBLISH INTENT] @hasna/conversations", + })).rejects.toThrow(/could not be read back/); + }); + + // The DM half of the same guard. With no channel to compare, the recipient is + // the only thing separating our row from a notification DM fanned out to a + // mentioned third party, so it must still be enforced there. + test("still refuses a DM whose response names a different recipient and cannot be read back", async () => { + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + create: async () => ({ + message: { + id: 999002, + uuid: "eeeeeeee-1111-4222-8333-444444444444", + session_id: "dm:someone-else", + from_agent: "silvanus", + to_agent: "someone-else", + channel: null, + content: "you were mentioned", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + await expect(store.sendMessage({ + uuid: "ffffffff-1111-4222-8333-444444444444", + from: "silvanus", + to: "manius", + content: "a direct message", + })).rejects.toThrow(/could not be read back/); + }); + + // And the DM ACCEPT path, so the DM branch is exercised in both directions + // rather than only proved capable of refusing. + test("reports a DM the server echoed back under a server-minted UUID", async () => { + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + create: async () => ({ + message: { + id: 668600, + uuid: "11111111-2222-4333-8444-555555555555", + session_id: "dm:manius", + from_agent: "silvanus", + to_agent: "manius", + channel: null, + content: "a direct message", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + const message = await store.sendMessage({ + uuid: "ffffffff-9999-4aaa-8bbb-cccccccccccc", + from: "silvanus", + to: "manius", + content: "a direct message", + }); + + expect(message).toMatchObject({ id: 668600, to_agent: "manius" }); + }); + + // Returning the id silently would leave a caller unable to tell an + // authoritative UUID read-back from the weaker routing check, and would leave + // nothing to mark this path dead once the server serves /messages/by-uuid. + test("discloses that confirmation degraded to the routing echo", async () => { + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + create: async () => ({ + message: { + id: 668700, + uuid: "22222222-3333-4444-8555-666666666666", + session_id: "channel:git-publishing", + from_agent: "silvanus", + to_agent: "git-publishing", + channel: "git-publishing", + content: "degraded confirmation", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + const message = (await store.sendMessage({ + uuid: "33333333-4444-4555-8666-777777777777", + from: "silvanus", + to: "silvanus", + channel: "git-publishing", + content: "degraded confirmation", + })) as unknown as { id: number; write_confirmation?: { degraded: boolean; method: string } }; + + expect(message.id).toBe(668700); + expect(message.write_confirmation).toMatchObject({ degraded: true, method: "routing-echo" }); + }); + + // The other side: an AUTHORITATIVE confirmation must carry no degradation + // marker, or the flag means nothing and cannot signal the path is dead. + test("does NOT mark confirmation degraded when the server honours the caller UUID", async () => { + const boundUuid = "44444444-5555-4666-8777-888888888888"; + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: {} as unknown as HasnaStorageClient["transport"], + create: async (_r: string, body: Record) => ({ + message: { + id: 668701, + uuid: body.uuid, + session_id: "channel:git-publishing", + from_agent: "silvanus", + to_agent: "git-publishing", + channel: "git-publishing", + content: "authoritative", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + const message = (await store.sendMessage({ + uuid: boundUuid, + from: "silvanus", + to: "silvanus", + channel: "git-publishing", + content: "authoritative", + })) as unknown as { id: number; write_confirmation?: unknown }; + + expect(message.id).toBe(668701); + expect(message.write_confirmation).toBeUndefined(); + }); + + // A blank identity on either side must not satisfy an accept test. Without + // the non-empty guard, `"" === ""` passes and the sender check asserts + // nothing at all. + test("refuses to accept an echo whose sender identity is blank on both sides", async () => { + const notFound = Object.assign(new Error("Not Found"), { name: "HasnaHttpError", status: 404 }); + const client = { + name: "conversations", + baseUrl: "https://conversations.hasna.xyz/v1", + transport: { + get: async () => { + throw notFound; + }, + } as unknown as HasnaStorageClient["transport"], + create: async () => ({ + message: { + id: 668702, + uuid: "55555555-6666-4777-8888-999999999999", + session_id: "channel:git-publishing", + from_agent: "", + to_agent: "git-publishing", + channel: "git-publishing", + content: "blank sender", + }, + }), + } as unknown as HasnaStorageClient; + const store = new ApiStore(client); + + await expect(store.sendMessage({ + uuid: "66666666-7777-4888-8999-aaaaaaaaaaaa", + from: "", + to: "silvanus", + channel: "git-publishing", + content: "blank sender", + })).rejects.toThrow(/could not be read back/); + }); + // `messages.id`/`messages.reply_to` are Postgres BIGINT, and node-postgres // serializes int8 as a STRING. Measured against the live deployed server: // GET /v1/messages returns `"id": "603183"` — a string, not a number. diff --git a/src/lib/store/api-store.ts b/src/lib/store/api-store.ts index fd226eb..6f3ad9f 100644 --- a/src/lib/store/api-store.ts +++ b/src/lib/store/api-store.ts @@ -50,6 +50,83 @@ function prune(q: Q): Record { return out; } +/** + * Case-insensitive compare of two identity strings, where BLANK NEVER MATCHES. + * + * The empty-vs-empty case is the one that matters: a plain `l === r` returns + * true for two absent values, so a guard built on it would pass on nothing at + * all rather than on a match. Every use below is an accept test, so an + * unknown identity must fail it. + */ +function sameIdentity(a: unknown, b: unknown): boolean { + const l = typeof a === "string" ? a.trim().toLowerCase() : ""; + const r = typeof b === "string" ? b.trim().toLowerCase() : ""; + if (!l || !r) return false; + return l === r; +} + +/** + * Is `returned` the row this very `sendMessage` call submitted? + * + * Compares the routing identity the CALLER supplied — sender, recipient and + * channel — because that is precisely what separates our row from the hazard + * the caller-bound UUID exists to catch: a mention-notification DM fanned out + * by the same write, which is addressed to a DIFFERENT agent and carries no + * channel. + * + * It deliberately does NOT compare content. The server may legitimately store + * a redacted body, and `attachSendRedaction` is the surface that reports that + * divergence to the author; re-deriving it here would reject a correct write + * for the wrong reason. (`describeSendRedaction` cannot help either — it + * reports that two bodies DIFFER, not that the difference is a redaction, so + * using it as an accept test would accept any difference at all.) + * + * STATE THE LIMIT, because this is an accept path: it proves the returned row + * is addressed exactly as requested and carries a usable id. It does NOT prove + * the row is not some other message with identical routing. That residual is + * accepted only where the alternative is failing 100% of successful writes on + * a server that cannot be asked — a certain, universal false failure traded + * for a narrow ambiguity, and only after the authoritative UUID read-back has + * already been tried and found unanswerable. + */ +function echoesSubmittedWrite( + opts: { from?: string; to?: string; channel?: string | null }, + returned: { id?: unknown; from_agent?: unknown; to_agent?: unknown; channel?: unknown }, +): boolean { + // Without a usable id there is nothing to report, so there is nothing to accept. + const id = Number(returned.id); + if (!Number.isFinite(id) || id <= 0) return false; + + if (!sameIdentity(returned.from_agent, opts.from)) return false; + + const wantChannel = opts.channel ? normalizeChannelName(opts.channel) : ""; + const gotChannel = typeof returned.channel === "string" && returned.channel + ? normalizeChannelName(returned.channel) + : ""; + if (wantChannel !== gotChannel) return false; + + if (wantChannel) { + // CHANNEL POST. The server OWNS the recipient field here and rewrites it to + // the channel, whatever the caller passed. Measured on the deployed server: + // sent to="silvanus" channel="scratch-d8f3f963" + // returned to_agent="scratch-d8f3f963" + // and the CLI's own channel path passes `to: to || from`, i.e. the SENDER. + // So insisting on `to_agent === opts.to` would reject every correct channel + // send — which is exactly what an earlier revision of this function did, + // caught only by running the real CLI against the real server rather than + // by the unit fixture, which had been written with to == channel and so + // could not fail. The channel plus the sender IS the identity here, and the + // mention-notification DM this guard exists to reject carries NO channel, + // so the channel comparison above already separates it. + return sameIdentity(returned.to_agent, gotChannel) || sameIdentity(returned.to_agent, opts.to); + } + + // DIRECT MESSAGE. No channel to discriminate on, so the recipient is the + // discriminator — and it is precisely what tells our row apart from a + // notification DM addressed to a mentioned third party. + return sameIdentity(returned.to_agent, opts.to); +} + /** Duck-typed HasnaHttpError status check (class identity differs across bundles). */ function isHttpStatus(error: unknown, status: number): boolean { return Boolean( @@ -579,13 +656,53 @@ export class ApiStore implements ConversationsStore { // observed. Never trust a later mutable numeric id as the identity of the // row we wrote: read back by the caller-bound immutable UUID instead. const exact = await this.getMessageByUuid(messageUuid); - if (!exact) { - throw new Error( - `Message write returned UUID ${returned.uuid || "(missing)"} instead of ${messageUuid}, ` + - `and the exact row could not be read back. Refusing to report a numeric message id.` - ); + if (exact) return attachSendRedaction(opts.content, exact) as never; + + // Nothing under our UUID — and that has TWO causes this transport cannot + // tell apart, because `getMessageByUuid` maps every 404 to null and a + // server with no `/messages/by-uuid` route answers with the SAME bare 404 + // as a genuine row-miss. Measured on the deployed server, 2026-08-05: + // /v1/messages/by-uuid/ -> 404 {"error":"Not found"} (no route) + // /v1/definitely-not-a-route -> 404 {"error":"Not found"} (no route) + // /v1/messages/999999999 -> 404 {"error":"Message not found"} (real miss) + // That server also drops the caller `uuid` on create — it is absent from + // the route's published request schema — mints its own, and returns OUR + // row. So the strict check reported EVERY successful send as a failure. + // + // Treating "could not verify" as "did not write" is the expensive + // direction: a caller's natural response is to re-send, on a shared + // channel, where the retry reports the same false failure. + // + // So ask the one question still answerable: is the row the server DID + // return the write we just submitted? + if (echoesSubmittedWrite(opts, returned)) { + // DISCLOSE THE DOWNGRADE rather than returning silently. Confirmation + // fell from "the server handed back the row under the UUID we bound" to + // "the row it handed back is routed the way we asked", which is a weaker + // claim, and a caller that cannot tell the two apart cannot know which + // guarantee its id carries. This mirrors how the same file already + // handles a server-side row cap (`truncated`) instead of presenting a + // degraded result as a complete one. + // + // It is also the only thing that will ever mark this path dead: once the + // server serves `/messages/by-uuid`, the flag stops appearing, and its + // absence is the signal that this fallback can be removed. + return attachSendRedaction(opts.content, { + ...returned, + write_confirmation: { + degraded: true, + method: "routing-echo", + message: + "The server did not preserve the caller-bound message UUID and cannot be queried by UUID, " + + "so this id was confirmed from the routing of the row it returned, not by reading the row back.", + }, + }) as never; } - return attachSendRedaction(opts.content, exact) as never; + + throw new Error( + `Message write returned UUID ${returned.uuid || "(missing)"} instead of ${messageUuid}, ` + + `and the exact row could not be read back. Refusing to report a numeric message id.` + ); }; getMessageById: ConversationsStore["getMessageById"] = async (id) => { const body = await this.client.get<{ message: Record }>("messages", String(id));