229 lines
13 KiB
Markdown
229 lines
13 KiB
Markdown
# Smoke Test Implementation Review Findings
|
||
|
||
Status: All four findings 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
|
||
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`](../../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.
|
||
|
||
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`](../../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.
|
||
|
||
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`](../../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.
|
||
|
||
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`](../../smoke_diagnostics.go),
|
||
lines 296–336; [`CloneSmokeTestReport`](../../internal/domain/smoke_diagnostics.go),
|
||
lines 180–221; and [`fromDomainExecutionTarget`](../../convert.go).
|
||
|
||
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.
|