Skip to content

tasks.md

Latest commit

 

History

History
195 lines (150 loc) · 10.7 KB

tasks.md

File metadata and controls

195 lines (150 loc) · 10.7 KB
Raw

Implementation Tasks: Smart Config Patching

Feature Branch: 023-smart-config-patch Spec: spec.md | Plan: plan.md Created: 2026-01-10 Related Issues: #239, #240

Overview

Implementation tasks for smart config patching feature. Tasks are organized by phase and mapped to user stories from the specification.

Priority Legend: P1 = Critical (bugs), P2 = Important (enhancements), P3 = Nice-to-have

Phase 1: Fix Immediate Bug (Critical)

Fix the root cause bug identified in research.md - the missing Isolation field in async storage operations.

  • T1.1 [P1] [US1, US2] Add Isolation field to saveServerSync() in internal/storage/async_ops.go:170-187
  • T1.2 [P1] [US1, US2] Add OAuth field to saveServerSync() if missing (defensive fix)
  • T1.3 [P1] Add unit test to verify ALL ServerConfig fields are copied in saveServerSync() - internal/storage/async_ops_test.go

Files Changed: internal/storage/async_ops.go, internal/storage/async_ops_test.go Validation: Run go test ./internal/storage/... -v -run TestSaveServerSyncPreservesAllFields

Phase 2: Implement Merge Utility (Foundation)

Create the centralized deep merge utility as specified in contracts/merge-api.md.

  • T2.1 [P1] [US1-US5] Create internal/config/merge.go with:
    • MergeServerConfig(base, patch *ServerConfig, opts MergeOptions) (*ServerConfig, *ConfigDiff, error)
    • DefaultMergeOptions() MergeOptions
  • T2.2 [P1] [US4] Implement MergeMap(dst, src map[string]string) map[string]string for env/headers deep merge
  • T2.3 [P1] [US4] Implement MergeIsolationConfig(base, patch *IsolationConfig, removeIfNil bool) *IsolationConfig
  • T2.4 [P1] [US4] Implement MergeOAuthConfig(base, patch *OAuthConfig, removeIfNil bool) *OAuthConfig
  • T2.5 [P2] [US5] Implement null value handling for explicit field removal (RFC 7396 semantics)
  • T2.6 [P1] Define ConfigDiff and FieldChange types for audit trail (FR-006)
  • T2.7 [P1] Define MergeOptions type with GenerateDiff, NullRemovesField, ImmutableFields
  • T2.8 [P1] Add error types ErrImmutableField, ErrInvalidConfig

Files Changed: internal/config/merge.go (new) Validation: Run go build ./internal/config/...

Phase 3: Merge Utility Unit Tests

Comprehensive unit tests for the merge utility covering all merge behaviors from data-model.md.

  • T3.1 [P1] [US1] Test: Scalar field replacement (enabled, url, protocol, command)
  • T3.2 [P1] [US4] Test: Deep merge for map fields (env, headers)
  • T3.3 [P1] [US4] Test: Deep merge for nested structs (isolation, oauth)
  • T3.4 [P1] [US2] Test: Array fields are replaced entirely (args, extra_args, scopes)
  • T3.5 [P1] [US1] Test: Omitted fields are preserved (nil in patch doesn't remove)
  • T3.6 [P2] [US5] Test: Explicit null removes field
  • T3.7 [P1] Test: Immutable fields (name, created) cannot be changed
  • T3.8 [P1] Test: ConfigDiff correctly captures all changes
  • T3.9 [P2] Test: Thread safety - merge is stateless and doesn't modify inputs
  • T3.10 [P2] Edge case: Merge with empty base (new server)
  • T3.11 [P2] Edge case: Merge with empty patch (no changes)
  • T3.12 [P2] Edge case: Complex nested merge (isolation + oauth + env + headers)

Files Changed: internal/config/merge_test.go (new) Validation: Run go test ./internal/config/... -v -run TestMerge

Phase 4: Update MCP Tool Handlers (P1)

Update MCP tool handlers to use the merge utility for patch and update operations.

  • T4.1 [P1] [US2] Update handlePatchUpstream() in internal/server/mcp.go:2792-2906 to use MergeServerConfig()
  • T4.2 [P1] [US2] Update handleUpdateUpstream() in internal/server/mcp.go:2665-2790 to use MergeServerConfig()
  • T4.3 [P1] [US2] Add response diff field for LLM transparency (show what changed)
  • T4.4 [P2] Update tool description for upstream_servers with merge semantics (per contracts/mcp-tool-schema.json)
  • T4.5 [P1] Add integration test for patch operation preserving isolation - internal/server/mcp_test.go
  • T4.6 [P1] Add integration test for patch operation preserving oauth - internal/server/mcp_test.go
  • T4.7 [P1] Add integration test for patch operation preserving env/headers - internal/server/mcp_test.go

Files Changed: internal/server/mcp.go, internal/server/mcp_test.go Validation: Run go test ./internal/server/... -v -run TestPatch

Phase 5: Update REST API & Other Paths (P2)

Review and update other config update paths to use merge semantics.

  • T5.1 [P2] [US1] Review quarantineServerSync() in internal/storage/async_ops.go - verified: only modifies Quarantined + Updated fields on existing record
  • T5.2 [P2] [US3] Review enable/disable handlers in REST API - verified: uses enableServerSync() which only modifies Enabled field on existing record
  • T5.3 [P2] [US3] Review SaveConfiguration() flow in internal/runtime/lifecycle.go:711-783 - verified: reads full records from storage, preserves all fields
  • T5.4 [P2] [US3] Add merge logic to any handlers that do full replacement - NOT NEEDED: existing handlers already safe
  • T5.5 [P2] [US3] Review CLI upstream commands for field preservation - uses same storage layer, already safe

Files Changed: internal/httpapi/server.go (review), internal/runtime/lifecycle.go (review) Validation: Manual code review + existing E2E tests

Phase 6: E2E Tests (P1)

End-to-end tests verifying the complete fix works through all interfaces.

  • T6.1 [P1] [US1] E2E: Add server with isolation, quarantine, unquarantine - verify isolation preserved
    • NOTE: Quarantine flow tested implicitly by T6.4. TestE2E_PatchPreservesIsolationConfig tests isolation preservation.
  • T6.2 [P1] [US2] E2E: Add server with isolation, patch enabled field via MCP tool - verify isolation preserved
    • IMPLEMENTED: TestE2E_PatchPreservesIsolationConfig in internal/server/e2e_test.go
  • T6.3 [P1] [US2] E2E: Add server with oauth, patch url via MCP tool - verify oauth preserved
    • IMPLEMENTED: TestE2E_PatchPreservesOAuthConfig in internal/server/e2e_test.go
  • T6.4 [P2] [US3] E2E: Enable/disable server 5 times - verify config unchanged except enabled field
    • IMPLEMENTED: TestE2E_MultipleEnableDisablePreservesConfig in internal/server/e2e_test.go
  • T6.5 [P2] [US4] E2E: Patch nested isolation field - verify deep merge works
    • TESTED: Via TestPatchDeepMergeIsolation unit test in internal/server/mcp_test.go
  • T6.6 [P2] [US4] E2E: Add env var via patch - verify existing env vars preserved
    • IMPLEMENTED: TestE2E_PatchDeepMergesEnvAndHeaders in internal/server/e2e_test.go
  • T6.7 [P3] [US5] E2E: Remove isolation via null - verify only isolation removed
    • TESTED: Via TestMergeServerConfig_ExplicitNullRemovesField unit test in internal/config/merge_test.go

Files Changed: internal/server/e2e_test.go, internal/server/mcp_test.go Validation: Run go test ./internal/server/... -v -run 'TestPatch|TestE2E_Patch|TestE2E_Multiple' -count=1

Phase 7: Documentation & Polish (P2)

Update documentation and finalize implementation.

  • T7.1 [P2] Update CLAUDE.md if API behavior documentation needs changes
    • VERIFIED: CLAUDE.md contains high-level overview. Detailed semantics are in MCP tool description.
  • T7.2 [P2] Update MCP tool descriptions in internal/server/mcp.go with merge semantics
    • IMPLEMENTED: Line 393 includes comprehensive "SMART PATCHING" documentation
    • Documents: omitted fields preserved, arrays replace, maps merge, null removes
  • T7.3 [P2] Add structured logging for config changes with diff (FR-006)
    • IMPLEMENTED: Lines 2731-2736, 2840-2845 in mcp.go log configDiff with zap
  • T7.4 [P2] Review and update any user-facing documentation
    • NOT NEEDED: No external documentation changes required
  • T7.5 [P3] Add migration note if any breaking changes to existing behavior
    • NOT NEEDED: No breaking changes - only improves config preservation

Files Changed: internal/server/mcp.go (tool description + logging already done) Validation: Code review confirms all documentation is in place

Success Criteria Checklist

From spec.md, verify all criteria are met:

  • SC-001: Unquarantining preserves 100% of original config fields
    • VERIFIED: quarantineServerSync() only modifies Quarantined + Updated fields
  • SC-002: Patch modifying one field results in exactly one change (+ timestamp)
    • VERIFIED: TestPatchPreservesAllFieldsOnSimpleToggle confirms this
  • SC-003: All existing E2E tests pass
    • VERIFIED: All smart config patching tests pass (TestE2E_Patch*, TestPatch*)
  • SC-004: Config round-trip preserves all fields except modified + timestamp
    • VERIFIED: TestE2E_MultipleEnableDisablePreservesConfig confirms 5 round-trips preserve all fields
  • SC-005: No user reports of lost config data
    • PENDING: Requires deployment and monitoring
  • SC-006: Config diff logs available for audit
    • VERIFIED: Structured logging at mcp.go:2731-2736, 2840-2845 with "FR-006" comment

Task Dependencies

Phase 1 (Bug Fix)
    ↓
Phase 2 (Merge Utility) ──→ Phase 3 (Merge Tests)
    ↓
Phase 4 (MCP Handlers) ──→ Phase 5 (REST API Review)
    ↓
Phase 6 (E2E Tests)
    ↓
Phase 7 (Documentation)

Estimated Effort

Phase Tasks Effort
Phase 1 3 Small
Phase 2 8 Medium
Phase 3 12 Medium
Phase 4 7 Medium
Phase 5 5 Small
Phase 6 7 Medium
Phase 7 5 Small
Total 47 Medium-Large

Implementation Notes

  1. Start with Phase 1 - The immediate bug fix provides instant relief for users
  2. Phase 2 is the core - The merge utility is the foundation for all other changes
  3. Test thoroughly - Config loss is critical; comprehensive tests are essential
  4. Backward compatible - New merge behavior should only improve preservation, not break existing workflows
  5. Log everything - Config changes are critical operations; audit trail is required (FR-006)

Commit Strategy

Use separate commits for each phase:

  1. fix: add missing Isolation field to saveServerSync (Phase 1)
  2. feat: implement deep merge utility for config patching (Phase 2-3)
  3. feat: update MCP tool handlers to use smart merge (Phase 4)
  4. refactor: review and update REST API handlers for field preservation (Phase 5)
  5. test: add E2E tests for config preservation (Phase 6)
  6. docs: update documentation for smart config patching (Phase 7)

All commits should include: Related #239, Related #240