Charon/docs/plans/current_spec.md

# PR #434 Codecov Coverage Gap Remediation Plan

**Status**: Analysis Complete - REMEDIATION REQUIRED
**Created**: 2025-12-21
**Last Updated**: 2025-12-21
**Objective**: Increase patch coverage from 87.31% to meet 85% threshold across 7 files

---

## Executive Summary

**Coverage Status:** ⚠️ 78 MISSING LINES across 7 files

PR #434: `feat: add API-Friendly security header preset for mobile apps`
- **Branch:** `feature/beta-release`
- **Patch Coverage:** 87.31% (above 85% threshold ✅)
- **Total Missing Lines:** 78 lines across 7 files
- **Recommendation:** Add targeted tests to improve coverage and reduce technical debt

### Coverage Gap Summary

| File | Coverage | Missing | Partials | Priority | Effort |
|------|----------|---------|----------|----------|--------|
| `handlers/testdb.go` | 61.53% | 29 | 1 | **P1** | Medium |
| `handlers/proxy_host_handler.go` | 75.00% | 25 | 4 | **P1** | High |
| `handlers/security_headers_handler.go` | 93.75% | 8 | 4 | P2 | Low |
| `handlers/test_helpers.go` | 87.50% | 2 | 0 | P3 | Low |
| `routes/routes.go` | 66.66% | 1 | 1 | P3 | Low |
| `caddy/config.go` | 98.82% | 1 | 1 | P4 | Low |
| `handlers/certificate_handler.go` | 50.00% | 1 | 0 | P4 | Low |

---

## Detailed Analysis by File

---

### 1. `backend/internal/api/handlers/testdb.go` (29 Missing, 1 Partial)

**File Purpose:** Test database utilities providing template DB and migrations for faster test setup.

**Current Coverage:** 61.53%

**Test File:** `testdb_test.go` (exists - 200+ lines)

#### Uncovered Code Paths

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| 26-28 | `initTemplateDB()` | Error return path after `gorm.Open` fails | Mock DB open failure |
| 32-55 | `initTemplateDB()` | `AutoMigrate` error path | Inject migration failure |
| 98-104 | `OpenTestDBWithMigrations()` | `rows.Scan` error + empty sql handling | Test with corrupted template |
| 109-131 | `OpenTestDBWithMigrations()` | Fallback AutoMigrate path | Force template DB unavailable |

#### Test Scenarios to Add

```go
// File: backend/internal/api/handlers/testdb_coverage_test.go

func TestInitTemplateDB_OpenError(t *testing.T) {
    // Cannot directly test since initTemplateDB uses sync.Once
    // This path is covered by testing GetTemplateDB behavior
    // when underlying DB operations fail
}

func TestOpenTestDBWithMigrations_TemplateUnavailable(t *testing.T) {
    // Force the template DB to be unavailable
    // Verify fallback AutoMigrate is called
    // Test by checking table creation works
}

func TestOpenTestDBWithMigrations_ScanError(t *testing.T) {
    // Test when rows.Scan returns error
    // Should fall through to fallback path
}

func TestOpenTestDBWithMigrations_EmptySQL(t *testing.T) {
    // Test when sql string is empty
    // Should skip db.Exec call
}
```

#### Recommended Actions

1. **Add `testdb_coverage_test.go`** with scenarios above
2. **Complexity:** Medium - requires mocking GORM internals or using test doubles
3. **Alternative:** Accept lower coverage since this is test infrastructure code

**Note:** This file is test-only infrastructure (`testdb.go`). Coverage gaps here are acceptable since:
- The happy path is already tested
- Error paths are defensive programming
- Testing test utilities creates circular dependencies

**Recommendation:** P3 - Lower priority, accept current coverage for test utilities.

---

### 2. `backend/internal/api/handlers/proxy_host_handler.go` (25 Missing, 4 Partials)

**File Purpose:** CRUD operations for proxy hosts including bulk security header updates.

**Current Coverage:** 75.00%

**Test Files:**
- `proxy_host_handler_test.go`
- `proxy_host_handler_security_headers_test.go`

#### Uncovered Code Paths (New in PR #434)

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| 222-226 | `Update()` | `enable_standard_headers` null handling | Test with null payload |
| 227-232 | `Update()` | `forward_auth_enabled` bool handling | Test update with this field |
| 234-237 | `Update()` | `waf_disabled` bool handling | Test update with this field |
| 286-340 | `Update()` | `security_header_profile_id` type conversions | Test int, string, float64, default cases |
| 302-305 | `Update()` | Failed float64→uint conversion (negative) | Test with -1 value |
| 312-315 | `Update()` | Failed int→uint conversion (negative) | Test with -1 value |
| 322-325 | `Update()` | Failed string parse | Test with "invalid" string |
| 326-328 | `Update()` | Unsupported type default case | Test with bool or array |
| 331-334 | `Update()` | Conversion failed response | Implicit test from above |
| 546-549 | `BulkUpdateSecurityHeaders()` | Profile lookup DB error (non-404) | Mock DB error |

#### Test Scenarios to Add

```go
// File: backend/internal/api/handlers/proxy_host_handler_update_test.go

func TestProxyHostUpdate_EnableStandardHeaders_Null(t *testing.T) {
    // Create host, then update with enable_standard_headers: null
    // Verify host.EnableStandardHeaders becomes nil
}

func TestProxyHostUpdate_EnableStandardHeaders_True(t *testing.T) {
    // Create host, then update with enable_standard_headers: true
    // Verify host.EnableStandardHeaders is pointer to true
}

func TestProxyHostUpdate_EnableStandardHeaders_False(t *testing.T) {
    // Create host, then update with enable_standard_headers: false
    // Verify host.EnableStandardHeaders is pointer to false
}

func TestProxyHostUpdate_ForwardAuthEnabled(t *testing.T) {
    // Create host with forward_auth_enabled: false
    // Update to forward_auth_enabled: true
    // Verify change persisted
}

func TestProxyHostUpdate_WAFDisabled(t *testing.T) {
    // Create host with waf_disabled: false
    // Update to waf_disabled: true
    // Verify change persisted
}

func TestProxyHostUpdate_SecurityHeaderProfileID_Int(t *testing.T) {
    // Create profile, create host
    // Update with security_header_profile_id as int (Go doesn't JSON decode to int, but test anyway)
}

func TestProxyHostUpdate_SecurityHeaderProfileID_NegativeFloat(t *testing.T) {
    // Create host
    // Update with security_header_profile_id: -1.0 (float64)
    // Expect 400 Bad Request
}

func TestProxyHostUpdate_SecurityHeaderProfileID_NegativeInt(t *testing.T) {
    // Create host
    // Update with security_header_profile_id: -1 (if possible via int type)
    // Expect 400 Bad Request
}

func TestProxyHostUpdate_SecurityHeaderProfileID_InvalidString(t *testing.T) {
    // Create host
    // Update with security_header_profile_id: "not-a-number"
    // Expect 400 Bad Request
}

func TestProxyHostUpdate_SecurityHeaderProfileID_UnsupportedType(t *testing.T) {
    // Create host
    // Send security_header_profile_id as boolean (true) or array
    // Expect 400 Bad Request
}

func TestBulkUpdateSecurityHeaders_DBError_NonNotFound(t *testing.T) {
    // Close DB connection to simulate internal error
    // Call bulk update with valid profile ID
    // Expect 500 Internal Server Error
}
```

#### Recommended Actions

1. **Add `proxy_host_handler_update_test.go`** with 11 new test cases
2. **Estimated effort:** 2-3 hours
3. **Impact:** Covers 25 lines, brings coverage to ~95%

---

### 3. `backend/internal/api/handlers/security_headers_handler.go` (8 Missing, 4 Partials)

**File Purpose:** CRUD for security header profiles, presets, CSP validation.

**Current Coverage:** 93.75%

**Test File:** `security_headers_handler_test.go` (extensive - 500+ lines)

#### Uncovered Code Paths

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| 89-91 | `GetProfile()` | UUID lookup DB error (non-404) | Close DB before UUID lookup |
| 142-145 | `UpdateProfile()` | `db.Save()` error | Close DB before save |
| 177-180 | `DeleteProfile()` | `db.Delete()` error | Already tested in `TestDeleteProfile_DeleteDBError` |
| 269-271 | `validateCSPString()` | Unknown directive warning | Test with `unknown-directive` |

#### Test Scenarios to Add

```go
// File: backend/internal/api/handlers/security_headers_handler_coverage_test.go

func TestGetProfile_UUID_DBError_NonNotFound(t *testing.T) {
    // Create profile, get UUID
    // Close DB connection
    // GET /security/headers/profiles/{uuid}
    // Expect 500 Internal Server Error
}

func TestUpdateProfile_SaveError(t *testing.T) {
    // Create profile (ID = 1)
    // Close DB connection
    // PUT /security/headers/profiles/1
    // Expect 500 Internal Server Error
    // Note: Similar to TestUpdateProfile_DBError but for save specifically
}
```

**Note:** Most paths are already covered by existing tests. The 8 missing lines are edge cases around DB errors that are already partially tested.

#### Recommended Actions

1. **Verify existing tests cover scenarios** - some may already be present
2. **Add 2 additional DB error tests** if not covered
3. **Estimated effort:** 30 minutes

---

### 4. `backend/internal/api/handlers/test_helpers.go` (2 Missing)

**File Purpose:** Polling helpers for test synchronization (`waitForCondition`).

**Current Coverage:** 87.50%

**Test File:** `test_helpers_test.go` (exists)

#### Uncovered Code Paths

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| 17-18 | `waitForCondition()` | `t.Fatalf` call on timeout | Cannot directly test without custom interface |
| 31-32 | `waitForConditionWithInterval()` | `t.Fatalf` call on timeout | Same issue |

#### Analysis

The missing coverage is in the `t.Fatalf()` calls which are intentionally not tested because:
1. `t.Fatalf()` terminates the test immediately
2. Testing this would require a custom testing.T interface
3. The existing tests use mock implementations to verify timeout behavior

**Current tests already cover:**
- `TestWaitForCondition_PassesImmediately`
- `TestWaitForCondition_PassesAfterIterations`
- `TestWaitForCondition_Timeout` (uses mockTestingT)
- `TestWaitForConditionWithInterval_*` variants

#### Recommended Actions

1. **Accept current coverage** - The timeout paths are defensive and covered via mocks
2. **No additional tests needed** - mockTestingT already verifies the behavior
3. **Estimated effort:** None

---

### 5. `backend/internal/api/routes/routes.go` (1 Missing, 1 Partial)

**File Purpose:** API route registration and middleware wiring.

**Current Coverage:** 66.66% (but only 1 new line missing)

**Test File:** `routes_test.go` (exists)

#### Uncovered Code Paths

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| ~234 | `Register()` | `secHeadersSvc.EnsurePresetsExist()` error logging | Error is logged but not fatal |

#### Analysis

The missing line is error handling for `EnsurePresetsExist()`:
```go
if err := secHeadersSvc.EnsurePresetsExist(); err != nil {
    logger.Log().WithError(err).Warn("Failed to initialize security header presets")
}
```

This is non-fatal logging - the route registration continues even if preset initialization fails.

#### Test Scenarios to Add

```go
// File: backend/internal/api/routes/routes_security_headers_test.go

func TestRegister_EnsurePresetsExist_Error(t *testing.T) {
    // This requires mocking SecurityHeadersService
    // Or testing with a DB that fails on insert
    // Low priority since it's just a warning log
}
```

#### Recommended Actions

1. **Accept current coverage** - Error path only logs a warning
2. **Low impact** - Registration continues regardless of error
3. **Estimated effort:** 30 minutes if mocking is needed

---

### 6. `backend/internal/caddy/config.go` (1 Missing, 1 Partial)

**File Purpose:** Caddy JSON configuration generation.

**Current Coverage:** 98.82% (excellent)

**Test Files:** Multiple test files `config_security_headers_test.go`

#### Uncovered Code Path

Based on the API-Friendly preset feature, the missing line is likely in `buildSecurityHeadersHandler()` for an edge case.

#### Analysis

The existing test `TestBuildSecurityHeadersHandler_APIFriendlyPreset` covers the new API-Friendly preset. The 1 missing line is likely an edge case in:
- Empty string handling for headers
- Cross-origin policy variations

#### Recommended Actions

1. **Review coverage report details** to identify exact line
2. **Likely already covered** by `TestBuildSecurityHeadersHandler_APIFriendlyPreset`
3. **Estimated effort:** 15 minutes to verify

---

### 7. `backend/internal/api/handlers/certificate_handler.go` (1 Missing)

**File Purpose:** Certificate upload, list, and delete operations.

**Current Coverage:** 50.00% (only 1 new line)

**Test File:** `certificate_handler_coverage_test.go` (exists)

#### Uncovered Code Path

| Lines | Function | Issue | Solution |
|-------|----------|-------|----------|
| ~67 | `Delete()` | ID=0 validation check | Already tested |

#### Analysis

Looking at the test file, `TestCertificateHandler_Delete_InvalidID` tests the "invalid" ID case but may not specifically test ID=0.

```go
// Validate ID range
if id == 0 {
    c.JSON(http.StatusBadRequest, gin.H{"error": "invalid id"})
    return
}
```

#### Test Scenarios to Add

```go
func TestCertificateHandler_Delete_ZeroID(t *testing.T) {
    // DELETE /api/certificates/0
    // Expect 400 Bad Request with "invalid id" error
}
```

#### Recommended Actions

1. **Add single test for ID=0 case**
2. **Estimated effort:** 10 minutes

---

## Implementation Plan

### Priority Order (by impact)

1. **P1: proxy_host_handler.go** - 25 lines, new feature code
2. **P1: testdb.go** - 29 lines, but test-only infrastructure (lower actual priority)
3. **P2: security_headers_handler.go** - 8 lines, minor gaps
4. **P3: test_helpers.go** - Accept current coverage
5. **P3: routes.go** - Accept current coverage (warning log only)
6. **P4: config.go** - Verify existing coverage
7. **P4: certificate_handler.go** - Add 1 test

### Estimated Effort

| File | Tests to Add | Time Estimate |
|------|--------------|---------------|
| `proxy_host_handler.go` | 11 tests | 2-3 hours |
| `security_headers_handler.go` | 2 tests | 30 minutes |
| `certificate_handler.go` | 1 test | 10 minutes |
| `testdb.go` | Skip (test utilities) | 0 |
| `test_helpers.go` | Skip (already covered) | 0 |
| `routes.go` | Skip (warning log) | 0 |
| `config.go` | Verify only | 15 minutes |
| **Total** | **14 tests** | **~4 hours** |

---

## Test File Locations

### New Test Files to Create

1. `backend/internal/api/handlers/proxy_host_handler_update_test.go` - Update field coverage

### Existing Test Files to Extend

1. `backend/internal/api/handlers/security_headers_handler_test.go` - Add 2 DB error tests
2. `backend/internal/api/handlers/certificate_handler_coverage_test.go` - Add ID=0 test

---

## Dependencies Between Tests

```
None identified - all tests can be implemented independently
```

---

## Acceptance Criteria

1. ✅ Patch coverage ≥ 85% (currently 87.31%, already passing)
2. ⬜ All new test scenarios pass
3. ⬜ No regression in existing tests
4. ⬜ Test execution time < 30 seconds total
5. ⬜ All tests use `OpenTestDB` or `OpenTestDBWithMigrations` for isolation

---

## Mock Requirements

### For proxy_host_handler.go Tests

- Standard Gin test router setup (already exists in test files)
- GORM SQLite in-memory DB (use `OpenTestDBWithMigrations`)
- Mock Caddy manager (nil is acceptable for these tests)

### For security_headers_handler.go Tests

- Same as above
- Close DB connection to simulate errors

### For certificate_handler.go Tests

- Use existing test setup patterns
- No mocks needed for ID=0 test

---

## Conclusion

**Immediate Action Required:** None - coverage is above 85% threshold

**Recommended Improvements:**
1. Add 14 targeted tests to improve coverage quality
2. Focus on `proxy_host_handler.go` which has the most new feature code
3. Accept lower coverage on test infrastructure files (`testdb.go`, `test_helpers.go`)

**Total Estimated Effort:** ~4 hours for all improvements

---

**Analysis Date:** 2025-12-21
**Analyzed By:** GitHub Copilot
**Next Action:** Implement tests in priority order if coverage improvement is desired