From 5747ec49c6d7c5baec0c7e53e6d6cdd69bcb20df Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 20 Oct 2026 00:30:35 +0000 Subject: [PATCH] Count import depth only for selected sources --- docs/roadmap/remediation.md | 30 +++++++++++++++++++++-- internal/engine/imports.go | 7 +++--- internal/engine/imports_test.go | 42 +++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 6 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index ab2b5f4..bf61636 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # 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 `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 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. @@ -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 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 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 outside the repository. No code, dependencies, release procedure, consumer 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. diff --git a/internal/engine/imports.go b/internal/engine/imports.go index d4110e7..df6e38a 100644 --- a/internal/engine/imports.go +++ b/internal/engine/imports.go @@ -400,10 +400,6 @@ func (s *importTraversal) walkDeclarations(basePath string, declarations []impor 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)}} } - if depth+1 > s.limits.depth { - location := spec.location - return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the active import-depth limit", &location)} - } policies[index] = policy } for index, spec := range declarations { @@ -478,6 +474,9 @@ func (s *importTraversal) visitPath(candidate selectedImportCandidate, spec impo if !s.access.contains(canonical) { 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) identity := s.access.identity(canonical) diff --git a/internal/engine/imports_test.go b/internal/engine/imports_test.go index f0479ed..7cab458 100644 --- a/internal/engine/imports_test.go +++ b/internal/engine/imports_test.go @@ -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) { directory := t.TempDir() mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: imported.yml}]}\n")