diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5dad20f..7944253 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Smoke Test Implementation Review Findings -Status: all three findings completed 2026-10-13. Review completed 2026-10-13. +Status: Findings 1–3 completed; Finding 4 open. Reviews completed 2026-10-13. Reviewed implementation commits after `8fbd058` through `2f735aedf9b5dcc98164292db46ae1eab3e15cde`, against the @@ -12,7 +12,8 @@ The facade/internal boundary, final-value configuration ownership, catalog-owned diagnostic metadata, shared generation/capacity machinery, private inference cases, and neutral factual reporting are in place. No additional strengthened contracts or prohibitions requiring remediation were identified. The findings -below are reporting-correctness gaps, not reasons to redesign the feature. +from the initial review are reporting-correctness gaps, not reasons to redesign +the feature. A follow-up duplication review is recorded as Finding 4. ## Finding 1: Preserve Caller Cancellation Independently Of Body Error Type @@ -138,7 +139,65 @@ The strengthened orchestration assertions failed before the fix. Final validatio passed with `GOWORK=off`: `go test -race ./...`, `go vet ./...`, the three offline consumer examples, and `git diff --check`. -## Review Validation +## Finding 4: Consolidate Smoke Report Ownership Copying + +Priority: P3. Status: open. Maintainability/efficiency consolidation candidate. + +Follow-up review covers the implementation and remediation commits through +`5ea6f06a6d0be32ecb68bc98513f02f29be1cae3`. + +Locations: [`fromDomainSmokeTestReport`](../../smoke_diagnostics.go), +lines 296–336; [`CloneSmokeTestReport`](../../internal/domain/smoke_diagnostics.go), +lines 180–221; and [`fromDomainExecutionTarget`](../../convert.go). + +The facade first deep-clones the entire internal report through +`domain.CloneSmokeTestReport`. Its conversion then copies the same mutable data +again: applicability/fact pointers, timing pointers, HTTP status pointers, nested +provider maps, and the attempt slice. The domain clone already walks every +attempt and copies its facts/timings; the facade performs another complete walk +and repeats those ownership copies while projecting the public types. The +intermediate cloned report is local to that conversion and is not independently +returned or retained. + +This is a duplicate full-report ownership path, not merely the necessary mapping +between public and internal declarations. It adds avoidable intermediate copying +and allocation across the whole report, including nested provider data and every +attempt. No functional corruption was reproduced, and this review does not claim +a measured latency improvement; the duplicate operations are evident directly +from the current call path. + +Recommended outcome: establish one copying/materialization boundary for the +public report, preferably copying mutable values while building the public +representation instead of first cloning an entire intermediate report. Reuse +appropriate existing ownership helpers and preserve copy-error handling and +credential exclusion. An alternative is to project an already-owned clone without +cloning its members again, where that is simpler. A focused conversion cleanup +is sufficient; public/internal types remain distinct and a new generic cloning +framework is unnecessary. Keep domain cloning where it serves a genuinely +separate ownership boundary. + +Validation: retain the existing public report-detachment coverage and applicable +domain clone coverage wherever that cloner remains in use. Extend the narrow +conversion/ownership coverage only where needed to protect +nested provider maps, optional facts/timings, and attempts from unintended sharing +with retained data or other results. Do not make exact allocation counts or helper +call order new contracts. + +The review found no other substantially duplicated execution subsystem warranting +consolidation. Ordinary and diagnostic generation delegate to the same private +coordinator loop; request construction, credential resolution, profile overrides, +capacity management, failure categories, usage arithmetic, and structural +validation reuse existing owners. Diagnostic selection, private case assembly, +provider-specific authentication recognition, and partial-report orchestration +serve different contracts from ordinary application preparation. Small pointer +helpers and DTO projections alone do not justify additional remediation entries. + +Focused ownership tests passed on this revision: +`GOWORK=off go test . ./internal/domain -run +'TestSmokeTestConcludedInferenceMismatchHasNilGoErrorAndDetachedValues|TestCloneSmokeTestReportOwnsMutableValues' +-count=1`. No implementation was changed during the duplication review. + +## Initial Review Validation Passed with `GOWORK=off`: