Record smoke test review findings

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.