98 lines
4.7 KiB
Markdown
98 lines
4.7 KiB
Markdown
# ConfigKit Inheritance Review Remediation
|
||
|
||
## Review Scope And Overall Result
|
||
|
||
Reviewed on 2026-10-04, comparing implementation commits `ec99d48` through
|
||
`6450994` with the baseline `9e74df0`, the ConfigKit feature roadmap, and its
|
||
eight-stage implementation plan.
|
||
|
||
The architecture matches the intended design. ConfigKit v0.5.0 owns source
|
||
composition and native sibling inheritance before decoding. Notarius retains
|
||
its typed defaults, presence-sensitive validation, declaration-relative path
|
||
normalization, detached runtime configuration, catalog-dependent resolution,
|
||
and private redacted inspection boundary. The production configuration adapter
|
||
needed no changes; the work adds the dependency update, integration tests,
|
||
documentation, and the derived development pipeline example.
|
||
|
||
No production regression or inadvertently strengthened requirement was found.
|
||
In particular, the implementation adds no inheritance whitelist, new schema
|
||
version, custom merging, path-containment rule, template exemption, or release
|
||
gate. The newly reserved `extends` key is the expressly adopted upstream
|
||
compatibility change.
|
||
|
||
Two follow-ups remain: a path-test blind spot and an incomplete documented
|
||
command. This document records remediation work only; it does not authorize
|
||
unrelated runtime refactoring.
|
||
|
||
## Finding 1 — Make The Path Fixture Distinguish Declaring Directories
|
||
|
||
Priority: P2. Category: verification gap.
|
||
|
||
Evidence: `internal/core/config/inheritance_test.go:338–436`,
|
||
`TestInheritedReferencePathsKeepDeclaringSource`.
|
||
|
||
The parent, child, and overlay files live in `root/parents`, `root/children`,
|
||
and `root/overlays`. Their resource paths all use `../fixtures/<name>`, and
|
||
the expected paths all point into `root/fixtures`. Resolving one of these
|
||
paths from any of the three directories yields the same result. Thus, choosing
|
||
the child's directory for an inherited step/nested reference, or choosing the
|
||
parent's directory for an overlay replacement, could still pass this test.
|
||
|
||
`TestInheritedInspectionRetainsOriginsRedactionAndOwnership` does distinguish
|
||
the parent directory for a simple inherited `./shared.txt` reference. It does
|
||
not close the blind spot for the nested, ordered-step, direct-child replacement,
|
||
and overlay cases in the larger path fixture.
|
||
|
||
Required change:
|
||
|
||
- Revise the existing path fixture to use directory-distinguishing relative
|
||
paths for these cases, such as `./shared.txt`, `./step.txt`, and a child-local
|
||
replacement. Assert inherited paths against the parent's directory, direct
|
||
replacements against the child's directory, and overlay-supplied paths
|
||
against the overlay's directory.
|
||
- Retain a separate legitimate `../` example to protect the existing host-path
|
||
behavior. Avoid making every path converge on one shared sibling directory.
|
||
- Keep this as a focused real-file configuration test. Change production
|
||
resolution only if the improved assertions expose an actual defect.
|
||
|
||
Acceptance: choosing a parent, child, or overlay directory incorrectly produces
|
||
a different absolute path and fails the relevant assertion. Nested inheritance,
|
||
inherited ordered-step references, child replacements, and overlay replacements
|
||
still pass with the actual resolver.
|
||
|
||
Validation: `GOWORK=off go test ./internal/core/config`.
|
||
|
||
## Finding 2 — Supply Input In The Development Pipeline Command
|
||
|
||
Priority: P3. Category: documentation accuracy.
|
||
|
||
Evidence: `docs/config.md:43–49`, in Maintained Examples.
|
||
|
||
The new example says to select the development pipeline with
|
||
`notarius run dnd-session-development --config /path/to/dnd-complete.config.yml`.
|
||
The run command requires `--input`. Executing the equivalent command against
|
||
the maintained configuration returns `notarius: run requires --input` with CLI
|
||
exit status 2, before pipeline execution.
|
||
|
||
Required change: include a transcript input placeholder in the invocation,
|
||
for example `--input /path/to/transcript.json`, and retain the link to the CLI
|
||
reference for the full invocation contract. No CLI behavior change or new test
|
||
framework is needed.
|
||
|
||
Acceptance: the documented command supplies both required run inputs—the
|
||
pipeline ID and input path—and uses the implemented flag names. Verify against
|
||
the CLI reference and existing parser coverage without a live model request.
|
||
|
||
## Checks Completed During Review
|
||
|
||
- `GOWORK=off go test ./...` passed.
|
||
- `GOWORK=off go vet ./...` passed.
|
||
- `GOWORK=off go build` of `./cmd/notarius` to a temporary destination passed.
|
||
- `GOWORK=off go mod tidy -diff` produced no changes.
|
||
- The selected ConfigKit module is v0.5.0; PromptKit remains v0.11.2.
|
||
- The implementation diff passed whitespace checks.
|
||
- The incomplete development invocation was reproduced without provider calls.
|
||
|
||
The passing suite establishes the current tested behavior; it does not resolve
|
||
the path-fixture blind spot identified above.
|