Record import discovery review findings
This commit is contained in:
183
docs/roadmap/remediation.md
Normal file
183
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,183 @@
|
||||
# Import Discovery and Scheduling Review Findings
|
||||
|
||||
Status: open; six independently reproduced findings require remediation.
|
||||
|
||||
Reviewed implementation: `aa93aed..15943e2`, ending at
|
||||
`15943e233ca3cd1286c03c0e7085871db344cd4b`, against the accepted
|
||||
[roadmap](imports.md), [implementation plan](implementation.md),
|
||||
[ADR 0015](../adr/0015-shared-dependency-ready-ordering.md), and
|
||||
[ADR 0016](../adr/0016-independent-import-controls.md).
|
||||
|
||||
The shared scheduler, scoped assembly, independent defaults, and detached
|
||||
inspection surface are present. The findings below concern their integration
|
||||
and discovery edge cases, not a proposal to change the accepted architecture.
|
||||
Implement fixes with focused regressions, preserving the selected semantics
|
||||
and existing resource limits. This review made no implementation changes.
|
||||
|
||||
## 1. [ ] P1 — Preserve native source identity and relative import bases
|
||||
|
||||
Owners: [directory selection](../../internal/engine/import_selection.go),
|
||||
especially assignment of `target.identity` to a file candidate's `accessPath`
|
||||
around lines 184–194; [glob selection](../../internal/engine/glob.go), around
|
||||
lines 81–90; and source admission in `visitPath`.
|
||||
|
||||
Directory and wildcard-glob selection use resolved symlink targets as native
|
||||
source-admission paths. Direct `path` selection retains the supplied filesystem's
|
||||
namespace path. The same native source therefore has different identity,
|
||||
relative import behavior, and resource context depending on its selector.
|
||||
The plan explicitly separates traversal identity from native admission identity.
|
||||
|
||||
Public reproduction with `NewFilesystem("alias", os.DirFS(root))`:
|
||||
|
||||
- `aliases/item.yml` links to `../data/item.yml`.
|
||||
- That item's YAML imports `helper.yml` and contributes `value: 1`.
|
||||
- `aliases/helper.yml` contributes `owner: alias`; `data/helper.yml` contributes
|
||||
`owner: target`.
|
||||
- A root main document selecting `path: aliases/item.yml` yields
|
||||
`SourceInfo.Path == "aliases/item.yml"` and `owner == "alias"`.
|
||||
- Selecting `dir: aliases` with `pattern: item.yml`, or
|
||||
`glob: 'aliases/ite*.yml'`, instead yields `SourceInfo.Path == "data/item.yml"`
|
||||
and `owner == "target"`.
|
||||
|
||||
The prior implementation at `aa93aed` yields the alias path and alias helper
|
||||
for the same wildcard glob. This is a regression that silently changes final
|
||||
configuration, not merely different reporting. It can also change repeated-source
|
||||
admission and the stable scheduling key.
|
||||
|
||||
Remediation: retain separate candidate spelling, traversal/cycle identity, and
|
||||
native admission path. Do not silently apply host-style canonical source identity
|
||||
to native filesystems. Verify path, directory, and glob equivalence for a file
|
||||
alias and an alias directory, including nested imports, source context, origins,
|
||||
and existing repeated-source behavior. Keep host canonicalization unchanged.
|
||||
|
||||
## 2. [ ] P1 — Scope main-version protection to the actual effective path
|
||||
|
||||
Owner: [composePendingItems](../../internal/engine/imports.go), around lines
|
||||
694–695, and its call to `importedVersionClaim`.
|
||||
|
||||
Pending item assembly passes the global protected version rule into an item-local
|
||||
mapping. Consequently, an ordinary item field named `version` is treated as if
|
||||
it occupied the protected main-root `/version` path. Imported item bodies have
|
||||
no `importRoleMain` exemption in that local scope, so their own domain fields
|
||||
are rejected as well as helper contributions.
|
||||
|
||||
Public reproduction: the main document declares `version: 1` and imports an
|
||||
item with `destination: [prompts]` and `placement: sequence-item`. The item is
|
||||
`{id: prompt, version: domain-version}`. Load with imports enabled, default
|
||||
source-version mode, and a `VersionRule` at `Keys("version")` whose checker
|
||||
accepts the main header. Current loading fails with `import.version` at
|
||||
`/version`; the identical input succeeds at `aa93aed`.
|
||||
|
||||
The item's effective field is `/prompts/0/version`, not `/version`. This
|
||||
unintentionally strengthens main-version ownership into a field-name restriction
|
||||
inside item scopes, contrary to the requested preservation of version semantics.
|
||||
|
||||
Remediation: use correctly projected/scoped protection, or defer the check to a
|
||||
boundary with the actual effective item coordinates. Preserve protection where
|
||||
the effective paths really overlap. Cover imported/nested items, item helpers,
|
||||
main-item loading, underlays, and enabled source-version modes without adding
|
||||
blanket exemptions or banning ordinary domain version fields.
|
||||
|
||||
## 3. [ ] P2 — Accept valid multi-component native filesystem roots
|
||||
|
||||
Owner: [resolveNativeImportPath](../../internal/engine/filesystem_links.go),
|
||||
especially lines 170–181.
|
||||
|
||||
The resolver splits a namespace-relative path from its first component and
|
||||
requires every intermediate prefix to be inside the configured root. For root
|
||||
`nested/root`, the first inspected prefix `nested` necessarily fails containment,
|
||||
even though the complete selected path lies within `nested/root`.
|
||||
|
||||
Public reproduction: use a link-aware `os.DirFS` namespace containing
|
||||
`nested/root/main.yml` and `nested/root/parts/value.yml`. Main imports `dir: parts`
|
||||
with `pattern: '*.yml'`. Supply `FilesystemContext{FS: &filesystem,
|
||||
Root: "nested/root"}`. Loading fails with `import.outside_root` on the directory
|
||||
selector despite all inputs being contained and no symlink being involved.
|
||||
|
||||
Remediation: resolve relative to the selected native root, or distinguish root
|
||||
prefix traversal from an actual escape. Preserve containment on selected and
|
||||
resolved targets. Test directory and glob discovery under multi-component roots,
|
||||
with ordinary entries and supported symlinks. Do not require consumers to flatten
|
||||
their roots or remove optional link-inspection capability.
|
||||
|
||||
## 4. [ ] P2 — Distinguish finite symlink reuse from actual cycles
|
||||
|
||||
Owners: [resolveHostImportPath and resolveNativeImportPath](../../internal/engine/filesystem_links.go),
|
||||
especially the `activeLinks` sets around lines 115–134 and 172–194.
|
||||
|
||||
These sets retain every symlink encountered during resolution of a whole path;
|
||||
entries are never removed after an expansion completes. A finite pathname that
|
||||
uses the same link twice is therefore rejected as a cycle. This is distinct from
|
||||
the correctly scoped active-directory set in recursive directory traversal.
|
||||
|
||||
Public host reproduction: create `parts/up -> ..` inside the configuration root,
|
||||
and a regular `parts/value.yml`. Import
|
||||
`dir: parts/up/parts/up/parts` with `pattern: '*.yml'` and default nonrecursive
|
||||
discovery. That finite explicit root resolves to `parts`; loading instead fails
|
||||
with `import.cycle`. No recursive directory walk or infinite link expansion is
|
||||
requested.
|
||||
|
||||
Remediation: identify actual active symlink expansion cycles, not any repeated
|
||||
link occurrence. Preserve ordinary explicit-root access semantics and the
|
||||
separate active-path directory-cycle rule. Add finite-reuse and real-loop
|
||||
regressions for host and link-aware native access; do not replace this defect
|
||||
with an arbitrary new link or path-depth restriction.
|
||||
|
||||
## 5. [ ] P2 — Validate the complete depth token before saturating its value
|
||||
|
||||
Owner: [parseImportDepth](../../internal/engine/import_spec.go), around lines
|
||||
423–430.
|
||||
|
||||
On integer overflow, the parser immediately returns the saturated bound and
|
||||
success. It never validates the remainder of the numeric token, so a sufficiently
|
||||
large fractional or exponent-form number is accepted as an integer depth.
|
||||
|
||||
Public reproduction: load a main document declaring an optional missing
|
||||
directory, `recursive: true`, and either
|
||||
`depth: 999999999999999999999999.5` or
|
||||
`depth: 999999999999999999999999e1`. Both loads succeed. Both declarations should
|
||||
fail `import.invalid`, independently of whether discovery selects files.
|
||||
|
||||
Remediation: validate all characters while saturating the numeric accumulator,
|
||||
or validate integer grammar before conversion. Continue accepting arbitrarily
|
||||
large valid nonnegative integers and numeric `-0`; do not introduce a machine-size
|
||||
maximum. Cover invalid suffixes after the overflow point as well as ordinary
|
||||
invalid depths and valid large integers.
|
||||
|
||||
## 6. [ ] P2 — Apply the active import-depth limit to actual inclusion edges
|
||||
|
||||
Owner: [walkDeclarations](../../internal/engine/imports.go), around lines 403–405.
|
||||
|
||||
The active-edge limit is checked for every declaration before candidate selection.
|
||||
An empty list or absent optional search at the depth boundary is therefore
|
||||
treated as an additional active import edge, even though it loads no source.
|
||||
This strengthens the established active-edge budget into a declaration-depth
|
||||
restriction and conflicts with the selected empty/absent contribution semantics.
|
||||
|
||||
Public reproduction: a main file followed by 64 imported files loads successfully
|
||||
when the final file contributes `end: true` and no imports. Adding either
|
||||
`path: []` or an optional `glob: 'missing/*.yml'` to that last file produces
|
||||
`import.limit_exceeded`. The same 64 actual inclusion edges are present in all
|
||||
three cases.
|
||||
|
||||
Remediation: enforce the existing depth bound when a selected source would add
|
||||
an inclusion edge. Continue validating declaration syntax, policy applicability,
|
||||
and selection resource budgets even when no source is selected. At the boundary,
|
||||
empty lists and optional zero selections must succeed, while a real next source
|
||||
must still fail. Keep the existing numeric resource limit unchanged.
|
||||
|
||||
## Review validation
|
||||
|
||||
Passed on the reviewed revision with Go `go1.26.9 linux/amd64`:
|
||||
|
||||
- `./scripts/check.sh` with the installed Go binary directory first on `PATH`,
|
||||
`GOTOOLCHAIN=local`, `GOPROXY=off`, and a build cache outside the repository.
|
||||
- `go test -race ./...` with the same offline toolchain selection.
|
||||
- `git diff --check aa93aed..15943e2`.
|
||||
|
||||
All six findings were independently reproduced through public APIs using a
|
||||
temporary Go program, in-memory filesystems, and disposable real filesystem
|
||||
fixtures outside the repository. The native wildcard-alias and ordinary item-
|
||||
version regressions were also compared with an archived `aa93aed` checkout
|
||||
outside the repository. No code, dependencies, release procedure, consumer
|
||||
repository, or accepted ADR decision text was changed during the review.
|
||||
Reference in New Issue
Block a user