Count import depth only for selected sources

This commit is contained in:
2026-10-20 00:30:35 +00:00
parent d3a714a1cb
commit 5747ec49c6
3 changed files with 73 additions and 6 deletions

View File

@@ -1,6 +1,6 @@
# Import Discovery and Scheduling Review Findings # Import Discovery and Scheduling Review Findings
Status: in progress; issues 1–5 complete, one finding remains. Status: complete; all six findings addressed in separate validated commits.
Reviewed implementation: `aa93aed..15943e2`, ending at Reviewed implementation: `aa93aed..15943e2`, ending at
`15943e233ca3cd1286c03c0e7085871db344cd4b`, against the accepted `15943e233ca3cd1286c03c0e7085871db344cd4b`, against the accepted
@@ -177,7 +177,7 @@ suffixes after overflow and retain valid huge integers and `-0`. The two invalid
cases failed before the fix. Existing depth/specification coverage and the full cases failed before the fix. Existing depth/specification coverage and the full
everyday check passed; the accepted integer range was not narrowed. everyday check passed; the accepted integer range was not narrowed.
## 6. [ ] P2 — Apply the active import-depth limit to actual inclusion edges ## 6. [x] P2 — Apply the active import-depth limit to actual inclusion edges
Owner: [walkDeclarations](../../internal/engine/imports.go), around lines 403–405. Owner: [walkDeclarations](../../internal/engine/imports.go), around lines 403–405.
@@ -199,6 +199,13 @@ 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 empty lists and optional zero selections must succeed, while a real next source
must still fail. Keep the existing numeric resource limit unchanged. must still fail. Keep the existing numeric resource limit unchanged.
Completed: the depth bound is checked for an existing selected target at source
inclusion, rather than for every declaration. A test-controlled one-edge budget
covers empty/required-empty lists, absent path/directory/glob selections, actual
next sources, and invalid declarations. The original 64-edge public examples
now succeed with empty optional contributions; a real next edge still fails.
The full everyday and race checks passed without changing the resource limit.
## Review validation ## Review validation
Passed on the reviewed revision with Go `go1.26.9 linux/amd64`: Passed on the reviewed revision with Go `go1.26.9 linux/amd64`:
@@ -214,3 +221,22 @@ fixtures outside the repository. The native wildcard-alias and ordinary item-
version regressions were also compared with an archived `aa93aed` checkout version regressions were also compared with an archived `aa93aed` checkout
outside the repository. No code, dependencies, release procedure, consumer outside the repository. No code, dependencies, release procedure, consumer
repository, or accepted ADR decision text was changed during the review. repository, or accepted ADR decision text was changed during the review.
## Remediation validation
All six findings have completion evidence above. Final validation passed with
Go `go1.26.9 linux/amd64`, local toolchain selection, offline dependencies, and
build caches outside the repository:
- `./scripts/check.sh` (formatting, whitespace, full tests, vet, and build).
- `go test -race ./...`.
- Local documentation-link checks and `git diff --check`.
The original external public-API reproductions were rerun against the completed
fixes. Native path/directory/glob aliases now retain the same helper origin and
resource context; ordinary item versions load; nested native roots and finite
symlink reuse load; invalid depth suffixes fail; and empty contributions do not
consume active inclusion depth. Focused regressions also retain actual version
ownership, real cycle/escape errors, invalid-declaration handling, and the depth
budget for real sources. No dependencies, public controls, release gates, or
downstream repositories were changed.

View File

@@ -400,10 +400,6 @@ func (s *importTraversal) walkDeclarations(basePath string, declarations []impor
site := spec.fieldSites["placement"] site := spec.fieldSites["placement"]
return []Diagnostic{{Code: CodeImportInvalid, Severity: SeverityError, Path: site.path.clone(), Message: "sequence-item placement requires a nonempty destination", Primary: cloneLocation(&site.location)}} return []Diagnostic{{Code: CodeImportInvalid, Severity: SeverityError, Path: site.path.clone(), Message: "sequence-item placement requires a nonempty destination", Primary: cloneLocation(&site.location)}}
} }
if depth+1 > s.limits.depth {
location := spec.location
return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the active import-depth limit", &location)}
}
policies[index] = policy policies[index] = policy
} }
for index, spec := range declarations { for index, spec := range declarations {
@@ -478,6 +474,9 @@ func (s *importTraversal) visitPath(candidate selectedImportCandidate, spec impo
if !s.access.contains(canonical) { if !s.access.contains(canonical) {
return []Diagnostic{importDiagnostic(CodeImportOutsideRoot, "import target resolves outside the configuration root", &location)} return []Diagnostic{importDiagnostic(CodeImportOutsideRoot, "import target resolves outside the configuration root", &location)}
} }
if depth > s.limits.depth {
return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the active import-depth limit", &location)}
}
currentRoute := s.currentRoute(location) currentRoute := s.currentRoute(location)
identity := s.access.identity(canonical) identity := s.access.identity(canonical)

View File

@@ -1149,6 +1149,48 @@ func TestImportTraversalEnforcesPrivateBudgets(t *testing.T) {
} }
} }
func TestImportDepthBudgetCountsSelectedEdges(t *testing.T) {
for _, test := range []struct {
name, declaration string
code Code
}{
{name: "no declaration"},
{name: "empty paths", declaration: "path: []"},
{name: "required empty paths", declaration: "path: []\n required: true"},
{name: "missing path", declaration: "path: absent.yml"},
{name: "missing directory", declaration: "dir: absent"},
{name: "missing glob", declaration: "glob: 'absent/*.yml'"},
{name: "selected path", declaration: "path: parts/next.yml", code: CodeImportLimitExceeded},
{name: "selected directory", declaration: "dir: parts", code: CodeImportLimitExceeded},
{name: "selected glob", declaration: "glob: 'parts/*.yml'", code: CodeImportLimitExceeded},
{name: "invalid empty declaration", declaration: "dir: []\n depth: 1", code: CodeImportInvalid},
{name: "invalid absent glob", declaration: "glob: '['", code: CodeImportInvalid},
} {
t.Run(test.name, func(t *testing.T) {
directory := t.TempDir()
mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: leaf.yml}]}\n")
leaf := "end: true\n"
if test.declaration != "" {
leaf += "configkit:\n imports:\n - " + test.declaration + "\n"
}
writeImportFile(t, directory, "leaf.yml", leaf)
writeImportFile(t, directory, "parts/next.yml", "next: true\n")
limits := defaultImportLimits()
limits.depth = 1
composed, sources, diagnostics := loadImportedDocuments(File(mainPath), nil, parseImportMain(t, mainPath), nil, limits)
if test.code != "" {
if len(diagnostics) != 1 || diagnostics[0].Code != test.code {
t.Fatalf("diagnostics = %#v, want %s", diagnostics, test.code)
}
return
}
if len(diagnostics) != 0 || composed == nil || len(sources) != 2 {
t.Fatalf("depth-boundary composition: sources=%#v diagnostics=%#v", sources, diagnostics)
}
})
}
}
func TestImportTraversalAcceptsExactPrivateBudgets(t *testing.T) { func TestImportTraversalAcceptsExactPrivateBudgets(t *testing.T) {
directory := t.TempDir() directory := t.TempDir()
mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: imported.yml}]}\n") mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: imported.yml}]}\n")