124 lines
6.7 KiB
Markdown
124 lines
6.7 KiB
Markdown
# Kit Upgrade Review Remediation
|
||
|
||
## Resolution
|
||
|
||
Completed 2026-09-26. No production changes were needed.
|
||
|
||
- `internal/framework/llm/promptkit_observation_test.go` now covers retries in
|
||
both initial and repair generations, aggregate versus legacy accounting,
|
||
unknown/known-zero/partial usage, retry-wait cancellation, exhausted execution,
|
||
preparation failure, and overlapping request isolation.
|
||
- `internal/framework/pipeline/debug_execution_test.go` exercises the real
|
||
scheduled client and debug wrapper together. It verifies returned and persisted
|
||
summaries for provider errors and cancellation without a body, split debug and
|
||
fallback content, terminal references, and explicit usage-presence JSON fields.
|
||
- Temporary regression probes confirmed that discarding failed responses in the
|
||
scheduler and collapsing usage presence into known zero cause the new tests
|
||
to fail. Those probes were reverted.
|
||
- Focused LLM/pipeline tests, their race-enabled suites, and
|
||
`GOWORK=off go test ./...` passed. Existing no-debug and redaction coverage was
|
||
retained. Runtime budgets, scheduler policy, and output contracts are unchanged.
|
||
|
||
The original finding below is retained as review context.
|
||
|
||
## 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.
|