Record configuration collections review findings
This commit is contained in:
@@ -1,228 +1,164 @@
|
|||||||
# Smoke Test Implementation Review Findings
|
# Configuration Collections Implementation Review Findings
|
||||||
|
|
||||||
Status: All four findings completed 2026-10-13.
|
Status: three findings open. Review completed 2026-10-17.
|
||||||
|
|
||||||
Reviewed implementation commits after `8fbd058` through
|
Reviewed implementation after `89af2a1` through `0221cd4` against the accepted
|
||||||
`2f735aedf9b5dcc98164292db46ae1eab3e15cde`, against the
|
[feature roadmap](configuration-collections.md), [implementation plan](implementation.md),
|
||||||
[feature roadmap](smoke-tests.md), [implementation plan](implementation.md),
|
[configuration-boundary ADR](../adr/0001-configuration-boundary.md), current
|
||||||
public GoDoc, and applicable repository policies. No implementation changes
|
GoDoc, and applicable repository policies. This replaces the previous completed
|
||||||
were made during this review.
|
smoke-test remediation log; its findings and completion records remain in Git.
|
||||||
|
|
||||||
The facade/internal boundary, final-value configuration ownership, catalog-owned
|
## Architecture Assessment
|
||||||
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
|
The targeted architecture is substantially in place:
|
||||||
|
|
||||||
Priority: P2. Status: completed 2026-10-13.
|
- ConfigKit v0.8.0 composes caller-selected inputs and applies the shared optional
|
||||||
|
specification-header policy. PromptKit interprets final effective values and
|
||||||
|
resource contexts without flattening the retained configuration or replaying
|
||||||
|
source history.
|
||||||
|
- Root resources retain unqualified identities; collection resources use full
|
||||||
|
namespace/local-key identities in captured runtime indexes. Stored references
|
||||||
|
remain exact, and inheritance stays with ConfigKit.
|
||||||
|
- Scoped engine views share the assembled runner, model clients, inspection,
|
||||||
|
inventory, and capacity machinery. Qualification is centralized at engine
|
||||||
|
entry points; prepared execution retains its captured selections.
|
||||||
|
- Prompt selection is ID-only, with optional descriptive version labels.
|
||||||
|
Namespace-filtered prompt/profile/backend listing, resource namespace discovery,
|
||||||
|
and effective collection inventory are available.
|
||||||
|
- Automatic catalog dependencies and built-in profiles are removed. Consumer
|
||||||
|
migration guidance and offline collection examples are present.
|
||||||
|
|
||||||
Locations: [`finishHTTP`](../../internal/usecase/smoke_tester.go), lines 355–366,
|
The findings below are focused ownership, construction-efficiency, and contract
|
||||||
and [`runOpenRouterKey`](../../internal/usecase/smoke_authentication.go),
|
documentation corrections, not reasons to redesign the feature. No additional
|
||||||
lines 107–110.
|
unrequested runtime restrictions were identified in the reviewed paths. Finding 3
|
||||||
|
addresses a retained public guarantee that no longer describes scoped views.
|
||||||
|
|
||||||
`finishHTTP` returns a caller-context error only when `report.Failure` has already
|
## Finding 1: Share Origin Metadata Across Collection Materialization
|
||||||
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
|
Priority: P2. Status: open.
|
||||||
`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
|
Locations: [load.go](../../internal/configuration/load.go), lines 257–262, and
|
||||||
work independently of the underlying I/O error's identity. Return the retained
|
[materializePrompts](../../internal/configuration/prompts.go), lines 84–95.
|
||||||
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
|
The loader now calls `materializePrompts` separately for every collection. Each
|
||||||
that cancels the caller and returns a non-context I/O error. Its existing fake
|
call creates its own lazy metadata cache and calls `state.result.Metadata()`
|
||||||
returns `context.Canceled`, which masks this defect. Check partial-report and
|
when it encounters a file-backed message. In the selected ConfigKit v0.8.0,
|
||||||
context identity rather than exact error text; reuse existing timeout coverage.
|
`Result.Metadata` defensively clones the whole metadata snapshot, including
|
||||||
|
field/history maps and paths. The cache is consequently reused within one
|
||||||
|
collection but discarded before the next.
|
||||||
|
|
||||||
Completion: HTTP probe failures now check the caller context at the failed-I/O
|
With C collections requiring origins and M total metadata entries, this adds
|
||||||
boundary before classification, preserving partial response evidence even for
|
O(C × M) copying during construction; when resource count grows with collection
|
||||||
opaque transport/body errors. Completed authentication is not revoked when body
|
count, this portion becomes quadratic. Before the collection refactor, the same
|
||||||
closure subsequently cancels the caller. The new regressions failed before the
|
cache covered the single prompt-materialization pass.
|
||||||
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
|
An offline diagnostic benchmark used 4, 16, and 64 collections, each containing
|
||||||
|
one prompt with one `content_file`, all reading a tiny in-memory file. A single
|
||||||
|
iteration per case reported:
|
||||||
|
|
||||||
Priority: P2. Status: completed 2026-10-13.
|
| Collections | Allocated bytes per construction |
|
||||||
|
| --- | ---: |
|
||||||
|
| 4 | 1,662,192 |
|
||||||
|
| 16 | 10,267,520 |
|
||||||
|
| 64 | 104,352,336 |
|
||||||
|
|
||||||
Locations: [`recordGenerationEvidence`](../../internal/usecase/smoke_tester.go),
|
An allocation-space profile across those diagnostic loads attributed about 79%
|
||||||
lines 240–248, and
|
of allocated space to ConfigKit metadata cloning reached through
|
||||||
[`resolveSmokeTestEndpoint`](../../internal/usecase/smoke_selection.go),
|
`materializePrompts`' origin callback. These are illustrative local measurements,
|
||||||
lines 171–188.
|
not stable thresholds or a claim about all constructor workloads.
|
||||||
|
|
||||||
Inference selection leaves the diagnostic endpoint at the profile's base URL,
|
Required outcome: reuse one construction-wide lazy metadata snapshot or an
|
||||||
and every attempt copies that value into `InitialEndpoint`. The built-in client
|
equivalent narrow lookup across root and collection prompt materialization.
|
||||||
actually dispatches to that base plus `/chat/completions`. Thus a request with
|
Preserve exact effective paths, inherited origins, resource contexts, and
|
||||||
no redirect reports different initial and final destinations even though no
|
handle closure. Keep the change local and construction-scoped; no new source
|
||||||
redirect occurred. `InitialEndpoint` is documented as the destination before
|
validation, global cache, or upstream API extension is needed.
|
||||||
redirects, not the base used to construct the request.
|
|
||||||
|
|
||||||
An offline public-API reproduction observes an actual request to
|
Validation: retain origin/inheritance/content-capture coverage with more than
|
||||||
`https://openrouter.ai/api/v1/chat/completions`, while the attempt reports
|
one file-backed collection, and use a small scaling benchmark to verify the
|
||||||
`InitialEndpoint="https://openrouter.ai/api/v1"` and
|
repeated full-snapshot cost is removed. Do not make exact allocations, elapsed
|
||||||
`Endpoint="https://openrouter.ai/api/v1/chat/completions"`. `Target.Endpoint` also
|
times, or private helper call order a new contract.
|
||||||
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
|
## Finding 2: Detach The Optional Label In Internal Prepared Snapshots
|
||||||
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
|
Priority: P3. Status: open.
|
||||||
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
|
Location: [clonePreparedRun](../../internal/usecase/prepared_execution.go),
|
||||||
through internal generation evidence. Diagnostic target/initial-attempt URLs
|
lines 226–244; also its use by `PrepareExecution` and `PreparedExecution.Details`.
|
||||||
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
|
`domain.PreparedRun.PromptVersion` changed from a string to `*string`, but
|
||||||
|
`clonePreparedRun` still performs only the initial struct assignment for that
|
||||||
|
field. Its returned snapshot shares the label pointer with its source.
|
||||||
|
`PrepareExecution` uses this copier to produce retained details from the
|
||||||
|
execution snapshot, and `Details` uses it again for each returned copy.
|
||||||
|
|
||||||
Priority: P3. Status: completed 2026-10-13.
|
A temporary package-level probe constructed a prepared handle with label
|
||||||
|
`original`, obtained its internal `Details`, changed that result's label to
|
||||||
|
`mutated`, then obtained `Details` again. The second result incorrectly contained
|
||||||
|
`mutated`. This contradicts the internal method's fresh-copy contract and the
|
||||||
|
planned ownership of optional metadata. The same copier leaves retained details
|
||||||
|
and execution metadata sharing this pointer.
|
||||||
|
|
||||||
Location: [`SmokeTester.Run`](../../internal/usecase/smoke_tester.go),
|
The public facade's `fromDomainPreparedRun` currently copies the pointer again,
|
||||||
lines 52–87, and its early return paths.
|
so public callers remain protected by that additional boundary. This is an
|
||||||
|
internal ownership gap, not a reproduced public mutation or credential leak.
|
||||||
|
|
||||||
Reports begin with a zero-valued `ObservedTokenUsage`. Unsupported/inapplicable
|
Required outcome: copy the optional label with the existing nil-aware string
|
||||||
operations and cancellation before generation return it without applying the
|
copy helper when cloning prepared runs. Retain absent versus declared-empty
|
||||||
shared zero-attempt accounting. The public
|
semantics and the existing public ownership protection; a cloning-framework
|
||||||
[`ObservedTokenCount` contract](../../observations.go) defines aggregate zero
|
refactor is unnecessary.
|
||||||
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
|
Validation: extend the existing owning use-case prepared-details test with a
|
||||||
with an already-canceled context both produce no attempts but return
|
non-nil version label. Mutation of one returned internal snapshot must not change
|
||||||
`TotalTokens={Value:0, Known:false, Complete:false}`. The same applies to the
|
retained details, the execution snapshot, or a later result. Reuse existing
|
||||||
other token fields. There is no unobserved generation to justify unknown usage.
|
nil/empty public metadata coverage rather than reproducing it at every layer.
|
||||||
|
|
||||||
Required outcome: use the existing zero-attempt usage accumulator consistently
|
## Finding 3: Document Engine Ownership In Terms Of The Shared Assembly
|
||||||
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
|
Priority: P3. Status: open.
|
||||||
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
|
Location: [Engine GoDoc](../../engine.go), lines 85–101; related
|
||||||
whenever no attempt was dispatched, replacing scattered early-return assignments.
|
[prepared-handle GoDoc](../../prepared_execution.go) and
|
||||||
Unsupported/inapplicable operations and early cancellation return known, complete
|
[WithNamespaces GoDoc](../../namespaces.go).
|
||||||
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
|
The Engine declaration still says that each Engine owns independent
|
||||||
|
backend-capacity pools. `WithNamespaces` now returns another `*Engine` sharing
|
||||||
|
the same runner and capacity manager, as expressly required by the feature.
|
||||||
|
The new `TestScopedCollectionRunsShareBackendCapacity` demonstrates this sharing;
|
||||||
|
the runtime is correct, but the public declaration promises the opposite for
|
||||||
|
these returned Engine values.
|
||||||
|
|
||||||
Priority: P3. Status: completed 2026-10-13.
|
Prepared handles are likewise described as bound to the Engine that created
|
||||||
|
them, without clarifying that namespace views share that underlying owner.
|
||||||
|
The current public tests intentionally run a scoped handle through the original
|
||||||
|
engine. Documentation should make that distinction clear rather than imply
|
||||||
|
per-facade capacity or handle ownership.
|
||||||
|
|
||||||
Follow-up review covers the implementation and remediation commits through
|
Required outcome: distinguish independently constructed engines from namespace
|
||||||
`5ea6f06a6d0be32ecb68bc98513f02f29be1cae3`.
|
views of one assembled engine. State that views share clients/capacity and the
|
||||||
|
prepared-execution owner, with selections already frozen in prepared handles.
|
||||||
|
Preserve independent ownership between separate `NewEngine` constructions.
|
||||||
|
Do not change the implementation to satisfy the stale wording or introduce
|
||||||
|
scope-mismatch rejection.
|
||||||
|
|
||||||
Locations: [`fromDomainSmokeTestReport`](../../smoke_diagnostics.go),
|
Validation: review the affected GoDoc together. Reuse the passing shared-capacity
|
||||||
lines 296–336; [`CloneSmokeTestReport`](../../internal/domain/smoke_diagnostics.go),
|
and cross-view prepared-handle tests; no new behavior or test matrix is needed.
|
||||||
lines 180–221; and [`fromDomainExecutionTarget`](../../convert.go).
|
|
||||||
|
|
||||||
The facade first deep-clones the entire internal report through
|
## Review Validation
|
||||||
`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
|
Passed on the reviewed committed tree with `GOWORK=off`:
|
||||||
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 test -race ./...`;
|
||||||
- `go vet ./...`;
|
- `go vet ./...`;
|
||||||
- `python3 scripts/check-examples.py`.
|
- `python3 scripts/check-examples.py`.
|
||||||
|
|
||||||
The three findings were reproduced through the public API using a temporary
|
Source review covered the configuration handoff, header policy, paths/origins
|
||||||
external Go program and a controlled HTTP RoundTripper; no live provider was
|
and sensitivity, root/collection indexing, optional metadata copying, all scoped
|
||||||
contacted and no reproduction source was added to the repository. The passing
|
selection paths, listing/inventory behavior, shared capacity, dependency removal,
|
||||||
suite at the reviewed revision did not cover these cases; the completed
|
and migration contracts. Representative existing tests were inspected as well as
|
||||||
remediations add the corresponding regression coverage.
|
executed; the green suite does not cover the internal label-aliasing probe.
|
||||||
|
|
||||||
Both catalogs remain selected at locally validated `v3.20261013.0` candidates.
|
The package-level ownership probe failed as described in Finding 2. The
|
||||||
Their publication and any publication-date re-pinning remain separate release
|
construction benchmark and allocation profile support Finding 1. Both probes
|
||||||
preparation, as deliberately agreed, rather than an implementation defect or a
|
used temporary Go test overlays outside the repository; neither changes the
|
||||||
new review gate.
|
implementation or adds permanent tests. No live provider or real credential
|
||||||
|
was used. Only this remediation document is changed by the review.
|
||||||
|
|||||||
Reference in New Issue
Block a user