diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..39c5a28 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,140 @@ +# Import Integration Correctness Audit + +Status: open; three findings require correction. + +Reviewed tree: `91db99b` (including the six completed fixes through `5747ec4`). +Scope: import discovery/defaults and shared dependency-ready scheduling, including +their interaction with existing strict composition, precedence tiers, sequence +placement, source-version checking, inheritance, provenance/history, passive +resource contexts, and maintained consumer examples. The completed design and +implementation plan remain available at `5747ec4:docs/roadmap/imports.md` and +`5747ec4:docs/roadmap/implementation.md`. + +This review changes no production code or existing contracts. Preserve the +accepted syntax, defaults, scheduling, containment, and namespace decisions when +addressing these findings. + +## 1. [ ] P2 — Accept contained absolute symlinks in directory-root prefixes + +Owners: [`importEntryTarget`](../../internal/engine/filesystem_links.go), +especially the initial host `a.root.Lstat(relative)` around line 30, and +[`selectDirectoryImportCandidates`](../../internal/engine/import_selection.go), +around lines 114–119. + +A declared directory root below a contained absolute symlink fails even though +the same source is accessible through the other selectors. This contradicts the +[import contract](../config.md#explicit-imports): explicit roots retain normal +access behavior, and contained relative or absolute host symlink targets are +accepted. + +Reproduction on Linux with Go 1.26.9: + +- A configuration root contains `real/parts/part.yml` with `value: included`. +- `absolute` is a symlink to the absolute path of that root's `real` directory. +- `main.yml` enables imports and declares `dir: absolute/parts` with + `pattern: '*.yml'`. +- Loading fails with `import.invalid` at `/configkit/imports/0/dir`, reporting + `directory entry could not be inspected`. +- `path: absolute/parts/part.yml`, `glob: 'absolute/parts/*.yml'`, and + `dir: absolute` with recursive discovery all succeed and load the same file. + Replacing the absolute symlink target with relative `real` also succeeds. + +Named-target validation already accepts the contained canonical directory. +Discovery then inspects its original spelling through `os.Root.Lstat`, which +rejects an absolute symlink in an intermediate component before the existing +component-wise resolver can handle it. Resolving the link as the final component +does not encounter that failure, explaining the inconsistent results. + +Remediation: make explicit directory-root inspection handle contained symlinks +in intermediate components as well as final components. Keep candidate spelling +separate from canonical host access/source identity, and preserve containment, +cycle detection, native identity, and discovered-entry `follow` behavior. Do not +solve this by prohibiting absolute symlinks or relaxing root containment. Add a +focused public regression for a directory root below an absolute contained link; +reuse existing escaping-link and cycle coverage. + +## 2. [ ] P2 — Describe scheduled provenance and history order consistently + +Owner: [configuration contract](../config.md), especially layered metadata +around lines 431–439, historical explanation around lines 501–504, and strict +composition diagnostics around lines 821–825. + +These sections still promise source/declaration traversal order for composite +origin locations, base-source history, and conflict locations. The updated import +section correctly distinguishes authored acquisition from dependency-ready +assembly. Current inspection and diagnostics follow assembly order within a +strict scope, not source-record acquisition order. + +Public reproduction: + +- `main.yml` contributes `settings: {main: main}` and imports `z.yml`, then + `a.yml`; each import contributes a distinct child under `settings`. +- `Sources()` returns `main.yml`, `z.yml`, `a.yml`, with IDs `source-1`, + `source-2`, `source-3`. +- The composite origin and `Explain(/settings)` input history use `a.yml`, + `z.yml`, `main.yml` (`source-3`, `source-2`, `source-1`). +- In a separate main/child scalar conflict, the imported child's claim is + primary and the main claim is related, despite the main source being acquired + first. + +This is the intended scheduler behavior, not a production ordering defect. +Remediation: update the older contract sections to distinguish acquisition order +from scoped assembly/replay order and link to the existing ordering definition. +Keep source IDs and source records in acquisition order, precedence tiers in +precedence order, and item-local scope boundaries intact. Do not restore the old +DFS behavior or introduce an independent provenance ordering rule. Existing +scheduled-placement and import-order tests already cover the main mechanics; +check the revised wording against the public cases above. + +## 3. [ ] P3 — Qualify the empty-path inspection exception by requiredness + +Owner: [import inspection contract](../config.md#explicit-imports), around lines +1010–1011. + +The contract says that empty `path: []` succeeds but does not produce an +optional-empty record. That is true only when the declaration is effectively +required. With omitted/default-false `required`, the implementation returns one +`ImportSelections` record with selector `path`, outcome `none`, and the +source-local declaration path. Adding `required: true` succeeds without that +record. + +The accepted roadmap explicitly includes deliberate empty lists among optional +no-selection outcomes. The implementation plan singles out **required** empty +paths for the exception. The +[engine evidence tests](../../internal/engine/import_evidence_test.go) already +exercise both forms and intentionally expect evidence for the optional form; +the [public record comment](../../records.go) agrees with that behavior. + +Remediation: qualify the documentation exception as applying to effectively +required empty path lists. Retain optional empty-list evidence and existing +tests rather than changing production behavior to match the inaccurate sentence. +Preserve the distinction between no selection and missing-file evidence. + +## Validation and remaining architecture + +The ordinary development check and an uncached full race run passed on the +reviewed production tree: + +```sh +PATH=/snap/go/current/bin:$PATH GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off \ + GOCACHE=/tmp/configkit-review-cache-shared ./scripts/check.sh +GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off \ + GOCACHE=/tmp/configkit-review-cache-shared \ + /snap/go/current/bin/go test -race -count=1 ./... +``` + +The suite includes executable public examples and maintained consumer fixtures. +Additional disposable public-API probes outside the repository confirmed the +three findings and exercised scheduled item/helper placement in ordinary and +underlay loads, nested sequence values, projected source destinations, consumed +version-header sensitivity, and agreement of effective values with inspection. +The filesystem reproduction used real temporary files and symlinks. + +The root-owned facade, private engine, parser-independent document tree, and YAML +adapter remain separated. Discovery retains authored acquisition order; the +shared scheduler governs scoped assembly and sibling dependencies. Strict +composition within tiers, recursive precedence between tiers, destination-local +inheritance, and the subsequent typed lifecycle retain their existing roles. +No further correctness issue was confirmed in the reviewed interactions. These +checks do not provide macOS or external downstream-application validation, and +do not constitute release publication.