Import Discovery and Scheduling Review Findings

Status: in progress; issues 1–2 complete, four findings remain.

Reviewed implementation: aa93aed..15943e2, ending at 15943e233ca3cd1286c03c0e7085871db344cd4b, against the accepted roadmap, implementation plan, ADR 0015, and ADR 0016.

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. [x] P1 — Preserve native source identity and relative import bases

Owners: directory selection, especially assignment of target.identity to a file candidate's accessPath around lines 184–194; glob selection, 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.

Completed: native directory and glob discovery retain namespace admission paths, while traversal cycle identities remain resolved. Added public regressions for file and directory aliases across all three selectors, helper/resource origins, source contexts, repeated paths, and distinct alias admission. The regression failed before the fix. Focused discovery/resource tests and ./scripts/check.sh passed with Go 1.26.9 and offline dependency selection.

2. [x] P1 — Scope main-version protection to the actual effective path

Owner: composePendingItems, 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.

Completed: removed the global-rule check against item-local field names; enclosing composition checks retain effective-path protection, including the existing main-item scoped check. New public regressions cover item bodies, helpers, nested items, underlays, and selected main items, and failed before the change. The full everyday check passed, including existing default/enabled source-version and assigned-index ownership regressions.

3. [ ] P2 — Accept valid multi-component native filesystem roots

Owner: resolveNativeImportPath, 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, 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, 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, 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.

11 KiB Raw Blame History Unescape Escape