207 lines
12 KiB
Markdown
207 lines
12 KiB
Markdown
# Consolidation Review Findings
|
||
|
||
Reviewed revision: `557e006a4ab98b666735a71b3845418c702f7cec`.
|
||
Comparison baseline: `8ab1568` (the committed implementation plan).
|
||
|
||
The review compared the [consolidation target](consolidation.md) and
|
||
[implementation plan](implementation.md) with the completed sprint, including
|
||
public/private conversions, preflight and acquisition, import selection and
|
||
evidence, filesystem contexts, inheritance, projection, schema construction,
|
||
changed tests, and documentation. The development check (`./scripts/check.sh`)
|
||
and full race suite (`go test -race ./...`) passed with Go 1.26.10 on Linux.
|
||
|
||
## 1. Complete The Non-Import Resolved-Context Handoff
|
||
|
||
Status: complete (2026-10-25). Priority: medium. Category: implementation gap and maintainability.
|
||
No user-facing correctness regression has been established for this finding.
|
||
|
||
**Target.** Roadmap internal target 2 and implementation Stages 7–9 select one
|
||
resolved filesystem description shared by validation/acquisition and passive
|
||
context construction. Stage 8 explicitly requires populating the description
|
||
at the existing non-import validation point and resolving only facts that are
|
||
still missing later.
|
||
|
||
**Evidence.** In [acquisition.go](../../internal/engine/acquisition.go),
|
||
`acquireInput` calls `validateFilesystemContext` at line 181 for inputs without
|
||
import traversal and with an applicable explicit context. That validator returns
|
||
only a diagnostic. Its host/native branches derive a resolved description but
|
||
discard it. At lines 220–226, acquisition sees `input.resolvedFilesystem == nil`
|
||
and calls `resolveFilesystemContext` again before building passive views.
|
||
|
||
For example, an ordinary `Load` of a byte or mapping source with explicit host
|
||
`FilesystemContext` and imports disabled resolves/canonicalizes the base and
|
||
root during validation, then derives those same facts again for its source
|
||
context. The retained description therefore does not complete the intended
|
||
validation-to-context handoff.
|
||
|
||
Additionally, [filesystem.go](../../internal/engine/filesystem.go) still contains
|
||
`validateAbsentHostFilesystemContext` (line 40), with its own root/base
|
||
canonicalization, root-defaulting, and containment logic. That derivation
|
||
duplicates `resolveAbsentFilesystemContext` in
|
||
[filesystem_context.go](../../internal/engine/filesystem_context.go) (line 29).
|
||
The two paths already maintain separate containment-diagnostic messages. Graph
|
||
caller inspection confirms that the old validator is reachable from
|
||
`acquireInput`; it is not unused migration scaffolding.
|
||
|
||
**Why address it.** The new model exists, but one important path still discards
|
||
and reconstructs its results, while absent-host context derivation has two
|
||
production implementations. This leaves part of audit finding 5 unresolved and
|
||
preserves the drift risk the consolidation was intended to remove. This is a
|
||
structural completion issue, not a claim of measured performance loss or a new
|
||
single-lookup guarantee.
|
||
|
||
**Required revision.**
|
||
|
||
- Have non-import context resolution/validation return or populate the resolved
|
||
description, and retain it on `assembledInput` at the existing validation
|
||
point. Later passive-view construction and ordinary-overlay handling should
|
||
reuse those applicable facts.
|
||
- Route absent-host derivation through the common resolved-context owner and
|
||
remove the duplicate derivation from the old validator. Keep the existing
|
||
directory/access checks and close-error handling at their appropriate points.
|
||
- Preserve deferred discovery/ordinary-overlay checks, root-only absent host
|
||
context, host/native distinctions, diagnostics, and borrowed filesystem
|
||
ownership. Independently required validation must not disappear merely to
|
||
reduce filesystem calls.
|
||
- Keep internal documentation aligned with the completed handoff. Describe
|
||
aggregate accounting as load-wide and repetition/namespace state as
|
||
tier-scoped; the new internal overview/import prose currently calls the latter
|
||
per-tier accounting despite its distinct role.
|
||
|
||
**Verification.** Review the non-import call path to confirm that applicable
|
||
resolved facts are carried forward and absent-host derivation has one owner.
|
||
Reuse the existing host/native, discovery selected/absent, ordinary/layered
|
||
overlay, and passive-resource-context tests. Add a focused behavioral regression
|
||
only for a consequential gap exposed by the change; do not assert incidental
|
||
canonicalization/stat call counts. Run the development check and applicable
|
||
ownership race checks after remediation.
|
||
|
||
**Completion.** Non-import validation now returns its resolved filesystem
|
||
description and acquisition retains it for passive contexts and ordinary
|
||
overlays, including absent inputs. Absent-host derivation uses the common
|
||
resolver; the duplicate validator was removed. Validation timing, directory and
|
||
root-open/close checks, and the existing absent-input containment diagnostic
|
||
are retained. Internal documentation now distinguishes load-wide accounting
|
||
from tier-scoped repetition/namespace state. Existing filesystem, discovery,
|
||
overlay, and resource tests, the development check, and `go test -race ./...`
|
||
passed with Go 1.26.10 on Linux. No new incidental call-count assertions were
|
||
added.
|
||
|
||
## Algorithmic Audit Scope
|
||
|
||
Audited worktree: `d898cb1`, with production code at the reviewed implementation
|
||
revision above. This follow-up used source inspection and graph caller traces,
|
||
not benchmarks or new runtime measurements. It covered acquisition and import
|
||
bookkeeping, scope scheduling and projection, filesystem context handoffs,
|
||
inheritance preparation/combination/finalization, and reflective projection and
|
||
schema construction.
|
||
|
||
Findings 2–3 are retained inefficiencies in paths carried through the refactor,
|
||
not algorithmic regressions introduced by the consolidation. They warrant
|
||
focused follow-up without changing the selected configuration model. The audit
|
||
did not identify an additional correctness defect. Finding 1 was open at the
|
||
time of that audit; completion is recorded with the finding above.
|
||
|
||
The selected stable Kahn scheduler, deterministic sorting, composition rules,
|
||
scope boundaries, and required ownership copies are not reconsidered here.
|
||
Previously deferred experiments remain in [future.md](future.md#3-performance-experiments).
|
||
The separately completed [clone review](consolidation.md#oq-2-how-much-clone-cleanup-should-this-effort-commit-to)
|
||
concluded that no refactor was warranted. These findings do not establish new
|
||
public complexity guarantees, input restrictions, performance thresholds, or
|
||
release gates.
|
||
|
||
## 2. Avoid Searching All Sources For Each Empty Import Selection
|
||
|
||
Status: complete (2026-10-25). Priority: low. Category: algorithmic efficiency.
|
||
|
||
**Evidence.** `walkDeclarations` calls `recordEmptyImportSelection` for each
|
||
optional declaration that admits no source. In
|
||
[import_acquisition.go](../../internal/engine/import_acquisition.go), lines
|
||
289–300, the latter finds the declaring source by scanning `s.sources` from the
|
||
beginning, even though traversal already knows which source owns the declaration.
|
||
|
||
**Complexity.** With S acquired sources and D empty declarations, source lookup
|
||
takes O(S × D) comparisons in the worst case, excluding path-copying and source
|
||
selection itself. A broad main import of N constant-size fragments, each with
|
||
one empty optional declaration, visits source positions 1 through N: the sum of
|
||
lookup work is Θ(N²), despite linear source and selection-evidence volume and
|
||
constant import depth. Existing input budgets bound the workload but do not
|
||
remove this avoidable scaling within those budgets.
|
||
|
||
**Potential refactoring.** Carry the declaring source's integer index through
|
||
the declaration traversal, or maintain a traversal-local source-ID-to-index map.
|
||
Either approach makes each evidence-owner lookup O(1), with O(S + D) total
|
||
bookkeeping for the map alternative. Keep source order and IDs unchanged. Use
|
||
indices rather than retained pointers into the appendable source slice, and
|
||
preserve optional-missing rollback and failed-source retention if maintaining
|
||
an index across admission.
|
||
|
||
**Verification.** Reuse the source-local/detached empty-selection and partial
|
||
failure coverage in [import_evidence_test.go](../../internal/engine/import_evidence_test.go).
|
||
A broad-fragment behavioral case is useful if existing coverage does not protect
|
||
the chosen index handoff. Verify the removed scan by source review rather than
|
||
adding timing thresholds or assertions about private lookup counts.
|
||
|
||
**Completion.** Declaration traversal now receives the declaring source's stable
|
||
integer position, captured at admission and carried through mapping and item
|
||
imports. Empty selections append evidence directly to that record without
|
||
searching the source slice or retaining pointers into it. The broad-fragment
|
||
test covers source-slice growth, missing-source rollback, and parent evidence
|
||
after child traversal; existing detached evidence, nested item, underlay, and
|
||
partial-failure tests remain effective. Focused tests, the development check,
|
||
and race-enabled import/destination tests passed with Go 1.26.10 on Linux.
|
||
|
||
## 3. Restrict Import-Scope Edge Preparation To Relevant Nodes
|
||
|
||
Status: complete (2026-10-25). Priority: medium. Category: algorithmic efficiency and allocation
|
||
scaling.
|
||
|
||
**Evidence.** `scheduleAcquisitionScopes` in
|
||
[imports.go](../../internal/engine/imports.go), lines 57–64, recursively schedules
|
||
the outer scope and every pending item-local scope using the same complete
|
||
inclusion graph. `scheduleContributionSlots` in
|
||
[import_graph.go](../../internal/engine/import_graph.go), lines 185–213, constructs
|
||
a local identity-to-slot map but then scans every graph edge and allocates an
|
||
edge slice with capacity `len(graph.edges)` for each scope with at least two
|
||
eligible contributions. Most edges need not touch that scope.
|
||
|
||
**Complexity.** For Q such scopes and E inclusion edges, preparing the scheduler
|
||
inputs takes O(Q × E) edge inspections and requests O(Q × E) edge-slice capacity
|
||
over the operation. This is cumulative allocation demand, not a claim that all
|
||
those slices remain live together. For N sibling item imports, each with one
|
||
distinct mapping helper, Q and E are both Θ(N), making this preparation Θ(N²)
|
||
even at constant nesting depth. The selected scheduler itself is not the
|
||
problem: each small item-local graph has constant size, and the outer graph
|
||
requires its normal deterministic scheduling work.
|
||
|
||
**Potential refactoring.** Build a traversal-local adjacency index once after
|
||
acquisition, then gather edges for a scope by its participating source identities
|
||
and retain only edges whose endpoints both have local slots. Alternatively,
|
||
partition the existing edges by their applicable scopes once. Preserve the
|
||
current endpoint-membership rule, item-boundary representation, slot placement,
|
||
group scheduling, and stable Kahn tie-breaking. For the broad sibling family,
|
||
this reduces edge preparation to O(E + P), where P is the total number of local
|
||
slots, followed by the unchanged scheduler cost. Keep this an indexing change
|
||
around the agreed scheduler, not a different ordering algorithm or a cross-load
|
||
cache.
|
||
|
||
**Verification.** Reuse item/helper interleaving and grouped import scheduling
|
||
coverage in [imports_test.go](../../internal/engine/imports_test.go) and
|
||
[load_request_test.go](../../internal/engine/load_request_test.go), together with
|
||
existing destination/provenance evidence tests. Cover broad independent item
|
||
scopes if a consequential gap remains; verify index construction and edge
|
||
visitation bounds by source review without new performance gates.
|
||
|
||
**Completion.** Scheduling now builds an importer-to-target adjacency index once
|
||
for an acquisition and shares it across outer and pending item scopes. Grouped
|
||
scheduling builds its own index once. Local preparation visits only adjacency
|
||
lists for participating identities, includes only edges with both endpoints in
|
||
the local slot map, and allocates edge storage as needed rather than reserving
|
||
the whole graph per scope. For the broad sibling family, preparation is O(E + P)
|
||
before the unchanged stable Kahn scheduling. The inclusion graph and cycle
|
||
diagnostics remain unchanged. Tests cover excluded endpoints/unselected slots
|
||
and broad main/underlay item scopes with retained helper origins and projected
|
||
destinations. Focused scheduling tests, the development check, documentation
|
||
links, and `go test -race ./...` passed with Go 1.26.10 on Linux. No timing or
|
||
allocation thresholds were introduced.
|