Files
ConfigKit/docs/roadmap/remediation.md

7.4 KiB
Raw Blame History

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.

Owners: importEntryTarget, especially the initial host a.root.Lstat(relative) around line 30, and selectDirectoryImportCandidates, 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: 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, 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, 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 already exercise both forms and intentionally expect evidence for the optional form; the public record comment 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:

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.