diff --git a/convert.go b/convert.go index bbf1052..eaad194 100644 --- a/convert.go +++ b/convert.go @@ -211,6 +211,12 @@ func toDomainExecutionTargetOverride(override *ExecutionTargetOverride) (*domain } func fromDomainExecutionTarget(target domain.ExecutionTarget) ExecutionTarget { + return fromDomainExecutionTargetWithOwnedParams(target, copyAnyMap(target.ExtraParams)) +} + +// fromDomainExecutionTargetWithOwnedParams projects scalar settings and takes +// ownership of extraParams, which the caller has already copied. +func fromDomainExecutionTargetWithOwnedParams(target domain.ExecutionTarget, extraParams map[string]any) ExecutionTarget { return ExecutionTarget{ BackendID: target.BackendID, Endpoint: target.Endpoint, @@ -222,7 +228,7 @@ func fromDomainExecutionTarget(target domain.ExecutionTarget) ExecutionTarget { ServiceTier: target.ServiceTier, ReasoningEffort: target.ReasoningEffort, APIKeyEnv: target.APIKeyEnv, - ExtraParams: copyAnyMap(target.ExtraParams), + ExtraParams: extraParams, } } diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 7944253..0fdbf85 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Smoke Test Implementation Review Findings -Status: Findings 1–3 completed; Finding 4 open. Reviews completed 2026-10-13. +Status: All four findings completed 2026-10-13. Reviewed implementation commits after `8fbd058` through `2f735aedf9b5dcc98164292db46ae1eab3e15cde`, against the @@ -141,7 +141,7 @@ consumer examples, and `git diff --check`. ## Finding 4: Consolidate Smoke Report Ownership Copying -Priority: P3. Status: open. Maintainability/efficiency consolidation candidate. +Priority: P3. Status: completed 2026-10-13. Follow-up review covers the implementation and remediation commits through `5ea6f06a6d0be32ecb68bc98513f02f29be1cae3`. @@ -197,6 +197,17 @@ Focused ownership tests passed on this revision: 'TestSmokeTestConcludedInferenceMismatchHasNilGoErrorAndDetachedValues|TestCloneSmokeTestReportOwnsMutableValues' -count=1`. No implementation was changed during the duplication review. +Completion: the facade now copies mutable values directly into the public report, +without first cloning an intermediate domain report. Provider parameters retain +the existing JSON-value copy/error handling and pass through the shared target +projection without another copy; credentials remain excluded. Domain cloning +continues to serve its separate internal ownership boundary. Focused conversion +coverage protects retained source values, independently converted results, nested +maps/slices, optional facts/timings, attempts, credential exclusion, and copy errors. +Existing public and domain ownership tests remain unchanged. Validation passed +with `GOWORK=off`: focused ownership tests, `go test -race ./...`, `go vet ./...`, +the three offline consumer examples, and `git diff --check`. + ## Initial Review Validation Passed with `GOWORK=off`: diff --git a/smoke_conversion_internal_test.go b/smoke_conversion_internal_test.go new file mode 100644 index 0000000..5f5ce28 --- /dev/null +++ b/smoke_conversion_internal_test.go @@ -0,0 +1,84 @@ +package promptkit + +import ( + "encoding/json" + "reflect" + "strings" + "testing" + "time" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" +) + +func TestSmokeReportConversionOwnsMutableValues(t *testing.T) { + applicable, reachable := true, true + status := 200 + wait, duration := 3*time.Millisecond, 5*time.Millisecond + source := domain.SmokeTestReport{ + Applicable: &applicable, + Target: domain.SmokeTestTarget{EffectiveSettings: domain.ExecutionTarget{ + APIKey: "synthetic-private-key", + ExtraParams: map[string]any{"nested": []any{map[string]any{"value": "original"}}}, + }}, + Facts: domain.SmokeTestFacts{EndpointReachable: &reachable}, + Timing: domain.SmokeTestTiming{CapacityWait: &wait}, + Attempts: []domain.SmokeTestAttempt{{ + Endpoint: "https://example.test/chat/completions", + HTTPStatus: &status, + Facts: domain.SmokeTestFacts{EndpointReachable: &reachable}, + Timing: domain.SmokeTestTiming{RequestDuration: &duration}, + }}, + } + converted, err := fromDomainSmokeTestReport(source) + if err != nil { + t.Fatal(err) + } + other, err := fromDomainSmokeTestReport(source) + if err != nil { + t.Fatal(err) + } + encoded, err := json.Marshal(converted) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(encoded), source.Target.EffectiveSettings.APIKey) { + t.Fatal("converted report included a credential") + } + + *converted.Applicable = false + *converted.Facts.EndpointReachable = false + *converted.Timing.CapacityWait = time.Second + converted.Attempts[0].Endpoint = "changed" + *converted.Attempts[0].HTTPStatus = 503 + *converted.Attempts[0].Facts.EndpointReachable = false + *converted.Attempts[0].Timing.RequestDuration = time.Second + converted.Target.EffectiveSettings.ExtraParams["nested"].([]any)[0].(map[string]any)["value"] = "changed" + + if !applicable || !reachable || status != 200 || wait != 3*time.Millisecond || duration != 5*time.Millisecond { + t.Fatal("converted report shared optional scalar pointers with its source") + } + if source.Attempts[0].Endpoint != "https://example.test/chat/completions" { + t.Fatal("converted report shared its attempt slice with its source") + } + if got := source.Target.EffectiveSettings.ExtraParams["nested"].([]any)[0].(map[string]any)["value"]; got != "original" { + t.Fatalf("converted report changed source provider parameters: %v", got) + } + later, err := fromDomainSmokeTestReport(source) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(other, later) { + t.Fatal("converted report mutation affected another result") + } +} + +func TestSmokeReportConversionPreservesProviderMapCopyError(t *testing.T) { + report, err := fromDomainSmokeTestReport(domain.SmokeTestReport{ + Target: domain.SmokeTestTarget{EffectiveSettings: domain.ExecutionTarget{ + ExtraParams: map[string]any{"unsupported": func() {}}, + }}, + }) + if err == nil || report != nil { + t.Fatalf("invalid provider data produced report=%+v, error=%v", report, err) + } +} diff --git a/smoke_diagnostics.go b/smoke_diagnostics.go index 2edbb48..980eec7 100644 --- a/smoke_diagnostics.go +++ b/smoke_diagnostics.go @@ -6,6 +6,7 @@ import ( "time" "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" ) // SmokeTestKind identifies one built-in diagnostic operation. @@ -294,35 +295,35 @@ func toDomainSmokeTestRequest(request SmokeTestRequest) (domain.SmokeTestRequest } func fromDomainSmokeTestReport(report domain.SmokeTestReport) (*SmokeTestReport, error) { - owned, err := domain.CloneSmokeTestReport(report) + extraParams, err := jsonvalue.CopyMap(report.Target.EffectiveSettings.ExtraParams) if err != nil { return nil, err } result := &SmokeTestReport{ - Kind: SmokeTestKind(owned.Kind), - Supported: owned.Supported, - Applicable: copySmokeTestBoolPtr(owned.Applicable), - Completed: owned.Completed, - Reason: SmokeTestReason(owned.Reason), + Kind: SmokeTestKind(report.Kind), + Supported: report.Supported, + Applicable: copySmokeTestBoolPtr(report.Applicable), + Completed: report.Completed, + Reason: SmokeTestReason(report.Reason), Target: SmokeTestTarget{ - BackendID: owned.Target.BackendID, - ProfileID: owned.Target.ProfileID, - PromptID: owned.Target.PromptID, - PromptVersion: owned.Target.PromptVersion, - DefaultSourceBackendID: owned.Target.DefaultSourceBackendID, - SelectionSource: owned.Target.SelectionSource, - Endpoint: owned.Target.Endpoint, - EffectiveSettings: fromDomainExecutionTarget(owned.Target.EffectiveSettings), - SettingsPresence: fromDomainExecutionTargetPresence(owned.Target.SettingsPresence), + BackendID: report.Target.BackendID, + ProfileID: report.Target.ProfileID, + PromptID: report.Target.PromptID, + PromptVersion: report.Target.PromptVersion, + DefaultSourceBackendID: report.Target.DefaultSourceBackendID, + SelectionSource: report.Target.SelectionSource, + Endpoint: report.Target.Endpoint, + EffectiveSettings: fromDomainExecutionTargetWithOwnedParams(report.Target.EffectiveSettings, extraParams), + SettingsPresence: fromDomainExecutionTargetPresence(report.Target.SettingsPresence), }, - Facts: fromDomainSmokeTestFacts(owned.Facts), - Timing: fromDomainSmokeTestTiming(owned.Timing), - Usage: fromDomainObservedTokenUsage(owned.Usage), - Failure: ExecutionFailureKind(owned.Failure), + Facts: fromDomainSmokeTestFacts(report.Facts), + Timing: fromDomainSmokeTestTiming(report.Timing), + Usage: fromDomainObservedTokenUsage(report.Usage), + Failure: ExecutionFailureKind(report.Failure), } - if owned.Attempts != nil { - result.Attempts = make([]SmokeTestAttempt, len(owned.Attempts)) - for index, attempt := range owned.Attempts { + if report.Attempts != nil { + result.Attempts = make([]SmokeTestAttempt, len(report.Attempts)) + for index, attempt := range report.Attempts { result.Attempts[index] = SmokeTestAttempt{ Dispatched: attempt.Dispatched, InitialEndpoint: attempt.InitialEndpoint,