fix(cli): strip CWD .env leakage and respect platform adapters in serve#1068
Closed
fix(cli): strip CWD .env leakage and respect platform adapters in serve#1068
Conversation
…in serve Two related bugs in the #1067 class — both violate "CLI should never load target repo env" and "archon serve should respect configured adapters". 1. CWD .env leak (partially addressed by override:true in v0.3.5, still leaked) Bun's runtime auto-loads `.env`/`.env.local`/`.env.development`/`.env.production` from CWD into `process.env` before any user code runs. When `archon` is invoked from inside a target repo, that repo's .env leaked into the Archon process: - Overlapping keys were overridden by `~/.archon/.env` (override:true) ✓ - Non-overlapping keys (LOG_LEVEL, MAX_CONCURRENT_CONVERSATIONS, random markers, etc.) survived and contaminated logging, config, and every `process.env.X` read outside the subprocess allowlist. Fix: stripCwdEnv() parses the CWD .env files, deletes matching keys from process.env, and must run BEFORE any module that reads env at init time (notably @archon/paths/logger which reads LOG_LEVEL in buildLogger()). Exposed as a side-effect boot subpath @archon/paths/strip-cwd-env-boot so it runs during import resolution, before the logger module loads. 2. archon serve hardcoded skipPlatformAdapters:true serve.ts passed `skipPlatformAdapters: true` unconditionally, making every `archon serve` run Web-only regardless of tokens in ~/.archon/.env. The setup wizard writes TELEGRAM_BOT_TOKEN / DISCORD_BOT_TOKEN / etc., but nothing ever read them. Fix: remove the hardcoded true. startServer() already gates each adapter on its own token env var, so adapters auto-start only when configured. Validation: - All 9 package type-checks clean - Lint clean, format clean, all tests pass (including 6 new tests) - E2E: `archon workflow run archon-assist` from a target repo with LOG_LEVEL=debug in its .env now produces info-level logs (leak gone) and completes successfully - E2E: `bun --filter @archon/server dev` starts github, discord, slack adapters when their tokens are present
📝 WalkthroughWalkthroughThe changes introduce a new CWD environment variable stripping module that executes during module initialization to sanitize Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paths/src/strip-cwd-env.ts`:
- Around line 34-39: The current deletion unconditionally removes any env var
whose key appears in the target repo's .env, which can wipe legitimate
shell-provided values; update the stripCwdEnv logic to only delete when the
existing process.env[key] appears to contain a CWD-leak (e.g., its value
includes or startsWith the detected cwd or the leaked path pattern you computed)
or matches the exact value read from the repo .env, otherwise leave
process.env[key] intact and skip pushing key into stripped; locate this logic in
the stripCwdEnv function where the variable key is checked and deleted and
adjust the conditional to verify the value contains the cwd/leaked path before
performing delete and pushing to stripped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05d9a28e-a28a-4403-a2ab-bacad66742ac
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/cli/src/cli.tspackages/cli/src/commands/serve.tspackages/paths/package.jsonpackages/paths/src/index.tspackages/paths/src/strip-cwd-env-boot.tspackages/paths/src/strip-cwd-env.test.tspackages/paths/src/strip-cwd-env.tspackages/server/src/index.ts
Comment on lines
+34
to
+39
| if (key in process.env) { | ||
| // Dynamic delete is required: keys come from the target repo's .env | ||
| // at runtime, so they cannot be known statically. | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete process.env[key]; | ||
| stripped.push(key); |
There was a problem hiding this comment.
Avoid deleting shell-provided env vars while stripping CWD leakage
At Line 34, the current key in process.env check deletes any existing env var if that key is present in CWD .env*. This can wipe legitimate operator/shell values (including critical vars) that were never leaked by Bun.
💡 Proposed fix
- for (const key of Object.keys(parsed)) {
- if (key in process.env) {
+ for (const key of Object.keys(parsed)) {
+ const currentValue = process.env[key];
+ // Strip only when process.env currently matches the CWD .env value.
+ // This avoids deleting externally provided env vars with different values.
+ if (currentValue !== undefined && currentValue === parsed[key]) {
// Dynamic delete is required: keys come from the target repo's .env
// at runtime, so they cannot be known statically.
// eslint-disable-next-line `@typescript-eslint/no-dynamic-delete`
delete process.env[key];
stripped.push(key);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (key in process.env) { | |
| // Dynamic delete is required: keys come from the target repo's .env | |
| // at runtime, so they cannot be known statically. | |
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | |
| delete process.env[key]; | |
| stripped.push(key); | |
| for (const key of Object.keys(parsed)) { | |
| const currentValue = process.env[key]; | |
| // Strip only when process.env currently matches the CWD .env value. | |
| // This avoids deleting externally provided env vars with different values. | |
| if (currentValue !== undefined && currentValue === parsed[key]) { | |
| // Dynamic delete is required: keys come from the target repo's .env | |
| // at runtime, so they cannot be known statically. | |
| // eslint-disable-next-line `@typescript-eslint/no-dynamic-delete` | |
| delete process.env[key]; | |
| stripped.push(key); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paths/src/strip-cwd-env.ts` around lines 34 - 39, The current
deletion unconditionally removes any env var whose key appears in the target
repo's .env, which can wipe legitimate shell-provided values; update the
stripCwdEnv logic to only delete when the existing process.env[key] appears to
contain a CWD-leak (e.g., its value includes or startsWith the detected cwd or
the leaked path pattern you computed) or matches the exact value read from the
repo .env, otherwise leave process.env[key] intact and skip pushing key into
stripped; locate this logic in the stripCwdEnv function where the variable key
is checked and deleted and adjust the conditional to verify the value contains
the cwd/leaked path before performing delete and pushing to stripped.
This was referenced Apr 11, 2026
Collaborator
Author
Collaborator
Author
|
Superseded by #1092 (now merged). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two sub-issues reported in #1067. Both violate the design rule "CLI should never load target repo env" and "archon serve should respect configured adapters".
1. CWD .env leak (partially addressed in v0.3.5, still leaked)
Bug: Bun's runtime auto-loads CWD `.env` / `.env.local` / `.env.development` / `.env.production` into `process.env` before any user code runs. When `archon` is invoked from inside a target repo, that repo's .env leaked into the Archon process. The v0.3.5 `override: true` fix only covered overlapping keys:
Reproduction on archon-stable (v0.3.5):
```bash
mkdir /tmp/leak-test && cd /tmp/leak-test && git init
echo 'LOG_LEVEL=debug' > .env
archon workflow list # logs come out at level:20 (debug) — LEAKED
```
Fix: `stripCwdEnv()` parses CWD .env files and deletes matching keys from `process.env`. Must run before any module that reads env at init time — notably `@archon/paths/logger` which reads `LOG_LEVEL` in `buildLogger()` at module load. Exposed as a side-effect boot subpath `@archon/paths/strip-cwd-env-boot` so it runs during import resolution, before the logger module loads.
```typescript
// packages/cli/src/cli.ts — must be the first import
import '@archon/paths/strip-cwd-env-boot';
```
The function itself (`@archon/paths/strip-cwd-env`) is a pure testable unit; the boot wrapper is the side-effect entry point.
2. `archon serve` hardcoded `skipPlatformAdapters: true`
Bug: `packages/cli/src/commands/serve.ts:63` passed `skipPlatformAdapters: true` unconditionally, making every `archon serve` invocation Web-only regardless of what tokens are in `~/.archon/.env`. The setup wizard writes `TELEGRAM_BOT_TOKEN` / `DISCORD_BOT_TOKEN` / `SLACK_*` etc., but nothing ever read them from `serve`.
Fix: Remove the hardcoded `true`. `startServer()` already gates each adapter individually on its own token env var (`packages/server/src/index.ts:299-654`), so adapters auto-start only when configured. Users who only want the web UI simply leave the tokens unset — the server logs `no_platform_adapters_configured` and runs Web-only.
Validation
Files
Rollback
`git revert ` — single commit, isolated to env boot and one-line serve.ts change.
Summary by CodeRabbit
New Features
Bug Fixes
Tests