diff --git a/docs/development.md b/docs/development.md index 2909376..ea408c0 100644 --- a/docs/development.md +++ b/docs/development.md @@ -43,7 +43,7 @@ public behavior. | Shared source-version checks | [Configuration contract](config.md#shared-source-version-checks), [ADR 0013](adr/0013-shared-source-version-checks.md), and the version/source-version tests below | | Documentation changes | The owning document | | Licensing, dependencies, attribution, or release-document changes | [Project grant](../COPYRIGHT) and [attribution](oss/attribution.md); release changes also follow the release procedure | -| Architecture decisions or future work | [Future features](roadmap/future.md) and relevant ADRs listed below | +| Architecture decisions or future work | [Future features](roadmap/future.md), [consolidated audit review](roadmap/audit.md), and relevant ADRs listed below | | Code and test changes | The owning contract and tests from the map below | | Examples or copyable assets | The owning implemented contract and [consumer adoption guide](consumers/README.md) | | Release preparation | [Release procedure](release.md) | @@ -199,6 +199,9 @@ git show 34cbc0d:docs/roadmap/remediation.md ``` Potential enhancements are tracked in [future features](roadmap/future.md). +The [consolidated audit review](roadmap/audit.md) owns the open findings from +the nine 2026-10-25 audits, their evidence, and scope questions. The original +reports are preserved at `ecd5d5d` under `docs/roadmap/`. Install the Go version required by `go.mod` or a compatible newer toolchain. Acquire declared dependencies explicitly before entering an offline diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..1478031 --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,926 @@ +# Consolidated Audit Review + +Status: review pending; implementation scope has not been selected. +Consolidated: 2026-10-25. Baseline: `ecd5d5d`. + +This document consolidates nine sequential audits for maintainer review. It owns +their open findings, supporting evidence, proposed remedies, and relationships. +It is not an implementation plan, an acceptance of every recommendation, or a +new set of release gates. The [configuration contract](../config.md), public Go +declarations, and [architecture policy](../policy/architecture.md) continue to +own behavior and guarantees. + +There are nine distinct reported defects: four from correctness, three from +resources, one from error handling, and one from security. Concurrency adds +evidence for an existing callback-lifetime defect. Other entries are cleanup, +efficiency, maintainability, or test improvements. Regression recommendations +are attached to the defects they protect rather than counted as additional +product fixes. + +The 35 original numbered findings map to 31 review entries after deduplication +and integration of related test work. All entries remain open and unselected. +Their medium/low priorities retain the original assessments; those labels do not +imply severity certification or implementation order. IDs remain stable as work +is selected or completed. + +## Evidence And Provenance + +All nine audits were dated 2026-10-25. Their sequential commits changed only the +audit documents, so their production baseline is the same. This consolidation +does not independently reproduce the findings or claim new runtime evidence. + +| Original report | Reviewed revision | Evidence contributed | +| --- | --- | --- | +| `correctness.md` | `47944d2` | Source review and standalone public-API reproductions of four defects. | +| `efficiency.md` | `5098b56` | Source-derived scaling cases and proposed algorithm changes; no benchmarks. | +| `complexity.md` | `320d0c5` | Responsibility/caller review, graph complexity estimates, and maintainability candidates. | +| `concurrency.md` | `a802734` | Ownership review, race checks, and corroboration of callback lifetime after panic. | +| `resources.md` | `0cca633` | Bounded allocation, retained-node, and host-handle diagnostics. | +| `error-handling.md` | `5925066` | Directory read/close fault injection through the public API. | +| `security.md` | `b30c9f1` | Trust-boundary review, bounded symlink operation counts, and a historical dependency spot-check. | +| `testing.md` | `eb5a82f` | Aggregate statement coverage, assertion review, and regression/test-cleanup recommendations. | +| `deadcode.md` | `b1c278b` | Type-aware reference inventory, graph/caller checks, and source review of private cleanup candidates. | + +The full original reports, including exact probes, commands, source line +references, and validation limitations, are preserved at `ecd5d5d`. For example: + +```sh +git show ecd5d5d:docs/roadmap/correctness.md +git show ecd5d5d:docs/roadmap/security.md +``` + +The [original-finding map](#original-finding-map) accounts for every numbered +finding, including duplicates and test recommendations integrated elsewhere. +Source links below identify implementation owners; historical line numbers are +left in the originals because later edits will move them. + +### Recorded Validation And Its Limits + +The audits report successful development checks, ordinary tests, full race tests, +`go vet`, and focused suites as specified in their individual records, using +Go 1.26.10 on Linux amd64. Concurrency additionally repeated six relevant engine +tests with race instrumentation, count 20, and CPUs 1, 4, and 8. Testing ran the +suite with shuffle enabled three times. These results did not detect the newly +reported defects and are not evidence that those defects have been fixed. + +Testing recorded 771 ordinary test functions, three fuzz targets, and 29 executable +examples. Aggregate cross-package coverage was 90.3%: root 81.3%, engine 91.0%, +document 100%, YAML 89.9%, and maintained consumers 79.2%. Those figures locate +gaps; they are not targets or a measure of assertion strength. Only checked-in +fuzz seeds ran, not an extended campaign or mutation analysis. + +Standalone diagnostics stayed outside the repository. Some potentially fatal +cases used disposable processes, small limits, or operation ceilings; larger +scaling examples were derived rather than executed. The dead-code inventory +considered Linux, Darwin, and Windows build configurations, but no audit reports +native macOS or Windows execution. Graph scores and reachability results were +triage aids, not proof that a function is unnecessary or a numerical refactoring +threshold. Cached analysis tooling was used outside the repository; no new +project tools or dependencies were introduced. + +The security report's module-path advisory spot-check and successful module +integrity verification are historical evidence only. It recorded no listed +advisories for the selected YAML v4, ReleaseKit, or Goldmark v2 module paths, and +recorded the selected `x/mod` version as fixed for the two reports it found. It +did not perform complete toolchain/transitive reachability analysis or supply- +chain attestation; dedicated vulnerability, security, and secret scanners were +unavailable. This consolidation neither refreshes nor broadens that conclusion. + +## Review Priorities And Dependencies + +Recommended review order is reported defects with their regression coverage, +bounded cleanup, targeted efficiency changes, and then broader structural +refactoring. Small prerequisite cleanups can accompany a defect fix. The order +of the groups below is for navigation, not an execution sequence. + +| Relationship | Consequence for scope selection | +| --- | --- | +| A02 and A05 | Make descriptor construction safe before reusing successful dynamic-type eligibility results. | +| B01 and B02 | Panic cleanup must close callback state and the owned host root independently. Neither fix substitutes for the other. | +| C01 and its filesystem tests | Strengthen error/handle assertions while adding failure-phase regressions, using one shared test owner. | +| C02 and C03 | Link expansion correctness is independent of optional glob readability extraction. Do not make the latter a prerequisite. | +| D01, D02, and D03 | Remove confirmed unused copies where selected, then account for the complete retained graph. Removing one unused tree does not fix the remaining omissions. | +| E01, E02, and E03 | Consider policy-state costs and responsibility boundaries together, but preserve separate scaling and behavioral acceptance cases. | +| F01 and F02 | Removing the test-only acquisition mode does not by itself simplify active lifecycle handoffs. | +| F04, F09, and F12 | Replace useful facade ownership/query coverage before deleting disconnected or representation-coupled scaffolding. | +| F05 and F07 | Removing unused placement bridges is a bounded cleanup independent of the wider transport refactor. | + +## A. Schema And Typed Values + +### A01. Terminate Pointer-Only Recursive Descriptor Traversal + +Type: reported defect. Priority: medium. Source: correctness 1; testing 1. + +**Problem and evidence.** In [schema.go](../../internal/engine/schema.go), a legal +type such as `type Loop *Loop` produces a pointer descriptor whose element is +itself. [decode.go](../../internal/engine/decode.go) unwraps that descriptor +without consuming a non-null input node; `resolveSchemaPath` likewise assumes +pointer traversal eventually reaches another kind. A public load with a `Loop` +field and `value: {}` overflowed the stack in a process with a small stack limit. +`value: null` succeeded, but resolving a normalization path for that field hung +until the diagnostic process was stopped. The [schema +contract](../config.md#application-schema-binding) permits recursive types with +acyclic values; the inline-field restriction is not a blanket pointer restriction. + +**Recommendation and acceptance.** Detect repeated descriptors while unwrapping +pointer-only chains in decoding and schema-path resolution, and review analogous +helpers. Incompatible non-null input must fail safely; path traversal must +terminate and reject unaddressable descendants. Preserve supported nil/null +values and ordinary recursive structs. Do not add a blanket schema rejection or +arbitrary depth cap. Add bounded regression coverage for both decoding and helper +path resolution, keeping fatal/hanging probes out of the default test process. + +### A02. Make Failed Dynamic Descriptor Construction Transactional + +Type: reported defect. Priority: medium. Source: correctness 2; testing 1. + +**Problem and evidence.** `dynamicDescriptor` in +[value_operation.go](../../internal/engine/value_operation.go) passes the load's +descriptor cache to `schemaBuilder`. Failed construction leaves cached container +placeholders. For a dynamic struct containing an unsupported channel field, two +`Normalize` calls in one resolver, first with a typed nil pointer and then a +non-nil pointer to that type, reproduced a nil-pointer panic on the second call. +The first call reported a diagnostic but left a reusable partial descriptor. +The application callback itself did not panic. + +**Recommendation and acceptance.** Stage new descriptors and commit only on +successful construction, or roll back all additions from a failed build. Include +recursive references among newly built descriptors; clearing only the immediate +failed container or adding a projection nil guard is insufficient. Preserve +previously valid descriptors, load-local ownership, and safe path-specific +diagnostics under [typed projection](../config.md#typed-projection-and-ownership) +and [resolution](../config.md#recorded-application-resolution). Test repeated +operations after a failed build in the same load. Coordinate A05 only after this +failure-state boundary is sound. + +### A03. Distinguish Acyclic Slice Views In Cycle Detection + +Type: reported defect. Priority: medium. Source: correctness 3; testing 1. + +**Problem and evidence.** `valueVisit` / `visitFor` in +[value_operation.go](../../internal/engine/value_operation.go) identify slices by +type, kind, and starting pointer, omitting length. The finite value built by +`shared := []any{"leaf", nil}; shared[1] = shared[:1]` was rejected through mapping +sources, typed defaults, and CLI overrides, while an equivalent independently +allocated value succeeded. The nested view contains only the leaf, not its +enclosing view. The same helper serves projection/cloning and +[value.go](../../internal/engine/value.go). + +**Recommendation and acceptance.** Include visible slice length in active-visit +identity, or use an equivalent distinction. Preserve rejection of actual self +and mutual cycles, nil/empty behavior, and detached output; preserving alias +identity is not required. Regressions should cover the shared source, override, +projection, and cloning paths under the existing supported-acyclic-value contract, +alongside true-cycle rejection. + +### A04. Enforce Existing Node Budgets Before Container Reservations + +Type: reported defect. Priority: medium. Source: resources 1; testing 1. + +**Problem and evidence.** `projectSequence` and `projectMap` in +[projection.go](../../internal/engine/projection.go) reserve space for the full +container, and maps collect all keys, before child charging in +[value_operation.go](../../internal/engine/value_operation.go). Defaults containing +a maximum-length slice of zero-sized structs caused a `makeslice: cap out of range` +panic instead of `schema.projection_limit`; that diagnostic did not allocate a +huge input backing array. Less extreme cases can allocate excessive temporary +memory before rejection. Application resolution can reach these paths too. + +**Recommendation and acceptance.** Check the known minimum node cost against the +remaining existing allowance with overflow-safe arithmetic, or bound initial +capacity and grow as charging succeeds. Review map reservation/key collection as +well as sequences. Preserve node/depth semantics, inline handling, and useful +diagnostic paths. Use bounded regressions, including small controlled allowances, +without enormous allocations. This enforces an existing budget, not a new length +cap or universal heap-size guarantee. + +### A05. Reuse Successful Dynamic-Type Eligibility Checks Locally + +Type: optimization candidate. Priority: low. Source: efficiency 6. + +**Problem and evidence.** Each dynamic value in projection/cloning invokes +`descriptorContainsSecret` with a new visited map, even when its descriptor is +cached. N values of one concrete type with descriptor-graph size S therefore cost +O(N × S) eligibility work. N interface values containing nil pointers to one wide +supported struct can produce only N null values while repeating the full type +walk. This is a source-derived bound, not measured runtime. + +**Recommendation and acceptance.** After A02, consider remembering successfully +approved concrete descriptors within `valueOperation`, reducing repeated-type +eligibility work to O(S + N). Cache only completed successful eligibility checks, +not traversal-local visits, partial descriptors, or path-bearing failures. +Preserve rejection of dynamic secret-reference types, including nil pointers and +recursive descriptor cases. This does not select cross-load schema caching or a +clone traversal redesign. + +## B. Callback And Filesystem Lifetime + +### B01. Deactivate Callback Contexts During Panic Unwinding + +Type: reported defect. Priority: low. Sources: correctness 4, concurrency 1; +testing 1; corroborated by error-handling and security reviews. + +**Problem and evidence.** `runResolution` in +[resolution.go](../../internal/engine/resolution.go) and `runValidation` / +`reportVersion` in [validation.go](../../internal/engine/validation.go) deactivate +contexts only after normal callback return. Public probes retained the context, +panicked, recovered outside `Load`, and then successfully queried resolution or +wrote validation/version diagnostics. The concurrency audit reproduced this +with race instrumentation without reporting a data race. The defect violates +callback lifetime, not a newly supported concurrent-use scenario. + +**Recommendation and acceptance.** Defer deactivation within each callback +invocation, closing immediately at normal return and on unwinding, before later +projection or metadata work. Preserve the original panic and prohibit retained +context use through the established closed-context behavior. Test all three +callback kinds with external recovery. No recovery service, lock, atomic flag, +or broader callback thread-safety guarantee is needed. B02 separately owns root +handle cleanup. + +### B02. Close The Owned Import Root When A Version Callback Panics + +Type: reported defect. Priority: medium. Source: resources 3; testing 1. + +**Problem and evidence.** Host import acquisition in +[import_acquisition.go](../../internal/engine/import_acquisition.go) opens an +`os.Root` through [filesystem_access.go](../../internal/engine/filesystem_access.go) +but relies on explicit ordinary-path closes. An imported version-check panic +bypasses them. A disposable Linux probe disabled GC, recovered outside `Load`, +and observed one remaining root descriptor through `/proc/self/fd`; the normal +control returned to zero. The imported file's own deferred close still ran. + +**Recommendation and acceptance.** Install deferred fallback cleanup at the +owning boundary, preserving explicit normal close timing and diagnostics, as +selected-overlay acquisition already does. Avoid double-close confusion, leave +borrowed filesystems untouched, and propagate the original panic. A regression +must observe the owned host root, not merely a native file-close counter, and +document platform prerequisites. Context deactivation in B01 is a separate +requirement. This restores load-scope cleanup without adding panic recovery. + +## C. Import Selection And Link Resolution + +### C01. Preserve Directory Failure Phase And Strengthen Its Tests + +Type: reported defect with test improvements. Priority: medium. +Sources: error-handling 1, testing 3. + +**Problem and evidence.** `filesystemAccess.readDirectory` in +[filesystem_access.go](../../internal/engine/filesystem_access.go) returns open, +read, and close errors through one unqualified error channel. +[import_selection.go](../../internal/engine/import_selection.go) and +[glob.go](../../internal/engine/glob.go) treat any `fs.ErrNotExist` as absence. +A borrowed filesystem probe selected a failing directory plus a good directory, +using both `dir` and `glob` with `required: true`. Injecting `fs.ErrNotExist` +alongside entries from `ReadDir`, or from `Close`, let loads succeed with the bad +directory's contribution omitted. Permission-classified controls failed. All +eight selector/phase/error cases ran. This violates the existing distinction +between optional absence and failed acquisition. + +**Recommendation and acceptance.** Preserve missing-on-open versus post-open +failure, for example with a small private failure record analogous to regular +file acquisition. Keep genuine missing-target rules, entry budgets, first-error +precedence, borrowed filesystem ownership, and safe existing traversal diagnostics. +Review both batched `fs.ReadDirFile` and standard fallback paths. A successful +second directory must not hide enumeration/close failure; raw backing error text +must not leak. No retries or filesystem transaction are proposed. + +**Test improvements.** The shared fixture in +[native_filesystem_test.go](../../internal/engine/native_filesystem_test.go) and +[resource_context_test.go](../../internal/engine/resource_context_test.go) currently +checks aggregate closes and often only non-nil errors. One leaked handle and +one double-close can cancel out. Its canary check examines the terse error +summary, not structured diagnostics. Track per-handle closure, assert nil result, +diagnostic code and source, and inspect diagnostic messages/rendering for canaries. +Add entries/data-plus-error and close-phase cases through narrow failure seams. +Preserve intentionally ignored read-only regular-file close errors; this does +not impose a universal close-error policy. + +### C02. Avoid Reexpanding Completed Finite Symlink Dependencies + +Type: reported availability defect. Priority: medium. Source: security 1; +testing 1. + +**Problem and evidence.** Host/native resolvers in +[filesystem_links.go](../../internal/engine/filesystem_links.go) track active links +but reuse no completed resolution. A finite contained graph `s00 -> .`, +`s01 -> s00/s00`, `s02 -> s01/s01`, and so on repeats completed dependencies. +Its work follows T(N) = 2T(N-1) + O(1), despite linear graph size. Directory +selection resolves discovered links before deciding on recursive descent, so +`recursive: false` does not avoid it. Existing source/entry/node/import-depth +budgets do not account for the repeated expansion. + +A real-link fixture exposed through a native `fs.ReadLinkFS` wrapper recorded +32, 128, 512, 2,048, and 8,192 `Lstat` calls for 3, 5, 7, 9, and 11 entries; +corresponding `ReadLink` counts were 11, 57, 247, 1,013, and 4,083. Thirteen +entries reached a diagnostic ceiling of 12,000 `Lstat` calls. With following +disabled, that fixture used 17 `Lstat` calls and no `ReadLink`. The host resolver's +matching logic was reviewed, not separately operation-count tested. + +**Recommendation and acceptance.** Reuse fully resolved targets within the +resolution operation, keyed for the rooted namespace and full link path, or use +equivalent dependency evaluation. Retain active-path cycle detection, containment, +finite reuse, relative/absolute host semantics, and the stable-input assumption. +A global visited-means-cycle rule would incorrectly reject valid inputs. Cover +both resolvers and directory/glob entry points with small fixtures and bounded +operation observation, not timing thresholds or exhaustion tests. No arbitrary +link cap, public cache, or default-follow change is selected. + +The availability risk requires influence over imported filesystem data; it is +not an anonymous remote vulnerability by itself. No concurrent mutation or +outside-root target is needed, and accidental graphs can cause the same work. + +### C03. Separate Glob Entry Preparation From Traversal + +Type: optional refactor. Priority: medium. Source: complexity 4. + +**Problem and evidence.** The patterned branch of `selectGlobImportCandidates` +in [glob.go](../../internal/engine/glob.go) interleaves component traversal and +union/sorting with hidden/match rules, lexical/access paths, containment, links, +directory eligibility, namespace differences, labels, and candidate construction. +Directory selection already has a named entry-preparation operation. + +**Recommendation and acceptance.** Consider a glob-specific preparation helper +returning an admitted candidate, ordinary skip, or diagnostic. Keep literal +segments, component advancement, union/sorting, exclusions, and requiredness in +the selector. Preserve inspection/error order, budgets, native spelling, and +selected-file validation. Share low-level facts only where semantics agree; +no general visitor framework, discovery-algorithm change, or caching is implied. +C01/C02 can be fixed independently of this readability change. + +## D. Retained History And Unused Evidence + +### D01. Account For The Complete Published History Graph + +Type: reported defect. Priority: medium. Source: resources 2; testing 1. + +**Problem and evidence.** The checkpoint in [load.go](../../internal/engine/load.go) +and [history.go](../../internal/engine/history.go) counts a different root set +from [load_result.go](../../internal/engine/load_result.go) publication. Separately +cloned post-CLI input, `structuralInput`, `preCLI`, and later inheritance copies +are not all represented. A read-only diagnostic of a small successful load with +a no-op resolver counted 14 distinct nodes before including the two published +replay roots and 22 with them. Source inspection found that the checkpoint uses +a different, same-sized decoded-final instance and also counts 14 in this case. +Generalized to N scalar items, the reported counts were 3N + 8 versus 5N + 12; +800,000 items would pass the checkpoint yet exceed the existing 4,000,000-node +history allowance. That large case was derived, not allocated or executed. + +**Recommendation and acceptance.** Use pointer-aware accounting for every document +root actually retained at publication, including replay and copied inheritance +history, counting shared nodes once. Where practical, reserve/charge copies before +allocation. Coordinate removal of unused trees under D03 and recalculate the +retained set afterward; that removal alone does not establish complete accounting. +Reuse already-owned immutable snapshots only where equivalence and ownership +are demonstrated. This need not redesign public metadata copying. + +Use a small controlled allowance to test publication integration and the existing +`explain.limit_exceeded` outcome, as well as count-once sharing. Helper-only tests +cannot detect roots omitted by callers. Preserve [historical +inspection](../config.md#historical-value-explanation), detached ownership, and +the existing quota; no smaller limit or universal heap-size bound is proposed. + +### D02. Remove The Unconsumed Override-Result Pre-CLI Copy + +Type: cleanup. Priority: medium. Source: deadcode 1. + +**Problem and evidence.** `decodedOverrideResult.preCLI` in +[override_decode.go](../../internal/engine/override_decode.go) clones the entire +decoded final tree even without CLI assignments. Its readers are private tests; +production independently clones `before.final` into the active context in +[override_context.go](../../internal/engine/override_context.go). + +**Recommendation and acceptance.** Remove that result member, its clone, and +assertions solely about it. Preserve the independently owned pre-CLI context +that actually feeds resolution, publication, and explanation. Retain behavioral +CLI/history/ownership coverage. The change removes a redundant traversal, not +the active replay snapshot or its detachment boundary. + +### D03. Remove The Unqueried Published Structural-Input Copy + +Type: cleanup. Priority: medium. Source: deadcode 2. + +**Problem and evidence.** `Inspection.structuralInput` in +[load.go](../../internal/engine/load.go) is cloned and indexed at publication, +but has no production reader. [explain.go](../../internal/engine/explain.go) +uses source/inheritance replay, `preCLI`, and `input`. The immutable lookup is +addressed by supplied node pointers; indexing this otherwise unused root does +not make it useful and keeps its mappings alive. + +**Recommendation and acceptance.** Remove the inspection member, final clone, +and its `retainedTrees` inclusion. Keep earlier `resolved.structuralInput` uses +for path finalization and sensitivity. Check explanation, historical paths, +redaction, and ownership through active outputs. Coordinate D01 for all remaining +retained roots; do not infer that structural evidence is globally unused. + +### D04. Stop Constructing Unconsumed Combination Evidence + +Type: cleanup. Priority: medium. Source: deadcode 5. + +**Problem and evidence.** `sequenceCombinationResult.discardedLower` in +[sequence_combine.go](../../internal/engine/sequence_combine.go) allocates a list +of all replaced lower indexes with no production reader. Three tests inspect it. +`inheritanceReplacement.path`, `previous`, and `effective` in +[inheritance.go](../../internal/engine/inheritance.go) are also unread, but their +writers perform path/origin copies and displaced-subtree walks. Active inheritance +history uses operations and projected changes elsewhere. Replacement `before` +and `after` trees still feed node accounting. + +**Recommendation and acceptance.** Remove only unread members and work solely +feeding them. Keep replacement trees, item projections, replacement mappings, +`appendedHigher`, active provenance/history, and node accounting. Keep +`sequenceIndexes` for append and `displacedOrigins` for overlay callers. Replace +private unused-evidence assertions with existing behavior-level replacement, +removed-descendant, and history protection where necessary. Coordinate D01 and +E03 without deleting active evidence merely because another representation exists. + +## E. Collection Processing And Efficiency + +All complexity estimates here describe identified internal work with bounded- +depth examples and expected hash lookup costs. Actual path processing, copied +payload, and expanded output remain costs; no end-to-end complexity or timing +guarantee is proposed. + +### E01. Avoid Scanning Every Policy For Each Replacement + +Type: optimization candidate. Priority: medium. Source: efficiency 1. + +**Problem and evidence.** `retireCollectionPoliciesAtOrBelow` in +[collection_policy_state.go](../../internal/engine/collection_policy_state.go) +scans all P policies for each of Q ordinary replacements in +[overlay.go](../../internal/engine/overlay.go), even if the changed scalars are +unrelated to every collection. P = Q = N gives O(N²) visits for linear-sized +input and output within one tier. + +**Recommendation and acceptance.** Consider an operation-local semantic-path +index with descendant queries, maintained as policies change, or batch retirement +where timing remains equivalent. The disjoint case can approach O(P + Q), with +additional work charged to affected entries/path lengths. Preserve explicit-mode +persistence, record ownership, and the moment old identity retires; delayed +retirement must not clear newly established higher-tier policy. Evaluate existing +inheritance indexing without requiring identical interfaces. Retain a distinct +disjoint-replacement case even if implementation is coordinated with E02/E03. + +### E02. Avoid Recopying Unrelated Policy Across Small Tiers + +Type: optimization candidate. Priority: medium. Source: efficiency 2. + +**Problem and evidence.** `lowerCollectionPoliciesForClaims` in +[collection_retirement.go](../../internal/engine/collection_retirement.go) clones +and scans lower policy, and `resolveStrictCollectionPolicies` in +[compose.go](../../internal/engine/compose.go) clones it again. Overlay preparation +repeats full-state work and checks retained destinations. P initial policies +followed by T scalar-only tiers cost O(T × P) work/allocation although only O(P + T) +material exists. Copies include record order and nested record policy; these +are transient private copies, not independently published snapshots. + +**Recommendation and acceptance.** Separate read-only lower-policy lookup from +owned current-tier changes, localize work to incoming claims/declarations, and +apply changes to the assembly owner. Keep copies at actual mutable ownership +boundaries. Removing the immediate duplicate copy helps but does not alone fix +O(T × P) growth. Preserve pending declarations, replacement eligibility, and +independent nested record policies. This case remains distinct from E01 and the +deferred public metadata-copy experiment. + +### E03. Clarify Policy Preparation And Merge Evidence Application + +Type: optional refactor. Priority: medium. Source: complexity 3. + +**Problem and evidence.** [overlay.go](../../internal/engine/overlay.go) combines +claim/declaration preparation, operation resolution, callbacks, diagnostics, +origins/history, retirement, record policy rebuilding, and declaration-only +finalization. Overlay and [inheritance](../../internal/engine/inheritance_collection.go) +separately translate sequence results into history descriptors and reconstruct +record order. `combineSequence` already owns shared value combination. + +**Recommendation and acceptance.** Consider explicit preparation, application, +and finalization operations with a small result/owner for evidence and diagnostics. +Share straightforward history-descriptor construction; assess projected-record +selection sharing while keeping precedence and inheritance policy ownership +explicit. Do not unify distinct origin or mode-lifetime rules behind a generic +strategy framework. Coordinate E01/E02 so extraction does not preserve avoidable +full-table work. Preserve pending declarations, replacement ordering, retained +versus incoming record owners, and stable history. D04 can remove unused evidence +without requiring this broader refactor. + +### E04. Index Tier-History Projection Lookups + +Type: optimization candidate. Priority: medium. Source: efficiency 3. + +**Problem and evidence.** `projectTierHistoryApplications` in +[sequence_history_projection.go](../../internal/engine/sequence_history_projection.go) +calls unindexed `documentAtPath` for each mapped sequence. N sibling collections +selected by one wildcard identity registration incur O(N²) mapping comparisons +while projecting linear-sized source/history material. + +**Recommendation and acceptance.** Lazily prepare one read-only `documentLookup` +per original application value with mapped contributions, or collect needed nodes +in one traversal. With V nodes, S contributions, depth D, and I projected items, +that portion can be O(V + S × D + I). Preserve authored coordinates, sparse item +projections, replacement evidence, and independent output storage. This is load- +time coordinate projection, not branch-local explanation replay. + +### E05. Restore Compound Identity Without Repeated Mapping Scans + +Type: optimization candidate. Priority: medium. Source: efficiency 4. + +**Problem and evidence.** `preserveInheritanceIdentityFields` in +[inheritance_scope.go](../../internal/engine/inheritance_scope.go) searches child +and merged mappings for each identity field. K fields and C/M child/merged +entries cost O(K × (C + M)) comparisons plus required clones. A wide compound +identity can make this quadratic without expanding record output. + +**Recommendation and acceptance.** Index merged positions and walk child entries +against an identity-field set, or use equivalent local mapping indexes, for +O(K + C + M) lookup work plus copies. Preserve child-owned identity values and +locations, mapping order, and detached nodes. Do not change identity equality or +parent selection. Removing the restoration step instead requires a separate +equivalence review; apparent duplication alone is insufficient. + +## F. Remaining Maintenance And Test Improvements + +### F01. Remove The Test-Only Eager Acquisition Mode + +Type: cleanup. Priority: medium. Sources: complexity 1, deadcode 8. + +**Problem and evidence.** Both production constructors of the request declared in +[acquisition.go](../../internal/engine/acquisition.go) set +`inputAcquisitionRequest.deferAssembly` to true. Only private +test helpers select the eager composition branch in +[import_acquisition.go](../../internal/engine/import_acquisition.go). Acquisition +maintains two return protocols and tests can bypass the production phase handoff. + +**Recommendation and acceptance.** Return retained scopes consistently and remove +the unused production mode. Tests needing composition should invoke the active +composition phase through a small test-only helper; acquisition-only tests should +remain acquisition-only. Preserve closure, pending-item reservation, accounting, +diagnostics, and meaningful composition coverage. This does not retire public +loading workflows or imply that every nil-state/directive context is unused. + +### F02. Clarify Active Acquisition And Coordinator Handoffs + +Type: optional refactor. Priority: medium. Source: complexity 2. + +**Problem and evidence.** `acquireInput` in +[acquisition.go](../../internal/engine/acquisition.go) combines discovery, parsing, +directive/version branches, placement/accounting, dispatch, and passive contexts. +Version branches repeat source-ID/failure/result handling, and layered processing +extracts and restores directives. [load.go](../../internal/engine/load.go) contains +a substantial ordinary-load context fallback; [load_request.go](../../internal/engine/load_request.go) +repeats tier declaration/application/projection handoffs. Parsed, local, composed, +inherited, and version-path representations are difficult to follow together. + +**Recommendation and acceptance.** Consider cohesive parsed-input directive/version +processing and standalone placement/registration operations, plus explicit context, +structural-expansion, and repeated tier handoffs. Reuse existing state/results; +helpers that merely share the same mutable surrounding locals do not resolve +ownership. Preserve visible phase order, ordinary/layered overlay differences, +default/enabled version timing, warnings, exactly-once callbacks, and load-wide +versus tier-local state. F01 removes a different, inactive protocol; neither task +requires a new context hierarchy or public request model. + +### F03. Retire The Parallel Test-Only Import Directive Parser + +Type: cleanup. Priority: low. Source: deadcode 7. + +**Problem and evidence.** `parseImportSpecificationDirective` in +[import_spec.go](../../internal/engine/import_spec.go) is called only by the +`parseImportSpecsYAML` test fixture. It duplicates mapping/field dispatch already +owned by `parseRootDirectiveBlock` in [directives.go](../../internal/engine/directives.go). + +**Recommendation and acceptance.** Use the active parser with the appropriate +context, or parse the declaration list directly when that is the tested boundary, +then remove the obsolete implementation. Preserve malformed-declaration and +selector coverage. Keep the active imports-only context used in layered version +processing; thin test helpers delegating to production are not inherently dead. + +### F04. Remove The Disconnected Root Record-Clone Family + +Type: cleanup. Priority: low. Source: deadcode 3; related testing 6. + +**Problem and evidence.** The private family in root [records.go](../../records.go) +has no production facade entry: `cloneLocation`, `cloneLocations`, `cloneSourceInfo`, +`cloneImportSelections`, `cloneSourceVersionCheck`, `cloneSourceInfos`, `cloneOrigin`, +`cloneFieldInfo`, `cloneChange`, `cloneDiagnostic`, and `cloneDiagnostics`. +Helpers refer to each other; only tests call `cloneSourceInfo`. Active root-owned +records come through [conversion.go](../../conversion.go). + +**Recommendation and acceptance.** Delete the disconnected family and tests solely +protecting it. Keep conversion assertions and useful returned-record ownership +coverage through repeated public queries and mutation of mutable nested results. +Replacing an entire `Destination` field on a copied struct does not demonstrate +nested detachment. Keep active engine clones and `Path.clone`; do not expose +opaque path internals or invent a stronger copy guarantee. Coordinate F12. + +### F05. Remove Uncalled Engine Helpers + +Type: cleanup. Priority: low. Source: deadcode 4; placement subset also complexity 5. + +The type-aware audit found no callers or function-value uses in the inspected +production/test build configurations for the following helpers: + +| Candidate | Active counterpart to preserve | +| --- | --- | +| `mergeDisjointMappings` in [compose.go](../../internal/engine/compose.go) | `mergeDisjointMappingsAtPath` and shared recursive merging; the old helper references only itself. | +| `mergeHistoryDocument` in [history.go](../../internal/engine/history.go) | `mergeHistoryDocumentAtPath` with sequence evidence. | +| `inheritanceDirectiveLocation` in [inheritance.go](../../internal/engine/inheritance.go) | Preparation's directive entry/location handling. | +| `addSecretEvidencePath` in [secret.go](../../internal/engine/secret.go) | Projected version sensitivity evidence and the active path accumulator. | +| `PlacementModeFromFacade`, `PlacementModeForFacade` in [destination.go](../../internal/engine/destination.go) | Input/options conversion and `SourceInfoPlacementModeForFacade`. | + +**Recommendation and acceptance.** Remove these definitions and imports/comments +solely supporting them after confirming references at implementation time. Preserve +active counterparts and public root APIs. The placement pair can be removed +without selecting the broader transport work in F07. + +### F06. Trim Unread Private Result And Transport Members + +Type: cleanup. Priority: low. Source: deadcode 6. + +The audit identified the following populated members without production readers. +Some private tests inspect them. Remove only these slots and computations solely +feeding them, preserving validation and active outputs: + +| Owner and candidate members | Boundary to preserve | +| --- | --- | +| `collectionPolicyResolution.declarations`, `declarationsPresent`, `identityDeclarationPresent`, `applicationIdentityFields` in [collection_policy.go](../../internal/engine/collection_policy.go) | Local declaration normalization, `modeDeclarationPresent`, and `applicationIdentityPresent`. | +| `inputPresence.null` in [decode.go](../../internal/engine/decode.go) | Presence/location and effective-node null reporting. | +| `rootDirectiveExtraction.importsPresent`, `sequencesPresent` in [directives.go](../../internal/engine/directives.go) | Parsed declarations, diagnostics, and directive validation. | +| `importSelection.outcome` in [import_selection.go](../../internal/engine/import_selection.go) and glob selection | Candidate/admission behavior and the distinct, active public `importSelectionEvidence.outcome`. | +| `importDeclarationSpec.excludeSet` in [import_spec.go](../../internal/engine/import_spec.go) | Active `exclude` policy. | +| `schemaPath.field`, `dynamic` in [schema.go](../../internal/engine/schema.go) | Descriptor and success result used by path consumers; coordinate A01. | +| `selectedOverlayRequest.filesystem` in [selected_overlay.go](../../internal/engine/selected_overlay.go) | Explicit filesystem-access argument and borrowed ownership. | +| `sequenceDeclarationSpec.destinationLocation` in [sequence_declaration.go](../../internal/engine/sequence_declaration.go) | Other active declaration/field diagnostic locations. | +| `sourceVersionEvidence.absence`, `checked`; `sourceVersionAncestor.kind` in [source_version.go](../../internal/engine/source_version.go) | Actual checks, version/presence/location evidence, ancestor paths, and sensitivity. | +| `postDecodeOverrideContext.sources` in [override_context.go](../../internal/engine/override_context.go) | Local source list used to build callback resolution input. | + +**Acceptance.** Preserve public presence, null, selection, version, diagnostic, +and ownership behavior. Rework assertions on inactive annotations only where +they protect a meaningful supported outcome. A lack of selector reads alone is +not a general removal rule: map keys, embedded records, and interfaces require +separate analysis. + +### F07. Simplify Redundant Facade-To-Engine Transport + +Type: optional refactor. Priority: low. Source: complexity 5, excluding F05's +bounded placement-bridge removal. + +**Problem and evidence.** Root conversion, engine facade-transport records, engine +private defaults, decorators, and load normalization provide several mapping +sites for the same import-default fields. Placement likewise uses a record plus +a setter-style bridge. See [conversion.go](../../conversion.go), +[lifecycle.go](../../lifecycle.go), [layered.go](../../layered.go), +[import_spec.go](../../internal/engine/import_spec.go), and +[destination.go](../../internal/engine/destination.go). + +**Recommendation and acceptance.** Consider one engine representation for +import-default input and straightforward construction on existing option/input +records, only if this reduces total concepts and mapping sites. Keep domain-rule +and ownership conversions and separate registration compilation. Preserve +root-owned types/signatures, both loading APIs, invalid-enum validation, +nil-versus-empty patterns, and pointer/slice snapshots. No universal request DTO, +generated mapper, public internal aliases, or adapter framework is proposed. + +### F08. Index Secret-Failure Location Queries Lazily + +Type: optimization candidate. Priority: low. Source: efficiency 5. + +**Problem and evidence.** `secretDiagnostic` in +[secret_reference.go](../../internal/engine/secret_reference.go) calls unindexed +nearest-location traversal for each failed reference. N missing references in a +wide mapping incur O(N²) comparisons, even when they use one cached environment +name. Normal deterministic traversal/sorting does not require these scans. + +**Recommendation and acceptance.** Consider a lazy read-only `documentLookup` +shared during secret resolution, or carry relevant location state through the +walk. With V nodes, F failures, and depth D, attribution can be O(V + F × D), +apart from sorting. Keep the successful path free of an unnecessary eager index. +Preserve all diagnostics, ancestor fallback, source coordinates, and safe messages; +failing fast would change behavior rather than optimize it. + +### F09. Cover Public Historical-Path Enumeration + +Type: test improvement. Priority: medium. Source: testing 2; related testing 6. + +**Problem and evidence.** Root `Metadata.HistoricalPaths` in +[metadata.go](../../metadata.go) had no executed statements in the aggregate +profile. Engine tests cover it thoroughly, but not root path conversion. An empty +or incorrectly converted public result could leave engine tests green. + +**Recommendation and acceptance.** Add one external-package test enumerating +effective and removed paths, using returned paths with `Lookup` and `History`, +including a numeric-key/sequence shape change. Cover sorted unique pointer +spelling, zero metadata, and replacement of a returned slice element without +affecting later queries. Keep detailed path mutation/shape tests in the engine. +This can replace F12's private segment-representation assertion without copying +the complete internal matrix or exposing private segments. + +### F10. Strengthen Fuzz Detachment Assertions + +Type: test improvement. Priority: low. Source: testing 4. + +**Problem and evidence.** `FuzzCompositionAndOverlay` in +[composition_fuzz_test.go](../../internal/engine/composition_fuzz_test.go) appends +to the result root then compares inputs. A shallow root copy sharing existing +descendants can pass because append changes neither the input's slice header nor +elements within its old length. Existing deterministic merge tests already +exercise meaningful nested mutation. + +**Recommendation and acceptance.** Mutate an existing result descendant or +in-length element within the bounded fuzz target, then compare both inputs. +Handle empty containers separately and retain input immutability/commutativity +properties. Improve the existing assertion rather than adding another ownership +matrix or claiming all ownership coverage is absent. + +### F11. Replace Exact Stat-Call Choreography With Observable Ordering + +Type: test cleanup. Priority: low. Source: testing 5. + +**Problem and evidence.** `TestSelectNamedImportsChecksNormalizedPathsLexically` +in [import_selection_test.go](../../internal/engine/import_selection_test.go) +requires `a.yaml,a.yaml,z.yaml,z.yaml` stat calls but checks candidate count only. +Lexical handling matters; exactly two stats per file is not an established contract. + +**Recommendation and acceptance.** Assert ordered candidate paths and, where +needed, the first diagnostic site using failing targets. Consolidate with existing +selection tests and delete the standalone case if its protection is subsumed. +Keep meaningful observations such as not opening excluded contents. Do not turn +incidental helper call counts into an efficiency or behavior guarantee. + +### F12. Consolidate Facade Scaffolding Around Public Behavior + +Type: test cleanup. Priority: low. Source: testing 6; related deadcode 3. + +**Evidence and recommendations.** + +- Remove the duplicate six-type ownership loop in + [facade_compatibility_test.go](../../facade_compatibility_test.go), retaining the + comprehensive owner in [facade_candidate_test.go](../../facade_candidate_test.go), + compile-time interfaces, public construction, and distinct lifecycle checks. + A literal assignment equality assertion on a new `Result.Config` adds no + protection beyond compilation and ordinary Go assignment. +- Replace `TestWrappedPathsRetainEngineSegmentIntent` in + [facade_internal_test.go](../../facade_internal_test.go), which compares private + representations, with public contextual mapping/index behavior under F09. + Keep engine segment-intent coverage. +- Move `TestWrappedSourcesPreserveEngineOwnership` from direct `engine.Load` + through `source.toEngine()` to public `Load` and root error assertions. + Consolidate reader cases with [facade_equivalence_test.go](../../facade_equivalence_test.go) + while preserving explicit copied-reader and byte-source mutation coverage. +- F04 owns removal of the disconnected clone family and the ineffective whole- + destination assignment assertion in [records_test.go](../../records_test.go). + Keep useful conversion and returned-record ownership protection there or in + existing public source-snapshot tests. + +**Acceptance.** Remove scaffolding only after preserving its distinct behavioral +risks. Root integration tests still protect conversion, error typing, callbacks, +source identity, disclosure, and ownership; engine coverage does not substitute +for them. Static public/dependency-boundary tests remain useful. + +## Boundaries To Preserve + +The audits found the architecture coherent and did not establish a need for new +public features, a replacement loading API, package proliferation, or a general +visitor/strategy framework. Root-owned conversions, synchronous load-local state, +shared dependency scheduling, indexed combination, separated inheritance phases, +and passive resource contexts remain useful owners. The clone traversal was +already reviewed and retained at `34cbc0d:docs/roadmap/consolidation.md`; a high +complexity estimate does not reopen it. + +No additional supported-use data race, deadlock, background-work leak, credential +disclosure, command injection, authority bypass, or host containment escape was +established. These are limited audit observations, not absence guarantees. +Caller-owned mutable objects, blocking operations, safe callback messages, and +borrowed filesystem behavior retain their documented responsibilities. No new +locking, cancellation, retry, authentication, permission, or environment-variable +restriction follows from these findings. + +Preserve raw-error suppression, structured safe diagnostics, original application +panic propagation, intentional bootstrap partial writes, allowed partial writer +output, and documented read-only regular-file close handling. Regression tests +must enforce existing contracts rather than broaden them. Optional absence must +remain distinct from acquisition failure, and genuine finite reuse from cycles. + +Public APIs are not dead merely because repository examples omit them. Interface +dispatch, reflection, build constraints, identity tokens, map-key equality, and +uninstantiated generics require more than caller counts. In particular, +`decodeInlineMapEntry` and import-cycle diagnostics remain active. +`Inspection.resolution` retains recorded snapshots under the existing resolution +contract even though replay uses a coalesced view; it is not a cleanup candidate. + +Selected stable Kahn ordering, deterministic sorting, exact numeric identity, +and the expressly selected registration-overlap check are not reconsidered here. +Required detached copies and genuinely expanding output remain legitimate costs. +The broader replay, metadata-handoff, cross-load schema-cache, and contributor- +trace experiments stay in [future.md](future.md#3-performance-experiments). +Local fixes and removal of proven unused work do not select those experiments. + +## Validation Approach For Selected Work + +Use the existing [development guide](../development.md#baseline-validation) and +[testing policy](../policy/testing.md). Add meaningful regressions with the +selected fixes, at the narrowest stable boundary that detects the reported +failure. Extend existing owners rather than duplicating internal matrices at +every layer. Assert nil results and safe structured failures where relevant, +and verify retained state/handles independently when they have distinct owners. + +Keep termination, allocation, and link-expansion probes bounded. Use small +controlled budgets, disposable processes where necessary, and filesystem +observation where it detects the specific defect. No wall-clock thresholds, +coverage percentages, complexity-score limits, exact incidental syscall traces, +new resource caps, or extra release gates are selected. Algorithmic evidence is +not a measured speedup. Performance experiments need separate scope and suitable +measurement conditions; no benchmarks were run for this consolidation. + +Once selected work changes behavior or structure, update its canonical contract +or internal owner as appropriate. This review document remains the finding/status +owner until an implementation plan is prepared; completed records can retire +to Git history under the normal documentation lifecycle. + +## Original-Finding Map + +Original files are retrieved from `ecd5d5d:docs/roadmap/`. Every numbered +finding is mapped below; repeated citations are corroboration or shared coverage, +not extra fixes. + +| Original finding | Consolidated owner | +| --- | --- | +| correctness 1 | [A01](#a01-terminate-pointer-only-recursive-descriptor-traversal) | +| correctness 2 | [A02](#a02-make-failed-dynamic-descriptor-construction-transactional) | +| correctness 3 | [A03](#a03-distinguish-acyclic-slice-views-in-cycle-detection) | +| correctness 4 | [B01](#b01-deactivate-callback-contexts-during-panic-unwinding) | +| efficiency 1 | [E01](#e01-avoid-scanning-every-policy-for-each-replacement) | +| efficiency 2 | [E02](#e02-avoid-recopying-unrelated-policy-across-small-tiers) | +| efficiency 3 | [E04](#e04-index-tier-history-projection-lookups) | +| efficiency 4 | [E05](#e05-restore-compound-identity-without-repeated-mapping-scans) | +| efficiency 5 | [F08](#f08-index-secret-failure-location-queries-lazily) | +| efficiency 6 | [A05](#a05-reuse-successful-dynamic-type-eligibility-checks-locally) | +| complexity 1 | [F01](#f01-remove-the-test-only-eager-acquisition-mode) | +| complexity 2 | [F02](#f02-clarify-active-acquisition-and-coordinator-handoffs) | +| complexity 3 | [E03](#e03-clarify-policy-preparation-and-merge-evidence-application) | +| complexity 4 | [C03](#c03-separate-glob-entry-preparation-from-traversal) | +| complexity 5 | [F07](#f07-simplify-redundant-facade-to-engine-transport), with bounded bridge removal in [F05](#f05-remove-uncalled-engine-helpers) | +| concurrency 1 | [B01](#b01-deactivate-callback-contexts-during-panic-unwinding), corroboration | +| resources 1 | [A04](#a04-enforce-existing-node-budgets-before-container-reservations) | +| resources 2 | [D01](#d01-account-for-the-complete-published-history-graph) | +| resources 3 | [B02](#b02-close-the-owned-import-root-when-a-version-callback-panics) | +| error-handling 1 | [C01](#c01-preserve-directory-failure-phase-and-strengthen-its-tests) | +| security 1 | [C02](#c02-avoid-reexpanding-completed-finite-symlink-dependencies) | +| testing 1 | Regression acceptance under A01–A04, B01–B02, C02, and D01 | +| testing 2 | [F09](#f09-cover-public-historical-path-enumeration) | +| testing 3 | [C01](#c01-preserve-directory-failure-phase-and-strengthen-its-tests), including shared fixture assertions | +| testing 4 | [F10](#f10-strengthen-fuzz-detachment-assertions) | +| testing 5 | [F11](#f11-replace-exact-stat-call-choreography-with-observable-ordering) | +| testing 6 | [F12](#f12-consolidate-facade-scaffolding-around-public-behavior), with F04 and F09 | +| deadcode 1 | [D02](#d02-remove-the-unconsumed-override-result-pre-cli-copy) | +| deadcode 2 | [D03](#d03-remove-the-unqueried-published-structural-input-copy) | +| deadcode 3 | [F04](#f04-remove-the-disconnected-root-record-clone-family) | +| deadcode 4 | [F05](#f05-remove-uncalled-engine-helpers) | +| deadcode 5 | [D04](#d04-stop-constructing-unconsumed-combination-evidence) | +| deadcode 6 | [F06](#f06-trim-unread-private-result-and-transport-members) | +| deadcode 7 | [F03](#f03-retire-the-parallel-test-only-import-directive-parser) | +| deadcode 8 | [F01](#f01-remove-the-test-only-eager-acquisition-mode), duplicate of complexity 1 | + +## Open Questions + +### OQ-1. What Should The First Implementation Scope Include? + +**Recommendation:** select all nine reported defects with their regression +coverage, plus the bounded cleanup and test improvements where their evidence +holds on the implementation baseline. The unused history copies deserve early +coordination with D01, and callback context/root closure deserve independent +acceptance within the same work area. Keep fixes reviewable and distinct even +when delivered in one sprint. + +**Alternative:** deliver defect fixes first, then handle cleanup and independent +test improvements in a separate effort. This produces a smaller initial scope +and simpler attribution, but retains known redundant work and can revisit the +same files/tests. Cleanup should not delay a consequential fix merely to achieve +a tidier overall structure. + +### OQ-2. Which Efficiency Candidates Should Accompany Remediation? + +**Recommendation:** consider E04/E05 as narrow local improvements, and A05/F08 +as lower-priority options after their correctness/diagnostic boundaries are sound. +Evaluate E01/E02 together with E03 before committing to a policy-state redesign: +their distinct scaling cases need preserved acceptance even if they share an +implementation. These candidates have source-level evidence of avoidable work, +but no new measured speedup claim. + +**Alternative:** defer all six efficiency candidates until representative consumer +profiling establishes their practical cost. This concentrates the first effort +on correctness and cleanup, at the cost of retaining demonstrated growth patterns. +It does not block C02's availability fix or A04/D01's enforcement of existing +resource budgets. Broader experiments in `future.md` remain separately deferred. + +### OQ-3. How Much Structural Refactoring Should Be Selected? + +**Recommendation:** accept F01's removal of the test-only protocol as bounded +cleanup; assess active acquisition handoffs (F02) and policy/evidence ownership +(E03) with their affected fixes before selecting a wider extraction. Leave glob +entry extraction (C03) and transport simplification (F07) optional unless a +concrete design reduces concepts and clarifies ownership. + +**Alternative:** include all four structural refactors in the same effort for a +more uniform maintenance result. This offers broader readability improvements +but increases review scope and the chance of moving complexity rather than +removing it. Neither approach calls for a new public API, generic framework, or +changes to the established configuration model. diff --git a/docs/roadmap/complexity.md b/docs/roadmap/complexity.md deleted file mode 100644 index e46e390..0000000 --- a/docs/roadmap/complexity.md +++ /dev/null @@ -1,283 +0,0 @@ -# Maintainability And Complexity Audit - -Reviewed revision: `320d0c5f439a059a7081cf7532c9b5d65891f5c7`. -Audit date: 2026-10-25. - -## Scope And Method - -This audit examines readability, responsibility boundaries, state handoffs, and -interface duplication. It covers the root facade, engine subsystems, document -and YAML boundaries, and relevant tests, documentation, and prior refactoring -decisions. It does not propose new feature behavior or performance guarantees. - -The codebase-memory graph was refreshed and its function spans, cyclomatic and -cognitive complexity estimates, parameter counts, and caller traces were used -to select code for inspection. The graph contains 907 non-test functions/methods -and 940 test-file functions/methods. Its index excludes examples, documentation, -and scripts; tracked-file inventory and direct inspection supplemented it. -Standalone `gocyclo`, `gocognit`, and `golangci-lint` were unavailable. No tool -installation, benchmarks, or empirical performance testing was needed. - -The tracked production inventory, excluding examples, contains 16 root Go files, -97 engine files, one document file, and two YAML adapter files: approximately -22,612 physical lines overall. The largest production files are `compose.go` -(599 lines), `history.go` (568), `overlay.go` (564), and `discovery.go` (540). -These measurements locate responsibilities; they are not refactoring thresholds. - -Representative graph measurements: - -| Function | File | Function span | Cognitive estimate | -| --- | --- | ---: | ---: | -| `valueOperation.clone` | `internal/engine/clone.go` | 130 | 104 | -| `acquireInput` | `internal/engine/acquisition.go` | 202 | 99 | -| `valueBuilder.build` | `internal/engine/value.go` | 104 | 93 | -| `decodeValue` | `internal/engine/decode.go` | 115 | 85 | -| `selectGlobImportCandidates` | `internal/engine/glob.go` | 112 | 73 | -| `assembleLayeredInput` | `internal/engine/load_request.go` | 146 | 63 | -| `executeLoadRequest` | `internal/engine/load.go` | 198 | 56 | -| `mergeOverlayMappingsWithCollectionPolicy` | `internal/engine/overlay.go` | 163 | 46 | - -Spans include comments and blank lines; scores are tool estimates for comparative -triage, not standardized quality grades. Source review, rather than a numerical -cutoff, determines the findings below. - -## Architectural Assessment - -The [implemented architecture](../internal/overview.md) remains coherent: the -root owns the supported public types and explicit conversions; the engine owns -loading and frozen inspection; the document package owns the ordered tree; and -the YAML adapter isolates the upstream parser. Ordinary and layered loads share -one normalized request and execution lifecycle. Imports and inheritance retain -their separate scopes and use the common dependency scheduler. - -The recent consolidation improved import ownership and file boundaries, separated -inheritance preparation/combination/finalization, shared filesystem facts, and -clarified schema and projection responsibilities. The findings concern remaining -maintenance opportunities, not a recommendation to repeat that refactor or split -the engine into a package per subsystem. - -The existing [correctness](correctness.md) and [efficiency](efficiency.md) audits -remain separate owners of their findings. No additional runtime defect was -established in this pass. Related risks are referenced rather than duplicated. - -## 1. Remove The Test-Only Eager Acquisition Mode - -Status: open. Priority: medium. Scope: engine acquisition and its test helpers. - -**Relevant code.** `inputAcquisitionRequest.deferAssembly` in -[acquisition.go](../../internal/engine/acquisition.go), and the final branch of -`acquireImportedWithAccess` in -[import_acquisition.go](../../internal/engine/import_acquisition.go), lines -206–224. Production construction sites in `executeLoadRequest` -([load.go](../../internal/engine/load.go), lines 210–214) and -`acquireUnderlayGroupScopes` -([load_request.go](../../internal/engine/load_request.go), lines 243–246) both -set this flag to true. - -**Complexity.** Caller tracing found no production caller selecting eager -composition. The false branch remains exercised by private test entry helpers, -including `testImportRequest`, `testAcquireImport`, and `testAcquireInput` in -[imports_test.go](../../internal/engine/imports_test.go). Acquisition therefore -maintains two return protocols: an acquired scope with a nil composed root, or -an already composed document. The eager branch reconstructs `assembledInput` -and reenters composition from the acquisition function, while the published -loading paths perform that phase later. Tests can consequently exercise a -different handoff from the one consumers use. - -**Potential refactoring.** Make acquisition return retained scopes consistently -and remove the unused production mode. Tests needing composed values can invoke -the real composition phase explicitly through a small test-only convenience -helper. Keep acquisition-only tests acquisition-only. Preserve handle closure, -pending-item placement reservation, accounting, diagnostics, and meaningful -composition coverage. This removes a private alternative path, not support for -any public loading workflow. Do not infer that other nil-state or directive -contexts are unused without separately tracing their callers. - -## 2. Clarify Acquisition And Load-Coordinator Phase Handoffs - -Status: open. Priority: medium. Scope: engine, across acquisition, version, -assembly, and lifecycle files. - -**Relevant code.** `acquireInput` in -[acquisition.go](../../internal/engine/acquisition.go), `executeLoadRequest` in -[load.go](../../internal/engine/load.go), and `assembleLayeredInput` in -[load_request.go](../../internal/engine/load_request.go). - -**Complexity.** `acquireInput` still combines search outcome handling, parsing, -disabled-directive checks, three directive/version-processing branches, -import-versus-standalone dispatch, placement/accounting, and passive context -construction. Its version-processing section repeats source-ID extraction, -failure handling, and `mainProcessing` updates, while the layered branch must -extract directives separately and restore them after version processing. - -The coordinators also manipulate several intentionally distinct representations: -parsed source, local payload, acquired scope, composed root, inherited root, -local/protected version paths, and effective version evidence. For example, -`executeLoadRequest` includes a substantial ordinary-load resource-context -fallback between assembly and inheritance, while `assembleLayeredInput` repeats -declaration gathering, tier application, and destination projection for underlays -and main. Understanding a source-version or placement change requires tracing -both those mechanics and the overall phase order through long functions. - -**Potential refactoring.** Extract cohesive operations for processing one parsed -input's directives/version evidence and for placing/registering a standalone -contribution. Reuse the existing processing result and acquisition state rather -than inventing another context hierarchy. Isolate the substantial context and -structural-expansion operations from the top-level lifecycle, and consider one -helper for the repeated tier application/projection handoff. Keep phase order, -error exits, and source acquisition order readable in the coordinators. - -Extraction is useful only when it gives one operation a clear owner and result; -moving blocks into helpers that still share the same mutable surrounding locals -would not solve the problem. Preserve the deliberate ordinary/layered overlay -differences, default versus enabled version-check timing, warning accumulation, -exactly-once callbacks, and load-wide versus tier-local state. This is separate -from finding 1: removing the eager branch does not simplify these active paths. - -## 3. Separate Collection-Policy Preparation From Merge Evidence Application - -Status: open. Priority: medium. Scope: engine precedence and inheritance. - -**Relevant code.** `mergeOverlayMappingsWithCollectionPolicy` and -`applyOverlaySequenceEvidence` in -[overlay.go](../../internal/engine/overlay.go); -`resolveStrictCollectionPolicies` in -[compose.go](../../internal/engine/compose.go); -`combineInheritanceTemplate` in -[inheritance_combine.go](../../internal/engine/inheritance_combine.go); and the -policy/origin helpers in -[inheritance_collection.go](../../internal/engine/inheritance_collection.go). - -**Complexity.** The overlay coordinator prepares claims and declaration groups, -discovers applicable destinations, resolves operations, configures recursive -merge callbacks, accumulates diagnostics/history, reconciles declaration-only -policies after mutation, and sorts the output. Its callbacks close over several -maps and mutate the assembly while reporting failures through an outer slice. -`applyOverlaySequenceEvidence` then mixes policy retirement, displaced origins, -replacement records, copied incoming origins, and record-policy reconstruction. - -There is also concrete duplicated bookkeeping: overlay and inheritance callbacks -both translate a sequence result into replace-versus-mapped history descriptors; -overlay evidence and `applyInheritanceSequencePolicy` both rebuild record policy -order from lower/higher item projections. The shared `combineSequence` is already -the correct value-combination owner, but maintainers must separately coordinate -these sidecar translations when its result representation changes. - -**Potential refactoring.** Give overlay preparation, value/evidence application, -and declaration-only finalization explicit local boundaries. Let a small -operation owner or result carry the diagnostic and evidence collections currently -captured by the large callbacks. Share the straightforward history-descriptor -construction; evaluate a common projected-record selection helper while retaining -named precedence and inheritance ownership rules. - -Do not merge the distinct origin semantics or hide mode-lifetime differences -behind a generic strategy framework. Keep `combineSequence` and the recursive -mapping merge shared, and consume their result rather than recomputing values. -Coordinate this work with collection-policy findings 1–2 in the -[efficiency audit](efficiency.md) so clearer preparation does not preserve or -introduce repeated whole-table copying/scanning. Preserve replacement ordering, -pending declarations, new versus retained record owners, and stable history. - -## 4. Extract Patterned Glob Entry Preparation From Traversal Orchestration - -Status: open. Priority: medium. Scope: engine import discovery. - -**Relevant code.** `selectGlobImportCandidates` in -[glob.go](../../internal/engine/glob.go), especially its patterned-entry branch. -The corresponding directory-search entry operation already has a named owner, -`prepareImportDirectoryEntry` in -[import_selection.go](../../internal/engine/import_selection.go). - -**Complexity.** The glob function nests pattern-segment traversal, parent -candidates, and directory entries. Its innermost branch handles matching, -hidden-entry policy, lexical and access paths, containment, link inspection, -follow behavior, intermediate-directory eligibility, native/host identity -differences, canonicalization, labels, and candidate creation. Set-union and -component-advancement mechanics are interleaved with those admission decisions, -making the important lexical-versus-canonical distinction difficult to follow. - -**Potential refactoring.** Extract a glob-specific entry preparation operation -returning an admitted candidate, an ordinary skip, or a diagnostic. Leave pattern -component advancement, literal segments, per-component union/sorting, and final -exclusion/requiredness visible in the selector. Reuse low-level filesystem facts -where semantics coincide; directory and glob admission need not become one -general visitor interface. Preserve inspection/error order, entry accounting, -native namespace spelling, and selected-file validation. This is a readability -change, not a proposal to change the selected discovery algorithm or add caching. - -## 5. Simplify Redundant Facade-To-Engine Input Transport - -Status: open. Priority: low. Scope: root facade and engine, across packages. - -**Relevant code.** Root `toEngineImportDefaults` in -[conversion.go](../../conversion.go), `toEngineOptions` in -[lifecycle.go](../../lifecycle.go), and `toEngineLayeredOptions` in -[layered.go](../../layered.go); engine transport/private defaults and option -decorators in [import_spec.go](../../internal/engine/import_spec.go), lines -46–87; and placement transport helpers in -[destination.go](../../internal/engine/destination.go), lines 23–39. - -**Complexity.** Import defaults have a root-owned public representation, an -engine facade-transport representation, and a second engine-private representation -with the same five fields. Conversion constructs the transport, an option -decorator converts it to private defaults, and load normalization snapshots it -again. Placement similarly requires constructing an engine record and then -passing it through a separate setter-style helper. Even a simple control change -has multiple mechanical mapping sites across the package boundary. - -Caller and text-reference checks also found no uses of the standalone -`PlacementModeFromFacade` and `PlacementModeForFacade` functions beyond their -declarations. The similarly named `SourceInfoPlacementModeForFacade` is used by -root source-record conversion and is not dead code. The unused pair adds a -second apparent placement conversion route for maintainers to investigate. - -Root type ownership and explicit conversion are deliberate and useful; the -opportunity is the extra engine-internal representation/construction protocol, -not eliminating the facade. The public wrappers do not need to change. - -**Potential refactoring.** Remove the two unused enum bridges. Then consider one -engine representation of import-default input values and a straightforward -construction surface on the existing engine option/input records. Review inert -placement decorators as part of the same bounded cleanup. Keep helpers that -actually encode domain rules or ownership conversion, and keep -definition/registration compilation separate from transport. -Avoid adding another universal request DTO, code generator, reflective mapper, -or adapter framework to remove a few mechanical functions. - -Preserve root-owned named types and signatures, invalid-enum validation, -nil-versus-empty patterns, pointer/slice snapshots, and the two public loading -entry points. Exported declarations inside Go's `internal` boundary need not -become public root aliases or bridge methods. Pursue this low-priority cleanup -only if it reduces total concepts and mapping sites, rather than relocating them. - -## Areas To Retain And Review Boundaries - -- The clone traversal was explicitly reviewed and retained in the completed - consolidation record at `34cbc0d:docs/roadmap/consolidation.md`. Its high score - is not new evidence for reopening that work. `valueBuilder.build`, typed - decoding, and the YAML translator also contain substantial but recognizable - type dispatch; no repository-wide visitor framework is recommended. -- The cohesive engine package and explicit root record conversions follow - [ADR 0006](../adr/0006-public-facade.md). Function/file counts alone do not - justify package extraction, public API consolidation, or generated conversions. -- The shared scheduler, mapping/sequence primitives, recently separated - inheritance phases, schema field assembly, and resolved filesystem facts remain - useful common owners. Preserve those improvements. -- `history.go` and the larger integration-test files have several substantial - routines, but file size alone did not establish another prioritized refactor. - The deferred replay, copy-handoff, caching, and contributor-index experiments - remain in [future work](future.md#3-performance-experiments). - -These are opportunities for further consideration, not a staged implementation -plan or new release blockers. Favor the concrete unused-mode removal and active -acquisition/collection boundaries before lower-value transport cleanup. A later -implementation should preserve the established contracts and relevant behavioral -coverage; complexity scores, helper counts, and private call sequences should -not become test thresholds or repository guarantees. - -## Validation - -This audit changes only this findings document. No production code, tests, -dependencies, policies, or public contracts are changed. Validation consists of -the existing local documentation-link test and `git diff --check`; runtime and -performance measurements are not part of this review. diff --git a/docs/roadmap/concurrency.md b/docs/roadmap/concurrency.md deleted file mode 100644 index b468389..0000000 --- a/docs/roadmap/concurrency.md +++ /dev/null @@ -1,108 +0,0 @@ -# Concurrency And Lifetime Audit - -Reviewed revision: `a80273433e87ca955e5a5d9e99b616d8da10f185`. -Audit date: 2026-10-25. - -## Scope And Assessment - -This audit reviewed source reuse, concurrent independent loads, schema and -acquisition state, callback lifetime, publication, frozen metadata/explanation/ -diff/rendering, resource contexts, and secrets bootstrap. Knowledge-graph searches -and caller traces were supplemented by source inspection and existing tests. - -No new data race, deadlock, unsafe synchronization, goroutine leak, channel -management defect, or cancellation/shutdown defect was established under the -documented usage contracts. One previously reported callback-lifetime defect -remains reproducible and relevant to teardown; it is referenced below rather -than assigned a second independent remediation task. - -The [implemented architecture](../internal/overview.md) provides a straightforward -ownership model for concurrency: - -- Loading runs synchronously through one engine lifecycle. Production code - launches no library-owned goroutines and has no channel, worker, timer, watcher, - or background shutdown protocol to manage. -- Acquisition, version evidence, schema caches, reflection traversal, composition, - and callback records belong to a load or operation, rather than a shared global - cache. The root facade delegates without introducing a second shared state owner. -- Reader-source copies share a pointer to `readerSource`, whose atomic one-use - claim precedes reading. Byte and mapping sources retain constructor snapshots. -- Publication builds the retained inspection indexes before returning. Query, - explanation, diff, and rendering paths read frozen state and allocate their - own mutable results or replay trees. Typed configuration and returned metadata - are detached from the retained inspection snapshot. -- Returned resource contexts contain passive root information or a borrowed - filesystem. Host resource opens acquire their own handles, with ownership - transferred to the returned file, rather than sharing the acquisition handle. - -These observations support the existing model; they are not new blanket -thread-safety guarantees for application callbacks, borrowed objects, or I/O. - -## 1. Callback Lifetime Guards Survive Panic Unwinding — Previously Reported - -Status: open, tracked by -[correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding). -Priority: low. Category: callback teardown across the facade/engine boundary. - -**Relevant code.** `runResolution` in -[resolution.go](../../internal/engine/resolution.go), and `runValidation` / -`reportVersion` in [validation.go](../../internal/engine/validation.go), deactivate -the callback state only after a normal return. The root callback wrappers in -[lifecycle.go](../../lifecycle.go) expose that same engine context. - -**Circumstances and consequence.** If a callback retains its context and panics, -and application code recovers outside the load, the retained context still -accepts queries or diagnostic writes after callback exit. It should instead -produce the established closed-context failure. The abandoned per-load state -therefore remains accessible beyond its intended lifetime. Reusing the existing -external diagnostic probe reproduced this for resolution, semantic validation, -and version-check contexts at the reviewed revision. - -**Potential remediation.** Use invocation-scoped deferred deactivation, as -described in the linked correctness finding, preserving immediate closure on -normal return and unchanged propagation of application panics. There is no need -to add context locks, atomic lifecycle flags, panic recovery, or background-work -management. Concurrent use of a live callback context is already outside its -supported contract; this finding does not establish an additional supported-use -race or justify broadening that contract. - -## Validation - -The following passed with Go 1.26.10 on Linux amd64: - -```sh -go test -race ./... -count=1 -go vet ./... -``` - -The six existing engine concurrency tests for shared definitions, copied-reader -claims, concurrent inspection, selected-overlay loads, and resolution-input -isolation also passed with `-race -count=20 -cpu=1,4,8`. These tests live in -[independence_test.go](../../internal/engine/independence_test.go), -[source_test.go](../../internal/engine/source_test.go), -[explain_test.go](../../internal/engine/explain_test.go), and -[provenance_test.go](../../internal/engine/provenance_test.go). - -The previously created standalone callback-panic probe was rerun outside the -repository with race instrumentation. It confirmed the known lifetime defect -without reporting a data race; that probe performs sequential post-panic calls, -not unsupported concurrent callback-context use. No new tests or production -changes were made. Race detection covers executed paths and is not a proof of -all possible interleavings. No native macOS or Windows execution was performed. - -## Contract Boundaries Preserved - -- Read-only shared definitions and independent result ownership remain the - existing model. Callers remain responsible for synchronization of their own - mutable options, callback effects, readers, filesystems, and output writers. -- Secrets bootstrap is explicitly a startup operation with no environment - compare-and-set guarantee; its check-then-set behavior is not a new finding. - See the [bootstrap contract](../config.md#explicit-secrets-directory-bootstrap). -- Synchronous loading has no cancellation API or background refresh service. - Caller-supplied blocking operations are not evidence of a leaked ConfigKit - goroutine or a missing promised shutdown mechanism. See - [completion and resource lifetime](../config.md#completion-and-resource-lifetime). - -This report adds no synchronization requirements, new API, or release gates. -The existing [correctness](correctness.md), [efficiency](efficiency.md), and -[maintainability](complexity.md) findings retain their own scopes and status. diff --git a/docs/roadmap/correctness.md b/docs/roadmap/correctness.md deleted file mode 100644 index 4508be3..0000000 --- a/docs/roadmap/correctness.md +++ /dev/null @@ -1,191 +0,0 @@ -# Functional Correctness Audit - -Reviewed revision: `47944d22d1ad45a7f537780effb3d05f998eef14` -(Update ReleaseKit to v0.4.2). -Audit date: 2026-10-25. - -## Scope And Evidence - -This audit reviewed the root facade and its engine handoffs, source/discovery -and filesystem handling, composition and collection rules, inheritance, typed -decoding and ownership, application callbacks, secrets, metadata, explanation, -and effective-value comparison against the configuration contract and relevant -implementation/tests. It was a source-level functional review with targeted -diagnostics, not an exhaustive proof or a performance/security certification. - -The existing development check (`./scripts/check.sh`) and full race-enabled -suite (`go test -race ./...`) passed using Go 1.26.10 on Linux amd64. Standalone -public-API probes outside the repository reproduced the findings below. No -production code or repository tests were added or changed. No native macOS or -Windows validation was performed, and no new release gates are proposed. - -## 1. Pointer-Only Recursive Types Can Overflow The Stack Or Hang - -Status: open. Priority: medium. Category: schema/decoder termination. - -**Relevant code.** `schemaBuilder.describe` in -[schema.go](../../internal/engine/schema.go), lines 119–140, caches a placeholder -before describing pointer elements. A legal Go type such as `type Loop *Loop` -therefore produces a pointer descriptor whose element is itself. -`decodeValue` in [decode.go](../../internal/engine/decode.go), lines 93–106, -recursively unwraps pointers without consuming the input node. The pointer -unwrapping loops in `resolveSchemaPath` (`schema.go`, lines 347–350 and 387–390) -also assume that a non-pointer descriptor will eventually be reached. - -**Reproduction.** With the public package imported as `configkit`: - -```go -type Loop *Loop -type Config struct { - Value Loop `yaml:"value"` -} - -configkit.Load[Config](configkit.Bytes("input", []byte("value: {}\n"))) -``` - -This does not return a decode error. It repeatedly calls `decodeValue` with the -same input and descriptor until the process reports a fatal stack overflow. -A disposable probe using `debug.SetMaxStack(64 << 10)` demonstrated the failure -without allowing the process to grow a large stack. Explicit `value: null` -loads successfully. With that null input, adding a resolver that calls -`Normalize` on `/value` with `Loop(nil)` instead hangs in `resolveSchemaPath`; -the separate diagnostic process was stopped by a three-second timeout. - -**Expected behavior.** Incompatible non-null input must fail with a safe -diagnostic, and schema-path resolution must terminate. The -[schema contract](../config.md#application-schema-binding) supports recursive -types with acyclic values; the existing explicit pointer-only restriction is -for inline fields, not all pointer fields. This is not a request to recover -application panics or reject ordinary recursive structs. - -**Potential remediation.** Detect repeated descriptors while unwrapping -pointer-only chains in decoding and schema-path traversal. Preserve supported -nil/null values and finite acyclic values, return an appropriate decode error -for an incompatible non-null node, and reject descendants that cannot be -addressed. Review analogous pointer-unwrapping helpers for the same assumption. -A blanket schema rejection of pointer-only recursive types would instead be a -contract decision, since it would also reject currently valid nil values; do -not introduce it or an arbitrary pointer-depth cap merely to stop the loop. - -## 2. Failed Dynamic Schema Construction Leaves A Reusable Partial Descriptor - -Status: open. Priority: medium. Category: error handling and shared operation state. - -**Relevant code.** `valueOperation.dynamicDescriptor` in -[value_operation.go](../../internal/engine/value_operation.go), lines 36–48, -passes the load's existing descriptor cache directly to a new schema builder. -`schemaBuilder.describe` in [schema.go](../../internal/engine/schema.go), lines -119–140 and 167–191, caches container placeholders but returns child errors -without removing those placeholders. A subsequent lookup returns the partial -descriptor as if construction had succeeded. - -**Reproduction.** Use a configuration with `Value any` and this dynamic type: - -```go -type Invalid struct { - Channel chan int `yaml:"channel"` -} -``` - -In one resolver, execute these two assignments in order: - -```go -configkit.Normalize(r, configkit.Keys("value"), &c.Value, - any((*Invalid)(nil)), "first unsupported value") -configkit.Normalize(r, configkit.Keys("value"), &c.Value, - any(&Invalid{}), "second unsupported value") -``` - -The first helper correctly records an invalid-value diagnostic because channels -are unsupported. It nevertheless leaves the `*Invalid` descriptor cached with -a nil element. The second helper reuses it, reaches projection with that nil -element, and panics with a nil-pointer dereference instead of returning a -`LoadError`. This was reproduced through ordinary public `Load` and `Normalize`; -the application callback itself does not panic. - -**Expected behavior.** Unsupported dynamic/helper values must produce safe -schema/resolution diagnostics under the -[typed projection](../config.md#typed-projection-and-ownership) and -[resolution contracts](../config.md#recorded-application-resolution). -One reported validation failure must not corrupt the descriptor cache used by -later operations in the same callback. - -**Potential remediation.** Make dynamic descriptor construction transactional: -stage newly discovered descriptors and commit them only after a successful -build, or track and roll back all cache additions belonging to a failed build. -Account for recursive descriptors that refer to other partially built types, -not just the immediate failed container. Preserve previously valid descriptors -and the load-local cache ownership. A nil guard in projection alone would not -repair invalid cache entries being treated as successful schema descriptions. - -## 3. Cycle Detection Rejects Acyclic Slice Views Sharing A Starting Address - -Status: open. Priority: medium. Category: supported-value correctness. - -**Relevant code.** `valueVisit` and `visitFor` in -[value_operation.go](../../internal/engine/value_operation.go), lines 20–24 -and 97–116, identify a slice by its type, kind, and starting pointer, but not its -length. Both reflective projection/cloning and the parser-neutral value builder -in [value.go](../../internal/engine/value.go) use that identity for active-path -cycle detection. - -**Reproduction.** This value is finite and acyclic: its nested one-element view -contains only the string, not the enclosing two-element view. - -```go -shared := []any{"leaf", nil} -shared[1] = shared[:1] -``` - -Place it at `data` in a `Mapping` source, return it from typed defaults for -`Data []any`, or supply it as a CLI override. Observed results at `/data/1` were, -respectively, `source.invalid`, `schema.cycle`, and `override.invalid_value`. -The equivalent independently allocated value -`[]any{"leaf", []any{"leaf"}}` loads and renders successfully. A true -self-referential slice was also checked and remains correctly rejected. - -**Expected behavior.** The -[mapping source](../config.md#sources), -[projection/ownership](../config.md#typed-projection-and-ownership), and -[override](../config.md#explicit-cli-overrides) contracts permit supported -acyclic values. Shared backing storage alone is not a cycle, and detaching it -does not require preserving alias identity in the returned configuration. - -**Potential remediation.** Include the visible slice extent (its length) in the -active-visit identity, or use an equivalent identity that distinguishes these -views. Preserve detection of actual self/mutual cycles, empty/nil distinctions, -and independent ownership of retained values. Cover the shared helper's source, -override, projection, and cloning callers rather than fixing only one entry point. - -## 4. Callback Contexts Remain Active After Panic Unwinding - -Status: open. Priority: low. Category: callback-lifetime contract. - -**Relevant code.** `runResolution` in -[resolution.go](../../internal/engine/resolution.go), lines 283–291, and -`runValidation`/`reportVersion` in -[validation.go](../../internal/engine/validation.go), lines 99–106 and 250–257, -set `state.active = false` only after the application callback returns normally. -The root facade forwards the context without an additional lifetime guard. - -**Reproduction.** Retain the supplied context in a resolver, semantic validator, -or version checker, then panic in that callback. Recover the application panic -outside `Load` and invoke the retained context. Public-API probes showed that -`Resolution.IsExplicit` still returns a value and `Validation.Warning` still -accepts a report for both validation and version contexts; none produces the -documented closed-context panic. - -**Expected behavior.** The -[resolution](../config.md#recorded-application-resolution) and -[validation](../config.md#semantic-validation) contracts make these contexts -callback-scoped and require retained contexts to reject use after callback exit. -Application panics remain programming failures and should propagate unchanged; -closing the context during unwinding does not require ConfigKit to recover them -or return a partial result. This finding concerns only the lifetime guard after -an application chooses to recover its own panic. - -**Potential remediation.** Deactivate the context with a defer scoped to each -callback invocation, retaining immediate deactivation on normal return as well -as on panic unwinding. Do not leave the context active through later projection -or metadata work, and do not convert or suppress the callback panic. Apply the -same lifetime handling to resolution, semantic validation, and version checks. diff --git a/docs/roadmap/deadcode.md b/docs/roadmap/deadcode.md deleted file mode 100644 index cee891d..0000000 --- a/docs/roadmap/deadcode.md +++ /dev/null @@ -1,307 +0,0 @@ -# Dead Code And Obsolete Functionality Audit - -Reviewed revision: `b1c278b38bead1fceeb80b99d2f336b0cb6fe2fd`. -Audit date: 2026-10-25. - -## Scope And Method - -This audit covers the public facade, engine, document and YAML packages, -consumer examples, and development/release entry points. It identifies private -implementation cleanup, not public features to retire or new release gates. -All findings below are open opportunities for further consideration. - -Knowledge-graph discovery and caller tracing were supplemented by a temporary, -type-aware reference inventory built outside the repository with the locally -cached `golang.org/x/tools` v0.49.0. It distinguishes declarations, production -references, test references, and direct field writes. Static inventories were -run for Linux, Darwin, and Windows; only Linux tests were executed. A separate -SSA/Rapid Type Analysis pass over test executables supplied additional leads. -Neither absent graph edges nor an unreachable result from the selected test -programs was treated as proof of dead library code. In particular, uninstantiated -generic declarations and unexercised public APIs require caller/source review. - -The standalone `deadcode`, `unused`, and `staticcheck` commands were not installed. -Building the cached standalone `deadcode` command offline encountered an uncached -telemetry dependency; the temporary analysis instead used cached packages/SSA/RTA -libraries directly. Repository dependencies and tooling were not changed. - -Candidates were checked for interface dispatch, reflection, whole-record use, -map-key equality, build constraints, public consumers, and documented retention. -The existing Linux suite and `go vet ./...` pass with Go 1.26.10. No benchmarks, -new tests, or production changes were made. - -## Architectural Assessment - -The [implemented architecture](../internal/overview.md) remains coherent. The -root owns the supported API and explicit conversions; `internal/engine` owns -synchronous acquisition, composition, inheritance, decoding, CLI application, -resolution, validation, and frozen inspection. `internal/document` owns the -ordered tree, and `internal/yaml` owns parser/dumper integration. Ordinary and -layered loading share one lifecycle. - -Most findings are remnants beside that shared lifecycle: unused snapshots, -private clone families disconnected from the facade, obsolete helper entry -points, and bookkeeping whose results no longer reach an active consumer. -No supported public feature or deliberate compatibility mechanism was established -as obsolete. Existing correctness and resource findings remain owned by their -separate audit documents. - -## 1. Remove The Unconsumed Override-Result Pre-CLI Snapshot - -Priority: medium. Scope: engine loading and override handoff. - -**Relevant code.** `decodedOverrideResult.preCLI` and `applyDecodedOverrides` -in [override_decode.go](../../internal/engine/override_decode.go), lines 9–18, -33, and 79; `preparePostDecodeOverrideContext` in -[override_context.go](../../internal/engine/override_context.go), lines 20–84. - -**Evidence.** `applyDecodedOverrides` clones the complete `decoded.final` tree -and returns it in `preCLI`. Type-aware references identify no production read -of that particular field. Its readers are assertions in `override_test.go` and -`override_context_test.go`. The production recipient instead independently clones -`before.final` into `postDecodeOverrideContext.preCLI`. - -This performs an extra traversal and allocation proportional to the decoded -tree on successful ordinary and layered loads, including loads without CLI -assignments. It does not supply the pre-CLI snapshot used by explanation. - -**Potential cleanup.** Remove the unused result member and its clone, and -update assertions tied to that member. Keep the independently owned pre-CLI -state that actually feeds resolution/publication and explanation. This is not a -proposal to share mutable trees or redesign the remaining ownership handoffs. - -## 2. Retire The Unqueried Published Structural-Input Clone - -Priority: medium. Scope: engine publication and immutable lookup. - -**Relevant code.** `Inspection.structuralInput` in -[load.go](../../internal/engine/load.go), line 96, and `publishLoadResult` in -[load_result.go](../../internal/engine/load_result.go), lines 112, 119, and 131. -Lookup ownership is in -[document_lookup.go](../../internal/engine/document_lookup.go); active replay is -in [explain.go](../../internal/engine/explain.go). - -**Evidence.** Publication clones `resolved.structuralInput`, stores the clone -on `Inspection`, and indexes it as an additional immutable tree. No production -reader accesses `Inspection.structuralInput`; one private test populates it. -Explanation replays source applications and inheritance operations, then uses -`preCLI` and `input`. It does not query this separately cloned structural root. -The lookup is addressed by a supplied node pointer, not by searching all indexed -roots, so indexing the otherwise unreachable clone does not make it useful. -Its mapping-node keys also keep the tree alive. - -**Potential cleanup.** Remove the final publication clone, unused inspection -member, and that clone's inclusion in `retainedTrees`. Preserve -`resolved.structuralInput` where it is still used for path finalization and -sensitivity classification before publication. Do not indiscriminately remove -earlier structural evidence or active source/application history. - -Coordinate this bounded removal with finding 2 in the -[resource audit](resources.md): removing an unused tree does not by itself fix -accounting for the other retained replay trees. - -## 3. Remove The Facade's Disconnected Record-Clone Family - -Priority: low. Scope: root facade, across the root/engine conversion boundary. - -**Relevant code.** The private helper block in -[records.go](../../records.go), lines 317–430: -`cloneLocation`, `cloneLocations`, `cloneSourceInfo`, `cloneImportSelections`, -`cloneSourceVersionCheck`, `cloneSourceInfos`, `cloneOrigin`, `cloneFieldInfo`, -`cloneChange`, `cloneDiagnostic`, and `cloneDiagnostics`. - -**Evidence.** None is reachable from a production facade entry point. -`cloneSourceInfos`, `cloneFieldInfo`, `cloneChange`, and `cloneDiagnostics` -have no references at all. Their supporting helpers refer to one another; -the only external callers of this family are private `records_test.go` calls -to `cloneSourceInfo`. Public results already acquire root-owned records through -`fromEngineSourceInfo`, `fromEngineOrigin`, `fromEngineFieldInfo`, -`fromEngineChange`, and diagnostic conversions in -[conversion.go](../../conversion.go). The engine's separate clone helpers are -active and are not removal candidates. - -**Potential cleanup.** Delete the disconnected root helper family. Keep relevant -conversion checks, and express ownership checks through repeated public record -queries and mutation of returned nested data rather than testing an unused -clone API. This complements the facade-test observations in the -[testing audit](testing.md); it does not eliminate explicit conversions, -root-owned types, or active `Path.clone` callers. - -## 4. Remove Uncalled Engine Helper Remnants - -Priority: low. Scope: individual engine helpers and facade transport. - -The following definitions have no callers or function-value references in -production or tests, across the inspected build configurations: - -| Definition | Relevant code | Evidence and active replacement | -| --- | --- | --- | -| `mergeDisjointMappings` | [compose.go](../../internal/engine/compose.go), line 486 | Its only reference is its own recursion. Strict composition uses `mergeDisjointMappingsAtPath` and the shared recursive merge machinery. | -| `mergeHistoryDocument` | [history.go](../../internal/engine/history.go), line 240 | No caller remains. Replay calls `mergeHistoryDocumentAtPath` directly with sequence evidence. | -| `inheritanceDirectiveLocation` | [inheritance.go](../../internal/engine/inheritance.go), line 176 | No caller remains. Active preparation uses the directive entry and its location directly. | -| `addSecretEvidencePath` | [secret.go](../../internal/engine/secret.go), line 137 | No caller remains. Classification obtains projected sensitivity paths from version evidence and feeds the active path accumulator. | -| `PlacementModeFromFacade`, `PlacementModeForFacade` | [destination.go](../../internal/engine/destination.go), lines 23 and 26 | Neither is used by the facade. Input/options decorators perform the incoming conversion; `SourceInfoPlacementModeForFacade` supplies outgoing evidence. | - -**Potential cleanup.** Delete these definitions and any imports/comments left -solely for them, keeping their active counterparts. These are private functions -or internal-package exports, not supported root API. The unused placement pair -was already identified in finding 5 of the [complexity audit](complexity.md); -this audit confirms that bounded removal without expanding its broader transport -refactor. - -## 5. Stop Constructing Unconsumed Combination Evidence - -Priority: medium. Scope: sequence combination and inheritance bookkeeping. - -**Relevant code and evidence.** - -- `sequenceCombinationResult.discardedLower` in - [sequence_combine.go](../../internal/engine/sequence_combine.go), lines 26 - and 72, is populated with a newly allocated list of every lower sequence - index during replacement. It has no production reader; three test assertions - inspect it. Overlay and inheritance owners already handle whole replacement - from the selected operation and lower value. The result therefore allocates - an unused `O(n)` index slice for an `n`-item lower sequence. -- `inheritanceReplacement.path`, `.previous`, and `.effective` in - [inheritance.go](../../internal/engine/inheritance.go), lines 135–141, are - written by [inheritance_combine.go](../../internal/engine/inheritance_combine.go) - and [inheritance_collection.go](../../internal/engine/inheritance_collection.go) - but never read by production. Populating them performs path/origin copies - and `displacedOrigins` subtree walks whose results are discarded. The active - history path uses `inheritanceOperation`, its origin maps, and - `projectedInheritanceChanges` in - [inheritance_history.go](../../internal/engine/inheritance_history.go). - The replacement's `before` and `after` trees, however, are read by retained-node - accounting in [inheritance_finalize.go](../../internal/engine/inheritance_finalize.go) - and [history.go](../../internal/engine/history.go). - -**Potential cleanup.** Remove the unread members and computations exclusively -feeding them. Preserve higher/lower item projections, replacement mappings, -`appendedHigher`, active provenance/history, and replacement node accounting. -Keep `sequenceIndexes` where append still uses it, and keep `displacedOrigins` -where overlay processing needs it. Adjust private evidence assertions while -retaining behavior-level replacement, removed-descendant, and history tests. -This consolidates duplicate evidence ownership rather than removing inspection -functionality or changing collection semantics. - -## 6. Trim Write-Only Private Result And Transport Metadata - -Priority: low. Scope: engine phase handoffs and private result shapes. - -The following members are populated but have no production reader. Some are -inspected by private tests; none is exposed as a root record or callback field. -They add apparent responsibilities to result shapes that active consumers do not -actually use: - -| Private record and members | Relevant code and evidence | -| --- | --- | -| `collectionPolicyResolution.declarations`, `declarationsPresent`, `identityDeclarationPresent`, `applicationIdentityFields` | [collection_policy.go](../../internal/engine/collection_policy.go), lines 14–24. The resolver uses local normalized declarations/fields to compute the active policy; these returned copies/flags are not consumed. Preserve `modeDeclarationPresent` and `applicationIdentityPresent`, which are consumed. | -| `inputPresence.null` | [decode.go](../../internal/engine/decode.go), line 25. Presence collection writes it, but current metadata null reporting uses the effective node's kind. The presence entry and its location remain active. | -| `rootDirectiveExtraction.importsPresent`, `sequencesPresent` | [directives.go](../../internal/engine/directives.go), lines 25–26. Parsing sets them, but callers use the parsed declaration slices and diagnostics. | -| `importSelection.outcome` | [import_selection.go](../../internal/engine/import_selection.go), line 29, and [glob.go](../../internal/engine/glob.go). Selectors set it, but traversal uses candidate lists and admitted-source counts. The separate `importSelectionEvidence.outcome` is consumed by public source-record conversion and must remain. | -| `importDeclarationSpec.excludeSet` | [import_spec.go](../../internal/engine/import_spec.go), line 38. Parsing sets it; active policy resolution consumes `exclude`, not this flag. | -| `schemaPath.field`, `dynamic` | [schema.go](../../internal/engine/schema.go), lines 335–339. Path resolution tracks and returns them, but callers consume only the descriptor and success result. | -| `selectedOverlayRequest.filesystem` | [selected_overlay.go](../../internal/engine/selected_overlay.go), line 33. Callers populate it, but the loader uses its explicit filesystem-access argument. | -| `sequenceDeclarationSpec.destinationLocation` | [sequence_declaration.go](../../internal/engine/sequence_declaration.go), line 17. The parser stores it, but later diagnostics use other retained declaration/field locations. | -| `sourceVersionEvidence.absence`, `checked`; `sourceVersionAncestor.kind` | [source_version.go](../../internal/engine/source_version.go), lines 27–43. These annotations have no production consumers. Actual checking, version/presence/location evidence, and ancestor paths are consumed separately. | -| `postDecodeOverrideContext.sources` | [override_context.go](../../internal/engine/override_context.go), line 14. The local source list is used to build resolution input, but returning it again in this member serves only tests. | - -**Potential cleanup.** Remove these unused result slots and any bookkeeping -whose sole purpose is filling them. Keep computations that validate or produce -the active result, and keep public presence, null, selection, version, ownership, -and diagnostic behavior unchanged. In particular, do not remove the source list -used for callback input, public empty-selection evidence, declaration validation, -or filesystem access. Rework assertions on inactive annotations to check the -actual supported outcome where they protect a meaningful risk. - -This is a bounded handoff cleanup, not a requirement to remove every field with -no explicit selector read: map keys, embedded records, and interface contracts -need separate treatment. - -## 7. Consolidate The Test-Only Import Directive Parser - -Priority: low. Scope: engine import parsing and test fixtures. - -**Relevant code.** `parseImportSpecificationDirective` in -[import_spec.go](../../internal/engine/import_spec.go), lines 183–201; -`parseImportSpecsYAML` in -[import_spec_test.go](../../internal/engine/import_spec_test.go); and -`parseRootDirectiveBlock` in [directives.go](../../internal/engine/directives.go). - -**Evidence.** The older parser has exactly one direct caller, the test fixture -helper. That helper feeds many selector/declaration tests, but no supported load -uses this parser. The active root parser owns the same mapping/field dispatch -and delegates import declarations to the same `parseImportDeclarations` function, -while also supporting the appropriate sequence-declaration context. This is a -parallel directive-validation implementation, not merely a thin test adapter. - -**Potential cleanup.** Have the fixture helper use the active root-directive -parser with the appropriate context, or parse the import declaration list -directly when that is the test's actual boundary. Remove the obsolete production -parser after preserving relevant malformed-declaration and selector coverage. -Do not retire the active imports-only context wholesale: layered version -processing still uses it after extracting the outer directives. - -## 8. Remove The Test-Only Eager Acquisition Mode - -Priority: medium. Scope: engine acquisition. Previously recorded in finding 1 -of the [complexity audit](complexity.md); not a second independent defect. - -**Relevant code.** `inputAcquisitionRequest.deferAssembly` in -[acquisition.go](../../internal/engine/acquisition.go), and the final branch of -`acquireImportedWithAccess` in -[import_acquisition.go](../../internal/engine/import_acquisition.go), lines 206–224. - -**Evidence.** Both production request construction sites, `executeLoadRequest` -in [load.go](../../internal/engine/load.go) and `acquireUnderlayGroupScopes` -in [load_request.go](../../internal/engine/load_request.go), set the flag to -true. Only private test entry helpers choose the eager path. Its assembly call -and alternate return protocol are therefore disconnected from supported loading. - -**Potential cleanup.** Make acquisition consistently return retained scopes. -Tests needing composition can explicitly invoke the active composition phase -through a test helper. Preserve pending-item reservation, resource closure, -accounting, diagnostics, and composition coverage. Thin test conveniences that -delegate to active implementations are not generally dead functionality; the -problem here is the alternate production phase protocol maintained solely for -tests. - -## Retained Functionality And Review Limits - -- Public exports remain supported even when current in-repository programs do - not call them. Discovery presets, `Metadata.HistoricalPaths`, generic APIs, - diagnostic codes, and optional controls are not removal recommendations. -- Heap methods, filesystem adapters, error/string/format methods, writer - methods, reflection descriptors, callback bindings, and platform-specific - implementations may be live without direct calls. They were not classified - as dead on reference counts alone. Nonzero-size identity tokens and - whole-struct map keys also have meaningful behavior without field reads. -- `decodeInlineMapEntry` and import cycle diagnostics have real production - callers despite gaps in current execution coverage. They remain necessary. -- `Inspection.resolution` has no production selector reader, but the - [recorded-resolution contract](../config.md#recorded-application-resolution) - documents retaining each recorded before/after snapshot. This audit does not - propose dropping that retention merely because explanation uses a coalesced - replay view. Any separate retention redesign must respect that contract. -- The retained inclusion graph, historical paths, sensitivity evidence, and - passive contexts serve supported behavior. Deferred experiments in - [future work](future.md#3-performance-experiments) are not obsolete features - to remove. Neither are deliberate overlay/default/version semantics. -- This audit establishes cleanup candidates, not measured performance gains or - an exhaustive proof that all unsupported input states are unreachable. - -## Validation - -The following checks passed against unchanged production code: - -```sh -env PATH=/snap/go/current/bin:$PATH GOTOOLCHAIN=local GOTELEMETRY=off \ - GOPROXY=off GOCACHE=/tmp/configkit-review-cache-shared \ - go test ./... -count=1 -env PATH=/snap/go/current/bin:$PATH GOTOOLCHAIN=local GOTELEMETRY=off \ - GOPROXY=off GOCACHE=/tmp/configkit-review-cache-shared go vet ./... -``` - -The audit document is additionally checked by the existing local documentation -link test and whitespace checks. No production code, tests, dependencies, -policies, public contracts, or release machinery are changed by this audit. diff --git a/docs/roadmap/efficiency.md b/docs/roadmap/efficiency.md deleted file mode 100644 index 4463468..0000000 --- a/docs/roadmap/efficiency.md +++ /dev/null @@ -1,245 +0,0 @@ -# Algorithmic Efficiency Audit - -Reviewed revision: `5098b56655bd383e22a57d6cf09d99db3e92090f`. -Audit date: 2026-10-25. - -## Scope And Assessment - -This source-level audit reviewed the public facade, acquisition and discovery, -strict composition, precedence assembly, collection policy, inheritance, schema -and typed-value operations, callbacks, secrets, inspection, and comparison. -Knowledge-graph searches and caller traces guided source inspection; the bounds -below follow from the actual loops and handoffs, not timing measurements. - -The implementation already uses indexed mapping merges, tuple-indexed sequence -matching, sparse item projections, scoped import adjacency, inheritance policy -localization, and worklist sensitivity propagation. The findings are remaining -batch-processing costs, not a recommendation to replace those mechanisms. - -The review excludes expressly selected algorithms, including deterministic -sorting, stable Kahn scheduling, exact numeric identity normalization, and the -pairwise registration-overlap check expressly selected in the completed sequence -plan at `d0a605a:docs/roadmap/implementation.md`. It also leaves the -[deferred performance experiments](future.md#3-performance-experiments) deferred. -Required detached copies and genuinely expanding configuration/history output -are not treated as avoidable overhead. - -Complexities describe the identified internal work, not an end-to-end runtime -bound or a new consumer guarantee. Examples use bounded-depth paths and -constant-sized records unless noted; hash-index alternatives use expected lookup -costs, with string processing and actual copied payload accounted for separately. -Existing resource limits bound realizable inputs but do not remove these growth -patterns. No new limits, public caches, dependencies, or release gates are proposed. - -No additional functional defect was established during this pass. The four open -findings in the separate [functional correctness audit](correctness.md) remain -applicable and are not duplicated here. - -## 1. Policy Retirement Scans Every Collection For Every Replacement - -Status: open. Priority: medium. - -**Relevant code.** `collectionPolicyScope.retireCollectionPoliciesAtOrBelow` in -[collection_policy_state.go](../../internal/engine/collection_policy_state.go), -lines 163–174, scans the complete policy table. The replacement hook in -`mergeOverlayMappingsWithCollectionPolicy` -([overlay.go](../../internal/engine/overlay.go), lines 182–185) calls it for each -ordinary replaced value. `applyOverlaySequenceEvidence` also calls it for -whole-collection replacement. - -**Scaling case.** A lower tier has P independently declared collection policies -and Q ordinary scalar settings elsewhere. A higher tier changes those Q scalar -settings without touching a collection. Each scalar replacement scans all P -policies even though none is affected: O(P × Q) policy visits. Taking P = Q = N -gives quadratic work for linear-sized inputs, effective values, and policy state. -This occurs within one precedence merge; it does not depend on many tiers, -registration-overlap checking, or expanding inheritance output. - -**Potential refactoring.** Prepare an operation-local semantic path index over -the policy table and use descendant queries for retirement, maintaining it as -policies change. Alternatively, batch retirement against the old policy state -where its timing can be preserved. For the disjoint example, preparation plus -queries can be O(P + Q); generally charge additional work to affected entries -and path lengths. Preserve explicit-mode persistence, record ownership, and the -point at which old identity is retired. A delayed blanket retirement must not -clear newly established higher-tier policy. Existing inheritance policy indexing -provides a related mechanism to evaluate, not a requirement to unify its interface. - -## 2. Small Tiers Recopy Unrelated Accumulated Collection Policy - -Status: open. Priority: medium. - -**Relevant code.** `resolveStrictCollectionPolicies` in -[compose.go](../../internal/engine/compose.go), lines 330–334, first obtains -`effectiveLower` through `lowerCollectionPoliciesForClaims`, then clones the -result. The latter helper -([collection_retirement.go](../../internal/engine/collection_retirement.go), -lines 65–71) already deep-clones and scans the entire lower scope. -`cloneCollectionPolicyScope` / `cloneCollectionPolicy` in -[collection_policy_state.go](../../internal/engine/collection_policy_state.go) -also copy record-order and nested record-policy state. - -`assembleLayeredInput` in [load_request.go](../../internal/engine/load_request.go) -supplies the accumulated lower policy when composing each underlay group and -when applying that group to the accumulator. The overlay resolver repeats this -whole-table preparation and additionally checks all retained policy destinations -against the incoming tree (`overlay.go`, lines 145–150). - -**Scaling case.** An initial underlay establishes P constant-sized collection -policies. Each of T later underlays contributes only one unrelated scalar, with -no declarations or collection changes. Despite having no relevant collection -work, each tier copies/scans all P policies: O(T × P) work and cumulative -allocation. P = T = N yields quadratic bookkeeping for O(P + T) input and final -policy/value sizes. These are transient copies of unchanged private policy, not -T separately published snapshots required by the public ownership contract. - -**Potential refactoring.** Separate read-only lower-policy lookup from the owned -policy changes produced by the current tier. Resolve only policies intersecting -incoming claims/declarations, and apply those changes to the owned assembly -state. A path index or localized view can avoid scanning unrelated entries; -retain copies where state actually crosses a mutable ownership boundary. At -minimum, avoid the immediate duplicate deep copy, but removing only that copy -does not change the O(T × P) growth. Aim for work proportional to initial state, -incoming material, and affected policy rather than the complete accumulated -table per tier. Preserve pending declarations, replacement eligibility, and -independent nested record-policy ownership. - -This is distinct from finding 1: one tier can avoid repeated retirement scans -and still recopy the full table at every subsequent tier. It is also distinct -from the deferred metadata-copy/public-record handoff experiment. - -## 3. Tier-History Projection Repeatedly Searches Wide Mappings - -Status: open. Priority: medium. - -**Relevant code.** `projectTierHistoryApplications` in -[sequence_history_projection.go](../../internal/engine/sequence_history_projection.go), -lines 17–30, calls unindexed `documentAtPath` for each mapped sequence in a source -application. That helper delegates to `documentAtPathWithLookup` with no lookup; -[document_walk.go](../../internal/engine/document_walk.go), lines 51–58, then -linearly searches each mapping on the path. - -**Scaling case.** A source has N sibling collections under `collections`, each -with a constant number of records. A single wildcard registration can identify -all of them. When this tier upserts/appends over lower collections, the source -application contains N sequence contributions requiring projection. Locating -each `/collections/` searches the same N-entry mapping, costing -1 + 2 + ... + N entry comparisons: O(N²), despite linear-sized source material -and projection output. This does not require N application registrations. - -**Potential refactoring.** Lazily build one read-only `documentLookup` for each -original application value with active mapped contributions and reuse it, or -collect the relevant sequence nodes in one traversal. With V input nodes, S -contributions, maximum depth D, and I projected items, the lookup/projection -portion can be O(V + S × D + I), rather than repeated wide scans. Preserve -authored coordinates, noncontiguous item projections, replacement evidence, -and independent output storage. This concerns load-time coordinate projection, -not the deferred branch-local explanation-replay experiment. - -## 4. Compound Identity Restoration Rescans Both Records Per Field - -Status: open. Priority: medium. - -**Relevant code.** `preserveInheritanceIdentityFields` in -[inheritance_scope.go](../../internal/engine/inheritance_scope.go), lines 143–161, -searches the child's mapping for each identity field, then searches the merged -mapping again. `combineInheritanceTemplate` invokes it after merging the parent -payload with the child in -[inheritance_combine.go](../../internal/engine/inheritance_combine.go), line 104. - -**Scaling case.** For K identity fields, C child entries, and M merged entries, -the searches cost O(K × (C + M)), plus the required identity-value clones. -A single parent/child relationship with K = C = M = N direct scalar identity -fields performs quadratic comparisons for linear-sized records and identity -metadata. There is no declared small bound on compound-identity width. The -shared identity validator and parent-identity exclusion already use mapping or -membership indexes; this restoration step does not. - -**Potential refactoring.** Index merged entry positions once and walk child -entries against an identity-field set, or use equivalent operation-local mapping -indexes. This can be O(K + C + M), plus required copies. Preserve child-owned -identity values and locations, existing mapping order, and detached nodes; do -not change parent lookup or identity equality. If the step proves redundant -after the established merge, removal needs a separate equivalence review rather -than an assumption that identity preservation is unnecessary. - -## 5. Secret Failure Reporting Repeats Unindexed Location Searches - -Status: open. Priority: low; affects batches of failures. - -**Relevant code.** `resolveSecretValue` reports each failed reference through -`secretDiagnostic` in -[secret_reference.go](../../internal/engine/secret_reference.go), lines 53–78 and -112–117. That helper calls unindexed `nearestDocumentLocation`; the mapping -branch of `nearestDocumentLocationWithLookup` in -[validation.go](../../internal/engine/validation.go), lines 144–155, falls back -to a linear search when no lookup is supplied. - -**Scaling case.** An N-entry mapping of secret references whose environment -values are all absent produces N diagnostics. Each location lookup starts at -the root and scans the mapping for its key, yielding O(N²) comparisons for -O(N) references and diagnostics. Reusing the same environment name still -reproduces the lookup pattern even though environment lookup itself is cached. -Deterministic traversal and diagnostic sorting add their ordinary O(N log N) -costs; they do not require the quadratic location work. - -**Potential refactoring.** Share a lazily initialized read-only `documentLookup` -for the duration of secret resolution, using `nearestDocumentLocationWithLookup` -for failures, or carry matching document/location state through traversal. For -V input nodes and F failed references at depth D, attribution can be -O(V + F × D), before existing diagnostic sorting. Keep the success path free -of an eager index if it is unnecessary. Preserve all diagnostics, nearest-known -ancestor fallback, source coordinates, and safe messages; failing fast instead -would change behavior rather than optimize it. - -## 6. Dynamic Values Repeat A Type-Graph Eligibility Walk - -Status: open. Priority: low; most relevant to repeated dynamic container types. - -**Relevant code.** `valueOperation.dynamicDescriptor` in -[value_operation.go](../../internal/engine/value_operation.go), lines 36–48, -uses the schema descriptor cache but still calls `descriptorContainsSecret` -with a fresh visited map on every value. That helper (lines 51–68) traverses -the reachable descriptor graph. Dynamic branches in projection and cloning -invoke it for every non-nil interface value, before handling the concrete value. - -**Scaling case.** Let N dynamic values have the same concrete pointer type whose -descriptor graph has size S (nodes plus member/element edges). Each call rechecks -the same graph, costing O(N × S), even after descriptor construction is cached. -For example, typed defaults can supply N interface entries holding nil pointers -to one wide, supported, secret-free struct type. Actual values and projected -output contain only N nulls, while the struct has S fields. Taking N = S makes -the repeated eligibility walk quadratic in the combined type/value description, -without requiring quadratic value expansion. - -**Potential refactoring.** Remember successfully approved dynamic descriptors -by concrete type within `valueOperation`, after complete construction and the -secret-semantics check. The repeated-type case becomes O(S + N) eligibility -work, with one entry per approved concrete type. Do not treat a traversal-local -visited marker as a completed negative result for a cyclic descriptor graph. -Do not cache partial descriptors or path-bearing failures; coordinate with -finding 2 of the [correctness audit](correctness.md), which identifies failed -construction cache poisoning. Preserve rejection of dynamic secret-reference -types, including nil pointers to them. This is operation-local reuse, not the -deferred cross-load schema cache or a redesign of typed cloning. - -## Validation And Recommended Order - -Existing focused tests passed on Go 1.26.10, Linux amd64: - -```sh -go test ./internal/engine -run 'Collection|Sequence|Inheritance|Secret|Dynamic' -count=1 -go test . -run '^TestLocalDocumentationLinks$' -count=1 -git diff --check -``` - -No benchmarks, timing comparisons, or new tests were needed to establish the -loop bounds. Passing tests do not establish efficient growth. Production code -and existing correctness findings are unchanged by this audit. - -Consider findings 1 and 2 together when selecting a policy-state refactoring, -while keeping their acceptance cases distinct. Findings 3 and 4 are narrower -independent changes; findings 5 and 6 are lower-priority follow-ups. These are -opportunities for consideration, not an implementation sequence or new release -blockers. Any later refactoring should preserve the existing contracts and use -focused behavioral coverage without adding performance thresholds. diff --git a/docs/roadmap/error-handling.md b/docs/roadmap/error-handling.md deleted file mode 100644 index c095d1a..0000000 --- a/docs/roadmap/error-handling.md +++ /dev/null @@ -1,158 +0,0 @@ -# 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. diff --git a/docs/roadmap/resources.md b/docs/roadmap/resources.md deleted file mode 100644 index e1ee559..0000000 --- a/docs/roadmap/resources.md +++ /dev/null @@ -1,189 +0,0 @@ -# Resource Management Audit - -Reviewed revision: `0cca633a67abd3d0fdcb7db6e83260dcaf1eaac8`. -Audit date: 2026-10-25. - -## Scope And Architecture - -The audit covered input snapshots and readers, discovery, filesystem acquisition, -imports and selected overlays, typed projection and cloning, composition and -inheritance, callback teardown, retained inspection history, rendering/diff, and -secrets bootstrap. Knowledge-graph discovery and caller tracing were supplemented -by source inspection, existing tests, and small standalone diagnostics outside -the repository. No benchmarks or large-memory stress tests were conducted. - -The [implemented architecture](../internal/overview.md) has a mostly clear -resource-ownership model: - -- The public facade delegates to a synchronous engine lifecycle, with private - document and YAML packages. Loading creates no library-owned background - goroutines, timers, network connections, or shutdown service. -- Ordinary file reads close owned handles, including read/stat failures. Host - import and overlay acquisition own their root handles; caller-supplied readers - and filesystems remain borrowed. -- Published inspection retains immutable documents, history, indexes, and passive - resource contexts. A later explicit resource open transfers ownership of that - returned file to the caller; publication does not normally retain acquisition - handles. -- Retained history and detached query results are intentional ownership choices, - not leaks by themselves. The findings below concern enforcing existing limits - and closing owned resources, rather than removing that functionality. - -Three new findings warrant remediation. All remain open. - -## 1. Typed Containers Allocate Before Their Node Budget Is Enforced - -Priority: medium. Category: peak memory and failure handling. - -**Relevant code.** `projectSequence` and `projectMap` in -[projection.go](../../internal/engine/projection.go), `valueOperation.charge` in -[value_operation.go](../../internal/engine/value_operation.go), and the defaults -projection in [decode.go](../../internal/engine/decode.go). - -**Circumstances.** Sequence projection reserves `value.Len()` document pointers -before visiting any children. Map projection similarly reserves the complete -entry array and obtains all map keys before charging individual entries. The -existing node limit therefore rejects an oversized typed value only after a -potentially much larger allocation has already occurred. Defaults and values -returned by application resolution can reach these paths. - -**Evidence.** A standalone public-API diagnostic loaded `NoSource()` with defaults -containing an `Items []struct{}` field initialized with -`make([]struct{}, int(^uint(0)>>1))`. Zero-sized elements make the input slice -inexpensive, but projection immediately panicked with -`runtime error: makeslice: cap out of range`. It did not return the existing -`schema.projection_limit` diagnostic. No enormous backing array was successfully -allocated by this diagnostic. - -**Consequence.** Less extreme oversized containers can cause excessive temporary -memory allocation before rejection; sufficiently large lengths panic or exhaust -memory instead of reaching the intended limit failure. This is a library-created -allocation, distinct from memory an application has already allocated itself. - -**Potential remediation.** Before bulk reservations, compare the known minimum -node cost of the container against the operation's remaining existing allowance, -using overflow-safe arithmetic. Alternatively, bound initial capacity and grow -only as successfully charged elements are accepted. Review the analogous map -reservation/key collection. Preserve current node/depth semantics, inline-field -behavior, and useful diagnostic paths; do not introduce an unrelated container -length cap or promise a universal heap-size bound. - -## 2. Retained-History Accounting Omits Published Replay Trees - -Priority: medium. Category: retained memory and lifecycle integration. - -**Relevant code.** The history check in `executeLoadRequest` in -[load.go](../../internal/engine/load.go), `historyNodesWithin` in -[history.go](../../internal/engine/history.go), `preparePostDecodeOverrideContext` -in [override_context.go](../../internal/engine/override_context.go), and -`resolveLoadInput` / `publishLoadResult` in -[load_result.go](../../internal/engine/load_result.go). - -**Expected behavior.** The -[retained-history contract](../config.md#historical-value-explanation) limits retained -history to 4,000,000 distinct document nodes, counting shared nodes once and -reporting `explain.limit_exceeded` when exceeded. - -**Actual behavior.** The load check visits the original decoded baseline and -pre-CLI final tree, the resolved final tree, application trees, resolution deltas, -and the acquisition inheritance scope. Publication retains a different root set: -it includes a separately cloned post-CLI decoded tree as inspection input, plus -independent `structuralInput` and `preCLI` replay snapshots. The replay snapshots -are cloned again during publication and included in its immutable lookup. They -are not passed to the budget check. Inheritance operation copies are also made -after that check. Counting the earlier root set does not bound the distinct -nodes actually kept alive by the result. - -**Evidence.** A successful public load of `items: [1, 2]`, decoded into -`struct { Items []int }` with a no-op resolver, was inspected by a read-only -standalone reflection diagnostic. Its baseline/input/final/application roots -contained 14 distinct nodes; including the two published replay roots yielded -22. The checkpoint uses a different decoded-final instance of the same size in -this example. Source inspection establishes that its count is also 14. - -For this fixture generalized to N scalar items, the checkpoint counts `3N + 8` -nodes, while the published roots retain `5N + 12`. Thus 800,000 items would pass -the history checkpoint with 2,400,008 nodes yet retain 4,000,012. Such a source -also fits the existing per-source byte and node allowances. This larger case is -a structural derivation, not an executed allocation test. - -**Consequence.** Successful loads can exceed the established retained-history -limit, with additional retained lookup/index overhead. The quota currently gives -an incomplete account of the memory-owning snapshot graph. - -**Potential remediation.** Apply one pointer-aware accounting operation to the -complete set of document roots actually retained at publication, including replay -snapshots and copied inheritance history. Preserve count-once semantics for -shared nodes. Where practical, charge or reserve these copies before creating -them so failure does not first incur avoidable peak allocation. Reusing an -already-owned immutable snapshot can be considered where equivalent, but this -finding does not require the broader metadata-copy redesign deferred in -[future experiments](future.md#3-performance-experiments), weaker detachment, a -smaller limit, or removal of historical inspection. - -## 3. An Imported Version-Check Panic Leaves The Host Root Open - -Priority: medium. Category: file-descriptor cleanup during unwinding. - -**Relevant code.** `acquireImportedInput` / `acquireImportedWithAccess` and imported -version processing in -[import_acquisition.go](../../internal/engine/import_acquisition.go), together with -`openHostAccess` / `filesystemAccess.close` in -[filesystem_access.go](../../internal/engine/filesystem_access.go). Compare the -deferred fallback cleanup in -[selected_overlay.go](../../internal/engine/selected_overlay.go) and deferred file -cleanup in [source_file.go](../../internal/engine/source_file.go). - -**Circumstances.** Host import acquisition opens an `os.Root`, then closes it -explicitly on ordinary failure paths and after import traversal. It has no -deferred fallback closure. A user version-check callback runs for imported -sources while that root is open. If it panics and the application recovers outside -`Load`, unwinding bypasses all those explicit root closes. The imported file's -own deferred close still runs; the outstanding resource is the directory/root -handle. - -**Evidence.** A standalone Linux diagnostic created a temporary main file and one -import, both with a version, enabled source-version checks, and panicked only on -the imported source's check. With automatic GC disabled only in that disposable -diagnostic process, the number of `/proc/self/fd` entries targeting the temporary -root went from zero to one after external recovery. The non-panicking control -ran both checks and returned to zero. One outstanding root was sufficient to -demonstrate the issue; no descriptor-exhaustion workload was run. - -**Consequence.** Library-owned root cleanup depends on eventual runtime cleanup -instead of load-scope unwinding. Repeated recovered callback failures can retain -descriptors long enough to exhaust them. This does not imply that ConfigKit must -recover application panics or successfully return a configuration after one. - -**Potential remediation.** Install a deferred fallback close at the owning -acquisition boundary, while preserving normal explicit close timing and close -error diagnostics, as the selected-overlay path already does. Avoid double-close -confusion and leave borrowed native filesystems untouched. Preserve panic -propagation. This is separate from -[correctness finding 4](correctness.md#4-callback-contexts-remain-active-after-panic-unwinding): -deactivating a callback context alone does not close the root, and closing the -root alone does not deactivate that context. - -## Validation And Boundaries - -The following passed with Go 1.26.10 on Linux amd64, using cached dependencies -with automatic toolchain/network acquisition disabled: - -```sh -go test ./internal/engine -run 'Closes|Close|Budget|Limits|Limit|SecretsDir' -count=1 -go test ./... -count=1 -``` - -The three standalone diagnostics described above were run outside the repository; -they added no production code or repository tests. The history diagnostic only -read private fields to count pointers; it did not mutate inspection state. No -native macOS or Windows checks were performed. - -No additional network, timer, goroutine, buffer-pool, or ordinary-return handle -leak was established. Borrowed blocking readers/callbacks do not create a new -cancellation guarantee. Previously reported recursive-schema stack exhaustion -and callback-state retention remain owned by the -[correctness audit](correctness.md), rather than duplicated here. Existing -algorithmic and maintainability findings remain owned by their respective -audits. This report adds no API, release gate, new resource quota, or background -management requirement. diff --git a/docs/roadmap/security.md b/docs/roadmap/security.md deleted file mode 100644 index 8ebde95..0000000 --- a/docs/roadmap/security.md +++ /dev/null @@ -1,194 +0,0 @@ -# Security Audit - -Reviewed revision: `b30c9f167dba16a9192d9bfde4aeb2b9c597ce0e`. -Audit date: 2026-10-25. - -## Scope And Trust Model - -This review examined input validation and budgets, import/discovery containment, -link resolution, collection authority, application callbacks, secret references -and bootstrap, historical sensitivity, diagnostics, effective rendering and -diffs, and the runtime/development dependency boundary. Knowledge-graph searches -and caller traces were supplemented by source inspection, existing tests, a -bounded standalone diagnostic, and an advisory-database spot-check. - -The [architecture](../policy/architecture.md) and -[configuration contract](../config.md) establish these boundaries: - -- ConfigKit is a synchronous startup library, not a network service or an - authentication/authorization framework. Applications select inputs, roots, - filesystem implementations, callbacks, and credential use. Applications own - remote acquisition and authentication of that data. -- The public facade delegates to a private engine and private document/YAML - packages. Input composition and inspection do not execute shell commands or - launch a background service. Ordinary loading does not change the process - environment; explicit secrets-directory bootstrap is the narrow exception. -- Host imports use canonical containment checks and owned rooted access under - the documented stable-input assumption. Borrowed native filesystems retain - their own behavior and are explicitly not a general sandbox guarantee. -- Secret references resolve after composition and application resolution. The - trusted returned configuration can expose credentials through `Value()`; - inspection retains names rather than credentials. Tags, a path predicate, - inheritance, and recorded dependencies govern redaction of other values. - Application-supplied messages and unrecorded secret transformations remain - application responsibilities. - -One new availability finding warrants remediation. No new credential disclosure, -command injection, application-authority bypass, or host containment escape was -established. This is a source-level audit with bounded checks, not a certification -that every input, dependency, platform, or deployment is secure. - -## 1. Finite Symlink Reuse Can Cause Exponential Import-Resolution Work - -Status: open. Priority: medium. Category: availability / input amplification. - -**Relevant code.** `resolveHostImportPath` and `resolveNativeImportPath` in -[filesystem_links.go](../../internal/engine/filesystem_links.go), lines 129–242, -expand link-target components with a frame stack and track only currently active -links. Completion removes a link from that set; a later use resolves its entire -target again. No completed-target result is reused. - -`importEntryTarget` invokes these resolvers for discovered links. Both -[directory selection](../../internal/engine/import_selection.go) and -[glob selection](../../internal/engine/glob.go) reach it. Directory selection -resolves the target before deciding whether recursive descent is enabled, so -`recursive: false` does not avoid this work. Native resolution is used when the -borrowed filesystem provides `fs.ReadLinkFS`; host resolution uses the analogous -rooted implementation. - -**Plausible scenario.** An application loads a configuration tree whose imported -assets can be supplied by a less-trusted user or configuration provider. That -provider controls link entries inside an allowed import directory. The -application's own main file need only select that directory with link following -enabled, which is the current default. No concurrent filesystem mutation, -outside-root access, executable callback, or malicious filesystem implementation -is required. This is conditional on influence over imported filesystem data; -it is not an anonymous remote vulnerability in ConfigKit by itself. - -Inside an ordinary `links` directory, create this finite graph: - -```text -s00 -> . -s01 -> s00/s00 -s02 -> s01/s01 -s03 -> s02/s02 -... -``` - -Each link resolves to the same contained directory. It has no active link cycle: -the first reference to a lower link finishes before the second begins. Resolving -level N nevertheless repeats the level N-1 expansion twice, giving a recurrence -of `T(N) = 2T(N-1) + O(1)` component work for this fixed-name fixture. Directory -enumeration contains only N+1 entries, and no imported configuration nodes or -new import edges account for those repeated expansions. Consequently, source, -entry, input-node, and active-import-depth budgets do not bound this work by the -size of the selected graph. This is repeated resolution of a compact graph, not -genuinely expanding configuration output or ordinary deterministic sorting. - -**Evidence.** A standalone public-API diagnostic created real temporary symlinks -and exposed them through `NewFilesystem` backed by `os.DirFS`. A thin -`fs.ReadLinkFS` wrapper counted metadata/link operations and imposed a diagnostic -ceiling of 12,000 `Lstat` calls. The main input selected `dir: links` with link -following enabled and default nonrecursive discovery. Successful loads produced -these counts: - -| Link entries | `Lstat` calls | `ReadLink` calls | -| --- | ---: | ---: | -| 3 | 32 | 11 | -| 5 | 128 | 57 | -| 7 | 512 | 247 | -| 9 | 2,048 | 1,013 | -| 11 | 8,192 | 4,083 | - -A 13-entry fixture reached the diagnostic ceiling and returned a safe load -failure when the wrapper stopped it. The same fixture with `follow: false` -completed with 17 `Lstat` calls and zero `ReadLink` calls. The operation counts -substantiate amplification in the native resolver; the host resolver's matching -frame-expansion logic was verified by source inspection and caller tracing, -not a separate host operation-count test. No timings, large attack graph, or -external-system exploitation were used. - -**Impact.** A small imported asset tree can monopolize startup/load execution for -an impractically long period. Applications that expose configuration validation -or repeated loading to less-trusted providers can also tie up their own workers. -The normal trusted-operator-only deployment reduces the adversarial exposure, -but accidental alias graphs can cause the same availability failure. - -**Potential remediation.** Reuse fully resolved link-target results within the -resolution operation, keyed by the appropriate rooted namespace and full link -path, while retaining active-path cycle detection and containment checks. -Equivalent dependency-based evaluation is another option. Preserve legitimate -finite reuse, relative/absolute host-link semantics, and the stable-input model; -do not replace the active set with a global "already visited means cycle" rule. -Review both resolvers and directory/glob entry points. The objective is to avoid -re-expanding completed dependencies, not to introduce a public cache or reject -valid graphs with an arbitrary link-count cap. Consumers can already opt out of -following discovered links where appropriate; changing that default is not part -of this recommendation. - -## Previously Reported Risks - -The [correctness audit](correctness.md) already records pointer-only recursive -schema nontermination, failed dynamic-descriptor cache state, and callback guards -left active during panic unwinding. The [resource audit](resources.md) owns -pre-budget typed-container allocation, incomplete retained-history accounting, -and an import-root cleanup gap after callback panic. These can affect availability -when their documented preconditions hold; they are not newly discovered remote -exploits or duplicate remediation tasks here. - -The [error-handling audit](error-handling.md) separately owns silently omitted -directory contributions after certain enumeration/close failures. The -[efficiency audit](efficiency.md) owns previously identified avoidable batch costs. -This review does not reclassify every expensive operation or application -programming error as a security vulnerability. - -## Dependency Spot-Check - -The ordinary package's non-standard-library dependency graph contains only -`go.yaml.in/yaml/v4 v4.0.0-rc.6`; ReleaseKit, Goldmark, and `x/mod` are maintainer -tool dependencies rather than ConfigKit runtime imports. Cached module integrity -verification passed. - -The current [Go vulnerability database module index](https://vuln.go.dev/index/modules.json) -contained no entries for the selected YAML v4, ReleaseKit, or Goldmark v2 module -paths. Its two `golang.org/x/mod` reports, -[GO-2026-6179](https://pkg.go.dev/vuln/GO-2026-6179) and -[GO-2026-6180](https://pkg.go.dev/vuln/GO-2026-6180), identify v0.40.0 as fixed, -which is the version selected here. The related toolchain fixes also precede the -Go 1.26.10 toolchain used for this audit. The database metadata reported a modified -time of `2026-10-21T00:27:29Z` when checked. - -This was a module-path/advisory spot-check, not a complete standard-library, -toolchain, transitive-reachability, or supply-chain attestation. `govulncheck`, -`gosec`, and `gitleaks` were not installed; no scanners were added to the project. -Absence of an advisory is not proof of absence of vulnerabilities, and -`go mod verify` does not audit dependency behavior or establish that upstream -content is safe. -The [database API documentation](https://go.dev/doc/security/vuln/database) -describes the advisory source used for this check. - -## Validation And Preserved Boundaries - -The following passed with Go 1.26.10 on Linux amd64 using cached dependencies and -disabled automatic dependency/toolchain acquisition: - -```sh -go test -race ./... -count=1 -go test ./internal/engine -run 'Secret|Sensitive|Sensitivity|Contain|Escap|Outside|Symlink|Nonregular|Budget|Limit|Diagnostic' -count=1 -go vet ./... -go mod verify -``` - -These checks include existing canary-based non-disclosure, historical and -bilateral diff sensitivity, safe diagnostic, parser-policy, containment, and -nonregular-file cases. The bounded link diagnostic ran outside the repository. -No production code or repository tests were added or changed; no native -macOS/Windows run, active external exploitation, or exhaustive fuzzing was done. - -This report does not propose authentication in the library, arbitrary environment -name restrictions, secret-file permission requirements, retries, memory erasure, -or protection against privileged concurrent filesystem mutation. Those would -need their own concrete requirement and threat analysis. It preserves borrowed -filesystem ownership, application-owned safe callback messages, public secret -reference names, explicit partial bootstrap semantics, and the current release -gates. diff --git a/docs/roadmap/testing.md b/docs/roadmap/testing.md deleted file mode 100644 index f7091fc..0000000 --- a/docs/roadmap/testing.md +++ /dev/null @@ -1,281 +0,0 @@ -# Test Coverage And Effectiveness Audit - -Reviewed revision: `eb5a82f1f3a3286506c2d527e7980db7ccae5398`. -Audit date: 2026-10-25. - -## Assessment And Architecture - -The suite broadly follows the [testing policy](../policy/testing.md): real -parsing, composition, decoding, and filesystem fixtures dominate; narrow doubles -control I/O and environment failures. Engine tests own mechanisms and semantics, -root tests protect public conversions and integration, and maintained consumers -exercise representative workflows. The private document tree and YAML adapter -support the synchronous engine behind the root-owned public facade; see the -[component overview](../internal/overview.md). - -Strengths include presence/default/zero distinctions, strict composition versus -precedence, collection policy, inheritance/history agreement, safe rendering and -bilateral diff sensitivity, independent loads, reader claims, callback ordering, -bootstrap partial failures, and realistic source/containment behavior. Parser and -path fuzz targets have deterministic seeds in the ordinary suite. Architecture, -documentation, and development-check tests protect explicit repository contracts. - -Six targeted findings follow: four coverage/assertion improvements and two areas -for removal or consolidation. They do not justify a wholesale test purge, a -percentage target, or another test for every private helper. No new production -defect was established during this pass; the existing audits own their findings. - -## Coverage Evidence - -On Go 1.26.10, Linux amd64, the ordinary suite listed 771 `Test` functions, three -fuzz targets, and 29 executable examples. These counts exclude Windows-only -compiled tests and do not count table cases as separate test functions. - -Cross-package coverage, aggregated by source block across all test binaries, was: - -| Package | Covered statements | Coverage | -| --- | ---: | ---: | -| Root public package | 348 / 428 | 81.3% | -| `internal/engine` | 9,089 / 9,987 | 91.0% | -| `internal/document` | 13 / 13 | 100.0% | -| `internal/yaml` | 151 / 168 | 89.9% | -| Maintained consumer examples | 259 / 327 | 79.2% | -| Total | 9,860 / 10,923 | 90.3% | - -The document package has no standalone tests, but its code is covered through -real collaborators. The per-test-binary percentages printed by `-coverpkg=./...` -have a different denominator and must not be mistaken for these aggregate -package percentages. Statement coverage measures execution, not assertion -strength or state-space coverage: the current suite passes despite the defects -already recorded in the correctness, resource, error-handling, and security audits. - -Some zero-coverage private functions have no production caller; trivial public -preset forwards are also unexecuted. Neither observation alone warrants adding -tests. Prioritize the behavioral risks below over filling those percentages. - -## 1. Confirmed Exceptional Cases Lack Durable Regression Coverage - -Status: open. Priority: medium. Category: gaps in state transitions and boundaries. - -The earlier audits reproduced defects outside the checked-in suite. Relevant -tests exercise nearby ordinary cases but do not yet protect these transitions: - -- **Schema and dynamic-value failure state.** - [schema_test.go](../../internal/engine/schema_test.go) covers ordinary recursive - structs; [projection_test.go](../../internal/engine/projection_test.go) checks - independent invalid values and cycles. Neither covers pointer-only recursive - descriptors, acyclic slice views sharing a starting pointer, or a second - dynamic/helper operation after a failed descriptor build in the same load. - Add the narrowly scoped regressions described by - [correctness findings 1–3](correctness.md), retaining null/acyclic success cases - and asserting safe classification and termination. Avoid an unbounded - stack-overflow/hang fixture in the default test process. -- **Exceptional callback exit.** The retained-context tests in - [resolution_test.go](../../internal/engine/resolution_test.go) and - [facade_context_test.go](../../facade_context_test.go) verify normal callback - return. File-close tests cover ordinary load failures. They do not exercise - external recovery after version/resolution/validation callback panic or an - imported version check while a host root is owned. Extend the existing owners - when fixing [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). - Preserve propagation of the original panic and verify closure of the callback - state and the actually owned root independently; a native file-close counter - alone cannot verify an `os.Root` lifetime. -- **Budgets before allocation and through publication.** - `TestProjectionRejectsNonFiniteLimitsAndCodecFailuresSafely` checks a small - scalar tree against supplied limits. `TestHistoryNodeBudgetCountsSharedTreesOnce` - and the inheritance-history budget tests verify the accounting helper with - explicitly supplied roots. They cannot detect a container reservation before - charging, or roots omitted by the publication caller. Add bounded regressions - with [resource findings 1–2](resources.md): oversized zero-sized-element input - can avoid a huge input allocation, and a small controlled history allowance can - test the complete retained-root boundary. Keep count-once sharing coverage; - do not allocate millions of nodes just to cross the production default. -- **Nested finite link expansion.** - [filesystem_links_test.go](../../internal/engine/filesystem_links_test.go) and - the public finite-reuse test cover simple reuse and actual cycles, but not the - compact repeated-dependency graph in [security finding 1](security.md#1-finite-symlink-reuse-can-cause-exponential-import-resolution-work). - When that remediation is selected, use a small real link fixture and a bounded - operation observer at the filesystem boundary. Protect finite reuse and - nonrecursive discovery without running an exhaustion workload or adding a - wall-clock performance threshold. - -These are test accompaniments to the original remediation owners, not duplicate -product fixes or a request for a broad new cross-product matrix. The existing -tests should remain where they protect distinct ordinary behavior. - -## 2. Public Historical-Path Enumeration Is Not Exercised - -Status: open. Priority: medium. Category: public facade integration. - -**Relevant code/tests.** `Metadata.HistoricalPaths` in -[metadata.go](../../metadata.go), lines 49–56, has zero executed statements in -the aggregate profile. Every checked-in `HistoricalPaths` test call is in the -engine package. `TestMetadataHistoricalPaths` in -[metadata_test.go](../../internal/engine/metadata_test.go) thoroughly covers the -engine method, including removed descendants, escapes, contextual numeric -tokens, and detached results, but does not cross the public conversion. - -**Risk.** Returning no paths or incorrectly converting the numeric segment intent -at the root boundary could leave that engine suite green while breaking the -documented consumer enumeration/query workflow. Coverage of `History` and -`Lookup` separately does not exercise enumeration's result conversion. - -**Improvement.** Add one small external-package public-API test that enumerates -effective and removed paths, then uses the returned paths with `Lookup` and -`History`, including a numeric-key/sequence shape change. Check sorted unique -pointer spelling, zero metadata, and replacement of a returned slice element -without affecting later queries. Keep detailed path mutation and engine shape -cases with their existing owner; do not duplicate the complete internal matrix -or expose private segments to test the facade. - -## 3. Native-Filesystem Failure Tests Observe Too Little - -Status: open. Priority: medium. Category: doubles and inadequate assertions. - -**Relevant tests.** `TestNativeFilesystemClosesFilesOnSuccessAndFailure`, -`observedFilesystem`, and `observedFile` in -[native_filesystem_test.go](../../internal/engine/native_filesystem_test.go), -especially lines 150–162 and 278–319. The fixture is also used by -`TestLoadingClosesNativeFilesAcrossPipelineOutcomes` in -[resource_context_test.go](../../internal/engine/resource_context_test.go). - -**Deficiencies and plausible survivors.** - -- Failure cases require only a non-nil error, not a nil result, the expected - diagnostic classification, or evidence identifying the intended failed - source. An unrelated early failure could satisfy the assertion. -- The non-disclosure check searches only `err.Error()` for the fixture canary. - `LoadError.Error` deliberately renders an error count, not diagnostic messages. - Accidentally forwarding the raw filesystem error into `Diagnostics()` could - therefore pass this check. The parser/callback canary tests cover different - failure-producing boundaries. -- `assertClosed` checks only total opens versus total `Close` calls. The counter - increments even for a repeated close, so one leaked handle and one double-close - can cancel out. It does not prove that each acquired handle was released. -- The double supplies immediate generic read/directory errors and successful - closes. It cannot model entries/data plus an error, or a close error whose - identity resembles absence. This leaves the - [directory failure-phase defect](error-handling.md#1-directory-read-and-close-failures-can-be-misclassified-as-absence) - unprotected on both the batched and fallback paths. - -**Improvement.** Keep the real in-memory filesystem backing and small controlled -failure seams. Track each returned handle's close state, assert the relevant -structured load error and nil result, and inspect diagnostics/rendered diagnostics -for the canary. Add only the read/close phase cases needed by the existing -error-handling remediation, including a second successful directory so required -selection cannot mask the failed one. Preserve intentionally ignored read-only -regular-file close errors; do not invent a universal close-error contract. - -## 4. Fuzz Detachment Assertions Miss Shared Descendants - -Status: open. Priority: low. Category: ineffective ownership assertion. - -**Relevant target.** `FuzzCompositionAndOverlay` in -[composition_fuzz_test.go](../../internal/engine/composition_fuzz_test.go), -lines 46–49 and 63–66, appends a new root mapping entry and then compares inputs -against their earlier copies to establish result detachment. - -**Deficiency.** A shallow-copied root can still share existing entries, descendant -nodes, or sequence backing storage. Appending through the copied root's slice -header changes neither the input's header nor its existing elements; even reuse -of the backing array writes beyond the input's old length. The comparison can -pass while the result still shares mutable descendants. It primarily detects -aliasing the root object itself, not the deeper ownership property in its error -message. - -**Improvement.** Within the existing bounded target, mutate an existing result -descendant or an element inside the original slice length, then check both -inputs. Keep empty-container cases separate and retain the existing input -immutability and commutativity properties. Deterministic tests in -[compose_test.go](../../internal/engine/compose_test.go), -[overlay_test.go](../../internal/engine/overlay_test.go), and -[recursive_merge_test.go](../../internal/engine/recursive_merge_test.go) already -give meaningful nested-mutation protection; improve the fuzz assertion rather -than adding a parallel matrix or claiming all ownership coverage is absent. - -## 5. Remove Exact Stat-Call Choreography From The Ordering Test - -Status: open. Priority: low. Category: removal/rewrite candidate. - -`TestSelectNamedImportsChecksNormalizedPathsLexically` in -[import_selection_test.go](../../internal/engine/import_selection_test.go), -lines 151–166, asserts the exact trace `a.yaml,a.yaml,z.yaml,z.yaml`. - -Lexically ordered target handling is meaningful. Performing exactly two `Stat` -calls per target is not an established requirement: reusing successful preflight -metadata or changing a filesystem helper could preserve selection, safe failure, -and ordering while failing this test. The selected-candidate assertion meanwhile -checks only the count, not the order or paths. - -Remove the duplicated-stat trace expectation. Check ordered candidate paths and, -if first-failure order needs separate protection, the selected diagnostic site -using two failing targets. Consolidate that case into the existing selection -tests and delete this standalone test if its distinct protection is then -subsumed. Keep meaningful interaction assertions such as not opening excluded -file contents; those protect an actual resource/selection behavior. - -## 6. Consolidate Redundant And Representation-Coupled Facade Scaffolding - -Status: open. Priority: low. Category: removal/consolidation candidates. - -- In [facade_compatibility_test.go](../../facade_compatibility_test.go), the - six-type package-ownership loop in `TestFacadeCompatibility` is a subset of - the comprehensive `TestFacadeValuesRemainRootOwned` loop in - [facade_candidate_test.go](../../facade_candidate_test.go). Remove the duplicate - loop while retaining the compile-time interface checks, public construction, - and any distinct lifecycle behavior. The runtime assertion that a freshly - constructed `Result.Config` still equals its literal field value adds no - protection beyond ordinary Go assignment and compilation. -- `TestWrappedPathsRetainEngineSegmentIntent` in - [facade_internal_test.go](../../facade_internal_test.go) compares private - `.value` representations for inequality. Distinct representations are not the - consumer contract; correct contextual mapping/index lookup is. Replace its - protection with public query behavior, coordinated with finding 2, then remove - the private-representation test. Keep the engine's own segment-intent tests. -- In that same file, `TestWrappedSourcesPreserveEngineOwnership` invokes - `engine.Load` directly through `source.toEngine()`. Its byte snapshot and - copied-reader risks are legitimate, but the facade test should use public - `Load` and root-owned errors. Consolidate overlapping reader behavior with - `TestRepeatedImportsAcrossTiersAndReaderConsumption` in - [facade_equivalence_test.go](../../facade_equivalence_test.go), preserving - explicit copied-source and byte-mutation coverage before deleting scaffolding. -- The clone portion of `TestSourceInfoDestinationConversionsAndCopies` in - [records_test.go](../../records_test.go), lines 39–42, replaces the whole copied - `Destination` field. A shallow struct copy passes this without proving nested - detachment. Remove that non-protective assertion or consolidate the relevant - returned-record ownership check into existing public source-snapshot tests; - retain conversion checks that protect fields at the facade boundary. `Path` - is opaque and immutable through its public builders, so exposing internals or - adding a new deep-copy guarantee is not justified by this test. - -This is a small cleanup, not a recommendation to remove root integration tests -because the engine has tests. Public conversion, error typing, callback bridging, -source namespace identity, disclosure, and returned-record ownership have real -cross-package risks. The static public/dependency-boundary checks also protect -the expressly selected architecture and should remain. - -## Validation And Limits Of This Review - -The following passed using cached dependencies with automatic network/toolchain -acquisition disabled: - -```sh -go test ./... -coverpkg=./... -covermode=atomic -coverprofile=/tmp/configkit-testing-audit.ZNsVuW/coverage.out -count=1 -go tool cover -func=/tmp/configkit-testing-audit.ZNsVuW/coverage.out -go test ./... -shuffle=on -count=3 -``` - -The profile stayed outside the repository. Coverage blocks were combined across -test binaries before calculating package totals. A separate JSON test run found -no individual skipped test/subtest on this Linux host; `internal/document` was -reported as having no standalone tests. This is not native macOS/Windows evidence. -Only the checked-in fuzz seeds were run; no extended fuzz campaign or mutation -testing was performed. Assertion weaknesses above are grounded in source-level -counterexamples, not a claimed measured mutation score. - -No production code, tests, fixtures, dependencies, or release gates were changed. -Recommended additions should accompany the relevant fixes and existing test -owners. Use small controlled budgets and independent outcomes, retain selected -stable formatting/code/path contracts, and avoid making implementation details -or speculative guarantees into new policy through assertions.