Files
ConfigKit/docs/roadmap/remediation.md

8.1 KiB
Raw Blame History

Import Integration Correctness Audit

Status: in progress; finding 1 complete, findings 2 and 3 remain.

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.

The audit made no production code or contract changes. 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.

Completed: host entry inspection resolves intermediate parent components through the existing containment-aware resolver before inspecting the final entry. Native handling and the final entry's follow policy are unchanged. Added a public regression for directory roots below an absolute contained link with both follow settings, including canonical source identity. Confirmed that the test fails before the fix and passes afterward. Focused facade and engine directory, glob, symlink, and cycle tests passed, as did ./scripts/check.sh, go test -race -count=1 ./..., and git diff --check. The original public reproduction now succeeds for all selectors.

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.