Record underlay implementation review findings

docs/roadmap/remediation.md

@@ -0,0 +1,119 @@
+# Filesystem Sources and Underlays Review Findings
+
+Reviewed implementation: `cbf031a`, covering the implementation commits after
+`27df838`. These findings concern the agreed source, precedence, and provenance
+contracts in the [roadmap](sources-and-layering.md) and
+[implementation plan](implementation.md). No implementation fixes are included
+in this review.
+
+## 1. Preserve underlay attribution for sequence descendants (P2)
+
+**Owners:** `internal/engine/load_request.go:174`,
+`internal/engine/compose.go:113`, and `internal/engine/load_result.go:39`.
+
+The first nonempty underlay seeds the accumulator using a composed document.
+Composition claims intentionally stop at sequences because sequences are atomic
+for strict conflict checking. Consequently its origin index contains the sequence
+itself but not sequence elements or nested fields inside them. Relabeling that
+index as `LayerUnderlay` leaves those descendants with the decoder's default
+`LayerBase` attribution. Later overlay insertion recursively collects origins,
+so attribution also depends on whether the same input is the initial tier.
+
+**Reproduction:** load `NoSource()` as main and an underlay mapping containing
+`items: [{name: first}]`. The load succeeds. `Metadata.Lookup` reports
+`LayerUnderlay` for `/items`, but `LayerBase` for `/items/0` and `/items/0/name`,
+all referring to the same underlay source ID. `Metadata.History` repeats the
+incorrect base attribution for the descendants.
+
+**Required correction:** cover every surviving input node when assigning origins,
+including descendants of atomic sequence values. Keep strict-composition claims
+and sequence replacement semantics unchanged. Ensure resolver entry metadata,
+returned metadata, and explanation agree with each acquired source's role.
+
+**Regression coverage:** a first-tier sequence containing nested mappings,
+including an imported contribution, retained beneath main input; compare with
+equivalent later-tier insertion. Assert source ID and underlay role for elements
+and their fields as well as the sequence container.
+
+## 2. Preserve later same-role mapping contributions in history (P2)
+
+**Owner:** `internal/engine/load_metadata.go:128` (`applyOverlayMetadata`).
+
+The final-history reconciliation skips an effective origin when its layer and
+operation match the last recorded event. Multiple underlay tiers share
+`LayerUnderlay`, so those two fields do not establish that the recorded event
+contains the effective source contributors. The comparison cannot distinguish
+contributions from repeated tiers of the same role.
+
+**Reproduction:** with no main document, supply three ordered underlays:
+
+```yaml
+# Tier 1
+x: 1
+# Tier 2
+x: {}
+# Tier 3
+x: {y: 2}
+```
+
+The final `/x` origin correctly includes sources 2 and 3. Its metadata history
+contains only source 1 and the replacement by source 2; source 3's contribution
+is omitted. The returned effective provenance and chronological history disagree.
+
+**Required correction:** determine whether the effective provenance is already
+represented using contributing origins, not merely layer/operation equality.
+Preserve meaningful same-tier-role contributions without duplicating events
+already represented. Keep existing replacement, equal-value, and displaced-path
+history behavior.
+
+**Regression coverage:** scalar-to-mapping replacement followed by a recursive
+mapping contribution from a later underlay; assert the final contributor appears
+in history and that metadata and explanation describe consistent source origins.
+
+## 3. Honor standard filesystem names in resource contexts (P2)
+
+**Owner:** `internal/engine/resource_context.go:38` and `:77`.
+
+Both resource adapters reject any name containing a backslash, beyond
+`fs.ValidPath` validation. A backslash is a legal literal filename character in
+an `io/fs` namespace and on the Linux host filesystem. Native source acquisition
+accepts such paths, but resource-context access rejects them. This introduces an
+unrequested path restriction and applies host-style separator assumptions to a
+generic filesystem.
+
+**Reproduction:** supply `fstest.MapFS` with `config.yml` and a resource named
+`asset\name`. Loading `config.yml` succeeds; the resource name passes
+`fs.ValidPath` and can be read directly from the supplied filesystem.
+The equivalent read through the returned context fails with `fs.ErrInvalid`:
+
+```go
+context, ok := result.SourceContext(sourceID) // ok is true
+_, err := fs.ReadFile(context.FS, name)       // rejects the valid name
+```
+
+**Required correction:** use standard slash-path validation for native contexts
+and preserve literal characters accepted by the supplied filesystem. For host
+contexts, use platform-appropriate conversion while preserving the existing
+contained-root behavior; native namespace semantics must not inherit Windows
+separator restrictions. This is not a request to broaden root traversal or
+change symlink guarantees.
+
+**Regression coverage:** access a literal-backslash resource through both native
+Open/ReadDir paths where applicable and a Linux host context; retain invalid-path
+and outside-root coverage. Avoid rejecting valid native names on other platforms
+merely because the corresponding host path would be invalid there.
+
+## Review validation
+
+- `./scripts/check.sh` passed (tests, vet, build, formatting, and whitespace).
+- `GOPROXY=off go test -race ./...` passed.
+- Broad-catalog and many-small-tier benchmarks ran with `-benchmem` and 50 ms
+  samples. Allocated bytes scaled approximately with tested input sizes; these
+  measurements are review evidence, not timing gates.
+- A temporary external Go program reproduced all three findings through the
+  public API; no diagnostic/test code was added to the repository.
+
+The intended facade, shared synchronous lifecycle, strict within-tier composition,
+recursive precedence, mapping snapshotting, and passive resource-context design
+are present. The findings above are correctness gaps within that design, not a
+recommendation to expand its scope or change its selected combination semantics.