fix: preserve paperclip runtime env in exec tool defaults#59485
fix: preserve paperclip runtime env in exec tool defaults#59485bboyyan wants to merge 3 commits intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bda3f4e6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!params.paperclipRuntimeAuth) { | ||
| return fn(); |
There was a problem hiding this comment.
Serialize all runs while PAPERCLIP env is globally overridden
runEmbeddedPiAgent now writes Paperclip credentials into process.env for the duration of a run, but withPaperclipRuntimeEnvLock skips locking when paperclipRuntimeAuth is absent. That allows a concurrent run on another command lane (for example subagent or cron) to execute while PAPERCLIP_* is still set by a different run, so unrelated exec calls can inherit and leak the wrong Paperclip auth header/API key. Because the env mutation is process-global, non-Paperclip runs must also be blocked (or the env must be passed per-invocation) to avoid cross-run credential contamination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix Paperclip runtime auth/env propagation so that Paperclip credentials (e.g., PAPERCLIP_AUTH_HEADER, PAPERCLIP_API_KEY) survive the final wiring step into exec-tool execution, and adds regression tests around this behavior.
Changes:
- Update
createOpenClawCodingToolsto preferoptions.exec.paperclipRuntimeEnv(with fallback tooptions.paperclipRuntimeEnv) when constructing exec tool defaults. - Add Paperclip runtime-auth env application/restoration (with a lock to avoid cross-run contamination) plus tests validating env behavior.
- Adjust several tests’
fetchmocks to be typed astypeof fetch.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/pi-embedded-runner.paperclip-auth.test.ts | Adds a unit test for applying/restoring Paperclip env vars. |
| src/infra/clawhub.test.ts | Tweaks fetch mock typing/casting in ClawHub helper tests. |
| src/agents/pi-tools.ts | Attempts to plumb paperclipRuntimeEnv into exec tool defaults. |
| src/agents/pi-tools.paperclip-runtime-env.test.ts | Adds regression coverage intended to verify Paperclip env reaches exec. |
| src/agents/pi-embedded-runner/run.ts | Adds Paperclip env application/restoration + a lock around embedded runs. |
| src/agents/pi-embedded-runner.run-embedded-pi-agent.auth-profile-rotation.e2e.test.ts | Adjusts fetch mock typing via a helper cast. |
| extensions/voice-call/src/providers/twilio/api.test.ts | Adjusts fetch mock typing via a helper cast. |
| extensions/msteams/src/graph.test.ts | Adjusts fetch mock typing via a helper cast. |
| extensions/microsoft/speech-provider.test.ts | Adjusts fetch mock typing via a helper cast. |
| messageProvider: options?.messageProvider, | ||
| currentChannelId: options?.currentChannelId, | ||
| currentThreadTs: options?.currentThreadTs, | ||
| accountId: options?.agentAccountId, | ||
| paperclipRuntimeEnv: options?.exec?.paperclipRuntimeEnv ?? options?.paperclipRuntimeEnv, |
There was a problem hiding this comment.
createExecTool() currently does not define or consume a paperclipRuntimeEnv default (and ExecToolDefaults doesn’t include this field). As written, this value is effectively ignored, so Paperclip env won’t actually be injected into the exec subprocess. Consider either (a) adding paperclipRuntimeEnv support to ExecToolDefaults + createExecTool and merging it into the effective env, or (b) wiring it here by merging into args.env before delegating to the exec tool.
| const tools = createOpenClawCodingTools({ | ||
| workspaceDir: process.cwd(), | ||
| exec: { | ||
| host: "gateway", | ||
| security: "full", | ||
| ask: "off", | ||
| paperclipRuntimeEnv: { | ||
| PAPERCLIP_AUTH_HEADER: "Bearer test-header", |
There was a problem hiding this comment.
This test passes paperclipRuntimeEnv under exec, but createOpenClawCodingTools types exec as ExecToolDefaults & ProcessToolDefaults, and ExecToolDefaults currently doesn’t include paperclipRuntimeEnv. This will fail TypeScript excess-property checking, and even at runtime the exec tool won’t see these values unless the exec tool defaults explicitly merge them into env.
| "PAPERCLIP_AUTH_HEADER", | ||
| ] as const; | ||
|
|
||
| let paperclipRuntimeEnvLock: Promise<void> = Promise.resolve(); | ||
|
|
||
| async function withPaperclipRuntimeEnvLock<T>( | ||
| params: Pick< | ||
| RunEmbeddedPiAgentParams, | ||
| "paperclipRuntimeAuth" | "runId" | "sessionId" | "sessionKey" | ||
| >, |
There was a problem hiding this comment.
RunEmbeddedPiAgentParams (src/agents/pi-embedded-runner/run/params.ts) does not currently define a paperclipRuntimeAuth field, so this Pick<..."paperclipRuntimeAuth"...> and subsequent params.paperclipRuntimeAuth usage will not typecheck. Add paperclipRuntimeAuth (and its shape) to RunEmbeddedPiAgentParams (or change these helpers to accept an explicit Paperclip auth type) to keep the runner buildable.
| "PAPERCLIP_AUTH_HEADER", | |
| ] as const; | |
| let paperclipRuntimeEnvLock: Promise<void> = Promise.resolve(); | |
| async function withPaperclipRuntimeEnvLock<T>( | |
| params: Pick< | |
| RunEmbeddedPiAgentParams, | |
| "paperclipRuntimeAuth" | "runId" | "sessionId" | "sessionKey" | |
| >, | |
| "PAPERCLIP_AUTH_HEADER", | |
| ] as const; | |
| let paperclipRuntimeEnvLock: Promise<void> = Promise.resolve(); | |
| type PaperclipRuntimeEnvLockParams = { | |
| paperclipRuntimeAuth?: unknown; | |
| runId: string; | |
| sessionId: string; | |
| sessionKey: string; | |
| }; | |
| async function withPaperclipRuntimeEnvLock<T>( | |
| params: PaperclipRuntimeEnvLockParams, |
Greptile SummaryThis PR fixes a one-line env-plumbing bug in Key concerns:
Confidence Score: 3/5
|
| const redactedSessionId = redactRunIdentifier(params.sessionId); | ||
| const redactedSessionKey = redactRunIdentifier(params.sessionKey); | ||
| const redactedWorkspace = redactRunIdentifier(resolvedWorkspace); | ||
| if (workspaceResolution.usedFallback) { |
There was a problem hiding this comment.
__paperclipAuthEnvTestUtils exports internal implementation details (applyPaperclipRuntimeAuthEnv and PAPERCLIP_RUNTIME_ENV_KEYS) directly from the production run.ts file. This couples the test surface to the implementation and leaks internals into the public module interface.
Consider moving these to a dedicated test-helper file (e.g. run.test-utils.ts) or using Vitest's module-mocking approach so that the production module has no test-only exports.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run.ts
Line: 359-362
Comment:
**Test utilities exported from production module**
`__paperclipAuthEnvTestUtils` exports internal implementation details (`applyPaperclipRuntimeAuthEnv` and `PAPERCLIP_RUNTIME_ENV_KEYS`) directly from the production `run.ts` file. This couples the test surface to the implementation and leaks internals into the public module interface.
Consider moving these to a dedicated test-helper file (e.g. `run.test-utils.ts`) or using Vitest's module-mocking approach so that the production module has no test-only exports.
How can I resolve this? If you propose a fix, please make it concise.| it("passes exec.paperclipRuntimeEnv through to the exec tool defaults", async () => { | ||
| const tools = createOpenClawCodingTools({ | ||
| workspaceDir: process.cwd(), | ||
| exec: { | ||
| host: "gateway", | ||
| security: "full", | ||
| ask: "off", | ||
| paperclipRuntimeEnv: { | ||
| PAPERCLIP_AUTH_HEADER: "Bearer test-header", | ||
| PAPERCLIP_API_KEY: "pcp_test_key", | ||
| }, | ||
| }, | ||
| sessionKey: "agent:test:paperclip", | ||
| }); | ||
|
|
||
| const execTool = tools.find((tool) => tool.name === "exec"); | ||
| expect(execTool).toBeTruthy(); | ||
|
|
||
| const result = await execTool!.execute( | ||
| "toolcall-test", | ||
| { | ||
| command: | ||
| 'node -e "console.log(JSON.stringify({auth:process.env.PAPERCLIP_AUTH_HEADER||null,key:process.env.PAPERCLIP_API_KEY||null}))"', | ||
| host: "gateway", | ||
| security: "full", | ||
| ask: "off", | ||
| }, | ||
| new AbortController().signal, | ||
| async () => {}, | ||
| ); | ||
|
|
||
| const text = JSON.stringify(result); | ||
| expect(text).toContain("Bearer test-header"); | ||
| expect(text).toContain("pcp_test_key"); | ||
| }); |
There was a problem hiding this comment.
paperclipRuntimeEnv fallback path is not tested
The regression test passes the env through options.exec.paperclipRuntimeEnv, which correctly exercises the primary path in the fix (options?.exec?.paperclipRuntimeEnv ?? …).
The complementary fallback — providing options.paperclipRuntimeEnv at the top level without an options.exec.paperclipRuntimeEnv — is not covered by any test. A second test case mirroring the existing one but placing the paperclipRuntimeEnv map at the top level of options (rather than under exec) would close this gap and guard against the ?? being accidentally reversed in a future refactor.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.paperclip-runtime-env.test.ts
Line: 5-39
Comment:
**Top-level `paperclipRuntimeEnv` fallback path is not tested**
The regression test passes the env through `options.exec.paperclipRuntimeEnv`, which correctly exercises the primary path in the fix (`options?.exec?.paperclipRuntimeEnv ?? …`).
The complementary fallback — providing `options.paperclipRuntimeEnv` at the top level _without_ an `options.exec.paperclipRuntimeEnv` — is not covered by any test. A second test case mirroring the existing one but placing the `paperclipRuntimeEnv` map at the top level of options (rather than under `exec`) would close this gap and guard against the `??` being accidentally reversed in a future refactor.
How can I resolve this? If you propose a fix, please make it concise.- avoid mutating process.env for no-auth embedded runs - add top-level paperclipRuntimeEnv fallback coverage - move paperclip auth env test helpers out of production module exports - dedupe asFetch into shared test helper
|
Fixed both comments.
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a58ff0276
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paperclipRuntimeEnv: params.paperclipRuntimeEnv, | ||
| paperclipRuntimeAuth: params.paperclipRuntimeAuth, |
There was a problem hiding this comment.
Forward Paperclip runtime env into attempt tool wiring
Passing paperclipRuntimeEnv/paperclipRuntimeAuth into runEmbeddedAttempt here does not actually deliver those values to the exec tool, because run/attempt.ts builds createOpenClawCodingTools(...) without forwarding either field (see src/agents/pi-embedded-runner/run/attempt.ts:1779-1834). In the real embedded-run path, exec still runs without PAPERCLIP_* variables, so Paperclip-authenticated commands continue to fail even though this commit adds plumbing and a regression test.
Useful? React with 👍 / 👎.
|
Codex review: found issues before merge. Summary Reproducibility: yes. Static inspection is enough to reproduce the PR defects: the branch uses fields missing from current main types, Next step before merge Security Review findings
Review detailsBest possible solution: Decide the product boundary first; if Paperclip exec env is obsolete, close this PR as superseded by provider runtime auth, otherwise rebuild it as a typed end-to-end exec env contract with attempt forwarding, exec merging, sanitizer review, docs, and regression tests. Do we have a high-confidence way to reproduce the issue? Yes. Static inspection is enough to reproduce the PR defects: the branch uses fields missing from current main types, Is this the best way to solve the issue? No. The PR is not the best current fix because it adds Paperclip-specific core plumbing only partway through the pipeline; the safer path is either the existing generic provider runtime-auth mechanism or a fully typed and reviewed generic exec-env contract. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against f523620abe7f. |
Summary
paperclipRuntimeEnvwhen wiring exec tool defaults increateOpenClawCodingToolsPAPERCLIP_AUTH_HEADERandPAPERCLIP_API_KEYreach the exec toolRoot cause
createOpenClawCodingToolswas building the exec tool with:paperclipRuntimeEnv: options?.paperclipRuntimeEnvBut the runtime auth env was being attached under:
options.exec.paperclipRuntimeEnvThat mismatch meant Paperclip runtime auth could be present upstream, but get dropped at the
pi-tools.ts -> createExecTool()callsite before reaching exec defaults.Why this change
This is a small env-plumbing bugfix, not a new integration surface:
Validation
src/agents/pi-tools.paperclip-runtime-env.test.tspnpm vitest run src/agents/pi-tools.paperclip-runtime-env.test.tsnpm run buildNotes
During local validation, the failure manifested as issue-bound runs receiving runtime auth upstream but losing
PAPERCLIP_*env before exec. This change fixes that last-hop drop.