diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..dacc1ad --- /dev/null +++ b/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.