Record redundant smoke report copying
This commit is contained in:
@@ -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`:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user