From 3ac92c81859077048264b518466247cd5fa248d7 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 15:59:59 +0000 Subject: [PATCH 1/2] test(audit): assert the log-less 404 writes NO access record, for any result (BLO-34901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both BLO-34738 guards pinned `details: { result: "allowed" }`, so they asserted the absence of one particular record rather than the invariant. A change that books the same log-less 404 as `result: "denied"` is still a false access record — the reader is entitled and the read simply found nothing — and it passed both guards unchanged. Drop the `details` matcher on both: no `heartbeat.run_log_accessed` / `workspace_operation.log_accessed` record of any kind. Test-only. No change under server/src/routes or server/src/services. Controls, one mutation at a time, four runs: A1 audit("allowed") moved back above heartbeat.readLog -> FAILS (existing control held) A2 route books the log-less 404 as "denied" -> FAILS A2' same mutation, old `result: "allowed"` matcher restored -> PASSES (the gap this closes) B1/B2/B2' same three on the workspace-operation route -> FAILS / FAILS / PASSES Unmutated: 35/35 pass; tsc --noEmit -p server/tsconfig.json exit 0. --- server/src/__tests__/agent-live-run-routes.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/agent-live-run-routes.test.ts b/server/src/__tests__/agent-live-run-routes.test.ts index a48c2dafca5e..d38533723669 100644 --- a/server/src/__tests__/agent-live-run-routes.test.ts +++ b/server/src/__tests__/agent-live-run-routes.test.ts @@ -610,6 +610,11 @@ describe("agent live run routes", () => { * log, and the `allowed` audit sat above that call — so a 404 that disclosed nothing was booked * as a read. Control: move `logRunLogAccessAudit(..., "allowed", ...)` back above `readLog` and * this fails (verified, not assumed). + * + * BLO-34901: no `result` matcher. The invariant is that this 404 records NOTHING — the reader is + * entitled, so booking it `denied` is equally false, and a matcher pinned to `"allowed"` passes + * that mutation unchanged. Second control (also verified): make the route write + * `logRunLogAccessAudit(..., "denied", ...)` on this path and this fails. */ it("does not audit an allowed run log read when the run stored no log", async () => { const app = await createApp(); @@ -624,7 +629,6 @@ describe("agent live run routes", () => { expect(res.status, JSON.stringify(res.body)).toBe(404); expect(mockLogActivity).not.toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ action: "heartbeat.run_log_accessed", - details: expect.objectContaining({ result: "allowed" }), })); }); @@ -743,6 +747,9 @@ describe("agent live run routes", () => { * `logStore: null` on the fixture rather than only rejecting `readLog`: that is the shape the * closure records, and it keeps the audit's own `logStore` field honest if the ordering ever * regresses. + * + * BLO-34901: no `result` matcher, same reasoning as the heartbeat guard above — and second + * control verified here too. */ it("does not audit an allowed workspace-operation log read when the operation stored no log", async () => { mockWorkspaceOperationService.getById.mockResolvedValue( @@ -762,7 +769,6 @@ describe("agent live run routes", () => { expect(res.status, JSON.stringify(res.body)).toBe(404); expect(mockLogActivity).not.toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ action: "workspace_operation.log_accessed", - details: expect.objectContaining({ result: "allowed" }), })); }); From eeee0861a3874fc3b69a654f49dfebb77e74d491 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Mon, 21 Sep 2026 03:40:20 +0000 Subject: [PATCH 2/2] test(audit): name the guards for the property they assert, and pin the exercised path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally review suggestions 1 and 2 at head 3ac92c81, both correct. 1. The titles still read "an allowed ... log read" while the assertions no longer pin `result`. That is the BLO-34901 defect with the sign flipped — a name narrower than the property asserted, and a live re-narrowing vector for a future reader reconciling the two. Renamed to "... at all". 2. With the `result` matcher gone, the absence assertion alone passes for any mutation that 404s BEFORE `readLog`, since those write no audit record either — the test would then exercise nothing. Added the symmetric positive `expect(readLog).toHaveBeenCalled()`, matching the idiom already used one test over on the cross-tenant case (`:585`, `:722`). Mutation controls, one site at a time, both pairs verified: C run-log route, `return` above `readLog` -> fails on the new positive C' same mutation, positive commented out -> PASSES (the hole) D workspace-op route, same mutation -> fails on the new positive D' same mutation, positive commented out -> PASSES (the hole) 35/35 pass at final state; `tsc --noEmit -p server/tsconfig.json` exit 0. Test-only; no change under server/src/routes/ or server/src/services/. BLO-34901 --- .../src/__tests__/agent-live-run-routes.test.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/server/src/__tests__/agent-live-run-routes.test.ts b/server/src/__tests__/agent-live-run-routes.test.ts index d38533723669..d40298c09bf6 100644 --- a/server/src/__tests__/agent-live-run-routes.test.ts +++ b/server/src/__tests__/agent-live-run-routes.test.ts @@ -615,8 +615,13 @@ describe("agent live run routes", () => { * entitled, so booking it `denied` is equally false, and a matcher pinned to `"allowed"` passes * that mutation unchanged. Second control (also verified): make the route write * `logRunLogAccessAudit(..., "denied", ...)` on this path and this fails. + * + * The `readLog` positive pins the path the absence assertion is about. Without it, any mutation + * that 404s BEFORE `readLog` writes no audit either, so the absence assertion passes while + * nothing is exercised — verified: a `return` above `readLog` fails this test, and fails nothing + * if the positive is removed. */ - it("does not audit an allowed run log read when the run stored no log", async () => { + it("does not audit a run log read at all when the run stored no log", async () => { const app = await createApp(); const { notFound } = await vi.importActual("../errors.js"); mockHeartbeatService.readLog.mockRejectedValue(notFound("Run log not found")); @@ -627,6 +632,7 @@ describe("agent live run routes", () => { ); expect(res.status, JSON.stringify(res.body)).toBe(404); + expect(mockHeartbeatService.readLog).toHaveBeenCalled(); expect(mockLogActivity).not.toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ action: "heartbeat.run_log_accessed", })); @@ -749,9 +755,11 @@ describe("agent live run routes", () => { * regresses. * * BLO-34901: no `result` matcher, same reasoning as the heartbeat guard above — and second - * control verified here too. + * control verified here too. The `readLog` positive pins the exercised path for the same reason, + * with its own verified mutation: a `return` above `readLog` fails this test only while that + * assertion is present. */ - it("does not audit an allowed workspace-operation log read when the operation stored no log", async () => { + it("does not audit a workspace-operation log read at all when the operation stored no log", async () => { mockWorkspaceOperationService.getById.mockResolvedValue( workspaceOperationLogFixture({ logStore: null, logRef: null }), ); @@ -767,6 +775,7 @@ describe("agent live run routes", () => { ); expect(res.status, JSON.stringify(res.body)).toBe(404); + expect(mockWorkspaceOperationService.readLog).toHaveBeenCalled(); expect(mockLogActivity).not.toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ action: "workspace_operation.log_accessed", }));