[Feature] lpop env - set env without .env

[tombeckenham]

Related Issue

Fixes #38

Summary of Changes

Added an env command that sets environment variables before calling a process. This way the user can replace the .env file altogether

Risk Assessment

• Low
• Medium
• High

Additional Notes

We might want to look at supporting prefixes e.g. VITE_ when injecting into front end code

[claude]

Claude finished @tombeckenham's task —— View job

---

Pull Request Review

I've completed a comprehensive review of the new env command implementation. Here's my detailed feedback:

Todo List:

• Read and analyze the changed files
• Review CLI implementation in src/cli.ts
• Review test coverage in tests/cli.test.ts
• Check for security concerns
• Evaluate code quality and best practices
• Provide comprehensive feedback

✅ Strengths

Code Quality & Architecture

• Excellent integration: The env command follows the existing architectural patterns perfectly (lines 122-129 in src/cli.ts)
• Consistent error handling: Uses the same error handling patterns as other commands with proper chalk coloring
• Clean separation of concerns: The handleEnv method is well-structured and focused

Test Coverage

• Comprehensive test suite: 187 new lines of tests covering all scenarios including:
  • No command provided (display variables only)
  • Running commands with environment variables
  • Error handling for spawn failures
  • Edge cases like no variables found
• Proper mocking: Child process spawn is properly mocked with realistic behavior
• Environment preservation: Tests verify that existing process.env is preserved correctly

Security

• Safe environment handling: Variables are properly merged with existing environment without overriding system-critical variables
• No shell injection: Uses spawn() with separate command and args arrays (line 428)
• Keychain isolation: Leverages existing secure keychain patterns

🔧 Areas for Improvement

Performance Considerations

1. Unnecessary keychain calls: When no command is provided, the method still retrieves variables just to display them. Consider adding a --dry-run or --show flag for clarity (lines 407-412 in src/cli.ts)

Error Handling & UX

2. Process exit handling: The child.on('close') handler (line 434) doesn't differentiate between successful exits and errors - it passes through any exit code
3. Command validation: No validation that the command exists before attempting to spawn it

Code Style

4. Type safety: The spawn mock in tests could be more type-safe (lines 131-136 in tests/cli.test.ts)

💡 Suggestions

Minor Enhancements

• Consider adding a --verbose flag to show which variables are being loaded
• Add command validation to provide better error messages for non-existent commands
• Consider supporting command prefixes (mentioned in PR notes) as a future enhancement

Documentation

• The help text is clear and follows existing patterns
• Consider adding examples in the description for complex use cases

🔍 Specific Code Comments

src/cli.ts:428-431

const child = spawn(command[0], command.slice(1), {
  env,
  stdio: 'inherit',
});

✅ Good: Proper use of spawn() with separate args array prevents shell injection

src/cli.ts:416-419

const env = { ...process.env };
for (const { key, value } of variables) {
  env[key] = value;
}

✅ Good: Safe environment merging that preserves existing variables

tests/cli.test.ts:685-696

✅ Excellent: Test verifies that keychain variables override existing environment variables while preserving others

📊 Risk Assessment Agreement

I agree with the Medium risk assessment. The implementation:

• ✅ Follows secure patterns
• ✅ Has comprehensive tests
• ⚠️ Adds new process spawning functionality (medium complexity)
• ✅ Uses existing, proven keychain mechanisms

🎯 Overall Assessment

This is a well-implemented feature that successfully replaces the need for .env files. The code quality is high, follows project conventions, and includes excellent test coverage. The implementation is secure and performant.

Recommendation: ✅ Approve - Ready to merge with the minor suggestions above as potential future enhancements.

[claude]

Claude finished @tombeckenham's task —— View job

---

Pull Request Review

I've completed a comprehensive review of the new env command implementation. Here's my detailed feedback:

Todo List:

• Read and analyze the changed files
• Review CLI implementation in src/cli.ts
• Review test coverage in tests/cli.test.ts
• Check documentation updates
• Evaluate security concerns
• Assess performance considerations
• Provide comprehensive feedback

✅ Strengths

Code Quality & Architecture

• Excellent integration: The env command follows the existing architectural patterns perfectly (src/cli.ts:122-129)
• Consistent error handling: Uses the same error handling patterns as other commands with proper chalk coloring
• Clean separation of concerns: The handleEnv method (src/cli.ts:389-450) is well-structured and focused

Test Coverage

• Comprehensive test suite: 187 new lines of tests covering all scenarios including:
  • No command provided (display variables only) - tests/cli.test.ts:612-628
  • Running commands with environment variables - tests/cli.test.ts:642-662
  • Error handling for spawn failures - tests/cli.test.ts:699-729
  • Edge cases like no variables found - tests/cli.test.ts:630-640
• Proper mocking: Child process spawn is properly mocked with realistic behavior (tests/cli.test.ts:131-136)
• Environment preservation: Tests verify that existing process.env is preserved correctly (tests/cli.test.ts:682-697)

Security

• Safe environment handling: Variables are properly merged with existing environment without overriding system-critical variables (src/cli.ts:416-419)
• No shell injection: Uses spawn() with separate command and args arrays (src/cli.ts:428-431)
• Keychain isolation: Leverages existing secure keychain patterns

Documentation

• Clear README updates: Excellent documentation with visual examples and use cases (README.md:68-78, 142-155, 238-254)
• Security-first workflow: Well-documented approach for eliminating .env files entirely (README.md:238-254)

🔧 Areas for Improvement

Performance Considerations

1. Unnecessary keychain calls: When no command is provided, the method still retrieves variables just to display them. Consider adding a --dry-run or --show flag for clarity (src/cli.ts:407-412)

Error Handling & UX

2. Process exit handling: The child.on('close') handler (src/cli.ts:434-436) doesn't differentiate between successful exits and errors - it passes through any exit code
3. Command validation: No validation that the command exists before attempting to spawn it

Code Style

4. Type safety: The spawn mock in tests could be more type-safe (tests/cli.test.ts:131-136)

💡 Suggestions

Minor Enhancements

• Consider adding a --verbose flag to show which variables are being loaded
• Add command validation to provide better error messages for non-existent commands
• Consider supporting command prefixes (mentioned in PR notes) as a future enhancement

Documentation

• The help text is clear and follows existing patterns
• Consider adding examples in the description for complex use cases

🔍 Specific Code Comments

src/cli.ts:428-431

const child = spawn(command[0], command.slice(1), {
  env,
  stdio: 'inherit',
});

✅ Good: Proper use of spawn() with separate args array prevents shell injection

src/cli.ts:416-419

const env = { ...process.env };
for (const { key, value } of variables) {
  env[key] = value;
}

✅ Good: Safe environment merging that preserves existing variables

tests/cli.test.ts:685-696

✅ Excellent: Test verifies that keychain variables override existing environment variables while preserving others

📊 Risk Assessment Agreement

I agree with the Medium risk assessment. The implementation:

• ✅ Follows secure patterns
• ✅ Has comprehensive tests
• ⚠️ Adds new process spawning functionality (medium complexity)
• ✅ Uses existing, proven keychain mechanisms

🎯 Overall Assessment

This is a well-implemented feature that successfully replaces the need for .env files. The code quality is high, follows project conventions, and includes excellent test coverage. The implementation is secure and performant.

Recommendation: ✅ Approve - Ready to merge with the minor suggestions above as potential future enhancements.

---

[nickbeckenham]

please validate the tests are testing the expected behaviour based on documentation in particular the -- argument.

[nickbeckenham]

This is a bit inflammatory. Can probably just not have this step. instead mention the secrets are injected without a .env

[nickbeckenham]

Most of these tests are different to the documentation in particular there is no -- arg.

[claude]

Claude finished @tombeckenham's task —— View job

---

Pull Request Review

I've completed a comprehensive review of the new env command implementation, addressing the specific concerns raised by @nickbeckenham.

Todo List:

• Read and analyze the changed files
• Review the specific concern about -- argument testing vs documentation
• Check CLI implementation in src/cli.ts
• Review test coverage in tests/cli.test.ts
• Evaluate code quality and security
• Address review comments about documentation mismatch
• Provide comprehensive feedback

---

🚨 Critical Issue Found - Test/Documentation Mismatch

The reviewer @nickbeckenham was absolutely correct! There's a significant mismatch between the documentation and test behavior:

Documentation Claims:

• All examples show lpop env -- <command> format (README.md:72-74, README.md:169, README.md:216-217)
• Help text specifically states: "use -- to separate lpop options from command" (src/cli.ts:124)
• The -- separator is presented as the primary/expected way to use the command

Test Reality:

• Most tests use lpop env <command> WITHOUT the -- separator (tests/cli.test.ts:648, tests/cli.test.ts:667, tests/cli.test.ts:687)
• Only 6 out of 12 env command tests actually use the -- separator
• The implementation does support both formats, but tests suggest the non--- format is equally valid

Implementation Analysis:

The code in src/cli.ts:402-426 correctly handles both:

1. lpop env -- npm start (with -- separator)
2. lpop env npm start (without -- separator)

But this creates confusion about the intended interface.

✅ Code Quality & Security Analysis

Strengths:

• Excellent security: Proper environment merging without overriding system variables (src/cli.ts:447-450)
• No shell injection: Uses spawn() with separate args array (src/cli.ts:459)
• Comprehensive error handling: Covers spawn errors and keychain failures
• Clean architecture: Follows existing patterns perfectly

Areas for Improvement:

• Process exit handling: child.on('close') passes through any exit code without validation (src/cli.ts:465-467)
• Command validation: No validation that command exists before spawning

🔧 Recommended Actions

1. Fix Test/Documentation Alignment (Required)

Either:

• Option A: Update tests to primarily use -- separator (recommended)
• Option B: Update documentation to show both formats are valid

2. Address README Comment (Required)

Remove the inflammatory language in README.md:245 as requested by reviewer.

3. Minor Enhancements (Optional)

• Add command existence validation
• Improve exit code handling differentiation

📊 Risk Assessment

Agrees with Medium risk - the implementation is secure and well-tested, but the documentation mismatch could confuse users about the correct usage pattern.

🎯 Overall Assessment

Conditional Approval - The core implementation is excellent, but the test/documentation mismatch needs to be resolved before merge to ensure consistent user experience.

---