From 165cef7aecb9f2e3782c99e2d4fa50eb4a519015 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 17 Oct 2026 13:37:52 +0000 Subject: [PATCH] Record configuration collections review findings --- docs/roadmap/remediation.md | 314 ++++++++++++++---------------------- 1 file changed, 125 insertions(+), 189 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 0fdbf85..a5c53aa 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -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 -`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. +Reviewed implementation after `89af2a1` through `0221cd4` against the accepted +[feature roadmap](configuration-collections.md), [implementation plan](implementation.md), +[configuration-boundary ADR](../adr/0001-configuration-boundary.md), current +GoDoc, and applicable repository policies. This replaces the previous completed +smoke-test remediation log; its findings and completion records remain in Git. -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. +## Architecture Assessment -## 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, -and [`runOpenRouterKey`](../../internal/usecase/smoke_authentication.go), -lines 107–110. +The findings below are focused ownership, construction-efficiency, and contract +documentation corrections, not reasons to redesign the feature. No additional +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 -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. +## Finding 1: Share Origin Metadata Across Collection Materialization -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`. +Priority: P2. Status: open. -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. +Locations: [load.go](../../internal/configuration/load.go), lines 257–262, and +[materializePrompts](../../internal/configuration/prompts.go), lines 84–95. -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. +The loader now calls `materializePrompts` separately for every collection. Each +call creates its own lazy metadata cache and calls `state.result.Metadata()` +when it encounters a file-backed message. In the selected ConfigKit v0.8.0, +`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 -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. +With C collections requiring origins and M total metadata entries, this adds +O(C × M) copying during construction; when resource count grows with collection +count, this portion becomes quadratic. Before the collection refactor, the same +cache covered the single prompt-materialization pass. -## 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), -lines 240–248, and -[`resolveSmokeTestEndpoint`](../../internal/usecase/smoke_selection.go), -lines 171–188. +An allocation-space profile across those diagnostic loads attributed about 79% +of allocated space to ConfigKit metadata cloning reached through +`materializePrompts`' origin callback. These are illustrative local measurements, +not stable thresholds or a claim about all constructor workloads. -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. +Required outcome: reuse one construction-wide lazy metadata snapshot or an +equivalent narrow lookup across root and collection prompt materialization. +Preserve exact effective paths, inherited origins, resource contexts, and +handle closure. Keep the change local and construction-scoped; no new source +validation, global cache, or upstream API extension is needed. -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. +Validation: retain origin/inheritance/content-capture coverage with more than +one file-backed collection, and use a small scaling benchmark to verify the +repeated full-snapshot cost is removed. Do not make exact allocations, elapsed +times, or private helper call order a new contract. -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. +## Finding 2: Detach The Optional Label In Internal Prepared Snapshots -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. +Priority: P3. Status: open. -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. +Location: [clonePreparedRun](../../internal/usecase/prepared_execution.go), +lines 226–244; also its use by `PrepareExecution` and `PreparedExecution.Details`. -## 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), -lines 52–87, and its early return paths. +The public facade's `fromDomainPreparedRun` currently copies the pointer again, +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 -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. +Required outcome: copy the optional label with the existing nil-aware string +copy helper when cloning prepared runs. Retain absent versus declared-empty +semantics and the existing public ownership protection; a cloning-framework +refactor is unnecessary. -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. +Validation: extend the existing owning use-case prepared-details test with a +non-nil version label. Mutation of one returned internal snapshot must not change +retained details, the execution snapshot, or a later result. Reuse existing +nil/empty public metadata coverage rather than reproducing it at every layer. -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. +## Finding 3: Document Engine Ownership In Terms Of The Shared Assembly -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. +Priority: P3. Status: open. -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`. +Location: [Engine GoDoc](../../engine.go), lines 85–101; related +[prepared-handle GoDoc](../../prepared_execution.go) and +[WithNamespaces GoDoc](../../namespaces.go). -## 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 -`5ea6f06a6d0be32ecb68bc98513f02f29be1cae3`. +Required outcome: distinguish independently constructed engines from namespace +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), -lines 296–336; [`CloneSmokeTestReport`](../../internal/domain/smoke_diagnostics.go), -lines 180–221; and [`fromDomainExecutionTarget`](../../convert.go). +Validation: review the affected GoDoc together. Reuse the passing shared-capacity +and cross-view prepared-handle tests; no new behavior or test matrix is needed. -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. +## Review Validation -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`: +Passed on the reviewed committed tree 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. +Source review covered the configuration handoff, header policy, paths/origins +and sensitivity, root/collection indexing, optional metadata copying, all scoped +selection paths, listing/inventory behavior, shared capacity, dependency removal, +and migration contracts. Representative existing tests were inspected as well as +executed; the green suite does not cover the internal label-aliasing probe. -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. +The package-level ownership probe failed as described in Finding 2. The +construction benchmark and allocation profile support Finding 1. Both probes +used temporary Go test overlays outside the repository; neither changes the +implementation or adds permanent tests. No live provider or real credential +was used. Only this remediation document is changed by the review.