Files
configkit/docs/roadmap/remediation.md

237 lines
17 KiB
Markdown
Raw Normal View History

# Main-relative companion implementation review
Reviewed the eight implementation commits from `ae0c94d` through `18af2d2`
against the [implementation plan](implementation.md) and
[feature roadmap](main-relative-inputs.md).
| Stage | Review result |
| --- | --- |
| 1 | The root facade copies binding and fallback descriptions. Preflight checks active binding scope and static context shape. Concrete overrides and inactive overlays take precedence over shadowed settings. |
| 2 | The acquired main's resolved context feeds underlay acquisition. Derived inputs retain the namespace and containment root, clear the main's physical path, and use receiving-tier host identity. Fallback selection and namespace anchors use the chosen context. |
| 3 | Bound traversal and contextless imports share declaration policy and placement preparation, with source-local diagnostic sites. |
| 4 | Without a base, the loader retains payload and version processing, honors requiredness and the empty named-path exception, and emits no fabricated selection evidence. Root assembly has a second implementation. See Finding 2. |
| 5 | Layered overlays select explicit, main-derived, then fallback context. Lookup base and containment root are separate. Native link handling leaves a gap in containment and duplicate detection. See Finding 1. |
| 6 | The executable example covers discovery, embedded defaults, mounted file and recursive directory imports, transitive imports, whole-record upsert, and the selected overlay. A companion test rejects duplicate peers. |
| 7 | Public and engine tests cover discovery, source and version evidence, optional resources, budgets, and cleanup. The exhaustion and lazy fallback branches need focused regression coverage. See Finding 3. |
| 8 | The configuration contract, public comments, architecture policy, and ADR describe deferred binding. Two supporting descriptions misstate ownership. See Finding 4. |
## Finding 1 — Native overlay symlinks bypass containment and duplicate checks
**Status** — Complete. Native overlay lookup resolves supported symlinks before containment and repeat checks. The overlay tests cover escapes, aliases in both directions, and a contained target.
**Priority P2.** The layered branch passes the inherited root and lookup base
to the shared overlay loader. That loader calls `filesystemAccess.canonical`.
For a native filesystem, the function stats the path and returns its lexical
spelling even when the filesystem implements `fs.ReadLinkFS`. The root and
duplicate checks then compare that spelling, while the subsequent read can
follow the link. The same limitation predates this sprint in the shared
loader. Automatic derivation makes it reachable without an explicit overlay
filesystem, and the new contract describes the inherited root as the
containment boundary. [Layered overlay selection](../../internal/engine/load_request.go#L192)
· [Overlay checks](../../internal/engine/selected_overlay.go#L74)
· [Native path lookup](../../internal/engine/filesystem_access.go#L94)
· [Overlay contract](../config.md#selected-overlays)
Reproduced through `LoadLayered` on Go 1.27.3 using
`NewFilesystem("native", os.DirFS(dir))`, which exposes `fs.ReadLinkFS`.
The main is `scope/main.yml` with a context rooted at `scope`. There is no
explicit overlay context.
- `scope/overlays/escape.yml` points to `../../outside.yml`. The load
succeeds and takes `count: 9` from outside the root.
- `scope/overlays/repeat.yml` points to `../main.yml`. The load succeeds
and reports the main content a second time under the overlay alias.
Apply capability-aware link resolution to native overlay targets, using the
existing native link resolver where applicable. Use the resolved path for
containment, repetition, and reading. Check the resolved identity against
already acquired files in the same namespace, including aliases where those
files retain lexical paths. Add focused derived-overlay regressions for the
escape and repeat cases. Preserve root-relative lookup for ordinary and
explicit overlays and document the limit for native filesystems that cannot
expose link targets. This requires no general sandbox guarantee for caller
filesystems. [Native link resolver](../../internal/engine/filesystem_links.go#L227)
## Finding 2 — Contextless imports duplicate root contribution assembly
2026-11-09 21:18:16 +00:00
**Status** — Complete. Bound and contextless imports share root charging, placement, and sequence-item scope assembly. The full test suite passed.
**Priority P3.** Stage 4 calls for sharing the root contribution assembly
and accounting. The new contextless path has its own source charging,
mapping placement, wrapper charging, pending sequence-item scope
construction, and placement reservation. Bound imports perform those same
steps independently. Both paths pass today’s tests, but changes to placement
or accounting now need to be made twice for the same declaring payload.
The initial limit diagnostics already differ in specificity.
[Contextless path](../../internal/engine/contextless_imports.go#L51)
· [Bound import assembly](../../internal/engine/import_acquisition.go#L115)
Share the preparation and finalization of the declaring root contribution
where the paths have the same semantics. Keep the contextless requiredness
decision and bound traversal separate. Preserve their existing evidence and
the contextless path's lack of filesystem access. Use existing mapping,
sequence-item, and budget checks to verify the extraction.
## Finding 3 — Exhaustion and lazy fallback lack focused regression coverage
**Status** — Complete. Public tests cover search exhaustion, root-only main context, and missing fallbacks when they win or are shadowed.
**Priority P3.** The context-selection test uses an existing fallback when
checking that main derivation wins. It would still pass if the loader
prematurely opened that fallback. The no-base tests use `NoSource()`, and
the discovery integration covers a successful selection or a selected
parse failure. These cases leave the optional-search exhaustion handoff to
both companion and overlay selection unprotected.
[Context selection tests](../../facade_main_relative_test.go#L39)
· [Discovery integration tests](../../facade_main_relative_integration_test.go#L12)
A disposable public API probe confirmed the current behavior. Exhausted
optional search without context retained the declaring payload with no empty
selection evidence. The same search with a root-only main context loaded a
companion and overlay despite a well-shaped but nonexistent fallback. With
the main context removed, the selected missing fallback failed. Add a small
public regression table for those outcomes. Include an overlay-only failure
case if the shared fixture would otherwise stop at the companion error
before reaching overlay selection.
## Finding 4 — Supporting documentation misstates binding ownership
2026-11-09 21:19:05 +00:00
**Status** — Complete. The consumer guide identifies Input.MainRelative, and the internal guide describes the receiving tier’s identity bookkeeping.
**Priority P3.** The consumer guide locates main-relative bindings in
`LayeredOptions`, but a companion binds through `Input.MainRelative`.
The import internals guide says passing the main context replaces tier-local
identity state. The engine preserves the receiving tier's bookkeeping and
sets the derived host token to that tier's token. Correct these descriptions
and leave the observable rules with the configuration contract.
[Consumer guide](../consumers/README.md#choosing-a-loading-entry-point)
· [Import internals](../internal/imports.md#discovery-and-acquisition)
· [Receiving tier](../../internal/engine/load_request.go#L276)
## Verification
The everyday checker and the full race suite passed on Linux with Go 1.27.3.
Disposable public API probes reproduced Finding 1 and checked the exhaustion
and fallback behavior in Finding 3. The review found no additional contract
strengthening beyond the native containment wording discussed in Finding 1.
2026-11-09 20:59:12 +00:00
## Integration follow-up
The follow-up reviewed the same implementation through its existing callers
and downstream consumers. Findings 1–4 remain open.
| Stage | Interaction reviewed |
| --- | --- |
| 1 | The public facade copies the new descriptions into each request. Existing concrete context checks, disabled import handling, and overlay name validation still run on their established paths. |
| 2 | Derived imports preserve the receiving tier's repeat detection and the native namespace token. A probe confirmed that a derived import and explicit file repeat within the same namespace, while separately constructed namespaces with the same label and path compose independently. Standalone host validation diverges from explicit context handling. See Finding 5. |
| 3 | Both traversal paths prepare declarations from the existing parsed specs. Placement diagnostics still use the local mapping scope for sequence items. |
| 4 | With optional imports and no base, the declaring source enters the established scope composition and version evidence paths. A probe with a mounted sequence item and required source version produced the same record and version evidence with or without an explicit base. Only the bound path reported a completed empty selection. |
| 5 | The selected overlay uses the existing loader, version protection, aggregate limits, and source context construction after main and underlay assembly. The inherited native link limitation is recorded in Finding 1. |
| 6 | The executable example exercises the handoff from discovery through embedded and operator tiers, identified record upsert, and overlay application using existing composition operations. |
| 7 | Focused regressions passed for main-relative loading, ordinary and layered overlays, source contexts, import defaults, source versions, and sequence-item placement. The host access mismatch calls for the regression in Finding 5. |
| 8 | The public contract requires a usable base for `RequireBase` and describes the established context checks. Finding 5 concerns code behavior under that contract. The ownership corrections from Finding 4 still apply. |
## Finding 5 — Derived host roots skip standalone access validation
**Priority P2.** When a companion binds to the main context with imports
disabled, `acquireStandaloneInput` checks the resolved directories with
`Stat`. An equivalent explicit context goes through
`validateFilesystemContext`, which also opens and closes the host root.
The derived path can therefore accept a directory that the existing
validation rejects. It publishes a passive `SourceContext` whose host
filesystem immediately fails to open that same root.
[Derived and explicit validation branches](../../internal/engine/acquisition.go#L139)
· [Existing context validator](../../internal/engine/filesystem.go#L10)
· [Passive host filesystem](../../internal/engine/resource_context.go#L77)
A public API probe reproduced the difference as an unprivileged user on
Linux with Go 1.27.3. Put a readable `main.yml` and `resource.txt` in a
directory with owner search permission and no read permission (mode
`0100`). The main file can be read by name and its directory can be
statted, but `os.OpenRoot` returns permission denied. An imports-disabled
companion with `MainRelativeContext{RequireBase: true}` loads and returns
a source context, then `fs.ReadFile` through that context fails. The
same companion with an explicit context at that directory fails during
loading with `import.permission`. The derived case also fails with
`import.permission` when imports are enabled.
Share the existing resolved-context validation between derived and
explicit standalone acquisition, including the host root open and close
checks. Keep the native directory checks and existing error handling.
An access failure in the selected context should remain terminal.
Add a regression at the derived versus explicit boundary, using a
deterministic root-open failure or a permission fixture that verifies
the platform actually denies the open. The check concerns root access
during loading. Later resource operations retain their existing filesystem
semantics. [Layered loading contract](../config.md#layered-loading)
## Algorithm audit
The audit covers the implementation from `ae0c94d` through `18af2d2`.
It analyzes the work and storage implied by the code. The planning decisions
on precedence, selection, import traversal, and context ownership remain the
basis for the analysis.
| Stage | Work and storage reviewed |
| --- | --- |
| 1 | Facade conversion and binding preflight visit each input once, with work proportional to the option data copied or validated. The existing diagnostic sort orders failures after collection. |
| 2 | Binding and namespace anchor selection use a fixed number of linear passes. The temporary context table and per-input resolved copies are avoidable. See Finding 7. |
| 3 | Declaration preparation visits each declaration once, but resolving a shared pattern default copies its entries for each inheriting directory declaration. See Finding 6. |
| 4 | The no-base path prepares declarations and scans requiredness before assembling one payload. The pattern copies contribute a multiplicative cost even though no traversal runs. Root assembly duplication is already tracked in Finding 2. |
| 5 | Context selection considers at most three choices, and the shared overlay loader makes one linear pass over acquired sources for duplicate detection. The native path correctness issue remains in Finding 1. |
| 6 | The example uses one layered load and the existing declaration, import, and tier assembly paths. It adds no separate scan for companion files. |
| 7 | Source budgeting and evidence reuse the existing load state. The focused suite passed for the new loading paths and their import, overlay, and sequence-item dependencies. |
| 8 | The documented single discovery and deferred selection match the call path reviewed above. The identified improvements concern private storage and work, with no change to the selected algorithms or public behavior. |
## Finding 6 — Skipped directory imports copy shared patterns per declaration
**Priority P3.** With no base, `acquireContextlessInput` calls
`prepareImportDeclarations`, which holds one full resolved policy per
declaration. Each directory declaration that inherits the load-wide
`ImportDefaults.Pattern` allocates a separate copy of the pattern slice.
The no-base path reads the requiredness from those policies and never uses
the copied patterns.
[No-base preparation](../../internal/engine/contextless_imports.go#L22)
· [Policy slice](../../internal/engine/import_acquisition.go#L245)
· [Default pattern copy](../../internal/engine/import_spec.go#L122)
For **D** directory declarations that inherit **P** patterns, this adds
Θ(D × P) string-entry copies in time and peak temporary storage for a
single input. It becomes quadratic when both counts grow together. The
pattern text itself is shared. The work is separate from validating the
default patterns once and from matching filenames on the bound path.
Keep the load-owned default pattern snapshot and borrow its slice read-only
when resolving an inherited pattern. The selectors already read these
entries without modifying them. This reduces pattern storage and copying
to Θ(D + P) for the policy headers and shared entries. A separate option
is to share the context-independent validation and have the no-base path
retain only the policy fields it actually uses. Either approach should
preserve explicit empty pattern overrides, validation of skipped
declarations, and existing diagnostic sites.
[Default snapshot](../../internal/engine/import_spec.go#L72)
· [Filename filtering](../../internal/engine/import_selection.go#L230)
## Finding 7 — Each derived companion keeps a copy of the same tier context
**Priority P3.** `acquireUnderlayGroupScopes` allocates a context pointer
slot for every input, even in tiers with no bindings. Each derived input
then gets an addressable copy of the same main context with the same
cleared main path and receiving-tier host token. The separate copies
carry no input-specific values.
[Context preparation](../../internal/engine/load_request.go#L276)
For a tier with **U** inputs and **C** derived bindings, this adds Θ(U)
temporary pointer slots and Θ(C) context values. The passes themselves
are linear. Build at most one read-only derived context per tier and
select it during the existing ordered anchor and acquisition passes.
Use the private input's selected concrete context for fallbacks. This
reduces the extra context bookkeeping to O(1), excluding the existing
input list and native namespace map. Preserve per-input validation and
handle ownership, as well as first-input namespace anchors. Share the
context description without caching filesystem observations.
Complexity was derived from the loops and copies above. Focused Go 1.27.3
tests passed for the companion workflow, layered loading, overlays,
import policy and selection, and sequence-item placement.