Record error handling audit findings

docs/roadmap/error-handling.md

@@ -0,0 +1,158 @@
+# Error-Handling Audit
+
+Reviewed revision: `5925066d35795b2dc1d31d069a1fb23024a62ab8`.
+Audit date: 2026-10-25.
+
+## Scope And Architecture
+
+The audit traced source construction and discovery, host/native filesystem
+acquisition, import selection, grouped composition, inheritance, typed decoding
+and codecs, application callbacks, secrets bootstrap, diagnostics, inspection,
+and rendering/diff failure paths. It reviewed error classification, propagation,
+partial state, exceptional cleanup, and the root facade's conversions against
+the [configuration contract](../config.md) and existing tests.
+
+The [implemented architecture](../internal/overview.md) separates public wrappers
+from one synchronous private engine lifecycle. The document package provides the
+owned tree; the YAML adapter isolates parser/dumper failures. Engine loading
+collects safe structured diagnostics and returns a nil result on failure; the
+facade translates engine load errors into root-owned `LoadError` values. Secrets
+bootstrap is a separate operation with its own partial report and error type.
+Published inspections read frozen state rather than reloading input.
+
+One new finding warrants remediation. Previously recorded exceptional-path
+defects are cross-referenced below rather than duplicated as new work.
+
+## 1. Directory Read And Close Failures Can Be Misclassified As Absence
+
+Status: open. Priority: medium. Category: filesystem/selection error propagation.
+
+**Relevant code.** `filesystemAccess.readDirectory` in
+[filesystem_access.go](../../internal/engine/filesystem_access.go), lines 144–184,
+returns errors from opening a directory, reading its entries, and closing it
+through the same unqualified error result. `visitBoundedImportDirectory` in
+[directory_walk.go](../../internal/engine/directory_walk.go) forwards that error.
+Both `importDirectoryEntries` in
+[import_selection.go](../../internal/engine/import_selection.go), lines 295–306,
+and `readGlobEntries` in [glob.go](../../internal/engine/glob.go), lines 187–198,
+interpret any resulting `fs.ErrNotExist` as an absent directory and discard its
+entries without a diagnostic.
+
+**Failure condition.** A directory has been successfully inspected and opened,
+but its `ReadDir` fails with an error wrapping `fs.ErrNotExist`, possibly together
+with entries already read. The same issue occurs if enumeration succeeds but
+`Close` returns that classification. A caller-supplied `fs.FS` can legitimately
+encounter such failures during its backing operations. The helper loses the
+distinction between an absent target at lookup/open and a failure after opening.
+
+**Expected behavior.** Preserve the existing
+[absence and requiredness rules](../config.md#explicit-imports) for genuinely
+missing targets, while reporting a safe load failure when enumeration fails
+after opening. A close failure that the directory helper elects to report must
+not become an empty successful selection just because of its error identity.
+This follows the existing distinction between absence and failed acquisition;
+the regular-file helper in
+[source_file.go](../../internal/engine/source_file.go) already restricts its
+`missingTarget` flag to preflight/open failures rather than subsequent reads.
+
+**Actual behavior and consequence.** The failed directory is treated as though
+it contributed no files. Optional loading can succeed with an incomplete
+configuration. Required directory/glob selection can also succeed if another
+directory supplies a match, since requiredness is checked against the surviving
+combined selection. If it was the only directory, the outcome is either a
+successful optional empty selection or a misleading `import.missing` error,
+rather than the acquisition failure.
+
+**Evidence.** A standalone public-API diagnostic used a borrowed filesystem
+backed by `fstest.MapFS` with these files:
+
+```text
+main.yml
+bad/b.yml    # b: 2
+good/a.yml   # a: 1
+```
+
+The main input selected either `dir: [bad, good]` or `glob: '*/*.yml'`, with
+`required: true`. The filesystem kept metadata and file opens working and
+wrapped only the opened `bad` directory. Its wrapper returned an injected error
+either alongside its `ReadDir` entries or from `Close` after enumeration.
+
+For both selectors and both failure phases, `fs.ErrNotExist` produced a
+successful load with `a=1, b=0`. Replacing only the injected error with
+`fs.ErrPermission` failed loading (`import.invalid` for directory search and
+`import.permission` for glob search). All eight small cases were executed; no
+repository test or production code was changed. The filesystem still contained
+the omitted file, so this was not a missing-target fixture.
+
+**Potential remediation.** Preserve failure phase across the shared directory
+boundary, for example with a small private result/failure record distinguishing
+missing-on-open from read/close failure, analogous to `sourceReadFailure`.
+Keep the existing preflight and open-time missing-directory behavior, entry
+budgets, first-error precedence, and caller-owned filesystem lifetime. Map later
+failures to the existing safe traversal diagnostics without exposing raw backing
+error text. Review the standard `fs.ReadDir` fallback as well as the batched
+`fs.ReadDirFile` path. No retry policy, filesystem transaction, rollback service,
+or new restriction on supported filesystems is needed.
+
+## Existing Findings Relevant To Error Handling
+
+These remain open in their original owners; this audit creates no duplicate
+remediation requirement:
+
+- [Correctness finding 1](correctness.md#1-pointer-only-recursive-types-can-overflow-the-stack-or-hang):
+  pointer-only recursive descriptors can turn incompatible input or a helper
+  query into stack exhaustion/nontermination instead of a safe failure.
+- [Correctness finding 2](correctness.md#2-failed-dynamic-schema-construction-leaves-a-reusable-partial-descriptor):
+  failed dynamic descriptor construction leaves partial cache state; a later
+  helper in the same callback can panic instead of returning the recorded load
+  diagnostics. Error-state rollback belongs to that finding.
+- [Correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding)
+  and [resource finding 3](resources.md#3-an-imported-version-check-panic-leaves-the-host-root-open):
+  panic unwinding bypasses callback deactivation and imported host-root cleanup.
+  Application panics should still propagate; neither finding requests recovery
+  or publication of partial results.
+- [Resource findings 1 and 2](resources.md): oversized typed-container allocation
+  can panic before a quota diagnostic, and incomplete history accounting can
+  omit an established limit failure.
+
+## Deliberate Behaviors Preserved
+
+- Raw parser, reader, codec, filesystem, and returned callback error text is
+  intentionally suppressed at the loading boundary. Stable codes, safe messages,
+  semantic paths, and available locations provide context; adding raw error
+  chains would conflict with the [safe-failure contract](../config.md#safe-failures).
+- Version/resolution callbacks that return an error without reporting one receive
+  a generic hook-failed diagnostic. Warnings alone do not suppress that failure.
+  Application callback panics remain programming failures.
+- Read-only regular-file close errors are explicitly ignored by the shared
+  helper. Cleanup failures during an existing primary failure do not need to
+  replace that primary failure. These choices are not findings by themselves.
+- Secrets bootstrap stages reads before writes and documents that an apply
+  failure leaves earlier writes in place, reports the failed entry, and leaves
+  later entries unapplied. That partial-state reporting is intentional, not a
+  missing rollback guarantee.
+- Effective rendering, diagnostic rendering, explanation output, and bootstrap
+  reports propagate writer failures, including short writes; partial output is
+  permitted. Internal snapshot/diff serialization uses safe errors or redacted
+  fallback snapshots rather than exposing internal serialization details.
+- ConfigKit owns no background cancellation, timeout, retry, or shutdown service.
+  Synchronous application readers/callbacks can block; this audit does not invent
+  a cancellation guarantee or move their resource ownership into ConfigKit.
+
+## Validation
+
+The following passed with Go 1.26.10 on Linux amd64, using cached dependencies
+with network/toolchain acquisition disabled:
+
+```sh
+go test ./... -count=1
+go vet ./...
+go test ./internal/engine -run 'Failure|Failures|Error|Errors|Preflight|Propagates|HookOrder' -count=1
+go test ./internal/yaml -run 'Safe|Writer|Malformed' -count=1
+```
+
+The standalone fault-injection diagnostic ran outside the repository. No
+benchmarks, new repository tests, production changes, or native macOS/Windows
+validation were performed. Passing existing tests does not cover the newly
+identified phase-classification case. This report adds no public contract,
+security hardening requirement, or release gate.