security: prevent target repo .env from leaking into subprocesses (#1135)#1169
Merged
security: prevent target repo .env from leaking into subprocesses (#1135)#1169
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the environment-consent/bypass model end-to-end: deletes allow_env_keys / allow_target_repo_keys / --allow-env-keys surfaces, removes the env-leak scanner and its exports, eliminates API/UI/CLI consent endpoints and tests, and hardens subprocess invocation to avoid auto-loading repo .env files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant CloneSvc as Clone/Register
participant DB as Database
Client->>Server: POST /api/codebases { url | path }
Server->>CloneSvc: cloneRepository(url) / registerRepository(path)
CloneSvc->>Server: registration result (name, repository_url, default_cwd, ai_assistant_type)
Server->>DB: createCodebase(data) %% no allow_env_keys field
DB-->>Server: created Codebase
Server-->>Client: 201 Created (codebase payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Wirasm
force-pushed
the
feature/remove-env-consent-model
branch
from
003d3c6 to
983c5b5
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/server/src/routes/api.ts (1)
1504-1510:⚠️ Potential issue | 🟠 MajorRemove
allowEnvKeysfrom the published request schema too.The handler no longer honors the flag, but the generated client contract still exposes
AddCodebaseBody.allowEnvKeysinpackages/web/src/lib/api.generated.d.ts:2021-2025. That leaves a silent no-op bypass knob in the public API. Please drop the field frompackages/server/src/routes/schemas/codebase.schemas.tsand regenerate the frontend types before merging; if you want stale clients to fail loudly instead of being ignored, make the object schema strict as well.🧩 Suggested fix
// packages/server/src/routes/schemas/codebase.schemas.ts export const addCodebaseBodySchema = z.object({ url: z.string().optional(), path: z.string().optional(), - allowEnvKeys: z.boolean().optional(), });bun generate:typesAs per coding guidelines,
packages/server/src/**/*.ts: "Use Hono OpenAPI integration with Zod schemas for REST API routes; generate OpenAPI spec and frontend types viabun generate:types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 1504 - 1510, Remove the deprecated allowEnvKeys field from the published request schema by deleting it from the Zod schema export (addCodebaseBodySchema) in codebase.schemas.ts so the handler and OpenAPI contract match; then regenerate frontend types (run the project type generation, e.g. bun generate:types) so packages/web no longer exposes AddCodebaseBody.allowEnvKeys — optionally make the object schema strict to cause stale clients to fail loudly instead of being ignored.packages/web/src/lib/api.ts (1)
159-166:⚠️ Potential issue | 🟠 MajorRegenerate the OpenAPI contract instead of narrowing this wrapper by hand.
packages/web/src/lib/api.generated.d.ts:1-50still advertisesCreateCodebaseBody.allowEnvKeys,UpdateCodebaseBody, and the PATCH consent route. NarrowingaddCodebase()locally hides that drift in the handwritten client, but the published web contract is still stale and still exposes the removed consent model. Please update/regenerate the OpenAPI types in this PR and then type this wrapper from the generated schema again so the web client has a single source of truth.Based on learnings,
archon/webderives types fromsrc/lib/api.generated.d.ts(generated OpenAPI spec).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/api.ts` around lines 159 - 166, The handwritten addCodebase wrapper was narrowed locally and hides that the generated OpenAPI types (packages/web/src/lib/api.generated.d.ts) are stale and still expose removed fields like CreateCodebaseBody.allowEnvKeys, UpdateCodebaseBody and the PATCH consent route; regenerate the OpenAPI client/types so the generated file is up-to-date, then change addCodebase to consume the generated type (e.g., use the generated CreateCodebaseBody / CodebaseResponse types from api.generated.d.ts) instead of a manually authored union, and remove any handwritten type overrides so the web client derives its types from the single generated contract.packages/web/src/routes/SettingsPage.tsx (1)
264-283:⚠️ Potential issue | 🟡 MinorKeep add-project input classification consistent across screens.
packages/web/src/routes/ChatPage.tsxandpackages/web/src/components/layout/Sidebar.tsxstill choose{ url }for remote repos and{ path }for local ones before callingaddCodebase(). This mutation now always posts{ path }, so the same Git URL will be classified differently depending on where the user adds the project. Please reuse the existing URL-vs-path logic here, or extract a shared helper now that the pattern exists in three entry points.As per coding guidelines, "Extract shared utilities only after the same pattern appears at least three times and has stabilized (DRY + Rule of Three principle)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/SettingsPage.tsx` around lines 264 - 283, The add-project payload in SettingsPage (handleAddSubmit -> addMutation -> addCodebase) always sends { path }, causing inconsistent classification versus ChatPage.tsx and Sidebar.tsx which send { url } for remotes; update SettingsPage to use the same URL-vs-path logic used in ChatPage.tsx and Sidebar.tsx (or extract a small shared helper like buildCodebasePayload or isRemoteRepo and call it from handleAddSubmit) so addMutation calls addCodebase with either { url } for remote repos or { path } for local ones, preserving the existing behavior of addCodebase across all entry points.
🧹 Nitpick comments (3)
CLAUDE.md (1)
767-767: Add the migration note here.This removes the old flag from the docs, but it still doesn’t tell users what to do instead. A short note that target-repo
.envvalues are never forwarded anymore, and that managed env injection (config.yaml env:/ codebase env vars) is the supported replacement, would make upgrades much less surprising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 767, Update the documentation entry for "POST /api/codebases" to include a short migration note informing users that the old flag has been removed and that target-repo `.env` values are no longer forwarded; instruct them to use managed env injection instead by setting env values in `config.yaml` under `env:` or by using the codebase-level environment variables mechanism as the supported replacement, and mention any necessary steps to move existing `.env` keys to the new location.packages/providers/src/env.test.ts (1)
7-12: AddXDG_CONFIG_HOMEto saved env vars for consistent cleanup.The
XDG_CONFIG_HOMEtest (lines 40-46) sets this env var but relies on inline cleanup. If the test fails beforedelete, it could pollute subsequent tests. Adding it tosavedEnvensures consistent cleanup viaafterEach.♻️ Suggested fix
beforeEach(() => { // Save keys we'll modify - for (const key of ['CUSTOM_REPO_KEY', 'ANTHROPIC_API_KEY', 'PATH', 'HOME']) { + for (const key of ['CUSTOM_REPO_KEY', 'ANTHROPIC_API_KEY', 'PATH', 'HOME', 'XDG_CONFIG_HOME']) { savedEnv[key] = process.env[key]; } });Then remove the manual cleanup at line 45.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/env.test.ts` around lines 7 - 12, Add "XDG_CONFIG_HOME" to the array of keys saved in the beforeEach so savedEnv captures it (update the for loop initialization that currently lists 'CUSTOM_REPO_KEY','ANTHROPIC_API_KEY','PATH','HOME'); this ensures afterEach restores XDG_CONFIG_HOME if a test fails. Also remove the manual delete of process.env.XDG_CONFIG_HOME inside the XDG_CONFIG_HOME test (the inline cleanup referenced in the test) since global savedEnv/afterEach will handle restoration.packages/providers/src/env.ts (1)
28-53: Consider adding Windows-specific environment variables.The allowlist covers Unix/macOS essentials but may be incomplete for Windows subprocess functionality. Windows uses different variables like
USERPROFILE(instead ofHOME),COMSPEC,SYSTEMROOT,PATHEXT, andAPPDATA.Since the learnings indicate this is a single-developer tool, this may not be a priority, but worth noting if Windows support is intended.
♻️ Windows-compatible allowlist additions
const OS_ESSENTIALS: ReadonlySet<string> = new Set([ 'PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'TERM', 'COLORTERM', 'FORCE_COLOR', 'NO_COLOR', 'TMPDIR', 'TEMP', 'TMP', 'LANG', 'LC_ALL', 'LC_COLLATE', 'LC_CTYPE', 'LC_MESSAGES', 'LC_MONETARY', 'LC_NUMERIC', 'LC_TIME', 'HOSTNAME', 'EDITOR', 'VISUAL', 'PAGER', + // Windows equivalents + 'USERPROFILE', + 'APPDATA', + 'LOCALAPPDATA', + 'SYSTEMROOT', + 'COMSPEC', + 'PATHEXT', + 'PROGRAMFILES', + 'WINDIR', ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/env.ts` around lines 28 - 53, The OS_ESSENTIALS allowlist omits common Windows environment variables which can break subprocesses on Windows; update the ReadonlySet named OS_ESSENTIALS to include Windows-specific entries such as USERPROFILE, COMSPEC, SYSTEMROOT, PATHEXT, APPDATA (and optionally LOCALAPPDATA, PROGRAMFILES, PROGRAMFILES(X86), and WINDIR) so that Windows child processes receive expected environment keys; modify the OS_ESSENTIALS declaration in the same module to add these symbols to the set and keep the list ReadonlySet semantics intact.
🤖 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/core/src/db/codebases.ts`:
- Around line 23-24: The INSERT currently uses RETURNING * and the module's
SELECT helpers still use SELECT *, which causes the deprecated allow_env_keys to
be returned; update the INSERT and all SELECT helpers in this file to use the
existing codebaseColumns projection instead of * so the allow_env_keys column is
excluded — locate the queries in functions like the create/insert method (where
RETURNING * is used) and any get/list/select helpers and swap their
RETURNING/SELECT * to reference the codebaseColumns symbol (or the same column
list) so responses no longer include allow_env_keys.
---
Outside diff comments:
In `@packages/server/src/routes/api.ts`:
- Around line 1504-1510: Remove the deprecated allowEnvKeys field from the
published request schema by deleting it from the Zod schema export
(addCodebaseBodySchema) in codebase.schemas.ts so the handler and OpenAPI
contract match; then regenerate frontend types (run the project type generation,
e.g. bun generate:types) so packages/web no longer exposes
AddCodebaseBody.allowEnvKeys — optionally make the object schema strict to cause
stale clients to fail loudly instead of being ignored.
In `@packages/web/src/lib/api.ts`:
- Around line 159-166: The handwritten addCodebase wrapper was narrowed locally
and hides that the generated OpenAPI types
(packages/web/src/lib/api.generated.d.ts) are stale and still expose removed
fields like CreateCodebaseBody.allowEnvKeys, UpdateCodebaseBody and the PATCH
consent route; regenerate the OpenAPI client/types so the generated file is
up-to-date, then change addCodebase to consume the generated type (e.g., use the
generated CreateCodebaseBody / CodebaseResponse types from api.generated.d.ts)
instead of a manually authored union, and remove any handwritten type overrides
so the web client derives its types from the single generated contract.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 264-283: The add-project payload in SettingsPage (handleAddSubmit
-> addMutation -> addCodebase) always sends { path }, causing inconsistent
classification versus ChatPage.tsx and Sidebar.tsx which send { url } for
remotes; update SettingsPage to use the same URL-vs-path logic used in
ChatPage.tsx and Sidebar.tsx (or extract a small shared helper like
buildCodebasePayload or isRemoteRepo and call it from handleAddSubmit) so
addMutation calls addCodebase with either { url } for remote repos or { path }
for local ones, preserving the existing behavior of addCodebase across all entry
points.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 767: Update the documentation entry for "POST /api/codebases" to include
a short migration note informing users that the old flag has been removed and
that target-repo `.env` values are no longer forwarded; instruct them to use
managed env injection instead by setting env values in `config.yaml` under
`env:` or by using the codebase-level environment variables mechanism as the
supported replacement, and mention any necessary steps to move existing `.env`
keys to the new location.
In `@packages/providers/src/env.test.ts`:
- Around line 7-12: Add "XDG_CONFIG_HOME" to the array of keys saved in the
beforeEach so savedEnv captures it (update the for loop initialization that
currently lists 'CUSTOM_REPO_KEY','ANTHROPIC_API_KEY','PATH','HOME'); this
ensures afterEach restores XDG_CONFIG_HOME if a test fails. Also remove the
manual delete of process.env.XDG_CONFIG_HOME inside the XDG_CONFIG_HOME test
(the inline cleanup referenced in the test) since global savedEnv/afterEach will
handle restoration.
In `@packages/providers/src/env.ts`:
- Around line 28-53: The OS_ESSENTIALS allowlist omits common Windows
environment variables which can break subprocesses on Windows; update the
ReadonlySet named OS_ESSENTIALS to include Windows-specific entries such as
USERPROFILE, COMSPEC, SYSTEMROOT, PATHEXT, APPDATA (and optionally LOCALAPPDATA,
PROGRAMFILES, PROGRAMFILES(X86), and WINDIR) so that Windows child processes
receive expected environment keys; modify the OS_ESSENTIALS declaration in the
same module to add these symbols to the set and keep the list ReadonlySet
semantics intact.
🪄 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: f82d3a69-e4ee-4f43-8ac7-f07e79294c94
📒 Files selected for processing (31)
CLAUDE.mdpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/package.jsonpackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/handlers/command-handler.test.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.tspackages/providers/src/env.test.tspackages/providers/src/env.tspackages/providers/src/index.tspackages/server/src/index.tspackages/server/src/routes/api.codebases.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/codebase.schemas.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/executor-shared.ts
💤 Files with no reviewable changes (16)
- packages/core/src/index.ts
- packages/core/src/types/index.ts
- packages/core/src/db/adapters/sqlite.ts
- packages/workflows/src/executor-shared.ts
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/cli/src/cli.ts
- packages/core/src/config/config-types.ts
- packages/core/src/handlers/command-handler.test.ts
- packages/core/src/orchestrator/orchestrator-isolation.test.ts
- packages/server/src/index.ts
- packages/core/src/orchestrator/orchestrator-agent.test.ts
- packages/core/src/config/config-loader.ts
- packages/server/src/routes/schemas/codebase.schemas.ts
- packages/core/src/handlers/clone.test.ts
- packages/core/src/utils/env-leak-scanner.test.ts
- packages/core/src/utils/env-leak-scanner.ts
Comment on lines
+23
to
+24
| 'INSERT INTO remote_agent_codebases (name, repository_url, default_cwd, ai_assistant_type) VALUES ($1, $2, $3, $4) RETURNING *', | ||
| [data.name, data.repository_url ?? null, data.default_cwd, assistantType] |
There was a problem hiding this comment.
Stop returning the legacy allow_env_keys field.
Dropping the column from the INSERT is only half of the removal. Because the table still keeps allow_env_keys, this RETURNING * and the other SELECT * queries in this module will continue to surface the deprecated consent flag at runtime. packages/server/src/routes/api.ts returns those Codebase rows directly, so the removed primitive can still leak back out over GET/POST responses.
🧩 Suggested fix
+const codebaseColumns =
+ 'id, name, repository_url, default_cwd, ai_assistant_type, commands, created_at, updated_at';
+
export async function createCodebase(data: {
name: string;
repository_url?: string;
default_cwd: string;
ai_assistant_type?: string;
}): Promise<Codebase> {
const assistantType = data.ai_assistant_type ?? 'claude';
const result = await pool.query<Codebase>(
- 'INSERT INTO remote_agent_codebases (name, repository_url, default_cwd, ai_assistant_type) VALUES ($1, $2, $3, $4) RETURNING *',
+ `INSERT INTO remote_agent_codebases (name, repository_url, default_cwd, ai_assistant_type)
+ VALUES ($1, $2, $3, $4)
+ RETURNING ${codebaseColumns}`,
[data.name, data.repository_url ?? null, data.default_cwd, assistantType]
);Apply the same codebaseColumns projection to the SELECT * helpers in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/db/codebases.ts` around lines 23 - 24, The INSERT currently
uses RETURNING * and the module's SELECT helpers still use SELECT *, which
causes the deprecated allow_env_keys to be returned; update the INSERT and all
SELECT helpers in this file to use the existing codebaseColumns projection
instead of * so the allow_env_keys column is excluded — locate the queries in
functions like the create/insert method (where RETURNING * is used) and any
get/list/select helpers and swap their RETURNING/SELECT * to reference the
codebaseColumns symbol (or the same column list) so responses no longer include
allow_env_keys.
983c5b5 to
c755a96
Compare
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/env-integration.test.ts`:
- Around line 224-229: The test currently loads fixtures with config({ path:
join(cwdDir, '.env.local'), processEnv: {} }) and then blindly injects
localParsed.parsed into process.env, which can produce false-positive passes if
loading fails; update the test to assert the parse succeeded immediately after
each config call (e.g., assert/expect that localParsed.parsed is defined or
throw a clear Error) before mutating process.env for the '.env.local' case and
do the same for the corresponding '.env.development' load (the other parsed
variable), so the test fails fast when a fixture fails to parse.
🪄 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: c96b405c-daa6-49dd-9e32-c34d81a79c48
📒 Files selected for processing (27)
CLAUDE.mdpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/package.jsonpackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/handlers/command-handler.test.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.tspackages/paths/src/env-integration.test.tspackages/server/src/index.tspackages/server/src/routes/api.codebases.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/codebase.schemas.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/executor-shared.ts
💤 Files with no reviewable changes (16)
- packages/core/src/types/index.ts
- packages/core/src/orchestrator/orchestrator-agent.test.ts
- packages/workflows/src/executor-shared.ts
- packages/core/src/db/adapters/sqlite.ts
- packages/server/src/index.ts
- packages/core/src/handlers/command-handler.test.ts
- packages/core/src/index.ts
- packages/core/src/config/config-types.ts
- packages/core/src/config/config-loader.ts
- packages/server/src/routes/schemas/codebase.schemas.ts
- packages/core/src/handlers/clone.test.ts
- packages/core/src/utils/env-leak-scanner.test.ts
- packages/core/src/utils/env-leak-scanner.ts
- packages/cli/src/cli.ts
- packages/core/src/orchestrator/orchestrator-isolation.test.ts
- packages/core/src/orchestrator/orchestrator.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/db/codebases.test.ts
- packages/web/src/routes/SettingsPage.tsx
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/package.json
- packages/web/src/lib/api.ts
- packages/core/src/db/codebases.ts
- packages/server/src/routes/api.codebases.test.ts
- packages/core/src/handlers/clone.ts
Comment on lines
+224
to
+229
| const localParsed = config({ path: join(cwdDir, '.env.local'), processEnv: {} }); | ||
| if (localParsed.parsed) { | ||
| for (const [key, value] of Object.entries(localParsed.parsed)) { | ||
| process.env[key] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert fixture parse success to avoid false-positive test passes.
If the fixture load ever fails, these tests can still pass while not actually validating strip behavior for .env.local/.env.development.
✅ Proposed test-hardening diff
- const localParsed = config({ path: join(cwdDir, '.env.local'), processEnv: {} });
- if (localParsed.parsed) {
- for (const [key, value] of Object.entries(localParsed.parsed)) {
- process.env[key] = value;
- }
- }
+ const localParsed = config({ path: join(cwdDir, '.env.local'), processEnv: {} });
+ expect(localParsed.error).toBeUndefined();
+ expect(localParsed.parsed).toBeDefined();
+ for (const [key, value] of Object.entries(localParsed.parsed ?? {})) {
+ process.env[key] = value;
+ }
- const devParsed = config({ path: join(cwdDir, '.env.development'), processEnv: {} });
- if (devParsed.parsed) {
- for (const [key, value] of Object.entries(devParsed.parsed)) {
- process.env[key] = value;
- }
- }
+ const devParsed = config({ path: join(cwdDir, '.env.development'), processEnv: {} });
+ expect(devParsed.error).toBeUndefined();
+ expect(devParsed.parsed).toBeDefined();
+ for (const [key, value] of Object.entries(devParsed.parsed ?? {})) {
+ process.env[key] = value;
+ }Based on learnings: "Applies to **/*.test.ts : Keep tests deterministic — no flaky timing or network dependence without guardrails. Ensure local validation commands (bun run validate) map directly to CI expectations."
Also applies to: 243-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paths/src/env-integration.test.ts` around lines 224 - 229, The test
currently loads fixtures with config({ path: join(cwdDir, '.env.local'),
processEnv: {} }) and then blindly injects localParsed.parsed into process.env,
which can produce false-positive passes if loading fails; update the test to
assert the parse succeeded immediately after each config call (e.g.,
assert/expect that localParsed.parsed is defined or throw a clear Error) before
mutating process.env for the '.env.local' case and do the same for the
corresponding '.env.development' load (the other parsed variable), so the test
fails fast when a fixture fails to parse.
c755a96 to
2d019b8
Compare
Wirasm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/db/codebases.ts (1)
23-24:⚠️ Potential issue | 🟠 Major
RETURNING *still exposes deprecated consent fields.Even after removing
allow_env_keysfrom the INSERT input, wildcard projections (RETURNING *here andSELECT *in this module) can still surface the legacy column at runtime.Suggested fix (explicit column projection)
+const codebaseColumns = + 'id, name, repository_url, default_cwd, ai_assistant_type, commands, created_at, updated_at'; const result = await pool.query<Codebase>( - 'INSERT INTO remote_agent_codebases (name, repository_url, default_cwd, ai_assistant_type) VALUES ($1, $2, $3, $4) RETURNING *', + `INSERT INTO remote_agent_codebases (name, repository_url, default_cwd, ai_assistant_type) + VALUES ($1, $2, $3, $4) + RETURNING ${codebaseColumns}`, [data.name, data.repository_url ?? null, data.default_cwd, assistantType] );Also replace remaining
SELECT *queries in this file withSELECT ${codebaseColumns}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/db/codebases.ts` around lines 23 - 24, The INSERT uses "RETURNING *" which can leak deprecated columns; update the insert query in the code that constructs the SQL for remote_agent_codebases to explicitly project columns using the existing codebaseColumns projection (replace RETURNING * with RETURNING ${codebaseColumns} or return the inserted row via the same codebaseColumns list), and also replace any "SELECT *" usages in this module with "SELECT ${codebaseColumns}" so only the allowed columns (as defined by the codebaseColumns symbol) are returned.
🤖 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/workflows/src/dag-executor.test.ts`:
- Around line 5266-5313: The test "bun script node does not leak repo .env from
execution cwd (`#1135`)" can be flaky if LEAKED_REPO_SECRET exists in the host
env; modify the test to save process.env.LEAKED_REPO_SECRET at the start,
delete/unset process.env.LEAKED_REPO_SECRET before creating the repo .env and
before calling executeDagWorkflow, and restore the original value in a finally
block after the test completes; ensure the cleanup covers all early returns so
the environment is always restored (reference the test name, the
LEAKED_REPO_SECRET variable, the writeFile call that creates join(testDir,
'.env'), and the executeDagWorkflow invocation to locate where to clear/restore
the env var).
---
Duplicate comments:
In `@packages/core/src/db/codebases.ts`:
- Around line 23-24: The INSERT uses "RETURNING *" which can leak deprecated
columns; update the insert query in the code that constructs the SQL for
remote_agent_codebases to explicitly project columns using the existing
codebaseColumns projection (replace RETURNING * with RETURNING
${codebaseColumns} or return the inserted row via the same codebaseColumns
list), and also replace any "SELECT *" usages in this module with "SELECT
${codebaseColumns}" so only the allowed columns (as defined by the
codebaseColumns symbol) are returned.
🪄 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: 17377761-65a5-4e5c-860d-e22be0d8cb93
📒 Files selected for processing (30)
CLAUDE.mdpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/package.jsonpackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/handlers/command-handler.test.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.tspackages/paths/src/env-integration.test.tspackages/server/src/index.tspackages/server/src/routes/api.codebases.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/codebase.schemas.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.tspackages/workflows/src/script-node-deps.test.ts
💤 Files with no reviewable changes (16)
- packages/core/src/orchestrator/orchestrator-isolation.test.ts
- packages/core/src/handlers/command-handler.test.ts
- packages/core/src/orchestrator/orchestrator-agent.test.ts
- packages/cli/src/cli.ts
- packages/core/src/db/adapters/sqlite.ts
- packages/workflows/src/executor-shared.ts
- packages/core/src/types/index.ts
- packages/core/src/index.ts
- packages/server/src/index.ts
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/core/src/handlers/clone.test.ts
- packages/core/src/config/config-loader.ts
- packages/server/src/routes/schemas/codebase.schemas.ts
- packages/core/src/utils/env-leak-scanner.test.ts
- packages/core/src/config/config-types.ts
- packages/core/src/utils/env-leak-scanner.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/package.json
- packages/core/src/db/codebases.test.ts
- packages/server/src/routes/api.codebases.test.ts
- CLAUDE.md
- packages/web/src/lib/api.ts
- packages/paths/src/env-integration.test.ts
Comment on lines
+5266
to
+5313
| it('bun script node does not leak repo .env from execution cwd (#1135)', async () => { | ||
| // Regression test: place a .env with a marker in the execution cwd. | ||
| // The bun script must NOT see it because --no-env-file is passed. | ||
| const mockDeps = createMockDeps(); | ||
| const platform = createMockPlatform(); | ||
| const workflowRun = makeWorkflowRun('env-leak-run-id', { | ||
| workflow_name: 'env-leak-test', | ||
| conversation_id: 'conv-env-leak', | ||
| user_message: 'env leak test', | ||
| }); | ||
|
|
||
| // Write a .env with a marker in the script execution cwd | ||
| await writeFile(join(testDir, '.env'), 'LEAKED_REPO_SECRET=should_not_appear\n'); | ||
|
|
||
| const scriptNode: ScriptNode = { | ||
| id: 'env-check', | ||
| script: 'console.log(process.env.LEAKED_REPO_SECRET ?? "CLEAN")', | ||
| runtime: 'bun', | ||
| }; | ||
|
|
||
| await executeDagWorkflow( | ||
| mockDeps, | ||
| platform, | ||
| 'conv-env-leak', | ||
| testDir, | ||
| { name: 'env-leak-test', nodes: [scriptNode] }, | ||
| workflowRun, | ||
| 'claude', | ||
| undefined, | ||
| join(testDir, 'artifacts'), | ||
| join(testDir, 'logs'), | ||
| 'main', | ||
| 'docs/', | ||
| minimalConfig | ||
| ); | ||
|
|
||
| // The node output should be "CLEAN" — the repo .env was not loaded | ||
| const completedEvents = (mockDeps.store.createWorkflowEvent as ReturnType<typeof mock>).mock | ||
| .calls; | ||
| const completedEvent = completedEvents.find( | ||
| (call: unknown[]) => | ||
| (call[0] as { event_type: string }).event_type === 'node_completed' && | ||
| (call[0] as { step_name: string }).step_name === 'env-check' | ||
| ); | ||
| expect(completedEvent).toBeDefined(); | ||
| const eventData = (completedEvent![0] as { data: { node_output: string } }).data; | ||
| expect(eventData.node_output).toBe('CLEAN'); | ||
| }); |
There was a problem hiding this comment.
Stabilize this regression test against host environment leakage.
The test can fail spuriously if LEAKED_REPO_SECRET is already set in the CI/runtime environment. Clear and restore it within the test to make the assertion deterministic.
✅ Suggested deterministic guard
it('bun script node does not leak repo .env from execution cwd (`#1135`)', async () => {
+ const previousLeakedRepoSecret = process.env.LEAKED_REPO_SECRET;
+ delete process.env.LEAKED_REPO_SECRET;
+ try {
// Regression test: place a .env with a marker in the execution cwd.
// The bun script must NOT see it because --no-env-file is passed.
const mockDeps = createMockDeps();
const platform = createMockPlatform();
const workflowRun = makeWorkflowRun('env-leak-run-id', {
@@
const eventData = (completedEvent![0] as { data: { node_output: string } }).data;
expect(eventData.node_output).toBe('CLEAN');
+ } finally {
+ if (previousLeakedRepoSecret === undefined) {
+ delete process.env.LEAKED_REPO_SECRET;
+ } else {
+ process.env.LEAKED_REPO_SECRET = previousLeakedRepoSecret;
+ }
+ }
});Based on learnings: "Applies to **/*.test.ts : Keep tests deterministic — no flaky timing or network dependence without guardrails. Ensure local validation commands (bun run validate) map directly to CI expectations."
📝 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
| it('bun script node does not leak repo .env from execution cwd (#1135)', async () => { | |
| // Regression test: place a .env with a marker in the execution cwd. | |
| // The bun script must NOT see it because --no-env-file is passed. | |
| const mockDeps = createMockDeps(); | |
| const platform = createMockPlatform(); | |
| const workflowRun = makeWorkflowRun('env-leak-run-id', { | |
| workflow_name: 'env-leak-test', | |
| conversation_id: 'conv-env-leak', | |
| user_message: 'env leak test', | |
| }); | |
| // Write a .env with a marker in the script execution cwd | |
| await writeFile(join(testDir, '.env'), 'LEAKED_REPO_SECRET=should_not_appear\n'); | |
| const scriptNode: ScriptNode = { | |
| id: 'env-check', | |
| script: 'console.log(process.env.LEAKED_REPO_SECRET ?? "CLEAN")', | |
| runtime: 'bun', | |
| }; | |
| await executeDagWorkflow( | |
| mockDeps, | |
| platform, | |
| 'conv-env-leak', | |
| testDir, | |
| { name: 'env-leak-test', nodes: [scriptNode] }, | |
| workflowRun, | |
| 'claude', | |
| undefined, | |
| join(testDir, 'artifacts'), | |
| join(testDir, 'logs'), | |
| 'main', | |
| 'docs/', | |
| minimalConfig | |
| ); | |
| // The node output should be "CLEAN" — the repo .env was not loaded | |
| const completedEvents = (mockDeps.store.createWorkflowEvent as ReturnType<typeof mock>).mock | |
| .calls; | |
| const completedEvent = completedEvents.find( | |
| (call: unknown[]) => | |
| (call[0] as { event_type: string }).event_type === 'node_completed' && | |
| (call[0] as { step_name: string }).step_name === 'env-check' | |
| ); | |
| expect(completedEvent).toBeDefined(); | |
| const eventData = (completedEvent![0] as { data: { node_output: string } }).data; | |
| expect(eventData.node_output).toBe('CLEAN'); | |
| }); | |
| it('bun script node does not leak repo .env from execution cwd (`#1135`)', async () => { | |
| const previousLeakedRepoSecret = process.env.LEAKED_REPO_SECRET; | |
| delete process.env.LEAKED_REPO_SECRET; | |
| try { | |
| // Regression test: place a .env with a marker in the execution cwd. | |
| // The bun script must NOT see it because --no-env-file is passed. | |
| const mockDeps = createMockDeps(); | |
| const platform = createMockPlatform(); | |
| const workflowRun = makeWorkflowRun('env-leak-run-id', { | |
| workflow_name: 'env-leak-test', | |
| conversation_id: 'conv-env-leak', | |
| user_message: 'env leak test', | |
| }); | |
| // Write a .env with a marker in the script execution cwd | |
| await writeFile(join(testDir, '.env'), 'LEAKED_REPO_SECRET=should_not_appear\n'); | |
| const scriptNode: ScriptNode = { | |
| id: 'env-check', | |
| script: 'console.log(process.env.LEAKED_REPO_SECRET ?? "CLEAN")', | |
| runtime: 'bun', | |
| }; | |
| await executeDagWorkflow( | |
| mockDeps, | |
| platform, | |
| 'conv-env-leak', | |
| testDir, | |
| { name: 'env-leak-test', nodes: [scriptNode] }, | |
| workflowRun, | |
| 'claude', | |
| undefined, | |
| join(testDir, 'artifacts'), | |
| join(testDir, 'logs'), | |
| 'main', | |
| 'docs/', | |
| minimalConfig | |
| ); | |
| // The node output should be "CLEAN" — the repo .env was not loaded | |
| const completedEvents = (mockDeps.store.createWorkflowEvent as ReturnType<typeof mock>).mock | |
| .calls; | |
| const completedEvent = completedEvents.find( | |
| (call: unknown[]) => | |
| (call[0] as { event_type: string }).event_type === 'node_completed' && | |
| (call[0] as { step_name: string }).step_name === 'env-check' | |
| ); | |
| expect(completedEvent).toBeDefined(); | |
| const eventData = (completedEvent![0] as { data: { node_output: string } }).data; | |
| expect(eventData.node_output).toBe('CLEAN'); | |
| } finally { | |
| if (previousLeakedRepoSecret === undefined) { | |
| delete process.env.LEAKED_REPO_SECRET; | |
| } else { | |
| process.env.LEAKED_REPO_SECRET = previousLeakedRepoSecret; | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/dag-executor.test.ts` around lines 5266 - 5313, The
test "bun script node does not leak repo .env from execution cwd (`#1135`)" can be
flaky if LEAKED_REPO_SECRET exists in the host env; modify the test to save
process.env.LEAKED_REPO_SECRET at the start, delete/unset
process.env.LEAKED_REPO_SECRET before creating the repo .env and before calling
executeDagWorkflow, and restore the original value in a finally block after the
test completes; ensure the cleanup covers all early returns so the environment
is always restored (reference the test name, the LEAKED_REPO_SECRET variable,
the writeFile call that creates join(testDir, '.env'), and the
executeDagWorkflow invocation to locate where to clear/restore the env var).
2d019b8 to
370633b
Compare
Wirasm
changed the title
Wirasm
force-pushed
the
feature/remove-env-consent-model
branch
from
370633b to
8d9b03e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/workflows/src/dag-executor.test.ts (1)
5266-5313:⚠️ Potential issue | 🟡 MinorMake this env-leak regression test deterministic against host env state.
At Line 5278 onward, this can fail spuriously if
process.env.LEAKED_REPO_SECRETis already set by CI/runner. Save, clear, and restore that env var around the test body.Proposed fix
it('bun script node does not leak repo .env from execution cwd (`#1135`)', async () => { + const previousLeakedRepoSecret = process.env.LEAKED_REPO_SECRET; + delete process.env.LEAKED_REPO_SECRET; + try { // Regression test: place a .env with a marker in the execution cwd. // The bun script must NOT see it because --no-env-file is passed. const mockDeps = createMockDeps(); const platform = createMockPlatform(); @@ ); expect(completedEvent).toBeDefined(); const eventData = (completedEvent![0] as { data: { node_output: string } }).data; expect(eventData.node_output).toBe('CLEAN'); + } finally { + if (previousLeakedRepoSecret === undefined) { + delete process.env.LEAKED_REPO_SECRET; + } else { + process.env.LEAKED_REPO_SECRET = previousLeakedRepoSecret; + } + } });Based on learnings: "Applies to **/*.test.ts : Keep tests deterministic — no flaky timing or network dependence without guardrails. Ensure local validation commands (
bun run validate) map directly to CI expectations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 5266 - 5313, The test 'bun script node does not leak repo .env from execution cwd (`#1135`)' can be flaky if process.env.LEAKED_REPO_SECRET is already set on the host; before writing the repo .env and invoking executeDagWorkflow, capture the original value of process.env.LEAKED_REPO_SECRET, delete/clear it (so the test starts with a clean env), run the test body, and finally restore the original value in a finally block so CI state is preserved; update the test surrounding the ScriptNode invocation (the it block with id 'env-check' and calls to writeFile and executeDagWorkflow) to perform save/clear/restore of process.env.LEAKED_REPO_SECRET to make the test deterministic.
🧹 Nitpick comments (2)
packages/web/src/routes/SettingsPage.tsx (1)
264-266: Prefer a named interface for the mutation input.The new inline
{ path: string }object shape drifts from the repo’s TS convention. Pull this into a small interface/type alias so the mutation input is reusable and consistent.♻️ Suggested refactor
+interface AddCodebaseInput { + path: string; +} + const addMutation = useMutation({ - mutationFn: ({ path }: { path: string }) => addCodebase({ path }), + mutationFn: ({ path }: AddCodebaseInput) => addCodebase({ path }), onSuccess: () => {As per coding guidelines:
**/*.{ts,tsx}: Useinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/SettingsPage.tsx` around lines 264 - 266, Replace the inline mutation input type in the useMutation call with a named interface to follow the repo convention: define an interface (e.g., AddCodebaseInput) describing { path: string } and use that as the generic/parameter type for the mutationFn and any callers; update the mutation declaration (addMutation / mutationFn) to reference AddCodebaseInput and ensure addCodebase({ path }) still accepts the same shape.packages/server/src/routes/api.ts (1)
1508-1510: Use explicit field checks here for clarity instead of truthiness +?? ''.The
?? ''fallback is unnecessary—the schema's.refine()already guarantees exactly one ofurlorpathis present and non-empty (both use.min(1)). The current code is safe, but explicit branching would make the XOR constraint more obvious to readers.♻️ Suggested refactor
- const result = body.url - ? await cloneRepository(body.url) - : await registerRepository(body.path ?? ''); + let result; + if (body.url !== undefined) { + result = await cloneRepository(body.url); + } else { + result = await registerRepository(body.path!); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 1508 - 1510, Replace the terse ternary that uses truthiness plus the `?? ''` fallback with an explicit branch to make the XOR constraint clear: check if body.url is present and call cloneRepository(body.url), otherwise call registerRepository(body.path) (no `?? ''`); reference the existing symbols cloneRepository, registerRepository, and the request body fields body.url/body.path and keep the branching logic consistent with the schema's refine() that guarantees exactly one is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5266-5313: The test 'bun script node does not leak repo .env from
execution cwd (`#1135`)' can be flaky if process.env.LEAKED_REPO_SECRET is already
set on the host; before writing the repo .env and invoking executeDagWorkflow,
capture the original value of process.env.LEAKED_REPO_SECRET, delete/clear it
(so the test starts with a clean env), run the test body, and finally restore
the original value in a finally block so CI state is preserved; update the test
surrounding the ScriptNode invocation (the it block with id 'env-check' and
calls to writeFile and executeDagWorkflow) to perform save/clear/restore of
process.env.LEAKED_REPO_SECRET to make the test deterministic.
---
Nitpick comments:
In `@packages/server/src/routes/api.ts`:
- Around line 1508-1510: Replace the terse ternary that uses truthiness plus the
`?? ''` fallback with an explicit branch to make the XOR constraint clear: check
if body.url is present and call cloneRepository(body.url), otherwise call
registerRepository(body.path) (no `?? ''`); reference the existing symbols
cloneRepository, registerRepository, and the request body fields
body.url/body.path and keep the branching logic consistent with the schema's
refine() that guarantees exactly one is provided.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 264-266: Replace the inline mutation input type in the useMutation
call with a named interface to follow the repo convention: define an interface
(e.g., AddCodebaseInput) describing { path: string } and use that as the
generic/parameter type for the mutationFn and any callers; update the mutation
declaration (addMutation / mutationFn) to reference AddCodebaseInput and ensure
addCodebase({ path }) still accepts the same shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07b95212-4df4-4a79-bc29-c63f5aa9c059
📒 Files selected for processing (31)
CLAUDE.mdpackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/package.jsonpackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/codebases.test.tspackages/core/src/db/codebases.tspackages/core/src/handlers/clone.test.tspackages/core/src/handlers/clone.tspackages/core/src/handlers/command-handler.test.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/types/index.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.tspackages/paths/src/env-integration.test.tspackages/providers/src/claude/provider.tspackages/server/src/index.tspackages/server/src/routes/api.codebases.test.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/codebase.schemas.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.tspackages/workflows/src/script-node-deps.test.ts
💤 Files with no reviewable changes (16)
- packages/cli/src/cli.ts
- packages/core/src/types/index.ts
- packages/core/src/index.ts
- packages/workflows/src/executor-shared.ts
- packages/core/src/handlers/command-handler.test.ts
- packages/core/src/orchestrator/orchestrator-isolation.test.ts
- packages/core/src/orchestrator/orchestrator.test.ts
- packages/core/src/config/config-loader.ts
- packages/core/src/orchestrator/orchestrator-agent.test.ts
- packages/server/src/index.ts
- packages/core/src/db/adapters/sqlite.ts
- packages/core/src/config/config-types.ts
- packages/server/src/routes/schemas/codebase.schemas.ts
- packages/core/src/handlers/clone.test.ts
- packages/core/src/utils/env-leak-scanner.test.ts
- packages/core/src/utils/env-leak-scanner.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/db/codebases.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/workflows/src/dag-executor.ts
- packages/workflows/src/script-node-deps.test.ts
- packages/core/package.json
- packages/core/src/db/codebases.ts
- packages/web/src/lib/api.ts
- packages/paths/src/env-integration.test.ts
Collaborator
Author
PR Review SummaryReviewed by: code-reviewer, docs-impact, silent-failure-hunter, pr-test-analyzer Critical Issues (0 found)None. Important Issues (3 found)
Suggestions (3 found)
Strengths
Documentation Issues
VerdictNEEDS FIXES — The 2 stale doc pages and the missing Recommended Actions
|
Wirasm
force-pushed
the
feature/remove-env-consent-model
branch
from
8d9b03e to
1065a01
Compare
…ction (#1135) Remove the entire env-leak scanning/consent infrastructure: scanner, allow_env_keys DB column usage, allow_target_repo_keys config, PATCH consent route, --allow-env-keys CLI flag, and UI consent toggle. The env-leak gate was the wrong primitive. Target repo .env protection is already structural: - stripCwdEnv() at boot removes Bun-auto-loaded CWD .env keys - Archon loads its own env sources afterward (~/.archon/.env) - process.env is clean before any subprocess spawns - Managed env injection (config.yaml env: + DB vars) is unchanged No scanning, no consent, no blocking. Any repo can be registered and used. Subprocesses receive the already-clean process.env.
Wirasm
force-pushed
the
feature/remove-env-consent-model
branch
from
1065a01 to
7f65d17
Compare
This was referenced Apr 13, 2026
This was referenced Apr 13, 2026
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.
Summary
.envfiles leaked into both Claude Code and Bun script subprocesses. Claude Code runs via Bun, which auto-loads.envfrom CWD. This caused (1) repo secrets leaking into AI subprocesses, and (2) repo API keys silently overriding Archon's auth (breaking global auth).--no-env-fileto both Claude Code subprocess spawning (executableArgs) and Bun script node execution. Removed the entire scanning/consent model which was the wrong fix for this problem.stripCwdEnv()boot protection, managed env injection (config.yaml env: + DB vars),{ ...process.env }subprocess env constructionE2E Evidence
Tested from within a Claude Code session against a repo with contaminated
.env:Claude prompt node — before fix:
Claude prompt node — after fix (
executableArgs: ['--no-env-file']):Bun script node — before fix:
Bun script node — after fix (
bun --no-env-file -e):Bash node — always clean (bash doesn't auto-load
.env).Late registration via curl — registered repo after server boot, ran workflow, zero leaks in both bash and bun nodes.
Architecture
Change Metadata
securitymultiChanges
Fix: Claude Code subprocess env leak (packages/providers/src/claude/provider.ts)
executableArgs: ['--no-env-file']to SDK Options.envFix: Bun script node env leak (packages/workflows/src/dag-executor.ts)
--no-env-fileflag tobun -e(inline) andbun run(named) script invocations.envin script node subprocessesRemove: Consent/scanner model (20+ files, -935 lines)
env-leak-scanner.tsentirelyallow_env_keysfrom DB types, API schemas, Codebase interfaceallow_target_repo_keysfrom config types and loader--allow-env-keysCLI flag, UI consent toggleTests added:
.envin CWD verifies output isCLEAN--no-env-filein argsValidation Evidence
```bash
bun run validate # all pass, exit 0
```
Security Impact
--no-env-fileon Claude Code subprocess: prevents repo.envfrom overriding Archon auth tokens (this was silently breaking global auth when repos hadANTHROPIC_API_KEY)--no-env-fileon Bun script nodes: prevents repo secrets from leaking into script executionCompatibility / Migration
--allow-env-keysCLI flag removed (no longer needed)allow_target_repo_keysconfig silently ignoredallow_env_keysDB column remains as dead column.envvars should use managed env injection insteadKnown remaining gaps
executableArgsequivalent yet (envInjection: false)Summary by CodeRabbit