fix(adapters): forward resolved adapter env to local agents, keep runtime env authoritative (#9617)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Local coding adapters (Claude, Codex) run each agent heartbeat in a spawned child process; Paperclip resolves adapter-configured env — including `secret_ref` bindings — into the env for that process > - The resolved adapter env was not being forwarded reliably: config env could overwrite Paperclip's own runtime vars, and a warm/resumable ACP session could keep serving stale env because its fingerprint ignored the resolved env > - This mattered because a configured API key or other secret could be silently absent from the agent shell, and a resumed session would never pick up an updated value — while a config binding could also override runtime identity/wake vars > - This pull request keeps Paperclip-managed `PAPERCLIP_*` runtime env authoritative over config, and folds a stable hash of the applied adapter env into the session fingerprint so an env change forces a fresh launch > - The benefit is that adapter-configured env (plain values and resolved secrets) reliably reaches the agent process, updates are picked up on the next launch, and runtime identity can never be clobbered by config ## Linked Issues or Issue Description No public GitHub issue exists, so the underlying bug is described inline following the bug-report template. **What happened** Env keys configured on a local adapter (plain values and `secret_ref` bindings, resolved server-side into plain strings) did not reliably reach the spawned agent process. Two distinct gaps: (1) when merging config env into the process env, a config key in the reserved `PAPERCLIP_*` namespace could overwrite a Paperclip-managed runtime variable (identity, wake, workspace, API access); (2) a warm-handle / resumable ACP session computed its reuse fingerprint from `secretManifestHash` only, which misses plain-value edits and same-version secret rotations — so a resumed session kept serving stale env and never re-launched with updated values. **Expected behavior** Non-`PAPERCLIP_*` adapter env (plain + resolved secret values) is forwarded to the agent process; a change to any applied forwarded value invalidates a warm/resumable session so the next launch sources the latest env; configured `PAPERCLIP_*` entries can never override Paperclip runtime env, while an explicitly configured `PAPERCLIP_API_KEY` (stable per-run config) is still honored and its rotation also busts the session. **Steps to reproduce** Configure an adapter with an env key (e.g. a `secret_ref` API key) and a resumable ACP session. On resume, the updated env value is not sourced; separately, a `PAPERCLIP_*` config key overrides the runtime value. **Deployment mode** Local adapters (Claude / Codex) via the shared adapter-utils execution path. ## What Changed - `packages/adapter-utils/src/server-utils.ts`: add `isPaperclipRuntimeEnvKey` and, in `refreshPaperclipWorkspaceEnvForExecution` (used by all local adapters), skip a `PAPERCLIP_*` config key when Paperclip has already assigned it this run; all other keys still forward. - `packages/adapter-utils/src/acpx-engine/execute.ts`: apply the same `PAPERCLIP_*` non-override rule (via the shared helper) when merging config env, capture the applied config env in `resolvedAdapterEnv`, and fold a stable `adapterEnvHash` of it into the session fingerprint so an env change forces a fresh launch. Per-wake `PAPERCLIP_*` runtime vars are assigned earlier and never enter that map, so they stay out of the hash; stable configured `PAPERCLIP_*` values (e.g. an explicit `PAPERCLIP_API_KEY`) are included so rotating one busts the session. - Added unit/integration tests for plain + secret forwarding, `PAPERCLIP_*` non-override, the explicit-API-key path, fingerprint refresh-on-env-change vs. stable-across-wakes, and rotation of a configured `PAPERCLIP_API_KEY`. ## Verification - `npx vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts packages/adapter-utils/src/server-utils.test.ts` → 2 files, 111 tests passing (includes the new cases). - Tests assert: forwarded plain/secret values appear in the spawned wrapper `.env`; a `PAPERCLIP_*` config key does not override the runtime value; changing an applied forwarded env value (including a rotated `PAPERCLIP_API_KEY`) changes `configFingerprint`, while a new wake with the same config env keeps it stable. ## Risks Low risk. Behavior change is limited to (a) config env no longer overriding `PAPERCLIP_*` runtime vars — a security-positive tightening — and (b) a resumable session re-launching when its applied config env changes, which is the intended fix. Per-wake `PAPERCLIP_*` churn is deliberately excluded from the fingerprint so normal sessions still resume across heartbeats. Existing sessions get a new fingerprint once on first deploy (the added `adapterEnvHash` field), which is expected. No secret values are logged (key-name redaction in the adapter plus manifest-driven redaction of `meta.env`). ## Model Used Claude Opus 4.8 (Anthropic), model id `claude-opus-4-8`, extended reasoning with tool use. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
b287281940
commit
0ecae2cd7e
|
|
@ -557,6 +557,95 @@ describe("shared ACPX engine runtime behavior", () => {
|
|||
expect(env).not.toContain("old-key");
|
||||
});
|
||||
|
||||
it("forwards resolved adapter env (plain + secret) to the wrapper without overriding runtime vars", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const stateDir = path.join(root, "state");
|
||||
|
||||
await runExecutor(
|
||||
{
|
||||
agentCommand: "node ./fake-acp.js",
|
||||
stateDir,
|
||||
env: {
|
||||
OOGA_BOOGA_123: "plain-value",
|
||||
// Server-resolved secret_ref values arrive here as plain strings.
|
||||
OPENROUTER_API_KEY: "resolved-secret-value",
|
||||
// Reserved-namespace config keys must not clobber runtime identity/wake.
|
||||
PAPERCLIP_TASK_ID: "attacker-issue",
|
||||
},
|
||||
},
|
||||
{
|
||||
authToken: "runtime-secret-token",
|
||||
context: { taskId: "issue-real", wakeReason: "issue_assigned" },
|
||||
},
|
||||
);
|
||||
|
||||
const wrappers = await fs.readdir(path.join(stateDir, "wrappers"));
|
||||
const envPath = path.join(stateDir, "wrappers", wrappers.find((name) => name.endsWith(".env"))!);
|
||||
const env = await fs.readFile(envPath, "utf8");
|
||||
|
||||
expect(env).toContain("OOGA_BOOGA_123='plain-value'");
|
||||
expect(env).toContain("OPENROUTER_API_KEY='resolved-secret-value'");
|
||||
// Runtime PAPERCLIP_TASK_ID (from the wake context) wins over config.
|
||||
expect(env).toContain("PAPERCLIP_TASK_ID='issue-real'");
|
||||
expect(env).not.toContain("attacker-issue");
|
||||
});
|
||||
|
||||
it("busts the session fingerprint when resolved adapter env changes but not across wakes", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const stateDir = path.join(root, "state");
|
||||
const baseConfig = { agentCommand: "node ./fake-acp.js", stateDir };
|
||||
|
||||
const first = await runExecutor(
|
||||
{ ...baseConfig, env: { OPENROUTER_API_KEY: "value-1" } },
|
||||
{ context: { taskId: "issue-1", wakeReason: "issue_assigned" } },
|
||||
);
|
||||
const changedEnv = await runExecutor(
|
||||
{ ...baseConfig, env: { OPENROUTER_API_KEY: "value-2" } },
|
||||
{ context: { taskId: "issue-1", wakeReason: "issue_assigned" } },
|
||||
);
|
||||
const sameEnvNewWake = await runExecutor(
|
||||
{ ...baseConfig, env: { OPENROUTER_API_KEY: "value-1" } },
|
||||
{ context: { taskId: "issue-1", wakeReason: "comment", wakeCommentId: "c-9" } },
|
||||
);
|
||||
|
||||
const fp = (r: { result: { sessionParams?: unknown } }) =>
|
||||
(r.result.sessionParams as { configFingerprint?: string } | undefined)?.configFingerprint;
|
||||
|
||||
// A changed forwarded env value invalidates warm-handle / session reuse so
|
||||
// the next launch sources the latest env.
|
||||
expect(fp(first)).toBeDefined();
|
||||
expect(fp(changedEnv)).not.toBe(fp(first));
|
||||
// A new heartbeat with the same config env keeps the fingerprint stable, so
|
||||
// per-wake PAPERCLIP_* churn does not needlessly reset the session.
|
||||
expect(fp(sameEnvNewWake)).toBe(fp(first));
|
||||
});
|
||||
|
||||
it("busts the session fingerprint when a stable configured PAPERCLIP_* value rotates", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const stateDir = path.join(root, "state");
|
||||
const baseConfig = { agentCommand: "node ./fake-acp.js", stateDir };
|
||||
|
||||
// An explicitly configured PAPERCLIP_API_KEY is stable per-run config (not a
|
||||
// per-wake runtime var): rotating it must invalidate a warm/resumable session
|
||||
// so the next launch sources the new key, even across an otherwise-identical
|
||||
// wake context.
|
||||
const context = { taskId: "issue-1", wakeReason: "issue_assigned" };
|
||||
const withKey = await runExecutor(
|
||||
{ ...baseConfig, env: { PAPERCLIP_API_KEY: "explicit-key-1" } },
|
||||
{ context },
|
||||
);
|
||||
const rotatedKey = await runExecutor(
|
||||
{ ...baseConfig, env: { PAPERCLIP_API_KEY: "explicit-key-2" } },
|
||||
{ context },
|
||||
);
|
||||
|
||||
const fp = (r: { result: { sessionParams?: unknown } }) =>
|
||||
(r.result.sessionParams as { configFingerprint?: string } | undefined)?.configFingerprint;
|
||||
|
||||
expect(fp(withKey)).toBeDefined();
|
||||
expect(fp(rotatedKey)).not.toBe(fp(withKey));
|
||||
});
|
||||
|
||||
it("shapes ACPX wrapper workspace env for remote execution identities", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const stateDir = path.join(root, "state");
|
||||
|
|
|
|||
|
|
@ -33,6 +33,7 @@ import {
|
|||
ensureAbsoluteDirectory,
|
||||
ensurePathInEnv,
|
||||
ensurePaperclipSkillSymlink,
|
||||
isPaperclipRuntimeEnvKey,
|
||||
joinPromptSections,
|
||||
materializePaperclipSkillCopy,
|
||||
parseObject,
|
||||
|
|
@ -1044,8 +1045,24 @@ async function buildRuntime(input: {
|
|||
executionCwd: shapedWorkspaceEnv.workspaceCwd,
|
||||
executionTargetIsRemote,
|
||||
});
|
||||
// Resolved adapter env (plain + server-resolved secret_ref values) that we
|
||||
// forward to the spawned agent process. Captured so a stable hash of it can be
|
||||
// folded into the session fingerprint below — a change here must invalidate a
|
||||
// warm/resumable session so the next launch picks up the latest env. Only
|
||||
// user/adapter-configured env flows through this loop; per-wake PAPERCLIP_*
|
||||
// runtime vars (PAPERCLIP_RUN_ID, wake/approval ids, ...) were assigned to
|
||||
// `env` above and are never present in shapedEnvConfig, so they inherently
|
||||
// stay out of the hash and don't reset the session every heartbeat.
|
||||
const resolvedAdapterEnv: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(shapedEnvConfig)) {
|
||||
if (typeof value === "string") env[key] = value;
|
||||
if (typeof value !== "string") continue;
|
||||
// Runtime PAPERCLIP_* always wins over config: skip a PAPERCLIP_* key that
|
||||
// Paperclip has already assigned this run. A PAPERCLIP_* key Paperclip did
|
||||
// NOT set (e.g. an explicitly configured PAPERCLIP_API_KEY, applied here) is
|
||||
// stable per-run config, so it applies and feeds the fingerprint hash below.
|
||||
if (isPaperclipRuntimeEnvKey(key) && key in env) continue;
|
||||
env[key] = value;
|
||||
resolvedAdapterEnv[key] = value;
|
||||
}
|
||||
if (!hasExplicitApiKey && authToken) env.PAPERCLIP_API_KEY = authToken;
|
||||
// For the claude agent, set model via ANTHROPIC_MODEL at startup rather than
|
||||
|
|
@ -1217,6 +1234,14 @@ async function buildRuntime(input: {
|
|||
: null,
|
||||
mcpServers: mcpIdentity,
|
||||
secretManifestHash: shortHash(secretManifest),
|
||||
// Fold the resolved adapter env (all applied user-configured values —
|
||||
// plain, secret_ref, and stable PAPERCLIP_* config such as an explicit
|
||||
// PAPERCLIP_API_KEY) into the fingerprint so a change to any forwarded value
|
||||
// invalidates a warm handle / resumable session and forces a fresh launch
|
||||
// that sources the latest env. secretManifestHash alone misses plain-value
|
||||
// edits and same-version secret rotations. Per-wake runtime vars never enter
|
||||
// resolvedAdapterEnv, so they don't churn the fingerprint every heartbeat.
|
||||
adapterEnvHash: shortHash(resolvedAdapterEnv),
|
||||
});
|
||||
const taskKey = asString(input.ctx.runtime.taskKey, "") || wakeTaskId || workspaceId || "default";
|
||||
const sessionKey = `paperclip:${agent.companyId}:${agent.id}:${taskKey}:${fingerprint}`;
|
||||
|
|
|
|||
|
|
@ -2053,6 +2053,50 @@ describe("refreshPaperclipWorkspaceEnvForExecution", () => {
|
|||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it("forwards resolved adapter env but never overrides Paperclip runtime env", () => {
|
||||
const env: Record<string, string> = {
|
||||
PAPERCLIP_RUN_ID: "run-1",
|
||||
PAPERCLIP_TASK_ID: "issue-1",
|
||||
PAPERCLIP_API_URL: "http://runtime:3100",
|
||||
};
|
||||
|
||||
refreshPaperclipWorkspaceEnvForExecution({
|
||||
env,
|
||||
envConfig: {
|
||||
// Plain non-PAPERCLIP key.
|
||||
OOGA_BOOGA_123: "plain-value",
|
||||
// Server-resolved secret_ref value arrives as a plain string here.
|
||||
OPENROUTER_API_KEY: "resolved-secret-value",
|
||||
// Reserved-namespace keys must not clobber runtime identity/wake vars.
|
||||
PAPERCLIP_TASK_ID: "attacker-issue",
|
||||
PAPERCLIP_API_URL: "http://evil:9999",
|
||||
},
|
||||
workspaceCwd: null,
|
||||
});
|
||||
|
||||
expect(env.OOGA_BOOGA_123).toBe("plain-value");
|
||||
expect(env.OPENROUTER_API_KEY).toBe("resolved-secret-value");
|
||||
expect(env.PAPERCLIP_TASK_ID).toBe("issue-1");
|
||||
expect(env.PAPERCLIP_API_URL).toBe("http://runtime:3100");
|
||||
});
|
||||
|
||||
it("applies a configured PAPERCLIP_* key only when Paperclip has not set it", () => {
|
||||
const env: Record<string, string> = {};
|
||||
|
||||
refreshPaperclipWorkspaceEnvForExecution({
|
||||
env,
|
||||
envConfig: {
|
||||
PAPERCLIP_API_KEY: "explicit-key",
|
||||
},
|
||||
workspaceCwd: null,
|
||||
});
|
||||
|
||||
// Paperclip did not assign PAPERCLIP_API_KEY before the merge, so an
|
||||
// explicitly configured value is allowed through (adapters apply the run
|
||||
// token here only when no explicit key was configured).
|
||||
expect(env.PAPERCLIP_API_KEY).toBe("explicit-key");
|
||||
});
|
||||
});
|
||||
|
||||
describe("appendWithByteCap", () => {
|
||||
|
|
|
|||
|
|
@ -107,6 +107,13 @@ const DEFAULT_PAPERCLIP_INSTANCE_ID = "default";
|
|||
const PATH_SEGMENT_RE = /^[a-zA-Z0-9_-]+$/;
|
||||
const SENSITIVE_ENV_KEY = /(key|token|secret|password|passwd|authorization|cookie)/i;
|
||||
const REDACTED_LOG_VALUE = "***REDACTED***";
|
||||
|
||||
// PAPERCLIP_* is the reserved runtime namespace: these vars are generated by
|
||||
// Paperclip per run (identity, wake, workspace, API access). Adapter/user
|
||||
// config env must never override them.
|
||||
export function isPaperclipRuntimeEnvKey(key: string): boolean {
|
||||
return key.startsWith("PAPERCLIP_");
|
||||
}
|
||||
const PAPERCLIP_SKILL_ROOT_RELATIVE_CANDIDATES = [
|
||||
"../../skills",
|
||||
"../../../../../skills",
|
||||
|
|
@ -1954,6 +1961,14 @@ export function refreshPaperclipWorkspaceEnvForExecution(input: {
|
|||
executionTargetIsRemote: input.executionTargetIsRemote,
|
||||
});
|
||||
for (const [key, value] of Object.entries(shapedEnvConfig)) {
|
||||
// Adapter/user-configured env must never override a Paperclip-managed
|
||||
// runtime variable. Non-PAPERCLIP_* keys (plain values and resolved
|
||||
// secret_ref values) always forward to the spawned process; a PAPERCLIP_*
|
||||
// key from config only applies when Paperclip has NOT already assigned it
|
||||
// for this run (e.g. an explicitly configured PAPERCLIP_API_KEY that the
|
||||
// adapter applies after this merge). This keeps runtime identity, wake, and
|
||||
// workspace vars authoritative regardless of what a config binding sets.
|
||||
if (isPaperclipRuntimeEnvKey(key) && key in input.env) continue;
|
||||
input.env[key] = value;
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue