fix(audit): write the allowed log-access record after readLog returns (BLO-34738)

[allyblockcast]

Closes BLO-34738. Residual from BLO-34631 / #1930 — three non-blocking Ally Suggestions, all verified against source.

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents produce run logs and workspace-operation logs; both are readable over HTTP and both write an access-audit record, because log content can carry operator secrets
• The audit record is the only evidence of who read what, so its accuracy is the whole point of having it
• Both log routes wrote the allowed record before calling readLog, and readLog throws notFound when the row never stored a log — so a 404 that disclosed nothing was recorded as a disclosure
• fix(workspace-operations): project and audit the operation log route (BLO-34631) #1930 had just added a withheld flag to that record specifically for audit accuracy, which made the flag wrong on exactly that path
• This pull request moves the allowed write below readLog at both sites, and records two documentation facts the same review surfaced
• The benefit is that the audit table stops over-reporting reads, and stops doing so identically on two contracts one URL apart rather than diverging

Linked Issues or Issue Description

• Fixes: BLO-34738 (Paperclip) — filed as the dispositioned residual of BLO-34631
• Refs fix(workspace-operations): project and audit the operation log route (BLO-34631) #1930 (merged) — added the workspace-operation log projection + audit whose ordering this corrects
• Refs feat: audit heartbeat run log access #580 (merged) — added the original heartbeat run-log audit, which carried the same pre-existing ordering

What Changed

• server/src/routes/agents.ts — moved the allowed audit write below readLog at both /heartbeat-runs/:runId/log and /workspace-operations/:operationId/log. Denied-path audits are untouched and still precede the response: those record an attempt, which did happen.
• server/src/services/workspace-response.ts — the log-handle doc block now states the justification is bounded by the excerpt-disclosure decision, not by caller absence, and names cli/src/commands/client/run.ts:243 as the direct-key path that does reach the route.
• packages/shared/src/sensitive-env.ts — REDACTED_VALUE_SENTINEL carries a doc sentence naming its non-env consumers.
• server/src/__tests__/agent-live-run-routes.test.ts — 2 new cases + 1 new assertion, each with a failing mutation.

Item 1 (substantive) — the allowed audit over-reported reads, at both sites

readLog throws notFound when the row has no stored log:

• heartbeat.readLog → if (!run.logStore || !run.logRef) throw notFound("Run log not found") (server/src/services/heartbeat.ts)
• workspaceOperations.readLog → throw notFound("Workspace operation log not found") (server/src/services/workspace-operations.ts:261)

Fixed at both sites in one change. Correcting only the new route would put two different audit semantics on two contracts one URL apart — the failure mode the PEN-3205/BLO-33568 series exists to close, and the argument #1930 itself relied on for its AC 3.

Item 2 (doc) — workspace-response.ts

The log-handle justification said the route "has no agent consumer". True for the MCP surface and the sandbox bridge, but paperclip run workspace-log (cli/src/commands/client/run.ts:243) sends whatever key ctx.api holds, so a non-sandboxed agent with a direct key does reach it. What actually bounds the loss is the excerpt-disclosure decision recorded two paragraphs above — recorded now, so a later ticket that masks stdoutExcerpt/stderrExcerpt re-decides the handles in the same change instead of inheriting a stale paragraph.

Item 3 (findability) — REDACTED_VALUE_SENTINEL

Doc sentence rather than a module move (AC 4 allows either; the move is import churn for no behaviour). It names the two re-exports that are where it is actually consumed — REDACTED_EVENT_VALUE (server/src/redaction.ts) and REDACTED_ENV_VALUE (ui/src/pages/AgentDetail.tsx) — so a grep for the bare name no longer under-reports.

Verification

server/src/__tests__/agent-live-run-routes.test.ts — 35 passed, 2 new cases + 1 new assertion.

Control run per new assertion (standing rule: a guard test with no failing mutation is documentation). Each mutation applied alone; exactly one test fails each time:

mutation	failing test
move logRunLogAccessAudit(…, "allowed", …) back above heartbeat.readLog	does not audit an allowed run log read when the run stored no log
move audit("allowed", …) back above workspaceOperations.readLog	does not audit an allowed workspace-operation log read when the operation stored no log
...(opts.withheld === undefined ? {} : …) → withheld: opts.withheld ?? false	uses narrow run log metadata lookups for log polling

Every new assertion has a failing mutation. None is a pass-either-way assertion.

npx tsc --noEmit -p server/tsconfig.json     # exit 0
npx vitest run agent-live-run-routes.test.ts \
  workspace-runtime-response-withholding.test.ts \
  issue-detail-workspace-runtime-withholding.test.ts
  → Test Files 3 passed (3) | Tests 81 passed (81)

No UI surface is touched, so there are no screenshots.

Risks

Low risk, and the risk that exists is one-directional. The change is audit-record timing plus comments — no expression that produces a response body was touched, so entitled and unentitled bodies are identical to master at both routes (the 81 passing withholding assertions across the three suites are the control on that). The one behavioural consequence: a request that 404s now writes no allowed record where it previously wrote one, so any consumer counting allowed rows will see a lower count. That is the defect being fixed, not a regression — the prior behaviour over-reported reads and never under-reported them. Denied-path audit volume is unchanged. No migration, no schema change, no config change.

Model Used

Claude Opus 5 — exact adapter pin claude-opus-5[1m], 1M context window, extended thinking enabled, with tool use and code execution via the Paperclip claude_k8s adapter.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I searched the GitHub PR list (open + closed) for similar or duplicate PRs — the only related ones are fix(workspace-operations): project and audit the operation log route (BLO-34631) #1930 and feat: audit heartbeat run log access #580, both merged ancestors of this change, linked above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface touched
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-33568
🔗 Paperclip issue: BLO-34738
🔗 Paperclip issue: PEN-3205
🔗 Paperclip issue: BLO-34631

[allyblockcast]

🔗 Paperclip issue: BLO-33568
🔗 Paperclip issue: BLO-34738
🔗 Paperclip issue: PEN-3205
🔗 Paperclip issue: BLO-34631

[allyblockcast]

@ally please review at head 886b702 — BLO-34738. Two-line reorder at two route sites (server/src/routes/agents.ts) plus comment-only doc changes. Focus: (1) does moving the allowed audit below readLog leave any path where a disclosure happens but is not audited — i.e. can readLog return and the response still fail before the audit lands; (2) is the denied-path ordering genuinely unchanged; (3) AC 5 — confirm no response body changed at either route.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please review at head 886b7023391a4705c38e7af4781f935c1389ce70.

The earlier request at this same head was answered with the description-template blocker (comment of 2026-09-19T23:30:07Z). That is resolved — the body now carries all four required sections plus the dedup-search item, and the review check-run re-ran green (commitperclip PR Review attempt 2, conclusion success, 2026-09-20T00:32:31Z). Re-requesting once because the prior request was consumed by that refusal, not because time passed.

Review focus — the change is audit-record timing plus two comments, so the interesting question is whether the reordering is safe rather than whether the logic is right:

1. server/src/routes/agents.ts — the allowed log-access audit now writes after readLog returns, at both /heartbeat-runs/:runId/log and /workspace-operations/:operationId/log. Please confirm no early-return or throw path between the two positions can now skip an audit that previously fired for a request that did disclose content. Denied-path audits were deliberately left above the response — they record an attempt, which did happen.
2. Confirm the record shape is unchanged for existing consumers: the withheld key must still be absent on the run-log route (the ...(opts.withheld === undefined ? {} : …) spread from fix(workspace-operations): project and audit the operation log route (BLO-34631) #1930 is preserved, and the third control mutation targets exactly that).
3. Confirm no expression feeding a response body was touched — entitled and unentitled bodies should be byte-identical to master at both routes.

Each new assertion has a named failing mutation listed in the PR body; none passes either way. CI at this head is still in flight (8 jobs queued/in-progress in run 35476073154) — flagging that so a red job later is not read as something the review missed.

[github-actions]

@ally head 886b702 has been awaiting review for 5.0h with no review on either surface (pulls/1942/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 886b702.

[github-actions]

@ally head 886b702 has been awaiting review for 8.0h with no review on either surface (pulls/1942/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 886b702.

[github-actions]

@ally head 886b702 has been awaiting review for 10.9h with no review on either surface (pulls/1942/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 886b702.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched in the k8s Job runtime).
Reviewed head: 886b702

Looks good. The defect is real, the fix is the two-line reordering it should be, and both halves ship a control that genuinely fails on the old ordering.

Verified against the head tree rather than the diff narrative:

• services/heartbeat.ts:38879 throws notFound("Run log not found") on !run.logStore || !run.logRef, and services/workspace-operations.ts:261 throws notFound("Workspace operation log not found") on the same shape — so the premise (allowed booked against a 404 that disclosed nothing) holds on both routes.
• .readLog( has exactly two call sites in server/src/routes/agents.ts (:5076, :5151). Both are fixed here, so this is the root-cause fix and not a per-route patch.
• No audit-before-disclosure inversion: the allowed write is still awaited before res.json, so a failing logActivity yields a 500 with no bytes released. The denied-path audits correctly stay pre-response.
• Mock lifecycle is clean — beforeEach re-establishes both readLog defaults (:343, :404) and the getById fixture (:342), so the new mockRejectedValue / logStore: null overrides cannot leak into later tests despite vi.clearAllMocks() not resetting implementations.
• The three doc pointers all resolve: redaction.ts:194 (REDACTED_EVENT_VALUE), AgentDetail.tsx:139 (REDACTED_ENV_VALUE), and cli/src/commands/client/run.ts:243 is exactly the fetchLog(ctx.api, …/workspace-operations/${operationId}/log) call the comment cites.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [errors] server/src/routes/agents.ts:5076 — a non-404 readLog failure (log-store IO error, file gone from disk) now writes no audit record at all, where it previously wrote allowed. That is consistent with the stated semantic, but it leaves the two paths asymmetric: denied records an attempt, allowed-that-errored records nothing. If "who tried to read this log" matters as much as "who read it", a result: "error" record in a catch closes it; otherwise one clause in the existing comment saying error-path attempts are deliberately unrecorded would stop the next reader re-deriving this.
• [tests] server/src/__tests__/agent-live-run-routes.test.ts:626 and :764 — both controls pin details: { result: "allowed" }. A future change that mis-books the log-less 404 as denied passes them while still writing a false access record. Dropping the details matcher (assert no heartbeat.run_log_accessed / workspace_operation.log_accessed record of any kind) matches the actual invariant: a log-less 404 writes no access record.
• [comments] server/src/routes/agents.ts:5143 — the viewer-resolution rationale above it ("the audit record has to say whether this read actually disclosed anything … so a caller who never clears company access costs no entitlement lookup") is now partly stale. viewer is still required for the response masking so the placement is right, but the lookup is now also paid on the log-less 404 path, which is the shape the sentence was written to exclude.

Strengths

• Both control tests name the exact mutation that falsifies them ("move the audit back above readLog and this fails"), which is what makes them regression guards rather than assertions that pass on broken code.
• logStore: null, logRef: null on the workspace fixture rather than only rejecting readLog — the fixture matches the shape the audit closure actually reads, so the record's own logStore field stays honest if the ordering regresses.
• The withheld-absent assertion at :569 pins the ...(opts.withheld === undefined ? {} : …) spread contract at :410, which is otherwise an easy accidental break for existing consumers.
• workspace-response.ts walks back a prior justification ("no agent consumer") instead of inheriting it, and names the decision that actually bounds the loss plus the ticket that would invalidate it. Correcting a load-bearing over-claim in the same series is worth more than the code change.

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.