From 1d29b321dd17e115deb4d7695132dba532455678 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 25 Sep 2026 23:59:43 +0000 Subject: [PATCH] Record remaining kit upgrade verification work --- docs/roadmap/remediation.md | 102 ++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 00000000..5a641a7c --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,102 @@ +# Kit Upgrade Review Remediation + +## Review Scope + +Reviewed implementation commits `31b48d2` through `7ac9d94` against +[the feature roadmap](upgrade-kit.md) and [the implementation plan](implementation.md). +The review covered configuration selection/defaults, production client wiring, +error translation, execution observation, scheduler/debug propagation, current +documentation, and tests. No production model calls were made. + +The implemented architecture is consistent with the intended design: ConfigKit +v0.4.0 owns composition and overlay provenance behind the configuration boundary; +PromptKit v0.11.0 owns transient retries and structural repairs behind the LLM +adapter. Notarius retains stage retry/disposition policy, completion-scoped +scheduling, application-owned error/observation types, and opt-in debug output. +No new runtime defect or unrequested restriction was established in this review. + +## Finding 1 — Complete The Missing Retry And Observation Verification + +**Priority:** Medium. This is a verification gap, not a demonstrated runtime +failure. Stages 5–7 are marked complete, but their consequential cross-boundary +acceptance tests were not implemented. + +### Evidence And Risk + +- `internal/framework/llm/promptkit_client_test.go:58` adds only a successful, + single-attempt observation assertion with positive prompt-token usage. It + does not exercise absent versus partial/known-zero usage, failed/canceled + execution summaries, preparation failure, or concurrent request isolation. +- `TestPromptKitClientAppliesConfiguredBuiltInTransientRetries` covers basic + status recovery, explicit zero, exhaustion, and a noneligible status. It + discards the response, so does not protect terminal accounting. There is no + combined transient-retry/structural-repair scenario or cancellation while + waiting on Retry-After. +- `internal/framework/llm/scheduled_client.go:40` now preserves a response when + the inner call fails. No corresponding test protects that behavior. Returning + an empty response again would silently remove precisely the failed-call + observations this feature is intended to retain. +- `internal/framework/pipeline/debug.go:267` and `:277` add execution summaries + to response artifacts and call references, plus fallback/content-splitting + propagation. Neither the debug nor terminal-debug tests were extended to + exercise these fields. Passing existing tests does not prove that failed-call + metadata reaches persisted records. + +These are Notarius-owned integration risks, not a request to re-test PromptKit's +backoff algorithm. Losing failed-call observations or misrepresenting incomplete +usage would be difficult to detect in ordinary successful runs. A focused set +of behavioral tests is justified under the testing policy. + +### Required Remediation + +Use existing test assets, local HTTP fixtures, scheduler wrappers, and debug +recorders. Combine related assertions; no fixed test count or new test framework +is required. + +1. Add an adapter scenario with a transient failure, a structurally invalid + completion, and a successful repair. Exercise retries in both generations + with small explicit budgets, and assert terminal attempts/retries/repair + counts without double-counting. Use controlled usage fields to distinguish + known zero, unknown, and partial aggregate usage. Preserve legacy usage + semantics independently. +2. Cover failed and canceled completions, including cancellation during a + Retry-After wait. Verify caller cancellation identity, bounded completion, + and retained execution metadata. Use signaling and a generous bounded test + timeout, not exact sleep durations or jitter assertions. Cover a preparation + failure with no execution summary. Add a small concurrent-call check for + request-local summary isolation, which race tests alone do not prove. +3. Exercise an error response through the real scheduled client and existing + debug wrapper/recorder seams. Assert that the summary reaches response + artifacts and terminal call references even without a provider response + body. Cover the content-splitting/fallback path with a representative response. + Reuse existing no-debug and redaction tests rather than duplicating their + whole inventories. Keep new summary assertions metadata-only. + +Retain existing scheduler policy, stage/validator budgets, cancellation +disposition, warning semantics, and debug opt-in behavior. Do not introduce a +total deadline, new retry caps, telemetry artifacts, or live API requirements +to make these tests easier. Fix production code only if these checks reveal a +violation of the already selected behavior. + +**Acceptance:** The added tests fail if failed responses lose their execution +summary at an adapter/wrapper boundary, usage presence is collapsed into zero, +or retry/repair totals are double-counted. They pass with the intended behavior +and provide evidence for the previously unverified stage acceptance criteria. + +**Validation:** Run focused LLM/pipeline tests, race-enabled tests for those +packages, and `GOWORK=off go test ./...`. No production credentials are needed. + +## Checks Completed During Review + +- `GOWORK=off go test ./...` +- `GOWORK=off go vet ./...` +- `GOWORK=off go build ./...` +- `GOWORK=off go mod tidy -diff` +- `GOWORK=off go test -race ./internal/framework/llm ./internal/framework/pipeline` +- CLI configuration validation for the minimal and complete examples, including + the complete example with `--config-overlay development`. +- `git diff --check` + +All passed. Existing green tests do not substitute for the missing behavioral +coverage above. Temporary diagnostic test material used during review was +removed; no application code was changed.