Smoke Test Implementation Review Findings

Status: All four findings completed 2026-10-13.

Reviewed implementation commits after 8fbd058 through 2f735aedf9b5dcc98164292db46ae1eab3e15cde, against the feature roadmap, implementation plan, 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 from the initial review are reporting-correctness gaps, not reasons to redesign the feature. A follow-up duplication review is recorded as Finding 4.

Finding 1: Preserve Caller Cancellation Independently Of Body Error Type

Priority: P2. Status: completed 2026-10-13.

Locations: finishHTTP, lines 355–366, and runOpenRouterKey, 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.

Completion: HTTP probe failures now check the caller context at the failed-I/O boundary before classification, preserving partial response evidence even for opaque transport/body errors. Completed authentication is not revoked when body closure subsequently cancels the caller. The new regressions failed before the fix; GOWORK=off go test -race ./internal/usecase ., focused use-case vet, and git diff --check passed. Existing configured-timeout coverage remains green.

Finding 2: Report The Actual Initial Inference Request Destination

Priority: P2. Status: completed 2026-10-13.

Locations: recordGenerationEvidence, lines 240–248, and resolveSmokeTestEndpoint, 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.

Completion: built-in generation now passes the frozen prepared request URL through internal generation evidence. Diagnostic target/initial-attempt URLs use that value, while the configured base stays in effective settings and injected inference leaves HTTP destinations unknown. Public GoDoc and the internal generation document describe the corrected behavior. No-redirect, redirect, and injected regressions failed before the fix; race-enabled tests passed for the facade, model client, generation, use case, and engine packages.

Finding 3: Initialize Zero-Dispatch Usage Consistently

Priority: P3. Status: completed 2026-10-13.

Location: SmokeTester.Run, 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 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.

Completion: report finalization now applies the shared zero-attempt accumulator whenever no attempt was dispatched, replacing scattered early-return assignments. Unsupported/inapplicable operations and early cancellation return known, complete zero usage; actual dispatches retain their reported or unknown usage unchanged. The strengthened orchestration assertions failed before the fix. Final validation passed with GOWORK=off: go test -race ./..., go vet ./..., the three offline consumer examples, and git diff --check.

Finding 4: Consolidate Smoke Report Ownership Copying

Priority: P3. Status: completed 2026-10-13.

Follow-up review covers the implementation and remediation commits through 5ea6f06a6d0be32ecb68bc98513f02f29be1cae3.

Locations: fromDomainSmokeTestReport, lines 296–336; CloneSmokeTestReport, lines 180–221; and fromDomainExecutionTarget.

The facade first deep-clones the entire internal report through domain.CloneSmokeTestReport. Its conversion then copies the same mutable data again: applicability/fact pointers, timing pointers, HTTP status pointers, nested provider maps, and the attempt slice. The domain clone already walks every attempt and copies its facts/timings; the facade performs another complete walk and repeats those ownership copies while projecting the public types. The intermediate cloned report is local to that conversion and is not independently returned or retained.

This is a duplicate full-report ownership path, not merely the necessary mapping between public and internal declarations. It adds avoidable intermediate copying and allocation across the whole report, including nested provider data and every attempt. No functional corruption was reproduced, and this review does not claim a measured latency improvement; the duplicate operations are evident directly from the current call path.

Recommended outcome: establish one copying/materialization boundary for the public report, preferably copying mutable values while building the public representation instead of first cloning an entire intermediate report. Reuse appropriate existing ownership helpers and preserve copy-error handling and credential exclusion. An alternative is to project an already-owned clone without cloning its members again, where that is simpler. A focused conversion cleanup is sufficient; public/internal types remain distinct and a new generic cloning framework is unnecessary. Keep domain cloning where it serves a genuinely separate ownership boundary.

Validation: retain the existing public report-detachment coverage and applicable domain clone coverage wherever that cloner remains in use. Extend the narrow conversion/ownership coverage only where needed to protect nested provider maps, optional facts/timings, and attempts from unintended sharing with retained data or other results. Do not make exact allocation counts or helper call order new contracts.

The review found no other substantially duplicated execution subsystem warranting consolidation. Ordinary and diagnostic generation delegate to the same private coordinator loop; request construction, credential resolution, profile overrides, capacity management, failure categories, usage arithmetic, and structural validation reuse existing owners. Diagnostic selection, private case assembly, provider-specific authentication recognition, and partial-report orchestration serve different contracts from ordinary application preparation. Small pointer helpers and DTO projections alone do not justify additional remediation entries.

Focused ownership tests passed on this revision: GOWORK=off go test . ./internal/domain -run 'TestSmokeTestConcludedInferenceMismatchHasNilGoErrorAndDetachedValues|TestCloneSmokeTestReportOwnsMutableValues' -count=1. No implementation was changed during the duplication review.

Completion: the facade now copies mutable values directly into the public report, without first cloning an intermediate domain report. Provider parameters retain the existing JSON-value copy/error handling and pass through the shared target projection without another copy; credentials remain excluded. Domain cloning continues to serve its separate internal ownership boundary. Focused conversion coverage protects retained source values, independently converted results, nested maps/slices, optional facts/timings, attempts, credential exclusion, and copy errors. Existing public and domain ownership tests remain unchanged. Validation passed with GOWORK=off: focused ownership tests, go test -race ./..., go vet ./..., the three offline consumer examples, and git diff --check.

Initial 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 at the reviewed revision did not cover these cases; the completed remediations add the corresponding regression coverage.

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.

13 KiB Raw Blame History Unescape Escape