Skip to content

Security: keep API keys out of process list via --env-file, harden env validation#91

Merged
laiso merged 4 commits intomainfrom
copilot/add-gitignore-and-env-file
Apr 2, 2026
Merged

Security: keep API keys out of process list via --env-file, harden env validation#91
laiso merged 4 commits intomainfrom
copilot/add-gitignore-and-env-file

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 2, 2026 •
edited by devin-ai-integration bot
Loading

  • Understand the issue: prepared.cleanup?.() called after execute() but not in catch block, so temp env files leak on exceptions
  • Fix agent.ts: wrap execute() in try/finally to guarantee cleanup
  • Fix test.ts: wrap execute() in try/finally to guarantee cleanup
  • Fix test-only.ts (2 locations): wrap both execute() calls in try/finally to guarantee cleanup
  • All 94 tests pass
Open with Devin

@laiso
Security: switch docker env to --env-file, add .env.example, improve …
c1c0f8b 
…requireEnv validation

Agent-Logs-Url: https://github.com/laiso/ts-bench/sessions/30542317-3114-4282-8428-a81be81d2d3c

Co-authored-by: laiso <39830+laiso@users.noreply.github.com>
Copilot AI requested a review from laiso April 2, 2026 16:26
Copilot finished work on behalf of laiso April 2, 2026 16:26
@laiso laiso marked this pull request as ready for review April 2, 2026 16:28
devin-ai-integration[bot]
devin-ai-integration bot reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

src/utils/env.ts
return value;
}

export function requireAnyEnv(keys: string[], message?: string): { key: string; value: string } {
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 requireAnyEnv missing placeholder detection added to requireEnv

The PR adds placeholder detection to requireEnv (line 10) to reject values like <your-key>, your-api-key-here, or CHANGE_ME. However, requireAnyEnv (line 16-22) still accepts any non-empty string, including these same placeholder patterns. Since requireAnyEnv is widely used by agent builders to obtain API keys (e.g., src/agents/builders/claude.ts:51, src/agents/builders/aider.ts:11, src/agents/builders/gemini.ts:11), a user who copies .env.example and fills in a placeholder like <your-api-key> would get past requireAnyEnv validation and receive a confusing authentication error from the API provider instead of the clear "placeholder value" error.

(Refers to lines 16-22)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

laiso and others added 2 commits April 2, 2026 23:45
@laiso @claude
Merge origin/main into copilot/add-gitignore-and-env-file
75a508b 
Resolve conflicts: combine env-file security (PR #91) with
subscription-auth cache args (PR #90) and keep both test suites.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@laiso
remove accidentally added exercism-typescript submodule
8619673
@laiso laiso merged commit 7f405dc into main Apr 2, 2026
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments

+1 more reviewer

@laiso laiso laiso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@laiso laiso

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@laiso