diff --git a/docs/internal/llm.md b/docs/internal/llm.md index da3ab1b..9cc79ef 100644 --- a/docs/internal/llm.md +++ b/docs/internal/llm.md @@ -41,6 +41,10 @@ then: the complete-request failure remains a request failure; 6. resolves authentication and freezes the URL, body, and optional credential. +Provider values are detached with trusted copying before encoding. Custom JSON +encoding methods and per-value encoding-failure diagnostics operate on those +private copies rather than retained prepared or diagnostic metadata. + Each execution of that prepared request creates a fresh HTTP request, body reader, and attempt timeout context. It closes the response body and returns the HTTP status, decoded response or error, reported token values and presence, diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 4a46c7a..04f6511 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -7,17 +7,29 @@ against [responsibility-simplification.md](responsibility-simplification.md) and its accepted audit decisions. The implementation spans `448e64d` through `5ee602cc4b5a3afe4e7d361eb77181ef0940c7cd`, after planning commit `de46935`. -The targeted architecture is substantially implemented. One ownership regression -remains open below. No additional unintended restriction or public guarantee was -identified; removing copier-derived schema limits is the selected acceptance -change, while runtime provider-value support remains in place. The earlier +The targeted architecture is implemented, and the ownership regression below +is now addressed. Its evidence and review table describe the pre-fix baseline; +the completion marker records the correction. No additional unintended +restriction or public guarantee was identified. Removing copier-derived schema +limits remains the selected acceptance change, while runtime provider-value +support remains in place. The earlier 33-stage review and its five completed findings are retained separately below. ### R1. Isolate Custom JSON Encoders From Retained Metadata [P2] -**Status:** Open. **Stages:** 2, 13–15; particularly the shared prepared snapshot +**Status:** Complete. **Stages:** 2, 13–15; particularly the shared prepared snapshot and removal of private smoke-target detachment. +Completion: built-in generation preparation uses trusted detachment before +payload construction, so whole-payload encoding and per-value encoding-failure +diagnostics cannot mutate retained provider values through their receivers. +The single prepared snapshot and named/typed custom encoding remain intact. +Public regressions failed before the fix and now protect prepared details, +result settings, smoke settings, caller input, custom wire output, and failure +classification without dispatch. Go 1.27.3 `go test -race ./...` and +`go vet ./...` passed, including the existing retry, repair, and concurrent +prepared-lifecycle coverage. Internal model-client documentation was updated. + **Locations:** [provider payload construction](../../internal/llm/openai_compatible_client.go), `openAIChatRequestPayload`, lines 468–475, and `PrepareGeneration`, lines 133–139; [prepared metadata sharing](../../internal/usecase/prepared_execution.go), @@ -94,6 +106,9 @@ injected initial/repair mutation and concurrent prepared lifecycle protection. ### Stage-By-Stage Review +This is the pre-remediation review record; R1's completion above addresses its +identified gap without changing the other selected boundaries. + | Stage | Current evidence and result | | --- | --- | | 1 | Constructor routing defaults and their only error path are removed. Resolved endpoint/model checks and HTTP-client timeout ownership remain. No additional finding. | @@ -114,9 +129,10 @@ injected initial/repair mutation and concurrent prepared lifecycle protection. | 16 | Canonical format, integration, model-client, runner, source, and component guidance reflects the selected changes without new resource/provider promises. The immutability description is not fully realized until R1 is fixed. | | 17 | Fresh full race tests and vet pass, but those checks do not cover R1. Documentation/history links and delivered scope were reviewed; completion still requires the ownership correction. | -### Current Validation And Limits +### Review Validation And Limits -Using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, and `GOPROXY=off`: +At the reviewed pre-fix baseline, using Go 1.27.3 with `GOTOOLCHAIN=local`, +`GOWORK=off`, and `GOPROXY=off`: - `go test -race ./...` passed, including the maintained offline examples. - `go vet ./...` passed. @@ -124,9 +140,10 @@ Using Go 1.27.3 with `GOTOOLCHAIN=local`, `GOWORK=off`, and `GOPROXY=off`: errors and unchanged caller values; the two-file historical overlay prevented both mutations. -No production code or repository tests were changed. The source review and -targeted probe establish R1 despite the passing existing suite. The review does -not claim universal callback safety, provider compatibility, or defect-free code. +The review changed no production code or repository tests. Its source analysis +and targeted probe established R1 despite the passing existing suite; the +completion marker above records the subsequent fix and validation. The review +does not claim universal callback safety, provider compatibility, or defect-free code. ## Earlier Audit Remediation Review: Scope And Status diff --git a/internal/llm/openai_compatible_client.go b/internal/llm/openai_compatible_client.go index 129c921..0535f87 100644 --- a/internal/llm/openai_compatible_client.go +++ b/internal/llm/openai_compatible_client.go @@ -17,6 +17,7 @@ import ( "go.maximumdirect.net/promptkit/internal/defaults" "go.maximumdirect.net/promptkit/internal/domain" + "go.maximumdirect.net/promptkit/internal/jsonvalue" ) var ( @@ -125,6 +126,9 @@ func (c *OpenAICompatibleClient) PrepareGeneration(req domain.GenerateRequest) ( return nil, fmt.Errorf("%w: invalid endpoint path: %v", ErrInvalidRequest, err) } + // Custom JSON encoders may mutate their receivers. Both payload encoding + // and encoding-failure diagnostics must use values detached from metadata. + req.Target.ExtraParams = jsonvalue.CloneTrusted(req.Target.ExtraParams).(map[string]any) wirePayload, err := openAIChatRequestPayload(req) if err != nil { return nil, fmt.Errorf("%w: %v", ErrInvalidRequest, err) diff --git a/provider_encoding_ownership_contract_test.go b/provider_encoding_ownership_contract_test.go new file mode 100644 index 0000000..e005b60 --- /dev/null +++ b/provider_encoding_ownership_contract_test.go @@ -0,0 +1,110 @@ +package promptkit_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "reflect" + "testing" + + "go.maximumdirect.net/promptkit" +) + +type receiverMutatingProviderMap map[string]string + +func (value receiverMutatingProviderMap) MarshalJSON() ([]byte, error) { + value["value"] = "encoded" + if value["fail"] == "yes" { + return nil, errors.New("synthetic encoding failure") + } + return json.Marshal(map[string]string(value)) +} + +func TestBuiltInEncodingPreservesPreparedProviderValues(t *testing.T) { + for _, fail := range []bool{false, true} { + name := "successful encoding" + if fail { + name = "encoding failure diagnostics" + } + t.Run(name, func(t *testing.T) { + calls := 0 + config := simpleConfig("message") + config.HTTPClient = &http.Client{Transport: encodingOwnershipTransport(t, &calls)} + engine, err := promptkit.NewEngine(config) + if err != nil { + t.Fatal(err) + } + options := receiverMutatingProviderMap{"value": "original"} + if fail { + options["fail"] = "yes" + } + prepared, err := engine.PrepareExecution(context.Background(), promptkit.RunRequest{ + PromptID: "prompt", Execution: &promptkit.ExecutionTargetOverride{ + ExtraParams: map[string]any{"options": options}, + }, + }) + if err != nil { + t.Fatal(err) + } + before := prepared.Details() + result, err := engine.RunPrepared(context.Background(), prepared) + if fail { + if !errors.Is(err, promptkit.ErrInvalidRequest) || result != nil || calls != 0 { + t.Fatalf("encoding failure: result=%+v error=%v dispatches=%d", result, err, calls) + } + } else if err != nil || result == nil || calls != 1 { + t.Fatalf("successful encoding: result=%+v error=%v dispatches=%d", result, err, calls) + } else if result.EffectiveModelParams.ExtraParams["options"].(receiverMutatingProviderMap)["value"] != "original" { + t.Fatal("custom encoder changed result settings") + } + if !reflect.DeepEqual(before, prepared.Details()) { + t.Fatal("custom encoder changed retained prepared details") + } + if options["value"] != "original" { + t.Fatal("custom encoder changed caller input") + } + }) + } +} + +func TestBuiltInEncodingPreservesSmokeProviderValues(t *testing.T) { + calls := 0 + config := simpleConfig("unused") + config.HTTPClient = &http.Client{Transport: encodingOwnershipTransport(t, &calls)} + engine, err := promptkit.NewEngine(config) + if err != nil { + t.Fatal(err) + } + options := receiverMutatingProviderMap{"value": "original"} + report, err := engine.SmokeTest(context.Background(), promptkit.SmokeTestRequest{ + Kind: promptkit.SmokeTestTextInference, ProfileID: "profile", + Execution: &promptkit.ExecutionTargetOverride{ExtraParams: map[string]any{"options": options}}, + }) + if err != nil || report == nil || !report.Completed || calls != 1 { + t.Fatalf("smoke result=%+v error=%v dispatches=%d", report, err, calls) + } + if report.Target.EffectiveSettings.ExtraParams["options"].(receiverMutatingProviderMap)["value"] != "original" { + t.Fatal("custom encoder changed smoke report settings") + } + if options["value"] != "original" { + t.Fatal("custom encoder changed caller input") + } +} + +func encodingOwnershipTransport(t *testing.T, calls *int) roundTripFunc { + t.Helper() + return func(request *http.Request) (*http.Response, error) { + *calls += 1 + var payload struct { + Options map[string]string `json:"options"` + } + if err := json.NewDecoder(request.Body).Decode(&payload); err != nil { + return nil, err + } + if payload.Options["value"] != "encoded" { + t.Errorf("custom wire encoding was lost: options=%v", payload.Options) + } + return observedHTTPResponse(http.StatusOK, `{"choices":[{"message":{"content":"PROMPTKIT_OK"}}]}`), nil + } +}