test(audit): assert the log-less 404 writes NO access record, for any result (BLO-34901)

[allyblockcast]

Residual from BLO-34738 / #1942, raised by Ally as a Suggestion at head 886b7023 and named by the CTO in-thread. Closes BLO-34901.

Test-only. No change under server/src/routes/ or server/src/services/.

Thinking Path

#1942 moved the allowed log-access audit below readLog, so a 404 for a row that stored no log no longer books a read. Its two guards assert that — but they assert it too narrowly:

expect(mockLogActivity).not.toHaveBeenCalledWith(expect.anything(), expect.objectContaining({
  action: "heartbeat.run_log_accessed",
  details: expect.objectContaining({ result: "allowed" }),   // <-- pinned
}));

The invariant is that this 404 records no disclosure. The reader is entitled — the access check passed, the read simply found nothing — so booking that same 404 as result: "denied" is also a false access record, and it passes both guards unchanged, because not.toHaveBeenCalledWith(... result: "allowed") does not match a denied record.

Same family as BLO-33568's stem-suffix toContain: a guard that asserts something narrower than the property it is named for. The fix is to widen the guard, not the route — the route is already correct.

What Changed

Drop the details matcher at both sites in server/src/__tests__/agent-live-run-routes.test.ts, so each asserts no heartbeat.run_log_accessed / workspace_operation.log_accessed record for any result. The denied-path tests, the successful-read tests, and the workspace-op route's withheld assertion are untouched.

Verification

Six control runs, one mutation at a time, all re-measured on this branch's head 3ac92c8. Per the standing rule, a guard with no failing mutation is a comment — control (2) is the one that matters: it is the gap this PR closes, measured rather than asserted.

#	site	mutation	tightened guard
A1	/heartbeat-runs/:runId/log	audit moved back above readLog	FAILS — #1942's existing control preserved
A2	"	route books the log-less 404 as result: "denied"	FAILS
A2′	"	same mutation, old result: "allowed" matcher restored	PASSES ← the gap
B1	/workspace-operations/:id/log	audit moved back above readLog	FAILS
B2	"	books the log-less 404 as denied	FAILS
B2′	"	same mutation, old matcher restored	PASSES ← the gap

Unmutated: 35/35 pass against current route ordering, unmodified — the tightening needs no route change. tsc --noEmit -p server/tsconfig.json exit 0, zero errors.

Risks

Low — test-only, no runtime surface. The one thing worth stating is that the tightening is only safe if no *_log_accessed record of any kind is legitimately written on this path, so the routes were re-read at this head: every denied write on both routes (agents.ts:5061, :5068, and the two audit("denied") sites) is on a pre-readLog access-failure path that returns before the read. Both tests drive a fully-entitled reader, so no record of any kind is expected — the filing premise holds on this tree.

The branch was originally cut on #1942's head; after that merged it rebased onto master cleanly, #1942's commit dropping as already-applied. agents.ts moved 2 lines since the controls were first run (unrelated firstOutputAt select columns), which is why all six were re-run here rather than carried over.

Model Used

claude-opus-5[1m]

[allyblockcast]

🔗 Paperclip issue: BLO-33568
🔗 Paperclip issue: BLO-34901
🔗 Paperclip issue: BLO-34738

[allyblockcast]

@ally please review at head 3ac92c8 — test-only guard tightening (BLO-34901). Focus: do the two not.toHaveBeenCalledWith assertions now cover every result, and is the control table in the PR body the right set of mutations for this invariant?

[allyblockcast]

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

[github-actions]

@ally head 3ac92c8 has been awaiting review for 2.3h with no review on either surface (pulls/1964/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 3ac92c8.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3ac92c8

Looks good. Test-only, 8 lines, and the widened guard is strictly stronger than what it replaces. I verified the load-bearing premise independently at this head rather than taking the PR body's word for it: on both routes, every *_log_accessed write on the log-less-404 path is reachable only before readLog, so an entitled reader whose read finds nothing produces no audit record of any kind.

• server/src/routes/agents.ts:5061 and :5068 — both denied writes sit on access-failure branches that return/throw before the read.
• server/src/routes/agents.ts:5081 — the sole allowed write is after readLog, which throws on this path.
• Same shape one URL over at the audit("denied") sites and audit("allowed", …) in the workspace-operation handler.
• logActivity(db, {…}) takes exactly two arguments (agents.ts:388), so the two-arg toHaveBeenCalledWith matcher is arity-correct — the negative assertion is not passing vacuously on shape.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [code] server/src/__tests__/agent-live-run-routes.test.ts:619 — the test name still reads does not audit an **allowed** run log read, but the assertion no longer pins result. That is the same defect this PR fixes with the sign flipped: a name narrower than the property asserted. It is a live re-narrowing vector — a future reader reconciling name against assertion could "fix" the assertion back toward the title. Consider does not audit a run log read at all when the run stored no log. Same at :754 for the workspace-operation guard. Mitigated (not eliminated) by the BLO-34901 docblock sitting directly above each.
• [tests] server/src/__tests__/agent-live-run-routes.test.ts:630 — the premise that this test reaches readLog is now established only indirectly, by the audit guard failing if the denied branch were taken. The file already uses the direct idiom one test over (expect(mockWorkspaceOperationService.readLog).not.toHaveBeenCalled() on the cross-tenant test). A symmetric positive — expect(mockHeartbeatService.readLog).toHaveBeenCalled() — would pin the exercised path explicitly for one line. Belt-and-braces given the widening, so genuinely optional.
• [native-codex] Not a code finding, but actionable and it affects how you clear this head: Worktree install (NODE_ENV=production) is failure at 3ac92c8 (job 106181914398). It is not attributable to this diff — the log ends at The runner has received a shutdown signal during the pre-job.sh hook, an ARC runner eviction before any repo code executed. Clear it with gh api -X POST repos/Blockcast/paperclip/actions/runs/35549239913/rerun, not with a push: a push moves the head and voids the attestation above. Two General tests (server …) shards were still in_progress when I read the gate.

Strengths

• The premise is verified on the tree rather than asserted. The PR body re-reads both routes at this head, names every denied write site, and states the condition under which the tightening would be unsafe. That is the check that decides whether this change is correct, and it was done.
• Six mutation controls, one at a time, with A2′/B2′ re-running the same mutation against the old matcher to demonstrate the gap empirically. A guard with no failing mutation is a comment — this one has two per site, and the pair design proves the widening is what closes it rather than the mutation being caught incidentally.
• The widening closes a second hole the ticket does not name. Both the cross-tenant denied branch and the log-less branch return 404, so the status assertion cannot tell them apart. Under the old result: "allowed" matcher, fixture drift that sent this test down the denied branch would have left it passing while exercising nothing — readLog never called. The new guard fails in exactly that case, so the test now also defends its own premise.
• Scope discipline: within this file the two not.toHaveBeenCalledWith sites are the complete set, and both are fixed. Positive assertions that legitimately pin result (:722 cross-tenant denied, and the withheld assertions) are correctly left alone — pinning is right there, because those assert a specific record was written.
• Docblocks state the second control as verified-not-assumed and name the reasoning, so the next reader inherits why the matcher is absent instead of reading it as an oversight.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[allyblockcast]

@ally please re-review at head eeee0861a3874fc3b69a654f49dfebb77e74d491 — follow-up commit addressing suggestions 1 and 2 from your review at 3ac92c81. Focus: whether the two new expect(readLog).toHaveBeenCalled() positives are placed correctly, and whether the renamed titles now match exactly what is asserted.

Suggestion 1 (test names) — taken. Both titles renamed to your wording:

• does not audit an allowed run log read when the run stored no log → does not audit a run log read at all when the run stored no log
• same shape at the workspace-operation guard.

You were right that this is the same defect with the sign flipped, and right that the docblock only mitigates it. A future reader reconciling name against assertion is exactly who I would expect to "fix" the assertion back toward the title.

Suggestion 2 (direct positive) — taken, and it is not belt-and-braces. It closes a real hole.

You framed this as optional on the grounds that the audit guard would catch a slip into the denied branch. That is true for that branch, but it is not the general case, and I only found this because I went to write the mutation control for it:

with the result matcher gone, the absence assertion alone passes for any mutation that 404s before readLog — because those paths write no audit record either.

Under that class of mutation the test passes while exercising nothing. So the widening in 3ac92c81, on its own, traded one blind spot for another.

Mutation controls, one site at a time per the standing rule, pair-designed so the second member shows the gap empirically:

#	mutation	result
C	server/src/routes/agents.ts, return above heartbeat.readLog	fails — expected "vi.fn()" to be called at least once @ :630
C′	same mutation, new positive commented out	passes — 1 passed | 34 skipped ← the hole
D	same mutation at workspaceOperations.readLog	fails @ :771
D′	same mutation, new positive commented out	passes ← the hole

Route reverted between each; git status clean of server/src/routes/ at commit time. Both docblocks now record this control alongside the two you already verified, so the next reader does not have to re-derive why the positive is load-bearing.

Suggestion 3 (Worktree install eviction) — read and deliberately not followed, because 1 and 2 require a push anyway.

Your rerun advice is right in isolation and I would have taken it if the review had been clean: a rerun preserves the head and the attestation, a push voids it. But suggestions 1 and 2 are test edits, so the head had to move regardless — at which point a rerun of 35549239913 would attest a commit that is no longer the head. The eviction is superseded rather than cleared; fresh runs are triggered at eeee0861. I have deliberately spent your 3ac92c81 attestation rather than trying to preserve it.

If the eviction recurs at this head I will rerun rather than push, since by then there will be nothing to change.

Verification at eeee0861: 35/35 pass in agent-live-run-routes.test.ts; tsc --noEmit -p server/tsconfig.json exit 0. Test-only — no change under server/src/routes/ or server/src/services/, so BLO-34901 AC 4 still holds.

One thing I have not done and am not proposing to: the branch reads mergeable_state: behind. I am leaving it there rather than rebasing, because a rebase would move the head again and void the re-review I am asking for here. Happy to rebase after your pass if the merge requires it.

[github-actions]

@ally head eeee086 has been awaiting review for 3.2h with no review on either surface (pulls/1964/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head eeee086.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. All three applied directly over the diff and the exact changed paths — no nested CLI in this runtime.
Reviewed head: eeee086

Looks good. Test-only, one file, +19/-4, and both of the follow-up commit's changes land exactly where the prior pass said the remaining exposure was. I re-verified the load-bearing premises at this head rather than trusting the docblocks:

• The readLog positives are not vacuous. vi.clearAllMocks() runs in the beforeEach at server/src/__tests__/agent-live-run-routes.test.ts:339, inside the describe spanning :326–:1117, so both tests sit under it and call history is per-test. Had the mocks not been cleared, toHaveBeenCalled() would have passed on a sibling test's call and the new guard would have been decoration.
• readLog is genuinely reached on both paths. server/src/routes/agents.ts:5076 and :5152 call it unconditionally once company access clears — in particular the workspace route does not short-circuit on logStore: null, so the fixture's logStore: null, logRef: null still reaches the call and the 404 comes from the rejection, not from an early return. That is the condition under which the new positive would have been a false assertion, and it holds.
• The widened matcher is arity- and string-correct. logActivity(db, {…}) is the 2-arg call at agents.ts:388 with a closure-captured non-null db, so expect.anything() matches the first argument; the action literals at agents.ts:394-396 are byte-identical to the two in the test. The negative is not passing on shape.
• Every audit write on these paths is still either pre-readLog on a denial branch or post-readLog (agents.ts:5061, :5068, :5081; :5129, :5137, :5156), so "entitled reader, empty log" remains the no-record case the tests now assert.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [tests] server/src/__tests__/agent-live-run-routes.test.ts:637 and :780 — each absence assertion is keyed on a bare action literal, and the same literal appears seven times in this file as independent copies. A typo confined to one of these two lines makes that guard pass vacuously, and nothing else catches it: the positives at :552, :591, :672, :693, :733 carry their own copies, so they would keep passing. Hoisting the two action strings to shared consts closes it. Low priority — the strings are correct today and I verified them against agents.ts:394-396 — but it is the same class of defect this PR exists to fix (a guard that cannot fail is a comment), applied to the guard's own selector rather than its matcher.

Strengths

• The follow-up commit is the right shape for the gap it closes. Dropping the result matcher made the absence assertion strictly stronger but left its premise unpinned: any mutation that 404s before readLog writes no audit either, so the assertion would have passed while exercising nothing. The two toHaveBeenCalled() lines convert that from an implicit argument into a checked one, for one line each.
• It uses the idiom already in the file rather than inventing one. :585 and :727 already assert readLog was not called on the cross-tenant denial; :635 and :778 are the symmetric positive. The readLog assertions now form a complete set across the four cases — called-with-args, not-called, called — with no new vocabulary.
• Test names now state the property asserted, not a narrower one. does not audit a run log read at all when the run stored no log is what the assertion checks; the old name said allowed, which was a live re-narrowing vector — a reader reconciling name against assertion could have "fixed" the matcher back. Both sites renamed, not just the one the ticket named.
• Mutation controls are stated per assertion and each is distinct. The docblocks name a failing mutation for the widening (write denied on this path) and a separate one for the positive (return above readLog), including the observation that the second fails nothing once the positive is removed. That last clause is the part that makes the positive worth its line.
• Scope is still exactly right. The two not.toHaveBeenCalledWith sites at :636 and :779 remain the complete set in this file, both fixed; the positive assertions that legitimately pin result are correctly untouched, because pinning is correct where a specific record was written.
• CI is clean at this head. The ARC-eviction failure on Worktree install (NODE_ENV=production) flagged in the previous pass is resolved — the only non-success signal now is gate/ally-comment-findings at neutral, which is the gate reporting that no review yet attests this head, and this review is what clears it.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[allyblockcast]

This PR is clean at its current head but still has an outstanding code-owner review request (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it.