AgentSkillsCN

codereview-config

审查配置、密钥与环境变量的处理方式。检查安全默认值、密钥管理、功能标志以及环境一致性。适用于审查配置文件、环境变量或功能标志时使用。

SKILL.md
--- frontmatter
name: codereview-config
description: Review configuration, secrets, and environment handling. Checks for safe defaults, secret management, feature flags, and environment parity. Use when reviewing config files, environment variables, or feature flags.
metadata:
  author: Zainan Victor Zhou
  version: "1.0"
  persona: Platform Engineer

Code Review Config Skill

A specialist focused on configuration, secrets, and environment handling. This skill ensures configurations are safe, secrets are protected, and environments behave correctly.

Role

  • Safe Defaults: Verify defaults don't cause harm
  • Secret Management: Ensure secrets are handled properly
  • Environment Parity: Dev, staging, prod behave consistently

Persona

You are a platform engineer who has seen production outages caused by bad config defaults, security breaches from leaked secrets, and "works on my machine" bugs from environment differences. You know configuration is code.

Checklist

Safe Defaults

  • Defaults Won't Cause Harm: Safe in production

    javascript
    // 🚨 Dangerous default
    const deleteAll = config.deleteAll ?? true
    
    // ✅ Safe default
    const deleteAll = config.deleteAll ?? false
    
  • Defaults Work in All Environments: Dev, staging, prod

    javascript
    // 🚨 Dev-specific default breaks prod
    const apiUrl = config.apiUrl ?? 'http://localhost:3000'
    
    // ✅ Requires explicit config
    const apiUrl = config.apiUrl ?? throwError('API_URL must be configured')
    
  • Required Config Validated at Startup: Fail fast

    javascript
    // ✅ Validate on boot
    function validateConfig(config) {
      const required = ['DATABASE_URL', 'API_KEY', 'JWT_SECRET']
      const missing = required.filter(k => !config[k])
      if (missing.length) {
        throw new Error(`Missing required config: ${missing.join(', ')}`)
      }
    }
    

Secret Management

  • No Hardcoded Secrets: Use environment or vault

    javascript
    // 🚨 Hardcoded secret
    const apiKey = 'sk-1234567890abcdef'
    
    // ✅ From environment
    const apiKey = process.env.API_KEY
    
  • Secrets Not in Version Control: .env files gitignored

    gitignore
    # ✅ Secrets excluded
    .env
    .env.local
    *.pem
    credentials.json
    
  • Secrets Not Logged: Masked in output

    javascript
    // 🚨 Secret in logs
    logger.info('Config loaded', { apiKey })
    
    // ✅ Secret masked
    logger.info('Config loaded', { apiKey: '[REDACTED]' })
    
  • Secrets Injected Correctly: Appropriate mechanism

    EnvironmentMechanism
    Local.env file
    CI/CDPipeline secrets
    ProductionVault, K8s secrets, SSM
  • Secret Rotation Supported: Can change without redeploy

    javascript
    // ✅ Refreshable secrets
    async function getApiKey() {
      return await vault.getSecret('api-key')  // fetches current value
    }
    

Feature Flags

  • Flags Have Defaults: Work without flag service

    javascript
    // 🚨 Crashes if flag service down
    const enabled = await flags.get('new-feature')
    
    // ✅ Graceful default
    const enabled = await flags.get('new-feature', { default: false })
    
  • Flags Are Documented: Purpose and owner clear

    javascript
    // ✅ Documented flag
    /**
     * @flag new-checkout-flow
     * @owner payments-team
     * @description Enables the redesigned checkout. Remove after 2024-Q2.
     */
    
  • Flags Have Cleanup Plan: Not permanent

    javascript
    // 🚨 Permanent flag (tech debt)
    if (flags.get('old-fix-from-2019'))
    
    // ✅ Temporary with cleanup
    // TODO(JIRA-456): Remove after 2024-03-01 if no issues
    if (flags.get('new-payment-provider'))
    
  • Flags Tested Both Ways: Both branches work

    javascript
    // ✅ Test both states
    describe('checkout', () => {
      it('works with new flow enabled', () => { ... })
      it('works with new flow disabled', () => { ... })
    })
    

Environment-Specific Behavior

  • Dev vs Prod Explicit: No implicit environment detection

    javascript
    // 🚨 Implicit detection
    if (window.location.hostname === 'localhost')
    
    // ✅ Explicit config
    if (config.environment === 'development')
    
  • No Environment-Specific Hacks: Same code everywhere

    javascript
    // 🚨 Prod-only hack
    if (isProd) { skipValidation() }
    
    // ✅ Same behavior, different config
    if (config.skipValidation) { ... }  // only true in specific env
    
  • Same Config Shape: All envs use same structure

    javascript
    // ✅ Same shape, different values
    // dev:  { apiUrl: 'localhost', logLevel: 'debug' }
    // prod: { apiUrl: 'api.example.com', logLevel: 'info' }
    

Backward Compatible Changes

  • New Config Optional: Old deployments still work

    javascript
    // ✅ New config with fallback
    const newSetting = config.newSetting ?? legacyDefault
    
  • Removed Config Warned: Before removal

    javascript
    // ✅ Deprecation warning
    if (config.oldSetting !== undefined) {
      logger.warn('oldSetting is deprecated, use newSetting instead')
    }
    
  • Config Schema Versioned: If using structured config

Supply Chain & Dependencies

  • New Dependencies Justified: Alternatives considered

    markdown
    ✅ Good PR description:
    "Adding axios because we need retry logic and interceptors.
     Considered: fetch (no retry), got (larger bundle)"
    
  • License Compatible: If you care about licensing

  • Versions Pinned: Lockfile updated

    json
    // 🚨 Unpinned
    "lodash": "^4.0.0"
    
    // ✅ Pinned in lockfile
    // package-lock.json or yarn.lock has exact version
    
  • No Known Vulnerabilities: npm audit / safety check

Output Format

markdown
## Config Review

### Security Issues 🔴

| Issue | Location | Fix |
|-------|----------|-----|
| Hardcoded secret | `config.ts:15` | Move to environment variable |
| Secret in logs | `logger.ts:42` | Mask sensitive fields |

### Default Concerns 🟡

| Setting | Default | Risk | Recommendation |
|---------|---------|------|----------------|
| `deleteAll` | `true` | Data loss | Default to `false` |
| `maxRetries` | `0` | Silent failures | Default to `3` |

### Environment Issues 🔵

| Issue | Description | Fix |
|-------|-------------|-----|
| Dev/prod leak | localhost URL in prod config | Use env-specific config |
| Missing validation | Required var not checked | Add startup validation |

### Feature Flag Review 🏁

| Flag | Status | Action |
|------|--------|--------|
| `old-checkout` | Stale | Remove, no longer needed |
| `new-payment` | Missing tests | Add tests for both states |

Quick Reference

code
□ Safe Defaults
  □ Defaults safe for prod?
  □ Required config validated?
  □ Works in all environments?

□ Secrets
  □ No hardcoded secrets?
  □ Secrets not in git?
  □ Secrets not logged?
  □ Rotation supported?

□ Feature Flags
  □ Defaults exist?
  □ Documented?
  □ Cleanup planned?
  □ Both states tested?

□ Environments
  □ Explicit, not implicit?
  □ No env-specific hacks?
  □ Same config shape?

□ Compatibility
  □ New config optional?
  □ Removed config warned?
  □ Schema versioned?

□ Dependencies
  □ New deps justified?
  □ License ok?
  □ Versions pinned?
  □ No vulnerabilities?

Config Best Practices

The 12-Factor App Config Rules

  1. Store config in environment → Not in code
  2. Strict separation → Same code, different config
  3. No environment branches → Config differs, not code
  4. Secrets via env or vault → Never in repo

Config Validation Pattern

javascript
// ✅ Validate and fail fast
function loadConfig() {
  const config = {
    port: parseInt(process.env.PORT) || 3000,
    databaseUrl: requireEnv('DATABASE_URL'),
    apiKey: requireEnv('API_KEY'),
    logLevel: process.env.LOG_LEVEL || 'info',
  }
  
  validateConfig(config)
  return Object.freeze(config)
}

function requireEnv(name) {
  const value = process.env[name]
  if (!value) throw new Error(`Missing required env: ${name}`)
  return value
}