Fix profile loading with ConfigKit history
All checks were successful
ci/woodpecker/push/verify Pipeline was successful
ci/woodpecker/tag/release Pipeline was successful

This commit is contained in:
2026-09-07 13:10:29 +00:00
parent 3ce198197d
commit ed4623bc41
6 changed files with 130 additions and 45 deletions

View File

@@ -89,10 +89,11 @@ directory resolves from the pipeline root. Preserve digest relocation invariance
Status: Resolved — P2
Resolution: Named-profile loading now validates the independently resolved base
through the same ConfigKit definition and public metadata history before accepting
the selected result. Duplicate ordinary or family definitions therefore fail even
when an overlay nulls their container or all of Scriptorium. A single base owner
Resolution: Loading now discovers base declarations through ConfigKit's public
historical-path enumeration and metadata history from the selected result,
without separately resolving the base (see finding 7). Duplicate ordinary or
family definitions therefore fail even when an overlay nulls their container or
all of Scriptorium. A single base owner
with a legitimate profile override remains supported.
**Evidence:** `rejectDuplicateBaseDefinitions` at
@@ -240,7 +241,73 @@ rollback, retries, or repeated directory loading.
useful skipped-empty diagnostic, without leaking a synthetic secret or polluting
show output. A two-profile diff still bootstraps and reports only once.
## Verification and scope
## 7. Do not resolve unused base credentials when loading a profile
Status: Resolved — P2
Resolution: Upgraded to released ConfigKit v0.1.2 and replaced the extra base load
with `Metadata.HistoricalPaths()` enumeration of declaration paths in the selected
result. Ownership checks still inspect base-layer history for both ordinary
artifacts and families, even when overlays remove them. Regression tests verify
that a profile can replace an unset production credential with a resolvable testing
reference or remove it with null. Existing hidden-duplicate and legitimate-override
tests remain intact. No local dependency replacement or second composer is needed.
Follow-up review: 2026-09-07 at `3ce1981`. The original six reproductions are
addressed, but the fix for finding 3 introduces this regression.
**Evidence:** `internal/config/pipeline_configkit.go:49-57` loads the base again
without a profile to check duplicate base definitions. This calls the complete
typed ConfigKit loader, including secret resolution, domain resolution, and
validation—not just declaration ownership inspection. Consequently, loading a
valid testing profile also requires credentials from the unselected base.
**Reproduction:** In an otherwise valid base, set `audita.llm_api_key` to
`NARRATIO_REVIEW_PROD_KEY`. In `profiles/testing.yml`, replace it with
`NARRATIO_REVIEW_TEST_KEY`. Leave the production variable unset and supply only
the testing variable with a synthetic value. `config validate --config <root>
--profile testing` fails with `secret.unset` at the base credential declaration.
Supplying both variables makes it pass. A profile that instead sets
`audita.llm_api_key: null` also fails when the unused production variable is
unset. Both failures were reproduced through the current CLI.
**Impact:** Operators cannot use an independent testing credential environment
or remove a production credential requirement through a profile. Commands fail
despite having all credentials required by their effective configuration. The
extra full load also unnecessarily repeats configuration processing.
**Required change:** Enforce unique base ownership without requiring the
unselected base to succeed as a separately resolved runtime configuration.
Inspect complete declaration history through ConfigKit's public metadata,
including declarations displaced by overlays. If additional upstream metadata
support is needed, address that explicitly rather than requiring unused secrets,
suppressing arbitrary load failures, reparsing YAML, or restoring a second
composer. Preserve the duplicate-definition protection added for finding 3.
**Acceptance:** Add public-boundary regression tests for a profile replacing an
unset production credential with an available testing credential, and for a
profile removing that credential with null. Both must load successfully without
the production variable. Retain failures for duplicate base ordinary/family
definitions hidden by null overlays, along with legitimate profile override and
profile-only definition coverage. Resolve secrets only for the selected effective
configuration.
## Follow-up verification
After adopting ConfigKit v0.1.2 and completing finding 7, the full
`scripts/check-release-candidate.sh v2.0.0` check passed: module hygiene,
formatting/whitespace, uncached tests and race tests, vet, build, documentation,
maintained examples, and Linux amd64/arm64 plus macOS arm64 release cross-builds.
`go mod verify` also passed. The new credential regressions failed before the
ownership-check fix and passed afterward. No local module replacements, release
tags, or pushes were used.
At `3ce1981`, `go test ./...`, `go test -race ./...`, `go vet ./...`, and
`go build ./...` passed. The new finding was reproduced with a freshly built CLI,
temporary configuration files, and synthetic credentials; no external adapters
or cloud services were invoked. That review pass changed only this review record.
## Original verification and scope
The following checks passed at the reviewed commit: