Skip to content

fix(coding-agent): secure resident ACP recovery - #1236

Open
sethkarten wants to merge 30 commits into
mainfrom
v080/acp-p1-final-core
Open

fix(coding-agent): secure resident ACP recovery#1236
sethkarten wants to merge 30 commits into
mainfrom
v080/acp-p1-final-core

Conversation

@sethkarten

@sethkarten sethkarten commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep reattachable ACP sessions resident while leaving --no-session runs client-owned
  • recover failed resident workers only from a fresh client-supplied launch environment
  • strip credential-bearing environment and runtime configuration before durable persistence
  • preserve safe recovery settings, supervisor ownership, and registry identity across restarts
  • reap crashed-worker orphans and detached children during lifecycle teardown

Behavioral contract

A resident worker that cannot be safely recovered remains failed until an authorized local client supplies fresh transient launch context. Durable descriptors remain recovery recipes, not credential stores.

Stack

This is the parent of #1239. Merge #1236 first, then retarget and merge #1239.

Validation

  • required CI and CodeQL checks pass at dc6f1a8a774f6295f4591e2f01422090b57fa832
  • focused coverage includes resident ACP lifecycle, credential sanitization, recovery fencing, PID reuse, descriptor conflicts, and orphan cleanup
  • no paid or live evaluation is part of GitHub CI

Note

High Risk
Changes authentication and daemon worker recovery in security-sensitive paths; existing on-disk descriptors may lose stored env vars and need clients to re-supply credentials on reconnect.

Overview
Resident ACP sessions with a session file stay resident after stdio disconnect; --no-session ACP and non-ACP modes remain client-owned. Interactive startup uses isClientOwnedDaemonSession instead of treating all sessions as client-owned.

Credential hygiene: Worker descriptors and supervisor config are sanitized before disk write via sanitizeAgentSessionRuntimeConfigForDurableStorage and filterPersistedDaemonLaunchEnv (allowlisted non-secret launch keys only). Loaded descriptors are re-sanitized and rewritten when stale secrets are found.

Fail-closed recovery: Ownerless resident workers that crash no longer auto-relaunch from descriptor or supervisor ambient env—they enter failed until a local client resumes with a fresh launchEnv. reuseWorkerForCreate single-flights concurrent resumes and consumes one-shot recovery credentials after success or exhaustion. Failed resident attach falls through to create with fresh launch env.

Daemon shutdown kills tracked detached children; orphan reaping is split for dead workers without credential-based replacement.

Reviewed by Cursor Bugbot for commit 2ab4ff1. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Secure resident ACP daemon worker recovery by filtering secrets from persisted descriptors

  • Adds sanitizeAgentSessionRuntimeConfigForDurableStorage in agent-session-config.ts to strip credential-like fields before writing configs to disk; applied in supervisor startup, worker persistence, and descriptor loading.
  • Adds filterPersistedDaemonLaunchEnv in daemon-protocol.ts with an explicit allowlist of non-secret env keys permitted in resident worker descriptors.
  • Resident worker recovery now fails closed when no fresh credentials are available: the worker is marked failed and waits for a client to supply a launchEnv on the next resume request instead of retrying with stale or absent env.
  • isClientOwnedDaemonSession in main.ts makes ACP sessions with a session file resident by default; --no-session keeps them client-owned.
  • Risk: existing resident worker descriptors with credentials persisted will have those fields stripped on next supervisor load, requiring clients to supply fresh launchEnv for recovery.

Macroscope summarized 2ab4ff1.

@sethkarten
sethkarten changed the base branch from v080/final-core-main-project-mcp-hardened to main August 11, 2026 18:05
@sethkarten
sethkarten marked this pull request as ready for review August 12, 2026 06:18
Comment thread packages/coding-agent/test/acp-resident-lifecycle.test.ts
Comment thread packages/coding-agent/src/package-manager-cli.ts
Comment thread packages/coding-agent/src/package-manager-cli.ts
@sethkarten
sethkarten requested a review from alexzhang13 August 13, 2026 01:18
@sethkarten sethkarten changed the title test(acp): preserve resident recovery on final core fix(coding-agent): secure resident ACP recovery Aug 17, 2026
@sethkarten
sethkarten requested a review from snimu August 17, 2026 19:01
sethkarten added a commit that referenced this pull request Aug 17, 2026
Comment thread packages/coding-agent/src/modes/daemon/daemon-supervisor.ts
Comment thread packages/coding-agent/src/modes/daemon/daemon-protocol.ts
sethkarten added a commit that referenced this pull request Aug 17, 2026
Comment thread packages/coding-agent/src/modes/daemon/daemon-protocol.ts
sethkarten added a commit that referenced this pull request Aug 17, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f8e20fc. Configure here.

Comment thread packages/coding-agent/src/modes/daemon/daemon-supervisor.ts Outdated
sethkarten added a commit that referenced this pull request Aug 17, 2026
sethkarten added a commit that referenced this pull request Aug 17, 2026
@sethkarten
sethkarten requested a review from samsja August 17, 2026 21:30
sethkarten added a commit that referenced this pull request Aug 17, 2026
// Provider/extension config is open-ended. Match composed credential names
// narrowly while preserving product fields such as networkAccess, productAccess,
// productKeyId, tokenBudget, and tokenLimit.
return /(?:apikey|apisecret|secretaccesskey|accesskeyid|accesstoken|refreshtoken|sessiontoken|clientsecret|bearertoken|authtoken|idtoken|token|password|credential|authorization|cookie|privatekey|signingkey)$/.test(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical core/agent-session-config.ts:146

Extension credentials such as --provider-secret-key=... remain in the durable worker descriptor instead of being redacted. After normalization, provider-secret-key becomes providersecretkey, which is neither an exact secret field nor matched by the suffix regexp; add secretkey to that regexp so composed extension credential names are removed before JSON persistence.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/agent-session-config.ts around line 146:

Extension credentials such as `--provider-secret-key=...` remain in the durable worker descriptor instead of being redacted. After normalization, `provider-secret-key` becomes `providersecretkey`, which is neither an exact secret field nor matched by the suffix regexp; add `secretkey` to that regexp so composed extension credential names are removed before JSON persistence.

Evidence trail:
packages/coding-agent/src/cli/daemon-command.ts:613-616 (2ab4ff1); packages/coding-agent/src/core/agent-session-config.ts:56-94,96-148 (2ab4ff1); packages/coding-agent/src/modes/daemon/daemon-supervisor.ts:1035-1041 (2ab4ff1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants