diff --git a/docs/roadmap/audit/catalog.md b/docs/roadmap/audit/catalog.md index 778c727..bd4781b 100644 --- a/docs/roadmap/audit/catalog.md +++ b/docs/roadmap/audit/catalog.md @@ -1,6 +1,6 @@ # Consolidated audit catalog -Status: **26 distinct open findings: four P2 and 22 P3**, consolidated from 30 +Status: **25 distinct open findings: three P2 and 22 P3**, consolidated from 30 numbered entries in seven audit documents. Reviewed audit state: `db732e3a1de55c844f77870e1c6db683c2685d5d`. @@ -58,6 +58,11 @@ Also noted in the [documentation cross-references](13-documentation.md#previousl ## F02 — P2: CLI provider parameters share mutable state across requests +**Complete.** CLI JSON is copied at captured binding application and each +aggregate seed boundary. Both regressions failed before the fix and focused +aligned-workspace tests passed, including nested objects and arrays, nil and +empty values, and numeric types. + ckprompt assigns parsed `ExtraParams` maps directly to request destinations, and smoke seeds share those nested maps. Mutation by one callback can change later smoke requests or another destination receiving the same captured bindings. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 671c4e7..5d0fd56 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Audit remediation implementation plan -Status: in progress; Stage 1 complete. +Status: in progress; Stages 1–2 complete. Baseline: `d492c7747175c01a53261206c2b4356e53453621`. Before this sprint, intervening changes recorded decisions and retired the completed older @@ -135,6 +135,13 @@ successful incomplete aggregate or lose cancellation beside another cause. ## Stage 2 — Isolate parsed provider-parameter JSON +**Complete.** Both captured binding replay and aggregate request seed creation +copy nested JSON objects and arrays. The two new boundary regressions failed +against the original implementation; focused request-flag and aggregate tests +passed in the aligned workspace with Go 1.27.0, `GOTOOLCHAIN=local`, +`GOPROXY=off`, and `-mod=readonly`. Nil, empty, integer and decimal values are +covered. + Findings: F02. Sources: ckprompt `request_flags.go` and `smoke_command.go`. 1. Add one private recursive copier for the known CLI JSON object/array diff --git a/integrations/ckprompt/request_flags.go b/integrations/ckprompt/request_flags.go index aaadc13..21b04ff 100644 --- a/integrations/ckprompt/request_flags.go +++ b/integrations/ckprompt/request_flags.go @@ -85,6 +85,7 @@ func (f ExecutionFlags[I]) bindings() []requestBinding[I] { } // NewExecutionFlags binds execution controls to a field of the caller's input. +// Parsed provider parameters are copied when the binding is applied. func NewExecutionFlags[I any](destination func(*I) *ExecutionInput) ExecutionFlags[I] { target := func(input *I) *promptkit.ExecutionTargetOverride { value := destination(input) @@ -104,7 +105,36 @@ func NewExecutionFlags[I any](destination func(*I) *ExecutionInput) ExecutionFla ReasoningEffort: executionFlag(requestFlags.ReasoningEffort.meta, parseDefinitionString, func(i *I, v string) { target(i).ReasoningEffort = &v }), APIKeyEnv: executionFlag(requestFlags.APIKeyEnv.meta, parseDefinitionString, func(i *I, v string) { target(i).APIKeyEnv = v }), TransientRetries: executionFlag(requestFlags.TransientRetries.meta, strconv.Atoi, func(i *I, v int) { destination(i).TransientRetries = &v }), - ExtraParams: executionFlag(requestFlags.ExtraParams.meta, parseExtraParams, func(i *I, v map[string]any) { target(i).ExtraParams = v }), + ExtraParams: executionFlag(requestFlags.ExtraParams.meta, parseExtraParams, func(i *I, v map[string]any) { target(i).ExtraParams = copyProviderParams(v) }), + } +} + +func copyProviderParams(params map[string]any) map[string]any { + if params == nil { + return nil + } + result := make(map[string]any, len(params)) + for key, value := range params { + result[key] = copyProviderValue(value) + } + return result +} + +func copyProviderValue(value any) any { + switch value := value.(type) { + case map[string]any: + return copyProviderParams(value) + case []any: + if value == nil { + return []any(nil) + } + result := make([]any, len(value)) + for i, element := range value { + result[i] = copyProviderValue(element) + } + return result + default: + return value } } diff --git a/integrations/ckprompt/request_flags_test.go b/integrations/ckprompt/request_flags_test.go index 6dc44ec..b87186d 100644 --- a/integrations/ckprompt/request_flags_test.go +++ b/integrations/ckprompt/request_flags_test.go @@ -76,3 +76,33 @@ func TestValidationForwardsExecutionControls(t *testing.T) { t.Fatalf("result/request = %#v %#v", result, received) } } + +func TestExecutionExtraParamsCapturedApplicationsAreIndependent(t *testing.T) { + type target struct{ execution ckprompt.ExecutionInput } + type outer struct{ captured commandkit.FlagValues[target] } + flag := ckprompt.NewExecutionFlags(func(i *target) *ckprompt.ExecutionInput { return &i.execution }).ExtraParams + captured := commandkit.CaptureFlags([]commandkit.Flag[target]{flag}, func(i *outer) *commandkit.FlagValues[target] { return &i.captured }) + command := commandkit.NewCommand(commandkit.CommandSpec[outer]{Name: "run", Flags: captured, Run: func(_ commandkit.Invocation, input outer) commandkit.Outcome { + var first, second target + if err := input.captured.ApplyTo(&first, flag); err != nil { + t.Fatal(err) + } + first.execution.Execution.ExtraParams["nested"].(map[string]any)["array"].([]any)[0].(map[string]any)["number"] = int64(9) + if err := input.captured.ApplySuppliedTo(&second, flag); err != nil { + t.Fatal(err) + } + value := second.execution.Execution.ExtraParams["nested"].(map[string]any)["array"].([]any)[0].(map[string]any)["number"] + if value != int64(1) { + t.Errorf("second value = %#v", value) + } + return commandkit.Success() + }}) + app, err := commandkit.New(commandkit.Identity{Name: "app"}, command) + if err != nil { + t.Fatal(err) + } + result := app.Execute(context.Background(), []string{"run", `--llm-extra-params={"nested":{"array":[{"number":1}]}}`}, commandkit.Streams{Out: io.Discard, Err: io.Discard}) + if result.Kind != commandkit.OutcomeSuccess { + t.Fatal(result) + } +} diff --git a/integrations/ckprompt/smoke_aggregate_test.go b/integrations/ckprompt/smoke_aggregate_test.go index e528a6c..a34f80e 100644 --- a/integrations/ckprompt/smoke_aggregate_test.go +++ b/integrations/ckprompt/smoke_aggregate_test.go @@ -219,3 +219,33 @@ func TestSmokeAggregateCancellationRetainsPartialResults(t *testing.T) { t.Fatalf("partial aggregate entries = %#v", document.Tests) } } + +func TestSmokeAggregateIsolatesProviderParameters(t *testing.T) { + calls := 0 + app := newPromptTestApp(t, Command(Suite[struct{}, struct{}]{ + SmokeTest: func(_ context.Context, request SmokeRequest, _, _ struct{}) (commandkit.Acquired[*promptkit.SmokeTestReport], error) { + calls++ + params := request.Request.Execution.ExtraParams + value := params["nested"].(map[string]any)["array"].([]any)[0].(map[string]any) + if value["number"] != int64(1) { + t.Errorf("call %d: %#v", calls, value) + } + value["number"] = int64(9) + return commandkit.Acquired[*promptkit.SmokeTestReport]{Value: &promptkit.SmokeTestReport{Kind: request.Request.Kind}}, nil + }, + })) + outcome := app.Execute(context.Background(), []string{"promptkit", "test", "all", `--llm-extra-params={"nested":{"array":[{"number":1}]}}`}, commandkit.Streams{Out: io.Discard, Err: io.Discard}) + if outcome.Kind != commandkit.OutcomeSuccess || calls != 4 { + t.Fatalf("outcome=%#v calls=%d", outcome, calls) + } +} + +func TestCopyProviderParamsPreservesEmptyAndNilValues(t *testing.T) { + if copyProviderParams(nil) != nil { + t.Fatal("nil object became non-nil") + } + copied := copyProviderParams(map[string]any{"emptyObject": map[string]any{}, "nilObject": map[string]any(nil), "emptyArray": []any{}, "nilArray": []any(nil), "null": nil, "decimal": float64(1.5)}) + if copied["emptyObject"].(map[string]any) == nil || copied["nilObject"].(map[string]any) != nil || copied["emptyArray"].([]any) == nil || copied["nilArray"].([]any) != nil || copied["null"] != nil || copied["decimal"] != float64(1.5) { + t.Fatalf("copied = %#v", copied) + } +} diff --git a/integrations/ckprompt/smoke_command.go b/integrations/ckprompt/smoke_command.go index 925ee41..d80c46d 100644 --- a/integrations/ckprompt/smoke_command.go +++ b/integrations/ckprompt/smoke_command.go @@ -232,6 +232,7 @@ func smokeRequestForKind(seed promptkit.SmokeTestRequest, kind promptkit.SmokeTe request.Kind = kind if seed.Execution != nil { execution := *seed.Execution + execution.ExtraParams = copyProviderParams(seed.Execution.ExtraParams) execution.Temperature = copyFloat64(seed.Execution.Temperature) execution.MaxTokens = copyInt(seed.Execution.MaxTokens) execution.TopP = copyFloat64(seed.Execution.TopP)