Limit shell inheritance of pi provider keys (TASK-139)

cli/__tests__/adapters.pi.shell-env.test.mjs

@@ -0,0 +1,91 @@
+import { jest } from '@jest/globals';
+import { readFileSync } from 'node:fs';
+import { createContext, SourceTextModule, SyntheticModule } from 'node:vm';
+
+const execute = jest.fn();
+const localExec = jest.fn();
+const createBashTool = jest.fn(() => ({ name: 'bash', execute }));
+// pi resolves its own bundled SDK inside its extension loader. It is not a CLI
+// dependency, so link that import explicitly while evaluating the shipped file.
+const context = createContext({ process });
+const sdk = new SyntheticModule(['createBashTool', 'createLocalBashOperations'], function linkSdk() {
+  this.setExport('createBashTool', createBashTool);
+  this.setExport('createLocalBashOperations', () => ({ exec: localExec }));
+}, { context });
+const extension = new SourceTextModule(readFileSync(new URL('../src/lib/adapters/pi-shell-env.mjs', import.meta.url), 'utf8'), { context });
+await extension.link((name) => {
+  expect(name).toBe('@earendil-works/pi-coding-agent');
+  return sdk;
+});
+await extension.evaluate();
+const install = extension.namespace.default;
+
+let tool;
+let manual;
+beforeEach(() => {
+  jest.clearAllMocks();
+  process.env.COMMONLY_PI_PROVIDER_KEY_ENV = 'CUSTOM_PROVIDER_KEY';
+  process.env.CUSTOM_PROVIDER_KEY = 'fake-provider-key';
+  process.env.COMMONLY_LITELLM_KEY = 'fake-default-key';
+  install({
+    registerTool(value) { tool = value; },
+    on(name, handler) { expect(name).toBe('user_bash'); manual = handler; },
+  });
+});
+afterEach(() => {
+  delete process.env.COMMONLY_PI_PROVIDER_KEY_ENV;
+  delete process.env.CUSTOM_PROVIDER_KEY;
+  delete process.env.COMMONLY_LITELLM_KEY;
+});
+
+test('model bash strips the configured and default keys without mutating provider auth', async () => {
+  const hook = createBashTool.mock.calls[0][1].spawnHook;
+  const env = { ...process.env, SAFE_VALUE: 'keep' };
+  const result = hook({ command: 'env', cwd: '/seat', env });
+  expect(result.command).toBe('env');
+  expect(result.cwd).toBe('/seat');
+  expect(result.env.CUSTOM_PROVIDER_KEY).toBeUndefined();
+  expect(result.env.COMMONLY_LITELLM_KEY).toBeUndefined();
+  expect(result.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBeUndefined();
+  expect(result.env.SAFE_VALUE).toBe('keep');
+  expect(env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key');
+  expect(process.env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key');
+
+  execute.mockResolvedValue('result');
+  await expect(tool.execute('id', { command: 'pwd' }, 'signal', 'update', { cwd: '/current' }))
+    .resolves.toBe('result');
+  expect(createBashTool.mock.calls[1]).toEqual(['/current', { spawnHook: hook }]);
+  expect(execute).toHaveBeenCalledWith('id', { command: 'pwd' }, 'signal', 'update', { cwd: '/current' });
+});
+
+test('mixed-case excluded names cannot survive as plain-object keys', () => {
+  const env = {
+    Custom_Provider_Key: 'fake-custom', commonly_litellm_key: 'fake-default',
+    Commonly_PI_Provider_Key_Env: 'CUSTOM_PROVIDER_KEY', Path: '/safe',
+  };
+  const hook = createBashTool.mock.calls[0][1].spawnHook;
+  expect(hook({ env }).env).toEqual({ Path: '/safe' });
+  manual().operations.exec('env', '/seat', { env });
+  expect(localExec.mock.calls[0][2].env).toEqual({ Path: '/safe' });
+  expect(env.Custom_Provider_Key).toBe('fake-custom');
+});
+
+test.each([true, false])('manual shell strips credentials with explicit env = %s', async (explicit) => {
+  const env = { ...process.env, SAFE_VALUE: 'keep' };
+  const options = { signal: 'signal', timeout: 10, ...(explicit ? { env } : {}) };
+  localExec.mockResolvedValue({ exitCode: 0 });
+  await expect(manual().operations.exec('env', '/seat', options)).resolves.toEqual({ exitCode: 0 });
+  const [command, cwd, passed] = localExec.mock.calls[0];
+  expect([command, cwd, passed.signal, passed.timeout]).toEqual(['env', '/seat', 'signal', 10]);
+  expect(passed.env.CUSTOM_PROVIDER_KEY).toBeUndefined();
+  expect(passed.env.COMMONLY_LITELLM_KEY).toBeUndefined();
+  expect(passed.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBeUndefined();
+  if (explicit) expect(passed.env.SAFE_VALUE).toBe('keep');
+  expect(env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key');
+  expect(process.env.CUSTOM_PROVIDER_KEY).toBe('fake-provider-key');
+});
+
+test('missing provider-key metadata refuses extension setup', () => {
+  delete process.env.COMMONLY_PI_PROVIDER_KEY_ENV;
+  expect(() => install({})).toThrow('missing provider key environment name');
+});

cli/__tests__/adapters.pi.test.mjs

@@ -84,7 +84,9 @@ describe('spawn', () => {
     expect(args[args.indexOf('--session-id') + 1]).toBe(res.newSessionId);
     expect(args).not.toContain('--session');
     expect(args[args.length - 1]).toContain('hello');
-    expect(args).not.toContain('-e'); // no MCP declared → no bridge
+    expect(args.filter((arg) => arg === '-e')).toHaveLength(1);
+    expect(args[args.indexOf('-e') + 1]).toMatch(/pi-shell-env\.mjs$/);
+    expect(opts.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBe('COMMONLY_LITELLM_KEY');
     expect(opts.cwd).toBe('/tmp/seat');
     expect(opts.env.PI_CODING_AGENT_DIR).toBe(join(home, 'agent'));
     // Provider by env reference: the key never lands in models.json.
@@ -247,7 +249,8 @@ describe('spawn', () => {
       // No list means no fd 3 either: the pipe exists only when there is a secret.
       expect(calls[0].opts.stdio).toEqual(['ignore', 'pipe', 'pipe']);
       expect(calls[0].proc.fd3Payload).toBe('');
-      expect(calls[0].args).not.toContain('-e');
+      expect(calls[0].args).not.toContain('/x/bridge.mjs');
+      expect(calls[0].args[calls[0].args.indexOf('-e') + 1]).toMatch(/pi-shell-env\.mjs$/);
       expect(JSON.stringify(calls[0].opts.env)).not.toContain('cm_agent_secret');
       expect(warn.mock.calls.filter(([line]) => line.includes(GRANT_BROKER_REFUSAL))).toHaveLength(1);
     } finally {
@@ -258,6 +261,8 @@ describe('spawn', () => {
   test('the environment can name its own provider and model; effort maps onto pi\'s ladder', async () => {
     const { impl, calls } = makeSpawnImpl({ stdout: assistant('x') });
     await pi.spawn('p', baseCtx({ _spawnImpl: impl, env: { DS_KEY: 'k' }, environment: { model: 'deepseek-flash', effort: 'none', provider: { name: 'deepseek', baseUrl: 'https://api.deepseek.com/v1', apiKeyEnv: 'DS_KEY' } } }));
+    expect(calls[0].opts.env.COMMONLY_PI_PROVIDER_KEY_ENV).toBe('DS_KEY');
+    expect(calls[0].opts.env.DS_KEY).toBe('k');
     const models = JSON.parse(await readFile(join(home, 'agent', 'models.json'), 'utf8'));
     expect(models.providers.deepseek.apiKey).toBe('$DS_KEY');
     expect(calls[0].args).toEqual(expect.arrayContaining(['--provider', 'deepseek', '--model', 'deepseek-flash', '--thinking', 'off']));

cli/src/lib/adapters/pi-shell-env.mjs

@@ -0,0 +1,41 @@
+/**
+ * Keep the provider key available to pi's per-call auth resolver, but out of
+ * shells started by its model bash tool and by manual ! / !! commands.
+ * This limits environment inheritance; pi itself is not an OS sandbox.
+ */
+import { createBashTool, createLocalBashOperations } from '@earendil-works/pi-coding-agent';
+
+export default function commonlyShellEnvironment(pi) {
+  const providerKey = process.env.COMMONLY_PI_PROVIDER_KEY_ENV;
+  if (!providerKey) throw new Error('pi adapter: missing provider key environment name');
+  const excluded = new Set([
+    providerKey, 'COMMONLY_LITELLM_KEY', 'COMMONLY_PI_PROVIDER_KEY_ENV',
+  ].map((key) => key.toUpperCase()));
+  const cleanEnv = (env) => {
+    const clean = { ...env };
+    // Windows environment lookup ignores case; spreading it loses that behavior.
+    for (const key of Object.keys(clean)) {
+      if (excluded.has(key.toUpperCase())) delete clean[key];
+    }
+    return clean;
+  };
+  const spawnHook = (context) => ({ ...context, env: cleanEnv(context.env) });
+  const bash = createBashTool(process.cwd(), { spawnHook });
+  pi.registerTool({
+    ...bash,
+    execute(id, params, signal, onUpdate, ctx) {
+      return createBashTool(ctx.cwd, { spawnHook }).execute(id, params, signal, onUpdate, ctx);
+    },
+  });
+  const local = createLocalBashOperations();
+  pi.on('user_bash', () => ({
+    operations: {
+      exec(command, cwd, options) {
+        return local.exec(command, cwd, {
+          ...options,
+          env: cleanEnv(options.env ?? process.env),
+        });
+      },
+    },
+  }));
+}

cli/src/lib/adapters/pi.js

@@ -72,6 +72,7 @@ const THINKING = { none: 'off', off: 'off', minimal: 'minimal', low: 'low', medi
 export const thinkingFor = (effort) => (effort ? THINKING[String(effort).toLowerCase()] || null : null);
 
 const BRIDGE_PATH = join(dirname(fileURLToPath(import.meta.url)), 'pi-commonly-mcp.mjs');
+const SHELL_ENV_PATH = join(dirname(fileURLToPath(import.meta.url)), 'pi-shell-env.mjs');
 
 // Same substitution contract as claude.js / codex.js: ${COMMONLY_*}
 // placeholders in the declared MCP env are the wrapper's per-(agent, pod)
@@ -457,6 +458,9 @@ export default {
         ...baseEnv,
         PI_CODING_AGENT_DIR: agentDir,
         PI_SKIP_VERSION_CHECK: '1',
+        // A variable NAME, never its value. The always-loaded extension uses
+        // this to scrub custom provider keys as well as the default LiteLLM key.
+        COMMONLY_PI_PROVIDER_KEY_ENV: provider.apiKeyEnv,
       };
       // `baseEnv` is normally the process environment, which carries the
       // bootstrap export. Nothing below this process needs the value: the bridge
@@ -472,6 +476,8 @@ export default {
         prompt: fullPrompt, provider: provider.name, model, thinking, sessionId, isResume, sessionDir,
         bridge: servers.length ? (ctx._bridgePath || BRIDGE_PATH) : null,
       });
+      // Shell isolation must not depend on whether this turn declares MCP.
+      args.splice(args.length - 1, 0, '-e', SHELL_ENV_PATH);
 
       const reply = await runPi({
         args,

docs/cli/README.md

@@ -86,6 +86,21 @@ commonly <command>
 
 Requires Node 20+. No compiled build step — source is ESM.
 
+### pi provider credentials
+
+The pi adapter keeps its configured provider key in pi's environment because
+pi resolves authentication on each model call. The always-loaded
+`pi-shell-env.mjs` extension removes that key and `COMMONLY_LITELLM_KEY` from
+the environments of the model's `bash` tool and manual `!` / `!!` commands.
+It loads even when no MCP server is declared. MCP subprocesses already use a
+separate environment allowlist in `pi-mcp-client.mjs`.
+
+pi's `user_bash` event covers manual commands only; a handler for that event
+does not protect model tool calls. Both paths are exercised with pi 0.84.1.
+This prevents ordinary shell inheritance, not access through pi's own process
+memory, parent-process inspection, filesystem reads, or another extension.
+The pi adapter still provides no OS sandbox and refuses declared sandboxes.
+
 ---
 
 ## Command reference