41 lines
4.6 KiB
Markdown
41 lines
4.6 KiB
Markdown
# Research: Compare Job Legacy Drift Path Cleanup
|
|
|
|
## Decision 1: Treat the strategy-driven compare path as the only authoritative execution engine
|
|
|
|
- **Decision**: Use the existing `CompareStrategyRegistry` -> `IntuneCompareStrategy` path as the sole supported compare execution boundary.
|
|
- **Rationale**: Current code inspection shows `CompareBaselineToTenantJob::handle()` selecting a strategy, resolving it, and calling `strategy->compare(...)` before normalizing subject results and writing findings. No productive call path from `handle()` reaches the retained monolithic `computeDrift()` block.
|
|
- **Alternatives considered**:
|
|
- Keep the legacy block as a documented fallback. Rejected because it leaves the file structurally dishonest and suggests a second engine still exists.
|
|
- Add a feature flag between strategy and legacy execution. Rejected because there is no legitimate second execution mode left to preserve.
|
|
|
|
## Decision 2: Delete the dead drift cluster instead of archiving or deprecating it
|
|
|
|
- **Decision**: Remove `computeDrift()` and its exclusive helper cluster directly from `CompareBaselineToTenantJob`.
|
|
- **Rationale**: The retained cluster duplicates pre-strategy compare logic that has already been extracted into the active strategy implementation. Keeping it in place continues to mislead reviewers and inflates the orchestration file without operational value.
|
|
- **Alternatives considered**:
|
|
- Move the dead methods to a trait or archive class. Rejected because it preserves confusion and ownership cost without any runtime benefit.
|
|
- Leave the methods in place with a deprecation comment. Rejected because dead code still obscures the real call graph even when labeled.
|
|
|
|
## Decision 3: Preserve existing compare contracts, findings, and run semantics unchanged
|
|
|
|
- **Decision**: Keep `CompareStrategyRegistry`, `IntuneCompareStrategy`, `CompareStrategySelection`, `CompareSubjectResult`, `CompareFindingCandidate`, existing finding writers, and existing run context semantics unchanged.
|
|
- **Rationale**: Spec 205 is a closure cleanup, not a second strategy extraction spec. The safest path is deletion of dead code while leaving the live contracts and persisted truths untouched.
|
|
- **Alternatives considered**:
|
|
- Fold in additional compare refactors while editing the job. Rejected because that turns a narrow cleanup into a mixed review.
|
|
- Rename or reframe current compare contracts for symmetry. Rejected because it is unrelated to dead-path removal.
|
|
|
|
## Decision 4: Use a focused compare plus run-guard pack as the minimum regression slice
|
|
|
|
- **Decision**: Validate the cleanup with `FindingFidelityTest`, `BaselineCompareFindingsTest`, `BaselineCompareGapClassificationTest`, `BaselineCompareWhyNoFindingsReasonCodeTest`, `Spec116OneEngineGuardTest`, `OperationLifecycleOpsUxGuardTest`, `BaselineOperationRunGuardTest`, `OperationSummaryKeysSpecTest`, `SummaryCountsWhitelistTest`, `Spec118NoLegacyBaselineDriftGuardTest`, and `BaselineCompareMatrixCompareAllActionTest` as the required focused regression pack.
|
|
- **Rationale**: `FindingFidelityTest` exercises the compare execution path and evidence selection behavior, `BaselineCompareFindingsTest` protects finding lifecycle and summary outcomes, the gap and reason-code tests protect warning and reason semantics, `Spec116OneEngineGuardTest` keeps the one-engine orchestration invariant explicit, the `OperationRun` and summary-count guards protect lifecycle invariants, and the legacy guard keeps helper re-entry visible in CI. Together they provide high confidence for a mechanical delete without requiring a broad slow suite.
|
|
- **Alternatives considered**:
|
|
- Run the full baseline compare suite for every cleanup iteration. Rejected as optional rather than required for a small internal delete.
|
|
- Skip targeted tests and rely only on formatting or static inspection. Rejected as insufficient confidence.
|
|
|
|
## Decision 5: Keep the planning artifacts logical and invariant-focused
|
|
|
|
- **Decision**: Document the cleanup through a logical internal contract and a no-new-entity data model rather than inventing cleanup-specific services, APIs, or persistence.
|
|
- **Rationale**: The plan workflow still needs explicit design artifacts, but Spec 205 adds no new feature surface. The correct documentation shape is therefore an invariant record of the unchanged compare boundaries after dead-code deletion.
|
|
- **Alternatives considered**:
|
|
- Skip the contract artifact entirely because no new endpoint exists. Rejected because the planning workflow requires a contract deliverable.
|
|
- Invent a cleanup-specific service or endpoint in the design docs. Rejected because it would introduce fake architecture not warranted by the spec. |