Preserve operation ownership in terminal observations
This commit is contained in:
@@ -10,6 +10,10 @@ lifecycle instrumentation spans the facade and runner boundaries.
|
||||
Prepared execution starts only after its private payload is atomically claimed;
|
||||
the facade receives that invocation's recorder privately and emits the terminal
|
||||
event after public error mapping. Failed claims and preparation emit nothing.
|
||||
The public projection retains private operation-owner metadata separately from
|
||||
inspectable child causes. Terminal classification uses that owner, preserving
|
||||
context precedence and generation-specific transport/provider details rather
|
||||
than mistaking a collaborator's nested lookup error for the parent's operation.
|
||||
|
||||
The facade masks inherited recorder and prepared-claim state when entering
|
||||
preparation or execution. Delegated calls from an injected collaborator retain
|
||||
|
||||
@@ -27,7 +27,7 @@ listing, and evidence-union improvements. No selected workstream was deferred.
|
||||
The designs below preserve the selected target and rationale. Historical planning
|
||||
evidence later in this document describes the original baseline, not current
|
||||
replacement measurements. Follow-up corrections are tracked in the remediation
|
||||
record; finding 5 remains outstanding at this status update.
|
||||
record; all five follow-up findings are now addressed.
|
||||
Private helper names are descriptive unless identifying an existing declaration.
|
||||
Catalog numbers are traceability identifiers, not a required implementation
|
||||
order or 34 separate delivery stages. The scope
|
||||
|
||||
@@ -9,13 +9,13 @@ The implementation changes begin after planning commit `6a1a32a`.
|
||||
|
||||
An integration-focused follow-up reviewed the same implementation at `356563a`,
|
||||
the documentation-only commit recording the original review. It adds finding 5;
|
||||
the earlier findings remain open and are not duplicated.
|
||||
the earlier findings were still open at that audit baseline and are not duplicated.
|
||||
|
||||
The broad architecture and selected simplifications are in place. Five findings
|
||||
remain open below: three correctness/completeness issues, one error-projection
|
||||
efficiency issue, and one documentation-status gap. These reviews implement none
|
||||
of their remedies. The existing race suite passing does not cover the reproduced
|
||||
edge cases.
|
||||
The broad architecture and selected simplifications are in place. All five
|
||||
findings are now addressed, with completion markers below: three correctness/
|
||||
completeness issues, one error-projection efficiency issue, and one documentation
|
||||
status gap. Finding descriptions and audit evidence retain the historical
|
||||
pre-fix behavior; the completion markers describe the delivered corrections.
|
||||
|
||||
The accepted artifact ownership, supported convenience-source types, and
|
||||
self-contained schema changes are intentional compatibility changes. No
|
||||
@@ -211,7 +211,12 @@ retirement can follow when appropriate.
|
||||
|
||||
### 5. Integrate Operation Ownership With Terminal Observation Classification [P2]
|
||||
|
||||
**Status:** Open. **Related stages:** 3–4, 25–26.
|
||||
**Status:** Complete. **Related stages:** 3–4, 25–26.
|
||||
|
||||
Completion: a private public-boundary wrapper retains operation ownership and a
|
||||
single mapped cause. Terminal observations classify that owner while retaining
|
||||
context precedence and generation-specific transport/provider details. Observed
|
||||
run, prepared-run, repair, and artifact regressions retain child cause identity.
|
||||
|
||||
**Locations:** [terminal classifier](../../observations.go), `observedFinish`,
|
||||
lines 160–192; [public error projection](../../errors.go), `mapPublicError`;
|
||||
@@ -365,7 +370,16 @@ efficiency, and scalability of the implemented stages. It confirms the
|
||||
exponential behavior in finding 3 and adds the recurrence and refactoring
|
||||
opportunity there, rather than creating a duplicate finding. No additional
|
||||
distinct algorithmic or ordinary-correctness finding was identified in this
|
||||
pass. All five existing findings remain open.
|
||||
pass. All five findings were open at that audit baseline; their completion
|
||||
markers above own the current remediation status.
|
||||
|
||||
Remediation completion validation: Go 1.27.3 with `GOTOOLCHAIN=local`,
|
||||
`GOWORK=off`, and `GOPROXY=off` passed the complete
|
||||
`go test -race -count=1 ./...` suite and `go vet ./...`. New regressions cover
|
||||
authentication read interruption at the overflow boundary, host/FS schema
|
||||
portability, relocated native schema submission, nested-cause duplication, and
|
||||
operation-owned terminal observations. Focused checks passed before each code
|
||||
unit was committed. Documentation links, formatting, and whitespace were checked.
|
||||
|
||||
The review used the plan and roadmap to exclude deliberate algorithm choices:
|
||||
the small-to-large evidence merge, owned provider overlay, bounded feedback
|
||||
|
||||
14
errors.go
14
errors.go
@@ -13,6 +13,18 @@ import (
|
||||
"go.maximumdirect.net/promptkit/internal/usecase"
|
||||
)
|
||||
|
||||
// publicOperationError keeps classification separate from inspectable causes.
|
||||
// Cache wording so formatting nested operations does not repeatedly rebuild it.
|
||||
type publicOperationError struct {
|
||||
owner error
|
||||
cause error
|
||||
message string
|
||||
}
|
||||
|
||||
func (e *publicOperationError) Error() string { return e.message }
|
||||
func (e *publicOperationError) Unwrap() error { return e.cause }
|
||||
func (e *publicOperationError) Is(target error) bool { return errors.Is(e.owner, target) }
|
||||
|
||||
func mapPublicError(err error) error {
|
||||
if err == nil {
|
||||
return nil
|
||||
@@ -32,7 +44,7 @@ func mapPublicError(err error) error {
|
||||
}
|
||||
// The projection already retains ordinary causes; adding the original
|
||||
// again would duplicate its entire subtree at each enclosing operation.
|
||||
return errors.Join(owner, mapped)
|
||||
return &publicOperationError{owner: owner, cause: mapped, message: fmt.Sprintf("%v: %v", owner, mapped)}
|
||||
}
|
||||
}
|
||||
return mapUnownedPublicError(err)
|
||||
|
||||
@@ -49,3 +49,51 @@ func TestNestedEngineErrorKeepsOwningOperation(t *testing.T) {
|
||||
t.Fatalf("artifact error = %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestObservedNestedErrorsKeepTheParentOperation(t *testing.T) {
|
||||
child := newSimpleEngine(t, "child")
|
||||
_, childErr := child.Run(context.Background(), promptkit.RunRequest{PromptID: "missing"})
|
||||
for _, mode := range []string{"run", "prepared", "repair", "artifact"} {
|
||||
t.Run(mode, func(t *testing.T) {
|
||||
var events []promptkit.ExecutionEvent
|
||||
req := promptkit.RunRequest{PromptID: "prompt", Observer: func(e promptkit.ExecutionEvent) { events = append(events, e) }}
|
||||
engine := newSimpleEngine(t, "parent", promptkit.WithLLMClient(&fakeLLMClient{err: childErr}))
|
||||
want, owner := promptkit.ExecutionFailureGeneration, promptkit.ErrLLMGenerate
|
||||
if mode == "artifact" {
|
||||
engine = newSimpleEngine(t, "parent", promptkit.WithArtifactReader(nestedErrorReader{cause: childErr}))
|
||||
req.Inputs = map[string]promptkit.ArtifactRef{"extra": promptkit.Inline("data")}
|
||||
want, owner = promptkit.ExecutionFailureArtifactLoad, promptkit.ErrArtifactLoad
|
||||
}
|
||||
if mode == "repair" {
|
||||
calls := 0
|
||||
engine = newSimpleEngine(t, "parent", promptkit.WithLLMClient(observationClientFunc(func(context.Context, promptkit.GenerateRequest) (*promptkit.GenerateResponse, error) {
|
||||
calls++
|
||||
if calls == 1 {
|
||||
return &promptkit.GenerateResponse{Content: "invalid JSON"}, nil
|
||||
}
|
||||
return nil, childErr
|
||||
})))
|
||||
req.Validation = &promptkit.OutputContract{Format: promptkit.FormatJSON, ValidationMode: promptkit.ValidationJSON, RepairAttempts: 1}
|
||||
}
|
||||
var err error
|
||||
if mode == "prepared" {
|
||||
handle, prepErr := engine.PrepareExecution(context.Background(), req)
|
||||
if prepErr != nil {
|
||||
t.Fatal(prepErr)
|
||||
}
|
||||
_, err = engine.RunPrepared(context.Background(), handle)
|
||||
} else {
|
||||
_, err = engine.Run(context.Background(), req)
|
||||
}
|
||||
if !errors.Is(err, owner) || !errors.Is(err, childErr) {
|
||||
t.Fatalf("lost owner/cause: %v", err)
|
||||
}
|
||||
if len(events) == 0 || events[len(events)-1].Failure != want {
|
||||
t.Fatalf("terminal classification: %+v", events)
|
||||
}
|
||||
if mode != "artifact" && events[len(events)-2].Failure != want {
|
||||
t.Fatalf("attempt classification: %+v", events)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -151,6 +151,12 @@ func observedFinish(result *RunResult, err error) domain.ExecutionEvent {
|
||||
return domain.ExecutionEvent{Outcome: outcome}
|
||||
}
|
||||
value := domain.ExecutionEvent{Outcome: domain.ExecutionOutcomeError}
|
||||
category := err
|
||||
var operation *publicOperationError
|
||||
if errors.As(err, &operation) {
|
||||
category = operation.owner
|
||||
}
|
||||
generationDetails := operation == nil || errors.Is(category, ErrLLMGenerate)
|
||||
var exhausted *RetryExhaustedError
|
||||
value.RetriesExhausted = errors.As(err, &exhausted)
|
||||
var provider *GenerationError
|
||||
@@ -162,33 +168,33 @@ func observedFinish(result *RunResult, err error) domain.ExecutionEvent {
|
||||
value.Failure = domain.ExecutionFailureCanceled
|
||||
case errors.Is(err, context.DeadlineExceeded):
|
||||
value.Failure = domain.ExecutionFailureDeadline
|
||||
case errors.Is(err, ErrTransport):
|
||||
case generationDetails && errors.Is(err, ErrTransport):
|
||||
value.Failure = domain.ExecutionFailureTransport
|
||||
case value.StatusCode != 0:
|
||||
case generationDetails && value.StatusCode != 0:
|
||||
value.Failure = domain.ExecutionFailureProviderStatus
|
||||
case errors.Is(err, ErrMalformedResponse):
|
||||
case generationDetails && errors.Is(err, ErrMalformedResponse):
|
||||
value.Failure = domain.ExecutionFailureMalformedResponse
|
||||
case errors.Is(err, ErrCapacityExceeded):
|
||||
case errors.Is(category, ErrCapacityExceeded):
|
||||
value.Failure = domain.ExecutionFailureCapacity
|
||||
case errors.Is(err, ErrInvalidConfig):
|
||||
case errors.Is(category, ErrInvalidConfig):
|
||||
value.Failure = domain.ExecutionFailureInvalidConfig
|
||||
case errors.Is(err, ErrPromptNotFound):
|
||||
case errors.Is(category, ErrPromptNotFound):
|
||||
value.Failure = domain.ExecutionFailurePromptNotFound
|
||||
case errors.Is(err, ErrProfileNotFound):
|
||||
case errors.Is(category, ErrProfileNotFound):
|
||||
value.Failure = domain.ExecutionFailureProfileNotFound
|
||||
case errors.Is(err, ErrPromptLoad):
|
||||
case errors.Is(category, ErrPromptLoad):
|
||||
value.Failure = domain.ExecutionFailurePromptLoad
|
||||
case errors.Is(err, ErrProfileLoad):
|
||||
case errors.Is(category, ErrProfileLoad):
|
||||
value.Failure = domain.ExecutionFailureProfileLoad
|
||||
case errors.Is(err, ErrInvalidRequest):
|
||||
case errors.Is(category, ErrInvalidRequest):
|
||||
value.Failure = domain.ExecutionFailureInvalidRequest
|
||||
case errors.Is(err, ErrArtifactLoad):
|
||||
case errors.Is(category, ErrArtifactLoad):
|
||||
value.Failure = domain.ExecutionFailureArtifactLoad
|
||||
case errors.Is(err, ErrPromptRender):
|
||||
case errors.Is(category, ErrPromptRender):
|
||||
value.Failure = domain.ExecutionFailurePromptRender
|
||||
case errors.Is(err, ErrValidation):
|
||||
case errors.Is(category, ErrValidation):
|
||||
value.Failure = domain.ExecutionFailureValidation
|
||||
case errors.Is(err, ErrLLMGenerate):
|
||||
case errors.Is(category, ErrLLMGenerate):
|
||||
value.Failure = domain.ExecutionFailureGeneration
|
||||
default:
|
||||
value.Failure = domain.ExecutionFailureOther
|
||||
|
||||
Reference in New Issue
Block a user