feat: Implement ReconcileManifest RPC for drift detection#1696
feat: Implement ReconcileManifest RPC for drift detection#1696
Conversation
Add ReconcileManifest RPC to ManifestHistoryService that compares a stored manifest version against live service state and reports any drift as structured output. This is a read-only safety-net operation with no auto-repair in Phase 1. Key changes: - Add ReconcileManifest RPC, DriftItem, DriftType enum, and ReconcileSummary to manifest_history_service.proto - Add ReconcileService that reuses ExportService for live state and ManifestDiffer for comparison, mapping diff actions to drift types (DELETE->MISSING, CREATE->EXTRA, UPDATE->MODIFIED) - Add ReconcileManifest gRPC handler with section filtering support - Wire ReconcileService into RegisterManifestHistoryService when ExportCollectors are available - Add filterManifestSections for scoped reconciliation
📝 WalkthroughWalkthroughAdds a read-only manifest reconciliation feature: new ReconcileManifest RPC, a ReconcileService that compares stored manifests to live exported state and produces per-resource drift items and an aggregate summary, handler wiring to expose the RPC, and comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HistoryHandler
participant ReconcileService
participant HistoryService
participant ExportService
participant Differ
participant ProtoConverter
Client->>HistoryHandler: ReconcileManifest(version, includeSections)
HistoryHandler->>ReconcileService: Reconcile(ctx, version, includeSections)
ReconcileService->>HistoryService: Load stored manifest (version)
HistoryService-->>ReconcileService: Stored manifest
ReconcileService->>ExportService: Export live state
ExportService-->>ReconcileService: Live manifest
ReconcileService->>ReconcileService: Filter stored manifest (includeSections)
ReconcileService->>Differ: Diff(filtered stored, live)
Differ-->>ReconcileService: DiffPlan
ReconcileService->>ProtoConverter: ToProtoResponse(diff results)
ProtoConverter-->>ReconcileService: ReconcileManifestResponse
ReconcileService-->>HistoryHandler: ReconcileManifestResponse
HistoryHandler-->>Client: ReconcileManifestResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Claude Code ReviewCommit: SummaryClean implementation of the This satisfies the PRD 045 requirement: "ReconcileManifest: Compares the DB-stored manifest against live resource state across services. Reports drift." What's good:
Risk Assessment
Findings
Bot Review NotesCodeRabbit: Validate CodeRabbit: Don't diff a partial export as if it were complete live state ( CodeRabbit: Previously Flagged
|
- Populate reconciled_at timestamp in ToProtoResponse - Use ErrHistoryServiceRequired sentinel instead of ErrNilRepository for nil history parameter validation
bjcoombs
dismissed
claude[bot]’s stale review
Both issues addressed in b8e4231: populated reconciled_at in ToProtoResponse and switched to ErrHistoryServiceRequired sentinel.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/proto/meridian/control_plane/v1/manifest_history_service.proto`:
- Around line 339-349: Validate the repeated string field include_sections (in
manifest_history_service.proto) against the canonical set of supported section
names (the same enum/strings used by ExportManifestRequest and the reconcile
logic) rather than accepting arbitrary strings: add proto-level validation if
possible (e.g., replace/augment repeated string with a repeated enum or use a
custom buf.validate rule) or enforce this in the server-side handler that
processes include_sections (e.g., the reconcile function) to reject the request
when an unknown name appears and return a clear validation error, and
additionally fail the request if none of the requested sections resolve so a
typo (like "instrument" vs "instruments") does not silently produce a no-op
success.
In `@services/control-plane/internal/manifest/reconcile.go`:
- Around line 100-125: The code currently diffs exportResult.Manifest
unconditionally; detect partial/incomplete exports first (e.g., inspect
exportResult.Partial or exportResult.IncompleteSections—or if the exporter
signals incompleteness only via exportResult.Warnings, detect relevant
unreachable/partial warnings) and then either fail the reconcile with a clear
error (return fmt.Errorf("export incomplete: %v", exportResult.Warnings) ) or
remove the incomplete sections from exportResult.Manifest by calling
filterManifestSections on exportResult.Manifest (similar to how filteredStored
is produced) before calling s.d.Diff; update the call sites referencing
exportResult.Manifest, exportResult.Warnings, s.d.Diff, filterManifestSections,
and includeSections accordingly so the differ never sees a partial live
manifest.
- Around line 247-270: The ToProtoResponse method on ReconcileResult is dropping
the ReconciledAt timestamp, so update ReconcileResult.ToProtoResponse to set
resp.ReconciledAt = timestamppb.New(r.ReconciledAt) (or nil-check if zero) when
building the controlplanev1.ReconcileManifestResponse; add the required
"google.golang.org/protobuf/types/known/timestamppb" import and update or add a
unit test asserting the response.ReconciledAt is populated after calling
Reconcile() to ensure the field is serialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9038d9fc-3875-44ce-80c6-88ba6af1d49f
📒 Files selected for processing (5)
api/proto/meridian/control_plane/v1/manifest_history_service.protoservices/control-plane/internal/manifest/grpc_handler.goservices/control-plane/internal/manifest/reconcile.goservices/control-plane/internal/manifest/reconcile_test.goservices/control-plane/service/manifest_history.go
| // include_sections filters which manifest sections to reconcile. | ||
| // If empty, all sections are reconciled. Valid values match ExportManifestRequest. | ||
| repeated string include_sections = 2 [(buf.validate.field).repeated = { | ||
| max_items: 20 | ||
| items: { | ||
| string: { | ||
| min_len: 1 | ||
| max_len: 64 | ||
| } | ||
| } | ||
| }]; |
There was a problem hiding this comment.
Validate include_sections against the supported section set.
This field currently accepts arbitrary strings, and the reconcile path silently drops unknown names. A typo like "instrument" instead of "instruments" will reconcile zero sections and return a clean result, which is a false negative for a drift detector. Please reject unknown values here, or fail the request when none of the requested sections resolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/proto/meridian/control_plane/v1/manifest_history_service.proto` around
lines 339 - 349, Validate the repeated string field include_sections (in
manifest_history_service.proto) against the canonical set of supported section
names (the same enum/strings used by ExportManifestRequest and the reconcile
logic) rather than accepting arbitrary strings: add proto-level validation if
possible (e.g., replace/augment repeated string with a repeated enum or use a
custom buf.validate rule) or enforce this in the server-side handler that
processes include_sections (e.g., the reconcile function) to reject the request
when an unknown name appears and return a clear validation error, and
additionally fail the request if none of the requested sections resolve so a
typo (like "instrument" vs "instruments") does not silently produce a no-op
success.
| // Export live state using the same section filter. | ||
| exportResult, err := s.exporter.Export(ctx, includeSections, storedVersion) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("export live state: %w", err) | ||
| } | ||
|
|
||
| // If include_sections is specified, zero out sections not in the filter | ||
| // on the stored manifest so the differ only sees requested sections. | ||
| filteredStored := storedManifest | ||
| if len(includeSections) > 0 { | ||
| filteredStored = filterManifestSections(storedManifest, includeSections) | ||
| } | ||
|
|
||
| // Diff stored (as "last-applied") against live (as "new") to detect drift. | ||
| // Stored=old, Live=new: | ||
| // - DELETE actions mean resource in stored but not live -> MISSING | ||
| // - CREATE actions mean resource in live but not stored -> EXTRA | ||
| // - UPDATE actions mean resource differs -> MODIFIED | ||
| plan, err := s.d.Diff(ctx, filteredStored, exportResult.Manifest, differ.WithSkipSafetyChecks()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("diff failed: %w", err) | ||
| } | ||
|
|
||
| result := diffPlanToReconcileResult(plan, storedVersion) | ||
| result.ReconciledAt = time.Now().UTC() | ||
| result.Warnings = exportResult.Warnings |
There was a problem hiding this comment.
Don't diff a partial export as if it were complete live state.
The export/reconcile contract already allows warnings for unreachable services and partial data, but this path still diffs exportResult.Manifest unconditionally and only attaches the warnings afterward. That means a collector outage can surface as DRIFT_TYPE_MISSING / DRIFT_TYPE_EXTRA instead of “unchecked”. Please fail reconciliation or explicitly exclude incomplete sections before calling the differ.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/control-plane/internal/manifest/reconcile.go` around lines 100 -
125, The code currently diffs exportResult.Manifest unconditionally; detect
partial/incomplete exports first (e.g., inspect exportResult.Partial or
exportResult.IncompleteSections—or if the exporter signals incompleteness only
via exportResult.Warnings, detect relevant unreachable/partial warnings) and
then either fail the reconcile with a clear error (return fmt.Errorf("export
incomplete: %v", exportResult.Warnings) ) or remove the incomplete sections from
exportResult.Manifest by calling filterManifestSections on exportResult.Manifest
(similar to how filteredStored is produced) before calling s.d.Diff; update the
call sites referencing exportResult.Manifest, exportResult.Warnings, s.d.Diff,
filterManifestSections, and includeSections accordingly so the differ never sees
a partial live manifest.
There was a problem hiding this comment.
Both previous findings resolved in b8e4231. No correctness issues remain. CodeRabbit's open threads (section validation, partial export diffing) are valid quality improvements for a future PR but don't affect correctness of this read-only Phase 1 safety-net. See summary comment for full analysis.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/control-plane/internal/manifest/reconcile.go (1)
101-126:⚠️ Potential issue | 🟠 MajorDon't diff partial live exports as authoritative state.
exportResult.Warningsis only attached afters.d.Diff(...)runs. If the export path can return partial manifests when a collector is unavailable, this will turn “unchecked” sections into falseMISSING/EXTRAdrift. Please reject incomplete exports or strip unchecked sections before diffing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/manifest/reconcile.go` around lines 101 - 126, The export can be partial (exportResult contains Warnings) and currently you run s.d.Diff(...) before handling that, which causes unchecked/missing sections to be treated as authoritative; after calling s.exporter.Export(ctx, includeSections, storedVersion) inspect exportResult for warnings or partial-coverage indicators and either (A) reject the export by returning an error (e.g., fmt.Errorf("incomplete export: %v", exportResult.Warnings)) or (B) strip out unchecked sections from exportResult.Manifest using the same filterManifestSections helper (or a new helper that removes sections marked as unchecked) so that s.d.Diff(ctx, filteredStored, filteredLive, ...) only compares sections both exported and requested; adjust the use sites diffPlanToReconcileResult and result.Warnings to preserve/report warnings but ensure the diff is only run against the cleaned/validated manifests.
🧹 Nitpick comments (1)
services/control-plane/internal/manifest/reconcile.go (1)
203-245: Consider validating section names in the RPC handler to surface typos or invalid selections.
filterManifestSections()silently ignores unknown section names—if a client passes a typo or invalid section name (e.g.,["instuments"]instead of["instruments"]), the function returns a manifest with only metadata, making drift detection report "no drift" when nothing was actually checked. The gRPC handler does not validateinclude_sectionsbefore callingReconcile(), unlike other handlers in the file that explicitly check for invalid arguments.Add validation in
ReconcileManifest()to reject requests with unrecognized section names (similar to theInvalidArgumentchecks for other parameters), or explicitly document that unknown sections are silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/manifest/reconcile.go` around lines 203 - 245, Add validation for unknown section names in the ReconcileManifest RPC path: before calling filterManifestSections, call parseSections(includeSections) (or reuse its logic) and check that every provided name maps to a known Section constant (SectionInstruments, SectionAccountTypes, SectionValuationRules, SectionSagas, SectionMarketData, SectionOrganizations, SectionInternalAccounts, SectionOperationalGateway, SectionMappings, SectionPartyTypes, SectionPaymentRails); if any name is unrecognized return a gRPC InvalidArgument error indicating the invalid section(s) instead of proceeding silently. This ensures requests with typos (e.g., "instuments") are rejected by ReconcileManifest rather than filtered out by filterManifestSections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@services/control-plane/internal/manifest/reconcile.go`:
- Around line 101-126: The export can be partial (exportResult contains
Warnings) and currently you run s.d.Diff(...) before handling that, which causes
unchecked/missing sections to be treated as authoritative; after calling
s.exporter.Export(ctx, includeSections, storedVersion) inspect exportResult for
warnings or partial-coverage indicators and either (A) reject the export by
returning an error (e.g., fmt.Errorf("incomplete export: %v",
exportResult.Warnings)) or (B) strip out unchecked sections from
exportResult.Manifest using the same filterManifestSections helper (or a new
helper that removes sections marked as unchecked) so that s.d.Diff(ctx,
filteredStored, filteredLive, ...) only compares sections both exported and
requested; adjust the use sites diffPlanToReconcileResult and result.Warnings to
preserve/report warnings but ensure the diff is only run against the
cleaned/validated manifests.
---
Nitpick comments:
In `@services/control-plane/internal/manifest/reconcile.go`:
- Around line 203-245: Add validation for unknown section names in the
ReconcileManifest RPC path: before calling filterManifestSections, call
parseSections(includeSections) (or reuse its logic) and check that every
provided name maps to a known Section constant (SectionInstruments,
SectionAccountTypes, SectionValuationRules, SectionSagas, SectionMarketData,
SectionOrganizations, SectionInternalAccounts, SectionOperationalGateway,
SectionMappings, SectionPartyTypes, SectionPaymentRails); if any name is
unrecognized return a gRPC InvalidArgument error indicating the invalid
section(s) instead of proceeding silently. This ensures requests with typos
(e.g., "instuments") are rejected by ReconcileManifest rather than filtered out
by filterManifestSections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4ac6072-a1ff-40b3-9d23-a7f0155190cb
📒 Files selected for processing (2)
services/control-plane/internal/manifest/reconcile.goservices/control-plane/internal/manifest/reconcile_test.go
bjcoombs
dismissed
coderabbitai[bot]’s stale review
All issues addressed: reconciled_at fixed in b8e4231. include_sections validation and partial-export behavior are consistent with existing ExportManifest design.
bjcoombs
deleted the
045-manifest-source-of-truth--15--reconcile-manifest-rpc
branch
1 participant
Summary
ReconcileManifestRPC toManifestHistoryServicethat compares a stored manifest version against live service state and reports drift as structured output (MISSING/MODIFIED/EXTRA)ExportServicefor live state collection andManifestDifferfor comparison, mapping diff actions to drift typesReconcileServiceinto service registration whenExportCollectorsare availableChanges
manifest_history_service.protoReconcileManifestRPC,DriftItem,DriftTypeenum,ReconcileSummarymessagesreconcile.goReconcileServicewithdiffPlanToReconcileResultconversion,filterManifestSectionsfor scoped reconciliationgrpc_handler.goReconcileManifesthandler,NewHistoryHandlerWithReconcileconstructormanifest_history.go(service)ReconcileServiceintoRegisterManifestHistoryServiceDrift type mapping
The differ compares stored (old) vs live (new):
DELETEaction (in stored but not live) maps toDRIFT_TYPE_MISSINGCREATEaction (in live but not stored) maps toDRIFT_TYPE_EXTRAUPDATEaction (both exist but differ) maps toDRIFT_TYPE_MODIFIEDTest plan
NewReconcileServiceconstructor validation (nil history, nil exporter, nil differ)diffPlanToReconcileResultwith all drift types (missing, extra, modified, mixed, no drift)toDriftTypeProtoenum conversionReconcileResult.ToProtoResponseserializationfilterManifestSectionssection filteringManifestDifferwith stored vs live manifests