Retire completed consolidation roadmap documents
This commit is contained in:
@@ -1,206 +0,0 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user