fix(server): redact agent secrets on mutating responses (BLO-18969)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent credentials live in adapter_config — plaintext env bindings and Bearer … values in mcpServers.*.headers
• redactAgentSecrets() scrubs those, but it was only wired into the read paths; every mutating route returned the agent row verbatim
• So a budget-only PATCH /api/agents/:id — touching no credential field at all — handed the caller the agent's entire credential set, and that response lands in agent transcripts and run logs, which are read far more widely than the secret store
• This pull request applies the same redaction to every response that serializes an agent, and closes the seam inside buildAgentDetail so new routes are safe by default
• The benefit is that credential material stops flowing into transcripts on ordinary admin operations

Linked Issues or Issue Description

Refs BLO-18969 (Paperclip-internal). No public GitHub issue.

Bug: GET /api/agents/:id returns "LINEAR_API_KEY": "***" and "Authorization": "***REDACTED***". PATCH /api/agents/:id returned the same object un-redacted, with live credentials inline as {"type":"plain","value":"<secret>"} and Bearer <token> in mcpServers.*.headers.

Related prior work (both merged, both read-path only): #640 "[codex] redact nested agent adapter secrets" and #642 "[codex] preserve nested redacted adapter values". This PR extends the same redaction to the write paths those two left uncovered. No open duplicate PR.

Found when a routine cap-adjustment pass issued 12 PATCH /api/agents/{id} calls carrying only {budgetMonthlyCents} and harvested ~9 credential categories from the responses. Rotation of the exposed material is tracked separately from this code fix.

What Changed

• buildAgentDetail() now redacts on the non-restricted branch, so the seam is closed by default rather than at each call site. The two GET call sites that wrapped it externally drop their now-redundant wrapper.
• Wrapped the routes that return a raw agent row: PATCH /agents/:id, POST /agents/:id/{pause,resume,clear-error,approve,terminate}, POST /agents/:id/config-revisions/:revisionId/rollback, POST /companies/:companyId/agents. PATCH /agents/:id/permissions is covered via buildAgentDetail.
• POST /companies/:companyId/agent-hires also redacts approval.payload — it embeds the requested adapterConfig twice (once directly, once under requestedConfigurationSnapshot), so redacting only agent would have left the same credentials on the wire one key over.
• Documented the invariant on redactAgentSecrets: every agent-serializing response must go through it, redactForRestrictedAgentView, or redactAgentConfiguration.
• Tests: new agent secret redaction on mutating responses block in server/src/__tests__/agent-secret-redaction.test.ts.
• Fixed that file's createDbStub to return a real thenable. Its then was vi.fn().mockResolvedValue(...), which returns a promise instead of invoking the awaiting continuation — any route that awaited a db.select()…where() hung for the full 60s test timeout rather than failing. No existing test reached that path.

secret_ref / user_secret_ref bindings are unaffected: redactEventPayload passes them through as pointers, so they never carry a resolved value on a response regardless of projectionClass.

Routes deliberately not changed, having been checked: PATCH /agents/:id/instructions-path, PATCH /agents/:id/instructions-bundle, PUT|DELETE /agents/:id/instructions-bundle/file return narrow payloads that never serialize adapterConfig; DELETE /agents/:id returns {ok: true}.

Verification

Fail-then-pass on the new tests, which is the point — they must not pass against unpatched master:

# with server/src/routes/agents.ts stashed (i.e. master behavior)
$ pnpm --filter @paperclipai/server exec vitest run src/__tests__/agent-secret-redaction.test.ts
  × PATCH /agents/:id redacts secrets on a budget-only patch
  × PATCH /agents/:id/permissions redacts secrets
  × POST /agents/:id/pause redacts secrets
  × POST /agents/:id/resume redacts secrets
  × POST /agents/:id/terminate redacts secrets
  × POST /agents/:id/config-revisions/:revisionId/rollback redacts secrets
  Tests  6 failed | 18 passed (24)

# with the fix
  Tests  24 passed (24)

Regression sweep over the agent route suites — 13 files, 182 tests, all passing:

$ pnpm --filter @paperclipai/server exec vitest run \
    src/__tests__/agent-permissions-routes.test.ts \
    src/__tests__/agent-secret-redaction.test.ts \
    src/__tests__/agent-hires-instructions-materialize.test.ts
  Test Files  3 passed (3)   Tests  81 passed (81)

$ pnpm --filter @paperclipai/server exec vitest run \
    src/__tests__/agent-adapter-validation-routes.test.ts \
    src/__tests__/agent-cross-tenant-authz-routes.test.ts \
    src/__tests__/agent-image-bump-route.test.ts \
    src/__tests__/agent-instructions-routes.test.ts \
    src/__tests__/agent-live-run-routes.test.ts \
    src/__tests__/agent-skills-routes.test.ts \
    src/__tests__/agents-pending-approval-config.test.ts \
    src/__tests__/agents-service-secret-bindings.test.ts \
    src/__tests__/built-in-agent-routes.test.ts \
    src/__tests__/agent-test-environment-routes.test.ts
  Test Files  10 passed (10)   Tests  101 passed (101)

$ pnpm --filter @paperclipai/server exec tsc --noEmit -p tsconfig.json
  (clean)

Response shape for a budget-only PATCH /api/agents/:id, before → after:

// before
"env": { "OPENAI_API_KEY": "sk-secret-key-12345", "DATABASE_URL": "postgres://user:pass@host/db" },
"headers": { "Authorization": "Bearer gbrain_at_secret_12345" }

// after
"env": { "OPENAI_API_KEY": "***", "DATABASE_URL": "***" },
"headers": { "Authorization": "***REDACTED***" }

Risks

Low, but two things a reviewer should weigh:

• Round-trip safety. redactAgentSecrets replaces env values with the "***" sentinel, so a UI read-edit-save could previously persist the sentinel over a live value. That hazard already existed on the GET path and is already handled by stripRedactedEnvBindingsFromAdapterConfig on the PATCH/POST ingress (BLO-5xxx). This PR widens which responses carry the sentinel; it does not change the ingress guard. Worth confirming no client depends on reading back a real credential from a mutation response — none in this repo does.
• Behavioral shift for API consumers. Any external caller that relied on a mutation response to echo real credential values will now see "***" / "***REDACTED***". That is the intended fix, not a regression, but it is a wire-visible change.
• No migration, no schema change, no UI change.

Model Used

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

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 have searched GitHub for duplicate or related PRs and linked them 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 change
• I have updated relevant documentation to reflect my changes — none applies; the invariant is documented in-code on redactAgentSecrets
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-18969

[allyblockcast]

🔗 Paperclip issue: BLO-18969

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/routes/agents.ts:2749 — Nested adapter env bindings can still leave this mutation response in plaintext. redactAgentSecrets() force-masks only the top-level agent.adapterConfig.env; the approval payload here, and runtimeConfig.modelProfiles.*.adapterConfig.env on the returned agent, go through redactEventPayload(). That generic sanitizer preserves { "type": "plain", "value": "..." } when the env variable name does not match its secret-key regex, so a credential stored under a valid ordinary name such as SIGNING_MATERIAL or FOO is echoed by the route, including the duplicate requestedConfigurationSnapshot copy. This leaves the exact transcript-exposure class the PR intends to close. Add a structural adapter-config redactor that masks every env value independent of its key, apply it recursively to runtime model-profile configs and both approval payload copies, and add success-path tests for the hire response using a non-secret-looking env key in canonical plain-binding form.

Strengths

• The changed raw-agent mutation paths consistently route through one redaction helper.
• The new tests assert successful status codes, preventing error responses from passing vacuous leak checks.
• Top-level env values and MCP authorization headers are covered clearly.

Recommended Action

1. Fix the nested env redaction gap before merge.

[kkroo]

@ally pushed CI cleanup fix in 39edd78.

Root cause: server 3/4 failed on a transient PostgreSQL 40P01 deadlock from cleanupHeartbeatTestState while truncating test tables. The helper now uses the existing transient DB retry wrapper for cleanup truncation, and the retry detector follows wrapped cause chains like DrizzleQueryError -> PostgresError.

Verification:

• pnpm exec vitest run server/src/__tests__/db-retry.test.ts --reporter=dot
• pnpm exec vitest run server/src/__tests__/plugin-agent-invoke-wake-fanout.test.ts --reporter=dot
• pnpm --filter @paperclipai/server typecheck
• git diff --check

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:ef5544a critical 1 — still-present — server/src/routes/agents.ts:2749 — The exact current head still serializes the hire approval through redactEventPayload(), while redactAgentSecrets() likewise sends runtimeConfig through that generic sanitizer at line 1845. redactEventPayload() preserves { "type": "plain", "value": "..." } under ordinary env names, so nested model-profile adapter configs and both approval payload copies can still expose plaintext.

Critical Issues (1)

• [prior:ef5544a critical 1; pr-review-toolkit + gstack/review + native-codex] server/src/routes/agents.ts:2749 — Nested adapter env bindings remain incompletely redacted. The route supports runtimeConfig.modelProfiles.*.adapterConfig.env, but only top-level agent.adapterConfig.env is force-masked; nested configs and the approval payload use key-name-based generic redaction. A valid ordinary env key such as SIGNING_MATERIAL or FOO with a plain binding is therefore echoed, including under requestedConfigurationSnapshot. Apply structural adapter-config redaction recursively to model-profile runtime configs and both approval payload copies, then add success-path tests using a non-secret-looking env key.

Strengths

• The CI cleanup change retries the transient TRUNCATE with bounded jittered backoff.
• Cause-chain traversal covers the Drizzle wrapper shape that hid PostgreSQL 40P01 errors.
• Mutation-route tests assert successful responses before checking for leaked credentials.

Recommended Action

1. Fix the carried-forward Critical nested env-redaction gap before merge.

[kkroo]

Merged current origin/master into the branch and resolved the cleanup helper conflict, preserving the transient cleanup retry with jitter.

Verification on the merge result:

• pnpm exec vitest run server/src/__tests__/agent-secret-redaction.test.ts server/src/__tests__/db-retry.test.ts server/src/__tests__/plugin-agent-invoke-wake-fanout.test.ts --reporter=dot
• pnpm --filter @paperclipai/server typecheck
• git diff --check

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:ef5544a critical 1 — still-present — server/src/routes/agents.ts:2786 — The exact current head still sends the hire approval payload through redactEventPayload(), while runtimeConfig uses the same generic sanitizer at lines 1845-1847. Unlike the structural masking applied to top-level adapterConfig.env, this can preserve plain bindings under ordinary nested env names.

Critical Issues (1)

• [prior:ef5544a critical 1; pr-review-toolkit + gstack/review + native-codex] server/src/routes/agents.ts:2786 — Nested adapter env bindings remain incompletely redacted. runtimeConfig.modelProfiles.*.adapterConfig.env and both approval-payload copies still rely on key-name-based generic redaction, so a valid ordinary env key such as SIGNING_MATERIAL or FOO can be echoed with its { "type": "plain", "value": "..." } value. Apply structural adapter-config redaction recursively to runtime model profiles and the approval payload, then add success-path tests using a non-secret-looking env key.

Strengths

• Raw-agent mutation responses consistently use the centralized redaction helper.
• Mutation-route tests assert successful status codes before checking for leaked credentials.
• The retry helper now recognizes PostgreSQL SQLSTATEs through the error cause chain, and cleanup adds jitter to reduce repeated deadlock collisions.

Recommended Action

1. Fix the carried-forward Critical nested env-redaction gap before merge.

[allyblockcast]

This merged (20:25Z, by @kkroo) while I was working the review — the Critical from Ally's last review at head 531036ef was still open at merge time, and it is a real leak, not a false positive.

Reproduced before fixing: four new tests fail against this PR's merged head and pass with the follow-up. The gap is that redaction decided what to mask from the key's name, so a {type:"plain",value} binding under an ordinary key kept its plaintext — runtimeConfig.modelProfiles.*.adapterConfig.env.SIGNING_MATERIAL, a bare FOO, and a value smuggled onto a secret_ref.

Follow-up: #844 — makes the agent-config redaction structural rather than key-name based, and extends it to both approval read paths (a hire_agent payload embeds adapterConfig, so payloads stored before the change were still leaking on read).

No action needed here; tracking on BLO-18969.