Detach version labels in prepared execution snapshots
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
# Configuration Collections Implementation Review Findings
|
# Configuration Collections Implementation Review Findings
|
||||||
|
|
||||||
Status: Finding 1 completed; Findings 2–3 open. Review completed 2026-10-17.
|
Status: Findings 1–2 completed; Finding 3 open. Review completed 2026-10-17.
|
||||||
|
|
||||||
Reviewed implementation after `89af2a1` through `0221cd4` against the accepted
|
Reviewed implementation after `89af2a1` through `0221cd4` against the accepted
|
||||||
[feature roadmap](configuration-collections.md), [implementation plan](implementation.md),
|
[feature roadmap](configuration-collections.md), [implementation plan](implementation.md),
|
||||||
@@ -89,7 +89,7 @@ and public API tests, configuration vet, and `git diff --check`.
|
|||||||
|
|
||||||
## Finding 2: Detach The Optional Label In Internal Prepared Snapshots
|
## Finding 2: Detach The Optional Label In Internal Prepared Snapshots
|
||||||
|
|
||||||
Priority: P3. Status: open.
|
Priority: P3. Status: completed 2026-10-17.
|
||||||
|
|
||||||
Location: [clonePreparedRun](../../internal/usecase/prepared_execution.go),
|
Location: [clonePreparedRun](../../internal/usecase/prepared_execution.go),
|
||||||
lines 226–244; also its use by `PrepareExecution` and `PreparedExecution.Details`.
|
lines 226–244; also its use by `PrepareExecution` and `PreparedExecution.Details`.
|
||||||
@@ -121,6 +121,14 @@ non-nil version label. Mutation of one returned internal snapshot must not chang
|
|||||||
retained details, the execution snapshot, or a later result. Reuse existing
|
retained details, the execution snapshot, or a later result. Reuse existing
|
||||||
nil/empty public metadata coverage rather than reproducing it at every layer.
|
nil/empty public metadata coverage rather than reproducing it at every layer.
|
||||||
|
|
||||||
|
Completion: the prepared-run copier now detaches the optional version label with
|
||||||
|
the existing nil-aware helper. Extended the owning prepared-execution snapshot
|
||||||
|
test to mutate source and returned labels and verify later details, execution
|
||||||
|
metadata, and post-execution details remain unchanged. The regression failed
|
||||||
|
before the fix and passes afterward. Validation passed with `GOWORK=off`:
|
||||||
|
the focused regression, `go test -race ./...`, `go vet ./...`, all three offline
|
||||||
|
examples, and `git diff --check`.
|
||||||
|
|
||||||
## Finding 3: Document Engine Ownership In Terms Of The Shared Assembly
|
## Finding 3: Document Engine Ownership In Terms Of The Shared Assembly
|
||||||
|
|
||||||
Priority: P3. Status: open.
|
Priority: P3. Status: open.
|
||||||
|
|||||||
@@ -229,6 +229,7 @@ func clonePreparedRun(source *domain.PreparedRun) (*domain.PreparedRun, error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
copied := *source
|
copied := *source
|
||||||
|
copied.PromptVersion = copyStringPointer(source.PromptVersion)
|
||||||
extraParams, err := jsonvalue.CopyMap(source.EffectiveModelParams.ExtraParams)
|
extraParams, err := jsonvalue.CopyMap(source.EffectiveModelParams.ExtraParams)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
|
|||||||
@@ -662,6 +662,7 @@ func TestRunnerPreparedExecutionDetachesTransferredSnapshotSources(t *testing.T)
|
|||||||
"properties": map[string]any{"value": map[string]any{"type": "string"}},
|
"properties": map[string]any{"value": map[string]any{"type": "string"}},
|
||||||
}
|
}
|
||||||
definition := promptDef(domain.FormatJSON, domain.ValidationJSONSchema, 0)
|
definition := promptDef(domain.FormatJSON, domain.ValidationJSONSchema, 0)
|
||||||
|
wantPromptVersion := *definition.Version
|
||||||
definition.Validation.SchemaPath = "schema.json"
|
definition.Validation.SchemaPath = "schema.json"
|
||||||
control := &domain.CacheControl{Type: domain.CacheControlEphemeral, TTL: "1h"}
|
control := &domain.CacheControl{Type: domain.CacheControlEphemeral, TTL: "1h"}
|
||||||
rendered := &domain.RenderedPrompt{Messages: []domain.RenderedMessage{{
|
rendered := &domain.RenderedPrompt{Messages: []domain.RenderedMessage{{
|
||||||
@@ -697,6 +698,7 @@ func TestRunnerPreparedExecutionDetachesTransferredSnapshotSources(t *testing.T)
|
|||||||
}
|
}
|
||||||
mutateExtra(profileExtras, "mutated profile source")
|
mutateExtra(profileExtras, "mutated profile source")
|
||||||
mutateExtra(requestExtras, "mutated request source")
|
mutateExtra(requestExtras, "mutated request source")
|
||||||
|
*definition.Version = "mutated definition label"
|
||||||
rendered.Messages[0].Content = "mutated renderer source"
|
rendered.Messages[0].Content = "mutated renderer source"
|
||||||
control.TTL = "mutated renderer cache metadata"
|
control.TTL = "mutated renderer cache metadata"
|
||||||
schemaDocument["properties"].(map[string]any)["value"].(map[string]any)["type"] = "integer"
|
schemaDocument["properties"].(map[string]any)["value"].(map[string]any)["type"] = "integer"
|
||||||
@@ -711,11 +713,21 @@ func TestRunnerPreparedExecutionDetachesTransferredSnapshotSources(t *testing.T)
|
|||||||
details.StructuredOutput.JSONSchema.Schema.(map[string]any)["properties"].(map[string]any)["value"].(map[string]any)["type"] != "string" {
|
details.StructuredOutput.JSONSchema.Schema.(map[string]any)["properties"].(map[string]any)["value"].(map[string]any)["type"] != "string" {
|
||||||
t.Fatalf("prepared details retained a source alias: %#v", details)
|
t.Fatalf("prepared details retained a source alias: %#v", details)
|
||||||
}
|
}
|
||||||
|
if details.PromptVersion == nil || *details.PromptVersion != wantPromptVersion {
|
||||||
|
t.Fatalf("prepared details did not preserve the captured version: %+v", details)
|
||||||
|
}
|
||||||
|
*details.PromptVersion = "mutated details label"
|
||||||
|
if again := prepared.Details(); again == nil || again.PromptVersion == nil || *again.PromptVersion != wantPromptVersion {
|
||||||
|
t.Fatalf("details version mutation escaped into the retained snapshot: %+v", again)
|
||||||
|
}
|
||||||
|
|
||||||
result, err := runner.RunPrepared(context.Background(), prepared)
|
result, err := runner.RunPrepared(context.Background(), prepared)
|
||||||
if err != nil || result == nil || !result.Validation.IsValid || result.InputHashes["transcript"] != "captured-input-hash" {
|
if err != nil || result == nil || !result.Validation.IsValid || result.InputHashes["transcript"] != "captured-input-hash" {
|
||||||
t.Fatalf("run prepared snapshot=(%+v, %v)", result, err)
|
t.Fatalf("run prepared snapshot=(%+v, %v)", result, err)
|
||||||
}
|
}
|
||||||
|
if result.PromptVersion == nil || *result.PromptVersion != wantPromptVersion {
|
||||||
|
t.Fatalf("details version mutation escaped into execution metadata: %+v", result)
|
||||||
|
}
|
||||||
if len(llmClient.lastReq.Prompt.Messages) != 1 || llmClient.lastReq.StructuredOutput == nil ||
|
if len(llmClient.lastReq.Prompt.Messages) != 1 || llmClient.lastReq.StructuredOutput == nil ||
|
||||||
llmClient.lastReq.StructuredOutput.JSONSchema == nil ||
|
llmClient.lastReq.StructuredOutput.JSONSchema == nil ||
|
||||||
extraLabel(llmClient.lastReq.Target.ExtraParams) != wantLabel ||
|
extraLabel(llmClient.lastReq.Target.ExtraParams) != wantLabel ||
|
||||||
@@ -725,6 +737,7 @@ func TestRunnerPreparedExecutionDetachesTransferredSnapshotSources(t *testing.T)
|
|||||||
t.Fatalf("execution snapshot retained a source alias: %#v", llmClient.lastReq)
|
t.Fatalf("execution snapshot retained a source alias: %#v", llmClient.lastReq)
|
||||||
}
|
}
|
||||||
if completedDetails := prepared.Details(); completedDetails == nil ||
|
if completedDetails := prepared.Details(); completedDetails == nil ||
|
||||||
|
completedDetails.PromptVersion == nil || *completedDetails.PromptVersion != wantPromptVersion ||
|
||||||
len(completedDetails.Messages) != 1 || completedDetails.Messages[0].Content != "captured rendering" ||
|
len(completedDetails.Messages) != 1 || completedDetails.Messages[0].Content != "captured rendering" ||
|
||||||
extraLabel(completedDetails.EffectiveModelParams.ExtraParams) != wantLabel {
|
extraLabel(completedDetails.EffectiveModelParams.ExtraParams) != wantLabel {
|
||||||
t.Fatalf("completed details changed after source mutation: %#v", completedDetails)
|
t.Fatalf("completed details changed after source mutation: %#v", completedDetails)
|
||||||
|
|||||||
Reference in New Issue
Block a user