Retire completed import integration remediation
This commit is contained in:
@@ -160,6 +160,10 @@ git show 5747ec4:docs/roadmap/implementation.md
|
||||
git show 5747ec4:docs/roadmap/remediation.md
|
||||
```
|
||||
|
||||
The subsequent import-integration audit and completed remediation record are
|
||||
preserved at `6903c9e:docs/roadmap/remediation.md`; retrieve them with
|
||||
`git show 6903c9e:docs/roadmap/remediation.md`.
|
||||
|
||||
Potential enhancements are tracked in [future features](roadmap/future.md).
|
||||
|
||||
Install the Go version required by `go.mod` or a compatible newer toolchain.
|
||||
|
||||
@@ -1,163 +0,0 @@
|
||||
# Import Integration Correctness Audit
|
||||
|
||||
Status: complete; all three findings addressed in separate validated units.
|
||||
|
||||
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. [x] 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.
|
||||
|
||||
Completed: the import inspection contract now states that empty path lists
|
||||
succeed regardless of requiredness, recording `none` only when effectively
|
||||
optional. Production behavior and existing tests are unchanged. Verified both
|
||||
forms through the public API and the existing optional-empty evidence test;
|
||||
`./scripts/check.sh`, local documentation links, and `git diff --check` passed.
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user