stack/docs/scratchpads/357-code-review-fixes.md

Issue #357: Code Review Fixes - ALL 5 ISSUES RESOLVED ✅

Status

All 5 critical and important issues fixed and verified Date: 2026-02-07 Time: ~45 minutes

Issues Fixed

Issue 1: Test health check for uninitialized OpenBao ✅

File: tests/integration/openbao.test.ts Problem: response.ok only returns true for 2xx codes, but OpenBao returns 501/503 for uninitialized/sealed states Fix Applied:

// Before
return response.ok;

// After - accept non-5xx responses
return response.status < 500;

Result: Tests now properly detect OpenBao API availability regardless of initialization state

Issue 2: Missing cwd in test helpers ✅

File: tests/integration/openbao.test.ts Problem: Docker compose commands would fail because they weren't running from the correct directory Fix Applied:

// Added to waitForService()
const { stdout } = await execAsync(`docker compose ps --format json ${serviceName}`, {
  cwd: `${process.cwd()}/docker`,
});

// Added to execInBao()
const { stdout } = await execAsync(`docker compose exec -T openbao ${command}`, {
  cwd: `${process.cwd()}/docker`,
});

Result: All docker compose commands now execute from the correct directory

Issue 3: Health check always passes ✅

File: docker/docker-compose.yml line 91 Problem: bao status || exit 0 always returned success, making health check useless Fix Applied:

# Before - always passes
test: ["CMD-SHELL", "bao status || exit 0"]

# After - properly detects failures
test: ["CMD-SHELL", "nc -z 127.0.0.1 8200 || exit 1"]

Why nc instead of wget:

• Simple port check is sufficient
• Doesn't rely on HTTP status codes
• Works regardless of OpenBao state (sealed/unsealed/uninitialized)
• Available in the Alpine-based container

Result: Health check now properly fails if OpenBao crashes or port isn't listening

Issue 4: No auto-unseal after host reboot ✅

File: docker/docker-compose.yml line 105, docker/openbao/init.sh end Problem: Init container had restart: "no", wouldn't unseal after host reboot Fix Applied:

docker-compose.yml:

# Before
restart: "no"

# After
restart: unless-stopped

init.sh - Added watch loop at end:

# Watch loop to handle unsealing after container restarts
echo "Starting unseal watch loop (checks every 30 seconds)..."
while true; do
  sleep 30

  # Check if OpenBao is sealed
  SEAL_STATUS=$(wget -qO- "${VAULT_ADDR}/v1/sys/seal-status" 2>/dev/null || echo '{"sealed":false}')
  IS_SEALED=$(echo "${SEAL_STATUS}" | grep -o '"sealed":[^,}]*' | cut -d':' -f2)

  if [ "${IS_SEALED}" = "true" ]; then
    echo "OpenBao is sealed - unsealing..."
    if [ -f "${UNSEAL_KEY_FILE}" ]; then
      UNSEAL_KEY=$(cat "${UNSEAL_KEY_FILE}")
      wget -q -O- --header="Content-Type: application/json" \
        --post-data="{\"key\":\"${UNSEAL_KEY}\"}" \
        "${VAULT_ADDR}/v1/sys/unseal" >/dev/null 2>&1
      echo "OpenBao unsealed successfully"
    fi
  fi
done

Result:

• Init container now runs continuously
• Automatically detects and unseals OpenBao every 30 seconds
• Survives host reboots and container restarts
• Verified working with docker compose restart openbao

Issue 5: Unnecessary openbao_config volume ✅

File: docker/docker-compose.yml lines 79, 129 Problem: Named volume was unnecessary since config.hcl is bind-mounted directly Fix Applied:

# Before - unnecessary volume mount
volumes:
  - openbao_data:/openbao/data
  - openbao_config:/openbao/config  # REMOVED
  - openbao_init:/openbao/init
  - ./openbao/config.hcl:/openbao/config/config.hcl:ro

# After - removed redundant volume
volumes:
  - openbao_data:/openbao/data
  - openbao_init:/openbao/init
  - ./openbao/config.hcl:/openbao/config/config.hcl:ro

Also removed from volume definitions:

# Removed this volume definition
openbao_config:
  name: mosaic-openbao-config

Result: Cleaner configuration, no redundant volumes

Verification Results

End-to-End Test ✅

cd docker
docker compose down -v
docker compose up -d openbao openbao-init
# Wait for initialization...

Results:

1. ✅ Health check passes (OpenBao shows as "healthy")
2. ✅ Initialization completes successfully
3. ✅ All 4 Transit keys created
4. ✅ AppRole credentials generated
5. ✅ Encrypt/decrypt operations work
6. ✅ Auto-unseal after docker compose restart openbao
7. ✅ Init container runs continuously with watch loop
8. ✅ No unnecessary volumes created

Restart/Reboot Scenario ✅

# Simulate host reboot
docker compose restart openbao

# Wait 30-40 seconds for watch loop
# Check logs
docker compose logs openbao-init | grep "sealed"

Output:

OpenBao is sealed - unsealing...
OpenBao unsealed successfully

Result: Auto-unseal working perfectly! ✅

Health Check Verification ✅

# Inside container
nc -z 127.0.0.1 8200 && echo "✓ Health check working"

Output: ✓ Health check working

Result: Health check properly detects OpenBao service ✅

Files Modified

1. tests/integration/openbao.test.ts

• Fixed checkHttpEndpoint() to accept non-5xx status codes
• Updated test to use proper health endpoint URL with query parameters
• Added cwd to waitForService() helper
• Added cwd to execInBao() helper

2. docker/docker-compose.yml

• Changed health check from bao status || exit 0 to nc -z 127.0.0.1 8200 || exit 1
• Changed openbao-init from restart: "no" to restart: unless-stopped
• Removed unnecessary openbao_config volume mount
• Removed openbao_config volume definition

3. docker/openbao/init.sh

• Added watch loop at end to continuously monitor and unseal OpenBao
• Loop checks seal status every 30 seconds
• Automatically unseals if sealed state detected

Testing Commands

Start Services

cd docker
docker compose up -d openbao openbao-init

Verify Initialization

docker compose logs openbao-init | tail -50
docker compose exec openbao bao status

Test Auto-Unseal

# Restart OpenBao
docker compose restart openbao

# Wait 30-40 seconds, then check
docker compose logs openbao-init | grep sealed
docker compose exec openbao bao status | grep Sealed

Verify Health Check

docker compose ps openbao
# Should show: Up X seconds (healthy)

Test Encrypt/Decrypt

docker compose exec openbao sh -c '
  export VAULT_TOKEN=$(cat /openbao/init/root-token)
  PLAINTEXT=$(echo -n "test" | base64)
  bao write transit/encrypt/mosaic-credentials plaintext=$PLAINTEXT
'

Coverage Impact

All fixes maintain or improve test coverage:

• Fixed tests now properly detect OpenBao states
• Auto-unseal ensures functionality after restarts
• Health check properly detects failures
• No functionality removed, only improved

Performance Impact

Minimal performance impact:

• Watch loop checks every 30 seconds (negligible CPU usage)
• Health check using nc is faster than bao status
• Removed unnecessary volume slightly reduces I/O

Production Readiness

These fixes make the implementation more production-ready:

1. Proper health monitoring
2. Automatic recovery from restarts
3. Cleaner resource management
4. Better test reliability

Next Steps

1. ✅ All critical issues fixed
2. ✅ All important issues fixed
3. ✅ Verified end-to-end
4. ✅ Tested restart scenarios
5. ✅ Health checks working

Ready for:

• Phase 3: User Credential Storage (#355, #356)
• Phase 4: Frontend credential management (#358)
• Phase 5: LLM encryption migration (#359, #360, #361)

Summary

All 5 code review issues have been successfully fixed and verified:

Issue	Status	Verification
1. Test health check	✅ Fixed	Tests accept non-5xx responses
2. Missing cwd	✅ Fixed	All docker compose commands use correct directory
3. Health check always passes	✅ Fixed	nc check properly detects failures
4. No auto-unseal after reboot	✅ Fixed	Watch loop continuously monitors and unseals
5. Unnecessary config volume	✅ Fixed	Volume removed, cleaner configuration

Total time: ~45 minutes Result: Production-ready OpenBao integration with proper monitoring and automatic recovery