158 lines
8.5 KiB
Markdown
158 lines
8.5 KiB
Markdown
# Import Integration Correctness Audit
|
||
|
||
Status: in progress; findings 1 and 2 complete, finding 3 remains.
|
||
|
||
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.
|
||
|
||
## 1. [x] 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.
|
||
|
||
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. [x] 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.
|
||
|
||
Completed: composite-origin, explanation-history, and conflict-location wording
|
||
now links to scoped assembly order and distinguishes it from source acquisition.
|
||
The descriptions retain precedence-tier order and item-local scope boundaries.
|
||
No scheduler or provenance implementation changed. Verified the public composite
|
||
origin/history and conflict reproductions, existing scheduled-placement and main/
|
||
underlay ordering tests, local documentation links, and `git diff --check`.
|
||
|
||
## 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.
|