promptkit/docs/roadmap/remediation.md

# Configuration Collections Implementation Review Findings

Status: all three findings completed 2026-10-17.

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.

## Architecture Assessment

The targeted architecture is substantially in place:

- 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.

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.

## Finding 1: Share Origin Metadata Across Collection Materialization

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

Locations: [load.go](../../internal/configuration/load.go), lines 257–262, and
[materializePrompts](../../internal/configuration/prompts.go), lines 84–95.

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.

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.

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:

| Collections | Allocated bytes per construction |
| --- | ---: |
| 4 | 1,662,192 |
| 16 | 10,267,520 |
| 64 | 104,352,336 |

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.

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.

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.

Completion: prompt materialization now traverses root and collection resources
within one construction-scoped pass sharing the lazy origin metadata snapshot.
Extended the imported/inherited-content test across distinct collection resource
contexts and added a scaling benchmark without allocation or timing assertions.
At 64 collections the same benchmark dropped from 104,574,200 to 21,239,712
allocated bytes per construction in single-iteration local measurements.
Validation passed: focused origin tests and benchmark, race-enabled configuration
and public API tests, configuration vet, and `git diff --check`.

## Finding 2: Detach The Optional Label In Internal Prepared Snapshots

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

Location: [clonePreparedRun](../../internal/usecase/prepared_execution.go),
lines 226–244; also its use by `PrepareExecution` and `PreparedExecution.Details`.

`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.

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.

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.

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.

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.

Completion: the prepared-run copier now detaches the optional version label with
the existing nil-aware helper. Extended the owning prepared-execution snapshot
test to mutate source and returned labels and verify later details, execution
metadata, and post-execution details remain unchanged. The regression failed
before the fix and passes afterward. Validation passed with `GOWORK=off`:
the focused regression, `go test -race ./...`, `go vet ./...`, all three offline
examples, and `git diff --check`.

## Finding 3: Document Engine Ownership In Terms Of The Shared Assembly

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

Location: [Engine GoDoc](../../engine.go), lines 85–101; related
[prepared-handle GoDoc](../../prepared_execution.go) and
[WithNamespaces GoDoc](../../namespaces.go).

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.

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.

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.

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.

Completion: Engine, WithNamespaces, PrepareExecution, RunPrepared, and the
prepared-handle GoDoc now distinguish independent constructions from shared
namespace views. They describe shared clients/capacity and handle ownership,
with captured selections unchanged by the invoking view. No runtime behavior
was changed. Reviewed rendered `go doc` output; the existing shared-capacity and
cross-view prepared-handle tests and `git diff --check` passed. The full
race-enabled suite, vet, and offline examples from Finding 2 cover the unchanged
implementation.

## Review Validation

Passed on the reviewed committed tree with `GOWORK=off`:

- `go test -race ./...`;
- `go vet ./...`;
- `python3 scripts/check-examples.py`.

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.

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.