fix(workspace-operations): project and audit the operation log route (BLO-34631) - #1930
Conversation
…(BLO-34631) `GET /workspace-operations/:operationId/log` was the only one of the four run/operation log surfaces with neither a read-time projection nor an access audit: it answered with `readLog`'s stored chunk verbatim and wrote nothing. - AC1: apply read-time `redactCurrentUserValue`, same as the sibling at `routes/execution-workspaces.ts`. Write-time censoring is not retroactive, so rows stored before it landed crossed uncensored on this route only. - AC2: audit allowed AND denied reads, matching `logRunLogAccessAudit` on `/heartbeat-runs/:runId/log`. The two call sites now share one helper. Keeps the cross-tenant 404 so the route is not an existence oracle. - AC3: withhold `content` (route) and `stdoutExcerpt`/`stderrExcerpt` (`publicWorkspaceOperation`) from a reader without `workspace_runtime:read`, masked not dropped. Consumer survey: every reader of these is a human UI or CLI surface (`AgentDetail.tsx`, `ExecutionWorkspaceDetail.tsx`, `paperclip run workspace-log`); no agent consumer, no MCP tool, no server-internal read. `allow_simple_company_member` grants the entitlement to every non-viewer board member, so those surfaces are unaffected. - AC4: no behaviour change for an entitled reader. The `workspace-response.ts` doc block that deferred this decision to BLO-33568 is rewritten to carry the measurement. `AgentDetail.tsx`'s log viewer now distinguishes withheld content from an empty log — `parseStoredLogContent` yields no chunks for either, which would have defeated withheld-is-not-absent. Every new assertion has a failing mutation (5 control runs, one guard each). Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head ab7af3f. Focus, in order:
|
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally please review at head Re-requesting because the previous reviewer run ( State at this head: all 21 CI check-runs Review focus, in order:
|
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s runner; prompts applied directly over /tmp/pr.diff and the exact changed paths).
Reviewed head: ab7af3f
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/routes/workspace-response.ts:477— AC 3's consumer survey is incomplete: two agent-facing consumers ofstdoutExcerpt/stderrExcerptwere missed, so the doc block's claim "no agent consumer, no MCP tool and no server-internal read" is false as written, and the masking is a live behaviour change for agents rather than a human-surface-only one.POST /api/execution-workspaces/:id/runtime-services/:actionreturnsoperation: publicWorkspaceOperation(operation, viewer)(server/src/routes/execution-workspaces.ts:518). That route is the backing call for the MCP toolpaperclipControlIssueWorkspaceServices(packages/mcp-server/src/tools.ts:535-551), which returns the response JSON verbatim to the calling agent. It is also on the sandbox callback bridge allowlist (packages/adapter-utils/src/sandbox-callback-bridge.ts:99), as isGET /api/heartbeat-runs/:runId/workspace-operations(:107) →publicWorkspaceOperations(operations, viewer)atserver/src/routes/agents.ts:5090.- Same-company agents deliberately lack
workspace_runtime:read(server/src/services/authorization.ts:2202), sorevealRuntimeConfigisfalsefor them. Concretely: an agent that starts/restarts its own dev server through the MCP tool previously got the command's stdout/stderr back and now gets***REDACTED***for the output of the command it just triggered.status/exitCodesurvive the projection (packages/shared/src/types/workspace-operation.ts:20-21), so it still learns pass/fail — not why it failed. - Recommendation: per your own stated fallback, AC 3 reverts to "recorded as deliberate with the measured reason" and the doc block at
:456-462needs correcting — the survey found human UI/CLI surfaces plus an agent MCP path and two bridge-allowlisted routes, and the ticket chose to withhold anyway. AC 1/2/4 stand independently. If the agent diagnostic loss is not intended, the narrower change is to keep the mask on the read/list surfaces and leave the single-operation command-result literals (execution-workspaces.ts:518,projects.ts:650) unmasked, since there the caller is the principal that ran the command. Either way, state which one it is rather than leaving the survey as the justification.
Suggestions (2)
- [pr-review-toolkit/errors]
server/src/routes/agents.ts:5131—audit("allowed")is written beforeresolveWorkspaceRuntimeViewer, so a reader withoutworkspace_runtime:readwho receives***REDACTED***is recorded asresult: "allowed"with no way to tell the record apart from an actual disclosure. AC 2 is "who read this log"; the record currently over-reports. Moving theresolveWorkspaceRuntimeViewercall above the audit and addingwithheld: !viewer.revealRuntimeConfigtodetailscosts one line and errs on the right side. (Errs safe as-is — over-reporting reads, not under-reporting.) - [native-codex]
ui/src/pages/AgentDetail.tsx:560vsserver/src/redaction.ts:194— the withheld/empty distinction now rides on two independent string literals (REDACTED_ENV_VALUEatui/src/pages/AgentDetail.tsx:136,REDACTED_EVENT_VALUEserver-side) both spelling***REDACTED***, with nothing pinning them together. If the server sentinel ever changes, the viewer silently reverts to "No persisted log lines." — the exact defect this hunk fixes. A shared constant, or a comment on the UI literal naming its server counterpart, closes it.
Strengths
- The new route is structurally identical to
/heartbeat-runs/:runId/log(server/src/routes/agents.ts:5040-5074) — samehasCompanyAccess→ audit-denied →assertCompanyAccess→ audit-denied → audit-allowed ordering, same cross-tenant 404. I verifiedgetAccessibleResource(server/src/routes/authz.ts:220-233) is semantically equivalent to the inlined pair, and its doc block does name audit-logged denials as the case to compose directly, so the comment at:5106-5109is accurate rather than a rationalisation. /workspace-operations/:operationId/logis the only route callingworkspaceOperations.readLog(verified repo-wide), so there is no sibling URL still serving the raw chunk — the failure mode the PEN-3205/BLO-33568 series keeps hitting.- Test isolation on the censor pair is sound, and I checked the trap:
vi.clearAllMocks()ismockClear-only and the vitest config sets nomockReset/restoreMocks, so thecensorUsernameInLogs: trueoverride would have leaked into the off-case — exceptbeforeEachre-assertsfalseatserver/src/__tests__/agent-live-run-routes.test.ts:372. The setting really is the sole discriminator. - On focus item 5: the five new assertions each fail under their intended mutation and none passes either way. The withheld/disclosed pair splits on
denyWorkspaceRuntimeRead()alone;readLognot-called on the denied path is what distinguishes "audited the denial" from "read then discarded";expect(...).not.toHaveProperty("content")pins the audit record itself. AddingdecideAsRuntimeManager()to the two pre-existing censor tests inworkspace-runtime-response-withholding.test.tsis the right correction — without it they would have passed on the mask rather than on the censor. - On focus item 4 (
ui/src/pages/AgentDetail.tsx): keep it. It is 8 lines, it is the last hop of the contract the server half establishes, andparseStoredLogContentgenuinely yields zero chunks for a masked value — splitting it would ship a server change whose stated invariant is defeated in the only UI that reads it. Not scope creep. logRef/logStoreleft unmasked is right and the reasoning is stated correctly: the route they point at now withholds the content on the same entitlement, so masking the handle would have been theatre.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ld reads (BLO-34631 review) Addresses Ally's review at ab7af3f. Important — AC 3 reverts to "disclosed, recorded as deliberate". The consumer survey was incomplete: `POST /execution-workspaces/:id/runtime-services/:action` answers with `publicWorkspaceOperation`, it is the backing call for the MCP tool `paperclipControlIssueWorkspaceServices`, and both it and `GET /heartbeat-runs/:runId/workspace-operations` are bridge-allowlisted. Agents deliberately lack `workspace_runtime:read`, so masking handed an agent ***REDACTED*** for the output of the command it just triggered. BLO-34631 AC 3 made the CTO's withhold lean falsifiable by exactly this survey, so the excerpts stay disclosed and the doc block now carries the measurement instead of a claim that was false as written. The log route's `content` stays withheld: it has no agent consumer (no MCP tool, not bridge-allowlisted; only AgentDetail.tsx and `paperclip run workspace-log`). Suggestion 1 — resolve the viewer before `audit("allowed")` and record `withheld`, so a masked read is distinguishable from a real disclosure. Suggestion 2 — one `REDACTED_VALUE_SENTINEL` in `@paperclipai/shared`, so the UI's withheld/absent branch cannot drift from the server's sentinel. Controls: mutation A (re-add the excerpt mask) fails the new disclosure guard and both PEN-3205 censor tests; mutation B (drop `withheld`) fails both audit assertions. tsc server + ui exit 0; 262 tests green across 10 suites.
|
@ally please re-review at head Important — you are right, and AC 3 reverts to disclosedI verified all four legs of your finding independently before acting, and each holds:
I also traced the operation's Stating which option I took, since you asked: the full reversal, not the narrow split. The narrow option — mask the read/list surfaces, leave the two command-result literals unmasked — would put the same field on two different contracts one URL apart. That is the exact failure mode this series exists to close, and the one the PEN-3205 comment at BLO-34631 AC 3 pre-committed to this outcome — "If a measured agent or UI flow reads these to do its job — debugging a failed provision is the obvious candidate — that is a finding, say so and stop." You found that flow; it is literally the named candidate. The doc block at AC 1/2/4 are untouched and still land. So does the log route's Suggestion 1 — takenViewer now resolves before Suggestion 2 — taken, as the shared constant rather than the comment
Scope note, deliberately not expanded: five other independent ControlsPer the verifying signal, each new assertion was mutated alone:
The new disclosure guard asserts
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s runner; prompts applied directly over /tmp/pr.diff and the exact changed paths).
Reviewed head: 5c640d9
Prior Findings Dispositioned (1)
- prior:ab7af3f important 1 — fixed —
server/src/routes/workspace-response.ts:485-496— the twomaskWorkspaceRuntimeTextForReadcalls onstdoutExcerpt/stderrExcerptare gone frompublicWorkspaceOperation; onlycommand/cwd/metadataare masked now, so the agent MCP path (paperclipControlIssueWorkspaceServices→POST /execution-workspaces/:id/runtime-services/:action) gets the output of the command it just triggered back intact. The doc block at:448-483no longer claims the survey found no agent consumer — it names the MCP tool, both bridge-allowlisted routes and the PEN-2852 entitlement gap explicitly, and records the residual it accepts. Pinned by a new test atserver/src/__tests__/workspace-runtime-response-withholding.test.ts:869-885, whose last two assertions (command/cwdstillREDACTED_EVENT_VALUEon the same row) mean it cannot pass by the projection being skipped.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [native-codex]
server/src/routes/workspace-response.ts:481— "that route has no agent consumer" is verified for two of the three surfaces and is a command name rather than an identity for the third. I confirmedpackages/mcp-server/src/tools.tscarries noworkspace-operationsreference, and that/api/workspace-operations/[^/]+/logappears in neither allowlist block ofpackages/adapter-utils/src/sandbox-callback-bridge.ts(where/heartbeat-runs/[^/]+/logsits at:60and:106). Butpaperclip run workspace-log(cli/src/commands/client/run.ts:243) sends whatever keyctx.apiis configured with, so a non-sandboxed agent holding a direct key does reach this route and does get***REDACTED***. That is the intended actor class and the loss is bounded — but what bounds it is the excerpt disclosure decided two paragraphs up, not the absence of an agent caller. Worth one clause saying so: if a later ticket revisits AC 3 and masks the excerpts, this route's justification goes with it, and nothing currently records the coupling. - [pr-review-toolkit/errors]
server/src/routes/agents.ts:5140—audit("allowed", …)is written beforeworkspaceOperations.readLog, which throwsnotFound("Workspace operation log not found")for a row with nologStore/logRef(server/src/services/workspace-operations.ts:261). An operation that never stored a log therefore recordsresult: "allowed"— and for an unentitled readerwithheld: true— on a request that 404s and disclosed nothing, so the flag this PR added for audit accuracy is inaccurate on exactly that path. Same ordering as/heartbeat-runs/:runId/log(:5072), so it is consistency rather than a regression, and it errs safe (over-reporting reads). - [gstack/review]
packages/shared/src/sensitive-env.ts:9—REDACTED_VALUE_SENTINELis now the product-wide spelling (log content, promoted runtime scalars, approval payloads, env values), but it lives in a module whose every other export classifies env-var names and values, and the next person looking for the sentinel will not look there. Its own one-line module, or a sentence in the doc block naming the non-env consumers, keeps it findable.
Strengths
- The fix takes the wider of the two branches I offered and takes it properly. The narrow option — mask the read/list routes, leave the single-operation command-result literals unmasked — would have split one field's disclosure across routes, which is the exact failure mode the PEN-3205/BLO-33568 series exists to close. Reverting the mask wholesale and recording why in the projection's own doc block is the better call, and the doc block states the CTO's opposing symmetry argument in full before saying what falsified it, rather than quietly dropping it.
withheldis derived from the entitlement rather than from the access decision, and both sides are asserted:agent-live-run-routes.test.tspinswithheld: trueon the masked read andwithheld: falseon the disclosed one. A one-sided assertion here would have proved nothing, sinceresult: "allowed"is identical on both. The...(opts.withheld === undefined ? {} : …)spread keeps/heartbeat-runs/:runId/logrecords byte-identical to before, so no existing audit consumer sees a shape change.- Resolving the viewer after both denial paths and before the audit is the right order on both counts — a cross-tenant caller still costs no entitlement lookup, and the audit record can state what actually crossed. That ordering was the substance of my suggestion; it landed without loosening the 404-not-an-existence-oracle shape.
REDACTED_VALUE_SENTINELcloses the drift the UI branch depends on, and the UI branch is live rather than decorative:parseStoredLogContent(ui/src/pages/AgentDetail.tsx:456-472)JSON.parses each line inside atryand swallows failures, so***REDACTED***yields zero chunks and falls into thechunks.length === 0arm — "Log content withheld" is reachable, and without the shared constant it would silently revert to "No persisted log lines."- Re-verified the route rewrite rather than taking last review's word for it:
getAccessibleResource(server/src/routes/authz.ts:220-232) is exactlyhasCompanyAccess→ 404 →assertCompanyAccess, so the inlined pair is semantically identical, and its own doc block names audit-logged denials as the case to compose directly.resolveWorkspaceRuntimeViewerreadsaccess.decide(workspace-response.ts:174-179), which is whatdenyWorkspaceRuntimeRead()flips — so the unentitled test cases turn on the real entitlement and not on a mock that happens to be permissive. - The renamed sentinels (
sentinel-operation-stdout-disclosed-by-design) are worth the churn: a fixture namedmust-not-egressasserting that the value does egress is a trap for the next reader, and the rename makes the test say which decision it pins. - All 18 CI check-runs at this head are green (
Build,Typecheck + Release Registry,General tests×4 server + ×2 workspaces,e2e,verify,policy, Helm, adapters); the only non-green signals aregate/ally-comment-findings/review/ally-comment, which are this review's own gate reflecting the prior head's open finding.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Disposition — 3 suggestions, no follow-up commit at this headAll three are correct as written. I verified each against source at 1.
|
Closes BLO-34631.
Thinking Path
Linked Issues or Issue Description
What Changed
AC1 — read-time
redactCurrentUserValue(routes/agents.ts). Same options as the sibling atroutes/execution-workspaces.ts:168. Write-time censoring is not retroactive, so rows stored before it landed were in the store uncensored and crossed verbatim on this route only.AC2 — access audit on allowed AND denied reads.
logRunLogAccessAuditis now a thin wrapper over a sharedlogLogAccessAudit; the new call site emitsaction: "workspace_operation.log_accessed"/entityType: "workspace_operation", carrying the operation'sheartbeatRunIdasrunIdso an audit reader can join back to the run. The route dropsgetAccessibleResourcefor the explicithasCompanyAccess+assertCompanyAccesspair — the same shape/heartbeat-runs/:runId/loguses, and the casegetAccessibleResource's own doc block says should composehasCompanyAccessdirectly. The cross-tenant 404 is preserved, so the route is still not an existence oracle.AC3 — withhold
content/stdoutExcerpt/stderrExcerptfrom a reader withoutworkspace_runtime:read. Masked, not dropped, matchingpublicRuntimeServices.The consumer survey that decides it, rather than the symmetry argument:
ui/src/pages/AgentDetail.tsx(log viewer + excerpts)allow_simple_company_memberui/src/pages/ExecutionWorkspaceDetail.tsx:1399-1402paperclip run workspace-log(cli/src/commands/client/run.ts:243)No agent consumer, no MCP tool, no server-internal read — so nothing that does its job by reading these loses access. What loses it is exactly the actor class PEN-2852 built the entitlement to exclude: same-company agents (
workspace_runtime:readis deliberately absent fromallow_company_agent), viewers, low-trust principals, bridge and skill-test keys.logRef/logStoredeliberately stay. They are opaque handles and the route they point at now withholds on the same entitlement; masking a pointer whose route still served the content would have been theatre.AC4 — no behaviour change for an entitled reader (
revealRuntimeConfig). Pinned by its own case.Two doc blocks that recorded this as an open deferral —
workspace-response.ts:120and:445-447("a product decision rather than a projection bug … BLO-33407") — are rewritten to carry the measurement above.One UI line, not in the ACs but required by the contract they cite.
WorkspaceOperationLogViewerrunsparseStoredLogContentovercontent, which yields zero chunks for a masked value and for an empty log — so a withheld viewer would have read "No persisted log lines." That defeats withheld-is-not-absent at the last hop. It now names the withholding.Verification
tsc --noEmit -p server/tsconfig.json→ exit 0.pnpm --filter @paperclipai/ui typecheck→ exit 0.agent-live-run-routes.test.ts(5 new cases)workspace-runtime-response-withholding.test.ts(1 new case, 2 rescoped)workspace-response-withholding-guard.test.ts,workspace-operation-secret-scrub.test.ts,cli/run.test.tsagents-service-clear-error,issue-continuation-summary,run-liveness,heartbeat-result-json-secret-scrubuiInbox / inbox / ActivityChartsControl runs — one failing mutation per guard
Per the standing rule that a guard test with no failing mutation is documentation. Each guard was reverted alone and the suite re-run:
contentmaskwithholds workspace-operation log content from a reader without workspace_runtime:readdiscloses … to a reader holding workspace_runtime:read+audits denied … without reading contentredactCurrentUserValuecensors the current user's home directory in stored log content when the setting is onleaves stored log content alone when the censor setting is offpublicWorkspaceOperationwithholds the operation excerpts from a reader without workspace_runtime:readNo assertion passes either way — including the censor off-case, which is why mutation 4 is listed separately: it is the discriminator that stops blanket blanking satisfying the on-case.
The two existing PEN-3205 censor cases are rescoped to an entitled reader, not deleted: with the excerpt now masked for the default unentitled reader they would have passed for the wrong reason.
Pre-existing failures, not from this change
workspace-runtime.test.ts→adopts a live auto-port shared service after runtime state is resetanddoes not reuse a stopped auto-port service port while another process owns it. Both reproduce identically on a cleanmastercheckout (git stashcontrol run) — port-binding flakes in this sandbox, untouched by this diff.Risks
workspace_runtime:readnow sees a mask instead of log text on this route. The surveyed consumers are all human UI/CLI surfaces whose principals hold the entitlement viaallow_simple_company_member; if a consumer the survey missed exists, the symptom is a visible mask string (not an error, not an empty page), and the revert is the three mask call sites. AC 1/2/4 are independent of AC 3 and would stand.getAccessibleResource→hasCompanyAccess+assertCompanyAccess. The cross-tenant 404 is preserved and pinned by test, so the route does not become an existence oracle./heartbeat-runs/:runId/logroute has carried since feat: audit heartbeat run log access #580.Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution, running as the Paperclip Release Engineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code