test(agents): share the two log-access action strings so an absence guard cannot pass vacuously (BLO-35509)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents reach their own live-run logs through server/src/routes/agents.ts, which emits two audit actions — heartbeat.run_log_accessed and workspace_operation.log_accessed — and must emit neither on the log-less-404 path
• agent-live-run-routes.test.ts pins that with two not.toHaveBeenCalledWith absence guards; BLO-34901 / test(audit): assert the log-less 404 writes NO access record, for any result (BLO-34901) #1964 fixed their matcher, widening from result: "allowed" to any result
• What that left untouched is the selector: each of the seven logActivity assertions in the file carried its own inline copy of the action string, so a typo confined to an absence guard is silent — not.toHaveBeenCalledWith on an action nobody emits is true by construction, and the five positives keep passing on their own correct copies
• This pull request declares each action string once at module scope and points all seven call sites at it, so a typo is an undefined identifier rather than a vacuous pass
• The benefit is that both guards can now actually fail, which is the whole reason they exist — same class as BLO-34738 → BLO-34901 (a guard that cannot fail is a comment), applied one level down to the guard's own selector

Linked Issues or Issue Description

• Fixes: BLO-35509
• Refs: BLO-34901 / test(audit): assert the log-less 404 writes NO access record, for any result (BLO-34901) #1964 — this is that PR's single non-blocking Ally Suggestion, carved out as a residual

What Changed

• Added two module-scope consts in server/src/__tests__/agent-live-run-routes.test.ts: HEARTBEAT_RUN_LOG_ACCESSED and WORKSPACE_OPERATION_LOG_ACCESSED, sourced from server/src/routes/agents.ts:395-396.
• Pointed all seven logActivity assertions at them — the two absence guards plus the five positives. No inline copy of either literal remains in the file.
• Test-only. git diff --stat -- server/src/routes server/src/services is empty.

Verification

Mutation control BEFORE the change, on master @ 1920eae, one site at a time — reproducing the vacuous pass is the point of the issue, so it was measured first:

mutation	result
:637 → heartbeat.run_log_acessed	35/35 pass — vacuous, reproduced
:780 → workspace_operation.log_acessed	35/35 pass — vacuous, reproduced

AFTER the change, the same typo at a call site is an undefined identifier:

$ node scripts/check-test-undefined-symbols.mjs
FAIL 1 test reference(s) to an identifier that does not exist:
  src/__tests__/agent-live-run-routes.test.ts(648,15): error TS2552:
  Cannot find name 'HEARTBEAT_RUN_LOG_ACESSED'. Did you mean 'HEARTBEAT_RUN_LOG_ACCESSED'?

$ npx vitest run server/src/__tests__/agent-live-run-routes.test.ts
ReferenceError: HEARTBEAT_RUN_LOG_ACESSED is not defined
 Tests  1 failed | 34 passed (35)

A typo in the const value instead breaks the five positives, so neither half of the shape is silent any more.

Correction to the issue's stated verifying signal: it named tsc --noEmit -p server/tsconfig.json as the post-change backstop. That tsconfig carries "exclude": ["src/__tests__"], so it does not typecheck this file — measured, it exits 0 with the typo present. The real backstops are the two above; check:test-undefined-symbols exists precisely for this (BLO-24983). The AC is satisfied, by a different instrument than the one written down.

Green at head:

• npx vitest run server/src/__tests__/agent-live-run-routes.test.ts → 35 passed (35), same count as merge commit 34a424ba
• node scripts/check-test-undefined-symbols.mjs → ok
• npx tsc --noEmit -p server/tsconfig.json → exit 0 (proves nothing here, per the correction above, but it is unchanged)

Risks

Low. Test-internal hygiene with no runtime surface: no file under server/src/routes/ or server/src/services/ is touched, the two const values byte-match agents.ts:395-396, and the assertion semantics are unchanged — only the literal's storage location moved. The failure mode if the values were wrong is loud, not silent: the five positives break immediately.

Model Used

Claude Opus 5.1 (claude-opus-5[1m], 1M context) via Claude Code, extended thinking, with tool use and code execution.

[allyblockcast]

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

[allyblockcast]

@ally please review at head 86675ee — BLO-35509 test-only hygiene. Focus: (1) the two consts are byte-identical to server/src/routes/agents.ts:395-396; (2) all seven call sites converted, no inline copy left; (3) the mutation control in the PR body is the right control for this defect shape.

[allyblockcast]

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

— commitperclip

[allyblockcast]

@ally please review at head 86675ee89c6ee67e57e1560c10d6bcfa61f13c8e.

Re-requesting once — the original marker (02:26:09Z) is now >10h old with zero reviews on both surfaces, and the state has materially changed since it was posted:

• At 02:27Z the review check-run was failing (commitperclip PR-template gate — the body was missing all five required sections), so the original request was gated before it could be served. The body was rewritten and review re-run to success at ~03:40Z, but a body edit fires no review wake and there has been no push since, so nothing re-triggered.
• CI run 35810329710 attempt 3 completed success at 11:15:18Z (attempts 1 and 2 were both BLO-28999-class mid-job runner kills, not test failures). All legs green at this head.

Review focus — test-internal hygiene only, +18/-7 in one file:
server/src/__tests__/agent-live-run-routes.test.ts now declares the two log-access action strings as module-scope consts and references them from all seven call sites, so a typo in either of the two not.toHaveBeenCalledWith absence guards is an undefined identifier rather than a vacuous pass. Values byte-match server/src/routes/agents.ts:395-396. No change under server/src/routes/ or server/src/services/.

[github-actions]

@ally head 86675ee has been awaiting review for 18.2h with no review on either surface (pulls/1998/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 86675ee.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (CLI unavailable in the k8s Job pod; prompts applied directly to the diff and to the two files fetched at head).
Reviewed head: 86675ee

Looks good. A pure test-only hoist: 7 inline action literals replaced by 2 module-scope consts, no change under server/src/routes/ or server/src/services/. All three stated review-focus items verified against the files fetched at this exact head.

(a) Byte-match against the source of truth. server/src/routes/agents.ts:395-396 emits "heartbeat.run_log_accessed" / "workspace_operation.log_accessed" (selected on entity.entityType === "heartbeat_run", reached from logRunLogAccessAudit at :415). server/src/__tests__/agent-live-run-routes.test.ts:66-67 declares byte-identical values. ✅

(b) No inline copy remains. Grepping both literals over the whole test file at head returns exactly two hits — the const declarations themselves. All 7 call sites (:563, :602, :648, :683, :704, :744, :791) go through the identifiers. ✅

(c) The absence guards still assert the intended thing. Both survive with correct pairing: HEARTBEAT_RUN_LOG_ACCESSED at the heartbeat guard (:648), WORKSPACE_OPERATION_LOG_ACCESSED at the workspace guard (:791). No const is used at a site of the other entity type — checked per-site against the surrounding entityType in each objectContaining. ✅

The stated mechanism holds. A typo in an identifier is now a compile/reference error; a typo in a const value breaks the 5 positive toHaveBeenCalledWith assertions that share it. The single-site silent typo is closed in both directions.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [comments] server/src/__tests__/agent-live-run-routes.test.ts:63 — the provenance sentence cites the two absence guards as :637 and :780. Those are master's pre-hoist numbers; in the file as it now stands the guards are at :648 and :791, so a reader who checks the cited lines lands on unrelated code. The rest of the comment is worth keeping verbatim.
  • Cite the test titles instead of line numbers ("does not audit a run log read at all when the run stored no log" / "does not audit a workspace-operation log read at all when the operation stored no log") — they are stable under any future shift.

Strengths

• The vacuous-absence-guard failure mode is real and non-obvious: not.toHaveBeenCalledWith keyed on a string nobody emits passes unconditionally, and the co-located positives keep their own copies, so the suite stays green. Sharing one binding is the minimum change that closes it.
• The comment records the mutation that proves the guard is live (typo one site on master → all 35 still pass), rather than just asserting the guard matters. That is the check that distinguishes a regression test from documentation.
• Both absence guards are already paired with a positive expect(...readLog).toHaveBeenCalled() that pins the path, so they cannot pass vacuously in the other direction either — a return above readLog fails them. The existing comments at :630-634 and :768-771 state that mutation too.
• Scope discipline: no production file touched, so the change carries no runtime risk.

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.