diff --git a/docs/internal/observation.md b/docs/internal/observation.md index 072d197..9256625 100644 --- a/docs/internal/observation.md +++ b/docs/internal/observation.md @@ -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 diff --git a/docs/roadmap/audit-remediation.md b/docs/roadmap/audit-remediation.md index 8c49bf8..315773c 100644 --- a/docs/roadmap/audit-remediation.md +++ b/docs/roadmap/audit-remediation.md @@ -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 diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index d44b718..95a9cd5 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -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 diff --git a/errors.go b/errors.go index 99974bd..34b8a1e 100644 --- a/errors.go +++ b/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) diff --git a/nested_error_contract_test.go b/nested_error_contract_test.go index 687d97b..8233886 100644 --- a/nested_error_contract_test.go +++ b/nested_error_contract_test.go @@ -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) + } + }) + } +} diff --git a/observations.go b/observations.go index 8f5ace1..a14d604 100644 --- a/observations.go +++ b/observations.go @@ -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