From ca8971306a3b8f96e9b6773fc89ea3381eae66e1 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 21:31:10 -0700 Subject: [PATCH 1/2] Test the append-only guard the database actually runs The immutability test read `0000_schema.sql` and asserted on the function defined there. 0007 replaced that function and 0012 replaced it again and added the truncate trigger, so the assertions described a definition no database runs, and would have gone on passing if the live one were edited out from under them. A mirror test pinned to one file is a test of that file. It now reads the whole migration chain, requires both triggers, and fails if a later migration drops either. Also covers the trigger's own branch for a malformed retention window. `sweepAuditTrail` returns before it opens a connection, so the existing test proves the caller's guard and never reaches the trigger, which is the branch anything not going through the sweep would meet. Both reported by @beardthelion in #138 and written in their PR #139, which arrived before the fix that merged and covered ground it did not. Catches the changelog up with tonight's fixes, including the truncate hole with the caveat about the owner role that the report was careful to state. --- CHANGELOG.md | 35 ++++++++++++++++ .../tests/audit-retention.integration.test.ts | 31 ++++++++++++++ server/tests/audit.test.ts | 41 ++++++++++++++----- 3 files changed, 97 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e952b6e..3cc5bb43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,41 @@ Sessions survive and nobody signs in again. is unavailable never blocks a sign-in. ### Fixed +- **The audit trail could be erased with one statement.** It is append-only because a database + trigger refuses updates and deletes, and that trigger is row-level, so `TRUNCATE` never reached it: + anything holding `DATABASE_URL` could empty the table and nothing raised. That is the case the + guarantee exists for, since it is enforced in the database precisely because the application is not + the only thing that reaches the table. A statement-level trigger now refuses a truncate, and it + answers before the retention setting is read, so declaring a retention window no longer permits one + either. Retention itself is unchanged: rows older than the window are still removed, and recent + ones are still refused. The connection the application uses is the database owner in the shipped + compose file, and an owner can still disable or drop a trigger; closing that needs a role with + `INSERT` and `SELECT` only, which is a separate change. Reported by @beardthelion, who also named + the failure mode of the obvious fix and saved it from shipping as one. +- **A declined take-the-wheel destroyed the conversation.** A Bot that asks for help with a sign-in + and never gets it left an assistant message holding a tool call that nothing ever answered, and + every later turn in that thread failed at the provider. Declining once meant nothing you typed + afterwards got an answer, with no way back but a new chat. Unanswered calls are now answered when + the history is rebuilt, with the truth rather than a fake success: no result came, the run has + ended, carry on without it and say what could not be done. +- **The audit trail could not say why a conversation went where it did.** It recorded the router's + choice and recorded nothing at all when a person named a coworker with `@`, which is + indistinguishable from a row that failed to write. A mention is now recorded too, as the person's + own choice, without asking the model a question they had already answered. The audit page names the + coworker and separates the three cases: chosen by the person, matched by the router, or the default + because nothing matched. +- **A Bot with half a connector sent people to a sign-in box.** Granted a vendor's search but not its + read, it found the document, could not read it, and opened the vendor's website to try, where it + met a sign-in wall and asked the person to take the wheel. They already had access; the missing + thing was the Bot's grant, and nothing said so. A gap in what a Bot holds is now reported as a gap: + it names the capability it would need and says an administrator can grant it on that connector. +- **Answers arrived with no sign of where they came from.** Asked a compliance question, a Bot + replied with a filing obligation, a dollar threshold, a deadline and a retention period, and the + audit trail for that turn held one row: the routing decision. A confident unsourced answer is + indistinguishable from a confident wrong one. Every Bot is now told to cite what it read and to say + plainly when an answer is from its own knowledge instead. It is told this by the deployment rather + than per agent, so it cannot be missing from the next Bot somebody adds, and it is explicitly not + an instruction to go hunting for a source. - **A Bot browsed to a vendor it already had tools for.** Granted Google Drive, asked what was in a document, it opened `drive.google.com` in its own browser, met a sign-in page that browser can never satisfy, and asked the person to sign in to an account they had already connected. A tool diff --git a/server/tests/audit-retention.integration.test.ts b/server/tests/audit-retention.integration.test.ts index 8cec8221..5170d0fd 100644 --- a/server/tests/audit-retention.integration.test.ts +++ b/server/tests/audit-retention.integration.test.ts @@ -113,6 +113,37 @@ describe("keeping the audit trail to a retention policy", () => { expect(await remaining()).toBe(1); }); + test("the database refuses a malformed window even when the caller does not", async () => { + /* + * The test above stops in TypeScript. `sweepAuditTrail` returns `{deleted: null}` before it ever + * opens a connection, so it proves the caller's guard and says nothing about the trigger, which + * has a branch of its own for this that nothing exercised. That is the shape where a guard reads + * as covered and is not: anything reaching the table without going through the sweep meets the + * trigger and only the trigger. + * + * These set the value the sweep would have set and then delete directly. + * + * Reported by @beardthelion. + */ + await event(400, 1); + + for (const window of ["0", "-1", "", "garbage"]) { + expect( + await refusedAsAppendOnly(() => + database.transaction(async (tx) => { + await tx.execute( + sql`select set_config('openbot.audit_retention_days', ${window}, true)`, + ); + await tx + .delete(auditEvents) + .where(eq(auditEvents.targetType, MARKER)); + }), + ), + ).toBe(true); + } + expect(await remaining()).toBe(1); + }); + test("a row exactly inside the window survives", async () => { // The boundary. Off by a day here means a deployment promising 90 days keeps 89. await event(89, 1); diff --git a/server/tests/audit.test.ts b/server/tests/audit.test.ts index 30b61b2a..22a7dedf 100644 --- a/server/tests/audit.test.ts +++ b/server/tests/audit.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { readFile } from "node:fs/promises"; +import { readdir, readFile } from "node:fs/promises"; import { createApp } from "../src/app"; import { auditEventTypes, @@ -98,15 +98,36 @@ describe("audit payload redaction", () => { }); describe("audit event immutability", () => { - test("installs a database trigger that rejects updates and deletes", async () => { - const migration = await readFile( - new URL("../drizzle/0000_schema.sql", import.meta.url), - "utf8", - ); - - expect(migration).toContain("CREATE FUNCTION prevent_audit_event_mutation"); - expect(migration).toContain("BEFORE UPDATE OR DELETE ON audit_events"); - expect(migration).toContain("Audit events are append-only"); + /* + * What the guard DOES is proved against a real database in + * audit-retention.integration.test.ts. This asserts only that it is still installed by some + * migration, and it reads the whole chain to do it. + * + * Reading 0000 alone stopped being true a while ago: 0007 replaced the function and 0012 replaced + * it again and added the truncate trigger, so the old assertions described a definition no + * database runs and would have gone on passing if the current one were edited out from under + * them. A mirror test pinned to one file is a test of that file, not of the deployment. + * + * Reported by @beardthelion, alongside the TRUNCATE hole itself. + */ + test("some migration still installs the append-only guard", async () => { + const directory = new URL("../drizzle/", import.meta.url); + const files = (await readdir(directory)) + .filter((name) => name.endsWith(".sql")) + .sort(); + const chain = ( + await Promise.all( + files.map((name) => readFile(new URL(name, directory), "utf8")), + ) + ).join("\n"); + + expect(chain).toContain("FUNCTION prevent_audit_event_mutation"); + expect(chain).toContain("BEFORE UPDATE OR DELETE ON audit_events"); + expect(chain).toContain("BEFORE TRUNCATE ON audit_events"); + expect(chain).toContain("Audit events are append-only"); + // A later migration removing either trigger would otherwise satisfy every line above. + expect(chain).not.toContain("DROP TRIGGER audit_events_append_only"); + expect(chain).not.toContain("DROP TRIGGER audit_events_no_truncate"); }); }); From a89d230281fdbf25354281fa49e4aed2a7d5292d Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 21:32:37 -0700 Subject: [PATCH 2/2] Say who routed a conversation in the third person The mention row's reason read "you chose them yourself". True in the conversation and false on the audit page, where every row belongs to somebody else and an administrator is the one reading it. The person is already on the row as actorUserId; the reason only has to say what kind of decision it was. @zopeVaibhav had this right in #134. --- server/src/routing/routes.ts | 11 ++++++++++- server/tests/routing-routes.test.ts | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index 665ee4fd..5d523c20 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -116,7 +116,16 @@ export function createRoutingRoutes( 404, ); } - const reason = "you chose them yourself"; + /* + * Third person, because the audit page is not read by the person who chose. + * + * This said "you chose them yourself", which is true in the conversation and false on an + * administrator's screen, where every row is somebody else's. The person is already on the + * row as `actorUserId`; the reason only has to say what kind of decision it was. + * + * @zopeVaibhav had this right in #134. + */ + const reason = "named by the person asking"; await record(actorId(actor), chosen.id, reason, false, true, [chosen.id]); return context.json({ agentId: chosen.id, diff --git a/server/tests/routing-routes.test.ts b/server/tests/routing-routes.test.ts index d59ee589..974996b2 100644 --- a/server/tests/routing-routes.test.ts +++ b/server/tests/routing-routes.test.ts @@ -127,6 +127,8 @@ describe("recording which coworker a message went to", () => { viaMention: true, fallback: false, }); + // Third person: an administrator reading this row is not the person who chose. + expect(written[0]?.payload.reason).toBe("named by the person asking"); }); test("naming a coworker never asks the model", async () => {