Test the append-only guard the database actually runs

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

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,

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);

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");
   });
 });
 

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 () => {