5.6 KiB
Kit Upgrade Review Remediation
Review Scope
Reviewed implementation commits 31b48d2 through 7ac9d94 against
the feature roadmap and the implementation plan.
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:58adds 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.TestPromptKitClientAppliesConfiguredBuiltInTransientRetriescovers 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:40now 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:267and:277add 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.
- 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.
- 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.
- 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 -diffGOWORK=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.