7.6 KiB
Destination review findings
Reviewed d3d468a..945e111 against the approved
input-destinations roadmap,
input-destination implementation plan (preserved at
315f464:docs/roadmap/implementation.md), and
ADR 0011.
The root-owned destination fields, placement before strict composition, transitive import placement, effective-path version ownership, final lifecycle, source records, resource contexts, retained history, and existing disclosure mechanisms broadly match the selected architecture. The required correction below concerns an early diagnostic path, not loading or composition semantics. No additional unintended strengthening of contracts was identified.
1. Rebase known Mapping-source failure paths to the input destination (P2)
Status: complete. Known Mapping value-builder paths are now prefixed with the
normalized destination at acquisition, preserving semantic key/index segments.
Public regressions cover mounted main and underlay failures, escaped keys,
sequence indices, cyclic values, unchanged root paths, and source-constructor
errors retaining source coordinates. The mounted value cases failed before the
fix and pass afterward. Existing directive/syntax checks remain unchanged.
Focused destination/source/diagnostic/import tests, ./scripts/check.sh, and
GOTOOLCHAIN=local GOPROXY=off go test -race ./... passed on
go1.26.8 linux/amd64.
Affected: assembleBaseInputTracked in
load_input.go, lines 27–32 at the reviewed
commit; the Mapping failure originates in readSource in
source.go, lines 206–208.
Mapping snapshots its input through the existing value builder. When a nested
value is unsupported, cyclic, or exceeds a value-builder limit, the retained
issue already identifies a semantic path inside that application mapping.
readSource turns it into a structured diagnostic. The early return from
assembleBaseInputTracked annotates the source record with its destination but
returns those diagnostic paths unchanged.
For example, select this input as either main or an underlay:
configkit.Input{
Source: configkit.Mapping("bad mapping", map[string]any{
"item": map[string]any{"bad": func() {}},
}),
Destination: configkit.Keys("catalog", "entries"),
}
The load correctly fails with source.invalid, but Diagnostic.Path is
/item/bad rather than /catalog/entries/item/bad. The source record correctly
reports /catalog/entries, so the two inspection records use inconsistent
application coordinates. A consumer displaying or associating the failure with
the final application shape would point to the wrong configuration field.
This diverges from implementation decision 10 and Stage 6: existing paths that genuinely identify application content must be translated to effective paths exactly once. A Mapping value-builder issue has that information; it is not a source-read or YAML syntax failure for which a logical child path is unavailable.
Required remediation:
- Prefix known application-relative Mapping value-builder diagnostic paths with the normalized input destination at the acquisition boundary. Preserve key escaping and sequence-index intent in the existing semantic Path model.
- Keep source IDs, original primary/related locations, codes, value-free messages, source destinations, and nil-result behavior unchanged. Root placement must retain its current paths.
- Do not blanket-prefix parse/read failures, source-constructor errors, or structural import-directive diagnostics. Do not invent logical child paths for failures that do not identify application content.
- Add bounded regression coverage through the public API for mounted main and underlay Mapping failures, alongside unchanged root placement. Reuse existing syntax/directive tests to protect their different coordinate convention.
- Run the focused destination/source/diagnostic checks and the everyday check. Reconcile the completed plan's diagnostic evidence with this correction; do not redefine the agreed effective-path behavior to accept the gap.
Review validation
GOTOOLCHAIN=local ./scripts/check.shpassed at945e111ongo1.26.8 linux/amd64(formatting, whitespace, tests, vet, and build).GOTOOLCHAIN=local GOPROXY=off go test -race ./...passed.GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...passed.- An additional external-package probe in a disposable checkout outside the
repository failed for both main and underlay placement, observing
/item/badinstead of/catalog/entries/item/badwithsource.invalid. - No production fixes or regression tests were added to this repository during the review. No performance measurements or new guarantees were inferred.
Relative import destination review (2026-10-14)
Reviewed 7248a8b..10edd95 against the
relative-import roadmap, completed
implementation plan, and
ADR 0012.
The implementation matches the selected architecture: literal string segments append to each immediate parent's destination; placement uses the existing strict-composition and final-loading pipeline; source records retain distinct effective destinations without changing physical resource contexts. No runtime defect, missing feature behavior, or unintended strengthening of consumer contracts was identified. The completed input-destination finding above remains resolved. The documentation correction identified below is now resolved.
2. Restore ADR 0011's accepted decision text (P3)
Status: complete. Restored the accepted Decision paragraph verbatim from
7248a8b, leaving the extension link in ADR 0011's Status section and ADR 0012
unchanged. A comparison of everything from ## Date onward is identical to the
baseline; the full ADR diff against 7248a8b changes only Status. The uncached
local documentation-link check and git diff --check passed. The existing
TestFacadeDestinationMappingFailurePaths regression also passed, confirming
the earlier completed finding remains addressed. No runtime code, current
configuration/API documentation, or tests were changed.
Affected: ADR 0011, lines 34–42 at
10edd95. That commit rewrote its accepted Decision paragraph to incorporate
relative import destinations. Stage 1 of the implementation plan explicitly
required preserving that text, and the
documentation policy
treats accepted decision content as immutable. Rewriting it obscures the original
decision and its subsequent extension, despite the accurate current behavior.
Required remediation:
- Restore that Decision paragraph verbatim from
7248a8b(the same paragraph remains unchanged ata8beb3e). Keep the extension link in ADR 0011's Status section and retain ADR 0012 as the accepted extension record. - Leave the implemented relative-import behavior and current configuration/API documentation unchanged. No additional ADR or runtime tests are needed.
- Confirm the ADR 0011 diff against
7248a8bchanges only its Status section; run the local documentation-link and whitespace checks.
Relative import review validation
At 10edd95d4d9060bd46db51e0d6782c12dbda3120, using
go1.26.8 linux/amd64, all of these passed:
GOTOOLCHAIN=local ./scripts/check.sh(formatting, whitespace, tests, examples, links, package boundaries, vet, and build).GOTOOLCHAIN=local GOPROXY=off go test -race ./....GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./....
The review changed only this findings document; it did not implement fixes.