diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..4e6e9ca --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,134 @@ +# Smoke Test Implementation Review Findings + +Status: three open findings. Review completed 2026-10-13. + +Reviewed implementation commits after `8fbd058` through +`2f735aedf9b5dcc98164292db46ae1eab3e15cde`, against the +[feature roadmap](smoke-tests.md), [implementation plan](implementation.md), +public GoDoc, and applicable repository policies. No implementation changes +were made during this review. + +The facade/internal boundary, final-value configuration ownership, catalog-owned +diagnostic metadata, shared generation/capacity machinery, private inference +cases, and neutral factual reporting are in place. No additional strengthened +contracts or prohibitions requiring remediation were identified. The findings +below are reporting-correctness gaps, not reasons to redesign the feature. + +## Finding 1: Preserve Caller Cancellation Independently Of Body Error Type + +Priority: P2. Status: open. + +Locations: [`finishHTTP`](../../internal/usecase/smoke_tester.go), lines 355–366, +and [`runOpenRouterKey`](../../internal/usecase/smoke_authentication.go), +lines 107–110. + +`finishHTTP` returns a caller-context error only when `report.Failure` has already +been classified as canceled/deadline. Authentication body reads can fail with an +ordinary I/O error while the caller context is canceled. Their failure is then +classified as transport, so the operation incorrectly returns `Completed=true` +and nil Go error despite interruption before authentication recognition finished. + +An offline public-API reproduction supplies an HTTP 200 response whose body +`Read` cancels the caller context and returns `io.ErrUnexpectedEOF`. The current +result is `Completed=true`, `Failure="transport"`, nil error, and an already +canceled caller context. This conflicts with the accepted partial-report/error +boundary; callers cannot recognize the cancellation with `errors.Is`. + +Required outcome: recognize caller interruption during unfinished HTTP probe +work independently of the underlying I/O error's identity. Return the retained +report with `Completed=false`, the corresponding canceled/deadline failure, and +an error preserving `ctx.Err()` identity. Retain response/status/destination facts +already observed, leaving acceptance unknown when recognition did not complete. +Keep configured request-timeout failures with an active caller as concluded +reports, and do not revoke a diagnostic already completed before later cancellation. + +Regression coverage: extend the owning use-case cancellation test with a body +that cancels the caller and returns a non-context I/O error. Its existing fake +returns `context.Canceled`, which masks this defect. Check partial-report and +context identity rather than exact error text; reuse existing timeout coverage. + +## Finding 2: Report The Actual Initial Inference Request Destination + +Priority: P2. Status: open. + +Locations: [`recordGenerationEvidence`](../../internal/usecase/smoke_tester.go), +lines 240–248, and +[`resolveSmokeTestEndpoint`](../../internal/usecase/smoke_selection.go), +lines 171–188. + +Inference selection leaves the diagnostic endpoint at the profile's base URL, +and every attempt copies that value into `InitialEndpoint`. The built-in client +actually dispatches to that base plus `/chat/completions`. Thus a request with +no redirect reports different initial and final destinations even though no +redirect occurred. `InitialEndpoint` is documented as the destination before +redirects, not the base used to construct the request. + +An offline public-API reproduction observes an actual request to +`https://openrouter.ai/api/v1/chat/completions`, while the attempt reports +`InitialEndpoint="https://openrouter.ai/api/v1"` and +`Endpoint="https://openrouter.ai/api/v1/chat/completions"`. `Target.Endpoint` also +reports the base as its initially resolved diagnostic destination. This makes +destination attribution misleading to consumers. + +Required outcome: retain the actual pre-redirect request URL through the narrow +internal evidence path, using the existing prepared-request URL composition +rather than duplicating inference serialization/routing. Populate initial/final +endpoint fields from their appropriate sources, and align `Target.Endpoint` +with its stated meaning. Preserve the configured base in +`Target.EffectiveSettings.Endpoint`. Continue excluding URL user information; +do not infer an injected client's actual HTTP destinations from configuration. + +Regression coverage: one built-in inference request without redirects must +report its observed URL as both initial and final. Add a focused redirect case +that distinguishes the actual completion URL from the last destination. Reuse +existing HTTP redirect tests for credential/response attribution mechanics and +keep injected-client HTTP destination evidence unknown. + +## Finding 3: Initialize Zero-Dispatch Usage Consistently + +Priority: P3. Status: open. + +Location: [`SmokeTester.Run`](../../internal/usecase/smoke_tester.go), +lines 52–87, and its early return paths. + +Reports begin with a zero-valued `ObservedTokenUsage`. Unsupported/inapplicable +operations and cancellation before generation return it without applying the +shared zero-attempt accounting. The public +[`ObservedTokenCount` contract](../../observations.go) defines aggregate zero +as known and complete when nothing was dispatched. Missing-required-credential +and capacity-exhaustion returns already use the correct accumulator result, +making the behavior inconsistent across otherwise equivalent no-dispatch paths. + +Offline reproductions of inapplicable RakestrawHome authentication and inference +with an already-canceled context both produce no attempts but return +`TotalTokens={Value:0, Known:false, Complete:false}`. The same applies to the +other token fields. There is no unobserved generation to justify unknown usage. + +Required outcome: use the existing zero-attempt usage accumulator consistently +when no attempt was dispatched, including early cancellation and unsupported or +inapplicable outcomes. Preserve request facts as absent. Do not mark usage known +for an actual dispatch whose token fields were not reported or overwrite usage +retained from earlier dispatched attempts. + +Regression coverage: focused orchestration assertions for early canceled +inference and an inapplicable/unsupported operation, reusing accumulator coverage +for arithmetic. Existing coordinator tests already cover correct zero-dispatch +accounting after the generation boundary is entered. + +## Review Validation + +Passed with `GOWORK=off`: + +- `go test -race ./...`; +- `go vet ./...`; +- `python3 scripts/check-examples.py`. + +The three findings were reproduced through the public API using a temporary +external Go program and a controlled HTTP RoundTripper; no live provider was +contacted and no reproduction source was added to the repository. The passing +suite does not currently cover these cases. + +Both catalogs remain selected at locally validated `v3.20261013.0` candidates. +Their publication and any publication-date re-pinning remain separate release +preparation, as deliberately agreed, rather than an implementation defect or a +new review gate.