reduce logs when environment=test

[amikofalvy]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Ready	Preview	Comment	Oct 7, 2025 6:30pm
agents-manage-api	Ready	Preview	Comment	Oct 7, 2025 6:30pm
agents-manage-ui	Ready	Preview	Comment	Oct 7, 2025 6:30pm
agents-run-api	Ready	Preview	Comment	Oct 7, 2025 6:30pm

[claude]

Claude finished @amikofalvy's task —— View job

---

Code Review Complete

Summary

This PR modifies the default log level in the PinoLogger to be 'silent' when ENVIRONMENT=test, effectively eliminating log spam during test execution.

Change Analysis

File: packages/agents-core/src/utils/logger.ts:35
Change: level: process.env.LOG_LEVEL || 'info' → level: process.env.LOG_LEVEL || (process.env.ENVIRONMENT === 'test' ? 'silent' : 'info')

Review Feedback

✅ Strengths

1. Addresses Real Problem: Test output shows evidence of console logging in tests that could create noise
2. Follows Existing Patterns: The codebase already uses ENVIRONMENT checks in 8+ places for test-specific behavior
3. Preserves Override Capability: LOG_LEVEL env var still takes precedence when explicitly set
4. Correct Log Level Choice: 'silent' is the appropriate Pino level to suppress all output

⚠️ Architectural Considerations

Inconsistent Test Configuration (Medium Priority)

Different packages handle test logging inconsistently:

• agents-manage-api/vitest.config.ts:24 sets LOG_LEVEL: 'error'
• packages/agents-core/vitest.config.ts has no explicit LOG_LEVEL override
• This change applies the 'silent' behavior globally for any code using PinoLogger when ENVIRONMENT=test

Recommendation: Consider standardizing test logging configuration across all vitest configs rather than implicit environment-based logic. This would make the behavior more explicit and easier to debug.

Missing Test Coverage (High Priority)

This logging behavior change lacks test coverage:

// Suggested test in packages/agents-core/src/__tests__/utils/logger.test.ts
describe('PinoLogger environment-based log levels', () => {
  it('should set silent level when ENVIRONMENT=test and no LOG_LEVEL override', () => {
    const originalEnv = process.env.ENVIRONMENT;
    delete process.env.LOG_LEVEL;
    process.env.ENVIRONMENT = 'test';
    
    const logger = new PinoLogger('test');
    expect(logger.getPinoInstance().level).toBe('silent');
    
    process.env.ENVIRONMENT = originalEnv;
  });
  
  it('should respect LOG_LEVEL override even in test environment', () => {
    const originalValues = { 
      env: process.env.ENVIRONMENT, 
      level: process.env.LOG_LEVEL 
    };
    
    process.env.ENVIRONMENT = 'test';
    process.env.LOG_LEVEL = 'debug';
    
    const logger = new PinoLogger('test');
    expect(logger.getPinoInstance().level).toBe('debug');
    
    Object.assign(process.env, originalValues);
  });
});

🔍 Technical Implementation

Logic Correctness

The ternary logic is correct but could be more readable:

// Current (works but dense)
level: process.env.LOG_LEVEL || (process.env.ENVIRONMENT === 'test' ? 'silent' : 'info')

// More explicit alternative
level: process.env.LOG_LEVEL || getDefaultLogLevel()

function getDefaultLogLevel(): string {
  return process.env.ENVIRONMENT === 'test' ? 'silent' : 'info';
}

Environment Value Validation

The code doesn't validate the ENVIRONMENT value. While this works, it's less robust than using the validated env from packages/agents-core/src/env.ts which defines the schema:

ENVIRONMENT: z.enum(['development', 'production', 'pentest', 'test']).optional()

📋 Minor Issues

1. Documentation: The change lacks a comment explaining why test environments get silent logging
2. Type Safety: Direct process.env access vs using typed env validation

🎯 Recommendations

For this PR:

1. Add tests for the new environment-based logic (High Priority)
2. Add inline comment explaining the test environment behavior

Follow-up considerations:

1. Standardize vitest configs to use consistent logging approaches
2. Consider using typed env imports for better type safety
3. Document logging strategy in development guidelines

✅ Approval Status

This change is functionally correct and addresses a legitimate issue. The implementation is simple and follows existing patterns. However, the missing test coverage should be addressed before merging.

---