# Optional Improvements for 001-fiber-middleware-integration

**Created**: 2025-11-11  
**Status**: Deferred - Address after core implementation  
**Source**: `/speckit.analyze` findings (Medium/Low priority)

---

## Medium Priority Improvements

### A2: Log Retention Policy Clarity
- **Issue**: FR-006 mentions "configured retention policy" but doesn't specify units/format
- **Current Impact**: Implementation will need to infer format
- **Recommendation**: Add to spec.md FR-006:
  ```
  Retention policy specified in days (integer), e.g., 30 for app logs, 90 for access logs.
  Implemented via Lumberjack MaxAge parameter.
  ```
- **Effort**: 10 minutes (documentation only)

---

### A3: Rate Limiting Default Values
- **Issue**: FR-018a says "configurable requests per time window" but no default values
- **Current Impact**: Developers must guess initial values
- **Recommendation**: Add to spec.md FR-018a:
  ```
  Default: 100 requests per minute per IP
  Supported time units: second (s), minute (m), hour (h)
  Example config: max=100, window=1m
  ```
- **Effort**: 15 minutes (spec update + config.yaml example)

---

### A5: Phase 0 Research Status
- **Issue**: plan.md Phase 0 lists "Best Redis client" as unknown but Technical Context already decided
- **Current Impact**: Confusion about whether research is needed
- **Recommendation**: Update plan.md Phase 0:
  - Remove "Best Redis client" from unknowns (already decided: go-redis/redis/v8)
  - Or clarify: "Validate go-redis/redis/v8 choice (performance benchmarks, connection pool tuning)"
- **Effort**: 5 minutes (documentation clarity)

---

### A6: Config Validation Rules Location
- **Issue**: T009 says "config loading with validation" but validation rules not documented
- **Current Impact**: Developer must design validation rules during implementation
- **Recommendation**: Create section in data-model.md or spec.md:
  ```markdown
  ## Configuration Validation Rules
  - server.port: 1024-65535 (int)
  - server.host: non-empty string
  - redis.addr: host:port format
  - logging.max_size: 1-1000 (MB)
  - logging.max_age: 1-365 (days)
  ```
- **Effort**: 30 minutes (requires design decisions)

---

### U2: Access Log Format Specification
- **Issue**: FR-011 mentions logging HTTP requests but doesn't specify JSON schema for access.log
- **Current Impact**: Access log structure determined during implementation
- **Recommendation**: Add to spec.md or data-model.md:
  ```json
  {
    "timestamp": "2025-11-10T15:30:45Z",
    "level": "info",
    "request_id": "550e8400-e29b-...",
    "method": "GET",
    "path": "/api/v1/users",
    "status": 200,
    "duration_ms": 45,
    "ip": "192.168.1.100",
    "user_agent": "Mozilla/5.0...",
    "user_id": "12345" // if authenticated
  }
  ```
- **Effort**: 20 minutes (design + documentation)

---

### U3: Panic Response Details
- **Issue**: FR-014 says "HTTP 500 with unified error response" but unclear about stack trace handling
- **Current Impact**: Security concern - should stack traces be in production responses?
- **Recommendation**: Clarify in spec.md FR-014:
  ```
  Development/Staging: Include sanitized error message in response.msg, full stack trace in logs only
  Production: Generic error message "Internal server error", full details in logs only
  Response format: {"code": 1000, "data": null, "msg": "Internal server error"}
  ```
- **Effort**: 15 minutes (security consideration + spec update)

---

### U4: Rate Limiter Documentation Format
- **Issue**: FR-020 mentions "documentation" but unclear where (quickstart.md? inline comments? separate docs/)
- **Current Impact**: Documentation may be incomplete or scattered
- **Recommendation**: Specify in spec.md FR-020:
  ```
  Documentation location: quickstart.md section "Enabling Rate Limiting"
  Must include: Configuration parameters, per-endpoint setup, testing examples
  Code example: Uncomment middleware, adjust max/window, test with curl loop
  ```
- **Effort**: 10 minutes (clarify requirements)

---

### U5: Non-Writable Log Directory Behavior
- **Issue**: Edge case says "fail to start with clear error" but no HTTP status code if started
- **Current Impact**: Unclear behavior if directory becomes non-writable at runtime
- **Recommendation**: Clarify in spec.md Edge Cases:
  ```
  Startup: Fail immediately with exit code 1 and error message before listening on port
  Runtime: If directory becomes non-writable, log error to stderr and return 503 on health check
  ```
- **Effort**: 15 minutes (design decision + spec update)

---

### U6: Performance Test Task Missing
- **Issue**: plan.md mentions "1000+ req/s capacity" but no performance test task in tasks.md
- **Current Impact**: Performance goal not validated
- **Recommendation**: Add task to tasks.md Phase 10:
  ```
  - [ ] T117a [P] Load test with 1000 req/s for 60s, verify P95 < 200ms (use hey or wrk)
  ```
- **Effort**: 1 hour (implementation + infrastructure setup)

---

### I1: Logger Instances Documentation
- **Issue**: spec.md mentions Zap but tasks reference appLogger/accessLogger instances
- **Current Impact**: Spec doesn't explicitly require two logger instances
- **Recommendation**: Add to spec.md FR-004 or Key Entities:
  ```
  System maintains two independent Zap logger instances:
  - appLogger: For application-level logs (business logic, errors, debug)
  - accessLogger: For HTTP access logs (request/response details)
  Each instance has separate Lumberjack rotation configuration.
  ```
- **Effort**: 10 minutes (documentation clarity)

---

### I3: Fail-Closed Timing Validation
- **Issue**: spec.md FR-016b says "immediately" but T063 doesn't validate timing (< 100ms)
- **Current Impact**: "Immediate" is subjective, no performance assertion
- **Recommendation**: Update tasks.md T063:
  ```
  - [ ] T063 [P] [US6] Unit test for Redis unavailable (fail closed with < 100ms response time)
  ```
  Or clarify spec.md: "immediately = same request cycle, no retry delays"
- **Effort**: 5 minutes (clarify requirements OR 30 minutes add timing assertion)

---

## Low Priority Improvements

### D1: Response Format Duplication
- **Issue**: FR-007 and US3 both define unified response format
- **Current Impact**: Redundancy, potential inconsistency if one updated
- **Recommendation**: Keep FR-007 as normative, update US3 acceptance criteria:
  ```
  Change: "the response contains `{...}`"
  To: "the response follows unified format defined in FR-007"
  ```
- **Effort**: 5 minutes (reduce duplication)

---

### D2: Code Quality Task Consolidation
- **Issue**: T096-T099 are four separate tasks that could run in one script
- **Current Impact**: Overhead running tasks sequentially
- **Recommendation**: Consider combining to T096-combined:
  ```
  - [ ] T096 [P] Run code quality checks: gofmt -l ., go vet ./..., golangci-lint run, check doc comments
  ```
  Or keep separate for granular progress tracking (current approach is also valid)
- **Effort**: 15 minutes (script creation) OR keep as-is (no change needed)

---

### I4: Task Count Verification
- **Issue**: plan.md says 126 tasks, tasks.md has T001-T126 (now T127 with T012a)
- **Current Impact**: None - counts match after update
- **Recommendation**: No action needed, already addressed in C1 fix
- **Effort**: 0 minutes (already resolved)

---

## Priority Recommendation

**Before Implementation**:
- None (all HIGH priority issues already fixed)

**During Implementation** (if time permits):
- A6: Config validation rules (needed for T009 implementation)
- U2: Access log format (needed for T050 implementation)
- U3: Panic response details (security consideration)

**After Implementation** (polish phase):
- A2, A3: Add default values to config.yaml and documentation
- U4, U5, I1: Documentation improvements
- U6: Performance testing (if infrastructure available)
- D1, D2, I3: Nice-to-have optimizations

---

## Summary

- **Total Optional Improvements**: 14 items
- **Medium Priority**: 9 items (mostly documentation/specification clarity)
- **Low Priority**: 5 items (minor optimizations, already acceptable as-is)
- **Estimated Total Effort**: ~4 hours for all medium priority items
- **Recommended Approach**: Address medium priority items during implementation when context is fresh