fix(approvals): gate hire_agent embedded agent config on agent_config:read

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents are hired through a board approval queue: a hire_agent approval card carries the proposed hire's full adapterConfig / runtimeConfig, including its mcpServers upstreams
• Those configs are redacted before display, but the redaction is deliberately surgical — credential values are masked while scheme, principal, host, port and path of each mcpServers.*.url survive, so an operator can still diagnose a misconfigured agent
• GET /api/agents/{agentId} treats that surviving structure as sensitive and withholds it from any caller lacking agent_config:read. The approval read paths returned the same material under company_scope:read, which is auto-allowed to every same-company agent
• So a peer agent could not read another agent's config directly, but could read it off that agent's hire card — the gate existed on the path someone was thinking about and not on its sibling
• This pull request applies the same agent_config:read gate to the embedded configuration on all three approval read paths, without gating the card itself
• The benefit is that an agent's MCP upstream topology — which upstream a peer is pointed at and under which principal — stops being a reconnaissance surface available to every same-company agent, while the board approval queue keeps working unchanged

Linked Issues or Issue Description

Refs PEN-2777, PEN-2747, PEN-2370, PEN-2620 (internal tracker).

Problem. redactApprovalPayloadByType routes hire_agent through the same strict redactAgentConfigPayload the agent routes use, so credential values on the approval path are already masked (that part was fixed in #1561). What remained was a grant-scope difference, not a response-content one:

Path	Guard	Discloses
GET /api/agents/{agentId}	agent_config:read — redactForRestrictedAgentView blanks both configs otherwise	full adapterConfig, only if granted
GET /api/approvals/{id}, GET /api/companies/{companyId}/approvals on a hire_agent card	only company_scope:read, auto-allowed to every same-company agent (reason: "allow_company_agent")	same shape, ungated

Because masking keeps the config diagnosable, what leaks through the weaker gate is the topology: k8s-mcp-admin.internal vs k8s-mcp.internal is itself informative, and adapterConfig.mcpServers is exactly where an agent's k8s tier is set. It tells an agent which peer is worth targeting.

No credential value was retrieved or quoted while investigating, and the affected endpoints were not probed — the finding is from source.

What Changed

• server/src/redaction.ts — new withholdAgentConfigFromApprovalPayload(type, payload). For hire_agent payloads it walks the object and blanks every adapterConfig / runtimeConfig subtree to {} at any depth, returning the blanked paths as withheldFields. Depth-recursive on purpose: the payload already carries the pair twice (requestedConfigurationSnapshot.adapterConfig), and a future copy of the key would otherwise reopen the hole silently. Non-hire payloads pass through untouched.
  • Blanked to {} rather than ***REDACTED*** to match redactForRestrictedAgentView's existing restricted-agent shape, and to keep that sentinel meaning "a scanner blanked this" rather than "you may not see this".
  • This function is an authorization filter, distinct in kind from the redactors above it in the file, and the docstring says so.
• server/src/routes/authz.ts — new shared actorCanReadAgentConfig(req, access, companyId), mirroring actorCanReadConfigurationsForCompany in routes/agents.ts: board members of the company keep the read; agent actors must hold agent_config:read. Placed here rather than copied per module — a copy of this gate existing on one path and not its sibling is what the bug was.
• server/src/routes/approvals.ts — redactApprovalPayload / approvalResolutionResponse now take a required { includeAgentConfig } option, resolved per request from that gate. Required rather than defaulted so a future approval-serializing route has to state which side of the gate it is on instead of inheriting a permissive default.
• server/src/services/issue-approvals.ts + server/src/routes/issues.ts — the same gate on the third read path to this payload. listApprovalsForIssue is reached by GET /api/issues/{id}/approvals behind issue:read, also allow_company_agent. Found by grepping the importers of redactApprovalPayloadForDisplay rather than its call sites. It defaults to withholding, so a caller that never resolves the verdict discloses nothing.
• docs/api/approvals.md — documents the two-gate rule and the withheldFields response field.
• Tests: new server/src/__tests__/approval-agent-config-authz-routes.test.ts (route-level, all three actors), plus unit coverage in redaction.test.ts and service coverage in issue-approvals-service.test.ts.

What is deliberately not changed

• The write side. The snapshot is still stored via redactEventPayload and still replayed verbatim over the agent row by activatePendingApproval — which is what stops a pending agent tampering with its own config before the board sees it. This is a read projection only.
• Card visibility. hire_agent approvals stay listable and decidable under company_scope:read. Option 2 ("hold the whole card") would have risked the board queue; option 3 ("drop the config entirely") would have cost board readers a real capability.
• The existing credential masking, which is orthogonal and still asserted by the tests.

Audited and clean

Other consumers of the raw payload disclose no config: attention.ts reads only title/summary/recommendedAction; approval-gate-reconciler.ts reads payload->gate; plugin-host-services.ts getOrchestrationSummary projects ids/status; approvals.ts listSummary excludes payload entirely. The MCP tools (paperclipGetApproval, paperclipListApprovals, paperclipListIssueApprovals) are thin proxies over exactly the gated routes.

Verification

pnpm --filter @paperclipai/server exec vitest run \
  src/__tests__/approval-agent-config-authz-routes.test.ts \
  src/__tests__/issue-approvals-service.test.ts \
  src/__tests__/redaction.test.ts
pnpm -r typecheck

Results on this branch:

Run	Result
Tests, source reverted to parent 8db52f0 (tests kept)	3 files failed — 8 failed / 49 passed (57)
Tests, fix applied	3 files passed — 57 passed (57)
pnpm -r typecheck (all packages)	exit 0, 0 error TS

Fail-first is the point here, not a formality. The tests were run against the unmodified source before the fix and asserted red. PEN-2747 — the parent of this work — survived as long as it did because a test asserted the leak as expected output; a green-only run on this change would prove nothing. Both runs are on the same post-install toolchain (vitest 4.1.8), so the only variable is the five source files.

The failures are for the right reason, not a missing import:

FAIL src/__tests__/issue-approvals-service.test.ts:103
AssertionError: expected '{"name":"Worker","adapterConfig":{"mc…'
  not to contain 'k8s-mcp-admin.internal'
Received: {"name":"Worker","adapterConfig":{"mcpServers":{"k8s":
  {"url":"https://svc@k8s-mcp-admin.internal:8443/mcp"}}},
  "requestedConfigurationSnapshot":{"adapterType":"claude_k8s",
  "adapterConfig":{"mcpServers":{"k8s":{"url":"https://svc@…"}}}}}

That is the disclosure itself, asserted as red on the parent commit — the upstream host surviving into the response on the ungated path, in both the top-level config and the nested snapshot.

Coverage asserted:

• Agent without agent_config:read: GET /approvals/:id and the company list both return the card with adapterConfig, runtimeConfig, and both requestedConfigurationSnapshot.* subtrees = {}; the serialized body does not contain the upstream host anywhere; name, adapterType and status still readable.
• Agent with agent_config:read: config present, withheldFields: [], and the pre-existing credential masking still applied.
• Board reader: list, detail, and POST /approvals/:id/approve all still round-trip the full payload — the queue is verified whole, not assumed.
• Non-hire_agent payloads pass through byte-identical.
• GET /issues/:id/approvals withholds by default at the service layer.

Risks

Low-to-moderate, and confined to the read projection.

• Behavioural shift for ungated agent callers. An agent without agent_config:read that today reads adapterConfig off an approval card will now get {}. That is the point of the change, but it is a response-shape change for existing callers. withheldFields is added so such a caller can tell "withheld" from "absent" rather than silently mis-reading an empty config as no config.
• The requester's own card is also withheld. Checked, and it costs no capability: approval reads were already lossy (env values masked), so a read-modify-write resubmit was never viable, and resubmit takes its payload from the caller.
• POST /companies/:id/approvals responses are gated too, so a creating agent lacking the grant does not get its own submitted config echoed back. Harmless — it supplied the payload.
• No migration, no schema change, no write-path change. Fully revertable by reverting this commit; nothing persists in the new shape.
• Possible textual conflict with fix(approvals): authorize issueIds on approval create (BLO-23763) #1271 (BLO-23763), which touches routes/approvals.ts and routes/issues.ts for an unrelated concern (authorizing issueIds on create). Different hunks; conflict only if it lands first.

Model Used

Claude Opus 5 (claude-opus-5, 1M context), extended thinking, with tool use (repository read/edit, shell, GitHub) 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, API-only
• 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: PEN-2777
🔗 Paperclip issue: PEN-2620
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: BLO-23763
🔗 Paperclip issue: PEN-2747

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [native-codex] server/src/redaction.ts:701 — Consider adding a focused regression case for non-object adapterConfig/runtimeConfig values if those shapes can enter persisted hire payloads; the current withholding intentionally only replaces object-shaped subtrees.

Strengths

• The authorization verdict is centralized and threaded through all three approval serialization paths, including issue-linked approvals.
• Withholding is applied recursively, covers duplicated configuration snapshots, and remains separate from credential redaction and the write path.
• Route-level tests cover restricted agents, granted agents, board readers, and non-hire payloads.

Recommended Action

1. Consider the Suggestion opportunistically.

[allyblockcast]

Addressed in 0b1e4c5 — the suggestion found a real hole, not just a test gap

server/src/redaction.ts:701 — Consider adding a focused regression case for non-object adapterConfig/runtimeConfig values if those shapes can enter persisted hire payloads; the current withholding intentionally only replaces object-shaped subtrees.

I chased the conditional rather than assuming it, and the answer is yes, they can — so the object-only guard wasn't a deliberate narrowing, it was a bypass of the gate this PR adds. Thanks for the pull; a test alone would have locked in the bug.

Why the shape is reachable. Three write paths build hire_agent payloads. Only one guarantees an object:

Path	Shape guarantee
POST /agents/:id/hire (routes/agents.ts:2949-2956)	object — assigns redactEventPayload(...) ?? {}
POST /companies/:companyId/approvals (routes/approvals.ts:417)	none
resubmit (routes/approvals.ts:638)	none

approvalPayloadSchema is z.object({ title, gate? }).catchall(z.unknown()) (packages/shared/src/validators/approval.ts) — adapterConfig is unconstrained. And normalizeHireApprovalPayloadForPersistence (services/secrets.ts:3873) only normalizes when asRecord(payload.adapterConfig) is truthy; a non-record rides through in the spread untouched.

What it cost. isPlainObject excludes arrays (redaction.ts:234), so an array-shaped config wasn't blanked — the walk recursed into it, where the keys are mcpServers/url and never adapterConfig, so nothing matched. Probed against b85d8f9:

adapterConfig: [{ mcpServers: { k8s: { url: "https://svc-account@k8s-mcp-admin.internal:8443/mcp" } } }]
→ {"adapterConfig":[{"mcpServers":{"k8s":{"url":"https://svc-account@k8s-mcp-admin.internal:8443/mcp"}}}]}
   withheldFields: []

Full topology to every company_scope:read reader — and withheldFields: [], so the response did not even admit to withholding nothing. A JSON-encoded string under the same key leaked identically; my old comment's claim that "a string keeps its meaning without disclosing anything" was simply wrong.

Fix. Withholding is now shape-independent: any value present under adapterConfig/runtimeConfig is blanked, at any depth. null/undefined stay readable — they carry no topology, and blanking them would cost the board queue the distinction between "you may not see this" and "no config was requested".

Tests (both fail against b85d8f9, pass here, per the ticket's rule that the assertion must fail against current code first):

• redaction.test.ts — array + JSON-string shapes blanked with correct withheldFields; null left readable and not reported withheld.
• approval-agent-config-authz-routes.test.ts — same shape end-to-end through GET /approvals/:id, so the gate is proven at the route, not just at the helper.

Verified: tsc --noEmit clean; 183 passed across all 10 approval + redaction suites. docs/api/approvals.md updated — it said "subtree", which implied the object-only behaviour.

Unchanged, as before: the write-side snapshot and the activatePendingApproval replay.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The authorization decision is shared and applied consistently to detail, company-list, and issue-linked approval projections.
• Recursive withholding covers duplicated snapshots and non-object config values while preserving the existing credential redaction layer.
• Route and service tests cover ungranted, granted, and board actors as well as the third issue-linked read path.

Recommended Action

1. No blocking changes requested.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The new authorization decision is centralized and threaded through all approval serialization paths, including issue-linked approvals.
• Configuration withholding recursively handles nested copies and unusual persisted value shapes while preserving board access and existing credential redaction.
• Route, service, and redaction tests cover denied agents, granted agents, board readers, nested snapshots, non-object values, and non-hire payloads.

Recommended Action

1. No Critical or Important changes required from this review.
2. Consider the remaining CI checks as they complete.

[allyblockcast]

Ally's suggestion on b85d8f96 (regression case for non-object adapterConfig/runtimeConfig) was addressed in 0b1e4c55, and re-reviewed clean there — recording the two follow-ups since, so the merge actor does not have to reconstruct them.

The suggestion was load-bearing, so the fix went past it. Those shapes can reach a persisted hire payload: approvalPayloadSchema is a .catchall(z.unknown()) and normalizeHireApprovalPayloadForPersistence passes a non-record through untouched, so the generic create and resubmit routes store whatever the filer sent. Adding only a regression case would have documented the gap; isPlainObject(entry) → entry !== null && entry !== undefined removes it, so the filer's chosen shape can no longer pick the authorization outcome. That is the same defect class as the original bug — a gate present on one spelling and absent on its sibling — which is why it was worth closing rather than testing around. null/undefined stay readable on purpose: they carry no topology, and blanking them would cost the board queue the distinction between "withheld" and "no config requested".

Branch brought up to date (0b1e4c55 → e28a2274). It was 11 behind, which blocked merge under the up-to-date requirement. Before updating I checked the upstream commits against this gate's surface, because MERGEABLE only rules out a textual conflict — a new approval-serializing route landing upstream would sit outside the gate with no conflict marker anywhere. None of the 11 touch redaction.ts, routes/approvals.ts, routes/issues.ts, services/issue-approvals.ts or routes/authz.ts. The one new agent-facing route (GET /agents/me/recovery-actions, PEN-2756) uses explicit .select({...}) projections with no config fields, so it discloses nothing this gate would need to cover.

Verified post-merge that the merge preserved the fix rather than assuming it: withholdAgentConfigFromApprovalPayload and the shape-independent predicate at redaction.ts:702/:725, actorCanReadAgentConfig at authz.ts:187.

State: CI green on 0b1e4c55 and on e28a2274 (the two skipping rows are non-blocking skips, not failures); review/ally-comment passing; now 0 behind, mergeStateStatus: CLEAN.

Ready for a human merge — not self-merging or self-approving.

[kkroo]

Approved exact current head e28a227 after Ally reported 0 Critical and 0 Important findings. All checks are green; merge through the protected queue.