78 lines
4.4 KiB
Markdown
78 lines
4.4 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: open. 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.
|