security(skills): withhold agent config from skill test-run snapshots (PEN-2839)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Every agent record carries adapterConfig, and adapterConfig.env holds provider base URLs and API keys — so any read path that reaches an agent record is a credential-disclosure surface
• GET /agents/:id already recognises this: it hands the material only to a caller holding agent_config:read, redacting via redactForRestrictedAgentView
• But that redaction lives at the route layer, so agents.getById returns the raw row to any service-layer caller — and services/company-skills.ts imports nothing at all from redaction.ts
• So snapshotAgentConfig copied the whole adapterConfig/runtimeConfig pair into the agent_config_snapshot jsonb column, and GET …/test-runs[/:runId] served it back under assertCompanyAccess, which admits any same-company agent actor
• This pull request withholds the pair on read and redacts it on write, extracting the existing withhold walk instead of copying it
• The benefit is that the third recurrence of one entitlement bug is closed at both ends, and the shared walk means the next sibling path inherits the fix rather than re-deriving it

Linked Issues or Issue Description

• Refs PEN-2839 — this door
• Refs PEN-2370 (ask 3, criterion b2) — the parent series
• Refs PEN-2777 (fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574), Refs PEN-2778 (security(portability): redact agent credentials on the company export path (PEN-2778) #1578) — doors #8b and ops(opencode-k8s): bump to context-overflow auto-compact + gbrain MCP baseline #9, the sibling paths
• Related PRs searched and reviewed: fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574, security(portability): redact agent credentials on the company export path (PEN-2778) #1578, security(mcp-gateway): give response bodies exactly one exit, and enforce it (PEN-2370) #1567, security(mcp-gateway): let a grant enumerate the tools it exposes (PEN-2735) #1573, security(redaction): mask credential-bearing URLs, args and headers in agent config (PEN-2747) #1561, fix(claude_k8s): make adapterConfig.env default-deny, not name-pattern (BLO-22546) #1463. fix(claude_k8s): make adapterConfig.env default-deny, not name-pattern (BLO-22546) #1463 also touches adapterConfig.env but is a different concern (making the claude_k8s adapter's env default-deny at the adapter layer, BLO-22546); it does not overlap this read/write projection. No duplicate PR exists.

What Changed

• Read projection — toCompanySkillTestRun withholds adapterConfig/runtimeConfig from the served snapshot on both …/test-runs and …/test-runs/:runId. This is what repairs rows already persisted in the clear, which a write-path fix cannot reach.
• Write path — snapshotAgentConfig routes the pair through redactAgentConfigPayload before persisting, so the column stops being credential material at rest.
• Extraction, not a copy — the read filter is withholdAgentConfigKeys, extracted from withholdAgentConfigFromApprovalPayload so both callers delegate to one implementation. This class propagates by copying: doors fix(heartbeat): trust SDK success signal when wrapper exits non-zero #7–ops(opencode-k8s): bump to kubectl-logs streaming (Staff Engineer 'failed' fix) #10 are the same disclosure re-derived on paths whose authors could not see each other, and a fresh copy would not have carried the array/JSON-string bypass Ally caught in fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574.
• The approval wrapper keeps its hire_agent-only precondition, pinned by a test so the extraction cannot silently widen PEN-2777's scoping.
• Fields anything actually reads (name, model, assignedSkills, instructionsRef) are derived before withholding and survive. The only UI consumer is agentConfigSnapshot?.name.

Why the gate did not hold

assertCompanyAccess refuses only cross-company access, so any same-company agent passed it. Note the verb asymmetry: creating a test run is gated (skills.test + assertCanOrchestrateSkillTestHarness), but reading one was not — an agent with no skill-test permission still read every recorded snapshot.

path	entitlement	treatment of adapterConfig
GET /agents/:id	agent_config:read	redacted; residue gated
hire_agent approval card	company_scope:read	withheld (fixed, #1574)
GET …/test-runs/:runId (this PR)	company_scope:read	was neither masked nor withheld

Verification

Both suites run locally against embedded Postgres; 67 passed, 0 skipped. The DB suite describe.skips silently when Postgres is unsupported, so I confirmed by name that each test actually executed rather than trusting the aggregate.

Fail-first, each end reverted independently — this is the evidence that neither end is load-bearing for the other:

mutation	read-projection test	write-path test	disclosed value in failure text
revert read projection only	FAIL	pass	yes (x4)
revert write path only	pass	FAIL	yes (x4)

Reverting the read projection surfaced the credential verbatim in the served response body:

AssertionError: detail disclosed the credential:
  expected '{"id":"f4db3a8e…' not to contain '[paperclip-egress-scrub redacted: vendor-key]'
… "agentConfigSnapshot":{"adapterConfig":{"env":{"OPENAI_API_KEY":"[paperclip-egress-scrub redacted: vendor-key]"},
  "mcpServers":{"k8s":{"url":"https://svc-account@k8s-mcp-admin.internal:8443/mcp"}}}}

Each end failing under its own mutation while the other stays green is the point: a write-only fix would have left every pre-existing row disclosing, and the read projection alone would have left the column plaintext at rest.

Pre-existing PEN-2777 tests stay green across the extraction (they are among the 67).

Typecheck: tsc --noEmit output is byte-identical between this branch and pristine master in the same environment. Two residual errors about agentMeRecoveryActionsQuerySchema reproduce on unmodified master here and are an artifact of my local dependency wiring, not this branch.

Risks

• Low risk, but not zero. The read projection changes the response shape of two endpoints: agentConfigSnapshot.adapterConfig and .runtimeConfig are now absent. The only consumer read in the UI is agentConfigSnapshot?.name, which is unaffected; any out-of-tree consumer depending on the withheld pair would see it disappear. That is the intended security outcome, but it is a behavioural change rather than a pure internal refactor.
• Extraction risk. withholdAgentConfigFromApprovalPayload is a shipped security control (PEN-2777/fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574); refactoring it risks widening or narrowing its scope. Mitigated by keeping the hire_agent-only precondition in the wrapper and pinning it with a test, and by the pre-existing fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574 tests staying green.
• Not a migration. Already-persisted rows keep their plaintext in the column; the read projection is what prevents disclosure of those rows. Backfilling/scrubbing the existing column contents is deliberately out of scope here and left to the series' cleanup — flagged so it is not assumed done.
• No schema change, no new dependency, no change to any authorization decision.

Model Used

Claude Opus 5 (claude-opus-5[1m]), 1M context window, extended thinking enabled, driven through the Claude Code agent harness with tool use (file edits, local test execution, GitHub API reads).

Handling

No credential value was read, quoted, or committed; all fixtures are synthetic. The vulnerable endpoint was never called — calling it is the exposure. Every claim in the finding was read from source at master c75a306.

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 — n/a, no doc surface changed
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet; server test shards still running at time of writing
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

Refs PEN-2370 (ask 3, criterion b2), PEN-2777, PEN-2778, PRs #1567 #1573 #1574 #1578.

[allyblockcast]

🔗 Paperclip issue: PEN-2839
🔗 Paperclip issue: PEN-2777
🔗 Paperclip issue: PEN-2778
🔗 Paperclip issue: PEN-2370

[allyblockcast]

🔗 Paperclip issue: PEN-2839
🔗 Paperclip issue: PEN-2777
🔗 Paperclip issue: PEN-2778
🔗 Paperclip issue: PEN-2370

[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 — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack executors were unavailable in this isolated runtime; their checks were applied directly to the exact PR diff.
Reviewed head: 5daee94

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The read projection and write-path protections are independently implemented, covering both historical plaintext snapshots and newly persisted snapshots.
• withholdAgentConfigKeys is shared without widening the hire-approval wrapper's type precondition, and the tests cover nested, non-object, and pre-existing snapshot shapes.
• The synthetic tests assert both credential values and MCP topology are absent from served responses, while preserving the documented non-sensitive snapshot fields.

Recommended Action

1. No Critical or Important issues found; this review is a formal self-review comment because the PR is authored by the Ally GitHub App.
2. Consider the Suggestions opportunistically.

[kkroo]

Approved after Ally exact-head review. Ally reports zero Critical and Important findings on the current head; all required checks are green.